Repository: kubernetes/kubernetes (language: go)

Issue:
`SyncPod` may fail to start a pod with init container if `RunContainerError` occurs and SidecarContainers is enabled
### What happened?

It was consistently observed in a testing environment that a node-critical pod failed to start. `Created container setup` event was present but `Started container setup` was missing(`setup` is an init container). Also the following error was present but strangely only once: 
```
init container &Container{Name:setup,...} start failed in pod ... : RunContainerError: error reading from server: EOF
```

After some digging it seemed `m.runtimeService.StartContainer` throwing an error matched the observations above.
https://github.com/kubernetes/kubernetes/blob/7a4c962341e6ccd89db6736911e7e3bc7dd5165d/pkg/kubelet/kuberuntime/kuberuntime_container.go#L277-L286

### What did you expect to happen?

Eventually the init container and the pod to start

### How can we reproduce it (as minimally and precisely as possible)?

The following tests in `pkg/kubelet/kuberuntime/kuberuntime_manager_test.go` recreate the problem by `fakeRuntime.InjectError("StartContainer", errors.New("any error"))` and show how depending on `features.SidecarContainers` the behavior is different. When the feature gate is enabled the last init container gets stuck in `CONTAINER_CREATED` and never gets started. Therefore the pod get's stuck and never progresses

