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

Fixies dockerd #22278

Merged
merged 1 commit into from
May 11, 2016
Merged

Fixies dockerd #22278

merged 1 commit into from
May 11, 2016

Conversation

runcom
Copy link
Member

@runcom runcom commented Apr 24, 2016

  • second commit use dockerd instead of docker daemon in systemd unit file ExecStart

@runcom
Copy link
Member Author

runcom commented Apr 24, 2016

ping @jfrazelle

@jessfraz
Copy link
Contributor

When did the name change? Sorry I'm super behind on the repo. Looks good to
me tho

On Sunday, April 24, 2016, Antonio Murdaca [email protected] wrote:

ping @jfrazelle https://github.com/jfrazelle


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#22278 (comment)

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

@mlaventure
Copy link
Contributor

@jfrazelle changed here: #20639

@HackToday
Copy link
Contributor

don't we fix doc related changes for that binary change ?

@kencochrane
Copy link
Contributor

👍

@thaJeztah
Copy link
Member

I guess the other init scripts also need to be modified? https://github.com/docker/docker/tree/master/contrib/init

@runcom
Copy link
Member Author

runcom commented Apr 25, 2016

I didn't because I've honestly no expertise :/ could be done in a follow up I guess

@runcom
Copy link
Member Author

runcom commented Apr 25, 2016

I guess I can grep for docker daemon though 😄

@runcom
Copy link
Member Author

runcom commented Apr 26, 2016

updated, ping @tianon

@mlaventure
Copy link
Contributor

Given that the value for BASE in the scripts is being changed to dockerd we need to rename our default configuration file to dockerd in /etc/default/ and /etc/sysconfig too.

@runcom
Copy link
Member Author

runcom commented Apr 26, 2016

@mlaventure where are those files

@@ -2,9 +2,9 @@
# Copyright 1999-2013 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2

command="${DOCKER_BINARY:-/usr/bin/docker}"
command="${DOCKER_BINARY:-/usr/bin/dockerd}"
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure what we ought to do here -- if folks have an existing /etc/conf.d/docker, then this means that their DOCKER_BINARY entry now needs to change to point to dockerd instead, so I think we should probably rename the var to make this really explicit and clear (and update docker.confd appropriately).

@runcom
Copy link
Member Author

runcom commented Apr 26, 2016

updated @tianon @mlaventure PTAL

@@ -7,7 +7,7 @@
#DOCKER_PIDFILE="/run/docker.pid"

# where the docker daemon itself is run from
#DOCKER_BINARY="/usr/bin/docker"
DOCKER_BINARY="/usr/bin/dockerd"
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant we should rename this to be DOCKERD_BINARY instead, so that if someone has an existing file, their DOCKER_BINARY setting won't be used as if it were dockerd to poor effect.

@mlaventure
Copy link
Contributor

@runcom

There are a few files that'd need to be updated if we decide to use dockerd instead of docker daemon from now on, including some markdown one.

The easiest way to find them is to run: git grep 'sysconfig/docker' and git grep 'default/docker'

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 26, 2016

@runcom we merged some stuff about cmd/docker{d}. Did it affect your PR?

@runcom
Copy link
Member Author

runcom commented Apr 26, 2016

@LK4D4 I don't think so as long as it's always installed into /usr/bin/ as dockerd :)

@runcom
Copy link
Member Author

runcom commented Apr 28, 2016

looks like doc stuff for dockerd has been updated here #22386 ping @thaJeztah

@thaJeztah
Copy link
Member

@runcom thanks for the ping, was already looking at that :-)

we probably need to update the completion scripts as well?

@mlaventure
Copy link
Contributor

mlaventure commented May 5, 2016

ping @thaJeztah

@runcom made me realize on #22519 that this was still open.

Do we still want to have the completion scripts updated as part of this PR?

P.S: It needs a rebase btw

@thaJeztah
Copy link
Member

@mlaventure no, I'm fine with doing that in a separate PR (unless @runcom wants to do it here). Perhaps we should create a tracking issue with all that's left for 1.12 to fix related to the docker daemon to dockerd change

@LK4D4
Copy link
Contributor

LK4D4 commented May 6, 2016

@runcom would you mind to answer @tianon comments?

@runcom
Copy link
Member Author

runcom commented May 6, 2016

Yes I missed pushing my changes, will do

@runcom
Copy link
Member Author

runcom commented May 8, 2016

@LK4D4 @tianon Fixed, let me know if this needs any other things

@LK4D4
Copy link
Contributor

LK4D4 commented May 9, 2016

LGTM
ping @tianon

@runcom
Copy link
Member Author

runcom commented May 10, 2016

ping @tianon

@@ -107,7 +107,7 @@ case "$1" in
--pidfile "$DOCKER_SSD_PIDFILE" \
Copy link
Member

Choose a reason for hiding this comment

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

the DOCKER var in the line above this should be DOCKERD now 😄 (both here, at the top of this file, and in sysvinit-debian/docker.default)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and in upstart/docker.conf

Copy link
Member Author

Choose a reason for hiding this comment

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

damn me :) thx for checking again

@@ -8,7 +8,7 @@
#

# Customize location of Docker binary (especially for development testing).
#DOCKER="/usr/local/bin/docker"
DOCKERD="/usr/bin/dockerd"
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I think this is the last one -- this should still be /usr/local/bin/dockerd and should be commented out by default. 😇

Signed-off-by: Antonio Murdaca <[email protected]>
@runcom
Copy link
Member Author

runcom commented May 11, 2016

@tianon updated again

@tianon
Copy link
Member

tianon commented May 11, 2016

Very nice, LGTM 👍 ❤️

@LK4D4
Copy link
Contributor

LK4D4 commented May 11, 2016

LGTM
thanks @runcom

@LK4D4 LK4D4 merged commit fd3a795 into moby:master May 11, 2016
@runcom runcom deleted the fixies-dockerd branch May 11, 2016 14:57
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.

9 participants