fix: publish provision result atomically - #9401
Open
r2k1 wants to merge 3 commits into
Open
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
r2k1
requested review from
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
September 4, 2026 09:15
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
Windows Unit Test Results 3 files 13 suites 48s ⏱️ Results for commit 2bdd729. ♻️ This comment has been updated with latest results. |
Contributor
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.
Pull request overview
This PR makes provisioning result publication atomic so readers never observe a partially-written provision.json, while keeping provision.complete as a compatibility signal until the deprecation window ends.
Changes:
- Add atomic “stage then rename” publishing for
provision.jsonincse_start.sh, and ensure completion markers are created after publication. - Add Go-side atomic fallback publishing (
writeProvisionResultAtomically) and tests covering preservation/idempotency/error paths. - Update documentation to describe the new provisioning result semantics and compatibility plan.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/parts/linux/cloud-init/artifacts/cse_start_spec.sh | Adds shellspec coverage for atomic provision result publishing and failure modes. |
| parts/linux/cloud-init/artifacts/cse_start.sh | Introduces publishProvisionResponse to stage+rename provision.json and write completion markers. |
| aks-node-controller/const.go | Documents future removal of provision.complete compatibility. |
| aks-node-controller/app_test.go | Adds tests for atomic publishing, preservation, and fsnotify event semantics. |
| aks-node-controller/app.go | Implements Go fallback atomic write + completion marker logic. |
| aks-node-controller/README.md | Updates provisioning flow docs to emphasize atomic provision.json publication and compatibility behavior. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+726
to
+727
| data, marshalErr := json.Marshal(provisionResult) | ||
| if marshalErr != nil { |
Comment on lines
+741
to
+743
| if _, statErr := os.Stat(filepaths.ProvisionCompleteFile); statErr == nil { | ||
| return nil | ||
| } else if !errors.Is(statErr, os.ErrNotExist) { |
Comment on lines
+771
to
+773
| } | ||
| if err := tmp.Chmod(0644); err != nil { | ||
| return fmt.Errorf("set temporary provision result mode %s: %w", tmpPath, err) |
| }) | ||
| } | ||
|
|
||
| func TestAtomicRenameProducesCreateEvent(t *testing.T) { |
| case watchErr := <-watcher.Errors: | ||
| require.NoError(t, watchErr) | ||
| case <-timeout: | ||
| t.Fatal("timed out waiting for Create event after atomic rename") |
| ### Provisioning Flow | ||
|
|
||
| Here is an indepth explanation of the provisioning flow. Upon first startup, CustomData is made available to the VM, after which cloud-init is able to process the content, in this case, writing the bootstrap config to disk. The binary is triggered by a systemd unit, [`aks-node-controller.service`](https://github.com/Azure/AgentBaker/blob/dev/parts/linux/cloud-init/artifacts/aks-node-controller.service) which is automatically run once cloud-init is complete. In this way, we are ensuring the bootstrapping config is present on the node and can proceeed to run the go binary to start the bootstrapping process. | ||
| On first startup, cloud-init processes CustomData and writes the bootstrap configuration to disk. The cloud boothook starts [`aks-node-controller.service`](../parts/linux/cloud-init/artifacts/aks-node-controller.service) after the configuration is available, and the controller starts the bootstrap process. |
Comment on lines
+777
to
782
| } | ||
| if err := os.Rename(tmpPath, path); err != nil { | ||
| return fmt.Errorf("publish provision result %s: %w", path, err) | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unquoted JSON expansion can corrupt results, and baked PIS results can mask real-node bootstrap failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
parts/linux/cloud-init/artifacts/cse_start.sh:17
- 🟡 Medium Risk — 🔧 Script Logic: Expanding
responseunquoted allows field splitting and pathname expansion in both emitted and staged JSON. For example, anOutputvalue containingfiles * hereis rewritten using filenames from the working directory, corrupting diagnostics and potentially producing invalid JSON. Useprintfwith a quoted argument for both writes.
if ! echo ${response} > "${response_tmp}" ||
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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 this PR does / why we need it:
provision-waitusesprovision.completebecause writers could expose a partialprovision.json. Both Linux writers now stage the result beside the target and rename it into place. Bash keeps the detailed CSE result; ANC writes a fallback when bootstrap cannot start.The PR keeps
provision.completefor compatibility. We can remove it after 2026-10-01, once compatible VHDs have shipped.Which issue(s) this PR fixes:
N/A