fix container_oom_events_total always returns 0.

[chengjoey]

fix #3015
In a Kubernetes pod, if a container is OOM-killed, it will be deleted and a new container will be created. Therefore, the container_oom_events_total metric will always be 0. this pr refactor the collector of oom events, and retain the deleted container oom information for a period of events. And add flag oom_event_retain_time to decide how long the oom event will be keep, default is 5 minutes

[k8s-ci-robot]

Hi @chengjoey. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[chengjoey]

/assign @iwankgb @kragniz

Is it feasible to keep the container metrics with oomkilled without deleting them?
please take a look

[chengjoey]

/kind bug

[iwankgb]

What happens when PID 1 forks another process and the forked process get OOM-killed?

[chengjoey]

What happens when PID 1 forks another process and the forked process get OOM-killed?

The forked process that was OOM-killed can still read relevant log information from /dev/kmsg. It should still be possible to associate with the corresponding container.

[szuecs]

@chengjoey what happens if a container is killed every second?
Do I understand correctly that we keep creating new container metrics or would this counter increase to >1?
In case we would create new container metrics, then I would call this feature a memory leak, because every container start will create a new set of metrics and now we would store them forever as far as I understand. (I am not very familiar with the code)

[ishworgurung]

memory leak [ ... ]

@szuecs In what way would it be a memory leak ?

[szuecs]

@ishworgurung maybe the wording is not correct, but it will increase memory overtime, which is never GCed and finally cadvisor get oom. As far as I understand.
Increasing the counter is great, though. Having old metrics forever is likely an issue.
Maybe it's also not part of this PR, feel free to ignore this.

[chengjoey]

@ishworgurung maybe the wording is not correct, but it will increase memory overtime, which is never GCed and finally cadvisor get oom. As far as I understand. Increasing the counter is great, though. Having old metrics forever is likely an issue. Maybe it's also not part of this PR, feel free to ignore this.

hi @szuecs @ishworgurung , I have made modifications in this PR, putting the oom event metric information in a separate map, and adding the flag oom_event_retain_time to configure the retention time. Oom metric that exceeds this time will still be deleted to prevent memory leaks.

@iwankgb could you please task a review when you have time

[dims]

/ok-to-test

[dims]

@chengjoey please resolve merge conflicts:

[chengjoey]

/test pull-cadvisor-e2e

[chengjoey]

@chengjoey please resolve merge conflicts:

Thanks @dims , pr has been rebased

[chengjoey]

/test pull-cadvisor-e2e

[chengjoey]

/test pull-cadvisor-e2e

[chengjoey]

/test pull-cadvisor-e2e

[k8s-ci-robot]

@chengjoey: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cadvisor-e2e	0b6dfeb	link	true	/test pull-cadvisor-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[nlamirault]

Hi, any news on this bugfix?
We're waiting for an alert on OOMKilled event (kubernetes-monitoring/kubernetes-mixin#822)
Thanks.

[taraspos]

Hi!
just wanted to bump this issue again. Would be great to get it fixed.

[tsipo]

Hi @pschichtel and others.
I was testing this issue on Kubernetes using a small image I have built - see here. I have noticed that when I use that tool in a forked mode - this and this use-cases, I go get container_oom_events_total == 1 as the container continues to live enough time after the OOMKill for cAdvisor to be scraped. If I change the AFTER_FORK_INTERVAL env var to 0, and the container exits immediately, I gon't get container_oom_events_total == 1 as the container is de-registered immediately from cAdvisor.
FYI.

[frittentheke]

@dims @iwankgb @kragniz may I kindly ask for an update on this issue?
Monitoring OOM events seems to be in a quite confusing slightly messy state ...

I was discussing the monitoring aspects of OOM events in Kubernetes with e.g. @dgrisonnet in kubernetes/kubernetes#69676 (comment) and was pointed at kubernetes/kubernetes#108004 which supposedly enabled Kubelet to expose the OOM metrics from Cadvisor. But in the end those need to be available, which, looking at this very issue / PR here, seems to be buggy still?

[chengjoey]

now, the main reason is probably the container OOM. Kubelet will kill the container and create a new one, so the historical OOM events are lost. I think the current PR idea is correct. Maybe there are some things I missing.

[sellers]

now, the main reason is probably the container OOM. Kubelet will kill the container and create a new one, so the historical OOM events are lost. I think the current PR idea is correct. Maybe there are some things I missing.
Do I understand correctly that this whole issue is a race condition; in that prometheus can not scrape the container to get the metric for the OOM event because the said container the provides that metric has OOMed? That would make the merge request offered here have more context to those following this thread. It would also give context to @tsipo 's above test cases.

[frittentheke]

@dims may I kindly ask again if there is any way forward with this PR? Google's Autopilot paper (https://research.google/pubs/autopilot-workload-autoscaling-at-google-scale/) talks at length about how OOM events are used to rightsize application resource requests - so having cadvisor and Kubernetes do their best to provide these metrics and events should be obvious.
Please kindly also see my references in #3278 (comment).

[dgrisonnet]

This is dependent on the existence of the container:

cadvisor/manager/manager.go

Lines 1307 to 1311 in 0b6dfeb

	conts, err := m.getRequestedContainers(oomInstance.ContainerName, request)
	if err != nil {
	klog.V(2).Infof("failed getting container info for %q: %v", oomInstance.ContainerName, err)
	continue
	}

If we want to address the current dependency issue of the metric on the container lifecycle, I think we shouldn't rely on the containers seen by the manager at all and rather just produce metrics based on the info that cadvisor parses via the oom parser.

[pschichtel]

shouldn't it be possible to track the necessary information before containers are killed to have them available independent of the container lifecycle?

[dgrisonnet]

It is possible but that would require to cache all the container data.
For a separate reason, there was an attempt to create a cache in #2974, but I don't know if any of the maintainers would be willing to pull on the trigger on such a big change. At least it wasn't conceivable in the other PR due to the memory implications.
Also Kubernetes is and has been to trying to move away from cadvisor in favor of container-runtime metrics which doesn't go in the favor of such an investment in cadvisor.

[pschichtel]

It is possible but that would require to cache all the container data.

true, but not all container data, just the bare-minimum to be able to produce useful metrics

[dgrisonnet]

rawLabels also depends on the presence of the container. Instead of using the default labels that we can't always rely on with OOM kills, it might be better to define new labels specific to the OOM metrics

[fabiand]

Anything we can do in order to move this forward?

[frittentheke]

There is at least some movement in regards to OOM detection and parsing ... (#3715) @jesseward

[fabiand]

Off-topic @frittentheke your's is one of the best nick's I've seen in a while.

[dims]

needs a rebase please

[ringerc]

Would it be feasible to consume the containerd event stream to get this information and increment the counter?

If I tail crictl events and filter it with

jq '
  .
  |select(.containerEventType=="CONTAINER_STOPPED_EVENT")
  |del(.podSandboxStatus)
  |.containersStatuses|=[
    .[]
    |select(.reason=="OOMKilled")
    |del(.mounts,.user,.imageId,.resources,.logPath,.annotations,.imageRef,.image)
  ]
'

produces events like

{
  "containerId": "dbc4b8a02e5b1bf90229b82500b4b46006e168487870e14f1a7f778a4b2a7cbd",
  "containerEventType": "CONTAINER_STOPPED_EVENT",
  "createdAt": "1767061346879749884",
  "containersStatuses": [
    {
      "id": "ce9e36fedc7c0b5b6b435b267cac397c53d4a7d3df3bb2154cd9de7e905d9bfe",
      "metadata": {
        "name": "oomtest",
        "attempt": 1
      },
      "state": "CONTAINER_EXITED",
      "createdAt": "1767061323866327412",
      "startedAt": "1767061324060497453",
      "finishedAt": "1767061325952232903",
      "exitCode": 137,
      "reason": "OOMKilled",
      "message": "",
      "labels": {
        "io.kubernetes.container.name": "oomtest",
        "io.kubernetes.pod.name": "oomtest-86889bb6d5-tnrqh",
        "io.kubernetes.pod.namespace": "default",
        "io.kubernetes.pod.uid": "dd9e8754-f83a-4eab-b242-cee7138411b2"
      }
    },
    {
      "id": "dbc4b8a02e5b1bf90229b82500b4b46006e168487870e14f1a7f778a4b2a7cbd",
      "metadata": {
        "name": "oomtest",
        "attempt": 2
      },
      "state": "CONTAINER_EXITED",
      "createdAt": "1767061344714647928",
      "startedAt": "1767061344889964019",
      "finishedAt": "1767061346792728130",
      "exitCode": 137,
      "reason": "OOMKilled",
      "message": "",
      "labels": {
        "io.kubernetes.container.name": "oomtest",
        "io.kubernetes.pod.name": "oomtest-86889bb6d5-tnrqh",
        "io.kubernetes.pod.namespace": "default",
        "io.kubernetes.pod.uid": "dd9e8754-f83a-4eab-b242-cee7138411b2"
      }
    }
  ]
}

when run with this test deployment spec #3015 (comment)

This should provide the information containerd needs to maintain a counter, right?

IMO it doesn't actually make sense to dimension this metric with the container's id at all; that makes it difficult to query, as it's necessary to compute the increase of all the counters, then sum them. Instead, this metric's current name should be deprecated, and a new metric defined with dimensions pod, container, omitting the id and name that vary per-container-instance. The event stream has labels for io.kubernetes.pod.name and io.kubernetes.container.name, so that should be sufficient to associate them with a metric owned by the Pod and dimensioned by container-name.

[ahippler]

Hi. Any chance that this will be fixed? I would really like to use the metric.

[frittentheke]

@dims may I kindly ask what the reason for closing this one was?
Is this already addressed elsewhere?

• Expose remaining cgroup v2 memory.events counters (oom, oom_kill, oom_group_kill, low) #3889
• oomparser - new constructor to support streaming of new/current OOM events only. #3716

the state of OOM events are still puzzling to me.