Repository navigation
fix: report PIS bake completion - #9260
Artur Khantimirov (r2k1) wants to merge 1 commit into
Conversation
Windows Unit Test Results 3 files 18 suites 56s ⏱️ Results for commit a407939. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Unifies Linux CSE completion reporting for normal provisioning and PIS image-bake runs.
Changes:
- Always writes
provision.complete; PIS runs also writebase_prep.complete. - Cleans transient provisioning state before E2E image capture.
- Adds ShellSpec coverage and documents marker semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
parts/linux/cloud-init/artifacts/cse_start.sh |
Updates completion-marker handling. |
spec/parts/linux/cloud-init/artifacts/cse_start_spec.sh |
Tests marker behavior across modes. |
e2e/test_helpers.go |
Validates and cleans PIS bake state. |
aks-node-controller/README.md |
Documents dual-stage reporting and cleanup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pkg/agent/baker.go:264
- This comment still says a bake produces
provision.complete, but the new pre-provision branch deliberately never writes that durable marker; it writes/run/azure/pre-provision.complete, and the controller must watch it. Keeping the capability contract accurate is important for callers implementing the minimum-VHD rollout gate.
// the CSE command is `aks-node-controller provision-wait` and it reports the result recorded in
// provision.json once provision.complete appears.
//
// A PreProvisionOnly (PIS image bake) run only produces provision.complete on a VHD whose
// cse_start.sh writes it for the pre-provision phase. Older VHDs write only base_prep.complete, so
aks-node-controller/app.go:939
- 🟡 Medium Risk — 🔧 Script Logic: The volatile marker lasts for the entire boot, but this fast path treats any existing copy as belonging to the current attempt. If provisioning is restarted on the same bake VM, the previous marker remains, so
provision-waitcan immediately read the previous result (or race withresetStaleProvisionResultremoving it) before the newcse_start.shrun finishes. Clear or rotate this marker at the start of each attempt, with ordering that guarantees the waiter cannot observe the old copy, and cover the retry path in tests.
if marker, found := firstExistingMarker(markers); found {
e2e/test_helpers.go:115
- This capability description names
provision.complete, but the bake path now explicitly avoids that durable marker and relies on/run/azure/pre-provision.completeplus matching controller support. As written, it can lead maintainers to gate only on the script change and overlook the controller-side prerequisite.
// A bake only reports its result through provision.complete on a VHD whose cse_start.sh writes that
// marker for a pre-provision run. Released VHDs predating that change write only base_prep.complete,
|
CI evidence for the volatile-marker design (branch head
Stage ordering shows the assertions gate the capture — So on a VHD built from this PR: the bake wrote Backward compatibility on released VHDs ( Flakes seen and resolved on retry (none marker-related —
Still red: All GitHub checks pass. |
Artur Khantimirov (r2k1)
changed the title
Artur Khantimirov (r2k1)
changed the title
| // EnableScriptlessPreProvision lets a PreProvisionOnly (image bake) run use scriptless phase 2, | ||
| // so the bake reports its result through the same `aks-node-controller provision-wait` CSE | ||
| // command as a normal node. Default false. Enabling it requires a VHD whose cse_start.sh writes | ||
| // /run/azure/pre-provision.complete and whose aks-node-controller watches it, and requires the | ||
| // caller to propagate PreProvisionOnly into the AKSNodeConfig it builds. |
| cleanup := "sudo rm -f /opt/azure/containers/provision.complete /var/log/azure/aks/provision.json" + | ||
| " /opt/azure/containers/aks-node-controller-config.json /opt/azure/containers/aks-node-controller-nbc-cmd.sh" |
|
CI on Everything passes except
The gate builds VHDs from this PR and runs E2E against them, so it is the only place a bake exercises the new marker. From its Stage ordering shows the assertions gate the capture — So on a VHD built from this PR the bake wrote both Windows: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
e2e/test_helpers.go:161
- 🟡 Medium Risk — 🏗️ Architecture: This generalization cleanup leaves
/opt/azure/containers/enabled_features.shin the captured image. The scriptless boothook only writes that file when the current request has feature toggles, so a derived node with no toggles will not overwrite a bake-time file;aks-node-controller-launcher.shwill then export the stale bake-time flags. Remove this per-run configuration file before capture alongside the other controller inputs.
cleanup := "sudo rm -f /opt/azure/containers/provision.complete /var/log/azure/aks/provision.json" +
" /opt/azure/containers/aks-node-controller-config.json /opt/azure/containers/aks-node-controller-nbc-cmd.sh" +
" /opt/azure/containers/boothook.sh /opt/bin/boothook.sh"
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2add17d1-59c8-4a9e-b7ec-2ca102204765
e6090c3 to
a407939
Compare
|
|
|
There was a problem hiding this comment.
🟡 Changes recommended
The scriptless pre-provision exception can trigger full node provisioning, while incorrect completion-marker expectations and eligibility handling can reject successful bakes or cause older-VHD timeouts.
7 open issues
provision.jsonfails (for example,…/run/azure/pre-provision.complete, but the changed CSE and…
Review effort: Balanced
| return config.EnableScriptlessNBCCSECmd || | ||
| (config.EnableScriptlessAKSNodeConfig || config.IsCustomDataOnlyProvisioningEnabled()) && !config.PreProvisionOnly |
| if scriptless { | ||
| checks = append(checks, ValidateFileExists(ctx, scenario, "/opt/azure/containers/provision.complete")) | ||
| } else { | ||
| checks = append(checks, ValidateFileDoesNotExist(ctx, scenario, "/opt/azure/containers/provision.complete")) |
| } | ||
| nbc.PreProvisionOnly = true | ||
| nbc.EnableScriptlessNBCCSECmd = false | ||
| nbc.EnableScriptlessNBCCSECmd = scriptless |
What
Make
aks-node-controller provision-waitreport the result of a PIS image bake.How
base_prep.completeandprovision.completeafterprovision.json.provision.complete.supportsScriptlessProvisioningaccepts NBC scriptless phase 2 during pre-provision, while other scriptless modes keep their existing restrictions.AKS-RP generalization removes the per-run result and configuration files before image capture while preserving
base_prep.complete.Rollout
Rollout is controlled by the RP's existing
PreProvisionOnlyfallback in PR 16870787 — no AgentBaker flag is needed.PreProvisionOnlyinto AKSNodeConfig.Validation
go test ./pkg/...— passedshellspec spec/parts/linux/cloud-init/artifacts/cse_start_spec.sh— passedcd e2e && go test ./... && go vet ./...— passed