I don't think that will be a problem because: * The number of external interfaces will be one or two (each interface is an expensive network card). * We'll have as many TC rules as ports. If we have 100 ports (for example) in a compute node, that will be 100 rules (max). That is not a big deal for the TC engine when matching with the queue ID.
Regards. On Thu, Mar 30, 2023 at 7:00 PM Ihar Hrachyshka <[email protected]> wrote: > 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
