Repository: kubernetes/kubernetes (language: go)

Issue:
Static pods with restartPolicy: OnFailure remain pending if kubelet restarts after container create but before start.
### What happened?

Given a static pod with `restartPolicy: OnFailure`, and  a Kubelet crash while re-creating a failed container in the static pod, the pod may remain pending indefinitely.

1. Run a cluster with the patch at https://github.com/hoskeri/kubernetes/commit/60b103a1df145a688a72ebe1473deddec57109e0. The patch causes kubelet to crash when attempting to start a container that's been created after a failure.
2. Start a static pod with `restartPolicy: OnFailure`.

```yaml
apiVersion: v1
kind: Pod
metadata:
  name: staticpod-failure-tester
spec: 
  restartPolicy: OnFailure
  hostNetwork: true
  initContainers:
  - name: staticpod-failure-init-1
    image: "debian:unstable"
    command:
    - echo 
    - "staticpod-failure-tester-init"
  containers:
  - name: staticpod-failure-tester-main
    image: "redis:latest"
    command:
    - "redis-server"
```


4. Arm the crash to occur on the next StartContainer of our container.

echo -n staticpod-failure-tester-main > /run/crash-container-names

6. Kill the shim process and `crictl stopp $pod`. 

(Careful, modify the command to be more specific if other containers are running in your env)

`kill -9 $SHIM_PID; crictl pods -q|xargs crictl stopp`

7. Kubelet should create a replacement container for the killed container in the same sandbox.

9. Kubelet crashes and restarts on StartContainer.

10. After the first crash, kubelet will not trigger the crash again, because it will not attempt a StartContainer again. 
Instead, `computePodActions` wants to stop the existing sandbox, but this does not converge because containerd ignores the `StopContainer` request. 

```
info msg="Container to stop \"469b9ca1059f90783892692b7ecf22b967d015a390446b44ee29894026b7e46a\" must be in running or unknown state, current state \"CONTAINER_CREATED\""
```

```
kuberuntime_manager.go:1058] "computePodActions got for pod" podActions="KillPod: true, CreateSandbox: false, UpdatePodResources: false, Attempt: 3, InitContainersToStart: [], ContainersToStart: [], EphemeralContainersToStart: [],ContainersToUpdate: map[], ContainersToKill: map[]" pod="default/staticpod-failure-tester-i-ea7ab8cd-6939-4748-b544-21d797194f5e"
```

11. The pod remains `Pending` indefinitely.

The real-world version of the above sequence involves a reboot of control plane host vm while Kubelet is still recovering pods from a previous crash - so two reboots in quick succession. 

Based on my reading of the code, it seems to me that the core issue is in the `containerSucceeded` function treats an uninitialized container as having succeeded. We incorrectly read the golang zero value of `containerStatus.ExitCode` as a successful exit code of `0` from the container. 

https://github.com/kubernetes/kubernetes/blob/8f80e0146726c42edefdfaeda6123872a5ec0981/pkg/kubelet/kuberuntime/kuberuntime_manager.go#L535

This is not an issue if the restartPolicy is `Always` because https://github.com/kubernetes/kubernetes/blob/8f80e0146726c42edefdfaeda6123872a5ec0981/pkg/kubelet/kuberuntime/kuberuntime_manager.go#L851 is not reached in that case.



### What did you expect to happen?

The static pod should either end at a terminal phase, or restart the successfully.

### How can we reproduce it (as minimally and precisely as possible)?

See reproduction steps in the main report. 

Here's a strawman change that fixes the issue for my test case. 

https://github.com/hoskeri/kubernetes/commit/5270c2916f78b78aab6845de94cd4d06b271ecd3

### Anything else we need to know?

_No response_

### Kubernetes version

<details>

```console
$ kubectl version
Client Version: v1.30.0-alpha.3.320+19df52c41a25ce
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
```

</details>


### Cloud provider

<details>
NA
</details>


### OS version

<details>

```console
# On Linux:
$ cat /etc/os-release

PRETTY_NAME="Debian GNU/Linux trixie/sid"
NAME="Debian GNU/Linux"
VERSION_CODENAME=trixie
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

$ uname -a
Linux i-ea7ab8cd-6939-4748-b544-21d797194f5e 6.6.15-cloud-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.6.15-2 (2024-02-04) x86_64 GNU/Linux
```
</details>


### Install tools

<details>

</details>


### Container runtime (CRI) and version (if applicable)

<details>
```
Type:          io.containerd.grpc.v1
ID:            cri
Requires:     
               io.containerd.event.v1
               io.containerd.service.v1
Platforms:     linux/amd64
Exports:      
               CRIVersionAlpha      v1alpha2
               CRIVersion           v1
```
</details>


### Related plugins (CNI, CSI, ...) and versions (if applicable)

<details>
NA
</details>



Discussion / hints:
/sig node
Out of curiosity, what is the use case for running a static pod with a restart policy other than `Always`? It seems like anything other than always would be somewhat arbitrary, since the state would be tracked unreliably?
> After the first crash, kubelet will not trigger the crash again, because it will not attempt a StartContainer again.
Instead, computePodActions wants to stop the existing sandbox, but this does not converge because containerd ignores the StopContainer request.

@hoskeri it seems like there are two issues:
 1. kubelet tries to kill the container because it thinks that the container is in a running state. Did you see containerd/cri reporting the container as running? If so, there might be a containerd issue.
 2. kubelet not starting the a new replacement container because it considered the pod succeeded. 

Did you verify that with restart policy "always", the problem wouldn't occur (meaning (1) wouldn't block (2))?

> Out of curiosity, what is the use case for running a static pod with a restart policy other than Always? It seems like anything other than always would be somewhat arbitrary, since the state would be tracked unreliably?

+1 to this question too.
> Out of curiosity, what is the use case for running a static pod with a restart policy other than `Always`? It seems like anything other than always would be somewhat arbitrary, since the state would be tracked unreliably?

I agree, we are switching the restartPolicy to Always as the fix for this issue in our application. The earlier setting may have been an oversight, or a misunderstanding of what OnFailure does. 

However, I think this is still a bug, or atleast a footgun that needs to be documented. 

- restartPolicy: OnFailure is definitely applicable to jobs/cronjobs, for example. 
- I think the same issue applies to initContainers? I have not yet confirmed if that's the case. 
- 
> > After the first crash, kubelet will not trigger the crash again, because it will not attempt a StartContainer again.
> > Instead, computePodActions wants to stop the existing sandbox, but this does not converge because containerd ignores the StopContainer request.
> 
> @hoskeri it seems like there are two issues:
> 
> 1. kubelet tries to kill the container because it thinks that the container is in a running state. Did you see containerd/cri reporting the container as running? If so, there might be a containerd issue.

No, it did not, the container was not started after create, and the state as seen by kubelet remained at 'created'. 

> 2. kubelet not starting the a new replacement container because it considered the pod succeeded.
> 
> Did you verify that with restart policy "always", the problem wouldn't occur (meaning (1) wouldn't block (2))?

I have not verified this, but I think restartPolicy: Always works because the 'created' container is started, and then killPod can succeed in killing the now running container. 
 
> > Out of curiosity, what is the use case for running a static pod with a restart policy other than Always? It seems like anything other than always would be somewhat arbitrary, since the state would be tracked unreliably?
> 
> +1 to this question too.

We are switching to Always for our application, but there might be other cases (see my other comment) where this might still be a problem. 


> No, it did not, the container was not started after create, and the state as seen by kubelet remained at 'created'.

In this case, kubelet shouldn't even try to stop the container. I was confused as why kubelet repeatedly trying to do that (not just a race condition during a specific window).

> I have not verified this, but I think restartPolicy: Always works because the 'created' container is started, and then killPod can succeed in killing the now running container.

I didn't trace the code, but it's also possible that kubelet wouldn't get past the "killing container" phase if containerd returns an error on stopping the container. Then it'd never actually reach the creating/starting containers phase. 

It sounds like there are some legit issues here that should be fixed, but I wonder if we should also stop supporting non-`Always` restart policies on static pods. We would probably want to just overwrite the restart policy on creation rather than restrict it through validation, for backwards compatibility.
Setting  needs-information since we need to understand the use-case better.

/triage needs-information
@AnishShah: The label(s) `triage/needs-information.` cannot be applied, because the repository doesn't have them.

<details>

