OCPBUGS-57150: E2E: Additional Functional tests for align cpus by UncoreCache Feature #1324

mrniranjan · 2025-04-26T02:12:34Z

This PR contains Additional tests related to CPUManager policy option to align Cpus based on Uncore cache.
Contains tests related to Multiple Pods, Containers and with SMT disabled

mrniranjan · 2025-04-30T11:09:50Z

This PR depends on #1302

Adds Additional tests related to CPUManager policy option to align Cpus based on Uncore cache. Contains tests related to Multiple Pods, multiple containers and with SMT disabled Signed-off-by: Niranjan M.R <[email protected]>

ffromani

initial review, nothing huge but quite a few questions inside

ffromani · 2025-05-26T14:13:28Z

test/e2e/performanceprofile/functests/13_llc/llc.go

-					corev1.ResourceMemory: resource.MustParse("100Mi"),
+		Context("With Multiple Pods", func() {
+			DescribeTable("Align multiple Guaranteed pods",
+				func(l3uncoreCacheSharing string) {


let's not use a string, use a enum-like type. Example:

type L3UncoreCacheShareMode string const ( L3UncoreCacheShareEqual L3UncoreCacheShareMode = "equal" L3UncoreCacheShareUnequal L3UncoreCacheShareMode = "unequal" )

fixed in the latest commit

ffromani · 2025-05-26T14:14:47Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+						targetNode                 = workerRTNodes[0]
+						cpusetCfg                  = &controller.CpuSet{}
+						cpusetList []cpuset.CPUSet
+						podCpuList []string


this seems more like podCpuRequirementList or podCpuRequirements

fixed in the latest commit

ffromani · 2025-05-26T14:15:28Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+						dpList     []*appsv1.Deployment
+					)
+
+					podLabel := make(map[string]string)


why if we initialize again in the for loop body below on line 519?

fixed in the latest commit

ffromani · 2025-05-26T14:17:17Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+
+		Context("Multiple Containers", func() {
+			DescribeTable("Verify CPU Allignmen with multiple containers",
+				func(deploymentName string, alignment string, sideCarContainerName []string) {


ditto for alignment

fixed in the latest commit

ffromani · 2025-05-26T14:17:39Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+					if l3uncoreCacheSharing == "equal" {
+						Expect(cpusetList[0]).To(Equal(cpusetList[1]))
+					} else {
+						Expect(cpusetList[0]).ToNot(Equal(cpusetList[1]))


we should check the retrieved cpulist also match the expected size (podCpuList)

fixed in the latest commit

ffromani · 2025-05-26T14:19:54Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+						Expect(err).ToNot(HaveOccurred())
+						containerWithnonIntegralCpu2, err := cpuset.Parse(cpusetCfg.Cpus)
+						Expect(err).ToNot(HaveOccurred())
+						Expect(containerWithnonIntegralCpu2).To(Equal(expectedBurstablePodCpus))


cpusets should be compared for equality using their Equal method

fixed in the latest commit

ffromani · 2025-05-26T14:20:07Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+						Expect(err).ToNot(HaveOccurred())
+						containerWithnonIntegralCpu1, err := cpuset.Parse(cpusetCfg.Cpus)
+						Expect(err).ToNot(HaveOccurred())
+						Expect(containerWithnonIntegralCpu1).To(Equal(expectedBurstablePodCpus))


see comment for line 665 below

fixed in the latest commit

ffromani · 2025-05-26T14:21:26Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+				TopologyPolicy: &policy,
+			}
+
+			profile.Spec.AdditionalKernelArgs = []string{"nosmt", "module_blacklist=irdma"}


where do we revert this setting?

We have a AfteraAll section at the start.
https://github.com/openshift/cluster-node-tuning-operator/blob/main/test/e2e/performanceprofile/functests/13_llc/llc.go#L130

ffromani · 2025-05-26T14:23:20Z

test/e2e/performanceprofile/functests/13_llc/llc.go

@@ -662,3 +941,36 @@ func waitForDeploymentPodsDeletion(ctx context.Context, targetNode *corev1.Node,
 		Expect(err).ToNot(HaveOccurred())
 	}
 }
+
+func createSidecarContainers(names []string, cpuResources []string) []corev1.Container {


these are not "sidecar" containers as kube intend them, so this function should probably be called "multicontainer" or so

fixed in the latest commit

Instead of using string, the arguments to table driven test cases are converted to enum-like variables using constants. Other Minor fixes like comparing cpuset variable using inbuilt Equal operator Signed-off-by: Niranjan M.R <[email protected]>

shajmakh · 2025-06-02T07:47:30Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+						podCpuRequirementList = []string{fmt.Sprintf("%d", L3CacheGroupSize), fmt.Sprintf("%d", L3CacheGroupSize/2)}
+					}
+
+					for i := range 2 {


can you add a comment please why 2?

addressed in the latest commit

shajmakh

Thank you for working on this. I am not well familiar with the feature yet hence cannot provide feedback on the anticipated pods cpus results, but I didn't spot anything breaking too. I'll defer the LGTM to more expert members in the feature. However, I added few comments, which are non blocking and can be even addressed in a cleanup PR to allow running the tests in the CI and not keep them hanging. Overall I recommend adding more logs about the test steps to make the test log more readable and enable better troubleshooting on failures.

shajmakh · 2025-06-02T08:11:24Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+						err = getter.Container(ctx, &testpod, testpod.Spec.Containers[0].Name, cpusetCfg)
+						Expect(err).ToNot(HaveOccurred())
+						podCpuset, err := cpuset.Parse(cpusetCfg.Cpus)
+						testlog.TaggedInfof("Pod", "Cpus used by pod %v are %v", testpod.Name, podCpuset.String())


refering to a namespace-scoped resources is usually done by / to easily identify the resource

shajmakh · 2025-06-02T08:13:35Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+						podList := &corev1.PodList{}
+						podLabel["test-app"] = fmt.Sprintf("telcoApp-%d", i)
+						listOptions := &client.ListOptions{Namespace: testutils.NamespaceTesting, LabelSelector: labels.SelectorFromSet(podLabel)}
+						Eventually(func() bool {


a log (inside the eventually or outside) about waiting for the deployment to be ready can be helpful to track tests steps

addressed in the latest commit

shajmakh · 2025-06-02T08:23:20Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+					}
+					// Here pods of both deployments can take the full core even as it will follow
+					// packed allocation
+					// From KEP: takeUncoreCache and takePartialUncore will still follow a "packed" allocation principle as the rest of the implementation.


this KEP: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/4800-cpumanager-split-uncorecache/README.md?

shajmakh · 2025-06-02T08:25:24Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+						cpuResources := []string{fmt.Sprintf("%d", cpuSize)}
+						containerList = createMultipleContainers(containerName, cpuResources)
+					case partialAlignment:
+						// WIth 2 guaranteed containers, where 1 is requesting half of L3CacheGroupSize


addressed in the latest commit

shajmakh · 2025-06-02T09:08:17Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+						Expect(err).ToNot(HaveOccurred())
+						containerWithnonIntegralCpu1, err := cpuset.Parse(cpusetCfg.Cpus)
+						Expect(err).ToNot(HaveOccurred())
+						Expect(containerWithnonIntegralCpu1.Equals(expectedBurstablePodCpus)).To(BeTrue())


for these types of assertions I always prefer to print out the values of each variable if the assert failed to enhance the troubleshooting, here and in similar assertions in this PR. the reason for this is that when such Expect() fails one can only know that "Expected false to equal true" and no other details is provided about the actual compared values.

addressed in the latest commit

shajmakh · 2025-06-02T09:14:12Z

test/e2e/performanceprofile/functests/13_llc/llc.go

+	return containers
+}
+
+// transformToNoSMT moves takes a map which contains cores and its siblings


addressed in the latest commit

Add messages when assertions fail Signed-off-by: Niranjan M.R <[email protected]>

mrniranjan · 2025-06-04T17:40:06Z

/test e2e-aws-ovn

openshift-ci · 2025-06-04T20:27:34Z

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

ffromani

/approve
/lgtm

Let's have this. If needed, we can iterate and improve later

openshift-ci · 2025-06-05T13:48:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

ffromani · 2025-06-05T13:48:57Z

/retitle NO-JIRA: E2E: Additional Functional tests for align cpus by UncoreCache Feature

openshift-ci-robot · 2025-06-05T13:49:12Z

@mrniranjan: This pull request explicitly references no jira issue.

In response to this:

This PR contains Additional tests related to CPUManager policy option to align Cpus based on Uncore cache.
Contains tests related to Multiple Pods, Containers and with SMT disabled

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.

ffromani · 2025-06-05T13:49:21Z

/jira refresh

openshift-ci-robot · 2025-06-05T13:49:25Z

@ffromani: This pull request explicitly references no jira issue.

In response to this:

/jira refresh

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 · 2025-06-05T15:33:35Z

[ART PR BUILD NOTIFIER]

Distgit: cluster-node-tuning-operator
This PR has been included in build cluster-node-tuning-operator-container-v4.20.0-202506051441.p0.geae77fb.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot · 2025-06-06T07:28:28Z

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

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

In response to this:

This PR contains Additional tests related to CPUManager policy option to align Cpus based on Uncore cache.
Contains tests related to Multiple Pods, Containers and with SMT disabled

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.

mrniranjan · 2025-06-26T09:59:10Z

/cherry-pick release-4.19

openshift-cherrypick-robot · 2025-06-26T10:03:25Z

@mrniranjan: new pull request created: #1348

In response to this:

/cherry-pick release-4.19

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.

mrniranjan marked this pull request as draft April 26, 2025 02:12

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 Apr 26, 2025

openshift-ci bot requested review from rbaturov and swatisehgal April 26, 2025 02:13

mrniranjan force-pushed the llc_take3_1 branch from d236507 to 0eeb608 Compare April 30, 2025 11:06

mrniranjan marked this pull request as ready for review April 30, 2025 11:06

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 Apr 30, 2025

openshift-ci bot requested review from ffromani and MarSik April 30, 2025 11:07

mrniranjan force-pushed the llc_take3_1 branch 3 times, most recently from df99dbd to 1667034 Compare May 20, 2025 05:54

mrniranjan force-pushed the llc_take3_1 branch from 1667034 to e34145c Compare May 20, 2025 06:20

ffromani reviewed May 26, 2025

View reviewed changes

shajmakh reviewed Jun 2, 2025

View reviewed changes

Add comments and log messages for clear description

981e3ff

Add messages when assertions fail Signed-off-by: Niranjan M.R <[email protected]>

ffromani reviewed Jun 5, 2025

View reviewed changes

openshift-ci bot assigned ffromani Jun 5, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2025

openshift-ci bot changed the title ~~E2E: Additional Functional tests for align cpus by UncoreCache Feature~~ NO-JIRA: E2E: Additional Functional tests for align cpus by UncoreCache Feature Jun 5, 2025

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

openshift-merge-bot bot merged commit eae77fb into openshift:main Jun 5, 2025
18 checks passed

mrniranjan changed the title ~~NO-JIRA: E2E: Additional Functional tests for align cpus by UncoreCache Feature~~ OCPBUGS-57150: E2E: Additional Functional tests for align cpus by UncoreCache Feature Jun 6, 2025

openshift-cherrypick-robot mentioned this pull request Jun 26, 2025

[release-4.19] OCPBUGS-58144: E2E: Additional Functional tests for align cpus by UncoreCache Feature #1348

Merged

OCPBUGS-57150: E2E: Additional Functional tests for align cpus by UncoreCache Feature #1324

OCPBUGS-57150: E2E: Additional Functional tests for align cpus by UncoreCache Feature #1324

Uh oh!

Conversation

mrniranjan commented Apr 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrniranjan commented Apr 30, 2025

Uh oh!

ffromani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shajmakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrniranjan commented Jun 4, 2025

Uh oh!

openshift-ci bot commented Jun 4, 2025

mrniranjan commented Apr 26, 2025 •

edited

Loading