From 906286c743ec18cd428aa78a766fd46157fceb97 Mon Sep 17 00:00:00 2001
From: Lee Verberne <verb@google.com>
Date: Fri, 2 Aug 2019 19:33:49 +0000
Subject: [PATCH] Change order kubelet starts containers

This starts ephemeral containers prior to init containers so that
ephemeral containers will still be started when init containers fail to
start.

Also improves tests and comments with review suggestions.
---
 pkg/kubelet/container/helpers_test.go         | 10 +++---
 pkg/kubelet/kubelet_pods.go                   |  2 +-
 .../kuberuntime/kuberuntime_manager.go        | 32 +++++++++++--------
 3 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/pkg/kubelet/container/helpers_test.go b/pkg/kubelet/container/helpers_test.go
index eff41d4e01d..3acb1120cbd 100644
--- a/pkg/kubelet/container/helpers_test.go
+++ b/pkg/kubelet/container/helpers_test.go
@@ -383,10 +383,12 @@ func TestGetContainerSpec(t *testing.T) {
 			wantContainer: &v1.Container{Name: "debug-container"},
 		},
 	} {
-		gotContainer := GetContainerSpec(tc.havePod, tc.haveName)
-		if diff := cmp.Diff(tc.wantContainer, gotContainer); diff != "" {
-			t.Errorf("GetContainerSpec for %q returned diff (-want +got):%v", tc.name, diff)
-		}
+		t.Run(tc.name, func(t *testing.T) {
+			gotContainer := GetContainerSpec(tc.havePod, tc.haveName)
+			if diff := cmp.Diff(tc.wantContainer, gotContainer); diff != "" {
+				t.Fatalf("GetContainerSpec for %q returned diff (-want +got):%v", tc.name, diff)
+			}
+		})
 	}
 }
 
diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go
index a1a355379b2..d59828aaa37 100644
--- a/pkg/kubelet/kubelet_pods.go
+++ b/pkg/kubelet/kubelet_pods.go
@@ -1420,7 +1420,7 @@ func (kl *Kubelet) convertStatusToAPIStatus(pod *v1.Pod, podStatus *kubecontaine
 			ecSpecs = append(ecSpecs, v1.Container(pod.Spec.EphemeralContainers[i].EphemeralContainerCommon))
 		}
 
-		// TODO(verb): By now we've iterated podStatus 3 times. We could refactor this to make a single
+		// #80875: By now we've iterated podStatus 3 times. We could refactor this to make a single
 		// pass through podStatus.ContainerStatuses
 		apiPodStatus.EphemeralContainerStatuses = kl.convertToAPIContainerStatuses(
 			pod, podStatus,
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager.go b/pkg/kubelet/kuberuntime/kuberuntime_manager.go
index 3cda1f3cb6c..af235957365 100644
--- a/pkg/kubelet/kuberuntime/kuberuntime_manager.go
+++ b/pkg/kubelet/kuberuntime/kuberuntime_manager.go
@@ -625,9 +625,9 @@ func (m *kubeGenericRuntimeManager) computePodActions(pod *v1.Pod, podStatus *ku
 //  2. Kill pod sandbox if necessary.
 //  3. Kill any containers that should not be running.
 //  4. Create sandbox if necessary.
-//  5. Create init containers.
-//  6. Create normal containers.
-//  7. Create ephemeral containers.
+//  5. Create ephemeral containers.
+//  6. Create init containers.
+//  7. Create normal containers.
 func (m *kubeGenericRuntimeManager) SyncPod(pod *v1.Pod, podStatus *kubecontainer.PodStatus, pullSecrets []v1.Secret, backOff *flowcontrol.Backoff) (result kubecontainer.PodSyncResult) {
 	// Step 1: Compute sandbox and container changes.
 	podContainerChanges := m.computePodActions(pod, podStatus)
@@ -758,7 +758,8 @@ func (m *kubeGenericRuntimeManager) SyncPod(pod *v1.Pod, podStatus *kubecontaine
 	}
 
 	// Helper containing boilerplate common to starting all types of containers.
-	// typeName is a label used to describe this type of container in log messages.
+	// typeName is a label used to describe this type of container in log messages,
+	// currently: "container", "init container" or "ephemeral container"
 	start := func(typeName string, container *v1.Container) error {
 		startContainerResult := kubecontainer.NewSyncResult(kubecontainer.StartContainer, container.Name)
 		result.AddSyncResult(startContainerResult)
@@ -787,7 +788,18 @@ func (m *kubeGenericRuntimeManager) SyncPod(pod *v1.Pod, podStatus *kubecontaine
 		return nil
 	}
 
-	// Step 5: start the init container.
+	// Step 5: start ephemeral containers
+	// These are started "prior" to init containers to allow running ephemeral containers even when there
+	// are errors starting an init container. In practice init containers will start first since ephemeral
+	// containers cannot be specified on pod creation.
+	if utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) {
+		for _, idx := range podContainerChanges.EphemeralContainersToStart {
+			c := (*v1.Container)(&pod.Spec.EphemeralContainers[idx].EphemeralContainerCommon)
+			start("ephemeral container", c)
+		}
+	}
+
+	// Step 6: start the init container.
 	if container := podContainerChanges.NextInitContainerToStart; container != nil {
 		// Start the next init container.
 		if err := start("init container", container); err != nil {
@@ -798,19 +810,11 @@ func (m *kubeGenericRuntimeManager) SyncPod(pod *v1.Pod, podStatus *kubecontaine
 		klog.V(4).Infof("Completed init container %q for pod %q", container.Name, format.Pod(pod))
 	}
 
-	// Step 6: start containers in podContainerChanges.ContainersToStart.
+	// Step 7: start containers in podContainerChanges.ContainersToStart.
 	for _, idx := range podContainerChanges.ContainersToStart {
 		start("container", &pod.Spec.Containers[idx])
 	}
 
-	// Step 7: start ephemeral containers
-	if utilfeature.DefaultFeatureGate.Enabled(features.EphemeralContainers) {
-		for _, idx := range podContainerChanges.EphemeralContainersToStart {
-			c := (*v1.Container)(&pod.Spec.EphemeralContainers[idx].EphemeralContainerCommon)
-			start("ephemeral container", c)
-		}
-	}
-
 	return
 }