Repository navigation
fix(gpu): preserve customer-installed PIS drivers - #9702
Ben Brady (benjamin-brady) wants to merge 3 commits into
Conversation
Windows Unit Test Results 3 files 18 suites 50s ⏱️ Results for commit cea1cea. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The E2E sentinel can survive destructive cleanup and incorrectly report that customer drivers were preserved.
Review effort: Balanced
Findings: 1
What changed in this PR
Preserves customer-installed GPU drivers on PIS-derived nodes by moving managed-driver cleanup to image preparation.
Changes:
- Adds verified distro-specific bake cleanup and success-only completion markers.
- Restricts destructive node cleanup to runs that execute basePrep.
- Adds ShellSpec checks and a GPU VHD-caching scenario.
| File | Description |
|---|---|
| spec/parts/linux/cloud-init/artifacts/cse_main_gpu_pis_spec.sh | Tests cleanup across PIS phases. |
| spec/parts/linux/cloud-init/artifacts/cse_install_ubuntu_spec.sh | Tests Ubuntu cleanup verification. |
| spec/parts/linux/cloud-init/artifacts/cse_install_mariner_spec.sh | Tests cache cleanup verification. |
| parts/linux/cloud-init/artifacts/ubuntu/cse_install_ubuntu.sh | Verifies driver teardown before capture. |
| parts/linux/cloud-init/artifacts/mariner/cse_install_mariner.sh | Verifies bake-time cache removal. |
| parts/linux/cloud-init/artifacts/cse_start.sh | Marks only successful image preparation. |
| parts/linux/cloud-init/artifacts/cse_main.sh | Gates cleanup by provisioning phase. |
| e2e/scenario/types.go | Adds a post-basePrep callback. |
| e2e/scenario/scenario.go | Registers GPU driver-preservation scenario. |
| e2e/scenario/provision.go | Runs callback and installs sentinel. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2cbf2cf to
d9ddd90
Compare
AgentBaker Linux gate detectiveRun: 184124893 Detective summary: E2E finished with VHDCaching scenarios failing only in cleanup while deleting gallery image versions; ARM returned ScopeLocked for resource group Likely cause / signature: Confidence: High for E2E cleanup/SIG resource-group lock infra flake. Recommended owner/action: E2E/VHD infra owners should remove or avoid the Strongest alternative: PR 9702 changes E2E cleanup/provisioning code, so a cleanup regression is plausible; however, the same locked resource-group signature also appears on unrelated PR 9699. Evidence: build run, failed timeline records, focused E2E task log, build metadata, PR changed files, and wiki tracker. |
|
The real-driver PIS scenario now passes in CI on the default westus3 gallery: GPU build 184188675, The overall GPU gate remains red solely because |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
6 open findings
s.Runtime.NBCin code that previously didn’t depend on the runtime… · NewEXIT_CODEis newly required for this branch; if it’s unset (or non-numeric) this-eqtest will… · New|| ... || ... && return 0chaining, and… · Newapt-mark showautomatch only allows an optional:arm64suffix. Ifapt-markoutputs other… · Newmktemp, then mocksrm(), which prevents cleanup and can… · Newcse_main.shusing pattern-basedawk/sed, which is… · New
1 resolved since last review
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
|
Tim Wright (@timmy-wright) Nishchay (@awesomenix) could you review this for today’s release cut? I’ve removed the general CustomData compression changes and slimmed the diff from 1,423 to 893 changed lines, keeping both CUDA and Grace Blackwell support. The fix cleans AKS-owned drivers during the PIS bake, before customer scripts, and preserves customer drivers on derived nodes. Important compatibility point: opted-out Ubuntu PIS bakes, including CPU-only pools, require a new source image containing the cleanup helper. Local regression/scenario tests pass. Fresh-image GPU PIS E2E is still pending, so this is a request to review in parallel—not a claim that it is ready to merge. The PR description has the exact validation and remaining gate. |
Review: Preserve customer-installed PIS GPU driversWhat is goodThis PR fixes the ownership boundary, not just the deletion symptom. Cleaning AKS-owned artifacts before customer customization, then preserving the resulting driver on derived nodes, is the right lifecycle model.
Review outcomeReviewed #9702 at No confirmed current reliability or performance defect was found. There is one blocking validation gate below. The design recommendations are non-blocking; the final readability comment is a nitpick. The documented source-image requirement is an intentional compatibility change, not an additional finding: opted-out Ubuntu PIS bakes, including CPU-only pools, require a source VHD containing the new helper. Ordinary and derived nodes do not require it. Release sequencing must respect that requirement. Comments, ordered by importance1. Blocking: Verify the complete lifecycle with a fresh branch-built VHDWhere: PR validation evidence; The PR description still explicitly marks fresh-image E2E validation as pending. The local tests pass, but most destructive cleanup operations are stubbed. A green GPU pipeline check alone does not establish that the run exercised the new baked helper. Before merge, attach a successful run using a branch-built Why blocking: This is the main changed lifecycle, and it combines destructive cleanup with two VM runs and an image capture. This is a verification gate, not a claim that the implementation is broken. Clear it when the evidence is available; it does not inherently require a code change. 2. Non-blocking design recommendation: Keep configuration ownership knowledge synchronizedWhere: The expected configuration, including comments, is copied from the authoritative files under Risk: A future template edit can leave the cleanup verifier stale. Even a comment-only edit to Recommendation: Keep exact ownership verification, but tie its expected content to the authoritative templates. A focused consistency test is the smallest interim safeguard. Build-time generation is an option if this duplication continues to grow. Trade-off: A consistency test retains duplication but catches drift cheaply. Generation removes manual synchronization at the cost of additional build plumbing. Evidence grade: Weak for the general DRY prescription, which is primarily practitioner consensus; the duplicated content is directly observable. This is not a demonstrated failure in the current revision. 3. Non-blocking design recommendation: Make the cleanup state and dependency contract explicitWhere: Fresh cleanup, interrupted cleanup, and already-completed cleanup are represented through combinations of files, booleans, and missing configuration. Understanding the allowed transitions requires tracing both the orchestrator and the teardown function. The helper also relies on previously sourced distro cleanup functions and globals. Risk: A later change can preserve one path while accidentally breaking retries or legacy-layout handling. Recommendation: Add a concise contract describing the entry states, the pending marker's lifetime, required sourced functions/globals, and successful postconditions. Remove the unused Trade-off: This makes the existing design easier to audit without multiplying abstractions. It does not remove the safety complexity, which is justified. Evidence grade: Moderate for control-flow and naming guidance; its applicability to shell is indirect, not proof that a particular refactor would reduce defects. 4. Non-blocking design recommendation: Separate inspection failure from a positive resultWhere: The predicates return success both when artifacts remain and when inspection fails. The package predicate also accepts an allowlist, despite its name suggesting a general presence check, and writes inspection-error text to the same output channel as package names. Risk: A future caller could interpret "could not inspect" as "found a package," or misunderstand what the allowlist excludes. The current callers remain conservative; this is an interface-design concern. Recommendation: At minimum, document the return and output contracts. If changing the interface, distinguish found, absent, and inspection-failed outcomes, send diagnostics to stderr, and explicitly retain fail-closed behavior at every caller. Trade-off: Distinct outcomes add some branching but make the safety policy explicit rather than hiding it inside a presence query. Evidence grade: Moderate for aligning names and contracts with behavior. This does not justify weakening the existing failure handling. 5. Nitpick: Make the cleanup eligibility rules read like the lifecycle policyWhere: The compound shell conditions are valid, but nested command groups and mixed Recommendation: Prefer explicit Trade-off: A few more lines in exchange for more direct control flow. No behavioral change or new helper abstraction is needed. Evidence grade: Moderate for control-flow clarity. This is a readability preference, not a demonstrated bug, and should not delay merge. Validation performed during review
The selected ShellSpec suites contain 4 phase-dispatch examples, 3 Ubuntu-cleanup examples, 59 Mariner-installer examples, and 2 Packer-helper examples. These results establish local regression coverage, not a live fresh-image preservation pass or measured provisioning performance. The inspected Linux E2E build, 187241126, reported an Azure Linux iptables validation failure rather than a GPU cleanup failure. That observation does not clear the fresh-image validation gate above. |
Tim Wright (timmy-wright)
left a comment
Tim Wright (timmy-wright)
left a comment
There was a problem hiding this comment.
The lifecycle ownership boundary and conservative cleanup checks are good. Full review report: #9702 (comment)
The inline comments below are non-blocking design recommendations and one nitpick. The report separately identifies fresh branch-built VHD E2E evidence as the blocking validation gate; this review does not assert a current correctness defect.
Move strict Ubuntu cleanup into a VHD-baked helper so traditional CustomData stays under its size limit. Fail closed before customer customization; do not clean customer drivers on PIS-derived nodes. Refs #9702 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
3022615 to
d285aa8
Compare
Move strict Ubuntu cleanup into a VHD-baked helper so traditional CustomData stays under its size limit. Fail closed before customer customization; do not clean customer drivers on PIS-derived nodes. Refs #9702 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
d285aa8 to
b447e74
Compare
Move strict Ubuntu cleanup into a VHD-baked helper so traditional CustomData stays under its size limit. Fail closed before customer customization; do not clean customer drivers on PIS-derived nodes. Refs #9702 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise a customer T4 installation after basePrep using a branch-built image; validate the installed driver on the derived VM. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
b447e74 to
cea1cea
Compare
|
|
|
There was a problem hiding this comment.
🔵 Needs a closer look
Grace Blackwell teardown needs ARM64 validation by a GPU/image owner because live E2E evidence covers only T4/CUDA, leaving GB package purging, service shutdown, and retry recovery tested only with mocks.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
| done | ||
| } | ||
|
|
||
| cleanUpGPUDriversForBasePrep() { |
There was a problem hiding this comment.
I'm not sure I see the value of this method. All it seems to do is wrap the "cleanUpGPUDrivers" with additional logging and failure modes. Can't we just use the "cleanUpGPUDrivers" without any changes instead?
| [ -n "${package}" ] && purge_packages+=("${package}") | ||
| done <<< "${packages}" | ||
|
|
||
| for service in nvidia-device-plugin nvidia-dcgm-exporter nvidia-dcgm nvidia-imex nvidia-persistenced; do |
There was a problem hiding this comment.
Why wasn't this code being run before? It seems like we're trying to do more of a cleanup than earlier instead of doing the same cleanup - but moving it from node prep into base prep.
What this PR does / why we need it:
PIS-derived nodes skip
basePrep; theirnodePreppreviously deleted a customer-installed GPU driver. Remove AKS-owned GPU artifacts during the opted-out PIS bake, before customer customization, and skip destructive cleanup on derived nodes.base_prep.complete; Azure Linux retains distro-specific cleanup.nodePrepcleanup onbasePrephaving run in the same execution. Ordinary-node behavior, including the legacy skip tag, remains. Six template-guard lines omit Azure Linux-only AMD code from known non-AzureLinux CustomData, preserving raw baked scripts and unknown/custom images.nvidia-smiand absent loaded NVIDIA modules. Focused GB tests exercise real package selection, exact template consistency, and rejection before teardown on ownership/version/inspection failures.Contracts and scope: every opted-out Ubuntu PIS bake, including CPU-only pools, requires a new source image containing the helper; absence fails explicitly. Ordinary nodes and derived PIS nodes do not require the helper. Bake opt-out must be explicit in CustomData (
EnableNvidia=false/GPU_NODE=false); a legacy tag alone does not request PIS bake cleanup. No new dependency on cached IMDS was added. Old CSEs/captures are not retroactively fixed. GRID mismatch validation is unchanged. This PR does not depend on #9753 or include general CustomData compression changes.Review cleanup published: three GitHub-verified signed commits, dated 2026-10-28 NZDT, at head
cea1cead071770d3d6bbcfbfabe7997d5b15ef88; base8c4b6950c6601bd509ee8805a62aa6c8c7060ff5(2026-10-27T14:27:06-07:00, #9919). Production, regression tests, and E2E changes remain separate. Effective diff: +1080/−10 across 25 files. All existing inline threads have individual replies and are resolved; the legacy-tag thread records the contract decision rather than claiming tag-driven bake cleanup was implemented. No new review has been requested.CustomData: the final traditional Ubuntu 24.04 T4 fixture measures 64,772 gzip bytes / 86,364 Base64 characters, leaving 763 bytes / 1,016 characters under the 65,535/87,380 limits. Phase-1 bake measures 361/484. These synthetic fixtures do not guarantee every customer/custom-cloud configuration fits. The earlier Ubuntu 22.04 measurement was on the previous head, not rerun for this cleanup.
go test ./pkg/agent -count=1cd e2e && go test ./scenario ./runner -count=1go test ./pkg/agent -run '^(TestPISGPU|TestGPUAMA)' -count=1 -vgit diff --checkL5: exact focused ShellSpec command
Final-head cloud validation is in progress, not yet green. Automatic VHD gate 188061809, general E2E 188061815, Windows E2E 188061823, and default GPU E2E 188061829 were started for this head. A bounded watcher will verify the new
2404gen2containerdimage's source, metadata and West US 3 replica, then queue GPU E2E withVHD_BUILD_ID=188061809and:scenarioFilter=Name=Ubuntu2404_GPUDriverOptOut_VHDCaching,Name=Ubuntu2404Gen2_GPUNoDriver,Name=Ubuntu2404_GPUA10,Name=Ubuntu2404_NvidiaDevicePluginRunningThis targets PIS opt-out, ordinary opt-out, managed GRID, and device-plugin/workload behavior on the same final PR-built source image. No targeted run result or live Grace Blackwell pass is claimed yet.
Previous-head evidence only: PIS E2E 188034547 passed first attempt on head
b447e74197(2026-10-28 NZDT), using fresh VHD build 188023103, image1.1793138116.9440. Logs confirmed bake cleanup, customer installation, capture, derived basePrep skip, and installer/module/version/nvidia-smivalidation. All 39 applicable checks passed on that head. Its ordinary GPU run used a main-gallery image, so it is not final-image non-PIS proof.Which issue(s) this PR fixes:
Fixes AB#39923593 (Don't delete user installed GPU drivers); depends on AB#39198532.