On Wed, Nov 2, 2022 at 7:39 AM Xavier Simonart <[email protected]> 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 <[email protected]> wrote:
>
> > On Fri, Oct 28, 2022 at 11:36 AM Dumitru Ceara <[email protected]> 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 <[email protected]>
> > > >
> > > > ---
> > > > v2: - Use bool instead of int for binding count to better reflect only
> > > > one additional binding is supported.
> > > > - Fix use after free.
> > > > - Remove debug logging from test case
> > > > v3: - Based on Dumitru's and Mark's feedback:
> > > > - Support any number of interfaces bound to the same port
> > > > - Use recomputes to make code simpler (this is corner case)
> > > > - Added test case using three interfaces bound to te same port
> > > > v4: - Updated based on Ales' feedback
> > > > - Also support scenario for port-types different than localport
> > > > - Added test case for VIF port
> > > > - Rebased on latest main
> > > > ---
> > > > 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
> > > [email protected]
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
> >
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
