Skip to content

fix(lcow/v2): report physically backed VM stats#2834

Open
shreyanshjain7174 wants to merge 1 commit into
microsoft:mainfrom
shreyanshjain7174:user/shsancheti/fix-lcowv2-physically-backed-stats
Open

fix(lcow/v2): report physically backed VM stats#2834
shreyanshjain7174 wants to merge 1 commit into
microsoft:mainfrom
shreyanshjain7174:user/shsancheti/fix-lcowv2-physically-backed-stats

Conversation

@shreyanshjain7174

@shreyanshjain7174 shreyanshjain7174 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • derive LCOW V2 controller memory backing from the HCS Memory.AllowOvercommit setting on cold boot and migration import
  • avoid looking up a vmmem process for physically backed UVMs
  • add focused coverage for the memory-backing decision

Problem

LCOW V2 physically backed sandbox stats had two failure modes:

  1. the controller sourced isPhysicallyBacked from the stronger FullyPhysicallyBacked device annotation instead of !Memory.AllowOvercommit, so a non-overcommitted VM could report a zero working set;
  2. Stats() unconditionally called LookupVMMEM before branching, even though physically backed UVMs use HCS AssignedMemory. When that lookup failed, containerd dropped the metric and callers observed an empty metrics response.

The change mirrors the established LCOW V1 behavior: memory is physically backed when overcommit is disabled, and vmmem working-set lookup is only used for VA-backed UVMs.

Validation

  • go test -tags "windows lcow" ./internal/controller/vm/... -count=1
  • go test -tags "windows lcow" ./internal/builder/vm/lcow/... -count=1
  • rebuilt containerd-shim-lcow-v2.exe from this branch and ran Test_SandboxStats_WorkingSet_PhysicallyBacked/LCOW against a local build 26200 host: PASS
  • restored the original deployed shim after the functional run

@shreyanshjain7174
shreyanshjain7174 requested a review from a team as a code owner July 21, 2026 08:52
// the controller migrating so only migration APIs are permitted.
c.vmID = state.GetVmID()
c.sandboxOptions = sandboxOptionsFromProto(state.GetSandboxOptions())
if c.sandboxOptions != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need sandboxOptions.FullyPhysicallyBacked option?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we still need and retain sandboxOptions.FullyPhysicallyBacked. This change only stops using it as the stats predicate. Effective memory backing is !Memory.AllowOvercommit, while FullyPhysicallyBacked additionally controls device backing (for example, disabling VPMEM in favor of SCSI) and is still round-tripped in the migration payload. A caller can set AllowOvercommit=false without setting FullyPhysicallyBacked; that was the failing case. This mirrors V1's separate physicallyBacked and devicesPhysicallyBacked fields.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key is whether the option remains required for device/VPMEM behavior even though it is no longer the right stats predicate.

Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
@shreyanshjain7174
shreyanshjain7174 force-pushed the user/shsancheti/fix-lcowv2-physically-backed-stats branch from b1474f8 to 1fc6a13 Compare July 21, 2026 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants