OCPBUGS-29596: e2e: Enhance tests related to crio annotations #955

mrniranjan · 2024-02-14T13:30:15Z

Add new test case for testing cpu-quota.crio.io annotation
Move the existing cpu load balancing test under single container where all the tests related to crio annotation exist
Modify tests to be executed under different runtimes and cgroup versions

openshift-ci-robot · 2024-02-16T07:43:59Z

@mrniranjan: This pull request references Jira Issue OCPBUGS-29596, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (mniranja@redhat.com), skipping review request.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Add new test case for testing cpu-quota.crio.io annotation

Move the existing cpu load balancing test under single container where all the tests related to crio annotation exist

Modify tests to be executed under different runtimes and cgroup versions

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-02-20T09:42:40Z

test/e2e/performanceprofile/functests/utils/cgroup/controller/controller.go

+	Period string
+}
+
+type CpuStat struct {


Each struct should represent a cgroup controller. i.e cpu,cpuset,memory,pids...
stats is not a controller, it's a field under the cpu controller
Could you please elaborate why this change is needed?

We discussed this before stat has multi line values where the number files are different in v1 and v2 and with current implementation reading stat is difficult where the current implementation tries to read multiple files and thats a problem. And we wanted stat values to be be stored in map . So in the earlier discussion we decide stat to be handled separately.

@mrniranjan Can't you simply read the cpu.stat in a separate call and store it in cfg.cpuStat?
Add the logic here:
https://github.com/openshift/cluster-node-tuning-operator/blob/master/test/e2e/performanceprofile/functests/utils/cgroup/v1/v1.go#L45
And here:
https://github.com/openshift/cluster-node-tuning-operator/blob/master/test/e2e/performanceprofile/functests/utils/cgroup/v2/v2.go#L45

something like:

cmd = []string{ "/bin/cat", dirPath + "/cpu.stat", } b, err := pods.ExecCommandOnPod(cm.k8sClient, pod, containerName, cmd) // do some parsing logic and store in map

addressed in the latest commit

Tal-or · 2024-02-20T09:52:19Z

test/e2e/performanceprofile/functests/utils/cgroup/v2/v2.go

+	interfacevalues := strings.Split(output, "\r\n")
+	// cpu.stat always contains 3 stats usage_usec, user_usec, system_usec
+	// only when cpu controller is enabled other stats like nr_periods etc are enabled
+	if len(interfacevalues) < 4 {


A more robust way to check whether cpu controller enabled on cgroup v2 is by checking the cgroup.controllers and cgroup.subtree_control files.
From the documentation https://man7.org/linux/man-pages/man7/cgroups.7.html:
Active cgroups must be specified via the files
cgroup.controllers and cgroup.subtree_control.

It's only a suggestion, if we want to keep the function identical this is also an option but I think it would be better to move the common (identical for v1 and v2) functions into a separate package.
This can be handled in a separate PR as well

Yeah i will address this in separate PR , for now i will modify the message.

Tal-or · 2024-02-20T09:55:38Z

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

@@ -507,11 +381,12 @@ var _ = Describe("[rfe_id:27363][performance] CPU Management", Ordered, func() {

 			containersCgroups = strings.Trim(containersCgroups, "\n")
 			containersCgroupsDirs := strings.Split(containersCgroups, "\n")
-			Expect(len(containersCgroupsDirs)).To(Equal(2), "unexpected amount of containers cgroups")
+			//Expect(len(containersCgroupsDirs)).To(Equal(2), "unexpected amount of containers cgroups")


remove comment

fixed in the latest commit

The comment is still here, probably some rebase issue

fixed in the latest commit

Tal-or · 2024-02-20T09:56:02Z

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

@@ -654,6 +529,126 @@ var _ = Describe("[rfe_id:27363][performance] CPU Management", Ordered, func() {
 		)

 	})
+	When("Crio Annotations", func() {


I think Context is more readable here

fixed in the latest commit

Tal-or · 2024-02-20T09:57:57Z

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

+			if len(defaultCpuNotInSchedulingDomains) > 0 {
+				pods, err := pods.GetPodsOnNode(context.TODO(), workerRTNode.Name)
+				if err != nil {
+					testlog.Warningf("cannot list pods on %q: %v", workerRTNode.Name, err)


Is warning enough here?

hmm, i am not sure too, though this code is mostly used as debug to know that we found some pods before the test starts and for some reason we are unable to list the pods before throwing the error: " the test expects all CPUs within a scheduling domain when starting"

Tal-or · 2024-02-20T09:58:30Z

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

+				testlog.Infof("deleting test pod %s/%s UID=%q", testpod.Namespace, testpod.Name, podUID)
+				err := testclient.Client.Get(ctx, client.ObjectKeyFromObject(testpod), testpod)
+				Expect(err).ToNot(HaveOccurred())
+				//testpodUID := testpod.UID


remove comment

fixed in the latest commit

Tal-or · 2024-02-20T09:59:22Z

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

+				err := getter.Container(ctx, testpod, testpod.Spec.Containers[0].Name, cpusetCfg)
+				Expect(err).ToNot(HaveOccurred())
+				// Get cpus used by the container
+				tasksetcmd := []string{"taskset", "-pc", "1"}


it's always better to provide full path for the command

fixed in the latest commit

Tal-or · 2024-02-20T10:02:57Z

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

+				parts := strings.Split(strings.TrimSpace(podCpusStr), ":")
+				testpodCpus := strings.TrimSpace(parts[1])
+				testlog.Infof("%v pod is using %v cpus", testpod.Name, string(testpodCpus))
+				Expect(cpusetCfg.Cpus).To(Equal(testpodCpus), "cpuset.cpus not matching the process affinity")


The comparison here is done between strings but they can be logically equals while unequal by string.
Example:
setA: 1-3,7
setB: 1,2,3,7
logically equals but the strings are different.
parse them into cpuset and then compare.

fixed in the latest commit

mrniranjan · 2024-03-05T08:12:17Z

/retest-required

Tal-or · 2024-03-13T09:23:17Z

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

+			if cgroupV2 {
+				cpusetPath = "/rootfs/sys/fs/cgroup/kubepods.slice"
+			} else {
+				cpusetPath = "/rootfs/sys/fs/cgroup/cpuset"


On v1 the kubepods.slice part exists as well

correct, on v1 the paths differ

shajmakh · 2024-03-05T11:46:03Z

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

+				podAffinityCpuset, err := cpuset.Parse(testpodCpus)
+				Expect(err).ToNot(HaveOccurred(), "Unable to parse cpus %s used by %s pod", testpodCpus, testpod.Name)
+				cgroupCpuset, err := cpuset.Parse(cpusetCfg.Cpus)
+				Expect(err).ToNot(HaveOccurred(), "Unable to parse cpus from cgroups.cpugset")


cpugset or cpuset?

probably typo

fixed in the latest commit

shajmakh · 2024-03-14T11:22:26Z

test/e2e/performanceprofile/functests/utils/cgroup/v2/v2.go

@@ -77,7 +77,7 @@ func (cm *ControllersManager) CpuStat(ctx context.Context, pod *corev1.Pod, cont
 	// cpu.stat always contains 3 stats usage_usec, user_usec, system_usec
 	// only when cpu controller is enabled other stats like nr_periods etc are enabled
 	if len(interfacevalues) < 4 {
-		return nil, fmt.Errorf("CPU Controller is not enabled")
+		return nil, fmt.Errorf("cpu.stat doesn't contain all the cpu metrics required")


minor: this was restored in the later commit:
8092af0#diff-f2536864a27be1f85973bce12efd1a1447d9fab4bb03170997c538c5e5650e81R81
if not intended to change it here, I think better to discard this line change

fixed in the latest commit

Tal-or · 2024-03-14T11:29:53Z

test/e2e/performanceprofile/functests/utils/cgroup/v1/v1.go

@@ -57,9 +57,36 @@ func (cm *ControllersManager) Cpu(ctx context.Context, pod *corev1.Pod, containe
 	output := strings.Split(string(b), "\r\n")
 	cfg.Quota = output[0]
 	cfg.Period = output[1]
+	cfg.Stat, err = stat(cm.k8sClient, pod, containerName, childName)


We should check the error value

fixed in the latest commit

mrniranjan · 2024-03-15T11:53:53Z

/test e2e-gcp-pao-workloadhints

mrniranjan · 2024-03-25T10:08:17Z

/test e2e-gcp-pao

mrniranjan · 2024-04-02T06:17:25Z

/retest-required

yanirq · 2024-04-03T15:28:41Z

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

+
+			By("Starting the pod")
+			testpod.Spec.NodeSelector = testutils.NodeSelectorLabels
+			testpod.Spec.Containers[0].Image = "quay.io/mniranja/busycpus"


we should probably replace this quay url

@yanirq fixed it, using http://quay.io/ocp-edge-qe/busycpus

mrniranjan · 2024-04-05T00:42:11Z

/retest-required

yanirq · 2024-04-07T11:32:49Z

@mrniranjan does this PR warrant all the commits ? (9 commits) perhaps we should squash them

Tal-or · 2024-04-07T19:50:29Z

/lgtm

1. Add new test case for testing cpu-quota.crio.io annotation 2. Move the existing cpu load balancing test under single container where all the tests related to crio annotation exist 3. Modify tests to be executed under different runtimes and cgroup versions Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> fix the cpu controller path Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> reword test case descriptions and add test case id Fixing review comments 1. Use cpuset to compare 2 cpu string instead of string comparision 2. Remove unnecessary code comments Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> Modify the error message when all cpu metric are not available in cpu.stat Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> make Stat part of Cpu structure replace cpuStat method with stat function Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> minor typo fixes and remove lines with comments Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> fix the path of the cpu controller in cgroupv2 env Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> use ocp-edge-qe quay repository for busycpus image Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> fetch busycpusImage using environment variables This patch uses env variable IMAGE_REGISTRY and BUSY_CPUS_IMAGE for QE image registry and busycpus image This is required to execute tests in disconnected environment where quay.io is not accessible and have to use internal private registry Signed-off-by: Niranjan M.R <mrniranjan@redhat.com>

mrniranjan · 2024-04-08T06:44:29Z

@yanirq i have squashed the commits

yanirq · 2024-04-08T08:26:49Z

/approve

openshift-ci · 2024-04-08T08:27:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrniranjan, yanirq

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

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

openshift-ci · 2024-04-08T09:44:58Z

@mrniranjan: all tests passed!

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.

yanirq · 2024-04-08T09:49:57Z

/lgtm

openshift-ci-robot · 2024-04-08T09:56:14Z

@mrniranjan: Jira Issue OCPBUGS-29596: All pull requests linked via external trackers have merged:

openshift/cluster-node-tuning-operator#955

Jira Issue OCPBUGS-29596 has been moved to the MODIFIED state.

In response to this:

Add new test case for testing cpu-quota.crio.io annotation

Move the existing cpu load balancing test under single container where all the tests related to crio annotation exist

Modify tests to be executed under different runtimes and cgroup versions

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.

openshift-bot · 2024-04-08T11:31:03Z

[ART PR BUILD NOTIFIER]

This PR has been included in build cluster-node-tuning-operator-container-v4.16.0-202404080642.p0.g5cc1952.assembly.stream.el9 for distgit cluster-node-tuning-operator.
All builds following this will include this PR.

openshift-ci bot requested review from dagrayvid and swatisehgal February 14, 2024 13:31

mrniranjan changed the title ~~e2e: Enhance tests related to crio annotations~~ OCPBUGS-29596: e2e: Enhance tests related to crio annotations Feb 16, 2024

Tal-or reviewed Feb 20, 2024

View reviewed changes

mrniranjan force-pushed the crio_crun branch 2 times, most recently from 55eeed9 to 36c5e63 Compare March 13, 2024 09:21

Tal-or reviewed Mar 13, 2024

View reviewed changes

mrniranjan force-pushed the crio_crun branch from 36c5e63 to 8092af0 Compare March 13, 2024 09:23

shajmakh reviewed Mar 14, 2024

View reviewed changes

Tal-or reviewed Mar 14, 2024

View reviewed changes

yanirq reviewed Apr 3, 2024

View reviewed changes

mrniranjan force-pushed the crio_crun branch 2 times, most recently from 2fbc81d to d2a3e80 Compare April 4, 2024 13:59

mrniranjan force-pushed the crio_crun branch from 691e19a to 4743abf Compare April 5, 2024 07:03

openshift-ci bot assigned Tal-or Apr 7, 2024

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

mrniranjan force-pushed the crio_crun branch from 4743abf to d5b4497 Compare April 8, 2024 06:44

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2024

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

openshift-ci bot assigned yanirq Apr 8, 2024

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

openshift-merge-bot bot merged commit 5cc1952 into openshift:master Apr 8, 2024
16 checks passed

OCPBUGS-29596: e2e: Enhance tests related to crio annotations #955

OCPBUGS-29596: e2e: Enhance tests related to crio annotations #955

Conversation

mrniranjan commented Feb 14, 2024

openshift-ci-robot commented Feb 16, 2024

Choose a reason for hiding this comment

mrniranjan Mar 4, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrniranjan commented Mar 5, 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

mrniranjan commented Mar 15, 2024

mrniranjan commented Mar 25, 2024

mrniranjan commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrniranjan commented Apr 5, 2024

yanirq commented Apr 7, 2024

Tal-or commented Apr 7, 2024

mrniranjan commented Apr 8, 2024

yanirq commented Apr 8, 2024

openshift-ci bot commented Apr 8, 2024

openshift-ci bot commented Apr 8, 2024

yanirq commented Apr 8, 2024

openshift-ci-robot commented Apr 8, 2024

openshift-bot commented Apr 8, 2024

mrniranjan Mar 4, 2024 •

edited