Skip to content

Set cgroup values via libcontainer#98374

Closed
odinuge wants to merge 1 commit intokubernetes:masterfrom
odinuge:cgroup-systemd-set
Closed

Set cgroup values via libcontainer#98374
odinuge wants to merge 1 commit intokubernetes:masterfrom
odinuge:cgroup-systemd-set

Conversation

@odinuge
Copy link
Copy Markdown
Member

@odinuge odinuge commented Jan 25, 2021

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

WIP to run all e2e tests.

Currently we set cgroup values in pkg/kubelet/cm/cgroup_manager_linux.go manually per subsystem, howerever, when using the systemd driver, we should set them via opencontainers/runc/libcontainer to make sure we update the systemd slices and scopes.

Will come back with more info when I have tested more.

Which issue(s) this PR fixes:

Fixes #88197

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix resource enforcement when using systemd cgroup driver

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 25, 2021
@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Jan 25, 2021

/retest

@odinuge odinuge force-pushed the cgroup-systemd-set branch from ae50fad to c9f9d74 Compare January 26, 2021 12:20
@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 4, 2021

/retest

2 similar comments
@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 4, 2021

/retest

@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 4, 2021

/retest

@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 4, 2021

/test pull-kubernetes-node-crio-cgrpv2-e2e

@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 4, 2021

Still having some issues with cgroup v2 that need to be addressed. However, cc:

/cc @rphillips @olegch

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

@odinuge: GitHub didn't allow me to request PR reviews from the following users: olegch.

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

Details

In response to this:

Still having some issues with cgroup v2 that need to be addressed. However, cc:

/cc @rphillips @olegch

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.

@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 4, 2021

/retest

@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 4, 2021

/test pull-kubernetes-node-crio-cgrpv2-e2e

@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 5, 2021

/retest

@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 5, 2021

/test pull-kubernetes-node-crio-cgrpv2-e2e

@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 5, 2021

/retest

@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 5, 2021

/test pull-kubernetes-node-crio-cgrpv2-e2e

1 similar comment
@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented Feb 6, 2021

/test pull-kubernetes-node-crio-cgrpv2-e2e

@odinuge odinuge force-pushed the cgroup-systemd-set branch from 94f610c to e5787c8 Compare May 19, 2021 11:12
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2021
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: odinuge
To complete the pull request process, please ask for approval from klueska after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

@odinuge odinuge force-pushed the cgroup-systemd-set branch from e5787c8 to 86403ba Compare May 19, 2021 11:13
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2021
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented May 19, 2021

@odinuge: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-node-crio-cgrpv2-e2e 94f610cc7ebd7eba7813059323a46a0c3fcb2052 link /test pull-kubernetes-node-crio-cgrpv2-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

@odinuge odinuge force-pushed the cgroup-systemd-set branch from 86403ba to 83a6fbb Compare May 19, 2021 11:16
This sets cgroup config via libcontainer to make sure we apply the
correct values to the systemd slices and scopes.
@odinuge odinuge force-pushed the cgroup-systemd-set branch from 83a6fbb to f72158a Compare May 19, 2021 11:17
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. labels May 19, 2021
if unified {
libcontainerCgroupConfig.Path = m.Name(cgroupConfig.Name)
} else {
libcontainerCgroupConfig.Paths = m.buildCgroupPaths(cgroupConfig.Name)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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


// setSupportedSubsystemsV1 sets cgroup resource limits on cgroup v1 only on the supported
// subsystems. ie. cpu and memory. We don't use libcontainer's cgroup/fs/Set()
// method as it doesn't allow us to skip updates on the devices cgroup
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is now resolved by setting Resources.SkipDevices = true.

@harche
Copy link
Copy Markdown
Contributor

harche commented May 19, 2021

The last run for the job pull-kubernetes-node-crio-e2e with the latest changes seem to have passed for crio + systemd configuration https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/98374/pull-kubernetes-node-crio-e2e/1394975602431234048/

Putting it here explicitly because that job doesn't show up in the ci job list.

@dims
Copy link
Copy Markdown
Member

dims commented May 19, 2021

Great news @harche !

@ehashman
Copy link
Copy Markdown
Member

/test pull-kubernetes-node-crio-e2e

@dims
Copy link
Copy Markdown
Member

dims commented May 19, 2021

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label May 19, 2021
@kolyshkin
Copy link
Copy Markdown
Contributor

I have included the commit from this PR to #102147, as it fixes the regression caused by rc94/rc95 update.

@kolyshkin
Copy link
Copy Markdown
Contributor

I think this one can be closed in favor of #102147 (which includes a version of commit from this PR for runc rc94+).

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

@odinuge: PR needs rebase.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2021
@odinuge
Copy link
Copy Markdown
Member Author

odinuge commented May 21, 2021

/close

In favor of #102147

@odinuge odinuge closed this May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

Development

Successfully merging this pull request may close these issues.

kubelet incorrectly configures kubepods.slice systemd unit when kube-reserved and/or system-reserved is used

10 participants