CNF-9173: e2e: cgroups: introduce cgroup package #906

Tal-or · 2024-01-04T16:08:36Z

This package provides a Getter which is cgroup version agnostic.
It provides a unified api for testing the mixedcpus feature for both v1 and v2.

The package can be expand in the future for testing additional features that requires cgroups inspection. (for example runc vs crun)

How to use:

getter := BuildGetter(context.TODO(), controllerRuntimeClient, k8sClient)
cpusetCfg := &controller.CpuSet{}
err := getter.Container(context.TODO(), pod, containerName, cpusetCfg)
// error handling
fmt.Println(cpusetCfg.Cpus)

Tal-or · 2024-01-04T16:08:55Z

/cc @mrniranjan

Tal-or · 2024-01-04T16:10:59Z

This PR was part of #892 which we decide to extract in order merge it more quickly since it needed for the cgroupv2 testing

openshift-ci-robot · 2024-01-04T16:18:23Z

@Tal-or: This pull request references CNF-9173 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

This package provides a Getter which is cgroup version agnostic.
It provides a unified api for testing the mixedcpus feature for both v1 and v2.

The package can be expand in the future for testing additional features that requires cgroups inspection. (for example runc vs crun)

How to use:
getter := BuildGetter(context.TODO())
cfg := getter.GetConfig(context.TODO(), client, pod,containerName)

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

looks nice, but we need some form of testing, at least partial

ffromani · 2024-01-04T16:31:42Z

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

+	Period string
+}
+
+type Getter interface {


I'd actually get rid of this interface entirely.
Plus, in golang better to define interface on the consumer side: https://www.thoughtworks.com/insights/blog/programming-languages/mistakes-to-avoid-when-coming-from-an-object-oriented-language

Thank you for the useful information, it really helps understand the principles better.

So in our case it means the interface would be implemented in the test files themselves, i.e. 11_mixedcpus/mixedcpus.go and optionally 7_performance_kubelet_node/cgroups.go.
But what if both packages want to use the exact interface? should both declare it?

And my second questions is how do you suggest the BuildGetter to return a struct, while it suppose to return an abstraction for getting the cgroup information.

Of course we can implement the BuildGetter on the consumer side (where there it would make sense to expect interface), but again it's a duplication of code, since at least two packages (from what I know of) would need to implement it to retrieve the existing cgroup configuration on the node.

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

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

mrniranjan · 2024-01-08T09:02:30Z

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

+	CPUSet string
+	Quota  string
+	Period string
+}


Can we define structure in a more hierarchial fashion i.e: Controllers->Controller-interfacefiles

