[enhancement] add optional (features) fields in daemon.json to enable buildkit by AntaresS · Pull Request #37593 · moby/moby

AntaresS · 2018-08-06T01:55:32Z

Signed-off-by: Anda Xu [email protected]

- What I did

Add optional fields to allow enable buildkit as Dockerfile builder from daemon.json.
- Add new flag --buildkit in dockerd to enable buildkit as Dockerfile builder. (This is also a pre-req by the daemon implementaion)
no longer assuming buildkit is on by default if "--experimental" is true.

- How I did it

- How to verify it

also dockerd --help will print help info of --buildkit flag

 --buildkit                                Enable buildkit as dockerfile builder

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

codecov · 2018-08-06T05:15:42Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7d4fa69). Click here to learn what that means.
The diff coverage is 6.66%.

@@            Coverage Diff            @@
##             master   #37593   +/-   ##
=========================================
  Coverage          ?   35.82%           
=========================================
  Files             ?      607           
  Lines             ?    44733           
  Branches          ?        0           
=========================================
  Hits              ?    16025           
  Misses            ?    26497           
  Partials          ?     2211

AntaresS · 2018-08-06T18:41:08Z

@tonistiigi @tiborvass @dhiltgen ptal

AntaresS · 2018-08-07T00:14:35Z

cc @andrewhsu

tonistiigi · 2018-08-07T00:15:38Z

@tiborvass I don't think we need a flag. Config should be enough.

tonistiigi · 2018-08-07T00:17:58Z

cmd/dockerd/daemon.go

Why is this a middleware?

I guess it's because all the other things that were setting headers were middlewares as well.

@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.

This looks very weird that API command that does not have anything to do with builder returns Buildkit=true.

AntaresS · 2018-08-07T00:30:49Z

I don't think we need a flag. Config should be enough.

@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 daemon.json is also a valid flag in dockerd cli. Otherwise, an error will be returned by

moby/daemon/config/config.go

Line 470 in f57f260

    
           return fmt.Errorf("the following directives don't match any configuration option: %s", strings.Join(unknown, ", "))

tiborvass · 2018-08-07T00:53:46Z

api/types/types.go

BuildKit that's how it's spelled elsewhere especially for API types

Should it be FeatureBuildKit ? Also, need to make the flag longer, or modify the config condition.

AntaresS · 2018-08-07T18:12:37Z

@tonistiigi @tiborvass Made "buildkit" an exception from flag validation as we discussed. Now it's only an optional field in daemon.json

cpuguy83

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.

cpuguy83 · 2018-08-08T00:03:10Z

api/server/middleware/special.go

What makes these special as opposed to just headers?

cpuguy83 · 2018-08-08T00:03:33Z

api/server/middleware/special.go

Worried that we are leaking implementation details into the API package here.

@cpuguy83 @AntaresS let's do Feature-BuildKit-Enabled

tiborvass · 2018-08-09T00:21:54Z

daemon/config/config.go

Let's make this FeatureBuildKit and json:"feature-buildkit"

tiborvass · 2018-08-09T00:24:20Z

cmd/dockerd/daemon.go

@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.

@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?

tiborvass · 2018-08-09T00:24:56Z

api/types/types.go

FeatureBuildKit

thaJeztah · 2018-08-17T17:23:12Z

For the daemon.json, I'm +1 on using "features": {"buildkit": true} (or {"features": [{"buildkit":true}]}). Yes; it's verbose, but I think it's clearer that it's to enable/disable features, and we can add more features without changing the overall format

tiborvass · 2018-08-17T17:25:07Z

@thaJeztah alright fine with {"features": [{"buildkit":true}]} and Builder-Version.

AntaresS · 2018-08-17T23:36:13Z

@tiborvass @vdemeester @cpuguy83 @thaJeztah lgty?

tiborvass · 2018-08-19T06:59:19Z

client/ping.go

@AntaresS this should be:

if bv := serverResp.header.Get("Builder-Version"); bv != "" { ping.BuilderVersion = bv }

tiborvass · 2018-08-19T06:59:38Z

client/ping.go

wrong header in comment

tiborvass

LGTM

tiborvass · 2018-08-19T07:31:17Z

@vdemeester @thaJeztah @cpuguy83 PTAL

thaJeztah

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: buildkit

Changing 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-8

Live-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"}

thaJeztah · 2018-08-19T09:25:22Z

cmd/dockerd/daemon.go

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)

@thaJeztah yes. So it doesn't matter.

Shouldn't the header be set if no daemon.json is present (thus config.Features["buildkit"] is not set)?

@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.

Still confused by this; how would the client know that buildkit is available?

@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

thaJeztah · 2018-08-19T09:26:12Z

daemon/config/config.go

This should be buildkit now?

Yes, I thought this was fixed. @AntaresS

Or maybe just features

thaJeztah · 2018-08-19T09:31:52Z

client/ping.go

This should also be documented in the swagger; https://github.com/moby/moby/blob/896d1b1c61a48e2df1a7b4644ddde6ee97db6111/api/swagger.yaml?utf8=✓#L6967-L6985

Signed-off-by: Anda Xu <[email protected]>

thaJeztah

LGTM

AntaresS requested a review from dnephin as a code owner August 6, 2018 01:55

