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

Detect the number of CPUs used by Docker (#5464) #5466

Merged
merged 11 commits into from Sep 13, 2019

Conversation

@uilianries
Copy link
Member

commented Jul 9, 2019

Inspect Docker filesystem to detect the real CPU number available for usage.

Is there a test that can I do using CI?

Changelog: Fix: Detect the number of CPUs used by Docker (#5464)
Docs: conan-io/docs#1359
fixes #5464

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

- Inspect Docker filesystem to detect the real CPU
  number available for usage.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Copy link
Member

left a comment

I am concerned about the direct opening of different files here without a try/except clause to fail-safe in the case that those files are not present

conans/client/tools/oss.py Outdated Show resolved Hide resolved
Signed-off-by: Uilian Ries <uilianries@gmail.com>
def is_running_in_docker():
try:
cgroup = "/proc/self/cgroup"
return (os.path.exists('/.dockerenv') or

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 10, 2019

Contributor

I don't think we should explicitly check for docker in that case (there are container systems, like CRI-O / podman, for instance), if there are cgroups and limits in place, we should respect them anyway, not just in docker

This comment has been minimized.

Copy link
@uilianries

uilianries Jul 10, 2019

Author Member

yeah, I need to investigate other container services. I only used docker because it's related to the original issue. I'll take a look in others

This comment has been minimized.

Copy link
@uilianries

uilianries Jul 11, 2019

Author Member

Since we can obtain cpu count by cgroup, I removed docker verification.

with open(os.path.join(base_cpus, "cpu.cfs_period_us"), 'r') as period_fd:
cfs_period_us = int(period_fd.read())
if cfs_quota_us > -1 and cfs_period_us > 0:
return math.ceil(cfs_quota_us / cfs_period_us)

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 10, 2019

Contributor

if cfs_quota_us is zero, we may end up with -j0, which will have an error: option requires a positive integral argument

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 10, 2019

Contributor

also, result of math.ceil is double (like 1.0), which wouldn't be a valid target for make -j

This comment has been minimized.

Copy link
@uilianries

uilianries Jul 10, 2019

Author Member

good catch, but users are able to set --cpus=0.5, which will be promoted to 1 by math.ceil. Also, if someone sets cpus to 0 it's a prone error IMO.

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 10, 2019

Contributor

no, I mean ceil(0.5) = 1.0, not just 1, so it will be -j1.0 which is invalid
so correct thing is int(math.ceil(...))

This comment has been minimized.

Copy link
@uilianries

uilianries Jul 10, 2019

Author Member

Are you sure?

Python 3.7.2 (default, Mar 27 2019, 09:22:51) 
[GCC 8.2.1 20181127] on linux
import math
>>> math.ceil(0.5)
1
>>> math.ceil(0.3)
1
>>> math.ceil(0.4)
1
>>> math.ceil(0.1)
1
>>> math.ceil(0)
0
>>> math.ceil(1)
1
>>> math.ceil(1.0)
1

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 10, 2019

Contributor

yes:

python -c "import math; print(math.ceil(1))"
1.0
python -c "import math; print(math.ceil(1.5))"
2.0

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 10, 2019

Contributor

also from doc:

math.ceil(x)
Return the ceiling of x as a float, the smallest integer value greater than or equal to x.

This comment has been minimized.

Copy link
@uilianries

uilianries Jul 10, 2019

Author Member

now I see, you are running python 2 ... well let's truncate. Thanks for finding it.

Copy link
Contributor

left a comment

I think it deserves a test, you can easily mock open (see osinfo_test for an example)

Signed-off-by: Uilian Ries <uilianries@gmail.com>
conans/client/tools/oss.py Outdated Show resolved Hide resolved
uilianries added 3 commits Jul 10, 2019
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
conans/client/tools/oss.py Outdated Show resolved Hide resolved
@@ -144,6 +144,19 @@ def cpu_count_test(self):
with six.assertRaisesRegex(self, ConanException, "Invalid CONAN_CPU_COUNT value"):
tools.cpu_count(output=output)

def test_cpu_count_in_container(self):

This comment has been minimized.

Copy link
@uilianries

uilianries Jul 11, 2019

Author Member

This test is not working. The idea here is patching tools.load to return a specific value. Any ideas how to fix it?

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 12, 2019

Contributor

maybe because load has two args?

This comment has been minimized.

Copy link
@uilianries

uilianries Jul 12, 2019

Author Member

no, I think it's related to path used in patch, but I can't figure out.

This comment has been minimized.

Copy link
@uilianries

uilianries Jul 18, 2019

Author Member

It's working now. I just exported helper functions to a new class.

uilianries added 2 commits Jul 17, 2019
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@lasote lasote added this to the 1.19 milestone Jul 26, 2019
@sarikakaran sarikakaran moved this from Triaging to In Review in Milestone 1.19 Release Jul 30, 2019
@lasote
lasote approved these changes Sep 13, 2019
conans/client/tools/oss.py Outdated Show resolved Hide resolved
conans/client/tools/oss.py Outdated Show resolved Hide resolved
@lasote lasote self-assigned this Sep 13, 2019
uilianries and others added 2 commits Sep 13, 2019
Co-Authored-By: Luis Martinez de Bartolome Izquierdo <lasote@gmail.com>
Co-Authored-By: Luis Martinez de Bartolome Izquierdo <lasote@gmail.com>
@lasote lasote merged commit 982a970 into conan-io:develop Sep 13, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
Milestone 1.19 Release automation moved this from In Review to Done Sep 13, 2019
Copy link
Member

left a comment

This is not important, the functionality is good! :)
Just wanted to contribute my 2 cents to some SW engineering details. They do NOT need to be fixed now, but just to share my views on this simple example.

def cpu_count(output=None):
output = default_output(output, 'conans.client.tools.oss.cpu_count')

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 13, 2019

Member

Why removing this output? This would make the output.warn() below to crash if output=None (otherwise, remove the default and check with an assert for valid output object)

@@ -24,14 +24,35 @@ def args_to_string(args):
return " ".join("'" + arg.replace("'", r"'\''") + "'" for arg in args)


class CpuProperties(object):

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 13, 2019

Member
  • Do not use bare exceptions, they capture Ctrl+C and other interrupts too

This is a bit too much Object Oriented for me without a real goal:

  • All methods should be static (and are not, this is an indicator)
  • It is mainly designed for testing with mocks, which I get, but you can get the same with one function:
 def get_cpus(quota_file=None, period_file=None):
        try:
            cfs_quota_us = int(load(quota_file or "/sys/fs/cgroup/cpu/cpu.cfs_quota_us"))
            cfs_period_us = int(load(period_file or "/sys/fs/cgroup/cpu/cpu.cfs_period_us"))
            if cfs_quota_us > 0 and cfs_period_us > 0:
                return int(math.ceil(cfs_quota_us / cfs_period_us))
        except Exception:
            pass
        return multiprocessing.cpu_count()

Now it is easy to test too, and also testing is more real, it test also the load of file + conversion to integer

@lasote lasote removed their assignment Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.