[ovs-dev] [PATCH v5] ovn-controller: Assign zone-id consistently

Russell Bryant russell at ovn.org
Mon Feb 15 21:26:32 UTC 2016


On 02/15/2016 03:46 PM, Ramu Ramamurthy wrote:
> Currently, ovn-controller does not record the
> lport->zoneid map, and so, after ovn-controller restart,
> zone-ids may get set inconsistently on lports, resulting
> in possible hits to already established connections.
> 
> Set zone-id as an external-id of the interface record,
> and recover the zone-id from that record
> 
> Reported-by: Russell Bryant <russell at ovn.org>
> Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1538696
> Signed-off-by: Ramu Ramamurthy <ramu.ramamurthy at us.ibm.com>
> ---
>  Changes v3 to v4: update as per code-review
>    * simplify code to update zone-id only when needed
>    * update documentation for external id
>    * update authors
>  Changes v4 to v5: fix formatting
> 
>  AUTHORS                             |  1 +
>  ovn/controller/binding.c            | 39 +++++++++++++++++++++++++++++++++++--
>  ovn/controller/ovn-controller.8.xml | 13 +++++++++++++
>  3 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/AUTHORS b/AUTHORS
> index 936394d..597899d 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -161,6 +161,7 @@ pritesh                 pritesh.kothari at cisco.com
>  Pravin B Shelar         pshelar at nicira.com
>  Raju Subramanian        rsubramanian at nicira.com
>  Rami Rosen              ramirose at gmail.com
> +Ramu Ramamurthy         ramu.ramamurthy at us.ibm.com
>  Randall Sharo           andall.sharo at navy.mil
>  Ravi Kerur              Ravi.Kerur at telekom.com
>  Reid Price              reid at nicira.com
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index cb12cea..543686c 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -50,7 +50,39 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>  }
>  
>  static void
> -get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports)
> +update_local_zone_id(const struct ovsrec_interface *iface_rec, const char *iface_id,
> +                     struct simap *ct_zones, unsigned long *ct_zone_bitmap,
> +                     struct controller_ctx *ctx)
> +{
> +    int zone_id;
> +    const char *OVN_ZONE_ID = "ovn-zone-id";

I'd make this:

    static const char OVN_ZONE_ID[] = "ovn-zone-id";

> +
> +    zone_id = smap_get_int(&iface_rec->external_ids, OVN_ZONE_ID, 0);
> +    if (zone_id && !simap_contains(ct_zones, iface_id)) {
> +        /* get zone-id from the local interface record */
> +        bitmap_set1(ct_zone_bitmap, zone_id);
> +        simap_put(ct_zones, iface_id, zone_id);
> +    }
> +    if (!zone_id && simap_contains(ct_zones, iface_id) &&
> +        ctx->ovs_idl_txn) {
> +        /* zone-id has been assigned to lport, but not
> +         * recorded in the local interface record yet */

This code gets hit the 2nd time binding_run() gets called, which seems a
bit odd to me.

It'd be nice to see this get done when we choose a zone ID inside of
update_ct_zones().  all_lports could become an shash that also has
references to the interface record.  Then update_ct_zones() would have
what it needs to set the zone.

Unfortunately, I thought of another issue that complicates this whole
approach.  A single interface does not necessarily map to a single
logical port and zone ID.  We support sub-ports, initially aimed at
modelling containers in VMs.  That means we need to track N different
zone IDs on a single interface record.  For more info, see "Life Cycle
of a Container Interface Inside a VM" in ovn-archtecture(7).

For that use case, we could easily have many hundreds of sub-ports using
a single interface.  Maybe we should have external-id keys of the form
"ovn-zone-id-<name>" where <name> is the logical port the zone id is
for?  We'd need some additional code for cleaning up zone IDs for
sub-ports that have been deleted.

We'll also need some additional code to handle remembering the zone-id
for "localnet" ports, which are handled slightly differently.  They
don't have iface-id set, but they do have "ovn-localnet-port" set to the
name of the logical port.

> +        struct smap new;
> +        char zone[12];
> +        zone_id = simap_get(ct_zones, iface_id);
> +        snprintf(zone, sizeof zone, "%d", zone_id);
> +        smap_clone(&new, &iface_rec->external_ids);
> +        smap_replace(&new, OVN_ZONE_ID, zone);
> +        ovsrec_interface_verify_external_ids(iface_rec);
> +        ovsrec_interface_set_external_ids(iface_rec, &new);
> +        smap_destroy(&new);
> +    }
> +}
> +
> +static void
> +get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports,
> +                    struct simap *ct_zones, unsigned long *ct_zone_bitmap,
> +                    struct controller_ctx *ctx)
>  {
>      int i;
>  
> @@ -72,10 +104,13 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports)
>                  continue;
>              }
>              shash_add(lports, iface_id, iface_rec);
> +            update_local_zone_id(iface_rec, iface_id, ct_zones,
> +                                 ct_zone_bitmap, ctx);
>          }
>      }
>  }
>  
> +

Unrelated formatting change here.

>  static void
>  update_ct_zones(struct sset *lports, struct simap *ct_zones,
>                  unsigned long *ct_zone_bitmap)
> @@ -165,7 +200,7 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
>  
>      struct shash lports = SHASH_INITIALIZER(&lports);
>      if (br_int) {
> -        get_local_iface_ids(br_int, &lports);
> +        get_local_iface_ids(br_int, &lports, ct_zones, ct_zone_bitmap, ctx);
>      } else {
>          /* We have no integration bridge, therefore no local logical ports.
>           * We'll remove our chassis from all port binding records below. */
> diff --git a/ovn/controller/ovn-controller.8.xml b/ovn/controller/ovn-controller.8.xml
> index b261af9..383de61 100644
> --- a/ovn/controller/ovn-controller.8.xml
> +++ b/ovn/controller/ovn-controller.8.xml
> @@ -194,6 +194,19 @@
>            logical patch port that it implements.
>          </p>
>        </dd>
> +
> +      <dt>
> +        <code>external-ids:ovn-zone-id</code> in the
> +        <code>Interface</code> table
> +      </dt>
> +
> +      <dd>
> +        <p>
> +          This key is set by the <code>ovn-controller</code> to identify the
> +          conntrack-zone id used for the OVN logical port. Its value specifies
> +          the zone-id.
> +        </p>
> +      </dd>
>      </dl>
>  
>      <h1>Runtime Management Commands</h1>
> 


-- 
Russell Bryant



More information about the dev mailing list