fix the task setting the runtime path #8208

Iceber · 2023-03-06T07:29:27Z

containerd/pkg/cri/server/container_start.go

Lines 124 to 126 in 5da7e2c

    
           if ociRuntime.Path != "" { 
        
           	taskOpts = append(taskOpts, containerd.WithRuntimePath(ociRuntime.Path)) 
        
           }

Although the runtime path is set in the cri server, the runtime path is not set to the request in the NewTask

Add TaskInfo.RuntimePath field to prevent overwriting TaskInfo.runtime, directly overwriting TaskInfo.runtime may cause exceptions to other NewTaskOpts

k8s-ci-robot · 2023-03-06T07:29:37Z

Hi @Iceber. 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.

mxpv · 2023-03-07T17:49:03Z

task.go

+	// RuntimePath is an absolute path that can be used to overwrite path
+	// to a shim runtime binary.
+	RuntimePath string
+


runtime field is now unused? Do we need to keep both?

The runtime field will be set to container.Runtime.Name, which will be used for TaskOpts such as WithNoPivotRoot to determine the runtime，it should not be changed

containerd/task_opts_unix.go

Lines 55 to 60 in 29e10a1

func WithNoPivotRoot(_ context.Context, _ *Client, ti *TaskInfo) error {

if CheckRuntime(ti.Runtime(), "io.containerd.runc") {

if ti.Options == nil {

ti.Options = &options.Options{}

}

opts, ok := ti.Options.(*options.Options)

Could you add comment lines to explain that?

Added some comments to the runtime field

The problem is that the client doesn't use the services/tasks/v1.CreateTaskRequest.RuntimePath.

Container info has field named by Runtime.Name. It's consistency with keeping both. RuntimePath is more like override option.

Iceber · 2023-03-14T04:50:13Z

@AkihiroSuda @dmcgowan PTAL, Is there anything that needs to be modified?

AkihiroSuda · 2023-03-14T04:55:06Z

task_opts.go

 func WithRuntimePath(absRuntimePath string) NewTaskOpts {
 	return func(ctx context.Context, client *Client, info *TaskInfo) error {
-		info.runtime = absRuntimePath
+		info.RuntimePath = absRuntimePath


Is this a breaking change?

This is not a breaking change.

In the past if the user used WithRuntimePath alone then the runtime path would not take effect.

And if the user uses WithRuntimePath + other NewTaskOpts (e.g. WithNoPivotRoot), then the wrong runtime options are generated and the task creation fails

WithNoPivotRoot and other NewTaskOpts generate the runtime options based on runtime.
For io.containerd.runc.* runtime, if runtime changes to runtime path, the generated option type is not *options.Options

containerd/task_opts_unix.go

Lines 55 to 70 in 29e10a1

func WithNoPivotRoot(_ context.Context, _ *Client, ti *TaskInfo) error {

if CheckRuntime(ti.Runtime(), "io.containerd.runc") {

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.NoPivotRoot = true

} else {

if ti.Options == nil {

ti.Options = &runctypes.CreateOptions{

NoPivotRoot: true,

}

return nil

AkihiroSuda · 2023-03-14T04:55:20Z

container.go

 			})
 		}
 	}
+	request.RuntimePath = info.RuntimePath


Can we have a test?

I added a test, on linux it is green, but on windows it cannot find the shim binary.
https://github.com/containerd/containerd/actions/runs/4414423604/jobs/7736233010#step:16:37

container_test.go:269: failed to start shim: start failed: : exec: "C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\shim1680046787": file does not exist: unknown

Added .exe suffix after the file name, no more errors reported.

@AkihiroSuda It's green on windows and linux, PTAL

AkihiroSuda · 2023-03-14T23:56:37Z

integration/client/container_test.go

+	return string(shimPath), nil
+}
+
+func copyShim(shimPath string) (string, error) {


Probably you can just use https://pkg.go.dev/github.com/containerd/[email protected]/fs#CopyFile

Updated, Thanks

Iceber · 2023-03-15T08:43:40Z

failed tests are not related to pr, need to retest

Iceber · 2023-03-21T04:29:56Z

@estesp PTAL, Thanks

Signed-off-by: Iceber Gu <[email protected]>

fuweid

LGTM

fuweid · 2023-03-31T04:25:48Z

task.go

+	// RuntimePath is an absolute path that can be used to overwrite path
+	// to a shim runtime binary.
+	RuntimePath string
+


The problem is that the client doesn't use the services/tasks/v1.CreateTaskRequest.RuntimePath.

Container info has field named by Runtime.Name. It's consistency with keeping both. RuntimePath is more like override option.

Iceber · 2023-03-31T06:15:34Z

@fuweid It looks useful to cherry-pick to release/1.6 and release/1.7

fuweid · 2023-03-31T08:03:45Z

@Iceber please go ahead. Thanks

k8s-ci-robot added the needs-ok-to-test label Mar 6, 2023

mxpv reviewed Mar 7, 2023

View reviewed changes

AkihiroSuda reviewed Mar 14, 2023

View reviewed changes

Iceber force-pushed the fix_runtime_path branch 6 times, most recently from 8ee39db to 837c5ec Compare March 14, 2023 12:58

AkihiroSuda reviewed Mar 14, 2023

View reviewed changes

Iceber force-pushed the fix_runtime_path branch 2 times, most recently from 1495c4d to 29e9850 Compare March 15, 2023 06:36

AkihiroSuda approved these changes Mar 15, 2023

View reviewed changes

Iceber requested a review from mxpv March 17, 2023 03:05

WithRuntimePath uses the TaskInfo.RuntimePath field

97064b0

Signed-off-by: Iceber Gu <[email protected]>

Iceber force-pushed the fix_runtime_path branch from 29e9850 to a7be4f2 Compare March 29, 2023 03:35

integration: add container start test using abs runtime path

c89438e

Signed-off-by: Iceber Gu <[email protected]>

Iceber force-pushed the fix_runtime_path branch from a7be4f2 to c89438e Compare March 29, 2023 03:55

fuweid approved these changes Mar 31, 2023

View reviewed changes

fuweid merged commit 988ee8f into containerd:main Mar 31, 2023

fuweid added cherry-pick/1.6.x cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Mar 31, 2023

Iceber mentioned this pull request Apr 27, 2023

[release/1.7] fix the task setting the runtime path #8453

Merged

Iceber mentioned this pull request Apr 27, 2023

[release/1.6] fix the task setting the runtime path #8454

Merged

dmcgowan added cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch cherry-pick/1.6.x labels May 2, 2023

	if ociRuntime.Path != "" {
	taskOpts = append(taskOpts, containerd.WithRuntimePath(ociRuntime.Path))
	}

	func WithNoPivotRoot(_ context.Context, _ Client, ti TaskInfo) error {
	if CheckRuntime(ti.Runtime(), "io.containerd.runc") {
	if ti.Options == nil {
	ti.Options = &options.Options{}
	}
	opts, ok := ti.Options.(*options.Options)

fix the task setting the runtime path #8208

fix the task setting the runtime path #8208

Uh oh!

Conversation

Iceber commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iceber Mar 8, 2023 • 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!

Iceber commented Mar 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iceber Mar 14, 2023 • 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!

Iceber Mar 14, 2023 • 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!

Iceber commented Mar 15, 2023

Uh oh!

Iceber commented Mar 21, 2023

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iceber commented Mar 31, 2023

Uh oh!

fuweid commented Mar 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Iceber commented Mar 6, 2023 •

edited

Loading

Iceber Mar 8, 2023 •

edited

Loading

Iceber Mar 14, 2023 •

edited

Loading

Iceber Mar 14, 2023 •

edited

Loading