> 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
