Hi Xavier, some minor comments below.
On Thu, Oct 20, 2022 at 4:53 PM Xavier Simonart <[email protected]> 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 <[email protected]> > > --- > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Sorry for nit-picking, my comment is mainly about the sleep in the test. Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
