Use the container whose limit is hit for system OOMs

[dashpole]

What type of PR is this?
/kind bug
/sig node
/priority important-soon

What this PR does / why we need it:
cAdvisor populates ContainerName and VictimContainerName from matching the regexp: Task in (.) killed as a result of limit of (.). A SystemOOM should mean VictimContainerName == "/", as we are looking for OOMs that are "killed as a results of limit of /". However, we incorrectly check ContainerName instead in the kubelet.

Which issue(s) this PR fixes:
Fixes #88868

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix detection of SystemOOMs in which the victim is a container.

/assign @sjenning @derekwaynecarr @dchen1107

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/kubelet/oom/OWNERS [dashpole]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dashpole]

/retest

[tedyu]

/lgtm

[dashpole]

/hold
i'll give others a day to make sure this looks correct to them.

[mattjmcnaughton]

/lgtm
I believe this lgtm.

Can you add cAdvisor populates ContainerName and VictimContainerName from matching the regexp: Task in (.*) killed as a result of limit of (.*). A SystemOOM should mean VictimContainerName == "/", as we are looking for OOMs that are "killed as a results of limit of /". However, we incorrectly check ContainerName instead in the kubelet. from the issue into either the commit message or the pr?

Also, if I'm reading the git history correct, does this mean this feature never worked? In other words, would there ever have been a time where the container name is /? If this never worked, does that reflect a lack of e2e testing?

[dashpole]

Also, if I'm reading the git history correct, does this mean this feature never worked? In other words, would there ever have been a time where the container name is /? If this never worked, does that reflect a lack of e2e testing?

Yeah, it never worked. I thought about that as well in light of this issue. The main problem is that e2e testing would be quite flaky. SystemOOMs have been known to cause problems for the host VM, so a test that triggers a real SystemOOM would often fail.

[dashpole]

/retest

[dashpole]

/retest

[mattjmcnaughton]

Also, if I'm reading the git history correct, does this mean this feature never worked? In other words, would there ever have been a time where the container name is /? If this never worked, does that reflect a lack of e2e testing?

Yeah, it never worked. I thought about that as well in light of this issue. The main problem is that e2e testing would be quite flaky. SystemOOMs have been known to cause problems for the host VM, so a test that triggers a real SystemOOM would often fail.

/retest

Yeah... that's a strong point that performing e2e tests with true system OOM events is a tricky game to play haha...

Updating the unit tests and clear comments/commit messages may be the best we can do for now.

[sjenning]

readding
/lgtm

[dashpole]

thanks for taking a look @sjenning
/hold cancel

[KielChan]

I have came up with another problem about oom.
It is java app with tomcat. OOMKiller on system reject tomcat to apply more memory, and kill this tomcat process. Java app container does not restart and no oom event report.
Is it the same as this problem?

[dashpole]

No, this is for SystemOOM events, which you (hopefully) did not hit