update sidecars kep, changes to TerminationGracePeriod/Pod phase

[Joseph-Irving]

After some discussion on the implementation PR kubernetes/kubernetes#80744 some issues cropped up that need clarifying in the KEP.

Firstly TerminationGracePeriod, I've updated the wording to be more explicit over how exactly it would behave in the current design.
There was a suggestion that perhaps both the Sidecars and Containers could have a separate GracePeriod, e.g. if TerminationGracePeriod: 30s, the Containers would have 30 seconds to Terminate and then the Sidecars would have 30 seconds. So you could end up with 2xGracePeriod. But I'm not sure whether we should go with this or not.

In regards to PodPhase, we originally said not to change this I noticed one particularly odd case which made me think it might need a tweak for this to work.
If your Pod has RestartPolicy: OnFailure, once your non-sidecars have completed, the sidecar will get sent a termination signal, if it exits with a non-zero code the Pod Phase would be calculated as Runnning despite all containers being permanently stopped.
So changing pod phase to ignore sidecars for these scenarios is a potential solution, it would also stop a sidecar that exited badly causing a Pod to be Failed in a RestartPolicy: Never scenario. You could argue that all sidecars should exit cleanly with a zero, but realistically I don't think we can assume this would always be the case.

So I would appreciate some other people's thoughts on which direction we should go with these two things.

/sig apps
/sig node

[k8s-ci-robot]

Hi @Joseph-Irving. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Joseph-Irving]

cc @dchen1107 @derekwaynecarr

[derekwaynecarr]

I have no major concerns with this proposal. I still wish we could properly send termination grace period to CRI providers at container startup time so power down of host can handle those appropriately by integrating with the init system, but that is a different topic.

/lgtm

[dchen1107]

Joseph, thanks for updating KEP and make this much more cleaner. The new proposal makes sense to me.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, derekwaynecarr, Joseph-Irving

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

The pull request process is described here

Needs approval from an approver in each of these files:

• keps/sig-apps/OWNERS [derekwaynecarr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment