OOM Notify refactoring #307
Conversation
cgroups/systemd/notify_linux.go
Outdated
There was a problem hiding this comment.
Do you think we should change the function sigs to take the map[string]string for the actual paths that we setup like RemovePaths and Stats now take?
There was a problem hiding this comment.
Hmm, I don't know, it need only one cgroup as I see.
There was a problem hiding this comment.
Yes, but passing the cgroups like this it has to regenerate the path dynamically instead of using the static path initially used and setup.
|
@crosbymichael I just removed |
cgroups/notify_linux.go
Outdated
There was a problem hiding this comment.
How about exposing an API at libcontainer package level that will take 'state' as an argument instead of exposing this? That way users of this API need not interpret libcontainer 'state'?
There was a problem hiding this comment.
Maybe then as method of State? Why argument?
There was a problem hiding this comment.
+1 for not having state as an argument. But a method of state seems weird
to me. One option is to wait for the new API to get rid of the state
argument.
On Sun, Dec 21, 2014 at 8:51 AM, Alexander Morozov <[email protected]
wrote:
In cgroups/notify_linux.go
#307 (diff)
:
return nil, err- }
- return notifyOnOOM(d)
-}-func notifyOnOOM(d *data) (<-chan struct{}, error) {
- dir, err := d.path("memory")
- if err != nil {
return nil, err- }
+// NotifyOnOOM returns channel on which you can expect event about OOM,
+// if process died without OOM this channel will be closed.
+// dir should be path to memory cgroup
+func NotifyOnOOM(dir string) (<-chan struct{}, error) {Maybe then as method of State? Why argument?
—
Reply to this email directly or view it on GitHub
https://github.com/docker/libcontainer/pull/307/files#r22148698.
There was a problem hiding this comment.
Okay, I'm agree. I'll fix now.
|
@vishh I made changes, no |
notify_linux.go
Outdated
Now there is function NotifyOnOOM in libcontainer package, which receives *libcontainer.State as argument. Signed-off-by: Alexander Morozov <[email protected]>
|
Fixed comment and commit message. |
|
LGTM. @LK4D4 are you working on a fix to retry cgroups deletion? |
|
@vishh Yes, right now. |
|
LGTM |
1 similar comment
|
LGTM |
This commit contains changes for docker: * user.GetGroupFile to user.GetGroupPath docker-archive/libcontainer#301 * Add systemd support for OOM docker-archive/libcontainer#307 * Support for custom namespaces docker-archive/libcontainer#279, docker-archive/libcontainer#312 * Fixes moby#9699 docker-archive/libcontainer#308 Signed-off-by: Alexander Morozov <[email protected]>
5 participants
Now
NotifyOnOOMis incgroupspackage and should be used with path tomemorycgroup, which can be obtained from containerState