Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set vlan to 0 if not defined. #180

Conversation

alonSadan
Copy link
Contributor

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
Copy link
Contributor Author

@adrianchiris
Copy link
Contributor

A couple of nits in the commit message:

This commit makes SRIOV-CNi treat no-valn as zero vlan,

CNi -> CNI
valn -> vlan

To prevent future misunderstandings a note is added tot the README

tot -> to

Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of nits but i'm OK with this change.

README.md Outdated
@@ -141,6 +141,10 @@ The below config will configure a VF using a userspace driver (uio/vfio) for use

**Note** [DHCP](https://github.com/containernetworking/plugins/tree/master/plugins/ipam/dhcp) IPAM plugin can not be used for VF bound to a dpdk driver (uio/vfio).

**Note** When VLAN is not specified in the Network-Attachment-Definition, or when it is given a value of 0,
VFs connected tho this network will have no vlan tag.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: tho -> to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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 behavior
is not "natural" and could cause confusion.

This commit makes SRIOV-CNI treat no-vlan as zero vlan,
which will effectively remove the vlan tag from the VF.

To prevent future misunderstandings a note is added to the README
about the new behavior.

Signed-off-by: alonsadan <[email protected]>
@alonSadan alonSadan force-pushed the remove_vlan_from_vf_when_no_vlan_on_nad branch from a36a14d to fe8eed9 Compare April 20, 2021 10:11
@alonSadan
Copy link
Contributor Author

A couple of nits in the commit message:

This commit makes SRIOV-CNi treat no-valn as zero vlan,

CNi -> CNI
valn -> vlan

To prevent future misunderstandings a note is added tot the README

tot -> to

Thanks for the review.
Done.

Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alonSadan , LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants