feat(shim): add task wrapper and containerd access session by sidneychang · Pull Request #657 · urunc-dev/urunc

sidneychang · 2026-05-11T16:28:34Z

Description

The containerd access layer adds a narrow Session helper that owns a
short-lived gRPC connection, namespace propagation, task-level container
metadata lookup, and gRPC error conversion. It keeps the raw connection private
and exposes only common task/container metadata through getters.

The generated service-client constructors are package-private helpers. They are
not exposed to feature code directly; later feature PRs can use them inside the
containerd access package to build narrow annotation or snapshot/view
access/resource constructors for only the clients each feature needs.

Related issues

Fix Come up with a common interface for the sim communication with contianerd #642
Related to Feat/inject missing annot in shim #565
Related to feat(view): add per-container snapshot views for boot artifacts #639
Related discussion: LFx Mentorship (urunc Optimize rootfs handling with block-based snapshotters) #523

How was this tested?

LLM usage

GPT-5

Checklist

I have read the contribution guide.
The linter passes locally (make lint).
The e2e tests of at least one tool pass locally (make test_ctr, make test_nerdctl, make test_docker, make test_crictl).
If LLMs were used: I have read the llm policy.

netlify · 2026-05-11T16:28:44Z

✅ Deploy Preview for urunc canceled.

Name	Link
🔨 Latest commit	`4bcd59f`
🔍 Latest deploy log	https://app.netlify.com/projects/urunc/deploys/6a0473733dd7d200073e0f37

sidneychang · 2026-05-11T16:36:12Z

Hi @cmainas and @jiwahn,

I opened this draft PR for the initial task service wrapper and narrow containerd access session.

There are still some implementation details to clean up, but I would like to first check whether the overall design direction looks reasonable to you.

Please let me know if this layering matches what we discussed, or if you think I should adjust the structure before continuing with the remaining details.

Thanks!

cmainas

Hello @sidneychang ,

I have added a few comments in the code. Please correct me if I have misunderstood something.

Also, is it necessary to wrap the manager too?

cmainas · 2026-05-12T07:08:52Z

+	namespace   string
+	containerID string
+
+	mu        sync.Mutex


Why do we need a mutex? As far as I understand we will have one container, which is going to get initialized once and then it should immutable.

cmainas · 2026-05-12T07:23:08Z

+
+// Container returns the task-level container metadata, fetching it once from
+// containerd and reusing it for subsequent feature helpers.
+func (s *ContainerdSession) Container(ctx context.Context) (*containersapi.Container, error) {


Since we are working in the task level we are handling one container per time. Therefore, we can add the lofic of obtaining the metadata when we create anew session.

sidneychang · 2026-05-12T08:05:28Z

Also, is it necessary to wrap the manager too?

Hello, @cmainas
I added this to hook the shim binary delete path, which goes through Manager.Stop instead of TaskService.Delete.

My idea was to keep all the shim wrapper logic in this PR, so the later view feature can build on top of it. If we prefer not to introduce it before it is actually used, I can drop it here and add the manager wrapper back in the view feature PR.

jiwahn

Hi Sidney, thanks for this implementation. I think this aligns really well with the direction we discussed in the issue. I left comment regarding resource scoping part. Please let me know if I misunderstood something.

jiwahn · 2026-05-12T11:23:04Z

+func (s *ContainerdSession) ContainersClient() containersapi.ContainersClient {
+	return containersapi.NewContainersClient(s.conn)
+}


This may not need to be exported, since it is currently only used internally by loadContainer()

jiwahn · 2026-05-12T11:25:33Z

+// ImagesClient returns a generated images service client scoped to the session
+// connection.
+func (s *ContainerdSession) ImagesClient() imagesapi.ImagesClient {
+	return imagesapi.NewImagesClient(s.conn)
+}
+
+// ContentClient returns a generated content service client scoped to the
+// session connection.
+func (s *ContainerdSession) ContentClient() contentapi.ContentClient {
+	return contentapi.NewContentClient(s.conn)
+}
+
+// SnapshotsClient returns a generated snapshots service client scoped to the
+// session connection.
+func (s *ContainerdSession) SnapshotsClient() snapshotsapi.SnapshotsClient {
+	return snapshotsapi.NewSnapshotsClient(s.conn)
+}
+
+// LeasesClient returns a generated leases service client scoped to the session
+// connection.
+func (s *ContainerdSession) LeasesClient() leasesapi.LeasesClient {
+	return leasesapi.NewLeasesClient(s.conn)
+}


In the issue discussion, my understanding was that each feature should only access the containerd resources it actually needs. For example, the annotation path should not need access to the snapshotter/lease clients, and the snapshot-view path should not need access to image/content clients unless required. This seems slightly different from the direction of limiting resource access per feature.

I am not sure if this is ideal, but we could do something like this while keeping these client constructors private.
WDYT @sidneychang @cmainas

type annotationResources struct { container *containersapi.Container images imagesapi.ImagesClient content contentapi.ContentClient } func newAnnotationResources(s *ContainerdSession) annotationResources { return annotationResources{ container: s.Container(), images: s.imagesClient(), content: s.contentClient(), } }

I agree with @jiwahn . We do not need to expose these functions and they can be called whenever required. In any case, it is just a stub, no reason to expose it.

cmainas

Hello @sidneychang , I have added some more comments.

Regarding the manager, I would suggest we keep it separately. The manager and the task have different semantics.

cmainas · 2026-05-12T12:38:48Z

+		return fmt.Errorf("get container %q: empty response", s.containerID)
+	}
+
+	s.container = resp.GetContainer()


