register a terminating pod if containers running #124890

olyazavr · 2024-05-15T15:21:38Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

When a pod is terminating, but its containers are still running (eg. it's running a prestop hook), and then kubelet restarts, it will re-sync but not the volumes, and that causes this error: MountVolume.SetUp failed for volume \"kube-public-cert\" : object \"browsers-dashboard\"/\"kube-public-cert\" not registered" which can only be resolved by force-deleting the pod

See this gist for logs: https://gist.github.com/olyazavr/f1980620e89dead80e72a23fcfe2014d

Basically we want this logic that happens in SyncPod but in the SyncTerminatingPod flow

Testing: I have a pod with a prestop hook to sleep for 10m, and a secret mounted on it. I kill the pod, then restart kubelet. Without this change, it will error out with the mount volume error, and with it, that error does not appear and the pod terminates gracefully

Which issue(s) this PR fixes:

Fixes #113289

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixes a bug where if a pod is terminating and has a secret or configmap mounted and kubelet restarts, it would get MountVolume.SetUp errors and not complete terminating unless force-deleted

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

k8s-ci-robot · 2024-05-15T15:21:47Z

Hi @olyazavr. 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.

olyazavr · 2024-05-15T15:23:13Z

/sig node

HirazawaUi · 2024-05-15T16:20:56Z

/ok-to-test
I guess we should write an e2e test for it?

k8s-ci-robot · 2024-05-15T19:11:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: olyazavr
Once this PR has been reviewed and has the lgtm label, please assign mrunalp for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/kubelet/OWNERS
test/e2e_node/OWNERS

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

bart0sh · 2024-05-20T09:03:10Z

/triage accepted
/priority important-longterm

HirazawaUi · 2024-05-20T11:14:48Z

test/e2e_node/terminating_pod_test.go

+			gomega.Eventually(ctx, func(ctx context.Context) error {
+				return verifyPodDisappeared(ctx, f.ClientSet, podName, ns)
+			}, 2*time.Minute, time.Second*1).Should(gomega.BeNil())


Suggested change

gomega.Eventually(ctx, func(ctx context.Context) error {

return verifyPodDisappeared(ctx, f.ClientSet, podName, ns)

}, 2*time.Minute, time.Second*1).Should(gomega.BeNil())

gomega.Eventually(ctx, func() bool {

_, err := cl.CoreV1().Pods(ns).Get(ctx, name, metav1.GetOptions{})

return apierrors.IsNotFound(err)

}, 2*time.Minute, time.Second).Should(gomega.BeTrue())

It'll be easier.

HirazawaUi · 2024-05-20T11:16:39Z

test/e2e_node/terminating_pod_test.go

+	"github.com/onsi/gomega"
+)
+
+var _ = SIGDescribe("TerminatingPod", func() {


I think it should move to test/e2e_node/terminate_pods_test.go

Signed-off-by: Olga Shestopalova <oshestopalova1@gmail.com>

olyazavr · 2024-05-20T17:46:48Z

/retest

olyazavr · 2024-05-29T16:46:27Z

Could I get some review on this? We still see this issue and it would be great to have a fix that doesn't require hard deleting the pods on the node to fix

olyazavr · 2024-06-05T14:09:57Z

@haircommander could I get some clarity as to why this was moved to the "archive it" section of the board? This problem still exists and this patch is currently running in our production environment to address it

HirazawaUi · 2024-06-17T15:34:12Z

/hold
Wait for the #113289 (comment) to be answered.

haircommander · 2024-07-19T16:02:14Z

@haircommander could I get some clarity as to why this was moved to the "archive it" section of the board? This problem still exists and this patch is currently running in our production environment to address it

sorry for the late reply, this got buried in my inbox. I archived it on the sig node ci board. it has some testing changes but it's primarily the concern of the main board (where it remains in the needs reviewer state).

k8s-triage-robot · 2024-10-17T16:10:53Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-11-16T16:16:46Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-12-16T16:55:50Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-12-16T16:55:56Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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 requested review from dchen1107 and Random-Liu May 15, 2024 15:22

HirazawaUi reviewed May 20, 2024

View reviewed changes

register a terminating pod if containers running

8985e43

Signed-off-by: Olga Shestopalova <oshestopalova1@gmail.com>

olyazavr added 4 commits May 20, 2024 12:28

add e2e test

4f03bc0

Signed-off-by: Olga Shestopalova <oshestopalova1@gmail.com>

fix lint

54462b4

Signed-off-by: Olga Shestopalova <oshestopalova1@gmail.com>

move test, clean it up

0b1b0b4

Signed-off-by: Olga Shestopalova <oshestopalova1@gmail.com>

lint

4331dd7

Signed-off-by: Olga Shestopalova <oshestopalova1@gmail.com>

olyazavr force-pushed the register-terminating-pod branch from 0c98f49 to 4331dd7 Compare May 20, 2024 16:28

olyazavr requested a review from HirazawaUi May 28, 2024 14:27

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 17, 2024

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 17, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 16, 2024

k8s-ci-robot closed this Dec 16, 2024

-			gomega.Eventually(ctx, func(ctx context.Context) error {
-				return verifyPodDisappeared(ctx, f.ClientSet, podName, ns)
-			}, 2*time.Minute, time.Second*1).Should(gomega.BeNil())
+		gomega.Eventually(ctx, func() bool {
+			_, err := cl.CoreV1().Pods(ns).Get(ctx, name, metav1.GetOptions{})
+			return apierrors.IsNotFound(err)
+		}, 2*time.Minute, time.Second).Should(gomega.BeTrue())

register a terminating pod if containers running #124890

register a terminating pod if containers running #124890

Uh oh!

Conversation

olyazavr commented May 15, 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.:

Uh oh!

k8s-ci-robot commented May 15, 2024

Uh oh!

olyazavr commented May 15, 2024

Uh oh!

HirazawaUi commented May 15, 2024

Uh oh!

k8s-ci-robot commented May 15, 2024

Uh oh!

bart0sh commented May 20, 2024

Uh oh!

HirazawaUi May 20, 2024

Choose a reason for hiding this comment

Uh oh!

HirazawaUi May 20, 2024

Choose a reason for hiding this comment

Uh oh!

olyazavr commented May 20, 2024

Uh oh!

olyazavr commented May 29, 2024

Uh oh!

olyazavr commented Jun 5, 2024

Uh oh!

HirazawaUi commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haircommander commented Jul 19, 2024

Uh oh!

k8s-triage-robot commented Oct 17, 2024

Uh oh!

k8s-triage-robot commented Nov 16, 2024

Uh oh!

k8s-triage-robot commented Dec 16, 2024

Uh oh!

k8s-ci-robot commented Dec 16, 2024

Uh oh!

Uh oh!

HirazawaUi commented Jun 17, 2024 •

edited

Loading