feat(aws-pcs): migrate CNG first-boot setup from UserData to PCS node lifecycle actions

[DaisukeMiyamoto]

Summary

Moves ALL first-boot logic in the four CNG templates from cloud-init UserData
to PCS node lifecycle actions; the templates carry no UserData at all now.
Requires PCS agent >= 1.5.0-1 (PCS-Ready DLAMI 2026-07-20+; the default SSM
latest resolution qualifies — see docs/PCS-READY-DLAMI.md).

Script	Stage	ExecutionPolicy	OnError
needrestart-guard	nodeBootstrapped	FIRST_BOOT_ONLY	CONTINUE
mount-openzfs-home	nodeBootstrapped	EVERY_BOOT	TERMINATE
mount-lustre-fsx (if set)	nodeBootstrapped	EVERY_BOOT	TERMINATE
setup-directory (if enabled)	nodeBootstrapped	FIRST_BOOT_ONLY	CONTINUE
install-enroot-pyxis (if enabled)	nodeBootstrapped	FIRST_BOOT_ONLY	CONTINUE
install-monitoring (if enabled)	nodeReady	FIRST_BOOT_ONLY	CONTINUE

Why

Resiliency. Node setup now fails safe instead of degrading silently:

• A node that can't mount /home / /fsx is terminated and replaced —
  previously it stayed in service and accepted jobs without shared storage.
• Script delivery is retried by the PCS agent, and the boot path no longer
  shells out to the AWS CLI at all — the CRT region-redirect issue that broke
  cross-region fetches on P5/P6 is gone structurally.
• Reboots re-run the (idempotent) mount scripts, so a rebooted node regains
  /fsx; the old once-per-instance runcmd silently lost it.
• Every script has its own error policy and log
  (/var/log/amazon/pcs/lifecycle/actions/<stage>/<name>.log), so partial
  failures are visible and scoped instead of buried in one cloud-init
  transcript.

Operability. Lifecycle config updates reach existing CNGs with DRAIN
semantics (UserData edits required CNG re-creation), scripts are plain bash
files that can be linted (cloud-init's dash-only runcmd once shipped a
node-bricking bashism), and the console shows the per-CNG script list.

Breaking changes

