I'm considering we could keep the GraphDriver .. for graphdrivers, and omit the GraphDriver field entirely when snapshotters are used..

@thaJeztah , I assume then when using graph drivers StorageDriver would be omitted, wdyt?

... in that case we don't necessarily have to adopt this information; or at least we need to consider if this is information that's we want to expose.

The current inspect output contains various paths that are effectively an internal implementation detail, which includes some paths for files used as internal storage mechanism, e.g.;

        "ResolvConfPath": "/var/lib/docker/containers/21d2ca40cfb4fa05f8f267a6fc6c0ecfa36d9fe046ec7ed896570cb28b0dc867/resolv.conf",
        "HostnamePath": "/var/lib/docker/containers/21d2ca40cfb4fa05f8f267a6fc6c0ecfa36d9fe046ec7ed896570cb28b0dc867/hostname",
        "HostsPath": "/var/lib/docker/containers/21d2ca40cfb4fa05f8f267a6fc6c0ecfa36d9fe046ec7ed896570cb28b0dc867/hosts",
        "LogPath": "/var/lib/docker/containers/21d2ca40cfb4fa05f8f267a6fc6c0ecfa36d9fe046ec7ed896570cb28b0dc867/21d2ca40cfb4fa05f8f267a6fc6c0ecfa36d9fe046ec7ed896570cb28b0dc867-json.log",

Those paths were added "because it was simple to add them, so why not?", but because they were exposed somewhat indicated that it's OK to access, and "invited" users to start using those paths to their liking.

• tools scraping those log-files and/or rotating them
• users modifying the filenames on the host
• users modifying the container filesystem on the host

And the reverse; users with Docker Desktop on macOS or Windows being confused "that path doesn't exist on my host.. where is it?

I don't think this is future proof, containerd can use multiple storage drivers, one may just be the default

This one is for image inspect; you mean an image that is present in multiple snapshotters?

For images, it could be unpacked into zero or more snapshotter. If we want to start favoring the content store as the source of truth for image storage and treat the snapshotter as a cached of the unpacked value, then it may be confusing to generically say this is how the image is "stored".

Yeah, so we either should omit it entirely, or go all the way and enumerate all snapshots.

But that would bring back the discussion if such unpacked content belongs to the "image", so probably content store only would make more sense to track.

Some of that might impact how we present sizes in "docker image ls --tree" and "docker system df".

I guess for containers, it's less ambiguous, as they would always be tied to a specific snapshotter. We must still verify our code if we always use the container's property to reference the snapshotter though, as I recall early implementations tended to use the daemon's default as source of truth (thus not accounting for snapshotters to be differing per container)

If we want to start favoring the content store as the source of truth for image storage and treat the snapshotter as a cached of the unpacked value, then it may be confusing to generically say this is how the image is "stored".

👍🏼 , pushed an updated revision to not have the field on image inspect. I still included the change to have the graph driver field be omitted because I assumed in the case the image is stored in containerd content store graph driver does not make sense.

as I recall early implementations tended to use the daemon's default as source of truth (thus not accounting for snapshotters to be differing per container)

Probably a few spots where we will find that assumption and need to refactor/redesign. First one that comes to mind is the image lease issue.

pushed an updated revision to not have the field on image inspect.

Yes, based on the discussion above, I think it makes sense to omit the field, and it keeps our options open for further discussion.

Probably a few spots where we will find that assumption and need to refactor/redesign. First one that comes to mind is the image lease issue.

Thanks! Yeah, I made a pass once in #45274, but it's possible that other locations are still there; I think this was the tracking-ticket I created for that;

• containerd integration: verify behavior when switching snapshotters #45273

IMO, this should not be tied to "driver" specifically.

I think a more general "Storage" struct would be better and more future proof.

Currently the only thing we need to store for container is the snapshot for the rootfs of the container.

package container

type Storage struct {
  RootFS  *RootFSStorage `json:",omitempty"`
}

