-
Notifications
You must be signed in to change notification settings - Fork 315
Factor out setting of allowed devices so that we can update allowed devi... #7
Conversation
cgroups/systemd/apply_systemd.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: createAndEnterCgroup()? Then call on the above function:
createAndEnterCgroup(...)
UpdateAllowedDevices(...)
LGTM |
Overall this LGTM. This will be taken over by an Update() in the libcontainer API. For now we should be okay. Waiting on @crosbymichael to OK as well. |
Just to add some context of why we want to add a device at runtime... |
Ping @rjnagal @crosbymichael |
testing now |
This is also useful for subuser.org. While |
* This commit is dependent on several pull requests being merged into docker and libcontainer that expose the devadd and devrm API. moby/moby#6369 docker-archive/libcontainer#6 docker-archive/libcontainer#7 Change-Id: I6dcd7d809027bfe6ddcd9d521e519656a1ad526a
Ping @rjnagal @crosbymichael |
This needs to support non systemd but it maybe better to wait on #143 so that you can easily get the existing cgroup paths for a container. |
…evices Co-Authored-By: Ian Main <[email protected]> Docker-DCO-1.1-Signed-off-by: Chris Alfonso <[email protected]> (github: calfonso)
@crosbymichael I think that the cgroup API implementation for non systemd should be part of a different patch and PR, this one is intended just to allow updating the devices on the already implemented systemd based managed cgroups impl. Thoughts? |
@calfonso is "UpdateAllowedDevices" systemd specific at all? It seems to me, that all it's doing is writting to the cgroups devices.allow file. The only difference in the implementation between systemd and fs should be the path. |
@timthelion Correct. There isn't anything specific to systemd in the update func, the only thing that is specific to systemd is the location of this implementation. I'm not completely sure about the details of the original implementation, I know the freezer subsystem isn't implemented at the kernel level to work on non-systems - not sure if that was the original impitus to split them out or not. |
I believe this is no longer needed now that joinDevices has been added upstream. We do need to capitalize the joinDevices method to make it accessable to docker though. Stay tuned fo that patch. Closing this PR. |
...ces
Co-Authored-By: Ian Main [email protected]
Docker-DCO-1.1-Signed-off-by: Chris Alfonso [email protected] (github: calfonso)