| Message ID | 20221114092437.2807815-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Han Zhou |
| Headers | show |
| Series | [ovs-dev,v5] ovn-controller: Fixed missing flows after interface deletion | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Mon, Nov 14, 2022 at 10:24 AM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> > > --- > 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. > + */ > + 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Reviewed-by: Ales Musil <amusil@redhat.com>
On Mon, Nov 14, 2022 at 1:24 AM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> > > --- > 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 <hzhou@ovn.org> > + 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 <xsimonar@redhat.com> > > --- > 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. > 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? > + 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([])
On Thu, Nov 17, 2022 at 10:39 AM Ihar Hrachyshka <ihrachys@redhat.com> 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 <xsimonar@redhat.com> > > > > --- > > 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. > > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Han Thanks for your comment. Comments embedded below Thanks Xavier On Wed, Nov 16, 2022 at 9:04 AM Han Zhou <hzhou@ovn.org> wrote: > > > On Mon, Nov 14, 2022 at 1:24 AM Xavier Simonart <xsimonar@redhat.com> > 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 <xsimonar@redhat.com> > > > > --- > > 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 <hzhou@ovn.org> > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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 <hzhou@ovn.org> wrote: > On Thu, Nov 17, 2022 at 10:39 AM Ihar Hrachyshka <ihrachys@redhat.com> > 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 <xsimonar@redhat.com> > > > > > > --- > > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
Makes sense, thank you for clarification. Acked-by: Ihar Hrachyshka <ihar@redhat.com> 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 <xsimonar@redhat.com> 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 <hzhou@ovn.org> wrote: >> >> On Thu, Nov 17, 2022 at 10:39 AM Ihar Hrachyshka <ihrachys@redhat.com> >> 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 <xsimonar@redhat.com> >> > > >> > > --- >> > > 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 >> > dev@openvswitch.org >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On Fri, Nov 18, 2022 at 6:01 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > Hi Han > > Thanks for your comment. > Comments embedded below > > Thanks > Xavier > > On Wed, Nov 16, 2022 at 9:04 AM Han Zhou <hzhou@ovn.org> wrote: >> >> >> >> On Mon, Nov 14, 2022 at 1:24 AM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> >> > >> > --- >> > 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 <hzhou@ovn.org> >> > 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). Thanks for explaining. So it was actually releasing for the wrong VIF. This fix is necessary. It was more of a hidden problem and would be exposed when there are multiple bindings. If you don't mind, could you split this part as a separate patch (it should be the first one in the series)? (Or I could do this when merging if it is more convenient.) Thanks, Han > > + 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 >> > dev@openvswitch.org >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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. + */ + 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([])
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 <xsimonar@redhat.com> --- 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(+)