In response to [this](https://github.com/kubernetes/kubernetes/issues/123839#issuecomment-1989572818):

>Setting  needs-information since we need to understand the use-case better.
>
>/triage needs-information.


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
/triage needs-information
> > No, it did not, the container was not started after create, and the state as seen by kubelet remained at 'created'.
> 
> In this case, kubelet shouldn't even try to stop the container. I was confused as why kubelet repeatedly trying to do that (not just a race condition during a specific window).
> 

Kubelet is trying to stop the container because `KillPod: true`. 

```
kuberuntime_manager.go:1058] "computePodActions got for pod" podActions="KillPod: true, CreateSandbox: false, UpdatePodResources: false, Attempt: 3, InitContainersToStart: [], ContainersToStart: [], EphemeralContainersToStart: [],ContainersToUpdate: map[], ContainersToKill: map[]" pod="default/staticpod-failure-tester-i-ea7ab8cd-6939-4748-b544-21d797194f5e"
```

> > I have not verified this, but I think restartPolicy: Always works because the 'created' container is started, and then killPod can succeed in killing the now running container.
> 
> I didn't trace the code, but it's also possible that kubelet wouldn't get past the "killing container" phase if containerd returns an error on stopping the container. Then it'd never actually reach the creating/starting containers phase.

containerd does not return an error. The 'CONTAINER_STARTED' message is an informative message. No error is returned. https://github.com/containerd/containerd/blob/f8fbdfdd6f191bf93321501170d3f85675890601/internal/cri/server/container_stop.go#L82



Hi @AnishShah, Could you please clarify what more-information is needed? 

- more evidence that a bug exists? 
- more details on the precise root cause? 
- anything else?
@hoskeri , during sig-node CI bug triaging, we discussed this issue and wanted to understand whether we have a use-case to support `restartPolicy: OnFailure` in Static pods.
The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues 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 issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues 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 issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle rotten`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle rotten

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/
I think all static pods should be restart: Always, so I'm not sure this is a case we currently want to support. We even are in the process of removing the `runOnce` kubelet mode, which allows them to be run to completion. What use case for onFailure do you have?
/cc

Fix patch:
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager.go b/pkg/kubelet/kuberuntime/kuberuntime_manager.go
index 2c7a8af44f404..7808a331fd90a 100644
--- a/pkg/kubelet/kuberuntime/kuberuntime_manager.go
+++ b/pkg/kubelet/kuberuntime/kuberuntime_manager.go
@@ -534,10 +534,11 @@ func shouldRestartOnFailure(pod *v1.Pod) bool {
 
 func containerSucceeded(c *v1.Container, podStatus *kubecontainer.PodStatus) bool {
 	cStatus := podStatus.FindContainerStatusByName(c.Name)
-	if cStatus == nil || cStatus.State == kubecontainer.ContainerStateRunning {
+	if cStatus == nil {
 		return false
 	}
-	return cStatus.ExitCode == 0
+	// Container has exited, with an exit code of 0.
+	return cStatus.State == kubecontainer.ContainerStateExited && cStatus.ExitCode == 0
 }
 
 func isInPlacePodVerticalScalingAllowed(pod *v1.Pod) bool {


Test patch:
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go
index 732a6f7f1c881..e39f04da9c6d4 100644
--- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go
+++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go
@@ -994,6 +994,22 @@ func TestComputePodActions(t *testing.T) {
 				ContainersToKill:  getKillMap(basePod, baseStatus, []int{}),
 			},
 		},
+		"restart created but not started containers if RestartPolicy == OnFailure": {
+			mutatePodFn: func(pod *v1.Pod) { pod.Spec.RestartPolicy = v1.RestartPolicyOnFailure },
+			mutateStatusFn: func(status *kubecontainer.PodStatus) {
+				// The first container completed, don't restart it.
+				status.ContainerStatuses[0].State = kubecontainer.ContainerStateExited
+				status.ContainerStatuses[0].ExitCode = 0
+
+				// The second container was created, but never started.
+				status.ContainerStatuses[1].State = kubecontainer.ContainerStateCreated
+			},
+			actions: podActions{
+				SandboxID:         baseStatus.SandboxStatuses[0].Id,
+				ContainersToStart: []int{1},
+				ContainersToKill:  getKillMap(basePod, baseStatus, []int{}),
+			},
+		},
 		"don't restart containers if RestartPolicy == Never": {
 			mutatePodFn: func(pod *v1.Pod) { pod.Spec.RestartPolicy = v1.RestartPolicyNever },
 			mutateStatusFn: func(status *kubecontainer.PodStatus) {