Kubelet should not report multiple stats for the same container in the summary endpoint

[yujuhong]

We've observed that sometimes container cgroups may be left behind uncleaned even though the container has terminated. In those cases, cadvisor would continue reporting stats for the terminated container, which then is shown in kubelet's /stats/summary endpoint. Once kubelet starts a new container instance, it'd report both the terminated and the new container stats simultaneously.

"podRef": {
     "name": "foo",
     "namespace": "default",
     "uid": "11ebda1a-24de-11e7-b5f8-42010a8000fe"
    },
    "startTime": "2017-05-01T13:40:22Z",
    "containers": [
     {
      "name": "bar",
      "startTime": "2017-05-01T13:40:25Z",
      "cpu": {
       "time": "2017-05-17T11:14:08Z",
       "usageNanoCores": 158370,
       "usageCoreNanoSeconds": 192768296377
      },
     ...
     },
     {
      "name": "bar",
      "startTime": "2017-04-19T08:56:26Z",
      "cpu": {
       "time": "2017-05-17T11:14:12Z",
       "usageNanoCores": 0,
       "usageCoreNanoSeconds": 145043746869
      },
      ...
     },

The duplicated entries for the same container may confuse the monitoring components and cause loss of stats.

kubelet should do the due diligence and filters out the entries for the dead containers. These entries can be (mostly) identified by looking at the startTime (earlier than the new entry) and the usage (zero usage). Alternatively, kubelet can check the existence of the container by asking the container runtime, and filters out entries accordingly. However, due to the gap between the stats collection time and the existence check time, this may filter out more stats than desired. I propose we use the simpler, heuristics to remove the extra stats.

/cc @dchen1107 @timstclair @piosz
/cc @kubernetes/sig-node-bugs

[piosz]

How about the approach where in case of duplicated containers names we leave only the one with the newest startTime?

[yujuhong]

How about the approach where in case of duplicated containers names we leave only the one with the newest startTime?

That's what I suggested -- using some heuristics to filter.

[piosz]

I just wanted to say that IMO very simple heuristic is enough.

[yujuhong]

I just wanted to say that IMO very simple heuristic is enough.

What if kubelet actually creates two containers by mistake? Shouldn't we expose their stats to raise attention? I think it's worth being more conservative and only filters out the containers we think are terminated.

[yguo0905]

/assign

[loburm]

I have added small heuristic to the heapster, that will not allow him to report metrics multiple times for the same container.

@yguo0905 Should we close this issue, or you want also to change behavior of kubelet?

[yujuhong]

@yguo0905 Should we close this issue, or you want also to change behavior of kubelet?

I think it's still a good idea to add the safety in kubelet. @yguo0905 already has a PR #48739 up. I haven't had time to review yet though.

[yujuhong]

Fixed by #48739