e2e, persistent-ip: Add primary-UDN tests #72

RamLavi · 2024-10-01T12:00:41Z

What this PR does / why we need it:

Since the same test portfolio is run for both secondary/primary UDN, we could either copy the entire tests for primary UDN (making a lot of duplicate code), OR try to join them. This PR is choosing the latter, joining both options (secondary and primary UDNs) to the same test suite.

The main change between the two is the way both tests retrieve the IPs from the VMI:
For secondary: retrieve the IPs from the VMI.Status
For primary UDN: retrieve the IPs from the pod's network-status annotation (this may change in future iterations).

This PR:

primes the e2e/persistentips_test.go for introduction of the primary-udn test cases.
- generalizes the NAD creation using the role parameter.
- generalizes the "getIP" function to work on both cases (secondary/primary UDN).
- simplifies the NAD name to be static, to simplify case complexity.
- generalizes the "generate" VMI function to work on both cases (secondary/primary UDN).
adds the primary-udn test cases.

With this change, we now cover ipam-extentions operation on VMI/VMs using primary-UDN.

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 #

Special notes for your reviewer:

Release note:

NONE

This will enable the network-segmentation feature on OVNK, needed in order to run VM workloads with primary UDN. Signed-off-by: Ram Lavi <[email protected]>

RamLavi · 2024-10-01T12:32:45Z

@qinqon I eventually did not parametize the generateVM function, as it also needs the nad.Name which is not static (randomized)... so this is the only place where we use If(role ==...). I can live with that, tell me what you think though.

qinqon

just nits

qinqon · 2024-10-02T13:08:32Z

test/e2e/persistentips_test.go

@@ -40,6 +40,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )

+const networkInterfaceName = "multus"


this is both the VMI interface/network logical name and the field at VMI status to look for so and is just for secondaries so let's name this
secondaryLogicalNetworkInterfaceName

qinqon · 2024-10-02T13:09:10Z

test/e2e/persistentips_test.go

-			vmi                  *kubevirtv1.VirtualMachineInstance
-			nad                  *nadv1.NetworkAttachmentDefinition
+			td         testenv.TestData
+			getIPsFunc func(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error)


Let's call this ipsFrom

qinqon · 2024-10-02T13:11:58Z

test/e2e/persistentips_test.go

@@ -312,3 +317,7 @@ func removeFinalizersPatch() ([]byte, error) {
 	}
 	return json.Marshal(patch)
 }
