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

Split the binary into two: client and daemon #20639

Merged
merged 2 commits into from
Apr 23, 2016
Merged

Conversation

dnephin
Copy link
Member

@dnephin dnephin commented Feb 24, 2016

Fixes #20362

./client is a new directory for the docker binary (aka the client)
./docker is the old directory which builds the docker-daemon binary (aka the daemon)

docker daemon is supported from the client by exec-ing the docker-daemon binary with the same args.

Notes:

  • I think the PostProcess() of CommonFlags could use some cleanup, but I'd like to wait for another branch since it's not required for this change. For now I've used a closure.
  • docker daemon will exec docker-daemon using exec.LookPath() to find it. It does not assume they are in the same directory.
  • When exec'ed all args are passed to docker-daemon. This means that a call like docker --graph=... daemon --bip ... will actually work. The logic to prevent that would be non-trivial, so I feel like it's better to just allow this. There's very little reason to prevent it now, since the flags on docker-daemon are a combination of both the common flags and daemon flags, they need to be unique anyway.
  • dynbinary builds both the client and the daemon, and the deb and rpm packages include both binaries.

Questions:

  • Is docker-daemon the right name for the daemon binary? (another suggestion is dockerd which would make things easier for the windows build).

@lowenna
Copy link
Member

lowenna commented Feb 24, 2016

@mikedougherty @dnephin @mebersol @tiborvass @icecrime This will have an impact to at least the Windows to Windows Jenkins CI script, and I think likely the Windows to Linux Jenkins CI script. Also the deployment scripts on Windows. I have to ask - what is the value of doing this? Is there an issue having a single binary? Or a specific goal being addressed by this change?

@mikedougherty
Copy link
Contributor

@jhowardmsft overall I understand it to be splitting two programs that do vastly different things. I like the idea of it. Of course there will be a transition period :) but this looks worth it to me.

(this is an argument for running the CI scripts from the revision the code is from but that raises potential security issues)

@thaJeztah
Copy link
Member

Also see #20362 for the roadmap issue

@lowenna
Copy link
Member

lowenna commented Feb 24, 2016

OK, I a little looking to see what it would take to convert the CI scripts to run on Windows TP4 in this world, and continue to run before this gets merged. First issue is the name of the daemon binary. The way Windows CI works is it starts the daemon under test with the name docker-gitcommitid.exe. When the script goes through its clear-down for reliability, it looks for any processes matching docker-* and kills them. That way it doesn't kill the control daemon (just docker.exe). So two ways to fix this - either change the script to not use - as seperator, or call the daemon binary something other than docker-daemon.exe. It's generally unusual for native exe's on Windows to include a - in their name. (Not sure about Linux norms.) Could the binary be called dockerd.exe instead?

@dnephin
Copy link
Member Author

dnephin commented Feb 24, 2016

Could the binary be called dockerd.exe instead?

I think the name is still very flexible, so this is possible. I'd like to hear other opinions on naming before making any changes, to try and reduce the number of times I rename it.

When the script goes through its clear-down for reliability, it looks for any processes matching docker-* and kills them.

That sounds ... aggressive. Why doesn't it just track the pids of the daemons that get started, and just kill the specific processes that it needs to kill?

@lowenna
Copy link
Member

lowenna commented Feb 24, 2016

@dnephin - Yes, aggressive, but what it takes to keep windowsTP4 reliable with the current set of issues. (Should add in the context of this, it won't necessarily know PIDs what was running previously, hence the arbitrary nuking of processes and much else)

@dnephin dnephin force-pushed the split_client branch 4 times, most recently from 3cee02d to e6394a6 Compare February 26, 2016 22:13
@lowenna
Copy link
Member

lowenna commented Feb 29, 2016

@icecrime Who's the 'test this' comment addressed to?

@icecrime icecrime added status/failing-ci Indicates that the PR in its current state fails the test suite and removed area/cli impact/cli labels Feb 29, 2016
@icecrime
Copy link
Contributor

@jhowardmsft "test this please" is the magic sentence to trigger the rebuild of the "documentation" job. Very much not confusing, right? 😆

@lowenna
Copy link
Member

lowenna commented Feb 29, 2016

Oh, THAT's how you retrigger documentation! 👿

@tiborvass
Copy link
Contributor

As far as design goes, this makes sense to me so +1.
For the name of the daemon, docker-daemon is better since we still want docker daemon to work by shelling out to docker-daemon. It feels more natural that way. It also opens up possible future git-like cli design.

@lowenna
Copy link
Member

lowenna commented Mar 1, 2016

@tiborvass While it might 'feel more natural that way' to Linux users, it's pretty alien to Windows users...

@duglin
Copy link
Contributor

duglin commented Mar 1, 2016

I prefer dockerd since I'm used to sshd, ftpd, etc....

@lowenna
Copy link
Member

lowenna commented Mar 1, 2016

...and containerd soon 😈

@thaJeztah
Copy link
Member

Perhaps the daemon should just be called docker-engine 😇

@lowenna
Copy link
Member

lowenna commented Mar 1, 2016

@thaJeztah It's the - I have a problem with 😇

@tiborvass
Copy link
Contributor

Ping @dnephin let me know if you want us to carry it if you're not available.

Signed-off-by: Daniel Nephin <[email protected]>
@dnephin
Copy link
Member Author

dnephin commented Apr 23, 2016

I don't really have a good way to test out these windows only changes right now, but I pushed what I think is a fix.

@lowenna
Copy link
Member

lowenna commented Apr 23, 2016

It got past unit tests on Windows with that update. LGTM if full CI passes

@tiborvass
Copy link
Contributor

tiborvass commented Apr 23, 2016

the failed test on windowsTP5 is unrelated to this PR

19:47:48 ----------------------------------------------------------------------
19:47:48 FAIL: docker_cli_restart_test.go:222: DockerSuite.TestRestartContainerwithRestartPolicy
19:47:48 
19:47:48 docker_cli_restart_test.go:229:
19:47:48     c.Assert(err, checker.IsNil)
19:47:48 ... value *errors.errorString = &errors.errorString{s:"condition \"\"false true\" == \"false false\"\" not true in time"} ("condition \"\"false true\" == \"false false\"\" not true in time")
19:47:48 
19:48:10 
19:48:10 ----------------------------------------------------------------------

Possibly related to #22042

@lowenna
Copy link
Member

lowenna commented Apr 23, 2016

Yes, agreed. Merging.

@lowenna lowenna merged commit 27f44b8 into moby:master Apr 23, 2016
@tiborvass tiborvass removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 23, 2016
@dnephin dnephin deleted the split_client branch April 23, 2016 22:30
@mlaventure mlaventure mentioned this pull request Apr 24, 2016
@nathanleclaire
Copy link
Contributor

@nathanleclaire hey, yo. Heard you like to know when format stuff is changing and its happening again. U R welcome ;)

Thanks !

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.