set vlan to 0 if not defined. #29
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alonSadan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @alonSadan. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
needs-ok-to-test
|
@alonSadan: GitHub didn't allow me to request PR reviews from the following users: adrianchiris, martinkennelly. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
It is more common that when no vlan tag is specified, the interface will have no vlan at all. The sriov-CNI acts differtly, and when a NAD is not configured with vlan, the VF will keep it's old vlan tag. This behaviour is not "natural" and could cause confusion. This commit makes SRIOV-CNi treat no-valn as zero vlan, which will effectively remove the vlan tag from the VF. To prevent future misunderstandings a note is added tot the README about the new behavior. Signed-off-by: alonsadan <[email protected]>
alonSadan
force-pushed
the
remove_vlan_from_vf_when_no_vlan_on_nad
branch
from
8a433d7 to
fe2341c
Compare
|
@alonSadan Shouldn't this change go to upstream repository for discussion? My 2-cents but would defer to adrian and zenghui due to their experience here: It does open up a can of worms and changes the philosophy of SRIOV CNI where we inherit the VF config, and reapply it after the VF is released from a container. Maybe, we can have a discussion of what defaults we can incorporate when fields are not defined but I am in favor of this change. |
Yes, this needs to be upstreamed first.
I tend to agree with the use case description. If user wants to set a vlan for VF, then he or she would configure it in the sriov-cni net-attach-def, rather than doing it while initializing the VFs. In SR-IOV Operator, we set vlan=0 explicitly while creating sriov-cni net-attach-def (we have no choice here as the API definition for vlan has @adrianchiris looking at the original issue, I do recognize that there is a difference on how user would expect in the case of vlan=null or vlan=0, and this PR would change the default behavior of sriov-cni for users who expects vlan=null to be equal to existing non-zero vlan on VF. how do you think we can move forward on this issue? |
We should have an issue in upstream repo for discussion. i will check internally to see if there is a use-case for that. This (not setting vlan explicitly to 0 when no vlan specified in net-attach-def) becomes a pain-point if a previous cmdDel (For a different network with vlan) did not clean up the VF properly, this VF then gets reassigned to a pod with a net-attach-def that does not contain vlan in its CNI config, am i correct ? |
Yes, that's my understanding as well. |
|
Thank you @zshi-redhat @martinkennelly @adrianchiris . |
|
This was solved in upstream k8snetworkplumbingwg/sriov-cni#180 and backport to openshift: #31 |
It is more common that when no vlan tag is specified,
the interface will have no vlan at all.
The sriov-CNI acts differtly, and when a NAD is not configured with
vlan, the VF will keep it's old vlan tag. This behaviour
is not "natural" and could cause confusion.
This commit makes SRIOV-CNi treat no-valn as zero vlan,
which will effectively remove the vlan tag from the VF.
To prevent future misunderstandings a note is added tot the README
about the new behavior.
Fixes #25
Signed-off-by: alonsadan [email protected]