CRI: Clearly define the expectation of Summary API and CRI Stats

[Random-Liu]

In Kubernetes 1.8, we added the support to let kubelet get container stats from CRI.

However, it's not clearly defined whether stats should be returned for non-running container.

Today's situation:

1. Cadvisor integration: it removes stats for terminated containers (containers without cpu/memory usage) https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/stats/cadvisor_stats_provider.go#L93,
   and our summary api node e2e test makes the same assumption https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/summary_test.go#L51;
2. CRI stats integration: it shows all container stats got from container runtime. It does include stats for terminated container if container runtime returns terminated container stats.

We should:

1. Clearly define the behavior of Summary API in this case.
2. If stats for terminated containers are not required, we should either update kubelet to filter them out, or define in CRI that CRI stats should not include stats with zero memory/cpu usage;
3. If stats for terminated containers are required, we should update Kubelet to properly consume it.

For now, I'll just skip stats with zero memory or cpu usage in cri-containerd.

@timstclair @yujuhong @yguo0905 @dashpole @feiskyer
/cc @kubernetes/sig-node-api-reviews

[yguo0905]

To be precise, we only exclude the duplicated terminated container in the cadvisor case.

kubernetes/pkg/kubelet/stats/cadvisor_stats_provider.go

Lines 198 to 199 in 2eadc73

	// A ContainerInfo is considered to be of a terminated container if it has an
	// older CreationTime and zero CPU instantaneous and memory RSS usage.

[Random-Liu]

@yguo0905 If it only excludes the duplicated terminated container, I feel like this logic should be in kubelet instead of container runtime.

Container runtime should just return stats for all containers.

[yguo0905]

Yeah, this logic was added only for fixing the issue #47853, where cadvisor reported metrics of uncleaned container cgroups.

[yujuhong]

I think providing stats for all containers are good. kubelet can filter them out if necessary. cadvisor simply doesn't support that since it only tracks cgroups (for running containers).

@yguo0905 If it only excludes the duplicated terminated container, I feel like this logic should be in kubelet instead of container runtime.

Container runtime should just return stats for all containers.

Excluding duplicated terminated containers is just a workaround for bugs in the existing integration. It was hard to fix for cadvisor+docker because the concept of container in the two components are different.
I don't expect we see duplicated from runtimes integrated via CRI. If we did, it'd be a bug that we need to fix in the specific CRI shim.

[Random-Liu]

@yujuhong Thanks. Then based on the above discussion I'll close containerd/cri#330 and update kubelet instead. :)

Thanks!

[Random-Liu]

I don't expect we see duplicated from runtimes integrated via CRI. If we did, it'd be a bug that we need to fix in the specific CRI shim.

What do you mean "duplicated"? Say we have a container which has been restarted once.
Currently, CRI stats will return stats for both - the newly created one and the terminated one.
Is this duplication in our opinion? /cc @yguo0905