kubelet: Pass --config to use kubeadm generated configuration #5697

nanikjava · 2019-10-22T21:32:06Z

Kubelet startup parameters does not include the --config flag. This flag pass the
location of the configuration file. During minikube startup process this file
is copied over to the VM. Fix test cases.

k8s-ci-robot · 2019-10-22T21:32:14Z

Hi @nanikjava. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

minikube-bot · 2019-10-22T21:33:02Z

Can one of the admins verify this patch?

TravisBuddy · 2019-10-22T21:37:44Z

Travis tests have failed

Hey @nanikjava,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

make test

which go-bindata || GO111MODULE=off GOBIN="/home/travis/gopath/bin" go get github.com/jteeuwen/go-bindata/...
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.12.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/assets/assets.go -pkg assets deploy/addons/...
gofmt -s -w pkg/minikube/assets/assets.go
which go-bindata || GO111MODULE=off GOBIN="/home/travis/gopath/bin" go get github.com/jteeuwen/go-bindata/...
/home/travis/gopath/bin/go-bindata
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.12.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/translate/translations.go -pkg translate translations/...
gofmt -s -w pkg/minikube/translate/translations.go
./test.sh
= make lint =============================================================
golangci/golangci-lint info checking GitHub for tag 'v1.20.0'
golangci/golangci-lint info found version: 1.20.0 for v1.20.0/linux/amd64
golangci/golangci-lint info installed out/linters/golangci-lint
pkg/minikube/bootstrapper/kubeadm/versions.go:220: File is not `goimports`-ed (goimports)

Makefile:341: recipe for target 'lint-ci' failed
make[1]: *** [lint-ci] Error 1
= go mod ================================================================
ok
Makefile:245: recipe for target 'test' failed
make: *** [test] Error 4

TravisBuddy Request Identifier: 2d2f33d0-f514-11e9-9e2b-01a46d152451

nanikjava · 2019-10-22T21:38:07Z

Tested for both kvm2 and vbox. Test done by changing the value inside pkg/minikube/bootstrapper/kubeadm/templates.go

