daemon: remove SnapshotterFromGraphDriver mapping

[thaJeztah]

• partially reverts Make the snapshotter configurable #40
• applying the changes from upstream daemon: make the snapshotter configurable moby/moby#43983

It was decided in upstream to not do mapping of graphdriver-names to
snapshotters, and instead require the actual name of the snapshotter
to be used when configuring the daemon's storage driver.

This patch removes the code that was used for mapping to make it
closer to upstream.

- A picture of a cute animal (not mandatory but encouraged)

[vvoland]

LGTM.

Just one thing came to my mind - if we differentiate overlayfs from overlay2 (even though they work in the same fashion), is it okay not to do that for zfs and btrfs?

[thaJeztah]

Just one thing came to my mind - if we differentiate overlayfs from overlay2 (even though they work in the same fashion), is it okay not to do that for zfs and btrfs?

Ultimately, it will be the combination of containerd-snapshoter + driver name that decides if it's a snapshotter named zfs or a graph driver named zfs. From that perspective, I think the "fully qualified" name would've been nice, but yeah, containerd doesn't support that itself, so 🤷‍♂️ (I guess we can also decorate the output a bit more in the CLI side when formatting the docker info output, TBD)

[thaJeztah]

Thx; bringing this one in 👍

[tonistiigi]

It was decided in upstream to not do mapping of graphdriver-names to
snapshotters, and instead require the actual name of the snapshotter
to be used when configuring the daemon's storage driver.

Why was it decided like this? This is a breaking change for all existing Docker installations.

This also completely breaks dind environment atm:

root@ef4773d500eb:# docker buildx build .
[+] Building 0.0s (2/2) FINISHED
 => ERROR [internal] load build definition from Dockerfile                                                   0.0s
 => ERROR [internal] load .dockerignore                                                                      0.0s
------
 > [internal] load build definition from Dockerfile:
------
------
 > [internal] load .dockerignore:
------
ERROR: failed to solve: failed to read dockerfile: failed to add snapshot bhvlptv4kcw97r3inkqjatoi0 to lease: bucket name required: unknown

c68323010ab5cf80beb7c825d4a2da11bbc541e2 is the first bad commit
commit c68323010ab5cf80beb7c825d4a2da11bbc541e2
Author: Sebastiaan van Stijn <github@gone.nl>
Date:   Wed Aug 24 15:34:27 2022 +0200

    daemon: remove SnapshotterFromGraphDriver mapping

    It was decided in upstream to not do mapping of graphdriver-names to
    snapshotters, and instead require the actual name of the snapshotter
    to be used when configuring the daemon's storage driver.

    This patch removes the code that was used for mapping to make it
    closer to upstream.

    Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

 daemon/containerd/snapshotters.go      | 19 -----------
 daemon/containerd/snapshotters_test.go | 58 ----------------------------------
 daemon/daemon.go                       | 17 +++++-----
 3 files changed, 8 insertions(+), 86 deletions(-)
 delete mode 100644 daemon/containerd/snapshotters.go
 delete mode 100644 daemon/containerd/snapshotters_test.go

I guess it has something to do with that environment defining default driver

root@86b71a45d04c:/go/src/github.com/docker/docker# env | grep DOCKER
DOCKER_GITCOMMIT=c68323010a
DOCKER_BUILDKIT=1
DOCKER_BUILDTAGS=
DOCKER_GRAPHDRIVER=overlay2

Edit: no, it does not start to work even if I unset DOCKER_GRAPHDRIVER

[thaJeztah]

In the case above, how does it configure the docker-in-docker container to use containerd-snapshotters? Wouldn't it require setting that up already?

[tonistiigi]

root@3d4025fee435:/go/src/github.com/docker/docker# cat /etc/docker/daemon.json
{
	"runtimes": {
		"crun": {
			"path": "/usr/local/bin/crun"
		}
	},
  "features": {
    "containerd-snapshotter": true
  }
}

The feature option is because currently, the daemon works with multiple backends so that containerd preview can be tested. We will not indefinitely maintain multiple storage stacks. When containerd stack is complete, the other one will be removed.

[thaJeztah]

The mapping (overlay, overlay2 -> overlayfs) was removed, because people considered it to be considered confusing; containerd's snapshotter is named overlayfs, so the preference was to stick with containerd's naming for all snapshotters, and not have magic mappings.

Feel free to bring it up in the maintainers channel though.