```go

func TestASidecarContainersTrue(t *testing.T) {
	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true)
	expected := []*cRecord{
		{name: "init1", attempt: 0, state: runtimeapi.ContainerState_CONTAINER_CREATED},
	}
	singleInit(t, expected)
}

func TestASidecarContainersFalse(t *testing.T) {
	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, false)
	expected := []*cRecord{
		{name: "init1", attempt: 0, state: runtimeapi.ContainerState_CONTAINER_CREATED},
		{name: "init1", attempt: 1, state: runtimeapi.ContainerState_CONTAINER_RUNNING},
	}
	singleInit(t, expected)
}

func singleInit(t *testing.T, finalCRecord []*cRecord) {
	ctx := context.Background()
	fakeRuntime, _, m, err := createTestRuntimeManager()
	assert.NoError(t, err)

	initContainers := []v1.Container{
		{
			Name:            "init1",
			Image:           "init",
			ImagePullPolicy: v1.PullIfNotPresent,
		},
	}
	containers := []v1.Container{
		{
			Name:            "foo1",
			Image:           "busybox",
			ImagePullPolicy: v1.PullIfNotPresent,
		},
		{
			Name:            "foo2",
			Image:           "alpine",
			ImagePullPolicy: v1.PullIfNotPresent,
		},
	}
	pod := &v1.Pod{
		ObjectMeta: metav1.ObjectMeta{
			UID:       "12345678",
			Name:      "foo",
			Namespace: "new",
		},
		Spec: v1.PodSpec{
			Containers:     containers,
			InitContainers: initContainers,
		},
	}

	backOff := flowcontrol.NewBackOff(time.Second, time.Minute)

	// 0. should fail creating init container
	podStatus, err := m.GetPodStatus(ctx, pod.UID, pod.Name, pod.Namespace)
	assert.NoError(t, err)
	fakeRuntime.InjectError("StartContainer", errors.New("any error"))
	result := m.SyncPod(context.Background(), pod, podStatus, []v1.Secret{}, backOff)
	err = result.Error()
	fmt.Println(err)
	assert.Error(t, err)
	expected := []*cRecord{
		{name: initContainers[0].Name, attempt: 0, state: runtimeapi.ContainerState_CONTAINER_CREATED},
	}
	verifyContainerStatuses(t, fakeRuntime, expected, "start only the init container")

	// 2. sync multiple times and test for expected final cRecords
	for _ = range 10 {
		podStatus, err = m.GetPodStatus(ctx, pod.UID, pod.Name, pod.Namespace)
		assert.NoError(t, err)
		result = m.SyncPod(ctx, pod, podStatus, []v1.Secret{}, backOff)
		assert.NoError(t, result.Error())
		verifyContainerStatuses(t, fakeRuntime, finalCRecord, "syncing")
	}
}

func TestBSidecarContainersTrue(t *testing.T) {
	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true)
	expected := []*cRecord{
		{name: "init1", attempt: 0, state: runtimeapi.ContainerState_CONTAINER_EXITED},
		{name: "init2", attempt: 0, state: runtimeapi.ContainerState_CONTAINER_CREATED},
	}
	multipleInits(t, expected)
}

func TestBSidecarContainersFalse(t *testing.T) {
	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, false)
	expected := []*cRecord{
		{name: "init1", attempt: 0, state: runtimeapi.ContainerState_CONTAINER_EXITED},
		{name: "init2", attempt: 0, state: runtimeapi.ContainerState_CONTAINER_CREATED},
		{name: "init2", attempt: 1, state: runtimeapi.ContainerState_CONTAINER_RUNNING},
	}
	multipleInits(t, expected)
}

func multipleInits(t *testing.T, finalCRecord []*cRecord) {
	ctx := context.Background()
	fakeRuntime, _, m, err := createTestRuntimeManager()
	assert.NoError(t, err)

	initContainers := []v1.Container{
		{
			Name:            "init1",
			Image:           "init",
			ImagePullPolicy: v1.PullIfNotPresent,
		},
		{
			Name:            "init2",
			Image:           "init2",
			ImagePullPolicy: v1.PullIfNotPresent,
		},
	}
	containers := []v1.Container{
		{
			Name:            "foo1",
			Image:           "busybox",
			ImagePullPolicy: v1.PullIfNotPresent,
		},
		{
			Name:            "foo2",
			Image:           "alpine",
			ImagePullPolicy: v1.PullIfNotPresent,
		},
	}
	pod := &v1.Pod{
		ObjectMeta: metav1.ObjectMeta{
			UID:       "12345678",
			Name:      "foo",
			Namespace: "new",
		},
		Spec: v1.PodSpec{
			Containers:     containers,
			InitContainers: initContainers,
		},
	}

	backOff := flowcontrol.NewBackOff(time.Second, time.Minute)

	// 1. should  create the firs init container.
	podStatus, err := m.GetPodStatus(ctx, pod.UID, pod.Name, pod.Namespace)
	assert.NoError(t, err)
	result := m.SyncPod(context.Background(), pod, podStatus, []v1.Secret{}, backOff)
	assert.NoError(t, result.Error())
	expected := []*cRecord{
		{name: initContainers[0].Name, attempt: 0, state: runtimeapi.ContainerState_CONTAINER_RUNNING},
	}
	verifyContainerStatuses(t, fakeRuntime, expected, "start the first init container")

	// 2. finish first init and fail to start the second
	// Stop init container instance 0.
	sandboxIDs, err := m.getSandboxIDByPodUID(ctx, pod.UID, nil)
	require.NoError(t, err)
	sandboxID := sandboxIDs[0]
	initID0, err := fakeRuntime.GetContainerID(sandboxID, initContainers[0].Name, 0)
	require.NoError(t, err)
	fakeRuntime.StopContainer(ctx, initID0, 0)
	// Simulate StartContainer failure
	fakeRuntime.InjectError("StartContainer", errors.New("any error"))
	// Sync again.
	podStatus, err = m.GetPodStatus(ctx, pod.UID, pod.Name, pod.Namespace)
	assert.NoError(t, err)
	result = m.SyncPod(ctx, pod, podStatus, []v1.Secret{}, backOff)
	assert.Error(t, result.Error())
	expected = []*cRecord{
		{name: initContainers[0].Name, attempt: 0, state: runtimeapi.ContainerState_CONTAINER_EXITED},
		{name: initContainers[1].Name, attempt: 0, state: runtimeapi.ContainerState_CONTAINER_CREATED},
	}
	verifyContainerStatuses(t, fakeRuntime, expected, "finish first init and fail to start the second")

	// sync multiple times and test for expected final cRecords
	for _ = range 10 {
		podStatus, err = m.GetPodStatus(ctx, pod.UID, pod.Name, pod.Namespace)
		assert.NoError(t, err)
		result = m.SyncPod(ctx, pod, podStatus, []v1.Secret{}, backOff)
		assert.NoError(t, result.Error())
		verifyContainerStatuses(t, fakeRuntime, finalCRecord, "syncing")
	}
}
```

### Anything else we need to know?

The problem seems to stem from:
https://github.com/kubernetes/kubernetes/blob/7a4c962341e6ccd89db6736911e7e3bc7dd5165d/pkg/kubelet/kuberuntime/kuberuntime_container.go#L1015-L1018

There `kubecontainer.ContainerStateCreated` case get's skipped and the created container is expected to get started by the previous one by the following lines
https://github.com/kubernetes/kubernetes/blob/7a4c962341e6ccd89db6736911e7e3bc7dd5165d/pkg/kubelet/kuberuntime/kuberuntime_container.go#L1122
or
https://github.com/kubernetes/kubernetes/blob/7a4c962341e6ccd89db6736911e7e3bc7dd5165d/pkg/kubelet/kuberuntime/kuberuntime_container.go#L1171
But the provided tests show that this does not happen. Also if there is only one init container with `kubecontainer.ContainerStateCreated` the following logic should execute but won't
https://github.com/kubernetes/kubernetes/blob/7a4c962341e6ccd89db6736911e7e3bc7dd5165d/pkg/kubelet/kuberuntime/kuberuntime_container.go#L1215-L1219
because in this case this get's executed
https://github.com/kubernetes/kubernetes/blob/7a4c962341e6ccd89db6736911e7e3bc7dd5165d/pkg/kubelet/kuberuntime/kuberuntime_container.go#L1209-L1212

### Kubernetes version

<details>

```console
$ kubectl version
Client Version: v1.30.1
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.30.0
```

</details>


### Cloud provider

<details>

</details>


### OS version

<details>

```console
# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here
```

</details>


### Install tools

<details>

</details>


### Container runtime (CRI) and version (if applicable)

<details>

</details>


### Related plugins (CNI, CSI, ...) and versions (if applicable)

<details>

</details>



Discussion / hints:
/sig node

@gjkim42, @matthyx, @SergeyKanzhelev can you help checking this issue report as according to @Kostov6 it is a regression introduced with https://github.com/kubernetes/kubernetes/pull/116429? Thanks in advance!
I'll take a look.
/assign
I guess this is a valid bug.
/triage accepted
/priority important-soon

@Kostov6 
Can you provide a more specific scenario for this?
Why do container runtime fail to start the container? (How can I reproduce it in e2e test?)
https://github.com/kubernetes/kubernetes/blob/00236ae0d73d2455a2470469ed1005674f8ed61f/pkg/kubelet/kuberuntime/kuberuntime_container.go#L1085-L1087
^
I guess this is the wrong assumption.
If the container is in the `ContainerStateCreated` state, the kubelet should resync it.

Fix patch:
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container.go b/pkg/kubelet/kuberuntime/kuberuntime_container.go
index b049540d0d338..a9ab3ef4a3336 100644
--- a/pkg/kubelet/kuberuntime/kuberuntime_container.go
+++ b/pkg/kubelet/kuberuntime/kuberuntime_container.go
@@ -1084,7 +1084,12 @@ func (m *kubeGenericRuntimeManager) computeInitContainerActions(pod *v1.Pod, pod
 
 		switch status.State {
 		case kubecontainer.ContainerStateCreated:
-			// nothing to do but wait for it to start
+			// The main sync loop should have created and started the container
+			// in one step. If the init container is in the 'created' state,
+			// it is likely that the container runtime failed to start it. To
+			// prevent the container from getting stuck in the 'created' state,
+			// restart it.
+			changes.InitContainersToStart = append(changes.InitContainersToStart, i)
 
 		case kubecontainer.ContainerStateRunning:
 			if !types.IsRestartableInitContainer(container) {


Test patch:
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go
index 732a6f7f1c881..949b3421e06c9 100644
--- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go
+++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go
@@ -1170,6 +1170,17 @@ func TestComputePodActions(t *testing.T) {
 				ContainersToStart: []int{1},
 			},
 		},
+		"Restart the container if the container is in created state": {
+			mutatePodFn: func(pod *v1.Pod) { pod.Spec.RestartPolicy = v1.RestartPolicyNever },
+			mutateStatusFn: func(status *kubecontainer.PodStatus) {
+				status.ContainerStatuses[1].State = kubecontainer.ContainerStateCreated
+			},
+			actions: podActions{
+				SandboxID:         baseStatus.SandboxStatuses[0].Id,
+				ContainersToKill:  map[kubecontainer.ContainerID]containerToKillInfo{},
+				ContainersToStart: []int{1},
+			},
+		},
 	} {
 		pod, status := makeBasePodAndStatus()
 		if test.mutatePodFn != nil {
@@ -1527,12 +1538,17 @@ func TestComputePodActionsWithRestartableInitContainers(t *testing.T) {
 				ContainersToKill:      getKillMapWithInitContainers(basePod, baseStatus, []int{}),
 			},
 		},
-		"initialization in progress; do nothing": {
+		"an init container is stuck in the created state; restart it": {
 			mutatePodFn: func(pod *v1.Pod) { pod.Spec.RestartPolicy = v1.RestartPolicyAlways },
 			mutateStatusFn: func(pod *v1.Pod, status *kubecontainer.PodStatus) {
 				status.ContainerStatuses[2].State = kubecontainer.ContainerStateCreated
 			},
-			actions: noAction,
+			actions: podActions{
+				SandboxID:             baseStatus.SandboxStatuses[0].Id,
+				InitContainersToStart: []int{2},
+				ContainersToStart:     []int{},
+				ContainersToKill:      getKillMapWithInitContainers(basePod, baseStatus, []int{}),
+			},
 		},
 		"restartable init container has started; start the next": {
 			mutatePodFn: func(pod *v1.Pod) { pod.Spec.RestartPolicy = v1.RestartPolicyAlways },