Come up with a common interface for the sim communication with contianerd

[cmainas]

We currently have two PRs #639 and #565 which both create anew taskService plugin for urunc's contianerd shim and both setup a new connection with contianerd. Also, there will be some dependency on the annotations and therefore we need to come up with a plan to integrate both approaches in one.

Part of the integration is to come up with a common structure to handle and use the containerd's new connection and resource. For that purpose, let;s use this issue to coordinate the integration of the work and also decide the interface for containerd communication.

As a first step, I would suggest @jiwahn and @sidneychang to share the APIs of contianerd they interact with and how they handle the connection or any other resources.

[sidneychang]

I think the main integration point we need to align on is how the urunc shim should access containerd APIs.

Currently, PR #565 accesses containerd directly inside the task service logic. It creates the connection, initializes the container/image/content clients, and performs the annotation injection logic from there. This works, but if we integrate it with PR #639 in this form, the shared taskService may become too large. It would start mixing several different responsibilities: coordinating the task lifecycle, managing containerd communication, and implementing feature-specific logic.

I think we can make the integration cleaner by introducing a common shim-side containerd client/helper first.

This common client would provide the shared containerd access layer. For example, it can handle connection setup, connection cleanup, namespace propagation, error conversion, and low-level wrappers around the containerd APIs needed by urunc.

Then each feature can build its own logic on top of that common client:

• PR Feat/inject missing annot in shim #565 can expose a helper for annotation injection.
• PR feat(view): add per-container snapshot views for boot artifacts #639 can expose helpers for snapshot-view preparation and cleanup.
• The shared taskService only coordinates these helpers instead of containing their implementation details.

In other words, the structure could be:

taskService
  -> coordinates the task lifecycle and calls feature helpers

common containerd client/helper
  -> owns containerd connection and low-level API access

annotation helper
  -> owns PR #565-specific annotation injection logic

snapshot-view helper
  -> owns PR #639-specific snapshot-view logic

This would avoid having two independent task service plugins and two independent containerd connection implementations. It would also make taskService easier to read, because it would describe what needs to happen, while the feature helpers describe how each part is implemented.

After we agree on this direction, we can discuss how the common client should be organized internally. For example, we can either keep the wrappers in one file, or use one shared client/session and split the API wrappers by containerd subsystem, such as containers, images, content, snapshots, and leases. The important part is that both PRs share the same containerd access layer and keep feature-specific logic outside of taskService.

@jiwahn @cmainas What do you think about this direction? Do you have any suggestions on how we should define the common client/helper interface?

[jiwahn]

For context, here is a related discussion: #523 (comment)

[sidneychang]

I think the common containerd access layer should be a small shim-side client wrapper, instead of directly using the full high-level containerd.Client inside the task service or feature-specific logic.

There is a similar pattern in runwasi. The shim/instance side receives the containerd address and namespace, and then creates its own narrow containerd client wrapper. That wrapper is based on generated service clients, such as containers, images, content, and leases. It is used to load WASM modules from the OCI image and, in the precompile path, to manage content and leases. So shim-side access to containerd is not unprecedented, but the access should be scoped to the runtime-specific needs rather than exposing a general-purpose containerd client.

More concretely, in the runwasi WASM shim path, the instance configuration carries both the containerd address and the namespace. When a new instance is created, the shim-side code connects back to containerd and uses a small wrapper to:

• get the container metadata by container ID;
• get the image name from the container metadata;
• get the image target/manifest from the image service;
• read the manifest, image config, and WASM layers from the content store;
• optionally create a lease and write precompiled content back to the content store.

This is close to what we need in urunc for the annotation-injection path, where we also need to go from containerID -> image ref -> image target -> manifest/config. The difference is that urunc can keep the scope smaller: initially we only need to read image metadata/content for annotations, and use snapshots/leases for the snapshot-view feature. We do not need to expose content writes or a general-purpose containerd client API.

For urunc, I think we can follow the same idea, but keep the scope narrower. The common client layer can own:

• containerd address resolution;
• gRPC connection lifecycle;
• namespace propagation;
• lease context propagation;
• gRPC error conversion to containerd errdefs;
• narrow wrappers for the containerd subsystems that urunc actually needs.

One possible structure could be:

pkg/shimwrap/ctrd/
  factory.go      // address resolution and session creation
  session.go      // one connection plus typed subsystem clients
  context.go      // namespace and lease helpers
  errors.go       // errdefs conversion and error helpers
  containers.go   // container metadata helpers
  images.go       // image target helpers
  content.go      // content read / manifest helpers
  snapshots.go    // snapshot stat/view/mount/remove helpers
  leases.go       // lease create/delete helpers

Of course, the exact location and naming of this client layer can still be discussed. For example, it could be named ctrd, containerdapi, containerdclient, or it could remain under pkg/shimwrap. The structure above is mainly meant to describe the responsibility boundaries, not to mandate the final package layout.

The central type could be:

type Session struct {
    conn *grpc.ClientConn

    Containers *Containers
    Images     *Images
    Content    *Content
    Snapshots  *Snapshots
    Leases     *Leases
}

This layer should expose only containerd primitives, for example:

Containers.ImageRef(ctx, id)
Containers.SnapshotInfo(ctx, id)
Images.Target(ctx, ref)
Content.ReadBlob(ctx, digest, size)
Content.ResolveManifestDescriptor(ctx, target)
Snapshots.Stat(ctx, snapshotter, key)
Snapshots.GetOrCreateView(ctx, snapshotter, viewKey, parentKey)
Snapshots.Remove(ctx, snapshotter, key)
Leases.Create(ctx, leaseID)
Leases.Delete(ctx, leaseID)

Feature-specific logic should stay outside this layer. For example, this client layer should not know about com.urunc.unikernel.*, config.json patching, the snapshotView policy, urunc-view.json, or which snapshotters should be treated as block-based snapshotters. Those should live in separate annotation and snapshot-view helpers.

Then the task service only needs to coordinate the flow:

taskService.Create
  -> open one ctrd.Session
  -> annotation helper uses containers/images/content
  -> snapshot-view helper uses containers/snapshots/leases
  -> call the wrapped runc TaskService.Create

This allows PR #565 and PR #639 to share one containerd access path, instead of introducing two independent task plugins and two independent containerd connection implementations. It also keeps the shim from becoming a general-purpose containerd client: the API surface is narrow, explicit, and easier to review.

[jiwahn]

Hi @sidneychang, thanks for this detailed outline.

I think this comment breaks down most of the integration issues we need to resolve. From my understanding, we can split the work into roughly four parts:

1. introducing the urunc-specific TaskService / task plugin wrapper
2. introducing a small common containerd access wrapper
3. adding the annotation-injection-specific logic
4. adding the snapshot-view / lease-specific logic

It would also be good to decide who will take each part once the maintainers confirm the overall direction.

I agree with the idea of avoiding unnecessary APIs and only adding the containerd operations that urunc actually needs. The proposed structure and the Session type also look reasonable to me:

pkg/shimwrap/ctrd/
  factory.go
  session.go
  context.go
  errors.go
  containers.go
  images.go
  content.go
  snapshots.go
  leases.go

Happy with these separation as well.

One additional point from my side is that I may need a Docker-specific access path in some cases. For example, if Docker is using the legacy image store rather than the containerd image store, the image metadata may not be available through containerd’s content store. Docker Engine 29.0 and later uses the containerd image store by default for fresh installations, but upgraded installations can continue using the legacy graph-driver-based image store until the containerd image store is explicitly enabled. So I think we should keep this compatibility case in mind, even if the main path should remain the containerd-based one.
https://docs.docker.com/engine/storage/containerd

Overall, this covers pretty much everything from my perspective. Once @cmainas confirms the direction, I think we can move forward with this split.

[cmainas]

Hello @jiwahn and @sidneychang ,

In other words, the structure could be:

taskService
  -> coordinates the task lifecycle and calls feature helpers

common containerd client/helper
  -> owns containerd connection and low-level API access

annotation helper
  -> owns PR #565-specific annotation injection logic

snapshot-view helper
  -> owns PR #639-specific snapshot-view logic

The structure sounds good.

This would avoid having two independent task service plugins and two independent containerd connection implementations. It would also make taskService easier to read, because it would describe what needs to happen, while the feature helpers describe how each part is implemented.

There is no reason for two independent task service plugins. On the contrary, it should be one since the snasphot features depend on the annotations.

I think the common containerd access layer should be a small shim-side client wrapper, instead of directly using the full high-level containerd.Client inside the task service or feature-specific logic.

Yes, we should not import the whole containerd client for security reasons too.

There is a similar pattern in runwasi. The shim/instance side receives the containerd address and namespace, and then creates its own narrow containerd client wrapper. That wrapper is based on generated service clients, such as containers, images, content, and leases. It is used to load WASM modules from the OCI image and, in the precompile path, to manage content and leases. So shim-side access to containerd is not unprecedented, but the access should be scoped to the runtime-specific needs rather than exposing a general-purpose containerd client.

