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

Improve detection of containerized environments - fix issue #1732 #1879

Merged
merged 1 commit into from Jul 12, 2018

Conversation

Projects
None yet
4 participants
@flavio
Member

flavio commented Jul 10, 2018

The code determining if Portus is running containerized is broken on Kubernetes because in this case PID 1 doesn't belong to the docker cgroup slice.

That causes the code to invoke hostnamectl, that tries to access kdbus from within a container. That causes Rails to fail immediately.

fixes #1732

Improve detection of containerized environments - fix issue #1732
The code determining if Portus is running containerized is broken on
Kubernetes because in this case PID 1 doesn't belong to the `docker`
cgroup slice.

That causes the code to invoke `hostnamectl`, that tries to access
kdbus from within a container. That causes Rails to fail immediately.

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
@flavio

This comment has been minimized.

Show comment
Hide comment
@flavio

flavio Jul 10, 2018

Member

@mssola the code is gone from master. However I think we should fix it inside of the v2.3 branch.

Member

flavio commented Jul 10, 2018

@mssola the code is gone from master. However I think we should fix it inside of the v2.3 branch.

@jordimassaguerpla

I can't test it right now but given the detailed bug report, this makes a lot of sense , hence I am approving this. Thanks for this fix.

@mssola mssola merged commit eaab41d into v2.3 Jul 12, 2018

3 checks passed

DCO All commits have a DCO sign-off from the author
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mssola mssola deleted the v2.3-fix-1732 branch Jul 12, 2018

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 12, 2018

Contributor

Thanks a lot 👏

Contributor

mssola commented Jul 12, 2018

Thanks a lot 👏

@Monnoroch

This comment has been minimized.

Show comment
Hide comment
@Monnoroch

Monnoroch Jul 16, 2018

@flavio also, docker is not the only container runtime. There's also containerd, rkt and probably more. Checking for kubepods is great for kubernetes, but this check has to be redesigned to handle more runtimes.

Monnoroch commented Jul 16, 2018

@flavio also, docker is not the only container runtime. There's also containerd, rkt and probably more. Checking for kubepods is great for kubernetes, but this check has to be redesigned to handle more runtimes.

@flavio

This comment has been minimized.

Show comment
Hide comment
@flavio

flavio Jul 16, 2018

Member

@Monnoroch by default kubernetes places all the user PODs under this cgroup namespace, despite of the CRI in use.

Member

flavio commented Jul 16, 2018

@Monnoroch by default kubernetes places all the user PODs under this cgroup namespace, despite of the CRI in use.

@Monnoroch

This comment has been minimized.

Show comment
Hide comment
@Monnoroch

Monnoroch Jul 16, 2018

There is life outside kubernetes too. Besides, what if they change the cgroup name? Current implementation looks like a hack, really. I think there should be a way for a user to force assuming a containerized environment. Or, better yet, making the software environment-agnostic.

Monnoroch commented Jul 16, 2018

There is life outside kubernetes too. Besides, what if they change the cgroup name? Current implementation looks like a hack, really. I think there should be a way for a user to force assuming a containerized environment. Or, better yet, making the software environment-agnostic.

@flavio

This comment has been minimized.

Show comment
Hide comment
@flavio

flavio Aug 7, 2018

Member

As far as I've seen this code is gone from the master branch of Portus. I see this just as a temporary fix.

@mssola can you confirm what I've said, we are not going to have to deal with this code anymore. Am I right?

Member

flavio commented Aug 7, 2018

As far as I've seen this code is gone from the master branch of Portus. I see this just as a temporary fix.

@mssola can you confirm what I've said, we are not going to have to deal with this code anymore. Am I right?

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Aug 8, 2018

Contributor

As far as I've seen this code is gone from the master branch of Portus. I see this just as a temporary fix.

That's correct. This fix only applied to the v2.3 branch. For the master branch we are using this portusctl, which does not need this kind of check.

Contributor

mssola commented Aug 8, 2018

As far as I've seen this code is gone from the master branch of Portus. I see this just as a temporary fix.

That's correct. This fix only applied to the v2.3 branch. For the master branch we are using this portusctl, which does not need this kind of check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment