On Thu, Mar 30, 2023 at 6:44 AM Rodolfo Alonso Hernandez
<[email protected]> wrote:
>
> Hello:
>
> In OpenStack Neutron we are only using the "qos_min_rate" setting in the LSP
> (for max rate and DSCP we use QoS registers), only for egress traffic. The
> way it is supposed to work is the TC rules should be applied on the egress
> interface. In the case of VLAN or flat networks (not tunneled), the physical
> bridge will have an interface; this interface should receive the TC rules.
>
> When testing the current code (not the new patch series) using OpenStack, we
> deployed the environment with two physical bridges connected to two physical
> networks. Then we created two ports, one per network. In OpenStack Neutron we
> are only using the "qos_min_rate" setting. Each time a QoS was defined for a
> LSP, both physical bridges external interfaces were receiving the TC
> configuration, instead of only the physical interface of the network.
>
I think this is just the way it's implemented: every egress iface gets
the same queues configured. But these queues are tied to ports
(through unique qdisc_queue_id). It's just that, in general, some
queues on some egress interfaces won't be utilized (depending on the
logical topology).
Is it a *real* problem? Do we expect e.g. performance issues because
of unnecessary queues defined?
> NOTE: in Neutron we are only defining the "qos_min_rate", as commented
> before. Because OVN makes a direct translation between the OVN parameters and
> the TC rules, we also need to define the "qos_max_rate", same as when
> executing a TC command ("rate" cannot be defined without "ceil"). But this is
> something we need to fix in Neutron.
>
> About the new series, it is difficult for Neutron to set the "qos_ovs_port"
> in the LSP. Neutron is acting as a CMS and can't read the local OVS database
> of a particular node. We can have access to OVN NB and SB, but currently it
> is not possible to read each node's local OVS database. If this is going to
> be the implementation, we'll need to refactor how "min-bw" feature works,
> using some agents deployed in the nodes to retrieve this info (NOTE: we have
> agents in the nodes but not for this purpose; in any case, that can be
> considered).
>
> Regards.
>
> On Wed, Mar 29, 2023 at 11:31 PM Lorenzo Bianconi
> <[email protected]> wrote:
>>
>> > Hi Lorenzo et al.
>>
>> Hi Ihar,
>>
>> thx for reviewing the series.
>>
>> >
>> > I took a look at the series and have a number of concerns. Note that I
>> > looked at the series in context of OpenStack and the way Neutron uses
>> > qos_ options. Some points below are not directly relevant to the
>> > series but I think they add important context.
>> >
>> > ---
>> >
>> > First, I will agree with the direction of the patch - it is beneficial
>> > to switch from TC to using OVS queues to configure LSP level qos_*
>> > settings.
>> >
>> > But, see below.
>> >
>> > 1. The 2/3 patch explicitly claims that the qos_min_rate option is for
>> > localnet ports only: "QoS is only supported on localnet port for the
>> > moment". Actually, it's vice versa: the option is documented for VIF
>> > (regular) ports
>> > only:https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/ovn-nb.xml#L1096
>> > To compare, the localnet section of the document doesn't list this
>> > option:
>> > https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/ovn-nb.xml#L1034
>> >
>> > This assumption about the application of the options to localnet
>> > ports, and not to other port types, seems to creep into the 1/3 patch
>> > of the series too.
>> >
>> > I tried to understand where the misunderstanding comes from. Perhaps
>> > it's because the "egress qos" system test was added to cover a
>> > supposed I-P regression where qos is not applied to localnet ports.
>> > Then later, when adding qos_min_rate option support, I patched the
>> > existing test case, without realizing that it sets qos rules to
>> > localnet port, not regular VIF port (as per NB documentation). We
>> > should probably fix the test case (or add a new one for regular ports
>> > if we decide that localnet qos is a valid scenario).
>> >
>> > Or perhaps it's because we call consider_localnet_lport with qos_map
>> > passed, so it also configures qos queues for these ports.
>> >
>> > Regardless, it's clear that regular (VIF) ports should support qos_
>> > options, so any changes to disable qos for them is invalid. Actually,
>> > OpenStack Neutron already uses qos_min_rate option to configure its
>> > minimal bandwidth guarantees, and these guarantees are set for regular
>> > ports. I don't know if localnet ports should also support qos rules
>> > (if so, documentation should be updated to include the options in
>> > localnet section).
>>
>> ack, I will fix the VIF port use-case in RFC v2. It was not clear to me
>> OpenStack just applies QoS on logical switch ports, thx for pointing this
>> out.
>>
>> >
>> > 2. AFAIU there's some bug in qos code where policies that are not set
>> > for a port are being applied, when there are multiple localnet ports.
>> > (At least that's what I figured from a discussion with Rodolfo;
>> > Rodolfo, could you please describe the exact scenario that you
>> > experienced?) But it's not related to issues with qos_ options set to
>> > localnet ports (because Neutron doesn't set them for localnets.)
>> >
>> > 3. Side note: while reviewing the series, I noticed that
>> > port_has_qos_params function doesn't check if qos_min_rate option is
>> > set. This is not a problem of your series, but should be fixed.
>>
>> ack, I will add a fix for it.
>>
>> >
>> > 4. Side note about OpenStack: AFAIU for a physical interface to be
>> > included in qdisc setup, it should be marked with
>> > ovn-egress-iface=true. But I couldn't find code in Neutron that would
>> > configure it. (Nor anything relevant on Github.) Rodolfo, could you
>> > please clarify if any component sets the option up?
>> >
>> > ---
>> >
>> > For reference, here is how qos works right now:
>> > - qos_ options are set for regular (VIF) port.
>> > - northd allocates qdisc_queue_id for each qos-enabled (regular) port.
>> > - ovn-controller sets metric for the ID, plus creates qdisc queues on
>> > physical interfaces that are related to the ports (through
>> > corresponding localnet ports on the same switch).
>> >
>> > (Let me know if the above doesn't match your vision of how things work
>> > now.)
>>
>> I will share a RFC v2 so we can continue the discussion from there.
>>
>> Regards,
>> Lorenzo
>>
>> >
>> > Thanks,
>> > Ihar
>> >
>> > On Mon, Mar 20, 2023 at 2:33 PM Mark Michelson <[email protected]> wrote:
>> > >
>> > > Hi Lorenzo,
>> > >
>> > > Since this is an RFC series, I did not do an in-depth review of the
>> > > patches. I just took a quick look.
>> > >
>> > > My only concern is that patch 1 breaks existing configurations for QoS,
>> > > since the ovs-port parameter is now required. Is there any way to
>> > > preserve existing behavior [1] for deployments that do not have this
>> > > option set?
>> > >
>> > > Other than that, at a high level it seems like a good series.
>> > >
>> > > Thanks,
>> > > Mark Michelson
>> > >
>> > >
>> > > [1] By "existing behavior" I don't mean to configure tc directly. I just
>> > > mean that QoS will end up set up with the same behavior as it did before
>> > > this patch series.
>> > >
>> > > On 3/17/23 16:00, Lorenzo Bianconi wrote:
>> > > > Rework OVN QoS implementation in order to configure it through OVS QoS
>> > > > table instead of running tc command directly bypassing OVS.
>> > > >
>> > > > Lorenzo Bianconi (3):
>> > > > controller: fix interface qos aliasing
>> > > > controller: configure qos through ovs qos table and do not run tc
>> > > > directly
>> > > > controller: get rid of egress_ifaces
>> > > >
>> > > > controller/binding.c | 374
>> > > > ++++++++++++------------------------
>> > > > controller/binding.h | 1 -
>> > > > controller/ovn-controller.c | 15 +-
>> > > > northd/northd.c | 3 +-
>> > > > northd/ovn-northd.8.xml | 6 -
>> > > > ovn-nb.xml | 5 +
>> > > > ovn-sb.xml | 5 +
>> > > > tests/ovn-performance.at | 5 -
>> > > > tests/system-ovn.at | 41 +++-
>> > > > 9 files changed, 184 insertions(+), 271 deletions(-)
>> > > >
>> > >
>> > > _______________________________________________
>> > > dev mailing list
>> > > [email protected]
>> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> > >
>> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
