On Tue, Nov 22, 2022 at 2:36 AM Han Zhou <[email protected]> wrote: > > On Mon, Nov 21, 2022 at 4:40 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]> > > --- > > controller/binding.c | 43 +++++++---- > > controller/binding.h | 1 + > > tests/ovn.at | 168 +++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 200 insertions(+), 12 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index db1eb7a40..5df62baef 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -1873,6 +1873,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( > > @@ -2163,6 +2164,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; > > } > > > > @@ -2181,6 +2186,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); > > @@ -2235,18 +2247,24 @@ consider_iface_release(const struct > ovsrec_interface *iface_rec, > > lbinding = local_binding_find(local_bindings, iface_id); > > > > if (lbinding) { > > - 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; > > + 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. > > + */ > > + 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; > > + } > > } > > } > > > > @@ -3058,6 +3076,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 > > > > Acked-by: Han Zhou <[email protected]>
Thanks. I applied both the patches to the main branch. I suppose this needs to be backported. I'll backport tomorrow. Numan > _______________________________________________ > 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
