Skip to content

feat(aws-pcs): add integrated monitoring, P6 GPU node groups, and container runtime to the PCS reference cluster - #1120

Merged
KeitaW merged 145 commits into
awslabs:mainfrom
DaisukeMiyamoto:deploy-monitoring
Jun 6, 2026
Merged

KeitaW merged 145 commits into
awslabs:mainfrom
DaisukeMiyamoto:deploy-monitoring

Conversation

@DaisukeMiyamoto

@DaisukeMiyamoto DaisukeMiyamoto commented Jun 3, 2026 •

Copy link
Copy Markdown
Collaborator

Purpose

The AWS PCS reference architecture already delivers a one-click (deploy-all),
ready-to-train ML cluster. This PR extends that existing experience with the pieces ML training
on PCS most commonly needs:

  • an integrated Prometheus/Grafana monitoring stack (login node + compute exporters),
  • P6 (B200/B300) GPU node groups with EFA, alongside the existing P5,
  • a container runtime (Enroot/Pyxis) you can pre-bake into an AMI or install at first boot,
  • region-aware FSx performance tiers (so the one-click flow works across more regions), and
  • a verified end-to-end test guide with results from real hardware.

The base also adopts the PCS-ready DLAMI, so the one-click flow reaches a usable cluster
without a required custom AMI build (BuildAMI=true stays available for fastest node boot). All
validated on real hardware (p5 / p6-b200 / p6-b300) before submission.

Relates to: aws-parallelcluster-monitoring PRs #44 (Ubuntu/PCS support), #48 (node-local /opt
install), #49 (DCGM Docker-29.x tag) — all merged upstream; this PR pins to the resulting v2.6.5.


Changes

Major updates

  • Integrated monitoring stack (DeployMonitoring). Prometheus + Grafana + exporters deploy
    to the login node and DCGM/node exporters to compute nodes via the post-install hook. IAM
    (MonitoringPolicy) is built into cluster.yaml; Prometheus scopes discovery by the
    aws:pcs:cluster-id tag and tells login from compute by the monitoring-role tag (the
    dashboards key on a hardcoded instance_name label, not the EC2 Name tag). Pinned to
    v2.6.5 (node-local /opt install fixes the shared-/home stale-file-handle race;
    DCGM tag works on Docker 29.x).
    Optional GrafanaPublicAccessCidr opens 443 on a login-only SG scoped to a CIDR, as an
    alternative to SSM port-forwarding. (Future direction: offer an AWS-managed backend — Amazon
    Managed Service for Prometheus + Amazon Managed Grafana — as an alternative to the self-hosted
    stack on the login node; tracked in docs/ROADMAP.md.)

  • P6 GPU node groups (add-cng-p6-b200.yaml, add-cng-p6-b300.yaml) + P5 multi-NIC EFA.
    New B200/B300 templates with the correct EFA NIC layout; deploy-all auto-selects the P-series
    template by instance type. EFA interface count is auto-derived from the instance type
    (p5/p5e=32, p5en=16, p6-b200=8, p6-b300=16-of-17) — no manual NetworkInterfaceCount.
    CapacityReservationId documented as Capacity-Block-only.

  • Container runtime as a choice (Enroot 3.5.0 / Pyxis v0.20.0). Pre-bake into a custom AMI
    (BuildAMI=true) or install at first boot (PostInstallScriptUrl). The installer is
    externalized to scripts/install-enroot-pyxis.sh (generic post-install hook, not Enroot-specific).

  • One-click / human-friendly deploy-all. Console parameter groups + labels, sensible
    defaults, and auto-resolving AmiId that defaults to the PCS-ready DLAMI from SSM when
    left empty. Building on the PCS-ready DLAMI means the one-click flow needs no custom AMI build
    to get a working cluster — it shortens time-to-first-cluster, while BuildAMI=true remains
    available for those who want Enroot/Pyxis pre-baked for the fastest node boot.

  • README restructure + verified test guide. README leads with one-click ML-training-ready,
    groups config to the console, and links a screenshot. New tests/README.md is a single guide
    with a Verified configurations matrix (real-HW results) plus runnable sbatch/scripts
    (nvidia-smi, NCCL all_reduce, FSDP Llama-2).

Minor updates

  • Region-aware FSx storage tiers. FSx for Lustre / OpenZFS support different deployment types
    and throughput tiers per region, so LustreDeploymentType (PERSISTENT_1/2 + free-form
    throughput) and OpenZFSDeploymentType are now parameters plumbed through from deploy-all.
    Defaults are unchanged; users in regions that lack a given tier can pick a supported one.
  • cluster.yaml: dropped 5 unused params (now just PrivateSubnetId + DefaultSecurityGroupId);
    corrected inaccurate top-level Descriptions across templates.
  • install-enroot-pyxis.sh: wait for apt/dpkg lock + retry apt-get — fixes intermittent
    first-boot exit 100 (unattended-upgrades lock contention).
  • Monitoring post-install retries up to 3× on transient failure (apt lock / NFS settle).
  • Compute-node RootVolumeSize default 300 GiB (Enroot import needs local-disk overlay; can't
    overlay on Lustre); consistent across CNG templates.
  • Docs: split PARAMETERS.md + architecture-components.md into docs/; add docs/ROADMAP.md
    (Multi-AZ prereq, managed monitoring, Trainium, test automation, LDAP/AD, IAM doc, P6e-GB200/300).
  • BuildAMI=true documented to pair with PostInstallScriptUrl="" to avoid a boot-time
    double-install.

Test Plan

Environment:

  • AWS Service: AWS PCS (Slurm 25.11), pcs-ml-cluster-deploy-all.yaml
  • Instance type: p5.48xlarge, p6-b200.48xlarge, p6-b300.48xlarge (Capacity Blocks); c6i (CPU/login)
  • Number of nodes: 2× GPU per family; login + cpu1

Test commands:

# CPU/login + monitoring (BuildAMI=false, Enroot/Pyxis via PostInstallScriptUrl)
aws cloudformation create-stack --stack-name pcs-test \
  --template-url .../pcs-ml-cluster-deploy-all.yaml \
  --parameters ParameterKey=PrimarySubnetAZ,ParameterValue=us-west-2a \
               ParameterKey=DeployMonitoring,ParameterValue=true ...

# Pyxis container + NCCL + FSDP (see architectures/aws-pcs/tests/README.md)
srun --partition=cpu1 --container-image=ubuntu:22.04 bash -c 'echo OK'
sbatch -p gpu-p6-b200 tests/02-nccl-tests.sbatch
sbatch -p gpu-p6-b200 tests/03-fsdp-llama2.sbatch

# Pre-baked AMI path (BuildAMI=true, PostInstallScriptUrl="") — no double-install
aws cloudformation create-stack --stack-name pcs-amibuild-test \
  --parameters ParameterKey=BuildAMI,ParameterValue=true \
               ParameterKey=PostInstallScriptUrl,ParameterValue="" ...

Test Results

