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

Create the systemd.service for Debian which respects /etc/default/docker #12134

Closed
wants to merge 2 commits into from

Conversation

onorua
Copy link

@onorua onorua commented Apr 7, 2015

As you might already know, Debian 8 will migrate to systemd, most of the users used to change configuration parameters inside the /etc/default/docker. In order to release a pain of migration, I propose to have docker.service which respects /etc/default/docker configuration options.

@GordonTheTurtle
Copy link

Can you please sign your commits following these rules:

https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work

The easiest way to do this is to amend the last commit:

$ git clone -b "master" git@github.com:onorua/docker.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

This will update the existing PR, so you do not need to open a new one.

Signed-off-by: Yaroslav Molochko <onorua@gmail.com>
@@ -0,0 +1,3 @@
Lokesh Mandvekar <lsm5@fedoraproject.org> (@lsm5)
Brandon Philips <brandon.philips@coreos.com> (@philips)
Jessie Frazelle <jess@docker.com> (@jfrazelle)
Copy link
Contributor

Choose a reason for hiding this comment

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

why did this change?

@jessfraz
Copy link
Contributor

jessfraz commented Apr 7, 2015

wait, why cant we use the one we already have? do we need another? I think it should be a 1 line change

@onorua
Copy link
Author

onorua commented Apr 8, 2015

Actually you are right, if the file doesn't exists it will not read anything and make ENV variable empty, which is exactly what we need.
I just saw the similar approach for sysvinit with 2 different folders for 2 different platforms and decided to do the same /etc/sysconfig/docker for RH based and /etc/default/docker for Debian based, but then when I was going to push it, decided that 99% of RH systems already have systemd and already have all necessary configuration in systemd service file.
Any way, I'll update PR to have 2 lines changed instead of 3 additional files.

Signed-off-by: Yaroslav Molocko <onorua@gmail.com>
@jessfraz
Copy link
Contributor

jessfraz commented Apr 8, 2015

LGTM ping @tianon @philips @lsm5

@philips
Copy link
Contributor

philips commented Apr 8, 2015

No, this should be done as a drop-in by Debian. See the .d files in Example 2 here: http://www.freedesktop.org/software/systemd/man/systemd.unit.html#Examples

@onorua
Copy link
Author

onorua commented Apr 9, 2015

What if user did not install "docker.io" package from debian repository but from get.docker.com repository?

@lsm5
Copy link
Contributor

lsm5 commented Apr 9, 2015

-1 if we want to keep this file distro-agnostic. I had #7220 nack-ed which used /etc/sysconfig/docker as that ended up being RH family-specific, so the rpms now use their own unitfiles to take care of that. Also looks like Gentoo uses /etc/conf.d and other distros might have their own.

@philips RE: drop-ins, I guess chances are high a stale drop-in could end up confusing the user (correct me if I'm wrong)

@onorua
Copy link
Author

onorua commented Apr 9, 2015

That is why I've created systemd-debian in a first place, to make debian specific version of the file. Should I revet my changes to that debian specific systemd folder?

@lsm5
Copy link
Contributor

lsm5 commented Apr 9, 2015

I'm ok with having separate dirs per distro (hoping we don't end up with far too many), upto the others to ack/nack this.

@jessfraz
Copy link
Contributor

jessfraz commented Apr 9, 2015

I feel like this could be fixed w the drop in and tianons patch to have
distro specific builds but idk, I just don't want to have to maintain a
bunch of slightly different init scripts... Where does it end ya know

On Thursday, April 9, 2015, Lokesh Mandvekar notifications@github.com
wrote:

I'm ok with having separate dirs per distro (hoping we don't end up with
far too many), upto the others to ack/nack this.


Reply to this email directly or view it on GitHub
#12134 (comment).

@tianon
Copy link
Member

tianon commented Apr 9, 2015

I definitely agree that what's solved by these files is more appropriately solved here via drop-ins (and systemd upstream prefers that method).

Here's another kink with the EnvironmentFile method: (from https://bugs.debian.org/767441, which has a lot more verbose context)

Oct 31 11:07:42 helix systemd[1]: Ignoring invalid environment 'export http_proxy=http://127.0.0.1:3128/[10]': /etc/default/docker

(ie, the syntax for /etc/default/docker is explicitly "sourced shell script", where EnvironmentFile is key=val pairs)

@onorua
Copy link
Author

onorua commented Apr 10, 2015

So, as far as I undertood I need to reject/cancel this PR and create a new request to Debian maintainer of docker? Or there is something I can do here?

@jessfraz
Copy link
Contributor

I think what we need is a PR that will go in after
#12111 so we can have jessie specific
things :) sorry for the confusion

On Fri, Apr 10, 2015 at 11:16 AM, onorua notifications@github.com wrote:

So, as far as I undertood I need to reject/cancel this PR and create a new
request to Debian maintainer of docker? Or there is something I can do
here?


Reply to this email directly or view it on GitHub
#12134 (comment).

@tianon
Copy link
Member

tianon commented Apr 10, 2015 via email

@jessfraz
Copy link
Contributor

oh damn ya so then nm sorry

On Fri, Apr 10, 2015 at 11:31 AM, Tianon Gravi notifications@github.com
wrote:

Well, even still, I'm personally -1 on this in general, both because the
usage of "/etc/default/docker" is different (as I explained above) and
because systemd upstream recommends drop-ins for the types of changes one
would previously use "/etc/default/docker" for (such as modifying the
daemon startup parameters).


Reply to this email directly or view it on GitHub
#12134 (comment).

@onorua
Copy link
Author

onorua commented Apr 10, 2015

I totally agree with you to go mainstream drop-in approach. I just really worry that somebody will update system, and let say storage engine got overwritten to default one, which means:
best option - several minutes of downtime to download new images instead of seconds for daemon restart
worse option - disk got full, registry is not available at that time due to network problem, what ever terrible thing can happen, and user doesn't even know what to do.
As a workaround, it can be a comment inside /etc/default/docker, such as:
"Guy, you use systemd, stop using bad shell scripts for managing your services, use /lib/systemd/systemd/docker.service file instead"
But I don't see any real solution to prevent "broken" systems during system upgrade.

@philips
Copy link
Contributor

philips commented Apr 10, 2015

as an aside it is really annoying that you cannot append args via drop-ins. You can only rely on environment variable replacement and replace the whole thing but there is no way to layer-on appends.

@jessfraz
Copy link
Contributor

closing as I believe we can prevent broken systems with @vbatts patch that was merged to see if they are not using the same graph driver as before, and I think debian users will know these things are going to happen and they are pretty smart in figuring out how to migrate, this will surely not be the only init script on their system I do not think we need to hold their hand

@jessfraz jessfraz closed this Apr 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants