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

Han Zhou hzhou at ovn.org
Thu Sep 17 23:11:49 UTC 2020


On Thu, Sep 17, 2020 at 2:41 PM Lorenzo Bianconi <
lorenzo.bianconi at redhat.com> wrote:
>
> On Sep 17, Han Zhou wrote:
> > 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?
>
> because otherwise if you do not have any local tunnel configured
qos_map_ptr will
> be NULL and you will not update the qos_map
>
I am still not clear. Sounds like it is a different bug to be fixed? The
title is about a problem in I-P, but here it is for full compute. If that's
the case, could you send a separate fix with more detailed information such
as how to reproduce and the test case to cover?

> >
> > >              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?
>
> I have this discussion with Numan as well and we agreed to implement a
simple solution
> in the first attempt since this routine will not run so often with
ovn-egress-iface
> set to true. Another possible approach would be to cache the value as
done for
> iface-id and recompute only when the value is toggled. What do you think?
>
I am ok with a simple solution but it seems incorrect to handle only the
change from "false" to "true" without handling the change from "true" to
"false".

> Regards,
> Lorenzo
>
> >
> > 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


More information about the dev mailing list