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 an id for Docker containers #3

Merged
merged 4 commits into from
Feb 1, 2023
Merged

Conversation

juanbretti
Copy link
Contributor

Usual Linux images for Docker containers, does not have the originally listed files. Following the conversation at denisbrodbeck/machineid#10, I am including the following: Because /proc/self/cgroup seams to not be working on same Docker versions, I am also including /proc/self/mountinfo.

Tested on Python 3.11 running on Debian GNU/Linux 11 (bullseye) inside a Docker.

Usual Linux images for Docker containers, does not have the originally listed files.
Following the conversation at denisbrodbeck/machineid#10, I am including the following:
Because `/proc/self/cgroup` seams to not be working on same Docker versions, I am also including `/proc/self/mountinfo`.

Tested on `Python 3.11` running on `Debian GNU/Linux 11 (bullseye)` inside a Docker.
Copy link
Member

@ezekg ezekg left a comment

Choose a reason for hiding this comment

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

Let's clean up the formatting a bit. We also need to have an explicit check for Docker before using any of these. Lastly, there's a question regarding your other approach of using mountinfo.

machineid/__init__.py Outdated Show resolved Hide resolved
if not id:
id = __exec__('head -1 /proc/self/cgroup|cut -d/ -f3')
if not id:
id = __exec__("grep 'systemd' /proc/self/mountinfo|cut -d/ -f3")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
id = __exec__("grep 'systemd' /proc/self/mountinfo|cut -d/ -f3")
id = __exec__("grep 'systemd' /proc/self/mountinfo | cut -d/ -f3")

Copy link
Member

Choose a reason for hiding this comment

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

Can you give me more detail on what /proc/self/mountinfo is and how it can be used as a unique ID? I don't think mountinfo is Docker-specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ezekg , this idea of using mountinfo, came from this post: https://stackoverflow.com/a/71823877/3780957
This post mentions, some versions do not have the ID at /proc/self/cgroup.
I am not sure if this is Docker specific. I can confirm that I check on a Docker instance, and that file has the same ID as /proc/self/cgroup.

Copy link
Member

@ezekg ezekg Jan 30, 2023

Choose a reason for hiding this comment

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

Got more info here. I think this is fine as long as you add a similar if 'docker' in mountinfo check as above.

Copy link
Member

Choose a reason for hiding this comment

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

Also more discussion with the above workarounds is at opencontainers/runtime-spec#1105.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick question, if __read__('/proc/self/mountinfo') returns None (because the file does not exist).
Will if 'docker' in mountinfo: return an exception?

Copy link
Member

Choose a reason for hiding this comment

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

You’re right. I was only supplying pseudo code. I guess we should also check if mountinfo and 'docker' in mountinfo:. We shouldn’t raise an exception.

Copy link
Member

Choose a reason for hiding this comment

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

You also need to fix the indentation (you're using 4 spaces but the rest of the file uses 2 spaces).

Please make sure the code runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You’re right. I was only supplying pseudo code. I guess we should also check if mountinfo and 'docker' in mountinfo:. We shouldn’t raise an exception.

There is no additional exception.

Currently on a Docker container the if 'docker' in cgroup: does not find docker, the ending Exception works as intended. Same test with mountinfo.

When the full code does not find an ID raises the following:

Python 3.11.0 (main, Nov 15 2022, 19:58:01) [GCC 10.2.1 20210110] on linux                                                    
Type "help", "copyright", "credits" or "license" for more information.                                                        
>>> import machineid                                                                                                          
>>> print(machineid.id())                                                                                                     
Traceback (most recent call last):                                                                                            
  File "<stdin>", line 1, in <module>                                                                                         
  File "/usr/local/lib/python3.11/site-packages/machineid/__init__.py", line 90, in id                                        
    raise Exception('failed to obtain id on platform {}'.format(platform))                                                    
Exception: failed to obtain id on platform linux

Following your review, I made some updates on the patch.
* Fixed the indentation to 2.
* Checked on a Docker container the `if 'docker' in cgroup:` works when the `docker` is not found. Same test with `mountinfo`.
* When the full code does not find an ID raises the following:

`Python 3.11.0 (main, Nov 15 2022, 19:58:01) [GCC 10.2.1 20210110] on linux                                                    
Type "help", "copyright", "credits" or "license" for more information.                                                        
>>> import machineid                                                                                                          
>>> print(machineid.id())                                                                                                     
Traceback (most recent call last):                                                                                            
  File "<stdin>", line 1, in <module>                                                                                         
  File "/usr/local/lib/python3.11/site-packages/machineid/__init__.py", line 90, in id                                        
    raise Exception('failed to obtain id on platform {}'.format(platform))                                                    
Exception: failed to obtain id on platform linux`
Check if `cgroup` and `mountinfo` are not None, before checking if `docker` is inside the file.
To not rise a possible error when is trying to check `if 'docker' in None` (when the file does not exist).
@ezekg ezekg merged commit 7869558 into keygen-sh:master Feb 1, 2023
@ezekg
Copy link
Member

ezekg commented Feb 1, 2023

Released in v0.3.0: https://pypi.org/project/py-machineid/0.3.0/

@juanbretti
Copy link
Contributor Author

Great! Now is time to update my app and its dependencies :)
Thank you for all the support and insights on this PR.

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.

2 participants