OCPBUGS-5064: E2E: Per Core Runtime Tuning Test automation #509

mrniranjan · 2022-11-08T11:19:29Z

Signed-off-by: Niranjan M.R mrniranjan@redhat.com

mrniranjan · 2022-11-09T00:40:16Z

/retest-required

mrniranjan · 2022-11-09T06:24:31Z

/retest-required

marioferh

some comments

marioferh · 2022-11-09T10:27:28Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				//First enable HighPowerConsumption
+				By("Modifying profile")
+				profile.Spec.WorkloadHints = &performancev2.WorkloadHints{
+					HighPowerConsumption:  pointer.Bool(true),


This test override this one right?
https://github.com/marioferh/cluster-node-tuning-operator/blob/5d9c2e11ad5595b7724edb327ed1a34e83518fc1/test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go#L638

Yes, this test we are checking if we can move from one Hint to another is the system tuned appropriately

Then we can remove the previous one?

marioferh · 2022-11-09T10:28:00Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+
+			})
+
+			It("[test_id:54179]Verify System is tuned when reverting from PerPodPowerManagement to HighPowerConsumption", func() {


And this one overrides this: https://github.com/marioferh/cluster-node-tuning-operator/blob/5d9c2e11ad5595b7724edb327ed1a34e83518fc1/test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go#L676

Yes, this test we are checking if we can move from one Hint to another is the system tuned appropriately

marioferh · 2022-11-09T10:29:47Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				checkTunedParameters(workerRTNodes, stalldEnabled, sysctlMap, kernelParameters, rtKernel)
+			})
+
+			It("[test_id:54184]Verify enabling both HighPowerConsumption and PerPodPowerManagment fails", func() {


this test must fail, where we are catching the expected error?

yes this test fails, we are checking if it fails. May be i should check the error

Yep that's what I mean

@marioferh Addressed this on latest patch

marioferh · 2022-11-09T10:31:23Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				)).To(HaveOccurred())
+			})
+
+			It("[test_id:54185]Verify sysfs paramters of guaranteed pod with powersave annotiations", func() {


this test is of cri-o/cri-o#5927, @bartwensley PTAL?

marioferh · 2022-11-09T12:29:45Z

/retest-required

marioferh · 2022-11-09T16:11:17Z

/retest-required

bartwensley · 2022-11-09T19:25:29Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+					"cpu-c-states.crio.io":      "enable",
+					"cpu-freq-governor.crio.io": "schedutil",


What will the default c-state/p-state configuration be for the server running the test? Just wondering if these will have any impact if the system is configured for low power by default?

@bartwensley In this case system configuration is what the perPodPowerManagement configures. so intel_pstate is set passive. And the bios is set it's power settings to OS controlled.

OK - and I assume c-states are also enabled in the BIOS?

@bartwensley Yes C-states are enabled in BIOS

bartwensley · 2022-11-09T19:27:34Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+					Expect(output).To(Equal("schedutil"))
+					Expect(err).ToNot(HaveOccurred())
+				}
+				deleteTestPod(testpod)


After the pod is deleted, it would be good to check that the cpus that were used by the pod have been set back to their original pm_qos_resume_latency and scaling_governor.

ack, will add that

@bartwensley i have addressed this in the latest patch

bartwensley · 2022-11-09T19:31:23Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				}, (cluster.ComputeTestTimeout(30*time.Second, RunningOnSingleNode)), 5*time.Second).ShouldNot(BeEmpty(),
+					fmt.Sprintf("cannot find cgroup for container %q", containerID))
+
+				By("Checking what CPU the pod is using")


Maybe it is too much, but I wonder if it would be useful to check that cpus not in use by the pod did not have their pm_qos_resume_latency_us or scaling_governor modified?

ack will check that

@bartwensley i have addressed this in the latest patch

