NO-JIRA: e2e:sriov: deploy sriov devices as part of config suite #981

Tal-or · 2024-03-04T16:10:18Z

As part of test coverage enhancement, we want to integrate
the performance tests (functests) on a cluster which has sriov devices.

For that we want to detect whether the cluster supports sriov and act as follow:
If it's not supported or the test failed to create sriov devices, skips the setup and run test as usual.
This meant to keep the existing behavior intact.

If the cluster supports sriov, create some devices and run all the tests.

Signed-off-by: Talor Itzhak <[email protected]>

As part of test coverage enhancement, we want to integrate the performance tests (functests) on a cluster which has sriov devices. For that we want to detect whether the cluster supports sriov and act as follow: If it's not supported or the test failed to create sriov devices, skips the setup and run test as usual. This meant to keep the existing behavior intact. If the cluster supports sriov, create some devices and run all the tests. Signed-off-by: Talor Itzhak <[email protected]>

We can control the run of the SRI-OV setup using ginkgo labels. Signed-off-by: Talor Itzhak <[email protected]>

openshift-ci-robot · 2024-03-04T16:10:24Z

@Tal-or: This pull request explicitly references no jira issue.

In response to this:

As part of test coverage enhancement, we want to integrate
the performance tests (functests) on a cluster which has sriov devices.

For that we want to detect whether the cluster supports sriov and act as follow:
If it's not supported or the test failed to create sriov devices, skips the setup and run test as usual.
This meant to keep the existing behavior intact.

If the cluster supports sriov, create some devices and run all the 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 openshift-eng/jira-lifecycle-plugin repository.

Tal-or · 2024-03-04T16:11:18Z

/cc @SchSeba PTAL

openshift-ci · 2024-03-04T16:12:49Z

@Tal-or: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @SchSeba PTAL

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.

Tal-or · 2024-03-04T16:19:00Z

/test e2e-telco5g-cnftests

ffromani

I'm still not sold about setting up SR-IOV devices inside the tests (vs in the lane setup stage)

ffromani · 2024-03-05T05:31:56Z

go.mod

+	github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.1.1-0.20201119153432-9d213757d22d
+	github.com/k8snetworkplumbingwg/sriov-network-operator v1.2.0


can this be a issue when we integrate the tests into cnf-tests?

if you mean cnf-tests image, we do not integrate tests there anymore apart from the latency suite

ffromani · 2024-03-05T05:33:30Z

test/e2e/performanceprofile/functests/0_config/config.go

@@ -134,6 +135,14 @@ var _ = Describe("[performance][config] Performance configuration", Ordered, fun
 		format.Object(performanceProfile, 2)
 	})

