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?

internal/guest/runtime/hcsv2/workload_container.go

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.

helsaawy

Unsure about the container root path change, since it will break functional tests (e.g., TestLCOW_TeeLogPath), and probably several of our CRI integration tests too

helsaawy · 2026-04-13T17:06:03Z

internal/guestpath/paths.go

 	// LCOWRootPrefixInUVM is the path inside UVM where LCOW container's root
 	// file system will be mounted
-	LCOWRootPrefixInUVM = "/run/gcs/c"
+	LCOWRootPrefixInUVM = "/run/gcs/pods"


this will probably break a lot of our functional and cri tests that hardcode the /run/gcs/c as a container's bundle prefix.

Good catch — reverted. LCOWRootPrefixInUVM stays as /run/gcs/c and hcsoci/create.go is unchanged from main. This PR now only touches the guest side: the GCS uses whatever OCIBundlePath the shim sends, making it path-agnostic. No shim-side changes needed.

helsaawy · 2026-04-13T17:07:31Z

internal/guest/runtime/runc/utils.go

+func getLogDir(bundlePath string) string {
+	return bundlePath
 }


can we just delete this? its more confusing to keep it since its basically the identity function

Done — deleted getLogDir entirely. makeLogDir and getLogPath now use bundlePath directly.

Refactor the guest runtime to derive all sandbox paths from the host-provided OCIBundlePath rather than reconstructing them from hardcoded prefixes. This makes the GCS path-agnostic: it works with any OCIBundlePath the shim sends, regardless of prefix. Key changes: - runc.NewRuntime() no longer takes a base log path; runc.log is co-located with each container's bundlePath - Host.sandboxRoots map tracks the resolved sandbox root per container - CreateContainer registers sandbox roots; RemoveContainer cleans up - Sandbox, standalone, and workload container setup functions accept a resolved sandboxRoot parameter - New *FromRoot helpers in spec.go for sandbox subdirectory paths - runtime.Runtime.CreateContainer now takes sandboxID parameter Signed-off-by: Shreyansh Jain <shreyanshjain7174@gmail.com> Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>

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

internal/guest/runtime/hcsv2/workload_container.go Show resolved Hide resolved

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

msscotb assigned helsaawy and rawahars Apr 7, 2026

shreyanshjain7174 force-pushed the sandbox-path-refactor branch from c73b7f3 to a4dbd30 Compare April 8, 2026 15:46

shreyanshjain7174 mentioned this pull request Apr 8, 2026

Adds guest-side GCS changes for V2 shim support #2669

Open

shreyanshjain7174 force-pushed the sandbox-path-refactor branch 5 times, most recently from 227430d to ef409ba Compare April 13, 2026 15:28

helsaawy reviewed Apr 13, 2026

View reviewed changes

shreyanshjain7174 requested a review from rawahars April 14, 2026 11:20

shreyanshjain7174 force-pushed the sandbox-path-refactor branch from ef409ba to 57ce748 Compare April 14, 2026 13:44

shreyanshjain7174 requested a review from helsaawy April 14, 2026 15:54

anmaxvl approved these changes Apr 14, 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shreyanshjain7174 commented Mar 31, 2026 •

edited

Loading