On Mon, Oct 24, 2022 at 5:22 PM 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
> ---
> 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 <[email protected]>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
