Ensure there is one running static pod with the same full name #104743

gjkim42 · 2021-09-03T01:49:48Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR ensures there is no more than one running static pod with the same full name so that static pods are able to be updated/deleted gracefully.

Which issue(s) this PR fixes:

Fixes #97722

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Respect grace period when updating static pods.

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

gjkim42 · 2021-09-03T03:43:41Z

/retest

gjkim42 · 2021-09-03T06:36:50Z

/assign @smarterclayton

Made an alternative to #104031

gjkim42 · 2021-09-03T06:38:54Z

/retest

I think kubelet-serial tests are broken

https://prow.k8s.io/?repo=kubernetes%2Fkubernetes&job=pull-kubernetes-node-kubelet-serial

gjkim42 · 2021-09-03T06:42:37Z

/sig node
/priority important-soon

gjkim42 · 2021-10-19T01:13:52Z

/retest

k8s-ci-robot · 2021-10-19T07:59:03Z

@gjkim42: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-node-kubelet-serial-containerd	530706b74c43ae5e025c49f4acc3ee6d5117684c	link	false	`/test pull-kubernetes-node-kubelet-serial-containerd`
pull-kubernetes-node-kubelet-serial	530706b74c43ae5e025c49f4acc3ee6d5117684c	link	false	`/test pull-kubernetes-node-kubelet-serial`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

gjkim42 · 2021-10-19T08:05:12Z

/retest

gjkim42 · 2021-10-21T22:15:19Z

/assign @ehashman
For final review

gjkim42 · 2021-11-03T00:06:33Z

/cc @ehashman @derekwaynecarr @Random-Liu
Need final review from pod_worker maintainers :)

ehashman · 2021-11-04T23:02:06Z

pkg/kubelet/pod_workers.go

+	if !started {
+		return false
+	}


Is it not possible for a pod that hasn't been started to be terminating? I'm thinking of the edge case for a static pod that might be in the middle of setup (e.g. an initContainer) and gets removed before it reaches Running.

Maybe it could still be in waitingToStartStaticPodsByFullname?

I'm thinking of the edge case for a static pod that might be in the middle of setup (e.g. an initContainer) and gets removed before it reaches Running.

A static pod can start initContainer after the static pod has started(moved into startedStaticPodsFullname).

And I think that the non-started static pod does not have the mirror pod, so only the started static pod should be passed to this function.

ehashman · 2021-11-04T23:08:24Z

pkg/kubelet/pod_workers.go

