Skip to content
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

Add ability to set cgroup parent for all containers #18379

Closed
wants to merge 1 commit into from
Closed

Add ability to set cgroup parent for all containers #18379

wants to merge 1 commit into from

Conversation

LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Dec 2, 2015

Fix #18022
ping @vishh

@LK4D4 LK4D4 added this to the 1.10 milestone Dec 2, 2015
@@ -76,6 +77,7 @@ func (config *Config) InstallFlags(cmd *flag.FlagSet, usageFn func(string) strin
cmd.BoolVar(&config.Bridge.EnableUserlandProxy, []string{"-userland-proxy"}, true, usageFn("Use userland proxy for loopback traffic"))
cmd.BoolVar(&config.EnableCors, []string{"#api-enable-cors", "#-api-enable-cors"}, false, usageFn("Enable CORS headers in the remote API, this is deprecated by --api-cors-header"))
cmd.StringVar(&config.CorsHeaders, []string{"-api-cors-header"}, "", usageFn("Set CORS headers in the remote API"))
cmd.StringVar(&config.CgroupParent, []string{"-cgroup-parent"}, "docker", usageFn("Set parent cgroup for all containers"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not default to /docker to be accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

@vishh
Copy link
Contributor

vishh commented Dec 2, 2015

Just one nit. Otherwise LGTM. Do you want to include a note in the docs on how this flag can be used alongside systemd?

@LK4D4
Copy link
Contributor Author

LK4D4 commented Dec 2, 2015

@vishh Actually people can't use it with systemd until opencontainers/runc#336 :/

@vishh
Copy link
Contributor

vishh commented Dec 3, 2015

Why is that the case? This is a daemon level flag. On systemd machines, if
users set this flag to /system.slice for example, the fs cgroups driver
will create /system.slice/ID cgroups.

On Wed, Dec 2, 2015 at 2:31 PM, Alexander Morozov [email protected]
wrote:

@vishh https://github.com/vishh Actually people can't use it with
systemd until opencontainers/runc#336
opencontainers/runc#336 :/


Reply to this email directly or view it on GitHub
#18379 (comment).

@@ -19,6 +19,7 @@ weight = -1
--api-cors-header="" Set CORS headers in the remote API
-b, --bridge="" Attach containers to a network bridge
--bip="" Specify network bridge IP
--cgroup-parent=docker Set parent cgroup for all containers
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be "/docker"?

@hqhq
Copy link
Contributor

hqhq commented Dec 3, 2015

Maybe we should mention the path is relative to cgroup mount point in docs?

@LK4D4
Copy link
Contributor Author

LK4D4 commented Dec 3, 2015

@vishh Yup, you're right, I'll try tomorrow. I conviniently have systemd machine.
@hqhq I'll add more docs if design will be approved.

@hqhq
Copy link
Contributor

hqhq commented Dec 3, 2015

@vishh I think @LK4D4 means people can't use it with systemd cgroup until opencontainers/runc#336
And I doubt set this flag to /system.slice with fs cgroup would work well on systemd since it can't handle name=systemd.

@LK4D4
Copy link
Contributor Author

LK4D4 commented Dec 3, 2015

@hqhq I have patch for name=systemd which just creates dirs there :) It seems like I can see hierarchy in systemd-cgls, but not sure if it's real.

@hqhq
Copy link
Contributor

hqhq commented Dec 3, 2015

@LK4D4 Great, so we are simulating the work systemd api does in fs cgroup. Have you checked systemd source code to make sure we did all the necessary work?

@hqhq
Copy link
Contributor

hqhq commented Dec 3, 2015

@LK4D4 Do you mean create dirs and join it? I think that'll be enough for name=systemd, but not sure if systemd does other things. Haven't check source code yet.

@LK4D4
Copy link
Contributor Author

LK4D4 commented Dec 3, 2015

@hqhq yeah, I meant that.

@mrunalp
Copy link
Contributor

mrunalp commented Dec 3, 2015

There are now two cases with systemd:

  1. Systems using fs cgroups implementations with systemd (which this is addressing)
  2. Systems using systemd cgroups implementation which still needs systemd: support cgroup parent with specified slice opencontainers/runc#336

@hqhq
Copy link
Contributor

hqhq commented Dec 4, 2015

Hmm, #18395 seems to be a simpler fix, this PR seems not necessary now?

@LK4D4
Copy link
Contributor Author

LK4D4 commented Dec 4, 2015

@hqhq It's actually new feature, not fix.

@hqhq
Copy link
Contributor

hqhq commented Dec 4, 2015

@LK4D4 Right, we can still have this feature, if people want all containers in specific directory and don't want to use --cgroup-parent every time.

@tiborvass
Copy link
Contributor

@LK4D4 needs rebase

@LK4D4
Copy link
Contributor Author

LK4D4 commented Dec 7, 2015

@tiborvass updated
@thaJeztah Would you mind to help me with docs?

@thaJeztah
Copy link
Member

@LK4D4 sure, I'll have a look; might be tomorrow morning (my timezone 😄)

Otherwise cgroup will be created under daemon cgroup (e.g. for `docker` if
daemon is in `daemoncgroup` cgroup: `/sys/fs/cgroup/memory/daemoncgroup/docker`).
By default this option is `/docker`. It can be changed per-container with
`--cgroup-parent`argument for `create` or `run`.
Copy link
Member

Choose a reason for hiding this comment

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

I had this draft, not really satisfied with it, but feel free to use if you think it reads better 😄

# Default cgroup parent

The `--cgroup-parent` option allows you to set the default cgroup parent
to use for containers. If this option is not set, it defaults to `/docker`.

If the cgroup has a leading forward slash (`/`), the cgroup is created
under the root cgroup, otherwise the cgroup is created under the daemon
cgroup.

Assuming the daemon is running in cgroup `daemoncgroup`,
`--cgroup-parent=/foobar` creates a cgroup in
`/sys/fs/cgroup/memory/foobar`, wheras using `--cgroup-parent=foobar`
creates the cgroup in `/sys/fs/cgroup/memory/daemoncgroup/foobar`

This setting can also be set per container, using the `--cgroup-parent`
option on `docker create` and `docker run`, and takes precedence over
the `--cgroup-parent` option on the daemon. 

@LK4D4
Copy link
Contributor Author

LK4D4 commented Dec 15, 2015

@thaJeztah I changed docs. However, it's still in design-review :)

@jessfraz
Copy link
Contributor

design/code LGTM

@cpuguy83
Copy link
Member

LGTM moving to docs review.

@thaJeztah
Copy link
Member

docs LGTM

We need updates to the completion scripts after this has merged, so I'll ping @albers and @sdurrheimer here (thanks guys!)

ping @moxiegirl for review

@albers
Copy link
Member

albers commented Dec 16, 2015

@thaJeztah OK.

@sdurrheimer
Copy link
Contributor

@thaJeztah Fine, I will do it soon.

@thaJeztah
Copy link
Member

thanks @albers and @sdurrheimer!!

@LK4D4
Copy link
Contributor Author

LK4D4 commented Dec 17, 2015

ping @moxiegirl

@@ -643,4 +644,20 @@ set like this:
/usr/local/bin/docker daemon -D -g /var/lib/docker -H unix:// > /var/lib/docker-machine/docker.log 2>&1


# Default cgroup parent
Copy link
Contributor

Choose a reason for hiding this comment

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

@LK4D4 Is this value only valid when the native.cgroupdriver is cgroupsfs? Or does it work if that value is also systemd? I'd change the text depending on your answer.

Regarding the material, it took me a few tries to untangle it. I'm sorry I couldn't test it entirely, but I think this might be better way to phrase the section. Please review.

--->

Default cgroup parent

The cgroups (control group) feature is used to limit a container's access to the
host's resources. The --cgroup-parent option sets the default cgroup parent
for all containers. You can also set this option per container when you call
docker create or docker run commands. The subcommand value overrides the
daemon's --cgroup-parent option setting.

The Docker host stores cgroups in relation to the /sys/fs/cgroup/memory root
directory. Where the cgroup is created in relation to this root directory,
depends on whether and how you specify the --cgroup-parent option value. If
you don't specify the --cgroup-parent option, the daemon uses
/sys/fs/cgroup/memory/docker as the parent.

If you set this option, the option value can start with a leading forward slash (/)
or without one. The presence or absence of a slash determines where the cgroup
is located in the /sys/fs/cgroup/memory directory. If you specify a slash in
the value, for example --cgroup-parent=/foobar, Docker creates a
/sys/fs/cgroup/memory/foobar folder.

If you don't include the slash, Docker creates a
/sys/fs/cgroup/memory/daemoncgroup folder and places your group within it. For
example, if you specify --cgroup-parent=foobar the system creates the cgroup
in the /sys/fs/cgroup/memory/daemoncgroup/foobar directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should work for systemd driver if the argument is a systemd slice.

Sent from my iPhone

On Dec 19, 2015, at 8:21 PM, moxiegirl [email protected] wrote:

In docs/reference/commandline/daemon.md:

@@ -643,4 +644,20 @@ set like this:
/usr/local/bin/docker daemon -D -g /var/lib/docker -H unix:// > /var/lib/docker-machine/docker.log 2>&1

+# Default cgroup parent
@LK4D4 Is this value only valid when the native.cgroupdriver is cgroupsfs? Or does it work if that value is also systemd?


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrunalp can you provide me with an example of specifying the option in this way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like "containers.slice"

Sent from my iPhone

On Dec 19, 2015, at 9:27 PM, moxiegirl [email protected] wrote:

In docs/reference/commandline/daemon.md:

@@ -643,4 +644,20 @@ set like this:
/usr/local/bin/docker daemon -D -g /var/lib/docker -H unix:// > /var/lib/docker-machine/docker.log 2>&1

+# Default cgroup parent
@mrunalp can you provide me with an example of specifying the option in this way?


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

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

@moxiegirl: What you have is a good start. Couple of high level comments:

  1. This option is available both for "docker daemon" and "docker run". We can add help at the daemon level and reference it from the run subcommand (or vice versa).
  2. Given that the behaviour is different depending on native.cgroupdriver choice, it's better to split this section by the same (native.cgroupdriver)

Proceeding to review what you have for cgroupfs:

  • Avoid having references to the cgroup memory subsystem alone. For ex, /sys/fs/cgroup/memory. All cgroup subsystems listed in /proc/subsystems will have the newly created cgroup.
  • Reference to daemoncgroup sounds incorrect. Can you point the source of this info?

For cgroup-parent option on systemd cgroups, you can use the following content:

When cgroups are managed by systemd (using --exec-opt native.cgroupdriver=systemd), containers are created by default under system.slice. Custom systemd slices can be used to group containers and control resource limits on them. Custom slices can be created by adding slice units in /etc/systemd/system. For example, create a limits.slice (ref. http://www.freedesktop.org/software/systemd/man/systemd.resource-control.html)

Run containers in limits.slice using --cgroup-parent:
docker run -it --cgroup-parent=limits.slice busybox sh
docker run -it --cgroup-parent=limits.slice ubuntu bash

Using systemd-cgls, observe that both containers are created under limits.slice. Note that limits.slice is in the same hierarchical level as system.slice.

├─system.slice
│ ├─thermald.service
│ │ └─654 /usr/sbin/thermald --no-daemon --dbus-enable
│ ├─dbus.service
│ │ ├─ 728
...

├─limits.slice
│ ├─docker-d8119d36eea49874ddfa8e8f83c3aa72730b3e57ae6cb43bfd4d299154a24a5c.scope
│ │ └─2330 bash
│ └─docker-d4f053a9bbefb30831ead236ed9716522f3eb2cc50d3443f30ea41f4da766939.scope
│ └─2284 sh

For each container, a docker-containerID.scope is created under /sys/fs/cgroup/subsystem/<cgroup-parent

@moxiegirl
Copy link
Contributor

@anusha-ragunathan Great feedback, as I suspected there was more. My preference is to actually improve the documentation around cgroups and pull all the detail into there --- rather than duplicate it among so many commands. The commands can ref the detail page. I'll come chat to you about that.

@thaJeztah
Copy link
Member

@moxiegirl do you want to update the docs as part of this PR, or do you want to merge this PR, and address the docs in a follow-up? Just checking to see if we can move this PR forward

@LK4D4 unfortunately, this does need a rebase now 😢

@moxiegirl
Copy link
Contributor

@thaJeztah I'll carry the docs on this. We need a redo of the cgroups section in general.

@thaJeztah
Copy link
Member

@moxiegirl thanks!

@LK4D4 can you rebase this so that we can merge?

@thaJeztah
Copy link
Member

Because @LK4D4 is on PTO, I asked @anusha-ragunathan to carry the rebase so that we can merge this ❤️

@thaJeztah
Copy link
Member

@anusha-ragunathan I think this can be closed, because it's carried in #19062?

@anusha-ragunathan
Copy link
Contributor

Yes @thaJeztah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.