10BC0 [enhancement] add optional (features) fields in daemon.json to enable buildkit by AntaresS · Pull Request #37593 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

[enhancement] add optional (features) fields in daemon.json to enable buildkit #37593

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 20, 2018
Merged

[enhancement] add optional (features) fields in daemon.json to enable buildkit #37593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions api/server/router/build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
package build // import "github.com/docker/docker/api/server/router/build"

import "github.com/docker/docker/api/server/router"
import (
"github.com/docker/docker/api/server/router"
"github.com/docker/docker/api/types"
)

// buildRouter is a router to talk with the build controller
type buildRouter struct {
backend Backend
daemon experimentalProvider
routes []router.Route
backend Backend
daemon experimentalProvider
routes []router.Route
builderVersion types.BuilderVersion
}

// NewRouter initializes a new build router
func NewRouter(b Backend, d experimentalProvider) router.Router {
r := &buildRouter{backend: b, daemon: d}
func NewRouter(b Backend, d experimentalProvider, bv types.BuilderVersion) router.Router {
r := &buildRouter{backend: b, daemon: d, builderVersion: bv}
r.initRoutes()
return r
}
Expand Down
6 changes: 4 additions & 2 deletions api/server/router/build/build_routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,10 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
return errdefs.InvalidParameter(errors.New("squash is only supported with experimental mode"))
}