GordonTheTurtle added the status/0-triage label Aug 6, 2018

AntaresS force-pushed the add-enable-buildkit branch 4 times, most recently from 0bdbb35 to ae1f83e Compare August 6, 2018 05:15

AntaresS force-pushed the add-enable-buildkit branch from ae1f83e to 96f5eeb Compare August 6, 2018 05:22

AntaresS force-pushed the add-enable-buildkit branch from 96f5eeb to 65bdc72 Compare August 6, 2018 18:46

AntaresS changed the title ~~[WIP] add optional fields in daemon.json to enable buildkit~~ [enhancement] add "--buildkit" flag and optional fields in daemon.json to enable buildkit Aug 6, 2018

AntaresS mentioned this pull request Aug 6, 2018

[enhancement] enable buildkit from daemon side docker/cli#1275

Merged

tonistiigi reviewed Aug 7, 2018

View reviewed changes

tiborvass reviewed Aug 7, 2018

View reviewed changes

AntaresS force-pushed the add-enable-buildkit branch 3 times, most recently from 7eb16e1 to 21e79dd Compare August 7, 2018 18:10

AntaresS changed the title ~~[enhancement] add "--buildkit" flag and optional fields in daemon.json to enable buildkit~~ [enhancement] add optional fields in daemon.json to enable buildkit Aug 7, 2018

AntaresS force-pushed the add-enable-buildkit branch from 21e79dd to e5bf6be Compare August 7, 2018 20:38

cpuguy83 requested changes Aug 8, 2018

View reviewed changes

tiborvass reviewed Aug 9, 2018

View reviewed changes

daemon/config/config.go Outdated

Copy link

Contributor

tiborvass Aug 9, 2018

Choose a reason for hiding this comment

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

Let's make this FeatureBuildKit and json:"feature-buildkit"

tiborvass reviewed Aug 9, 2018

View reviewed changes

AntaresS force-pushed the add-enable-buildkit branch 3 times, most recently from 86ce496 to f85b725 Compare August 9, 2018 17:24

AntaresS force-pushed the add-enable-buildkit branch 2 times, most recently from 614248d to 72eb175 Compare August 17, 2018 21:39

AntaresS force-pushed the add-enable-buildkit branch 2 times, most recently from df5643f to fc627b4 Compare August 18, 2018 00:50

tiborvass reviewed Aug 19, 2018

View reviewed changes

client/ping.go Outdated

Copy link

Contributor

tiborvass Aug 19, 2018

Choose a reason for hiding this comment

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

wrong header in comment

AntaresS force-pushed the add-enable-buildkit branch from fc627b4 to 66a2eb1 Compare August 19, 2018 07:29

tiborvass approved these changes Aug 19, 2018

View reviewed changes

thaJeztah requested changes Aug 19, 2018

View reviewed changes

thaJeztah added area/builder Build impact/api labels Aug 19, 2018

thaJeztah reviewed Aug 19, 2018

View reviewed changes

AntaresS force-pushed the add-enable-buildkit branch from 66a2eb1 to d9019e0 Compare August 19, 2018 21:50

add optional fields in daemon.json to enable buildkit

2be1766

Signed-off-by: Anda Xu <[email protected]>

AntaresS force-pushed the add-enable-buildkit branch from d9019e0 to 2be1766 Compare August 19, 2018 21:58

tiborvass added status/2-code-review and removed status/1-design-review labels Aug 20, 2018

thaJeztah approved these changes Aug 20, 2018

View reviewed changes

thaJeztah merged commit 9916827 into moby:master Aug 20, 2018

andrewhsu mentioned this pull request Aug 21, 2018

remove experimental guard for buildkit #37686

Merged

AntaresS mentioned this pull request Aug 23, 2018

update docs with the new features option in daemon.json docker/cli#1307

Merged

AntaresS mentioned this pull request Oct 5, 2018

[Enhancement] add buildkit enable nob in docker daemon #37587

Closed

thaJeztah changed the title ~~[enhancement] add optional fields in daemon.json to enable buildkit~~ [enhancement] add optional (features) fields in daemon.json to enable buildkit Feb 28, 2019

thaJeztah mentioned this pull request Feb 28, 2019

Start containers in their own cgroup namespaces #38377

Merged

thaJeztah mentioned this pull request Apr 3, 2019

Add undocumented /grpc endpoint and register BuildKit's controller #38990

Merged

vvoland mentioned this pull request Jul 27, 2022

Add Features in /info and add check for containerd feature flag rumpl/moby#26

Closed

Conversation

AntaresS commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AntaresS commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AntaresS commented Aug 7, 2018

Uh oh!

tonistiigi commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AntaresS commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AntaresS commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AntaresS Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 17, 2018

Uh oh!

tiborvass commented Aug 17, 2018

Uh oh!

AntaresS commented Aug 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass left a comment

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Aug 19, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AntaresS commented Aug 6, 2018 •

edited

Loading

codecov bot commented Aug 6, 2018 •

edited

Loading

AntaresS commented Aug 6, 2018 •

edited

Loading

AntaresS commented Aug 7, 2018 •

edited

Loading

AntaresS commented Aug 7, 2018 •

edited

Loading

AntaresS Aug 9, 2018 •

edited

Loading