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

Make current_container_facts work with newer Docker versions and latest ansible-test container changes #510

Merged
merged 7 commits into from
Nov 30, 2022

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

It does seem to no longer work, might be related to ansible/ansible#78550.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

current_container_facts

@felixfontein felixfontein changed the title [WIP] Debug current_container_facts Make current_container_facts work with newer Docker versions and latest ansible-test container changes Nov 30, 2022
@github-actions
Copy link

github-actions bot commented Nov 30, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and the docs are now incorporated into main:
https://ansible-collections.github.io/community.docker/branch/main

@felixfontein
Copy link
Collaborator Author

CC @mattclay, looking at /proc/self/cpuset to figure out the current container breaks down on newer Docker daemons, and also no longer works with the alpine3 container on AZP in ansible-test since ansible/ansible#78550 was merged (don't ask me why only for that one...). I did some trying around, and looking at /proc/self/mountinfo and searching for /etc/hostname seems to work pretty well as a quick replacement (though that assumes that the user didn't change their docker data directory to something else than /var/lib/docker/ - or maybe this is also a fixed string that isn't related to the position of that directory on the host system?). This might be useful for https://github.com/ansible/ansible/blob/devel/test/lib/ansible_test/_internal/docker_util.py#L576.

Copy link

@kristianheljas kristianheljas left a comment

Choose a reason for hiding this comment

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

Other than the outdated comment and test with docker, look good. Podman worked flawlessly

plugins/modules/current_container_facts.py Show resolved Hide resolved
plugins/modules/current_container_facts.py Outdated Show resolved Hide resolved
@mattclay
Copy link

@felixfontein Looking at /proc/self/cpuset is unreliable because of cgroup namespaces. It contains a host-specific path only when the host namespace is used:

$ docker run -it --rm --cgroupns host quay.io/bedrock/alpine:3.17.0 cat /proc/self/cpuset
/docker/a34f76a8f390f33188505c9be17734fcd41d6b1afc014bb36ad0b030a24ffe11
$ docker run -it --rm --cgroupns private quay.io/bedrock/alpine:3.17.0 cat /proc/self/cpuset
/

@felixfontein felixfontein merged commit c2d84ef into ansible-collections:main Nov 30, 2022
@felixfontein felixfontein deleted the current-container branch November 30, 2022 21:25
@felixfontein
Copy link
Collaborator Author

@kristianheljas @mattclay thanks for testing, reviewing, and commenting!

felixfontein added a commit to felixfontein/community.docker that referenced this pull request Nov 30, 2022
…st ansible-test container changes (ansible-collections#510)

* Add more debug output.

* Add basic integration test.

* Split into lines.

* Fix docker detection, add podman detection.

ci_complete

* Improve regular expression.

* Document that this module is trying its best, but might not be perfect.

* Update comment.

(cherry picked from commit c2d84ef)
@felixfontein
Copy link
Collaborator Author

Manual backport of bugfix part to stable-2 in #512.

felixfontein added a commit that referenced this pull request Nov 30, 2022
…ns and latest ansible-test container changes (#512)

* Make current_container_facts work with newer Docker versions and latest ansible-test container changes (#510)

* Add more debug output.

* Add basic integration test.

* Split into lines.

* Fix docker detection, add podman detection.

ci_complete

* Improve regular expression.

* Document that this module is trying its best, but might not be perfect.

* Update comment.

(cherry picked from commit c2d84ef)

* Remove new feature (podman support).
felixfontein added a commit that referenced this pull request Nov 30, 2022
…ns and latest ansible-test container changes (#512)

* Make current_container_facts work with newer Docker versions and latest ansible-test container changes (#510)

* Add more debug output.

* Add basic integration test.

* Split into lines.

* Fix docker detection, add podman detection.

ci_complete

* Improve regular expression.

* Document that this module is trying its best, but might not be perfect.

* Update comment.

(cherry picked from commit c2d84ef)

* Remove new feature (podman support).

(cherry picked from commit 3da9aa3)
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.

3 participants