CNF-13652: Test integration of SRIOV and NMState operators #1990

zeeke · 2024-08-08T07:17:50Z

This changes implements a simple test to verify the NMState operator does not interfere with the SR-IOV network operator.

openshift-ci-robot · 2024-08-08T07:17:53Z

@zeeke: This pull request references CNF-13652 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

This changes implements a simple test to verify the NMState operator does not interfere with the SR-IOV network operator.

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 openshift-eng/jira-lifecycle-plugin repository.

zeeke · 2024-08-27T17:00:46Z

/test ?

openshift-ci · 2024-08-27T17:00:50Z

@zeeke: The following commands are available to trigger required jobs:

/test ci
/test e2e-aws-ci-tests
/test images
/test ztp-ci

The following commands are available to trigger optional jobs:

/test e2e-aws-ran-profile
/test e2e-telco5g-cnftests
/test e2e-telco5g-hcp-cnftests
/test e2e-telco5g-sno-cnftests
/test security

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-kni-cnf-features-deploy-master-ci
pull-ci-openshift-kni-cnf-features-deploy-master-e2e-aws-ci-tests
pull-ci-openshift-kni-cnf-features-deploy-master-e2e-aws-ran-profile
pull-ci-openshift-kni-cnf-features-deploy-master-images
pull-ci-openshift-kni-cnf-features-deploy-master-security

In response to this:

/test ?

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-sigs/prow repository.

zeeke · 2024-08-27T17:04:16Z

/test e2e-telco5g-cnftests

zeeke · 2024-08-29T10:39:45Z

/test e2e-telco5g-cnftests

zeeke · 2024-08-29T12:57:16Z

/test e2e-telco5g-cnftests

zeeke · 2024-08-30T05:11:40Z

/test e2e-telco5g-cnftests

karampok · 2024-08-30T08:28:50Z

cnf-tests/testsuites/e2esuite/nmstate/nmstate_sriov.go

