fix: report PIS bake completion - #9260
Open
r2k1 wants to merge 12 commits into
Open
Conversation
The unchanged CSE command `aks-node-controller provision-wait` waits for /opt/azure/containers/provision.complete. A pre-provision (PIS bake) run wrote only base_prep.complete, so provision-wait never saw a result. cse_start.sh now always creates provision.complete and additionally creates base_prep.complete when PRE_PROVISION_ONLY=true. base_prep.complete stays the durable phase state captured in the image. Image generalization removes provision.complete before capture, so nodes created from the image still run nodePrep and report their own result. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
r2k1
requested review from
Chou Hu (AbelHu),
Devinwong,
Sri Harsha (SriHarsha001),
Abigail Liang (abigailliang-aks-sig-node),
Abdul Asfari (aboodasfari),
Nishchay (awesomenix),
Ben Brady (benjamin-brady),
Calvin S. (calvin197),
Cameron Meissner (cameronmeissner),
Sylvain Boily (djsly),
fcher,
Ganeshkumar Ashokavardhanan (ganeshkumarashok),
janenotjung-hue,
Karen Chen (karenychen),
lilypan26,
Mark Ibrahim (mxj220),
Peter Damianov (pdamianov-dev),
Patrick W. Healy (phealy),
Runzhen (runzhen),
Shun Lyu (sinmentis),
sulixu,
Tim Wright (timmy-wright),
Thibault Cohen (titilambert),
Xu Xue (xuexu6666) and
Zachary (zachary-bailey)
as code owners
August 20, 2026 02:40
Contributor
Windows Unit Test Results 3 files 13 suites 43s ⏱️ Results for commit e6090c3. ♻️ This comment has been updated with latest results. |
Contributor
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
pkg/agent/datamodel/types.go:1825
- 🔴 High Risk — 🔒 Security: This public switch's documentation says no image-pipeline cleanup is required, but the generated
aks-node-controller-nbc-cmd.shembeds the bake cluster's TLS bootstrap token. Rewriting it on the next boot prevents runtime use of the old token but does not remove that credential from the captured image. Document the RP scrub as a prerequisite before enabling this flag, consistent with the rollout requirement and the E2E generalization step.
// watches it, so it must stay false until the target VHD contains both. It is not a switch for
// image-pipeline behaviour: the bake's completion marker is volatile and its CSE command file
// is rewritten from each node's own CustomData on every boot, so no cleanup step is required
// for correctness.
pkg/agent/baker.go:267
- 🟢 Low Risk — 🏗️ Architecture: This comment names the wrong completion contract. The updated bake deliberately does not create
provision.complete; it creates/run/azure/pre-provision.complete, and the updated controller must watch that marker. Correcting this is important because this comment explains the VHD compatibility gate.
// 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
// provision-wait would block there until the CSE timeout. PreProvisionOnly therefore stays on the
// legacy CSE command unless the caller opts in through EnableScriptlessPreProvision, which is what
// gates enablement on the first VHD version containing the change.
parts/linux/cloud-init/artifacts/cse_start.sh:129
- The PR title and the first completion-marker section state that a bake always writes
/opt/azure/containers/provision.completeand that no new marker is added, but this implementation explicitly omits that file and introduces/run/azure/pre-provision.complete. Please update the PR description/title (or the implementation) so the documented rollout contract is unambiguous.
# result for its own. provision.complete is deliberately NOT written here: it is durable, and a
# captured copy would make cse_main.sh exit before nodePrep.
mkdir -p /run/azure
touch /run/azure/pre-provision.complete
e2e/test_helpers.go:117
- 🟢 Low Risk — 🏗️ Architecture: This helper's compatibility explanation still says the bake signals through
provision.complete, although the E2E now relies on/run/azure/pre-provision.completeplus controller support for that marker. Update it so failures against older VHDs can be diagnosed from the actual 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,
// so provision-wait would block there until the CSE timeout. Restrict this to images produced by the
// VHD build under test, which is what e2e_run.sh selects via SIG_VERSION_TAG_NAME=buildId.
Comment on lines
797
to
799
| if writeErr := a.writeProvisionResult(jsonPath, provisionResult); writeErr != nil { | ||
| slog.Error("failed to write provision.json file", "path", jsonPath, "error", writeErr) | ||
| } |
signalProvisionFailure wrote the completion marker even when a forced overwrite of provision.json had failed. The stale result was then still on disk, so waking provision-wait made it report that content - a stale success reported as this run's outcome. It now returns without writing a marker, so provision-wait and CSE time out instead, which fails closed. The path where the current failure publishes successfully is unchanged. Also corrects the write-if-absent comment, which claimed nothing stale can survive the reset: the reset deliberately short-circuits when the durable marker shows this node already completed provisioning. Drops two shellspec cases and two e2e assertions that were fully subsumed by neighbouring checks, and records that gating the flag on a VHD version must also scrub plaintext CustomData residue. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
e2e/config/config.go:176
- 🟡 Medium Risk — 🧪 Test Gate:
buildIdonly means that some explicitly selected build is used;e2e-local.shallowsVHD_BUILD_IDto point to any historical build. Such an older VHD is treated as supporting the new volatile marker, so the bake opts intoprovision-waitand times out. Gate this on an explicit capability/minimum version or on a verified current-build identity rather than the tag key alone.
func (c *Configuration) UsesFreshlyBuiltVHD() bool {
return c.SIGVersionTagName == "buildId"
aks-node-controller/app.go:227
- 🟡 Medium Risk — 🔧 Script Logic: This reset now runs for
provision --dry-run, so a command documented as only printing the command can delete the liveprovision.json. Keep both the reset and failure-marker writes disabled in dry-run mode; otherwise even an invalid dry run can mutate provisioning state.
if resetErr := a.resetStaleProvisionResult(); resetErr != nil {
pkg/agent/baker.go:267
- This compatibility comment describes the obsolete durable-marker design: a successful bake deliberately never writes
provision.complete; it reports through the volatile marker. Document both the new marker and the required controller support so callers understand the actual version gate.
// 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
// provision-wait would block there until the CSE timeout. PreProvisionOnly therefore stays on the
// legacy CSE command unless the caller opts in through EnableScriptlessPreProvision, which is what
// gates enablement on the first VHD version containing the change.
e2e/test_helpers.go:117
- This comment says the bake depends on
provision.complete, but the changed script intentionally uses/run/azure/pre-provision.completeand never writes the durable marker on success. Correcting this matters because this helper's gate is specifically meant to avoid hanging on VHDs without the volatile-marker contract.
// 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,
// so provision-wait would block there until the CSE timeout. Restrict this to images produced by the
// VHD build under test, which is what e2e_run.sh selects via SIG_VERSION_TAG_NAME=buildId.
parts/linux/cloud-init/artifacts/cse_start.sh:127
- The PR title and “Dual completion markers” section say the bake always writes durable
provision.complete, but this implementation deliberately does not and instead introduces/run/azure/pre-provision.complete. Update the PR contract and rollout text to match the volatile-marker design; otherwise downstream enablement may check for the wrong VHD capability.
# The bake reports its result through a volatile marker on tmpfs, which
# `aks-node-controller provision-wait` also watches. /run is cleared on boot, so this marker
# cannot survive image capture and a node created from the image can never mistake the bake's
# result for its own. provision.complete is deliberately NOT written here: it is durable, and a
# captured copy would make cse_main.sh exit before nodePrep.
Comment on lines
+1830
to
+1833
| // TODO: before the flag is turned on, the change that gates it on a VHD version must also scrub | ||
| // the plaintext CustomData residue an image capture would otherwise retain, using supported | ||
| // cloud-init cleanup. Deleting aks-node-controller-nbc-cmd.sh addresses one known credential | ||
| // file and does not make a captured image free of provisioning secrets. |
A minimum VHD version carrying the volatile marker and its watcher is necessary but not sufficient. The parser renders PRE_PROVISION_ONLY from aksnodeconfig Configuration.PreProvisionOnly, so unless the RP's GetAKSNodeConfig propagates NodeBootstrappingConfiguration.PreProvisionOnly into it, a bake driven by a provision config renders PRE_PROVISION_ONLY=false, runs full provisioning instead of basePrep only, and writes the durable provision.complete into the captured image. Documentation only. No behaviour change, no version named, and the plaintext CustomData scrub TODO for the final flag-flip is retained. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
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,
Contributor
Author
|
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. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
r2k1
changed the title
r2k1
changed the title
provision-wait waits for /opt/azure/containers/provision.complete, which a pre-provision (PIS image bake) run never wrote, so the bake could not report a result through the unchanged CSE command. cse_start.sh now always writes it, and additionally writes base_prep.complete when PRE_PROVISION_ONLY is set. RP image generalization removes provision.complete before capture, so a node created from the image still provisions and reports its own result. The aks-node-controller is unchanged. EnableScriptlessPreProvision (default false) makes a bake's use of scriptless generation explicit and gates it on the coordinated rollout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment on lines
+1817
to
+1821
| // 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. |
The RP's PreProvisionOnly fallback already gates the rollout, so a second capability flag duplicated that control. supportsScriptlessPhase2 now depends only on EnableScriptlessNBCCSECmd, so AgentBaker honours a caller that requests scriptless during a bake. Two existing tests asserted the previous fallback and are updated: a bake now renders scriptless custom data and gets the provision-wait CSE command. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment on lines
+159
to
+160
| 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" |
Contributor
Author
|
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: |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
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"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Make
aks-node-controller provision-waitreport the result of a PIS image bake.How
base_prep.completeandprovision.completeafterprovision.json.provision.complete.supportsScriptlessPhase2no longer excludes a pre-provision run, so AgentBaker honours a caller that requests scriptless generation for a bake.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