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 socket activation for RHEL based distributions #273

Merged
merged 1 commit into from Nov 30, 2018

Conversation

seemethere
Copy link
Contributor

Removes the systemd drop-in unit file for socket activation and instead
prefers socket activation by default for both RHEL based and DEBIAN
based distributions.

Socket activation for RHEL based distributions was tested on CentOS 7 and Fedora 28.

Signed-off-by: Eli Uriegas eli.uriegas@docker.com

Removes the systemd drop-in unit file for socket activation and instead
prefers socket activation by default for both RHEL based and DEBIAN
based distributions.

Socket activation for RHEL based distributions was tested on CentOS 7 and Fedora 28.

Signed-off-by: Eli Uriegas <eli.uriegas@docker.com>
@seemethere seemethere requested review from thaJeztah and a team November 29, 2018 00:49
Copy link
Contributor

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Was there a reason for not using a drop-in file?

Also note that this will be a breaking change for RHEL/CentOS users that currently set the host through daemon.json; try using;

mkdir -p /etc/docker
echo '{"hosts":["tcp://localhost:2375"]}' > /etc/docker/daemon.json

And then restart the docker.service

@andrewhsu
Copy link
Contributor

This will be a breaking change for users who set the host through daemon.json and have installed docker-ce from the nightly channel in the past 13 days since #257 was merged, but this will not be a breaking change in that same way for 18.09 users when this change is ported to 18.09 codeline because there was already a host port set: https://github.com/docker/docker-ce/blob/v18.09.0/components/packaging/systemd/docker.service#L13

@thaJeztah
Copy link
Member

thaJeztah commented Nov 29, 2018

but this will not be a breaking change in that same way for 18.09 users when this change is ported to 18.09 codeline because there was already a host port set

In previous releases, the host was only set on .deb packages, and not on RPM packages (because it didn't have socket activation) https://github.com/docker/docker-ce/blob/v18.06.1-ce/components/packaging/rpm/systemd/docker.service#L12

@andrewhsu
Copy link
Contributor

andrewhsu commented Nov 29, 2018

Socket activation was removed from docker engine in RPMs long time ago because of selinux policy incompatibilities, but since then the selinux policies are now compatible with socket activation. @seemethere had tested this PR's changes on a test build fedora and centos RPM packages.

Since we did not have host port set in 18.06.1 RPM packages, I would not recommend backporting this change to the 18.06 codeline.

Whereas since we already set the host port in 18.09.0 RPM packages, I would offer that it is safe to backport this change to the 18.09 codeline for the future 18.09.1 release.

@thaJeztah
Copy link
Member

Alright; if we do, we should make sure there's a note in changelog/release-notes with instructions how to resolve the issue if you're upgrading from an older release.

(And we should have a follow-up discussion around how daemon.json and flags should interact; possibly an exception for -H fd:///)

@andrewhsu andrewhsu added this to the 18.09.1 milestone Nov 30, 2018
@andrewhsu andrewhsu merged commit 0ee08c6 into docker:master Nov 30, 2018
@seemethere seemethere deleted the sockles branch December 3, 2018 19:10
@andrewhsu andrewhsu removed this from the 18.09.1 milestone Dec 6, 2018
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.

None yet

3 participants