guest: use OCIBundlePath as sandbox root source of truth by shreyanshjain7174 · Pull Request #2653 · microsoft/hcsshim

shreyanshjain7174 · 2026-03-31T12:55:20Z

Guest-side GCS currently derives sandbox paths by combining a hard-coded prefix (/run/gcs/c) with the sandbox ID, ignoring the OCIBundlePath the host already sends. This breaks with Shim v2 path layouts and multi-pod UVMs.

This PR stores settings.OCIBundlePath as the canonical sandbox root on the Host struct (sandboxRoots map), and threads the resolved root through all setup and cleanup functions instead of re-deriving it.

For virtual pods, the sandbox root is derived from OCIBundlePath's parent directory (preserving the virtual-pods/ layout). For workload containers, it's looked up from the mapping. A legacy fallback ensures backwards compatibility with shim v1.

When the old shim sends OCIBundlePath in the legacy format, all paths are identical to before — verified by parity tests and a full CRI lifecycle (crictl runp/create/start/stop/rm) on a Hyper-V UVM.

Depends on: #2655 (bug fix for leaked LCOW resources) — both PRs modify uvm.go cleanup paths. Will rebase after #2655 merges.

rawahars

Overall love the simplification of the virtual pod vs normal pod differentiation.

Major comments are regarding keeping existing utility methods, while removing virtual pod specific ones and then changing these methods to return paths/values which you are creating on the fly. That would make it easier to review too.

internal/guest/runtime/hcsv2/container.go

internal/guest/runtime/hcsv2/sandbox_container.go

internal/guest/runtime/hcsv2/uvm.go

internal/guest/runtime/hcsv2/workload_container.go

internal/guest/runtime/hcsv2/uvm.go

rawahars · 2026-03-31T21:03:48Z

@anmaxvl Can you please take a look too to see if any invariant of Security Policy gets impacted with this change. As I see it that should not change but please do confirm that.

anmaxvl · 2026-04-01T00:20:19Z

@micromaomao as well.

micromaomao · 2026-04-01T00:42:56Z

@anmaxvl:

@micromaomao as well.

I will have a closer look tomorrow, but I think this is probably good because #2581 added validation to settings.OCIBundlePath (not in this PR's branch yet but in main) to force it to be the expected value.

shreyanshjain7174 · 2026-04-01T06:41:23Z

@rawahars Addressed all 8 review comments. Also added scripts/validate-sandbox-paths.ps1 — a portable E2E validation script that exercises real CRI pod lifecycles via crictl and validates that /etc/hostname, /etc/hosts, and /etc/resolv.conf are correctly mounted from the sandbox root. It tests single-pod, sandbox+workload, and sequential multi-pod scenarios. All 13 checks pass locally.

Do you think this script is worth keeping in the repo for manual validation, or should it go in a separate PR / be dropped from this one?

rawahars

Major driving comment are that for the current change, try to align as close to original code structure as possible.

internal/guest/runtime/hcsv2/container.go

internal/guest/runtime/hcsv2/sandbox_container.go

internal/guest/runtime/hcsv2/standalone_container.go

internal/guest/runtime/hcsv2/uvm.go

internal/guest/runtime/hcsv2/workload_container.go

shreyanshjain7174 · 2026-04-01T16:08:12Z

Thanks for the detailed round 2 review. Pushed an update addressing all the comments:

Restored section comments in Container.Delete (remove user mounts, tmpfs mounts, hugepages)
Added utility methods back: getSandboxHostnamePath/HostsPath/ResolvPath in sandbox_container.go, getStandaloneHostnamePath/HostsPath/ResolvPath in standalone_container.go
Restored original setup function names: setupSandboxMountsPath, setupSandboxTmpfsMountsPath, etc.
Moved the root-based path helpers (SandboxMountsDirFromRoot, SandboxTmpfsMountsDirFromRoot, etc.) into spec.go alongside the existing ID-based methods
Moved GenerateWorkloadContainerNetworkMountsFromRoot into spec.go as well, removed the local inline version
Added whitespace and comments in registerSandboxRoot for readability
Kept the VirtualPod* methods in spec.go for now since GenerateWorkloadContainerNetworkMounts (used by securitypolicy) still depends on VirtualPodAwareSandboxRootDir. Happy to remove them in a follow-up once we wire that caller through the root-based path too.

All tests pass locally (unit + E2E via crictl).

shreyanshjain7174 · 2026-04-01T17:16:15Z

Note: this PR has a merge conflict with #2655 (bug fix for leaked LCOW resources) in uvm.go — specifically around RemoveContainer and the virtual pod cleanup paths. Once #2655 merges, I'll rebase on top of it.

helsaawy

Can we switch entirely to the *FromRoot functions and delete the [VirtualPodAware]*MountsDir/Source functions in internal/guest/spec?

helsaawy · 2026-04-01T17:05:23Z

internal/guest/runtime/hcsv2/workload_container.go