Config NCCL all_reduce (2-node peak busbw) FSDP Llama-2 7B
2× p6-b200.48xlarge (16× B200) ~654 GB/s (EFA, found 8 nics, #wrong 0) ~223 TFLOPS/GPU, ~86k tok/s
2× p6-b300.48xlarge (16× B300) ~760 GB/s busbw † ~195 TFLOPS/GPU, ~75k tok/s
2× p5.48xlarge (16× H100) ~480 GB/s (EFA, found 32 nics) —
Login + CPU + monitoring all Grafana targets up; dashboards populate —
BuildAMI=true (custom AMI, PostInstallScriptUrl="") enroot 3.5.0 + Pyxis baked in, no double-install, cpu1 container job clean —

† B300 2-node/16 GiB likely doesn't saturate all 16 EFA cards — flagged in tests/README.md
for a larger-message / more-node re-test before treating ~760 GB/s as peak.

Directory Structure

N/A — this PR updates the architectures/aws-pcs reference architecture (CloudFormation templates,
scripts, docs, validation guide), not a 3.test_cases/ training case.

Checklist

  • I have read the contributing guidelines.
  • I am working against the latest main branch.
  • I have searched existing open and recently merged PRs to confirm this is not a duplicate.
  • The contribution is self-contained with documentation and scripts.
  • External dependencies are pinned to a specific version or tag (no latest) — monitoring v2.6.5, Enroot 3.5.0, Pyxis v0.20.0.
  • A README is included or updated with prerequisites, instructions, and known issues.
  • New test cases follow the expected directory structure — N/A (architecture update, not a 3.test_cases/ case).

Notes for maintainers (deployment / release)

  • Before merge — templates and scripts must be hosted on S3 to deploy. deploy-all uses
    nested stacks, so CloudFormation needs the child templates at an S3 --template-url (a GitHub
    raw URL is not a valid TemplateURL). The install script is fetched over GitHub raw at
    boot, but the templates themselves must be uploaded to a reachable S3 bucket
    (S3BucketName/S3KeyPrefix) to validate or deploy this PR.
  • After merge — please copy the templates to the public bucket. Once merged, sync
    architectures/aws-pcs/assets/*.yaml to the public release bucket
    (s3://awsome-distributed-ai/templates/) so the README's one-click launch links and the
    workshop resolve against the released templates. The PostInstallScriptUrl default and the
    workshop URLs point at aws-samples/awslabs main (raw) and only resolve after this merge.
  • Workshop is already updated to match. The companion workshop
    (aiml-on-aws-parallel-computing-service-pcs) has been updated for these changes (minimal
    cluster.yaml params, auto-resolved AMI, EFA auto-derive, public Grafana access, container
    runtime as a choice, refreshed monitoring/observability + NCCL pages). As soon as this PR is
    merged and the templates are published to the public bucket, we will proceed with publishing
    the workshop update.

- Change FSx Lustre mount permissions from 0777 to 1777 (sticky bit)
- Add upstream repository reference to README
- Add Testing and Validation section documenting tested configurations
- Separate cluster core (Slurm scheduler) from compute nodes
- Use add-cng.yaml for both login and compute node groups
- Make queue creation optional in add-cng templates (empty QueueName for login nodes)
- Remove AmiId parameter from cluster.yaml (only needed for compute nodes)
- Update pcs-ml-cluster-deploy-all.yaml to deploy login and cpu1 as separate nested stacks
- Change default DeployOnDemandCNG to true (cpu1 queue enabled by default)
- Update README with new deployment architecture and examples
- Add AZ_ID variable at the start of each example
- Makes it easier to change AZ consistently across examples
- Aligns with CAPACITY_RESERVATION_ID variable pattern in Example 4
- Add link to aws-parallelcluster-monitoring GitHub repository
- Provides comprehensive monitoring solution for HPC clusters with Prometheus and Grafana
- Clarify what AWS PCS is in the introduction
- Mention it's a fully managed service for HPC with Slurm scheduler
- Include AWS PCS in the list of supported compute platforms
- Update 9.aws-pcs description to clarify it uses Slurm scheduler
Integrates aws-parallelcluster-monitoring stack (Prometheus, Grafana, DCGM)
into PCS cluster deployment with optional, backward-compatible configuration.

Changes:
- Add monitoring-iam-policy.yaml: IAM permissions for CloudWatch, SSM, Pricing API
- Update cluster.yaml: Add RoleName output for IAM policy attachment
- Update add-cng.yaml: Add DeployMonitoring/MonitoringVersion parameters and UserData script
- Update add-cng-p5.yaml: Add monitoring parameters and UserData for P5/P6 nodes
- Update pcs-ml-cluster-deploy-all.yaml: Add MonitoringIAMPolicyStack and pass parameters to all CNGs
- Update README.md: Add monitoring access instructions via Session Manager

Monitoring deployment:
- Login node: Prometheus, Grafana, custom metrics exporters
- Compute nodes with GPU: DCGM exporter (auto-detected)
- Compute nodes without GPU: Node exporter

Key features:
- Backward compatible: DeployMonitoring defaults to 'false' in add-cng.yaml
- Enabled by default in pcs-ml-cluster-deploy-all.yaml (DeployMonitoring='true')
- Installs appropriate components based on node type via post-install.sh from upstream
- Access via Session Manager port forwarding (local:8443 -> remote:443)
- Grafana password stored in SSM Parameter Store
The upstream aws-parallelcluster-monitoring installer hardcodes
PLATFORM_USER=ec2-user for PCS, but Ubuntu 24.04 uses ubuntu
as the default user.

Changes:
- Add symlink workaround in UserData: /home/ec2-user -> /home/ubuntu
- Apply to both add-cng.yaml and add-cng-p5.yaml
- Document known limitation in README.md

The workaround creates a symlink before running post-install.sh,
ensuring the monitoring stack installs to the correct home directory
on Ubuntu-based AMIs.

@KeitaW KeitaW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Few comments

Hit reproducibly during the 4-pattern CPU validation: the very first srun against a
freshly-launched cpu1 node fails with 'Job prolog failed' and drains the node, while
subsequent srun's land on sibling nodes and run cleanly.

Cause is below the templates: PCS bootstrap starts slurmd, then ~9s later systemd
forces a slurmd restart because cgroup-v2 cpuset.cpus/cpuset.mems setup fails ('No
space left on device' is the Linux misleading-error for an empty/invalid cpuset
value). The user srun arrives in that gap, the prolog handshake never reaches the
re-started slurmd, and the controller times out after 420s. Nothing this PR's scripts
modify (cgroup, slurmd unit, post-install timing) is in the path.

Recorded under §7 Known issues with the journal trace and the workaround
('scontrol update nodename=cpu1-N state=resume', or just resubmit and let it land on a
sibling). Recommendations recap renumbered to §8; existing OPERATIONS.md anchors
referenced from README/tests/PARAMETERS are unaffected (#1-#4).
…DIA HPC SDK, modules)

The cluster currently ships only the PCS DLAMI's pre-installed CUDA/NCCL/EFA stack
plus Enroot/Pyxis for containers — no native HPC package manager and no first-class
hook for traditional toolchains (Intel oneAPI, NVIDIA HPC SDK). Track these as future
work so users running source-built MPI/BLAS/scientific apps or Fortran workloads have
a documented path; once Spack lands, an Lmod-style module system rounds it out.
…e rationale

Two pending edits:

- §4 'Most-used parameters' table didn't list SlurmVersion. It became a structural
  parameter this round (drives Slurm OpenMetrics availability, the Pyxis build
  version, and the AMI's baked Slurm) and deserves a row alongside BuildAMI etc.
  Cross-references OPERATIONS.md §1 for the full version trade-off.
- §4 Container runtime: spell out what the in-stack BuildAMI=true template earns
  beyond a one-shot Image Builder run (managed pipeline: scheduled rebuilds, AMI
  lifecycle deprecation, SSM parameter publishing of the latest AMI ID), so the
  rationale matches Reply F to KeitaW's follow-up review and the next reviewer
  doesn't re-litigate it.
…ion)

Two small clarity fixes:

- 'Flexible capacity' was ambiguous — could be read as 'flexible WRT capacity'
  rather than 'supports a wide range of capacity-purchase options'. Reword to
  'Broad capacity-purchase support', explicit about covering OD / ODCR / Capacity
  Blocks for ML, and selected per node group.
- 'One-click or modular' duplicated the first bullet's one-click claim. Drop the
  one-click half and lead with the value of the modular path: composing individual
  stacks for infrastructure reuse / iterating on one piece at a time.
… of inline heredoc

The §7 'Running a multi-node GPU job' walk-through inlined a hand-rolled
nccl-test.sbatch heredoc, which duplicated the canonical launcher at
micro-benchmarks/nccl-tests/slurm/nccl-tests-container.sbatch and could drift from it.
Same reuse rule we already applied to tests/ (where 02-nccl-tests.sbatch was deleted
and Test 6 redirected to the canonical asset): document only the PCS-specific deltas
(login-node import + GPU partition name) and let the canonical sbatch carry the rest.
Also pins the NCCL image tag instead of using mutable latest. Pointer to the full
Test & Validation Guide added at the bottom.
…ed GPUs

The empty default pushed B300 users into a forced manual override (and a Grafana that
quietly stays empty until they figure that out). DCGM 4.5.2 covers Hopper / B200 / B300
with no DCGM_FI_DEV_* field changes vs 4.2.0 per upstream changelog, was validated on
real B300 hardware in this round, and the digest pull bypasses the Docker-29.x OCI-index
failure on newer NVCR tags. So default to it across the GPU range — every supported
family populates Grafana out of the box. The monitoring stack's older 4.2.0 pin remains
a one-line override for anyone who needs to match another fleet.

Templates (deploy-all + 4 CNGs): Default '' -> the validated digest, parameter
description rewritten to lead with the new behavior. README §8 / §4, PARAMETERS,
tests/README, OPERATIONS.md §3.1 (renamed: 'B300 needs DcgmExporterImage' ->
'DcgmExporterImage — the default, and when to change it') all updated; cross-references
re-pointed at the new anchor.
Two leftovers from the dev / review cycle that shouldn't ship to awslabs/main:

- README §8 'Prefer AWS-managed Prometheus/Grafana?' linked to
  github.com/DaisukeMiyamoto/awsome-distributed-ai/tree/deploy-monitoring/...
  (a fork URL on the in-flight branch). The path lives on awslabs/main already, so
  switch to a repo-relative link that resolves correctly under any fork/branch and
  doesn't bake the dev fork name into mainline docs.
- The 4 add-cng*.yaml templates' MonitoringRepo description carried the example
  'e.g. DaisukeMiyamoto/aws-parallelcluster-monitoring' as a 'how to test unreleased
  changes' hint. Defaults are correct (aws-samples/...); drop the personal-fork
  example from the prose so there's no leftover advertisement of an internal fork.
The Parameters/AWS::CloudFormation::Interface label still said
'(empty = default; set for B300)' from the era when the param defaulted to ''
and B300 users had to set it explicitly. Now that the default is the DCGM 4.5.2
digest covering Hopper/B200/B300 out of the box, the old label misleads. Update
to '(default DCGM 4.5.2 by digest; covers H100/B200/B300)'.
@DaisukeMiyamoto

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough, hardware-validated review — it caught real bugs. The structural and guided-path issues are all fixed on deploy-monitoring and re-validated end-to-end across the matrix at the top of tests/README.md: clean first-boot deploys on Slurm 25.05 and 25.11 (CPU + Pyxis), the BuildAMI=true path, and a 2× p6-b300 GPU run (NCCL/FSDP, monitoring v2.9.1). Rundown of the threads not answered individually below:

Repo hygiene / test duplication

  • 01-nvidia-smi.sbatch (one-liner): removed the file; Test 5 now shows the interactive srun … nvidia-smi one-liner instead. (91c0774)
  • 02-nccl-tests.sbatch duplicates micro-benchmarks/nccl-tests: removed; Test 6 now points at the canonical slurm/nccl-tests-container.sbatch and documents only the PCS deltas (login-node import, partition name). (91c0774)
  • 02-import-nccl-image.sh defaults to latest: removed with the script; the README example now pins a specific image tag. (91c0774)
  • 03-fsdp-llama2.sbatch duplicates 3.test_cases/pytorch/FSDP: removed; Test 7 now references the canonical FSDP case (create_venv.sh + llama2_7b-training.sbatch) with only the PCS deltas (venv/HF cache on /fsx, 2 nodes). (91c0774)

Boot-time / guided-path correctness

  • PostInstallScriptUrl wrong org (404): fixed aws-samples → awslabs in the default and grepped the repo for the same wrong-org URL. (d52c7ee)
  • P6 templates accept any InstanceType: added single-value AllowedValues to the p6-b200 and p6-b300 templates (p5 already had it), so a mismatched type fails at stack validation. (83d84df)
  • FSx Lustre throughput invalid for the PERSISTENT_1 fallback: the throughput params are now String + AllowedValues (the full valid grid), and a CloudFormation Rules section asserts the (deployment-type, throughput) pair at stack-create time — so a mismatch fails immediately with a clear message instead of deep in the nested FSx stack. Defaults unchanged. (bb32202)
  • FSx OpenZFS same coupling defect: same fix (Rules + AllowedValues) for HomeThroughput vs OpenZFSDeploymentType. (bb32202)
  • Shared ENROOT_CACHE_PATH breaks the 2nd user: made the cache per-user (/tmp/enroot/cache/user-$(id -u)), matching the data/runtime paths. (d52c7ee)

Beyond the original review (surfaced while validating the above on real hardware):

  • MonitoringVersion default → v2.9.1: carries the PCS /opt install fix, Docker-29.x DCGM tag, Grafana 13, and the upstream DCGM_EXPORTER_IMAGE support I added for B300 (Fix readme typo and formatting #51). (13b3781)
  • SlurmVersion flows everywhere it's needed: deploy-all → CNG UserData (as PCS_SLURM_VERSION) → install-enroot-pyxis.sh, and deploy-all → DLAMI build. The Pyxis SPANK plugin ABI is locked to its Slurm version, so the AMI is single-Slurm-version on purpose; both paths now build Pyxis only for the cluster's version. (87ebf94, 6e2d37c, 7bdd26e)
  • MetricsType is 25.11+ only: cluster.yaml was emitting it unconditionally, which made SlurmVersion=25.05 clusters fail to create. Gated on 25.11. (0f96116)
  • New docs/OPERATIONS.md: consolidates the operational caveats above (plus AMI pinning, FSx deployment-type/throughput coupling) so the README doesn't grow each time we hit one. (aef17d3, 1e401f9)

Each is linked to its commit above; happy to expand on any. The four threads with a design choice or a deeper finding I've answered in-thread.

@DaisukeMiyamoto

Copy link
Copy Markdown
Collaborator Author

Re: Do we need a separate add-cng-*.yaml per GPU type?

Thanks for laying out both options so concretely. I weighed them and kept the three files split, but addressed the two things you flagged (the drift, and documenting the decision).

Why split: Option A (Fn::ForEach + AWS::LanguageExtensions) needs CAPABILITY_AUTO_EXPAND at create time, which breaks the README/workshop one-click quick-create links — core to this PR's one-click goal. Option B (one file, no transform) means ~32 per-card !If wrappers plus card-0/DeviceIndex branching to cover b300's genuinely different topology (ENA-only card 0 + EFA on cards 1-16 at DeviceIndex 0, vs card 0 = EFA / DeviceIndex 1 on p5/b200) — and editing the already-validated p5/b200 NIC blocks. Given b300 is the odd one out and each flat list is trivially diffable against the EC2 docs, I judged split as the safer, more readable call.

What I changed (83d84df): documented the rationale in a header comment on all three templates; added the Fn::ForEach consolidation to docs/ROADMAP.md (with your AUTO_EXPAND caveat); and fixed the day-1 drift — the p6 templates now gate InstanceType with AllowedValues like p5. Happy to revisit Option A as a follow-up if the quick-create-link concern is resolved.

@DaisukeMiyamoto

Copy link
Copy Markdown
Collaborator Author

Re: removing the custom AMI-build path

Thanks for the prompt-and-press on the structural cost — the double-maintenance during this round on PCS_SLURM_VERSION and the single-version Pyxis pin was real, and you're right that the AMI's origin reason (no vendored PCS DLAMI) is gone now that the templates resolve the official dlami-base-ubuntu2404 parameter directly.

That said, I'd like to keep the path, for two reasons:

  1. Both pre-bake and first-boot install are independently useful. ParallelCluster keeps the same pair (CustomAmi + OnNodeConfigured/OnNodeStart) in its mainline today, even with an official PC-vendored AMI available — because the trade-off is real: pre-bake gives faster boots and deterministic state for fleets that scale frequently, first-boot install gives easy iteration on the customization itself. PCS has the same use cases. Removing the pre-bake path now would close that door for users who'd choose it on the same grounds PC users do; if anything the precedent suggests we should keep it for the same lifecycle.
  2. The in-stack template earns its complexity vs. a one-shot build. What it adds beyond an ad-hoc aws imagebuilder start-image-pipeline-execution is the managed pipeline: scheduled rebuilds (BuildSchedule=Weekly/Monthly), an AMI lifecycle policy that deprecates older AMIs on a configurable window, and SSM parameter publishing of the latest AMI ID. For users with a steady stream of clusters off the same PCS-tuned base, that's the difference between a CFN stack you forget about and a hand-rolled CI job with the same deprecate/notify story.

Concretely for the next reviewer: README §Container runtime now states why the in-stack build exists (managed pipeline + schedule + lifecycle + SSM publishing); OPERATIONS.md §2 already documents the single-Slurm-version semantics — that was the part of the recent hardening that doubled up with the script, so it's where the duplication tax landed.

If the pipeline parts turn out not to be used in practice, decoupling (or removal) in a follow-up PR is on the table — but I think removing it pre-merge here, before users have had a chance to opt in, would foreclose the option prematurely.

The README's Additional Resources list and ROADMAP's User-management item both
pointed at `6.ldap_server` / `architectures/6.ldap_server` -- but the actual
upstream layout puts it under `1.architectures/6.ldap_server`. From
`architectures/aws-pcs/` that resolves through a `../../1.architectures/`
relative path. Update both refs so the link doesn't 404 on the rendered PR.
… slim descriptions

Console parameter groupings reorganized for the 1-click case:

- New §3 'Container Runtime (Post-install Script)' carries just
  PostInstallScriptUrl/Args + RootVolumeSize -- the hook every cluster goes
  through. Previously these were buried alongside Image Builder params, which
  most users do not touch.
- AMI build params (BuildAMI/BaseAmiId/SemanticVersion/BuildSchedule) moved to
  a new §7 'Custom AMI Build (Optional - skip unless you need a pre-baked DLAMI)',
  ahead of the Developer group. They are an opt-in path; surfacing 'Optional'
  in the section title makes that obvious instead of confronting first-time
  users with SemanticVersion/BuildSchedule with no context.
- §2 PCS Cluster: SlurmVersion now precedes LoginNodeInstanceType. The Slurm
  major is the substantive cluster-shaping choice; the login type is rarely
  changed from the default.
- DcgmExporterImage was orphaned (not in any ParameterGroup, so the console
  rendered it under a generic 'Other' heading). Now grouped under §8 Developer/
  Advanced next to MonitoringVersion / MonitoringRepo where it belongs.

Description cleanups -- the long help blocks rendered as a wall of text in the
console; trimmed to the parameter-level essentials and pointed at OPERATIONS.md
for the rest:

- PostInstallScriptUrl: 12 lines -> 5 (kept the BuildAMI=true interaction note,
  dropped the duplicated 'PCS equivalent of OnNodeConfigured' phrasing).
- GrafanaPublicAccessCidr: 14 lines -> 5 (kept the unauthenticated-proxy-paths
  warning since that is the real footgun, dropped the per-network detail that
  belongs in OPERATIONS.md §3.2).
- BuildAMI: 4-line one-paragraph -> 4 short lines covering the trade-off, the
  PostInstallScriptUrl pairing, and the single-Slurm-version constraint.
- DcgmExporterImage: 6 lines -> 4 -- focused on 'why digest, not tag' since
  that's the non-obvious part. Migration / override examples stay in
  OPERATIONS.md §3.1.
- PerUnitStorageThroughput / HomeThroughput: tightened to call out that the
  valid grid is enforced by a Rule, since users see the validation error first.
- PrimarySubnetAZ: rewrote from 'AZ id where the subnets will be created' to
  'the only required choice' to match the 1-click framing.

ParameterLabels updated for the params whose section moved (AMI section now
flagged 'optional; auto-resolved if empty' on BaseAmiId so first-time users
know to leave it alone). CapacityReservationId label spelled out as 'Capacity
Block for ML reservation ID' so it reads correctly in the console even though
the parameter name itself is the generic 'CapacityReservationId'.

PARAMETERS.md ToC restructured to mirror the new 8 console groups; README §4
text 'all 7 console parameter groups' bumped to 8.

No default values changed. Validate-template still reports 42 parameters, no
orphans, all relative cross-links resolve.
… PCS Cluster

Two follow-ups on the 1-click parameter UX:

- Parameter Descriptions render as plain text in the CloudFormation Console --
  'Details: docs/OPERATIONS.md $X' is not clickable, just visual noise. Removed
  the docs pointers from BuildAMI / GrafanaPublicAccessCidr / DcgmExporterImage
  descriptions and folded the actually load-bearing fact back into the prose
  itself: BuildAMI gains a one-liner 'AMI is bound to the cluster's SlurmVersion
  (Pyxis ABI is locked to it)' so users do not need to chase the link to know
  why; DcgmExporterImage gains 'No effect on CPU nodes' as a guardrail; the
  GrafanaPublicAccessCidr description was already self-contained, so just trims
  the link. Long-form rationale stays in OPERATIONS.md, reachable from the
  README / PARAMETERS.md tables which DO render as links.

- RootVolumeSize moved out of '3. Container Runtime (Post-install Script)' and
  into '2. PCS Cluster Configuration'. The volume size applies to every node
  group (login + compute + AMI build), not the container runtime, so grouping
  it with the Pyxis hook implied a wrong scope. PARAMETERS.md table mirrors the
  move. Within $2 it now sits next to LoginNodeInstanceType, both being
  node-shape choices.
Console parameter labels (and PARAMETERS.md / README mentions) said 'FSx Lustre'
and 'FSx OpenZFS', which is not the AWS service name. Use 'FSx for Lustre' and
'FSx for OpenZFS' consistently. Also tag the filesystem each parameter governs
with its mount path -- '/fsx' for Lustre, '/home' for OpenZFS -- so a user
scanning the form sees at a glance which filesystem each row applies to (the
prior labels only tagged OpenZFS with '(home)' since that was the more obvious
'why this default'; symmetric tagging is clearer).

- pcs-ml-cluster-deploy-all.yaml: ParameterLabels for the 8 FSx params, and
  the section title for group 6, both updated.
- ml-cluster-prerequisites.yaml: 'Fsx Lustre storage size' / 'Fsx OpenZFS' in
  ParameterGroups + 3 stack-output Descriptions normalized.
- docs/PARAMETERS.md: section 6 heading + each parameter purpose column.
- README.md / docs/ROADMAP.md: stray 'FSx Lustre' prose mentions corrected.

No parameter names, defaults, or behavior change.
…er parameter

Per maintainer feedback, deploy-all no longer drives the in-stack ImageBuilder
build. Pre-baking Enroot/Pyxis is now an opt-in path you run separately, and the
result feeds back as a normal AMI ID -- the same shape as 'pin to the latest PCS
DLAMI' that production deploys want anyway. This removes the double-maintenance
on PCS_SLURM_VERSION / single-version Pyxis that lived on the in-stack path.

Cluster template (pcs-ml-cluster-deploy-all.yaml):
- DROP parameters: BuildAMI, BaseAmiId, SemanticVersion, BuildSchedule (4 of 42).
- DROP nested DLAMIStack and the BuildAMIEnabled Condition.
- ADD AmiId parameter under $2 PCS Cluster Configuration. Empty (default)
  auto-resolves to the latest PCS-Ready Deep Learning AMI from the SSM public
  parameter; explicit ami-xxx pins for production / custom AMIs. Each CNG's
  AmiId is now a plain !Ref AmiId -- the CNG templates already handle empty as
  SSM-resolve internally, so no second branch is needed at the deploy-all level.
- AllowedPattern enforces ami-xxx format on non-empty input.
- ParameterGroups collapses from 8 to 7 (the old 'Custom AMI Build' section is
  gone).

Standalone DLAMI template (pcs-ready-dlami-with-enroot-pyxis.yaml): unchanged --
already had its own SlurmVersion/BaseAmiId/SemanticVersion/BuildSchedule
parameters and DLAMIforPCSAmiId output. Now used as a separate stack rather
than nested.

README:
- New $9 'Pre-baking Enroot/Pyxis into a custom AMI (optional)' walks through
  the 3-step flow: build the AMI separately, read DLAMIforPCSAmiId, pass as
  AmiId to the cluster (with PostInstallScriptUrl='' for the cleanest boot).
- New 'AMI selection (AmiId)' subsection under $4 explains the SSM auto-resolve
  default (PCS-Ready Deep Learning AMI) and when to pin.
- $1 Key Features tip rewritten to point at the new $9 path instead of
  BuildAMI=true.
- Most-used-parameters table swaps the BuildAMI row for an AmiId row.

PARAMETERS.md: $3 Custom AMI Build group removed (down to 7 sections matching
the console). $2 PCS Cluster Configuration gains an AmiId row spelling out the
SSM auto-resolve default and the production pin.

OPERATIONS.md: $2 'Container runtime' rewritten -- 'PostInstall vs. AMI build'
becomes 'PostInstall vs. pre-baked AMI', and the section opens with 'they are
decoupled: the cluster stack does not run Image Builder.' Recommendations recap
$8 swaps the BuildAMI=true bullet for 'pre-bake separately and pass as AmiId'.

tests/README.md: matrix and verified-configs rows updated to express the
pre-baked path as 'build AMI separately, deploy cluster with AmiId=ami-xxx +
PostInstallScriptUrl=""'. Stack creation times split AMI build (~30m one-time,
separate stack) from cluster create (~25m), since they are no longer
serial/parallel inside one stack.

install-enroot-pyxis.sh: header comment about idempotency rewritten -- it used
to reference BuildAMI=true nodes; now talks about 'pre-baked AMI' generically.

CLAUDE.md: template inventory now flags pcs-ready-dlami-with-enroot-pyxis.yaml
as the standalone DLAMI template (no longer 'optional Image Builder
(BuildAMI=true)'). Defaults table swaps BuildAMI row for AmiId row. Common
issues entry on Pyxis ABI mentions the standalone DLAMI template, not BuildAMI.

Anchor slug for the new $9 README section is
'awslabs#9-pre-baking-enrootpyxis-into-a-custom-ami-optional' -- callers in README
intra-links and PARAMETERS.md cross-link both use it.

validate-template: deploy-all 39 params (was 42, -3 net), DLAMI 8 params
(unchanged), prereqs 11 (unchanged). All relative-path links resolve. Section
slugs match anchor refs.
The decoupling commit moved the AMI build out of deploy-all into a separate
standalone stack, but tests/README still bundled both paths under Test 2 (with
'(a) first-boot' and '(b) BuildAMI=true' branches), which is no longer how the
flow looks to a tester. Match the new template structure:

- Test 2 is now 'Enroot/Pyxis container runtime (first-boot install)' -- ONLY
  the default path (PostInstallScriptUrl on a cluster with no AmiId override).
  No more (a)/(b) split. Anchor: #test-2-enrootpyxis-container-runtime-first-boot-install.
- Test 8 'Pre-baked AMI build (standalone DLAMI template)' is new and stands on
  its own. It walks the actual flow now: build the AMI as a separate stack via
  pcs-ready-dlami-with-enroot-pyxis.yaml, read DLAMIforPCSAmiId, deploy a
  cluster pinned to it with AmiId=ami-xxx + PostInstallScriptUrl='', verify the
  Pyxis SPANK plugin matches SlurmVersion, run a container job, clean up.
- Pre-merge matrix row 2 splits into row 2 (default first-boot path) and a new
  row 8 (pre-baked AMI path, run only when pcs-ready-dlami-with-enroot-pyxis.yaml
  or scripts/install-enroot-pyxis.sh actually changes -- the dependency the
  decoupling makes explicit). Coverage matrix gains the same row.
- Test 2's regression-test rule for install-enroot-pyxis.sh keeps the bullet
  about retesting the AMI path, but rewords 'BuildAMI=true' as 'pre-baked AMI
  path (Test 8)' and adds the explicit 'rebuild the AMI per supported
  SlurmVersion' instruction (the script change isn't in the AMI until you do).

The Test 8 walkthrough uses the test bucket's URL (midaisuk-llm-dev) since it's
a pre-merge testing doc; that matches the deploy-all how-to elsewhere in the
file. SlurmVersion is parameterized so a tester runs the same block twice for
25.05 and 25.11.

No code/template changes -- this commit is docs-only.
…rasing

Two cleanups, no behavior change:

1. Naming consistency for the AWS-managed AMI. Across docs, the template prose
   was using three forms inconsistently:
     - 'PCS-ready DLAMI' (lowercase r)
     - 'PCS DLAMI' (no -Ready)
     - 'PCS-Ready Deep Learning AMI' (full form)
   Standardize on 'PCS-Ready DLAMI' for short references and keep
   'PCS-Ready Deep Learning AMI' for the first-mention long form. The official
   AWS docs use the latter; the short form picks the version that capitalizes
   'Ready' to match. README/PARAMETERS/OPERATIONS/ROADMAP/CLAUDE.md, all four
   add-cng templates, the standalone DLAMI template, and install-enroot-pyxis.sh
   header are all aligned. SSM parameter paths
   (/aws/service/pcs/ami/dlami-base-ubuntu2404/...) and CFN parameter / output
   names are unchanged -- those come from PCS itself.

2. deploy-all top-of-file Description still said 'optional custom AMI building'
   from when DLAMIStack was nested inside it. With the AMI build decoupled, the
   Description now lists only the things deploy-all actually orchestrates
   (Prerequisites + cluster + CNGs) and points at pcs-ready-dlami-with-enroot-pyxis.yaml
   as the separate stack to run for a pre-baked AMI, with a one-line on how to
   wire the result back via AmiId.

Audit results from this sweep (all clean):
- No 'BuildAMI' / 'SemanticVersion' / 'BuildSchedule' / 'BaseAmiId' refs anywhere
  outside the standalone DLAMI template (which legitimately owns those params).
- No 'in-stack AMI build' / 'nested DLAMI' / 'managed pipeline ... deploy-all'
  phrasing remains.
- All relative-path links in README/docs/tests resolve.
- All in-doc anchor refs match real section slugs (READMEs and OPERATIONS).
- Param-group count claim in README updated previously to 'all 7'.
- All 8 templates pass validate-template (39/8/11/24/24/24/24/7 params).
@DaisukeMiyamoto

Copy link
Copy Markdown
Collaborator Author

Re-considered the AMI-build path — decoupling done

Thanks for the prompt-and-press on this; revisiting it I came around to your view. The double-maintenance you flagged was the right reason to act, and walking through the full template, ParametersGroups, and the ParallelCluster precedent again, I no longer think keeping the in-stack build pulls its weight when the same outcome is just one extra step (build the AMI separately, pass ami-xxx as AmiId).

So I went ahead and pulled the AMI-build path out of deploy-all:

Decoupled

  • Dropped 4 parameters from pcs-ml-cluster-deploy-all.yaml: BuildAMI, BaseAmiId, SemanticVersion, BuildSchedule. Also dropped the BuildAMIEnabled Condition and the nested DLAMIStack resource. Each CNG's AmiId is now a plain !Ref AmiId — the CNG templates already handle empty as SSM-resolve internally, so no second branch is needed at the deploy-all level.
  • Added a top-level AmiId parameter under §2 PCS Cluster Configuration (rather than tucked under "AMI build"), since this is also the natural production knob — empty resolves the latest PCS-Ready DLAMI from SSM, explicit ami-xxx pins it. This is the same shape "pin the AMI for production" already wanted independent of any pre-bake.
  • The standalone pcs-ready-dlami-with-enroot-pyxis.yaml template stays intact (single-Slurm-version Pyxis, optional schedule/lifecycle/SSM-publish features). It's now what you run separately to get a custom AMI; you read its DLAMIforPCSAmiId output and pass it back to the cluster as AmiId. (88da755)

1-click parameter UX, while at it

  • ParameterGroups went from 8 to 7 sections, and the order/labels were tightened for the default 1-click case: SlurmVersion first in §2 (the substantive cluster choice), LoginNodeInstanceType after it, RootVolumeSize moved out of the container-runtime section into §2 PCS Cluster Configuration since it applies to every node group, not just the post-install hook. Long descriptions trimmed (PostInstallScriptUrl, GrafanaPublicAccessCidr, BuildAMI, DcgmExporterImage); FSx labels tagged with the mount path (/fsx, /home) so it's obvious which filesystem each row governs; DcgmExporterImage was orphaned (not in any ParameterGroup) and got grouped under §7 Developer/Advanced. (5bc200a, 2c295cc, 709b85a)
  • CapacityReservationId's description and console label both spell out "Capacity Block for ML reservation ID" explicitly, with the warning to not put an ODCR ID there, since the parameter name itself is the generic one.

Docs and tests follow the new structure

  • New README §9 "Pre-baking Enroot/Pyxis into a custom AMI (optional)" walks the 3-step flow: build the AMI as a separate stack → read DLAMIforPCSAmiId → deploy the cluster with AmiId=<ami-xxx> + PostInstallScriptUrl="" for the cleanest boot. Each step has a copy-pasteable aws cloudformation block.
  • tests/README.md Test 2 now covers only the default first-boot install path; the pre-baked-AMI flow is a new Test 8 that walks the standalone DLAMI build → cluster pin → verify, run per supported SlurmVersion (Pyxis SPANK plugin ABI is version-locked, so testing 25.05 and 25.11 separately is the only way to catch wrong-version spank_pyxis.so). The pre-merge matrix gains a row 8 you skip when neither pcs-ready-dlami-with-enroot-pyxis.yaml nor scripts/install-enroot-pyxis.sh changed. (c068374)
  • OPERATIONS.md §2 rewritten — the section header is now "PostInstall vs. pre-baked AMI" and the opening line is "they are decoupled: the cluster stack does not run Image Builder." PARAMETERS.md ToC mirrors the new 7-section console layout. README most-used-parameters table swaps the old BuildAMI row for an AmiId row.
  • One consistency sweep on top: the AMI is referred to as "PCS-Ready DLAMI" (short) / "PCS-Ready Deep Learning AMI" (long) consistently across every doc and template, after finding three different spellings in flight. (39d02a2)

Quick links to the structural commits, if useful for review: 88da755 (decoupling), c068374 (Test 8 split), 5bc200a (1-click groupings).

The cluster stack is now genuinely just "Prerequisites + cluster + CNGs", which is what deploy-all should mean. Defaults still produce the same fast-path one-click that worked before — the difference is only visible if you previously set BuildAMI=true, in which case the README §9 walkthrough is the substitute (and shorter than expected, since the 3 steps are basically what the in-stack version was doing anyway).

@DaisukeMiyamoto
DaisukeMiyamoto requested a review from KeitaW June 5, 2026 08:39

@KeitaW KeitaW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review (round 2)

This is an exemplary response round: 40 commits since round 1 resolving 16 of 17 prior findings, several of them more thoroughly than asked — FSx throughput/type coupling became CloudFormation Rules assertions (fail-at-create with clear messages), the AMI build was decoupled from deploy-all into a standalone path with AmiId promoted to a single top-level parameter, the B300 metrics gap became a DcgmExporterImage parameter defaulting to a DCGM 4.5.2 digest (plus upstream v2.9.1 support), and all four duplicated test scripts were deleted in favor of the canonical micro-benchmarks/nccl-tests and 3.test_cases/pytorch/FSDP assets. The one partial carry-over is the Grafana CIDR pattern (now a documented deliberate accept for 0.0.0.0/0, but still missing octet validation), and I have three minor new notes on content added this round. Nothing blocking remains from my side.



Resolution Status of Prior Findings

# Finding (round 1) Status
1 Separate add-cng-*.yaml per GPU instance type ✅ Resolved — took the documented-split option: each template's InstanceType now explains the per-family NIC lock-in, and OPERATIONS.md §6 records the rationale
2 01-nvidia-smi.sbatch → srun one-liner ✅ Resolved — file deleted; tests/README shows the interactive one-liner
3 NCCL test duplicates micro-benchmarks/nccl-tests ✅ Resolved — scripts deleted; tests/README now references the canonical sbatch (9 canonical-asset references)
4 02-import-nccl-image.sh defaults to latest ➖ N/A — file deleted with the reuse change
5 FSDP test duplicates 3.test_cases/pytorch/FSDP ✅ Resolved — deleted; canonical test case referenced
6 PostInstallScriptUrl wrong org (404) ✅ Resolved — default now awslabs/...@main (d52c7ee); fork refs stripped pre-merge (0af2621); resolves on merge
7 BuildAMI double-install unguarded ✅ Resolved — installer is now idempotent (per-component version-skip), and the param description documents the no-op; the AMI build decoupling removes the interaction entirely
8 p6 templates missing AllowedValues ✅ Resolved — both templates locked, with the NIC-topology rationale in the description (83d84df)
9 FSx Lustre throughput/type coupling ✅ Resolved — Rules/Assertions validate P1 (50/100/200) and P2 (125/250/500/1000) at create time; stronger than the suggested description fix
10 FSx OpenZFS throughput/type coupling ✅ Resolved — same Rules pattern for both the 64-grid (SINGLE_AZ_1/HA_1) and 160-grid (SINGLE_AZ_2/HA_2) families
11 Shared ENROOT_CACHE_PATH multi-user break ✅ Resolved — cache is now per-user (/tmp/enroot/cache/user-$(id -u), d52c7ee)
12 Duplicate PATH= append / slurmd restart context ✅ Resolved — idempotent writes, PCS_SLURM_VERSION-derived single-version PATH, and the manual-run caveat documented in the script header
13 Grafana CIDR accepts 0.0.0.0/0 🟡 Partial — exposure now fully disclosed and 0.0.0.0/0 documented as a deliberate PoC/workshop accept (OPERATIONS §3.2); the pattern still passes invalid octets — see the typo-guard note below
14 443 exposes unauthenticated Prometheus/Pushgateway ✅ Resolved (as documentation) — parameter description now names the unauthenticated proxy paths explicitly; OPERATIONS §3.2 covers it; the auth fix itself remains an upstream matter
15 B300 GPU dashboards won't populate (DCGM 4.2.0 pin) ✅ Resolved — DcgmExporterImage parameter, default DCGM 4.5.2 by digest (Docker-29.x-safe), MonitoringVersion bumped to v2.9.1 which honors the override (upstream #50)
16 Doc drifts (tag mechanism, AMI pinning, 24.11 scope) ✅ Resolved — AMI-pinning tip (OPERATIONS §4), 24.11 out-of-scope note, naming consistency sweep
17 Follow-up: remove/decouple the custom AMI-build path ✅ Resolved — Option B adopted: DLAMIStack + 4 parameters removed from deploy-all, the template is a documented standalone path with its own test flow (Test 8), and AmiId is one top-level parameter

Legend: ✅ Resolved · 🟡 Partial · ❌ Open · ➖ N/A (scope changed)



Things That Look Great

  • The FSx Rules section — assert-at-create with explicit valid-value lists and human-readable AssertDescriptions is better than the coupling fix I suggested, and it establishes a validation pattern the template family can reuse.
  • The AMI-build decoupling landed cleanly: deploy-all shrank by ~150 lines, AmiId is one comprehensible top-level parameter ("empty = latest PCS-Ready DLAMI; pin in production"), and the standalone path kept its own test flow (Test 8) rather than losing coverage.
  • The DcgmExporterImage solution is better than what I proposed — a parameter with a digest default plus upstream override support (v2.9.1, #50) fixes B300 and gives users an escape hatch for future DCGM/Docker incompatibilities, instead of hard-pinning anything.
  • Deleting all four test scripts in favor of canonical assets was decisive — tests/README now teaches with references instead of copies, which is exactly the "reuse existing assets" ideal.
  • The installer's idempotency design (per-component version-skip, documented no-op on pre-baked AMIs) turned the double-install foot-gun into a non-issue without any template conditional.
  • The cgroup-v2 known-issue entry in OPERATIONS.md — documenting a race you found in your own validation, with the journalctl evidence and recovery command, is the kind of operational content most reference architectures never ship.
  • The personal-fork reference sweep (0af2621) before merge — small thing, often forgotten.

Sources

  • Resolution verification at head 39d02a20cbc735a4932342b8d6207b97503c5dd8: pcs-ml-cluster-deploy-all.yaml (Rules section, parameter blocks), install-enroot-pyxis.sh (idempotency, per-user cache, PCS_SLURM_VERSION), add-cng-p6-b200.yaml (AllowedValues), OPERATIONS.md, tests/README.md
  • Round-1 evidence base (EFA topology, live-cluster verification, DCGM/NVCR analysis): see round-1 reviews linked above
  • arm64 digest for the dcgm-exporter note: NVCR manifest index for 4.5.2-4.8.1-ubuntu22.04 (inspected 2026-06-04)

Comment thread architectures/aws-pcs/assets/pcs-ml-cluster-deploy-all.yaml Outdated
newer NVCR tags). Override to pin a different build (also use a digest).
No effect on CPU nodes.
Default: 'nvcr.io/nvidia/k8s/dcgm-exporter@sha256:a7ad6547d4546eaf4dd5d6b4c0b4db4101e63ef7dc3cdff7f42b767d2c60b706'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

DcgmExporterImage digest default: note it's the linux/amd64 manifest

Glad the digest route landed, and pairing it with upstream v2.9.1's override support is the right shape. One small completeness note for the description: sha256:a7ad6547… is the digest of the linux/amd64 platform manifest specifically (the arm64 sibling is sha256:9b3969a9…). All instance types this template set deploys are x86_64 today, so nothing is broken — but since the parameter invites overrides "also use a digest", a parenthetical "(amd64 digest; arm64 nodes would need the matching arm64 digest)" would save the first Grace-based user a confusing pull failure if/when the ROADMAP's P6e-GB200/GB300 item lands.


`Job prolog failed`, the node enters `drained` state with `Reason=Prolog error`, and the
job is cancelled. The next `srun` (which lands on a sibling node) works.

**Cause.** A systemd / cgroup-v2 race during PCS bootstrap, observed on the PCS-ready

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

cgroup-v2 prolog race: great known-issue entry — add a tracking pointer if one exists

The new known-issue entry is exactly the kind of operational honesty that makes a reference architecture trustworthy — observed sequence, misleading-error explanation, root-cause scoping ("nothing this repo touches is in the path"), and a concrete recovery command. Two small asks: (a) if this has been reported to the PCS service team (or there's an internal/public issue to follow), a one-line "tracked at " gives readers a way to know when it's fixed and gives future maintainers a signal for when this section can be deleted; (b) if it hasn't been reported yet, it probably should be — the analysis here is already most of a support case.


@KeitaW KeitaW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM! Thank you @DaisukeMiyamoto

@KeitaW
KeitaW merged commit 10613e3 into awslabs:main Jun 6, 2026
DaisukeMiyamoto added a commit to DaisukeMiyamoto/awsome-distributed-ai that referenced this pull request Jun 18, 2026
…ss opt-in on roadmap

The inline S3GetScripts statement added on this branch was redundant: the instance
role already attaches AmazonS3ReadOnlyAccess (s3:Get*/List* on *), so a narrower
templates/scripts/* grant added nothing — and the wildcard bucket the review flagged was
moot under the broader managed policy. Remove both inline statements (base + login role).

The real over-grant is AmazonS3ReadOnlyAccess itself: upstream aws-hpc-recipes makes it
opt-in (EnableS3ReadOnly, off by default) but ml-pcs attaches it unconditionally. That's
a pre-existing (awslabs#1120) IAM-behaviour change with workload impact (FSDP/Megatron read S3),
so it's tracked on ROADMAP rather than changed in this PR.
KeitaW added a commit that referenced this pull request Jun 18, 2026
…user, multi-AZ, IAM, Regions, SSH) (#1137)

* update README for PCS

* add reference cluster with PCS

* Update PCS README: add deployment options, 1-click deploy, and fix S3 bucket name

* Add unified multi-NIC template for P5/P6 instances and update documentation

* Improve PCS deployment: rename to Pseries, update docs and examples

* Add placement group for on-demand P5/P6 instances in add-cng-p5

* Add launch button and refine PCS documentation

* add launch button image

* fix typos

* fix typos

* Upgrade Slurm support to 25.05/25.11, default to 25.11

* Switch default AMI to PCS-specific DLAMI image

* Change mount path of FSxL from /shared to /fsx

* Simplify DLAMI build to use PCS-ready base image with Enroot/Pyxis only

* fix typos

* fix typos

* update readme in pcs

* add BaseAMI parameter for ImageBuilder

* use awsome-distributed-ai bucket

* fix typos

* update architecture

* update architecture image

* Address PR review feedback: fix permissions, add documentation

- Change FSx Lustre mount permissions from 0777 to 1777 (sticky bit)
- Add upstream repository reference to README
- Add Testing and Validation section documenting tested configurations

* Refactor cluster deployment to use modular add-cng templates

- Separate cluster core (Slurm scheduler) from compute nodes
- Use add-cng.yaml for both login and compute node groups
- Make queue creation optional in add-cng templates (empty QueueName for login nodes)
- Remove AmiId parameter from cluster.yaml (only needed for compute nodes)
- Update pcs-ml-cluster-deploy-all.yaml to deploy login and cpu1 as separate nested stacks
- Change default DeployOnDemandCNG to true (cpu1 queue enabled by default)
- Update README with new deployment architecture and examples

* Use variables for AZ ID in deployment examples

- Add AZ_ID variable at the start of each example
- Makes it easier to change AZ consistently across examples
- Aligns with CAPACITY_RESERVATION_ID variable pattern in Example 4

* Add AWS ParallelCluster Monitoring reference

- Add link to aws-parallelcluster-monitoring GitHub repository
- Provides comprehensive monitoring solution for HPC clusters with Prometheus and Grafana

* Add AWS Parallel Computing Service description

- Clarify what AWS PCS is in the introduction
- Mention it's a fully managed service for HPC with Slurm scheduler

* Add AWS Parallel Computing Service to top-level README

- Include AWS PCS in the list of supported compute platforms
- Update 9.aws-pcs description to clarify it uses Slurm scheduler

* Address PR #1109 review feedback (Round 2)

- Fix stray blank line in root README workshop table
- Change ml-cluster-prerequisites.yaml license from Apache-2.0 to MIT-0
- Fix Pyxis Slurm version compatibility: install for both 25.05 and 25.11
- Update PseriesMinCount description to clarify static vs dynamic scaling
- Add AMI build time note (~30 minutes) to README

* Add architectures/aws-pcs directory for new structure

Created new architectures directory structure alongside existing
1.architectures to prepare for repository reorganization.

Contents:
- architectures/aws-pcs/: Complete AWS PCS architecture templates
- All CloudFormation templates with monitoring integration
- README and architecture diagrams

This maintains backward compatibility with 1.architectures/9.aws-pcs
while establishing the new standardized structure.

* Remove old 1.architectures/9.aws-pcs directory

Removed legacy directory structure as content has been migrated
to architectures/aws-pcs/ in the new standardized structure.

All templates, documentation, and tests are now available in:
architectures/aws-pcs/

* Update README.md to reference new architectures/aws-pcs path

Changed link from 1.architectures/9.aws-pcs to architectures/aws-pcs
to reflect the new directory structure.

* iam: sample policies for cluster admin + cluster user (SSM)

Two principals around the PCS reference cluster have very different
responsibilities, so split them into two sample policies:

- cluster-admin-policy.json (16 Sids, ~7.4 KB) — for the deploying
  principal. Covers create / update / delete of every resource the
  templates provision: CloudFormation parent + nested stacks; EC2
  networking lifecycle (VPC, subnet, IGW, NAT, EIP, SG, route table, VPC
  endpoint, launch template, placement group); FSx (Lustre + OpenZFS);
  pcs:*; IAM role + instance profile lifecycle scoped to *PCS* /
  *ImageBuilder* names with iam:PassRole gated by iam:PassedToService;
  SSM Parameter Store for Grafana password + DLAMI auto-resolve;
  KMS / Secrets Manager / Image Builder / CloudWatch Logs vended
  delivery. Derived primarily from the AWS-published "minimum permissions
  for an AWS PCS service administrator" policy
  (security-min-permissions.html), with VPC + FSx + IAM lifecycle layered
  because the all-in-one template provisions those itself.

- cluster-user-policy.json (6 Sids, ~1.5 KB) — for end users (ML
  engineers) of an already-deployed cluster. Covers ec2/cfn describe for
  login-node discovery; pcs:Get*/List* for cluster status; ssm:GetParameter
  scoped to /pcs/*/grafana/*; ssm:StartSession scoped via
  ssm:resourceTag/aws:pcs:compute-node-group-name=login* (so users CAN
  open shells on login nodes but CANNOT on compute nodes); session
  terminate-own only via session/${aws:username}-*. No write access of
  any kind to cluster resources.

Both policies validated cleanly (0 findings) by aws accessanalyzer
validate-policy.

iam/README.md walks through what each policy covers, the design
intent (combined CRUD vs phase-split, customer-managed vs inline,
why no AmazonPCSFullAccess), what is intentionally NOT covered (the
compute instance role uses the AWS-managed AWSPCSComputeNodePolicy),
how to attach, AWS-managed policy pairing recipe, and the recommended
refinement loop via aws accessanalyzer start-policy-generation against
CloudTrail history of a sandbox deploy.

Sample-grade — slightly broader than strict least-privilege so they
work end-to-end in typical accounts. Production users should tighten
with resource-level conditions (account ID, region, stack-name prefix,
tag matchers) before adopting.

* iam: ship as CloudFormation templates with 1-click Deploy buttons

Two changes that came out of real-world verification:

1) The combined cluster-admin policy was ~7.4 KB, exceeding the IAM
   per-policy 6,144-char limit (which applies to BOTH inline and
   customer-managed policies — the previous README claim that
   customer-managed had a 17,408-char limit was wrong; that 17,408 is
   the per-user-aggregate inline-policy ceiling).

   Split into:
     * cluster-admin-policy.json (5,769 chars, 14 Sids — core CFN/EC2/
       FSx/PCS/IAM/SSM/KMS/Secrets/Logs)
     * cluster-admin-imagebuilder-policy.json (1,674 chars, 2 Sids —
       Image Builder + scoped iam:PassRole to imagebuilder.amazonaws.com)
   Both fit inside the 6,144-char limit; most users only need the core.

2) The cluster-user policy used
   ssm:resourceTag/aws:pcs:compute-node-group-name to scope login-node
   access, but PCS does not actually emit that tag (verified via
   describe-instances on a deployed login node). Real tags are:
     * aws:pcs:compute-node-group-id (random PCS ID, can't be policy-pinned)
     * monitoring-role (semantic for monitoring exporters, not access)
     * Name (the all-in-one templates set this to PCS-login on login CNGs
       and PCS-<cng-name> on compute CNGs, e.g. PCS-cpu1, PCS-hpc8a)

   Switched the SSM-StartSession scope to
   ssm:resourceTag/Name=PCS-login*. The Name tag is operator-mutable, so
   the README now flags this as a hardening point with a forking option
   (add a dedicated IsLoginNode tag to the templates).

Wrap each policy file in a CloudFormation template and put a Deploy
button in iam/README.md:

  architectures/aws-pcs/assets/cluster-admin-iam.yaml
    Creates 2 customer-managed policies (core + optional Image Builder)
    + an IAM Group with both attached. AttachUsers param adds existing
    users to the group; AttachImageBuilderPolicy=true pulls in the IB
    statements (default false). Output: GroupName, CorePolicyArn,
    ImageBuilderPolicyArn (when applicable).

  architectures/aws-pcs/assets/cluster-user-iam.yaml
    Creates 1 customer-managed policy + IAM Group. Same AttachUsers
    pattern. Output: GroupName, PolicyArn.

YAML templates live in assets/ to match the rest of the reference
architecture's CFN artifacts (same S3 sync target, same Quick-create
URL pattern, same maintenance pipeline). The raw JSON files stay under
iam/ as the source of truth that the YAML embeds.

Verified end-to-end on us-east-2 / 2026-06-10:
  - Both IAM CFN stacks reach CREATE_COMPLETE
  - Test admin user (only customer-managed policies attached) deploys
    pcs-ml-cluster-deploy-all.yaml hpc8a configuration end-to-end with
    OnDemandEnableEfa=true to CREATE_COMPLETE in ~28 min, no AccessDenied
    in 332+ CloudTrail API calls; UpdateStack on
    OnDemandMaxCount→3 reaches UPDATE_COMPLETE; DeleteStack via the
    admin user is allowed (simulator: cloudformation:DeleteStack=allowed)
  - Test user can pcs:GetCluster, ssm:StartSession on login node (Name=
    PCS-login), and is implicitDeny on compute node (Name=PCS-hpc8a)
    and on cfn:CreateStack / pcs:CreateCluster / ec2:RunInstances
  - aws accessanalyzer validate-policy on all three JSON files: 0 errors

iam/README.md updated to:
  - Add Quick-create Deploy buttons pointing at the prod S3 path
    awsome-distributed-ai.s3.amazonaws.com/templates/cluster-{admin,user}-iam.yaml
  - Document the verification matrix and what the admin user actually
    needs (the Name-tag scope rationale + caveat)
  - Replace the wrong 17,408-char claim with the correct 6,144-char
    explanation and the split rationale
  - Keep the manual install path (raw JSON) for CLI-only environments

Templates were also uploaded to the test bucket (midaisuk-llm-dev) for
pre-merge sandbox testing.

* deploy-all: tighten parameters — auto-derive EFA interface count, default VPC name from stack name, label cleanup

Non-breaking improvements identified in the parameter audit (43 params,
no removals possible without breaking PR #1124 deployments):

1. **Auto-derive `OnDemandEfaInterfaceCount` from `OnDemandInstanceType`**
   (mirrors the pattern `add-cng-p5.yaml`/`add-cng-p6-*.yaml` use for GPU
   families). New default 0 means "auto"; old 1/2 values still work as
   explicit pins so existing stacks pass through unchanged. Auto map:

     hpc8a.96xlarge / hpc7a.{96,48,24,12}xlarge / hpc6id.32xlarge → 2
     anything else (hpc6a.48xlarge, c7i.metal, etc.) → 1

   Removes the 2-NIC HPC tax: hpc8a/hpc7a were the most common EFA
   targets but had to override the parameter; now the default just works.

2. **`VPCName` default → empty, auto-resolves to `${StackName}-VPC`**.
   Multiple deployments in one account now get unique VPC names without
   the user having to pass a different VPCName each time.

3. **ParameterGroup label cleanup**:
     "6. FSx for Lustre (/fsx) + FSx for OpenZFS (/home) (Advanced)"
       → "6. FSx for Lustre (/fsx) and FSx for OpenZFS (/home)"
       (Capacity / LustreDeploymentType / PerUnitStorageThroughput are
       commonly tuned and Region-dependent; not advanced)
     "7. Developer / Advanced (Nested Templates & Monitoring Source)"
       → "7. Developer (do not change for normal use)"

4. **ParameterLabel trims**:
     "Availability Zone (required)" → "Availability Zone"
       (CFN console already shows the asterisk for required params)
     "VPC name" → "VPC name (empty = ${StackName}-VPC)"
     "AMI ID (empty = latest PCS-Ready Deep Learning AMI from SSM; pin in production)"
       → "AMI ID (empty = SSM auto-resolve; pin in production)"
     "dcgm-exporter image (default DCGM 4.5.2 by digest; covers H100/B200/B300)"
       → "dcgm-exporter image (default covers H100/B200/B300)"
     "EFA interface count (1 for hpc6a, 2 for hpc8a/hpc7a/hpc6id)"
       → "EFA interface count (0 = auto from instance type)"

PARAMETERS.md updated to match.

Backward compatibility: every change preserves prior behavior for any
stack that was deployed off the published default — VPCName defaulting
from "ML-Cluster-VPC" to ${StackName}-VPC only affects new launches
where the user did not pass VPCName explicitly. Old stacks keep their
existing VPC name on UPDATE_STACK.

(Larger reductions tracked in subagent plan for the next minor: collapse
CngName/QueueName pairs, drop *MinCount, drop LustreVersion/Compression,
rename Capacity → LustreCapacity for prefix consistency. All of those
are renames or removes, so deferred to avoid breaking PR #1124 users.)

* feat: add optional OpenLDAP multi-user directory on login node

Adds opt-in multi-user support via OpenLDAP running on the login node.
Default is off (DeployDirectory=false) — existing single-user (ubuntu)
clusters are completely unchanged.

When DeployDirectory=true:
- Login node: installs slapd, stores DB on shared /home/ldap-db
  (OpenZFS), so the directory survives login CNG recreation. Admin
  password is stored in SSM Parameter Store at
  /pcs/<cluster-id>/ldap/admin-password.
- Compute nodes: installs SSSD with ldap provider, discovers login
  node IP via ec2:DescribeInstances tag query, configures NSS/PAM
  so LDAP users are visible as POSIX users on all nodes.
- Home directories auto-created via pam_mkhomedir on shared /home.
- Slurm sees LDAP users transparently via NSS (no Slurm config change).

New parameters on add-cng.yaml:
- DeployDirectory (true/false, default false)
- DirectoryDomainSuffix (e.g. dc=cluster,dc=internal)

New scripts:
- scripts/setup-openldap-server.sh — slapd install + config on login
- scripts/setup-ldap-client.sh — SSSD client config on compute nodes
- scripts/ldap-add-user.sh — helper to add POSIX users to the directory

Not yet wired into deploy-all.yaml or GPU templates (next commit).

* docs: add USER-MANAGEMENT.md — OpenLDAP multi-user operations guide

Covers enabling multi-user, managing users (add/remove/list/password/groups),
running jobs as specific users, SSH access for LDAP users, UID/GID conventions,
SSSD caching behavior, troubleshooting, data persistence, and upgrade path to
Simple AD / Managed AD.

* fix: point LDAP script URLs at fork branch (pre-merge)

* fix: apt-get update before slapd install + add multi-user test suite

- setup-openldap-server.sh: add apt-get update before install (PCS-Ready
  DLAMI has limited apt sources cached; without update, slapd package is
  not found)
- setup-ldap-client.sh: same fix
- tests/multi-user-test.md: comprehensive test suite (5 parts: server
  health, user lifecycle, Slurm integration, multi-node consistency,
  resilience). Split from README.md to keep the main test guide concise.
- tests/README.md: added Test 11 link to multi-user-test.md

* fix: SSSD min_id=1001 (GID 3000 was out of range) + persist admin pw + access methods doc

- setup-ldap-client.sh: change min_id from 10000 to 1001. GID 3000
  (clusterusers) was being filtered as "primary gid out of range" by
  SSSD, preventing user resolution. min_id=1001 excludes only system
  users (0-1000) while allowing all LDAP-provisioned GIDs.

- setup-openldap-server.sh: persist the admin password to
  /home/ldap-db/.admin-password (shared OpenZFS). Previously the
  randomly generated password was lost after the setup script exited,
  making it impossible to administer the directory after login node
  replacement. The file is chmod 600 and on shared storage.

- USER-MANAGEMENT.md: added "Access methods" section comparing SSM vs
  SSH-over-SSM vs Direct SSH (pro/con/best-for table), SSH key
  management approaches, and Slurm accounting with PCS (what's managed
  by PCS vs what admins still need to do manually with sacctmgr).

Verified: getent passwd testuser1 now returns
  testuser1:*:10001:3000:Test User 1:/home/testuser1:/bin/bash
on the login node after SSSD restart.

* feat: wire DeployDirectory into deploy-all + template structure diagram

- pcs-ml-cluster-deploy-all.yaml: add DeployDirectory + DirectoryDomainSuffix
  params (group 7), forward to both LoginNodeGroupStack and OnDemandCNGStack.
  Without this wiring, compute nodes never received DeployDirectory=true and
  SSSD client setup was skipped.
- docs/USER-MANAGEMENT.md: add ASCII art template structure diagram showing
  how DeployDirectory flows from deploy-all through the nested stacks into
  UserData on login (slapd) and compute (SSSD client) nodes.

* refactor: rename DeployDirectory → DirectoryService (enum: none | OpenLDAP-LoginNode)

Rename the boolean parameter to an enum that makes it clear both *what*
directory service is used and *where* it runs:

  DirectoryService:
    AllowedValues: [none, OpenLDAP-LoginNode]
    # Future: SimpleAD, ManagedAD, OpenLDAP-External

This prepares for adding Simple AD or external LDAP options in the future
without a breaking rename. The value "OpenLDAP-LoginNode" communicates
that slapd runs on the login node (not an external server).

Also:
- README.md: added template nesting structure diagram (ASCII art)
  showing how DirectoryService flows from deploy-all through login
  (slapd server) and compute (SSSD client) CNGs.
- Fixed DeployMonitoring default accidentally changed to 'none' by
  bulk sed (reverted to 'false' for add-cng, 'true' for deploy-all).

* ROADMAP: track DeployMonitoring → MonitoringStack rename for next minor

* fix: revert unintended Default:'false'→'none' on 4 boolean params

The bulk sed that renamed DeployDirectory→DirectoryService also changed
Default:'false' to Default:'none' on every parameter in both templates.
This broke FSxLustreEnableEfa, OnDemandEnableEfa, DeployPseriesCNG
(deploy-all) and EnableEfa (add-cng) — their AllowedValues are
[true,false] so 'none' is invalid.

Reverted to 'false' for all 4. Only DirectoryService (default 'none')
and MonitoringRole (default 'none') correctly use 'none'.

* security: store LDAP admin password in SSM Parameter Store, not on shared /home

The admin password was previously written to /home/ldap-db/.admin-password
(shared OpenZFS, readable by root on all nodes). This is a security
concern in multi-user clusters where compute-node users may have sudo.

Now:
- setup-openldap-server.sh first tries SSM PutParameter (SecureString)
  at /pcs/<cluster-id>/ldap/admin-password. SSM is access-controlled
  via IAM and auditable via CloudTrail.
- Falls back to the file only if the instance role lacks ssm:PutParameter
  (with a WARNING in the install log).
- cluster.yaml instance role: broadened the SSM Sid from grafana/* to
  also cover ldap/* (same pattern, same cluster-scoped resource ARN).
- Removed the duplicate SSM put from add-cng.yaml UserData (the script
  now handles it internally with CLUSTER_ID exported from UserData).
- USER-MANAGEMENT.md: added fallback note + clarified SSM is the
  primary storage mechanism.

* refactor: merge setup-openldap-server.sh + setup-ldap-client.sh → setup-directory.sh

Single script with role argument (server|client) replaces two separate
scripts. Benefits:
- One URL to manage, no naming inconsistency
- Full picture visible in one file (server + client + shared SSSD config)
- Natural extension point for future SimpleAD (add a case branch)
- Server role now also configures SSSD on the login node itself (so
  getent works locally without a separate manual step)

UserData in add-cng.yaml simplified to:
  curl setup-directory.sh
  if login → bash setup-directory.sh server
  else     → bash setup-directory.sh client

Environment variable interface unchanged (LDAP_DOMAIN_SUFFIX, CLUSTER_ID,
DIRECTORY_DNS_IPS for future SimpleAD). DIRECTORY_DNS_IPS="" triggers
login-IP discovery via ec2:DescribeInstances tag query.

Also: README template structure diagram updated with external scripts
and SSM parameter references.

* refactor: replace MonitoringRole-based directory branching with dedicated DirectoryRole param

MonitoringRole is for monitoring exporter placement — reusing it for
directory server/client branching was a semantic mismatch.

New parameter DirectoryRole (none | server | client):
- add-cng.yaml: DirectoryRole param added. UserData passes it directly
  to setup-directory.sh as the role argument. No more MonitoringRole
  reference in the directory block.
- deploy-all.yaml: forwards DirectoryRole='server' to login CNG and
  DirectoryRole='client' to compute CNG (both conditional on
  DirectoryEnabled). New Condition DirectoryEnabled added.
- Removed unused Conditions (IsLoginNode, SetupLdapServer, SetupLdapClient)
  that depended on MonitoringRole for directory logic.

MonitoringRole remains unchanged for its original purpose (monitoring
exporter placement). The two concerns are now fully independent.

* feat: fetch scripts from S3 instead of GitHub raw URLs

Scripts are now fetched from the same S3 bucket as the nested templates
(S3BucketName/S3KeyPrefix + scripts/). This eliminates:
- Hardcoded GitHub raw URLs that need branch-switching for testing
- Dependency on GitHub availability at instance boot time
- Version skew between templates and scripts

Changes:
- add-cng.yaml: UserData uses `aws s3 cp` from the S3 bucket to fetch
  setup-directory.sh. New params S3BucketName + S3KeyPrefix forwarded
  from deploy-all.
- deploy-all.yaml: forwards S3BucketName/S3KeyPrefix to both login and
  compute CNG stacks.
- cluster.yaml: instance role gets s3:GetObject on */templates/scripts/*
  so nodes can fetch scripts at boot.

Development workflow:
  1. Edit scripts/ locally
  2. aws s3 sync scripts/ s3://test-bucket/templates/scripts/
  3. Deploy with S3BucketName=test-bucket
  4. Scripts are fetched from test bucket — no branch URL changes needed

Production: scripts sync alongside templates to the prod bucket.
Future PCS post-install hook: pass the same S3 URL.

* move scripts/ → assets/scripts/ so prod S3 sync covers them automatically

The prod S3 sync pipeline runs:
  aws s3 sync assets/ s3://awsome-distributed-ai/templates/

Previously scripts/ was a sibling directory requiring a separate sync
command (which was never added). Moving scripts under assets/ means the
existing one-line sync deploys both templates and scripts with zero
operational change for maintainers.

Result on S3:
  s3://bucket/templates/pcs-ml-cluster-deploy-all.yaml
  s3://bucket/templates/add-cng.yaml
  s3://bucket/templates/scripts/setup-directory.sh    ← new
  s3://bucket/templates/scripts/install-enroot-pyxis.sh
  s3://bucket/templates/scripts/ldap-add-user.sh

UserData fetches via:
  aws s3 cp s3://${S3BucketName}/${S3KeyPrefix}scripts/setup-directory.sh

README path references updated.

* docs: add DEPLOY-TESTING.md — development deploy procedures

* docs: rewrite USER-MANAGEMENT.md for LDAP-unfamiliar admins + update README

USER-MANAGEMENT.md completely rewritten:
- Quick reference table at top (common tasks → one-liner commands)
- How-it-works diagram (slapd → SSSD → NSS/PAM flow)
- Step-by-step for every operation: add/delete/list users, reset password,
  create groups, batch add, Slurm accounting registration
- Verification steps (how to confirm user is visible on compute nodes)
- Troubleshooting section (common errors + fixes)
- UID/GID convention table
- Data persistence + backup/restore
- Template structure (how DirectoryRole flows)
- Upgrade path to Simple AD

README.md:
- Fixed template diagram: old script names → setup-directory.sh server/client
- Fixed "fetched from GitHub raw" → "fetched from S3"
- Added USER-MANAGEMENT.md + DEPLOY-TESTING.md to Additional Resources

* fix: update all stale path/param references after scripts/ relocation

- DeployDirectory → DirectoryService=OpenLDAP-LoginNode in tests/
- scripts/install-enroot-pyxis.sh → assets/scripts/... in template
  descriptions (add-cng*.yaml), ROADMAP, tests/README.md
- PostInstallScriptUrl default URL: .../scripts/... → .../assets/scripts/...
  (file was git-mv'd, GitHub raw URL must match new repo path)
- No remaining references to old paths or old param names

* README: separate boot scripts from helper scripts, add parallelcluster-monitoring link

* fix: move directory setup from MIME shellscript to runcmd block (PCS skips shellscript sections)

* tests: add accounting-test.md + gpu-healthcheck-test.md

Test 12 (accounting-test.md):
  - Validates Slurm managed accounting with LDAP multi-user
  - Covers: sacctmgr user/account creation, resource limits (GrpTRESRunMins),
    job tracking (sacct), reporting (sreport), fairshare, and
    AccountingPolicyEnforcement behavior (none vs associations,limits,safe)
  - Based on https://aws.amazon.com/blogs/hpc/introducing-managed-accounting-for-aws-parallel-computing-service/

Test 13 (gpu-healthcheck-test.md):
  - Integrates 4.validation_and_observability/2.gpu-cluster-healthcheck suite
  - Lightweight (checks 0-3: nvidia-smi, DCGM L2, EFA enum, topology) ~15 min
  - NCCL multi-node (check 5) with per-instance bandwidth thresholds
  - Slurm prolog integration guide for production
  - When-to-run decision table (deploy, pre-training, steady-state, quarantine)

README: added GPU Health Check link to Additional Resources.
tests/README.md: added Test 12 + Test 13 entries with links.

* tests: split README.md (842 lines) into category-based files

README.md was too long — reduced from 842 to 73 lines (index + matrix only).
Test procedures moved to per-category files:

  infra-test.md      — Tests 1-3, 8 (monitoring, container runtime, AMI build)
  compute-test.md    — Tests 4-6 (CPU queue, GPU families, NCCL EFA)
  training-test.md   — Test 7 (FSDP Llama-2 7B)
  hpc-efa-test.md    — Test 9 (EFA on CPU HPC, OSU benchmarks)
  storage-test.md    — Test 10 (FSx health + performance regression)
  multi-user-test.md — Tests 11-12 (OpenLDAP + accounting, merged)
  gpu-healthcheck-test.md — Test 13 (GPU health check suite)

Removed accounting-test.md (merged into multi-user-test.md since they
are always tested together).

* README: add §11 User Management section with link to USER-MANAGEMENT.md

* fix: move runcmd comment inside list item (YAML comment outside - | breaks cloud-init parsing)

* README: restructure — group advanced features under §8, simplify section numbering

Before: 13 top-level sections mixing core usage with advanced features and reference.
After: 11 sections with clear separation:

  §1-7: Core (Key Features → Running a GPU job)
  §8:   Advanced Features
        8.1 Monitoring
        8.2 Pre-baking AMI
        8.3 User Management (new — was standalone §11)
        8.4 IAM Permissions (new)
  §9:   Templates (reference)
  §10:  Testing and Validation (reference)
  §11:  Additional Resources

This makes it clear that §1-7 is the primary user path (deploy + run jobs),
and everything in §8 is opt-in for production/multi-user/security hardening.

* deploy-all: reorganize ParameterGroups — separate monitoring/directory from cluster core

Before: DeployMonitoring + GrafanaPublicAccessCidr were under "PCS Cluster
Configuration" (which is really scheduler/AMI/accounting settings).

After:
  §2. PCS Cluster Configuration — Slurm version, login instance, AMI, accounting
  §7. Additional Cluster Configuration — Monitoring + Multi-User Directory

This separates "what the cluster IS" (§2) from "what extra features are
layered on top" (§7). Also renamed §6 from "(Advanced)" to just "FSx Storage"
(Capacity/DeploymentType are commonly tuned, not advanced) and §8 to just
"Developer / Advanced" (shorter).

* deploy-all: move monitoring params to §7, Developer §8 keeps only S3 bucket/prefix

* feat: SSHAccessCidr + DeployMonitoring→MonitoringStack + GrafanaPublicAccessCidr→GrafanaAccessCidr

Three changes in deploy-all parameter interface:

1. **SSHAccessCidr** (new, §2 PCS Cluster Configuration):
   CIDR to open port 22 on the login node. Empty = SSH over SSM only.
   For multi-user clusters where users connect via standard SSH.

2. **DeployMonitoring → MonitoringStack** (rename, §7):
   Boolean true/false → enum: 'Prometheus-LoginNode' (default) | 'none'.
   Aligns with DirectoryService naming pattern (<what>-<where>).
   Nested stacks receive DeployMonitoring="true"/"false" via !If [MonitoringEnabled]
   so add-cng.yaml is unchanged (backward compat within the nest).

3. **GrafanaPublicAccessCidr → GrafanaAccessCidr** (rename, §7):
   Shorter name, same semantics. Opens HTTPS/443 on login node.

Security group redesign:
- LoginAccessSecurityGroup now conditionally includes BOTH SSH (from
  SSHAccessCidr) and HTTPS (from GrafanaAccessCidr) rules via !If.
- Created only when at least one CIDR is set (LoginAccessEnabled = OR).
- Attached only to login node via ExtraSecurityGroupId (compute unaffected).

New Conditions: SSHAccessEnabled, GrafanaAccessEnabled, LoginAccessEnabled
(OR of both), MonitoringEnabled.

* fix: remove --region from s3 cp (AWS CLI auto-resolves via IMDS credential provider, avoids IMDSv2 token issue with curl)

* feat: dedicated directory-role tag, helper-script install, multi-AZ subnets, region fixes

Multi-user directory fixes (compute client could not find the LDAP server):
- **Dedicated directory-role tag** (was reusing monitoring-role — design
  violation). add-cng.yaml now emits directory-role=server/client independently
  of monitoring-role; setup-directory.sh discovers the server via
  directory-role=server. Multi-user must not depend on the monitoring stack's tag.
- **Region resolution**: removed all `--region "$REGION"` from setup-directory.sh
  (REGION came from a token-less IMDS curl that returns empty under
  HttpTokens=required, breaking `aws ... --region ""`). The AWS CLI resolves
  region from the IMDS credential provider itself, so no --region is passed.
- **ldap-add-user.sh helper install**: server role now installs the helper to
  /usr/local/bin from the same S3 bucket (S3_BUCKET/S3_KEY_PREFIX passed by
  UserData), so admins have `ldap-add-user` on PATH (was missing — had to fetch
  from S3 manually).

Single-login-node constraint documented (OpenLDAP server is single-node by
design; DB is one MDB on shared /home): setup-directory.sh header,
USER-MANAGEMENT.md callout, DirectoryService param description.

Multi-AZ prerequisites (max 3 AZs: primary + 2 additional):
- AdditionalSubnetAZ2 / AdditionalSubnetAZ3 params (optional, empty = single-AZ).
- CIDR1 (10.1.0.0/16) now split into four /18 blocks (was two /17) — BREAKING
  change to private subnet CIDRs, approved.
- Additional subnets share the primary AZ's single NAT gateway (cross-AZ, no
  per-AZ NAT by design). Outputs exported for downstream use.

* deploy-all: forward AdditionalSubnetAZ2/3 to prerequisites (multi-AZ wiring)

* feat: wire multi-user directory into GPU templates (p5/p6-b200/p6-b300)

The three GPU add-cng templates now mirror add-cng.yaml's directory support:
- DirectoryService / DirectoryDomainSuffix / DirectoryRole / S3BucketName /
  S3KeyPrefix params
- DirectoryEnabled condition
- directory-role tag (independent of monitoring-role)
- setup-directory.sh runcmd block (S3-fetched, server installs ldap-add-user helper)

deploy-all forwards DirectoryRole=client to all three GPU CNG stacks (P5,
P6-B200, P6-B300), so GPU compute nodes join the directory as SSSD clients
exactly like CPU compute nodes. All four templates validate.

* docs: update README/PARAMETERS/ROADMAP for renamed + new params

- README.md: DeployMonitoring=true → MonitoringStack=Prometheus-LoginNode,
  GrafanaPublicAccessCidr → GrafanaAccessCidr (incl. Option B heading + anchor),
  added SSHAccessCidr mention in §6 Accessing the Cluster.
- PARAMETERS.md: new §2b Additional Cluster Configuration (MonitoringStack,
  GrafanaAccessCidr, DirectoryService, DirectoryDomainSuffix); SSHAccessCidr in §2;
  AdditionalSubnetAZ2/3 in §1 Network; fixed stale #9-pre-baking anchor → #82.
- ROADMAP.md: checked off Multi-AZ support, LDAP/AD user-management backend,
  DeployMonitoring→MonitoringStack rename (with done-notes + follow-ups).

* fix: setup-directory.sh waits for apt/dpkg lock (first-boot unattended-upgrades)

slapd/sssd install failed at first boot with "Could not get lock
/var/lib/dpkg/lock-frontend ... held by unattended-upgr" — same first-boot
race install-enroot-pyxis.sh already handles. Added wait_for_apt_lock + an
apt_get wrapper (3 retries) and routed all four apt-get calls through it.
Without this, set -euo pipefail aborted the script on the lock error, leaving
slapd uninstalled and the ldap-add-user helper unplaced.

Also documented (USER-MANAGEMENT.md): tag-based LDAP-server discovery is
scoped by pcs-cluster-id, so multiple PCS clusters can share one VPC safely.

* docs: finish param-rename sweep + add LDAP discovery / multi-VPC notes

- OPERATIONS.md: GrafanaPublicAccessCidr → GrafanaAccessCidr (2 refs)
- tests/README.md + infra-test.md: DeployMonitoring=true → MonitoringStack=Prometheus-LoginNode
- USER-MANAGEMENT.md: tag-based LDAP-server discovery section (how compute finds
  the login node by directory-role=server, IAM/ordering implications, override),
  and multi-cluster-per-VPC safety (pcs-cluster-id scopes the lookup).

* feat: bring in IAM policy stacks from feat/aws-pcs-updates

Conflict-free IAM additions for the major-update PR:
- iam/cluster-admin-policy.json + cluster-admin-imagebuilder-policy.json
- iam/cluster-user-policy.json
- iam/README.md (verification matrix + design rationale)
- assets/cluster-admin-iam.yaml + cluster-user-iam.yaml (1-click deploy stacks)

Least-privilege policies: cluster admin (deploy/update/delete) and cluster user
(SSM session + read-only). All templates validate; all JSON parses.

The deploy-all/PARAMETERS.md changes from feat/aws-pcs-updates (VPCName
auto-derive, EFA auto-count, label cleanup) are merged separately — they
conflict with the multi-user param reorg and need manual resolution.

* fix: install sssd-tools so deleted/modified LDAP users propagate (sss_cache)

User deletion (and any LDAP modify) was not visible via getent until the SSSD
cache entry's TTL expired — sss_cache wasn't available to invalidate it
(sssd-tools wasn't installed). Added sssd-tools to the client apt install, and
documented the `sudo sss_cache -E` step (login + compute) in USER-MANAGEMENT.md's
"Deleting a user" section. Verified on v8: after sss_cache -E a deleted user
disappears from getent immediately while other users are unaffected.

* tests: record major-update e2e validation matrix (run on real hardware)

* docs: fix USER-MANAGEMENT accuracy (verified against live accounting cluster)

Caught while validating Test 12 on a ManagedAccounting=enabled cluster:
- sacctmgr add/modify/remove must run as root (the accounting Administrator);
  ubuntu gets "Only admins/operators/coordinators can add accounts". Documented
  sudo + full-path usage; read-only sacct/sreport/show still work as ubuntu.
- Quick-reference table referenced non-existent helpers (ldap-list-users,
  ldap-delete-user, ldap-reset-password) and the bare name ldap-add-user.
  Only ldap-add-user.sh exists — fixed the table to use it (with .sh) and show
  raw ldapsearch/ldapdelete/ldappasswd for list/delete/reset.
- Noted that LDAP_ADMIN_PASSWORD must be passed inline to sudo
  (sudo VAR=... cmd); sudo -E alone drops it under env_reset/secure_path.

* tests: accounting + multi-user (Test 12) verified end-to-end on real hardware

* docs: reorder Advanced Features by priority, consolidate IAM into docs/IAM.md

README §8 Advanced Features reordered by priority:
  8.1 Monitoring (dashboards)  8.2 User Management  8.3 IAM Permissions  8.4 Pre-baking AMI
(was: Monitoring, Pre-baking, User Management, IAM)

Fixed the monitoring "dashboard" heading hierarchy: "Accessing Grafana" and its
Option A/B were at ### / #### (siblings of ### 8.1), so they broke out of the
section. Demoted to #### / ##### so they nest under 8.1. Renamed to
"Accessing the Grafana dashboards" and fixed all anchor links.

IAM docs consolidated:
- New docs/IAM.md — leads with the two roles (admin/user) and how to deploy
  their policy stacks, then security considerations, then the verification matrix.
- Deleted iam/ directory: the raw *.json policies were redundant (the CFN
  templates embed the policy inline as PolicyDocument), and iam/README.md is
  superseded by docs/IAM.md. IAM YAMLs already live in assets/ (S3-sync covered).
- README §8.3 rewritten to lead with the roles and link docs/IAM.md; template
  diagram + Additional Resources updated (iam/ → assets/, added IAM guide link).

Re-pointed the pre-baking anchor (#82 → #84) everywhere after the reorder.

* docs: clarify OnDemandEfaInterfaceCount auto-derive to avoid EFA confusion

The 0=auto default hid which instance type maps to how many EFA NICs, and gave
no signal that EFA only works on EFA-capable types. Expanded the param
description (deploy-all) + PARAMETERS.md to:
- list the per-type auto values explicitly (hpc8a/hpc7a/hpc6id=2, hpc6a/
  c7i.metal=1, anything else=1)
- state plainly that EnableEfa=true must only be used on an EFA-capable type
  (hpc6/hpc7/hpc8 + select metal); a non-EFA type like c6i.4xlarge fails to
  launch regardless of this count
- clarify when to override (pin a value for a new HPC type not in the auto map)

No behavior change — auto-derive stays; this only makes it legible so HPC users
aren't surprised. add-cng.yaml's EfaInterfaceCount (default 1, [1,2], no auto)
is unchanged: deploy-all resolves auto→concrete before forwarding, so the
terminal template still takes an explicit count.

* refactor: collapse OnDemandEnableEfa into OnDemandEfaInterfaceCount (0/1/2)

Per review: a 0=auto default + a separate boolean enable flag was confusing —
"set count to 0" reading as "auto" is unintuitive. Simplified to a single
parameter with literal meaning:

  OnDemandEfaInterfaceCount: 0 = no EFA (default) | 1 | 2 = enable with N NICs

- Removed OnDemandEnableEfa (deploy-all) and EnableEfa (add-cng) entirely;
  EFA is now driven by EfaInterfaceCount > 0.
- Removed the auto-derive logic (OnDemandEfaCountIsAuto / OnDemandIs2NicHpc
  conditions + the !If forwarding). The user sets the count explicitly (the
  per-type values are documented: hpc6a=1, hpc7a/hpc8a/hpc6id=2).
- add-cng EfaEnabled condition is now !Not [count == 0]; Has2ndEfaInterface is
  count == 2.
- Updated descriptions to state EFA needs an EFA-capable type (non-EFA type like
  c6i fails to launch with count > 0) — addresses the "which types support EFA"
  confusion directly.
- Swept README/PARAMETERS/DEPLOY-TESTING/hpc-efa-test/storage-test/prerequisites
  for the old param names + anchors. VPCName auto-derive (the other merged
  improvement) is kept as-is.

* fix: remove VPCName param from deploy-all + clear stale OnDemandEnableEfa/anchor refs

- VPCName removed from deploy-all (ParameterGroup, label, param def, HasUserVPCName
  condition); VPC name now hardcoded to ${StackName}-VPC. prerequisites.yaml keeps
  its VPCName param for standalone use.
- PARAMETERS.md: removed the VPCName row, added a note that the name is fixed.
- Cleared 2 stale OnDemandEnableEfa=true refs (README + PARAMETERS FSxLustreEnableEfa
  descriptions) → OnDemandEfaInterfaceCount > 0.
- Fixed broken ROADMAP anchor (#client-side-lustre-on-efa--gds-support is a list
  item, not a heading) → link to the file.
Found via full consistency audit.

* fix: VPC Name tag uses VPCName param (was hardcoded 'ML Cluster VPC')

The VPC resource's Name tag was hardcoded, so the VPCName parameter (and
deploy-all's ${StackName}-VPC value) only renamed the subnets, not the VPC
itself — every cluster's VPC showed up as 'ML Cluster VPC' in the console.
Now the VPC Name follows VPCName, so multiple deploys in one account get
distinct, stack-named VPCs as intended. Found during hpc8a EFA e2e (VPC showed
'ML Cluster VPC' instead of pcs-efa-hpc8a-VPC).

* tests: add region coverage matrix (us-east-1/2, us-west-2, ap-northeast-1, ap-south-1)

Validated deploy-all across 5 regions: all reach CREATE_COMPLETE with login +
6 monitoring containers + FSx Lustre/OpenZFS mounts. ap-south-1 (Mumbai) adds
p6-b200 x4 GPU validation (8 GPU/node, 8 EFA NICs, B200 driver 595.71.05).
Documented cross-region S3 fetch works (S3 global namespace) and the pre-merge
PostInstallScriptUrl 404 caveat.

* tests: record p6-b200 x4 NCCL all_reduce busbw (peak 377 GB/s over EFA, 32 GPU, Mumbai)

* tests: record FSDP distributed-training readiness on 32 B200 GPUs (Mumbai)

NCCL 32-GPU init + FSDP2 model wrap (0.41B) + optimizer succeed across 4x
p6-b200; 8 GPUs/node confirmed. Training step loop blocked only by HuggingFace
429/shard-FileNotFoundError on streaming allenai/c4 (external rate limit, not a
cluster issue).

* fix: split PCS instance-role perms inline by feature; drop ManagedPolicy

Two problems with the old MonitoringPolicy (AWS::IAM::ManagedPolicy, gated on
IsMonitoringEnabled):

1. It required the DEPLOYER to have iam:CreatePolicy — which the cluster-admin
   policy intentionally does not grant — so a cluster-admin principal could not
   actually run deploy-all (the ideal is: admin policy ⇒ deploy-all works). The
   only AWS::IAM::ManagedPolicy in the entire nest was this one.
2. It bundled multi-user (SSM /pcs/<id>/ldap/*), S3 script fetch, and KMS-decrypt
   perms together with monitoring perms, all behind IsMonitoringEnabled. So
   MonitoringStack=none + DirectoryService=OpenLDAP-LoginNode left the instance
   WITHOUT permission to store the LDAP admin password in SSM (silently fell back
   to a file).

Fix: instance-role perms are now inline AWS::IAM::Policy (PutRolePolicy, which
the admin policy grants), split by feature:
- PcsInstanceBasePolicy (always): pcs:RegisterComputeNodeGroupInstance, S3
  GetObject on templates/scripts/*, SSM Get/Put on this cluster's grafana+ldap
  params, KMS Decrypt via SSM. Baseline every instance needs — independent of
  monitoring or directory.
- MonitoringInstancePolicy (AWS::IAM::Policy, IsMonitoringEnabled only): just the
  monitoring reads (EC2/FSx describe, CFN describe, pricing, CloudWatch, logs,
  pcs:GetCluster).

Net effect: deploy-all needs no iam:CreatePolicy (admin policy already covers
CreateRole+PutRolePolicy), multi-user works regardless of MonitoringStack, and
the admin policy stays at its current size (no CreatePolicy/policy-ARN additions).

* tests: refine FSDP result — distributed stack proven; c4 streaming blocked by HF rate-limit

Ran the canonical llama3_2_1b-training.sbatch unchanged. NCCL c10d rendezvous +
FSDP2 wrap (1.15B) + optimizer succeed on 32 B200 GPUs (8/node). Training step
loop unreachable purely due to external HF 429 on allenai/c4 streaming (tree
API, 1024 shards) — confirmed external by exhausting token/cache/offline/
long-timeout/single-node mitigations. Cluster is training-ready (NCCL 377 GB/s);
c4-at-scale needs HF higher-rate account / mirror / pre-tokenized /fsx data.

* docs(IAM): refresh verification matrix against major-update templates

Re-validated with a role attached to ONLY the admin (resp. user) managed
policies on us-east-2:
- admin-only role runs deploy-all (multi-user + SSH + monitoring) to full
  CREATE_COMPLETE with no AccessDenied, and needs NO iam:CreatePolicy (instance
  perms are now inline) — confirms "cluster-admin policy ⇒ deploy-all works".
- user-only role: read-only allowed, all writes implicitDeny, login-node SSM
  session allowed but compute-node denied, grafana password readable but the
  new LDAP admin password (/pcs/*/ldap/*) NOT readable.
- Documented that the admin policy intentionally omits s3:GetObject (prod bucket
  is public; private template buckets need it granted separately).

* tests: record EFA OSU real-traffic numbers (hpc8a: osu_bw 26.3 GB/s, osu_mbw_mr 42.9 GB/s) + fix major-update summary

* docs(IAM): record admin-only-role delete-stack teardown (DELETE_COMPLETE, no AccessDenied)

* tests: record GPU health check 4/4 PASS on B200 (Mumbai p6-b200)

* tests: list all 18 PCS launch regions in coverage matrix (5 tested, 13 not-run)

* feat: PostInstallScriptUrl accepts s3:// (instance-role fetch) — works with private buckets

Dev accounts can't use public S3, and the post-install hook fetched its script
with curl (anonymous HTTP) — so a private-bucket S3 https URL 403'd and
Enroot/Pyxis silently didn't install. Now the runcmd dispatches by scheme:
  s3://*  → aws s3 cp  (instance role; private bucket OK)
  http(s) → curl       (public only; GitHub raw etc.)
in add-cng.yaml + all 3 GPU templates.

deploy-all: PostInstallScriptUrl default is now empty and, when empty, forwards
s3://${S3BucketName}/${S3KeyPrefix}scripts/install-enroot-pyxis.sh — so the
default install path uses the same bucket (and same IAM-authenticated fetch) as
setup-directory.sh. This unifies all first-party boot scripts on aws-s3-cp
(private-bucket friendly); only the monitoring post-install.sh stays on GitHub
raw, since it pulls from the external aws-parallelcluster-monitoring repo.

Updated the param descriptions (4 templates) and PARAMETERS.md: dropped the
stale "S3 hosting is not allowed" / "empty = skip" wording; empty now = auto
Enroot/Pyxis from the templates bucket, single space = skip.

Also: gpu-healthcheck-test.md trimmed to PCS-specific deltas (defer suite
mechanics to the suite's own README, per the reuse-don't-duplicate convention).

* docs: fix stale 'PostInstallScriptUrl empty = skip' wording + DcgmExporterImage anchor

After PostInstallScriptUrl's default flipped to empty=auto-install (s3://), several
docs still said empty = skip / cleanest boot. Corrected to: empty = auto-install
Enroot/Pyxis from the templates bucket (idempotent no-op on a pre-baked AMI); a
single space = skip. Fixed: README §8.4 (was internally contradictory — default IS
empty), OPERATIONS.md pre-baked-AMI note, tests/infra-test.md (4 spots incl. the
"log absent" assertion that would have failed with empty).

Also fixed PARAMETERS.md DcgmExporterImage anchor (single→double hyphen for the
em-dash heading slug). Found via a full docs-consistency re-audit of the
PostInstallScriptUrl/VPCName/EFA/MonitoringStack/IAM changes.

* tests: add docs-consistency lint (tests/lint-docs.sh) as pre-merge Test 0

A runnable docs⇄template lint, the docs counterpart of template-lint — runs in
seconds with no AWS. Catches the drift that bit us this PR (PostInstallScriptUrl
semantics changed, 6 docs went stale): it fails on removed/renamed param names
appearing as current (OnDemandEnableEfa, GrafanaPublicAccessCidr, the
DeployMonitoring=true deploy-all usage, "S3 hosting not allowed", iam/ dir refs),
on `PostInstallScriptUrl=""` shown as skip, on any deploy-all parameter missing a
PARAMETERS.md row, and on broken README internal anchors. Allow-context regexes
permit explicit "(renamed from…)" / history notes. Added as Test 0 in the
pre-merge matrix. Passes clean on the current docs.

* tests: add NCCL busbw validity note (377 GB/s is reasonable, not peak-tuned)

* tests: move p6-b200 NCCL/FSDP results out of README into their own test files

README is the index/matrix — detailed results belong with each test:
- NCCL all_reduce busbw (p6-b200 ×4, 377 GB/s peak + validity note) → compute-test.md Test 6
- FSDP: README only kept distributed-stack-works + a long HF-429 troubleshooting
  saga; per review that triage detail isn't test-doc material. Replaced with a
  concise "HuggingFace rate limits" caveat in training-test.md Test 7 (use a
  higher-rate HF account / mirror / pre-tokenized /fsx data for large runs).
- README region-coverage section now just points to those files.

* tests: rebuild region-coverage table + record s3:// PostInstall and Osaka OpenZFS findings

- Region coverage table reworked to the requested columns: Deploy / Mon /
  Storage (with the OpenZFS deployment type that worked) / Pyxis / GPU (CB·ODCR)
  / Verified date. 10 regions tested.
- **Osaka (ap-northeast-3) does NOT support OpenZFSDeploymentType=SINGLE_AZ_HA_2**
  (default deploy fails: Invalid deploymentType) — documented the SINGLE_AZ_1
  fallback. The other regions use the SINGLE_AZ_HA_2 default.
- infra-test.md Test 2: noted the default PostInstallScriptUrl is now s3:// and
  verified it installs Enroot/Pyxis from a PRIVATE bucket via the instance role
  (no public S3 needed — dev-account friendly).

* tests: add Megatron-LM GPT-3 (Test 7b) + intensive GPU health check results

- training-test.md: Test 7b Megatron-LM GPT-3 TP/PP/DP on p6-b200 x4
  (~134 TFLOP/s/GPU, lm loss 10.91->10.46, EFA efa-direct 8 nics)
- gpu-healthcheck-test.md: intensive suite EFA loopback PASS on p6-b200;
  caveat that check 5 (NCCL) needs the ECR '#' URI + in-image binary path,
  so validate multi-node NCCL via canonical Test 6
- README.md: matrix row 7,7b + Megatron canonical-asset row

* docs(roadmap): add targeted ODCR support for GPU node groups

Capture the follow-up to make CapacityReservationId work for targeted ODCRs
(not just Capacity Blocks): a CapacityReservationType enum
(none/capacity-block/targeted-odcr) that branches MarketType + placement group,
with none/capacity-block staying backward compatible. Notes how to verify
without GPU capacity (MinCount=0 launch-template assertion + cheap-type
consumption check).

* docs(readme): user-facing cleanup — surface new params, fix accuracy, slim advanced detail

- §4 Configuration: add DirectoryService / SSHAccessCidr / GrafanaAccessCidr /
  AdditionalSubnetAZ2-3 rows so the new user-facing features are discoverable
- §4: fix HomeThroughput AllowedValues (SINGLE_AZ_HA_1 includes 64)
- §1: correct capacity wording (open ODCR auto-consumed; targeted ODCR on roadmap)
- §9: p6-b300 row shows 17 interfaces (16 EFA + 1 ENA), consistent with §4
- move EFA-on-CPU + FSx-Lustre-EFA into Advanced (§8.5/§8.6); consolidate the
  DCGM version detail into a note at the end of §8.1 Monitoring
- §8.2: note the single-login-node / SPOF constraint for DirectoryService
- §7: replace specific NCCL busbw numbers with a pointer to tests/compute-test.md
- §4: document OnDemandInstanceType + PseriesInstanceType accepted values

* docs: fix OpenZFS throughput values, note Slurm-version paths, drop stale diagram memo

- OPERATIONS.md: correct OpenZFS HomeThroughput allowed values (192/384/768 are
  not valid AWS values); SINGLE_AZ_2 groups with HA_2 (160..10240),
  SINGLE_AZ_HA_1/SINGLE_AZ_1 = 64..4096 — matches the template Rules
- USER-MANAGEMENT.md: note that slurm-25.11 paths must become slurm-25.05 when
  deployed with SlurmVersion=25.05
- delete docs/architecture-components.md: an orphan diagram-drafting memo (not
  linked anywhere, the architecture image is already published) with stale
  content (32x EFA on all GPUs, single private subnet, optional Enroot/Pyxis)

* docs: align README config order with deploy-all, refresh features/architecture, slim test memos

README:
- §1 Key Features: add multi-user (OpenLDAP), access control (IAM + SSH/Grafana
  CIDR), and multi-AZ / broad Region coverage
- §2 Architecture: subnets across up to 3 AZs, SSH/Grafana CIDR access, and the
  optional OpenLDAP / IAM add-ons
- §4 Configuration: reorder the parameter table to match the deploy-all console
  parameter-group order (Network → PCS Cluster → On-Demand → GPU → Additional);
  point OnDemandEfaInterfaceCount at §8.5

tests/README:
- drop the one-off 'GPU health check verified on B200' memo and the Mumbai
  'exercised most deeply' paragraph (results live in the per-test files)
- region-coverage GPU column is now a simple ran-on-reserved-capacity check
  (us-east-1/us-east-2/us-west-2 verified via Capacity Block for ML); drop the
  per-instance-type detail from the table

* docs: drop multi-AZ/Region key-feature bullet, note GPU column is from earlier rounds

- README §1: remove the 'Multi-AZ and broad Region coverage' Key Features bullet
  (multi-AZ stays documented in §4 / Architecture; not a headline feature)
- tests/README region coverage: clarify the GPU column ✅ reflects earlier GPU
  validation rounds, not necessarily the row's deploy/monitoring verification date

* docs(readme): keep Key Features concise — drop param names, link to detail sections

Key Features bullets describe capability + link to the relevant section instead
of embedding parameter names (MonitoringStack/GrafanaAccessCidr/DirectoryService/
SSHAccessCidr). Detailed config stays in §4 and §8.

* docs(readme): tidy §4 intro — single concise lead-in, group separators in the table

- replace the two overlapping intro paragraphs with one: only PrimarySubnetAZ is
  required; the table is the most-used subset grouped by the console's parameter
  groups (separator rows), with storage in its own table and the full reference
  in PARAMETERS.md
- keep the in-table group separator rows for discoverability

* deploy-all: regroup params — fold Container Runtime into Additional, move FSx after it

Container Runtime (PostInstallScriptUrl/Args) is rarely touched directly, so it no
longer gets its own console group: folded into '5. Additional Cluster Configuration
(Monitoring, Multi-User, Container Runtime)'. FSx Storage moves after Additional
(now group 6). New order: Network, PCS Cluster, On-Demand, GPU, Additional, FSx,
Developer. README §4 separators and PARAMETERS.md §2b updated to match.

* docs(readme): tighten Architecture/Monitoring, link GPU health check from §7

- move the PCS-Ready DLAMI 'no AMI build needed' note from Key Features into §2
  Architecture, pointing to §4 'AMI and container runtime' for the detail (dedup)
- §7: link the GPU Cluster Health Check suite as a pre-long-run check
- §8.1 Monitoring: replace the MonitoringRepo/Version/DcgmExporterImage bullets
  with a one-line pointer to PARAMETERS.md (users rarely change these); move the
  monitoring-role vs Name-tag note to the end of §8.1

* docs(readme): split §4 config table into small per-group tables

Replace the single long table (with in-table separator rows and verbose Purpose
cells) with one small table per console parameter group (Network / PCS cluster /
On-Demand / GPU / Additional). Trim each Purpose to one line and move the long
PseriesInstanceType accepted-values list to the GPU compute subsection. Easier to
scan; group structure is obvious from the subheadings.

* docs(readme): number §4 config groups, move Storage to Advanced §8.1 with deploy-small tip

- §4 config sub-tables now carry the console group numbers (1 Network … 5 Additional)
- move the FSx deployment-types/sizing content out of §4 into a new §8.1 Storage at
  the top of Advanced Features; renumber the rest (Monitoring 8.2 … FSx-EFA 8.7) and
  fix all §8.x anchor references in README + PARAMETERS.md
- add a 'deploy small, expand after' tip: FSx Lustre/OpenZFS can be grown after create,
  and a smaller filesystem deploys faster — start near minimum Capacity/HomeCapacity and
  expand once the stack is CREATE_COMPLETE

* docs: drop test-profile flags, align PARAMETERS with deploy-all groups, refocus DEPLOY-TESTING, move IAM verification to tests

- remove --profile claude (project test setting) from DEPLOY-TESTING.md and
  tests/infra-test.md; generalize fixed bucket/region to placeholders
- DEPLOY-TESTING.md rewritten for the real audience: a third party deploying
  not-yet-published templates by hosting them in their own S3 bucket and pointing
  S3BucketName/S3KeyPrefix at it
- PARAMETERS.md sections + order now match the deploy-all console parameter groups
  exactly (1 Network, 2 PCS Cluster, 3 On-Demand, 4 GPU, 5 Additional incl.
  monitoring repo/version/dcgm + container runtime, 6 FSx, 7 Developer); fix stale
  anchor links
- move the IAM verification results out of docs/IAM.md into a reproducible
  tests/iam-test.md (representative two-role use case); add the row to tests/README

* docs(readme): fold FSx-over-EFA (GDS) into §8.1 Storage as a subsection

The standalone §8.7 'FSx for Lustre over EFA (GPUDirect Storage)' is storage content,
so it's now a #### subsection at the end of §8.1 Storage instead of a separate top-level
Advanced feature. Removes the cross-reference note.

* docs(readme): add §8.7 pointing to DEPLOY-TESTING.md for unpublished-template deploys

Adds an Advanced Features subsection that links docs/DEPLOY-TESTING.md (deploying fork/
branch/PR templates from your own S3 bucket via S3BucketName/S3KeyPrefix). This also
gives DEPLOY-TESTING.md its README entry point — every docs/ file is now linked from the
README.

* docs(readme): fix §4 group 5 heading/content mismatch

The '5. Additional' heading said 'container runtime' but the table only listed the
common params (MonitoringStack/GrafanaAccessCidr/DirectoryService). Retitle to
'(monitoring, multi-user)' and add a one-line note that the rarely-changed
monitoring-source + container-runtime params live in the same console group (see
PARAMETERS.md).

* docs(readme): drop the redundant note under §4 group 5

The 'console's group 5 also holds…' line was redundant with the PARAMETERS.md
pointer right below it. Group 5 is just the heading + the 3 common params.

* docs(readme): match §4 group headings to the deploy-all ParameterGroup names

The §4 sub-table headings now use the exact console parameter-group labels
(1. Network Configuration … 5. Additional Cluster Configuration (Monitoring,
Multi-User, Container Runtime)), and group 5 lists PostInstallScriptUrl so the
heading and its rows agree.

* docs(tests): fix stale PostInstallScriptUrl caveat

The default is no longer the awslabs/main GitHub-raw URL — it's empty, which resolves
to s3://<S3BucketName>/<S3KeyPrefix>scripts/install-enroot-pyxis.sh. Rewrite the caveat:
when testing unpublished templates, point S3BucketName at your own bucket and sync the
scripts there, or first-boot Enroot/Pyxis fetch fails (cluster still CREATE_COMPLETE).
Link DEPLOY-TESTING.md.

* docs(tests): remove private test-bucket name from infra-test.md

Generalize the s3:// verification note and Test 8 build/deploy commands to use
<bucket>/<prefix> placeholders instead of the project's private midaisuk-llm-dev
bucket, matching DEPLOY-TESTING.md.

* docs: add Deploy buttons to IAM role table; move custom-AMI detail to docs/CUSTOM-AMI.md

- IAM.md: add 1-click Deploy buttons to the two-role summary table (matching the
  deploy table further down)
- move the §8.5 step-by-step pre-bake procedure into docs/CUSTOM-AMI.md; README §8.5
  keeps a concise summary + Launch button + link (heading text unchanged so the
  #85-... anchor referenced elsewhere stays valid)

* docs(iam): use the Launch-stack image for the policy Deploy buttons, single location

Match the custom-AMI style: the cluster-admin / cluster-user Deploy buttons now use the
launch-stack.svg image, kept in one place (the 'Deploying the policies' table); drop the
duplicate kbd buttons from the role summary table.

* docs(readme): use the Launch-stack image for all §9 template Deploy buttons

Replace the kbd 🚀 buttons in the Templates table with the launch-stack.svg image,
consistent with the Quick Start, §8.5 custom-AMI, and IAM policy Deploy buttons.

* docs(readme): add Launch-stack Deploy buttons to §8.4 IAM Permissions

Turn the two-role bullet list into a table with per-role Launch-stack Deploy buttons,
consistent with §9 Templates, §8.5, and docs/IAM.md.

* docs: pre-PR polish — number test headings to match the matrix, clarify NIC wording, flag CB billing

- tests: number the file headings (Tests 11-12 / Test 13 / Test 14) so they line up
  with the test matrix in tests/README.md
- compute-test: '16 cards' → 'the 16 EFA NICs' to avoid confusion with GPU count
- README §4: add a CB-billing ⚠️ to the CapacityReservationId row (links to GPU compute)

* docs(readme): drop the Capacity Block billing note (cost is out of scope for this doc)

* fix(aws-pcs): address PR review blockers — scope LDAP secret to login node, fix suffix truncation

Blocker 1 (directory takeover): the shared instance role let every compute node
read+decrypt the OpenLDAP admin password (/pcs/<id>/ldap/* SSM SecureString), so a
user job could assume the role via IMDS and take over the directory. Split the role:
the base profile (all compute CNGs) now grants only grafana/* SSM; a new login-only
role/profile (PcsLoginInstanceIamRole/Profile) carries ldap/* SSM + kms:Decrypt, and
deploy-all passes it to LoginNodeGroupStack only. SSSD on compute uses an anonymous
bind, so compute never needs the secret. MonitoringInstancePolicy now attaches to both
roles so the login node still gets monitoring perms.

Blocker 2 (suffix truncation): ldap-add-user.sh derived LDAP_DOMAIN_SUFFIX with
'awk -F=' which split on the '=' inside the value (dc=cluster,dc=internal -> dc),
breaking the documented add-a-user path. Use sed to strip only the key, keeping the
value intact.

* fix(aws-pcs): scope cluster-user ssm:StartSession documents separately from the login-node tag

The PCSClusterUser policy gated the SSM session-document ARNs (AWS-StartSSHSession,
AWS-StartPortForwardingSession*, SSM-SessionManagerRunShell) under the same
ssm:resourceTag/Name=PCS-login* condition as the instance ARN. Documents carry no Name
tag, so StringLike fails closed on the document sub-authorization and ssm:StartSession
with a custom document is denied — breaking SSH-over-SSM and port-forwarding for cluster
users (a plain Session Manager shell still works, so a smoke test passes).

Split into a tag-gated instance statement (SSMSessionToLoginNodeInstance) + an untagged
document statement (SSMSessionDocuments), matching AWS's Session Manager tag-restriction
pattern. Login/compute isolation is unchanged — the instance statement still scopes to
PCS-login*.

Verified with iam simulate-custom-policy (before -> after):
  SSH document:     implicitDeny -> allowed
  login instance:   allowed      -> allowed
  compute instance: deny         -> deny

Claude-Session: https://claude.ai/code/session_014tqpywkVSmnUMJGNYfogmF

* fix(aws-pcs): login instance role name must start with AWSPCS (PCS requirement)

The login-only role added for the LDAP-secret split got a CFN-generated name that
did not start with AWSPCS, so CreateComputeNodeGroup rejected it: 'AWS PCS can't
access a role associated with the instance profile because the role ARN is invalid'.
PCS requires the instance-profile role name to start with AWSPCS (or use path
/aws-pcs/). Give PcsLoginInstanceIamRole an explicit AWSPCS-pcs-login-<hash>-<region>-role
name (mirroring the base role) + DependsOn on the profile.

* fix(aws-pcs): hard-deny LDAP admin password on compute role (managed-key bypass)

Splitting ldap/* SSM + kms:Decrypt onto a login-only role was NOT sufficient: the
compute role keeps AmazonSSMManagedInstanceCore (ssm:GetParameter on *), and the
SecureString is encrypted with the AWS-managed key alias/aws/ssm whose key policy lets
any in-account principal decrypt via SSM. Verified on a live cluster that a compute
node could still 'get-parameter --with-decryption' the OpenLDAP admin password despite
having no kms:Decrypt in its own role.

Add an explicit Deny on ssm:GetParameter*/GetParametersByPath for /pcs/<id>/ldap/* to
the shared/compute base policy (explicit Deny overrides the managed-policy Allow). The
login-only role omits the Deny. Re-verified live on a fresh compute node:
COMPUTE_READ_LDAP=BLOCKED, login still reads it, grafana still readable.

* fix(aws-pcs): drop redundant inline S3GetScripts; note S3ReadOnlyAccess opt-in on roadmap

The inline S3GetScripts statement added on this branch was redundant: the instance
role already attaches AmazonS3ReadOnlyAccess (s3:Get*/List* on *), so a narrower
templates/scripts/* grant added nothing — and the wildcard bucket the review flagged was
moot under the broader managed policy. Remove both inline statements (base + login role).

The real over-grant is AmazonS3ReadOnlyAccess itself: upstream aws-hpc-recipes makes it
opt-in (EnableS3ReadOnly, off by default) but ml-pcs attaches it unconditionally. That's
a pre-existing (#1120) IAM-behaviour change with workload impact (FSDP/Megatron read S3),
so it's tracked on ROADMAP rather than changed in this PR.

* fix(aws-pcs): address non-blocking review items — require uid, MIT-0 headers, drop dead DirectoryService param

- ldap-add-user.sh: require an explicit uid (was a bounded RANDOM default that, with
  bash RANDOM capped at 32767, could not span the intended range and risked uidNumber
  collisions = same POSIX principal on shared /home,/fsx)
- add MIT-0 SPDX headers to ldap-add-user.sh, setup-directory.sh, tests/lint-docs.sh
  (matching install-enroot-pyxis.sh and the IAM templates)
- remove the dead DirectoryService parameter from add-cng*.yaml (only DirectoryRole /
  DirectoryDomainSuffix are used in their UserData); drop the now-unused pass-through
  from deploy-all (deploy-all keeps the top-level DirectoryService, which drives the
  DirectoryEnabled condition / DirectoryRole)

* docs(user-management): modular deploy — pass LoginInstanceProfileArn to the login CNG

Two updates to the modular-deployment section: (1) add-cng no longer takes
DirectoryService (dead param removed) — pass DirectoryRole/DirectoryDomainSuffix only;
(2) document that the login CNG must get LoginInstanceProfileArn (not the compute
InstanceProfileArn) when multi-…
@DaisukeMiyamoto
DaisukeMiyamoto deleted the deploy-monitoring branch June 21, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants