Hi @abel-von. 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.

/ok-to-test

/cc @mxpv @dmcgowan @fuweid

Thanks for working on this!

Apologies it took a while to review.

I wonder if we should introduce 2 different implementations to handle shims ("legacy" + "sandbox"). Might add an extra burden. Bootstrap params look pretty generic and can be useful for both implementations (not just for sandboxed shims), we probably could add "features" field to bootstrap params to tell us whether shim needs to be sandboxed or not. I also not sure if we're going to deprecate "legacy" implementation any time soon.

Possible we could abstract lifecycle management and have just one ShimManager instead of introducing having 3 sub managers? Some napkin notes:

classDiagram
    Task <|-- SandboxedShim
    Task <|-- TaskedShim
    ShimManager .. Task

    class ShimManager {
         tasks []Task
    }
    class Task {
         Load()
         Shutdown()
         
         TaskService()
         SandboxService()
    }
    class SandboxedShim {
    }
    class TaskedShim {
    }

Thanks for reply @mxpv . I think when a task is SANDBOXED, containerd should no longer handle any shim lifecycle related logic for it. containerd will just get an address from the sandbox to call Task API. "SandboxedShim" is actually one kind of implementations of sandbox, and all the shim lifecycle management is moved into "shim sandbox controller", so TaskManager here can no longer handle the lifecycle of shim, TaskManager just create/start/stop/delete Tasks by calling the API through the address in the sandbox, without any knowledge of the existence of the shim.

There are maybe other kind of sandbox implementations, rather than implementing the runtime sandbox API, we may just only implement the sandbox controller API. In this case there maybe no such a thing as shim at all, the Task API can be served in a VM, or in a remote machine, or in a thread inside a sandbox controller, anywhere.

So I think if we split the Task into the "shimTask"(which containerd will manage the shim lifecycle for the task), and the "sandboxedTask"(which containerd just get the address from the sandbox to call Task API), we may get a cleaner codes to handle the two kinds of thing. When a task is a "shimTask" containerd will handle shim for it, when a task is a "sandboxedTask", containerd will handle sandbox for it.

PR needs rebase.

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 7 days unless new comments are made or the stale label is removed.

This PR was closed because it has been stalled for 7 days with no activity.