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

ceph-detect-init: Add docker detection #13218

Merged
1 commit merged into from Feb 8, 2017

Conversation

Projects
None yet
2 participants
@guits
Copy link
Contributor

guits commented Feb 1, 2017

if running ceph-detect-init from a container, the given output is the
reference from the hosting system. This patch add the capability of
detecting if the tool is being run inside a container.

Signed-off-by: Guillaume Abrioux gabrioux@redhat.com

@@ -0,0 +1,3 @@
def choose_init():
"""Returns 'docker' because we are running in a container"""
return 'docker'

This comment has been minimized.

Copy link
@leseb

leseb Feb 1, 2017

Contributor

I believe it's clearer to return "none" since there is no init system within a container.
Also, people might use other container engine solution and might expect different results.
Otherwise the rest looks good me! Thanks!

@guits guits force-pushed the guits:ceph_detect_init branch 5 times, most recently from e71de17 to 67fb035 Feb 1, 2017

@leseb

leseb approved these changes Feb 3, 2017

@leseb

This comment has been minimized.

Copy link
Contributor

leseb commented Feb 3, 2017

@dachary when you have a moment. It's an easy one I guess :). Merci !

@@ -94,6 +96,12 @@ def _normalized_distro_name(distro):

def platform_information():
"""detect platform information from remote host."""
with open('/proc/self/cgroup', 'r') as f:

This comment has been minimized.

Copy link
@ghost

ghost Feb 3, 2017

gracefully fallback if this can't be open for any reason

@ghost ghost added core feature labels Feb 3, 2017

with open('/proc/self/cgroup', 'r') as f:
lines = f.read().splitlines()
for line in lines:
if "docker" in line.split(':')[2]:

This comment has been minimized.

Copy link
@ghost

ghost Feb 3, 2017

it should not traceback if the line cannot be split as required

@guits guits force-pushed the guits:ceph_detect_init branch 4 times, most recently from cd7ba75 to a4534cd Feb 3, 2017

@guits

This comment has been minimized.

Copy link
Contributor Author

guits commented Feb 6, 2017

@dachary I pushed some changes. When you have a moment, let me know.

if "docker" in line.split(':')[2]:
return ('docker', 'docker', 'docker')
except Exception as err:
logging.error("%s" % err)

This comment has been minimized.

Copy link
@ghost

ghost Feb 6, 2017

make that a better message and it's all good ;-)

@guits guits force-pushed the guits:ceph_detect_init branch 5 times, most recently from d2d664b to 92db23b Feb 7, 2017

ceph-detect-init: Add docker detection
if running `ceph-detect-init` from a container, the given output is the
reference from the hosting system. This patch add the capability of
detecting if the tool is being run inside a container.

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>

@guits guits force-pushed the guits:ceph_detect_init branch from 92db23b to f723098 Feb 8, 2017

@guits

This comment has been minimized.

Copy link
Contributor Author

guits commented Feb 8, 2017

@dachary it should be ok i think

@ghost

ghost approved these changes Feb 8, 2017

@ghost

This comment has been minimized.

Copy link

ghost commented Feb 8, 2017

these look good to merge. Note however that there are no integration tests for it, just unit tests. It does not seem necessary to block this change for this reason. But it would be good to add some kind of integration tests in the qa directory using docker containers.

@ghost ghost merged commit 72d6cd3 into ceph:master Feb 8, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.