Skip to content

parity: add LCOW HCS document parity tests between legacy and v2 builders#2629

Open
shreyanshjain7174 wants to merge 3 commits intomicrosoft:mainfrom
shreyanshjain7174:parity/lcow-document-test
Open

parity: add LCOW HCS document parity tests between legacy and v2 builders#2629
shreyanshjain7174 wants to merge 3 commits intomicrosoft:mainfrom
shreyanshjain7174:parity/lcow-document-test

Conversation

@shreyanshjain7174
Copy link
Contributor

@shreyanshjain7174 shreyanshjain7174 commented Mar 13, 2026

Summary

Adds parity tests that compare the HCS ComputeSystem documents produced by the
legacy shim pipeline and the new v2 LCOW builder (lcow.BuildSandboxConfig).

Both paths receive the same inputs — annotations, shim options, and devices —
so the resulting documents should be structurally identical.

How it works

Each test case feeds identical inputs to both pipelines:

Legacy: OCI spec + runhcsopts.Options
  → oci.UpdateSpecFromOptions
  → oci.ProcessAnnotations
  → oci.SpecToUVMCreateOpts → *OptionsLCOW
  → uvm.MakeLCOWDocument → HCS document

V2: vm.Spec + runhcsopts.Options
  → lcow.BuildSandboxConfig → HCS document + SandboxOptions

Documents are normalized before comparison:

  • Owner field zeroed (differs between paths)
  • SCSI/vPCI map keys sorted and re-indexed (both use random GUIDs)
  • Empty CpuGroup{} and StorageQoS{} treated as nil

Changes

  • internal/uvm/create_lcow.go: Export MakeLCOWDocument() — generates
    the HCS document without creating a VM or calling HCS. Mirrors
    CreateLCOW's struct initialization then calls makeLCOWDoc or
    makeLCOWSecurityDoc.

  • test/parity/: New test package with separated files:

    • doc.go — package docs with pipeline diagrams and run instructions
    • helpers_test.go — boot file setup, normalization, JSON logging
    • legacy_pipeline_test.go — wires the 4-step legacy pipeline
    • v2_pipeline_test.go — wraps BuildSandboxConfig
    • lcow_doc_test.go — 3 document parity tests + 5 field parity checks

Test cases

Test What it validates
default config Baseline — both paths produce identical documents with no overrides
custom CPU and memory Annotation-driven topology changes propagate identically
storage QoS IOPS and bandwidth settings match in both documents
SandboxOptions field parity (5 checks) NoWritableFileShares, ScratchEncryption, PolicyBasedRouting, PhysicallyBacked, VPMEMMultiMapping

Results

All 8 tests pass (3 document + 5 field).

More permutations (boot modes, VPMem disabled, cross-group combinations) will
be added in follow-up PRs once this foundation is reviewed and merged.

@shreyanshjain7174 shreyanshjain7174 requested a review from a team as a code owner March 13, 2026 10:29
Copy link
Contributor

@rawahars rawahars left a comment

Choose a reason for hiding this comment

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

Left comments.

Also, please ensure that the comments are verbose and easy to understand.

Address review feedback from PR microsoft#2629. Major changes:

Source changes (internal/uvm/):
- Export VerifyOptions, MakeLCOWDoc, MakeLCOWSecurityDoc as public functions
  so they can be called directly from the test package.
- Remove MakeLCOWDocument composite wrapper — test assembles the pipeline
  itself using the exported primitives.
- Add NewUtilityVMForDoc constructor in types.go. This is needed because
  MakeLCOWDoc takes a *UtilityVM parameter and reads its unexported fields
  (scsiControllerCount, vpmemMaxCount, etc). The test cannot set these
  fields directly from outside the package, so NewUtilityVMForDoc creates
  a minimal UtilityVM with only the fields needed for document generation.
- Original source code comments preserved — only function names changed
  from lowercase to uppercase.

