Rerun init containers when the pod needs to be restarted #47599

yujuhong · 2017-06-15T16:40:41Z

Whenever pod sandbox needs to be recreated, all containers associated
with it will be killed by kubelet. This change ensures that the init
containers will be rerun in such cases.

The change also refactors the compute logic so that the control flow of
init containers act is more aligned with the regular containers. Unit
tests are added to verify the logic.

This fixes #36485

k8s-github-robot · 2017-06-15T16:40:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yujuhong

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubelet/OWNERS~~ [yujuhong]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

yujuhong · 2017-06-19T22:44:53Z

/cc @yifan-gu
Still WIP; not a large-scale improvement, but at least this PR should make init containers work more similar to regular containers.

yujuhong · 2017-06-21T22:59:13Z

/cc @smarterclayton

yujuhong · 2017-06-22T01:41:22Z

/cc @dchen1107

yujuhong · 2017-06-22T18:30:31Z

pkg/kubelet/kuberuntime/kuberuntime_manager_test.go

+		mutatePodFn    func(*v1.Pod)
+		mutateStatusFn func(*kubecontainer.PodStatus)
+		actions        podActions
+	}{


@smarterclayton @dchen1107, please read the description of the test cases and make sure they meet your expectation. Thanks!

yujuhong · 2017-07-10T18:00:11Z

/retest

feiskyer · 2017-07-11T07:39:38Z

LGTM

munnerz · 2017-07-14T14:07:36Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

+		initContainerNames.Insert(container.Name)
+	}
+	for name := range initContainerNames {
+		count := 0


From what I can see here, this count variable never increments? It's printed below on L673, and in the previous implementation would be incremented on each iteration of the podStatus.ContainerStatuses loop.

I'm not particularly familiar with this bit of code, so I could be wrong. I'm currently attempting to port this patch to the 1.6 dockertools implementation.

Yes, the count should be incremented. Will do this later, after getting more comments. The variable is only used in the log message, so it does not affect the functionality of the code.

yujuhong · 2017-08-14T17:29:19Z

@dchen1107 @Random-Liu @smarterclayton PTAL.

smarterclayton · 2017-08-14T23:15:34Z

Tests look good to me. This is generally LGTM, although I can do a deeper pass if necessary.

yujuhong · 2017-08-14T23:44:58Z

Tests look good to me. This is generally LGTM, although I can do a deeper pass if necessary.

Thanks for the review! That's exactly what I needed from you and @dchen1107

@feiskyer already took a pass and LGTM'd the PR. I'll keep the PR open for a couple more days because @Random-Liu mentioned that he may have time to look at it.

Random-Liu · 2017-08-16T00:19:04Z

pkg/kubelet/apis/cri/testing/fake_runtime_service.go

+	defer r.Unlock()
+
+	for id, c := range r.Containers {
+		if c.Metadata.Name == name && c.Metadata.Attempt == attempt {


Why not compare sandboxID?

Good catch. Added.

Random-Liu · 2017-08-16T00:24:24Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -620,7 +620,7 @@ func (m *kubeGenericRuntimeManager) killContainersWithSyncResult(pod *v1.Pod, ru
 // pruneInitContainers ensures that before we begin creating init containers, we have reduced the number


s/pruneInitContainers/pruneInitContainersBeforeStart

Random-Liu · 2017-08-16T00:34:15Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

-			if _, ok := initContainersToKeep[status.ID]; ok {
+			// prune all other init containers that match this container name
+			glog.V(4).Infof("Removing init container %q instance %q %d", status.Name, status.ID.ID, count)
+			if err := m.DeleteContainer(status.ID); err != nil {


Why use DeleteContainer here but use runtimeService.RemoveContainer below.

You're right. Switching both to m.removeContainer(status.ID.ID) to make them consistent. I think we do need to clean up the log here since we don't do it anywhere else.

Random-Liu · 2017-08-16T00:36:04Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -620,7 +620,7 @@ func (m *kubeGenericRuntimeManager) killContainersWithSyncResult(pod *v1.Pod, ru
 // pruneInitContainers ensures that before we begin creating init containers, we have reduced the number
 // of outstanding init containers still present. This reduces load on the container garbage collector
 // by only preserving the most recent terminated init container.
-func (m *kubeGenericRuntimeManager) pruneInitContainersBeforeStart(pod *v1.Pod, podStatus *kubecontainer.PodStatus, initContainersToKeep map[kubecontainer.ContainerID]int) {
+func (m *kubeGenericRuntimeManager) pruneInitContainersBeforeStart(pod *v1.Pod, podStatus *kubecontainer.PodStatus) {


Maybe just add one argument, e.g. force bool or something.
If it's force, delete all; if not, keep latest exited one?

The 2 functions are almost the same, :)

I disagree. I think it's clear what each function does with the current naming. Adding a boolean would make the function more complicated and harder to test.

Random-Liu · 2017-08-16T01:24:23Z

pkg/kubelet/kuberuntime/kuberuntime_manager.go

 		containerStatus := podStatus.FindContainerStatusByName(container.Name)
+		// If container does not exist, or is not running, check whehter we


s/whehter/whether

Random-Liu · 2017-08-16T01:24:54Z

pkg/kubelet/kuberuntime/kuberuntime_manager.go

+			reason = "Container failed liveness probe."
+		} else {
+			// Keep the container.
+			keepCount += 1


I think both are perfectly fine unless this documented as convention(?)

Random-Liu · 2017-08-16T01:39:38Z

pkg/kubelet/kuberuntime/kuberuntime_manager.go

-	m.pruneInitContainersBeforeStart(pod, podStatus, podContainerChanges.InitContainersToKeep)
+	// This is an optmization because container removals are typically handled
+	// by container garbage collector.
+	m.pruneInitContainersBeforeStart(pod, podStatus)


We purge and prune, add an options seems cleaner.

Still think it's clear to keep both functions.

yujuhong · 2017-08-16T15:53:50Z

@Random-Liu comments addressed. PTAL again, thanks

yujuhong · 2017-08-16T21:56:06Z

Pushed a new commit to address a bug @Random-Liu found (good catch!). Also added a new unit test for it.

Random-Liu · 2017-08-16T22:21:09Z

@yujuhong LGTM without full confidence.

I could understand the current behavior, and it looks much cleaner than before!
However, It's hard to figure out the original behavior, so I don't have full confidence that there won't be regression or behavior change.

I think we could only rely on the test to catch it for us. :p

Whenever pod sandbox needs to be recreated, all containers associated with it will be killed by kubelet. This change ensures that the init containers will be rerun in such cases. The change also refactors the compute logic so that the control flow of init containers act is more aligned with the regular containers. Unit tests are added to verify the logic.

yujuhong · 2017-08-16T22:29:26Z

Rebased and squashed the commits.
Applying the lgtm label based on LGTMs from #47599 (comment) and #47599 (comment)

feiskyer · 2017-08-17T01:04:25Z

@yujuhong Adds a release-note for this?

k8s-github-robot · 2017-08-17T02:50:21Z

Automatic merge from submit-queue (batch tested with PRs 46317, 48922, 50651, 50230, 47599)

Automatic merge from submit-queue (batch tested with PRs 16889, 16865). UPSTREAM: 53857: kubelet sync pod throws more detailed events Also includes the following upstream dependant PRs: UPSTREAM: 50350: Wait for container cleanup before deletion UPSTREAM: 48970: Recreate pod sandbox when the sandbox does not have an IP address. UPSTREAM: 48589: When faild create pod sandbox record event. UPSTREAM: 48584: Move event type UPSTREAM: 47599: Rerun init containers when the pod needs to be restarted xrefs: kubernetes/kubernetes#53857 kubernetes/kubernetes#50350 kubernetes/kubernetes#48970 kubernetes/kubernetes#48589 kubernetes/kubernetes#48584 kubernetes/kubernetes#47599

yujuhong added the release-note-none Denotes a PR that doesn't merit a release note. label Jun 15, 2017

yujuhong self-assigned this Jun 15, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 15, 2017

yujuhong added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 15, 2017

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 15, 2017

yujuhong force-pushed the restart-init branch from e3b9daa to a8ef5c0 Compare June 15, 2017 18:06

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 15, 2017

yujuhong force-pushed the restart-init branch 3 times, most recently from 6a9cc3f to c683ca0 Compare June 15, 2017 22:54

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 16, 2017

k8s-ci-robot requested a review from yifan-gu June 19, 2017 22:44

yujuhong force-pushed the restart-init branch from 940d452 to 02bddd6 Compare June 21, 2017 22:55

yujuhong changed the title ~~WIP/testing: restart init containers~~ Rerun init containers when the pod needs to be restarted Jun 21, 2017

yujuhong removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 21, 2017

yujuhong assigned Random-Liu and feiskyer and unassigned yujuhong Jun 21, 2017

k8s-ci-robot requested a review from smarterclayton June 21, 2017 22:59

yujuhong added this to the v1.8 milestone Jun 22, 2017

k8s-ci-robot requested a review from dchen1107 June 22, 2017 01:41

dchen1107 assigned smarterclayton and unassigned feiskyer and Random-Liu Jun 22, 2017

yujuhong commented Jun 22, 2017

View reviewed changes

yujuhong assigned Random-Liu Jun 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2017

simonswine mentioned this pull request Jul 11, 2017

Containers from init-containers section do not execute during node reboots #36485

Closed

munnerz reviewed Jul 14, 2017

View reviewed changes

Random-Liu reviewed Aug 16, 2017

View reviewed changes

yujuhong force-pushed the restart-init branch from 115f60d to 152d8b9 Compare August 16, 2017 22:27

yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2017

yujuhong added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 17, 2017

k8s-github-robot merged commit 4bfe9b1 into kubernetes:master Aug 17, 2017

yujuhong deleted the restart-init branch September 11, 2017 20:32

Random-Liu mentioned this pull request Oct 4, 2017

emptyDir volume does not empty in pod after physical node restart #53423

Closed

joelsmith mentioned this pull request Oct 13, 2017

UPSTREAM: 53857: kubelet sync pod throws more detailed events openshift/origin#16865

Merged

luckyfengyong mentioned this pull request Nov 7, 2017

[incubator/elasticsearch] ES failed to start after reboot node due to vm.max_map_count is small helm/charts#2677

Closed

gyliu513 mentioned this pull request Nov 7, 2017

init container does not re-run after node reboot #55206

Closed

mike-capyh mentioned this pull request Mar 19, 2024

Kubernetes Skips Init Containers Beyond First During Some Pod Restarts #124002

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rerun init containers when the pod needs to be restarted #47599

Rerun init containers when the pod needs to be restarted #47599

yujuhong commented Jun 15, 2017 •

edited

Loading

k8s-github-robot commented Jun 15, 2017

yujuhong commented Jun 19, 2017

yujuhong commented Jun 21, 2017

yujuhong commented Jun 22, 2017

yujuhong Jun 22, 2017

yujuhong commented Jul 10, 2017

feiskyer commented Jul 11, 2017

munnerz Jul 14, 2017 •

edited

Loading

yujuhong Jul 14, 2017

yujuhong Aug 16, 2017

yujuhong commented Aug 14, 2017

smarterclayton commented Aug 14, 2017

yujuhong commented Aug 14, 2017

Random-Liu Aug 16, 2017

yujuhong Aug 16, 2017

Random-Liu Aug 16, 2017

yujuhong Aug 16, 2017

Random-Liu Aug 16, 2017

yujuhong Aug 16, 2017

Random-Liu Aug 16, 2017

yujuhong Aug 16, 2017

Random-Liu Aug 16, 2017

yujuhong Aug 16, 2017

Random-Liu Aug 16, 2017

yujuhong Aug 16, 2017

Random-Liu Aug 16, 2017

yujuhong Aug 16, 2017

yujuhong commented Aug 16, 2017

yujuhong commented Aug 16, 2017

Random-Liu commented Aug 16, 2017 •

edited

Loading

yujuhong commented Aug 16, 2017

feiskyer commented Aug 17, 2017

k8s-github-robot commented Aug 17, 2017

		@@ -620,7 +620,7 @@ func (m kubeGenericRuntimeManager) killContainersWithSyncResult(pod v1.Pod, ru
		// pruneInitContainers ensures that before we begin creating init containers, we have reduced the number

		containerStatus := podStatus.FindContainerStatusByName(container.Name)
		// If container does not exist, or is not running, check whehter we

Rerun init containers when the pod needs to be restarted #47599

Rerun init containers when the pod needs to be restarted #47599

Conversation

yujuhong commented Jun 15, 2017 • edited Loading

k8s-github-robot commented Jun 15, 2017

yujuhong commented Jun 19, 2017

yujuhong commented Jun 21, 2017

yujuhong commented Jun 22, 2017

Choose a reason for hiding this comment

yujuhong commented Jul 10, 2017

feiskyer commented Jul 11, 2017

munnerz Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujuhong commented Aug 14, 2017

smarterclayton commented Aug 14, 2017

yujuhong commented Aug 14, 2017

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

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

yujuhong commented Aug 16, 2017

yujuhong commented Aug 16, 2017

Random-Liu commented Aug 16, 2017 • edited Loading

yujuhong commented Aug 16, 2017

feiskyer commented Aug 17, 2017

k8s-github-robot commented Aug 17, 2017

yujuhong commented Jun 15, 2017 •

edited

Loading

munnerz Jul 14, 2017 •

edited

Loading

Random-Liu commented Aug 16, 2017 •

edited

Loading