From 917d728f9345af6f2310a9570594319bc4897d7d Mon Sep 17 00:00:00 2001 From: Shreyansh Sancheti Date: Mon, 16 Mar 2026 16:23:15 +0530 Subject: [PATCH] parity: add LCOW document permutation tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add TestLCOWDocumentParityPermutations to exercise annotation and option combinations that trigger different document construction branches in the legacy and v2 LCOW pipelines. Each test sets all required fields so comparisons check real values rather than defaults. Permutation categories: - CPU partial combinations (count, limit, weight) - Memory (overcommit disabled, cold discard hint) - Boot mode (kernel direct + VHD rootfs) - Feature flags (scratch encryption, writable overlay) - Device interactions (VPMem disabled → 4 SCSI controllers) - Cross-group (physically backed + VPMem + encryption) - Shim option overrides (annotation CPU/memory priority) - Kernel args (VPCIEnabled, time sync, process dump, initrd boot) Gap tests document three known v2 builder differences: - No CPUGroupID: legacy nil vs v2 empty CpuGroup struct - No StorageQoS: legacy nil vs v2 empty StorageQoS struct - Initrd boot: legacy VPMem controller vs v2 nil VirtualPMem Gap tests use inverted assertions — they expect a diff and only fail if documents unexpectedly match, signaling the v2 bug was fixed. Also adds normalizeKernelCmdLine and isOnlyKernelCmdLineWhitespaceDiff helpers to handle a known legacy quirk where initrd+KernelDirect boot produces a leading space in kernel command lines that v2 correctly omits. Signed-off-by: Shreyansh Sancheti --- test/parity/vm/hcs_document_creator_test.go | 74 +++++ test/parity/vm/lcow_doc_test.go | 343 +++++++++++++++++++- 2 files changed, 414 insertions(+), 3 deletions(-) diff --git a/test/parity/vm/hcs_document_creator_test.go b/test/parity/vm/hcs_document_creator_test.go index 3c7f2f33b0..8a9c229a39 100644 --- a/test/parity/vm/hcs_document_creator_test.go +++ b/test/parity/vm/hcs_document_creator_test.go @@ -8,10 +8,13 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" "github.com/opencontainers/runtime-spec/specs-go" + "github.com/google/go-cmp/cmp" + runhcsopts "github.com/Microsoft/hcsshim/cmd/containerd-shim-runhcs-v1/options" lcowbuilder "github.com/Microsoft/hcsshim/internal/builder/vm/lcow" hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" @@ -111,3 +114,74 @@ func jsonToString(v interface{}) string { } return string(b) } + +// normalizeKernelCmdLine trims leading/trailing whitespace from the kernel +// command line in the document. The legacy builder has a minor quirk that +// produces a leading space for initrd+KernelDirect boot. The v2 builder +// does not. Since HCS trims whitespace from kernel args, this difference +// is harmless and we normalize it away. +func normalizeKernelCmdLine(doc *hcsschema.ComputeSystem) { + if doc == nil || doc.VirtualMachine == nil || doc.VirtualMachine.Chipset == nil { + return + } + if kd := doc.VirtualMachine.Chipset.LinuxKernelDirect; kd != nil { + kd.KernelCmdLine = strings.TrimSpace(kd.KernelCmdLine) + } + if uefi := doc.VirtualMachine.Chipset.Uefi; uefi != nil && uefi.BootThis != nil { + uefi.BootThis.OptionalData = strings.TrimSpace(uefi.BootThis.OptionalData) + } +} + +// isOnlyKernelCmdLineWhitespaceDiff returns true if the only difference between +// two documents is leading/trailing whitespace in the kernel command line. +// This is a known legacy quirk where initrd+KernelDirect boot produces a +// leading space that v2 correctly omits. +func isOnlyKernelCmdLineWhitespaceDiff(legacy, v2 *hcsschema.ComputeSystem) bool { + // Deep copy and normalize, then re-compare. + legacyCopy := *legacy + v2Copy := *v2 + // Shallow copy the VM and chipset to avoid mutating originals. + if legacyCopy.VirtualMachine != nil { + vmCopy := *legacyCopy.VirtualMachine + legacyCopy.VirtualMachine = &vmCopy + if vmCopy.Chipset != nil { + chipCopy := *vmCopy.Chipset + legacyCopy.VirtualMachine.Chipset = &chipCopy + if chipCopy.LinuxKernelDirect != nil { + lkdCopy := *chipCopy.LinuxKernelDirect + legacyCopy.VirtualMachine.Chipset.LinuxKernelDirect = &lkdCopy + } + if chipCopy.Uefi != nil { + uefiCopy := *chipCopy.Uefi + legacyCopy.VirtualMachine.Chipset.Uefi = &uefiCopy + if uefiCopy.BootThis != nil { + btCopy := *uefiCopy.BootThis + legacyCopy.VirtualMachine.Chipset.Uefi.BootThis = &btCopy + } + } + } + } + if v2Copy.VirtualMachine != nil { + vmCopy := *v2Copy.VirtualMachine + v2Copy.VirtualMachine = &vmCopy + if vmCopy.Chipset != nil { + chipCopy := *vmCopy.Chipset + v2Copy.VirtualMachine.Chipset = &chipCopy + if chipCopy.LinuxKernelDirect != nil { + lkdCopy := *chipCopy.LinuxKernelDirect + v2Copy.VirtualMachine.Chipset.LinuxKernelDirect = &lkdCopy + } + if chipCopy.Uefi != nil { + uefiCopy := *chipCopy.Uefi + v2Copy.VirtualMachine.Chipset.Uefi = &uefiCopy + if uefiCopy.BootThis != nil { + btCopy := *uefiCopy.BootThis + v2Copy.VirtualMachine.Chipset.Uefi.BootThis = &btCopy + } + } + } + } + normalizeKernelCmdLine(&legacyCopy) + normalizeKernelCmdLine(&v2Copy) + return cmp.Diff(&legacyCopy, &v2Copy) == "" +} diff --git a/test/parity/vm/lcow_doc_test.go b/test/parity/vm/lcow_doc_test.go index d502fee84d..2753c9f284 100644 --- a/test/parity/vm/lcow_doc_test.go +++ b/test/parity/vm/lcow_doc_test.go @@ -5,12 +5,14 @@ package vm import ( "context" "maps" + "strings" "testing" "github.com/google/go-cmp/cmp" "github.com/opencontainers/runtime-spec/specs-go" runhcsopts "github.com/Microsoft/hcsshim/cmd/containerd-shim-runhcs-v1/options" + iannotations "github.com/Microsoft/hcsshim/internal/annotations" shimannotations "github.com/Microsoft/hcsshim/pkg/annotations" vm "github.com/Microsoft/hcsshim/sandbox-spec/vm/v2" ) @@ -124,9 +126,15 @@ func TestLCOWDocumentParity(t *testing.T) { } if diff := cmp.Diff(legacyDoc, v2Doc); diff != "" { - t.Logf("Legacy document:\n%s", jsonToString(legacyDoc)) - t.Logf("V2 document:\n%s", jsonToString(v2Doc)) - t.Errorf("LCOW HCS document mismatch (-legacy +v2):\n%s", diff) + // Check if the only difference is the legacy kernel cmdline + // leading space quirk. If so, warn instead of failing. + if isOnlyKernelCmdLineWhitespaceDiff(legacyDoc, v2Doc) { + t.Logf("WARNING: kernel cmdline has leading whitespace difference (legacy quirk): %s", diff) + } else { + t.Logf("Legacy document:\n%s", jsonToString(legacyDoc)) + t.Logf("V2 document:\n%s", jsonToString(v2Doc)) + t.Errorf("LCOW HCS document mismatch (-legacy +v2):\n%s", diff) + } } }) } @@ -201,3 +209,332 @@ func TestLCOWSandboxOptionsFieldParity(t *testing.T) { }) } } + +// TestLCOWDocumentParityPermutations exercises annotation and option combinations +// that trigger different document construction branches. Each test populates all +// fields it depends on so the comparison checks real values, not defaults. +func TestLCOWDocumentParityPermutations(t *testing.T) { + bootDir := setupBootFiles(t) + + tests := []struct { + name string + annotations map[string]string + devices []specs.WindowsDevice + shimOpts func() *runhcsopts.Options + }{ + // --- CPU partial combinations --- + + { + name: "CPU: count only", + annotations: map[string]string{ + shimannotations.ProcessorCount: "2", + shimannotations.CPUGroupID: "cpu-only-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + { + name: "CPU: limit only", + annotations: map[string]string{ + shimannotations.ProcessorLimit: "50000", + shimannotations.CPUGroupID: "limit-only-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + { + name: "CPU: weight only", + annotations: map[string]string{ + shimannotations.ProcessorWeight: "500", + shimannotations.CPUGroupID: "weight-only-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + + // --- Memory partial combinations --- + + { + name: "memory: overcommit disabled", + annotations: map[string]string{ + shimannotations.MemorySizeInMB: "2048", + shimannotations.AllowOvercommit: "false", + shimannotations.CPUGroupID: "mem-nocommit-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + { + name: "memory: cold discard hint", + annotations: map[string]string{ + shimannotations.MemorySizeInMB: "1024", + shimannotations.EnableColdDiscardHint: "true", + shimannotations.CPUGroupID: "cold-discard-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + + // --- Boot mode interactions --- + + { + name: "boot: kernel direct + VHD rootfs", + annotations: map[string]string{ + shimannotations.KernelDirectBoot: "true", + shimannotations.PreferredRootFSType: "vhd", + shimannotations.CPUGroupID: "vhd-boot-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + + // --- Feature flag combinations --- + + { + name: "feature: scratch encryption + disable writable shares", + annotations: map[string]string{ + shimannotations.LCOWEncryptedScratchDisk: "true", + shimannotations.DisableWritableFileShares: "true", + shimannotations.CPUGroupID: "scratch-enc-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + { + name: "feature: writable overlay dirs (VHD rootfs)", + annotations: map[string]string{ + shimannotations.PreferredRootFSType: "vhd", + shimannotations.KernelDirectBoot: "true", + iannotations.WritableOverlayDirs: "true", + shimannotations.CPUGroupID: "overlay-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + + // --- Device interactions --- + + { + name: "VPMem disabled (4 SCSI controllers)", + annotations: map[string]string{ + shimannotations.VPMemCount: "0", + shimannotations.CPUGroupID: "no-vpmem-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + + // --- Cross-group interactions --- + + { + name: "cross: physically backed + VPMem disabled + scratch encryption", + annotations: map[string]string{ + shimannotations.FullyPhysicallyBacked: "true", + shimannotations.VPMemCount: "0", + shimannotations.LCOWEncryptedScratchDisk: "true", + shimannotations.MemorySizeInMB: "4096", + shimannotations.CPUGroupID: "phys-backed-group", + shimannotations.StorageQoSIopsMaximum: "5000", + shimannotations.StorageQoSBandwidthMaximum: "1000000", + }, + }, + { + name: "cross: CPU + memory + MMIO + QoS + cold discard + VHD boot", + annotations: map[string]string{ + shimannotations.ProcessorCount: "2", + shimannotations.ProcessorLimit: "80000", + shimannotations.ProcessorWeight: "300", + shimannotations.CPUGroupID: "full-combo-group", + shimannotations.MemorySizeInMB: "4096", + shimannotations.AllowOvercommit: "true", + shimannotations.EnableColdDiscardHint: "true", + shimannotations.MemoryLowMMIOGapInMB: "512", + shimannotations.MemoryHighMMIOBaseInMB: "2048", + shimannotations.MemoryHighMMIOGapInMB: "1024", + shimannotations.StorageQoSIopsMaximum: "10000", + shimannotations.StorageQoSBandwidthMaximum: "2000000", + shimannotations.KernelDirectBoot: "true", + shimannotations.PreferredRootFSType: "vhd", + }, + }, + + // --- Shim options override vs annotation priority --- + + { + name: "override: annotation CPU overrides shim option CPU", + shimOpts: func() *runhcsopts.Options { + return &runhcsopts.Options{ + SandboxPlatform: "linux/amd64", + BootFilesRootPath: bootDir, + VmProcessorCount: 1, + } + }, + annotations: map[string]string{ + shimannotations.ProcessorCount: "2", + shimannotations.CPUGroupID: "override-cpu-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + { + name: "override: annotation memory overrides shim option memory", + shimOpts: func() *runhcsopts.Options { + return &runhcsopts.Options{ + SandboxPlatform: "linux/amd64", + BootFilesRootPath: bootDir, + VmMemorySizeInMb: 1024, + } + }, + annotations: map[string]string{ + shimannotations.MemorySizeInMB: "4096", + shimannotations.CPUGroupID: "override-mem-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + // --- Kernel args combinations --- + + { + name: "kernel args: VPCIEnabled + custom boot options", + annotations: map[string]string{ + shimannotations.VPCIEnabled: "true", + shimannotations.KernelBootOptions: "loglevel=7 debug", + shimannotations.CPUGroupID: "vpci-boot-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + { + name: "kernel args: disable time sync + process dump + writable overlay (VHD)", + annotations: map[string]string{ + shimannotations.KernelDirectBoot: "true", + shimannotations.PreferredRootFSType: "vhd", + shimannotations.DisableLCOWTimeSyncService: "true", + shimannotations.ContainerProcessDumpLocation: "/tmp/dumps", + iannotations.WritableOverlayDirs: "true", + shimannotations.CPUGroupID: "kargs-combo-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + { + name: "kernel args: initrd boot (kernel cmdline whitespace warning)", + annotations: map[string]string{ + shimannotations.PreferredRootFSType: "initrd", + shimannotations.KernelDirectBoot: "true", + shimannotations.CPUGroupID: "initrd-kargs-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + + // --- Cases that expose known differences between legacy and v2 --- + // These document real parity gaps for the v2 builder team to fix. + + { + // No CPUGroupID set — legacy produces CpuGroup=nil, + // v2 produces CpuGroup=&{Id:""} (unconditional init in topology.go). + name: "gap: no CPUGroupID (nil vs empty CpuGroup)", + annotations: map[string]string{ + shimannotations.ProcessorCount: "2", + shimannotations.MemorySizeInMB: "2048", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }, + { + // No QoS set — legacy produces StorageQoS=nil, + // v2 may produce StorageQoS=&{} depending on builder. + name: "gap: no StorageQoS (nil vs empty)", + annotations: map[string]string{ + shimannotations.ProcessorCount: "2", + shimannotations.MemorySizeInMB: "2048", + shimannotations.CPUGroupID: "no-qos-group", + }, + }, + { + // Initrd preferred — legacy allocates VPMem controller with no + // devices, v2 sets VirtualPMem=nil. + name: "gap: initrd boot (VPMem nil vs empty controller)", + annotations: map[string]string{ + shimannotations.PreferredRootFSType: "initrd", + shimannotations.KernelDirectBoot: "true", + shimannotations.CPUGroupID: "initrd-group", + shimannotations.StorageQoSIopsMaximum: "1000", + shimannotations.StorageQoSBandwidthMaximum: "100000", + }, + }} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + + shimOpts := &runhcsopts.Options{ + SandboxPlatform: "linux/amd64", + BootFilesRootPath: bootDir, + } + if tc.shimOpts != nil { + shimOpts = tc.shimOpts() + } + + legacySpec := specs.Spec{ + Annotations: maps.Clone(tc.annotations), + Linux: &specs.Linux{}, + Windows: &specs.Windows{ + HyperV: &specs.WindowsHyperV{}, + Devices: tc.devices, + }, + } + if legacySpec.Annotations == nil { + legacySpec.Annotations = map[string]string{} + } + legacyDoc, legacyOpts, err := buildLegacyLCOWDocument(ctx, legacySpec, shimOpts, bootDir) + if err != nil { + t.Fatalf("failed to build legacy LCOW document: %v", err) + } + + v2Spec := &vm.Spec{ + Annotations: maps.Clone(tc.annotations), + Devices: tc.devices, + } + if v2Spec.Annotations == nil { + v2Spec.Annotations = map[string]string{} + } + v2Doc, sandboxOpts, err := buildV2LCOWDocument(ctx, shimOpts, v2Spec, bootDir) + if err != nil { + t.Fatalf("failed to build v2 LCOW document: %v", err) + } + + if testing.Verbose() { + t.Logf("Legacy options: %+v", legacyOpts) + t.Logf("V2 sandbox options: %+v", sandboxOpts) + } + + diff := cmp.Diff(legacyDoc, v2Doc) + + // Gap tests document known v2 builder bugs. They expect a + // diff and only fail if the documents unexpectedly match, + // signaling the bug was fixed and the gap test should be + // removed. + if strings.HasPrefix(tc.name, "gap:") { + if diff == "" { + t.Errorf("gap test unexpectedly passed: v2 builder bug may be fixed; remove from gaps") + } else { + t.Logf("expected gap diff (-legacy +v2):\n%s", diff) + } + return + } + + if diff != "" { + if isOnlyKernelCmdLineWhitespaceDiff(legacyDoc, v2Doc) { + t.Logf("WARNING: kernel cmdline has leading whitespace difference (legacy quirk): %s", diff) + } else { + t.Logf("Legacy document:\n%s", jsonToString(legacyDoc)) + t.Logf("V2 document:\n%s", jsonToString(v2Doc)) + t.Errorf("LCOW HCS document mismatch (-legacy +v2):\n%s", diff) + } + } + }) + } +}