On Thu, Nov 17, 2022 at 10:39 AM Ihar Hrachyshka <[email protected]>
wrote:
>
> On 11/14/22 4:24 AM, Xavier Simonart wrote:
> > In the following scenario:
> > - interface "old" is created and external_ids:iface-id is set (to lp)
> > - interface "new" is created and external_ids:iface-id is set (to same
lp)
> > - interface "old" is deleted
> > flows related to lp were deleted.
> >
> > Note that after "new" interface is created, flows use "new" ofport.
> > The state where old and new interfaces have both external_ids:iface-id
set at
> > the same time is "invalid", and all flows are not installed for lpold.
> >
> > Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for pflow_output
engine.")
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866
> >
> > Signed-off-by: Xavier Simonart <[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,
>
> The description of the function says:
>
> "If the local_binding for this 'iface_rec' already exists and its
> already claimed, then this function will be no-op."
>
> which I don't think is true anymore with the patch applied. Consider
> adjusting the comment above.
>
> > lbinding = local_binding_create(iface_id, iface_rec);
> > local_binding_add(local_bindings, lbinding);
> > } else {
> > + if (lbinding->iface && lbinding->iface != iface_rec) {
> > + lbinding->multiple_bindings = true;
> > + b_ctx_out->local_lports_changed = true;
> > + }
> > lbinding->iface = iface_rec;
> > }
> >
> > @@ -2174,6 +2179,13 @@ consider_iface_claim(const struct
ovsrec_interface *iface_rec,
> > return true;
> > }
> >
> > + /* If multiple bindings to the same port, remove the "old" binding.
> > + * This ensures that change tracking is correct.
> > + */
> > + if (lbinding->multiple_bindings) {
> > + remove_related_lport(pb, b_ctx_out);
> > + }
> > +
> > enum en_lport_type lport_type = get_lport_type(pb);
> > if (lport_type == LP_LOCALPORT) {
> > return consider_localport(pb, b_ctx_in, b_ctx_out);
> > @@ -2226,6 +2238,29 @@ consider_iface_release(const struct
ovsrec_interface *iface_rec,
> > struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
> >
> > lbinding = local_binding_find(local_bindings, iface_id);
> > +
> > + if (lbinding) {
> > + if (lbinding->multiple_bindings) {
> > + VLOG_INFO("Multiple bindings for %s: force recompute to
clean up",
> > + iface_id);
> I wonder if it would be possible to handle this situation gracefully
> here, by looking through the list of interface records assigned to the
> same port, and if the list length is 1, then reinstate flows, otherwise
> do nothing. Thoughts?
Thanks Ihar. Here are my 2 cents:
It is possible, but it will also be more complex without bringing much
value. I-P is complex enough already. I think we'd better keep I-P for the
common scenarios where performance matters, and let such corner cases
caused by misconfiguration fall-back to recompute.
Han
> > + return false;
> > + } else {
> > + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport
: 0;
> > + if (lbinding->iface != iface_rec && !ofport) {
> > + /* If external_ids:iface-id is set within the same
transaction
> > + * as adding an interface to a bridge, ovn-controller
is
> > + * usually initially notified of ovs interface changes
with
> > + * ofport == 0. If the lport was bound to a different
interface
> > + * we do not want to release it.
> > + */
> > + VLOG_DBG("Not releasing lport %s as %s was claimed "
> > + "and %s was never bound)", iface_id,
lbinding->iface ?
> > + lbinding->iface->name : "", iface_rec->name);
> > + return true;
> > + }
> > + }
> > + }
> > +
> > struct binding_lport *b_lport =
> > local_binding_get_primary_or_localport_lport(lbinding);
> > if (is_binding_lport_this_chassis(b_lport,
b_ctx_in->chassis_rec)) {
> > @@ -3034,6 +3069,7 @@ local_binding_create(const char *name, const
struct ovsrec_interface *iface)
> > struct local_binding *lbinding = xzalloc(sizeof *lbinding);
> > lbinding->name = xstrdup(name);
> > lbinding->iface = iface;
> > + lbinding->multiple_bindings = false;
> > ovs_list_init(&lbinding->binding_lports);
> >
> > return lbinding;
> > diff --git a/controller/binding.h b/controller/binding.h
> > index ad959a9e6..6c3a98b02 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -135,6 +135,7 @@ struct local_binding {
> > char *name;
> > const struct ovsrec_interface *iface;
> > struct ovs_list binding_lports;
> > + bool multiple_bindings;
> > };
> >
> > struct local_binding_data {
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 6552681bd..7a918c639 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -33053,3 +33053,171 @@ check ovn-nbctl --wait=hv sync
> > OVN_CLEANUP([hv1])
> > AT_CLEANUP
> > ])
> > +
> > +m4_define([MULTIPLE_OVS_INT],
> > + [OVN_FOR_EACH_NORTHD([
> > + AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same
logical port ($1)])
> > + ovn_start
> > + net_add n1
> > +
> > + sim_add hv1
> > + as hv1
> > + ovs-vsctl add-br br-phys
> > + ovn_attach n1 br-phys 192.168.0.1
> > +
> > + get_flows()
> > + {
> > + cookie=${1}
> > + ovs-ofctl dump-flows br-int | grep $cookie |
> > + sed -e 's/duration=[[0-9.]]*s, //g' |
> > + sed -e 's/idle_age=[[0-9]]*, //g' |
> > + sed -e 's/n_packets=[[0-9]]*, //g' |
> > + sed -e 's/n_bytes=[[0-9]]*, //g'
> > + }
> > +
> > + check ovn-nbctl ls-add ls
> > + check ovn-nbctl lsp-add ls lp
> > + if test X$1 != X; then
> > + check ovn-nbctl lsp-set-type lp $1
> > + fi
> > + check ovn-nbctl lsp-set-addresses lp "00:00:00:01:01:02 192.168.1.2"
> > +
> > + check ovn-nbctl lsp-add ls vm1
> > + check ovn-nbctl lsp-set-addresses vm1 "00:00:00:01:01:11
192.168.1.11"
> > + check ovs-vsctl add-port br-int vm1 -- set interface vm1
type=internal external_ids:iface-id=vm1
> > +
> > + check ovn-nbctl --wait=hv sync
> > +
> > + check ovs-vsctl add-port br-int lpold -- set interface lpold
type=internal
> > + check ovs-vsctl set interface lpold external_ids:iface-id=lp
> > +
> > + OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns _uuid find
port_binding logical_port=lp) != x])
> > + echo ======================================================
> > + echo === Flows after iface-id set for the old interface ===
> > + echo ======================================================
> > + COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut
-d: -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//')
> > +
> > + OVS_WAIT_UNTIL([
> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpold)
> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
> > + ])
> > + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l`
> > + echo $nb_flows "flows after iface-id set for old interface"
> > +
> > + echo ======================================================
> > + echo === Flows after iface-id set for the new interface ===
> > + echo ======================================================
> > + # Set external_ids:iface-id within same transaction as adding the
port.
> > + # This will generally cause ovn-controller to get initially
notified of ovs interface changes with ofport == 0.
> > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew
type=internal -- set interface lpnew external_ids:iface-id=lp
> > + OVS_WAIT_UNTIL([
> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
> > + ])
> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
> > + flows_lpnew=$(get_flows $COOKIE)
> > +
> > + echo ======================================================
> > + echo ======= Flows after old interface is deleted =========
> > + echo ======================================================
> > + check ovs-vsctl del-port br-int lpold
> > + # We do not expect changes, so let's wait for controller to get
time to process any update
> > + check ovn-nbctl --wait=hv sync
> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
> > + flows_after_deletion=$(get_flows $COOKIE)
> > + check test "$flows_lpnew" = "$flows_after_deletion"
> > +
> > + echo ======================================================
> > + echo ======= Flows after lptemp interface is created ====
> > + echo ======================================================
> > + # Set external_ids:iface-id in a different transaction as adding
the port.
> > + # This will generally cause ovn-controller to get notified of ovs
interface changes with a proper ofport.
> > + check ovs-vsctl add-port br-int lptemp -- set Interface lptemp
type=internal
> > + check ovs-vsctl set Interface lptemp external_ids:iface-id=lp
> > + OVS_WAIT_UNTIL([
> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lptemp)
> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
> > + ])
> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
> > +
> > + echo ======================================================
> > + echo ======= Flows after lptemp interface is deleted ======
> > + echo ======================================================
> > + check ovs-vsctl del-port br-int lptemp
> > + OVS_WAIT_UNTIL([
> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
> > + echo $ofport
> > + ovs-ofctl dump-flows br-int | grep $COOKIE
> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
> > + ])
> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
> > + flows_after_deletion=$(get_flows $COOKIE)
> > + check test "$flows_lpnew" = "$flows_after_deletion"
> > +
> > + echo ======================================================
> > + echo ======= Flows after new interface is deleted =========
> > + echo ======================================================
> > + check ovs-vsctl del-port br-int lpnew
> > + OVS_WAIT_UNTIL([
> > + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l`
> > + test "${nb_flows}" = 0
> > + ])
> > +
> > + echo ======================================================
> > + echo ======= Three interfaces bound to the same port ======
> > + echo ======================================================
> > + check ovs-vsctl add-port br-int lpold -- set interface lpold
type=internal
> > + check ovs-vsctl set interface lpold external_ids:iface-id=lp
> > + check ovs-vsctl add-port br-int lpnew -- set interface lpnew
type=internal
> > + check ovs-vsctl set interface lpnew external_ids:iface-id=lp
> > +
> > + # Wait for lpnew flows to be installed
> > + OVS_WAIT_UNTIL([
> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
> > + ])
> > + flows_lpnew=$(get_flows $COOKIE)
> > + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l`
> > +
> > + check ovs-vsctl add-port br-int lptemp -- set Interface lptemp
type=internal
> > + check ovs-vsctl set Interface lptemp external_ids:iface-id=lp
> > +
> > + # Wait for lptemp flows to be installed
> > + OVS_WAIT_UNTIL([
> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lptemp)
> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
> > + ])
> > +
> > + # Delete both lpold and lptemp to go to a stable situation
> > + check ovs-vsctl del-port br-int lptemp
> > + check ovs-vsctl del-port br-int lpold
> > +
> > + OVS_WAIT_UNTIL([
> > + test 0 = $(ovs-vsctl show | grep "Port lpold" | wc -l)
> > + ])
> > +
> > + # Wait for correct/lpnew flows to be installed
> > + OVS_WAIT_UNTIL([
> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
> > + ])
> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
> > + flows_after_deletion=$(get_flows $COOKIE)
> > + check test "$flows_lpnew" = "$flows_after_deletion"
> > +
> > + # Check that recompute still works
> > + check ovn-appctl -t ovn-controller recompute
> > + OVS_WAIT_UNTIL([
> > + ofport=$(ovs-vsctl --bare --columns ofport find Interface
name=lpnew)
> > + ovs-ofctl dump-flows br-int | grep $COOKIE | grep
"actions=output:$ofport"
> > + ])
> > + check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep
$COOKIE | wc -l)
> > + flows_after_deletion=$(get_flows $COOKIE)
> > + check test "$flows_lpnew" = "$flows_after_deletion"
> > +
> > + OVN_CLEANUP([hv1])
> > + AT_CLEANUP
> > + ])])
> > +
> > +MULTIPLE_OVS_INT([localport])
> > +MULTIPLE_OVS_INT([])
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
