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.

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

cgroup names created by kubelet should be lowercased #42497

Merged

Conversation

derekwaynecarr
Copy link
Member

What this PR does / why we need it:
This PR modifies the kubelet to create cgroupfs names that are lowercased. This better aligns us with the naming convention for cgroups v2 and other cgroup managers in ecosystem (docker, systemd, etc.)

See: https://www.kernel.org/doc/Documentation/cgroup-v2.txt
"2-6-2. Avoid Name Collisions"

Special notes for your reviewer:
none

Release note:

kubelet created cgroups follow lowercase naming conventions

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 3, 2017
@derekwaynecarr
Copy link
Member Author

/cc @vishh

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 3, 2017
@derekwaynecarr derekwaynecarr assigned vishh and unassigned resouer Mar 3, 2017
@derekwaynecarr derekwaynecarr requested a review from sjenning March 3, 2017 20:09
@derekwaynecarr derekwaynecarr added this to the v1.6 milestone Mar 3, 2017
@derekwaynecarr derekwaynecarr added the kind/bug Categorizes issue or PR as related to a bug. label Mar 3, 2017
@derekwaynecarr
Copy link
Member Author

for reference, i believe future users will view our naming syntax confusing with its use of mixed case. as a result, i think this is a bug. there is no issue to reference, as users have not tried integrating or introspecting the cgroupfs yet.

@smarterclayton
Copy link
Contributor

Agree this is a bug in that we are potentially causing problems in the future

@vishh
Copy link
Contributor

vishh commented Mar 3, 2017

One more option is to have cgroup manager default to lower case names. It might break tests though and could lead to confusion if the defaulting isn't obvious.

@vishh vishh added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2017
@vishh
Copy link
Contributor

vishh commented Mar 3, 2017

Will tag once the tests pass.

@derekwaynecarr
Copy link
Member Author

everything but the cross build is happy due to node problem detector problems.

@vishh
Copy link
Contributor

vishh commented Mar 3, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: derekwaynecarr, vishh

Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @timstclair
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 6, 2017
@derekwaynecarr
Copy link
Member Author

rebased, retagging.

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2017
@derekwaynecarr
Copy link
Member Author

@k8s-bot kops aws e2e test this

@derekwaynecarr
Copy link
Member Author

GCE failure is #42597

The other is a federation flake not impacted by this pr.

@derekwaynecarr
Copy link
Member Author

@k8s-bot unit test this
@k8s-bot cvm gce e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 31db570 into kubernetes:master Mar 7, 2017
@vishh
Copy link
Contributor

vishh commented Mar 7, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants