refactor(shim): precompute guest rootfs after task create by sidneychang · Pull Request #702 · urunc-dev/urunc

sidneychang · 2026-05-18T18:29:07Z

Description

This PR precomputes guest rootfs in the urunc containerd task shim after inner TaskService.Create mounts bundle/rootfs, so block-backed snapshots (e.g. devmapper) can be selected without relying on getMountInfo on an unmounted path.

Problem

Previously, guest rootfs selection was performed only in the runtime Exec() path. In the containerd shim path, this made block-backed container rootfs detection fragile, because the runtime had to rediscover the guest rootfs from the bundle rootfs path later through getMountInfo.

For block-backed snapshots such as devmapper, the rootfs decision should be made only after the wrapped containerd task service has completed Create, when the bundle rootfs has already been prepared by the normal containerd/runc create flow. This allows urunc to reuse the existing rootfs selection logic on the actual bundle rootfs path, instead of introducing a separate shim-only rootfs discovery path.

Solution

This PR moves guest rootfs selection into the urunc containerd task shim after the wrapped TaskService.Create succeeds.

The shim now:

lets the inner task service run Create first;
reads the bundle config.json;
obtains the unikernel annotations and urunc configuration;
calls the shared ChooseRootfs implementation with the bundle path and OCI spec.Root.Path;
stores the selected RootfsParams in config.json under the internal annotation com.urunc.internal.rootfs.params.

Then, runtime Exec() consumes this precomputed rootfs result when the annotation is present. If the annotation is absent, such as in direct urunc / podman-style flows, Exec() falls back to running ChooseRootfs itself. This keeps the original runtime behavior while making the containerd shim path more reliable for block-backed rootfs selection.

Related issues

Fix Move guest rootfs selection earlier to the shim #684

How was this tested?

LLM usage

Cursor

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-18T18:29:13Z

✅ Deploy Preview for urunc canceled.

Name	Link
🔨 Latest commit	`9e58d34`
🔍 Latest deploy log	https://app.netlify.com/projects/urunc/deploys/6a1020fafe45d00008bdea8e

sidneychang · 2026-05-19T04:34:09Z

Hi @cmainas ,
Could you please take a look at PR #702 when you have time?
Thanks!

cmainas

Hello @sidneychang ,

I have added several comments in some changes. Moreover:

I do not think we need to encode for the new rootfs annotations that the shim creates. Please let me know if I am wrong.
Why do not we perform the chooseRootfa functionality after TaskService.Create? THis will simplify a lot the changes.
The logic of ChooseRootfs has changed a lot and it has made the code less cleaner. For example, it creates new instances for a unikernel interface multiple times, while just a single instance is enough as the original code.
The code for annotations is all around the place. Just place it in one file and let it be ugly. We will just throw it away.

cmainas · 2026-05-20T11:57:03Z

+	if err := conf.validate(); err != nil {
+		return nil, err
+	}
+	if err := conf.decode(); err != nil {


We do not need to decode the annotations passed form the shim. They should be plain test.

cmainas · 2026-05-20T12:02:26Z

 	return jsonConf, nil
 }

+// GetUnikernelConfigFromSpecAnnotations retrieves urunc configuration only


Since this function is called from the shim, then the annotations should already be in the config and hence we can reuse the previous function which will not fallback to urunc.json if the annotations exist in the config.json

cmainas · 2026-05-20T12:05:39Z

+	// Decode from State.Annotations only: the shim writes the internal key to
+	// Spec (config.json), and saveContainerState copies missing Spec keys into
+	// state.json during create/InitialSetup before reexec reaches Exec.


While this is correct and we should follow it across urunc, we do not need an explicit comment just for that..

cmainas · 2026-05-20T12:12:02Z

+
+// DecodeRootfsParams reads rootfs parameters from OCI annotations. The bool
+// return value reports whether the internal annotation was present.
+func DecodeRootfsParams(annotations map[string]string) (types.RootfsParams, bool, error) {


We can derive if the shim has done the rootfs selection by checking the MonRootfs value when it gets returned.

cmainas · 2026-05-20T12:58:53Z

+		return nil
+	}
+
+	unikernel, err := unikernels.New(unikernelType)


