kubelet: refactor kubecontainer.Pod, remove PodStatus from it.

[yifan-gu]

Related to #12619
Remove the PodStatus in kubecontainer.Pod.

[yifan-gu]

/cc @yujuhong @dchen1107

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

[vishh]

Listing all unit files on every GetPodStatus() call seems very expensive considering that we poll for Pod Status periodically as of now. Are there any alternatives to this approach? Inotify or cache unit files relevant to a pod, etc?

[yifan-gu]

@vishh Thanks for catching, yes, this involes a bunch of calls to stat. I will add a TODO here to optimize later. (e.g. list unit files at the first time when kubelet starts, and record every creation of the unit file names later, we just need the names here.)

[vishh]

Ok. Thanks!

[yifan-gu]

You are welcome :)

[yujuhong]

Docker is in the similar boat as it has to do to docker ps -a to list all containers in order to achieve this. Hopefully adding the runtime pod cache would solve this problem :)

[vishh]

ok to test

[vishh]

@yifan-gu: LGTM except for the one comment.

[k8s-bot]

GCE e2e build/test passed for commit 31e6ec8ab2da50d387a655f62bd0172342dd5156.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[yifan-gu]

@yujuhong Any feedback on the this? Note I am not adding ContainerStatus in kubecontainer.Container here, nor the PodIP in kubecontainer.Pod as AFAICS they are included in the PodStatus. But maybe you have some specific reason to add ContainerStatus in kubecontainer.Container?

[k8s-bot]

GCE e2e build/test passed for commit 69f796f.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[yujuhong]

@yifan-gu, my email setting for github is messed up, so I didn't get the notification. I will look at this asap. Thanks!

[yujuhong]

@yujuhong Any feedback on the this? Note I am not adding ContainerStatus in kubecontainer.Container here, nor the PodIP in kubecontainer.Pod as AFAICS they are included in the PodStatus. But maybe you have some specific reason to add ContainerStatus in kubecontainer.Container?

@yifan-gu, I don't necessary want the ContainerStatus here, but I'd like Pod to include all the information necessary for kubelet to generate api.PodStatus by itself.
That way, if we implement a runtime pod cache, whenever there is a change in the pod/container state, we will inspect the container and update the cache entry. Kubelet can always look at the cache and figure out the api.PodStatus itself, without having to ask individual runtimes.

In this case, I think you are right not to include ContainerStatus directly, but maybe we can think about adding fields like FinishedAt, ExitCode, etc. Another great reason to do this is that we can GC the container after we update the cache so that we don't have to keep so many containers around.

[yujuhong]

Removing PodStatus from Pod is always good. This PR LGTM as it is. We can talk about whether we want to add extra fields later :)
Thanks!

[yifan-gu]

@yujuhong Thanks for the heading up. I am thinking that can we let pleg manage and cache the []podstatus? Maybe we can add things that are missing (podID, podName, podNamespace for PodStatus, and container hash for ContainerStatus) ?

Then we can possibly remove the kubecontainer.Pod and related stuff. (The reason they exist mostly is because previously we wanted to implement the runtime-agnostic interfaces quickly, without bothering changing the api objects)

[yifan-gu]

Anyway, would like to get this merged as my following rkt patches will depend on this :)

[yujuhong]

@yujuhong Thanks for the heading up. I am thinking that can we let pleg manage and cache the []podstatus? Maybe we can add things that are missing (podID, podName, podNamespace for PodStatus, and container hash for ContainerStatus) ?

Then we can possibly remove the kubecontainer.Pod and related stuff. (The reason they exist mostly is because previously we wanted to implement the runtime-agnostic interfaces quickly, without bothering changing the api objects)

I think there are still merits of keeping kubeocntainer.Pod and types alike because the pod cache would be cleaner and it'd be easier to interface with other internal components. api.PodStatus has too many things that doesn't make sense for the underlying runtime. We'll also not be subject to how api.PodStatus is defined in this case :)