On Mon, Nov 14, 2022 at 1:24 AM 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 > v4: - Updated based on Ales' feedback > - Also support scenario for port-types different than localport > - Added test case for VIF port > - Rebased on latest main > v5: - Updated test case based on Numan/Dumitru's feedback (hit ofport = 0) > and added comments in code. > - Rebased on latest main. > --- > controller/binding.c | 36 ++++++++++ > controller/binding.h | 1 + > tests/ovn.at | 168 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 205 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index c3d2b2e42..5f04b9a74 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; > } > > @@ -2174,6 +2179,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, > return true; > } > > + /* If multiple bindings to the same port, remove the "old" binding. > + * This ensures that change tracking is correct. > + */ > + if (lbinding->multiple_bindings) { > + remove_related_lport(pb, b_ctx_out); > + } > + > enum en_lport_type lport_type = get_lport_type(pb); > if (lport_type == LP_LOCALPORT) { > return consider_localport(pb, b_ctx_in, b_ctx_out); > @@ -2226,6 +2238,29 @@ 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) { > + /* If external_ids:iface-id is set within the same transaction > + * as adding an interface to a bridge, ovn-controller is > + * usually initially notified of ovs interface changes with > + * ofport == 0. If the lport was bound to a different interface > + * we do not want to release it. > + */
Hi Xavier, I think this logic in the "else" block, if really needed, belongs to a separate patch. It looks like an optimization to the scenario when moving a logical port binding from one VIF to another on the same node. I guess this doesn't happen very often in the real world. If it happens in some corner cases, it shouldn't be a problem of letting it release and reclaim, right? So I wonder if this change is really necessary. Assume it is correct, it still adds some burden when reading/debugging the code. What do you think? For the rest of the patch: Acked-by: Han Zhou <[email protected]> > + 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 +3069,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..6c3a98b02 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; > + bool multiple_bindings; > }; > > struct local_binding_data { > diff --git a/tests/ovn.at b/tests/ovn.at > index 6552681bd..7a918c639 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -33053,3 +33053,171 @@ check ovn-nbctl --wait=hv sync > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +m4_define([MULTIPLE_OVS_INT], > + [OVN_FOR_EACH_NORTHD([ > + AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical port ($1)]) > + 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 > + if test X$1 != X; then > + check ovn-nbctl lsp-set-type lp $1 > + fi > + 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 ====================================================== > + # Set external_ids:iface-id within same transaction as adding the port. > + # This will generally cause ovn-controller to get initially notified of ovs interface changes with ofport == 0. > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal -- 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 for controller to get time to process any update > + check ovn-nbctl --wait=hv sync > + 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 ====================================================== > + # Set external_ids:iface-id in a different transaction as adding the port. > + # This will generally cause ovn-controller to get notified of ovs interface changes with a proper ofport. > + 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 > + ])]) > + > +MULTIPLE_OVS_INT([localport]) > +MULTIPLE_OVS_INT([]) > -- > 2.31.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