This check should not be performed here, but as before we should try each one of the options in one place.

cmainas · 2026-05-20T12:59:50Z

+// bundle layout and annotations. specRoot may be relative to bundle. rootfsMounts
+// carries CreateTask mounts from the shim; when nil, block backing may be probed
+// from the live container rootfs mount.
+func ChooseRootfs(bundle, specRoot string, annot map[string]string, cfg *UruncConfig, rootfsMounts []RootfsMount) (types.RootfsParams, error) {


This function changes the whole logic of choosing rootfs a lot. I do not really see why this is necessary and the previous logic should remain the same.

cmainas · 2026-05-20T13:03:21Z

+	if containerRootfsBlock == nil && shouldMountContainerRootfs(annot) {
+		unikernelPreview, err := unikernels.New(annot[annotType])
+		if err != nil {
+			return types.RootfsParams{}, err
+		}
+		if unikernelPreview.SupportsBlock() {
+			rootFsDevice, err := getMountInfo(rootfsDir)
+			if err == nil {
+				containerRootfsBlock = &rootFsDevice
+			} else if !errors.Is(err, ErrMountpoint) {
+				uniklog.Errorf("failed to get container's rootfs mount info: %v", err)
+			}
+		}
+	}


I do not really see the benefit if this checks here. We can directly call or use the logic from selectRootfs

cmainas · 2026-05-20T13:04:14Z

-	annotValue := rs.annot[annotMountRootfs]
+	return shouldMountContainerRootfs(rs.annot)
+}
+
+func shouldMountContainerRootfs(annot map[string]string) bool {
+	annotValue := annot[annotMountRootfs]


This would not be necessary if the logic of chooseRootfs remained the same.

cmainas · 2026-05-20T13:05:02Z

+func RootfsParamsAnnotation() string {
+	return annotInternalRootfsParams
+}
+


Unnecessary function

sidneychang · 2026-05-21T09:03:34Z

Thank you @cmainas for the detailed review and helpful suggestions.

I have updated the code according to your comments.

cmainas

Thank you @sidneychang for addressing the comments. The code seems way better now. I have added some not that important comments and uurnc ensures that even if the configuration does not exist, the defaults values will be used. It would be very helpful if we could also do that in the shim, reusing the configuration part from urunc.

cmainas · 2026-05-21T09:42:41Z

+		if u.Spec.Root != nil {
+			specRoot = u.Spec.Root.Path
+		}
+		rootfsParams, err = ChooseRootfs(u.State.Bundle, specRoot, u.State.Annotations, u.UruncCfg)


At this point we should assume that the u.Spec.Root exists. Not that important though.

cmainas · 2026-05-21T09:45:45Z

-	}
-	res.MonRootfs = monRootfs
-
-	return res, nil


I am not sure if we need to change this function. It is called from chooseRootfs and even in the levle of shim or in the level of urunc, there is no issue of creating it. We just need to make sure to delete it later.

Tracking who created the directory might be a nightmare though.

cmainas · 2026-05-21T09:50:18Z

 		}
 	}