+	It("should create SRI-OV devices if they are exist", func() {


typo: SR-IOV (here and elsewhere): https://en.wikipedia.org/wiki/Single-root_input/output_virtualization

I'll keep forgetting 👍

ffromani · 2024-03-05T05:36:18Z

test/e2e/performanceprofile/functests/utils/sriov/sriov.go

+	Eventually(func() int64 {
+		testedNode, err := testclient.K8sClient.CoreV1().Nodes().Get(ctx, node, metav1.GetOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		resNum, _ := testedNode.Status.Allocatable[corev1.ResourceName("openshift.io/"+resourceName)]


ideally the discovery function (or one of its helper) should return the full resource name we need to consume. Here we add a component of guesswork

+1.
I believe the final resource name should be documented somewhere because there are tests that expects to be passed a value of the device name to be executed (example).
and later then we can use this common value in these tests.

ffromani · 2024-03-05T05:36:54Z

test/e2e/performanceprofile/functests/utils/sriov/sriov.go

+	createNetwork(sriovDevice, networkName, namespace, OperatorNamespace, resourceName, metaPluginsConfig)
+}
+
+func createNetwork(sriovDevice *sriovv1.InterfaceExt, sriovNetworkName, sriovNetworkNamespace, operatorNamespace, resourceName, metaPluginsConfig string) {


do we need a 1-line wrapper which basically adapts the function signature?

Copied this part verbatim, we can change it, no problem

shajmakh

Thank you for working on this!
I added few comments below.

shajmakh · 2024-03-05T07:31:02Z

test/e2e/performanceprofile/functests/utils/sriov/sriov.go

+
+func CreatePolicyAndNetwork(ctx context.Context, sriovInfos *sriovcluster.EnabledNodes, namespace, networkName, resourceName, metaPluginsConfig string) {
+	GinkgoHelper()
+	numVfs := 4


is there any reason for creating this specific number?
we have some tests that require 64 vfs, I believe it would be useful to configure this via env variable

Nope, I copied the function verbatim from cnf-test.
I don't care how much devices there are, even one is enough because all I wanted to check is that rps mask applies on these devices as well

We can definitely add an env variable to configure the number, but we need to make sure it doesn't get lost in the documentation :)

shajmakh · 2024-03-05T07:37:42Z

test/e2e/performanceprofile/functests/utils/sriov/sriov.go

+	Eventually(func() int64 {
+		testedNode, err := testclient.K8sClient.CoreV1().Nodes().Get(ctx, node, metav1.GetOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		resNum, _ := testedNode.Status.Allocatable[corev1.ResourceName("openshift.io/"+resourceName)]


+1.
I believe the final resource name should be documented somewhere because there are tests that expects to be passed a value of the device name to be executed (example).
and later then we can use this common value in these tests.

shajmakh · 2024-03-05T07:41:22Z

test/e2e/performanceprofile/functests/utils/client/clients.go

@@ -19,6 +19,7 @@ import (
 	olmv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"

+	sriovtestclient "github.com/k8snetworkplumbingwg/sriov-network-operator/test/util/client"


I guess this should be in the above import set?

shajmakh · 2024-03-05T07:43:23Z

test/e2e/performanceprofile/functests/utils/sriov/sriov.go

+	sriovk8sv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
+	sriovv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1"
+	sriovcluster "github.com/k8snetworkplumbingwg/sriov-network-operator/test/util/cluster"
+	sriovnetwork "github.com/k8snetworkplumbingwg/sriov-network-operator/test/util/network"
+	testutils "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils"
+	testclient "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/client"
+	"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/cluster"
+	testlog "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/log"
+	"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/nodes"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"


if we add empty lines it will help clarify the different imports parties that are used.

shajmakh · 2024-03-05T07:51:31Z

test/e2e/performanceprofile/functests/0_config/config.go

@@ -134,6 +135,14 @@ var _ = Describe("[performance][config] Performance configuration", Ordered, fun
 		format.Object(performanceProfile, 2)
 	})

+	It("should create SRI-OV devices if they are exist", func() {


It() creates the devices if they don't exist, right?

I meant if the cluster supports it. I'll change that, thanks

shajmakh · 2024-03-05T07:56:20Z

test/e2e/performanceprofile/functests/0_config/config.go

@@ -134,6 +135,14 @@ var _ = Describe("[performance][config] Performance configuration", Ordered, fun
 		format.Object(performanceProfile, 2)
 	})

+	It("should create SRI-OV devices if they are exist", func() {
+		sriovInfos, err := sriovcluster.DiscoverSriov(testclient.SRIOVClient, sriov.OperatorNamespace)


from what I undertand, devices related tests will run if the operator is installed on the sriov.OperatorNamespace, and installing the operator is not part of the tests setup.
this means we need to setup the operator installation in the d/s ci pipelines. We'll check this accordingly.

Yes. I also know that on cnf-test they are installing the operator before the test starts:
https://github.com/openshift-kni/cnf-features-deploy/blob/master/hack/setup-build-index-image.sh#L62-L66

Tal-or · 2024-03-05T09:44:46Z

/hold
PR design is in ongoing discussion

openshift-ci · 2024-03-21T13:10:50Z

@Tal-or: The following tests 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-aws-ovn-techpreview	`b0ecfad`	link	true	`/test e2e-aws-ovn-techpreview`
ci/prow/e2e-aws-operator	`b0ecfad`	link	true	`/test e2e-aws-operator`
ci/prow/e2e-telco5g-cnftests	`b0ecfad`	link	false	`/test e2e-telco5g-cnftests`
ci/prow/e2e-aws-ovn	`b0ecfad`	link	true	`/test e2e-aws-ovn`
ci/prow/e2e-gcp-pao	`b0ecfad`	link	true	`/test e2e-gcp-pao`
ci/prow/e2e-upgrade	`b0ecfad`	link	true	`/test e2e-upgrade`
ci/prow/vet	`b0ecfad`	link	true	`/test vet`
ci/prow/e2e-gcp-pao-workloadhints	`b0ecfad`	link	true	`/test e2e-gcp-pao-workloadhints`
ci/prow/e2e-hypershift	`b0ecfad`	link	true	`/test e2e-hypershift`
ci/prow/e2e-gcp-pao-updating-profile	`b0ecfad`	link	true	`/test e2e-gcp-pao-updating-profile`
ci/prow/e2e-hypershift-pao	`b0ecfad`	link	true	`/test e2e-hypershift-pao`

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

openshift-merge-robot · 2024-03-27T08:52:44Z

PR needs rebase.

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.

Tal-or · 2024-04-01T11:46:19Z

/close
superseded by openshift-kni/cnf-features-deploy#1856

openshift-ci · 2024-04-01T11:46:32Z

@Tal-or: Closed this PR.

In response to this:

/close
superseded by openshift-kni/cnf-features-deploy#1856

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.

Tal-or added 3 commits March 4, 2024 17:54

vendor: dependencies for SRI-OV devices creation

59da152

Signed-off-by: Talor Itzhak <[email protected]>

e2e:sriov: add a label for better control

b0ecfad

We can control the run of the SRI-OV setup using ginkgo labels. Signed-off-by: Talor Itzhak <[email protected]>

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

openshift-ci bot requested a review from SchSeba March 4, 2024 16:12

ffromani reviewed Mar 5, 2024

View reviewed changes

shajmakh reviewed Mar 5, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2024

openshift-ci bot closed this Apr 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NO-JIRA: e2e:sriov: deploy sriov devices as part of config suite #981

NO-JIRA: e2e:sriov: deploy sriov devices as part of config suite #981

Tal-or commented Mar 4, 2024

openshift-ci-robot commented Mar 4, 2024

Tal-or commented Mar 4, 2024

openshift-ci bot commented Mar 4, 2024

Tal-or commented Mar 4, 2024

ffromani left a comment

ffromani Mar 5, 2024

shajmakh Mar 5, 2024

ffromani Mar 5, 2024

ffromani Mar 5, 2024

Tal-or Mar 5, 2024

ffromani Mar 5, 2024

shajmakh Mar 5, 2024

ffromani Mar 5, 2024

Tal-or Mar 5, 2024

shajmakh left a comment

shajmakh Mar 5, 2024

Tal-or Mar 5, 2024

Tal-or Mar 5, 2024

shajmakh Mar 5, 2024

shajmakh Mar 5, 2024

shajmakh Mar 5, 2024

shajmakh Mar 5, 2024

Tal-or Mar 5, 2024

shajmakh Mar 5, 2024 •

edited

Loading

Tal-or Mar 5, 2024

Tal-or commented Mar 5, 2024

openshift-ci bot commented Mar 21, 2024

openshift-merge-robot commented Mar 27, 2024

Tal-or commented Apr 1, 2024

openshift-ci bot commented Apr 1, 2024

		github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.1.1-0.20201119153432-9d213757d22d
		github.com/k8snetworkplumbingwg/sriov-network-operator v1.2.0

NO-JIRA: e2e:sriov: deploy sriov devices as part of config suite #981

NO-JIRA: e2e:sriov: deploy sriov devices as part of config suite #981

Conversation

Tal-or commented Mar 4, 2024

openshift-ci-robot commented Mar 4, 2024

Tal-or commented Mar 4, 2024

openshift-ci bot commented Mar 4, 2024

Tal-or commented Mar 4, 2024

ffromani 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

shajmakh 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

shajmakh Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tal-or commented Mar 5, 2024

openshift-ci bot commented Mar 21, 2024

openshift-merge-robot commented Mar 27, 2024

Tal-or commented Apr 1, 2024

openshift-ci bot commented Apr 1, 2024

shajmakh Mar 5, 2024 •

edited

Loading