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

Han Zhou hzhou at ovn.org
Fri Sep 18 16:00:40 UTC 2020


On Fri, Sep 18, 2020 at 8:34 AM Numan Siddique <nusiddiq at redhat.com> wrote:
>
>
>
> On Fri, Sep 18, 2020 at 8:53 PM Han Zhou <hzhou at ovn.org> wrote:
>>
>> On Fri, Sep 18, 2020 at 1:39 AM Numan Siddique <numans at ovn.org> wrote:
>> >
>> >
>> >
>> > 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).
>> >
>> Thanks for the explanation. If this is the case, then I think we should
fix
>> the is_iface_vif() function. In this case it is not a VIF. The code
already
>> ensures triggering recompute if a non-VIF has any updates. What do you
>> think?
>>
>
> But it is difficult to identify it right ?  I think the interface type
would be still ""
> when a user adds - ovs-vsctl add-port br-ex eth1.
>
> Maybe we check the bridge of the interface and figure it out ? What do
you think ?
>

Yes, since we know which bridge is the integration bridge. Would it work?

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


More information about the dev mailing list