Fix checks for vlan parameters #281
Conversation
mlguerrero12
commented
Oct 5, 2023
•
mlguerrero12
commented
- Add check to not allow non default values of qos and proto when vlan is zero.
- Remove check when vlan is zero and qos and proto are not in config since default values are now set.
mlguerrero12
force-pushed
the
removevlanqosprotocheck
branch
2 times, most recently
from
bcb1e33
to
f8813b8
Compare
mlguerrero12
force-pushed
the
removevlanqosprotocheck
branch
from
f8813b8
to
57096a8
Compare
mlguerrero12
changed the title
mlguerrero12
force-pushed
the
removevlanqosprotocheck
branch
from
57096a8
to
7433a68
Compare
|
@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. |
|
while at it, what about vlan proto == 802.1ad and vlan==0 ? |
|
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? It's a valid config. It doesn't return an error. |
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. |
|
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? |
|
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. |
|
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. |
|
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. |
mlguerrero12
force-pushed
the
removevlanqosprotocheck
branch
from
7433a68
to
21ca0ed
Compare
- Add check to not allow non default values of qos and proto when vlan is zero. - Remove check when vlan is zero and qos and proto are not in config since default values are now set. Signed-off-by: Marcelo Guerrero <[email protected]>
mlguerrero12
force-pushed
the
removevlanqosprotocheck
branch
from
21ca0ed
to
f76881b
Compare
|
+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. |
