That sounds great. I like simple.
On Tue, Feb 09, 2016 at 08:19:57AM +0530, Babu Shanmugam wrote: > Ben, I tested your solution and it works well. It is in fact simpler than my > approach. I will squash the other two commits along with this for v2. > > > Thank you, > Babu > > On Tuesday 09 February 2016 05:44 AM, Ben Pfaff wrote: > >On Mon, Feb 08, 2016 at 03:20:11PM +0530, Babu Shanmugam wrote: > >>>>The qos settings are managed using the 'options' fields in the > >>>>"Port_Binding" table. > >>>> > >>>>Signed-off-by: Babu Shanmugam <bscha...@redhat.com> > >>>This seems more complicated than necessary. In the > >>>SBREC_PORT_BINDING_FOR_EACH loop in binding_run(), I think that we only > >>>need to be able to find the ovsrec_interface associated with the > >>>binding_rec. Then we can check and possibly update the > >>>ovsrec_interface's ingress_policing_rate and ingress_policing_burst. > >>>Can you explain why there's this more complicated data structure? > >>If not for this iface_qos hash map, we need to run a loop OVSREC > >>Interface_table entries for every iteration of SBREC Port_Binding entry to > >>check against its policing_values and update it if needed. > >>If we have an api to fetch the Interface_table entry with the interface id, > >>this hash map wouldn't be needed. I was not able find such an api, but is > >>there any such api? > >> > >>Below is a summary on the use of this hash map; > >> > >> * At the start of binding_run(), policing values of all the interfaces > >> are hashed inside this map with the interface_id as the key. > >> * On each iteration of SBREC Port_Binding, this hash map is looked up > >> and policing values of the SBREC port_binding and OVSREC > >> Interface_table are compared and one of the following actions are taken > >> o If both the values are equal, then the hash map entry > >> corresponding to this interface id is removed > >> o If there is a difference, the difference is noted in the hash > >> map entry > >> * After looping SBREC_Port_Binding, the OVSREC Inteface_table is > >> looped for all the entries, and if the interface_id is present in > >> the hash map, its policing values are updated. > >Here's what I had in mind. It's untested. > > > >--8<--------------------------cut here-------------------------->8-- > > > >From: Ben Pfaff <b...@ovn.org> > >Date: Mon, 8 Feb 2016 16:14:20 -0800 > >Subject: [PATCH] ovn-controller: Copy southbound database policing settings > > into OVS config. > > > >Signed-off-by: Ben Pfaff <b...@ovn.org> > >--- > > ovn/controller/binding.c | 46 > > ++++++++++++++++++++++++++++------------- > > ovn/controller/ovn-controller.c | 4 ++++ > > 2 files changed, 36 insertions(+), 14 deletions(-) > > > >diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > >index ce9cccf..f295473 100644 > >--- a/ovn/controller/binding.c > >+++ b/ovn/controller/binding.c > >@@ -1,4 +1,4 @@ > >-/* Copyright (c) 2015 Nicira, Inc. > >+/* Copyright (c) 2015, 2016 Nicira, Inc. > > * > > * Licensed under the Apache License, Version 2.0 (the "License"); > > * you may not use this file except in compliance with the License. > >@@ -47,7 +47,7 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > } > > static void > >-get_local_iface_ids(const struct ovsrec_bridge *br_int, struct sset *lports) > >+get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash > >*lports) > > { > > int i; > >@@ -68,7 +68,7 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, > >struct sset *lports) > > if (!iface_id) { > > continue; > > } > >- sset_add(lports, iface_id); > >+ shash_add(lports, iface_id, iface_rec); > > } > > } > > } > >@@ -132,6 +132,17 @@ add_local_datapath(struct hmap *local_datapaths, > > } > > } > >+static void > >+update_qos(const struct ovsrec_interface *iface_rec, > >+ const struct sbrec_port_binding *pb) > >+{ > >+ int rate = smap_get_int(&pb->options, "policing_rate", 0); > >+ int burst = smap_get_int(&pb->options, "policing_burst", 0); > >+ > >+ ovsrec_interface_set_ingress_policing_rate(iface_rec, MAX(0, rate)); > >+ ovsrec_interface_set_ingress_policing_burst(iface_rec, MAX(0, burst)); > >+} > >+ > > void > > binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > > const char *chassis_id, struct simap *ct_zones, > >@@ -139,8 +150,6 @@ binding_run(struct controller_ctx *ctx, const struct > >ovsrec_bridge *br_int, > > { > > const struct sbrec_chassis *chassis_rec; > > const struct sbrec_port_binding *binding_rec; > >- struct sset lports, all_lports; > >- const char *name; > > if (!ctx->ovnsb_idl_txn) { > > return; > >@@ -151,15 +160,19 @@ binding_run(struct controller_ctx *ctx, const struct > >ovsrec_bridge *br_int, > > return; > > } > >- sset_init(&lports); > >- sset_init(&all_lports); > >+ struct shash lports = SHASH_INITIALIZER(&lports); > > if (br_int) { > > get_local_iface_ids(br_int, &lports); > > } else { > > /* We have no integration bridge, therefore no local logical ports. > > * We'll remove our chassis from all port binding records below. */ > > } > >- sset_clone(&all_lports, &lports); > >+ > >+ struct sset all_lports = SSET_INITIALIZER(&all_lports); > >+ struct shash_node *node; > >+ SHASH_FOR_EACH (node, &lports) { > >+ sset_add(&all_lports, node->name); > >+ } > > ovsdb_idl_txn_add_comment( > > ctx->ovnsb_idl_txn,"ovn-controller: updating port bindings for > > '%s'", > >@@ -169,14 +182,19 @@ binding_run(struct controller_ctx *ctx, const struct > >ovsrec_bridge *br_int, > > * chassis and update the binding accordingly. This includes both > > * directly connected logical ports and children of those ports. */ > > SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { > >- if (sset_find_and_delete(&lports, binding_rec->logical_port) || > >- (binding_rec->parent_port && binding_rec->parent_port[0] && > >- sset_contains(&all_lports, binding_rec->parent_port))) { > >+ const struct ovsrec_interface *iface_rec > >+ = shash_find_and_delete(&lports, binding_rec->logical_port); > >+ if (iface_rec > >+ || (binding_rec->parent_port && binding_rec->parent_port[0] && > >+ sset_contains(&all_lports, binding_rec->parent_port))) { > > if (binding_rec->parent_port && binding_rec->parent_port[0]) { > > /* Add child logical port to the set of all local ports. */ > > sset_add(&all_lports, binding_rec->logical_port); > > } > > add_local_datapath(local_datapaths, binding_rec); > >+ if (iface_rec) { > >+ update_qos(iface_rec, binding_rec); > >+ } > > if (binding_rec->chassis == chassis_rec) { > > continue; > > } > >@@ -199,13 +217,13 @@ binding_run(struct controller_ctx *ctx, const struct > >ovsrec_bridge *br_int, > > } > > } > >- SSET_FOR_EACH (name, &lports) { > >- VLOG_DBG("No port binding record for lport %s", name); > >+ SHASH_FOR_EACH (node, &lports) { > >+ VLOG_DBG("No port binding record for lport %s", node->name); > > } > > update_ct_zones(&all_lports, ct_zones, ct_zone_bitmap); > >- sset_destroy(&lports); > >+ shash_destroy(&lports); > > sset_destroy(&all_lports); > > } > >diff --git a/ovn/controller/ovn-controller.c > >b/ovn/controller/ovn-controller.c > >index 3638342..ac824ad 100644 > >--- a/ovn/controller/ovn-controller.c > >+++ b/ovn/controller/ovn-controller.c > >@@ -240,6 +240,10 @@ main(int argc, char *argv[]) > > ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_name); > > ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_type); > > ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_interface_col_options); > >+ ovsdb_idl_add_column(ovs_idl_loop.idl, > >+ &ovsrec_interface_col_ingress_policing_rate); > >+ ovsdb_idl_add_column(ovs_idl_loop.idl, > >+ &ovsrec_interface_col_ingress_policing_burst); > > ovsdb_idl_add_table(ovs_idl_loop.idl, &ovsrec_table_port); > > ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_port_col_name); > > ovsdb_idl_add_column(ovs_idl_loop.idl, &ovsrec_port_col_interfaces); > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev