Skip to content

daemon: make the snapshotter configurable#43983

Merged
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:daemon_configurable_snapshotter
Aug 24, 2022
Merged

daemon: make the snapshotter configurable#43983
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:daemon_configurable_snapshotter

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

Treat (storage/graph)Driver as snapshotter

Also moved some layerStore related initialization to the non-c8d case
because otherwise they get treated as a graphdriver plugins.

- Description for the changelog

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

@thaJeztah thaJeztah added area/storage Image Storage status/2-code-review impact/changelog area/images Image Service containerd-integration Issues and PRs related to containerd integration labels Aug 18, 2022
@thaJeztah thaJeztah added this to the v-next milestone Aug 18, 2022
Comment thread daemon/containerd/snapshotters_test.go Outdated
Comment thread daemon/daemon_unix.go
}

if driverName == "overlay" || driverName == "overlay2" {
if driverName == "overlay" || driverName == "overlay2" || driverName == "overlayfs" {
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely to cause confusions to the end users.

I think the containerd snapshotter name should be mapped to something like containerd://overlayfs or containerd:overlayfs.
Having a such prefix will also help avoiding the name conflict across the containerd snapshotter and the legacy Moby driver. e.g., "btrfs"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was some discussion about this in a maintainers call; see rumpl#40 (comment)

We discussed this in the maintainers meeting;

  • concensus was that the preference is to re-use the --storage-driver flag, as the term is generic enough, and was originally chosen for that reason (apparently there had been some internal discussion at that time to get rid of the "layers" concept, and early implementations of "snapshotter-like approaches" were already being designed)

  • for now, we would switch between "containerd snapshotters" and "storage drivers" based on the feature flag that was added (but snapshotter would of course at some point become the default)

  • if that feature is enabled, we must have some mapping for backward compatibility on (already familiar) names that have an equivalent in containerd, so;

    • overlay -> io.containerd.snapshotter.v1.overlayfs
    • overlay2 -> io.containerd.snapshotter.v1.overlayfs (same)
    • zfs -> io.containerd.snapshotter.v1.zfs
    • etc.
  • besides the "shorthand" (overlay2) names, also accept the "fully qualified" names (e.g. io.containerd.snapshotter.v1.overlayfs can be used in the daemon config)

  • if no explicit snapshotter is defined in the daemon config we can use containerd's default

  • but, we should consider having a better automatic selection (similar to how we currently select the most suitable snapshotter) instead of containerd's (currently hardcoded) default; this could be by querying (if possible) the list of available snapshotter-plugins that were loaded, and pick the first available one (based on a priority list).

  • perhaps this could use a contribution to containerd, to make containerd itself also make a similar selection (TBD)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it surprised me a bit that containerd didn't accept the full io.containerd.snapshotter.v1.overlayfs (which could've been an alternative).

That said; the point of view there was that both "old" and "new" (containerd-snapshotter) being supported was more of a transitional period, after which there would be only "one" option.

Is your concern the configuration side of things, or what's shown in docker info? (perhaps for the docker info case we could consider having a different presentation)

/cc @tianon @cpuguy83 @tonistiigi @corhere any thoughts?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ I should mention that we dropped the io.containerd.snapshotter.v1. handling in that PR, as it wouldn't (currently) match what's accepted by containerd, but perhaps that's a part that could be re-considered (and perhaps added in containerd itself)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(perhaps for the docker info case we could consider having a different presentation)

SGTM

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation will also have to emphasize that overlayfs is actually newer than overlay2

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; documentation will be added specifically about the containerd snapshotter integration; once all bits are integrated here in upstream, we will also update the "storage driver" section to describe them.

@thaJeztah thaJeztah force-pushed the daemon_configurable_snapshotter branch from 7ff9b1f to 1b7acd3 Compare August 19, 2022 20:57
@thaJeztah
Copy link
Copy Markdown
Member Author

rebased, because #43982 was merged

Treat (storage/graph)Driver as snapshotter

Also moved some layerStore related initialization to the non-c8d case
because otherwise they get treated as a graphdriver plugins.

Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the daemon_configurable_snapshotter branch from 1b7acd3 to 7a28b67 Compare August 22, 2022 16:58
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Aug 22, 2022

I updated this PR after discussion with @cpuguy83 @rumpl @corhere and others;

  • removed the name mapping (overlay2 -> overlayfs etc)
  • ImageService.LayerStoreStatus() now returns a driver-type: io.containerd.snapshotter.v1. This information is used to propagate the DriverStatus field in the /info endpoint.

The DriverStatus field in the API is documented as "for informational purposes", so it's not a strict "contract", but it's shown to the user, so allows the user to be informed that the daemon is using a containerd snapshotter (and disambiguate, e.g. aufs or zfs). Besides that, we may be able to use this for our integration tests (see rumpl#31 (comment))

With this PR:

mkdir -p /etc/docker/
echo '{"features":{"containerd-snapshotter":true}}' > /etc/docker/daemon.json
dockerd

docker info
...
Storage Driver: overlayfs
 driver-type: io.containerd.snapshotter.v1
Logging Driver: json-file
...

Comment thread daemon/containerd/service.go Outdated
With this patch:

    mkdir -p /etc/docker/
    echo '{"features":{"containerd-snapshotter":true}}' > /etc/docker/daemon.json
    dockerd

    docker info
    ...
    Storage Driver: overlayfs
     driver-type: io.containerd.snapshotter.v1
    Logging Driver: json-file

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Copy Markdown
Member Author

@tianon @tonistiigi @corhere @cpuguy83 this one LGTY?

@thaJeztah
Copy link
Copy Markdown
Member Author

@AkihiroSuda this LGTY?

@thaJeztah
Copy link
Copy Markdown
Member Author

Thanks! For others; I think this matches what was discussed, so I'll bring this in. If there's changes to be made, I'm happy to do so in a follow up (this PR will only be in master / v-next, so we'll have time to tweak if needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/images Image Service area/storage Image Storage containerd-integration Issues and PRs related to containerd integration impact/changelog status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

5 participants