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.

Sign up for GitHub

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

Jump to bottom

add tc numa sriov exclude true #1582

Merged

openshift-merge-robot merged 1 commit into openshift-kni:master from gkopels:add-numa-sriov-exclude-true-tc

Aug 2, 2023

gkopels commented Jul 30, 2023

No description provided.

openshift-ci bot requested review from Missxiaoguo and sakhoury

July 30, 2023 08:04

openshift-ci bot added the needs-ok-to-test label

Contributor

openshift-ci bot commented Jul 30, 2023

Hi @gkopels. Thanks for your PR.

I'm waiting for a openshift-kni member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

zeeke reviewed

View reviewed changes

Member

zeeke left a comment

I left a few comments

cnf-tests/testsuites/e2esuite/dpdk/numa_node_sriov.go

+              			g.Expect(err).ToNot(HaveOccurred())
+              			g.Expect(actualPod.Status.Phase).To(Equal(corev1.PodRunning))
+              			g.Expect(actualPod.Status.QOSClass).To(Equal(corev1.PodQOSGuaranteed))
+              		}, 30*time.Second, 1*time.Second).Should(Succeed())

Member

zeeke Jul 31, 2023

Good job on setting Duration and Timeout. Can you please fix it on line 122 as well? The default is 1s/100ms and it can lead to test flakes.

Author

gkopels Jul 31, 2023

Added

cnf-tests/testsuites/e2esuite/dpdk/numa_node_sriov.go Outdated

               	ExpectWithOffset(1, err).ToNot(HaveOccurred())
               }
+              func validateE2EICMPTraffic(pod *corev1.Pod, annotation string) error {
+              	By("Create SRIOV Network")

Member

zeeke Jul 31, 2023

nit: we can remove this, we are not creating network here.

Author

gkopels Jul 31, 2023

removed

cnf-tests/testsuites/e2esuite/dpdk/numa_node_sriov.go Outdated

+              		Create(context.Background(), serverPod, metav1.CreateOptions{})
+              	Expect(err).ToNot(HaveOccurred())
+              	_, err = pods.ExecCommand(client.Client, *pod, command)

Member

zeeke Jul 31, 2023

I'd wrap this around an Eventually block, as pod creation can take time. By doing so, we can also get rid of the return value.

Author

gkopels Jul 31, 2023

Added

gkopels force-pushed the add-numa-sriov-exclude-true-tc branch from 9f605c1 to bee53cc Compare

July 31, 2023 13:26

zeeke reviewed

View reviewed changes

cnf-tests/testsuites/e2esuite/dpdk/numa_node_sriov.go

               	ExpectWithOffset(1, err).ToNot(HaveOccurred())
               }