+	if _, err = os.Stat(rootfsParams.MonRootfs); err != nil {


It seems we can directly call MkdiAll. According to the documentation "If path is already a directory, MkdirAll does nothing and returns nil. " from https://pkg.go.dev/os#MkdirAll

sidneychang · 2026-05-21T15:21:04Z

Hi @cmainas —
I found that in the runtime code we already get default extra_binaries via UruncConfigFromMap / defaultExtraBinConfig(), but LoadUruncConfig (used by the shim) did not seed that before decoding config.toml.

When config.toml had only [log] and no [extra_binaries]:LoadUruncConfig used cfg := &UruncConfig{} and did not pre-seed defaultExtraBinConfig() (unlike UruncConfigFromMap), so a successful decode left ExtraBins empty → shim ChooseRootfs got an empty virtiofsd path → tryVirtiofs failed fileExists(vfsdPath) → Cloud Hypervisor only supports virtio shared-fs (no 9p fallback), so guest rootfs could not be chosen on shim Create.

Fix: initialize ExtraBins with defaultExtraBinConfig() before toml.DecodeFile, matching runtime behavior:

cfg := &UruncConfig{ExtraBins: defaultExtraBinConfig()}

CI should be green now.

cmainas · 2026-05-22T07:00:05Z

Hello @sidneychang , this is a nice catch. However, should not we use the default config for all the fields of the configuration? I mean we should extend your change and initialize cfg variable with he default config before the decoding takes place. In that way all non-specified values will be set to the default ones. However, we need to ensure that https://github.com/BurntSushi/toml does indeed follow this behavior.

Run ChooseRootfs in the shim after inner task Create, once the bundle rootfs is mounted (urunc-dev#684). Persist JSON RootfsParams in config.json as com.urunc.internal.rootfs.params so Exec reuses the choice; read state then spec annotations because reexec may not yet mirror config.json. Export ChooseRootfs from unikontainers. When the annotation is absent, Exec runs the same selection as the former u.chooseRootfs() (podman and urunc CLI). Ensure MonRootfs exists before rootfs setup in Exec. Fixes: urunc-dev#684 Signed-off-by: sidneychang <2190206983@qq.com>

cmainas

Thank you @sidneychang for this change.

Run ChooseRootfs in the shim after inner task Create, once the bundle rootfs is mounted (#684). Persist JSON RootfsParams in config.json as com.urunc.internal.rootfs.params so Exec reuses the choice; read state then spec annotations because reexec may not yet mirror config.json. Export ChooseRootfs from unikontainers. When the annotation is absent, Exec runs the same selection as the former u.chooseRootfs() (podman and urunc CLI). Ensure MonRootfs exists before rootfs setup in Exec. Fixes: #684 PR: #702 Signed-off-by: sidneychang <2190206983@qq.com> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>

cmainas reviewed May 20, 2026

View reviewed changes

sidneychang force-pushed the feat/shim-rootfs-precompute branch 7 times, most recently from 88a6e4c to e813c54 Compare May 21, 2026 08:59

sidneychang changed the title ~~refactor(shim): precompute guest rootfs before task create~~ refactor(shim): precompute guest rootfs after task create May 21, 2026

sidneychang force-pushed the feat/shim-rootfs-precompute branch from 1552ffa to e813c54 Compare May 21, 2026 09:33

cmainas reviewed May 21, 2026

View reviewed changes

sidneychang force-pushed the feat/shim-rootfs-precompute branch from e813c54 to d7beeca Compare May 21, 2026 10:19

sidneychang marked this pull request as ready for review May 21, 2026 10:23

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

sidneychang force-pushed the feat/shim-rootfs-precompute branch from d7beeca to da580a7 Compare May 21, 2026 15:12

sidneychang force-pushed the feat/shim-rootfs-precompute branch from da580a7 to 0601f25 Compare May 22, 2026 09:18

sidneychang force-pushed the feat/shim-rootfs-precompute branch from 0601f25 to 9e58d34 Compare May 22, 2026 09:25

cmainas added the takeover label May 22, 2026

urunc-bot Bot changed the base branch from main to main-pr702 May 22, 2026 12:16

cmainas approved these changes May 22, 2026

View reviewed changes

cmainas merged commit 36aa933 into urunc-dev:main-pr702 May 22, 2026
34 checks passed

cmainas mentioned this pull request May 22, 2026

Merge the management of the config file from the shim #719

Open

mdryaan mentioned this pull request May 25, 2026

refactor(shim): unify config.json access via PatchConfig #722

Open

4 tasks

Conversation

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

Uh oh!

Description

Problem

Solution

Related issues

How was this tested?

LLM usage

Checklist

Uh oh!

netlify Bot commented May 18, 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 19, 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!

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 21, 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!

Choose a reason for hiding this comment

Uh oh!

sidneychang commented May 21, 2026

Uh oh!

cmainas commented May 22, 2026

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sidneychang commented May 18, 2026 •

edited

Loading

netlify Bot commented May 18, 2026 •

edited

Loading