+func waitForNMStatePolicyToBeStable(nmstatePolicy *nmstatev1.NodeNetworkConfigurationPolicy) {
+	key := runtimeclient.ObjectKey{Name: nmstatePolicy.Name}
+	Eventually(func(g Gomega) {
+		err := client.Client.Get(context.Background(), key, nmstatePolicy)


the nmstatePolicy is using the arg var, which creates confusion about the scope of the variables. Maybe have a new one.

Help functions imo are easier to read/be refactored if they are not having many assertions. I always suggest the assertion to be in the main block and have function that return error/true.

the nmstatePolicy is using the arg var, which creates confusion about the scope of the variables. Maybe have a new one.

good point. And since the function only use the name of the policy, I refactor it to pass a string only.

Help functions imo are easier to read/be refactored if they are not having many assertions. I always suggest the assertion to be in the main block and have function that return error/true.

I'm not 100% with it: helper functions must be short (<20 lines, as a rule of thumb) and that's where they get their readability. On the other side, the call site needs to be way more readable, as it contains the complicated test logic. With the current implementation, the call site reads:

waitForNMStatePolicyToBeStable(nodeNetworkConfigPolicy.Name)

which reads straightforward to me.
With functions like these, we might lose reusability; a function that waits for different conditions is almost copy/paste, but that's another story.

karampok · 2024-08-30T08:30:57Z

cnf-tests/testsuites/e2esuite/nmstate/nmstate_sriov.go

+			testDevice := devices[0]
+			By("Using device " + testDevice.Name)
+
+			By("Configuring IPAM on Physical Function via NMState")


Is that different than "Configuring and IPv4 address"? just IPAM (Address Management) makes me think whereabouts/or other IPAM is being deployed

IPv4 is definitely more clear! changing

karampok · 2024-08-30T08:38:01Z

cnf-tests/testsuites/e2esuite/nmstate/nmstate_sriov.go

+	sriovclient = sriovtestclient.New("")
+}
+
+var _ = Describe("[nmstate] SR-IOV Network Operator Integration", Ordered, func() {


Why the ordered is needed?

To use the BeforeAll section.
https://onsi.github.io/ginkgo/#setup-in-ordered-containers-beforeall-and-afterall

The question might be, why did I used a BeforeAll if there is only one test case? The answer is "I hope to add more test cases in the future". It might be an early optimization, WDYT?

Pretty sure you can use BeforeEach, AfterEach stacked on top of Describe/Context to achieve what you need in the future.

According to the doc

Ginkgo did not include support for Ordered containers for quite some time. As you can see Ordered containers make it possible to circumvent the "Declare in container nodes, initialize in setup nodes" principle; and they make it possible to write dependent specs This comes at a cost, of course - specs in Ordered containers cannot be fully parallelized which can result in slower suite runtimes. Despite these cons, pragmatism prevailed and Ordered containers were introduced in response to real-world needs in the community. Nonetheless, we recommend using Ordered containers only when needed.

So I would say specs(tests) should be not ordered unless they really really need to be.

okay, let's go for the BeforeEach way

karampok · 2024-08-30T08:44:38Z

cnf-tests/testsuites/e2esuite/nmstate/nmstate_sriov.go

+				resNum := testedNode.Status.Allocatable[corev1.ResourceName("openshift.io/testnmstateresource")]
+				capacity, _ := resNum.AsInt64()
+				return capacity
+			}, 10*time.Minute, time.Second).Should(Equal(int64(10)))


So the end spec is " there should be 10 VRFs"?

I suggest to extend the description that appears if the spec fails, e.g.

Should(Equal(int64(10))," the number of VFs observes is not that same as the one sriov created") ``

sounds good. changing

cnf-tests/testsuites/e2esuite/nmstate/nmstate_sriov.go

karampok · 2024-08-30T08:46:55Z

cnf-tests/testsuites/e2esuite/nmstate/nmstate_sriov.go

+})
+
+func findNonPrimarySriovDevicesAndNode(n *cluster.EnabledNodes) (string, []*sriovv1.InterfaceExt, error) {
+	errs := []error{}


You could use this lib https://pkg.go.dev/errors to wrap errors

I'm using that package at the end of the function:

if len(retDevices) == 0 { return "", nil, fmt.Errorf("can't find any SR-IOV devices in cluster's nodes: %w", errors.Join(errs...)) }

How would you use it with Wrap(...)?

I think you can avoid the []array

func main() { var err error if false{ err1 := errors.New("err1") err = errors.Join(err, err1) err2 := errors.New("err2") err = errors.Join(err, err2) } fmt.Println(err) }

but to be honest, both works

going for the errors.Join(...) approach

karampok · 2024-08-30T08:54:02Z

cnf-tests/testsuites/e2esuite/nmstate/nmstate_sriov.go

+
+		if len(nonPrimaryDevices) > len(retDevices) {
+			retNode = node
+			retDevices = nonPrimaryDevices


Can you just return here in the happy path, and return the wrap errors. I might be missing if there is selection between two nodes that have sriov or not

good point: this function is not very clear. The point is to find the node with the most number of SR-IOV devices. Going to reword the variables and comment to make it clearer.

karampok · 2024-08-30T08:59:25Z

feature-configs/deploy/nmstate/operator_namespace.yaml

+  name: openshift-nmstate
+spec:
+  finalizers:
+  - kubernetes


[nit] my yamllint is complaining that it need --- and two spaces here (same for all yamls)

Let's make the linter happy 😉

zeeke · 2024-09-04T08:31:22Z

/test e2e-telco5g-cnftests

karampok · 2024-09-04T10:40:22Z

feature-configs/ci/nmstate/operator_subscription.yaml

+  name: kubernetes-nmstate-operator
+  source: art-nightly-operator-catalog
+  sourceNamespace: openshift-marketplace
+---


the --- goes to the top

yamllint feature-configs/ci/nmstate/kustomization.yaml feature-configs/ci/nmstate/kustomization.yaml 1:1 warning missing document start "---" (document-start)

just noticed all the yaml under feature-configs does not have --- . Lets make it as all the other and live with that until we decide to put yamllint in the CI. Does it sound good?

zeeke · 2024-09-04T16:13:00Z

/test e2e-telco5g-cnftests

zeeke · 2024-09-04T21:43:54Z

/retest
/test e2e-telco5g-cnftests

zeeke · 2024-10-07T12:43:43Z

/retest
/test e2e-telco5g-cnftests

fedepaol · 2024-10-09T14:03:55Z

cnf-tests/testsuites/e2esuite/knmstate/knmstate_sriov.go

+			Expect(err).ToNot(HaveOccurred())
+			DeferCleanup(sriovclient.Delete, context.Background(), sriovNetworkNodePolicy)
+
+			By("Verify Virtual Functions have been correctly configured")


nit: verifying that (same below)

zeeke · 2024-10-11T14:15:00Z

/test e2e-telco5g-cnftests

zeeke · 2024-10-11T16:47:29Z

/retest

zeeke · 2024-10-14T05:21:31Z

/test e2e-telco5g-cnftests
/retest

zeeke · 2024-10-15T15:06:58Z

/test e2e-telco5g-cnftests

zeeke · 2024-10-16T07:24:47Z

/retest

zeeke · 2024-10-16T07:25:20Z

/test e2e-telco5g-cnftests

zeeke · 2024-10-16T13:06:47Z

/retest

Signed-off-by: Andrea Panattoni <[email protected]>

Add a test that configures the same NIC via Kubernetes-NMState and SR-IOV network operator. The test ensure that if NMState doesn't have any SR-IOV configuration, then both operator should work correctly. Signed-off-by: Andrea Panattoni <[email protected]>

`kubernetes-nmsate` CI does not publish images on `quay.io/openshift/*`. The best way to deploy a recent version of the operator is to leverage the `quay.io/openshift-release-dev/ocp-release-nightly:iib-int-index-art-operators-4.17` index image, which picks latest nightly builds from Brew. Signed-off-by: Andrea Panattoni <[email protected]>

zeeke · 2024-10-23T09:59:19Z

/test e2e-telco5g-cnftests

openshift-ci · 2024-10-23T12:35:55Z

@zeeke: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-telco5g-cnftests	`a3faba8`	link	false	`/test e2e-telco5g-cnftests`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

zeeke · 2024-10-23T12:53:41Z

Failing tests

CNF Features e2e integration tests: [It] [bondcni] bond over macvlan should be able to create pod with bond interface over macvlan interfaces expand_more	1m4s
CNF Features e2e integration tests: [It] [tuningcni] tuningcni over macvlan should be able to create pod with sysctls over macvlan expand_more

are known bugs. See https://issues.redhat.com/browse/CNF-14296

fedepaol · 2024-10-23T13:01:49Z

/lgtm

zeeke · 2024-10-23T13:10:53Z

/override ci/prow/e2e-aws-ci-tests

openshift-ci · 2024-10-23T13:11:24Z

@zeeke: Overrode contexts on behalf of zeeke: ci/prow/e2e-aws-ci-tests

In response to this:

/override ci/prow/e2e-aws-ci-tests

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-sigs/prow repository.

Run `sriov/knmstate` combined tests defined in - openshift-kni/cnf-features-deploy#1990 as part of the cnf-tests suite. Signed-off-by: Andrea Panattoni <[email protected]>

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 8, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2024

openshift-ci bot requested review from kenyis and swatisehgal August 8, 2024 07:18

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

zeeke force-pushed the CNF-13652 branch from 4cb4eb5 to 3586df3 Compare August 27, 2024 12:51

zeeke force-pushed the CNF-13652 branch from 434118f to 5de2279 Compare August 27, 2024 17:03

zeeke force-pushed the CNF-13652 branch from 5de2279 to 329cb11 Compare August 28, 2024 07:37

zeeke force-pushed the CNF-13652 branch 2 times, most recently from 7692f80 to bcb630c Compare August 29, 2024 11:25

zeeke changed the title ~~[WIP] CNF-13652: Test integration of SRIOV and NMState operators~~ CNF-13652: Test integration of SRIOV and NMState operators Aug 29, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2024

zeeke force-pushed the CNF-13652 branch from bf832f9 to 5ce650d Compare August 30, 2024 07:06

karampok reviewed Aug 30, 2024

View reviewed changes

zeeke force-pushed the CNF-13652 branch 3 times, most recently from b7f7b62 to cd0875f Compare September 4, 2024 08:31

karampok reviewed Sep 4, 2024

View reviewed changes

zeeke force-pushed the CNF-13652 branch 2 times, most recently from 6b21f40 to f81a28a Compare September 4, 2024 16:12

fedepaol reviewed Oct 9, 2024

View reviewed changes

zeeke force-pushed the CNF-13652 branch from fc6416c to 8ffa480 Compare October 11, 2024 14:14

zeeke force-pushed the CNF-13652 branch from 8ffa480 to a3faba8 Compare October 15, 2024 15:06

zeeke force-pushed the CNF-13652 branch 3 times, most recently from 602c277 to 1047a6a Compare October 23, 2024 09:09

zeeke added 4 commits October 23, 2024 11:58

features: Add knmstate feature

3331320

Signed-off-by: Andrea Panattoni <[email protected]>

cnf-tests: add nmstate api dependency

0ec90c5

Signed-off-by: Andrea Panattoni <[email protected]>

zeeke force-pushed the CNF-13652 branch from 1047a6a to 99e4893 Compare October 23, 2024 09:59

openshift-ci bot assigned fedepaol Oct 23, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2024

openshift-merge-bot bot merged commit 872a4ce into openshift-kni:master Oct 23, 2024
6 of 7 checks passed

zeeke mentioned this pull request Oct 24, 2024

telco5g: Run knmstate cnf-tests openshift/release#58127

Open

CNF-13652: Test integration of SRIOV and NMState operators #1990

CNF-13652: Test integration of SRIOV and NMState operators #1990

Conversation

zeeke commented Aug 8, 2024

openshift-ci-robot commented Aug 8, 2024

zeeke commented Aug 27, 2024

openshift-ci bot commented Aug 27, 2024

zeeke commented Aug 27, 2024

zeeke commented Aug 29, 2024

zeeke commented Aug 29, 2024

zeeke commented Aug 30, 2024

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

Choose a reason for hiding this comment

zeeke commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeeke commented Sep 4, 2024

zeeke commented Sep 4, 2024

zeeke commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeeke commented Oct 11, 2024

zeeke commented Oct 11, 2024

zeeke commented Oct 14, 2024

zeeke commented Oct 15, 2024

zeeke commented Oct 16, 2024

zeeke commented Oct 16, 2024

zeeke commented Oct 16, 2024

zeeke commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024 • edited Loading

zeeke commented Oct 23, 2024

fedepaol commented Oct 23, 2024

zeeke commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024

openshift-ci bot commented Oct 23, 2024 •

edited

Loading