From a183b2d232fd3c0ca7cf4903b2392cce639ca7c5 Mon Sep 17 00:00:00 2001 From: Iceber Gu Date: Wed, 19 Mar 2025 16:10:33 +0800 Subject: [PATCH 1/3] update taskOptions based on runtimeOptions when creating a task Signed-off-by: Iceber Gu --- client/container.go | 3 ++- client/task.go | 28 +++++++++++++++++++++++ client/task_opts.go | 27 ++++++++-------------- client/task_opts_unix.go | 48 +++++++++++++--------------------------- 4 files changed, 54 insertions(+), 52 deletions(-) diff --git a/client/container.go b/client/container.go index b9cf25e93..5763ae6d7 100644 --- a/client/container.go +++ b/client/container.go @@ -279,7 +279,8 @@ func (c *container) NewTask(ctx context.Context, ioCreate cio.Creator, opts ...N } } info := TaskInfo{ - runtime: r.Runtime.Name, + runtime: r.Runtime.Name, + runtimeOptions: r.Runtime.Options, } for _, o := range opts { if err := o(ctx, c.client, &info); err != nil { diff --git a/client/task.go b/client/task.go index 20312a922..1e3c3f631 100644 --- a/client/task.go +++ b/client/task.go @@ -146,6 +146,11 @@ type TaskInfo struct { // runtime is the runtime name for the container, and cannot be changed. runtime string + + // runtimeOptions is the runtime options for the container, and when task options are set, + // they will be based on the runtimeOptions. + // https://github.com/containerd/containerd/issues/11568 + runtimeOptions typeurl.Any } // Runtime name for the container @@ -153,6 +158,29 @@ func (i *TaskInfo) Runtime() string { return i.runtime } +// getRuncOptions returns a reference to the runtime options for use by the task. +// If the set of options is not set by the opts passed into the NewTask creation +// this function first attempts to initialize the runtime options with a copy of the runtimeOptions, +// otherwise an empty set of options is assigned and returned +func (i *TaskInfo) getRuncOptions() (*options.Options, error) { + if i.Options != nil { + opts, ok := i.Options.(*options.Options) + if !ok { + return nil, errors.New("invalid runtime v2 options format") + } + return opts, nil + } + + opts := &options.Options{} + if i.runtimeOptions != nil && i.runtimeOptions.GetValue() != nil { + if err := typeurl.UnmarshalTo(i.runtimeOptions, opts); err != nil { + return nil, fmt.Errorf("failed to get runtime v2 options: %w", err) + } + } + i.Options = opts + return opts, nil +} + // Task is the executable object within containerd type Task interface { Process diff --git a/client/task_opts.go b/client/task_opts.go index 8e94d4c59..27bde3506 100644 --- a/client/task_opts.go +++ b/client/task_opts.go @@ -54,12 +54,9 @@ func WithRuntimePath(absRuntimePath string) NewTaskOpts { // usually it is served inside a sandbox, and we can get it from sandbox status. func WithTaskAPIEndpoint(address string, version uint32) NewTaskOpts { return func(ctx context.Context, client *Client, info *TaskInfo) error { - if info.Options == nil { - info.Options = &options.Options{} - } - opts, ok := info.Options.(*options.Options) - if !ok { - return errors.New("invalid runtime v2 options format") + opts, err := info.getRuncOptions() + if err != nil { + return err } opts.TaskApiAddress = address opts.TaskApiVersion = version @@ -119,12 +116,9 @@ func WithCheckpointImagePath(path string) CheckpointTaskOpts { // WithRestoreImagePath sets image path for create option func WithRestoreImagePath(path string) NewTaskOpts { return func(ctx context.Context, c *Client, ti *TaskInfo) error { - if ti.Options == nil { - ti.Options = &options.Options{} - } - opts, ok := ti.Options.(*options.Options) - if !ok { - return errors.New("invalid runtime v2 options format") + opts, err := ti.getRuncOptions() + if err != nil { + return err } opts.CriuImagePath = path return nil @@ -134,12 +128,9 @@ func WithRestoreImagePath(path string) NewTaskOpts { // WithRestoreWorkPath sets criu work path for create option func WithRestoreWorkPath(path string) NewTaskOpts { return func(ctx context.Context, c *Client, ti *TaskInfo) error { - if ti.Options == nil { - ti.Options = &options.Options{} - } - opts, ok := ti.Options.(*options.Options) - if !ok { - return errors.New("invalid runtime v2 options format") + opts, err := ti.getRuncOptions() + if err != nil { + return err } opts.CriuWorkPath = path return nil diff --git a/client/task_opts_unix.go b/client/task_opts_unix.go index d33e30284..26b5c17be 100644 --- a/client/task_opts_unix.go +++ b/client/task_opts_unix.go @@ -20,20 +20,14 @@ package client import ( "context" - "errors" - - "github.com/containerd/containerd/api/types/runc/options" ) // WithNoNewKeyring causes tasks not to be created with a new keyring for secret storage. // There is an upper limit on the number of keyrings in a linux system func WithNoNewKeyring(ctx context.Context, c *Client, ti *TaskInfo) error { - if ti.Options == nil { - ti.Options = &options.Options{} - } - opts, ok := ti.Options.(*options.Options) - if !ok { - return errors.New("invalid v2 shim create options format") + opts, err := ti.getRuncOptions() + if err != nil { + return err } opts.NoNewKeyring = true return nil @@ -41,12 +35,9 @@ func WithNoNewKeyring(ctx context.Context, c *Client, ti *TaskInfo) error { // WithNoPivotRoot instructs the runtime not to you pivot_root func WithNoPivotRoot(_ context.Context, _ *Client, ti *TaskInfo) error { - if ti.Options == nil { - ti.Options = &options.Options{} - } - opts, ok := ti.Options.(*options.Options) - if !ok { - return errors.New("invalid v2 shim create options format") + opts, err := ti.getRuncOptions() + if err != nil { + return err } opts.NoPivotRoot = true return nil @@ -55,12 +46,9 @@ func WithNoPivotRoot(_ context.Context, _ *Client, ti *TaskInfo) error { // WithShimCgroup sets the existing cgroup for the shim func WithShimCgroup(path string) NewTaskOpts { return func(ctx context.Context, c *Client, ti *TaskInfo) error { - if ti.Options == nil { - ti.Options = &options.Options{} - } - opts, ok := ti.Options.(*options.Options) - if !ok { - return errors.New("invalid v2 shim create options format") + opts, err := ti.getRuncOptions() + if err != nil { + return err } opts.ShimCgroup = path return nil @@ -70,12 +58,9 @@ func WithShimCgroup(path string) NewTaskOpts { // WithUIDOwner allows console I/O to work with the remapped UID in user namespace func WithUIDOwner(uid uint32) NewTaskOpts { return func(ctx context.Context, c *Client, ti *TaskInfo) error { - if ti.Options == nil { - ti.Options = &options.Options{} - } - opts, ok := ti.Options.(*options.Options) - if !ok { - return errors.New("invalid v2 shim create options format") + opts, err := ti.getRuncOptions() + if err != nil { + return err } opts.IoUid = uid return nil @@ -85,12 +70,9 @@ func WithUIDOwner(uid uint32) NewTaskOpts { // WithGIDOwner allows console I/O to work with the remapped GID in user namespace func WithGIDOwner(gid uint32) NewTaskOpts { return func(ctx context.Context, c *Client, ti *TaskInfo) error { - if ti.Options == nil { - ti.Options = &options.Options{} - } - opts, ok := ti.Options.(*options.Options) - if !ok { - return errors.New("invalid v2 shim create options format") + opts, err := ti.getRuncOptions() + if err != nil { + return err } opts.IoGid = gid return nil From 8a16a6a04ad081deac2f4907adda2326e62e5182 Mon Sep 17 00:00:00 2001 From: Iceber Gu Date: Wed, 26 Mar 2025 14:59:17 +0800 Subject: [PATCH 2/3] prefer task options for PluginInfo request Signed-off-by: Iceber Gu --- core/runtime/v2/task_manager.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/runtime/v2/task_manager.go b/core/runtime/v2/task_manager.go index 844070b71..f396ced52 100644 --- a/core/runtime/v2/task_manager.go +++ b/core/runtime/v2/task_manager.go @@ -266,12 +266,12 @@ func (m *TaskManager) validateRuntimeFeatures(ctx context.Context, opts runtime. return nil } - ropts := opts.RuntimeOptions - if ropts == nil || ropts.GetValue() == nil { - ropts = opts.TaskOptions + topts := opts.TaskOptions + if topts == nil || topts.GetValue() == nil { + topts = opts.RuntimeOptions } - pInfo, err := m.PluginInfo(ctx, &apitypes.RuntimeRequest{RuntimePath: opts.Runtime, Options: typeurl.MarshalProto(ropts)}) + pInfo, err := m.PluginInfo(ctx, &apitypes.RuntimeRequest{RuntimePath: opts.Runtime, Options: typeurl.MarshalProto(topts)}) if err != nil { return fmt.Errorf("runtime info: %w", err) } From 9f46e7a449a06934bfb4a9b4b9718c1f625b1693 Mon Sep 17 00:00:00 2001 From: Iceber Gu Date: Wed, 26 Mar 2025 15:00:08 +0800 Subject: [PATCH 3/3] integration/client: add tests for TaskOptions is not empty Co-authored-by: Wei Fu Signed-off-by: Iceber Gu --- integration/client/client_unix_test.go | 129 +++++++++++++++++++++ integration/client/container_linux_test.go | 13 +++ 2 files changed, 142 insertions(+) diff --git a/integration/client/client_unix_test.go b/integration/client/client_unix_test.go index 44c81ecce..0f8e0d6c7 100644 --- a/integration/client/client_unix_test.go +++ b/integration/client/client_unix_test.go @@ -19,12 +19,26 @@ package client import ( + "context" + "strings" + "sync" "testing" + "github.com/containerd/containerd/api/services/tasks/v1" + "github.com/containerd/containerd/api/types/runc/options" . "github.com/containerd/containerd/v2/client" "github.com/containerd/containerd/v2/integration/images" "github.com/containerd/containerd/v2/pkg/deprecation" + "github.com/containerd/containerd/v2/pkg/oci" + "github.com/containerd/containerd/v2/pkg/protobuf" + "github.com/containerd/containerd/v2/plugins" + "github.com/containerd/errdefs" + "github.com/containerd/errdefs/pkg/errgrpc" "github.com/containerd/platforms" + "github.com/containerd/typeurl/v2" + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" ) var ( @@ -63,3 +77,118 @@ func TestImagePullSchema1WithEmptyLayers(t *testing.T) { t.Fatal(err) } } + +func TestNewTaskWithRuntimeOption(t *testing.T) { + t.Parallel() + + fakeTasks := &fakeTaskService{ + TasksClient: tasks.NewTasksClient(nil), + createRequests: map[string]*tasks.CreateTaskRequest{}, + } + + cli, err := newClient(t, address, + WithServices(WithTaskClient(fakeTasks)), + ) + require.NoError(t, err) + defer cli.Close() + + var ( + image Image + ctx, cancel = testContext(t) + ) + defer cancel() + + image, err = cli.GetImage(ctx, testImage) + require.NoError(t, err) + + for _, tc := range []struct { + name string + runtimeOption *options.Options + taskOpts []NewTaskOpts + expectedOptions *options.Options + }{ + { + name: "should be empty options", + runtimeOption: &options.Options{ + BinaryName: "no-runc", + }, + expectedOptions: nil, + }, + { + name: "should overwrite IOUid/ShimCgroup", + runtimeOption: &options.Options{ + BinaryName: "no-runc", + ShimCgroup: "/abc", + IoUid: 1000, + SystemdCgroup: true, + }, + taskOpts: []NewTaskOpts{ + WithUIDOwner(2000), + WithGIDOwner(3000), + WithShimCgroup("/def"), + }, + expectedOptions: &options.Options{ + BinaryName: "no-runc", + ShimCgroup: "/def", + IoUid: 2000, + IoGid: 3000, + SystemdCgroup: true, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + id := strings.Replace(t.Name(), "/", "_", -1) + + container, err := cli.NewContainer( + ctx, + id, + WithNewSnapshotView(id, image), + WithNewSpec(oci.WithImageConfig(image), withExitStatus(7)), + WithRuntime(plugins.RuntimeRuncV2, tc.runtimeOption), + ) + require.NoError(t, err) + defer container.Delete(ctx, WithSnapshotCleanup) + + _, err = container.NewTask(ctx, empty(), tc.taskOpts...) + require.NoError(t, err) + + fakeTasks.Lock() + req := fakeTasks.createRequests[id] + fakeTasks.Unlock() + + if tc.expectedOptions == nil { + require.Nil(t, req.Options) + return + } + + gotOptions := &options.Options{} + require.NoError(t, typeurl.UnmarshalTo(req.Options, gotOptions)) + require.True(t, cmp.Equal(tc.expectedOptions, gotOptions, protobuf.Compare)) + }) + } +} + +type fakeTaskService struct { + sync.Mutex + createRequests map[string]*tasks.CreateTaskRequest + tasks.TasksClient +} + +func (ts *fakeTaskService) Create(ctx context.Context, in *tasks.CreateTaskRequest, opts ...grpc.CallOption) (*tasks.CreateTaskResponse, error) { + ts.Lock() + defer ts.Unlock() + + ts.createRequests[in.ContainerID] = in + return &tasks.CreateTaskResponse{ + ContainerID: in.ContainerID, + Pid: 1, + }, nil +} + +func (ts *fakeTaskService) Get(ctx context.Context, in *tasks.GetRequest, opts ...grpc.CallOption) (*tasks.GetResponse, error) { + return nil, errgrpc.ToGRPC(errdefs.ErrNotFound) +} + +func (ts *fakeTaskService) Delete(ctx context.Context, in *tasks.DeleteTaskRequest, opts ...grpc.CallOption) (*tasks.DeleteResponse, error) { + return nil, errgrpc.ToGRPC(errdefs.ErrNotFound) +} diff --git a/integration/client/container_linux_test.go b/integration/client/container_linux_test.go index 6a6513446..d05eca3e6 100644 --- a/integration/client/container_linux_test.go +++ b/integration/client/container_linux_test.go @@ -1090,6 +1090,19 @@ func TestContainerRuntimeOptionsv2(t *testing.T) { if !strings.Contains(err.Error(), `"no-runc"`) { t.Errorf("task creation should have failed because of lack of executable. Instead failed with: %v", err.Error()) } + + // It doesn't matter what the NewTaskOpts function is. We are using an existing function in the client package, + // which will cause the TaskOptions in the new task request to be non-empty. + // https://github.com/containerd/containerd/issues/11568 + task, err = container.NewTask(ctx, empty(), WithNoNewKeyring) + if err == nil { + t.Errorf("task creation should have failed") + task.Delete(ctx) + return + } + if !strings.Contains(err.Error(), `"no-runc"`) { + t.Errorf("task creation should have failed because of lack of executable. Instead failed with: %v", err.Error()) + } } func TestContainerKillInitPidHost(t *testing.T) {