[enhancement] add optional (features) fields in daemon.json to enable buildkit #37593
Conversation
0bdbb35 to
ae1f83e
Compare
Codecov Report
@@ Coverage Diff @@
## master #37593 +/- ##
=========================================
Coverage ? 35.82%
=========================================
Files ? 607
Lines ? 44733
Branches ? 0
=========================================
Hits ? 16025
Misses ? 26497
Partials ? 2211 |
ae1f83e to
96f5eeb
Compare
96f5eeb to
65bdc72
Compare
|
cc @andrewhsu |
|
@tiborvass I don't think we need a flag. Config should be enough. |
cmd/dockerd/daemon.go
Outdated
There was a problem hiding this comment.
I guess it's because all the other things that were setting headers were middlewares as well.
There was a problem hiding this comment.
@tonistiigi This is how the request headers are set, Docker-Experimental for one example. In an early discussion, we wanted to have _ping endpoint to carry the information of whether buildkit is enabled on daemon. And this middleware sets the header to _ping.
There was a problem hiding this comment.
This looks very weird that API command that does not have anything to do with builder returns Buildkit=true.
@tonistiigi I thought this way too. However, it seems that when the daemon starts and loads the config, there is always a flag check to make sure whatever in the Line 470 in f57f260 |
api/types/types.go
Outdated
There was a problem hiding this comment.
BuildKit that's how it's spelled elsewhere especially for API types
There was a problem hiding this comment.
Should it be FeatureBuildKit ? Also, need to make the flag longer, or modify the config condition.
7eb16e1 to
21e79dd
Compare
|
@tonistiigi @tiborvass Made "buildkit" an exception from flag validation as we discussed. Now it's only an optional field in daemon.json |
21e79dd to
e5bf6be
Compare
There was a problem hiding this comment.
I don't want to over-complicate this, but it seems we are now moving from a generic all-or-nothing flag to feature flags (liberal use of the term flag here).
I would expect that the pieces in the API to signal somehow that something is a feature flag rather than just some top-level field.
api/server/middleware/special.go
Outdated
There was a problem hiding this comment.
What makes these special as opposed to just headers?
api/server/middleware/special.go
Outdated
There was a problem hiding this comment.
Worried that we are leaking implementation details into the API package here.
daemon/config/config.go
Outdated
There was a problem hiding this comment.
Let's make this FeatureBuildKit and json:"feature-buildkit"
cmd/dockerd/daemon.go
Outdated
There was a problem hiding this comment.
@AntaresS we don't want this to be the case, because it will add the header on every response. I don't think you need middlewares, you should be able to just add the header in the ping handler.
There was a problem hiding this comment.
@tiborvass well the ping handler by itself doesn't know anything about daemon config. Unless we modify the Backend interface to get the info of whether buildkit is enabled or somehow pass that knowledge into NewRouter. Is this more desired?
api/types/types.go
Outdated
86ce496 to
f85b725
Compare
|
For the daemon.json, I'm +1 on using |
|
@thaJeztah alright fine with |
614248d to
72eb175
Compare
df5643f to
fc627b4
Compare
client/ping.go
Outdated
There was a problem hiding this comment.
@AntaresS this should be:
if bv := serverResp.header.Get("Builder-Version"); bv != "" {
ping.BuilderVersion = bv
}
client/ping.go
Outdated
fc627b4 to
66a2eb1
Compare
There was a problem hiding this comment.
looks like there's still some issues with the current implementation/code:
$ mkdir -p /etc/docker
$ echo '{"experimental":true, "features": {"buildkit":true}}' > /etc/docker/daemon.json
$ dockerd
unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives don't match any configuration option: buildkitChanging to feature-buildkit makes the daemon start;
$ echo '{"experimental":true, "features": {"feature-buildkit":true}}' > /etc/docker/daemon.json
...
INFO[2018-08-19T09:12:24.248409702Z] Daemon has completed initialization
INFO[2018-08-19T09:12:24.274417246Z] API listen on /var/run/docker.sock
But buildkit won't be enabled / visible in /_ping;
$ curl -v --unix-socket /var/run/docker.sock http://localhost/_ping
...
< HTTP/1.1 200 OK
< Api-Version: 1.39
< Docker-Experimental: true
< Ostype: linux
< Server: Docker/dev (linux)
< Date: Sun, 19 Aug 2018 09:13:17 GMT
< Content-Length: 2
< Content-Type: text/plain; charset=utf-8Live-reload also isn't implemented yet (could possibly be done in a follow-up);
$ rm /etc/docker/daemon.json
$ kill -HUP $(pidof dockerd)Got signal to reload configuration, reloading from: /etc/docker/daemon.json
INFO[2018-08-19T09:18:19.584233372Z] Reloaded configuration: {"mtu":1500,"pidfile":"/var/run/docker.pid","data-root":"/var/lib/docker","exec-root":"/var/run/docker","group":"docker","deprecated-key-path":"/etc/docker/key.json","max-concurrent-downloads":3,"max-concurrent-uploads":5,"shutdown-timeout":15,"hosts":["unix:///var/run/docker.sock"],"log-level":"info","swarm-default-advertise-addr":"","swarm-raft-heartbeat-tick":0,"swarm-raft-election-tick":0,"metrics-addr":"","log-driver":"json-file","ip":"0.0.0.0","icc":true,"iptables":true,"ip-forward":true,"ip-masq":true,"userland-proxy":true,"default-address-pools":{},"network-control-plane-mtu":1500,"disable-legacy-registry":true,"experimental":true,"containerd":"/var/run/docker/containerd/docker-containerd.sock","features":{"feature-buildkit":true},"runtimes":{"runc":{"path":"docker-runc"}},"default-runtime":"runc","oom-score-adjust":-500,"default-shm-size":67108864,"default-ipc-mode":"shareable","resolv-conf":"/etc/resolv.conf"}
cmd/dockerd/daemon.go
Outdated
There was a problem hiding this comment.
If the buildkit feature is not set in daemon.json, no Builder-Version header is set? I'd expect features: {buildkit:false}} and {} to be equivalent (i.e. in both cases buildkit is not active, so builder version is 1)
There was a problem hiding this comment.
@thaJeztah yes. So it doesn't matter.
There was a problem hiding this comment.
Shouldn't the header be set if no daemon.json is present (thus config.Features["buildkit"] is not set)?
There was a problem hiding this comment.
@thaJeztah We actually meant to make it distinguished, so features: {buildkit:false}} means buildkit is not allowed while {} (or not set in daemon.json) means it's up to client to choose builder version. For header it is the same concept.
There was a problem hiding this comment.
Still confused by this; how would the client know that buildkit is available?
There was a problem hiding this comment.
@thaJeztah It's obvious that when the features is set in daemon.json client will know the version. If it is not set in the config file (i.e. header will not include Builder-Version either), cli will use the env var to determine whether to make buildkit request or otherwise use the old builder. Maybe the changes from cli can also help you - docker/cli#1275
daemon/config/config.go
Outdated
There was a problem hiding this comment.
Yes, I thought this was fixed. @AntaresS
client/ping.go
Outdated
There was a problem hiding this comment.
This should also be documented in the swagger; https://github.com/moby/moby/blob/896d1b1c61a48e2df1a7b4644ddde6ee97db6111/api/swagger.yaml?utf8=✓#L6967-L6985
66a2eb1 to
d9019e0
Compare
Signed-off-by: Anda Xu <anda.xu@docker.com>
d9019e0 to
2be1766
Compare
Signed-off-by: Anda Xu anda.xu@docker.com
- What I did
--buildkitindockerdto enable buildkit as Dockerfile builder. (This is also a pre-req by the daemon implementaion)- How I did it
- How to verify it
also
dockerd --helpwill print help info of--buildkitflag- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)