[ovs-dev] [PATCH] ovn-controller: Verify bridge ports before changing.

Ben Pfaff blp at nicira.com
Sun Jun 14 17:50:11 UTC 2015


Thanks.  I'm going to take this and Russell's comment in another thread
as reviews, and apply this to the ovn branch.

On Sun, Jun 14, 2015 at 12:14:02PM +0200, Miguel Angel Ajo wrote:
> Good catch :)
> 
> Ben Pfaff wrote:
> >
> >OVSDB is transactional but it does not have built-in protection from dirty
> >reads. To avoid those, it's necessary to manually add verification to
> >transactions to ensure that any data reads whose values were essential to
> >later writes have not changed. ovn-controller didn't do that for
> >the "ports" column in the Bridge table, which means that if the set of
> >ports changed when it didn't expect it, it could revert changes made by
> >other database clients.
> >
> >In particular this showed up in a scale test, where ovn-controller would
> >delete "vif" ports added via ovs-vsctl.
> >
> >(It's easy to see exactly what happened by looking in the database log
> >with "ovsdb-tool -mm show-log".)
> >
> >Reported-by: Russell Bryant<rbryant at redhat.com>
> >Reported-at: http://openvswitch.org/pipermail/dev/2015-June/056326.html
> >Signed-off-by: Ben Pfaff<blp at nicira.com>
> >---
> >ovn/controller/chassis.c | 3 +++
> >1 file changed, 3 insertions(+)
> >
> >diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c
> >index 63e1160..cf18dd0 100644
> >--- a/ovn/controller/chassis.c
> >+++ b/ovn/controller/chassis.c
> >@@ -262,6 +262,7 @@ tunnel_add(struct tunnel_ctx *tc, const char
> >*new_chassis_id,
> >ports[i] = tc->br_int->ports[i];
> >}
> >ports[tc->br_int->n_ports] = port;
> >+ ovsrec_bridge_verify_ports(tc->br_int);
> >ovsrec_bridge_set_ports(tc->br_int, ports, tc->br_int->n_ports + 1);
> >
> >sset_add(&tc->port_names, port_name);
> >@@ -282,6 +283,7 @@ bridge_delete_port(const struct ovsrec_bridge *br,
> >ports[n++] = br->ports[i];
> >}
> >}
> >+ ovsrec_bridge_verify_ports(br);
> >ovsrec_bridge_set_ports(br, ports, n);
> >free(ports);
> >}
> >@@ -430,6 +432,7 @@ chassis_destroy(struct controller_ctx *ctx)
> >ports[n++] = ctx->br_int->ports[i];
> >}
> >}
> >+ ovsrec_bridge_verify_ports(ctx->br_int);
> >ovsrec_bridge_set_ports(ctx->br_int, ports, n);
> >free(ports);



More information about the dev mailing list