Where RootFSStorage would be a poor mangoopher's union (with 1 variant at the moment):

type RootFSStorage struct {
  Snapshot   *RootFSStorageSnapshot `json:",omitempty"` 
}

type RootFSStorageSnapshot struct {
  SnapshotID string
  SnapshotterName string
}

However, that would also give us the possibility to introduce other storage types like:

type RootFSStorage struct {
  Snapshot   *RootFSStorageSnapshot `json:",omitempty"` 
  // Just an illustration, let's not add it yet:
  // Content      *RootFSStorageContent `json:",omitempty"` 

  // Perhaps something different that can hold a rootfs useable by the container
  // PigeonFS          *PigeonFS `json:",omitempty"` 
}

type RootFSStorageContent struct {
  Blobs []ocispec.Descriptor
}

type PigeonFS struct {
  // IDK, something that uses pigeons to provide rootfs
}

We could even move the graphdrivers driver data here using this approach:

type RootFSStorage struct {
  GraphDriver  *RootFSStorageGraphDriver `json:",omitempty"` 
  Snapshot      *RootFSStorageSnapshot `json:",omitempty"` 
}

Which in the current state of things, would mean that we could distinguish a container running under container vs graphdrivers by the existence of one of the variants.

(names subject to discussion of course 😅)

WDYT?

@thaJeztah @dmcgowan @austinvazquez

IMO, this should not be tied to "driver" specifically.

I think a more general "Storage" struct would be better and more future proof.

Yes, I think that makes sense.

Where RootFSStorage would be a poor mangopher's union (with 1 variant at the moment):

If we do this, perhaps we should add some discriminator in the struct to make it work well with Swagger and so that we can unmarshal to the right type;

type Storage {
	Type StorageType
	*StorageTypeFoo // some struct with information specific to "Foo" storage
	*StorageTypeBar // ...
	*StorageTypeBaz // ...
}

In swagger we can set that as discriminator, and when unmarshaling can use the field to decide what type to unmarshal the fields to (also accounting for cases where fields may overlap).

One thing we should still keep in mind is to limit the information where needed; i.e., if we don't need it for internal processing, then we should avoid exposing it through the API (at least; if we don't expect users to be tinkering with those things; #50857 (comment))

We could even move the graphdrivers driver data here using this approach

I don't think we should change anything for the graphdrivers; ultimately they will be removed, so we can keep the existing GraphDriver field (with the omitempty), and it will go away once graphdrivers are no longer a thing.

Of course, the downside would be that we need to continue carrying the GraphDriver field itself in the structs in the API, well, unless we "patch" the response for backward compatibility, but that may also be a bit ugly, because all existing API versions have the field.

If we do this, perhaps we should add some discriminator in the struct to make it work well with Swagger and so that we can unmarshal to the right type;

Yeah not sure about that! Maybe it would still be valuable to have multiple options possible, like with the snapshotter and content store:

type RootFSStorage struct {
  Snapshot   *RootFSStorageSnapshot `json:",omitempty"` 
  // Just an illustration, let's not add it yet:
  Content    *RootFSStorageContent `json:",omitempty"` 
}

type RootFSStorageContent struct {
  Config ocispec.Descriptor
  Layers []ocispec.Descriptor
}

Just thinking out loud though

@thaJeztah @dmcgowan WDYT?

This line is missing in the main swagger.yaml (it's only in the v1.52.yaml 😅

Wow nice catch and also I forgot to add the Storage field in the container inspect response. Added in https://github.com/moby/moby/compare/e1d4ad209d70f3b34c49d177e9a6e182a6206905..efa077848f912b1c8febe579f345387e38008eb0

For a follow-up; we should check if we have a similar test for snapshotters;

• start a daemon with snapshotter X
• stop the daemon
• start the daemon with snapshotter Y (e.g. "vis" or equivalent)
• verify that we didn't try to switch the containers / images to a different snapshotter

(well; not sure if we support this scenario, but at least the stop/start as this test does)