I see that now. I believe that some of my comments are still applicable. I'll re-send them for v5.
On Mon, Feb 22, 2016 at 07:49:01PM -0500, Russell Bryant wrote: > Hi, Ben. I know we've had a lot of mailing list problems so you may have > missed some of the follow-up to this patch. It's up to v5. I believe > another revision is coming to address saving zone IDs for container-in-vm > and localnet ports. > > http://openvswitch.org/pipermail/dev/2016-February/066179.html > > On Mon, Feb 22, 2016 at 7:03 PM, Ben Pfaff <b...@ovn.org> wrote: > > > On Mon, Feb 08, 2016 at 10:12:50PM +0000, Suryanarayan Ramamurthy wrote: > > > Currently, conntrack zone-id is assigned to lport by ovn-controller, > > > but the ovn-controller does not remember what was earlier assigned > > > to the same lport (possibly in an earlier run across restart). > > > > > > So, after ovn-controller restart, the zone-ids may get set > > > inconsistently on lports, resulting in possible hits to > > > already established connections. > > > > > > Fix is to remember 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. > > > > > > This patch fixes: > > > https://bugs.launchpad.net/networking-ovn/+bug/1538696 > > > > > > Signed-off-by: Ramu Ramamurthy <ramu.ramamur...@us.ibm.com> > > > > Thank you for the patch. > > > > From this part of the patch, it looks like external-ids:zone-id is > > accepted even if there are duplicates, or if the value is not valid. I > > think that it should reject such cases: > > > + 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); > > > + } > > > > With that in mind, update_local_zone_ids() should also update > > external-ids:zone-id if it needs to change, instead of leaving it the > > same. > > > > This commit removes the XXX comment below. Does it fix the problem that > > comment points out? > > > > > @@ -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. */ > > > } > > > + update_local_zone_ids(br_int, ct_zones, ctx); > > > } > > > > Thanks, > > > > Ben. > > > > > > -- > Russell Bryant _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