+	if !p.allowStaticPodStart(status.fullname, pod.UID) {
+		p.workQueue.Enqueue(pod.UID, wait.Jitter(p.backOffPeriod, workerBackOffPeriodJitterFactor))
+		status.working = false
+		return false


If a static pod is never allowed to start, what happens? It appears that we keep requeueing it forever. Is that possible?

Yes, it is possible if the previous static pod is terminating forever, but I think we are ensuring that the termination requested pod terminates after terminationGracePeriodSeconds.

ehashman · 2021-11-04T23:11:20Z

pkg/kubelet/pod_workers.go

 	if status, ok := p.podSyncStatuses[pod.UID]; ok {
 		if status.terminatingAt.IsZero() {
 			klog.V(4).InfoS("Pod worker was terminated but did not have terminatingAt set, likely programmer error", "pod", klog.KObj(pod), "podUID", pod.UID)
 		}
 		status.terminatedAt = time.Now()
 		status.finished = true
 		status.working = false
+
+		if p.startedStaticPodsByFullname[status.fullname] == pod.UID {
+			delete(p.startedStaticPodsByFullname, status.fullname)


I don't see waitingToStartStaticPodsByFullname getting cleared out anywhere when it's != 0... should that happen here or somewhere else?

waitingToStartStaticPodsByFullname is basically a set of queues and there can be more than one waiting to start static pods with the same full name.
So we can't clear waitingToStartStaticPodsByFullname until it's empty.

ehashman · 2021-11-04T23:18:10Z

pkg/kubelet/pod_workers_test.go

+					types.UID("uid-2"),
+					types.UID("uid-2"),


Is the same UID repeated here intentional?

Maybe I just wanted to make sure that invalid and redundant waiting pods are trimmed.

ehashman · 2021-11-04T23:19:44Z

pkg/kubelet/pod_workers_test.go

+				} else {
+					t.Errorf("Pod should not be allowed")
+				}
+			}


Still need this added I think

ehashman · 2021-11-04T23:21:10Z

test/e2e_node/mirror_pod_grace_period_test.go

-				time.Sleep(time.Duration(200) * time.Millisecond)
-			}
+			ginkgo.By("wait for the mirror pod to be running for grace period")
+			gomega.Consistently(func() error {


Shouldn't this be Eventually if we're waiting? Consistently checks that it's true every time and will fail on the first false if it's not.

This ensures that the mirror pod is running for terminationGracePeriodSeconds consistently.

gjkim42 · 2021-11-11T13:29:07Z

@ehashman
Could you PTAL again? Thanks!

ehashman

Chatted with @smarterclayton about this, I think we should merge and get signal on this from CI ASAP.

Sorry for my delays on review!

/lgtm

…743-upstream-release-1.22 Automated cherry pick of #104743: Ensure there is one running static pod with the same full name

liggitt · 2022-01-25T13:39:49Z

for anyone subscribed to this PR, a revert is proposed in #107734

k8s-ci-robot requested review from andrewsykim and odinuge September 3, 2021 01:51

gjkim42 changed the title ~~Ensure pod uniqueness~~ Ensure there is one running pod with the same full name Sep 3, 2021

gjkim42 changed the title ~~Ensure there is one running pod with the same full name~~ WIP: Ensure there is one running pod with the same full name Sep 3, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 3, 2021

gjkim42 mentioned this pull request Sep 3, 2021

WIP: Reserve one conflicted static pod for each pod worker #104713

Closed

gjkim42 force-pushed the ensure-pod-uniqueness branch 2 times, most recently from ca4dfc1 to affcc7e Compare September 3, 2021 03:12

gjkim42 changed the title ~~WIP: Ensure there is one running pod with the same full name~~ WIP: Ensure there is one running static pod with the same full name Sep 3, 2021

k8s-ci-robot assigned smarterclayton Sep 3, 2021

gjkim42 changed the title ~~WIP: Ensure there is one running static pod with the same full name~~ Ensure there is one running static pod with the same full name Sep 3, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 3, 2021

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 3, 2021

gjkim42 added 2 commits October 19, 2021 16:30

NodeConformance: Respect grace period when updating static pod

0f2c94d

Ensure there is one running static pod with the same full name

3bce245

gjkim42 force-pushed the ensure-pod-uniqueness branch from e852ec2 to 3bce245 Compare October 19, 2021 07:30

k8s-ci-robot assigned ehashman Oct 21, 2021

gjkim42 mentioned this pull request Oct 27, 2021

Static pod should be updated by respecting terminationGracePeriodSeconds #97722

Closed

k8s-ci-robot requested review from derekwaynecarr and Random-Liu November 3, 2021 00:06

ehashman reviewed Nov 4, 2021

View reviewed changes

ehashman reviewed Nov 12, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2021

k8s-ci-robot merged commit 5f0a94b into kubernetes:master Nov 12, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 12, 2021

gjkim42 mentioned this pull request Nov 12, 2021

Automated cherry pick of #104743: Ensure there is one running static pod with the same full name #106394

Merged

gjkim42 deleted the ensure-pod-uniqueness branch November 14, 2021 11:51

k8s-ci-robot added a commit that referenced this pull request Dec 2, 2021

Merge pull request #106394 from gjkim42/automated-cherry-pick-of-#104…

da50388

…743-upstream-release-1.22 Automated cherry pick of #104743: Ensure there is one running static pod with the same full name

This was referenced Jan 21, 2022

kubelet: fix static pods not restarting in certain cases #107695

Merged

Some static pods fail to start on K8S 1.22 and 1.23 #107733

Closed

Revert "Ensure there is one running static pod with the same full name" #107734

Closed

liushi001 mentioned this pull request Jan 29, 2022

Kubelet cannot start static pod when move in manifest file after removing manifest file for a while #107483

Closed

ping035627 mentioned this pull request Oct 12, 2022

To avoid the staticpod cannot be created after kl.syncTerminated fails #112998

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure there is one running static pod with the same full name #104743

Ensure there is one running static pod with the same full name #104743

gjkim42 commented Sep 3, 2021 •

edited

Loading

gjkim42 commented Sep 3, 2021

gjkim42 commented Sep 3, 2021

gjkim42 commented Sep 3, 2021

gjkim42 commented Sep 3, 2021

gjkim42 commented Oct 19, 2021

k8s-ci-robot commented Oct 19, 2021 •

edited

Loading

gjkim42 commented Oct 19, 2021

gjkim42 commented Oct 21, 2021

gjkim42 commented Nov 3, 2021

ehashman Nov 4, 2021

gjkim42 Nov 5, 2021

ehashman Nov 4, 2021

gjkim42 Nov 5, 2021

ehashman Nov 4, 2021

gjkim42 Nov 5, 2021 •

edited

Loading

ehashman Nov 4, 2021

gjkim42 Nov 5, 2021

ehashman Nov 4, 2021

ehashman Nov 4, 2021

gjkim42 Nov 5, 2021

gjkim42 commented Nov 11, 2021

ehashman left a comment

liggitt commented Jan 25, 2022

Ensure there is one running static pod with the same full name #104743

Ensure there is one running static pod with the same full name #104743

Conversation

gjkim42 commented Sep 3, 2021 • edited 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.:

gjkim42 commented Sep 3, 2021

gjkim42 commented Sep 3, 2021

gjkim42 commented Sep 3, 2021

gjkim42 commented Sep 3, 2021

gjkim42 commented Oct 19, 2021

k8s-ci-robot commented Oct 19, 2021 • edited Loading

gjkim42 commented Oct 19, 2021

gjkim42 commented Oct 21, 2021

gjkim42 commented Nov 3, 2021

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

gjkim42 Nov 5, 2021 • edited Loading

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

gjkim42 commented Nov 11, 2021

ehashman left a comment

Choose a reason for hiding this comment

liggitt commented Jan 25, 2022

gjkim42 commented Sep 3, 2021 •

edited

Loading

k8s-ci-robot commented Oct 19, 2021 •

edited

Loading

gjkim42 Nov 5, 2021 •

edited

Loading