From 7ab3dc080ba3c590add9299e36c5496e6040a84e Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Tue, 28 Jun 2022 10:03:50 +0100 Subject: [PATCH 1/3] kubernetes: error about unused endpoint argument Signed-off-by: Justin Chadwell --- commands/create.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/commands/create.go b/commands/create.go index 87b53a80..364c2bcc 100644 --- a/commands/create.go +++ b/commands/create.go @@ -166,6 +166,9 @@ func runCreate(dockerCli command.Cli, in createOptions, args []string) error { } else { switch { case driverName == "kubernetes": + if len(args) > 0 { + logrus.Warnf("kubernetes driver does not support endpoint args %q", args[0]) + } // naming endpoint to make --append works ep = (&url.URL{ Scheme: driverName, From 3f5974b7f9db2ca4fddad5fd5cc9d9efa2aa06a9 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Tue, 28 Jun 2022 10:16:18 +0100 Subject: [PATCH 2/3] buildx: forbid mismatched drivers This patch reorders+refactors the runCreate function to ensure that we can detect and notify the user in the scenario that the user attempts to combine multiple drivers in a single builder, which is an unsupported scenario. Previously, we would just overwrite the previous builder with the new driver, potentially invalidating the already existing nodes. Signed-off-by: Justin Chadwell --- commands/create.go | 68 ++++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/commands/create.go b/commands/create.go index 364c2bcc..a4e81d2e 100644 --- a/commands/create.go +++ b/commands/create.go @@ -61,32 +61,6 @@ func runCreate(dockerCli command.Cli, in createOptions, args []string) error { } } - buildkitHost := os.Getenv("BUILDKIT_HOST") - - driverName := in.driver - if driverName == "" { - if len(args) == 0 && buildkitHost != "" { - driverName = "remote" - } else { - var arg string - if len(args) > 0 { - arg = args[0] - } - f, err := driver.GetDefaultFactory(ctx, arg, dockerCli.Client(), true) - if err != nil { - return err - } - if f == nil { - return errors.Errorf("no valid drivers found") - } - driverName = f.Name() - } - } - - if driver.GetFactory(driverName, true) == nil { - return errors.Errorf("failed to find driver %q", in.driver) - } - txn, release, err := storeutil.GetStore(dockerCli) if err != nil { return err @@ -121,17 +95,48 @@ func runCreate(dockerCli command.Cli, in createOptions, args []string) error { logrus.Warnf("failed to find %q for append, creating a new instance instead", in.name) } if in.actionLeave { - return errors.Errorf("failed to find instance %q for leave", name) + return errors.Errorf("failed to find instance %q for leave", in.name) } } else { return err } } + buildkitHost := os.Getenv("BUILDKIT_HOST") + + driverName := in.driver + if driverName == "" { + if ng != nil { + driverName = ng.Driver + } else if len(args) == 0 && buildkitHost != "" { + driverName = "remote" + } else { + var arg string + if len(args) > 0 { + arg = args[0] + } + f, err := driver.GetDefaultFactory(ctx, arg, dockerCli.Client(), true) + if err != nil { + return err + } + if f == nil { + return errors.Errorf("no valid drivers found") + } + driverName = f.Name() + } + } + if ng != nil { if in.nodeName == "" && !in.actionAppend { - return errors.Errorf("existing instance for %s but no append mode, specify --node to make changes for existing instances", name) + return errors.Errorf("existing instance for %q but no append mode, specify --node to make changes for existing instances", name) } + if driverName != ng.Driver { + return errors.Errorf("existing instance for %q but has mismatched driver %q", name, ng.Driver) + } + } + + if driver.GetFactory(driverName, true) == nil { + return errors.Errorf("failed to find driver %q", driverName) } ngOriginal := ng @@ -141,14 +146,11 @@ func runCreate(dockerCli command.Cli, in createOptions, args []string) error { if ng == nil { ng = &store.NodeGroup{ - Name: name, + Name: name, + Driver: driverName, } } - if ng.Driver == "" || in.driver != "" { - ng.Driver = driverName - } - var flags []string if in.flags != "" { flags, err = shlex.Split(in.flags) From ef0cbf20f492221eaaedc764bf9f441dab97df3d Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Wed, 27 Jul 2022 12:06:47 +0100 Subject: [PATCH 3/3] buildx: warn on editing nodes Previously, editing nodes to contain a new set of driver options or config files was unsupported, and silently dropping them. In this patch, we update with these, as well as add a new warning message that any new options may not taken into account until the builder restarts (which may apply to the flags, platforms and endpoints as well). Signed-off-by: Justin Chadwell --- commands/create.go | 3 +++ store/nodegroup.go | 35 +++++++++++++++++++++++++++-------- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/commands/create.go b/commands/create.go index a4e81d2e..074b309e 100644 --- a/commands/create.go +++ b/commands/create.go @@ -320,6 +320,9 @@ func createCmd(dockerCli command.Cli) *cobra.Command { } func csvToMap(in []string) (map[string]string, error) { + if len(in) == 0 { + return nil, nil + } m := make(map[string]string, len(in)) for _, s := range in { csvReader := csv.NewReader(strings.NewReader(s)) diff --git a/store/nodegroup.go b/store/nodegroup.go index b6288e09..0a31331c 100644 --- a/store/nodegroup.go +++ b/store/nodegroup.go @@ -8,6 +8,7 @@ import ( "github.com/docker/buildx/util/platformutil" specs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) type NodeGroup struct { @@ -59,17 +60,42 @@ func (ng *NodeGroup) Update(name, endpoint string, platforms []string, endpoints return err } + var files map[string][]byte + if configFile != "" { + files, err = confutil.LoadConfigFiles(configFile) + if err != nil { + return err + } + } + if i != -1 { n := ng.Nodes[i] + needsRestart := false if endpointsSet { n.Endpoint = endpoint + needsRestart = true } if len(platforms) > 0 { n.Platforms = pp } if flags != nil { n.Flags = flags + needsRestart = true + } + if do != nil { + n.DriverOpts = do + needsRestart = true + } + if configFile != "" { + for k, v := range files { + n.Files[k] = v + } + needsRestart = true + } + if needsRestart { + logrus.Warn("new settings may not be used until builder is restarted") } + ng.Nodes[i] = n if err := ng.validateDuplicates(endpoint, i); err != nil { return err @@ -92,14 +118,7 @@ func (ng *NodeGroup) Update(name, endpoint string, platforms []string, endpoints Platforms: pp, Flags: flags, DriverOpts: do, - } - - if configFile != "" { - files, err := confutil.LoadConfigFiles(configFile) - if err != nil { - return err - } - n.Files = files + Files: files, } ng.Nodes = append(ng.Nodes, n)