Skip to content

Split OOM event into OOM and OOM Kill #648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 15, 2015
Merged

Conversation

vmarmol
Copy link
Contributor

@vmarmol vmarmol commented Apr 15, 2015

No description provided.

@vmarmol
Copy link
Contributor Author

vmarmol commented Apr 15, 2015

/cc @vishh @rjnagal @kateknister

if err != nil {
glog.Errorf("failed to add OOM event for %q: %v", oomInstance.ContainerName, err)
}
glog.V(2).Infof("Created an OOM event in container %q at %v", oomInstance.ContainerName, oomInstance.TimeOfDeath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V(3) or V(4)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rjnagal
Copy link
Contributor

rjnagal commented Apr 15, 2015

So every OOM creates two events now? It seems a little confusing.

This will allow to watch and differentiate that the OOM victim may not
equal the OOM'd container.
@vishh
Copy link
Contributor

vishh commented Apr 15, 2015

LGTM.

@vmarmol
Copy link
Contributor Author

vmarmol commented Apr 15, 2015

@rjnagal its meant to simplify and allow for watching of OOM kills and OOMs separately.

For example:
Container /a/b watches for OOM kills on itself and its children. Container /a OOMs and kills a process in /a/b/c. Currently, my notification won't fire, with the new interface it will.

It is true that not all OOMs lead to OOM kills (and we don't handle those today, we should).

@rjnagal
Copy link
Contributor

rjnagal commented Apr 15, 2015

LGTM

I understand the rationale, but I worry that people not familiar with OOM working will get confused.
Let's get it in for now as your use-case is legitimate and needs to be covered. The API solution might be a more sophisticated spec when registering for watches.

rjnagal added a commit that referenced this pull request Apr 15, 2015
Split OOM event into OOM and OOM Kill
@rjnagal rjnagal merged commit 25f3124 into google:master Apr 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants