Attempt 2: Bump GCE containerVM to container-v1-3-v20160517 (Docker 1.11.1) again. #26001

dchen1107 · 2016-05-20T23:22:23Z

Workaround the issue of small root_maxkeys on the debian based container-vm image, and bump our image to the new alpha version for docker 1.11.1 validation.

ref: #23397 #25893

cc/ @vishh @timstclair

timstclair · 2016-05-20T23:33:39Z

cmd/kubelet/app/server.go

+	err = ioutil.WriteFile(path, []byte(fmt.Sprintf("%d", uint32(1000000))), 0644)
+	if err != nil {
+		return fmt.Errorf("failed to update %s: %v", path, err)
+	}


Ideally we would rebuild the image... should we do this in the salt configuration as a temporary measure?

I just don't want to cut another image release since it is time sink at validation stage. We shouldn't put this to image once we finished the validation. I am ok to go with salt, but thought there is no difference since it is a temporary solution.

cc/ @mikedanese for salt configuration

The difference is we can limit the change in salt to the container VM GCE setup, but here it goes out to everyone.

Yes, this one is goes to everyone. The number I chose (1000000) is actually the default one for other os distro including CoreOS, GCI, etc. If you want, I can read the number and reconfig it if the configured number < the default one?

@mikedanese is ok with updating this through salt. I will re-do this then.

I gave up on salt. Here is the block change I made,

{% if grains.cloud is defined
and grains.cloud == 'gce' %}
/proc/sys/kernel/keys/root_maxkeys:
file.managed:
- content: |
- '200000'
- replace: True
- show_diff: True
{% endif %}

But salt think the file has right content, and refuse to update:

[INFO ] Running state [/proc/sys/kernel/keys/root_maxkeys] at time 00:27:59.681909
[INFO ] Executing state file.managed for /proc/sys/kernel/keys/root_maxkeys
[INFO ] File /proc/sys/kernel/keys/root_maxkeys is in the correct state
[INFO ] Completed state [/proc/sys/kernel/keys/root_maxkeys] at time 00:27:59.684949
ID: /proc/sys/kernel/keys/root_maxkeys
Comment: File /proc/sys/kernel/keys/root_maxkeys is in the correct state

Oh well... let's just try to remember to remove this once the fix gets in the image.

We have set sysctl's before and we have always done it in code. Why wouldn't you want this in all clouds? I prefer this approach.

+1. We need to change this logic to be run only when needed.

dchen1107 · 2016-05-21T01:07:04Z

Addressed the comments, PTAL?

timstclair · 2016-05-21T01:10:05Z

cmd/kubelet/app/server.go

+	if err != nil {
+		return fmt.Errorf("failed to update %s: %v", maxkey_path, err)
+	}
+	const maxbyte_path = "/proc/sys/kernel/keys/root_maxbytes"


Nit: no underscore in go constants

timstclair · 2016-05-21T01:10:27Z

LGTM

vishh · 2016-05-21T01:11:22Z

This PR is fine. But I'd rather see this logic reside in pkg/kubelet/cm where we validate other kernel parameters. The move can happen in a separate PR. We only need to apply if the existing limits are not satisfactory.

vishh · 2016-05-21T01:11:26Z

LGTM

dchen1107 · 2016-05-23T16:57:43Z

@k8s-bot test this issue: #IGNORE

dchen1107 · 2016-05-24T00:11:17Z

@timstclair Like what we discussed offline, we changed the PR to log the warning instead of crashing kubelet since we run our builds as docker container. Also add the logic only updates the file when the value is smaller than the default one. PTAL. Thanks!

timstclair · 2016-05-24T00:13:10Z

cmd/kubelet/app/server.go

+	const minKeys uint64 = 1000000
+	key, err := ioutil.ReadFile(maxkeysPath)
+	if err != nil {
+		glog.Warningf("Cannot read keys quota in %s", maxkeysPath)


Log the error as well?

timstclair · 2016-05-24T00:15:33Z

A couple nits.

timstclair · 2016-05-24T00:56:03Z

Adding LGTM, but please add the err to the log if you get a chance before this goes in (or after)

dchen1107 · 2016-05-24T22:24:40Z

We need to start our soaking tests against docker 1.11.1 for 1.3 as early as possible. Bump the priority to p1.

are smaller than the default ones.

dchen1107 · 2016-05-25T18:50:01Z

Rebased again! :-)

dchen1107 · 2016-05-25T23:01:09Z

@k8s-bot ok to test

