Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Support docker cgroup limits #13488

Merged
merged 2 commits into from
Aug 21, 2017
Merged

Support docker cgroup limits #13488

merged 2 commits into from
Aug 21, 2017

Conversation

tmds
Copy link
Member

@tmds tmds commented Aug 19, 2017

  • The parsing would find the wrong '-' in lines like this:
    354 347 0:28 /system.slice/docker-654dd7b6b8bbfe1739ae3309b471e95ccc82b3a3f56b7879f0a811d68b5c4e1d.scope /sys/fs/cgroup/cpuacct,cpu ro,nosuid,nodev,noexec,relatime - cgroup cgroup rw,cpuacct,cpu
  • In a docker container, the mount root and relative path in the cgroup have the same value, the relative path must not be appended to the mount point.

The parsing would find the wrong '-' in lines like this:
354 347 0:28 /system.slice/docker-654dd7b6b8bbfe1739ae3309b471e95ccc82b3a3f56b7879f0a811d68b5c4e1d.scope /sys/fs/cgroup/cpuacct,cpu ro,nosuid,nodev,noexec,relatime - cgroup cgroup rw,cpuacct,cpu
@tmds tmds changed the title Fix cgroup mountinfo parsing Support docker cgroup limits Aug 21, 2017
@tmds
Copy link
Member Author

tmds commented Aug 21, 2017

I've tested this as follows:
main.cpp

#include "/home/tmds/repos/coreclr/src/gc/unix/cgroup.cpp"

int main()
{
	CGroup cgroup;
	unsigned int cpus = 0;
	size_t mem = 0;
	if (cgroup.GetPhysicalMemoryLimit(&mem))
	{
		printf("mem: %zu\n", mem);
	}
	if (cgroup.GetCpuLimit(&cpus))
	{
		printf("cpu: %d\n", cpus);
	}
	return 0;
}
g++ main.cpp

Dockerfile

FROM fedora:25
RUN yum install -y libstdc++.so.6
WORKDIR /root
ADD ./a.out .
CMD ./a.out
$ docker build -t docker_limit .
$ docker run docker_limit
mem: 9223372036854771712
$ docker run --memory 300m docker_limit
mem: 314572800
$ docker run --cpu-period 1000 --cpu-quota 1000  docker_limit
mem: 9223372036854771712
cpu: 1
$ docker run --cpu-period 1000 --cpu-quota 2000  docker_limit
mem: 9223372036854771712
cpu: 2

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much for finding and fixing it!

@janvorli
Copy link
Member

@dotnet-bot test Tizen armel Cross Debug Build please

@janvorli janvorli merged commit fa01b86 into dotnet:master Aug 21, 2017
sandreenko pushed a commit to sandreenko/coreclr that referenced this pull request Aug 21, 2017
* Fix cgroup mountinfo parsing

The parsing would find the wrong '-' in lines like this:
354 347 0:28 /system.slice/docker-654dd7b6b8bbfe1739ae3309b471e95ccc82b3a3f56b7879f0a811d68b5c4e1d.scope /sys/fs/cgroup/cpuacct,cpu ro,nosuid,nodev,noexec,relatime - cgroup cgroup rw,cpuacct,cpu

* cgroup: don't append cgroup relative path for reading docker limits
@tmds
Copy link
Member Author

tmds commented Aug 22, 2017

Thanks @janvorli

tmds added a commit to tmds/coreclr that referenced this pull request Aug 22, 2017
* Fix cgroup mountinfo parsing

The parsing would find the wrong '-' in lines like this:
354 347 0:28 /system.slice/docker-654dd7b6b8bbfe1739ae3309b471e95ccc82b3a3f56b7879f0a811d68b5c4e1d.scope /sys/fs/cgroup/cpuacct,cpu ro,nosuid,nodev,noexec,relatime - cgroup cgroup rw,cpuacct,cpu

* cgroup: don't append cgroup relative path for reading docker limits
@tmds
Copy link
Member Author

tmds commented Aug 22, 2017

@janvorli I'm having a look at what commits could be picked to bring this to 2.0.x.
These can be cherry-picked without merge-conflicts and add support for docker mem&cpu limits:
16baed5, 0d53e14, 5596717
Does that seem like a proper set?
Can I do a PR for these to be included in a 2.0 branch? which branch?

@janvorli
Copy link
Member

@tmds the release/2.0.0 branch is now frozen for the 2.0.1 release, we can back port it after it opens again.

@janvorli
Copy link
Member

@tmds I've ported these three commits to release/2.0.0 branch, so it will be part of the 2.0.2 release.

@tmds
Copy link
Member Author

tmds commented Sep 12, 2017

Thanks @janvorli !

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

Successfully merging this pull request may close these issues.

4 participants