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
"[email protected]" 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