alex-mohr · 2016-05-26T05:32:03Z

as buildcop: demoting to p2 because we have some p1 PRs to fix flaky tests and those should merge first. this should still merge before morning if SQ doesn't break horribly.

k8s-github-robot · 2016-05-26T07:41:58Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-05-26T08:23:40Z

GCE e2e build/test passed for commit a8ac041.

k8s-github-robot · 2016-05-26T08:33:13Z

Automatic merge from submit-queue

yujuhong · 2016-05-26T19:40:12Z

@dchen1107, FYI, this PR doesn't change the container vm version. The commit probably got lost due to the previous revert/rebase.

timstclair · 2016-05-26T19:52:17Z

Oh no!

googlebot added the cla: yes label May 20, 2016

dchen1107 assigned timstclair May 20, 2016

timstclair reviewed May 20, 2016
View reviewed changes

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels May 20, 2016

dchen1107 force-pushed the docker branch from 004709e to f29021e Compare May 21, 2016 01:06

timstclair reviewed May 21, 2016
View reviewed changes

timstclair added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2016

dchen1107 mentioned this pull request May 21, 2016

Cleanup system configuration at node #26005

Closed

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2016

dchen1107 added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels May 21, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 24, 2016

dchen1107 added this to the v1.3 milestone May 24, 2016

timstclair reviewed May 24, 2016
View reviewed changes

dchen1107 force-pushed the docker branch from 4f3dc6b to 432ed79 Compare May 24, 2016 00:19

timstclair added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2016

dchen1107 added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 24, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2016

dchen1107 added 2 commits May 25, 2016 11:47

Config root_maxkeys to 1000000, root_maxbytes to 25000000

303d5a1

Config the root_maxkeys and root_maxbytes if the existing values

a8ac041

are smaller than the default ones.

dchen1107 force-pushed the docker branch from 432ed79 to a8ac041 Compare May 25, 2016 18:48

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 May 25, 2016

dchen1107 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2016

alex-mohr added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels May 26, 2016

k8s-github-robot merged commit 6a1abc1 into kubernetes:master May 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt 2: Bump GCE containerVM to container-v1-3-v20160517 (Docker 1.11.1) again. #26001

Attempt 2: Bump GCE containerVM to container-v1-3-v20160517 (Docker 1.11.1) again. #26001

dchen1107 commented May 20, 2016 •

edited

Loading

timstclair May 20, 2016

dchen1107 May 20, 2016

dchen1107 May 20, 2016

timstclair May 20, 2016

dchen1107 May 20, 2016

dchen1107 May 20, 2016

dchen1107 May 21, 2016

timstclair May 21, 2016

mikedanese May 21, 2016

vishh May 23, 2016

dchen1107 commented May 21, 2016

timstclair May 21, 2016

timstclair commented May 21, 2016

vishh commented May 21, 2016

vishh commented May 21, 2016

dchen1107 commented May 23, 2016

dchen1107 commented May 24, 2016

timstclair May 24, 2016

dchen1107 May 24, 2016

timstclair commented May 24, 2016

timstclair commented May 24, 2016

dchen1107 commented May 24, 2016

dchen1107 commented May 25, 2016

dchen1107 commented May 25, 2016

alex-mohr commented May 26, 2016

k8s-github-robot commented May 26, 2016

k8s-bot commented May 26, 2016

k8s-github-robot commented May 26, 2016

yujuhong commented May 26, 2016

timstclair commented May 26, 2016

Attempt 2: Bump GCE containerVM to container-v1-3-v20160517 (Docker 1.11.1) again. #26001

Attempt 2: Bump GCE containerVM to container-v1-3-v20160517 (Docker 1.11.1) again. #26001

Conversation

dchen1107 commented May 20, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchen1107 commented May 21, 2016

Choose a reason for hiding this comment

timstclair commented May 21, 2016

vishh commented May 21, 2016

vishh commented May 21, 2016

dchen1107 commented May 23, 2016

dchen1107 commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timstclair commented May 24, 2016

timstclair commented May 24, 2016

dchen1107 commented May 24, 2016

dchen1107 commented May 25, 2016

dchen1107 commented May 25, 2016

alex-mohr commented May 26, 2016

k8s-github-robot commented May 26, 2016

k8s-bot commented May 26, 2016

k8s-github-robot commented May 26, 2016

yujuhong commented May 26, 2016

timstclair commented May 26, 2016

dchen1107 commented May 20, 2016 •

edited

Loading