google / gvisor-containerd-shim

containerd shim for gVisor
https://gvisor.dev
Apache License 2.0
79 stars 30 forks source link

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

Closed jmillikin-stripe closed 5 years ago

jmillikin-stripe commented 5 years ago

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 commented 5 years ago

PR: https://github.com/google/gvisor-containerd-shim/pull/20

Random-Liu commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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

jmillikin-stripe commented 5 years ago

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 commented 5 years ago

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.