Why do we need a double call to GetContainer? It has already been called and we can store the return value in a variable.

cmainas · 2026-05-12T12:41:07Z

+
+	if err := session.loadContainer(ctx); err != nil {
+		if closeErr := conn.Close(); closeErr != nil {
+			return nil, fmt.Errorf("%w; close containerd connection: %v", err, closeErr)


The first error here will not be very clear. Let's add a few words before the error., like loadContainer failed: %w.

cmainas · 2026-05-12T12:42:17Z

+	return s.container
+}
+
+func (s *ContainerdSession) loadContainer(ctx context.Context) error {


Since getting the container takes place once, we should not add it as a method for ContainerdSession, but just a helper function.

cmainas · 2026-05-12T12:43:26Z

+}
+
+// Namespace returns the containerd namespace bound to this session.
+func (s *ContainerdSession) Namespace() string {


Function name should be GetNamespace()

cmainas · 2026-05-12T12:43:59Z

+}
+
+// ContainerID returns the container ID bound to this session.
+func (s *ContainerdSession) ContainerID() string {


Function name should be GetContainerID()

cmainas · 2026-05-12T12:45:44Z

+}
+
+// Namespace returns the containerd namespace bound to this session.
+func (s *ContainerdSession) Namespace() string {
+	return s.namespace
+}
+
+// ContainerID returns the container ID bound to this session.
+func (s *ContainerdSession) ContainerID() string {
+	return s.containerID
+}
+
+// Container returns the task-level container metadata loaded during session
+// creation.
+func (s *ContainerdSession) Container() *containersapi.Container {


Function name should be GetContainer()

cmainas · 2026-05-12T12:46:33Z

+	return nil
+}
+
+func (s *ContainerdSession) withNamespace(ctx context.Context) context.Context {


This should be a helper function, no need to get access to ContainerdSession, since namespace is just a string.

cmainas · 2026-05-12T12:47:06Z

+func (s *ContainerdSession) ContainersClient() containersapi.ContainersClient {
+	return containersapi.NewContainersClient(s.conn)
+}


sidneychang · 2026-05-12T13:39:56Z

Hello @jiwahn @cmainas, Thanks for the reviews.

I initially exported the generated containerd client getters to avoid unused-code lint issues while the feature logic was not wired yet. Based on your feedback, I refined this so the constructors are private and the feature-specific resource structs expose only the clients each path needs.

Could you please take another look?

jiwahn · 2026-05-13T05:20:32Z

Thanks, Sidney. This looks good to me.

How about adding a short comment above OpenSession? It opens a containerd session and also loads the container metadata.

Other than that, I think this is good to go.

sidneychang · 2026-05-13T06:47:45Z

Thanks, @jiwahn. I have added the comment above OpenSession to clarify that it opens the containerd session and loads the container metadata.

cmainas

Hey all, I think there has been some misunderstanding.

We are trying to establish a common interface to use the shared containerd resources required form the annotations and the snapshots PRs later. We decided to only share the common parts and let the respective additions (annotations / snapshots) to retrieve whatever is necessary for them. I thought this will be the role of the Session struct. Based on the current implementation (BaseAccess), I am missing how the annotations / snapshots parts will have access to the containerd connection, which is not exposed to any of them.

I do not really understand the role of BaseAccess. Sure, we do not want to expose everything in each feature, but the containerd connection is necessary since in the snapshot part we instruct containerd to create a view snapshot.

Moreveor, why are we defining the AnnotationAccess and the SnapshotAccess in this PR? The upcoming PRs should be responsible to set and manage the data structures they require.

cmainas · 2026-05-13T07:48:01Z

+type BaseAccess struct {
+	namespace   string
+	containerID string
+	container   *containersapi.Container
+}
+
+func newBaseAccess(s *Session) BaseAccess {
+	return BaseAccess{
+		namespace:   s.namespace,
+		containerID: s.containerID,
+		container:   s.container,
+	}
+}


Why a new struct?

cmainas · 2026-05-13T07:49:28Z

+type AnnotationAccess struct {
+	BaseAccess
+
+	images  imagesapi.ImagesClient
+	content contentapi.ContentClient
+}
+
+func NewAnnotationAccess(s *Session) AnnotationAccess {
+	return AnnotationAccess{
+		BaseAccess: newBaseAccess(s),
+		images:     s.imagesClient(),
+		content:    s.contentClient(),
+	}
+}
+
+func (a AnnotationAccess) Images() imagesapi.ImagesClient {
+	return a.images
+}
+
+func (a AnnotationAccess) Content() contentapi.ContentClient {
+	return a.content
+}


Why defining in this PR the annotations parts?

cmainas · 2026-05-13T07:50:01Z

+type SnapshotViewAccess struct {
+	BaseAccess
+
+	snapshots snapshotsapi.SnapshotsClient
+	leases    leasesapi.LeasesClient
+}
+
+func NewSnapshotViewAccess(s *Session) SnapshotViewAccess {
+	return SnapshotViewAccess{
+		BaseAccess: newBaseAccess(s),
+		snapshots:  s.snapshotsClient(),
+		leases:     s.leasesClient(),
+	}
+}
+
+func (a SnapshotViewAccess) Snapshots() snapshotsapi.SnapshotsClient {
+	return a.snapshots
+}
+
+func (a SnapshotViewAccess) Leases() leasesapi.LeasesClient {
+	return a.leases
+}


Why defining in this PR the snapshot parts?

sidneychang · 2026-05-13T09:53:24Z

Sorry for the confusion, that was my mistake.

When I was thinking through how to avoid exposing the raw containerd connection directly to future feature-specific paths, I considered an access-type structure that was somewhat similar to inheritance / polymorphism. After reconsidering it, I realized that this was not the right abstraction here. During that experiment, I accidentally included some extra code in this PR.

I have fixed this by removing those access types.

cmainas · 2026-05-13T11:38:42Z

Thank you @sidneychang , if @jiwahn is also ok, we can rebase over main to merge.

jiwahn · 2026-05-13T12:44:13Z

Looks good to me too.

cmainas

Thank you @sidneychang for the addition and @jiwahn for the review.

PR: #657 Signed-off-by: sidneychang <2190206983@qq.com> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Reviewed-by: Jiwoo Ahn <ikwydls1314@gmail.com> Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>

Signed-off-by: sidneychang <2190206983@qq.com>

sidneychang force-pushed the feat/containerd-shim-access branch 2 times, most recently from f9fcc7b to 530c6dd Compare May 12, 2026 04:11

sidneychang marked this pull request as ready for review May 12, 2026 04:16

cmainas reviewed May 12, 2026

View reviewed changes

sidneychang force-pushed the feat/containerd-shim-access branch from 530c6dd to a888c83 Compare May 12, 2026 08:07

jiwahn reviewed May 12, 2026

View reviewed changes

cmainas reviewed May 12, 2026

View reviewed changes

sidneychang force-pushed the feat/containerd-shim-access branch from a888c83 to 457e776 Compare May 12, 2026 13:30

sidneychang force-pushed the feat/containerd-shim-access branch 2 times, most recently from 3c95b3d to 3e3c0dc Compare May 13, 2026 04:02

sidneychang force-pushed the feat/containerd-shim-access branch from 3e3c0dc to 7e843ed Compare May 13, 2026 06:16

cmainas reviewed May 13, 2026

View reviewed changes

sidneychang force-pushed the feat/containerd-shim-access branch 2 times, most recently from 36cb243 to ab565d0 Compare May 13, 2026 09:42

sidneychang force-pushed the feat/containerd-shim-access branch 2 times, most recently from ab565d0 to 838e1b8 Compare May 13, 2026 11:41

cmainas added the ok-to-test label May 13, 2026

sidneychang force-pushed the feat/containerd-shim-access branch 2 times, most recently from 62855e5 to 4bcd59f Compare May 13, 2026 12:49

cmainas added the takeover label May 13, 2026

urunc-bot Bot changed the base branch from main to main-pr657 May 13, 2026 15:04

cmainas approved these changes May 13, 2026

View reviewed changes

cmainas merged commit 7352aa1 into urunc-dev:main-pr657 May 13, 2026
34 checks passed

sidneychang added 2 commits May 13, 2026 20:49

feat(shim): add urunc task service wrapper

d951084

Signed-off-by: sidneychang <2190206983@qq.com>

feat(shim): add containerd access session

4bcd59f

Signed-off-by: sidneychang <2190206983@qq.com>

Conversation

sidneychang commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

How was this tested?

LLM usage

Checklist

Uh oh!

netlify Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for urunc canceled.

Uh oh!

sidneychang commented May 11, 2026

Uh oh!

cmainas left a comment

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!

sidneychang commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiwahn left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmainas left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

sidneychang commented May 12, 2026

Uh oh!

jiwahn commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sidneychang commented May 13, 2026

Uh oh!

cmainas left a comment

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!

sidneychang commented May 13, 2026

Uh oh!

cmainas commented May 13, 2026

Uh oh!

jiwahn commented May 13, 2026

Uh oh!

sidneychang commented May 11, 2026 •

edited

Loading

netlify Bot commented May 11, 2026 •

edited

Loading

sidneychang commented May 12, 2026 •

edited

Loading

jiwahn commented May 13, 2026 •

edited

Loading