Skip to content

Commit 85f8d9a

Browse files
committed
sidecar: Don't modify the pod phase
One decision to revisit was to see if really wanted to change the pod phase. It was clearly agreed here that we won't: kubernetes#1913 (comment) This commit just removes the section to discuss that (it is already agreed) and updates the KEP to reflect that. Signed-off-by: Rodrigo Campos <[email protected]>
1 parent 9eef7cf commit 85f8d9a

File tree

1 file changed

+1
-47
lines changed

1 file changed

+1
-47
lines changed

keps/sig-node/0753-sidecarcontainers.md

+1-47
Original file line numberDiff line numberDiff line change
@@ -391,9 +391,7 @@ This will be useful in scenarios such as when your sidecar is a proxy so that it
391391

392392
To solve the problem of Jobs that don't complete: When RestartPolicy!=Always if all normal containers have reached a terminal state (Succeeded for restartPolicy=OnFailure, or Succeeded/Failed for restartPolicy=Never), then all sidecar containers will be sent a SIGTERM.
393393

394-
PodPhase will be modified to not include Sidecars in its calculations, this is so that if a sidecar exits in failure it does not mark the pod as `Failed`. It also avoids the scenario in which a Pod has RestartPolicy `OnFailure`, if the containers all successfully complete, when the sidecar gets sent the shut down signal if it exits with a non-zero code the Pod phase would be calculated as `Running` despite all containers having exited permanently.
395-
396-
Sidecars are just normal containers in almost all respects, they have all the same attributes, they are included in pod state, obey pod restart policy etc. The only differences are lifecycle related.
394+
Sidecars are just normal containers in almost all respects, they have all the same attributes, they are included in pod state, obey pod restart policy, don't change pod phase in any way etc. The only differences are in the shutdown and startup of the pod.
397395

398396
### Implementation Details/Notes/Constraints
399397

@@ -1040,50 +1038,6 @@ respectively.
10401038

10411039
Rodrigo will reach out to users to verify, though.
10421040

1043-
#### Revisit if we want to modify the podPhase
1044-
1045-
The current proposal modifies the `podPhase`. The reasoning is this (c&p from
1046-
the proposal):
1047-
1048-
> PodPhase will be modified to not include Sidecars in its calculations, this is so that if a sidecar exits in failure it does not mark the pod as `Failed`. It also avoids the scenario in which a Pod has RestartPolicy `OnFailure`, if the containers all successfully complete, when the sidecar gets sent the shut down signal if it exits with a non-zero code the Pod phase would be calculated as `Running` despite all containers having exited permanently.
1049-
1050-
As noted by @zhan849 in [this review comment][pod-phase-review-comment], those
1051-
changes to the pod phase is a behavioural change regarding [current
1052-
documentation about the pod phase][pod-phase-doc].
1053-
1054-
[pod-phase-review-comment]: https://github.com/kubernetes/kubernetes/pull/80744/files?file-filters%5B%5D=.go#r379928630
1055-
[pod-phase-doc]: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-phase
1056-
1057-
##### Alternative 1: don't change the pod phase
1058-
1059-
We propose to not change the pod phase due to sidecar containers status. This
1060-
simplifies the code (no special behaviour is needed besides the startup or
1061-
shutdown sequences) and properly reflect the current documented phases.
1062-
1063-
Documentation says that during pending phase, not all containers have been
1064-
started. This is true for pods with sidecar containers as well, even when
1065-
sidecars are running and non-sidecar not yet.
1066-
1067-
The running state will also have the same meaning as currently documented: all
1068-
containers in the pod are running.
1069-
1070-
Furthermore, it seems correct to mark the pod as failed if a sidecar container
1071-
failed. For example, if a job has all containers exited successfully except for
1072-
a sidecar container that failed, marking it as failed seems the right call. That
1073-
sidecar container can be a container uploading files when the main container
1074-
finished. Just making sure it finishes to run seems the right thing to do.
1075-
1076-
##### Alternative 2: change the pod phase
1077-
1078-
Keep the current proposal to modify it, making sure everyone on the community is
1079-
on board with this modification.
1080-
1081-
##### Suggestion
1082-
1083-
Go for alternative 1, seems to lead to simpler code, pod phase seems to be
1084-
correct and no behavioural changes are done regarding the current documentation
1085-
that might need to have a special case when sidecars are running.
1086-
10871041
#### No container type standard
10881042

10891043
This proposal adds a field `type` to the pod spec containers array, but the only

0 commit comments

Comments
 (0)