Repository navigation
test(e2e): add CVM E2Es to AgentBaker E2Es - #9752
Zachary (zachary-bailey) wants to merge 46 commits into
Conversation
Windows Unit Test Results 3 files 18 suites 1m 11s ⏱️ Results for commit e0fd2a2. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
The TDX scenario uses an NVMe-only SKU with a gallery definition that lacks NVMe support.
2 open findings
What changed in this PR
Adds AgentBaker E2E coverage for confidential VM images across Ubuntu and Azure Linux.
Changes:
- Registers SEV-SNP and TDX CVM scenarios.
- Configures VMSS confidential-computing security settings.
- Adds CVM image definitions, filtering, and unit tests.
| File | Description |
|---|---|
e2e/scenario/types.go |
Adds the CVM scenario tag. |
e2e/scenario/scenario_cvm.go |
Defines CVM scenarios and VMSS configuration. |
e2e/scenario/scenario_cvm_test.go |
Tests CVM definitions and security settings. |
e2e/runner/filter_test.go |
Verifies CVM tag filtering. |
e2e/config/vhd.go |
Registers CVM gallery images. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
AgentBaker Linux gate detectiveRun: 185565566 Detective summary: One E2E scenario failed during LocalDNS conntrack validation. The raw-table validation had the expected rule counts ( Likely cause / signature: e2e-localdns-notrack-rule-regex-missing - recurring LocalDNS NOTRACK exact-rule validator sensitivity. Confidence: High: failed test record, E2E task log, timeline, and prior repair-linked row all isolate the same LocalDNS validation family. Recommended owner/action: Repair #40133693 tracks LocalDNS/E2E owner follow-up; PR owner likely does not need a CVM change for this failure. Strongest alternative: CVM E2E PR change caused node validation fallout, but the fatal assertion is LocalDNS-specific and recurs across unrelated PRs. Evidence: build run, failed test run 689181656, E2E task log 624, failed timeline, PR metadata, repair #40133693, and wiki tracker. |
There was a problem hiding this comment.
🟡 Changes recommended
CVM VMSS creation and specialized-image selection contain provisioning blockers.
5 open findings
Previously missed (1)
In code that hasn't changed since last review
e2e/scenario/scenario_cvm.go:107
🔴 High Risk — 🏗️ Architecture: Standard_DC2ads_v5 and Standard_DC2eds_v6 are confidential-compute SKUs that do not support host encryption; CVM configurations use the confidential disk security profile instead. Sending EncryptionAtHost=true makes VMSS creation fail before node validation. Omit this field (or set it to false) and update the assertion accordingly.
🧠 Review effort: Balanced
| builderImage := i | ||
| builderImage.Name = i.vhdBuilderGalleryImageName() | ||
| vhd, err := Azure.LatestSIGImageVersionByTag(ctx, &builderImage, Config.SIGVersionTagName, Config.SIGVersionTagValue, location) |
There was a problem hiding this comment.
🔵 Needs a closer look
Generalized-image publication and E2E image selection need correction before the new scenarios can run reliably.
23 open issues
And 3 more that still need to be addressed.
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Gallery publication fails when the source region is outside westus because replication targets omit that region.
21 open issues
And 1 more that still need to be addressed.
1 previously missed issue
In code that hasn't changed since last review
vhdbuilder/packer/publish-generalized-cvm-if-required.sh:92
🟡 Medium Risk — 🔧 Script Logic: When source_location is outside westus, Azure rejects image-version creation because the source region must be included in the target regions. Here --location uses source_location, but --target-regions contains only westus, so generalized CVM publication fails for those builds. Include both regions, deduplicating when the source is already westus.
2 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Missing publication on non-prefetch paths and inconsistent VM-size capability checks can fail builds and CVM provisioning.
22 open issues
And 2 more that still need to be addressed.
1 previously missed issue
In code that hasn't changed since last review
e2e/scenario/scenario_cvm.go:96
🟡 Medium Risk — 🏗️ Architecture: This changes the bootstrap VM size but leaves Runtime.VMSize at the global default. prepareAKSNode uses that default for capability checks and can set UseNVMe=true before applying the CVM VMSS override (provision.go:362–381). With an NVMe-capable global SKU, the SNP scenarios therefore request NvmeDisk placement for their SCSI-based Standard_DC2ads_v5 instances (vmss.go:596–603), preventing VM creation. Resolve the scenario's CVM size before capability detection and use it consistently for runtime, bootstrap, and VMSS settings.
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Source-region handling and partial-publication recovery can prevent CVM builds from producing usable E2E images and metadata.
23 open issues
And 3 more that still need to be addressed.
1 resolved since last review
Review effort: Balanced
| az sig image-version create "${version_args[@]}" \ | ||
| --location "${source_location}" \ | ||
| --os-vhd-uri "${SOURCE_VHD_URI}" \ | ||
| --os-vhd-storage-account "${source_storage_id}" \ | ||
| --target-regions westus \ | ||
| --tags "${version_tags[@]}" >/dev/null |
| wait "${gallery_pid}" || echo "##vso[task.logissue type=warning]Generalized CVM gallery publication failed (exit $?)" | ||
| return "${storage_status}" |
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved SKU selection and shell-validation issues can prevent CVM provisioning and fail validation.
22 open issues
And 2 more that still need to be addressed.
2 previously missed issues
In code that hasn't changed since last review
e2e/scenario/scenario_cvm.go:96
🟡 Medium Risk — 🏗️ Architecture: This sets the bootstrap configuration's VM size, but Runtime.VMSize still holds the global --vm-sku value (provision.go:262). Capability checks at provision.go:362-381 therefore inspect a different SKU. With an NVMe-capable global SKU, they overwrite UseNVMe=false for the SNP scenarios, and vmss.go:596-603 requests NVMe ephemeral-disk placement on the selected SCSI Standard_DC2ads_v5 VM, preventing provisioning. Propagate the scenario's selected VM size into the runtime before capability checks, keeping the runtime, bootstrap configuration, and VMSS SKU consistent.
spec/vhdbuilder/packer/publish_generalized_cvm_spec.sh:2
🟡 Medium Risk — 🔧 Script Logic: The new [[ ... ]] checks at lines 22, 39, and 40 fail the POSIX validation pass. .pipelines/scripts/verify_shell.sh:101-104 includes this spec and explicitly runs ShellCheck with --shell=sh and SC3010 enabled, overriding shell=bash. Suppress SC3010 for this intentionally Bash-only spec so make validate-shell passes; the repository already uses this suppression in spec/vhdbuilder/packer/ensure_sig_vhd_exists_spec.sh:112.
1 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Publication excludes required source-region replicas, and the new prefetch conditional conflicts with mandatory shell validation.
23 open issues
And 3 more that still need to be addressed.
1 previously missed issue
In code that hasn't changed since last review
vhdbuilder/packer/publish-generalized-cvm-if-required.sh:92
🟡 Medium Risk — 🔧 Script Logic: When the Image Builder output is outside westus, this creates the image version in ${source_location} but excludes that region from its replication targets. Azure Compute Gallery requires the source region in --target-regions, so publication fails, generalized metadata is omitted, and prefetch resources remain allocated for retry. Include both ${source_location} and westus, deduplicating when they match.
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The new publisher spec contains unsuppressed Bash conditionals that fail the repository’s shell validation gate.
24 open issues
And 4 more that still need to be addressed.
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Shell validation failures and mismatched publication and scenario regions can block reliable CVM E2E execution.
24 open issues
And 4 more that still need to be addressed.
1 resolved since last review
Review effort: Balanced
| --location "${source_location}" \ | ||
| --os-vhd-uri "${SOURCE_VHD_URI}" \ | ||
| --os-vhd-storage-account "${source_storage_id}" \ | ||
| --target-regions "${PACKER_BUILD_LOCATION}" \ |
There was a problem hiding this comment.
🟡 Changes recommended
Capability checks use the runner’s SKU rather than the CVM scenario’s effective size, which can select incompatible disk placement.
25 open issues
And 5 more that still need to be addressed.
Review effort: Balanced
| BootstrapConfigMutator: func(_ *Cluster, nbc *datamodel.NodeBootstrappingConfiguration) { | ||
| nbc.AgentPoolProfile.VMSize = vmSize | ||
| }, | ||
| VMConfigMutator: func(vmss *armcompute.VirtualMachineScaleSet) { | ||
| configureConfidentialVM(vmss, vmSize, securityEncryptionType) |
There was a problem hiding this comment.
🟡 Changes recommended
Completed blob copies prevent failed gallery publication from recovering on reruns, potentially leaving CVM coverage skipped.
25 open issues
And 5 more that still need to be addressed.
1 resolved since last review
Review effort: Balanced
|
|
||
| copy_optimized_vhd "${artifact_uri}" & | ||
| storage_pid=$! | ||
| copy_optimized_vhd_to_packer_gallery "${artifact_uri}" & |
|
|
|
There was a problem hiding this comment.
🔵 Needs a closer look
The publication and readiness flow lacks required ShellSpec coverage, while failures can cause E2Es to skip rather than detect regressions.
25 open issues
And 5 more that still need to be addressed.
1 previously missed issue
In code that hasn't changed since last review
vhdbuilder/packer/publish-generalized-cvm-if-required.sh:97
🟡 Medium Risk — 🧪 Test Coverage: The new publication/readiness flow has no ShellSpec coverage. Add mocked cases for CLI exit code 3 followed by a successful retry, other CLI errors, successful replication, failed replication, and timeout. Assert both retry behavior and exit status, with controlled polling so tests do not wait in real time. Publication failures become warnings in optimize.sh, and metadata can omit the image, so E2Es may skip instead of detecting regressions here. Follow the mocking pattern in spec/vhdbuilder/packer/ensure_sig_vhd_exists_spec.sh; AGENTS.md:62–65 requires ShellSpec testing.
Review effort: Balanced
What this PR does / why we need it:
This PR adds AgentBaker-side implementation for adding CVM E2Es
Which issue(s) this PR fixes:
Currently no CVM E2Es
Fixes #