Hi Han Thanks for your comment. Comments embedded below
Thanks Xavier On Wed, Nov 16, 2022 at 9:04 AM Han Zhou <[email protected]> wrote: > > > 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]> > > This is obviously some debatable part of the code as Numan and Dumitru already had questions around it (hence the additional comments in the code). To support the scenario (which I agree should not be very frequent), we can't simply remove this part w/o other changes. When adding and binding the second interface, if we release the interface when ofport = 0, then we lost the information that the port was bound when we receive the ofport for the second i/f and claim it (we set lbinding->iface to NULL in consider_iface_release and consider_iface_claim needs another logic to detect the double binding). > + 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
