| Message ID | 20221024152144.1787203-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v4] ovn-controller: Fixed missing flows after interface deletion | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Mon, Oct 24, 2022 at 5:22 PM 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 > --- > controller/binding.c | 30 ++++++++ > controller/binding.h | 1 + > tests/ovn.at | 165 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 196 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index c3d2b2e42..b8613c54d 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,23 @@ 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) { > + 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 +3063,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 f8b8db4df..29b14ec2a 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -32975,3 +32975,168 @@ 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 ====================================================== > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew > type=internal > + check ovs-vsctl 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 ====================================================== > + 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 > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
Hi Xavier, On 10/24/22 17:21, 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 > --- > controller/binding.c | 30 ++++++++ > controller/binding.h | 1 + > tests/ovn.at | 165 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 196 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index c3d2b2e42..b8613c54d 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,23 @@ 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) { > + 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; > + } I'm not sure I understand why this is needed. I removed it and ran the unit tests and everything passed. Would it be possible to add a test that exercises this case? Thanks, Dumitru > + } > + } > + > 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 +3063,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 f8b8db4df..29b14ec2a 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -32975,3 +32975,168 @@ 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 ====================================================== > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal > + check ovs-vsctl 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 ====================================================== > + 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 Fri, Oct 28, 2022 at 11:36 AM Dumitru Ceara <dceara@redhat.com> wrote: > > Hi Xavier, > > On 10/24/22 17:21, 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 > > --- > > controller/binding.c | 30 ++++++++ > > controller/binding.h | 1 + > > tests/ovn.at | 165 +++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 196 insertions(+) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index c3d2b2e42..b8613c54d 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,23 @@ 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) { > > + 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; > > + } > > I'm not sure I understand why this is needed. I removed it and ran the > unit tests and everything passed. > > Would it be possible to add a test that exercises this case? Yeah. I'm not sure if the above code is required or not ? I did some testing with the patch and here are a few observations. Lets say there is ovs port "p1" associated with logical port "lp1". If I run the below command, ovs-vsctl add-port br-int p2 -- set Interface p2 external_ids:iface-id=lp1 then ovn-controller in the local binding sets the iface to that of "p2" and the table 0 openflow changes from "in_port=p1" to "in_port=p2". I suppose that's the behavior even with the present main branch. Then If I run the below command ovs-vsctl add-port br-int p3 -- set Interface p3 external_ids:iface-id=lp1, then the same thing happens i.e local binding iface now references p3 and so on. If I delete the ovs port p1, - ovs-vsctl del-port p1, ovn-controller changes the local binding iface to p2 even though it was referencing the port to p3 before this. This is kind of weird. I'm sure the same behavior is seen with the main branch too. I'd suggest changing the behavior of the ovn-controller a bit differently if a logical port is linked to multiple ovs ports. I'd suggest that ovn-controller should not claim a logical port at all if there are multiple ovs ports. As soon as it sees a 2nd ovs port, it should release the logical port. Thoughts ? Thanks Numan > > Thanks, > Dumitru > > > + } > > + } > > + > > 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 +3063,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 f8b8db4df..29b14ec2a 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -32975,3 +32975,168 @@ 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 ====================================================== > > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal > > + check ovs-vsctl 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 ====================================================== > > + 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 Numan, Dumitru Thanks for looking at this patch and for your feedback. On Tue, Nov 1, 2022 at 10:41 PM Numan Siddique <numans@ovn.org> wrote: > On Fri, Oct 28, 2022 at 11:36 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > Hi Xavier, > > > > On 10/24/22 17:21, 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 > > > --- > > > controller/binding.c | 30 ++++++++ > > > controller/binding.h | 1 + > > > tests/ovn.at | 165 > +++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 196 insertions(+) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index c3d2b2e42..b8613c54d 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,23 @@ 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) { > > > + 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; > > > + } > > > > I'm not sure I understand why this is needed. I removed it and ran the > > unit tests and everything passed. > > > > Would it be possible to add a test that exercises this case? > > Yeah. I'm not sure if the above code is required or not ? > > When OVS interface is created and bound to the logical port, OVN gets notified of two changes: - an "ofport" (!=0) being assigned to the interface - a new OVS interface with "external_ids"/"iface-id"being set It can happen that the ofport change is reported after the "external_ids"/"iface-id" is set. In that case, without those lines, this is seen as a release of the interface This (usually) happens for instance if running - check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal -- set interface lpnew external_ids:iface-id=lp instead of - check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal - check ovs-vsctl set interface lpnew external_ids:iface-id=lp I'll change the test case (both commands in one transaction) for one of the additional OVS interfaces to get a better code coverage. I did some testing with the patch and here are a few observations. > > Lets say there is ovs port "p1" associated with logical port "lp1". > > If I run the below command, > > ovs-vsctl add-port br-int p2 -- set Interface p2 > external_ids:iface-id=lp1 > > then ovn-controller in the local binding sets the iface to that of > "p2" and the table 0 openflow changes from "in_port=p1" to > "in_port=p2". > I suppose that's the behavior even with the present main branch. > > Then If I run the below command > ovs-vsctl add-port br-int p3 -- set Interface p3 > external_ids:iface-id=lp1, > then the same thing happens i.e local binding iface now references p3 and > so on. > > If I delete the ovs port p1, - ovs-vsctl del-port p1, ovn-controller > changes the local binding iface to p2 even though it was referencing > the port to p3 before this. > > This is kind of weird. I'm sure the same behavior is seen with the > main branch too. > > I'd suggest changing the behavior of the ovn-controller a bit > differently if a logical port is linked to multiple ovs ports. > I'd suggest that ovn-controller should not claim a logical port at all > if there are multiple ovs ports. As soon as it sees a 2nd ovs port, > it should release the logical port. > > Thoughts ? > > Thanks > Numan > > The (initial) goal of this patch was to ensure the correctness of the behavior of OVN when one (and only one) interface is bound (e.g. the correct flows are installed when two ovs interfaces are bound and one is released). Without this patch, when two ovs interfaces were bound and one was deleted (i.e. we are in a clean state, one ovs interface bound to one port), then the flows related to that port were not properly installed. With and without this patch, if there is a recompute and multiple ovs interfaces are bound to the same port, then the behavior is "undefined" (i.e. one of the two remaining ovs interfaces will win, based on its name and not on the order of binding) and a warning is logged. The behavior Numan describes is "expected": 1. when there is a new binding, it wins (it's claimed). 2. when one ovs interface is released, and there were multiple interfaces bound to the same logical port, there is a recompute. 2a. if there is only one remaining bound interface, it wins of course. 2b. if there are multiple remaining bound interfaces, the behavior is "undefined" (i.e. one of the remaining interfaces will win, based on its name) and a warning is logged. We can of course change this behavior to what's proposed by Numan i.e. do not claim a logical port if bound to multiple interfaces. This behavior (Numan's proposal) has the advantage that's it is always well defined, and we do not need to remember the order of the bindings. However, it might also be seen as weird as we end up (for this corner case) releasing ports when ovs interface is bound and claiming port when ovs interface is removed. e.g.: 1. Ovs interface p1 is bound to port p. port p is claimed. 2. Ovs interface p2 is bound to p as well. port p is released (as two ovs interfaces bound to the same port) 3. Ovs interface p1 is released. Port p is claimed and bound to ovs interface p2. I do not have a strong opinion about the best behavior (I saw this as a temporary corner of a corner case :-)) so I can change the behavior if you feel it's better. Thanks for your feedback. Xavier > > > > > Thanks, > > Dumitru > > > > > + } > > > + } > > > + > > > 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 +3063,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 f8b8db4df..29b14ec2a 100644 > > > --- a/tests/ovn.at > > > +++ b/tests/ovn.at > > > @@ -32975,3 +32975,168 @@ 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 ====================================================== > > > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew > type=internal > > > + check ovs-vsctl 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 ====================================================== > > > + 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 > > > >
On Wed, Nov 2, 2022 at 7:39 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > Hi Numan, Dumitru > > Thanks for looking at this patch and for your feedback. > > > On Tue, Nov 1, 2022 at 10:41 PM Numan Siddique <numans@ovn.org> wrote: > > > On Fri, Oct 28, 2022 at 11:36 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > > > Hi Xavier, > > > > > > On 10/24/22 17:21, 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 > > > > --- > > > > controller/binding.c | 30 ++++++++ > > > > controller/binding.h | 1 + > > > > tests/ovn.at | 165 > > +++++++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 196 insertions(+) > > > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > > index c3d2b2e42..b8613c54d 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,23 @@ 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) { > > > > + 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; > > > > + } > > > > > > I'm not sure I understand why this is needed. I removed it and ran the > > > unit tests and everything passed. > > > > > > Would it be possible to add a test that exercises this case? > > > > Yeah. I'm not sure if the above code is required or not ? > > > > When OVS interface is created and bound to the logical port, OVN gets > notified of two changes: > - an "ofport" (!=0) being assigned to the interface > - a new OVS interface with "external_ids"/"iface-id"being set > It can happen that the ofport change is reported after the > "external_ids"/"iface-id" is set. In that case, without those lines, this > is seen as a release of the interface > This (usually) happens for instance if running > - check ovs-vsctl add-port br-int lpnew -- set interface lpnew > type=internal -- set interface lpnew external_ids:iface-id=lp > instead of > - check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal > - check ovs-vsctl set interface lpnew external_ids:iface-id=lp > I'll change the test case (both commands in one transaction) for one of the > additional OVS interfaces to get a better code coverage. > > I did some testing with the patch and here are a few observations. > > > > Lets say there is ovs port "p1" associated with logical port "lp1". > > > > If I run the below command, > > > > ovs-vsctl add-port br-int p2 -- set Interface p2 > > external_ids:iface-id=lp1 > > > > then ovn-controller in the local binding sets the iface to that of > > "p2" and the table 0 openflow changes from "in_port=p1" to > > "in_port=p2". > > I suppose that's the behavior even with the present main branch. > > > > Then If I run the below command > > ovs-vsctl add-port br-int p3 -- set Interface p3 > > external_ids:iface-id=lp1, > > then the same thing happens i.e local binding iface now references p3 and > > so on. > > > > If I delete the ovs port p1, - ovs-vsctl del-port p1, ovn-controller > > changes the local binding iface to p2 even though it was referencing > > the port to p3 before this. > > > > This is kind of weird. I'm sure the same behavior is seen with the > > main branch too. > > > > I'd suggest changing the behavior of the ovn-controller a bit > > differently if a logical port is linked to multiple ovs ports. > > I'd suggest that ovn-controller should not claim a logical port at all > > if there are multiple ovs ports. As soon as it sees a 2nd ovs port, > > it should release the logical port. > > > > Thoughts ? > > > > Thanks > > Numan > > > > > The (initial) goal of this patch was to ensure the correctness of the > behavior of OVN when one (and only one) interface is bound (e.g. the > correct flows are installed when two ovs interfaces are bound and one is > released). > Without this patch, when two ovs interfaces were bound and one was deleted > (i.e. we are in a clean state, one ovs interface bound to one port), then > the flows related to that port were not properly installed. > With and without this patch, if there is a recompute and multiple ovs > interfaces are bound to the same port, then the behavior is "undefined" > (i.e. one of the two remaining ovs interfaces will win, based on its name > and not on the order of binding) and a warning is logged. > > The behavior Numan describes is "expected": > 1. when there is a new binding, it wins (it's claimed). > 2. when one ovs interface is released, and there were multiple interfaces > bound to the same logical port, there is a recompute. > 2a. if there is only one remaining bound interface, it wins of course. > 2b. if there are multiple remaining bound interfaces, the behavior is > "undefined" (i.e. one of the remaining interfaces will win, based on its > name) and a warning is logged. > > We can of course change this behavior to what's proposed by Numan i.e. do > not claim a logical port if bound to multiple interfaces. This behavior > (Numan's proposal) has the advantage that's it is always well defined, and > we do not need to remember the order of the bindings. > However, it might also be seen as weird as we end up (for this corner case) > releasing ports when ovs interface is bound and claiming port when ovs > interface is removed. e.g.: > 1. Ovs interface p1 is bound to port p. port p is claimed. > 2. Ovs interface p2 is bound to p as well. port p is released (as two ovs > interfaces bound to the same port) > 3. Ovs interface p1 is released. Port p is claimed and bound to ovs > interface p2. > I do not have a strong opinion about the best behavior (I saw this as a > temporary corner of a corner case :-)) so I can change the behavior if you > feel it's better. Thanks for the explanation. I think it's better to have a well defined behavior. Also the scenario you mentioned above should be fine. This would fix the issue this patch is trying to fix anyway. Thanks Numan > > Thanks for your feedback. > Xavier > > > > > > > > > Thanks, > > > Dumitru > > > > > > > + } > > > > + } > > > > + > > > > 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 +3063,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 f8b8db4df..29b14ec2a 100644 > > > > --- a/tests/ovn.at > > > > +++ b/tests/ovn.at > > > > @@ -32975,3 +32975,168 @@ 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 ====================================================== > > > > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew > > type=internal > > > > + check ovs-vsctl 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 ====================================================== > > > > + 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 11/2/22 15:21, Numan Siddique wrote: > On Wed, Nov 2, 2022 at 7:39 AM Xavier Simonart <xsimonar@redhat.com> wrote: >> >> Hi Numan, Dumitru >> >> Thanks for looking at this patch and for your feedback. >> >> >> On Tue, Nov 1, 2022 at 10:41 PM Numan Siddique <numans@ovn.org> wrote: >> >>> On Fri, Oct 28, 2022 at 11:36 AM Dumitru Ceara <dceara@redhat.com> wrote: >>>> >>>> Hi Xavier, >>>> >>>> On 10/24/22 17:21, 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 >>>>> --- >>>>> controller/binding.c | 30 ++++++++ >>>>> controller/binding.h | 1 + >>>>> tests/ovn.at | 165 >>> +++++++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 196 insertions(+) >>>>> >>>>> diff --git a/controller/binding.c b/controller/binding.c >>>>> index c3d2b2e42..b8613c54d 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,23 @@ 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) { >>>>> + 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; >>>>> + } >>>> >>>> I'm not sure I understand why this is needed. I removed it and ran the >>>> unit tests and everything passed. >>>> >>>> Would it be possible to add a test that exercises this case? >>> >>> Yeah. I'm not sure if the above code is required or not ? >>> >>> When OVS interface is created and bound to the logical port, OVN gets >> notified of two changes: >> - an "ofport" (!=0) being assigned to the interface >> - a new OVS interface with "external_ids"/"iface-id"being set >> It can happen that the ofport change is reported after the >> "external_ids"/"iface-id" is set. In that case, without those lines, this >> is seen as a release of the interface >> This (usually) happens for instance if running >> - check ovs-vsctl add-port br-int lpnew -- set interface lpnew >> type=internal -- set interface lpnew external_ids:iface-id=lp >> instead of >> - check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal >> - check ovs-vsctl set interface lpnew external_ids:iface-id=lp >> I'll change the test case (both commands in one transaction) for one of the >> additional OVS interfaces to get a better code coverage. >> >> I did some testing with the patch and here are a few observations. >>> >>> Lets say there is ovs port "p1" associated with logical port "lp1". >>> >>> If I run the below command, >>> >>> ovs-vsctl add-port br-int p2 -- set Interface p2 >>> external_ids:iface-id=lp1 >>> >>> then ovn-controller in the local binding sets the iface to that of >>> "p2" and the table 0 openflow changes from "in_port=p1" to >>> "in_port=p2". >>> I suppose that's the behavior even with the present main branch. >>> >>> Then If I run the below command >>> ovs-vsctl add-port br-int p3 -- set Interface p3 >>> external_ids:iface-id=lp1, >>> then the same thing happens i.e local binding iface now references p3 and >>> so on. >>> >>> If I delete the ovs port p1, - ovs-vsctl del-port p1, ovn-controller >>> changes the local binding iface to p2 even though it was referencing >>> the port to p3 before this. >>> >>> This is kind of weird. I'm sure the same behavior is seen with the >>> main branch too. >>> >>> I'd suggest changing the behavior of the ovn-controller a bit >>> differently if a logical port is linked to multiple ovs ports. >>> I'd suggest that ovn-controller should not claim a logical port at all >>> if there are multiple ovs ports. As soon as it sees a 2nd ovs port, >>> it should release the logical port. >>> >>> Thoughts ? >>> >>> Thanks >>> Numan >>> >>> >> The (initial) goal of this patch was to ensure the correctness of the >> behavior of OVN when one (and only one) interface is bound (e.g. the >> correct flows are installed when two ovs interfaces are bound and one is >> released). >> Without this patch, when two ovs interfaces were bound and one was deleted >> (i.e. we are in a clean state, one ovs interface bound to one port), then >> the flows related to that port were not properly installed. >> With and without this patch, if there is a recompute and multiple ovs >> interfaces are bound to the same port, then the behavior is "undefined" >> (i.e. one of the two remaining ovs interfaces will win, based on its name >> and not on the order of binding) and a warning is logged. >> >> The behavior Numan describes is "expected": >> 1. when there is a new binding, it wins (it's claimed). >> 2. when one ovs interface is released, and there were multiple interfaces >> bound to the same logical port, there is a recompute. >> 2a. if there is only one remaining bound interface, it wins of course. >> 2b. if there are multiple remaining bound interfaces, the behavior is >> "undefined" (i.e. one of the remaining interfaces will win, based on its >> name) and a warning is logged. >> >> We can of course change this behavior to what's proposed by Numan i.e. do >> not claim a logical port if bound to multiple interfaces. This behavior >> (Numan's proposal) has the advantage that's it is always well defined, and >> we do not need to remember the order of the bindings. >> However, it might also be seen as weird as we end up (for this corner case) >> releasing ports when ovs interface is bound and claiming port when ovs >> interface is removed. e.g.: >> 1. Ovs interface p1 is bound to port p. port p is claimed. >> 2. Ovs interface p2 is bound to p as well. port p is released (as two ovs >> interfaces bound to the same port) >> 3. Ovs interface p1 is released. Port p is claimed and bound to ovs >> interface p2. >> I do not have a strong opinion about the best behavior (I saw this as a >> temporary corner of a corner case :-)) so I can change the behavior if you >> feel it's better. > > Thanks for the explanation. > I think it's better to have a well defined behavior. Also the > scenario you mentioned above > should be fine. This would fix the issue this patch is trying to fix anyway. > It's undefined behavior because the CMS is doing something it shouldn't be doing. The implementation ("we") can choose to deal with this in any way. So if we want to release the binding as soon as we detect two OVS ports for a single OVN one that's fine. So, just to bring another aspect in consideration, I would go for the solution that is the most simple from the code perspective. Thanks, Dumitru > Thanks > Numan > >> >> Thanks for your feedback. >> Xavier >> >>> >>>> >>>> Thanks, >>>> Dumitru >>>> >>>>> + } >>>>> + } >>>>> + >>>>> 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 +3063,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 f8b8db4df..29b14ec2a 100644 >>>>> --- a/tests/ovn.at >>>>> +++ b/tests/ovn.at >>>>> @@ -32975,3 +32975,168 @@ 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 ====================================================== >>>>> + check ovs-vsctl add-port br-int lpnew -- set interface lpnew >>> type=internal >>>>> + check ovs-vsctl 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 ====================================================== >>>>> + 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 Wed, Nov 2, 2022 at 3:31 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 11/2/22 15:21, Numan Siddique wrote: > > On Wed, Nov 2, 2022 at 7:39 AM Xavier Simonart <xsimonar@redhat.com> > wrote: > >> > >> Hi Numan, Dumitru > >> > >> Thanks for looking at this patch and for your feedback. > >> > >> > >> On Tue, Nov 1, 2022 at 10:41 PM Numan Siddique <numans@ovn.org> wrote: > >> > >>> On Fri, Oct 28, 2022 at 11:36 AM Dumitru Ceara <dceara@redhat.com> > wrote: > >>>> > >>>> Hi Xavier, > >>>> > >>>> On 10/24/22 17:21, 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 > >>>>> --- > >>>>> controller/binding.c | 30 ++++++++ > >>>>> controller/binding.h | 1 + > >>>>> tests/ovn.at | 165 > >>> +++++++++++++++++++++++++++++++++++++++++++ > >>>>> 3 files changed, 196 insertions(+) > >>>>> > >>>>> diff --git a/controller/binding.c b/controller/binding.c > >>>>> index c3d2b2e42..b8613c54d 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,23 @@ 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) { > >>>>> + 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; > >>>>> + } > >>>> > >>>> I'm not sure I understand why this is needed. I removed it and ran > the > >>>> unit tests and everything passed. > >>>> > >>>> Would it be possible to add a test that exercises this case? > >>> > >>> Yeah. I'm not sure if the above code is required or not ? > >>> > >>> When OVS interface is created and bound to the logical port, OVN gets > >> notified of two changes: > >> - an "ofport" (!=0) being assigned to the interface > >> - a new OVS interface with "external_ids"/"iface-id"being set > >> It can happen that the ofport change is reported after the > >> "external_ids"/"iface-id" is set. In that case, without those lines, > this > >> is seen as a release of the interface > >> This (usually) happens for instance if running > >> - check ovs-vsctl add-port br-int lpnew -- set interface lpnew > >> type=internal -- set interface lpnew external_ids:iface-id=lp > >> instead of > >> - check ovs-vsctl add-port br-int lpnew -- set interface lpnew > type=internal > >> - check ovs-vsctl set interface lpnew external_ids:iface-id=lp > >> I'll change the test case (both commands in one transaction) for one of > the > >> additional OVS interfaces to get a better code coverage. > >> > >> I did some testing with the patch and here are a few observations. > >>> > >>> Lets say there is ovs port "p1" associated with logical port "lp1". > >>> > >>> If I run the below command, > >>> > >>> ovs-vsctl add-port br-int p2 -- set Interface p2 > >>> external_ids:iface-id=lp1 > >>> > >>> then ovn-controller in the local binding sets the iface to that of > >>> "p2" and the table 0 openflow changes from "in_port=p1" to > >>> "in_port=p2". > >>> I suppose that's the behavior even with the present main branch. > >>> > >>> Then If I run the below command > >>> ovs-vsctl add-port br-int p3 -- set Interface p3 > >>> external_ids:iface-id=lp1, > >>> then the same thing happens i.e local binding iface now references p3 > and > >>> so on. > >>> > >>> If I delete the ovs port p1, - ovs-vsctl del-port p1, ovn-controller > >>> changes the local binding iface to p2 even though it was referencing > >>> the port to p3 before this. > >>> > >>> This is kind of weird. I'm sure the same behavior is seen with the > >>> main branch too. > >>> > >>> I'd suggest changing the behavior of the ovn-controller a bit > >>> differently if a logical port is linked to multiple ovs ports. > >>> I'd suggest that ovn-controller should not claim a logical port at all > >>> if there are multiple ovs ports. As soon as it sees a 2nd ovs port, > >>> it should release the logical port. > >>> > >>> Thoughts ? > >>> > >>> Thanks > >>> Numan > >>> > >>> > >> The (initial) goal of this patch was to ensure the correctness of the > >> behavior of OVN when one (and only one) interface is bound (e.g. the > >> correct flows are installed when two ovs interfaces are bound and one is > >> released). > >> Without this patch, when two ovs interfaces were bound and one was > deleted > >> (i.e. we are in a clean state, one ovs interface bound to one port), > then > >> the flows related to that port were not properly installed. > >> With and without this patch, if there is a recompute and multiple ovs > >> interfaces are bound to the same port, then the behavior is "undefined" > >> (i.e. one of the two remaining ovs interfaces will win, based on its > name > >> and not on the order of binding) and a warning is logged. > >> > >> The behavior Numan describes is "expected": > >> 1. when there is a new binding, it wins (it's claimed). > >> 2. when one ovs interface is released, and there were multiple > interfaces > >> bound to the same logical port, there is a recompute. > >> 2a. if there is only one remaining bound interface, it wins of course. > >> 2b. if there are multiple remaining bound interfaces, the behavior is > >> "undefined" (i.e. one of the remaining interfaces will win, based on its > >> name) and a warning is logged. > >> > >> We can of course change this behavior to what's proposed by Numan i.e. > do > >> not claim a logical port if bound to multiple interfaces. This behavior > >> (Numan's proposal) has the advantage that's it is always well defined, > and > >> we do not need to remember the order of the bindings. > >> However, it might also be seen as weird as we end up (for this corner > case) > >> releasing ports when ovs interface is bound and claiming port when ovs > >> interface is removed. e.g.: > >> 1. Ovs interface p1 is bound to port p. port p is claimed. > >> 2. Ovs interface p2 is bound to p as well. port p is released (as two > ovs > >> interfaces bound to the same port) > >> 3. Ovs interface p1 is released. Port p is claimed and bound to ovs > >> interface p2. > >> I do not have a strong opinion about the best behavior (I saw this as a > >> temporary corner of a corner case :-)) so I can change the behavior if > you > >> feel it's better. > > > > Thanks for the explanation. > > I think it's better to have a well defined behavior. Also the > > scenario you mentioned above > > should be fine. This would fix the issue this patch is trying to fix > anyway. > > > > It's undefined behavior because the CMS is doing something it shouldn't > be doing. The implementation ("we") can choose to deal with this in any > way. So if we want to release the binding as soon as we detect two OVS > ports for a single OVN one that's fine. > > So, just to bring another aspect in consideration, I would go for the > solution that is the most simple from the code perspective. > > The "undefined" behavior means that it is not defined by the order of the insertion, but based on the names of the OVS interfaces. It happens when we release an OVS interface while multiple (>=3) OVS interfaces were bound to the same port (which triggers a recompute), or when we recompute while having multiple interfaces bound to the same port. So, when moving from 2 to 1 OVS interface (by releasing an OVS interface), the behavior is perfectly defined. I am not sure how realistic is the use case of having 3 OVS interfaces bound to the same port. I personally find the "fully defined" behavior (releasing the port when multiple bindings are done, as proposed by Numan) more difficult to debug, as adding an OVS interface might now result in releasing the port. The implementation would also be slightly more complex, as we would end up (when multiple OVS interfaces are bound) with local binding with lbinding->iface = NULL, which is already used for parents of container ports. Please let me know what you think. I'll anyhow send a v5, changing the test case to hit the "ofport == 0" discussed above, and adding related comments both in test case and code. Thanks Xavier > Thanks, > Dumitru > > > Thanks > > Numan > > > >> > >> Thanks for your feedback. > >> Xavier > >> > >>> > >>>> > >>>> Thanks, > >>>> Dumitru > >>>> > >>>>> + } > >>>>> + } > >>>>> + > >>>>> 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 +3063,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 f8b8db4df..29b14ec2a 100644 > >>>>> --- a/tests/ovn.at > >>>>> +++ b/tests/ovn.at > >>>>> @@ -32975,3 +32975,168 @@ 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 ====================================================== > >>>>> + check ovs-vsctl add-port br-int lpnew -- set interface lpnew > >>> type=internal > >>>>> + check ovs-vsctl 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 ====================================================== > >>>>> + 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 Thu, Nov 3, 2022 at 7:10 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > On Wed, Nov 2, 2022 at 3:31 PM Dumitru Ceara <dceara@redhat.com> wrote: > > > On 11/2/22 15:21, Numan Siddique wrote: > > > On Wed, Nov 2, 2022 at 7:39 AM Xavier Simonart <xsimonar@redhat.com> > > wrote: > > >> > > >> Hi Numan, Dumitru > > >> > > >> Thanks for looking at this patch and for your feedback. > > >> > > >> > > >> On Tue, Nov 1, 2022 at 10:41 PM Numan Siddique <numans@ovn.org> wrote: > > >> > > >>> On Fri, Oct 28, 2022 at 11:36 AM Dumitru Ceara <dceara@redhat.com> > > wrote: > > >>>> > > >>>> Hi Xavier, > > >>>> > > >>>> On 10/24/22 17:21, 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 > > >>>>> --- > > >>>>> controller/binding.c | 30 ++++++++ > > >>>>> controller/binding.h | 1 + > > >>>>> tests/ovn.at | 165 > > >>> +++++++++++++++++++++++++++++++++++++++++++ > > >>>>> 3 files changed, 196 insertions(+) > > >>>>> > > >>>>> diff --git a/controller/binding.c b/controller/binding.c > > >>>>> index c3d2b2e42..b8613c54d 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,23 @@ 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) { > > >>>>> + 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; > > >>>>> + } > > >>>> > > >>>> I'm not sure I understand why this is needed. I removed it and ran > > the > > >>>> unit tests and everything passed. > > >>>> > > >>>> Would it be possible to add a test that exercises this case? > > >>> > > >>> Yeah. I'm not sure if the above code is required or not ? > > >>> > > >>> When OVS interface is created and bound to the logical port, OVN gets > > >> notified of two changes: > > >> - an "ofport" (!=0) being assigned to the interface > > >> - a new OVS interface with "external_ids"/"iface-id"being set > > >> It can happen that the ofport change is reported after the > > >> "external_ids"/"iface-id" is set. In that case, without those lines, > > this > > >> is seen as a release of the interface > > >> This (usually) happens for instance if running > > >> - check ovs-vsctl add-port br-int lpnew -- set interface lpnew > > >> type=internal -- set interface lpnew external_ids:iface-id=lp > > >> instead of > > >> - check ovs-vsctl add-port br-int lpnew -- set interface lpnew > > type=internal > > >> - check ovs-vsctl set interface lpnew external_ids:iface-id=lp > > >> I'll change the test case (both commands in one transaction) for one of > > the > > >> additional OVS interfaces to get a better code coverage. > > >> > > >> I did some testing with the patch and here are a few observations. > > >>> > > >>> Lets say there is ovs port "p1" associated with logical port "lp1". > > >>> > > >>> If I run the below command, > > >>> > > >>> ovs-vsctl add-port br-int p2 -- set Interface p2 > > >>> external_ids:iface-id=lp1 > > >>> > > >>> then ovn-controller in the local binding sets the iface to that of > > >>> "p2" and the table 0 openflow changes from "in_port=p1" to > > >>> "in_port=p2". > > >>> I suppose that's the behavior even with the present main branch. > > >>> > > >>> Then If I run the below command > > >>> ovs-vsctl add-port br-int p3 -- set Interface p3 > > >>> external_ids:iface-id=lp1, > > >>> then the same thing happens i.e local binding iface now references p3 > > and > > >>> so on. > > >>> > > >>> If I delete the ovs port p1, - ovs-vsctl del-port p1, ovn-controller > > >>> changes the local binding iface to p2 even though it was referencing > > >>> the port to p3 before this. > > >>> > > >>> This is kind of weird. I'm sure the same behavior is seen with the > > >>> main branch too. > > >>> > > >>> I'd suggest changing the behavior of the ovn-controller a bit > > >>> differently if a logical port is linked to multiple ovs ports. > > >>> I'd suggest that ovn-controller should not claim a logical port at all > > >>> if there are multiple ovs ports. As soon as it sees a 2nd ovs port, > > >>> it should release the logical port. > > >>> > > >>> Thoughts ? > > >>> > > >>> Thanks > > >>> Numan > > >>> > > >>> > > >> The (initial) goal of this patch was to ensure the correctness of the > > >> behavior of OVN when one (and only one) interface is bound (e.g. the > > >> correct flows are installed when two ovs interfaces are bound and one is > > >> released). > > >> Without this patch, when two ovs interfaces were bound and one was > > deleted > > >> (i.e. we are in a clean state, one ovs interface bound to one port), > > then > > >> the flows related to that port were not properly installed. > > >> With and without this patch, if there is a recompute and multiple ovs > > >> interfaces are bound to the same port, then the behavior is "undefined" > > >> (i.e. one of the two remaining ovs interfaces will win, based on its > > name > > >> and not on the order of binding) and a warning is logged. > > >> > > >> The behavior Numan describes is "expected": > > >> 1. when there is a new binding, it wins (it's claimed). > > >> 2. when one ovs interface is released, and there were multiple > > interfaces > > >> bound to the same logical port, there is a recompute. > > >> 2a. if there is only one remaining bound interface, it wins of course. > > >> 2b. if there are multiple remaining bound interfaces, the behavior is > > >> "undefined" (i.e. one of the remaining interfaces will win, based on its > > >> name) and a warning is logged. > > >> > > >> We can of course change this behavior to what's proposed by Numan i.e. > > do > > >> not claim a logical port if bound to multiple interfaces. This behavior > > >> (Numan's proposal) has the advantage that's it is always well defined, > > and > > >> we do not need to remember the order of the bindings. > > >> However, it might also be seen as weird as we end up (for this corner > > case) > > >> releasing ports when ovs interface is bound and claiming port when ovs > > >> interface is removed. e.g.: > > >> 1. Ovs interface p1 is bound to port p. port p is claimed. > > >> 2. Ovs interface p2 is bound to p as well. port p is released (as two > > ovs > > >> interfaces bound to the same port) > > >> 3. Ovs interface p1 is released. Port p is claimed and bound to ovs > > >> interface p2. > > >> I do not have a strong opinion about the best behavior (I saw this as a > > >> temporary corner of a corner case :-)) so I can change the behavior if > > you > > >> feel it's better. > > > > > > Thanks for the explanation. > > > I think it's better to have a well defined behavior. Also the > > > scenario you mentioned above > > > should be fine. This would fix the issue this patch is trying to fix > > anyway. > > > > > > > It's undefined behavior because the CMS is doing something it shouldn't > > be doing. The implementation ("we") can choose to deal with this in any > > way. So if we want to release the binding as soon as we detect two OVS > > ports for a single OVN one that's fine. > > > > So, just to bring another aspect in consideration, I would go for the > > solution that is the most simple from the code perspective. > > > > The "undefined" behavior means that it is not defined by the order of the > insertion, but based on the names of the OVS interfaces. > It happens when we release an OVS interface while multiple (>=3) OVS > interfaces were bound to the same port (which triggers a recompute), or > when we recompute while having multiple interfaces bound to the same port. > So, when moving from 2 to 1 OVS interface (by releasing an OVS interface), > the behavior is perfectly defined. > I am not sure how realistic is the use case of having 3 OVS interfaces > bound to the same port. > I personally find the "fully defined" behavior (releasing the port when > multiple bindings are done, as proposed by Numan) more difficult to debug, > as adding an OVS interface might now result in releasing the port. > The implementation would also be slightly more complex, as we would end up > (when multiple OVS interfaces are bound) with local binding with > lbinding->iface = NULL, which is already used for parents of container > ports. > Please let me know what you think. If it's complicating the code or hard to debug, I'm fine with the proposed approach here. > I'll anyhow send a v5, changing the test case to hit the "ofport == 0" > discussed above, and adding related comments both in test case and code. Sounds good. Numan > Thanks > Xavier > > > Thanks, > > Dumitru > > > > > Thanks > > > Numan > > > > > >> > > >> Thanks for your feedback. > > >> Xavier > > >> > > >>> > > >>>> > > >>>> Thanks, > > >>>> Dumitru > > >>>> > > >>>>> + } > > >>>>> + } > > >>>>> + > > >>>>> 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 +3063,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 f8b8db4df..29b14ec2a 100644 > > >>>>> --- a/tests/ovn.at > > >>>>> +++ b/tests/ovn.at > > >>>>> @@ -32975,3 +32975,168 @@ 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 ====================================================== > > >>>>> + check ovs-vsctl add-port br-int lpnew -- set interface lpnew > > >>> type=internal > > >>>>> + check ovs-vsctl 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 ====================================================== > > >>>>> + 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 > > >> > > > > > > > > _______________________________________________ > 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..b8613c54d 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,23 @@ 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) { + 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 +3063,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 f8b8db4df..29b14ec2a 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -32975,3 +32975,168 @@ 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 ====================================================== + check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal + check ovs-vsctl 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 ====================================================== + 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 --- controller/binding.c | 30 ++++++++ controller/binding.h | 1 + tests/ovn.at | 165 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 196 insertions(+)