KEP-3085: Add KEP for Pod network ready condition

[ddebroy]

• One-line PR description:
  KEP to surface PodHasNetwork condition in pods to indicate when pod sandbox creation and network configuration of pod through CRI runtime is complete.

• Issue link: Pod conditions around readiness to start containers after completion of pod sandbox creation  #3085

• Other comments:

[marosset]

I think this would be useful since it can be hard to determine why pods are not running if there are errors during CreatePodSandbox - kubernetes/kubernetes#104635

[ehashman]

/assign

[jsturtevant]

I think this would be useful since it can be hard to determine why pods are not running if there are errors during CreatePodSandbox - kubernetes/kubernetes#104635

some more info here: kubernetes/kubernetes#105984

[fujitatomoya]

it sounds reasonable and useful, a few minor nitpicks though.

[ddebroy]

/assign @derekwaynecarr
(based on sig-node discussion)

[sftim]

Some flavors of PodHasNetwork might need more values than just true and false. We could define the condition to be true if-and-only-if the networking is fully up, or we could use an unknown condition (:grimacing:) to cover cases such as “IPv6 is set up already but IPv4 still coming up”.

We might also want to define how to represent this condition if using a CNIs that implements NetworkPolicy. That might be better left to a separate condition, though, to avoid holding up this enhancement?

[ddebroy]

Some flavors of PodHasNetwork might need more values than just true and false. We could define the condition to be true if-and-only-if the networking is fully up, or we could use an unknown condition (😬) to cover cases such as “IPv6 is set up already but IPv4 still coming up”.

Setting up of the network concludes the pod "sandbox" creation process coordinated between Kubelet => CRI runtime => CNI plugins. Thus, in the context of this KEP, the new PodHasNetwork condition is essentially surfacing the state of the pod "sandbox" (whether the Kubelet => CRI runtime => CNI plugin invocation chain made it successfully) while allowing for other similar conditions in the future (like PodHasVolumes indicating all Kubelet => in-tree/csi plugin invocations have successfully configured and mounted the volumes) if needed.

If we want more granular network-related conditions (which is not within scope of the KEP), individual CNI plugins (with a API server client) can mark any necessary condition on the pod using extended conditions - for example, cni.kubernetes.io/pod-has-ipv4 and cni.kubernetes.io/pod-has-ipv6.

Note that until all CNI plugins in the configured chain do succeed, CRI runtime won't return an "in-process"/intermediate outcome to Kubelet. Either all networking configuration and IP allocation succeeded (i.e. networking is fully up for the pod) OR network configuration failed and the entire CRI sandbox creation + network configuration has to be retried.

[ddebroy]

@derekwaynecarr I have updated the KEP with the suggested PodHasNetwork condition. PTAL when you get a chance.

[sftim]

/retitle KEP-3085: Add KEP for Pod network ready condition

Hope that's OK.

[sftim]

@ddebroy consider omitting “draft” from the PR description at this point.

[johnbelamaric]

PRR looks fine, I will wait until there is SIG approval though because I am root in this repo

[dchen1107]

Thanks for pointing out the mismatching phase interprets on Initialized between pods with init containers and without. But the issue isn't obvious to the users since we hide the pod sandbox creation completely in the implementation. To the end user, it is more like all init containers are successfully run, now it is the time to start the app containers. With the newly proposed PodCondition: PodHasNetwork, shouldn't the issue surface to the end users now?

With init containers: PodHasNetwork -> Initialized -> ... -> Ready
Without init containers: Initialized -> PodHasNetwork -> ... -> Ready

Can we consolidate above flows? We can move Initialized after PodHasNetwork to indicate Kubelet now can start any app containers.

[ddebroy]

But the issue isn't obvious to the users since we hide the pod sandbox creation completely in the implementation. To the end user, it is more like all init containers are successfully run, now it is the time to start the app containers. With the newly proposed PodCondition: PodHasNetwork, shouldn't the issue surface to the end users now?

The inconsistency already surfaces with the way things are today when CSI/CRI/CNI is unable to mount volumes or launch the pod sandbox upon encountering errors: without init containers, today, the pod status reports the Initialized condition to be true but Kubelet also reports error events like FailedCreatePodSandBox, FailedMount, etc against the pod (which are reported as part of kubectl describe etc)

Can we consolidate above flows? We can move Initialized after PodHasNetwork to indicate Kubelet now can start any app containers.

We can certainly consider that. I will add that to the KEP post-merge of this PR.

[ddebroy]

We had a discussion on this in sig-node on July 5th. The suggestion was to carry on with the PodHasNetwork condition as described in this KEP as the ordering of transition of pod conditions should not be important. We can separately address the situation with Initialized condition for pods without init containers (mentioned above) in a separate KEP as that feels like an independent problem.

[dchen1107]

Please see my above comment.

[dchen1107]

Add some more comments, but overall KEP is well written and lgtm. To unblock the progress for such useful feature, I will approve this to meet the deadline.

/lgtm
/approve

[johnbelamaric]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, ddebroy, ehashman, johnbelamaric, qiutongs

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/prod-readiness/OWNERS [johnbelamaric]
• keps/sig-node/OWNERS [dchen1107,johnbelamaric]

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

[ddebroy]

/unhold

[wojtek-t]

@ddebroy - Nice features - I just added two minor comments. If you could adjust them in the follow up PR it would be great.

[wojtek-t]

I don't fully agree with this - by default each pod will get a new condition set.
So if someone is doing equality comparisons, they will start seeing changes.

[wojtek-t]

It may potentially increase pod-startup-time (which includes reporting the state) if kubelet is starved on QPS limits.

[This is fine - I'm just pointing this out for completeness...]

[ddebroy]

One thing to note is that the Kubelet Status Manager caches all status updates and queues and sends them to API server in an async fashion. So the updates to the pod conditions (existing ones as well as the new one added in this KEP) do not happen synchronously with actual pod creation and initialization activities. Given this, do you think the above may still be a concern @wojtek-t ?

[wojtek-t]

Yeah - I knew that. But still - if the conditions are not changing fast enough and individual phases take a bit of time, we may need to do more API calls. And in that case, it may increase pod startup if kubelet is cpu-starved.

As I mentioned - I don't treat it as a concern or anything blocking - I just wanted it to be added for completeness.