Skip to content

cgroup names created by kubelet should be lowercased#42497

Merged
k8s-github-robot merged 1 commit into
kubernetes:masterfrom
derekwaynecarr:lower_cgroup_names
Mar 7, 2017
Merged

cgroup names created by kubelet should be lowercased#42497
k8s-github-robot merged 1 commit into
kubernetes:masterfrom
derekwaynecarr:lower_cgroup_names

Conversation

@derekwaynecarr

@derekwaynecarr derekwaynecarr commented Mar 3, 2017

Copy link
Copy Markdown
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

derekwaynecarr commented Mar 3, 2017

Copy link
Copy Markdown
Member Author

/cc @vishh

@derekwaynecarr derekwaynecarr mentioned this pull request Mar 3, 2017
Merged
@k8s-reviewable

k8s-reviewable commented Mar 3, 2017

Copy link
Copy Markdown

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

derekwaynecarr commented Mar 3, 2017

Copy link
Copy Markdown
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

smarterclayton commented Mar 3, 2017

Copy link
Copy Markdown
Contributor

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

@sjenning

sjenning commented Mar 3, 2017

Copy link
Copy Markdown
Contributor

@derekwaynecarr i think this needs to be updated too

https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/pods_container_manager_test.go#L157

or here

https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/pods_container_manager_test.go#L64

@vishh

vishh commented Mar 3, 2017

Copy link
Copy Markdown
Contributor

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

vishh commented Mar 3, 2017

Copy link
Copy Markdown
Contributor

Will tag once the tests pass.

@derekwaynecarr

derekwaynecarr commented Mar 3, 2017

Copy link
Copy Markdown
Member Author

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

@vishh

vishh commented Mar 3, 2017

Copy link
Copy Markdown
Contributor

/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

k8s-github-robot commented Mar 3, 2017

Copy link
Copy Markdown

[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

derekwaynecarr commented Mar 6, 2017

Copy link
Copy Markdown
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

derekwaynecarr commented Mar 6, 2017

Copy link
Copy Markdown
Member Author

@k8s-bot kops aws e2e test this

@derekwaynecarr

derekwaynecarr commented Mar 7, 2017

Copy link
Copy Markdown
Member Author

GCE failure is #42597

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

@derekwaynecarr

derekwaynecarr commented Mar 7, 2017

Copy link
Copy Markdown
Member Author

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

@k8s-github-robot

k8s-github-robot commented Mar 7, 2017

Copy link
Copy Markdown

Automatic merge from submit-queue

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

vishh commented Mar 7, 2017 via email

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sjenning sjenning Awaiting requested review from sjenning

1 more reviewer

@vishh vishh vishh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vishh vishh

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

Milestone

v1.6

Development

Successfully merging this pull request may close these issues.

8 participants

@derekwaynecarr @k8s-reviewable @smarterclayton @sjenning @vishh @k8s-github-robot @resouer @k8s-ci-robot