Yes, run-wasi is a good reference.

Feature-specific logic should stay outside this layer. For example, this client layer should not know about com.urunc.unikernel.*, config.json patching, the snapshotView policy, urunc-view.json, or which snapshotters should be treated as block-based snapshotters. Those should live in separate annotation and snapshot-view helpers.

Then the task service only needs to coordinate the flow:

taskService.Create
  -> open one ctrd.Session
  -> annotation helper uses containers/images/content
  -> snapshot-view helper uses containers/snapshots/leases
  -> call the wrapped runc TaskService.Create

I totally agree with the separation of containerd resources with each task (annotations, snapshots) and I would go even further and not allow each task to access unnecessary resources. As an example, for retrieving the annotations, there is no need to access leases and snapshots. Another thing to keep in mind is that the implementation should be agnostic and reusable across the shim manager, task service, process etc. As a first iteration, we can focus on the taskService part, but some of the logic will eventually move to other parts.

So let me refine / suggest changes of the previous proposal.

taskService.Create
-> open one ctrd.Session
-> annotation helper uses containers/images/content
-> snapshot-view helper uses containers/snapshots/leases
-> call the wrapped runc TaskService.Create

This workflow sounds good and the first step (open one ctrd Session) should be the part that creates the shared resources. In my opinion the shared resources could be the following:

 type containerdSession struct {
    conn *grpc.ClientConn
    Container *Containers // one, since this is at the task level, every task is one container
    ctx context.Context
 }

As previously mentioned, snapshots and leases are irrelevant for the annotations retrieval and the annotations part should not have access to them. Similarly, Images and content do not seem to be required for the view snapshots (please correct me for that). This will also be helpful for the configuration which might opt out annotations or snapshots. Therefore, these resources should get created form the respective functionality, using common helper functions.

Then as correctly mentioned, every functionality should be as a helper function rather than separate taskService or even all in one function. For each functionality, there will be a step to acquire the necessary extra containerd resources through the respective client helper functions (images, content, leases, snapshots, etc.). I am not sure if it is necessary to have such structure with so many files. We might end up with one simple small function per file which is not helpful. Instead, we can have a single pkg/containerd-shim/containerd_client.go file with all helper functions related to the connection with containerd. Like:

Splitting the work as @jiwahn, suggested sounds a good plan:

1. introducing the urunc-specific TaskService / task plugin wrapper

2. introducing a small common containerd access wrapper

3. adding the annotation-injection-specific logic

4. adding the snapshot-view / lease-specific logic

and for the first 2 parts, whoever gets assigned, I would strongly suggest that the other person reviews the code too.

One additional point from my side is that I may need a Docker-specific access path in some cases.

This is another important point, that we need to ensure that the design is extensible. However, for the time being though, I would suggest we drop the docker case. Let;s focus on the containerd case first.

Let me know your thoughts about this plan and let;s start with the implementation.

[sidneychang]

Hi @cmainas,

Thanks, that makes sense to me.

My current understanding is that, for the containerd API part, we do not necessarily need to put all service clients into the shared session. Instead, the common layer can mainly provide the shared session / connection and the necessary common client, such as the containers client if it is needed at the task level.

Then each feature helper can acquire or wrap only the extra containerd clients it actually needs. For example, the annotation helper can use only the clients required for annotation retrieval, while the snapshot-view helper can separately use the snapshot / lease-related clients if needed. This should keep the access scoped and avoid giving each feature unnecessary containerd resources.

If this understanding is correct, I think we should be able to move the first two prerequisites forward fairly quickly. From this perspective, the first two steps seem not to be strongly coupled with the later feature-specific logic. So I think we can complete them relatively soon, and then each feature can build its own logic on top of this common foundation.

[cmainas]

Hello @sidneychang , that is correct. If @jiwahn is also fine with the approach then we can move to assigning them and implementing them.

[jiwahn]

Great, no factory pattern for now. With the narrowed implementation scope, I don’t think we need to split the first two parts anymore. Since both #565 and #639 already have some task service-related implementation, we should be able to cherry-pick the relevant commits from either PR and build on top of them.

@sidneychang, would you be okay with opening a PR for the taskService wrapper and the common containerd access wrapper? I think that would help us move faster. I can review it and then adapt the #565 annotation-injection logic on top of the common helper.