Feature: export image using the `Transfer` API by knight42 · Pull Request #7916 · containerd/containerd

knight42 · 2023-01-04T09:11:19Z

Superseded by #8191

Summary

Implement exporting image using the new Transfer API.

Implementation Details

Invoke the Transfer API with a dummy image.Store as source and archive.ImageExportStream as destination.

The reason why a dummy image.Store is chosen as source here is I learnt that the images to export are actually read on the containerd side, not ctr and we need to distinguish the export scenario from the others, so I choose to send a dummy image.Store to tell the local transfer service about that.

In addition, I have added some options to the archive.ImageExportStream to gain the same ability as "non-local" export. These options are hold in a config struct, does not follow the functional options pattern, b/c I think this would save me some typings. This might be opinionated, I could switch to functional options if that's necessary.

/cc @dmcgowan

k8s-ci-robot · 2023-01-04T09:11:28Z

Hi @knight42. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/transfer/archive/exporter.go

austinvazquez · 2023-01-04T17:48:27Z

pkg/transfer/archive/exporter.go

 // NewImageExportStream returns a image importer via tar stream
 // TODO: Add export options
-func NewImageExportStream(stream io.WriteCloser, mediaType string) *ImageExportStream {
+func NewImageExportStream(stream io.WriteCloser, mediaType string, opts ExportOptions) *ImageExportStream {


Would it be more idiomatic to use functional options pattern here with something like the following? It would be the same functionally but cleans up the callsites a bit imo.

type ImageExportStreamOpt func(*ImageExportStream) func NewImageExportStream(stream io.WriteCloser, mediaType string, opts ...ImageExportStreamOpt) *ImageExportStream { ... }

Edit: Oh just seen your comment in PR description. Personally I think options pattern matches other areas of containerd + cleans up callsites. See what others thinks.

OK I would like to change it later

Second @austinvazquez. The importer implementation is a good reference.
With this, Export(ctx context.Context, is images.Store, cs content.Store) can be significantly simplified.

I just realize there might be naming collision between the export and import options, regarding to the platform related ones. We might need to rename these options, like WithExportPlatforms(...), WithImportPlatforms, or simply put them into separate packages.

austinvazquez · 2023-01-04T18:05:17Z

pkg/transfer/local/export.go

+
+	err = is.Export(ctx, ts.images, ts.content)
+	if err != nil {
+		return err


Would we need to send an event to tops here that export has been cancelled due to error?

I would like to, but I don't know what is the content that I should send. I have referred to the implementation of import, it does not send neither 🤔

pkg/transfer/transfer.go

dmcgowan · 2023-01-04T20:05:55Z

api/types/transfer/importexport.proto

+	// Whether to include all platforms
+	bool all_platforms = 5;
+	// Skips the creation of the Docker compatible manifest.json file
+	bool skip_docker_manifest = 6;


Is this only required for compatibility with older versions of Docker now?

Still no version of docker that supports OCI layout.

pkg/transfer/archive/exporter.go

dmcgowan · 2023-01-05T02:19:07Z

cmd/ctr/commands/images/export.go

+			}
+
+			err := client.Transfer(ctx,
+				image.NewStore(""), // a dummy image store


We can create a new type that represents multiple images. The idea is that the source could be anything, it could be a single image from the image store or in this case multiple images from the image store. It could also be sourced from a registry. The images shouldn't be used as export options though since that is information about the source.

We can create a new type that represents multiple images

Yeah we could do that, shall we add a new type MultiImageStore with a GetImages method? like:

type MultiImageStore struct { images [] string } func (s *MultiImageStore) GetImageNames() []string { return s.images }

then we could add an interface for it:

type ImageNamesGetter interface { GetImageNames() []string }

then we could pass it to the method exportStream of localTransferService to get the image names specified by user.

I am not sure if there is a better way to do this, from my perspective how we get image in export is similar to push, the only difference is we could export multiple images, and the current implementation of image.Store only takes single image into consideration.

I propose we could split the current image.Store into a source image store and a destionation image store, while we could get multiple images from a source image store, but could only save only single image to a destination image store. Therefore we could reuse the source image store in export and push(we might want to support pushing multiple images later :P) WDYT?

We can likely re-use the ImageStore and provide an extra names array or extend to names. I think extra names may make sense so that it can implement a GetImage but optionally provide more image names. There could be cases in the future where we want to provide additional references on pull as well (such as store by tag@digest, a reference which might be created dynamically on store).

I'm playing around with a change to the image store transfer interface and import logic that may make this cleaner.

I'm playing around with a change to the image store transfer interface and import logic that may make this cleaner.

@dmcgowan That's great! I would like to help if there is anything I could do

fangn2

Thanks for the PR.

The top level question I have is the design of Transfer API for export feature, why is dummy image.Store used as source? Shouldn't ImageGetter be used which gets an image from images.store similar to push?

fangn2 · 2023-01-06T16:00:06Z

pkg/transfer/archive/exporter.go

 // NewImageExportStream returns a image importer via tar stream
 // TODO: Add export options
-func NewImageExportStream(stream io.WriteCloser, mediaType string) *ImageExportStream {
+func NewImageExportStream(stream io.WriteCloser, mediaType string, opts ExportOptions) *ImageExportStream {


Second @austinvazquez. The importer implementation is a good reference.
With this, Export(ctx context.Context, is images.Store, cs content.Store) can be significantly simplified.

fangn2 · 2023-01-13T04:06:21Z

pkg/transfer/transfer.go


+// ImageExporter exports images to a writer
+type ImageExporter interface {
+	Export(ctx context.Context, is images.Store, cs content.Store) error


Suggest remove variable name which is not necessary Export(context.Context, images.Store, content.Store) error for consistency with other interface methods.

fangn2 · 2023-01-13T04:22:25Z

pkg/transfer/local/export.go

+	"github.com/containerd/containerd/pkg/transfer"
+)
+
+func (ts *localTransferService) exportStream(ctx context.Context, is transfer.ImageExporter, tops *transfer.Config) error {


nit: variable name ie seems better for type transfer.ImageExporter

The profile of exportSteam doesn't look right to me.

I think it should be exportStream(ctx context.Context, ig transfer.ImageGetter, ie transfer.ImageExporter, tops *transfer.Config)

fangn2 · 2023-01-13T04:45:59Z

pkg/transfer/local/transfer.go

 		case transfer.ImagePusher:
 			return ts.push(ctx, s, d, topts)
+		case transfer.ImageExporter:
+			return ts.exportStream(ctx, d, topts)


I think it should be return ts.exportStream(ctx, s, d, topts) t comply the Transfer API structure.

Signed-off-by: Jian Zeng <[email protected]>

knight42

@dmcgowan Hi I have refactored the implementation a bit, PTAL

knight42 · 2023-02-28T17:26:37Z

pkg/transfer/image/imagestore.go

+// ListExportImageNames returns a list of image names to export
+func (is *Store) ListExportImageNames() []string {
+	return is.exportImageNames
+}


@dmcgowan I decided to save the names of images to be exported in the image store.

knight42 · 2023-02-28T17:27:49Z

pkg/transfer/archive/exporter.go

+type ExportOpt func(*ImageExportStream)
+
+func WithSkipNonDistributable() ExportOpt {
+	return func(ies *ImageExportStream) {
+		ies.skipNonDistributable = true
+	}
+}
+
+func WithSkipDockerManifest() ExportOpt {
+	return func(ies *ImageExportStream) {
+		ies.skipDockerManifest = true
+	}
+}
+
+func WithAllPlatforms() ExportOpt {
+	return func(ies *ImageExportStream) {
+		ies.allPlatforms = true
+	}
+}
+
+func WithPlatforms(platforms ...v1.Platform) ExportOpt {
+	return func(ies *ImageExportStream) {
+		ies.platforms = platforms
+	}
+}


conform to functional options pattern

dmcgowan · 2023-03-01T16:38:37Z

Thanks @knight42! I'm still testing this locally and have a few changes in the process. I am planning on carrying those changes though after I finish testing to make sure we can get this in for 1.7. It'll keep your commit history though. I'll comment here when done, thanks!

knight42 · 2023-03-02T02:27:43Z

Thanks @knight42! I'm still testing this locally and have a few changes in the process. I am planning on carrying those changes though after I finish testing to make sure we can get this in for 1.7. It'll keep your commit history though. I'll comment here when done, thanks!

Got it, sorry for the late response. I guess I cloud close this PR then, thank you.

k8s-ci-robot requested a review from dmcgowan January 4, 2023 09:11

k8s-ci-robot added the needs-ok-to-test label Jan 4, 2023

knight42 force-pushed the feat/transfer-export-image branch from 9848186 to 482f88d Compare January 4, 2023 09:18

austinvazquez reviewed Jan 4, 2023

View reviewed changes

pkg/transfer/transfer.go Show resolved Hide resolved

dmcgowan self-assigned this Jan 4, 2023

dmcgowan reviewed Jan 4, 2023

View reviewed changes

dmcgowan reviewed Jan 5, 2023

View reviewed changes

pkg/transfer/archive/exporter.go Outdated Show resolved Hide resolved

dmcgowan reviewed Jan 5, 2023

View reviewed changes

fangn2 reviewed Jan 13, 2023

View reviewed changes

dmcgowan added this to the 1.7 milestone Jan 21, 2023

knight42 added 5 commits February 28, 2023 23:36

feat(api): add fields to ImageExportStream

de84df7

Signed-off-by: Jian Zeng <[email protected]>

feat: export images using Transfer api

8724787

Signed-off-by: Jian Zeng <[email protected]>

test: fix integration test

efd7985

Signed-off-by: Jian Zeng <[email protected]>

address comments

084142e

Signed-off-by: Jian Zeng <[email protected]>

refactor(transfer): update export api

0c89552

Signed-off-by: Jian Zeng <[email protected]>

knight42 force-pushed the feat/transfer-export-image branch from 025fb09 to c0aea39 Compare February 28, 2023 17:20

refactor: get images which are to exported from the store

54ff192

Signed-off-by: Jian Zeng <[email protected]>

knight42 force-pushed the feat/transfer-export-image branch from c0aea39 to 54ff192 Compare February 28, 2023 17:23

knight42 commented Feb 28, 2023

View reviewed changes

dmcgowan mentioned this pull request Mar 1, 2023

Transfer export image #8191

Merged

knight42 closed this Mar 2, 2023

knight42 deleted the feat/transfer-export-image branch March 2, 2023 02:27

Conversation

knight42 commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation Details

Uh oh!

k8s-ci-robot commented Jan 4, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fangn2 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knight42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Mar 1, 2023

Uh oh!

knight42 commented Mar 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

knight42 commented Jan 4, 2023 •

edited

Loading

fangn2 left a comment •

edited

Loading