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

docker-container: place build containers in a separate cgroup #782

Merged
merged 2 commits into from
Sep 30, 2021

Conversation

djs55
Copy link
Contributor

@djs55 djs55 commented Sep 29, 2021

Previously build containers created by the docker-container driver were in the default parent cgroup, along with non-build containers. This made it hard to apply resource limits to all the builds on a machine.

This PR adds a --driver-opt cgroup-parent=CGROUP to allow the cgroup to be customised. A default value of /docker/buildx is set.

@thaJeztah
Copy link
Member

I noticed that the buildkit code embedded in dockerd also does some setting up of a parent cgroup (originally added (extracted to a function later) in moby/moby@d52485c);

https://github.com/moby/moby/blob/c4040417b6fe21911dc7ab5e57db27519dd44a6a/cmd/dockerd/daemon.go#L293

	cgroupParent := newCgroupParent(config)

https://github.com/moby/moby/blob/2b70006e3bfa492b8641ff443493983d832955f4/cmd/dockerd/daemon_unix.go#L125

func newCgroupParent(config *config.Config) string {
	cgroupParent := "docker"
	useSystemd := daemon.UsingSystemd(config)
	if useSystemd {
		cgroupParent = "system.slice"
	}
	if config.CgroupParent != "" {
		cgroupParent = config.CgroupParent
	}
	if useSystemd {
		cgroupParent = cgroupParent + ":" + "docker" + ":"
	}
	return cgroupParent
}
  • Are those used for the same purpose? (or is the above only used for containers used during build? (I think it may be only for build-containers, and therefore is set to match dockerd --cgroup-parent option)
  • The code above looks to be taking systemd cgroups into account; does that mean we need to set a different default depending on wether the daemon that runs the container uses systemd or not?

For the last bullet, I'm looking at the daemon code that handles (custom) parent cgroups for containers, which seems to confirm that; https://github.com/moby/moby/blob/306fa44b7ca59282dc8695e6d169c5b25698d0cb/daemon/daemon_unix.go#L709-L714

if hostConfig.CgroupParent != "" && UsingSystemd(daemon.configStore) {
	// CgroupParent for systemd cgroup should be named as "xxx.slice"
	if len(hostConfig.CgroupParent) <= 6 || !strings.HasSuffix(hostConfig.CgroupParent, ".slice") {
		return warnings, fmt.Errorf("cgroup-parent for systemd cgroup should be a valid slice named as \"xxx.slice\"")
	}
}

Wondering how to solve that

  • buildx could inspect the daemon's configuration before creating the container (and use format "A" or "B")
  • some special value (specific for the buildx builder case) to pass, and have the daemon perform the check and pick the right format to use
  • (not sure if possible) some "uniform" format that the client can use, and the daemon to convert it to the right format for the situation (this would benefit "any" container, but A) not sure if possible, and B) not sure if it's useful; setting a custom parent cgroup usually would mean that that cgroup has to be created up front?)

@thaJeztah
Copy link
Member

/cc @AkihiroSuda (perhaps you have some thoughts on the above)

}
if d.netMode != "" {
hc.NetworkMode = container.NetworkMode(d.netMode)
}
if d.cgroupParent != "" {
hc.CgroupParent = d.cgroupParent
Copy link
Collaborator

@AkihiroSuda AkihiroSuda Sep 29, 2021

Choose a reason for hiding this comment

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

Probably, this should be set only when d.DockerAPI.Info().CgroupDriver == “cgroupfs”

This allows the parent cgroup to be customised, which allows resource
limits to be imposed on build containers separately from "user"
containers.

Signed-off-by: David Scott <dave@recoil.org>
This allows resource limits to be applied to all builds on a host.
For example to limit the total amount of CPU used by builds:

https://medium.com/@asishrs/docker-limit-resource-utilization-using-cgroup-parent-72a646651f9d

Signed-off-by: David Scott <dave@recoil.org>
@djs55
Copy link
Contributor Author

djs55 commented Sep 29, 2021

Thanks, I've pushed an update which gates the setting on CgroupDriver == "cgroupfs" so hopefully it doesn't break anything if using systemd. I must admit I'm not very familiar with systemd, I need to spend some time playing with it.

@tonistiigi tonistiigi merged commit 4613697 into docker:master Sep 30, 2021
@djs55 djs55 deleted the cgroup-parent branch November 22, 2021 13:35
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.

4 participants