Skip to content

Conversation

@thaJeztah
Copy link
Member

ImageService: rename GraphDriverName to StorageDriver

Make the function name more generic, as it's no longer used only for graphdrivers but also for snapshotters.

daemon: info: fillDriverInfo() get driver-name from ImageService

Make the ImageService the source of truth for the storage-driver that's used.

daemon: remove daemon.graphdriver

It was only used as an intermediate variable to store what's returned
by layerstore.DriverName() / ImageService.StorageDriver()

- Description for the changelog

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

Make the function name more generic, as it's no longer used only
for graphdrivers but also for snapshotters.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Make the ImageService the source of truth for the storage-driver
that's used.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added area/storage Image Storage status/2-code-review area/images Image Distribution area/daemon Core Engine 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
// StorageDriver returns the name of the default storage-driver (snapshotter)
// used by the ImageService.
func (i *ImageService) StorageDriver() string {
return ""
Copy link
Member Author

Choose a reason for hiding this comment

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

currently always empty, but will be propagated once rumpl#40 is upstreamed

It was only used as an intermediate variable to store what's returned
by layerstore.DriverName() / ImageService.StorageDriver()

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Comment on lines -954 to -956
// As layerstore initialization may set the driver
d.graphDriver = layerStore.DriverName()

Copy link
Member

Choose a reason for hiding this comment

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

With this code removed, what happens if the layer store sets the driver? Is it moved to the image service storage driver?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the old (graph driver) image store, ImageService.StorageDriver() is just a proxy for layerstore.DriverName(), so the layerstore remains the source of truth;

func (i *ImageService) GraphDriverName() string {
return i.layerStore.DriverName()
}

And for snapshotters, it will use the snapshotter name (see https://github.com/moby/moby/pull/43982/files#r948993572 above)

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

LGTM

@tianon tianon merged commit 464882e into moby:master Aug 19, 2022
@thaJeztah thaJeztah deleted the daemon_remove_graphdriver_field branch August 19, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/daemon Core Engine area/images Image Distribution area/storage Image Storage containerd-integration Issues and PRs related to containerd integration status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

4 participants