Deprecate Kubelet RunOnce mode #125649

HirazawaUi · 2024-06-23T16:39:16Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Deprecate kubelet support for RunOnce mode.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added a alpha feature, behind the `LegacyNodeRunOnceMode` feature gate.
The feature gate is enabled by default. When the feature gate is disabled, we do not allow users to run in RunOnce mode.

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

https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/4580-deprecate-kubelet-runonce/README.md

HirazawaUi · 2024-06-23T16:40:58Z

/hold
Until kubernetes/enhancements#4736 merged.

HirazawaUi · 2024-06-25T16:45:03Z

/retest

HirazawaUi · 2024-06-26T01:58:26Z

/retest

HirazawaUi · 2024-06-26T02:23:20Z

/retest

toVersus · 2024-06-26T07:14:22Z

cmd/kubelet/app/server.go

+		if !utilfeature.DefaultFeatureGate.Enabled(features.LegacyNodeRunOnceMode) {
+			panic("Cannot run runonce mode when LegacyNodeRunOnceMode feature gate is disabled")
+		}
+		klog.InfoS("WARNING: RunOnce mode has been deprecated, and will be removed in a future release")


As mentioned in the KEP-4580, why don't you log this as a warning level message?

Suggested change

klog.InfoS("WARNING: RunOnce mode has been deprecated, and will be removed in a future release")

klog.Warning("WARNING: RunOnce mode has been deprecated, and will be removed in a future release")

~~I was just wondering, if kubelet's log level is set to error or higher, the deprecation warning is missed. Should we consider always writing deprecation message without relying on klog?~~ Never mind, since klog only has the -v option, it will probably always be logged even with -v=0.

As mentioned in the KEP-4580, why don't you log this as a warning level message?

This is what I originally wanted to do, but lint doesn't like the warning function, so I chose to use InfoS instead.

If the PR is approved, I will modify the KEP

Could just make it an error.

Suggested change

klog.InfoS("WARNING: RunOnce mode has been deprecated, and will be removed in a future release")

klog.ErrorS("RunOnce mode is deprecated, and will be removed in a future release")

HirazawaUi · 2024-07-10T11:59:10Z

/retest

a-mccarthy · 2024-07-17T10:57:31Z

Hi @HirazawaUi, I'm Abbie the comms lead for 1.31. Can you share the status of this PR? We are close to publishing the mid-cycle blog and this deprecation is listed. I'd like to make sure we should still include this in the blog.

Also, can you make sure this PR is linked in kubernetes/enhancements#4580? thanks!

cc: @neoaggelos

HirazawaUi · 2024-07-17T15:57:54Z

@a-mccarthy This PR is waiting for review. I will remind the reviewer of sig-node to review it later in slack.

HirazawaUi · 2024-07-17T16:09:40Z

Also, can you make sure this PR is linked in kubernetes/enhancements#4580? thanks!

added.

HirazawaUi · 2024-07-17T16:11:06Z

/cc @tallclair
May you take a look at it?

mrunalp · 2024-07-19T17:12:57Z

cmd/kubelet/app/server.go

+		if !utilfeature.DefaultFeatureGate.Enabled(features.LegacyNodeRunOnceMode) {
+			panic("Cannot run runonce mode when LegacyNodeRunOnceMode feature gate is disabled")
+		}
+		klog.InfoS("WARNING: RunOnce mode has been deprecated, and will be removed in a future release")


@thockin I see this akin to #126084. So, we should drop the language around removal, right?

@HirazawaUi Also, we should adjust the language to encourage migrating to alternatives covered in the KEP for this.

Also, we should adjust the language to encourage migrating to alternatives covered in the KEP for this.