+
+func getIPsFromVMIStatus(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error) {


this is just for secondaries since it passes the secondary network interface name let's call this secondaryNetworkVMIStatusIPs

so we have

ipsFrom: secondaryNetworkVMIStatusIPs

qinqon · 2024-10-02T13:13:01Z

test/e2e/persistentips_test.go

+
+type testParams struct {
+	role       string
+	getIPsFunc func(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error)


let's rename this to ipsFrom

maiqueb

Thank you.

maiqueb · 2024-10-02T13:18:19Z

hack/cluster.sh

@@ -3,7 +3,7 @@
 set -xe
 SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"

-KIND_ARGS="${KIND_ARGS:--ic -ikv -i6 -mne}"
+KIND_ARGS="${KIND_ARGS:--ic -ikv -i6 -mne -nse}"


do we want this on all lanes, or we want to add an additional lane, and test it there ?

I'm inclined to say we want the latter.

hmm.. regardless of the question whether we want to run the tests in a parallel lane, I don't think we should only enable it when we want to test it.
It's a feature that is planned to be enabled by default, so I don't see a good reason why not to add it for all the cluster lanes.

i also dont see why to add another lane tbh, we should not over test

OK, let's roll with it.

maiqueb · 2024-10-02T13:21:44Z

test/e2e/persistentips_test.go

-
-				Expect(vmi.Status.Interfaces).NotTo(BeEmpty())
-				Expect(vmi.Status.Interfaces[0].IPs).NotTo(BeEmpty())
+				Expect(testenv.ThisVMI(vmi)()).Should(testenv.MatchIPs(params.getIPsFunc, Not(BeEmpty())))


I honestly fail to understand how is this simpler than what we had before.

It's not simpler. It's adding another dimension in order to accommodate the primary UDN tests..

maiqueb · 2024-10-02T13:25:15Z

test/e2e/persistentips_test.go

@@ -112,8 +128,7 @@ var _ = Describe("Persistent IPs", func() {
 					WithTimeout(5 * time.Minute).
 					Should(testenv.ContainConditionVMIReady())

-				Expect(testenv.ThisVMI(vmi)()).Should(testenv.MatchIPsAtInterfaceByName(networkInterfaceName, ConsistOf(vmiIPsBeforeMigration)))
-
+				Expect(testenv.ThisVMI(vmi)()).Should(testenv.MatchIPs(params.getIPsFunc, ConsistOf(vmiIPsBeforeMigration)))


looking at this now, I think the condition was wrong before, and you're taking it a step further in the wrong direction.

I would expect to see something like:

Expect(testenv.VMIPs(vmi)()).To(ConsistOf(vmiIPsBeforeMigration))

meaning, IMHO the IP extraction should go in the expect body, rather than in a transformation applied in the matcher.

I think the reason this convoluted way of checking (original code):

Expect(testenv.ThisVMI(vmi)()).Should(testenv.MatchIPs(getIPsFunc, ConsistOf(vmiIPsBeforeMigration)))

is done so that we won't have to re-get the VMI instance.
Changing it to what you want needs to look like this:

Expect(testenv.Client.Get(context.Background(), client.ObjectKeyFromObject(vmi), vmi)).To(Succeed()) Expect(testenv.VMIPs(vmi)()).To(ConsistOf(vmiIPsBeforeMigration))

otherwise the VMI object in the test is not refreshed with the post-migration entity.

would you like this PR to include this refactorment? I kinda feel it should have a separate PR (which is why I didn't refactor it here) - but I don't mind doing it on this PR.

OK, I hate how this looks now, but this can be addressed in a follow up.

Just saying: this looks a lot more complex than it should be.

Hiding re-fetching the VM behind a function so I can save an eventually, is also not good IMHO.

+1. Will tackle in follow up PR

maiqueb · 2024-10-02T13:26:06Z

test/e2e/persistentips_test.go

-
-				Expect(vmi.Status.Interfaces).NotTo(BeEmpty())
-				Expect(vmi.Status.Interfaces[0].IPs).NotTo(BeEmpty())
+				IPs, err := params.getIPsFunc(vmi)


nit: ips, not IPs.

maiqueb · 2024-10-02T13:27:53Z

test/e2e/persistentips_test.go

@@ -311,3 +339,7 @@ func removeFinalizersPatch() ([]byte, error) {
 	}
 	return json.Marshal(patch)
 }
+
+func getIPsFromVMIStatus(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error) {


I don't get this function. All it does in wrap another function, and IIUC it can't error.

Hence, why do you have an error in the func's signature ?

Since the same test portfolio is run for both secondary/primary UDN, we could either copy the entire tests for primary UDN (making a lot of duplicate code), OR try to join them. The main change is the way both tests retrieve the IPs from the VMI:
For secondary: retrieve the IPs from the VMI.Status
For primary UDN: retrieve the IPs from the pod's network-status annotation.

So right now you're right - it's seems like a unneeded generalization of the function.
But what we eventually need is a general ipsFrom function, that will satisfy both options.

BTW once the IPs could be retrieved from the VMI.status also for primary UDNs then we could remove this complexity.
I will adjust the commit message to better explain this.

I still don't get it.

Why does the function's signature return an error ?

Will a follow up commit change this to throw an error ? If so, why don't you update the function's signature to return an error when it is needed ?

ACK, I'll introduce the error return param in the relevant commit.

DONE. It's now in a separate commit. PTAL

maiqueb · 2024-10-02T13:31:16Z

test/env/getter.go

+	err := Client.List(context.Background(), pods,
+		client.InNamespace(namespace),
+		client.MatchingLabels(labelSelector),
+		client.MatchingFields(fieldSelector))


let's be consistent; either everything in one line, or one item per line.

Suggested change

err := Client.List(context.Background(), pods,

client.InNamespace(namespace),

client.MatchingLabels(labelSelector),

client.MatchingFields(fieldSelector))

err := Client.List(

context.Background(),

pods,

client.InNamespace(namespace),

client.MatchingLabels(labelSelector),

client.MatchingFields(fieldSelector),

)

maiqueb · 2024-10-02T13:31:53Z

test/env/getter.go

+	}
+
+	if len(pods.Items) == 0 {
+		return nil, fmt.Errorf("failed to lookup pod")


maybe consider printing the selectors.

maiqueb · 2024-10-02T13:35:08Z

test/env/getter.go

+	if node := vmi.Status.NodeName; node != "" {
+		const nodeName = "spec.nodeName"
+		fieldSelectors[nodeName] = node
+	}


is this needed ? Trying to wrap my head around how could we reach this sorry state, and I just can't find it. Minus KubeVirt bugs of course.

I thought it was necessary because it's migration and we can two running pods, just on different nodes, but since we're also using the vmi label selector using the VMI.UID, then I guess it's redundant.
I don't know of any kubevirt bug it is meant to cover, so removing.

maiqueb · 2024-10-02T13:36:38Z

test/env/getter.go

+	return netStatus, nil
+}
+
+func getDefaultNetworkStatus(vmi *kubevirtv1.VirtualMachineInstance) (*nadv1.NetworkStatus, error) {


Do you mean cluster default network here ? In primary UDN we can have have a different primary network than the cluster default network.

Also, please drop the get.

I mean the network status with the default: true in it.
Removed the get

maiqueb · 2024-10-02T13:36:55Z

test/env/getter.go

+	return nil, fmt.Errorf("primary IPs not found")
+}
+
+func GetIPsFromNetworkStatusAnnotation(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error) {


Suggested change

func GetIPsFromNetworkStatusAnnotation(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error) {

func IPsFromNetworkStatusAnnotation(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error) {

RamLavi · 2024-10-07T07:15:17Z

Change: Address @qinqon + @maiqueb reviews

qinqon

nits on naming.

qinqon · 2024-10-07T07:21:56Z

test/env/getter.go

+func IPsFromNetworkStatusAnnotation(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error) {
+	defNetworkStatus, err := defaultNetworkStatus(vmi)
+	if err != nil {
+		return nil, err
+	}
+
+	return defNetworkStatus.IPs, nil
+}


Move this function to the test, and call it defaultNetworkStatusAnnotationIPs so the test reads

ipsFrom: defaultNetworkStatusAnnotationIPs

qinqon · 2024-10-07T08:28:26Z

@RamLavi I would like to remove the other "if role == secondary" from the test by exposing the VMI build functios to the test struct, I know we say we were not going to do so, but it do not feel right, what do you think ?

RamLavi · 2024-10-07T10:22:14Z

@RamLavi I would like to remove the other "if role == secondary" from the test by exposing the VMI build functios to the test struct, I know we say we were not going to do so, but it do not feel right, what do you think ?

I'm game, I'll create a NewVMI function with options..

Currently GenerateLayer2WithSubnetNAD is creating NADs for secondary roles (which is the default). Add role input param to the function in order to support primary role that will be used in future commits. Signed-off-by: Ram Lavi <[email protected]>

RamLavi · 2024-10-07T11:51:32Z

@RamLavi I would like to remove the other "if role == secondary" from the test by exposing the VMI build functios to the test struct, I know we say we were not going to do so, but it do not feel right, what do you think ?

DONE

qinqon

Another nit I forgot.

test/e2e/persistentips_test.go

qinqon · 2024-10-08T05:57:41Z

/lgtm

maiqueb

Thanks for the commit split. It makes it easier to review.

We're nearly there.

We can tackle the matchers in a follow-up PR.

maiqueb · 2024-10-08T16:09:05Z

hack/cluster.sh

@@ -3,7 +3,7 @@
 set -xe
 SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"

-KIND_ARGS="${KIND_ARGS:--ic -ikv -i6 -mne}"
+KIND_ARGS="${KIND_ARGS:--ic -ikv -i6 -mne -nse}"


OK, let's roll with it.

test/e2e/persistentips_test.go

maiqueb · 2024-10-08T17:19:26Z

test/e2e/persistentips_test.go

-
-				Expect(vmi.Status.Interfaces).NotTo(BeEmpty())
-				Expect(vmi.Status.Interfaces[0].IPs).NotTo(BeEmpty())
+				Expect(testenv.ThisVMI(vmi)()).Should(testenv.MatchIPs(ipsFrom, Not(BeEmpty())))


wouldn't this be the same as

Expect(...).NotTo(testenv.MatchIPs(ipsFrom, BeEmpty()))

I think so, but both options seem equally convoluted. Let's tackle it in a follow up PR as you suggest

test/e2e/persistentips_test.go

maiqueb · 2024-10-08T17:23:08Z

test/e2e/persistentips_test.go

@@ -311,3 +339,7 @@ func removeFinalizersPatch() ([]byte, error) {
 	}
 	return json.Marshal(patch)
 }
+
+func getIPsFromVMIStatus(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error) {


I still don't get it.

Why does the function's signature return an error ?

Will a follow up commit change this to throw an error ? If so, why don't you update the function's signature to return an error when it is needed ?

maiqueb · 2024-10-08T17:24:27Z

test/e2e/persistentips_test.go

@@ -112,8 +128,7 @@ var _ = Describe("Persistent IPs", func() {
 					WithTimeout(5 * time.Minute).
 					Should(testenv.ContainConditionVMIReady())

-				Expect(testenv.ThisVMI(vmi)()).Should(testenv.MatchIPsAtInterfaceByName(networkInterfaceName, ConsistOf(vmiIPsBeforeMigration)))
-
+				Expect(testenv.ThisVMI(vmi)()).Should(testenv.MatchIPs(params.getIPsFunc, ConsistOf(vmiIPsBeforeMigration)))


OK, I hate how this looks now, but this can be addressed in a follow up.

Just saying: this looks a lot more complex than it should be.

Hiding re-fetching the VM behind a function so I can save an eventually, is also not good IMHO.

In the current tests (where we only run persistent ip tests for secondary interfaces), the ips of the VMI is extracted via vmi.status. This is done both directly and via the matchers in testenv library. In order for the test suite to support running the same tests on primary UDN VMs, this function needs to be generalized in the following ways: 1. it should align all the IP retrievals to use this new function (right now the IPs are retrieved both directly accessing vmi.status and via a function). 2. it should move the function retrieving the IPs to a first-class functions variable, so that the way IPS are retrieved can be generalized. 3. it should only have the common parameters needed to get the IPs (which is the VMI object). Moving get the IP extraction to a general function, so that it could be consumed uniformly. Doing this will allow to more easily retrieve the IP in other cases such as primary UDN, where the IP is currently retrieved in another way. This will be added in future commits. Signed-off-by: Ram Lavi <[email protected]>

In the current tests (where we only run persistent ip tests for secondary interfaces), the VMI is created using the GenerateAlpineWithMultusVMI function. In order for the test suite to support running the same tests on primary UDN VMIs, refactoring the function to use VMI factory function. Moreover, the NAD Name is changed to be static, as it allows it to be consumed before runtime, as a test parameter for both the NAD and VMI creation. This shouldn't interfere with the tests operations as the NADs are created in different namespaces. Signed-off-by: Ram Lavi <[email protected]>

Expand the current test to run in a DescribeTableSubtree context. This is in preparation of adding more interface types in future commits. This commit does not add tests, nor change their operation. Signed-off-by: Ram Lavi <[email protected]>

RamLavi · 2024-10-08T19:18:16Z

Change: Fixed reviews. @maiqueb PTAL

Introducing the functions needed in order to run the current test suite for primary UDN interfaces: - defaultNetworkStatusAnnotationIPs - as the getIP function for primary UDN. - vmiWithPasst - as the vmi Generating function for primary UDN. These will be introduced as parameter functions when primary-UDN Entry is introduced in future commit. Moreover, since the function getting the IPs for primary-UDN case needs to return error, this is added to the general function signature. Signed-off-by: Ram Lavi <[email protected]>

Add a new test subtree entry for primary UDN interfaces, checking persistentIPs on workload with primary UDN. Signed-off-by: Ram Lavi <[email protected]>

RamLavi · 2024-10-08T19:20:48Z

Change: fixed dco issue

maiqueb

/lgtm
/approve
/hold

@qinqon please remove the hold if you're OK with this PR.

Thanks @RamLavi for adding this test.

kubevirt-bot · 2024-10-08T20:19:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maiqueb

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:

~~OWNERS~~ [maiqueb]

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

qinqon · 2024-10-08T21:04:40Z

/hold cancel

oshoval · 2024-10-14T10:08:25Z

test/e2e/persistentips_test.go

+version: 2
+ethernets:
+  eth0:
+    dhcp4: true`


btw this is not required as far as i remember for our machines right ?
going to try and drop it during the effort to convert to managedTap

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Oct 1, 2024

kubevirt-bot requested review from maiqueb and oshoval October 1, 2024 12:00

RamLavi mentioned this pull request Oct 1, 2024

e2e, persistent-ip: Add primary-UDN tests #71

Closed

kubevirt-bot added the size/L label Oct 1, 2024

cluster-up: Add nse flag to OVNK kind script

35c8704

This will enable the network-segmentation feature on OVNK, needed in order to run VM workloads with primary UDN. Signed-off-by: Ram Lavi <[email protected]>

RamLavi force-pushed the udn_e2e_describe branch from cf5b99e to 932079d Compare October 1, 2024 12:09

qinqon requested changes Oct 2, 2024

View reviewed changes

kubevirt-bot assigned qinqon Oct 2, 2024

maiqueb requested changes Oct 2, 2024

View reviewed changes

kubevirt-bot assigned maiqueb Oct 2, 2024

RamLavi force-pushed the udn_e2e_describe branch from 932079d to c2d54f7 Compare October 7, 2024 07:14

qinqon requested changes Oct 7, 2024

View reviewed changes

RamLavi force-pushed the udn_e2e_describe branch from c2d54f7 to 169064a Compare October 7, 2024 08:17

RamLavi force-pushed the udn_e2e_describe branch from 169064a to e075b19 Compare October 7, 2024 11:50

qinqon requested changes Oct 7, 2024

View reviewed changes

test/e2e/persistentips_test.go Show resolved Hide resolved

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2024

maiqueb requested changes Oct 8, 2024

View reviewed changes

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2024

RamLavi added 3 commits October 8, 2024 21:02

RamLavi force-pushed the udn_e2e_describe branch from e075b19 to 5461a44 Compare October 8, 2024 19:13

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Oct 8, 2024

RamLavi added 2 commits October 8, 2024 22:19

tests/e2e, persistentip: Add primary UDN test case

a873dfb

Add a new test subtree entry for primary UDN interfaces, checking persistentIPs on workload with primary UDN. Signed-off-by: Ram Lavi <[email protected]>

RamLavi force-pushed the udn_e2e_describe branch from 5461a44 to a873dfb Compare October 8, 2024 19:20

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Oct 8, 2024

maiqueb approved these changes Oct 8, 2024

View reviewed changes

kubevirt-bot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 8, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2024

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2024

kubevirt-bot merged commit 925bee6 into kubevirt:main Oct 8, 2024
4 checks passed

oshoval reviewed Oct 14, 2024

View reviewed changes

	func GetIPsFromNetworkStatusAnnotation(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error) {
	func IPsFromNetworkStatusAnnotation(vmi *kubevirtv1.VirtualMachineInstance) ([]string, error) {

e2e, persistent-ip: Add primary-UDN tests #72

e2e, persistent-ip: Add primary-UDN tests #72

Conversation

RamLavi commented Oct 1, 2024 • edited Loading

RamLavi commented Oct 1, 2024

qinqon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamLavi Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamLavi Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamLavi commented Oct 7, 2024

qinqon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinqon commented Oct 7, 2024

RamLavi commented Oct 7, 2024

RamLavi commented Oct 7, 2024

qinqon left a comment

Choose a reason for hiding this comment

qinqon commented Oct 8, 2024

maiqueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamLavi commented Oct 8, 2024

RamLavi commented Oct 8, 2024

maiqueb left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Oct 8, 2024

qinqon commented Oct 8, 2024

Choose a reason for hiding this comment

RamLavi commented Oct 1, 2024 •

edited

Loading

RamLavi Oct 8, 2024 •

edited

Loading

RamLavi Oct 7, 2024 •

edited

Loading