search

k8snetworkplumbingwg / sriov-cni

DPDK & SR-IOV CNI plugin
Apache License 2.0
298 stars 147 forks source link

Fix checks for vlan parameters #281

Closed mlguerrero12 closed 9 months ago

mlguerrero12 commented 9 months ago
mlguerrero12 commented 9 months ago

@adrianchiris, @SchSeba, @zeeke, could you please review this? The sriov network operator sets vlan and vlanQoS equal to zero by default which is a valid config. I'm removing the check that prevents that and adding back a check that I omitted in my last PR.

adrianchiris commented 9 months ago

while at it, what about vlan proto == 802.1ad and vlan==0 ?

mlguerrero12 commented 9 months ago

nothing happens, it is ignored when vlan is set to 0 (checked with ice and mlx5 drivers). Problem with qos was that in mlx5, the qos was set even if vlan is zero (didn't happen with ice, there it is simply ignored).

Is this really a problem? vf 0 link/ether 76:c9:0e:3f:be:95 brd ff:ff:ff:ff:ff:ff, qos 6, spoof checking off, link-state auto, trust on, query_rss off

It's a valid config. It doesn't return an error.

adrianchiris commented 9 months ago

nothing happens, it is ignored when vlan is set to 0 (checked with ice and mlx5 drivers).

so its invalid ? maybe we want to block it in cni ? currently 802.1ad and vlan=0 is allowed and as you say will work and be ignored.

mlguerrero12 commented 9 months ago

It's a valid config. The driver doesn't return an error. If it was invalid, then the driver should be the one returning the error I think.

Same with the qos for mlx5 (as mentioned in the previous comment), if it was invalid as we are claiming, why would the driver allow it?

My point is, why do we want to introduce an extra layer here in the cni and what is the criteria to classify something as invalid?

mlguerrero12 commented 9 months ago

Just to clarify, the drivers for all cases effectively set the vlan to 0. With ignore I mean the other parameters, i.e. if the qos is set to 5 and vlan to 0, the ice driver sets vlan 0 and qos 0, whereas the mlx5 sets vlan 0 and qos 5.

mlguerrero12 commented 9 months ago

I also wonder, the fact that mlx5 allows to set vlan 0 and qos 5, doesn't it mean that it supports the vlan 0 priority tag support? When a device needs to send priority-tagged packets but it doesn't know in which vlan resides.

mlguerrero12 commented 9 months ago

To speed up the merge since ci is broken due to this, I will restrict the default values of qos and proto to their defaults when vlan is zero. We can discuss what I mentioned above in a following PR.

adrianchiris commented 9 months ago

+1 for restricting this for now until there is a use-case for it.

its not the first time we see different behavior between drivers around VF configuration such as this.