Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from Feb 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69a21cd
e2e: change `ExecCommandOnPod` to accept container name
Tal-or Dec 14, 2023
58b3575
e2e:utils: introduce cgroup package
Tal-or Dec 19, 2023
129c80a
cgroup: exec separatly for each controller file
Tal-or Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/e2e/performanceprofile/functests/utils/cgroup/controller/controller.go
View file
Open in desktop
Expand Up @@ -10,7 +10,7 @@ type CpuSet struct {
Partition string
Exclusive string
// SchedLoadBalance true if enabled, only applicable for cgroupv1
SchedLoadBalance bool
SchedLoadBalance string
}

type Cpu struct {
Expand Down
52 changes: 34 additions & 18 deletions test/e2e/performanceprofile/functests/utils/cgroup/v1/v1.go
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/cgroup/controller"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/cgroup/runtime"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/log"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/pods"
)

Expand All @@ -26,35 +27,28 @@ func NewManager(c client.Client, k8sClient *kubernetes.Clientset) *ControllersMa

func (cm *ControllersManager) CpuSet(ctx context.Context, pod *corev1.Pod, containerName, childName, runtimeType string) (*controller.CpuSet, error) {
cfg := &controller.CpuSet{}
dirPath := path.Join(controller.CgroupMountPoint, childName)
cmd := []string{
"/bin/cat",
dirPath + "/cpuset/cpuset.cpus",
dirPath + "/cpuset/cpuset.cpu_exclusive",
dirPath + "/cpuset/cpuset.effective_cpus",
dirPath + "/cpuset/cpuset.mems",
dirPath + "/cpuset/cpuset.sched_load_balance",
dirPath := path.Join(controller.CgroupMountPoint, "cpuset", childName)
store := map[string]*string{
"cpuset.cpus": &cfg.Cpus,
"cpuset.cpus.exclusive": &cfg.Exclusive,
"cpuset.cpus.effective": &cfg.Effective,
"cpuset.sched_load_balance": &cfg.SchedLoadBalance,
"cpuset.mems": &cfg.Mems,
}
b, err := pods.ExecCommandOnPod(cm.k8sClient, pod, containerName, cmd)
err := cm.execAndStore(pod, containerName, dirPath, store)
if err != nil {
return nil, fmt.Errorf("failed to retrieve cgroup config for pod. pod=%q, container=%q; %w", client.ObjectKeyFromObject(pod).String(), containerName, err)
}
Tal-or marked this conversation as resolved.
Show resolved Hide resolved
output := strings.Split(string(b), "\r\n")
cfg.Cpus = output[0]
cfg.Exclusive = output[1]
cfg.Effective = output[2]
cfg.Mems = output[3]
cfg.SchedLoadBalance = output[4] == "1"
return cfg, nil
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dirPath := path.Join(controller.CgroupMountPoint, childName)
dirPath := path.Join(controller.CgroupMountPoint, "cpu", childName)
cmd := []string{
"/bin/cat",
dirPath + "/cpu/cpu.cfs_quota_us",
dirPath + "/cpu/cpu.cfs_period_us",
dirPath + "/cpu.cfs_quota_us",
dirPath + "/cpu.cfs_period_us",
}
b, err := pods.ExecCommandOnPod(cm.k8sClient, pod, containerName, cmd)
if err != nil {
Tal-or marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for k, v := range store {
fullPath := dirPath + "/" + k
cmd := []string{
"/bin/cat",
fullPath,
}
b, err := pods.ExecCommandOnPod(cm.k8sClient, pod, containerName, cmd)
if err != nil {
return err
}
if len(b) == 0 {
log.Warningf("empty value in cgroupv1 controller file; pod=%q,container=%q,file=%q", pod.Name, containerName, fullPath)
*v = ""
continue
}
output := strings.Trim(string(b), "\r\n")
*v = output
}
return nil
}
43 changes: 29 additions & 14 deletions test/e2e/performanceprofile/functests/utils/cgroup/v2/v2.go
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/cgroup/controller"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/cgroup/runtime"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/log"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/pods"
)

Expand All @@ -27,24 +28,17 @@ func NewManager(c client.Client, k8sClient *kubernetes.Clientset) *ControllersMa
func (cm *ControllersManager) CpuSet(ctx context.Context, pod *corev1.Pod, containerName, childName, runtimeType string) (*controller.CpuSet, error) {
cfg := &controller.CpuSet{}
dirPath := path.Join(controller.CgroupMountPoint, childName)
cmd := []string{
"/bin/cat",
dirPath + "/cpuset.cpus",
dirPath + "/cpuset.cpus.exclusive",
dirPath + "/cpuset.cpus.effective",
dirPath + "/cpuset.cpus.partition",
dirPath + "/cpuset.mems",
store := map[string]*string{
"cpuset.cpus": &cfg.Cpus,
"cpuset.cpus.exclusive": &cfg.Exclusive,
"cpuset.cpus.effective": &cfg.Effective,
"cpuset.cpus.partition": &cfg.Partition,
"cpuset.mems": &cfg.Mems,
}
b, err := pods.ExecCommandOnPod(cm.k8sClient, pod, containerName, cmd)
err := cm.execAndStore(pod, containerName, dirPath, store)
if err != nil {
return nil, fmt.Errorf("failed to retrieve cgroup config for pod. pod=%q, container=%q; %w", client.ObjectKeyFromObject(pod).String(), containerName, err)
}
output := strings.Split(string(b), "\r\n")
cfg.Cpus = output[0]
cfg.Exclusive = output[1]
cfg.Effective = output[2]
cfg.Partition = output[3]
cfg.Mems = output[4]
return cfg, nil
}

Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for k, v := range store {
fullPath := dirPath + "/" + k
cmd := []string{
"/bin/cat",
fullPath,
}
b, err := pods.ExecCommandOnPod(cm.k8sClient, pod, containerName, cmd)
if err != nil {
return err
}
if len(b) == 0 {
log.Warningf("empty value in cgroupv2 controller file; pod=%q,container=%q,file=%q", pod.Name, containerName, fullPath)
*v = ""
continue
}
output := strings.Trim(string(b), "\r\n")
*v = output
}
return nil
}