c8d: implement GetRepository (split GetRepository from ImageService) #45285

thaJeztah · 2023-04-06T10:45:37Z

The GetRepository method interacts directly with the registry, and does
not depend on the snapshotter, but is used for two purposes;

For the GET /distribution/{name:.*}/json route;

moby/api/server/router/distribution/backend.go

Lines 11 to 15 in dd3b71d

    
           // Backend is all the methods that need to be implemented 
        
           // to provide image specific functionality. 
        
           type Backend interface { 
        
           	GetRepository(context.Context, reference.Named, *registry.AuthConfig) (distribution.Repository, error) 
        
           }

And to satisfy the "executor.ImageBackend" interface as used by Swarm;

moby/daemon/cluster/executor/backend.go

Line 77 in 58c027a

type ImageBackend interface {

This patch removes the method from the ImageService interface, and instead
implements it through an composite struct that satisfies both interfaces,
and an ImageBackend() method is added to the daemon.

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

thaJeztah · 2023-04-06T10:51:30Z

daemon/containerd/service.go

-type RegistryConfigProvider interface {
+type RegistryService interface {


FWIW; I looked if we should un-export these interfaces, as they're only referenced locally, but I guess we exported them as they are used for the ImageServiceConfig below, and un-exporting them makes those fields awkward to use (and not show up in the documentation);

thaJeztah · 2023-04-06T10:54:03Z

daemon/containerd/image_pull.go

-func (i *ImageService) GetRepository(ctx context.Context, ref reference.Named, authConfig *registry.AuthConfig) (distribution.Repository, error) {
-	return nil, errdefs.NotImplemented(errors.New("not implemented"))
+func (i *ImageService) GetRepository(ctx context.Context, ref reference.Named, authConfig *registry.AuthConfig) (dist.Repository, error) {
+	return distribution.GetRepository(ctx, ref, &distribution.ImagePullConfig{


I guess one question is; could we implement such services with the containerd distribution code? (should we?)

Hmm, I think maybe the reverse would be better - why does it need to be dependent on ImageService?
Could we move it to RegistryService instead?

EDIT: Ah, now I read why it needs to be in ImageService 😅

However, currently this method must be implemented on the ImageService to satisfy the "executor.ImageBackend" interface as used by Swarm;

Maybe we could at least move it to executor.Backend (implemented by Daemon)? Or does swarmkit/other packages depend on this interface?

Maybe we could at least move it to executor.Backend (implemented by Daemon)? Or does swarmkit/other packages depend on this interface?

Hm.. good one. I should have a look what exactly is passed on to the cluster executor (I do think that interface also expects "push" and "pull", for which we do need the ImageService, so maybe it's not an option currently)

laurazard

LGTM

corhere

Swarmkit needs some struct which implements executor.ImageBackend, but it does not follow that GetRepository needs to be implemented in ImageService. The daemon struct already has ImageService() and RegistryService() accessor methods so it should be trivial to write an adapter struct which implements the interface without having to pollute the daemon or either ImageService implementation.

The GetRepository method interacts directly with the registry, and does not depend on the snapshotter, but is used for two purposes; For the GET /distribution/{name:.*}/json route; https://github.com/moby/moby/blob/dd3b71d17c614f837c4bba18baed9fa2cb31f1a4/api/server/router/distribution/backend.go#L11-L15 And to satisfy the "executor.ImageBackend" interface as used by Swarm; https://github.com/moby/moby/blob/58c027ac8ba19a3fa339c65274ea6704ccbec977/daemon/cluster/executor/backend.go#L77 This patch removes the method from the ImageService interface, and instead implements it through an composite struct that satisfies both interfaces, and an ImageBackend() method is added to the daemon. Signed-off-by: Sebastiaan van Stijn <github@gone.nl> remove GetRepository from ImageService Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2023-04-09T10:11:57Z

@corhere @vvoland @laurazard I rewrote it to move it to the daemon, using an intermediate struct / type. PTAL

rumpl · 2023-04-11T08:42:50Z

daemon/daemon.go

@@ -1466,6 +1471,14 @@ func (daemon *Daemon) ImageService() ImageService {
 	return daemon.imageService
 }

+// ImageBackend returns an image-backend for Swarm and the distribution router.
+func (daemon *Daemon) ImageBackend() executorpkg.ImageBackend {


It's rather unfortunate that we have this public method just so that we can create a router and a cluster on startup. The daemon here acts like an options object that carries around things used by other unrelated things

Yes, I agree. I think we should make a pass at looking what's there, and if all of these are needed. I feel there's a lot of "ad-hoc" (oh, we need a new backend, just add something).

thaJeztah · 2023-04-11T12:23:30Z

I'll bring this one in (hope it doesn't conflict with any of the other ones 🤞)

thaJeztah added area/distribution status/2-code-review area/images containerd-integration Issues and PRs related to containerd integration labels Apr 6, 2023

thaJeztah added this to the 24.0.0 milestone Apr 6, 2023

This comment was marked as outdated.

Sign in to view

thaJeztah commented Apr 6, 2023

View reviewed changes

laurazard approved these changes Apr 6, 2023

View reviewed changes

corhere requested changes Apr 6, 2023

View reviewed changes

thaJeztah force-pushed the c8d_implement_GetRepository branch 2 times, most recently from 4e0e04a to dd0807f Compare April 8, 2023 20:28

thaJeztah changed the title ~~c8d: implement GetRepository~~ c8d: implement GetRepository (split GetRepository from ImageService) Apr 9, 2023

thaJeztah force-pushed the c8d_implement_GetRepository branch from dd0807f to a5d46a1 Compare April 9, 2023 10:10

corhere approved these changes Apr 10, 2023

View reviewed changes

rumpl reviewed Apr 11, 2023

View reviewed changes

rumpl approved these changes Apr 11, 2023

View reviewed changes

thaJeztah merged commit 3ce6efc into moby:master Apr 11, 2023
92 checks passed

thaJeztah deleted the c8d_implement_GetRepository branch April 11, 2023 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c8d: implement GetRepository (split GetRepository from ImageService) #45285

c8d: implement GetRepository (split GetRepository from ImageService) #45285

thaJeztah commented Apr 6, 2023 •

edited

This comment was marked as outdated.

thaJeztah Apr 6, 2023

thaJeztah Apr 6, 2023

vvoland Apr 6, 2023 •

edited

vvoland Apr 6, 2023 •

edited

thaJeztah Apr 6, 2023

laurazard left a comment

corhere left a comment

thaJeztah commented Apr 9, 2023

rumpl Apr 11, 2023

thaJeztah Apr 11, 2023

thaJeztah commented Apr 11, 2023

	// Backend is all the methods that need to be implemented
	// to provide image specific functionality.
	type Backend interface {
	GetRepository(context.Context, reference.Named, *registry.AuthConfig) (distribution.Repository, error)
	}

		type RegistryConfigProvider interface {
		type RegistryService interface {

c8d: implement GetRepository (split GetRepository from ImageService) #45285

c8d: implement GetRepository (split GetRepository from ImageService) #45285

Conversation

thaJeztah commented Apr 6, 2023 • edited

This comment was marked as outdated.

thaJeztah Apr 6, 2023

Choose a reason for hiding this comment

thaJeztah Apr 6, 2023

Choose a reason for hiding this comment

vvoland Apr 6, 2023 • edited

Choose a reason for hiding this comment

vvoland Apr 6, 2023 • edited

Choose a reason for hiding this comment

thaJeztah Apr 6, 2023

Choose a reason for hiding this comment

laurazard left a comment

Choose a reason for hiding this comment

corhere left a comment

Choose a reason for hiding this comment

thaJeztah commented Apr 9, 2023

rumpl Apr 11, 2023

Choose a reason for hiding this comment

thaJeztah Apr 11, 2023

Choose a reason for hiding this comment

thaJeztah commented Apr 11, 2023

thaJeztah commented Apr 6, 2023 •

edited

vvoland Apr 6, 2023 •

edited

vvoland Apr 6, 2023 •

edited