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