[ovs-dev] [PATCH ovn] binding: fix localnet QoS configuration after I-P

Thu Sep 17 19:34:30 UTC 2020

On Thu, Sep 17, 2020 at 6:40 AM Lorenzo Bianconi <
lorenzo.bianconi at redhat.com> wrote:
>
> After the I-P has been introduced in commit 354bdba51a ("ovn-controller:
> I-P for SB port binding and OVS interface in runtime_data"), the QoS on
> localnet ports is not properly configured if the ovs interface is marked
> with "ovn-egress-iface" flag after the related record in the
> logical_switch_port table is set. The issue can be triggered with the
> following reproducer:
>
> $ovn-nbctl set Logical_Switch_Port ln-public options:qos_burst=1000
> $ovs-vsctl set interface eth0 external-ids:ovn-egress-iface=true
>
> Fix the issue triggering a recomputation after qos is configured for ovs
> interface
>
> Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS
> interface in runtime_data")
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> ---
>  controller/binding.c     | 10 ++++++-
>  tests/ovn-performance.at | 13 +++++++++
>  tests/system-ovn.at      | 60 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 3c102dc7f..c8b099991 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1436,7 +1436,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct
binding_ctx_out *b_ctx_out)
>              break;
>
>          case LP_LOCALNET: {
> -            consider_localnet_lport(pb, b_ctx_in, b_ctx_out,
qos_map_ptr);
> +            consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map);

Why this change?

>              struct localnet_lport *lnet_lport = xmalloc(sizeof
*lnet_lport);
>              lnet_lport->pb = pb;
>              ovs_list_push_back(&localnet_lports, &lnet_lport->list_node);
> @@ -1904,6 +1904,14 @@ binding_handle_ovs_interface_changes(struct
binding_ctx_in *b_ctx_in,
>              continue;
>          }
>
> +        bool egress_info = smap_get_bool(&iface_rec->external_ids,
> +                                         "ovn-egress-iface", false);
> +        if (egress_info) {
> +            handled = false;
> +            break;
> +        }
> +
> +

With this, any such interface change will result in full recompute, if any
of the changed interfaces has "ovn-egress-iface" == true, even if this
key-value itself didn't change.
On the other hand, if it changed from "true" to "false", the I-P will still
miss the processing of unconfigure the QoS?

Cc Numan. This is related to the discussion we had before, for how to
handle interface changes when iface-id and ofport didn't change but some
other fields changed. I am still thinking about whether we could simply
release and reclaim the port and reprocessing all information related to
the port, without having to handle the different combinations of the
changes within the interface (and doesn't need to trigger full recompute
either). But I am not as familiar as you for the implications to the later
stages of the I-P.

>          const char *iface_id = smap_get(&iface_rec->external_ids,
"iface-id");
>          int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
>          if (iface_id && ofport > 0) {
> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> index 3010860d5..6cc5b2174 100644
> --- a/tests/ovn-performance.at
> +++ b/tests/ovn-performance.at
> @@ -247,6 +247,8 @@ for i in 1 2 3; do
>      ovs-vsctl set open . external_ids:ovn-bridge-mappings="public:br-ex"
>      j=$((i + 2))
>      ovn_attach n1 br-phys 192.168.0.$j
> +    ip link add vgw$i type dummy
> +    ovs-vsctl add-port br-ex vgw$i
>  done
>
>  # Wait for the tunnel ports to be created and up.
> @@ -477,6 +479,17 @@ OVN_CONTROLLER_EXPECT_HIT(
>      [ovn-nbctl --wait=hv lrp-set-gateway-chassis lr1-public gw3 10 &&
ovn-nbctl --wait=hv sync]
>  )
>
> +# create QoS rule
> +OVN_CONTROLLER_EXPECT_NO_HIT(
> +    [hv1 hv2 gw1 gw2 gw3], [lflow_run],
> +    [ovn-nbctl --wait=hv set Logical_Switch_Port ln-public
options:qos_burst=1000]
> +)
> +
> +OVN_CONTROLLER_EXPECT_HIT(
> +    [gw1], [lflow_run],
> +    [as gw1 ovs-vsctl set interface vgw1
external-ids:ovn-egress-iface=true]
> +)
> +
>  # Make gw2 master. There is remote possibility that full recompute
>  # triggers for gw2 after it becomes master. Most of the time
>  # there will be no recompute.
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 4f72754c6..f779947f4 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -5397,3 +5397,63 @@ as
>  OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
>  /.*terminating with signal 15.*/d"])
>  AT_CLEANUP
> +
> +AT_SETUP([ovn -- egress qos])
> +AT_KEYWORDS([ovn-egress-qos])
> +
> +ovn_start
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_BR([br-int])
> +ADD_BR([br-ext])
> +
> +ovs-ofctl add-flow br-ext action=normal
> +# Set external-ids in br-int needed for ovn-controller
> +ovs-vsctl \
> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> +        -- set Open_vSwitch .
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> +        -- set bridge br-int fail-mode=secure
other-config:disable-in-band=true
> +
> +# Start ovn-controller
> +start_daemon ovn-controller
> +
> +ovn-nbctl ls-add sw0
> +
> +ADD_NAMESPACES(sw01)
> +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03")
> +ovn-nbctl lsp-add sw0 sw01 \
> +    -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2"
> +
> +ADD_NAMESPACES(public)
> +ADD_VETH(public, public, br-ext, "192.168.2.2/24", "f0:00:00:01:02:05")
> +
> +AT_CHECK([ovs-vsctl set Open_vSwitch .
external-ids:ovn-bridge-mappings=phynet:br-ext])
> +ovn-nbctl lsp-add sw0 public \
> +        -- lsp-set-addresses public unknown \
> +        -- lsp-set-type public localnet \
> +        -- lsp-set-options public network_name=phynet
> +
> +AT_CHECK([ovn-nbctl set Logical_Switch_Port public
options:qos_burst=1000])
> +AT_CHECK([ovs-vsctl set interface ovs-public
external-ids:ovn-egress-iface=true])
> +AT_CHECK([tc qdisc show | grep -q 'htb 1: dev ovs-public'])
> +
> +AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
qos_burst=1000])
> +AT_CHECK([tc qdisc show | grep -q 'htb 1: dev ovs-public'],[1])
> +
> +kill $(pidof ovn-controller)
> +
> +as ovn-sb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as ovn-nb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as northd
> +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> +/.*terminating with signal 15.*/d"])
> +AT_CLEANUP
> --
> 2.26.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev