Skip to content

fix(slime): pin numpy < 2 so Megatron init does not abort the train worker - #4

Merged
littlemex merged 2 commits into
epic/enable-slime-grpo-on-b300-h200from
feature/pin-numpy-lt2
Jul 3, 2026
Merged

littlemex merged 2 commits into
epic/enable-slime-grpo-on-b300-h200from
feature/pin-numpy-lt2

Conversation

@littlemex

Copy link
Copy Markdown
Owner

Purpose

Fourth fix in the epic to run the SLIME GRPO test case end-to-end on B300 / H200 (tracked by awslabs#1163). This merges into the epic branch epic/enable-slime-grpo-on-b300-h200, not into main directly; the epic is what goes to upstream in awslabs#1164.

With walls 1, 2 and 5 fixed, training now starts, the rollout HTTP server binds, and the train worker launches. The next wall is at Megatron initialization: the train worker aborts before the first step.

Changes

Megatron-LM refuses numpy 2.x. At init it asserts:

assert np.__version__.startswith("1."), "Megatron does not support numpy 2.x"

(the assert is in slime's slime/backends/megatron_utils/initialize.py L66, enforcing NVIDIA/Megatron-LM#1563). But the image ends up with numpy 2.x: sglang[all] — installed just before requirements.txt in slime.Dockerfile L160-L168 — pulls numpy 2.x transitively (numpy is a dependency of sglang, transformers, datasets, accelerate, scipy, and many others). So MegatronTrainRayActor dies during init with AssertionError: Megatron does not support numpy 2.x.

The fix is a one-line pin: add numpy<2 to requirements.txt.

Why requirements.txt specifically (placement matters): slime.Dockerfile installs in this order — sglang[all] (which introduces numpy 2.x), then requirements.txt, then the sgl-router wheel and slime itself (both effectively --no-deps). Putting the pin in requirements.txt means the requirements install downgrades numpy back to 1.x after sglang pulled 2.x, and nothing after it reintroduces 2.x. Placing the pin earlier (before sglang) would be undone; placing it in a separate later RUN would also work but adds a layer for no benefit.

I verified the downgrade is clean: pip install "numpy<2" resolves to numpy 1.26.4 with no dependency conflicts that block it (a pip install --dry-run reports only Would install numpy-1.26.4). tifffile prints an advisory that it prefers numpy>=2.1, but it is a transitive image dependency not used on the SLIME/Megatron/SGLang GRPO path, and both torch and megatron.core import cleanly under numpy 1.26.4.

Implementation

Alternatives considered (and why this is the most elegant)

The constraint is fixed and external: Megatron-LM does not support numpy 2.x (NVIDIA/Megatron-LM#1563), and this is a downstream test case that cannot change Megatron or SGLang. So the only decision is where and how to honor numpy<2. I considered each option:

  1. Pin numpy<2 in requirements.txt (chosen). One line, at the last dependency step that touches numpy, so it downgrades the 2.x that sglang[all] pulled and nothing after re-introduces it. It is exactly what upstream SLIME's own docker/Dockerfile does (pip install "numpy<2"), so it mirrors the sanctioned approach and stays close to upstream.

  2. A separate trailing RUN pip install "numpy<2" after the SLIME install. Functionally equivalent and slightly more order-robust (survives a future reorder or a new numpy-pulling install), but it adds a Docker layer and a second place that states the same constraint. Rejected as the default because it is strictly more machinery for no current benefit; the ordering is already correct and documented in the pin's comment. (If the team later wants order-independence for free, this is the drop-in upgrade — keeping <2, not ==.)

  3. Pin an exact numpy==1.26.4. Rejected: it would diverge from upstream SLIME's open-ended <2 and needlessly freeze the image on one 1.x release. <2 lets pip pick the best 1.x the rest of the set resolves to (currently 1.26.4) and is what Megatron actually requires (startswith("1.")).

  4. Patch/relax the Megatron assert, or install numpy 2.x-compatible Megatron. Rejected: that is changing the upstream training backend's supported configuration from a test case — out of scope, unverifiable here, and against the "install upstream unmodified where possible" principle. The assert is correct; the image should satisfy it, not defeat it.

  5. Force SGLang onto numpy 1.x from the start / avoid pulling 2.x. Not actionable: numpy 2.x arrives transitively through many packages (sglang, transformers, datasets, accelerate, scipy, ...). Constraining every producer is fragile; correcting once, after they have all resolved, is the clean seam. sglang 0.5.12.post1 itself declares plain numpy with no lower bound, so pinning <2 does not fight it.

Option 1 is the most elegant: one line, upstream-identical, at the correct layer, with no new Docker layer and no second source of truth.

Portability (B300 / other GPUs)

This pin is GPU-generation- and CUDA-independent, so it does not risk B300. numpy is a pure-CPU package: numpy 1.26.4 ships no native extension that links a CUDA runtime (verified: zero of its .so files reference libcudart/libcuda), and Megatron's guard is a pure version string check (np.__version__.startswith("1.")), not a GPU or CUDA-major concern. The same numpy<2 is required and equally harmless on H100, H200 and B300 — unlike the wall-5 .so selection, there is no per-generation behavior here to break.

Test Plan

This test case runs on Amazon EKS, so verification is done with kubectl exec into a Ray worker pod. The check below is copy-paste-safe (no <placeholder> tokens, no shell comment lines inside a command, no Ray job id required) and shows both the bug and the fix in one run. Set the namespace once:

export NAMESPACE=<your-namespace>

The check reports the effective numpy version and whether Megatron's init assert (np.__version__.startswith("1.")) would pass, and confirms torch/megatron import cleanly:

WPOD=$(kubectl -n "$NAMESPACE" get pod -l ray.io/group=gpu-workers -o jsonpath='{.items[0].metadata.name}')
kubectl -n "$NAMESPACE" exec -i "$WPOD" -c ray-worker -- python3 -W ignore - <<'PY'
import numpy
ok = numpy.__version__.startswith("1.")
print("numpy version =", numpy.__version__)
print("Megatron init assert np.__version__.startswith('1.') =>", "PASS" if ok else "FAIL (Megatron does not support numpy 2.x)")
import torch, megatron.core
print("import torch/megatron.core => OK")
PY

Expected with the pin (numpy resolved to 1.x, so the assert passes):

numpy version = 1.26.4
Megatron init assert np.__version__.startswith('1.') => PASS
import torch/megatron.core => OK

Without the pin the same check reports numpy version = 2.1.0 and FAIL, which is exactly the AssertionError: Megatron does not support numpy 2.x the train worker hits at init.

Test Results

Verified on hardware (H200, 2× p5en.48xlarge, CUDA 13.0):

  • The image builds cleanly from this slime.Dockerfile: during the build the requirements.txt install step uninstalls numpy 2.1.0 and installs numpy 1.26.4 (the pin takes effect after sglang[all] pulled 2.x), and the build completes and pushes.
  • A container started from that freshly built image (no manual change) reports numpy 1.26.4; Megatron's init assert passes; import torch and import megatron.core succeed.
before after numpy<2
numpy version 2.1.0 1.26.4
import torch / import megatron.core OK OK
Megatron init AssertionError: Megatron does not support numpy 2.x passes
train worker aborts at init reaches Megatron init and proceeds into the rollout/train/weight-sync cycle

Blast radius / risk

  • Low. One added constraint in requirements.txt. numpy 1.26.4 is the last 1.x release and is the version Megatron expects.
  • pip resolves the rest of the pinned set against numpy 1.x without conflict (verified via dry-run and by importing torch/megatron/sglang).
  • No CLI/API or recipe changes.

Checklist

  • I am working against the epic branch (not main directly).
  • The change is minimal (one pinned constraint, with a comment explaining the ordering).
  • External dependencies are pinned (numpy<2 resolves to 1.26.4).
  • Verified on hardware (H200 / CUDA 13): numpy is 1.x, Megatron init passes, the train worker proceeds.

littlemex added 2 commits July 3, 2026 21:19
…orker

Megatron-LM asserts numpy 1.x at init (per NVIDIA/Megatron-LM#1563), but
sglang[all] -- installed just before requirements.txt in slime.Dockerfile --
pulls numpy 2.x transitively, so MegatronTrainRayActor dies during init with
'AssertionError: Megatron does not support numpy 2.x'.

Pin numpy<2 in requirements.txt. Because the requirements install runs after
sglang[all] and the later slime/sgl-router installs use --no-deps, this
downgrades numpy back to 1.26.4 and nothing reintroduces 2.x. Verified on H200
(CUDA 13): numpy 1.26.4, torch/megatron import cleanly, Megatron init passes and
the train worker proceeds into the rollout/train cycle.
Fixes an inaccurate comment (the later ring_flash_attn install is not --no-deps)
and records why numpy 1.x survives to runtime: the only post-pin pip steps are
slime/sgl-router (--no-deps) and ring_flash_attn 0.1.8 (declares no deps), so
none reintroduce numpy 2.x. Notes that upstream SLIME's own Dockerfile pins
numpy<2 the same way, and adds a TODO tying the pin's removal to a future
MEGATRON_LM_VERSION that drops the numpy 1.x assert. No functional change.
@littlemex
littlemex merged commit 124751d into epic/enable-slime-grpo-on-b300-h200 Jul 3, 2026
@littlemex
littlemex deleted the feature/pin-numpy-lt2 branch July 3, 2026 21:24
littlemex pushed a commit that referenced this pull request Sep 9, 2026
… lifecycle actions (awslabs#1236)

* feat(aws-pcs): extract first-boot logic to scripts, harden all boot fetches

Phase 1 of the node-lifecycle-actions migration: move the needrestart
guard and the FSx OpenZFS//home + Lustre//fsx mounts out of inline
cloud-init runcmd into standalone bash scripts under assets/scripts/,
fetched from the templates bucket like the existing post-install and
directory scripts.

All five boot-script fetches now go through a pcs-fetch helper:
- 3 attempts 15s apart, every attempt logged to /var/log/pcs-boot-fetch.log,
  grep-able 'ERROR: failed to fetch' on final failure (a silent directory
  fetch failure on a replacement login node cost a support round-trip)
- pins the AWS CLI to the classic transfer client via a scoped
  AWS_CONFIG_FILE: on P5/P6 instance types 'auto' resolves to the CRT
  client, which does not follow S3 region redirects, so cross-region
  fetches fail (the root cause of post-install exit 127 on GPU CNGs)
- re-probes the bucket region each attempt; an empty probe falls back to
  the classic client's redirect-following instead of failing

HTTPS fetches (post-install http(s), monitoring GitHub raw) get
curl --retry 3 plus the same ERROR logging.

Verified e2e on us-east-2 deploy-all (login + compute): all fetches
logged, mounts up, Enroot/Pyxis exit 0, OpenLDAP server+client working,
ldap-add-user resolves on both nodes, srun + Pyxis container jobs pass.
Failure path verified: nonexistent key exits 1 after 3 logged attempts.

* feat(aws-pcs): replace CNG UserData with PCS node lifecycle actions

Move ALL first-boot logic from cloud-init UserData to NodeLifecycleActions
on the ComputeNodeGroup resource (requires PCS agent >= 1.5.0-1, in
PCS-Ready DLAMI builds since 2026-07-20 — see docs/PCS-READY-DLAMI.md).
The four CNG templates no longer carry a UserData block at all.

nodeBootstrapped (in order, before slurmd starts):
  1. needrestart-guard      FIRST_BOOT_ONLY / CONTINUE
  2. mount-openzfs-home     EVERY_BOOT / TERMINATE
  3. mount-lustre-fsx       EVERY_BOOT / TERMINATE   (only when Lustre is set)
  4. setup-directory        FIRST_BOOT_ONLY / CONTINUE (only when enabled)
  5. post-install           FIRST_BOOT_ONLY / CONTINUE (only when set)
nodeReady:
  6. install-monitoring     FIRST_BOOT_ONLY / CONTINUE (only when enabled)

Why all-at-once instead of piecemeal: nodeBootstrapped runs only after
cloud-init user-data completes (pcs_bootstrap_finalize), so any UserData
step that depends on a lifecycle-mounted filesystem deadlocks — verified
on a real deploy where directory setup waited 10 minutes for a /home
mount that could not happen until cloud-init exited. Dependencies must
live in one execution domain; within a stage, ordering is guaranteed.

Script interface changes (lifecycle actions pass positional args, not env):
- setup-directory.sh: role/domain-suffix/cluster-id/bucket/prefix as
  args 1-5 (env interface kept); generates the admin password itself
  (SSM reuse logic unchanged); hard-fails unless /home is a mountpoint.
- install-enroot-pyxis.sh: accepts the Slurm version as arg 1
  (PCS_SLURM_VERSION env kept for the custom-AMI build path).
- install-monitoring.sh (new): monitoring installer wrapper — GitHub
  fetch with curl retry, apt dpkg-lock drop-in, 3-attempt install loop.

The agent replaces the hand-rolled fetch plumbing: per-script retries and
logs (/var/log/amazon/pcs/lifecycle/actions/<stage>/<name>.log), and its
downloader is unaffected by the AWS CLI CRT region-redirect bug that
motivated pcs-fetch. Lifecycle config changes via UpdateComputeNodeGroup
now reach existing CNGs with DRAIN semantics instead of requiring CNG
recreation (the LaunchTemplate Version pin problem).

lint-docs.sh: the four-template lock-step check now covers the whole
NodeLifecycleActions block, and every referenced lifecycle script must
exist in assets/scripts/.

Verified e2e on us-east-2 deploy-all (login + compute, directory +
monitoring + Enroot/Pyxis enabled, cross-region templates bucket): all
six scripts exit 0 in order, /home //fsx mounted, slapd + SSSD up,
ldap-add-user resolves on both nodes, Grafana/Prometheus containers
running, srun + Pyxis container jobs pass.

* docs+fix(aws-pcs): align docs with lifecycle actions; keep space-skip working

Template fixes surfaced by the user-impact audit:
- PostInstallScriptUrl skip sentinel: deploy-all passes a single space
  through to add-cng, where the lifecycle ScriptLocation pattern would
  reject it and fail CNG creation. HasPostInstall/HasPostInstallArgs now
  treat empty AND single-space as 'no post-install' (verified: a CNG with
  the skip value creates cleanly with no post-install entry).
- AmiId descriptions (4x add-cng + deploy-all + PARAMETERS.md) now state
  the PCS agent >= 1.5.0-1 floor for pinned/custom AMIs.
- PostInstallScriptUrl/Args descriptions document the new contract:
  https:// only (no plain http), SlurmVersion as first argument,
  PostInstallScriptArgs as a single unsplit argument.

Docs updated for the new mechanics: log paths moved to
/var/log/amazon/pcs/lifecycle/actions/<stage>/<name>.log (README diagram,
OPERATIONS 2.3/4.1/4.2/4.4/6, PARAMETERS, DEPLOY-TESTING, USER-MANAGEMENT
5.2, tests/infra + storage + readme-walkthrough), mount failures now
TERMINATE and replace the node (storage-test troubleshooting), Lustre
tuning hooks reference lifecycle actions instead of UserData.

Pre-PR e2e on a fresh us-east-2 deploy-all (directory + monitoring +
Enroot/Pyxis): all six lifecycle scripts exit 0 on login + compute, no
UserData leftovers on nodes, docs commands run as written (executor grep,
sinfo, docker ps, enroot/pyxis paths, Prometheus targets up, Grafana 200),
srun + Pyxis container job pass, ldap-add-user resolves on both nodes,
skip-sentinel CNG attach validated.

* feat(aws-pcs)!: replace PostInstallScriptUrl/Args with InstallEnrootPyxis

The generic post-install hook existed because PCS had no native way to
run a custom script at first boot. Node lifecycle actions ARE that native
way now, and the only thing this repo ever shipped through the hook is
the Enroot/Pyxis installer — so name the feature for what it does:

- InstallEnrootPyxis ('true'/'false') replaces PostInstallScriptUrl +
  PostInstallScriptArgs on deploy-all and all four add-cng templates.
  The lifecycle entry is named install-enroot-pyxis (its log follows:
  .../nodeBootstrapped/install-enroot-pyxis.log).
- The script location is fixed to
  s3://<S3BucketName>/<S3KeyPrefix>scripts/install-enroot-pyxis.sh —
  dev overrides ride the existing S3BucketName/S3KeyPrefix redirection,
  which removes the last reason pre-merge deploys had to set a URL.
- Defaults preserve prior behavior at both layers: deploy-all 'true'
  (was: empty auto-installs), modular add-cng 'false' (was: empty skips).
  Users with custom post-install scripts add them to the compute node
  group's NodeLifecycleActions directly (documented in the param
  descriptions, README and PARAMETERS.md).
- lint-docs: PostInstallScriptUrl/Args are now BANNED names in docs;
  the empty-vs-space skip-wording check is gone with the sentinel.

BREAKING CHANGE: PostInstallScriptUrl / PostInstallScriptArgs no longer
exist. Set InstallEnrootPyxis=false instead of the single-space sentinel;
attach custom first-boot scripts as node lifecycle actions.

* chore(aws-pcs): publish new lifecycle scripts; lint manifest coverage

The publish manifest is an explicit allowlist — without these entries the
lifecycle-migration templates would reach the production bucket while
their scripts did not, and every post-merge deploy would fail at the
agent's script download (the mount scripts TERMINATE, so nodes would
enter a replace loop). Add the four new scripts:
needrestart-guard.sh, mount-openzfs-home.sh, mount-lustre-fsx.sh,
install-monitoring.sh.

lint-docs.sh now cross-checks every template-referenced lifecycle script
against the manifest so this class of gap fails the publish workflow's
lint step instead of production deploys (verified: removing an entry
makes the lint FAIL).

* docs(aws-pcs): migration notes for the lifecycle-actions changes; final sweep

Add OPERATIONS.md §8 documenting what changes for users coming from the
UserData-based templates (InstallEnrootPyxis replaces PostInstallScriptUrl/
Args, agent >= 1.5.0-1 floor, new log locations, mount failures now replace
the node, installer argument contract) — the repo had no in-tree record of
these behavior changes.

Sweep of README/docs/tests for remaining UserData-era wording: README
architecture diagram now lists all six lifecycle scripts, ROADMAP's FSx-EFA
item targets a lifecycle-action script, OPERATIONS 4.2/6.1 and
infra/storage-test phrasing updated.

* fix(aws-pcs): harden IMDS region lookup in the TERMINATE mount scripts

Under set -euo pipefail, REGION=$(curl ...) took curl's exit status, so a
connection failure killed the script on the assignment line and skipped the
${REGION:?} diagnostic one line below — on an OnError:TERMINATE action the
instance is then replaced before anyone can read the (truncated, error-less)
log. And curl -s (no -f) exits 0 on a 4xx/5xx and captures the error body, so
under HttpTokens=required a throttled token PUT or a 401 GET landed an HTML/401
body in REGION and passed the guard, producing a bogus FSx DNS name and a mount
failure -> terminate.

Add -f (4xx/5xx -> empty), --retry/--connect-timeout/--max-time (ride out a
throttled/slow IMDS), and || true (reach the :? diagnostic instead of dying on
the assignment). Both mount scripts, all three call sites. Verified: a
connection failure now exits 1 with the message instead of a silent exit 7.

Addresses review batch 1/6 (IMDS guard).

* Retry mount in TERMINATE lifecycle scripts (PR awslabs#1236 #2)

mount-openzfs-home.sh and mount-lustre-fsx.sh run as OnError:TERMINATE
lifecycle actions with a single-shot mount. The common first-boot NFS
DNS settle race (mount.nfs: Failed to resolve server) fails instantly,
which under TERMINATE replaces the node into the same window. Wrap the
mount in a bounded retry loop (6 attempts, 10s backoff) verified with
mountpoint(8), so TERMINATE fires only on a persistent failure.

* Constrain FSx filesystem ID params with AllowedPattern (PR awslabs#1236 #3)

FSxOpenZFSFilesystemId feeds the unconditional mount-openzfs-home action
(OnError:TERMINATE) with no AllowedPattern/Default. An empty or malformed
value passes stack create, then the mount script fails at boot and the
node is TERMINATEd and replaced into the same bad value -- an endless
terminate/replace loop. Add a mandatory '^fs-[0-9a-f]{8,17}$' pattern so
CFN rejects it at create time. Add the sibling '^$|^fs-...$' pattern to
FSxLustreFilesystemId, whose empty value is the documented 'no /fsx'
opt-out (HasLustre). Applied identically across all 4 add-cng templates.

* Constrain MonitoringRepo/MonitoringVersion with AllowedPattern (PR awslabs#1236 #4)

Both feed the raw.githubusercontent.com URL that install-monitoring.sh
fetches and bash-runs. These are admin-only params, so the AllowedPattern
is input hygiene (catch typos at CFN validation, match the AmiId/FSx-id
convention) rather than an anti-exploit measure.

MonitoringRepo: ^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$ (exactly owner/repo).
MonitoringVersion: ^[A-Za-z0-9._/-]+$ -- deliberately allows '/' so the
documented fork+branch testing workflow (feat/... branches) keeps working;
rejects whitespace and shell metacharacters. Applied across all 4
add-cng templates.

* Set only the slurmd needrestart override, not the whole hash (PR awslabs#1236 #5)

$nrconf{override_rc} = { qr(^slurmd) => 0 } reassigns the entire override_rc
hash, discarding needrestart's shipped defaults and leaving slurmd as the
only entry. Mutate the single key instead:
  $nrconf{override_rc}{qr(^slurmd)} = 0;
so the shipped defaults are preserved and only slurmd is added.

Pre-existing since awslabs#1165 (ac66712); this PR carries the one-line fix in
the extracted script rather than deferring it to a separate PR.

* Update OPERATIONS §6.2 needrestart snippet to match the #5 fix

The doc code sample still showed the whole-hash reassignment; align it with
the one-key form now written by needrestart-guard.sh.

* Document the stack-update upgrade path in OPERATIONS §8 (PR awslabs#1236 M1)

Two gaps the review flagged:
- §8 said existing stacks keep working as-is but never said the update
  itself DRAINs and replaces the whole fleet (launch-template version bump
  + NodeLifecycleActions change). Added a fleet-cycle note.
- A custom-bucket stack has none of the four scripts this revision adds;
  updating it fails mount-openzfs-home (TERMINATE) on every replacement =
  replace loop to an empty cluster. Added a sync-before-update prerequisite
  linking DEPLOY-TESTING §2.

Also: fixed the mount-row debug advice (a fleet-wide failure leaves no
surviving node; point to OnError:STOP_SEQUENCE / configure-cloudwatch-logs.sh),
noted the load-bearing S3 read in the PARAMETERS S3BucketName/S3KeyPrefix
rows, and framed the DEPLOY-TESTING sync as an update prerequisite.

* Remove the unused S3BucketName=local template sentinel (PR awslabs#1236 M2)

UseLocalTemplates (S3BucketName='local') switched nested-stack TemplateURLs
to relative paths for an aws cloudformation package local-dev flow. It is
undocumented, has no reference in docs/tests, and dates to the first
reference-cluster commit -- never exercised by this project's workflows,
which sync to a real bucket. It is also now a trap: the sentinel is
forwarded verbatim to the CNG child stacks as their script bucket, so boot
scripts resolve to a literal s3://local/... and fail; with the mounts now
load-bearing (OnError:TERMINATE) that terminates the node instead of
degrading gracefully. cfn package rewrites nested TemplateURLs but not the
runtime script fetch, so 'local' cannot supply boot scripts by design.
Delete the condition and collapse all 7 TemplateURLs to the S3 URL form.

* Verify LDAP admin bind before writing SSM in setup-directory (PR awslabs#1236 M3)

debconf-set-selections seeds slapd's olcRootPW only when apt-get actually
installs slapd. If slapd is already present, the install is a no-op and slapd
keeps its previous admin password, but the script still overwrote SSM with the
newly configured password and swallowed the failing OU-creation binds
(2>/dev/null || true) -- a false success leaving a stored credential that does
not bind. Add an ldapwhoami check after slapd is up: on bind failure, log
loudly and return before creating OUs or overwriting SSM, so a working stored
credential is never clobbered by a non-binding one. Directory action is
OnError:CONTINUE, so this logs and the node continues.

* fix(pcs): match whole fstab line and drop stray mount arg in mount-openzfs-home

grep -qF is a substring match: a commented-out /home fstab line (added by an
operator debugging a hung mount) satisfies the guard, so the active entry is
never re-added. mount -a then no-ops, the script reports success, and the
stash is restored onto — then deleted from — a local /home that the next boot
silently shadows. Use grep -qxF (whole-line match) on both guards so a
commented line no longer counts as present.

Also drop the stray 'defaults' positional from 'mount -a -t nfs defaults'
(introduced with the mount retry loop); mount -a takes no such argument.

* fix(pcs): don't let a failed /fsx chmod terminate a healthy node

mount-lustre-fsx.sh writes no fstab entry, so it re-mounts and re-chmods on
every boot (EVERY_BOOT). Under set -e a failing 'chmod 1777 /fsx' exits
non-zero even when the mount at the previous line succeeded, so OnError:
TERMINATE destroys a node whose /fsx is fine. A chmod failure here is a
shared-side condition (root_squash / read-only FSx / MDS hiccup), never
node-local — terminating just replaces the node into the same failure
(launch/drain/replace loop). Decouple the chmod from the exit status and warn
loudly instead, keeping the healthy node.

Leaves the every-boot chmod itself in place (a safe first-boot-only guard on
shared Lustre is non-trivial given .lustre/lost+found); tracked in review reply.

* docs(pcs): drop removed empty/single-space InstallEnrootPyxis semantics

InstallEnrootPyxis is now AllowedValues ['true','false']; empty and single-space
are no longer legal. Rewrote the three passages that still described them
(OPERATIONS.md 2.1, CUSTOM-AMI.md, PARAMETERS.md 5.3 preamble), naming the
per-template default (deploy-all 'true', add-cng* 'false'). Renamed the
PARAMETERS.md 5.3 heading and the README console label to the actual console
group 'Container Runtime (Enroot/Pyxis)', restoring the mirror-the-console
invariant. The OPERATIONS.md 8 migration row keeps its single-space mention
(correct: it explains the old behavior when migrating off PostInstallScript*).

* feat(pcs): default InstallEnrootPyxis=true in the standalone add-cng templates

The four add-cng*.yaml defaulted InstallEnrootPyxis to 'false' while
deploy-all defaults 'true', so the documented one-click path for adding a
queue to an existing cluster (README Launch Stack buttons) produced nodes with
no container runtime — srun --container-image would fail on the new queue
only, while the README says the runtime is on by default.

Rather than paper over the split with per-template scoping in the docs, make
the default consistent: the container runtime is a headline behavior of this
ML reference architecture, and the installer is idempotent (a fast no-op when
pre-baked into AmiId). Deploy-all already passes the value down explicitly, so
this only changes the standalone add-cng* path. Set 'false' to opt out.

Simplifies the docs that had to name the per-template default (OPERATIONS 2.1,
CUSTOM-AMI); README's unscoped 'on by default' is now accurate everywhere.
All four templates validate; docs lint passes.

* docs(pcs): update in-file comments crediting UserData for the moved interface

Five comments still described the pre-PR UserData env interface, one made false
by this PR's own change (setup-directory.sh:30 said LDAP_ADMIN_PASSWORD is
'auto-generated by UserData' though the script now generates it itself). Swept
all five to describe the current interface: the lifecycle action passes values
positionally (env vars remain for manual / custom-AMI runs), and SlurmVersion
arrives as $1 with PCS_SLURM_VERSION as the fallback.

* test(pcs): widen manifest cross-check to scripts fetched by other scripts

The manifest guard grepped assets/add-cng*.yaml only, so ldap-add-user.sh —
fetched at boot by setup-directory.sh, not via a template ScriptLocation — was
invisible to it. Dropping its manifest entry left both lint-docs.sh and the
staging script green while the object never reached the production bucket, so
the login node's aws s3 cp fails and the cluster comes up without the
ldap-add-user.sh helper USER-MANAGEMENT.md documents (silent degrade).

Scan assets/*.yaml and assets/scripts/*.sh so boot-time fetches from other
scripts are covered too. Verified: normal run still PASS (7/7 present and in
the manifest, no false positives); dropping the ldap-add-user.sh entry now
FAILs as intended.

* fix(pcs): don't silently rotate the LDAP admin password on an SSM read error

setup-directory.sh read the stored admin password with
'aws ssm get-parameter ... 2>/dev/null || echo ""', collapsing every failure
(ParameterNotFound, AccessDenied, throttling) into "nothing stored yet". A
transient read failure during a login-node replacement therefore regenerated
the password, reconfigured slapd, and overwrote SSM with --overwrite while the
user DB on /home/ldap-db persisted — the exact silent rotation the reuse block
exists to prevent.

Capture the exit status (set -e-safe if/else) and branch: reuse a returned
value; keep the freshly generated password only on rc==0-empty or a genuine
ParameterNotFound; on any other error, log loudly and return 1 rather than
regenerate against the persistent DB. The directory action is OnError:CONTINUE,
so the node continues (log-and-continue), consistent with the ldapwhoami bind
check added earlier for the debconf-only-on-fresh-install case.

* docs(pcs): drop non-working 'latest' from MonitoringVersion guidance

raw.githubusercontent.com resolves only real tag/branch refs, so a
MonitoringVersion of 'latest' 404s the post-install.sh fetch and (under
OnError:CONTINUE) leaves monitoring silently uninstalled. Remove 'latest'
from the parameter Description / ConstraintDescription across all five
templates and from PARAMETERS.md; the default stays a real tag (v2.10.2).

* fix(pcs): harden mount + monitoring boot scripts, fix doc anchors

mount-openzfs-home.sh: keep exactly one active /home fstab entry via
ensure_home_fstab_entry (mountpoint-field match, rewrite in place),
tolerate rsync exit 23/24 via safe_rsync so a benign ACL/vanished-file
code can't trip set -e into TERMINATE, and mount /home specifically
instead of mount -a so an unrelated custom-AMI NFS entry can't fail the
action.

install-monitoring.sh: fetch post-install.sh into a mktemp -d workdir
(no fixed /tmp path a dispatched job could race), reconcile the role
header (login=server, compute=exporters), note curl --retry doesn't
retry HTTP 4xx, scope the apt lock-timeout drop-in comment, and chmod
600 the upstream install log to close the Grafana-password exposure on
the multi-user login node.

docs: fix the 3.1 cross-file anchor double-hyphen (README, PARAMETERS)
and note that under CACHE_ONCE a re-sync needs an instance replacement
to reach a running node (DEPLOY-TESTING).

* fix(pcs): preserve existing /home dir modes on first-boot restore

The first-boot /home restore rsynced the node-local snapshot back over the
freshly mounted shared export with `-aA --ignore-existing`. For a directory
that already exists on the shared side (e.g. an admin-tightened /home/ubuntu),
rsync still applied the snapshot's mode/times to it, silently loosening the
shared directory's permissions on the next new node's first boot. Add
`--no-perms --omit-dir-times` so the restore only fills in missing files and
never rewrites the mode/times of a directory that already exists on the export.
`--ignore-existing` still protects existing shared files. (PR awslabs#1236 review #3)

Also:
- docs(OPERATIONS §8): note the standalone add-cng*.yaml default flip to
  InstallEnrootPyxis=true (deploy-all already defaulted true) so migrating
  stacks that relied on the old `false` default aren't surprised.
- docs(README): fix the Test 9 cross-file link (heading lives in
  tests/hpc-efa-test.md, not tests/README.md).
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.

1 participant