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.

Sign up for GitHub

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

Jump to bottom

kubeadm: do not set deprecated '--cgroup-driver' flag in kubeadm-flags.env, it will be set in config.yaml #90513

Merged
merged 1 commit into from
May 1, 2020
Merged

kubeadm: do not set deprecated '--cgroup-driver' flag in kubeadm-flags.env, it will be set in config.yaml #90513

merged 1 commit into from
May 1, 2020

Conversation

SataQiu
Copy link
Member

@SataQiu SataQiu commented Apr 27, 2020 •
edited
Loading

What type of PR is this?
/kind design

What this PR does / why we need it:
kubeadm: do not set deprecated '--cgroup-driver' flag in kubeadm-flags.env, this value will be set in config.yaml

--cgroup-driver string  Driver that the kubelet uses to manipulate cgroups on the host.  Possible values: 'cgroupfs', 'systemd' (default "cgroupfs") (DEPRECATED: This parameter should be set via the config file specified by the Kubelet's --config flag. See https://kubernetes.io/docs/tasks/administer-cluster/kubelet-config-file/ for more information.)

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#949

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

ACTION REQUIRED: kubeadm does not set the deprecated '--cgroup-driver' flag in /var/lib/kubelet/kubeadm-flags.env, it will be set in the kubelet config.yaml. If you have this flag in /var/lib/kubelet/kubeadm-flags.env or /etc/default/kubelet (/etc/sysconfig/kubelet for RPMs) please remove it and set the value using KubeletConfiguration

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

NONE

@SataQiu
kubeadm: do not set deprecated '--cgroup-driver' flag in kubeadm-flag…
afb1ea4 
…s.env, this value will be set in config.yaml
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 27, 2020
@k8s-ci-robot k8s-ci-robot requested review from ereslibre and kad April 27, 2020 07:37
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 27, 2020
@SataQiu
Copy link
Member Author

SataQiu commented Apr 27, 2020

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-kind-ipv6
/test pull-kubernetes-e2e-gce

@neolit123
Copy link
Member

neolit123 commented Apr 27, 2020 •
edited
Loading

thank you @SataQiu this looks clean to me.
/lgtm
/assign @rosti
for approval.

but i think we need an action required this time and here is why.
if the users are upgrading from e.g. 1.18 to 1.19 and the 1.19 kubelet no longer has flags such as --cgroup-driver or --resolv-conf it will fail executing on restart. kubeadm writes these flags on disk in:
/var/lib/kubelet/kubeadm-flags.env
and the user can override with:
/etc/default/kubelet

https://kubernetes.io/docs/setup/production-environment/tools/kubeadm/kubelet-integration/#the-kubelet-drop-in-file-for-systemd

the problem is these files persist on disk and kubeadm upgrade does not touch them.
so in my opinion the action required by the user is to remove these flags manually from the above files.

suggesting change:

ACTION REQUIRED: kubeadm: do not set the deprecated '--cgroup-driver' flag in /var/lib/kubelet/kubeadm-flags.env, it will be set in the kubelet config.yaml. If you have this flag in /var/lib/kubelet/kubeadm-flags.env or /etc/default/kubelet (/etc/sysconfig/kubelet for RPMs) please remove it and set the value using KubeletConfiguration

please also update the release note in this PR:
#90394

ACTION REQUIERD: kubeadm: fix the bug that kubeadm does not really respect resolvConf value set by user if systemd-resolved service is active. kubeadm no longer sets the flag in '--resolv-conf' in /var/lib/kubelet/kubeadm-flags.env. If you have this flag in /var/lib/kubelet/kubeadm-flags.env or /etc/default/kubelet (/etc/sysconfig/kubelet for RPMs) please remove it and set the value using KubeletConfiguration

(RPM path for user values is /etc/sysconfig/kubelet)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2020
@neolit123
Copy link
Member

and we need to update this in the docs:
https://kubernetes.io/docs/setup/production-environment/tools/kubeadm/install-kubeadm/#configure-cgroup-driver-used-by-kubelet-on-control-plane-node

@neolit123
Copy link
Member

/priority important-longterm

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 27, 2020
@SataQiu
Copy link
Member Author

SataQiu commented Apr 28, 2020

/test pull-kubernetes-e2e-gce

@SataQiu SataQiu mentioned this pull request Apr 28, 2020
Merged
rosti
rosti approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks @SataQiu !
/approve

cmd/kubeadm/app/util/config/initconfiguration.go
@@ -141,9 +141,9 @@ func SetAPIEndpointDynamicDefaults(cfg *kubeadmapi.APIEndpoint) error {
}

// SetClusterDynamicDefaults checks and sets values for the ClusterConfiguration object
func SetClusterDynamicDefaults(cfg *kubeadmapi.ClusterConfiguration, LocalAPIEndpoint *kubeadmapi.APIEndpoint) error {
func SetClusterDynamicDefaults(cfg *kubeadmapi.ClusterConfiguration, localAPIEndpoint *kubeadmapi.APIEndpoint, nodeRegOpts *kubeadmapi.NodeRegistrationOptions) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to myself: This looks fairly awkward here and is a prominent reminder that component configs should not be housed in the ClusterConfiguration as they are no longer cluster wide.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rosti, SataQiu

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2020
@neolit123
Copy link
Member

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@SataQiu
Copy link
Member Author

SataQiu commented Apr 29, 2020

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@neolit123
Copy link
Member

/retest

@neolit123
Copy link
Member

/test pull-kubernetes-files-remake

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 8caddda into kubernetes:master May 1, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone May 1, 2020
@atsikham atsikham mentioned this pull request Oct 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosti rosti rosti approved these changes

@ereslibre ereslibre Awaiting requested review from ereslibre

@kad kad Awaiting requested review from kad

Assignees

@neolit123 neolit123

@rosti rosti

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

kubeadm-flags.env contains deprecated kubelet flags
5 participants
@SataQiu @neolit123 @k8s-ci-robot @fejta-bot @rosti