Conversation
chmill-zz
requested review from
a team,
Devinwong,
C.YAO (YaoC),
Nishchay (awesomenix),
Cameron Meissner (cameronmeissner),
Sylvain Boily (djsly),
lilypan26,
Artur Khantimirov (r2k1),
Tim Wright (timmy-wright) and
Ye Wang (yewmsft)
as code owners
September 17, 2026 22:18
Contributor
Windows Unit Test Results 3 files 17 suites 1m 2s ⏱️ Results for commit 2dee3ce. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A failed kubelet restart can later be reported as successful without retrying the restart.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds generic Azure Linux 3 security-patch reconciliation while retaining legacy behavior and hotfix delivery.
Changes:
- Validates generic goals, selects pool-specific profiles, checkpoints progress, and reports status.
- Adds ShellSpec coverage for reconciliation and failure paths.
- Adds hotfix generation and VHD artifact validation.
File summaries
| File | Description |
|---|---|
mariner-package-update.sh |
Implements generic patch reconciliation. |
mariner-package-update_spec.sh |
Tests generic and legacy behavior. |
nodecustomdata.yml |
Delivers the updater to supported images. |
hotfix_generate.py |
Registers the updater for hotfixing. |
hotfix_generate_test.py |
Tests hotfix selection and guards. |
linux-vhd-content-test.sh |
Validates installed patching assets. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Status handling can lose or skip sibling component results, and repository rewrite failures may be reported as successful.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
parts/linux/cloud-init/artifacts/mariner/mariner-package-update.sh:247
- 🟡 Medium Risk — 🔧 Script Logic:
apply_security_patchis evaluated in anelif ... && ...condition, so Bash suppresseserrexitthroughout its nested calls.rewrite_reposstill relies onset -e; a failedcat/sedcan therefore be followed by a successful update and checkpoint, leaving a network-isolated node's repository configuration stale while reporting success. Add explicit failure checks for repository reads and mutations and propagate a nonzero result before running package updates.
This issue also appears in the following locations of the same file:
- line 500
- line 522
parts/linux/cloud-init/artifacts/mariner/mariner-package-update.sh:502
- 🟡 Medium Risk — 🏗️ Architecture: This rebuilds
componentsonly from this updater's local results andannotate --overwritethen erases any sibling component status already published for the same goal. Since the envelope supports multiple components and NPD is a separate consumer, a later security-patch reconciliation can discard NPD's result. Merge the existing annotation'scomponentswhen itscurrentHashmatches, then overwrite onlysecurityPatch; never merge components from a stale hash.
status=$(printf '%s' "${LIVE_PATCHING_COMPONENT_RESULTS}" | jq -c --arg currentHash "${goal}" '{currentHash:$currentHash,components:.}') || return 1
# shellcheck disable=SC2086
$KUBECTL annotate --overwrite node "${node_name}" "${LIVE_PATCHING_STATUS_ANNOTATION}=${status}"
parts/linux/cloud-init/artifacts/mariner/mariner-package-update.sh:523
- 🟡 Medium Risk — 🔧 Script Logic: This considers any same-hash status with only successful entries converged, even
{components:{}}or a status containing only a sibling such asnpd. In a multi-component goal, another reconciler can publish its result first and cause this updater to skip a pendingsecurityPatch. The fast path must require this updater's own component result to beSucceeded.
if [ -n "${status}" ] && printf '%s' "${status}" | jq -e --arg goal "${goal}" \
'.currentHash == $goal and (.components | type == "object") and (.components | all(.code == "Succeeded"))' > /dev/null 2>&1; then
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
|
Contributor
|
Contributor
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unsupported components can incorrectly cause the complete goal to be reported and cached as converged.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
parts/linux/cloud-init/artifacts/mariner/mariner-package-update.sh:548
- 🟡 Medium Risk — 🔄 Backward Compatibility: An unknown component is only logged, so the function still publishes the full goal hash as successful. On the next timer run, the
all(.code == "Succeeded")fast path accepts the partial/empty result map; if a later updater adds support for that component (for example the planned NPD work) while the goal is unchanged, it will never process it. Record unsupported components as failed so this version cannot claim convergence for work it did not perform.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This branch has not been deployed
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 type of PR is this?
/kind feature
What this PR does / why we need it
Adds AgentBaker-side consumption of the generic LPC
securityPatchcontract for Azure Linux 3. The existing RPM repository,tdnf --snapshottime, network-isolated repository, custom patching, and kubelet update behavior remain OS-specific insidemariner-package-update.sh.goldenTimestampand optionalkubeletVersion.live-patching-status, allowing status repair without repeating package updates.mariner-package-update.shfor Mariner/Azure Linux nodes; systemd units remain unchanged and ACL/OSGuard/Flatcar stay excluded.The RP/LPC side already emits the OS-neutral
securityPatch.nodeConfigconsumed here. NPD generalization is separate work.Which issue(s) this PR fixes
None.
Special notes for your reviewer
This intentionally starts with Azure Linux 3 generic consumption while preserving frozen v2 behavior. Existing custom-package validation, Kata package policy, and timer cadence are unchanged by this PR.
Does this PR introduce a user-facing change?
Validation
shellspec spec/parts/linux/cloud-init/artifacts/mariner-package-update_spec.sh: 42 examples, 0 failuresPYTHONDONTWRITEBYTECODE=1 python3 -m unittest hotfix.hotfix_generate_test: 26 tests passedgo test ./hotfix/render-nodecustomdata ./pkg/agent: passedshellcheck parts/linux/cloud-init/artifacts/mariner/mariner-package-update.sh: passedbash -nfor updater and VHD content test: passedgit diff --check: passedRequirements