+              func validateE2EICMPTraffic(pod *corev1.Pod, annotation string) {

Member

zeeke Jul 31, 2023

Sorry, I meant only the ExecCommand. putting the pod creation inside the Eventually loops bring to creating multiple pods.

I had in my mind something like

Suggested change

      
            func validateE2EICMPTraffic(pod *corev1.Pod, annotation string) {
          
            		serverPod := pods.DefinePod(sriovnamespaces.Test)
          
            		serverPod = pods.RedefinePodWithNetwork(serverPod, annotation)
          
            		command := []string{"bash", "-c", "ping -I net1 192.0.2.250 -c 5"}
          
            		serverPod, err := client.Client.Pods(sriovnamespaces.Test).
          
            			Create(context.Background(), serverPod, metav1.CreateOptions{})
          
            		Expect(err).ToNot(HaveOccurred())
          
            		Eventually(func(g Gomega) error {
          
                 		    _, err = pods.ExecCommand(client.Client, *pod, command)
          
                 		    return err
          
                 		}, 30*time.Second, 1*time.Second).Should(Succeed(), "ICMP traffic failed over SRIOV interface pod interface")

WDYT?

Author

gkopels Aug 1, 2023

Better! :)
Changed

gkopels force-pushed the add-numa-sriov-exclude-true-tc branch 2 times, most recently from fd3f012 to 10f7c34 Compare

August 1, 2023 09:29

SchSeba reviewed

View reviewed changes

cnf-tests/testsuites/e2esuite/dpdk/numa_node_sriov.go Outdated

@@ @@ -3,7 +3,9 @@ package dpdk @@
               import (
               	"context"
               	"fmt"
+              	"github.com/openshift-kni/cnf-features-deploy/cnf-tests/testsuites/pkg/performanceprofile"

Member

SchSeba Aug 1, 2023

nit: please just fix the import here

Author

gkopels Aug 1, 2023

Fixed

gkopels force-pushed the add-numa-sriov-exclude-true-tc branch from 10f7c34 to de4733a Compare

August 1, 2023 11:54

Member

zeeke commented Aug 1, 2023

/ok-to-test

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels

gkopels force-pushed the add-numa-sriov-exclude-true-tc branch from de4733a to bbfacc7 Compare

August 1, 2023 15:03


          cnf-tests: add tc numa sriov exclude true

552ac71

gkopels force-pushed the add-numa-sriov-exclude-true-tc branch from bbfacc7 to 552ac71 Compare

August 1, 2023 15:26

SchSeba reviewed

View reviewed changes

Member

SchSeba left a comment

LGTM

I will let @zeeke to approve

Member

zeeke commented Aug 2, 2023

e2e-gcp-ovn failures are not related to this PR:

Summarizing 6 Failures:
  [FAIL] [performance] Checking IRQBalance settings Verify GloballyDisableIrqLoadBalancing Spec field [It] [test_id:36150] Verify that IRQ load balancing is enabled/disabled correctly
  /go/src/github.com/openshift-kni/cnf-features-deploy/vendor/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/irqbalance.go:95
  [FAIL] [rfe_id:27363][performance] CPU Management when pod runs with the CPU load balancing runtime class [It] [test_id:32646] should disable CPU load balancing for CPU's used by the pod
  /go/src/github.com/openshift-kni/cnf-features-deploy/vendor/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/cpu_management.go:308
  [FAIL] [performance]Hugepages [rfe_id:27354]Huge pages support for container workloads [It] [test_id:27477][crit:high][vendor:[email protected]][level:acceptance] Huge pages support for container workloads
  /go/src/github.com/openshift-kni/cnf-features-deploy/vendor/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/hugepages.go:172
  [FAIL] [rfe_id:27368][performance] Network latency parameters adjusted by the Node Tuning Operator [It] [test_id:28467][crit:high][vendor:[email protected]][level:acceptance] Should contain configuration injected through the openshift-node-performance profile
  /go/src/github.com/openshift-kni/cnf-features-deploy/vendor/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/performance.go:1383
  [FAIL] [tuningcni] tuningcni over macvlan [It] pods with sysctl's over macvlan should be able to ping each other
  /go/src/github.com/openshift-kni/cnf-features-deploy/cnf-tests/testsuites/e2esuite/security/tuning.go:93
  [FAIL] [tuningcni] tuningcni over bond [It] pods with sysctls over bond should be able to ping each other
  /go/src/github.com/openshift-kni/cnf-features-deploy/cnf-tests/testsuites/e2esuite/security/tuning.go:137

/lgtm
/approve
/override ci/prow/e2e-gcp-ovn

openshift-ci bot assigned zeeke

openshift-ci bot added the lgtm label

Contributor

openshift-ci bot commented Aug 2, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gkopels, zeeke

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~~ [zeeke]

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

openshift-ci bot added the approved label

Contributor

openshift-ci bot commented Aug 2, 2023

@zeeke: Overrode contexts on behalf of zeeke: ci/prow/e2e-gcp-ovn

In response to this:

e2e-gcp-ovn failures are not related to this PR:

Summarizing 6 Failures:
 [FAIL] [performance] Checking IRQBalance settings Verify GloballyDisableIrqLoadBalancing Spec field [It] [test_id:36150] Verify that IRQ load balancing is enabled/disabled correctly
 /go/src/github.com/openshift-kni/cnf-features-deploy/vendor/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/irqbalance.go:95
 [FAIL] [rfe_id:27363][performance] CPU Management when pod runs with the CPU load balancing runtime class [It] [test_id:32646] should disable CPU load balancing for CPU's used by the pod
 /go/src/github.com/openshift-kni/cnf-features-deploy/vendor/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/cpu_management.go:308
 [FAIL] [performance]Hugepages [rfe_id:27354]Huge pages support for container workloads [It] [test_id:27477][crit:high][vendor:[email protected]][level:acceptance] Huge pages support for container workloads
 /go/src/github.com/openshift-kni/cnf-features-deploy/vendor/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/hugepages.go:172
 [FAIL] [rfe_id:27368][performance] Network latency parameters adjusted by the Node Tuning Operator [It] [test_id:28467][crit:high][vendor:[email protected]][level:acceptance] Should contain configuration injected through the openshift-node-performance profile
 /go/src/github.com/openshift-kni/cnf-features-deploy/vendor/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/performance.go:1383
 [FAIL] [tuningcni] tuningcni over macvlan [It] pods with sysctl's over macvlan should be able to ping each other
 /go/src/github.com/openshift-kni/cnf-features-deploy/cnf-tests/testsuites/e2esuite/security/tuning.go:93
 [FAIL] [tuningcni] tuningcni over bond [It] pods with sysctls over bond should be able to ping each other
 /go/src/github.com/openshift-kni/cnf-features-deploy/cnf-tests/testsuites/e2esuite/security/tuning.go:137

/lgtm
/approve
/override ci/prow/e2e-gcp-ovn

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.

openshift-merge-robot merged commit f47a2e8 into openshift-kni:master

zeeke mentioned this pull request

[release-4.13] Backport of [sriov] NUMA ExcludeTopology test cases #1629

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm ok-to-test