From d15224e4a82e07c3ab0406fd24700c90cc9637c0 Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Wed, 3 Aug 2022 11:20:54 +0200 Subject: [PATCH 1/9] Make the snapshotter configurable Signed-off-by: Djordje Lukic --- daemon/config/config.go | 4 ++++ daemon/containerd/image_exporter.go | 4 ++-- daemon/containerd/image_list.go | 2 +- daemon/containerd/image_pull.go | 4 ++-- daemon/containerd/service.go | 18 ++++++++++-------- daemon/create.go | 2 +- daemon/daemon.go | 3 ++- daemon/oci_linux.go | 14 +++++++++++--- daemon/start.go | 2 +- 9 files changed, 34 insertions(+), 19 deletions(-) diff --git a/daemon/config/config.go b/daemon/config/config.go index 645beb8c032f6..25fc5091aba1c 100644 --- a/daemon/config/config.go +++ b/daemon/config/config.go @@ -10,6 +10,7 @@ import ( "strings" "sync" + "github.com/containerd/containerd" "github.com/docker/docker/opts" "github.com/docker/docker/pkg/authorization" "github.com/docker/docker/registry" @@ -51,6 +52,9 @@ const ( // DefaultPluginNamespace is the name of the default containerd namespace used for plugins. DefaultPluginNamespace = "plugins.moby" + // DefaultContainerdSnapshotter is the name of the default containerd snapshotter used for creating container root fs + DefaultContainerdSnapshotter = containerd.DefaultSnapshotter + // LinuxV2RuntimeName is the runtime used to specify the containerd v2 runc shim LinuxV2RuntimeName = "io.containerd.runc.v2" diff --git a/daemon/containerd/image_exporter.go b/daemon/containerd/image_exporter.go index 13e03af5076cf..a681ade3c7167 100644 --- a/daemon/containerd/image_exporter.go +++ b/daemon/containerd/image_exporter.go @@ -48,14 +48,14 @@ func (i *ImageService) LoadImage(ctx context.Context, inTar io.ReadCloser, outSt for _, img := range imgs { platformImg := containerd.NewImageWithPlatform(i.client, img, platform) - unpacked, err := platformImg.IsUnpacked(ctx, containerd.DefaultSnapshotter) + unpacked, err := platformImg.IsUnpacked(ctx, i.snapshotter) if err != nil { logrus.WithError(err).WithField("image", img.Name).Error("IsUnpacked failed") continue } if !unpacked { - err := platformImg.Unpack(ctx, containerd.DefaultSnapshotter) + err := platformImg.Unpack(ctx, i.snapshotter) if err != nil { logrus.WithError(err).WithField("image", img.Name).Error("Failed to unpack image") return errors.Wrapf(err, "Failed to unpack image") diff --git a/daemon/containerd/image_list.go b/daemon/containerd/image_list.go index 05c040a3c7caf..0818d34533478 100644 --- a/daemon/containerd/image_list.go +++ b/daemon/containerd/image_list.go @@ -37,7 +37,7 @@ func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions) return nil, err } - snapshotter := i.client.SnapshotService(containerd.DefaultSnapshotter) + snapshotter := i.client.SnapshotService(i.snapshotter) sizeCache := make(map[digest.Digest]int64) snapshotSizeFn := func(d digest.Digest) (int64, error) { if s, ok := sizeCache[d]; ok { diff --git a/daemon/containerd/image_pull.go b/daemon/containerd/image_pull.go index bcc6e9ff7e48b..dff6bd0a29b0b 100644 --- a/daemon/containerd/image_pull.go +++ b/daemon/containerd/image_pull.go @@ -62,13 +62,13 @@ func (i *ImageService) PullImage(ctx context.Context, image, tagOrDigest string, return err } - unpacked, err := img.IsUnpacked(ctx, containerd.DefaultSnapshotter) + unpacked, err := img.IsUnpacked(ctx, i.snapshotter) if err != nil { return err } if !unpacked { - if err := img.Unpack(ctx, containerd.DefaultSnapshotter); err != nil { + if err := img.Unpack(ctx, i.snapshotter); err != nil { return err } } diff --git a/daemon/containerd/service.go b/daemon/containerd/service.go index bc9ff60a68648..b4bcec46b9339 100644 --- a/daemon/containerd/service.go +++ b/daemon/containerd/service.go @@ -19,16 +19,18 @@ import ( // ImageService implements daemon.ImageService type ImageService struct { - client *containerd.Client - usage singleflight.Group - containers container.Store + client *containerd.Client + usage singleflight.Group + containers container.Store + snapshotter string } // NewService creates a new ImageService. -func NewService(c *containerd.Client, containers container.Store) *ImageService { +func NewService(c *containerd.Client, containers container.Store, snapshotter string) *ImageService { return &ImageService{ - client: c, - containers: containers, + client: c, + containers: containers, + snapshotter: snapshotter, } } @@ -106,7 +108,7 @@ func (i *ImageService) ReleaseLayer(rwlayer layer.RWLayer) error { func (i *ImageService) LayerDiskUsage(ctx context.Context) (int64, error) { ch := i.usage.DoChan("LayerDiskUsage", func() (interface{}, error) { var allLayersSize int64 - snapshotter := i.client.SnapshotService(containerd.DefaultSnapshotter) + snapshotter := i.client.SnapshotService(i.snapshotter) snapshotter.Walk(ctx, func(ctx context.Context, info snapshots.Info) error { usage, err := snapshotter.Usage(ctx, info.Name) if err != nil { @@ -167,7 +169,7 @@ func (i *ImageService) GetLayerFolders(img *image.Image, rwLayer layer.RWLayer) // GetContainerLayerSize returns the real size & virtual size of the container. func (i *ImageService) GetContainerLayerSize(ctx context.Context, containerID string) (int64, int64, error) { - snapshotter := i.client.SnapshotService(containerd.DefaultSnapshotter) + snapshotter := i.client.SnapshotService(i.snapshotter) sizeCache := make(map[digest.Digest]int64) snapshotSizeFn := func(d digest.Digest) (int64, error) { if s, ok := sizeCache[d]; ok { diff --git a/daemon/create.go b/daemon/create.go index 5b2ec16c0d6f1..f75bd0bc124c7 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -189,7 +189,7 @@ func (daemon *Daemon) create(ctx context.Context, opts createOpts) (retC *contai return nil, err } parent := identity.ChainID(diffIDs).String() - s := daemon.containerdCli.SnapshotService(containerd.DefaultSnapshotter) + s := daemon.containerdCli.SnapshotService(daemon.graphDriver) if _, err := s.Prepare(ctx, ctr.ID, parent); err != nil { return nil, err } diff --git a/daemon/daemon.go b/daemon/daemon.go index 45e2f0f675d38..5baf464489afa 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -1027,7 +1027,8 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S d.linkIndex = newLinkIndex() if d.UsesSnapshotter() { - d.imageService = ctrd.NewService(d.containerdCli, d.containers) + d.graphDriver = d.configStore.GraphDriver + d.imageService = ctrd.NewService(d.containerdCli, d.containers, d.graphDriver) } else { ifs, err := image.NewFSStoreBackend(filepath.Join(imageRoot, "imagedb")) if err != nil { diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go index 8a61da2c2cebb..5bdd8dccfcd10 100644 --- a/daemon/oci_linux.go +++ b/daemon/oci_linux.go @@ -11,7 +11,6 @@ import ( "strings" cdcgroups "github.com/containerd/cgroups" - "github.com/containerd/containerd" "github.com/containerd/containerd/containers" coci "github.com/containerd/containerd/oci" "github.com/containerd/containerd/pkg/apparmor" @@ -1061,10 +1060,19 @@ func (daemon *Daemon) createSpec(ctx context.Context, c *container.Container) (r if daemon.configStore.Rootless { opts = append(opts, WithRootless(daemon)) } + + snapshotter := "" + snapshotKey := "" + if daemon.UsesSnapshotter() { + snapshotter = daemon.graphDriver + snapshotKey = c.ID + + } + return &s, coci.ApplyOpts(context.Background(), nil, &containers.Container{ ID: c.ID, - Snapshotter: containerd.DefaultSnapshotter, - SnapshotKey: c.ID, + Snapshotter: snapshotter, + SnapshotKey: snapshotKey, }, &s, opts...) } diff --git a/daemon/start.go b/daemon/start.go index f8b5e7ed2bf06..44c8c392805f0 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -179,7 +179,7 @@ func (daemon *Daemon) containerStart(ctx context.Context, container *container.C newContainerOpts := []containerd.NewContainerOpts{} if daemon.UsesSnapshotter() { - newContainerOpts = append(newContainerOpts, containerd.WithSnapshotter(containerd.DefaultSnapshotter)) + newContainerOpts = append(newContainerOpts, containerd.WithSnapshotter(daemon.graphDriver)) newContainerOpts = append(newContainerOpts, containerd.WithSnapshot(container.ID)) } From b6770ed7659f4ec91c34a61cea9aa09687d789aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Fri, 5 Aug 2022 18:09:59 +0200 Subject: [PATCH 2/9] c8d/daemon: Treat (storage/graph)Driver as snapshotter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also moved some layerStore related initialization to the non-c8d case because otherwise they get treated as a graphdriver plugins. Signed-off-by: Paweł Gronowski --- daemon/containerd/snapshotters.go | 25 ++++++ daemon/containerd/snapshotters_test.go | 60 +++++++++++++ daemon/daemon.go | 113 +++++++++++++------------ daemon/daemon_unix.go | 2 +- 4 files changed, 144 insertions(+), 56 deletions(-) create mode 100644 daemon/containerd/snapshotters.go create mode 100644 daemon/containerd/snapshotters_test.go diff --git a/daemon/containerd/snapshotters.go b/daemon/containerd/snapshotters.go new file mode 100644 index 0000000000000..97d5c9e5a2bc0 --- /dev/null +++ b/daemon/containerd/snapshotters.go @@ -0,0 +1,25 @@ +package containerd + +import ( + "strings" + + "github.com/containerd/containerd" +) + +// snapshotterFromGraphDriver returns the containerd snapshotter name based on the supplied graphdriver name. +// It handles both legacy names and translates them into corresponding containerd snapshotter names. +func SnapshotterFromGraphDriver(graphDriver string) string { + if graphDriver == "" { + graphDriver = containerd.DefaultSnapshotter + } + + switch graphDriver { + case "overlay", "overlay2": + graphDriver = "overlayfs" + case "windowsgraph": + graphDriver = "windows" + } + + graphDriver = strings.TrimPrefix(graphDriver, "io.containerd.snapshotter.v1.") + return graphDriver +} diff --git a/daemon/containerd/snapshotters_test.go b/daemon/containerd/snapshotters_test.go new file mode 100644 index 0000000000000..4d7e44697e5eb --- /dev/null +++ b/daemon/containerd/snapshotters_test.go @@ -0,0 +1,60 @@ +package containerd + +import ( + "testing" + + "github.com/containerd/containerd" +) + +func TestSnapshotterFromGraphDriver(t *testing.T) { + testCases := []struct { + desc string + input string + output string + }{ + { + desc: "empty defaults to containerd default", + input: "", + output: containerd.DefaultSnapshotter, + }, + { + desc: "overlay -> overlayfs", + input: "overlay", + output: "overlayfs", + }, + { + desc: "overlay2 -> overlayfs", + input: "overlay2", + output: "overlayfs", + }, + { + desc: "windowsgraph -> windows", + input: "windowsgraph", + output: "windows", + }, + { + desc: "containerd overlayfs", + input: "io.containerd.snapshotter.v1.overlayfs", + output: "overlayfs", + }, + { + desc: "containerd zfs", + input: "io.containerd.snapshotter.v1.zfs", + output: "zfs", + }, + { + desc: "unknown is unchanged", + input: "somefuturesnapshotter", + output: "somefuturesnapshotter", + }, + } + for _, tC := range testCases { + want := tC.output + t.Run(tC.desc, func(t *testing.T) { + got := SnapshotterFromGraphDriver(tC.input) + if want != got { + t.Errorf("Expected sanitizeGraphDriver to return %q, got %q", want, got) + } + }) + } +} diff --git a/daemon/daemon.go b/daemon/daemon.go index 5baf464489afa..daac2be00c8a6 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -838,21 +838,6 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S } } - if isWindows { - // On Windows we don't support the environment variable, or a user supplied graphdriver - d.graphDriver = "windowsfilter" - } else { - // 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. - if drv := os.Getenv("DOCKER_DRIVER"); drv != "" { - d.graphDriver = drv - logrus.Infof("Setting the storage driver from the $DOCKER_DRIVER environment variable (%s)", drv) - } else { - d.graphDriver = config.GraphDriver // May still be empty. Layerstore init determines instead. - } - } - d.registryService = registryService logger.RegisterPluginGetter(d.PluginStore) @@ -942,28 +927,6 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S return nil, err } - layerStore, err := layer.NewStoreFromOptions(layer.StoreOptions{ - Root: config.Root, - MetadataStorePathTemplate: filepath.Join(config.Root, "image", "%s", "layerdb"), - GraphDriver: d.graphDriver, - GraphDriverOptions: config.GraphOptions, - IDMapping: idMapping, - PluginGetter: d.PluginStore, - ExperimentalEnabled: config.Experimental, - }) - if err != nil { - return nil, err - } - - // As layerstore initialization may set the driver - d.graphDriver = layerStore.DriverName() - - // Configure and validate the kernels security support. Note this is a Linux/FreeBSD - // operation only, so it is safe to pass *just* the runtime OS graphdriver. - if err := configureKernelSecuritySupport(config, d.graphDriver); err != nil { - return nil, err - } - imageRoot := filepath.Join(config.Root, "image", d.graphDriver) d.volumes, err = volumesservice.NewVolumeService(config.Root, d.PluginStore, rootIDs, d) @@ -979,23 +942,6 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S logrus.WithError(err).Warnf("unable to migrate engine ID; a new engine ID will be generated") } - // We have a single tag/reference store for the daemon globally. However, it's - // stored under the graphdriver. On host platforms which only support a single - // container OS, but multiple selectable graphdrivers, this means depending on which - // graphdriver is chosen, the global reference store is under there. For - // platforms which support multiple container operating systems, this is slightly - // more problematic as where does the global ref store get located? Fortunately, - // for Windows, which is currently the only daemon supporting multiple container - // operating systems, the list of graphdrivers available isn't user configurable. - // For backwards compatibility, we just put it under the windowsfilter - // directory regardless. - refStoreLocation := filepath.Join(imageRoot, `repositories.json`) - rs, err := refstore.NewReferenceStore(refStoreLocation) - if err != nil { - return nil, fmt.Errorf("Couldn't create reference store repository: %s", err) - } - d.ReferenceStore = rs - // Check if Devices cgroup is mounted, it is hard requirement for container security, // on Linux. // @@ -1026,8 +972,26 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S d.linkIndex = newLinkIndex() + // On Windows we don't support the environment variable, or a user supplied graphdriver + // 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. + graphDriver := os.Getenv("DOCKER_DRIVER") + if isWindows { + graphDriver = "windowsfilter" + } else if graphDriver != "" { + logrus.Infof("Setting the storage driver from the $DOCKER_DRIVER environment variable (%s)", graphDriver) + } else { + graphDriver = config.GraphDriver + } + if d.UsesSnapshotter() { - d.graphDriver = d.configStore.GraphDriver + d.graphDriver = ctrd.SnapshotterFromGraphDriver(graphDriver) + // Configure and validate the kernels security support. Note this is a Linux/FreeBSD + // operation only, so it is safe to pass *just* the runtime OS graphdriver. + if err := configureKernelSecuritySupport(config, graphDriver); err != nil { + return nil, err + } d.imageService = ctrd.NewService(d.containerdCli, d.containers, d.graphDriver) } else { ifs, err := image.NewFSStoreBackend(filepath.Join(imageRoot, "imagedb")) @@ -1035,6 +999,45 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S return nil, err } + layerStore, err := layer.NewStoreFromOptions(layer.StoreOptions{ + Root: config.Root, + MetadataStorePathTemplate: filepath.Join(config.Root, "image", "%s", "layerdb"), + GraphDriver: graphDriver, + GraphDriverOptions: config.GraphOptions, + IDMapping: idMapping, + PluginGetter: d.PluginStore, + ExperimentalEnabled: config.Experimental, + }) + if err != nil { + return nil, err + } + + // As layerstore initialization may set the driver + d.graphDriver = layerStore.DriverName() + + // Configure and validate the kernels security support. Note this is a Linux/FreeBSD + // operation only, so it is safe to pass *just* the runtime OS graphdriver. + if err := configureKernelSecuritySupport(config, d.graphDriver); err != nil { + return nil, err + } + + // We have a single tag/reference store for the daemon globally. However, it's + // stored under the graphdriver. On host platforms which only support a single + // container OS, but multiple selectable graphdrivers, this means depending on which + // graphdriver is chosen, the global reference store is under there. For + // platforms which support multiple container operating systems, this is slightly + // more problematic as where does the global ref store get located? Fortunately, + // for Windows, which is currently the only daemon supporting multiple container + // operating systems, the list of graphdrivers available isn't user configurable. + // For backwards compatibility, we just put it under the windowsfilter + // directory regardless. + refStoreLocation := filepath.Join(imageRoot, `repositories.json`) + rs, err := refstore.NewReferenceStore(refStoreLocation) + if err != nil { + return nil, fmt.Errorf("Couldn't create reference store repository: %s", err) + } + d.ReferenceStore = rs + imageStore, err := image.NewImageStore(ifs, layerStore) if err != nil { return nil, err diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index e33bc8383d91d..b3c126353d667 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -820,7 +820,7 @@ func configureKernelSecuritySupport(config *config.Config, driverName string) er return nil } - if driverName == "overlay" || driverName == "overlay2" { + if driverName == "overlay" || driverName == "overlay2" || driverName == "overlayfs" { // If driver is overlay or overlay2, make sure kernel // supports selinux with overlay. supported, err := overlaySupportsSelinux() From 9b17bfa28730a6b81143d3642d8e3be7555d417c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 9 Aug 2022 10:47:28 +0200 Subject: [PATCH 3/9] daemon: move "imageroot" and "image-store" to where it's used daemon: move image-store creation to where it's used; imageRoot should not be set _before_ the graphdriver name is set, otherwise it may be empty, and would make it use `/var/lib/docker/image` instead of `/var/lib/docker/image//` Signed-off-by: Sebastiaan van Stijn --- daemon/daemon.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index daac2be00c8a6..9bd8bd452918a 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -927,8 +927,6 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S return nil, err } - imageRoot := filepath.Join(config.Root, "image", d.graphDriver) - d.volumes, err = volumesservice.NewVolumeService(config.Root, d.PluginStore, rootIDs, d) if err != nil { return nil, err @@ -994,11 +992,6 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S } d.imageService = ctrd.NewService(d.containerdCli, d.containers, d.graphDriver) } else { - ifs, err := image.NewFSStoreBackend(filepath.Join(imageRoot, "imagedb")) - if err != nil { - return nil, err - } - layerStore, err := layer.NewStoreFromOptions(layer.StoreOptions{ Root: config.Root, MetadataStorePathTemplate: filepath.Join(config.Root, "image", "%s", "layerdb"), @@ -1021,6 +1014,12 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S return nil, err } + imageRoot := filepath.Join(config.Root, "image", d.graphDriver) + ifs, err := image.NewFSStoreBackend(filepath.Join(imageRoot, "imagedb")) + if err != nil { + return nil, err + } + // We have a single tag/reference store for the daemon globally. However, it's // stored under the graphdriver. On host platforms which only support a single // container OS, but multiple selectable graphdrivers, this means depending on which From a6ca5676a0518e829a834cdc19ce1ebd56ed0839 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 9 Aug 2022 10:54:05 +0200 Subject: [PATCH 4/9] fix GoDoc for SnapshotterFromGraphDriver Signed-off-by: Sebastiaan van Stijn --- daemon/containerd/snapshotters.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/daemon/containerd/snapshotters.go b/daemon/containerd/snapshotters.go index 97d5c9e5a2bc0..f4fe3e3a22d9f 100644 --- a/daemon/containerd/snapshotters.go +++ b/daemon/containerd/snapshotters.go @@ -6,8 +6,9 @@ import ( "github.com/containerd/containerd" ) -// snapshotterFromGraphDriver returns the containerd snapshotter name based on the supplied graphdriver name. -// It handles both legacy names and translates them into corresponding containerd snapshotter names. +// SnapshotterFromGraphDriver returns the containerd snapshotter name based on +// the supplied graphdriver name. It handles both legacy names and translates +// them into corresponding containerd snapshotter names. func SnapshotterFromGraphDriver(graphDriver string) string { if graphDriver == "" { graphDriver = containerd.DefaultSnapshotter From 763463e4516e0d6b0ecc45a4fc32821907eaaea2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 9 Aug 2022 10:57:36 +0200 Subject: [PATCH 5/9] SnapshotterFromGraphDriver: fix name for windows graphdriver Signed-off-by: Sebastiaan van Stijn --- daemon/containerd/snapshotters.go | 2 +- daemon/containerd/snapshotters_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon/containerd/snapshotters.go b/daemon/containerd/snapshotters.go index f4fe3e3a22d9f..efcc2aeecece0 100644 --- a/daemon/containerd/snapshotters.go +++ b/daemon/containerd/snapshotters.go @@ -17,7 +17,7 @@ func SnapshotterFromGraphDriver(graphDriver string) string { switch graphDriver { case "overlay", "overlay2": graphDriver = "overlayfs" - case "windowsgraph": + case "windowsfilter": graphDriver = "windows" } diff --git a/daemon/containerd/snapshotters_test.go b/daemon/containerd/snapshotters_test.go index 4d7e44697e5eb..6310693c32a46 100644 --- a/daemon/containerd/snapshotters_test.go +++ b/daemon/containerd/snapshotters_test.go @@ -28,8 +28,8 @@ func TestSnapshotterFromGraphDriver(t *testing.T) { output: "overlayfs", }, { - desc: "windowsgraph -> windows", - input: "windowsgraph", + desc: "windowsfilter -> windows", + input: "windowsfilter", output: "windows", }, { From 48616eadb6f045f7108f8c8d507280253c4c2b17 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 9 Aug 2022 11:01:26 +0200 Subject: [PATCH 6/9] SnapshotterFromGraphDriver: combine cases to single switch - use early return - combine all cases to a single switch - don't trim io.containerd.snapshotter.v1 prefix, as containerd itself does not currently support fully-qualified plugin names either (we may consider adding support for it in future) Signed-off-by: Sebastiaan van Stijn --- daemon/containerd/snapshotters.go | 21 +++++++-------------- daemon/containerd/snapshotters_test.go | 4 ++-- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/daemon/containerd/snapshotters.go b/daemon/containerd/snapshotters.go index efcc2aeecece0..bd9e4766827a7 100644 --- a/daemon/containerd/snapshotters.go +++ b/daemon/containerd/snapshotters.go @@ -1,26 +1,19 @@ package containerd -import ( - "strings" - - "github.com/containerd/containerd" -) +import "github.com/containerd/containerd" // SnapshotterFromGraphDriver returns the containerd snapshotter name based on // the supplied graphdriver name. It handles both legacy names and translates // them into corresponding containerd snapshotter names. func SnapshotterFromGraphDriver(graphDriver string) string { - if graphDriver == "" { - graphDriver = containerd.DefaultSnapshotter - } - switch graphDriver { case "overlay", "overlay2": - graphDriver = "overlayfs" + return "overlayfs" case "windowsfilter": - graphDriver = "windows" + return "windows" + case "": + return containerd.DefaultSnapshotter + default: + return graphDriver } - - graphDriver = strings.TrimPrefix(graphDriver, "io.containerd.snapshotter.v1.") - return graphDriver } diff --git a/daemon/containerd/snapshotters_test.go b/daemon/containerd/snapshotters_test.go index 6310693c32a46..7326ddef9062a 100644 --- a/daemon/containerd/snapshotters_test.go +++ b/daemon/containerd/snapshotters_test.go @@ -34,12 +34,12 @@ func TestSnapshotterFromGraphDriver(t *testing.T) { }, { desc: "containerd overlayfs", - input: "io.containerd.snapshotter.v1.overlayfs", + input: "overlayfs", output: "overlayfs", }, { desc: "containerd zfs", - input: "io.containerd.snapshotter.v1.zfs", + input: "zfs", output: "zfs", }, { From 1b2202914c0d006561842c31f9818a9c8fa9b4a7 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 9 Aug 2022 11:02:32 +0200 Subject: [PATCH 7/9] TestSnapshotterFromGraphDriver: some refactoring - make sure to de-reference the whole struct - use gotest.tools - rename variables for consistency/clarity Signed-off-by: Sebastiaan van Stijn --- daemon/containerd/snapshotters_test.go | 60 +++++++++++++------------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/daemon/containerd/snapshotters_test.go b/daemon/containerd/snapshotters_test.go index 7326ddef9062a..9d05794bb5cfa 100644 --- a/daemon/containerd/snapshotters_test.go +++ b/daemon/containerd/snapshotters_test.go @@ -4,57 +4,55 @@ import ( "testing" "github.com/containerd/containerd" + "gotest.tools/v3/assert" ) func TestSnapshotterFromGraphDriver(t *testing.T) { testCases := []struct { - desc string - input string - output string + desc string + input string + expected string }{ { - desc: "empty defaults to containerd default", - input: "", - output: containerd.DefaultSnapshotter, + desc: "empty defaults to containerd default", + input: "", + expected: containerd.DefaultSnapshotter, }, { - desc: "overlay -> overlayfs", - input: "overlay", - output: "overlayfs", + desc: "overlay -> overlayfs", + input: "overlay", + expected: "overlayfs", }, { - desc: "overlay2 -> overlayfs", - input: "overlay2", - output: "overlayfs", + desc: "overlay2 -> overlayfs", + input: "overlay2", + expected: "overlayfs", }, { - desc: "windowsfilter -> windows", - input: "windowsfilter", - output: "windows", + desc: "windowsfilter -> windows", + input: "windowsfilter", + expected: "windows", }, { - desc: "containerd overlayfs", - input: "overlayfs", - output: "overlayfs", + desc: "containerd overlayfs", + input: "overlayfs", + expected: "overlayfs", }, { - desc: "containerd zfs", - input: "zfs", - output: "zfs", + desc: "containerd zfs", + input: "zfs", + expected: "zfs", }, { - desc: "unknown is unchanged", - input: "somefuturesnapshotter", - output: "somefuturesnapshotter", + desc: "unknown is unchanged", + input: "somefuturesnapshotter", + expected: "somefuturesnapshotter", }, } - for _, tC := range testCases { - want := tC.output - t.Run(tC.desc, func(t *testing.T) { - got := SnapshotterFromGraphDriver(tC.input) - if want != got { - t.Errorf("Expected sanitizeGraphDriver to return %q, got %q", want, got) - } + for _, tc := range testCases { + tc := tc + t.Run(tc.desc, func(t *testing.T) { + assert.Equal(t, SnapshotterFromGraphDriver(tc.input), tc.expected) }) } } From eecb4b10d38081f3fb5af372b9e0f6ea36e6bd3d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 9 Aug 2022 12:47:53 +0200 Subject: [PATCH 8/9] daemon: use local variable for snapshotter name Reduce the use of daemon.graphDriver field, which should be removed. Signed-off-by: Sebastiaan van Stijn --- daemon/daemon.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index 9bd8bd452918a..e59be1edd5b5a 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -984,13 +984,14 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S } if d.UsesSnapshotter() { - d.graphDriver = ctrd.SnapshotterFromGraphDriver(graphDriver) + snapshotter := ctrd.SnapshotterFromGraphDriver(graphDriver) // Configure and validate the kernels security support. Note this is a Linux/FreeBSD // operation only, so it is safe to pass *just* the runtime OS graphdriver. - if err := configureKernelSecuritySupport(config, graphDriver); err != nil { + if err := configureKernelSecuritySupport(config, snapshotter); err != nil { return nil, err } - d.imageService = ctrd.NewService(d.containerdCli, d.containers, d.graphDriver) + d.imageService = ctrd.NewService(d.containerdCli, d.containers, snapshotter) + d.graphDriver = snapshotter } else { layerStore, err := layer.NewStoreFromOptions(layer.StoreOptions{ Root: config.Root, From a549834ba2989bf00dc73332387a09d791048cd5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 9 Aug 2022 12:49:19 +0200 Subject: [PATCH 9/9] ImageService.GraphDriverName(): return selected snapshotter name Signed-off-by: Sebastiaan van Stijn --- daemon/containerd/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemon/containerd/service.go b/daemon/containerd/service.go index b4bcec46b9339..097a01bbbd91f 100644 --- a/daemon/containerd/service.go +++ b/daemon/containerd/service.go @@ -94,7 +94,7 @@ func (i *ImageService) Cleanup() error { // - newContainer // - to report an error in Daemon.Mount(container) func (i *ImageService) GraphDriverName() string { - return "containerd-snapshotter" + return i.snapshotter } // ReleaseLayer releases a layer allowing it to be removed