Transfer service #7320

dmcgowan · 2022-08-25T05:44:02Z

Adds transfer service and associated streaming service

Initial version is ready for review, for requested changes which do not change the API, follow ups will be tracked in the transfer service issue

k8s-ci-robot · 2022-08-25T05:44:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

akhilerm

Have a few questions/comments on the new services.

akhilerm · 2022-09-12T07:05:52Z

api/services/streaming/v1/streaming.proto

+	rpc Stream(stream google.protobuf.Any) returns (stream google.protobuf.Any);
+}
+
+message StreamInit {


What is the use of StreamInit message? Could find only one usage in the below code. A comment here explaining it would be good.

newbie question: If we use a message for stream init, shouldnt we need a stream terminate also? Or is it somehow handled internally?

I believe this is a message sent on init to notify id

api/services/transfer/v1/transfer.proto

akhilerm · 2022-09-12T07:09:55Z

api/types/transfer/imagestore.proto

+
+	// Content filters
+
+	repeated string platforms = 3; // Does this need a separate type? MatchComparer


Cant we have the platforms as a separate message type of OS/Arch/Variant.? Since that is how we denote the platforms everywhere else in containerd, I think that would be more suitable here.

akhilerm · 2022-09-12T07:10:13Z

api/types/transfer/imagestore.proto

+
+	// Unpack Configuration
+
+	repeated string unpack_platforms = 6;


Whats the difference between platforms and unpack_platforms?

I should probably just change unpack_platforms into a separate type. What it is really indicating is different unpack configurations which may be done in parallel. Normally this would be keyed on platform and snapshotter would be inferred from the platform. The client may want control over that to target the platform and the snapshotter.

akhilerm · 2022-09-12T07:11:03Z

api/types/transfer/import.proto

@@ -0,0 +1,29 @@
+/*


file name needs to be changed as we have both import and export messages being defined here.

akhilerm · 2022-09-12T07:24:52Z

api/types/transfer/streaming.proto

+
+option go_package = "github.com/containerd/containerd/api/types/transfer";
+
+message Data {


Are these the messages that will be used by api/streaming service?

I need to document this a little more, the streaming API is only responsible for managing and identifying the stream. The protocol of the stream, including the messages sent across the stream are determined by APIs which use them. Inside the API messages, there is just a stream ID string communicated, but the context of where it is used determines the protocol. At the very least the protocol should be defined or mentioned next to where the stream ID message is in the proto files.

akhilerm · 2022-09-12T07:26:22Z

docs/transfer.md

@@ -0,0 +1,32 @@
+# Transfer Service
+
+The transfer service is a simple flexible service which can be used to transfer artifact objects between a source and destination. The service determines whether the transfer between the source and destination is possible rather than the API.


The service determines whether the transfer between the source and destination is possible rather than the API.

Can we have a little more detailed explanation of what is really meant here?

akhilerm · 2022-09-12T07:27:24Z

docs/transfer.md

+| Image Store | Object stream (Archive) | "export" |
+| Object stream (Layer) | Mount/Snapshot | "unpack" |
+| Mount/Snapshot | Object stream (Layer) | "diff" |
+| Image Store | Image Store | "tag" |


Does retagging an image involve using the transfer service?

You could, today tagging is two API calls, Get and Create. The point is highlighting the flexibility. We will never have a Tag API call, but you could do the equivalent using the image API or also the transfer API. The transfer API could be more useful if the image stores have separate content backends or in the case where one of the image stores was actually inside a shim or some other sandboxed environment.

akhilerm · 2022-09-12T07:29:18Z

docs/transfer.md

+| Object stream (Layer) | Mount/Snapshot | "unpack" |
+| Mount/Snapshot | Object stream (Layer) | "diff" |
+| Image Store | Image Store | "tag" |
+| Registry | Registry | mirror registry image |


When mirroring registry images, will it be a 2 step process that is abstracted

registry1 -> image store image store -> registry2

or a direct registry -> registry transfer

The idea is that the transfer interface supports it, how it is implemented is up to the transfer plugin which implements it. The caller would just give two registry arguments though

akhilerm · 2022-09-12T07:35:25Z

cmd/ctr/commands/images/pull.go

@@ -66,6 +73,10 @@ command. As part of this process, we do the following:
 			Name:  "max-concurrent-downloads",
 			Usage: "Set the max concurrent downloads for each pull",
 		},
+		cli.BoolFlag{
+			Name:  "local",
+			Usage: "Print the resulting image's chain ID",


Is this a typo??

tianon · 2022-09-14T17:07:27Z

docs/transfer.md

+
+The transfer service is a simple flexible service which can be used to transfer artifact objects between a source and destination. The flexible API allows each implementation of the transfer interface to determines whether the transfer between the source and destination is possible. This allows new functionality to be added directly by implementations without versioning the API or requiring other implementations to handle an interface change.
+
+The transfer service if built upon the core ideas put forth by the libchan project, that an API with binary streams and data channels as first class objects is more flexible and opens a wider variety of use cases without requiring constant protocol and API updates. To accomplish this, the transfer service makes use of the streaming service to allow binary and object streams to be accessible by transfer objects even when using grpc and ttrpc.


Suggested change

The transfer service if built upon the core ideas put forth by the libchan project, that an API with binary streams and data channels as first class objects is more flexible and opens a wider variety of use cases without requiring constant protocol and API updates. To accomplish this, the transfer service makes use of the streaming service to allow binary and object streams to be accessible by transfer objects even when using grpc and ttrpc.

The transfer service is built upon the core ideas put forth by the libchan project, that an API with binary streams and data channels as first class objects is more flexible and opens a wider variety of use cases without requiring constant protocol and API updates. To accomplish this, the transfer service makes use of the streaming service to allow binary and object streams to be accessible by transfer objects even when using grpc and ttrpc.

(very minor typo 👀)

cpuguy83

Overall this looks great, only comments are on fit/finish sort of things.
I'd be for bringing this in sooner rather than later so we can iterate on it more easily and start integrating with cri.

cpuguy83 · 2022-09-30T20:33:16Z

transfer.go

+)
+
+func (c *Client) Transfer(ctx context.Context, src interface{}, dest interface{}, opts ...transfer.Opt) error {
+	return proxy.NewTransferer(transferapi.NewTransferClient(c.conn), c.streamCreator()).Transfer(ctx, src, dest, opts...)


Can we make the transfer service something that can be customized in the client like other services? e.g. containerd.New("", containerd.WithServices(containerd.WithTransferService(...))), and a way to get at just the transfer service (client.TransferService())

I guess we likely need the same for the stream service?

anakrish · 2022-10-04T23:15:27Z

@dmcgowan In our use-case, we need to customize image management for certain pods, whereas use containerd's default implementation for all other pods. To this end, currently we've written our own snapshotter leveraging the "Remote Snapshotter" contract/interface. We still need #6899 so that we can specify a different snapshotter per runtime, to complete our implementation.

In the future, do you think our requirements could be better implemented as a Transfer Service? Can we specify a separate service per pod/runtime?

wllenyj · 2022-10-13T18:20:16Z

In the future, do you think our requirements could be better implemented as a Transfer Service? Can we specify a separate service per pod/runtime?

Confidential computing has the same needs. In the future, can we use Transfer Service to support image pulling inside sandbox? And is it possible to apply to #5742? Or is this just the beginning of the next step?
This pr refactored the Images function using Transfer Service.
Sorry, my question is too much.

estesp · 2022-10-19T02:27:39Z

Needs rebase due to merge conflict

vvoland · 2022-11-10T14:38:34Z

pkg/transfer/local/transfer.go

+	images  images.Store
+
+	// semaphore.NewWeighted(int64(rCtx.MaxConcurrentDownloads))
+	limiter *semaphore.Weighted


How about making this overridable on per Transfer basic (with transfer.Opt)?

This would allow API client to choose a desired limit scope, for example:

Single operation - for example in one pull operation I user may not want to have more than 3 layers being downloaded concurrently. To achieve this, API client would pass a fresh semaphore.Weighted for each transfer. This is would be the equivalent of how current containerd's WithMaxConcurrentDownloads and WithMaxConcurrentUploadedLayers behave.

Global - user may want the concurrent download/uploads limits to be shared among all operations (within Transfer service) and possibly with other components in my code. API consumer would pass a semaphore.Weighted shared with other components. For instance this is what Moby engine would do if used this service in containerd integration to support max-concurrent-downloads and max-concurrent-uploads daemon configuration options.

This also allows more specific limits to be applied, for example the API user might dynamically decide the limits, or whether he wants them at all, depending on other conditions like for example if the repository is remote or local.

AkihiroSuda · 2022-11-14T23:25:45Z

Is this experimental?
If so, that status should be documented somewhere (RELEASES.md?)

RELEASES.md: track experimental features #7020

AkihiroSuda · 2022-11-14T23:29:30Z

pkg/transfer/local/import.go

+			tops.Progress(transfer.Progress{
+				Event: "saved",
+				Name:  img.Name,
+				//Digest: img.Target.Digest.String(),


AkihiroSuda · 2022-11-14T23:29:50Z

pkg/transfer/local/progress.go

+							Event:   j.transferState,
+							Name:    job.name,
+							Parents: job.parents,
+							//Digest:   job.desc.Digest.String(),


AkihiroSuda · 2022-11-14T23:31:27Z

pkg/transfer/transfer.go

+//  Higher level implementation just takes strings and options
+//  Lower level implementation takes pusher/fetcher?
+
+*/


Probably removable?

Yeah, can remove these now and refer to the docs

AkihiroSuda · 2022-11-14T23:32:25Z

plugin/plugin.go

@@ -78,8 +78,12 @@ const (
 	EventPlugin Type = "io.containerd.event.v1"
 	// LeasePlugin implements lease manager
 	LeasePlugin Type = "io.containerd.lease.v1"
+	// Streaming implements a stream manager
+	StreamingPlugin Type = "io.containerd.streaming.v1"


Will this cause confusion with stream processors?
I don't come up with a better name though 😅

I think if you are deep enough into containerd's architecture to be using stream processors it won't be too hard to understand this difference and find where it is documented. Confusion is hard to avoid but we can always attempt to address it in documentation if it becomes a frequently asked question. Also interesting here is you could potentially use the streaming API to do client side stream processors, so many there is some relationship in the future.

AkihiroSuda · 2022-11-14T23:33:27Z

services/streaming/service.go

+
+func (ss *serviceStream) Recv() (a typeurl.Any, err error) {
+	a, err = ss.s.Recv()
+	if err != io.EOF {


errors.Is can be used here?

fuweid

LGTM basically and leave some comments.

api/types/transfer/importexport.proto

api/types/transfer/progress.proto

services/streaming/service.go

services/transfer/service.go

estesp · 2022-11-16T15:19:20Z

I'm good with getting this in and making improvements as needed in follow-ups; looks like it needs one last rebase, though.

Signed-off-by: Derek McGowan <derek@mcg.dev>

Disable using transfer service by default for now Signed-off-by: Derek McGowan <derek@mcg.dev>

Signed-off-by: Derek McGowan <derek@mcg.dev>

mikebrow

LGTM
agree with Phil this is good to merge and iterate on..

Nice!

Signed-off-by: Derek McGowan <derek@mcg.dev>

mikebrow · 2022-12-01T00:25:39Z

/ok-to-test

estesp

LGTM

dmcgowan force-pushed the transfer-service branch 3 times, most recently from 4abab3b to fca4bd4 Compare August 25, 2022 05:59

dmcgowan added this to the 1.7 milestone Aug 25, 2022

akhilerm reviewed Sep 12, 2022

View reviewed changes

dmcgowan force-pushed the transfer-service branch from 6b95547 to b167c71 Compare September 14, 2022 05:16

tianon reviewed Sep 14, 2022

View reviewed changes

dmcgowan force-pushed the transfer-service branch from 774fbf3 to ce1262a Compare September 16, 2022 18:15

cpuguy83 reviewed Sep 30, 2022

View reviewed changes

dmcgowan force-pushed the transfer-service branch from a666138 to 56b0b28 Compare October 1, 2022 05:50

dmcgowan marked this pull request as ready for review October 7, 2022 20:47

dmcgowan added the impact/changelog label Oct 7, 2022

dmcgowan force-pushed the transfer-service branch from e7e0ff1 to 96b1b5e Compare October 20, 2022 18:39

dmcgowan mentioned this pull request Oct 27, 2022

Image Transfer service #7592

Closed

10 tasks

dmcgowan force-pushed the transfer-service branch from 15dfe4d to b0bf030 Compare November 3, 2022 19:40

vvoland reviewed Nov 10, 2022

View reviewed changes

AkihiroSuda reviewed Nov 14, 2022

View reviewed changes

fuweid reviewed Nov 15, 2022

View reviewed changes

dmcgowan force-pushed the transfer-service branch from 8750b92 to c54f3ad Compare November 15, 2022 05:50

dmcgowan force-pushed the transfer-service branch from c54f3ad to d743ca4 Compare November 29, 2022 07:50

dmcgowan added 8 commits November 30, 2022 12:56

Update import cli to use transfer service

ef61e13

Signed-off-by: Derek McGowan <derek@mcg.dev>

Update ctr to make local the default for distribution

42eedc0

Disable using transfer service by default for now Signed-off-by: Derek McGowan <derek@mcg.dev>

Update export API types

52341a2

Signed-off-by: Derek McGowan <derek@mcg.dev>

Add media type to export stream

0762a3a

Signed-off-by: Derek McGowan <derek@mcg.dev>

Combine stream fuzz tests

8304a61

Signed-off-by: Derek McGowan <derek@mcg.dev>

Add variables names to transfer interface

c387a52

Signed-off-by: Derek McGowan <derek@mcg.dev>

Rename local transfer plugin

01bd314

Signed-off-by: Derek McGowan <derek@mcg.dev>

Update transfer docs

f1598cf

Signed-off-by: Derek McGowan <derek@mcg.dev>

dmcgowan force-pushed the transfer-service branch from 29a3ab0 to 82ce861 Compare November 30, 2022 20:59

mikebrow approved these changes Nov 30, 2022

View reviewed changes

dmcgowan added 2 commits November 30, 2022 16:02

Cleanup code comments and lint fixes

fc27542

Signed-off-by: Derek McGowan <derek@mcg.dev>

Rename transferer to transferrer

f881625

Signed-off-by: Derek McGowan <derek@mcg.dev>

dmcgowan force-pushed the transfer-service branch from 82ce861 to f881625 Compare December 1, 2022 00:03

k8s-ci-robot added the ok-to-test label Dec 1, 2022

estesp approved these changes Dec 1, 2022

View reviewed changes

estesp merged commit ae6c244 into containerd:main Dec 1, 2022

stevenhorsman mentioned this pull request Dec 2, 2022

CCv0: Discussion options for removing the CC branch and running confidential-containers out of kata-containers main kata-containers/tests#4441

Open

ethan-lowman-dd mentioned this pull request Dec 2, 2022

[WIP] feat: Implement CRI image verification plugin system #6994

Closed

dmcgowan mentioned this pull request Dec 10, 2022

Prepare release notes for v1.7.0-beta.1 #7793

Merged

dmcgowan deleted the transfer-service branch December 14, 2022 17:19

dmcgowan mentioned this pull request Jan 3, 2023

Prepare release notes for v1.7.0-beta.2 #7903

Merged

surajssd mentioned this pull request Jan 12, 2023

Migrate over to the upstream containerd kata-containers/kata-containers#6067

Open

dmcgowan mentioned this pull request Jan 28, 2023

Prepare release notes for v1.7.0-beta.3 #8014

Merged

dmcgowan mentioned this pull request Feb 15, 2023

Prepare release notes for v1.7.0 beta.4 #8110

Merged

This was referenced Feb 25, 2023

Prepare release notes for v1.7.0-rc.0 #8162

Merged

Prepare release notes for v1.7.0-rc.1 #8192

Merged

This was referenced Mar 7, 2023

Add release notes for v1.7.0-rc.2 #8216

Merged

Prepare release notes for v1.7.0-rc.3 #8233

Merged

Prepare release notes for v1.7.0 #8242

Merged


		// Content filters

		repeated string platforms = 3; // Does this need a separate type? MatchComparer


		// Unpack Configuration

		repeated string unpack_platforms = 6;


		option go_package = "github.com/containerd/containerd/api/types/transfer";

		message Data {

		@@ -0,0 +1,32 @@
		# Transfer Service

		The transfer service is a simple flexible service which can be used to transfer artifact objects between a source and destination. The service determines whether the transfer between the source and destination is possible rather than the API.


		The transfer service is a simple flexible service which can be used to transfer artifact objects between a source and destination. The flexible API allows each implementation of the transfer interface to determines whether the transfer between the source and destination is possible. This allows new functionality to be added directly by implementations without versioning the API or requiring other implementations to handle an interface change.

		The transfer service if built upon the core ideas put forth by the libchan project, that an API with binary streams and data channels as first class objects is more flexible and opens a wider variety of use cases without requiring constant protocol and API updates. To accomplish this, the transfer service makes use of the streaming service to allow binary and object streams to be accessible by transfer objects even when using grpc and ttrpc.

Transfer service #7320

Transfer service #7320

Conversation

dmcgowan commented Aug 25, 2022 • edited Loading

k8s-ci-robot commented Aug 25, 2022

akhilerm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anakrish commented Oct 4, 2022

wllenyj commented Oct 13, 2022 • edited Loading

estesp commented Oct 19, 2022

Choose a reason for hiding this comment

AkihiroSuda commented Nov 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

estesp commented Nov 16, 2022

mikebrow left a comment

Choose a reason for hiding this comment

mikebrow commented Dec 1, 2022

estesp left a comment

Choose a reason for hiding this comment

dmcgowan commented Aug 25, 2022 •

edited

Loading

wllenyj commented Oct 13, 2022 •

edited

Loading