+func updateSandboxMounts(sandboxRoot string, spec *oci.Spec) error {
+	mountsDir := specGuest.SandboxMountsDirFromRoot(sandboxRoot)
+	tmpfsMountsDir := specGuest.SandboxTmpfsMountsDirFromRoot(sandboxRoot)



Might be worthwhile to add Sandbox[Tmpfs]MountsSourceFromRoot or something similar.

Added SandboxMountSourceFromRoot, SandboxTmpfsMountSourceFromRoot, and HugePagesMountSourceFromRoot to spec.go. The updateSandboxMounts and updateHugePageMounts functions in workload_container.go now use these instead of inline filepath.Join + TrimPrefix.

Replace heuristic sandbox path derivation (hard-coded /run/gcs/c prefix + ID) with host-provided OCIBundlePath as the canonical sandbox root directory. This change prepares the guest-side GCS for Shim v2 and multi-pod UVM support, where the host may use a different path layout than the legacy /run/gcs/c/<id>. Key changes: - Add sandboxRoots mapping on Host to store resolved sandbox root per sandbox ID - Sandbox containers: register OCIBundlePath as sandbox root - Virtual pods: derive sandbox root from OCIBundlePath parent + /virtual-pods/<id> - Workload containers: resolve sandbox root from Host mapping (fallback to legacy) - Standalone containers: use OCIBundlePath directly as root - Container.Delete: use stored sandboxRoot for cleanup paths - Remove duplicate setup functions (setupVirtualPod* merged into unified setup*) The refactor produces identical paths when the old shim sends OCIBundlePath in the legacy format, ensuring zero behavior change for existing deployments. Security: virtualPodID is validated against path traversal before use. Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>

rawahars · 2026-04-06T16:45:48Z

internal/guest/runtime/hcsv2/container.go

-		}
-
-		// remove user mounts in sandbox container - use virtual pod aware paths
-		if err := storage.UnmountAllInPath(ctx, specGuest.VirtualPodAwareSandboxMountsDir(c.id, virtualSandboxID), true); err != nil {


Are we planning on removing VirtualPodAwareSandboxMountsDir and other utility methods in next PR?

Yes, the follow-up PR (guest-pod-unification branch) removes VirtualPodAwareSandboxMountsDir and the other VirtualPod* methods as part of the unified pod model.

rawahars · 2026-04-06T16:52:48Z

internal/guest/runtime/hcsv2/standalone_container.go

+	// Set cgroup path
+	virtualSandboxID := spec.Annotations[annotations.VirtualPodID]
 	if virtualSandboxID != "" {
-		// Standalone container in virtual pod goes under /containers/virtual-pods/{virtualSandboxID}/{containerID}


nit: Consider adding this comment in next PR when you do the refactoring.

Will add in the follow-up.

rawahars · 2026-04-06T16:55:30Z

internal/guest/runtime/hcsv2/uvm.go

+	// sandboxRoots maps sandboxID to the resolved sandbox root directory.
+	// Populated via RegisterSandboxRoot during sandbox creation using
+	// the host-provided OCIBundlePath as source of truth.
+	sandboxRootsMutex sync.RWMutex


nit: Why not just use the containersMutex? If sandboxRoots is always used under containersMutex then you can skip sandboxRootsMutex.

Good point — sandboxRoots is only accessed within CreateContainer and RemoveContainer which both hold containersMutex. I'll consolidate to use containersMutex only in the follow-up to keep this PR minimal.

shreyanshjain7174 requested a review from a team as a code owner March 31, 2026 12:55

shreyanshjain7174 force-pushed the sandbox-path-refactor branch 2 times, most recently from bbc4d4d to 7deb168 Compare March 31, 2026 13:05

shreyanshjain7174 requested a review from rawahars March 31, 2026 16:04

shreyanshjain7174 force-pushed the sandbox-path-refactor branch from a67564e to 9de87ec Compare March 31, 2026 16:37

rawahars requested a review from helsaawy March 31, 2026 20:28

rawahars requested changes Mar 31, 2026

View reviewed changes

shreyanshjain7174 force-pushed the sandbox-path-refactor branch 2 times, most recently from c7fcebd to e1f8a6f Compare April 1, 2026 06:41

shreyanshjain7174 requested a review from rawahars April 1, 2026 06:41

rawahars requested changes Apr 1, 2026

View reviewed changes

shreyanshjain7174 force-pushed the sandbox-path-refactor branch from e1f8a6f to 0fb9f0b Compare April 1, 2026 16:07

shreyanshjain7174 force-pushed the sandbox-path-refactor branch 2 times, most recently from 08c0b9e to 7c4f900 Compare April 1, 2026 16:49

helsaawy reviewed Apr 1, 2026

View reviewed changes

shreyanshjain7174 force-pushed the sandbox-path-refactor branch from 7c4f900 to 003e8b3 Compare April 2, 2026 06:16

shreyanshjain7174 requested review from helsaawy and rawahars April 3, 2026 06:57

shreyanshjain7174 force-pushed the sandbox-path-refactor branch from 003e8b3 to c73b7f3 Compare April 6, 2026 10:03

shreyanshjain7174 mentioned this pull request Apr 6, 2026

guest: unify pod model for V1, virtual pod, and V2 shim support #2662

Open

rawahars reviewed Apr 6, 2026

View reviewed changes

rawahars approved these changes Apr 6, 2026

View reviewed changes

Conversation

shreyanshjain7174 commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rawahars left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rawahars commented Mar 31, 2026

Uh oh!

anmaxvl commented Apr 1, 2026

Uh oh!

micromaomao commented Apr 1, 2026

Uh oh!

shreyanshjain7174 commented Apr 1, 2026

Uh oh!

rawahars left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shreyanshjain7174 commented Apr 1, 2026

Uh oh!

shreyanshjain7174 commented Apr 1, 2026

Uh oh!

helsaawy 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shreyanshjain7174 commented Mar 31, 2026 •

edited

Loading