Add activeDeadlineSeconds to kubeadm upgrade-health-check job #127333

yuyabee · 2024-09-12T18:48:21Z

With #122079, kubeadm now relies on ttlSecondsAfterFinished to clean up upgrade-health-check once its pod reaches a terminal state. However, there is a case where the pod won't reach a terminal state and the job will not register a terminal state, hence no garbage collection.

For example, if the pause image is not present, ErrImagePull will make the pod keep retrying to pull the image and the pod will never reach a terminal state on its own. And the job will continue to wait for the pod to reach a terminal state which will not happen.

So we need to set activeDeadlineSeconds to prevent the job from waiting forever for the pod to reach a terminal state.

Without this, users invoking kubeadm upgrade plan need to cleanup the job outside of kubeadm even if they ignore the preflight result because the job still runs when the result is configured to be ignored via --ignore-prelight-errors=CreateJob flag.

Since the timeout for the polling in the CreateJob step in kubeadm is 15 seconds, we should set the activeDeadlineSeconds to the same timeout.

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: ensure that Pods from the upgrade preflight check `CreateJob` are properly terminated after a timeout.

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

k8s-ci-robot · 2024-09-12T18:48:30Z

Welcome @yuyabee!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-09-12T18:48:31Z

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

neolit123

thanks for the PR, this seems mostly OK, but need to double check a couple of aspects to make sure we are not introducing more complexity...

/cc @carlory

neolit123 · 2024-09-12T19:00:52Z

cmd/kubeadm/app/phases/upgrade/health.go

@@ -141,6 +141,7 @@ func createJob(client clientset.Interface, cfg *kubeadmapi.ClusterConfiguration)
 		Spec: batchv1.JobSpec{
 			BackoffLimit:            ptr.To[int32](0),
 			TTLSecondsAfterFinished: ptr.To[int32](int32(timeout.Seconds()) + 5), // Make sure it's more than 'timeout'.
+			ActiveDeadlineSeconds:   ptr.To[int64](int64(timeout.Seconds())),


just to confirm:

kubernetes/staging/src/k8s.io/apiserver/pkg/apis/example/v1/types.go

Lines 127 to 131 in a7702cb

// Optional duration in seconds the pod may be active on the node relative to

// StartTime before the system will actively try to mark it failed and kill associated containers.

// Value must be a positive integer.

// +optional

ActiveDeadlineSeconds *int64 `json:"activeDeadlineSeconds,omitempty" protobuf:"varint,5,opt,name=activeDeadlineSeconds"`

this means that if there is an error such as "image cannot be pulled" the job will fail?
that would be the goal.

we don't want the job to succeed because of the Deadline field here. ideally it should always fail if this timeout is reached,

another Q should it be a value between timeout and timeout +5? is it OK to match the value to the timeout of the poll?

this means that if there is an error such as "image cannot be pulled" the job will fail?

Yes and it should always fail if this timeout is reached.

For more details, after reaching ActiveDeadlineSeconds since the start timestamp, the pod fails and the job will have the following condition, marking it for garbage collection thanks to TTLSecondsAfterFinished, if the image is not available:

status: conditions: - lastProbeTime: xxx + 20 + something lastTransitionTime: xxx + 20 message: Job was active longer than specified deadline reason: DeadlineExceeded status: "True" type: Failed failed: 1 ready: 0 startTime: xxx terminating: 0 uncountedTerminatedPods: {}

Notice the lastTransitionTime is 20 seconds after the startTime with activeDeadlineSeconds set to 20.

In a success case, the type Complete will make the kubeadm to mark this preflight as pass:

status: completionTime: xxx + a few sec conditions: - lastProbeTime: xxx +. a few sec lastTransitionTime: xxx + a few sec status: "True" type: Complete ready: 0 startTime: xxx succeeded: 1 terminating: 0 uncountedTerminatedPods: {}

another Q should it be a value between timeout and timeout +5? is it OK to match the value to the timeout of the poll?

Let me make this change quickly (also consolidate into one shared var.

neolit123 · 2024-09-12T19:03:23Z

/release-note-edit

kubeadm: ensure that Pods from the upgrade preflight check `CreateJob` are properly terminated after a timeout.

neolit123 · 2024-09-12T19:03:34Z

^ is that an accurate release note?

neolit123 · 2024-09-12T19:05:02Z

/ok-to-test
/triage accepted
/kind bug

With kubernetes#122079, kubeadm now relies on `ttlSecondsAfterFinished` to clean up `upgrade-health-check` once its pod reaches a terminal state. However, there is a case where the pod won't reach a terminal state and the job will not register a terminal state, hence no garbage collection. For example, if the pause image is not present, `ErrImagePull` will make the pod keep retrying to pull the image and the pod will never reach a terminal state on its own. And the job will continue to wait for the pod to reach a terminal state which will not happen. So we need to set `activeDeadlineSeconds` to prevent the job from waiting forever for the pod to reach a terminal state. Without this, users invoking `kubeadm upgrade plan` need to cleanup the job outside of kubeadm even if they ignore the preflight result because the job still runs when the result is configured to be ignored via `--ignore-prelight-errors=CreateJob` flag. Since the timeout for the polling in the `CreateJob` step in kubeadm is 15 seconds, we should set the `activeDeadlineSeconds` to the same timeout.

yuyabee · 2024-09-12T19:41:32Z

The release note looks good to me, thank you!

carlory · 2024-09-13T06:14:44Z

/lgtm
/assign @neolit123

k8s-ci-robot · 2024-09-13T06:14:51Z

LGTM label has been added.

Git tree hash: 0713f08ae41bb61a69b0417501fba3e4395d22ea

neolit123

thanks
/approve

k8s-ci-robot · 2024-09-13T07:56:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, yuyabee

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/kubeadm/OWNERS~~ [neolit123]

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

SataQiu

/lgtm

thanks

…333-upstream-release-1.30 Automated cherry pick of #127333: Add activeDeadlineSeconds to kubeadm upgrade-health-check job

…333-upstream-release-1.31 Automated cherry pick of #127333: Add activeDeadlineSeconds to kubeadm upgrade-health-check job

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 12, 2024

k8s-ci-robot requested review from neolit123 and SataQiu September 12, 2024 18:48

neolit123 reviewed Sep 12, 2024

View reviewed changes

k8s-ci-robot requested a review from carlory September 12, 2024 19:01

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 12, 2024

yuyabee force-pushed the add-activeDeadlineSeconds-to-upgrade-health-check-job branch from dd3317a to db66416 Compare September 12, 2024 19:40

k8s-ci-robot assigned neolit123 and carlory Sep 13, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2024

neolit123 reviewed Sep 13, 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 Sep 13, 2024

SataQiu reviewed Sep 13, 2024

View reviewed changes

k8s-ci-robot assigned SataQiu Sep 13, 2024

k8s-ci-robot merged commit 19e8e59 into kubernetes:master Sep 13, 2024
14 checks passed

k8s-ci-robot added this to the v1.32 milestone Sep 13, 2024

yuyabee deleted the add-activeDeadlineSeconds-to-upgrade-health-check-job branch September 13, 2024 13:44

This was referenced Sep 13, 2024

Automated cherry pick of #127333: Add activeDeadlineSeconds to kubeadm upgrade-health-check job #127346

Merged

Automated cherry pick of #127333: Add activeDeadlineSeconds to kubeadm upgrade-health-check job #127347

Merged

k8s-ci-robot added a commit that referenced this pull request Sep 24, 2024

Merge pull request #127346 from yuyabee/automated-cherry-pick-of-#127…

6d0fd14

…333-upstream-release-1.30 Automated cherry pick of #127333: Add activeDeadlineSeconds to kubeadm upgrade-health-check job

k8s-ci-robot added a commit that referenced this pull request Sep 24, 2024

Merge pull request #127347 from yuyabee/automated-cherry-pick-of-#127…

c75e833

…333-upstream-release-1.31 Automated cherry pick of #127333: Add activeDeadlineSeconds to kubeadm upgrade-health-check job

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add activeDeadlineSeconds to kubeadm upgrade-health-check job #127333

Add activeDeadlineSeconds to kubeadm upgrade-health-check job #127333

yuyabee commented Sep 12, 2024 •

edited by k8s-ci-robot

Loading

k8s-ci-robot commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

neolit123 left a comment

neolit123 Sep 12, 2024 •

edited

Loading

yuyabee Sep 12, 2024

neolit123 commented Sep 12, 2024

neolit123 commented Sep 12, 2024

neolit123 commented Sep 12, 2024

yuyabee commented Sep 12, 2024

carlory commented Sep 13, 2024

k8s-ci-robot commented Sep 13, 2024

neolit123 left a comment

k8s-ci-robot commented Sep 13, 2024

SataQiu left a comment

	// Optional duration in seconds the pod may be active on the node relative to
	// StartTime before the system will actively try to mark it failed and kill associated containers.
	// Value must be a positive integer.
	// +optional
	ActiveDeadlineSeconds *int64 `json:"activeDeadlineSeconds,omitempty" protobuf:"varint,5,opt,name=activeDeadlineSeconds"`

Add activeDeadlineSeconds to kubeadm upgrade-health-check job #127333

Add activeDeadlineSeconds to kubeadm upgrade-health-check job #127333

Conversation

yuyabee commented Sep 12, 2024 • edited by k8s-ci-robot Loading

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.:

k8s-ci-robot commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

yuyabee Sep 12, 2024

Choose a reason for hiding this comment

neolit123 commented Sep 12, 2024

neolit123 commented Sep 12, 2024

neolit123 commented Sep 12, 2024

yuyabee commented Sep 12, 2024

carlory commented Sep 13, 2024

k8s-ci-robot commented Sep 13, 2024

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 13, 2024

SataQiu left a comment

Choose a reason for hiding this comment

yuyabee commented Sep 12, 2024 •

edited by k8s-ci-robot

Loading

neolit123 Sep 12, 2024 •

edited

Loading