The subject has an unexpected leading ":". That should probably be removed. I'd then replace " - " with ": " to more closely follow convention.
I get several whitespace errors when applying this patch: Applying: ovn-controller - Assign zone-ids consistently /home/rbryant/src/ovs/.git/rebase-apply/patch:43: trailing whitespace. if (!iface_id || /home/rbryant/src/ovs/.git/rebase-apply/patch:51: trailing whitespace. smap_clone(&new, &iface_rec->external_ids); /home/rbryant/src/ovs/.git/rebase-apply/patch:52: trailing whitespace. smap_replace(&new, "zone-id", zone); /home/rbryant/src/ovs/.git/rebase-apply/patch:63: trailing whitespace. get_local_iface_ids(const struct ovsrec_bridge *br_int, struct sset *lports, /home/rbryant/src/ovs/.git/rebase-apply/patch:89: trailing whitespace. unsigned long *ct_zone_bitmap, warning: 5 lines add whitespace errors. On 02/09/2016 08:46 PM, Suryanarayan Ramamurthy wrote: > Currently, conntrack zone-id is assigned to lport by ovn-controller, > but the ovn-controller does not record this map (lport->zoneid) > > So, after ovn-controller restart, zone-ids may get set > inconsistently on lports, resulting in possible hits to > already established connections. > > The fix is to set the zone-id as an external-id of the interface record > in the local ovs-db, and recover zone-ids assigned earlier to lports > from that record. > > Changes in v2: > - add a check for null br_int in update_local_zone_ids > > Reported-by: Russell Bryant <russell.bryant at gmail.com> In addition to Ben's commit message comments, please use "russ...@ovn.org" for me. Also replace " at " with "@". > Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1538696 > Signed-off-by: Ramu Ramamurthy <ramu.ramamurthy at us.ibm.com> > > --- > ovn/controller/binding.c | 71 > +++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 64 insertions(+), 7 deletions(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index ce9cccf..aa3efa4 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > @@ -46,10 +46,61 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > ovsdb_idl_add_column(ovs_idl, &ovsrec_interface_col_external_ids); > } > > + This looks like an unrelated formatting change. > static void > -get_local_iface_ids(const struct ovsrec_bridge *br_int, struct sset *lports) > +update_local_zone_ids(const struct ovsrec_bridge *br_int, struct simap > *ct_zones, > + struct controller_ctx *ctx) > { > int i; > + struct smap new; > + int zone_id; > + char *zone; I'd localize these variables to the block of code that uses them. > + > + if (!ctx->ovs_idl_txn || !br_int) { > + return; > + } > + > + for (i = 0; i < br_int->n_ports; i++) { > + const struct ovsrec_port *port_rec = br_int->ports[i]; > + const char *iface_id; > + int j; > + > + if (!strcmp(port_rec->name, br_int->name)) { > + continue; > + } > + > + for (j = 0; j < port_rec->n_interfaces; j++) { > + const struct ovsrec_interface *iface_rec; > + > + iface_rec = port_rec->interfaces[j]; > + iface_id = smap_get(&iface_rec->external_ids, "iface-id"); > + > + if (!iface_id || > + smap_get(&iface_rec->external_ids, "zone-id") || Maybe we should make this "ovn-zone-id" to make it clear that this is an ID managed by OVN. > + !simap_contains(ct_zones, iface_id)) { > + continue; > + } > + > + zone_id = simap_get(ct_zones, iface_id); > + zone = xasprintf("%d", zone_id); Since zone_id is an int, we know how long it can be. I'd prefer just a sufficiently sized char array on the stack vs. malloc/free every time. > + smap_clone(&new, &iface_rec->external_ids); > + smap_replace(&new, "zone-id", zone); > + ovsrec_interface_verify_external_ids(iface_rec); > + ovsrec_interface_set_external_ids(iface_rec, &new); > + free(zone); > + smap_destroy(&new); > + } > + } > +} > + > + > +static void > +get_local_iface_ids(const struct ovsrec_bridge *br_int, struct sset *lports, > + struct simap *ct_zones, unsigned long *ct_zone_bitmap) > +{ > + int i; > + const char *zone; > + int zone_id; > > for (i = 0; i < br_int->n_ports; i++) { > const struct ovsrec_port *port_rec = br_int->ports[i]; > @@ -69,13 +120,21 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, > struct sset *lports) > continue; > } > sset_add(lports, iface_id); > + zone = smap_get(&iface_rec->external_ids, "zone-id"); > + if (zone && ovs_scan(zone, "%d", &zone_id)) { > + bitmap_set1(ct_zone_bitmap, zone_id); > + simap_put(ct_zones, iface_id, zone_id); > + } > } > } > } > > + > static void > update_ct_zones(struct sset *lports, struct simap *ct_zones, > - unsigned long *ct_zone_bitmap) > + unsigned long *ct_zone_bitmap, > + const struct ovsrec_bridge *br_int, > + struct controller_ctx *ctx) > { > struct simap_node *ct_zone, *ct_zone_next; > const char *iface_id; > @@ -112,10 +171,8 @@ update_ct_zones(struct sset *lports, struct simap > *ct_zones, > bitmap_set1(ct_zone_bitmap, zone); > simap_put(ct_zones, iface_id, zone); > > - /* xxx We should erase any old entries for this > - * xxx zone, but we need a generic interface to the conntrack > - * xxx table. */ Please don't remove this comment. This particular issue still exists, I believe. This patch will help ensure that we continue to pick the same zone ID for the same port, even across ovn-controller restarts, but it doesn't address that a zone could have entries from previous usage that we need to clear out. > } > + update_local_zone_ids(br_int, ct_zones, ctx); > } > > static void > @@ -154,7 +211,7 @@ binding_run(struct controller_ctx *ctx, const struct > ovsrec_bridge *br_int, > sset_init(&lports); > sset_init(&all_lports); > if (br_int) { > - get_local_iface_ids(br_int, &lports); > + get_local_iface_ids(br_int, &lports, ct_zones, ct_zone_bitmap); > } else { > /* We have no integration bridge, therefore no local logical ports. > * We'll remove our chassis from all port binding records below. */ > @@ -203,7 +260,7 @@ binding_run(struct controller_ctx *ctx, const struct > ovsrec_bridge *br_int, > VLOG_DBG("No port binding record for lport %s", name); > } > > - update_ct_zones(&all_lports, ct_zones, ct_zone_bitmap); > + update_ct_zones(&all_lports, ct_zones, ct_zone_bitmap, br_int, ctx); > > sset_destroy(&lports); > sset_destroy(&all_lports); > -- Russell Bryant _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev