multus_tests, sriov: add vlan=0 to NAD #5093

alonSadan · 2021-02-24T14:44:19Z

What this PR does / why we need it:

There is a difference between a network(defined in networkAttachmentDefenition)
with vlan=0 attribute and a network with no vlan attribute at all.
If the VF has some vlan tag different than 0,
then when it is assigned to a pod (or a vm/i)
which is connected to a network with vlan=0 the vlan tag will be removed.
When the pod is connected to a network with no vlan attribute,
the vlan tag will remain as it is.

This behavior is implemented in SRIOV-CNI.
This is where the vlan is assigned :
https://github.com/k8snetworkplumbingwg/sriov-cni/blob/b18123d809f1c010cae19018e3395ed5981e76f7/pkg/sriov/sriov.go#L287

The investigation found that the source of the ping flakiness is that in our tests,
for some reason, the vlan tag remains from the ping-over-sriov+vlan test.
Then on the ping-over-sriov-no-vlan test, because the network does not have a vlan defined attribute,
it will not remove the existing vlan tag upon VF assignment.
Then, if unfortunately one VM/I will be assigned with a tagged VF it will not remove it, and if another VM/I assigned with
a VF with no vlan tag ,it will lead to a ping failure.

There is an open issue regarding this behavior on SRIOV-CNI:
openshift/sriov-cni#25

Another interesting thing is that when the net-attach-def is created by the SRIOV-operator,
after creating a sriovNetwork object, it will give the net-attach-def a value of 0,
no matter if the sriovNetwork object has a vlan attribute or not.

By creating the net-attach-def with a vlan=0 attribute,
the vlan tag is removed from the VF the test seems to pass consistently.

Signed-off-by: alonsadan asadan@redhat.com

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #4642

Special notes for your reviewer:

Release note:

NONE

EdDev · 2021-02-24T16:32:53Z

Thank you!

Could you please open an issue on the SR-IOV CNI and link it here?
Not specifying the VLAN should be interpreted by the CNI as VLAN=0, but it cannot assume it should not touch the existing VLAN, that is prone to bugs, especially when the user is not intending to have any VLAN.

BTW, in classic networking, there is a difference between a non VLAN and a VLAN=0. No VLAN means that it is not a trunk (no tagged traffic) while VLAN 0 means that the ethernet header includes a VLAN just for the priority bits. So I find this strange that they are considered the same.

alonSadan · 2021-02-25T09:02:40Z

Thank you!

Could you please open an issue on the SR-IOV CNI and link it here?
Not specifying the VLAN should be interpreted by the CNI as VLAN=0, but it cannot assume it should not touch the existing VLAN, that is prone to bugs, especially when the user is not intending to have any VLAN.

Done: openshift/sriov-cni#25

BTW, in classic networking, there is a difference between a non VLAN and a VLAN=0. No VLAN means that it is not a trunk (no tagged traffic) while VLAN 0 means that the ethernet header includes a VLAN just for the priority bits. So I find this strange that they are considered the same.

Thanks for the explanation :)

alonSadan · 2021-02-25T09:02:49Z

/retest

oshoval

Nice
is there a way to tell it to totally "untag" instead of VLAN ID 0 ?
in case 0 removes the vlan header its the same,

but there are basically two options that will allow traffic:

dont have vlan header (which is better than option 2)
have vlan header with vlan tag 0 (and then for example you can keep the 802.1p priority fields)

Edit:
i see now Edy said the same

alonSadan · 2021-02-25T09:59:27Z

Thanks @oshoval .
I agree.
I just want to emphasize that from what I saw,
when the NAD has a VLAN attribute with a value of 0,
the CNI will remove the VLAN tag from the VF completely, and will not give the VF a vlan tag 0.

alonSadan · 2021-02-28T15:37:57Z

/retest

alonSadan · 2021-03-01T08:01:09Z

/retest

alonSadan · 2021-03-02T12:10:55Z

/retest

EdDev · 2021-03-04T08:07:30Z

/sig network

alonSadan · 2021-03-04T14:54:34Z

Rebased on master

alonSadan · 2021-03-04T14:54:52Z

/cc @EdDev

EdDev

Commit message comments:

multus_tests, sriov: add vlan=0 to NAD

tests, sriov: Explicitly set vlan=0 to NADs with no VLAN defined

There is a difference between a network(defined in networkAttachmentDefenition)
with vlan=0 attribute and a network with no vlan attribute at all.
If the VF has some vlan tag different than 0,
then when it is assigned to a pod (or a vm/i)
which is connected to a network with vlan=0 the vlan tag will be removed.
When the pod is connected to a network with no vlan attribute,
the vlan tag will remain as it is.

This behaviour is intended and is implemented this way in SRIOV-CNI.
This is where the vlan is assigned :
https://github.com/k8snetworkplumbingwg/sriov-cni/blob/master/pkg/sriov/sriov.go#L287

The fact that it appears in the code does not mean it is intended.
You can just mention that the SRIOV-CNI is implementing this logic.

And this is where it is released:
https://github.com/k8snetworkplumbingwg/sriov-cni/blob/master/pkg/sriov/sriov.go#L386

I would not ref code, but if you do, ref a permanent link (the current one is not permanent and will not be relevant as soon as the code changes).

As can be seen there, When a VF is released,
the original attributes that were before the assignment are restored.
I think that the source of the ping flakiness is that in our tests,

If you are unsure, it is problematic. You can do something like this: "The investigation found that ...".

the network does not have a vlan defined attribute,
so the original attribute does not get restored,
what can lead to one VF keep it’s vlan tag from the ping-over-sriov+vlan test,
and then on the ping-over-sriov-no-vlan test, one of the VFs have a vlan tag,
and the other one doesn’t , so the ping fails.

Another interesting thing is that when the net-attach-def is created by the SRIOV-operator,
after creating a sriovNEtwork object, it will give the net-attach-def a value of 0,
no matter if the sriovNetwork object has a vlan attribute or not.

By creating the net-attach-def with a vlan=0 attribute,
the test seems to pass consistently on my local env.
The only mystery left is why this is happening only on the ConnectX-5 machine,
and it will require further investigation.

Please drop on my local env.
Regarding the mystery, I think we know the answer already. It seems to occur only on the ConnectX-5 SR-IOV NIC, where on numvfs change/reset, the VF config is preserved. This has not been seen on other NIC/s in the CI setups.

EdDev · 2021-03-04T14:55:22Z

Also, please reference the issue you opened on the CNI.

alonSadan · 2021-03-04T15:32:19Z

tests, sriov: Explicitly set vlan=0 to NADs with no VLAN defined

Done.

The fact that it appears in the code does not mean it is intended.
You can just mention that the SRIOV-CNI is implementing this logic.

Done. If it is intended or not will reveal in the issue I opened in SRIOV-CNI.

I would not ref code, but if you do, ref a permanent link (the current one is not permanent and will not be relevant as soon as the code changes).

Changed to perm links.

Please drop on my local env.
Regarding the mystery, I think we know the answer already. It seems to occur only on the ConnectX-5 SR-IOV NIC, where on numvfs change/reset, the VF config is preserved. This has not been seen on other NIC/s in the CI setups.

Dropped both "local env" and "mystery" part.

Also, please reference the issue you opened on the CNI.

I refed it here, but I will add it to the PR
description as well.

EdDev · 2021-03-04T15:46:40Z

As can be seen there, When a VF is released,
the original attributes that were before the assignment are restored.
The investigation found that the source of the ping flakiness is that in our tests,
the network does not have a vlan defined attribute,
so the original attribute does not get restored,
what can lead to one VF keep it’s vlan tag from the ping-over-sriov+vlan test,
and then on the ping-over-sriov-no-vlan test, one of the VFs have a vlan tag,
and the other one doesn’t , so the ping fails.

I am re-reading this and something is wrong here.
The release part is irrelevant at this stage and that logic seems fine to me: If nothing was done on assigning the VF, nothing should be done when releasing. That is symmetric.
If on assignment it will get changed, it will have to be changed on release as well.

If a test uses a VLAN, it means it was probably annotated on the pod and eventually together with the NAD, it will reach the CNI with a VLAN, at which time it will configure the VF and on release it will be restored.
The question is, is this restore part exists on the current CNI version we use in the tests.

When you now set VLAN=0 to be the default if no VLAN is specified in the pod annotation, then any value that was there before will get overwritten and solve the problem.

Therefore, the open question about what left a VLAN set there is still relevant. I would bet it is the CNI plugin version, but you will need to confirm this.

alonSadan · 2021-03-04T17:40:51Z

As can be seen there, When a VF is released,
the original attributes that were before the assignment are restored.
The investigation found that the source of the ping flakiness is that in our tests,
the network does not have a vlan defined attribute,
so the original attribute does not get restored,
what can lead to one VF keep it’s vlan tag from the ping-over-sriov+vlan test,
and then on the ping-over-sriov-no-vlan test, one of the VFs have a vlan tag,
and the other one doesn’t , so the ping fails.

I am re-reading this and something is wrong here.
The release part is irrelevant at this stage and that logic seems fine to me: If nothing was done on assigning the VF, nothing should be done when releasing. That is symmetric.
If on assignment it will get changed, it will have to be changed on release as well.

If a test uses a VLAN, it means it was probably annotated on the pod and eventually together with the NAD, it will reach the CNI with a VLAN, at which time it will configure the VF and on release it will be restored.
The question is, is this restore part exists on the current CNI version we use in the tests.

When you now set VLAN=0 to be the default if no VLAN is specified in the pod annotation, then any value that was there before will get overwritten and solve the problem.

Therefore, the open question about what left a VLAN set there is still relevant. I would bet it is the CNI plugin version, but you will need to confirm this.

Thanks Eddy for noticing that. My explanation indeed wasn't right. I changed the commit message and the PR description, and I think it is more accurate now.

There is a difference between a network (defined in networkAttachmentDefenition)with vlan=0 attribute, and a network with no vlan attribute at all. If the VF has some vlan tag different than 0, then when it is assigned to a pod (or a vmi) which is connected to a network with vlan=0, the vlan tag will be removed. When the pod is connected to a network with no vlan attribute, the vlan tag will remain as it is. This behavior is implemented in SRIOV-CNI. This is where the vlan is assigned : https://github.com/k8snetworkplumbingwg/sriov-cni/blob/b18123d809f1c010cae19018e3395ed5981e76f7/pkg/sriov/sriov.go#L287 The investigation found that the source of the ping flakiness is that in our tests, for some reason, the vlan tag remains from the ping-over-sriov+vlan test. Then on the ping-over-sriov-no-vlan test, because the network does not have a vlan defined attribute, it will not remove the existing vlan tag upon VF assignment. Then, if unfortunately one VMI will be assigned with a tagged VF, it will not remove the tag, and if another VMI assigned with a VF with no vlan tag ,it will lead to a ping failure. Another interesting thing is that when the net-attach-def is created by the SRIOV-operator, after creating a sriovNetwork object, it will give the net-attach-def a value of 0, no matter if the sriovNetwork object has a vlan attribute or not. By creating the net-attach-def with a vlan=0 attribute, the vlan tag is removed from the VF the test seems to pass consistently. Signed-off-by: alonsadan <asadan@redhat.com>

EdDev · 2021-03-04T18:05:07Z

Thank you, there are only two points left:

VM/I =>> VMI
Keep the lines no longer than 72 long. (except for the url link I guess)

EdDev

Thank you!

alonSadan · 2021-03-04T18:37:42Z

Thanks Eddy :)

@phoracek What do you think?

phoracek

/approve

Nice catch!

kubevirt-bot · 2021-03-05T08:43:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phoracek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~tests/OWNERS~~ [phoracek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-commenter-bot · 2021-03-05T19:21:08Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2021-03-06T12:21:08Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2021-03-07T12:46:36Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2021-03-08T06:04:08Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

alonSadan · 2021-03-08T07:45:45Z

/retest

The issue: kubevirt#4642 is resolved by: kubevirt#5093 All the details are in the PR. Since The PR got merged, the flakes were not seen even after dozens of runs. Therefore there is no need for the xfail anymore, and this commit removes it from the SRIOV suit. Signed-off-by: alonsadan <asadan@redhat.com>

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XS labels Feb 24, 2021

kubevirt-bot requested review from ILpinto and kbidarkar February 24, 2021 14:44

oshoval reviewed Feb 25, 2021

View reviewed changes

alonSadan force-pushed the add_vlan_0_to_nad branch from 5d7882e to 937945c Compare February 25, 2021 10:05

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2021

kubevirt-bot added the sig/network label Mar 4, 2021

alonSadan force-pushed the add_vlan_0_to_nad branch from 937945c to 1824766 Compare March 4, 2021 14:52

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2021

kubevirt-bot requested a review from EdDev March 4, 2021 14:54

EdDev reviewed Mar 4, 2021

View reviewed changes

alonSadan force-pushed the add_vlan_0_to_nad branch from 1824766 to 52d1537 Compare March 4, 2021 15:17

alonSadan force-pushed the add_vlan_0_to_nad branch from 52d1537 to 3bc0a9b Compare March 4, 2021 17:39

alonSadan force-pushed the add_vlan_0_to_nad branch from 3bc0a9b to a048a93 Compare March 4, 2021 17:43

alonSadan force-pushed the add_vlan_0_to_nad branch from a048a93 to 4b59795 Compare March 4, 2021 18:06

EdDev approved these changes Mar 4, 2021

View reviewed changes

kubevirt-bot assigned EdDev Mar 4, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2021

phoracek approved these changes Mar 5, 2021

View reviewed changes

kubevirt-bot assigned phoracek Mar 5, 2021

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2021

kubevirt-bot merged commit e95b9bc into kubevirt:master Mar 8, 2021

alonSadan mentioned this pull request Mar 8, 2021

Old VLAN tag remains on VF when NAD has no VLAN field. openshift/sriov-cni#25

Closed

alonSadan mentioned this pull request Mar 21, 2021

SRIOV [test_id:3956]/[test_id:3957] are flaky, ping is failing between two SRIOV vnic/s #4642

Closed

alonSadan mentioned this pull request Mar 21, 2021

tests, sriov: remove xfail #5282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multus_tests, sriov: add vlan=0 to NAD #5093

multus_tests, sriov: add vlan=0 to NAD #5093

alonSadan commented Feb 24, 2021 •

edited

Loading

EdDev commented Feb 24, 2021

alonSadan commented Feb 25, 2021 •

edited

Loading

alonSadan commented Feb 25, 2021

oshoval left a comment •

edited

Loading

alonSadan commented Feb 25, 2021

alonSadan commented Feb 28, 2021

alonSadan commented Mar 1, 2021

alonSadan commented Mar 2, 2021

EdDev commented Mar 4, 2021

alonSadan commented Mar 4, 2021

alonSadan commented Mar 4, 2021

EdDev left a comment

EdDev commented Mar 4, 2021

alonSadan commented Mar 4, 2021

EdDev commented Mar 4, 2021 •

edited

Loading

alonSadan commented Mar 4, 2021 •

edited

Loading

EdDev commented Mar 4, 2021 •

edited

Loading

EdDev left a comment

alonSadan commented Mar 4, 2021 •

edited

Loading

phoracek left a comment

kubevirt-bot commented Mar 5, 2021

kubevirt-commenter-bot commented Mar 5, 2021

kubevirt-commenter-bot commented Mar 6, 2021

kubevirt-commenter-bot commented Mar 7, 2021

kubevirt-commenter-bot commented Mar 8, 2021

alonSadan commented Mar 8, 2021

multus_tests, sriov: add vlan=0 to NAD #5093

multus_tests, sriov: add vlan=0 to NAD #5093

Conversation

alonSadan commented Feb 24, 2021 • edited Loading

EdDev commented Feb 24, 2021

alonSadan commented Feb 25, 2021 • edited Loading

alonSadan commented Feb 25, 2021

oshoval left a comment • edited Loading

Choose a reason for hiding this comment

alonSadan commented Feb 25, 2021

alonSadan commented Feb 28, 2021

alonSadan commented Mar 1, 2021

alonSadan commented Mar 2, 2021

EdDev commented Mar 4, 2021

alonSadan commented Mar 4, 2021

alonSadan commented Mar 4, 2021

EdDev left a comment

Choose a reason for hiding this comment

EdDev commented Mar 4, 2021

alonSadan commented Mar 4, 2021

EdDev commented Mar 4, 2021 • edited Loading

alonSadan commented Mar 4, 2021 • edited Loading

EdDev commented Mar 4, 2021 • edited Loading

EdDev left a comment

Choose a reason for hiding this comment

alonSadan commented Mar 4, 2021 • edited Loading

phoracek left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Mar 5, 2021

kubevirt-commenter-bot commented Mar 5, 2021

kubevirt-commenter-bot commented Mar 6, 2021

kubevirt-commenter-bot commented Mar 7, 2021

kubevirt-commenter-bot commented Mar 8, 2021

alonSadan commented Mar 8, 2021

alonSadan commented Feb 24, 2021 •

edited

Loading

alonSadan commented Feb 25, 2021 •

edited

Loading

oshoval left a comment •

edited

Loading

EdDev commented Mar 4, 2021 •

edited

Loading

alonSadan commented Mar 4, 2021 •

edited

Loading

EdDev commented Mar 4, 2021 •

edited

Loading

alonSadan commented Mar 4, 2021 •

edited

Loading