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

Numan Siddique numans at ovn.org
Fri Sep 18 08:38:53 UTC 2020


On Fri, Sep 18, 2020 at 4:42 AM Han Zhou <hzhou at ovn.org> wrote:

> 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?
>

+1. I think it could be a separate patch. May be the first patch of the
series uses qos_map for consider_localnet_lport()
instead of qos_map_ptr and the 2nd patch can address the actual issue.



>
> > >
> > > >              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.
>

Hi Han,

I don't think releasing and then claiming going to help in this case. This
patch is trying
to handle changes to an ovs interface which is part of provider bridge.
The interface
in consideration here doesn't map to any logical port.

If I understand correctly, ovn configures qdiscs (for qos) on this physical
interface attached
to the provider bridge to support qos (probably to support QoS for floating
ips).

Lorenzo - I agree with Han for the above comment on incorrectly handling
the case from false
to true.

I think the ovs interface handle should return false (so that a full
recompute is triggered) if
   (1) An ovs interface is added/delete/updated and it has
external_ids:ovn-egress-iface value
     is set (it could be true/false or anything)
   (2) An ovs interface is modified and the external_ids:ovn-egress-iface
key is deleted by the user.
      In this case a full recompute should be triggered so that we remove
the configured qdiscs on this
      interface.

I think (1) should be straightforward. But unfortunately for (2),
ovn-controller should store the old state.
In my initial discussion with Lorenzo, I didn't consider this scenario.

Thanks
Numan




> >
> > >
> > > >          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
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list