Hi Lorenzo et al. 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). 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. 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.) 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
