Support transfer to and from container#99
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for transferring tar streams to/from a container filesystem inside the VM, via containerd’s transfer API over the existing shim↔vminitd streaming path. It also refactors the shim to introduce a Sandbox plugin abstraction and updates streaming IDs from uint32 to string-based identifiers.
Changes:
- Implement vminitd-side transfer service with new transfer types (
ContainerFilesystem,ReadStream,WriteStream) and transferrers for container filesystem tar import/export. - Add shim-side streaming bridge and shim transfer proxy service; refactor task service to depend on a new
Sandboxplugin. - Update streaming interfaces to use string stream IDs, add VM
switchRootto enable pivot_root, and add integration/shim tests + busybox build artifacts.
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
shimtest/transfer_test.go |
Adds shim-level tests for copy-to/copy-from transfer operations and exec verification. |
shimtest/shim_test.go |
Adds shim lifecycle + exec tests and shared shim-start/connect helpers. |
shimtest/helpers_test.go |
Adds test harness utilities (PATH setup, busybox rootfs, OCI spec helpers, FIFO draining). |
plugins/vminit/streaming/plugin.go |
Updates vminitd vsock streaming service to use string IDs and adds StreamGetter() returning framed streaming.Stream. |
plugins/types.go |
Introduces a new plugin type: SandboxPlugin. |
plugins/shim/transfer/plugin.go |
Adds shim-side Transfer TTRPC service that forwards Transfer RPCs to vminitd via the sandbox client. |
plugins/shim/task/plugin.go |
Switches shim task plugin dependency from VM manager to sandbox. |
plugins/shim/streaming/plugin.go |
Adds shim-side streaming TTRPC service that bridges TTRPC streams to VM streams with framing. |
plugins/shim/sandbox/plugin.go |
Registers sandbox manager plugin that wraps a VM manager into a Sandbox. |
plugins/services/transfer/service.go |
Implements vminitd-side transfer service dispatching to transferrers with Any conversion + stream unmarshaling. |
internal/vminit/stream/stream.go |
Changes stream manager interface to Get(id string). |
internal/vminit/runc/platform.go |
Updates console copy paths to use string stream IDs (no numeric parsing). |
internal/vminit/process/utils.go |
Updates stream URI parsing to treat the stream ID as a string. |
internal/vminit/process/init.go |
Restores runc NoPivot behavior to follow NoPivotRoot (now supported via VM switchRoot). |
internal/vm/vm.go |
Updates VM StartStream API to accept a caller-provided string ID. |
internal/vm/libkrun/instance.go |
Implements string-ID stream handshake for libkrun-backed instances (length-prefixed ID + ack). |
internal/transfer/types.go |
Adds new transfer types (container filesystem + read/write streams) and registers them with containerd transfer plugins. |
internal/transfer/echo.go |
Adds an echo transferrer for streaming pipeline testing. |
internal/transfer/containerfs.go |
Adds tar import/export transferrer for container filesystem paths. |
internal/shim/task/vm.go |
Removes VM-instance management helpers (now handled via sandbox). |
internal/shim/task/service.go |
Refactors task service to use sandbox.Sandbox, plumbs sandbox options, and adds mount namespace enforcement. |
internal/shim/task/resources_config.go |
Converts resource config application from direct VM calls to sandbox options. |
internal/shim/task/networking_unix.go |
Converts network provider VM setup to sandbox options (WithNIC, init args). |
internal/shim/task/mount_other.go |
Updates mount setup signature to return sandbox options. |
internal/shim/task/mount_linux.go |
Refactors mount transformation to produce sandbox FS/disk options instead of calling VM methods directly. |
internal/shim/task/mount_darwin.go |
Updates mount transformation API to include sandbox options. |
internal/shim/task/mount.go |
Refactors disk addition to sandbox disk options and returns sandbox opts alongside mounts. |
internal/shim/task/io.go |
Switches process IO forwarding to string stream IDs and adds stream ID generation. |
internal/shim/task/bundle/bundle.go |
Adds EnsureMountNamespace bundle transformer to ensure mount namespace exists. |
internal/shim/sandbox/vm/vm.go |
Implements a VM-backed sandbox (Start/Stop/Client/StartStream). |
internal/shim/sandbox/sandbox.go |
Defines Sandbox interface and option types (FS, disk, NIC, resources, init args). |
internal/shim/manager/mount_linux.go |
Makes mount namespace setup best-effort when lacking privileges (EPERM). |
integration/vm_test.go |
Updates stream initialization test for string stream IDs. |
integration/transfer_test.go |
Adds integration test for echo transfer using framed streams + completion signaling. |
integration/test.sh |
Adds TestTransferEcho to integration test runner. |
go.sum |
Adds checksums for new/updated indirect deps. |
go.mod |
Updates module requirements (grpc direct, adds containerd/platforms, klauspost/compress indirect). |
docs/transfer-service.md |
Adds design/implementation plan document for transfer service. |
docker-bake.hcl |
Adds busybox build target. |
cmd/vminitd/main.go |
Imports transfer service plugin and adds switchRoot() to enable pivot_root in containers. |
cmd/containerd-shim-nerdbox-v1/main.go |
Imports new shim plugins (sandbox, streaming, transfer). |
api/types/transfer/v1/filesystem.pb.go |
Generated protobuf for ContainerFilesystem transfer type. |
api/types/transfer/v1/datastream.pb.go |
Generated protobuf for ReadStream/WriteStream transfer types. |
api/proto/nerdbox/types/transfer/v1/filesystem.proto |
Adds proto definition for ContainerFilesystem. |
api/proto/nerdbox/types/transfer/v1/datastream.proto |
Adds proto definitions for ReadStream and WriteStream. |
api/next.txtpb |
Updates API descriptor snapshot with new transfer protos. |
Makefile |
Adds busybox build target and shim test target; excludes shimtest from unit test target. |
Dockerfile |
Adds a busybox export stage to ship the busybox binary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
eede19a to
87ef3c2
Compare
87ef3c2 to
cea0628
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 38 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cea0628 to
a6fd73c
Compare
a6fd73c to
a9952f2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 38 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ab9cc33 to
f8b4084
Compare
f8b4084 to
00f13ac
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
05fd311 to
2957497
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2957497 to
1c68aaf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1e086b2 to
78c4620
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch header.Typeflag { | ||
| case tar.TypeDir: | ||
| if err := os.MkdirAll(target, os.FileMode(header.Mode)); err != nil { | ||
| return err | ||
| } | ||
| case tar.TypeReg: | ||
| if err := os.MkdirAll(filepath.Dir(target), 0755); err != nil { | ||
| return err |
There was a problem hiding this comment.
extractTarEntry only handles tar.TypeReg, but Go's tar reader can produce tar.TypeRegA (NUL) for regular files in some archives. Those files would be silently skipped. Consider treating tar.TypeRegA the same as tar.TypeReg, and returning an error for unknown/unsupported type flags instead of ignoring them.
| func init() { | ||
| registry.Register(&plugin.Registration{ | ||
| Type: cplugins.TTRPCPlugin, | ||
| ID: "streaming", | ||
| Requires: []plugin.Type{ | ||
| plugins.SandboxPlugin, | ||
| }, | ||
| InitFn: func(ic *plugin.InitContext) (interface{}, error) { |
There was a problem hiding this comment.
This shim plugin registers under cplugins.TTRPCPlugin, but other shim-side TTRPC services in this repo register under plugins.TTRPCPlugin (see plugins/shim/task/plugin.go). Using the containerd plugin type here can prevent the streaming proxy from being discovered/loaded by the shim. Switch Type to plugins.TTRPCPlugin and drop the cplugins import if no longer needed.
| log.G(ctx).WithField("stream", i.ID).Debug("registering stream") | ||
| if err := s.manager.Register(ctx, i.ID, ss); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Send ack after registering | ||
| e, _ := typeurl.MarshalAnyToProto(&ptypes.Empty{}) | ||
| if err := srv.Send(e); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| case <-cc: | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
If the client disconnects or the context is canceled, Stream returns without removing the registered stream from the manager. That can leave a stale entry in the stream manager (blocking re-registration of the same ID and leaking memory), and consumers may later Get a dead stream. Consider ensuring manager cleanup on exit (e.g., after the select, attempt Get + Close for the stream ID, ignoring NotFound).
Support copy from/to a container and stream echo. Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
78c4620 to
b087590
Compare
Signed-off-by: Derek McGowan <derek@mcg.dev>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, t := range s.transferrers { | ||
| if err := t.Transfer(ctx, src, dst); err == nil { | ||
| return &emptypb.Empty{}, nil | ||
| } else if !errdefs.IsNotImplemented(err) { | ||
| return nil, err | ||
| } | ||
| log.G(ctx).WithError(err).Debugf("transfer not implemented for %T to %T", src, dst) |
There was a problem hiding this comment.
err is declared in the if err := ... initializer and is out of scope at the subsequent log.G(ctx).WithError(err)... line, which will not compile. Assign the transfer error to a variable outside the if statement (or move the log into the else block) so the same err value can be logged and checked.
| for _, t := range s.transferrers { | |
| if err := t.Transfer(ctx, src, dst); err == nil { | |
| return &emptypb.Empty{}, nil | |
| } else if !errdefs.IsNotImplemented(err) { | |
| return nil, err | |
| } | |
| log.G(ctx).WithError(err).Debugf("transfer not implemented for %T to %T", src, dst) | |
| var transferErr error | |
| for _, t := range s.transferrers { | |
| if transferErr = t.Transfer(ctx, src, dst); transferErr == nil { | |
| return &emptypb.Empty{}, nil | |
| } else if !errdefs.IsNotImplemented(transferErr) { | |
| return nil, transferErr | |
| } else { | |
| log.G(ctx).WithError(transferErr).Debugf("transfer not implemented for %T to %T", src, dst) | |
| } |
Add implemenetation of the transfer service to allow sending or receiving a tar stream and applying or creating a tar from a directory inside a container
Adds new transfer types, these types should likely move to containerd but will start them here.
Based on #100