Makes sense, thank you for clarification. Acked-by: Ihar Hrachyshka <[email protected]>
FYI since the scenario of multiple interfaces carrying the same iface-id is invalid from OVN perspective and won't be handled gracefully (without recompute), we also move with the change to Neutron to first clean up old metadata interfaces, then create new ones: https://review.opendev.org/c/openstack/neutron/+/864777 This should avoid the scenario for OpenStack. On Fri, Nov 18, 2022 at 9:09 AM Xavier Simonart <[email protected]> wrote: > > Hi Han, Ihar > > Thanks for looking into this. > Han summarizes perfectly the perspective we tried to use in this patch: > balance the complexity of the code versus the gain. > We felt that the described scenario was a corner case, which happens rarely, > and hence was not worth the cost of adding too much complexity. > If we see in the future, for whatever reason, that the case happens more than > expected and the performance cost is important, then we could revisit this > patch. > > One more comment embedded. > Thanks > Xavier > > On Fri, Nov 18, 2022 at 3:04 AM Han Zhou <[email protected]> wrote: >> >> On Thu, Nov 17, 2022 at 10:39 AM Ihar Hrachyshka <[email protected]> >> wrote: >> > >> > On 11/14/22 4:24 AM, Xavier Simonart 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, >> > >> > The description of the function says: >> > >> > "If the local_binding for this 'iface_rec' already exists and its >> > already claimed, then this function will be no-op." >> > >> > which I don't think is true anymore with the patch applied. Consider >> > adjusting the comment above. > > I think that the comment is correct: if the binding for -- this -- iface_rec > already exists and is already claimed, it is a no-op. > It is not a no-op for new bindings, or If the binding exists with different > 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); >> > I wonder if it would be possible to handle this situation gracefully >> > here, by looking through the list of interface records assigned to the >> > same port, and if the list length is 1, then reinstate flows, otherwise >> > do nothing. Thoughts? >> >> Thanks Ihar. Here are my 2 cents: >> It is possible, but it will also be more complex without bringing much >> value. I-P is complex enough already. I think we'd better keep I-P for the >> common scenarios where performance matters, and let such corner cases >> caused by misconfiguration fall-back to recompute. >> >> Han >> >> > > + 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. >> > > + */ >> > > + 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([]) >> > >> > _______________________________________________ >> > 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 >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
