Skip to content

A more reliable way to get the container id.#1919

Merged
asottile merged 1 commit intopre-commit:masterfrom
adarnimrod:fix-container-id
Jul 3, 2021
Merged

A more reliable way to get the container id.#1919
asottile merged 1 commit intopre-commit:masterfrom
adarnimrod:fix-container-id

Conversation

@adarnimrod
Copy link
Copy Markdown
Contributor

@adarnimrod adarnimrod commented May 22, 2021

The hostname is not always the container id. Get the container id from
/proc/1/cgroup. Fixes #1918.

@asottile
Copy link
Copy Markdown
Member

asottile commented May 22, 2021

what happens if the container is configured with cgroup options? does that affect this value

also tests need to be fixed

@adarnimrod
Copy link
Copy Markdown
Contributor Author

adarnimrod commented May 22, 2021

If I understand your question correctly, it doesn't matter if the container is created with or without setting any resource limits, the control group is created anyway. From my experience, this is the most reliable way to get the container id from inside the container itself. I'll see what needs to be fixed in the tests. Thanks for the quick reply.

@asottile
Copy link
Copy Markdown
Member

asottile commented May 22, 2021

I meant more https://docs.docker.com/engine/reference/run/#specify-custom-cgroups -- does this not affect the contents of /proc/1/cgroup ?

@adarnimrod
Copy link
Copy Markdown
Contributor Author

adarnimrod commented May 27, 2021

I did the following test:

$ docker run --rm --cgroup-parent foo alpine:3.13 cat /proc/1/cgroup
12:freezer:/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
11:perf_event:/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
10:blkio:/system.slice/containerd.service/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
9:hugetlb:/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
8:pids:/system.slice/containerd.service/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
7:cpu,cpuacct:/system.slice/containerd.service/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
6:cpuset:/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
5:memory:/system.slice/containerd.service/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
4:rdma:/
3:devices:/system.slice/containerd.service/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
2:net_cls,net_prio:/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
1:name=systemd:/system.slice/containerd.service/foo/1c7532a506b51e4072102771d0b0cc30e2300f3f539cdd6e3d8b3046275bb79f
0::/system.slice/containerd.service

So it seems that the container ID is still there as expected. I'll go over the failing tests in the weekend.

asottile
asottile reviewed May 27, 2021
View reviewed changes
@adarnimrod adarnimrod force-pushed the fix-container-id branch 3 times, most recently from 908e38a to 1de2410 Compare May 27, 2021 17:05
@adarnimrod
Copy link
Copy Markdown
Contributor Author

adarnimrod commented May 27, 2021

@asottile I think I addressed everything and tests are now passing.

@adarnimrod adarnimrod requested a review from asottile May 27, 2021 17:36
@asottile
Copy link
Copy Markdown
Member

asottile commented May 28, 2021

according to this SO post the method you're using is not consistent on all kernel versions: https://stackoverflow.com/q/20995351/812183

@adarnimrod
Copy link
Copy Markdown
Contributor Author

adarnimrod commented May 28, 2021

You're going to make this on me, are you? 😜
I'm guessing you're referring to this specific answer https://stackoverflow.com/a/52081984 .
The above test I ran locally using Docker 20.10 and kernel version 5.4.
Here's the content of /proc/1/cgroup in Ubuntu Trusty (Docker 1.6 and kernel version 3.13):

11:hugetlb:/
10:perf_event:/docker/3eae25f6afca4ec0df8b9288c3658fde47a31dfe88c0a02474cda89d166e307d
9:blkio:/docker/3eae25f6afca4ec0df8b9288c3658fde47a31dfe88c0a02474cda89d166e307d
8:freezer:/docker/3eae25f6afca4ec0df8b9288c3658fde47a31dfe88c0a02474cda89d166e307d
7:devices:/docker/3eae25f6afca4ec0df8b9288c3658fde47a31dfe88c0a02474cda89d166e307d
6:memory:/docker/3eae25f6afca4ec0df8b9288c3658fde47a31dfe88c0a02474cda89d166e307d
5:cpuacct:/docker/3eae25f6afca4ec0df8b9288c3658fde47a31dfe88c0a02474cda89d166e307d
4:cpu:/docker/3eae25f6afca4ec0df8b9288c3658fde47a31dfe88c0a02474cda89d166e307d
3:cpuset:/docker/3eae25f6afca4ec0df8b9288c3658fde47a31dfe88c0a02474cda89d166e307d
2:name=systemd:/

And here's the content in Ubuntu Xenial (Docker 18.09 and kernel version 4.4):

11:perf_event:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
10:cpuset:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
9:freezer:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
8:net_cls,net_prio:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
7:hugetlb:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
6:memory:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
5:devices:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
4:pids:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
3:blkio:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
2:cpu,cpuacct:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b
1:name=systemd:/docker/7f2ee26c7145e4a6ac67fbba7284199ff92e4a4a3aea1eefde5edda7e2bee13b

From these tests I see that the first line doesn't always have the container id, however the line that starts with 6:memory: does. Is using the container id from that specific line an OK solution?

@adarnimrod adarnimrod force-pushed the fix-container-id branch 4 times, most recently from 57ed17d to ba7f9c8 Compare June 2, 2021 20:48
@adarnimrod adarnimrod force-pushed the fix-container-id branch 10 times, most recently from b2efbcc to a8f7e12 Compare June 13, 2021 18:50
@adarnimrod adarnimrod force-pushed the fix-container-id branch 2 times, most recently from a001d1d to 3d80a30 Compare June 23, 2021 18:35
@adarnimrod adarnimrod force-pushed the fix-container-id branch 2 times, most recently from 1161890 to 247373a Compare June 23, 2021 18:40
@adarnimrod
Copy link
Copy Markdown
Contributor Author

adarnimrod commented Jun 23, 2021

@asottile I think it's ready now.

@adarnimrod @asottile
A more reliable way to get the container id.
3e10209 
The hostname is not always the container id. Get the container id from
/proc/1/cgroup. Fixes pre-commit#1918.
@asottile asottile force-pushed the fix-container-id branch from 247373a to 3e10209 Compare July 3, 2021 17:46
asottile
asottile approved these changes Jul 3, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile merged commit edcbf8f into pre-commit:master Jul 3, 2021
@asottile asottile mentioned this pull request Jul 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asottile asottile asottile approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Failure to get the container id

2 participants

@adarnimrod @asottile