Documented in-repo in docs/OPERATIONS.md §8 ("Upgrading from
UserData-based templates"):

• PostInstallScriptUrl/PostInstallScriptArgs replaced by
  InstallEnrootPyxis (true/false; defaults preserve prior behavior).
  The generic hook only ever shipped the Enroot/Pyxis installer — custom
  first-boot scripts attach to the node group's lifecycle actions directly.
• Pinned/custom AMIs need PCS agent >= 1.5.0-1.
• Boot logs moved to the per-script lifecycle logs; mount failures now
  replace the node.

Publishing

.github/template-publish-manifest.yml adds the four new scripts (without
them, post-merge deploys would fail at script download). lint-docs.sh now
cross-checks every template-referenced script against the manifest in CI.

Verification

End-to-end on fresh deployments of the updated templates
(pcs-ml-cluster-deploy-all.yaml; login + compute; us-east-2 and
ap-south-1). Verified on every node type:

• All six lifecycle scripts run in order and exit 0, each writing its own
  log under /var/log/amazon/pcs/lifecycle/actions/; no UserData remnants
  on the nodes.
• /home (OpenZFS) and /fsx (Lustre) mounted; /home/ubuntu contents
  preserved; needrestart drop-in in place.
• Enroot/Pyxis installed before slurmd starts: srun --container-image=...
  works, including GPU containers (nvidia-smi sees the L4 on g6, all
  8× B200 on p6-b200).
• Multi-user directory: slapd (login) + SSSD (compute) up, ldap-add-user
  resolves on both node types.
• Monitoring: Grafana HTTPS 200, Prometheus targets up, nvidia-dcgm on GPU
  nodes.
• EFA: hpc7a.96xlarge ×2 with EfaInterfaceCount=2 + auto placement group —
  2-node job runs, efa provider visible in-job via fi_info.
• InstallEnrootPyxis=false skip path; docs commands run as written.

Coverage: CPU (c6i), single-NIC GPU (g6), EFA CPU (hpc7a ×2), and multi-NIC
GPU via Capacity Block (p6-b200, 8 NICs, ap-south-1 — exercises the mount
scripts' runtime IMDS region resolution). Negative paths: a missing S3
script exits 1 after logged retries; a script absent from the publish
manifest fails lint-docs.sh.

Docs

README (architecture diagram, param tables, AMI section), PARAMETERS,
OPERATIONS (§2, §4, §6 + new §8 migration notes), DEPLOY-TESTING,
USER-MANAGEMENT, CUSTOM-AMI, ROADMAP, tests/infra + storage +
readme-walkthrough — all UserData-era references updated; old parameter
names are lint-banned.

[KeitaW]

Review Batch 1/6 — Boot-path failure modes (the two TERMINATE actions)

The two mount scripts are the only lifecycle actions wired OnError: TERMINATE, so
they are the only two whose failure destroys the instance. Both findings below are
about that path.

[KeitaW]

The IMDS region guard can't fire on a connection failure, and passes garbage on an HTTP failure

mount-openzfs-home.sh:28-33 (and mount-lustre-fsx.sh:28-33, byte-identical) reads:

TOKEN=$(curl -s -X PUT ".../api/token" -H "..." || true)   # <- guarded
if [ -n "$TOKEN" ]; then
  REGION=$(curl -s -H "X-aws-ec2-metadata-token: $TOKEN" \
    .../placement/region)                                   # <- not guarded
else
  REGION=$(curl -s .../placement/region)                    # <- not guarded
fi
: "${REGION:?failed to resolve region from IMDS}"

The || true on the TOKEN line shows the intent, but under set -euo pipefail a
REGION=$(cmd) assignment takes the command substitution's exit status. When curl
fails to connect at all — rather than returning an empty body — the script dies on
the assignment line, so the :? diagnostic one line below is unreachable.

Verified in a container (Ubuntu 24.04, IMDS unreachable):

$ bash mount-openzfs-home.sh fs-0abcdef1234567890
EXIT=7
$ grep -c "failed to resolve region from IMDS" out.txt
0     # the guard never fired

and the mechanism in isolation, with and without the guard:

$ bash -c 'set -euo pipefail; R=$(curl -s --max-time 2 http://169.254.169.254/...); : "${R:?failed to resolve region from IMDS}"'
EXIT=7                                    # silent

$ bash -c 'set -euo pipefail; R=$(curl -s --max-time 2 http://169.254.169.254/... || true); : "${R:?failed to resolve region from IMDS}"'
bash: line 1: R: failed to resolve region from IMDS
EXIT=1                                    # guard restored

Impact. This is the one case where losing the message really costs something.
OnError: TERMINATE means the instance is gone before anyone can SSH in, and the AWS
guidance is explicit that lifecycle logs are local to the instance (Logging and
debugging).
So the operator investigating a node-replace loop opens
/var/log/amazon/pcs/lifecycle/actions/nodeBootstrapped/mount-openzfs-home.log, finds
it truncated with no error line, and has no instance left to inspect — for a failure
whose cause the script already knew and was written to report.

There is a second, opposite path to the same place. curl -s without -f exits 0 and
captures an HTTP error body:

$ V=$(curl -s  <404 url>); echo "$? [$V]"   ->  0 [404: Not Found]
$ V=$(curl -sf <404 url>); echo "$? [$V]"   ->  22 []

All four templates set HttpTokens: required (add-cng.yaml:499), so if the token PUT
is throttled and returns a body, [ -n "$TOKEN" ] at :27 is true, the region GET runs
with a bad token, and the 401 body lands in REGION. :33 sees a non-empty value and
passes — DNS becomes fs-abc.fsx.<html>.amazonaws.com, the mount fails, and the node is
terminated. The else branch at :30-31 has the same problem: under
HttpTokens: required an unauthenticated GET can only ever return a 401 body.

So the guard is unreachable on a connection failure and passable with garbage on an HTTP
failure — it fires only when curl succeeds and returns an empty body, which is the one
case IMDS doesn't really produce. Adding -f plus a retry fixes both halves at once.
Four sites (both branches, both scripts):

Suggested change

	REGION=$(curl -s -H "X-aws-ec2-metadata-token: $TOKEN" \
	http://169.254.169.254/latest/meta-data/placement/region)
	REGION=$(curl -sf --retry 5 --retry-connrefused --connect-timeout 2 --max-time 5 \
	-H "X-aws-ec2-metadata-token: $TOKEN" \
	http://169.254.169.254/latest/meta-data/placement/region || true)

[KeitaW]

TERMINATE on the mounts with no retry turns a known-transient failure into node churn

add-cng.yaml:367-368 (and :377-378 for Lustre) wire the mounts EVERY_BOOT +
OnError: TERMINATE, and the mount call in each script is single-shot:
mount-openzfs-home.sh:50, mount-lustre-fsx.sh:44.

The PCS guide puts the retry responsibility in the script:

Scripts are not retried. If an operation might experience transient failures, add
retry logic inside the script.
— Error handling

And this PR's own tests/storage-test.md:40 names the transient case:

The most common first-boot failure is the OpenZFS DNS name not being resolvable yet
(NFS settle race); the mount log will show mount.nfs: Failed to resolve server.

I reproduced that exact signature — it fails instantly, with no cushion anywhere in
the stack:

$ mount -a -t nfs defaults        # fstab -> fs-0nx.fsx.us-east-2.amazonaws.invalid
mount.nfs: Failed to resolve server fs-0nx...invalid: Name or service not known
EXIT=32  ELAPSED=0s

(To be fair to the NFS path: a name that resolves to an unreachable server does get
mount.nfs's own foreground retry — ~2 min in my test — before exit 32. The DNS case has
none, and the Lustre mount is single-shot in both cases.)

Impact. Under the old UserData that race produced a node without /home, which is
the thing this PR is right to fix. Under TERMINATE it produces a terminated node, and
the replacement launches into the same window. What stands out is that it inverts the
retry discipline in the rest of the PR: install-monitoring.sh:50-58 retries 3x,
install-monitoring.sh:32 passes curl --retry 3, setup-directory.sh:297 retries
discovery 30x — all three are OnError: CONTINUE, where a failure is survivable. The
only two scripts whose failure is fatal are the only two with no retry.

On a Capacity Block GPU node group this isn't a free retry: CB capacity is time-boxed,
and an FSx-side blip during scale-up becomes correlated churn across the group.

I'd suggest a bounded retry around the mount in both scripts so TERMINATE is
reserved for a persistent failure. STOP_SEQUENCE is also worth knowing about — it
stops the stage but leaves the instance up, which is the better setting while
iterating on a new mount.

[KeitaW]

The /home mount now hard-depends on an S3 fetch, and a failed fetch terminates the node

Under the removed UserData the /home mount was inline: fstab line plus mount -a, no
S3 object involved. S3 was only in the path for the post-install and directory scripts,
and both were log-and-continue, so a fetch failure left a working node.

add-cng.yaml:364 now fetches mount-openzfs-home.sh from
s3://${S3BucketName}/${S3KeyPrefix}scripts/ on every node, unconditionally, and
:368 is OnError: TERMINATE. A node that cannot retrieve that object is terminated and
replaced under the same configuration — a replace loop rather than a degraded node.

Much of this is covered and worth saying so: the instance roles carry
AmazonS3ReadOnlyAccess (cluster.yaml:161, :252), the agent retries the download 3x
with backoff, CACHE_ONCE means reboots never refetch, and the new manifest check in
lint-docs.sh closes the "script never got published" hole. What is left is the
environments those don't reach:

• CreateS3Endpoint gates the gateway endpoint (ml-cluster-prerequisites.yaml:249 →
  :464). Set it false and private-subnet nodes fall back to NAT for a fetch that now
  terminates them.
• A standalone add-cng*.yaml user supplying their own instance profile without
  s3:GetObject is in the same position, and the template gives no hint that the
  permission is now load-bearing.
• S3BucketName=local — the UseLocalTemplates sentinel at
  pcs-ml-cluster-deploy-all.yaml:638 — selects relative TemplateURLs for the nested
  stacks but is still forwarded verbatim as the script bucket to all five CNGs (:789,
  :837, :881, :925, :969). Every ScriptLocation then resolves to
  s3://local/..., which satisfies the schema's bucket-name pattern and fails at
  download. That path previously just skipped the post-install and booted fine; now it
  cannot produce a login node at all. The sentinel is undocumented, so this is minor on
  its own — but it is the clearest illustration of the escalation.

The sharpest case is the upgrade path itself. This PR adds four scripts that did not
exist before — needrestart-guard.sh, mount-openzfs-home.sh, mount-lustre-fsx.sh,
install-monitoring.sh. Any cluster deployed against a non-default S3BucketName /
S3KeyPrefix — a supported, documented path (PARAMETERS.md §7, the whole
DEPLOY-TESTING.md flow, and the private-bucket story this PR advertises) — has none of
them in its bucket. Update such a stack to this revision and, because the update itself
drains and replaces every node (see the §8 finding below), each replacement fails
mount-openzfs-home and is terminated. Unbounded replace loop, empty cluster.

The PR already knows this failure mode — tests/lint-docs.sh:87-94 spells it out
verbatim — but that guards this repo's manifest, not the operator's bucket, and §8
never mentions the sync as a prerequisite. DEPLOY-TESTING.md:46 presents it as a
testing step and §8 doesn't link there.

A §8 row saying "sync the new scripts to your bucket before updating the stack",
plus a line in DEPLOY-TESTING and in the S3BucketName / S3KeyPrefix rows of
PARAMETERS.md:117-118 (which still describe them as only "the S3 bucket the nested
templates are fetched from"), would cover this. The local sentinel is worth either
documenting or gating with a Rules assertion.

[KeitaW]

Review Batch 2/6 — Mount-script correctness and data integrity

These are all in the newly extracted scripts, and all but the first are new-in-PR
consequences of EVERY_BOOT or of the stash/restore block.

[KeitaW]

needrestart-guard.sh discards Ubuntu's 43 shipped restart exclusions, including ^docker

needrestart-guard.sh:24 assigns a whole new hashref rather than adding a key:

$nrconf{override_rc} = { qr(^slurmd) => 0 };

Ubuntu 24.04 ships an active, uncommented $nrconf{override_rc} in
/etc/needrestart/needrestart.conf and then sources conf.d/*.conf with eval, so the
drop-in replaces the shipped list wholesale. Measured on the target platform
(Ubuntu 24.04, needrestart 3.6-7ubuntu4.5):

baseline patterns (no drop-in):            43
AFTER this drop-in (whole-hash assign):     1
AFTER the one-line fix (key assign):       44
discarded: ^docker  ^network  ^getty@  ^user@\d+\.service  ^serial-getty@

Impact, specifically for this cluster. ^docker goes back to being a restart
candidate, and this same PR installs the Prometheus/Grafana/dcgm-exporter stack in
Docker on the login node — so the next unattended libc/openssl upgrade can bounce
docker.service and tear down the monitoring the PR just added. Losing ^dbus and
^user@N.service means the same upgrade can drop interactive sessions. A guard written
to stop upgrade-driven service disruption currently widens it.

To be clear about provenance: the wording is carried over verbatim from the old UserData,
so this is a pre-existing bug, not a regression from this PR. But the PR is what lifts it
into a standalone, reviewable file — which makes this the natural moment, and the fix is
one character class of change:

Suggested change

	$nrconf{override_rc} = { qr(^slurmd) => 0 };
	$nrconf{override_rc}{qr(^slurmd)} = 0;

[KeitaW]

The fstab guard matches commented-out lines, and nothing asserts the mount actually took

mount-openzfs-home.sh:41 and :49 use grep -qF "$FSTAB_LINE" /etc/fstab. -F is a
fixed-string substring match, not a line match:

$ printf '# %s\n' "$L" > fstab; grep -qF "$L" fstab && echo MATCHED
MATCHED
$ grep -qxF "$L" fstab || echo "correctly did not match"
correctly did not match

Scenario: an operator comments out the /home line to debug a hung mount, then reboots.
The guard finds its own string inside the comment, never re-adds an active entry,
mount -a -t nfs succeeds with nothing to do, and :57 prints /home mounted from ...
while /home is still local disk.

That is worse than it sounds because nothing between mount -a (:50) and the restore
rsync (:54) checks the result. The stash gets restored onto local disk, :55 deletes it,
and the node runs with a local /home that the next boot silently shadows — everything
written in between disappears. Two small changes close it:

Suggested change

	grep -qF "$FSTAB_LINE" /etc/fstab || echo "$FSTAB_LINE" >> /etc/fstab
	grep -qxF "$FSTAB_LINE" /etc/fstab || echo "$FSTAB_LINE" >> /etc/fstab

and a mountpoint -q /home || exit 1 immediately after mount -a at :50, so a mount
that didn't take terminates the node instead of quietly corrupting it.

[KeitaW]

rsync --ignore-existing still overwrites existing shared directory modes

The header at :10-13 says the restore uses --ignore-existing "so any pre-existing
shared state wins". That holds for files but not for directories — rsync still applies
attributes to existing directories:

BEFORE: src/alice=755  dst/alice=700
AFTER : dst/alice=755          # existing shared dir, mode overwritten
        collide=SHARED         # existing file correctly left alone

Scenario: a user's shared /home/alice is 0700. A node takes the slow path (mount not
yet up when the action runs) with a local /home/alice at 0755 — which is what
pam_mkhomedir creates, and setup-directory.sh:361 installs it with umask=0022. The
restore rsync then opens /home/alice to 0755 on the shared filesystem for the whole
cluster. On a multi-user cluster — which this PR's directory support exists to enable —
that is a quiet permissions downgrade of every home directory a slow-path node touches.

--no-perms --omit-dir-times on the restore, or restoring only paths that don't exist at
all, keeps the stated invariant.

[KeitaW]

A /tmp/home stash orphaned by an unclean reboot is merged into shared /home

:47 uses a fixed /tmp/home and mkdir -p, which succeeds on an existing directory
whatever its contents or owner. Two consequences, both requiring only conditions that
actually occur here:

Stale stash. /tmp on Ubuntu 24.04 is disk-backed, not tmpfs, so it survives a
reboot; and OnError: TERMINATE fires only on a non-zero exit, never on a panic or an EC2
scheduled reboot. If a node dies between :48 and :55, /tmp/home persists. On a later
boot that takes the slow path, mkdir -p doesn't clear it, :48 merges into it, and
:54 pushes the old snapshot into the shared filesystem. The sharp version: an admin
deletes a shared /home/ubuntu/.ssh/authorized_keys to revoke access, and this path
restores it.

Predictable path in a world-writable directory. Any user on the login node can
pre-create /tmp/home/<name>/.... Root's :54 then copies it into shared /home where
the path doesn't yet exist, and :48 first hands them a readable copy of local /home.
rsync -a preserves their ownership so this isn't privilege escalation, but it does let
any user plant content at shared paths — including under /home/ldap-db/.

STASH=$(mktemp -d) with a trap for cleanup fixes both.

[KeitaW]

chmod 1777 /fsx runs on every boot against shared storage, and gates the exit code

mount-lustre-fsx.sh:45. Because this script deliberately writes no fstab entry,
mountpoint -q /fsx at :39 is always false after a reboot, so :44-45 always run. Under
the old UserData this was once per instance; EVERY_BOOT is new here.

/fsx is shared storage, so the chmod isn't node-local. An admin who tightens the Lustre
root — chmod 0770 /fsx; chown root:clusterusers /fsx to stop cross-tenant writes — has it
silently reset to 1777 cluster-wide by the next node that reboots.

Second issue on the same line: under set -e a failing chmod exits non-zero even
though the mount at :44 succeeded, so OnError: TERMINATE destroys a node whose /fsx
is fine. Worth doing the chmod only when the script created the mount, and not letting it
gate the exit status.

[KeitaW]

Review Batch 3/6 — Migration completeness (docs the new lint can't see)

[KeitaW]

Three docs still describe the removed empty/single-space parameter semantics

InstallEnrootPyxis carries AllowedValues: ['true', 'false']
(pcs-ml-cluster-deploy-all.yaml:367-370), so neither an empty string nor a single
space is a legal value any more. Three passages still describe both:

• docs/OPERATIONS.md:55-57 — "Leaving it at the default — empty, which auto-installs
  from the templates bucket — ... the single space just avoids the download+check."
• docs/CUSTOM-AMI.md:58 — "Passing a single space skips the download+check entirely,
  shaving a few seconds off boot." This one is three lines below a line the PR updated,
  and it's the most costly: a reader who follows it gets a create-stack rejected on
  AllowedValues, not a faster boot.
• docs/PARAMETERS.md:87-90 — heading still reads "5.3. Additional Cluster
  Configuration: Post-Install Script" with "point it at your own script to customize",
  while the console group is now "Container Runtime (Enroot/Pyxis)" and the PR
  deliberately removed the ability to point it anywhere. That file's own preamble says
  the sections "match the CloudFormation console's parameter groups exactly", so it's
  contradicting its own stated invariant.

docs/OPERATIONS.md:55 also says "the default", which is the one place ambiguity
actually bites: the default is true in pcs-ml-cluster-deploy-all.yaml:367 and
false in the four add-cng*.yaml. Worth naming the template.

README.md:150 repeats the same stale console label, so both docs that promise to mirror
the console now disagree with it.

Worth naming why this slipped: the deleted check 2 (old lint-docs.sh:52-60) was the
only check that policed the value semantics rather than the parameter name. The new
banned-pattern in tests/lint-docs.sh:39 matches PostInstallScript(Url|Args),
and none of these three passages names the parameter — the guard is silent on all of
them (grep -rnE 'PostInstallScript(Url|Args)' docs/ README.md tests/*.md returns only
the OPERATIONS.md §8 migration row, which is correct usage). A second pattern on the
value semantics would close it, e.g. a banned single space / empty → auto in the
Enroot/Pyxis context.

The whole parenthetical at :55-57 needs rewriting rather than patching — it describes
two values that no longer exist. Something like: "Leaving it at the deploy-all default of
true also works on a pre-baked AMI — the installer detects Enroot/Pyxis is already
present and is a fast no-op; false skips the download and check entirely." Note that
add-cng*.yaml default to false, so "the default" needs the template named.

[KeitaW]

README says Enroot/Pyxis is "on by default", but its own Launch Stack buttons deploy the templates where it isn't

README.md:30 ("InstallEnrootPyxis, on by default") and README.md:151 (default
true) are both unscoped, and docs/PARAMETERS.md:94 documents the default as true.
That's correct for pcs-ml-cluster-deploy-all.yaml:367 — but all four standalone
templates default to false (add-cng.yaml:203, add-cng-p5.yaml:213,
add-cng-p6-b200.yaml:204, add-cng-p6-b300.yaml:207), and README.md:695-702 hands
out one-click Launch Stack buttons for exactly those four.

So the documented path for adding a GPU queue to an existing cluster produces nodes with
no container runtime, while the README two paragraphs up says it's on. First symptom is
srun --container-image=... failing with a Pyxis error on the new queue only.

To be clear, the false default is right — the old PostInstallScriptUrl default in
those templates was '' = skip, so this preserves prior behavior, and the second pass and
I independently agreed on that. The gap is purely documentary: the defaults now differ by
template and no doc says which is which. Scoping the two README claims to deploy-all (and
adding a §8 row) closes it.

[KeitaW]

setup-directory.sh and install-enroot-pyxis.sh still credit UserData for what the PR moved

The PR updated both script headers for the new positional interface, but five in-file
comments still describe the old one. One of them the PR's own change made false:

• setup-directory.sh:30 — "LDAP_ADMIN_PASSWORD — auto-generated by UserData (server
  role only)". As of setup-directory.sh:117 the script generates it itself; the
  comment at :121 was updated to say so, :30 was not.
• setup-directory.sh:27 — "Environment variables (from UserData):"
• setup-directory.sh:128 — "keeps the UserData-generated random password"
• setup-directory.sh:234 — "S3_BUCKET/S3_KEY_PREFIX passed by UserData"
• install-enroot-pyxis.sh:223 — "the UserData from the template's SlurmVersion",
  where :63 now takes it as $1

These are the file's own contract documentation for the interface this PR changed, so
they're worth the sweep — grep -n 'UserData' assets/scripts/*.sh finds all five.

[KeitaW]

Review Batch 4/6 — Guard coverage & hardening

Root-executed scripts from a public, mutable, unpinned S3 object

publish-architecture-templates.yml syncs with --acl public-read, and the templates
default S3BucketName: awsome-distributed-ai — a bucket the deploying customer doesn't
control. Those objects now run as root on every node at first boot, with no Checksum
and no S3VersionId.

Both fields exist on ScriptSource (confirmed against the live resource schema via
aws cloudformation describe-type --type-name AWS::PCS::ComputeNodeGroup), and the
guide recommends them:

Use checksums in production, especially for scripts from shared repositories.
— Best practices

The exposure predates this PR for install-enroot-pyxis.sh and setup-directory.sh,
so this isn't a regression — but the PR widens it from "optional scripts on some nodes"
to "every node, unconditionally", and with the mounts on TERMINATE a bad object also
becomes a fleet-wide node kill rather than a fleet-wide degradation.

I don't think a hardcoded Checksum in the template is right — it would need
regenerating on every script edit, and changing it triggers DRAIN. S3VersionId, or
publishing .sha256 companions the way the AWS-maintained script library does, are the
lighter options. Mostly I'd like this to be a recorded decision rather than an omission.

A sharper, cheaper instance of the same class. MonitoringRepo (add-cng.yaml:165)
and MonitoringVersion (:155) are plain Type: String with no AllowedPattern, and
install-monitoring.sh:32-34 splices them straight into
https://raw.githubusercontent.com/${MONITORING_REPO}/${MONITORING_VERSION}/post-install.sh,
then runs the result as root at :53. A ref containing ../ redirects the fetch
entirely — this resolves, it isn't theoretical:

.../aws-parallelcluster-monitoring/main/../../../torvalds/linux/master/README
  -> http=200  final=https://raw.githubusercontent.com/torvalds/linux/master/README

So a mistyped or attacker-supplied MonitoringVersion fetches and root-executes an
arbitrary repository's file. Two AllowedPatterns close it —
'^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$' on the repo and '^[A-Za-z0-9._-]+$' on the ref —
and they cost nothing. (Also worth noting v2.10.2 is a mutable git tag, and the
fetched script pulls a tarball and runs it as root with no checksum, so the pin is
weaker than it looks.)

The Grafana admin password is traced into a log file on the login node

install-monitoring.sh:53 runs the fetched entrypoint, and upstream at
post-install.sh@v2.10.2:85 does:

bash -x "${MONITORING_HOME}/installer/install.sh" 2>&1 | tee "${LOG_FILE}"
# LOG_FILE=/var/log/parallelcluster-monitoring-install.log

while installer/install.sh@v2.10.2:204-208 generates
GRAFANA_PASSWORD=$(openssl rand -hex 16) and passes it to aws ssm put-parameter --value "${GRAFANA_PASSWORD}". Under -x bash traces both the assignment and the
argument, so the plaintext reaches that log (and the lifecycle log) before the unset at
:211. I verified the upstream lines at the pinned tag; I have not measured the log's
mode on a live node, but nothing in either script tightens it from the default umask.

This is upstream behavior, not something the PR introduces — but the PR is what puts an
LDAP-multi-user login node and this installer on the same host, which is what makes the
log's readability matter. A chmod 600 on that path after the install, plus an upstream
issue, would cover it.

setup-directory.sh can time out initialising LDAP and still report success

setup-directory.sh:191-194 runs a bounded readiness loop for slapd but never checks
whether it actually succeeded, and the three base-entry operations at :197-213 suppress
every error with 2>/dev/null || true rather than just "already exists". If slapd
starts but never becomes bind-ready, the loop expires, all three ldapadds fail silently,
and the script still prints "OpenLDAP server ready" and exits 0 — so the FIRST_BOOT_ONLY
action is recorded successful and never retried, with no ou=People, ou=Groups or
cn=clusterusers in the directory.

This is the same shape as the false-success bug the PR fixes in needrestart-guard.sh
by adding set -euo pipefail — worth applying the same instinct here: fail the loop
explicitly, and distinguish "entry exists" from "server unreachable".

FIRST_BOOT_ONLY + CONTINUE failures are permanent and near-silent

setup-directory.sh:311 returns 1 after ~5 minutes of failed discovery, :57-60 exits 1
on the /home gate, and install-enroot-pyxis.sh:119,140,152,194 have no network retry
at all — one transient 503 exits under set -e. In each case OnError: CONTINUE plus
FIRST_BOOT_ONLY means the node joins the cluster degraded (no SSSD, or no container
runtime so every --container-image job fails) and is never retried. The only signal is a
line in a per-script file on an instance nobody is looking at.

A logger -t pcs-<name> alongside the existing echo would put these in syslog, where
the CloudWatch agent or an existing log pipeline can see them — cheap, and it turns a
silent permanent degradation into something detectable.

[KeitaW]

The new manifest cross-check misses scripts fetched by other scripts

The manifest check greps assets/add-cng*.yaml only. That covers every
ScriptLocation today, but ldap-add-user.sh is also fetched from the same bucket at
boot — by setup-directory.sh:246, not by a template — so it's invisible to the guard
even though it's the same "missing from the manifest → never reaches the production
bucket" failure the check exists to prevent. (The README at :753 files it under
"Helper scripts (NOT run at boot)", which is true of executing it and is probably why
it's easy to miss that it's fetched at boot.)

What makes this worth fixing rather than noting: neither gate catches it. Dropping
its manifest entry leaves lint-docs.sh reporting PASS and
stage-template-publish.py exiting 0 while silently staging 6 scripts instead of 7 — so
the object simply never reaches the bucket, setup-directory.sh:246 fails its
aws s3 cp, and the cluster comes up without the ldap-add-user.sh helper that
USER-MANAGEMENT.md documents as the way to add users. It degrades rather than loops
(the fetch is guarded and logs a WARNING at :251), but silently.

By contrast a broken manifest entry — right key:, wrong source: — is caught hard by
the staging script (ERROR: ... references 'scripts/mount-openzfs-home.sh', but no manifest entry publishes that key, on all four templates), so that path is already
well covered.

Widening the glob catches the gap and leaves the real tree clean:

FAIL: assets/scripts/ldap-add-user.sh is referenced by a template but missing from
      .github/template-publish-manifest.yml (would not be published ...)     exit=1
# restored manifest -> docs lint: PASS                                       exit=0

Suggested change

	for scr in $(grep -hoE 'scripts/[a-z0-9-]+\.sh' assets/add-cng*.yaml | sort -u); do
	for scr in $(grep -hoE 'scripts/[a-z0-9-]+\.sh' assets/*.yaml assets/scripts/*.sh | sort -u); do

[KeitaW]

Updating an existing stack drains and replaces every node — worth saying in §8

§8 correctly says existing stacks keep working as-is, then walks through what to change
when updating. What it doesn't say is that the update itself replaces the fleet:
CustomLaunchTemplate.Version tracks !GetAtt PCSLaunchTemplate.LatestVersionNumber
(add-cng.yaml:327), removing UserData bumps the launch template version, and adding
NodeLifecycleActions is itself a CNG config change.

Changing it through UpdateComputeNodeGroup affects only new instances and triggers the
DRAIN strategy so running jobs finish before nodes are replaced.
— Script caching and updates

DRAIN is the safe semantic and no jobs are lost, but an operator updating a cluster with
a multi-day run in flight should know the whole fleet cycles. §8 currently says the
reassuring-sounding opposite — "Existing deployed stacks keep working as-is" — which is
true of not updating and is easy to read as true of updating.

The same row also gives debugging advice that doesn't hold in the case it describes:
OPERATIONS.md:520 says to "check the mount script's lifecycle log on a surviving node",
but a fleet-wide mount failure leaves no surviving node, and lifecycle logs are local to
the instance. The guide's own recourse for that is onError: STOP_SEQUENCE to keep an
instance for inspection, or the AWS-maintained configure-cloudwatch-logs.sh action to
ship the log directory off-instance — neither is mentioned.

[KeitaW]

Review Batch 5/6 — Stage placement, robustness & parameter validation

install-monitoring at nodeReady runs on compute nodes — and its own header says otherwise

install-monitoring.sh:5-6 — a file this PR adds — opens with "install the
Prometheus/Grafana monitoring stack on the login node". But DeployMonitoring is
passed "true" to every node group in deploy-all (lines 710/780/823/872/916/960), and
MonitoringEnabled (add-cng.yaml:298) keys off DeployMonitoring alone, not
MonitoringRole — so the action is created on the four compute CNGs too, each with
MonitoringRole: compute (:828, :877, :921, :965). The README's own boot diagram
(:733) lists it under the compute CNG, so the wiring is deliberate and matches the old
UserData; it's the new header that describes a narrower deployment than the templates
create. Worth reconciling, because the stage change below only bites on compute nodes —
the ones the header says aren't in scope.

The guide flags the consequence of the stage:

nodeReady scripts run after slurmd starts, so there is a brief window in which the
scheduler might dispatch a job to the node before your nodeReady scripts finish.
— Lifecycle stages

Previously this block lived in a text/cloud-config UserData part, and per Working
with EC2 user data
those run before the instance registers with the PCS API — so the node genuinely
didn't accept jobs until the install finished. The template comment frames the move as
avoiding a multi-minute delay to node availability, which is a fair trade; the other
half (apt + Docker install running alongside the first job on a fresh node) just isn't
written down anywhere. A line in that comment, or nodeBootstrapped for the compute
role, would settle it.

An empty FSxOpenZFSFilesystemId is a genuine, unbreakable replace loop

add-cng.yaml:143 declares FSxOpenZFSFilesystemId as a bare Type: String — no
Default, no AllowedPattern, no MinLength — so an empty value is accepted at stack
creation. The agent then passes "" as $1, ${1:?} at mount-openzfs-home.sh:20 fires
(:? treats null the same as unset), and OnError: TERMINATE replaces the node. Unlike
the other terminate paths, the replacement receives the same empty argument, so this one
genuinely loops until someone deletes the stack.

The value is also written straight into /etc/fstab at :36 with no validation. An
AllowedPattern: '^fs-[0-9a-f]{8,17}$' on the parameter — the same shape AmiId already
uses in this template — turns a replace loop into a parameter-validation error.

[KeitaW]

MonitoringVersion=latest is advertised but cannot work

add-cng.yaml:155-160 offers 'latest' as a supported MonitoringVersion ("release tag,
branch, or latest"), but install-monitoring.sh:32-34 has to fetch post-install.sh at
that ref before anything upstream could resolve it, and the raw URL 404s:

v2.10.2   http=200
latest    http=404

The upstream repo has neither a heads/latest nor a tags/latest ref. With
OnError: CONTINUE + FIRST_BOOT_ONLY the node comes up with monitoring silently absent
and never retried. Either drop latest from the description or resolve it to a concrete
tag before the fetch.

[KeitaW]

The self-generated LDAP admin password can silently rotate on a login-node replacement

The old contract was ${LDAP_ADMIN_PASSWORD:?LDAP_ADMIN_PASSWORD must be set} — the caller
had to supply it. The positional interface has no slot for it, so :117 now falls back to
$(openssl rand -base64 16). Generating it in-script is the right call (arguments are
visible in API responses, and the guide says not to put secrets there). The gap is what
happens next.

:131-133 reads the existing value with aws ssm get-parameter ... 2>/dev/null || echo "",
which collapses every failure into "nothing stored yet" — ParameterNotFound, a missing
ssm:GetParameter grant, and throttling are indistinguishable. A transient read failure
during a login-node replacement therefore generates a fresh password, reconfigures slapd,
and overwrites SSM with --overwrite at :219-223, while the user DB on /home/ldap-db
persists — the documented scenario the comment at :121-127 exists to prevent.

Related, same block: the debconf answers at :144-154 only take effect if
apt-get install slapd at :155 actually installs. On a node where slapd is already
present, olcRootPW keeps its old value while the new password is still written to SSM —
so the stored credential doesn't bind. An ldapwhoami -x -D "cn=admin,${LDAP_DOMAIN_SUFFIX}" -w "$LDAP_ADMIN_PASSWORD" after :213, failing loudly, would catch both.

[KeitaW]

Review Batch 6/6 — Nits, pre-existing appendix, positives & sources

Nits

• mount-openzfs-home.sh:20, mount-lustre-fsx.sh:20-21: under set -u, ${1:?msg}
  never prints msg — bash reports $1: unbound variable first (verified: bash mount-openzfs-home.sh with no args → line 20: $1: unbound variable). The
  operator-facing text never reaches the lifecycle log. [ $# -ge 1 ] || { echo ...; exit 1; } would land it.

• install-monitoring.sh:35 says "after 3 attempts", but curl --retry doesn't retry
  4xx — a 404 exits after one (verified against a nonexistent repo). It's the message
  someone reads while debugging.

• mount-openzfs-home.sh:50: the trailing defaults in mount -a -t nfs defaults is a
  stray positional that util-linux silently ignores (checked — BOGUSWORD in its place
  behaves identically). Carried verbatim from the old UserData so nothing changed, but
  now that the line lives in a standalone reviewed script it's a good moment to drop it.

• mount-lustre-fsx.sh deliberately writes no fstab entry while mount-openzfs-home.sh
  does — both correct given EVERY_BOOT, but it's the kind of asymmetry a future reader
  "fixes". One line of comment would protect it.

• docs/PARAMETERS.md:35 says SlurmVersion is "passed to the post-install lifecycle
  action". No action has that name — add-cng.yaml:211 (updated by this PR) calls it
  the install-enroot-pyxis lifecycle action. Last surviving "post-install" in a
  parameter description.

• docs/DEPLOY-TESTING.md:46 — "Re-run this sync after every change you want to test" —
  is now incomplete in a way that costs a debugging cycle. With
  ScriptCachingPolicy: CACHE_ONCE (add-cng.yaml:354) the agent downloads each script
  once at first boot and never refetches, so a re-sync reaches no running node,
  including across a reboot for the EVERY_BOOT mount actions. The guide's
  troubleshooting table names this exactly ("Updated script not applied — CACHE_ONCE
  keeps the original copy"). Worth adding "replace the instance to pick up a changed
  script" — it's the difference between a 30-second retest and an hour of confusion.

• install-monitoring.sh:34 writes to a fixed /tmp/post-install.sh. nodeReady runs
  after the node is job-eligible (the template says so at add-cng.yaml:418-420), so a
  dispatched job can create that path as a directory first; curl then fails with exit 23,
  and OnError: CONTINUE + FIRST_BOOT_ONLY means monitoring is skipped permanently on
  that node. A root-owned mktemp -d removes the race.

• mount-openzfs-home.sh:48,54: rsync -A requests POSIX ACLs, but the destination is FSx
  OpenZFS over NFSv3 (nfsvers=3, :36), which has no ACL wire protocol — any file with a
  non-trivial ACL yields rsync exit 23, and :48 yields 24 if a file vanishes mid-copy
  (a cloud-init temp file, logrotate). Under set -e + TERMINATE either destroys the
  instance for a non-fatal condition. Worth accepting 23/24 explicitly.

• mount-openzfs-home.sh:50: mount -a -t nfs mounts every NFS entry in fstab, so on a
  custom AMI that also carries, say, /archive on NFS, /home can mount fine while
  /archive fails — mount -a returns non-zero and the node is terminated. Mounting
  /home specifically would scope the blast radius to the filesystem the action is about.

• mount-openzfs-home.sh:36,49: the fstab guard matches the exact line, which embeds
  both FS_ID and the IMDS-derived REGION. A stack update that changes
  FSxOpenZFSFilesystemId therefore appends a second /home entry rather than
  replacing the first; mount -a tries the deleted filesystem, fails, and the node is
  terminated. Bounded (replacements get a clean fstab) so it's a one-off replacement wave,
  not a loop — but surprising for a parameter edit. Matching on [[:space:]]/home[[:space:]]
  and rewriting would be safer.

• mount-openzfs-home.sh:39, mount-lustre-fsx.sh:39, setup-directory.sh:57:
  mountpoint -q says something is mounted, not what, and not that it's alive. A stale
  fstab entry on a custom AMI could mount another cluster's storage and both actions would
  report success; an evicted FSx server leaves mountpoint happy while slurmd starts on a
  node that hangs on first access. findmnt -no SOURCE /home compared against the expected
  DNS name, plus a timeout 10 stat /home, would make the early return trustworthy.

• setup-directory.sh:45: [ -n "${5:-}" ] && S3_KEY_PREFIX="$5" conflates "empty" with
  "not supplied", so an intentional S3KeyPrefix='' (scripts at the bucket root) falls back
  to templates/aws-pcs/ at :246 and the ldap-add-user.sh fetch looks in the wrong
  place — warning only, helper silently absent. The template's own !Sub handles empty
  correctly, so the script is the odd one out.

• tests/lint-docs.sh:66-70: the comment justifies the sed-strip with "the four
  templates legitimately nest the block at different depths", but the new awk anchors on
  exactly six spaces and the four raw blocks are byte-identical before stripping (same
  md5). The justification is inherited from the deleted guard_extract. Dropping the
  sed would make the check stronger — it would then also catch the relative-indentation
  drift the comment flags as a blind spot:

  drop the trailing | sed -E 's/^[[:space:]]+//' from the pipeline so the comparison
  sees the raw block.

Appendix — pre-existing, not introduced here

• README.md:560 and docs/PARAMETERS.md:78 link
  OPERATIONS.md#31-dcgmexporterimage-the-default-and-when-to-change-it, but the heading
  is ### 3.1 \DcgmExporterImage` — the default, and when to change it, which GitHub slugs with a double hyphen (...dcgmexporterimage--the-default...). Both links open the file without navigating. lint-docs.sh` checks README-internal anchors only, so
  cross-file anchors aren't covered.
• setup-directory.sh:242: the bucket-region curl -sI has no --connect-timeout /
  --max-time. It was outside the registration window under cloud-init; in
  nodeBootstrapped it now sits inside the ~30-minute budget alongside the 30x10s
  discovery loop at :297.
• setup-directory.sh:190: for i in $(seq 1 10); do ldapsearch ... && break; sleep 1; done has no post-loop failure branch, so an slapd that never comes up falls through
  silently.
• slapd.service on Ubuntu is After=slapd-sockets.target network.target with no
  RequiresMountsFor=/home. The PR's new mountpoint -q /home gate protects the script's
  own first-boot run, but on a reboot slapd can still win the race against the fstab NFS
  mount, create its MDB in the local empty /home/ldap-db, and then have it shadowed —
  login node up with an empty directory. A drop-in RequiresMountsFor=/home would close
  the reboot half of the same hazard the PR already reasons about.
• mount-openzfs-home.sh:54: --ignore-existing makes the shared copy authoritative
  permanently, so per-node state can never be refreshed. Rotate the EC2 key pair and
  replace the login node, and the new node's local authorized_keys is skipped in favour
  of the first node's — SSH access silently pinned to the old key. The header states the
  "shared wins" rule without noting this half of it.
• install-enroot-pyxis.sh:149-150 chmods /tmp/enroot — the download directory and cwd
  for apt_get install -y ./*.deb at :142 — to 1777. The data/cache children need
  it; the parent doesn't, and a world-writable .deb glob installed as root is arbitrary
  root code on any re-run. 0755 on the parent.
• install-enroot-pyxis.sh:152 fetches enroot.template.conf from an unpinned main
  branch into /etc/enroot/enroot.conf. CONTRIBUTING requires pinning to a tag or commit.
• install-monitoring.sh:44: the comment describes the apt drop-in as applying "to the
  caller and its children", but it's a persistent node-wide file — every future apt-get,
  including unattended-upgrades, inherits the 300s lock timeout for the instance's life.

Things That Look Great

• The extraction fixed a real false-success bug on the way past. The old inline
  needrestart block had no set -e and no &&, so echo "...excluded..." | tee printed
  success even when the mkdir/cat failed. needrestart-guard.sh:17 adds
  set -euo pipefail, and the two forms now diverge exactly as they should on a
  read-only /etc:

  old inline form: "needrestart: slurmd excluded from auto-restart"  EXIT=0  (file absent)
  needrestart-guard.sh: mkdir: cannot create directory ... Read-only  EXIT=1  (no false claim)
  

• Every ordering and policy claim in the description holds against the API contract.
  I checked the property names, casing and enums against the live resource schema
  (describe-type on AWS::PCS::ComputeNodeGroup) and the semantics against the user
  guide: nodeBootstrapped really does run before slurmd, EVERY_BOOT really does
  re-run on reboot, the log paths and the 3-attempt download retry are as described, and
  CACHE_ONCE means a reboot needs no S3 at all. cfn-lint and validate-template pass
  on all five changed templates.

• The mounts' idempotency is real, not asserted. First boot, reboot with /home
  already mounted, and reboot with the fstab entry present but unmounted all exit 0, the
  fstab keeps exactly one entry throughout, and --ignore-existing gives the shared copy
  precedence while restoring local-only files with ownership intact (700 ubuntu:ubuntu
  preserved, /tmp/home cleaned up).

• The /home mountpoint guard closes a genuine silent-corruption hole —
  setup-directory.sh:52-59 stops an LDAP DB being built on local disk and then shadowed
  by the NFS mount. Both branches behave correctly.

• Arguments carry no secrets. The LDAP admin password is generated in-script
  (:117) rather than passed as a lifecycle argument, which matches the guide's warning
  that arguments are visible in API responses and the console. Easy to get wrong when
  converting an env-var interface to positional args.

• The cross-region S3 fix survives exactly where it's still needed. The --region
  resolution is correctly dropped where the agent now owns the download, and kept at
  setup-directory.sh:242 for the one aws s3 cp still on the boot path. I checked the
  whole assets/scripts/ tree for other S3 fetches — there are none.

• The byte-identity lint carries forward its own known limitation. The new
  lifecycle_extract keeps the note that whitespace-stripping makes it blind to relative
  indentation drift, rather than just copying the technique — and it's more robust than
  what it replaces, since anchoring on the property name means a template that loses the
  block fails loudly instead of comparing equal. I mutation-tested all three new checks
  (drop from manifest / typo a ScriptLocation / drift one template's OnError); each
  fails correctly and passes again on restore.

• The publish gate is stronger than it looks. Beyond the new lint check,
  stage-template-publish.py independently refuses to stage when a template references a
  script no manifest entry publishes — I broke a manifest source: while leaving its
  key: intact and it failed hard with an ERROR for each of the four templates. So the
  "typo'd manifest entry" path is already well covered; the ldap-add-user.sh gap above
  is the one case that slips both.

• The size is justified. 23 files and ~1800 lines is past where I'd normally ask for
  a split, but 889 of those are the four UserData blocks coming out, and splitting would
  leave templates pointing at unpublished scripts. Landing it atomically is right.

Sources

AWS PCS node lifecycle actions (all verified live, 2026-08-23):

• Node lifecycle actions — stage definitions, the nodeReady job-dispatch window
• Configure lifecycle actions — agent >= 1.5.0-1, "Scripts are not retried", download retry, DRAIN on update, log paths
• Best practices — checksums in production, forward logs to CloudWatch, no secrets in arguments
• AWS-maintained scripts — mount-fsx-lustre.sh, configure-cloudwatch-logs.sh, .sha256 companions
• Working with EC2 user data — cloud-config runs before PCS registration

Live verification, 2026-08-23:

• aws cloudformation describe-type --type RESOURCE --type-name AWS::PCS::ComputeNodeGroup → NodeLifecycleActions present; ScriptSource exposes Checksum + S3VersionId; Arguments items have no minLength
• cfn-lint 1.55.1 → exit 0 on all four add-cng*.yaml; aws cloudformation validate-template → VALID on all 10 staged templates
• bash tests/lint-docs.sh → PASS; three mutation tests each fail correctly
• Container runs (Ubuntu 24.04): IMDS guard unreachable; mount.nfs DNS failure EXIT=32 ELAPSED=0s; mount-script first-boot/reboot/remount idempotency; needrestart-guard.sh vs the old inline form on read-only /etc

[DaisukeMiyamoto]

Thank you for the depth here — the live schema checks and the container repros made these easy to act on. Responses grouped under your batches; everything below is applied and re-verified end-to-end on a fresh deploy — /home and /fsx mounts on login and compute, exactly one active /home fstab entry (which survives a login-node replacement), monitoring up on both roles, and a container job returning 0 — unless marked as a deliberate decision.

Batch 1 & 2 — boot-path failure modes / mount correctness

• IMDS region guard not firing on a connection failure / passing a garbage region on an HTTP failure: fixed in both mount scripts — curl -sf so an HTTP error yields an empty string instead of a bogus body, and || true on the region assignment plus ${REGION:?} so a connection failure reaches the diagnostic instead of dying silently on the assignment line under set -e.
• TERMINATE on the mounts with no retry: fixed — both mounts now retry with backoff before the action can fail, so the known-transient NFS-DNS / Lustre settle race no longer terminates on the first miss; TERMINATE is reserved for a persistent failure.
• mount-openzfs-home.sh fstab guard + mount assertion: fixed. The guard now matches the /home mountpoint field and rewrites in place, and the mount is asserted with mountpoint -q /home inside the bounded retry so it can't report success silently.
• mount-lustre-fsx.sh:45 chmod 1777 gating the exit code: fixed — decoupled from the exit status. It must not terminate, because every realistic failure here (root-squash / read-only FS / MDS) is shared-side, so the replacement node hits the same condition and terminating would loop rather than heal.
• chmod 1777 running every boot on shared storage: left as-is, flagged. A reliable first-boot-only detector on shared Lustre is error-prone (.lustre/lost+found, admin-tighten vs default), and an admin who tightens /fsx root perms is already overriding the reference design. Can add a guarded path if it becomes a requirement.
• rsync perms downgrade (:54) and the fixed /tmp/home stash (:47): assessed, not changed. The action runs in nodeBootstrapped before any unprivileged login, and on reboot the fstab entry mounts /home before the script runs (the mountpoint -q early-exit), so the predictable-path and stale-stash windows don't open in normal operation; the --no-perms/trap rework carries its own regression risk on an EVERY_BOOT path. (The rsync ACL/vanished-file exits are now tolerated — Batch 6.)

Batch 3 — migration completeness (docs)

• Empty/single-space InstallEnrootPyxis still described, the "Post-Install Script" label, ambiguous "the default": fixed across OPERATIONS/CUSTOM-AMI/PARAMETERS; the heading and console label now read "Container Runtime (Enroot/Pyxis)".
• README "on by default" vs add-cng* defaulting false: fixed at the source — flipped the add-cng* default to true so the runtime is on everywhere and the README is accurate. deploy-all already passes the value down explicitly, the installer is idempotent, and false stays a one-setting opt-out; the standalone-queue footgun outweighed the old skip default.
• §8 upgrade path: documented the fleet cycle (launch-template + lifecycle change → DRAIN → every node replaced) and made "sync scripts before updating" an explicit prerequisite naming the added scripts and the TERMINATE consequence; fixed the "check a surviving node" advice for the fleet-wide case.
• In-file comments still crediting UserData for the moved interface: swept to the current positional-argument interface (including the LDAP_ADMIN_PASSWORD comment, now self-generated).
• S3BucketName: local sentinel: removed. Undocumented, unreferenced, and cfn package never rewrote the runtime boot-script fetch, so it couldn't supply scripts by design — removed rather than document a dead, now-load-bearing path.

Batch 4 — guard coverage & hardening

• MonitoringRepo/MonitoringVersion path traversal: added AllowedPatterns. MonitoringVersion deliberately still allows / so the documented fork+branch testing keeps working, while rejecting whitespace and shell metacharacters; the repo pattern allows exactly one slash.
• Root-executed scripts from a public, mutable, unpinned S3 object: recorded decision. The default bucket is AWS-owned and its objects vetted; a customer wanting self-controlled, pinnable scripts points S3BucketName at their own private bucket (the instance-role fetch supports it, no public-read needed). Not hardcoding a Checksum (regenerates on every edit, and a change triggers DRAIN); .sha256 companions / S3VersionId on the publish side is a reasonable follow-up.
• Grafana admin password traced into the install log: the trace is upstream (bash -x … | tee), so the script now chmod 600s the install log after completion to close the default-644 exposure on the multi-user login node. The root fix belongs upstream (drop -x / redact around the credential); worth an issue there.
• setup-directory.sh timing out yet reporting success: fixed — an ldapwhoami bind check after the readiness loop fails loudly and gates the OU adds / SSM write, instead of the loop expiring silently.
• Self-generated LDAP admin password silently rotating on replacement: fixed — the SSM read now captures its exit status and only regenerates on a genuine "not found", refusing to clobber a working credential on a transient error.
• Manifest cross-check missing scripts fetched by other scripts: fixed — widened the check to templates and boot scripts, so dropping the helper's manifest entry now fails the lint (verified with a negative test).
• Silent CONTINUE degradation: the specific cases you flagged are fixed at their source (dropping the non-resolving latest, widening the manifest cross-check — both above), and the per-script lifecycle logs capture the rest.

Batch 5 — stage placement & parameter validation

• install-monitoring header says "login node" but it runs on compute too: reconciled the header (login = server, compute = exporters, keyed on MonitoringRole) and noted the nodeReady job-dispatch window the stage trades for not blocking node availability.
• Empty FSxOpenZFSFilesystemId replace loop: added AllowedPattern: '^fs-[0-9a-f]{8,17}$' (matching the existing FSx-id / AmiId patterns), turning the terminate loop into a validation error.
• MonitoringVersion=latest advertised but never resolving: dropped latest from the Description/ConstraintDescription and the parameter docs rather than adding a boot-time GitHub-API resolve; the default is already a concrete tag.

Batch 6 — nits

• Fixed: mount -a -t nfs narrowed to mount /home; rsync exits 23/24 now tolerated; the FS-id-change duplicate-/home case covered by the mountpoint-field rewrite above; install-monitoring fixed /tmp/post-install.sh → mktemp -d; the "after 3 attempts" message now notes curl doesn't retry 4xx; the stray defaults on mount -a; the needrestart override_rc whole-hash reassignment → single-key; SlurmVersion's "post-install lifecycle action" → install-enroot-pyxis; the §3.1 cross-file anchor double-hyphen; the apt drop-in comment (it's a persistent node-wide file, not caller-scoped); and DEPLOY-TESTING now states a re-sync doesn't reach a running node under CACHE_ONCE (replace the instance).
• Noted, left as-is to keep the PR scoped (tracked for follow-up): ${1:?msg} printing unbound variable first; mountpoint -q vs a findmnt/stat liveness check; setup-directory.sh:45 empty-vs-unset S3KeyPrefix; dropping the lint-docs.sh sed-strip; the lustre/openzfs fstab asymmetry comment; and the pre-existing appendix items (enroot /tmp/enroot parent perms and unpinned enroot.conf, slapd RequiresMountsFor=/home, --ignore-existing key pinning, the setup-directory.sh:242 curl timeout).

Thanks again for the review.

[KeitaW]

Review Batch 1/3 — Response scoreboard

An exemplary response round: 18 commits resolving 16 of the 21 round-1 findings (plus
four of the nits), with
the two must-fix mount defects fixed in a way that goes past what I asked for. I re-ran
the checks that produced each finding against 3ad54d8f; the verified results are below.

Resolved — re-verified by execution:

• IMDS region resolution. -f plus --retry 5 --retry-connrefused --connect-timeout 2 --max-time 5 and || true on both branches. Both failure paths I reported are closed —
  the connection failure now reaches the guard, and an HTTP error body no longer passes it.
  Re-ran the round-1 repro: REGION: failed to resolve region from IMDS, exit 1 (round 1:
  exit 7, silent).
• needrestart. $nrconf{override_rc}{qr(^slurmd)} = 0;. Measured on Ubuntu 24.04 /
  needrestart 3.6-7ubuntu4.5: 44 patterns after the drop-in, with ^docker, ^dbus and
  ^slurmd all present (round 1: 1 pattern).
• fstab guard. ensure_home_fstab_entry uses grep -qxF and a sed that removes any
  active /home entry before appending. That closes both the commented-line bypass and the
  duplicate-entry-on-filesystem-change case I filed as a nit — one function, two findings.
  Verified: a commented-out line no longer defeats it (1 active entry), and a changed
  FS id/region rewrites rather than appends (still 1).
• Mount retry + scope. Bounded 6 × 10s retry in both scripts, each attempt gated on
  mount … && mountpoint -q, and mount /home replaces mount -a -t nfs — which also
  resolves the "an unrelated NFS entry can terminate the node" nit.
• safe_rsync tolerating exit 23/24, so an ACL-on-NFSv3 or a vanishing source file no
  longer terminates the instance.
• Full mount E2E re-run on the restructured script: first boot, reboot-already-mounted,
  and fstab-present-but-unmounted all exit 0, one fstab entry throughout, --ignore-existing
  precedence and local-file restore intact. The rework didn't regress the happy path.
• FSxOpenZFSFilesystemId / FSxLustreFilesystemId now carry AllowedPattern
  (^fs-… required / ^$|^fs-… optional), closing the unbreakable replace loop.
• LDAP. SSM read now distinguishes ParameterNotFound from a real failure and refuses
  to regenerate on the latter; and the new ldapwhoami bind check before OU creation and
  the SSM write closes the "already-installed slapd keeps its old olcRootPW" half. Both
  of my sub-findings, both fixed.
• Docs. §8 now states the drain-and-replace consequence and the
  sync-your-bucket-first prerequisite, both accurately and in the right order of alarm.
  Stale empty/single-space semantics gone; §5.3 heading matches the console group; the
  #31-dcgmexporterimage--… anchor fixed in both files.
• Lint widening, mktemp -d for the monitoring download, chmod 600 on the monitoring
  log, latest dropped from MonitoringVersion guidance, in-file UserData comments
  corrected, and the install-monitoring.sh header rewritten to state the compute-node
  reality and name the job-dispatch trade-off explicitly.

Still open — carried forward below: the MonitoringVersion pattern doesn't block what
it was added to block (#1), the restore rsync still rewrites shared directory modes (#2),
the /home stash is still a fixed /tmp path (#3), and chmod 1777 /fsx still reasserts
itself on every boot (#4).

[KeitaW]

Review Batch 2/3 — Carried forward

[KeitaW]

The new MonitoringVersion pattern doesn't block the traversal it was added for

AllowedPattern: '^[A-Za-z0-9._/-]+$' permits both / and ., so the exact value from
the round-1 comment still passes — and still redirects:

pattern test:  main/../../../torvalds/linux/master/README   -> MATCHES
fetch:         http=200  final=https://raw.githubusercontent.com/torvalds/linux/master/README

That content is then run as root at install-monitoring.sh:77. Allowing / is the right
call — feature/my-branch is a legitimate ref — so the fix is to require each path segment
to start with an alphanumeric, which makes a .. segment unrepresentable without lookahead
(CloudFormation's AllowedPattern can't rely on it):

Suggested change

	AllowedPattern: '^[A-Za-z0-9._/-]+$'
	AllowedPattern: '^[A-Za-z0-9][A-Za-z0-9._-]*(/[A-Za-z0-9][A-Za-z0-9._-]*)*$'

Verified against both: v2.10.2, main, feature/my-branch and release/1.2 all pass;
main/../../../…, ../.. and .. are all rejected.

MonitoringRepo at :178 has the same shape of gap — see the separate comment there.

[KeitaW]

MonitoringRepo admits .. segments too

Same shape as the MonitoringVersion gap above: ../.. matches
^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$, because .. satisfies [A-Za-z0-9._-]+. This one
resolves to a 404 rather than an attacker-chosen repo (verified), so it is not the same
severity — but the same segment anchoring makes it exact at no cost:

Suggested change

	AllowedPattern: '^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$'
	AllowedPattern: '^[A-Za-z0-9][A-Za-z0-9._-]*/[A-Za-z0-9][A-Za-z0-9._-]*$'

[KeitaW]

The restore rsync still reopens shared directory modes

Re-ran the round-1 test against 3ad54d8f with a shared /home/ubuntu at 0700 and a
local one at 0755:

>>> shared dir mode after the restore: 755      (unchanged from round 1)
    conflict file: SHARED-WINS                  (files still correctly skipped)

--ignore-existing governs files, not the attributes of existing directories, so the
header's "any pre-existing shared state wins" still doesn't hold for modes. On a
multi-user cluster this silently opens every home directory a slow-path node touches.

Suggested change

	safe_rsync -aA --ignore-existing /tmp/home/ /home
	safe_rsync -aA --ignore-existing --no-perms --omit-dir-times /tmp/home/ /home

(Or restore only paths that don't exist at all — either keeps the stated invariant.)

[KeitaW]

The /home stash is still a fixed /tmp path — and mktemp -d was applied one file over

install-monitoring.sh:38 adopted WORKDIR=$(mktemp -d) with a cleanup trap — exactly
the remedy — but mount-openzfs-home.sh still uses mkdir -p /tmp/home at :72 and
rm -rf /tmp/home at :99. Both round-1 consequences stand, and this is the one handling
the contents of every user's home directory:

• /tmp is disk-backed on Ubuntu 24.04 and TERMINATE fires only on a non-zero exit, so a
  panic or scheduled reboot between :73 and :99 leaves a stash that a later slow-path
  boot merges into shared /home — resurrecting deleted files.
• mkdir -p succeeds on an existing directory whatever its owner, so any login-node user
  can pre-create /tmp/home/<name>/… and have root copy it into shared /home.

The same three lines that fixed the monitoring script fix this one.

[KeitaW]

chmod 1777 /fsx no longer gates the exit code, but still reasserts itself every boot

The || echo WARNING fixes the half I was most worried about — a healthy node is no longer
destroyed by a shared-side chmod failure, and the comment explaining why is exactly right.

The other half is still open: because this script deliberately writes no fstab entry,
mountpoint -q /fsx is false on every reboot, so :71 runs every time — against shared
storage. An admin who tightens the Lustre root (chmod 0770 /fsx; chown root:clusterusers)
has it reset cluster-wide by the next node to reboot, and now silently, since the chmod
succeeds. Gating it on "this script just created the mount" would keep the first-boot
behavior and drop the reassertion.

[KeitaW]

Review Batch 3/3 — New in round 2, nits & sources

Nits

• setup-directory.sh:135-141 captures the SSM call with 2>&1, so ssm_out holds either
  the parameter value or the error text. On success with any stderr noise (a CLI
  deprecation warning) the value is polluted. The new ldapwhoami check would catch the
  consequence, so this is defensive only — but capturing stdout and stderr separately would
  be cleaner than making one variable carry both roles.
• Worst-case nodeBootstrapped time went up: the two mount retries are up to 6 × 10s each
  plus mount's own timeouts, on top of setup-directory's ≤5 min discovery and the Pyxis
  build. Still comfortably inside the ~30-minute registration budget in normal conditions,
  but the margin is thinner than it was — worth knowing if a future action is added to the
  same stage.
• The logger -t pcs-<name> suggestion from round 1 wasn't picked up. Not a blocker, and
  reasonable to defer — noting it only so it isn't lost.

Appendix — pre-existing, not introduced here

While verifying the anchor fix I checked every cross-file anchor in the tree. One is
broken, and it isn't from this PR: README.md:669 links to
./tests/README.md#test-9-efa-on-cpu-hpc-instances-hpc6a--hpc7a--hpc8a, but that heading
lives in tests/hpc-efa-test.md, not tests/README.md (introduced by #1137). Mentioning
it only because this round is already fixing anchors of exactly that class — lint-docs.sh
still checks README-internal anchors only, so cross-file ones stay uncovered.

Things That Look Great

• The mount rework went past the ask. I asked for a retry; the result also scopes the
  mount to /home, gates each attempt on mountpoint, rewrites stale fstab entries instead
  of appending, and tolerates rsync 23/24 — four separate round-1 items closed by one
  coherent restructure rather than four patches. The ensure_home_fstab_entry /
  safe_rsync helpers make the intent readable, and I re-ran the full three-path E2E to
  confirm the restructure didn't cost anything on the happy path.
• Every fix carries a comment explaining the failure it prevents — why -f matters on
  the IMDS call, why the chmod must not gate the exit, why the whole override_rc hash must
  not be reassigned, why mktemp -d beats a fixed path. That is the difference between a
  fix that survives the next refactor and one that gets undone by someone tidying up.
• The two LDAP fixes are better than what I suggested. I asked for an ldapwhoami
  check; you added that and the ParameterNotFound discrimination with a comment naming
  the silent-rotation scenario it prevents.
• §8 now reads like something an operator can act on — the drain-and-replace warning is
  stated before the table rather than buried in it, and the bucket-sync prerequisite spells
  out the replace-loop consequence rather than just saying "sync first".
• The install-monitoring.sh header now documents the trade-off instead of contradicting
  the wiring — including that a job can be dispatched while the install is still running.
  Writing the downside down is the right resolution for a deliberate design choice.

Sources

• Configure lifecycle actions — script retry semantics, CACHE_ONCE, DRAIN on update
• Best practices — the ~30-minute registration budget

Verified live, 2026-08-24 (all against 3ad54d8f):

• IMDS guard re-run in a container → REGION: failed to resolve region from IMDS, exit 1
• needrestart on Ubuntu 24.04 / 3.6-7ubuntu4.5 → 44 patterns, ^docker ^dbus ^slurmd present
• fstab guard vs a commented-out line → 1 active entry; vs a changed FS id/region → 1 entry
• mount E2E first-boot / reboot / remount → exit 0, one fstab entry, restore precedence intact
• restore rsync with shared 0700 + local 0755 → shared dir becomes 755
• MonitoringVersion pattern vs main/../../../torvalds/linux/master/README → matches; fetch resolves to that repo, http 200
• bash tests/lint-docs.sh → PASS; cfn-lint → clean on all four CNG templates; bash -n + shellcheck clean

[KeitaW]

Flipping InstallEnrootPyxis to true in the standalone templates is a behavior change worth a §8 row

Resolving the round-1 README contradiction by changing the templates rather than the docs
is the better call — the defaults are now consistent everywhere and the Launch Stack
buttons do what the README says.

It does mean the four standalone add-cng*.yaml changed behavior: the old
PostInstallScriptUrl: '' meant skip, and InstallEnrootPyxis: 'true' means install.
Anyone adding a GPU queue from those templates now gets a ~2-3 min Enroot/Pyxis install and
an S3 fetch they didn't get before. That's a fine default, but it's the one place the
migration no longer preserves prior behavior, and §8 is where someone would look for it.

[DaisukeMiyamoto]

Thanks for the thorough round-2 pass — each point is well-reasoned, and I went through all five against the actual PCS execution model. One change landed; the rest I'd argue don't need code changes, for the reasons below.

#3 — dir-mode reset on restore (fixed). Correct and reproducible: the restore rsync -aA --ignore-existing still re-applied the snapshot's mode to a directory that already exists on the shared export, so a tightened /home/ubuntu would be loosened on the next new node's first boot. Fixed by adding --no-perms --omit-dir-times — the restore now only fills in missing files and never rewrites an existing directory's mode/times (--ignore-existing continues to protect files). Verified with your test shape (shared 0700 + local 0755 → shared stays 0700).

#1 / #2 — AllowedPattern on MonitoringVersion / MonitoringRepo. These patterns were added in round 1 in response to your earlier feedback — so the gap here isn't their origin but my reply not spelling out their intent. To be clear now: they were only ever meant as input validation, not a security boundary, and the ..-admits-traversal observation is a fair read against a security bar they were never trying to meet. Stepping back, AllowedPattern can't serve a security purpose here at all: whoever sets these stack parameters can equally edit the template, ScriptLocation, and the IAM roles, and passes CAPABILITY_* — anyone able to influence the value already controls the whole deployment, so no regex adds privilege separation. Their legitimate role is to fail fast with a clear error instead of a 404 at node boot, and the current patterns already do that. So rather than tighten them toward a security guarantee they can't provide, I'd keep them as basic validation as-is — what was missing was this explanation, not a stricter regex.

#4 — resurrecting deleted files / pre-created /tmp/home. The premise can't occur: the stash→mount→restore only runs during first-boot nodeBootstrapped, and ensure_home_fstab_entry writes the /home fstab entry before the mount, so any subsequent same-instance boot early-exits at mountpoint -q /home and the restore never re-runs. An interruption before the fstab write means no shared mount happened; PCS replaces a panicked node with a fresh disk. There's also no unprivileged session at first boot to pre-create /tmp/home. I can switch the stash to mktemp -d + trap for consistency with install-monitoring.sh if you'd like, but it's cosmetic — happy either way.

#5 — chmod 1777 /fsx re-running on every boot. The mechanism is real (no fstab entry ⇒ /fsx self-mounts and re-chmods on every fresh/scaled node), but the premise — re-permissioning the Lustre root after cluster creation — isn't a supported operation in this design; 1777 is the intended sticky-world-writable shared root (like /tmp). Gating the chmod on "we just created the mount" is a no-op given there's no fstab early-exit, and the alternatives (fstab entry, cluster-wide sentinel) are either partial or error-prone across replacement. Since a persistent re-permission of the shared root isn't an anticipated operation, I'd keep the current behavior.

Also documented in OPERATIONS.md §8: the standalone add-cng*.yaml default is now InstallEnrootPyxis=true (deploy-all already defaulted true), and fixed a Test 9 cross-file doc link while here.