Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaisukeMiyamoto merged 2 commits into from
Jul 8, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e4bc36
fix(aws-pcs): keep needrestart guard in lock-step across CNG templates
DaisukeMiyamoto Jul 7, 2026
040d5fc
chore(aws-pcs): address lint nits from #1168 review
DaisukeMiyamoto Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions architectures/aws-pcs/assets/add-cng-p5.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,8 @@ Resources:
# also covers the versioned units, e.g. slurmd-25.11.)
# runcmd runs under /bin/sh (dash) - keep this POSIX-clean, no bashisms.
- |
mkdir -p /etc/needrestart/conf.d
cat > /etc/needrestart/conf.d/90-pcs-slurm.conf <<'NRCONF'
mkdir -p /etc/needrestart/conf.d &&
cat > /etc/needrestart/conf.d/90-pcs-slurm.conf <<'NRCONF' &&
# AWS PCS: never auto-restart slurmd - restarting it kills
# the jobs running under it. Managed by add-cng UserData.
$nrconf{override_rc} = { qr(^slurmd) => 0 };
Expand Down
4 changes: 2 additions & 2 deletions architectures/aws-pcs/assets/add-cng-p6-b200.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,8 @@ Resources:
# also covers the versioned units, e.g. slurmd-25.11.)
# runcmd runs under /bin/sh (dash) - keep this POSIX-clean, no bashisms.
- |
mkdir -p /etc/needrestart/conf.d
cat > /etc/needrestart/conf.d/90-pcs-slurm.conf <<'NRCONF'
mkdir -p /etc/needrestart/conf.d &&
cat > /etc/needrestart/conf.d/90-pcs-slurm.conf <<'NRCONF' &&
# AWS PCS: never auto-restart slurmd - restarting it kills
# the jobs running under it. Managed by add-cng UserData.
$nrconf{override_rc} = { qr(^slurmd) => 0 };
Expand Down
4 changes: 2 additions & 2 deletions architectures/aws-pcs/assets/add-cng-p6-b300.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,8 @@ Resources:
# also covers the versioned units, e.g. slurmd-25.11.)
# runcmd runs under /bin/sh (dash) - keep this POSIX-clean, no bashisms.
- |
mkdir -p /etc/needrestart/conf.d
cat > /etc/needrestart/conf.d/90-pcs-slurm.conf <<'NRCONF'
mkdir -p /etc/needrestart/conf.d &&
cat > /etc/needrestart/conf.d/90-pcs-slurm.conf <<'NRCONF' &&
# AWS PCS: never auto-restart slurmd - restarting it kills
# the jobs running under it. Managed by add-cng UserData.
$nrconf{override_rc} = { qr(^slurmd) => 0 };
Expand Down
8 changes: 4 additions & 4 deletions architectures/aws-pcs/assets/add-cng.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,12 +461,12 @@ Resources:
# slurmd auto-restart is suppressed. (PCS runs the controller managed-side;
# slurmd is the only Slurm systemd service on login/compute nodes. qr(^slurmd)
# also covers the versioned units, e.g. slurmd-25.11.)
# runcmd runs under /bin/sh (dash) — keep this POSIX-clean, no bashisms.
# runcmd runs under /bin/sh (dash) - keep this POSIX-clean, no bashisms.
- |
mkdir -p /etc/needrestart/conf.d
cat > /etc/needrestart/conf.d/90-pcs-slurm.conf <<'NRCONF'
mkdir -p /etc/needrestart/conf.d &&
cat > /etc/needrestart/conf.d/90-pcs-slurm.conf <<'NRCONF' &&
# AWS PCS: never auto-restart slurmd - restarting it kills
# the jobs running under it. Managed by add-cng.yaml UserData.
# the jobs running under it. Managed by add-cng UserData.
$nrconf{override_rc} = { qr(^slurmd) => 0 };
NRCONF
echo "needrestart: slurmd excluded from auto-restart" | tee /var/log/pcs-needrestart-guard.log
Expand Down
30 changes: 28 additions & 2 deletions architectures/aws-pcs/tests/lint-docs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,33 @@ for prm in $params; do
grep -q "\`$prm\`" docs/PARAMETERS.md || report "deploy-all parameter '$prm' is not documented in docs/PARAMETERS.md"
done

# 4. Same-file Markdown anchor links in README.md resolve to a real heading.
# 4. The needrestart/slurmd guard block must be byte-identical across the four
# CNG templates. It is hand-duplicated (no shared include), so an edit that
# lands in only some of the copies is exactly the drift this catches.
guard_extract() { # print the guard block: comment header through the log line
# Leading whitespace is stripped because the four templates legitimately nest
# the block at different depths. Side effect: the check cannot see RELATIVE
# indentation drift inside the block (e.g. an indented NRCONF terminator, or
# <<'NRCONF' switched to the tab-stripping <<-'NRCONF' in one template) —
# those would change deployed behavior while still comparing as identical.
awk '/--- Protect running jobs from unattended-upgrades \/ needrestart ---/{p=1}
p{print}
p&&/pcs-needrestart-guard\.log/{exit}' "$1" | sed -E 's/^[[:space:]]+//'

@KeitaW KeitaW Jul 8, 2026

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

}
ref=$(guard_extract assets/add-cng.yaml)
if [ -z "$ref" ]; then
report "needrestart guard block not found in assets/add-cng.yaml"
else
for t in add-cng-p5 add-cng-p6-b200 add-cng-p6-b300; do
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 <(printf '%s\n' "$ref") <(printf '%s\n' "$other") | sed 's/^/ /'
fi
done
fi

# 5. Same-file Markdown anchor links in README.md resolve to a real heading.
# (Cross-file and external links are out of scope — kept simple on purpose.)
while IFS= read -r anchor; do
# build the set of heading slugs in README
Expand All @@ -76,6 +102,6 @@ while IFS= read -r anchor; do
done < <(grep -oE '\]\(#[a-z0-9-]+\)' README.md | sed -E 's/\]\(#//; s/\)//' | sort -u)

if [ "$fail" -eq 0 ]; then
echo "docs lint: PASS (no stale parameter references, no empty=skip wording, all deploy-all params documented, README anchors resolve)"
echo "docs lint: PASS (no stale parameter references, no empty=skip wording, all deploy-all params documented, needrestart guard in lock-step, README anchors resolve)"
fi
exit $fail