Added. Do you think we should add the URL for podman kube? (I believe it's not necessary because the URL for podman kube might change).

bart0sh · 2024-07-20T13:08:07Z

/triage accepted
/priority important-soon

HirazawaUi · 2024-07-22T15:07:26Z

/test pull-kubernetes-unit

thockin · 2024-07-22T15:54:15Z

Unlike the gitRepo volume case, this is not a part of the API, so it counts as "admin-facing" (as per https://kubernetes.io/docs/reference/using-api/deprecation-policy). Thus it is possible to deprecate, but the KEP does not discuss the actual timeline. I'd appreciate clarity on that (see https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-a-feature-or-behavior in particular)

thockin · 2024-07-22T15:55:00Z

cmd/kubelet/app/server.go

@@ -1260,6 +1260,10 @@ func RunKubelet(ctx context.Context, kubeServer *options.KubeletServer, kubeDeps

 	// process pods and exit.
 	if runOnce {
+		if !utilfeature.DefaultFeatureGate.Enabled(features.LegacyNodeRunOnceMode) {
+			panic("Cannot run runonce mode when LegacyNodeRunOnceMode feature gate is disabled")


panic will dump a backtrace - is that really what we need? It will appear to users as a bug or crash.

It's not uncommon for panic to occur in kubelet, a similar use cases like:

kubernetes/pkg/kubelet/status/status_manager.go

Lines 199 to 207 in 0caeba5

if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) {

stateImpl, err := state.NewStateCheckpoint(m.stateFileDirectory, podStatusManagerStateFile)

if err != nil {

// This is a crictical, non-recoverable failure.

klog.ErrorS(err, "Could not initialize pod allocation checkpoint manager, please drain node and remove policy state file")

panic(err)

}

m.state = stateImpl

}

I leave this for.kubele reviewers, but IMO panic is not a very obvious failure mechanism

yeah I agree that panic is a bit much. Can you exist with an error and log an error instead? Personally I think panics should be reserved for coding errors, whereas this is a configuration error.

thockin · 2024-07-22T15:57:01Z

pkg/features/kube_features.go

@@ -1066,6 +1073,8 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS

 	LegacyServiceAccountTokenCleanUp: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // GA in 1.30; remove in 1.32

+	LegacyNodeRunOnceMode: {Default: true, PreRelease: featuregate.Alpha},


See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/feature-gates.md#deprecation

It describes how to deprecate with gates (though in this case you want to start with it enabled, to satisfy deprecation policy)

I would leave a TODO here to change default to false in or after 1.34

See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/feature-gates.md#deprecation

It describes how to deprecate with gates (though in this case you want to start with it enabled, to satisfy deprecation policy)

Thanks for the reminder.

I would leave a TODO here to change default to false in or after 1.34

Since the KEP describes that it will be set to false by default in v1.32, I have added comment according to the KEP.

The deprecation policy is a little vague about this. It says "Deprecated behaviors must function for no less than 1 year after their announced deprecation." Do we think that "function" means "by default" or "with effort" ?

My read of it was "by default"

All right, I have updated the comment.

cmd/kubelet/app/server.go

k8s-ci-robot · 2024-07-23T17:37:58Z

PR needs rebase.

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-sigs/prow repository.

thockin

LGTM if OK with node folks

/approve
/hold

Clear hold when fully reviewed

k8s-ci-robot · 2024-07-23T19:00:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HirazawaUi, thockin

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:

~~cmd/kubelet/OWNERS~~ [thockin]
~~pkg/features/OWNERS~~ [thockin]
~~pkg/kubelet/apis/config/OWNERS~~ [thockin]

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

haircommander · 2024-07-23T19:51:27Z

can you update the merge conflict too please @HirazawaUi ?

HirazawaUi · 2024-07-24T13:09:52Z

/close
ref: kubernetes/enhancements#4736 (comment)

k8s-ci-robot · 2024-07-24T13:09:57Z

@HirazawaUi: Closed this PR.

In response to this:

/close
ref: kubernetes/enhancements#4736 (comment)

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-sigs/prow repository.

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2024

k8s-ci-robot requested review from derekwaynecarr and dims June 23, 2024 16:41

HirazawaUi force-pushed the deprecate-kubelet-runonce branch from 5489da7 to 804ac2e Compare June 25, 2024 13:25

toVersus reviewed Jun 26, 2024

View reviewed changes

a-mccarthy mentioned this pull request Jul 17, 2024

Add kubernetes 1.31 deprecations and removals blog kubernetes/website#46948

Merged

HirazawaUi mentioned this pull request Jul 17, 2024

Deprecate & remove Kubelet RunOnce mode kubernetes/enhancements#4580

Closed

6 tasks

k8s-ci-robot requested a review from tallclair July 17, 2024 16:11

mrunalp reviewed Jul 19, 2024

View reviewed changes

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 20, 2024

HirazawaUi force-pushed the deprecate-kubelet-runonce branch 2 times, most recently from 317d21c to 7411183 Compare July 22, 2024 14:10

thockin reviewed Jul 22, 2024

View reviewed changes

cmd/kubelet/app/server.go Outdated Show resolved Hide resolved

HirazawaUi force-pushed the deprecate-kubelet-runonce branch from 7411183 to 03ba8f3 Compare July 22, 2024 16:38

Deprecate Kubelet RunOnce mode

bf3f2c5

HirazawaUi force-pushed the deprecate-kubelet-runonce branch from 03ba8f3 to bf3f2c5 Compare July 23, 2024 03:20

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2024

thockin reviewed Jul 23, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2024

k8s-ci-robot closed this Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate Kubelet RunOnce mode #125649

Deprecate Kubelet RunOnce mode #125649

HirazawaUi commented Jun 23, 2024

HirazawaUi commented Jun 23, 2024

HirazawaUi commented Jun 25, 2024

HirazawaUi commented Jun 26, 2024

HirazawaUi commented Jun 26, 2024

toVersus Jun 26, 2024 •

edited

Loading

HirazawaUi Jun 26, 2024

tallclair Jul 20, 2024

HirazawaUi commented Jul 10, 2024

a-mccarthy commented Jul 17, 2024

HirazawaUi commented Jul 17, 2024

HirazawaUi commented Jul 17, 2024

HirazawaUi commented Jul 17, 2024

mrunalp Jul 19, 2024 •

edited

Loading

mrunalp Jul 19, 2024

HirazawaUi Jul 20, 2024

bart0sh commented Jul 20, 2024

HirazawaUi commented Jul 22, 2024

thockin commented Jul 22, 2024

thockin Jul 22, 2024

HirazawaUi Jul 22, 2024

thockin Jul 23, 2024

haircommander Jul 23, 2024

thockin Jul 22, 2024

thockin Jul 22, 2024

HirazawaUi Jul 22, 2024

thockin Jul 22, 2024

HirazawaUi Jul 23, 2024 •

edited

Loading

k8s-ci-robot commented Jul 23, 2024

thockin left a comment

k8s-ci-robot commented Jul 23, 2024

haircommander commented Jul 23, 2024

HirazawaUi commented Jul 24, 2024

k8s-ci-robot commented Jul 24, 2024

	klog.InfoS("WARNING: RunOnce mode has been deprecated, and will be removed in a future release")
	klog.Warning("WARNING: RunOnce mode has been deprecated, and will be removed in a future release")

	klog.InfoS("WARNING: RunOnce mode has been deprecated, and will be removed in a future release")
	klog.ErrorS("RunOnce mode is deprecated, and will be removed in a future release")

	if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) {
	stateImpl, err := state.NewStateCheckpoint(m.stateFileDirectory, podStatusManagerStateFile)
	if err != nil {
	// This is a crictical, non-recoverable failure.
	klog.ErrorS(err, "Could not initialize pod allocation checkpoint manager, please drain node and remove policy state file")
	panic(err)
	}
	m.state = stateImpl
	}

		@@ -1066,6 +1073,8 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS

		LegacyServiceAccountTokenCleanUp: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // GA in 1.30; remove in 1.32

		LegacyNodeRunOnceMode: {Default: true, PreRelease: featuregate.Alpha},

Deprecate Kubelet RunOnce mode #125649

Deprecate Kubelet RunOnce mode #125649

Conversation

HirazawaUi commented Jun 23, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

HirazawaUi commented Jun 23, 2024

HirazawaUi commented Jun 25, 2024

HirazawaUi commented Jun 26, 2024

HirazawaUi commented Jun 26, 2024

toVersus Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HirazawaUi commented Jul 10, 2024

a-mccarthy commented Jul 17, 2024

HirazawaUi commented Jul 17, 2024

HirazawaUi commented Jul 17, 2024

HirazawaUi commented Jul 17, 2024

mrunalp Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bart0sh commented Jul 20, 2024

HirazawaUi commented Jul 22, 2024

thockin commented Jul 22, 2024

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

HirazawaUi Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 23, 2024

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 23, 2024

haircommander commented Jul 23, 2024

HirazawaUi commented Jul 24, 2024

k8s-ci-robot commented Jul 24, 2024

toVersus Jun 26, 2024 •

edited

Loading

mrunalp Jul 19, 2024 •

edited

Loading

HirazawaUi Jul 23, 2024 •

edited

Loading