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<rbry...@redhat.com> > >Reported-at: http://openvswitch.org/pipermail/dev/2015-June/056326.html > >Signed-off-by: Ben Pfaff<b...@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); _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