evictionHard:
nodefs.available: "0%"
nodefs.inodesFree: "0%"
imagefs.available: "0%"
`))

Following is kubelet log when reading the config.yaml (using 0,0,0 as evictionHard value)

container_manager_linux.go:270] Creating Container Manager object based on Node Config: {RuntimeCgroupsName: SystemCgroupsName: KubeletCgroupsName: ContainerRuntime:docker CgroupsPerQOS:true CgroupRoot:/ CgroupDriver:cgroupfs KubeletRootDir:/var/lib/kubelet ProtectKernelDefaults:false NodeAllocatableConfig:{KubeReservedCgroupName: SystemReservedCgroupName: EnforceNodeAllocatable:map[pods:{}] KubeReserved:map[] SystemReserved:map[] HardEvictionThresholds:[]} QOSReserved:map[] ExperimentalCPUManagerPolicy:none ExperimentalCPUManagerReconcilePeriod:10s ExperimentalPodPidsLimit:-1 EnforceCPULimits:true CPUCFSQuotaPeriod:100ms ExperimentalTopologyManagerPolicy:none}

Following is kubelet log when reading the config.yaml (using 10,10,10 as evictionHard value)

container_manager_linux.go:270] Creating Container Manager object based on Node Config: {RuntimeCgroupsName: SystemCgroupsName: KubeletCgroupsName: ContainerRuntime:docker CgroupsPerQOS:true CgroupRoot:/ CgroupDriver:cgroupfs KubeletRootDir:/var/lib/kubelet ProtectKernelDefaults:false NodeAllocatableConfig:{KubeReservedCgroupName: SystemReservedCgroupName: EnforceNodeAllocatable:map[pods:{}] KubeReseator:LessThan Value:{Quantity:<nil> Percentage:0.1} GracePeriod:0s MinReclaim:<nil>} {Signal:nodefs.available Operator:LessThan Value:{Quantity:<nil> Percentage:0.1} GracePeriod:0s MinReclaim:<nil>} {Signal:nodefs.inodesFree Operator:LessThan Value:{Quantity:<nil> Percentage:0.1} GracePeriod:0s MinReclaim:<nil>}]} QOSReserved:map[] ExperimentalCPUManagerPolicy:none ExperimentalCPUManagerReconcilePeriod:10s ExperimentalPodPidsLimit:-1 EnforceCPULimits:true CPUCFSQuotaPeriod:100ms ExperimentalTopologyManagerPolicy:none}

…es#5329 Kubelet startup parameters does not include the --config flag. This flag pass the location of the configuration file. During minikube startup process this file is copied over to the VM. Fix test cases.

codecov-io · 2019-10-22T21:48:58Z

Codecov Report

Merging #5697 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5697   +/-   ##
=======================================
  Coverage   37.83%   37.83%           
=======================================
  Files         106      106           
  Lines        7773     7773           
=======================================
  Hits         2941     2941           
  Misses       4452     4452           
  Partials      380      380

Impacted Files	Coverage Δ
pkg/minikube/bootstrapper/kubeadm/versions.go	`84.09% <ø> (ø)`	⬆️

nanikjava · 2019-10-22T21:51:43Z

/retest

k8s-ci-robot · 2019-10-22T21:51:56Z

@nanikjava: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

tstromberg

Looks great. Thank you for fixing this!

tstromberg · 2019-10-23T02:58:48Z

@minikube-bot OK to test

RA489 · 2019-10-23T06:50:37Z

/ok-to-test

thomas-riccardi · 2019-10-23T09:41:08Z

It seems some (if not most) kubelet parameters are now duplicated in kubelet commandline and config.yaml file.
It's probably better to have the values only once: either cmdline or config file.

I'm not even sure if all the values are the same:

the cmdline is built in /pkg/minikube/bootstrapper/kubeadm/versions.go
the kubelet config file is defined in /pkg/minikube/bootstrapper/kubeadm/templates.go, generated in /pkg/minikube/bootstrapper/kubeadm/kubeadm.go as a kubeadmCfg, but it's not really kubeadmin config (it's kubelet config), isn't it? (and there is a kubeletCfg next to it, but it contains other things... I'm lost :) )

nanikjava · 2019-10-23T20:29:08Z

It seems some (if not most) kubelet parameters are now duplicated in kubelet commandline and config.yaml file.
It's probably better to have the values only once: either cmdline or config file.

I'm not even sure if all the values are the same:

the cmdline is built in /pkg/minikube/bootstrapper/kubeadm/versions.go

the kubelet config file is defined in /pkg/minikube/bootstrapper/kubeadm/templates.go, generated in /pkg/minikube/bootstrapper/kubeadm/kubeadm.go as a kubeadmCfg, but it's not really kubeadmin config (it's kubelet config), isn't it? (and there is a kubeletCfg next to it, but it contains other things... I'm lost :) )

Can you please open a separate ticket issue for the above so we can take a look at it separately so it's easier to track.

Thanks

thomas-riccardi · 2019-10-24T11:50:12Z

Can you please open a separate ticket issue for the above so we can take a look at it separately so it's easier to track.

Thanks

I opened #5719, but it may be better to resolve the issue before merging this current PR (#5697), as it may introduce regressions if the parameters are different.

nanikjava · 2019-10-24T17:58:13Z

Can you please open a separate ticket issue for the above so we can take a look at it separately so it's easier to track.
Thanks

I opened #5719, but it may be better to resolve the issue before merging this current PR (#5697), as it may introduce regressions if the parameters are different.

Thanks @thomas-riccardi

Will wait for feedback from the reviewers about this.

tstromberg · 2019-10-28T23:16:46Z

It seems some (if not most) kubelet parameters are now duplicated in kubelet commandline and config.yaml file.
It's probably better to have the values only once: either cmdline or config file.
I'm not even sure if all the values are the same:

the cmdline is built in /pkg/minikube/bootstrapper/kubeadm/versions.go

the kubelet config file is defined in /pkg/minikube/bootstrapper/kubeadm/templates.go, generated in /pkg/minikube/bootstrapper/kubeadm/kubeadm.go as a kubeadmCfg, but it's not really kubeadmin config (it's kubelet config), isn't it? (and there is a kubeletCfg next to it, but it contains other things... I'm lost :) )

Can you please open a separate ticket issue for the above so we can take a look at it separately so it's easier to track.

Thanks

This overlap is intentional, because at first, very few kubelet flags were exposed by kubeadm. Flags always take precedence over configuration files. Here is my guidance:

If the kubelet configuration option can be set with kubeadm, add them to kubeadm/templates.go (the kubeadm API is more stable across versions)
Otherwise, add it to kubeadm/versions.go

Unless someone sees something I'm not, I think the conflict is OK as is. User-provided flags will still take precedence.

k8s-ci-robot · 2019-10-28T23:22:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nanikjava
To complete the pull request process, please assign ra489
You can assign the PR to them by writing /assign @ra489 in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

tstromberg · 2019-10-28T23:23:24Z

FYI, I think this PR is doing the right thing, and I would like to merge it in order to solve #5751 correctly, by asking kubeadm to program kubelet for setting set file limits.

tstromberg · 2019-10-28T23:28:10Z

/ok-to-test

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 22, 2019

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 22, 2019

k8s-ci-robot requested review from blueelvis and sharifelgamal October 22, 2019 21:32

nanikjava mentioned this pull request Oct 22, 2019

kubelet: The generated config is not loaded (imagefs.available not applied) #5329

Closed

Add --config to instruct kubelet to use internal config file kubernet…

80d2009

…es#5329 Kubelet startup parameters does not include the --config flag. This flag pass the location of the configuration file. During minikube startup process this file is copied over to the VM. Fix test cases.

nanikjava force-pushed the f-fix-5329 branch from 746b866 to 80d2009 Compare October 22, 2019 21:41

nanikjava changed the title ~~Add --config to instruct kubelet to use internal config file #5329~~ Add --config to instruct kubelet to use internal config file Oct 22, 2019

tstromberg approved these changes Oct 23, 2019

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 23, 2019

thomas-riccardi mentioned this pull request Oct 24, 2019

kubelet command-line arguments redundant with newly enabled config file #5719

Closed

Merge branch 'master' into f-fix-5329

58018ee

tstromberg changed the title ~~Add --config to instruct kubelet to use internal config file~~ kubelet: Pass --config flag to use kubeadm generated configuration Oct 29, 2019

tstromberg changed the title ~~kubelet: Pass --config flag to use kubeadm generated configuration~~ kubelet: Pass --config to use kubeadm generated configuration Oct 29, 2019

tstromberg merged commit 75377fa into kubernetes:master Oct 29, 2019

etuttle mentioned this pull request Nov 21, 2019

Error starting cluster: kubelet config: error: open /var/lib/kubelet/config.yaml: no such file or directory #5947

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: Pass --config to use kubeadm generated configuration #5697

kubelet: Pass --config to use kubeadm generated configuration #5697

nanikjava commented Oct 22, 2019 •

edited

Loading

k8s-ci-robot commented Oct 22, 2019

minikube-bot commented Oct 22, 2019

TravisBuddy commented Oct 22, 2019

nanikjava commented Oct 22, 2019

codecov-io commented Oct 22, 2019 •

edited

Loading

nanikjava commented Oct 22, 2019

k8s-ci-robot commented Oct 22, 2019

tstromberg left a comment

tstromberg commented Oct 23, 2019

RA489 commented Oct 23, 2019

thomas-riccardi commented Oct 23, 2019 •

edited

Loading

nanikjava commented Oct 23, 2019

thomas-riccardi commented Oct 24, 2019

nanikjava commented Oct 24, 2019

tstromberg commented Oct 28, 2019

k8s-ci-robot commented Oct 28, 2019

tstromberg commented Oct 28, 2019 •

edited

Loading

tstromberg commented Oct 28, 2019

kubelet: Pass --config to use kubeadm generated configuration #5697

kubelet: Pass --config to use kubeadm generated configuration #5697

Conversation

nanikjava commented Oct 22, 2019 • edited Loading

k8s-ci-robot commented Oct 22, 2019

minikube-bot commented Oct 22, 2019

TravisBuddy commented Oct 22, 2019

Travis tests have failed

1st Build

TravisBuddy Request Identifier: 2d2f33d0-f514-11e9-9e2b-01a46d152451

nanikjava commented Oct 22, 2019

codecov-io commented Oct 22, 2019 • edited Loading

Codecov Report

nanikjava commented Oct 22, 2019

k8s-ci-robot commented Oct 22, 2019

tstromberg left a comment

Choose a reason for hiding this comment

tstromberg commented Oct 23, 2019

RA489 commented Oct 23, 2019

thomas-riccardi commented Oct 23, 2019 • edited Loading

nanikjava commented Oct 23, 2019

thomas-riccardi commented Oct 24, 2019

nanikjava commented Oct 24, 2019

tstromberg commented Oct 28, 2019

k8s-ci-robot commented Oct 28, 2019

tstromberg commented Oct 28, 2019 • edited Loading

tstromberg commented Oct 28, 2019

nanikjava commented Oct 22, 2019 •

edited

Loading

codecov-io commented Oct 22, 2019 •

edited

Loading

thomas-riccardi commented Oct 23, 2019 •

edited

Loading

tstromberg commented Oct 28, 2019 •

edited

Loading