OCPBUGS-55989: E2E: Functional tests for align cpus by UncoreCache Feature by mrniranjan · Pull Request #1302 · openshift/cluster-node-tuning-operator

mrniranjan · 2025-03-04T07:18:29Z

This PR adds Functional test to UncoreCache CPUManager policy options feature.

ffromani · 2025-04-07T14:45:25Z

+// be running, so in the test we check if the deployment has reached the desiredStatus before
+// quering its pods
+func WaitForDesiredDeploymentStatus(ctx context.Context, deployment *appsv1.Deployment, cli client.Client, namespace, name string, desiredStatus appsv1.DeploymentStatus) error {
+	return wait.PollUntilContextTimeout(context.TODO(), 5*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) {


use ctx here?

fixed in the latest commit

ffromani · 2025-04-07T14:46:22Z

+func TaggedInfof(tag string, format string, args ...interface{}) {
+	logf("[INFO]", fmt.Sprintf("[%s] %s", tag, format), args...)
+}


not a fan, we should use contextual logging, but for the time being I guess we can use this

ffromani · 2025-04-07T14:47:03Z

+
+// GetL3SharedCPUs creates a function that retrieves cpus for a given Node
+// takes a worker cnf node and returns a closure when called with cpuId returns
+// the corresponding cpus of core complex to which cpuId is part of


what's the benefit of returning a closure?

Benefit is i can call the GetL3SharedCPU's once with actual node and fetch the shared L3 CPU multiple times without the need to pass the node information everytime and also i can re use the same code if there are multiple nodes. In some test cases where we have multiple pods or multiple containers per pods , i think this would be helpful

ffromani · 2025-04-07T14:47:31Z

+			Expect(err).ToNot(HaveOccurred())
+			ctx := context.Background()
+			for _, cnfnode := range workerRTNodes {
+				numaInfo, err := nodes.GetNumaNodes(context.TODO(), &cnfnode)


use ctx here?

fixed in the latest commit

ffromani · 2025-04-07T14:48:17Z

+			// Modify the profile such that we give 1 whole ccx to reserved cpus
+			By("Modifying the profile")
+			for _, node := range workerRTNodes {
+				numaCoreSiblings, err = nodes.GetCoreSiblings(context.TODO(), &node)


use ctx here?

fixed in the latest commit

ffromani · 2025-04-07T14:48:59Z

+			// Assign one whole L3 Cache group for reserved cpus.
+			for reservedCores := 0; reservedCores < 8; reservedCores++ {
+				cpusiblings := nodes.GetAndRemoveCpuSiblingsFromMap(numaCoreSiblings, reservedCores)
+				reserved = append(reserved, cpusiblings...)


can we use a cpuset?

you mean to use cpuset.CPUset ?, Performance profile expects reserved and isolated to be of type string
https://github.com/openshift/cluster-node-tuning-operator/blob/81e7ec52274d/pkg/apis/performanceprofile/v2/performanceprofile_types.go#L95

yes, we can use a cpuset for higher level operations on it and translate to string only when needed.

@ffromani To convert, i need to first modify https://github.com/openshift/cluster-node-tuning-operator/blob/main/test/e2e/performanceprofile/functests/utils/nodes/nodes.go#L389 to return []int and then convert to cpuset.cpu using cpuset.new([]int..)

I can do it but this will break all the existing automation , i can address this in another PR if that's okay with you.

@ffromani i have created another PR for addressing this: #1321

ffromani · 2025-04-07T14:49:56Z

+				err := testclient.DataPlaneClient.Delete(ctx, dp)
+				Expect(err).ToNot(HaveOccurred())
+				// Wait for some time for deployment to be deleted
+				time.Sleep(deploymentDeletionTime)


would be better to check the pod to be gone, sleeps tend to fail randomly creating hard to debug issues

fixed in the latest commit

ffromani · 2025-04-07T14:51:12Z

+			testlog.TaggedInfof("L3 Cache Group", "L3 Cache group associated with Pod %s using cpu %d is: %q", testpod.Name, cgroupCpuset.List()[0], cpus)
+		})
+
+		It("[test_id:77726] Multiple Pods are not sharing same L3 cache", func() {


this may be possible per the latest KEP though, we need to check against the KEP goals/non-goals

mrniranjan · 2025-04-11T05:46:17Z

/retest-required

mrniranjan · 2025-04-11T05:46:36Z

/test okd-scos-e2e-aws-ovn

ffromani · 2025-04-11T14:35:43Z

+			// Assign one whole L3 Cache group for reserved cpus.
+			for reservedCores := 0; reservedCores < 8; reservedCores++ {
+				cpusiblings := nodes.GetAndRemoveCpuSiblingsFromMap(numaCoreSiblings, reservedCores)
+				reserved = append(reserved, cpusiblings...)


yes, we can use a cpuset for higher level operations on it and translate to string only when needed.

ffromani · 2025-04-11T14:37:39Z

+		})
+
+		It("[test_id:77725] Align Guaranteed pod requesting 16 cpus to the whole CCX if available", func() {
+			ctx := context.Background()


we can get a context from ginkgo:

It("[test_id:77725] Align Guaranteed pod requesting 16 cpus to the whole CCX if available", func(ctx context.Context) {

we will need to handle interruptions, but that's a good thing to do anyway

fixed in the latest commit

ffromani · 2025-04-11T14:38:12Z

+			cpusetCfg := &controller.CpuSet{}
+			deploymentName := "test-deployment1"
+			rl := &corev1.ResourceList{
+				corev1.ResourceCPU:    resource.MustParse("16"),


how 16 was decided? can't we use the CCX size? we should never assume the CCX size is 16

fixed in the latest commit

ffromani · 2025-04-11T14:39:14Z

+		})
+
+		It("[test_id:77725] Verify guaranteed pod consumes the whole Uncore group after reboot", func() {
+			ctx := context.Background()


fixed in the latest commit

ffromani · 2025-04-11T14:39:23Z

+			deploymentName := "test-deployment2"
+			getCCX := nodes.GetL3SharedCPUs(&targetNode)
+			rl := &corev1.ResourceList{
+				corev1.ResourceCPU:    resource.MustParse("16"),


fixed in the latest commit

mrniranjan · 2025-04-14T14:17:36Z

/test okd-scos-e2e-aws-ovn

ffromani

almost there

ffromani · 2025-04-16T11:14:49Z

+			// Get cpu siblings from core 0-7
+			// Assign one whole L3 Cache group for reserved cpus.


this is a bit fragile but without machineinfo or similar is challenging to improve

Agree, for now i have used cpuset.CPUSet . addressed in the latest commit

ffromani · 2025-04-16T11:17:36Z

+			testlog.TaggedInfof("L3 Cache Group", "L3 Cache group associated with Pod %s using cpu %d is %q: ", testpod.Name, cgroupCpuset.List()[0], cpus)
+		})
+
+		It("[test_id:77725]  Verify guaranteed pod consumes the whole Uncore group after kubelet restart", func(ctx context.Context) {


we can safely infer this invariant from other tests though. If the CPU assignment doesn't change across kubelet restart, because how the silicon phisically is, then the L3 affinity/assignment can't change either.

I guess we should keep this test for the sake of being explicity though.

ffromani · 2025-04-16T11:18:32Z

+
+		It("[test_id:77726] Multiple Pods are not sharing same L3 cache", func(ctx context.Context) {
+			targetNode := workerRTNodes[0]
+			// create 2 deployments creating 2 gu pods asking for 8 cpus each


we should ask for number of cpus in a L3 group size /2 rather than hardcode 8

Addressed in latest commit

ffromani · 2025-04-16T11:19:34Z

+			testlog.TaggedInfof("L3 Cache Group", "L3 Cache group associated with Pod %s using cpu %d is: %q", testpod.Name, cgroupCpuset.List()[0], cpus)
+		})
+
+		It("[test_id:77726] Multiple Pods are not sharing same L3 cache", func(ctx context.Context) {


the test name must be changed because comments at the bottom of the test acknowledge pods can indeed share the same L3 block

Addressed in latest commit

mrniranjan · 2025-04-17T06:23:56Z

/retest-required

Add function WaitForDesiredDeploymentStatus to wait for deployment to be in the desired status. This is needed in cases where we reboot the node and want to ascertain if the deployment has reached the desired status Minor fix: use existing context variable instead of creating new context Signed-off-by: Niranjan M.R <mrniranjan@redhat.com>

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

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

Instead of using hard coded value , fetch the value of L3 Cache group size and use that value for the guaranteed pod that we create Signed-off-by: Niranjan M.R <mrniranjan@redhat.com>

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

Use cpuset.CPUSet type for all reserved, isolated variables when modifying performance profile Signed-off-by: Niranjan M.R <mrniranjan@redhat.com>

Uncomment lines related to saving initialProfile Signed-off-by: Niranjan M.R <mrniranjan@redhat.com>

mrniranjan · 2025-04-24T16:34:26Z

/retest-required

ffromani

/approve
/lgtm

we can improve later if needed

ffromani · 2025-05-08T09:00:25Z

+		llcPolicy                   string
+		mc                          *machineconfigv1.MachineConfig
+		getter                      cgroup.ControllersGetter
+		cgroupV2                    bool


nit: can this a local variable in BeforeAll?

ffromani · 2025-05-08T09:01:17Z

+				getCCX := nodes.GetL3SharedCPUs(&cnfnode)
+				reserved, err = getCCX(0)
+				Expect(err).ToNot(HaveOccurred())
+				onlineCPUSet, err = nodes.GetOnlineCPUsSet(context.TODO(), &cnfnode)


should use ctx

ffromani · 2025-05-08T09:02:40Z

+			By(fmt.Sprintf("Applying changes in performance profile and waiting until %s will start updating", poolName))
+			profilesupdate.WaitForTuningUpdating(ctx, profile)


this wait is proven fragile though. A better approach seems to be to record the object ResourceVersion, then send the update, then wait for the object to become Updated with resourceVersion != oldResourceVersion

openshift-ci · 2025-05-08T09:14:08Z

[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

Details

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

openshift-ci-robot · 2025-05-09T12:02:56Z

@mrniranjan: This pull request references Jira Issue OCPBUGS-55989, 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.20.0) matches configured target version for branch (4.20.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @mrniranjan

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

Details

In response to this:

This PR adds Functional test to UncoreCache CPUManager policy options feature.

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-ci · 2025-05-09T12:02:59Z

@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: mrniranjan.

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

Details

In response to this:

@mrniranjan: This pull request references Jira Issue OCPBUGS-55989, 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.20.0) matches configured target version for branch (4.20.0)

bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @mrniranjan

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

In response to this:

This PR adds Functional test to UncoreCache CPUManager policy options feature.

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.

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.

yanirq · 2025-05-12T13:52:36Z

/retest-required

openshift-ci-robot · 2025-05-12T15:54:50Z

/retest-required

Remaining retests: 0 against base HEAD a3371e1 and 2 for PR HEAD 893e5eb in total

openshift-ci-robot · 2025-05-13T12:14:05Z

/retest-required

Remaining retests: 0 against base HEAD a3371e1 and 2 for PR HEAD 893e5eb in total

openshift-ci · 2025-05-13T14:13:47Z

@mrniranjan: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci-robot · 2025-05-13T14:16:43Z

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

openshift/cluster-node-tuning-operator#1302

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

Details

In response to this:

This PR adds Functional test to UncoreCache CPUManager policy options feature.

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-05-14T08:31:27Z

[ART PR BUILD NOTIFIER]

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

mrniranjan · 2025-06-06T06:50:10Z

/cherry-pick release-4.19

openshift-cherrypick-robot · 2025-06-06T06:51:13Z

@mrniranjan: new pull request created: #1336

Details

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.

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 Mar 4, 2025

mrniranjan marked this pull request as draft March 4, 2025 07:18

openshift-ci Bot requested review from ffromani and yanirq March 4, 2025 07:19

mrniranjan force-pushed the llc_take2 branch 4 times, most recently from 0259989 to 09870a6 Compare March 5, 2025 06:04

mrniranjan closed this Apr 2, 2025

mrniranjan force-pushed the llc_take2 branch from eddcb3d to 3324d90 Compare April 2, 2025 13:30

mrniranjan reopened this Apr 2, 2025

mrniranjan marked this pull request as ready for review April 2, 2025 13:32

mrniranjan changed the title ~~[wip] E2E: Functional tests for align cpus by UncoreCache Feature~~ E2E: Functional tests for align cpus by UncoreCache Feature Apr 2, 2025

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 2, 2025

openshift-ci Bot requested a review from swatisehgal April 2, 2025 13:35

mrniranjan force-pushed the llc_take2 branch from 3324d90 to 38300d6 Compare April 2, 2025 13:40

ffromani reviewed Apr 7, 2025

View reviewed changes

mrniranjan force-pushed the llc_take2 branch 2 times, most recently from 55148e2 to cf52930 Compare April 10, 2025 20:56

ffromani reviewed Apr 11, 2025

View reviewed changes

mrniranjan force-pushed the llc_take2 branch from db2c72b to b24d5bd Compare April 14, 2025 10:27

ffromani reviewed Apr 16, 2025

View reviewed changes

mrniranjan force-pushed the llc_take2 branch 2 times, most recently from be0c3dc to 9963e02 Compare April 16, 2025 13:34

mrniranjan added 2 commits April 24, 2025 16:54

Add feature add additional tags to log messages

9976d31

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

mrniranjan added 5 commits April 24, 2025 16:54

Use GetL3SharedCPUs function once and call the closure function to fetch

031eab6

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

Get L3 Cache Group size to pod equivalent to its size

0aba039

Instead of using hard coded value , fetch the value of L3 Cache group size and use that value for the guaranteed pod that we create Signed-off-by: Niranjan M.R <mrniranjan@redhat.com>

use ginkgo context managment instead

43cc417

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

Use L3 Cache group associated with cpu 0 as reserved

105960f

Use cpuset.CPUSet type for all reserved, isolated variables when modifying performance profile Signed-off-by: Niranjan M.R <mrniranjan@redhat.com>

Remove duplicate testlog.infof

893e5eb

Uncomment lines related to saving initialProfile Signed-off-by: Niranjan M.R <mrniranjan@redhat.com>

mrniranjan force-pushed the llc_take2 branch from 9963e02 to 893e5eb Compare April 24, 2025 11:24

mrniranjan mentioned this pull request Apr 30, 2025

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

Merged

ffromani reviewed May 8, 2025

View reviewed changes

openshift-ci Bot assigned ffromani May 8, 2025

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2025

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2025

mrniranjan changed the title ~~E2E: Functional tests for align cpus by UncoreCache Feature~~ OCPBUGS-55989: E2E: Functional tests for align cpus by UncoreCache Feature May 9, 2025

openshift-merge-bot Bot merged commit 488ca89 into openshift:main May 13, 2025

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

[release-4.19] OCPBUGS-57148: E2E: Functional tests for align cpus by UncoreCache Feature #1336

Merged

qodo-for-rh-openshift Bot mentioned this pull request Aug 6, 2026

WIP: E2E: Minor fixes to Memory Manager and hugepages split tests #1578

Draft

		// Get cpu siblings from core 0-7
		// Assign one whole L3 Cache group for reserved cpus.

		By(fmt.Sprintf("Applying changes in performance profile and waiting until %s will start updating", poolName))
		profilesupdate.WaitForTuningUpdating(ctx, profile)

Uh oh!

Conversation

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

Uh oh!

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!

mrniranjan commented Apr 11, 2025

Uh oh!

mrniranjan commented Apr 11, 2025

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 Apr 14, 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!

mrniranjan commented Mar 4, 2025 •

edited

Loading