Test changes (test/parity/vm/):
- Moved from test/parity/ to test/parity/vm/ for future WCOW support.
- Removed functional build tag — these are not functional tests.
- Removed all normalization (nil-vs-empty, map sorting, owner zeroing).
  cmp.Diff handles maps natively. Real differences are not masked.
- Merged pipeline helpers into hcs_document_creator_test.go.
- Removed helpers_test.go — setupBootFiles and jsonToString moved inline.
- Generic doc.go that covers both LCOW and future WCOW.
- All test cases explicitly populate every field (CPU, memory, MMIO, QoS,
  CPUGroupID) so comparison always checks populated values, not defaults.
- Use maps.Clone for annotation copying.
- Use testing.Verbose() to gate debug logging.
- Descriptive error messages throughout.

All 8 tests pass (3 document parity + 5 field parity).

Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
@shreyanshjain7174 shreyanshjain7174 force-pushed the parity/lcow-document-test branch from 44a4cea to b6a131c Compare March 16, 2026 07:01
@shreyanshjain7174
Copy link
Contributor Author

shreyanshjain7174 commented Mar 16, 2026

Thanks for the thorough review @rawahars — really appreciate the detailed feedback. Pushed a full restructure addressing all 26 comments:

Source changes:

  • Removed MakeLCOWDocument composite wrapper as suggested. Instead exported the primitives: VerifyOptions, MakeLCOWDoc, MakeLCOWSecurityDoc.
  • Added NewUtilityVMForDoc constructor in types.go — this is needed because MakeLCOWDoc takes *UtilityVM and reads unexported fields like scsiControllerCount, vpmemMaxCount etc. The test can't set those from outside the package, so this constructor creates a minimal UtilityVM with only the doc-generation fields. Open to suggestions if there's a cleaner way to expose this.

Test restructure:

  • Moved to test/parity/vm/ for future WCOW support
  • Removed functional build tag
  • Removed all normalization — cmp.Diff handles maps, real differences aren't masked
  • Merged pipeline files into single hcs_document_creator_test.go
  • setupBootFiles and jsonToString live in the creator file
  • Generic doc.go following the pattern from builder/vm/lcow/doc.go
  • testing.Verbose() gated logging

Key finding during testing: The v2 builder unconditionally initializes CpuGroup (topology.go line 54) even when cpuGroupID is empty, while legacy only sets it when non-empty. All test cases now explicitly set CPUGroupID so both paths populate the field — all 8 tests pass (3 document + 5 field parity).

Add ./parity/... to the test-windows non-functional test glob so that
parity tests under test/parity/ are executed in CI alongside the
existing internal and pkg tests.

Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
func TestLCOWSandboxOptionsFieldParity(t *testing.T) {
bootDir := setupBootFiles(t)

tests := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to test the fields which are part of SandboxOptions, do you need runhcsoptions too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes — both pipelines require runhcsopts.Options to function. The legacy path needs it for oci.UpdateSpecFromOptions and boot file resolution, the v2 path needs it as a required parameter to BuildSandboxConfig (reads SandboxPlatform and BootFilesRootPath). The SandboxOptions fields themselves come from annotations, but the pipeline that produces them won't run without shimOpts.

t.Fatalf("failed to build v2 LCOW document: %v", err)
}

checks := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Maybe define this outside the inner t.Run. It becomes confusing inside the loop.
Also, I imagine you would need to test other fields too including Confidential Options.

- Rename package from 'vm' to 'vmparity' for clarity
- Rename hcs_document_creator_test.go to hcs_lcow_document_creator_test.go
  to distinguish LCOW-specific helpers from future WCOW
- Revert MakeLCOWSecurityDoc back to unexported (not used in tests)
- Simplify doc.go to match codebase convention (minimal, no file layout)
- Update NewUtilityVMForDoc comment explaining why it must stay in uvm
  package (UtilityVM fields are unexported)

Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
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