Skip to content
This repository has been archived by the owner on Apr 20, 2022. It is now read-only.

CreateTaskRequest options switch missing case for *runctypes.CreateOptions #19

Closed
jmillikin-stripe opened this issue Mar 13, 2019 · 7 comments
Labels
kind: bug Something isn't working

Comments

@jmillikin-stripe
Copy link
Contributor

In pkg/v2/service.go the Create() handler has a switch on option types. This switch is not handling one of the types that ContainerD 1.2.4 can produce, *runctypes.CreateOptions.

Additionally, the error message unsupported option type is extremely vague in that it doesn't indicate what part of the system the error comes from, or what the cause is.

@jmillikin-stripe
Copy link
Contributor Author

PR: #20

@Random-Liu
Copy link
Member

Random-Liu commented Mar 20, 2019

@jmillikin-stripe Are you using gvisor-containerd-shim with Kubernetes + containerd? Or just containerd?

With "Kubernetes + containerd", CreateOptions should never be used.
With just containerd, I don't think containerd client options are designed for non-runc runtimes right now, e.g. https://github.com/containerd/containerd/blob/master/task_opts_unix.go#L31

@jmillikin-stripe
Copy link
Contributor Author

I'm using it with containerd directly, not via Kubernetes or CRI.

It works fine except for this one particular missing type assertion, and the referenced PR does work for the subset of CreateOptions that gVisor supports.

@ianlewis
Copy link
Contributor

@jmillikin-stripe can you share your config.toml? Also, what exactly are you doing to get this error? I suppose you are creating a container via the containerd API rather than CRI?

@ianlewis
Copy link
Contributor

@Random-Liu I think the relevant code is here since it's 1.2.x. In this case there aren't any runtime checks since it wasn't implemented yet. I feel like we should probably handle this case.
https://github.com/containerd/containerd/blob/release/1.2/task_opts_unix.go

@ianlewis ianlewis added the kind: bug Something isn't working label Mar 22, 2019
@jmillikin-stripe
Copy link
Contributor Author

jmillikin-stripe commented Mar 22, 2019

I'm using the containerd API directly. The config.toml file has no effect on this behavior; the reproduction is in the following client code:

	var stdout, stderr bytes.Buffer
	task, err := container.NewTask(
		containerCtx,
		cio.NewCreator(
			cio.WithFIFODir(fifoDir),
			cio.WithStreams(
				nil, // stdin
				&stdout,
				&stderr,
			),
		),
		containerd.WithNoNewKeyring,
	)

The containerd.WithNoNewKeyring option is an optimization for runc containers. If set, the shim's creation request options will be set to a *runctypes.CreateOptions. I would like it if I could set that option unconditionally and let the shim ignore unknown options, but the failing type check here prevents that.

@Random-Liu
Copy link
Member

Random-Liu commented Mar 22, 2019

I'm using it with containerd directly, not via Kubernetes or CRI.

No wonder. That's what I guessed. :)

I think the relevant code is here since it's 1.2.x. In this case there aren't any runtime checks since it wasn't implemented yet. I feel like we should probably handle this case.

I'm fine with handling that :)
Just want to confirm this is caused by a directly use with containerd, not a bug in Kubernetes integration.

ianlewis pushed a commit that referenced this issue Mar 29, 2019
When ContainerD v1.2.4 creates a task, it may pass a *runctypes.CreateOptions in the request options field. This currently causes the gvisor-containerd-shim to reject the request.

This PR allows the shim to handle requests with creation options set, and also slightly improves the error message so future failures of this kind are easier to localize to the shim.

Fixes #19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants