fix(aws-pcs): stop needrestart from restarting slurmd and killing running jobs

[DaisukeMiyamoto]

fix(aws-pcs): stop needrestart from restarting slurmd and killing running jobs

Problem

On the Ubuntu PCS-ready DLAMI, unattended security upgrades (apt-daily-upgrade +
unattended-upgrades) update base libraries such as glibc. needrestart then runs in
automatic mode and restarts every service linked against an updated library. slurmd
links libc, so needrestart restarts slurmd — and restarting slurmd tears down the
slurmstepd steps under it, killing every job running on that node (the job stops and,
if requeued, restarts from scratch, losing in-progress work).

The kill time therefore lines up with apt-daily-upgrade.timer. This is a deterministic
interaction, reproducible on demand: it occurs whenever an unattended upgrade updates a
library slurmd uses.

Two clarifications, because both are easy to assume and both are wrong:

• It is not a Slurm-package upgrade. slurmd lives under /opt/aws/pcs/... and is not
  apt-managed; nothing upgrades Slurm here.
• It is not a reboot. Automatic-Reboot is off by default; the job dies solely from
  the slurmd restart.

Fix

Each compute-node-group template (add-cng.yaml and the P5 / P6-B200 / P6-B300 GPU
templates) writes a needrestart drop-in at first boot that excludes slurmd from
automatic restart:

# /etc/needrestart/conf.d/90-pcs-slurm.conf
$nrconf{override_rc} = { qr(^slurmd) => 0 };

• Security packages still install as before; needrestart still restarts everything else.
  Only the slurmd restart is suppressed (needrestart reports it as deferred), so an
  unattended upgrade can no longer stop a running job.
• slurmd is the only Slurm systemd service on login/compute nodes — PCS runs the
  controller managed-side, so there is no slurmctld/slurmdbd service to guard. The
  qr(^slurmd) pattern also covers the versioned units (e.g. slurmd-25.11).
• slurmd picks up the updated libraries the next time it restarts on its own terms (node
  replacement, power-save cycle, manual restart), which is acceptable for PCS, where nodes
  are replaced regularly.