if buildOptions.Version == types.BuilderBuildKit && !br.daemon.HasExperimental() {
return errdefs.InvalidParameter(errors.New("buildkit is only supported with experimental mode"))
// check if the builder feature has been enabled from daemon as well.
if buildOptions.Version == types.BuilderBuildKit &&
(br.builderVersion != types.BuilderBuildKit || !br.daemon.HasExperimental()) {
return errdefs.InvalidParameter(errors.New("buildkit is not enabled on daemon"))
}

out := io.Writer(output)
Expand Down
25 changes: 14 additions & 11 deletions api/server/router/system/system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,35 @@ package system // import "github.com/docker/docker/api/server/router/system"

import (
"github.com/docker/docker/api/server/router"
"github.com/docker/docker/api/types"
buildkit "github.com/docker/docker/builder/builder-next"
"github.com/docker/docker/builder/fscache"
)

// systemRouter provides information about the Docker system overall.
// It gathers information about host, daemon and container events.
type systemRouter struct {
backend Backend
cluster ClusterBackend
routes []router.Route
fscache *fscache.FSCache // legacy
builder *buildkit.Builder
backend Backend
cluster ClusterBackend
routes []router.Route
fscache *fscache.FSCache // legacy
builder *buildkit.Builder
builderVersion types.BuilderVersion
}

// NewRouter initializes a new system router
func NewRouter(b Backend, c ClusterBackend, fscache *fscache.FSCache, builder *buildkit.Builder) router.Router {
func NewRouter(b Backend, c ClusterBackend, fscache *fscache.FSCache, builder *buildkit.Builder, bv types.BuilderVersion) router.Router {
r := &systemRouter{
backend: b,
cluster: c,
fscache: fscache,
builder: builder,
backend: b,
cluster: c,
fscache: fscache,
builder: builder,
builderVersion: bv,
}

r.routes = []router.Route{
router.NewOptionsRoute("/{anyroute:.*}", optionsHandler),
router.NewGetRoute("/_ping", pingHandler),
router.NewGetRoute("/_ping", r.pingHandler),
router.NewGetRoute("/events", r.getEvents, router.WithCancel),
router.NewGetRoute("/info", r.getInfo),
router.NewGetRoute("/version", r.getVersion),
Expand Down
5 changes: 4 additions & 1 deletion api/server/router/system/system_routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ func optionsHandler(ctx context.Context, w http.ResponseWriter, r *http.Request,
return nil
}

func pingHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
func (s *systemRouter) pingHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
if bv := s.builderVersion; bv != "" {
w.Header().Set("Builder-Version", string(bv))
}
_, err := w.Write([]byte{'O', 'K'})
return err
}
Expand Down
3 changes: 3 additions & 0 deletions api/swagger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6972,6 +6972,9 @@ paths:
API-Version:
type: "string"
description: "Max API Version the server supports"
BuildKit-Version:
type: "string"
description: "Default version of docker image builder"
Docker-Experimental:
type: "boolean"
description: "If the server is running with experimental mode enabled"
Expand Down
7 changes: 4 additions & 3 deletions api/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,10 @@ type ContainerStats struct {
// Ping contains response of Engine API:
// GET "/_ping"
type Ping struct {
APIVersion string
OSType string
Experimental bool
APIVersion string
OSType string
Experimental bool
BuilderVersion BuilderVersion
}

// ComponentVersion describes the version information for a specific component.
Expand Down
5 changes: 4 additions & 1 deletion client/ping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"github.com/docker/docker/api/types"
)

// Ping pings the server and returns the value of the "Docker-Experimental", "OS-Type" & "API-Version" headers
// Ping pings the server and returns the value of the "Docker-Experimental", "Builder-Version", "OS-Type" & "API-Version" headers
func (cli *Client) Ping(ctx context.Context) (types.Ping, error) {
var ping types.Ping
req, err := cli.buildRequest("GET", path.Join(cli.basePath, "/_ping"), nil, nil)
Expand All @@ -27,6 +27,9 @@ func (cli *Client) Ping(ctx context.Context) (types.Ping, error) {
ping.Experimental = true
}
ping.OSType = serverResp.header.Get("OSType")
if bv := serverResp.header.Get("Builder-Version"); bv != "" {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ping.BuilderVersion = types.BuilderVersion(bv)
}
}
return ping, cli.checkResponseErr(serverResp)
}
1 change: 0 additions & 1 deletion cmd/dockerd/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ func installCommonConfigFlags(conf *config.Config, flags *pflag.FlagSet) {

flags.StringVar(&conf.SwarmDefaultAdvertiseAddr, "swarm-default-advertise-addr", "", "Set default address or interface for swarm advertised address")
flags.BoolVar(&conf.Experimental, "experimental", false, "Enable experimental features")

Copy link
Member

Choose a reason for hiding this comment

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

unrelated change?

flags.StringVar(&conf.MetricsAddress, "metrics-addr", "", "Set default address and port to serve the metrics api on")

flags.Var(opts.NewNamedListOptsRef("node-generic-resources", &conf.NodeGenericResources, opts.ValidateSingleGenericResource), "node-generic-resource", "Advertise user-defined resource")
Expand Down
23 changes: 16 additions & 7 deletions cmd/dockerd/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
swarmrouter "github.com/docker/docker/api/server/router/swarm"
systemrouter "github.com/docker/docker/api/server/router/system"
"github.com/docker/docker/api/server/router/volume"
"github.com/docker/docker/api/types"
buildkit "github.com/docker/docker/builder/builder-next"
"github.com/docker/docker/builder/dockerfile"
"github.com/docker/docker/builder/fscache"
Expand Down Expand Up @@ -253,6 +254,7 @@ type routerOptions struct {
buildBackend *buildbackend.Backend
buildCache *fscache.FSCache // legacy
buildkit *buildkit.Builder
builderVersion types.BuilderVersion
daemon *daemon.Daemon
api *apiserver.Server
cluster *cluster.Cluster
Expand Down Expand Up @@ -283,8 +285,7 @@ func newRouterOptions(config *config.Config, daemon *daemon.Daemon) (routerOptio
if err != nil {
return opts, err
}

buildkit, err := buildkit.New(buildkit.Opt{
bk, err := buildkit.New(buildkit.Opt{
SessionManager: sm,
Root: filepath.Join(config.Root, "buildkit"),
Dist: daemon.DistributionServices(),
Expand All @@ -293,16 +294,24 @@ func newRouterOptions(config *config.Config, daemon *daemon.Daemon) (routerOptio
return opts, err
}

bb, err := buildbackend.NewBackend(daemon.ImageService(), manager, buildCache, buildkit)
bb, err := buildbackend.NewBackend(daemon.ImageService(), manager, buildCache, bk)
if err != nil {
return opts, errors.Wrap(err, "failed to create buildmanager")
}

var bv types.BuilderVersion
if v, ok := config.Features["buildkit"]; ok {
if v {
bv = types.BuilderBuildKit
} else {
bv = types.BuilderV1
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

@thaJeztah yes. So it doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

Got it

}
}
return routerOptions{
sessionManager: sm,
buildBackend: bb,
buildCache: buildCache,
buildkit: buildkit,
buildkit: bk,
builderVersion: bv,
daemon: daemon,
}, nil
}
Expand Down Expand Up @@ -476,9 +485,9 @@ func initRouter(opts routerOptions) {
checkpointrouter.NewRouter(opts.daemon, decoder),
container.NewRouter(opts.daemon, decoder),
image.NewRouter(opts.daemon.ImageService()),
systemrouter.NewRouter(opts.daemon, opts.cluster, opts.buildCache, opts.buildkit),
systemrouter.NewRouter(opts.daemon, opts.cluster, opts.buildCache, opts.buildkit, opts.builderVersion),
volume.NewRouter(opts.daemon.VolumesService()),
build.NewRouter(opts.buildBackend, opts.daemon),
build.NewRouter(opts.buildBackend, opts.daemon, opts.builderVersion),
sessionrouter.NewRouter(opts.sessionManager),
swarmrouter.NewRouter(opts.cluster),
pluginrouter.NewRouter(opts.daemon.PluginManager()),
Expand Down
13 changes: 12 additions & 1 deletion daemon/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ var flatOptions = map[string]bool{
"default-ulimits": true,
}

// skipValidateOptions contains configuration keys
// that will be skipped from findConfigurationConflicts
// for unknown flag validation.
var skipValidateOptions = map[string]bool{
"features": true,
}

// LogConfig represents the default log configuration.
// It includes json tags to deserialize configuration from a file
// using the same names that the flags in the command line use.
Expand Down Expand Up @@ -203,6 +210,10 @@ type CommonConfig struct {
// should be configured with the CRI plugin enabled. This allows using
// Docker's containerd instance directly with a Kubernetes kubelet.
CriContainerd bool `json:"cri-containerd,omitempty"`

// Features contains a list of feature key value pairs indicating what features are enabled or disabled.
// If a certain feature doesn't appear in this list then it's unset (i.e. neither true nor false).
Features map[string]bool `json:"features,omitempty"`
}

// IsValueSet returns true if a configuration value
Expand Down Expand Up @@ -444,7 +455,7 @@ func findConfigurationConflicts(config map[string]interface{}, flags *pflag.Flag
// 1. Search keys from the file that we don't recognize as flags.
unknownKeys := make(map[string]interface{})
for key, value := range config {
if flag := flags.Lookup(key); flag == nil {
if flag := flags.Lookup(key); flag == nil && !skipValidateOptions[key] {
unknownKeys[key] = value
}
}
Expand Down
0