On 02/11/2016 08:18 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>
This looks good and seems to be working for me. I found one very minor
issue that I didn't notice the first time through.
> +static void
> +update_local_zone_ids(const struct ovsrec_bridge *br_int, struct simap
> *ct_zones,
> + struct controller_ctx *ctx)
> +{
> + int i;
> +
> + 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];
> + 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;
> + const char *iface_id;
> + struct smap new;
> + int zone_id;
> + char zone[12];
> +
> + 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, "ovn-zone-id") ||
I think you should remove the 1 line above. When loading zone IDs, we have:
zone = smap_get(&iface_rec->external_ids, "ovn-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);
}
This really shouldn't happen, but if somehow ovn-zone-id was set, but
ovs_scan() failed to parse it, we would end up not setting a zone ID at
all and would use the default ct zone, instead.
If you remove this 1 line, I think it will work how we'd want, which is
pretend it wasn't set at all, choose a new ID and overwrite the bad value.
You could also update the code that's loading the IDs to log an error if
ovn-zone-id was set but we failed to parse its value.
> + !simap_contains(ct_zones, iface_id)) {
> + continue;
> + }
> +
> + 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);
> }
> }
> }
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
