I couldn't figure out how you are planning to use ListPods in container garbage collection. Thinking about a scenario, a given pod with restart policy always has two containers: A and B. A is in crashloop, and per container cap is 5. How are you going to enforce the limit at the higher level?

@dchen1107 Good question! In the rocket world, I think we won't need to garbage collect each container, because it will be taken care by the PID 1 in the pod, so we only need to GC in the level of pods.
But in the docker scenario, I think maybe we can add an ID field in api.Container, so that we can get all A1 A2 A3 in the pod.

@dchen1107 Besides ID, there is another thing we need to store -- hash. Currently I did a hack which stores it on pod.Annotations..

We also usually need to keep around a dead container for its logs and to find out how it ended (idk how Rocket handles that)

That makes sense to enforce per container limit, and I like the idea to push down those logic to runtime library including docker.

But how to enforce per node limit? The node might end up with tons of pods which in Running phase, but a lot of crashing containers. When exceeding per node limit, kubelet have to recycle containers more aggressively without respecting per container limit.

Also I don't think I understand your suggestion on ID field in api.Container? How is it going to help?

Wait, I think I can just use the container statuses in the pod?
api.Pod.Status.Info

api.Pod.Status.Info includes per container detail status, but one entry per container name. I plan to extend it to include last terminated information for each container. But still it doesn't have rest terminated containers' information.

I do think we might need a container type in runtime abstraction, but open to other suggestions.

I think define container and pod in the container runtime here can be more clear. And actually there are lot of things in api.Container we don't actually use here.

How bad to have ListContainersInPod(string containerName, pod_uid string, ...) which returns all containers running or dead grouped by a pod?

That's not bad at all. But what could be the type of the returning containers? Since we want the container id (for killing) and hash (for comparison) in syncPod, we will need to store them somewhere.

At least we need to return map[id]api.Container, but we still need the hash, so maybe map[fullName]api.Container. But embedding hash in the name is not a good idea IMO. So I am still thinking about creating Container type in runtime, which can has any fields we want: id, hash...

Any suggestions?

Only thing that worries me with Status in Pod is that for Docker we'll need to inspect every container after we list. This will be a bit expensive. Lets go with this and see how it goes. The abstraction does make it much nicer to use.

@vmarmol Yes, I have thought about that. I think it might make more sense to let each syncPod() routine inspect its containers in the pod. Let me add a TODO to revisit this here, thank you!

nit: Sorry to be a pain, but can we rename this to GetPods()? We seem to be standardizing on Get instead of List with the functions below.

+1