Scope note: the drop-in is a standalone conf.d/*.conf naming only slurmd. If a future
DLAMI or Slurm unit addresses this differently, the file neither conflicts nor errors —
needrestart reads conf.d/*.conf in order and ignores keys it does not use — so it is
safe to keep even after an upstream fix.

Verification

Verified end-to-end on real hardware (PCS clusters, us-east-2), comparing an unmodified
cluster with one carrying this change. On each, a long-running job was started, then a real
apt-get upgrade + libc6 reinstall + needrestart -r a was run on the compute node:

	Without the drop-in	With the drop-in (this PR)
needrestart action on slurmd	restarts it	defers it (no restart)
slurmd PID	changes	unchanged
running job	killed	survives

Tests

• aws cloudformation validate-template passes on all assets/*.yaml.
• tests/lint-docs.sh: PASS.
• The rendered UserData runcmd step was checked under /bin/sh (dash) — POSIX-clean, no
  bashisms — and the generated 90-pcs-slurm.conf passes perl -c.

Docs

docs/OPERATIONS.md §6.2 documents the interaction and the shipped mitigation.

Files changed

• architectures/aws-pcs/assets/add-cng.yaml
• architectures/aws-pcs/assets/add-cng-p5.yaml
• architectures/aws-pcs/assets/add-cng-p6-b200.yaml
• architectures/aws-pcs/assets/add-cng-p6-b300.yaml
• architectures/aws-pcs/docs/OPERATIONS.md

[KeitaW]

Review Batch 1/2 — Deployment Pipeline & Operational Correctness (optional nits)

Thanks for this — a well-scoped fix with a genuinely thorough writeup and on-hardware verification. No blocking issues; three low-severity notes below (two inline FYIs on edge-case behavior, one worth acting on).

The four templates have already drifted cosmetically — worth keeping them in lock-step

The guard block is duplicated verbatim across add-cng.yaml, add-cng-p5.yaml, add-cng-p6-b200.yaml, and add-cng-p6-b300.yaml, and the diff already shows two small divergences in add-cng.yaml: it uses an em-dash (—) in the # runcmd runs under /bin/sh (dash) — keep … line where the other three use an ASCII hyphen, and its drop-in comment says Managed by add-cng.yaml UserData where the others say Managed by add-cng UserData. Both are inside # comments so they're harmless today — the payload is identical. The reason to flag it: four hand-maintained copies of the same block is exactly where a load-bearing edit later lands in three of four files. I'd suggest either normalizing add-cng.yaml back to the other three's wording now, or — better long-term — a small CI assertion that the drop-in block is byte-identical across the four templates so drift fails the build.

[KeitaW]

qr(^slurmd) also prefix-matches slurmdbd — no change needed here (FYI)

qr(^slurmd) is an unanchored-prefix regex, so it also matches slurmdbd.service in addition to slurmd and the versioned slurmd-25.11. On PCS login/compute nodes this has no practical effect — as the comment right above correctly notes, slurmd is the only Slurm unit here (the controller and slurmdbd are managed PCS-side). I'd leave it exactly as written: tightening it to qr(^slurmd($|-|\.)) would only guard a unit that isn't present on these nodes. Flagging purely so the prefix-match is a known, deliberate property. It's the same qr(^name) => 0 idiom needrestart ships for dbus/display-managers.

[KeitaW]

tee writes the success line unconditionally (nit)

The echo … | tee runs regardless of whether the preceding mkdir -p / cat > actually succeeded (no set -e, no && chain), so in the near-impossible failure case (e.g. a read-only /etc at first boot) the log would still read needrestart: slurmd excluded from auto-restart when it wasn't. Cosmetic only — the failure it would misreport isn't a realistic first-boot condition, so I wouldn't hold the PR for it. Mentioning it in case you'd prefer the log line to be strictly truthful (chain the write with && before the echo).

[KeitaW]

Review Batch 2/2 — Things That Look Great & Sources

Things That Look Great

• The two ways this heredoc could have broken are both handled. (1) Fn::Sub-safe: the whole UserData is Fn::Base64: !Sub |, so any literal ${…} in the body would be read as a CloudFormation variable and fail the template — there's no ${ anywhere in the added text ($nrconf{…} is $ + n, not ${; and = { qr(…) has a space between = and {). (2) Quoted heredoc + de-indentation: <<'NRCONF' stops dash from expanding $nrconf, and the terminator de-indents correctly out of the 14-space YAML literal block so the heredoc actually closes — both must hold, and your perl -c + hardware test show they do.
• The needrestart mechanism is exactly right — and I re-verified it end-to-end on a fresh box. On a throwaway Ubuntu 24.04 instance (needrestart 3.6, slurm-wlm 23.11): with the drop-in absent, needrestart -r a (after a libc6 reinstall orphans the mapped lib) restarts slurmd (PID changes, listed under Restarting services); with the exact drop-in this PR writes, slurmd moves to Service restarts being deferred and its PID is unchanged — while a cron control is restarted in both runs, confirming -r a still acts. That's precisely the deferred behavior described in the PR. (One scope note for future readers: the downstream job-kill depends on the unit's KillMode; stock Ubuntu's slurmd.service is KillMode=process, so that half is specific to the PCS slurmd unit — which your on-hardware PCS A/B already covers.)
• The verification is the gold standard for this repo — a real A/B on PCS hardware with an actual apt-get upgrade + libc6 reinstall + needrestart -r a, reporting slurmd PID and job survival as raw before/after rather than a claim.
• The security tradeoff is stated, not hidden — both the inline comment and OPERATIONS.md §6.2 make explicit that slurmd keeps running against the pre-upgrade libc until its next natural restart, the deliberate cost of the fix.
• Placement and scope are correct — first runcmd item, a persistent file that covers every future apt-daily-upgrade run; and a standalone conf.d/*.conf naming only slurmd, so a later DLAMI/Slurm change neither conflicts nor errors.

Sources

• needrestart override_rc semantics — upstream ex/needrestart.conf ships $nrconf{override_rc} = { qr(^dbus) => 0, qr(^gdm) => 0, … } under “Override service default selection (hash of regex)”; value 0 suppresses auto-restart, qr(^…) is a prefix match: https://github.com/liske/needrestart/blob/master/ex/needrestart.conf
• Fn::Sub substitution (literal $ + non-{ passes through; a bare ${ needs ${!Literal} escaping): https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-sub.html
• e2e re-verification: throwaway Ubuntu 24.04 EC2, needrestart 3.6 / slurm-wlm 23.11 — slurmd restarted without the drop-in, deferred with it, cron control restarted in both (verified live, 2026-07-07).

[KeitaW]

Approving. The fix is correct, tightly scoped, and independently verified end-to-end: on a fresh Ubuntu 24.04 box, needrestart -r a restarts slurmd without the drop-in and defers it with the exact drop-in this PR writes (a cron control restarts in both runs) — matching the on-hardware PCS A/B in the description. Fn::Sub-safe and POSIX-clean under dash. Only optional nits (see review comments); none blocking.