| Message ID | 20221020145302.563272-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] ovn-controller: Fixed missing flows after interface deletion | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Hi Xavier, some minor comments below. On Thu, Oct 20, 2022 at 4:53 PM Xavier Simonart <xsimonar@redhat.com> wrote: > In the following scenario: > - interface "old" is created and external_ids:iface-id is set (to lp) > - interface "new" is created and external_ids:iface-id is set (to same lp) > - interface "old" is deleted > flows related to lp were deleted. > > Note that after "new" interface is created, flows use "new" ofport. > The state where old and new interfaces have both external_ids:iface-id set > at > the same time is "invalid", and all flows are not installed for lpold. > > Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for pflow_output > engine.") > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866 > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > v2: - Use bool instead of int for binding count to better reflect only > one additional binding is supported. > - Fix use after free. > - Remove debug logging from test case > v3: - Based on Dumitru's and Mark's feedback: > - Support any number of interfaces bound to the same port > - Use recomputes to make code simpler (this is corner case) > - Added test case using three interfaces bound to te same port > --- > controller/binding.c | 24 +++++++ > controller/binding.h | 1 + > tests/ovn.at | 159 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 184 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index c3d2b2e42..b991c5a9b 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1866,6 +1866,7 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > lbinding = local_binding_create(iface_id, iface_rec); > local_binding_add(local_bindings, lbinding); > } else { > + lbinding->multiple_bindings = true; > static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(1, 5); > VLOG_WARN_RL( > @@ -2156,6 +2157,10 @@ consider_iface_claim(const struct ovsrec_interface > *iface_rec, > lbinding = local_binding_create(iface_id, iface_rec); > local_binding_add(local_bindings, lbinding); > } else { > + if (lbinding->iface && lbinding->iface != iface_rec) { > + lbinding->multiple_bindings = true; > + b_ctx_out->local_lports_changed = true; > + } > lbinding->iface = iface_rec; > } > > @@ -2226,6 +2231,24 @@ consider_iface_release(const struct > ovsrec_interface *iface_rec, > struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; > > lbinding = local_binding_find(local_bindings, iface_id); > + > + if (lbinding) { > + if (lbinding->multiple_bindings) { > + VLOG_INFO("Multiple bindings for %s: force recompute to clean > up", > + iface_id); > + return false; > + } else { > + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > + if (lbinding->iface != iface_rec && !ofport) { > + VLOG_DBG("Not releasing lport %s as %s was claimed " > + "and %s was never bound)", > + iface_id, lbinding->iface ? > lbinding->iface->name:"", > nit: missing spaces in :lbinding->iface->name:"" > + iface_rec->name); > + return true; > + } > + } > + } > + > struct binding_lport *b_lport = > local_binding_get_primary_or_localport_lport(lbinding); > if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) { > @@ -3034,6 +3057,7 @@ local_binding_create(const char *name, const struct > ovsrec_interface *iface) > struct local_binding *lbinding = xzalloc(sizeof *lbinding); > lbinding->name = xstrdup(name); > lbinding->iface = iface; > + lbinding->multiple_bindings = false; > ovs_list_init(&lbinding->binding_lports); > > return lbinding; > diff --git a/controller/binding.h b/controller/binding.h > index ad959a9e6..2a5624a07 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -135,6 +135,7 @@ struct local_binding { > char *name; > const struct ovsrec_interface *iface; > struct ovs_list binding_lports; > + unsigned int multiple_bindings; > nit: You should probably use the bool type to make it clear that this should be only true/false. > }; > > struct local_binding_data { > diff --git a/tests/ovn.at b/tests/ovn.at > index f8b8db4df..b8ec63bf2 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -32975,3 +32975,162 @@ check ovn-nbctl --wait=hv sync > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical > port]) > +ovn_start > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +get_flows() > +{ > + cookie=$1 > + ovs-ofctl dump-flows br-int | grep $cookie | > + sed -e 's/duration=[[0-9.]]*s, //g' | > + sed -e 's/idle_age=[[0-9]]*, //g' | > + sed -e 's/n_packets=[[0-9]]*, //g' | > + sed -e 's/n_bytes=[[0-9]]*, //g' > +} > + > +check ovn-nbctl ls-add ls > +check ovn-nbctl lsp-add ls lp > +check ovn-nbctl lsp-set-type lp localport > +check ovn-nbctl lsp-set-addresses lp "00:00:00:01:01:02 192.168.1.2" > + > +check ovn-nbctl lsp-add ls vm1 > +check ovn-nbctl lsp-set-addresses vm1 "00:00:00:01:01:11 192.168.1.11" > +check ovs-vsctl add-port br-int vm1 -- set interface vm1 type=internal > external_ids:iface-id=vm1 > + > +check ovn-nbctl --wait=hv sync > + > +check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal > +check ovs-vsctl set interface lpold external_ids:iface-id=lp > + > +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns _uuid find > port_binding logical_port=lp) != x]) > +echo ====================================================== > +echo === Flows after iface-id set for the old interface === > +echo ====================================================== > +COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut -d: > -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//') > + > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpold) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > +]) > +nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` > +echo $nb_flows "flows after iface-id set for old interface" > + > +echo ====================================================== > +echo === Flows after iface-id set for the new interface === > +echo ====================================================== > +check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal > +check ovs-vsctl set interface lpnew external_ids:iface-id=lp > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > +]) > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > +flows_lpnew=$(get_flows $COOKIE) > + > +echo ====================================================== > +echo ======= Flows after old interface is deleted ========= > +echo ====================================================== > +check ovs-vsctl del-port br-int lpold > +# We do not expect changes, so let's wait a few seconds to check nothing > changed > +sleep 2 > Wouldn't be ovn-nbctl --wait=hv sync enough? I have noticed that our tests are getting slower lately and adding another sleep does not seem like the best idea IMO. > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > +flows_after_deletion=$(get_flows $COOKIE) > +check test "$flows_lpnew" = "$flows_after_deletion" > + > +echo ====================================================== > +echo ======= Flows after lptemp interface is created ==== > +echo ====================================================== > +check ovs-vsctl add-port br-int lptemp -- set Interface lptemp > type=internal > +check ovs-vsctl set Interface lptemp external_ids:iface-id=lp > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lptemp) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > +]) > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > + > +echo ====================================================== > +echo ======= Flows after lptemp interface is deleted ====== > +echo ====================================================== > +check ovs-vsctl del-port br-int lptemp > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) > + echo $ofport > + ovs-ofctl dump-flows br-int | grep $COOKIE > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > +]) > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > +flows_after_deletion=$(get_flows $COOKIE) > +check test "$flows_lpnew" = "$flows_after_deletion" > + > +echo ====================================================== > +echo ======= Flows after new interface is deleted ========= > +echo ====================================================== > +check ovs-vsctl del-port br-int lpnew > +OVS_WAIT_UNTIL([ > + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` > + test "${nb_flows}" = 0 > +]) > + > +echo ====================================================== > +echo ======= Three interfaces bound to the same port ====== > +echo ====================================================== > +check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal > +check ovs-vsctl set interface lpold external_ids:iface-id=lp > +check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal > +check ovs-vsctl set interface lpnew external_ids:iface-id=lp > + > +# Wait for lpnew flows to be installed > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > +]) > +flows_lpnew=$(get_flows $COOKIE) > +nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` > + > +check ovs-vsctl add-port br-int lptemp -- set Interface lptemp > type=internal > +check ovs-vsctl set Interface lptemp external_ids:iface-id=lp > + > +# Wait for lptemp flows to be installed > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lptemp) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > +]) > + > +# Delete both lpold and lptemp to go to a stable situation > +check ovs-vsctl del-port br-int lptemp > +check ovs-vsctl del-port br-int lpold > + > +OVS_WAIT_UNTIL([ > + test 0 = $(ovs-vsctl show | grep "Port lpold" | wc -l) > +]) > + > +# Wait for correct/lpnew flows to be installed > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > +]) > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > +flows_after_deletion=$(get_flows $COOKIE) > +check test "$flows_lpnew" = "$flows_after_deletion" > + > +# Check that recompute still works > +check ovn-appctl -t ovn-controller recompute > +OVS_WAIT_UNTIL([ > + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep > "actions=output:$ofport" > +]) > +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | > wc -l) > +flows_after_deletion=$(get_flows $COOKIE) > +check test "$flows_lpnew" = "$flows_after_deletion" > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Sorry for nit-picking, my comment is mainly about the sleep in the test. Thanks, Ales
diff --git a/controller/binding.c b/controller/binding.c index c3d2b2e42..b991c5a9b 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1866,6 +1866,7 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, lbinding = local_binding_create(iface_id, iface_rec); local_binding_add(local_bindings, lbinding); } else { + lbinding->multiple_bindings = true; static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); VLOG_WARN_RL( @@ -2156,6 +2157,10 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, lbinding = local_binding_create(iface_id, iface_rec); local_binding_add(local_bindings, lbinding); } else { + if (lbinding->iface && lbinding->iface != iface_rec) { + lbinding->multiple_bindings = true; + b_ctx_out->local_lports_changed = true; + } lbinding->iface = iface_rec; } @@ -2226,6 +2231,24 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; lbinding = local_binding_find(local_bindings, iface_id); + + if (lbinding) { + if (lbinding->multiple_bindings) { + VLOG_INFO("Multiple bindings for %s: force recompute to clean up", + iface_id); + return false; + } else { + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; + if (lbinding->iface != iface_rec && !ofport) { + VLOG_DBG("Not releasing lport %s as %s was claimed " + "and %s was never bound)", + iface_id, lbinding->iface ? lbinding->iface->name:"", + iface_rec->name); + return true; + } + } + } + struct binding_lport *b_lport = local_binding_get_primary_or_localport_lport(lbinding); if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) { @@ -3034,6 +3057,7 @@ local_binding_create(const char *name, const struct ovsrec_interface *iface) struct local_binding *lbinding = xzalloc(sizeof *lbinding); lbinding->name = xstrdup(name); lbinding->iface = iface; + lbinding->multiple_bindings = false; ovs_list_init(&lbinding->binding_lports); return lbinding; diff --git a/controller/binding.h b/controller/binding.h index ad959a9e6..2a5624a07 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -135,6 +135,7 @@ struct local_binding { char *name; const struct ovsrec_interface *iface; struct ovs_list binding_lports; + unsigned int multiple_bindings; }; struct local_binding_data { diff --git a/tests/ovn.at b/tests/ovn.at index f8b8db4df..b8ec63bf2 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -32975,3 +32975,162 @@ check ovn-nbctl --wait=hv sync OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical port]) +ovn_start +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +get_flows() +{ + cookie=$1 + ovs-ofctl dump-flows br-int | grep $cookie | + sed -e 's/duration=[[0-9.]]*s, //g' | + sed -e 's/idle_age=[[0-9]]*, //g' | + sed -e 's/n_packets=[[0-9]]*, //g' | + sed -e 's/n_bytes=[[0-9]]*, //g' +} + +check ovn-nbctl ls-add ls +check ovn-nbctl lsp-add ls lp +check ovn-nbctl lsp-set-type lp localport +check ovn-nbctl lsp-set-addresses lp "00:00:00:01:01:02 192.168.1.2" + +check ovn-nbctl lsp-add ls vm1 +check ovn-nbctl lsp-set-addresses vm1 "00:00:00:01:01:11 192.168.1.11" +check ovs-vsctl add-port br-int vm1 -- set interface vm1 type=internal external_ids:iface-id=vm1 + +check ovn-nbctl --wait=hv sync + +check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal +check ovs-vsctl set interface lpold external_ids:iface-id=lp + +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns _uuid find port_binding logical_port=lp) != x]) +echo ====================================================== +echo === Flows after iface-id set for the old interface === +echo ====================================================== +COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut -d: -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//') + +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpold) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` +echo $nb_flows "flows after iface-id set for old interface" + +echo ====================================================== +echo === Flows after iface-id set for the new interface === +echo ====================================================== +check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal +check ovs-vsctl set interface lpnew external_ids:iface-id=lp +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_lpnew=$(get_flows $COOKIE) + +echo ====================================================== +echo ======= Flows after old interface is deleted ========= +echo ====================================================== +check ovs-vsctl del-port br-int lpold +# We do not expect changes, so let's wait a few seconds to check nothing changed +sleep 2 +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_after_deletion=$(get_flows $COOKIE) +check test "$flows_lpnew" = "$flows_after_deletion" + +echo ====================================================== +echo ======= Flows after lptemp interface is created ==== +echo ====================================================== +check ovs-vsctl add-port br-int lptemp -- set Interface lptemp type=internal +check ovs-vsctl set Interface lptemp external_ids:iface-id=lp +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lptemp) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) + +echo ====================================================== +echo ======= Flows after lptemp interface is deleted ====== +echo ====================================================== +check ovs-vsctl del-port br-int lptemp +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) + echo $ofport + ovs-ofctl dump-flows br-int | grep $COOKIE + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_after_deletion=$(get_flows $COOKIE) +check test "$flows_lpnew" = "$flows_after_deletion" + +echo ====================================================== +echo ======= Flows after new interface is deleted ========= +echo ====================================================== +check ovs-vsctl del-port br-int lpnew +OVS_WAIT_UNTIL([ + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` + test "${nb_flows}" = 0 +]) + +echo ====================================================== +echo ======= Three interfaces bound to the same port ====== +echo ====================================================== +check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal +check ovs-vsctl set interface lpold external_ids:iface-id=lp +check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal +check ovs-vsctl set interface lpnew external_ids:iface-id=lp + +# Wait for lpnew flows to be installed +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +flows_lpnew=$(get_flows $COOKIE) +nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` + +check ovs-vsctl add-port br-int lptemp -- set Interface lptemp type=internal +check ovs-vsctl set Interface lptemp external_ids:iface-id=lp + +# Wait for lptemp flows to be installed +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lptemp) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) + +# Delete both lpold and lptemp to go to a stable situation +check ovs-vsctl del-port br-int lptemp +check ovs-vsctl del-port br-int lpold + +OVS_WAIT_UNTIL([ + test 0 = $(ovs-vsctl show | grep "Port lpold" | wc -l) +]) + +# Wait for correct/lpnew flows to be installed +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_after_deletion=$(get_flows $COOKIE) +check test "$flows_lpnew" = "$flows_after_deletion" + +# Check that recompute still works +check ovn-appctl -t ovn-controller recompute +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_after_deletion=$(get_flows $COOKIE) +check test "$flows_lpnew" = "$flows_after_deletion" + +OVN_CLEANUP([hv1]) +AT_CLEANUP +])
In the following scenario: - interface "old" is created and external_ids:iface-id is set (to lp) - interface "new" is created and external_ids:iface-id is set (to same lp) - interface "old" is deleted flows related to lp were deleted. Note that after "new" interface is created, flows use "new" ofport. The state where old and new interfaces have both external_ids:iface-id set at the same time is "invalid", and all flows are not installed for lpold. Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for pflow_output engine.") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866 Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- v2: - Use bool instead of int for binding count to better reflect only one additional binding is supported. - Fix use after free. - Remove debug logging from test case v3: - Based on Dumitru's and Mark's feedback: - Support any number of interfaces bound to the same port - Use recomputes to make code simpler (this is corner case) - Added test case using three interfaces bound to te same port --- controller/binding.c | 24 +++++++ controller/binding.h | 1 + tests/ovn.at | 159 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 184 insertions(+)