On Fri, Nov 18, 2022 at 6:01 AM Xavier Simonart <[email protected]> wrote:
>
> Hi Han
>
> Thanks for your comment.
> Comments embedded below
>
> Thanks
> Xavier
>
> On Wed, Nov 16, 2022 at 9:04 AM Han Zhou <[email protected]> wrote:
>>
>>
>>
>> On Mon, Nov 14, 2022 at 1:24 AM Xavier Simonart <[email protected]>
wrote:
>> >
>> > In the following scenario:
>> > - interface "old" is created and external_ids:iface-id is set (to lp)
>> > - interface "new" is created and external_ids:iface-id is set (to same
lp)
>> > - interface "old" is deleted
>> > flows related to lp were deleted.
>> >
>> > Note that after "new" interface is created, flows use "new" ofport.
>> > The state where old and new interfaces have both external_ids:iface-id
set at
>> > the same time is "invalid", and all flows are not installed for lpold.
>> >
>> > Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for
pflow_output engine.")
>> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866
>> >
>> > Signed-off-by: Xavier Simonart <[email protected]>
>> >
>> > ---
>> > v2: - Use bool instead of int for binding count to better reflect only
>> > one additional binding is supported.
>> > - Fix use after free.
>> > - Remove debug logging from test case
>> > v3: - Based on Dumitru's and Mark's feedback:
>> > - Support any number of interfaces bound to the same port
>> > - Use recomputes to make code simpler (this is corner case)
>> > - Added test case using three interfaces bound to te same port
>> > v4: - Updated based on Ales' feedback
>> > - Also support scenario for port-types different than localport
>> > - Added test case for VIF port
>> > - Rebased on latest main
>> > v5: - Updated test case based on Numan/Dumitru's feedback (hit ofport
= 0)
>> > and added comments in code.
>> > - Rebased on latest main.
>> > ---
>> > controller/binding.c | 36 ++++++++++
>> > controller/binding.h | 1 +
>> > tests/ovn.at | 168 +++++++++++++++++++++++++++++++++++++++++++
>> > 3 files changed, 205 insertions(+)
>> >
>> > diff --git a/controller/binding.c b/controller/binding.c
>> > index c3d2b2e42..5f04b9a74 100644
>> > --- a/controller/binding.c
>> > +++ b/controller/binding.c
>> > @@ -1866,6 +1866,7 @@ build_local_bindings(struct binding_ctx_in
*b_ctx_in,
>> > lbinding = local_binding_create(iface_id,
iface_rec);
>> > local_binding_add(local_bindings, lbinding);
>> > } else {
>> > + lbinding->multiple_bindings = true;
>> > static struct vlog_rate_limit rl =
>> > VLOG_RATE_LIMIT_INIT(1, 5);
>> > VLOG_WARN_RL(
>> > @@ -2156,6 +2157,10 @@ consider_iface_claim(const struct
ovsrec_interface *iface_rec,
>> > lbinding = local_binding_create(iface_id, iface_rec);
>> > local_binding_add(local_bindings, lbinding);
>> > } else {
>> > + if (lbinding->iface && lbinding->iface != iface_rec) {
>> > + lbinding->multiple_bindings = true;
>> > + b_ctx_out->local_lports_changed = true;
>> > + }
>> > lbinding->iface = iface_rec;
>> > }
>> >
>> > @@ -2174,6 +2179,13 @@ consider_iface_claim(const struct
ovsrec_interface *iface_rec,
>> > return true;
>> > }
>> >
>> > + /* If multiple bindings to the same port, remove the "old"
binding.
>> > + * This ensures that change tracking is correct.
>> > + */
>> > + if (lbinding->multiple_bindings) {
>> > + remove_related_lport(pb, b_ctx_out);
>> > + }
>> > +
>> > enum en_lport_type lport_type = get_lport_type(pb);
>> > if (lport_type == LP_LOCALPORT) {
>> > return consider_localport(pb, b_ctx_in, b_ctx_out);
>> > @@ -2226,6 +2238,29 @@ consider_iface_release(const struct
ovsrec_interface *iface_rec,
>> > struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
>> >
>> > lbinding = local_binding_find(local_bindings, iface_id);
>> > +
>> > + if (lbinding) {
>> > + if (lbinding->multiple_bindings) {
>> > + VLOG_INFO("Multiple bindings for %s: force recompute to
clean up",
>> > + iface_id);
>> > + return false;
>> > + } else {
>> > + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport
: 0;
>> > + if (lbinding->iface != iface_rec && !ofport) {
>> > + /* If external_ids:iface-id is set within the same
transaction
>> > + * as adding an interface to a bridge, ovn-controller
is
>> > + * usually initially notified of ovs interface
changes with
>> > + * ofport == 0. If the lport was bound to a different
interface
>> > + * we do not want to release it.
>> > + */
>>
>> Hi Xavier, I think this logic in the "else" block, if really needed,
belongs to a separate patch.
>> It looks like an optimization to the scenario when moving a logical port
binding from one VIF to another on the same node. I guess this doesn't
happen very often in the real world. If it happens in some corner cases, it
shouldn't be a problem of letting it release and reclaim, right? So I
wonder if this change is really necessary. Assume it is correct, it still
adds some burden when reading/debugging the code. What do you think?
>>
>> For the rest of the patch:
>> Acked-by: Han Zhou <[email protected]>
>>
> This is obviously some debatable part of the code as Numan and Dumitru
already had questions around it (hence the additional comments in the code).
> To support the scenario (which I agree should not be very frequent), we
can't simply remove this part w/o other changes.
> When adding and binding the second interface, if we release the interface
when ofport = 0, then we lost the information that the port was bound when
we receive the ofport for the second i/f and claim it (we set
lbinding->iface to NULL in consider_iface_release and consider_iface_claim
needs another logic to detect the double binding).
Thanks for explaining. So it was actually releasing for the wrong VIF. This
fix is necessary. It was more of a hidden problem and would be exposed when
there are multiple bindings. If you don't mind, could you split this part
as a separate patch (it should be the first one in the series)? (Or I could
do this when merging if it is more convenient.)
Thanks,
Han
> > + VLOG_DBG("Not releasing lport %s as %s was claimed "
>>
>> > + "and %s was never bound)", iface_id,
lbinding->iface ?
>> > + lbinding->iface->name : "", iface_rec->name);
>> > + return true;
>> > + }
>> > + }
>> > + }
>> > +
>> > struct binding_lport *b_lport =
>> > local_binding_get_primary_or_localport_lport(lbinding);
>> > if (is_binding_lport_this_chassis(b_lport,
b_ctx_in->chassis_rec)) {
>> > @@ -3034,6 +3069,7 @@ local_binding_create(const char *name, const
struct ovsrec_interface *iface)
>> > struct local_binding *lbinding = xzalloc(sizeof *lbinding);
>> > lbinding->name = xstrdup(name);
>> > lbinding->iface = iface;
>> > + lbinding->multiple_bindings = false;
>> > ovs_list_init(&lbinding->binding_lports);
>> >
>> > return lbinding;
>> > diff --git a/controller/binding.h b/controller/binding.h
>> > index ad959a9e6..6c3a98b02 100644
>> > --- a/controller/binding.h
>> > +++ b/controller/binding.h
>> > @@ -135,6 +135,7 @@ struct local_binding {
>> > char *name;
>> > const struct ovsrec_interface *iface;
>> > struct ovs_list binding_lports;
>> > + bool multiple_bindings;
>> > };
>> >
>> > struct local_binding_data {
>> > diff --git a/tests/ovn.at b/tests/ovn.at
>> > index 6552681bd..7a918c639 100644
>> > --- a/tests/ovn.at
>> > +++ b/tests/ovn.at
>> > @@ -33053,3 +33053,171 @@ check ovn-nbctl --wait=hv sync
>> > OVN_CLEANUP([hv1])
>> > AT_CLEANUP
>> > ])
>> > +
>> > +m4_define([MULTIPLE_OVS_INT],
>> > + [OVN_FOR_EACH_NORTHD([
>> > + AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same
logical port ($1)])
>> > + ovn_start
>> > + net_add n1
>> > +
>> > + sim_add hv1
>> > + as hv1
>> > + ovs-vsctl add-br br-phys
>> > + ovn_attach n1 br-phys 192.168.0.1
>> > +
>> > + get_flows()
>> > + {
>> > + cookie=${1}
>> > + ovs-ofctl dump-flows br-int | grep $cookie |
>> > + sed -e 's/duration=[[0-9.]]*s, //g' |
>> > + sed -e 's/idle_age=[[0-9]]*, //g' |
>> > + sed -e 's/n_packets=[[0-9]]*, //g' |
>> > + sed -e 's/n_bytes=[[0-9]]*, //g'
>> > + }
>> > +
>> > + check ovn-nbctl ls-add ls
>> > + check ovn-nbctl lsp-add ls lp
>> > + if test X$1 != X; then
>> > + check ovn-nbctl lsp-set-type lp $1
>> > + fi
>> > + check ovn-nbctl lsp-set-addresses lp "00:00:00:01:01:02
192.168.1.2"
>> > +
>> > + check ovn-nbctl lsp-add ls vm1
>> > + check ovn-nbctl lsp-set-addresses vm1 "00:00:00:01:01:11
192.168.1.11"
>> > + check ovs-vsctl add-port br-int vm1 -- set interface vm1
type=internal external_ids:iface-id=vm1
>> > +
>> > + check ovn-nbctl --wait=hv sync
>> > +
>> > + check ovs-vsctl add-port br-int lpold -- set interface lpold
type=internal
>> > + check ovs-vsctl set interface lpold external_ids:iface-id=lp
>> > +
>> > + OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns _uuid find
port_binding logical_port=lp) != x])
>> > + echo ======================================================
>> > + echo === Flows after iface-id set for the old interface ===
>> > + echo ======================================================
>> > + COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut
-d: -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//')
>> > +
>> > + OVS_WAIT_UNTIL([
>> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpold)
>> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
>> > + ])
>> > + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l`
>> > + echo $nb_flows "flows after iface-id set for old interface"
>> > +
>> > + echo ======================================================
>> > + echo === Flows after iface-id set for the new interface ===
>> > + echo ======================================================
>> > + # Set external_ids:iface-id within same transaction as adding the
port.
>> > + # This will generally cause ovn-controller to get initially
notified of ovs interface changes with ofport == 0.
>> > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew
type=internal -- set interface lpnew external_ids:iface-id=lp
>> > + OVS_WAIT_UNTIL([
>> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
>> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
>> > + ])
>> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
>> > + flows_lpnew=$(get_flows $COOKIE)
>> > +
>> > + echo ======================================================
>> > + echo ======= Flows after old interface is deleted =========
>> > + echo ======================================================
>> > + check ovs-vsctl del-port br-int lpold
>> > + # We do not expect changes, so let's wait for controller to get
time to process any update
>> > + check ovn-nbctl --wait=hv sync
>> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
>> > + flows_after_deletion=$(get_flows $COOKIE)
>> > + check test "$flows_lpnew" = "$flows_after_deletion"
>> > +
>> > + echo ======================================================
>> > + echo ======= Flows after lptemp interface is created ====
>> > + echo ======================================================
>> > + # Set external_ids:iface-id in a different transaction as adding
the port.
>> > + # This will generally cause ovn-controller to get notified of ovs
interface changes with a proper ofport.
>> > + check ovs-vsctl add-port br-int lptemp -- set Interface lptemp
type=internal
>> > + check ovs-vsctl set Interface lptemp external_ids:iface-id=lp
>> > + OVS_WAIT_UNTIL([
>> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lptemp)
>> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
>> > + ])
>> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
>> > +
>> > + echo ======================================================
>> > + echo ======= Flows after lptemp interface is deleted ======
>> > + echo ======================================================
>> > + check ovs-vsctl del-port br-int lptemp
>> > + OVS_WAIT_UNTIL([
>> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
>> > + echo $ofport
>> > + ovs-ofctl dump-flows br-int | grep $COOKIE
>> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
>> > + ])
>> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
>> > + flows_after_deletion=$(get_flows $COOKIE)
>> > + check test "$flows_lpnew" = "$flows_after_deletion"
>> > +
>> > + echo ======================================================
>> > + echo ======= Flows after new interface is deleted =========
>> > + echo ======================================================
>> > + check ovs-vsctl del-port br-int lpnew
>> > + OVS_WAIT_UNTIL([
>> > + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l`
>> > + test "${nb_flows}" = 0
>> > + ])
>> > +
>> > + echo ======================================================
>> > + echo ======= Three interfaces bound to the same port ======
>> > + echo ======================================================
>> > + check ovs-vsctl add-port br-int lpold -- set interface lpold
type=internal
>> > + check ovs-vsctl set interface lpold external_ids:iface-id=lp
>> > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew
type=internal
>> > + check ovs-vsctl set interface lpnew external_ids:iface-id=lp
>> > +
>> > + # Wait for lpnew flows to be installed
>> > + OVS_WAIT_UNTIL([
>> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
>> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
>> > + ])
>> > + flows_lpnew=$(get_flows $COOKIE)
>> > + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l`
>> > +
>> > + check ovs-vsctl add-port br-int lptemp -- set Interface lptemp
type=internal
>> > + check ovs-vsctl set Interface lptemp external_ids:iface-id=lp
>> > +
>> > + # Wait for lptemp flows to be installed
>> > + OVS_WAIT_UNTIL([
>> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lptemp)
>> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
>> > + ])
>> > +
>> > + # Delete both lpold and lptemp to go to a stable situation
>> > + check ovs-vsctl del-port br-int lptemp
>> > + check ovs-vsctl del-port br-int lpold
>> > +
>> > + OVS_WAIT_UNTIL([
>> > + test 0 = $(ovs-vsctl show | grep "Port lpold" | wc -l)
>> > + ])
>> > +
>> > + # Wait for correct/lpnew flows to be installed
>> > + OVS_WAIT_UNTIL([
>> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
>> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
>> > + ])
>> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
>> > + flows_after_deletion=$(get_flows $COOKIE)
>> > + check test "$flows_lpnew" = "$flows_after_deletion"
>> > +
>> > + # Check that recompute still works
>> > + check ovn-appctl -t ovn-controller recompute
>> > + OVS_WAIT_UNTIL([
>> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
>> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
>> > + ])
>> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
>> > + flows_after_deletion=$(get_flows $COOKIE)
>> > + check test "$flows_lpnew" = "$flows_after_deletion"
>> > +
>> > + OVN_CLEANUP([hv1])
>> > + AT_CLEANUP
>> > + ])])
>> > +
>> > +MULTIPLE_OVS_INT([localport])
>> > +MULTIPLE_OVS_INT([])
>> > --
>> > 2.31.1
>> >
>> > _______________________________________________
>> > dev mailing list
>> > [email protected]
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