bartwensley · 2022-11-09T19:32:11Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				deleteTestPod(testpod)
+			})
+
+			It("[test_id:54186] Verify sysfs paramters of guaranteed pod with performance annotiations", func() {


The comments from the previous testcase would apply to this testcase as well.

@bartwensley i have addressed this in the latest patch

mrniranjan · 2022-11-29T06:12:28Z

/test e2e-gcp-pao

marioferh · 2022-11-29T12:57:42Z

/test e2e-gcp-pao

bartwensley · 2022-11-29T14:05:07Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				}, time.Minute, 5*time.Second).Should(ContainSubstring("HighPowerConsumption and PerPodPowerManagement can not be both enabled"))
+			})
+
+			It("[test_id:54185] Verify sysfs paramters of guaranteed pod with powersave annotiations", func() {


nit: spelling - parameters and annotations

@bartwensley addressed this in the latest patch

bartwensley · 2022-11-29T14:11:13Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+}
+
+//checkCpuGovernors Checks power settings of the cpus
+func checkCpuGovernors(cpus []int, targetNode *corev1.Node, pm_qos string, governor string) error {


nit: a better name might be checkCpuGovernorsAndResumeLatency

@bartwensley addressed this in the latest patch

bartwensley · 2022-11-29T14:13:55Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				Expect(err).ToNot(HaveOccurred())
+				cpus, err := cpuset.Parse(output)
+				targetCpus := cpus.ToSlice()
+				// Verify cpus assingned to the pod have performance powersettings


nit: spelling - assigned

@bartwensley addressed this in the latest patch

bartwensley · 2022-11-29T14:16:01Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				cpus, err := cpuset.Parse(output)
+				targetCpus := cpus.ToSlice()
+				// Verify cpus assingned to the pod have performance powersettings
+				By("Verify the rest of the cpus donot haver powersave settings")


This should be down one line. It should also say "have default settings".

@bartwensley addressed this in the latest patch

marioferh · 2022-11-29T16:35:10Z

/test e2e-gcp-pao

marioferh

/lgtm

marioferh · 2022-11-30T11:02:43Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				//First enable HighPowerConsumption
+				By("Modifying profile")
+				profile.Spec.WorkloadHints = &performancev2.WorkloadHints{
+					HighPowerConsumption:  pointer.Bool(true),


Then we can remove the previous one?

marioferh · 2022-11-30T11:02:48Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+
+			})
+
+			It("[test_id:54179]Verify System is tuned when reverting from PerPodPowerManagement to HighPowerConsumption", func() {


marioferh · 2022-11-30T11:02:57Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				checkTunedParameters(workerRTNodes, stalldEnabled, sysctlMap, kernelParameters, rtKernel)
+			})
+
+			It("[test_id:54184]Verify enabling both HighPowerConsumption and PerPodPowerManagment fails", func() {


mrniranjan · 2022-11-30T13:44:07Z

/retest-required

bartwensley · 2022-11-30T15:40:25Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				// Verify cpus not assigned to the pod have default settings
+				err = checkCpuGovernorsAndResumeLatency(targetCpus, &workerRTNodes[0], "n/a", "performance")


This is checking the targetCpus, which are the ones assigned to the pod - right? So the comment is wrong and the check is in the wrong place - it should be on line 1176 - above the "Verify the rest of the cpus donot have powersave settings).

bartwensley · 2022-11-30T15:41:57Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				Expect(err).ToNot(HaveOccurred())
+				cpus, err := cpuset.Parse(output)
+				targetCpus := cpus.ToSlice()
+				By("Verify the rest of the cpus donot haver powersave settings")


This is incorrect - the code is not checking whether the remaining cpus "donot have powersave settings" - it is checking that they have the default settings, which are a mix (c-states are enabled and governor is performance).

bartwensley · 2022-12-02T15:52:50Z

/lgtm

bartwensley

Looks good.

yanirq · 2023-02-06T08:31:25Z

/retest

yanirq · 2023-02-06T08:36:39Z

/hold - check if we need to add skip for lower than 8 cpus

mrniranjan · 2023-02-06T12:30:30Z

/test e2e-upgrade

mrniranjan · 2023-02-06T16:05:38Z

@yanirq can you check the latest ci results and also the changes i have added a new commit to the changes done . So if its good i will squash the commits.

mrniranjan · 2023-02-07T09:03:15Z

/retest-required

mrniranjan · 2023-02-07T12:35:58Z

/retest-required

mrniranjan · 2023-02-07T15:19:34Z

/test e2e-gcp-pao-updating-profile

yanirq · 2023-02-08T10:26:08Z

@mrniranjan please rebase this PR first on top of the latest changes , I would like to see an updated test run

Skip tests where we create a guaranteed pod with powersave and performance annotations as they requires baremetal with powermanagement settings in BIOS make changes required for ginkgov2 Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> Add new function to check harware capability Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> remove hardcoded online cpu count. Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> Minor fix reduce the total number of cpus from 80 to 32 Also add the checkHardwareCapability to PerPodPowermanagement test cases instead of offline cpu test cases Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> Use constant for totalCpus Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> Skip the tests if number of online cpus is not more than 32 Signed-off-by: Niranjan M.R <mrniranjan@redhat.com>

mrniranjan · 2023-02-08T13:31:40Z

/retest-required

mrniranjan · 2023-02-09T00:25:57Z

/retest-required

mrniranjan · 2023-02-09T06:04:33Z

/retest-required

yanirq · 2023-02-13T15:15:56Z

/lgtm

mrniranjan · 2023-02-14T05:44:07Z

@yanirq can we remove the Do not merge/hold flag ?

yanirq · 2023-02-14T09:34:22Z

/hold cancel

mrniranjan · 2023-02-14T13:51:06Z

/test e2e-gcp-pao-updating-profile

openshift-ci-robot · 2023-02-14T18:20:21Z

/retest-required

Remaining retests: 0 against base HEAD 93c97bc and 2 for PR HEAD 9eaeb6c in total

openshift-ci · 2023-02-15T00:28:42Z

@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.

openshift-ci-robot · 2023-02-15T00:33:36Z

@mrniranjan: All pull requests linked via external trackers have merged:

openshift/cluster-node-tuning-operator#509

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

In response to this:

Signed-off-by: Niranjan M.R mrniranjan@redhat.com

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.

Skip tests where we create a guaranteed pod with powersave and performance annotations as they requires baremetal with powermanagement settings in BIOS make changes required for ginkgov2 Add new function to check harware capability remove hardcoded online cpu count. Minor fix reduce the total number of cpus from 80 to 32 Also add the checkHardwareCapability to PerPodPowermanagement test cases instead of offline cpu test cases Use constant for totalCpus Skip the tests if number of online cpus is not more than 32 Signed-off-by: Niranjan M.R <mrniranjan@redhat.com> Co-authored-by: Niranjan M.R <mrniranjan@redhat.com>

openshift-ci bot requested review from dagrayvid and Tal-or November 8, 2022 11:19

mrniranjan force-pushed the pcrtuning_automation branch from 728e28c to 397284c Compare November 8, 2022 11:27

marioferh reviewed Nov 9, 2022

View reviewed changes

bartwensley reviewed Nov 9, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2022

mrniranjan force-pushed the pcrtuning_automation branch from 397284c to 9427ff0 Compare November 28, 2022 10:16

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 28, 2022

mrniranjan force-pushed the pcrtuning_automation branch 2 times, most recently from 52bcf1a to f59fd2a Compare November 28, 2022 12:56

bartwensley suggested changes Nov 29, 2022

View reviewed changes

openshift-ci bot assigned bartwensley Nov 29, 2022

mrniranjan force-pushed the pcrtuning_automation branch from f59fd2a to 44983e3 Compare November 30, 2022 09:47

marioferh reviewed Nov 30, 2022

View reviewed changes

openshift-ci bot assigned marioferh Nov 30, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2022

bartwensley suggested changes Nov 30, 2022

View reviewed changes

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2022

mrniranjan force-pushed the pcrtuning_automation branch from 44983e3 to 263cb9b Compare December 1, 2022 09:57

bartwensley approved these changes Dec 2, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2022

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Feb 6, 2023

mrniranjan force-pushed the pcrtuning_automation branch from ed8fae6 to 0ba9a50 Compare February 7, 2023 08:46

mrniranjan force-pushed the pcrtuning_automation branch from 0ba9a50 to 9eaeb6c Compare February 8, 2023 10:26

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 13, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2023

openshift-merge-robot merged commit 9382029 into openshift:master Feb 15, 2023


		})

		It("[test_id:54179]Verify System is tuned when reverting from PerPodPowerManagement to HighPowerConsumption", func() {

		"cpu-c-states.crio.io": "enable",
		"cpu-freq-governor.crio.io": "schedutil",

		// Verify cpus not assigned to the pod have default settings
		err = checkCpuGovernorsAndResumeLatency(targetCpus, &workerRTNodes[0], "n/a", "performance")

OCPBUGS-5064: E2E: Per Core Runtime Tuning Test automation #509

OCPBUGS-5064: E2E: Per Core Runtime Tuning Test automation #509

Conversation

mrniranjan commented Nov 8, 2022

mrniranjan commented Nov 9, 2022

mrniranjan commented Nov 9, 2022

marioferh 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

marioferh commented Nov 9, 2022

marioferh commented Nov 9, 2022

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 Nov 29, 2022

marioferh commented Nov 29, 2022

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

marioferh commented Nov 29, 2022

marioferh 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

mrniranjan commented Nov 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartwensley commented Dec 2, 2022

bartwensley left a comment

Choose a reason for hiding this comment

yanirq commented Feb 6, 2023

yanirq commented Feb 6, 2023

mrniranjan commented Feb 6, 2023

mrniranjan commented Feb 6, 2023

mrniranjan commented Feb 7, 2023

mrniranjan commented Feb 7, 2023

mrniranjan commented Feb 7, 2023

yanirq commented Feb 8, 2023

mrniranjan commented Feb 8, 2023

mrniranjan commented Feb 9, 2023

mrniranjan commented Feb 9, 2023

yanirq commented Feb 13, 2023

mrniranjan commented Feb 14, 2023

yanirq commented Feb 14, 2023

mrniranjan commented Feb 14, 2023

openshift-ci-robot commented Feb 14, 2023

openshift-ci bot commented Feb 15, 2023

openshift-ci-robot commented Feb 15, 2023