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 <russ...@ovn.org>
> Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1538696
> Signed-off-by: Ramu Ramamurthy <ramu.ramamur...@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.koth...@cisco.com
> Pravin B Shelar pshe...@nicira.com
> Raju Subramanian rsubraman...@nicira.com
> Rami Rosen ramir...@gmail.com
> +Ramu Ramamurthy ramu.ramamur...@us.ibm.com
> Randall Sharo andall.sh...@navy.mil
> Ravi Kerur ravi.ke...@telekom.com
> Reid Price r...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