type CgroupController struct { //suported cgroup Controllers cpuset cpu // any future cgroup controllers }

Then created methods that return interface files of that particular controller ?

mrniranjan · 2024-01-08T09:05:41Z

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

+
+func (m *Manager) GetConfig(ctx context.Context, c *kubernetes.Clientset, pod *corev1.Pod, containerName string) (*config.Config, error) {
+	var cmd []string
+	cfg := &config.Config{}


GetConfig here supports getting here only cpuset.cpus, cpu.max, but there are other interface files that equally important and we do need them in future like
cpuset.cpu.partition, cpuset.cpus.effective, cpuset.mems (this is specifically required for memory manager to know which numa interface the pod has been assigned).

mrniranjan · 2024-01-08T09:11:21Z

This PR is a good starting point, i think we can improve it in further iteration

openshift-ci-robot · 2024-01-10T20:27:00Z

@Tal-or: This pull request references CNF-9173 which is a valid jira issue.

In response to this:

This package provides a Getter which is cgroup version agnostic.
It provides a unified api for testing the mixedcpus feature for both v1 and v2.

The package can be expand in the future for testing additional features that requires cgroups inspection. (for example runc vs crun)

How to use:
getter := BuildGetter(context.TODO(), controllerRuntimeClient, k8sClient)
cfg := getter.GetConfig(context.TODO(), client, pod,containerName)

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-robot · 2024-01-10T20:29:09Z

@Tal-or: This pull request references CNF-9173 which is a valid jira issue.

In response to this:

This package provides a Getter which is cgroup version agnostic.
It provides a unified api for testing the mixedcpus feature for both v1 and v2.

The package can be expand in the future for testing additional features that requires cgroups inspection. (for example runc vs crun)

How to use:
getter := BuildGetter(context.TODO(), controllerRuntimeClient, k8sClient)
cpusetCfg := controller.CpuSet{}
err := getter.Container(context.TODO(), client, pod, containerName, cpusetCfg)
// error handling
cpusetCfg.Cpus

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-robot · 2024-01-10T20:29:25Z

@Tal-or: This pull request references CNF-9173 which is a valid jira issue.

In response to this:

This package provides a Getter which is cgroup version agnostic.
It provides a unified api for testing the mixedcpus feature for both v1 and v2.

The package can be expand in the future for testing additional features that requires cgroups inspection. (for example runc vs crun)

How to use:
getter := BuildGetter(context.TODO(), controllerRuntimeClient, k8sClient)
cpusetCfg := &controller.CpuSet{}
err := getter.Container(context.TODO(), client, pod, containerName, cpusetCfg)
// error handling
cpusetCfg.Cpus

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-robot · 2024-01-10T20:29:52Z

@Tal-or: This pull request references CNF-9173 which is a valid jira issue.

In response to this:

This package provides a Getter which is cgroup version agnostic.
It provides a unified api for testing the mixedcpus feature for both v1 and v2.

The package can be expand in the future for testing additional features that requires cgroups inspection. (for example runc vs crun)

How to use:
getter := BuildGetter(context.TODO(), controllerRuntimeClient, k8sClient)
cpusetCfg := &controller.CpuSet{}
err := getter.Container(context.TODO(), pod, containerName, cpusetCfg)
// error handling
cpusetCfg.Cpus

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-robot · 2024-01-10T20:30:09Z

@Tal-or: This pull request references CNF-9173 which is a valid jira issue.

In response to this:

This package provides a Getter which is cgroup version agnostic.
It provides a unified api for testing the mixedcpus feature for both v1 and v2.

The package can be expand in the future for testing additional features that requires cgroups inspection. (for example runc vs crun)

How to use:
getter := BuildGetter(context.TODO(), controllerRuntimeClient, k8sClient)
cpusetCfg := &controller.CpuSet{}
err := getter.Container(context.TODO(), pod, containerName, cpusetCfg)
// error handling
fmt.Println(cpusetCfg.Cpus)

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-01-10T20:31:51Z

@mrniranjan IMO the existing implementation is good enough for now and we can expand it later

ffromani · 2024-01-15T06:38:18Z

it's probabably wasteful to retry until we can consume cri-o/cri-o#7643 in our CI clusters

mrniranjan · 2024-01-15T07:30:24Z

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

+}
+
+func (cm *ControllersManager) Cpu(ctx context.Context, pod *corev1.Pod, containerName, runtimeType string) (*controller.Cpu, error) {
+	cfg := &controller.Cpu{}


We are passing runtimeType but we are not using this variable in Cpu function

Yes I need to learn the details, i just passed the argument for now

mrniranjan · 2024-01-15T07:30:39Z

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

+	return &ControllersManager{client: c, k8sClient: k8sClient}
+}
+
+func (cm *ControllersManager) CpuSet(ctx context.Context, pod *corev1.Pod, containerName, runtimeType string) (*controller.CpuSet, error) {


We are passing runtimeType but we are not using this variable in CpuSet function

mrniranjan · 2024-01-15T07:32:34Z

LGTM from my side, we can improve this further in future iterations.

Tal-or · 2024-01-15T09:27:16Z

it's probabably wasteful to retry until we can consume cri-o/cri-o#7643 in our CI clusters

This is why the updating lane is falling?

Tal-or · 2024-01-16T08:46:05Z

/test-required

Tal-or · 2024-01-16T08:46:25Z

/test required

openshift-ci · 2024-01-16T08:46:36Z

@Tal-or: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test e2e-aws-operator
/test e2e-aws-ovn
/test e2e-aws-ovn-techpreview
/test e2e-gcp-pao
/test e2e-gcp-pao-updating-profile
/test e2e-gcp-pao-workloadhints
/test e2e-hypershift
/test e2e-no-cluster
/test e2e-upgrade
/test images
/test unit
/test verify
/test vet

The following commands are available to trigger optional jobs:

/test lint

Use /test all to run all jobs.

In response to this:

/test required

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.

In case no name provided, it takes the name from the first container in the pod. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

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

Tal-or · 2024-01-18T17:34:11Z

/hold cancel

Tal-or · 2024-01-21T12:03:33Z

/retest

This package provides a ControllerGetter which is cgroup version agnostic. It provides a unified api for testing the mixedcpus feature for both v1 and v2. The package can be expand in the future for testing additional features that requires cgroups inspection. Also in the future this package should be runtime (crun/runc) agnostic. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

Tal-or · 2024-01-29T16:52:10Z

/retest

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

The exec command might return empty value. In order to have better control of what we get we should read the files in separate exec calls. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

mrniranjan · 2024-02-01T05:28:13Z

/lgtm

ffromani

LGTM
nice addition

ffromani · 2024-02-05T14:32:52Z

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

@@ -148,7 +148,7 @@ var _ = Describe("[performance]Hugepages", Ordered, func() {
 			Expect(err).ToNot(HaveOccurred())

 			cmd2 := []string{"/bin/bash", "-c", "tmux new -d 'LD_PRELOAD=libhugetlbfs.so HUGETLB_MORECORE=yes top -b > /dev/null'"}
-			_, err = pods.ExecCommandOnPod(testclient.K8sClient, testpod, cmd2)
+			_, err = pods.ExecCommandOnPod(testclient.K8sClient, testpod, "", cmd2)


this works. An alternative option could have been to add a variant function which accepts a container name and reimplement the existing ExecCommandOnPod (with the current signature) on top of it. But we can keep this approach.

Yes this is an option as well.

ffromani · 2024-02-05T14:38:54Z

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

@@ -99,3 +93,25 @@ func (cm *ControllersManager) Child(ctx context.Context, pod *corev1.Pod, contai
 	// TODO
 	return nil
 }
+
+func (cm *ControllersManager) execAndStore(pod *corev1.Pod, containerName, dirPath string, store map[string]*string) error {


not a big fan of mutating arguments (and of maps whose values are pointers) but I see why you are doing like this and I don't have compelling suggestions

Yea, it was the only way to generalize the calls per each controller file

ffromani · 2024-02-05T14:40:32Z

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

@@ -98,3 +92,24 @@ func (cm *ControllersManager) Child(ctx context.Context, pod *corev1.Pod, contai
 	}
 	return nil
 }
+func (cm *ControllersManager) execAndStore(pod *corev1.Pod, containerName, dirPath string, store map[string]*string) error {


this seems one of the instances on which seems better to generalize the function. But I think you did not because the v1 and v2 package have no common package to share library functions, and adding one doesn't seem so great. If that's the case I agree to keep it like this for starters.

Thanks. We may consider shared library in the future in case of additional common functions.

ffromani · 2024-02-05T14:41:14Z

/approve

we want this change in

ffromani · 2024-02-05T14:44:12Z

/approve

openshift-ci · 2024-02-05T14:49:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, Tal-or

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

openshift-ci · 2024-02-05T17:29:38Z

@Tal-or: 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-bot · 2024-02-05T19:41:18Z

[ART PR BUILD NOTIFIER]

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

Tal-or · 2024-02-08T15:04:58Z

/cherry-pick release-4.15

Needed for mixed-cpus e2e testing

openshift-cherrypick-robot · 2024-02-08T15:05:48Z

@Tal-or: #906 failed to apply on top of branch "release-4.15":

Applying: e2e: change `ExecCommandOnPod` to accept container name
Using index info to reconstruct a base tree...
M	test/e2e/performanceprofile/functests/1_performance/hugepages.go
M	test/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.go
M	test/e2e/performanceprofile/functests/utils/pods/pods.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/performanceprofile/functests/utils/pods/pods.go
CONFLICT (content): Merge conflict in test/e2e/performanceprofile/functests/utils/pods/pods.go
Auto-merging test/e2e/performanceprofile/functests/7_performance_kubelet_node/cgroups.go
Auto-merging test/e2e/performanceprofile/functests/1_performance/hugepages.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 e2e: change `ExecCommandOnPod` to accept container name
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.15

Needed for mixed-cpus e2e testing

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-ci bot requested a review from mrniranjan January 4, 2024 16:09

openshift-ci bot requested review from ffromani and yanirq January 4, 2024 16:15

Tal-or changed the title ~~e2e: cgroups: introduce cgroup package~~ CNF-9173: e2e: cgroups: introduce cgroup package Jan 4, 2024

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

ffromani reviewed Jan 4, 2024

View reviewed changes

Tal-or force-pushed the e2e_cgroups_util branch from 0891e79 to b42c534 Compare January 7, 2024 10:42

mrniranjan reviewed Jan 8, 2024

View reviewed changes

This was referenced Jan 9, 2024

CNF-9173: e2e: mixedcpus test #892

Merged

CNF-9173: e2e: mixedcpus: kubelet restart test #912

Merged

Tal-or force-pushed the e2e_cgroups_util branch from b42c534 to 59b3f14 Compare January 10, 2024 20:26

mrniranjan reviewed Jan 15, 2024

View reviewed changes

e2e: change ExecCommandOnPod to accept container name

69a21cd

In case no name provided, it takes the name from the first container in the pod. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

Tal-or force-pushed the e2e_cgroups_util branch from 1cc1cde to 81e2c02 Compare January 18, 2024 08:36

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2024

mrniranjan reviewed Jan 18, 2024

View reviewed changes

test/e2e/performanceprofile/functests/utils/cgroup/cgroup.go Outdated Show resolved Hide resolved

Tal-or force-pushed the e2e_cgroups_util branch from 81e2c02 to 36a1f9e Compare January 18, 2024 17:31

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

Tal-or force-pushed the e2e_cgroups_util branch from 36a1f9e to 58b3575 Compare January 28, 2024 18:08

mrniranjan reviewed Jan 30, 2024

View reviewed changes

test/e2e/performanceprofile/functests/utils/cgroup/v1/v1.go Show resolved Hide resolved

cgroup: exec separatly for each controller file

129c80a

The exec command might return empty value. In order to have better control of what we get we should read the files in separate exec calls. Signed-off-by: Talor Itzhak <titzhak@redhat.com>

openshift-ci bot assigned mrniranjan Feb 1, 2024

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

Tal-or requested a review from ffromani February 1, 2024 08:10

ffromani reviewed Feb 5, 2024

View reviewed changes

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

openshift-merge-bot bot merged commit 51e073f into openshift:master Feb 5, 2024
15 checks passed

Tal-or deleted the e2e_cgroups_util branch February 5, 2024 18:18

Tal-or mentioned this pull request Feb 8, 2024

[release-4.15] [manual] OCPBUGS-29376: e2e: cgroups: introduce cgroup package #942

Merged

rbaturov mentioned this pull request Apr 17, 2024

OCPBUGS-32407: [release-4.14] [manual]: Systemd processes not being moved to cpuset/systemd.slice fix #1040

Open

CNF-9173: e2e: cgroups: introduce cgroup package #906

CNF-9173: e2e: cgroups: introduce cgroup package #906

Conversation

Tal-or commented Jan 4, 2024 • edited

Tal-or commented Jan 4, 2024

Tal-or commented Jan 4, 2024

openshift-ci-robot commented Jan 4, 2024 • edited by openshift-ci bot

ffromani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tal-or Jan 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrniranjan commented Jan 8, 2024

openshift-ci-robot commented Jan 10, 2024 • edited by openshift-ci bot

openshift-ci-robot commented Jan 10, 2024 • edited by openshift-ci bot

openshift-ci-robot commented Jan 10, 2024 • edited by openshift-ci bot

openshift-ci-robot commented Jan 10, 2024 • edited by openshift-ci bot

openshift-ci-robot commented Jan 10, 2024 • edited by openshift-ci bot

Tal-or commented Jan 10, 2024

ffromani commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrniranjan commented Jan 15, 2024

Tal-or commented Jan 15, 2024

Tal-or commented Jan 16, 2024

Tal-or commented Jan 16, 2024

openshift-ci bot commented Jan 16, 2024

Tal-or commented Jan 18, 2024

Tal-or commented Jan 21, 2024

Tal-or commented Jan 29, 2024

mrniranjan commented Feb 1, 2024

ffromani 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

ffromani commented Feb 5, 2024

ffromani commented Feb 5, 2024

openshift-ci bot commented Feb 5, 2024

openshift-ci bot commented Feb 5, 2024

openshift-bot commented Feb 5, 2024

Tal-or commented Feb 8, 2024

openshift-cherrypick-robot commented Feb 8, 2024

Tal-or commented Jan 4, 2024 •

edited

openshift-ci-robot commented Jan 4, 2024 •

edited by openshift-ci bot

Tal-or Jan 7, 2024 •

edited

openshift-ci-robot commented Jan 10, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Jan 10, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Jan 10, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Jan 10, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Jan 10, 2024 •

edited by openshift-ci bot