Skip to content

fix(aws-pcs): keep needrestart guard in lock-step across CNG templates - #1168

Merged
DaisukeMiyamoto merged 2 commits into
awslabs:mainfrom
DaisukeMiyamoto:fix/needrestart-guard-lockstep
Jul 8, 2026
Merged

DaisukeMiyamoto merged 2 commits into
awslabs:mainfrom
DaisukeMiyamoto:fix/needrestart-guard-lockstep

Conversation

@DaisukeMiyamoto

@DaisukeMiyamoto DaisukeMiyamoto commented Jul 7, 2026 •

Copy link
Copy Markdown
Collaborator

What

Follow-up to #1165 review (non-blocking nits):

  • Normalize the guard block — add-cng.yaml's copy had drifted cosmetically
    (em-dash vs ASCII, add-cng.yaml vs add-cng in the drop-in comment); now
    byte-identical with the other three templates.
  • Lint check for future drift — tests/lint-docs.sh now asserts the
    needrestart guard block is byte-identical across add-cng / add-cng-p5 /
    add-cng-p6-b200 / add-cng-p6-b300, so an edit landing in only some of the
    four hand-maintained copies fails the lint. The whitespace-normalization
    blind spot (relative intra-block indentation drift) is documented in the
    extractor, and the failure diff is fed via printf (review feedback).
  • Truthful success log — chain the guard's mkdir/cat with && so
    needrestart: slurmd excluded from auto-restart is only logged when the
    drop-in was actually written.

Testing

  • Lint passes on the normalized templates; injecting a 1-char divergence into
    add-cng-p5.yaml makes it FAIL with a diff (re-verified after the printf
    change).
  • The chained heredoc block runs clean under dash (exit 0, conf + log written);
    with an unwritable target it exits 1 and writes no log line.
  • validate-template passes on all four templates.

Follow-up to awslabs#1165 review:
- normalize add-cng.yaml's guard-block comments to match the other three
  templates (ASCII dash, 'Managed by add-cng UserData')
- add a lint-docs.sh check that the guard block is byte-identical across
  add-cng / add-cng-p5 / add-cng-p6-b200 / add-cng-p6-b300, so a later
  edit that lands in only some of the four hand-maintained copies fails
  the lint
- chain the guard's mkdir/cat with && so the 'excluded from auto-restart'
  log line is only written when the drop-in was actually installed

Verified: lint passes, detects a 1-char divergence when injected; the
chained heredoc block runs clean under dash (and skips the log line
when the write fails); validate-template passes on all four templates.

@KeitaW KeitaW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean follow-up to #1165 — approving. I verified the three load-bearing claims empirically and they all hold; no blocking or should-fix issues, just two optional nits inline on the new lint.

What looks great

  • This is exactly the fix the #1165 review asked for — it normalizes add-cng.yaml's cosmetic drift (em-dash → ASCII hyphen, add-cng.yaml → add-cng in the drop-in comment) and adds the standing CI assertion that keeps the four hand-maintained copies byte-identical. Closing a review nit with a lint that prevents its recurrence is the right instinct.
  • The && chaining fixes a real truthfulness bug, verified under dash — mkdir -p … && cat > … <<'NRCONF' && echo … | tee …log parses as mkdir && cat && (echo | tee); the && after the heredoc redirect doesn't corrupt heredoc parsing. On success the conf + log are written (exit 0); with an unwritable target the compound returns non-zero and the "excluded from auto-restart" line is correctly not logged. The old newline-separated form logged success even when the cat failed.
  • The exit-code change is safe under cloud-init — runcmd runs under /bin/sh with no set -e, so a now-non-zero return surfaces in cloud-init-output.log without aborting later runcmd entries.
  • Quoted heredoc terminator is correct and necessary — <<'NRCONF' keeps the Perl $nrconf{override_rc} literal instead of expanding $nrconf.
  • The lint carries its own rationale and prints an indented diff pinpointing which copy drifted, so a maintainer hitting it in CI gets the fix handed to them.

Verified live (2026-07-08): ran guard_extract over all four templates at the PR head — byte-identical after normalization, the new check passes, and a 1-char injection into a copy fails it as intended.

guard_extract() { # print the guard block: comment header through the log line
awk '/--- Protect running jobs from unattended-upgrades \/ needrestart ---/{p=1}
p{print}
p&&/pcs-needrestart-guard\.log/{exit}' "$1" | sed -E 's/^[[:space:]]+//'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard comparison strips all leading whitespace — worth noting it can't see relative intra-block indentation drift

guard_extract normalizes each block with sed -E 's/^[[:space:]]+//' before comparing, which is the right call for the base indentation: the four templates legitimately nest the block at different depths, so a raw byte compare would false-positive. One side effect worth a comment — stripping all leading whitespace also blinds the check to relative indentation inside the block. If a future edit indented the NRCONF terminator, or switched <<'NRCONF' to the tab-stripping <<-'NRCONF' in only one template, the deployed heredoc would behave differently while this check still reported the blocks as identical. Nothing's wrong today (the block has no intentionally-indented content lines) — just worth naming the boundary so a later reader doesn't over-trust "byte-identical".

Suggested change
p&&/pcs-needrestart-guard\.log/{exit}' "$1" | sed -E 's/^[[:space:]]+//'
p&&/pcs-needrestart-guard\.log/{exit}' "$1" | sed -E 's/^[[:space:]]+//' # normalizes leading WS: catches text drift, not relative intra-block indentation

other=$(guard_extract "assets/$t.yaml")
if [ "$other" != "$ref" ]; then
report "needrestart guard block in assets/$t.yaml differs from assets/add-cng.yaml (keep the four copies byte-identical):"
diff <(echo "$ref") <(echo "$other") | sed 's/^/ /'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

printf '%s\n' is drift-proof where echo is safe only by luck of the first line

diff <(echo "$ref") <(echo "$other") works here because the block's first line always begins with #, but echo mangles a leading -n/-e token on some shells and can interpret backslash escapes. This only feeds the human-readable diff shown on failure (never the pass/fail decision — that's the earlier string compare), so it's cosmetic, but printf is the robust idiom and costs nothing:

Suggested change
diff <(echo "$ref") <(echo "$other") | sed 's/^/ /'
diff <(printf '%s\n' "$ref") <(printf '%s\n' "$other") | sed 's/^/ /'

- comment the whitespace-normalization blind spot in guard_extract
  (relative intra-block indentation drift is not detectable)
- printf instead of echo when feeding the failure diff
@DaisukeMiyamoto
DaisukeMiyamoto merged commit d307c18 into awslabs:main Jul 8, 2026
@DaisukeMiyamoto
DaisukeMiyamoto deleted the fix/needrestart-guard-lockstep branch July 8, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants