Merge pull request #1188 from jedevc/driver-opt-warnings

Introduce new errors for unsupported driver behaviors
docker · Aug 5, 2022 · 7b660c4 · 7b660c4
2 parents 18023d7 + ef0cbf2
commit 7b660c4
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 41 deletions.
diff --git 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,34 +95,62 @@ 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
 	if ngOriginal != nil {
 		ngOriginal = ngOriginal.Copy()
 	}
 
 	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)
@@ -166,6 +168,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,
@@ -315,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
@@ -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)