Merge pull request #51492 from vvoland/c8d-fix-selection

daemon: Fix image store choice priority for prior graphdriver state

daemon/daemon.go

@@ -873,7 +873,7 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S
 	}
 	d.configStore.Store(cfgStore)
 
-	imgStoreChoice, err := determineImageStoreChoice(config)
+	imgStoreChoice, err := determineImageStoreChoice(config, determineImageStoreChoiceOptions{})
 	if err != nil {
 		return nil, err
 	}
@@ -1096,7 +1096,7 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S
 		return nil, err
 	}
 
-	driverName := chooseDriver(ctx, cfgStore.GraphDriver, imgStoreChoice)
+	driverName := getDriverOverride(ctx, cfgStore.GraphDriver, imgStoreChoice)
 
 	var migrationConfig migration.Config
 	if imgStoreChoice.IsGraphDriver() {

daemon/image_store_choice.go

@@ -48,14 +48,14 @@ func (c imageStoreChoice) IsExplicit() bool {
 	}
 }
 
-// chooseDriver determines the storage driver name based on environment variables,
+// getDriverOverride determines the storage driver name based on environment variables,
 // configuration, and platform-specific logic.
 // On Windows we don't support the environment variable, or a user supplied graphdriver,
 // but it is allowed when using snapshotters.
 // Unix platforms however run a single graphdriver for all containers, and it can
 // be set through an environment variable, a daemon start parameter, or chosen through
 // initialization of the layerstore through driver priority order for example.
