Move the inspect code away from the image service by rumpl · Pull Request #43680 · moby/moby

rumpl · 2022-06-02T14:50:48Z

- What I did

The LoopkupImage method is only used by the inspect image route and
returns an api/type struct. The depenency to api/types of the
daemon/images package is wrong, the daemon doesn't need to know about
the api types.

This is a first step in making the image service smaller, it has too many methods as is:

GetLayerByID(string) (layer.RWLayer, error)
GetLayerMountID(string) (string, error)
Cleanup() error
GraphDriverName() string
CommitBuildStep(context.Context, backend.CommitConfig) (image.ID, error)
CreateImage(ctx context.Context, config []byte, parent string) (builder.Image, error)
GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ROLayer, error)
MakeImageCache(ctx context.Context, sourceRefs []string) (builder.ImageCache, error)
TagImageWithReference(ctx context.Context, imageID image.ID, newTag reference.Named) error
SquashImage(ctx context.Context, id, parent string) (string, error)
ExportImage(ctx context.Context, names []string, outStream io.Writer) error
ImageDelete(ctx context.Context, imageRef string, force, prune bool) ([]types.ImageDeleteResponseItem, error)
ImageHistory(ctx context.Context, name string) ([]*imagetypes.HistoryResponseItem, error)
Images(ctx context.Context, opts types.ImageListOptions) ([]*types.ImageSummary, error)
ImagesPrune(ctx context.Context, pruneFilters filters.Args) (*types.ImagesPruneReport, error)
ImportImage(ctx context.Context, src string, repository string, platform *specs.Platform, tag string, msg string, inConfig io.ReadCloser, outStream io.Writer, changes []string) error
LoadImage(ctx context.Context, inTar io.ReadCloser, outStream io.Writer, quiet bool) error
LookupImage(ctx context.Context, name string) (*types.ImageInspect, error)
PullImage(ctx context.Context, image, tag string, platform *specs.Platform, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error
PushImage(ctx context.Context, image, tag string, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error
SearchRegistryForImages(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *types.AuthConfig, metaHeaders map[string][]string) (*registrytypes.SearchResults, error)
TagImage(ctx context.Context, imageName, repository, tag string) (string, error)
GetRepository(context.Context, reference.Named, *types.AuthConfig) (distribution.Repository, error)
ImageDiskUsage(ctx context.Context) ([]*types.ImageSummary, error)
LayerDiskUsage(ctx context.Context) (int64, error)
ReleaseLayer(rwlayer layer.RWLayer) error
CommitImage(ctx context.Context, c backend.CommitConfig) (image.ID, error)
GetImage(ctx context.Context, refOrID string, platform *specs.Platform) (retImg *image.Image, retErr error)
CreateLayer(ctx context.Context, container *container.Container, initFunc layer.MountInit) (layer.RWLayer, error)
DistributionServices() images.DistributionServices
CountImages(ctx context.Context) (int, error)
LayerStoreStatus() [][2]string
GetContainerLayerSize(containerID string) (int64, int64)
UpdateConfig(maxDownloads, maxUploads *int)
Children(ctx context.Context, id image.ID) ([]image.ID, error)

And all of these either take some api/type struct as a parameter or return one:

ImageDelete(ctx context.Context, imageRef string, force, prune bool) ([]types.ImageDeleteResponseItem, error)
Images(ctx context.Context, opts types.ImageListOptions) ([]*types.ImageSummary, error)
ImagesPrune(ctx context.Context, pruneFilters filters.Args) (*types.ImagesPruneReport, error)
LookupImage(ctx context.Context, name string) (*types.ImageInspect, error)
PullImage(ctx context.Context, image, tag string, platform *specs.Platform, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error
PushImage(ctx context.Context, image, tag string, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error
SearchRegistryForImages(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *types.AuthConfig, metaHeaders map[string][]string) (*registrytypes.SearchResults, error)
GetRepository(context.Context, reference.Named, *types.AuthConfig) (distribution.Repository, error)
ImageDiskUsage(ctx context.Context) ([]*types.ImageSummary, error)

I would like to take a closer look at all of those and either change the types returned or move the methods around. Splitting all of this code will make the image service make more sense, as is, it's too large and does too many things.

- How I did it

Copied the code over to the route handler

- How to verify it

$ docker pull hello-world
$ docker image inspect hello-world # Should still work

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

tianon

FWIW, I found this significantly easier to review in my terminal with diff.colorMoved enabled so I could see that it's something like 99% moved code and a tiny fraction of actually modified bits (hopefully GitHub implements "moved code" detection eventually 😅).

rumpl · 2022-06-03T08:17:00Z

The build failure is not related:

[2022-06-02T16:47:51.987Z] #58 ERROR: executor failed running [/bin/sh -c echo 'deb https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_11/ /' > /etc/apt/sources.list.d/criu.list         && apt-get update         && apt-get install -y --no-install-recommends criu         && install -D /usr/sbin/criu /build/criu]: exit code: 100

If someone would be so kind and kick that job again :)

thaJeztah · 2022-06-21T14:48:38Z

api/server/router/image/image.go

 // NewRouter initializes a new image router
-func NewRouter(backend Backend) router.Router {
-	r := &imageRouter{backend: backend}
+func NewRouter(backend Backend, referenceBackend referenceBackend, imageStore imageStore, layerStore layer.Store) router.Router {


This signature is a bit "odd"; NewRouter() is exported, but the arguments it takes are non-exported interfaces; should those be exported, or should this take the interfaces that they already implement? (i.e. image.Store, reference.Store? 🤔

Indeed, I looked at the Backend interface that only references the methods needed by the router and thought I'd do the same, but then put the wrong visibility. I'll put image.Store and reference.Store as dependencies

The LoopkupImage method is only used by the inspect image route and returns an api/type struct. The depenency to api/types of the daemon/images package is wrong, the daemon doesn't need to know about the api types. Signed-off-by: Djordje Lukic <[email protected]>

thaJeztah

LGTM

failure is codecov/patch, which we disabled on master

rumpl force-pushed the move-image-inspect branch from 4c85659 to e98aab2 Compare June 2, 2022 16:43

tianon approved these changes Jun 2, 2022

View reviewed changes

tonistiigi approved these changes Jun 9, 2022

View reviewed changes

thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Jun 21, 2022

thaJeztah reviewed Jun 21, 2022

View reviewed changes

rumpl force-pushed the move-image-inspect branch from e98aab2 to b4ffe3a Compare June 22, 2022 13:09

thaJeztah approved these changes Jun 22, 2022

View reviewed changes

thaJeztah merged commit 0861539 into moby:master Jun 22, 2022

thaJeztah added this to the 22.06.0 milestone Jun 22, 2022

rumpl mentioned this pull request Jul 12, 2022

Create the containerd image service #43784

Merged

thaJeztah added the containerd-integration Issues and PRs related to containerd integration label Jul 21, 2022

ndeloof mentioned this pull request Jul 25, 2022

introduce GetImageWithDetails to manage image inspect data in backend rumpl/moby#27

Merged

crazy-max mentioned this pull request Sep 2, 2022

Dockerfile: pin criu version and build from source #44086

Open

rumpl deleted the move-image-inspect branch November 22, 2022 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Move the inspect code away from the image service#43680

Move the inspect code away from the image service#43680
thaJeztah merged 1 commit intomoby:masterfrom
rumpl:move-image-inspect

rumpl commented Jun 2, 2022

Uh oh!

tianon left a comment

Uh oh!

rumpl commented Jun 3, 2022

Uh oh!

thaJeztah Jun 21, 2022

Uh oh!

rumpl Jun 22, 2022

Uh oh!

thaJeztah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

rumpl commented Jun 2, 2022

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

rumpl commented Jun 3, 2022

Uh oh!

thaJeztah Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

rumpl Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants