Message ID | 20190716180110.21750-1-nusiddiq@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,v2] ovn-northd: Fix the ovn-northd continuous looping | expand |
On Tue, Jul 16, 2019 at 11:31:10PM +0530, nusiddiq@redhat.com wrote: > From: Numan Siddique <nusiddiq@redhat.com> > > ovn-northd wakes up continuously from poll_block(). This issue can be reproduced > in the sandbox with the below commands > > ovn-nbctl lr-add lr0 > ovn-nbctl ls-add public > ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > ovn-nbctl lsp-add public public-lr0 > ovn-nbctl lsp-set-type public-lr0 router > ovn-nbctl lsp-set-addresses public-lr0 router > ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public > ovn-nbctl lrp-set-gateway-chassis lr0-public chassis-1 20 > > This issue is seen after the commit [1], which makes use of the function - > sbrec_port_binding_update_nat_addresses_addvalue() to add a value to > Port_Binding.nat_addresses column. > > Looks like the IDL client code is sending the transactions to the > ovsdb-server repeatedly to update the Port_Binding.nat_addresses even > though the Southbound DB has updated the column when this function is > used. The actual bug seems to be in the IDL client code and that needs > to be fixed. This patch as a quick fix, fixes ovn-northd's continuous > loop by not using this function, instead making use of > sbrec_port_binding_set_nat_addresses(). The *_addvalue() and similar functions do operate blindly on data without regard to whether it's already present. This was actually intentional, to allow for cases where we want to update a record without having to verify its previous contents. However, I'm not sure that this ability is actually used in practice anywhere in the tree, so since it's causing confusion we might want to change its behavior. I think that would actually simplify code, too.
On Tue, Jul 16, 2019 at 02:07:15PM -0700, Ben Pfaff wrote: > On Tue, Jul 16, 2019 at 11:31:10PM +0530, nusiddiq@redhat.com wrote: > > From: Numan Siddique <nusiddiq@redhat.com> > > > > ovn-northd wakes up continuously from poll_block(). This issue can be reproduced > > in the sandbox with the below commands > > > > ovn-nbctl lr-add lr0 > > ovn-nbctl ls-add public > > ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > > ovn-nbctl lsp-add public public-lr0 > > ovn-nbctl lsp-set-type public-lr0 router > > ovn-nbctl lsp-set-addresses public-lr0 router > > ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public > > ovn-nbctl lrp-set-gateway-chassis lr0-public chassis-1 20 > > > > This issue is seen after the commit [1], which makes use of the function - > > sbrec_port_binding_update_nat_addresses_addvalue() to add a value to > > Port_Binding.nat_addresses column. > > > > Looks like the IDL client code is sending the transactions to the > > ovsdb-server repeatedly to update the Port_Binding.nat_addresses even > > though the Southbound DB has updated the column when this function is > > used. The actual bug seems to be in the IDL client code and that needs > > to be fixed. This patch as a quick fix, fixes ovn-northd's continuous > > loop by not using this function, instead making use of > > sbrec_port_binding_set_nat_addresses(). > > The *_addvalue() and similar functions do operate blindly on data > without regard to whether it's already present. This was actually > intentional, to allow for cases where we want to update a record without > having to verify its previous contents. However, I'm not sure that this > ability is actually used in practice anywhere in the tree, so since it's > causing confusion we might want to change its behavior. I think that > would actually simplify code, too. Oh, also this patch is correct either way, so I applied it. Thanks!
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 4929fb666..dd0d3d816 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -2532,13 +2532,6 @@ ovn_port_update_sbrec(struct northd_context *ctx, } } - sbrec_port_binding_set_nat_addresses(op->sb, - (const char **) nats, n_nats); - for (size_t i = 0; i < n_nats; i++) { - free(nats[i]); - } - free(nats); - /* Add the router mac and IPv4 addresses to * Port_Binding.nat_addresses so that GARP is sent for these * IPs by the ovn-controller on which the distributed gateway @@ -2580,10 +2573,18 @@ ovn_port_update_sbrec(struct northd_context *ctx, op->peer->od->l3redirect_port->json_key); } - sbrec_port_binding_update_nat_addresses_addvalue( - op->sb, ds_cstr(&garp_info)); + n_nats++; + nats = xrealloc(nats, (n_nats * sizeof *nats)); + nats[n_nats - 1] = ds_steal_cstr(&garp_info); ds_destroy(&garp_info); } + + sbrec_port_binding_set_nat_addresses(op->sb, + (const char **) nats, n_nats); + for (size_t i = 0; i < n_nats; i++) { + free(nats[i]); + } + free(nats); } sbrec_port_binding_set_parent_port(op->sb, op->nbsp->parent_name);