-func chooseDriver(ctx context.Context, cfgGraphDriver string, imgStoreChoice imageStoreChoice) string {
+func getDriverOverride(ctx context.Context, cfgGraphDriver string, imgStoreChoice imageStoreChoice) string {
 	driverName := os.Getenv("DOCKER_DRIVER")
 	if driverName == "" {
 		driverName = cfgGraphDriver
@@ -85,9 +85,25 @@ func chooseDriver(ctx context.Context, cfgGraphDriver string, imgStoreChoice ima
 	return driverName
 }
 
-func determineImageStoreChoice(cfgStore *config.Config) (imageStoreChoice, error) {
+type determineImageStoreChoiceOptions struct {
+	hasPriorDriver          func(root string) bool
+	isRegisteredGraphdriver func(driverName string) bool
+	runtimeOS               string
+}
+
+func determineImageStoreChoice(cfgStore *config.Config, opts determineImageStoreChoiceOptions) (imageStoreChoice, error) {
+	if opts.hasPriorDriver == nil {
+		opts.hasPriorDriver = graphdriver.HasPriorDriver
+	}
+	if opts.isRegisteredGraphdriver == nil {
+		opts.isRegisteredGraphdriver = graphdriver.IsRegistered
+	}
+	if opts.runtimeOS == "" {
+		opts.runtimeOS = runtime.GOOS
+	}
+
 	out := imageStoreChoiceContainerd
-	if runtime.GOOS == "windows" {
+	if opts.runtimeOS == "windows" {
 		out = imageStoreChoiceGraphdriver
 	}
 
@@ -111,44 +127,27 @@ func determineImageStoreChoice(cfgStore *config.Config) (imageStoreChoice, error
 		out = imageStoreChoiceGraphdriverExplicit
 	}
 
-	if driverName != "" {
-		if !out.IsExplicit() {
-			switch driverName {
-			case "vfs", "overlay2":
-				out = imageStoreChoiceGraphdriverExplicit
-			case "btrfs":
-				// The btrfs driver is not heavily used in containerd and has no
-				// advantage over overlayfs anymore since overlay works fine.
-				// If btrfs is explicitly chosen, the user most likely means graphdrivers.
-				out = imageStoreChoiceGraphdriverExplicit
-			}
-		}
-		if out.IsGraphDriver() {
-			if graphdriver.IsRegistered(driverName) {
-				return imageStoreChoiceGraphdriverExplicit, nil
-			} else {
-				return imageStoreChoiceGraphdriverExplicit, fmt.Errorf("graphdriver is explicitly enabled but %q is not registered, %v %v", driverName, cfgStore.Features, os.Getenv("TEST_INTEGRATION_USE_GRAPHDRIVER"))
+	if out == imageStoreChoiceContainerd {
+		if opts.hasPriorDriver(cfgStore.Root) {
+			return imageStoreChoiceGraphdriverPrior, nil
 		}
 	}
 
-		if runtime.GOOS == "windows" && !out.IsExplicit() {
-			switch driverName {
-			case "windows":
-				return imageStoreChoiceContainerdExplicit, nil
-			case "windowsfilter":
+	if driverName != "" {
+		if !out.IsExplicit() && opts.isRegisteredGraphdriver(driverName) {
 			return imageStoreChoiceGraphdriverExplicit, nil
 		}
+		if out.IsGraphDriver() {
+			if opts.isRegisteredGraphdriver(driverName) {
+				return imageStoreChoiceGraphdriverExplicit, nil
+			} else if out.IsExplicit() {
+				return imageStoreChoiceGraphdriverExplicit, fmt.Errorf("graphdriver is explicitly enabled but %q is not registered, %v %v", driverName, cfgStore.Features, os.Getenv("TEST_INTEGRATION_USE_GRAPHDRIVER"))
+			}
 		}
 
 		// Assume snapshotter is chosen
 		return imageStoreChoiceContainerdExplicit, nil
 	}
 
-	if out == imageStoreChoiceContainerd {
-		if graphdriver.HasPriorDriver(cfgStore.Root) {
-			return imageStoreChoiceGraphdriverPrior, nil
-		}
-	}
-
 	return out, nil
 }

daemon/image_store_choice_test.go

@@ -0,0 +1,269 @@
+package daemon
+
+import (
+	"slices"
+	"testing"
+
+	"github.com/moby/moby/v2/daemon/config"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+func TestDetermineImageStoreChoice(t *testing.T) {
+	str := func(s string) *string {
+		return &s
+	}
+
+	type testCase struct {
+		name                  string
+		envDockerDriver       *string
+		envTestUseGraphDriver *string
+		priorGraphDriver      bool
+		cfg                   *config.Config
+		expectedChoice        imageStoreChoice
+		expectError           bool
+		skipPlatform          string
+		onlyPlatform          string
+	}
+
+	tests := []testCase{
+		{
+			name: "containerd-snapshotter feature enabled",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					Features: map[string]bool{
+						"containerd-snapshotter": true,
+					},
+				},
+			},
+			expectedChoice: imageStoreChoiceContainerdExplicit,
+		},
+		{
+			name: "containerd-snapshotter feature disabled",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					Features: map[string]bool{
+						"containerd-snapshotter": false,
+					},
+				},
+			},
+			expectedChoice: imageStoreChoiceGraphdriverExplicit,
+		},
+		{
+			name:                  "TEST_INTEGRATION_USE_GRAPHDRIVER env var set",
+			envTestUseGraphDriver: str("1"),
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					Features: map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceGraphdriverExplicit,
+		},
+		{
+			name:                  "TEST_INTEGRATION_USE_GRAPHDRIVER takes precedence over feature flag",
+			envTestUseGraphDriver: str("1"),
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					Features: map[string]bool{
+						"containerd-snapshotter": true,
+					},
+				},
+			},
+			expectedChoice: imageStoreChoiceGraphdriverExplicit,
+		},
+		{
+			name: "native driver in config",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					GraphDriver: "native",
+					Features:    map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceContainerdExplicit,
+		},
+		{
+			name: "vfs driver in config",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					GraphDriver: "vfs",
+					Features:    map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceGraphdriverExplicit,
+		},
+		{
+			name:            "custom snapshotter",
+			envDockerDriver: str("my-custom-snapshotter"),
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					Features: map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceContainerdExplicit,
+		},
+	}
+
+	nonWindows := []testCase{
+		{
+			name: "default containerd on non-Windows",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					Features: map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceContainerd,
+		},
+	}
+
+	for _, gd := range []string{"fuse-overlayfs", "overlay2", "btrfs", "zfs"} {
+		nonWindows = append(nonWindows, testCase{
+			name: gd + " driver in config",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					GraphDriver: gd,
+					Features:    map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceGraphdriverExplicit,
+		})
+
+		nonWindows = append(nonWindows, testCase{
+			name: gd + " driver in config with prior data",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					GraphDriver: gd,
+					Features:    map[string]bool{},
+				},
+			},
+			priorGraphDriver: true,
+			expectedChoice:   imageStoreChoiceGraphdriverPrior,
+		})
+
+		nonWindows = append(nonWindows, testCase{
+			name: gd + " driver in config with containerd snapshotter feature enabled",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					GraphDriver: gd,
+					Features: map[string]bool{
+						"containerd-snapshotter": true,
+					},
+				},
+			},
+			expectedChoice: imageStoreChoiceContainerdExplicit,
+		})
+
+		nonWindows = append(nonWindows, testCase{
+			name: gd + " driver in config with containerd snapshotter feature disabled",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					GraphDriver: gd,
+					Features: map[string]bool{
+						"containerd-snapshotter": false,
+					},
+				},
+			},
+			expectedChoice: imageStoreChoiceGraphdriverExplicit,
+		})
+
+		nonWindows = append(nonWindows, testCase{
+			name:                  gd + " driver in config with TEST_INTEGRATION_USE_GRAPHDRIVER env var set",
+			envTestUseGraphDriver: str("1"),
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					GraphDriver: gd,
+					Features:    map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceGraphdriverExplicit,
+		})
+	}
+
+	windows := []testCase{
+		{
+			name: "default graphdriver on Windows",
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					Features: map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceGraphdriver,
+		},
+		{
+			name:            "windows driver on Windows",
+			envDockerDriver: str("windows"),
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					Features: map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceContainerdExplicit,
+		},
+		{
+			name:            "windowsfilter driver on Windows",
+			envDockerDriver: str("windowsfilter"),
+			cfg: &config.Config{
+				CommonConfig: config.CommonConfig{
+					Features: map[string]bool{},
+				},
+			},
+			expectedChoice: imageStoreChoiceGraphdriverExplicit,
+		},
+	}
+
+	for i := range nonWindows {
+		nonWindows[i].skipPlatform = "windows"
+	}
+	tests = append(tests, nonWindows...)
+
+	for i := range windows {
+		windows[i].onlyPlatform = "windows"
+	}
+	tests = append(tests, windows...)
+
+	registeredDrivers := []string{"fuse-overlayfs", "overlay2", "btrfs", "zfs", "vfs"}
+	windowsRegisteredDrivers := []string{"vfs", "windowsfilter"}
+
+	for _, os := range []string{"linux", "windows"} {
+		for _, tc := range tests {
+			if tc.skipPlatform != "" && os == tc.skipPlatform {
+				continue
+			}
+			if tc.onlyPlatform != "" && os != tc.onlyPlatform {
+				continue
+			}
+
+			t.Run(os+"/"+tc.name, func(t *testing.T) {
+				if tc.envDockerDriver != nil {
+					t.Setenv("DOCKER_DRIVER", *tc.envDockerDriver)
+				} else {
+					t.Setenv("DOCKER_DRIVER", "")
+				}
+				if tc.envTestUseGraphDriver != nil {
+					t.Setenv("TEST_INTEGRATION_USE_GRAPHDRIVER", *tc.envTestUseGraphDriver)
+				} else {
+					t.Setenv("TEST_INTEGRATION_USE_GRAPHDRIVER", "")
+				}
+
+				choice, err := determineImageStoreChoice(tc.cfg, determineImageStoreChoiceOptions{
+					runtimeOS: os,
+					hasPriorDriver: func(root string) bool {
+						return tc.priorGraphDriver
+					},
+					isRegisteredGraphdriver: func(driverName string) bool {
+						if os == "windows" {
+							return slices.Contains(windowsRegisteredDrivers, driverName)
+						}
+						return slices.Contains(registeredDrivers, driverName)
+					},
+				})
+				if tc.expectError {
+					assert.Error(t, err, "expected an error but got none")
+				} else {
+					assert.NilError(t, err)
+				}
+
+				assert.Check(t, is.Equal(tc.expectedChoice, choice))
+			})
+		}
+	}
+}