[ovs-dev] [PATCH v3 ovn] binding: fix localnet QoS configuration after I-P
Numan Siddique
numans at ovn.org
Thu Sep 24 07:21:56 UTC 2020
On Thu, Sep 24, 2020 at 2:37 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>
> ---
> Changes since v2:
> - drop patch 1/3
> - patch 2/3 has been applied
> - simplify full recomputation check
>
> Changes since v1:
> - move qos_map fix in a separated patch
> - add ovn-egress-iface info to binding I-P engine
> relying on shash for local_iface_ids
> ---
> controller/binding.c | 5 ++++
> tests/ovn-performance.at | 13 +++++++++
> tests/system-ovn.at | 60 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 78 insertions(+)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 99e9fae30..c9a097f66 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1870,6 +1870,11 @@ binding_handle_ovs_interface_changes(struct
> binding_ctx_in *b_ctx_in,
> b_ctx_in, b_ctx_out);
> }
>
> + if (smap_get(&iface_rec->external_ids, "ovn-egress-iface") ||
> + sset_contains(b_ctx_out->egress_ifaces, iface_rec->name)) {
> + handled = false;
> + }
> +
>
Hi Lorenzo,
Thanks for the v3. I think we can move the above block you added before
checking for the external_ids:iface-id.
When a normal ovs interface is deleted, after the consider_iface_release(),
this if block would get executed
even though there is no need to.
> if (!handled) {
> break;
> }
> 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])
>
When I run this test in a loop, the below AT_CHECK to check the qdiscs,
fails. Its mostlly
a timing issue. Can you please change this to OVS_WAIT_UNTIL ?
> +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])
>
Same here. Please change this to OVS_WAIT_UNTIL.
Thanks
Numan
> +
> +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
>
>
More information about the dev
mailing list