docs(aws-pcs): improve JUPYTER + USER-MANAGEMENT + IAM walkthroughs (verbatim-verified) - #1222
Conversation
…im-verified)
Two doc rewrites bundled to save review roundtrips — both fell out of
end-to-end verbatim replays on live Tokyo/us-east-2 clusters, so every
recipe below has been executed from the doc text alone.
## docs/USER-MANAGEMENT.md — full rewrite
- Frame the doc around the single-user default first; multi-user
becomes an opt-in section, so readers who don't need OpenLDAP can
close the tab immediately.
- New §2 "Adding your first LDAP user" walkthrough: recover admin
password (§2.1) → add user with SSH key (§2.2) → log in (§2.3, both
direct SSH and SSH-over-SSM via the PCS-API login lookup) → verify
on a compute node as the user (§2.4).
- §2.1 admin-password recipe now takes CLUSTER_ID/AWS_REGION from IMDS
on the login node (no reader-supplied "pcs_xxxxxxxx" placeholder).
- §3 "Detailed operations" restructured as a complete operator toolkit
(add / list / delete / reset-password / create-group / add-member),
all using `-W` / `-W -S` prompt paths so nothing sensitive lands in
shell history.
- §4 "Slurm accounting" split into 4.1 Register / 4.2 Limits and
enforcement / 4.3 Reporting. §4.1 reproduces the two-project scenario
from AWS's "Introducing managed accounting for AWS PCS" blog; §4.2
documents `associations,limits,safe` accepts+pends rather than
rejects (contra the blog); §4.3 keeps the two `sacct`/`sreport`
tool-behaviour notes verified in earlier live testing.
- Explicit callouts: console vs CLI deploy paths, submit-from-$HOME,
`--time=<mm>` required when a GrpTRESRunMins quota is set.
## assets/scripts/ldap-add-user.sh — reject duplicate UID/username
Same helper the walkthrough drives. LDAP does NOT enforce
uidNumber uniqueness (it's not part of the DN), so
`ldap-add-user.sh bob 10001 …` after alice at 10001 silently produced
two users sharing UID 10001 = same POSIX principal on shared /home + /fsx.
Pre-check with an unauthenticated ldapsearch; refuse the add with a
clear stderr message. Live-verified: dup UID and dup name both trip
the guard; a distinct UID creates cleanly.
## assets/cluster.yaml — ClusterName default `pcs-ml-cluster`
Aligns with the README/quick-create link's stack name so a first-time
user gets the same identifier whichever deploy path they follow.
deploy-all continues to pass `${AWS::StackName}` through, so this
default only affects direct `cluster.yaml` stacks.
## docs/JUPYTER.md — preinstall torch, GPU verify cell, tokenless mode, safer login lookup
- Step 1 also installs `torch --index-url .../cu124`, so the first
notebook can `import torch` without an install/restart roundtrip.
- Step 2 explicitly `cd $HOME` before `sbatch`. Slurm's job CWD is
the submitter's, and `--output=%u-jupyter-%j.log` resolves against
that; submitting from an SSM shell (`/var/…`) sent the log where
Step 3 couldn't find it (reproduced live).
- Step 3 replaces the leading-wildcard `Name=*login` login filter
and stale `ssm:resourceTag/Name = PCS-login*` IAM callout with the
cluster-scoped PCS-API resolver (matches README §6 / awslabs#1183 pattern).
- New "Verify GPU visibility from the notebook" cell prints
CUDA_VISIBLE_DEVICES / SLURM_JOB_GPUS, enumerates PyTorch device
properties, runs a matmul. Verified live on g6.12xlarge:
device_count()=1, NVIDIA L4 22 GiB SM 8.9, matmul completes.
- New "Running without a token (single-user clusters only)" section
documents the `--ServerApp.token='' --ServerApp.password=''` form
with a prominent DO-NOT-USE-ON-MULTI-USER callout — Jupyter binds
to the compute node's private IP, so the token is what separates
users on multi-user clusters.
## tests/multi-user-test.md — regression coverage for the above
- B5a: duplicate uidNumber / username rejected by ldap-add-user.sh
(guards commit 6f3b6ad).
- B7: password reset via `ldappasswd -W -S` interactive prompt.
- B8: group create + `add: memberUid` via ldapadd/ldapmodify -W.
- B9: end-to-end SSH-over-SSM as an LDAP user with PCS-API instance
lookup, then `srun` on a compute node.
- Part F: reproduces the blog scenario (project accounts, per-user
cap, --account=, safe-enforcement PD, `sacct`/`sreport` recipes).
## Verified
Doc-only re-run on a fresh Tokyo (ap-northeast-1) `pcs-ml-cluster`
stack deployed from the public bucket with
`DirectoryService=OpenLDAP-LoginNode`, `ManagedAccounting=enabled`,
`MonitoringStack=Prometheus-LoginNode`. Every fenced block above
executed as written (§6 login lookup, §7 srun container, §8.2
Grafana password + Option A monitoring-role lookup;
USER-MANAGEMENT §2/§3/§4/§5; JUPYTER Step 1/2/3 auth gates,
403/200/200, token file mode 600). GPU verify cell exercised on a
separate g6.12xlarge cluster.
…dancy Two rounds of tightening; content preserved, ~30 lines of overlap removed. - Merge the header "roles" table and the "Deploying" stacks table: they keyed the same two rows to different columns (What can do / Creates, Template link / Launch button). Fold into one "Role · Can do · Deploy" table where each row carries both the YAML link and the Launch button, and inline "broad — do not hand out" / "safe to hand out widely" callouts absorb the standalone splitting-the-roles paragraph. - §Deploying the policies: shorten the AttachUsers / AttachImageBuilderPolicy paragraph — the details are already in the CLI example above and the template's parameter descriptions. - §What the cluster user can do: drop the second CLUSTER_ID resolve (already resolved once at the top of the block). - §Considerations "Login-node access is scoped": trim implementation detail about the Name-tag emission chain (readers can check the template); keep the exact-match scope and the operator-mutable caveat. - Merge "admin policy split" and "no AmazonPCSFullAccess" bullets — same message (why the customer-managed policies look the way they do). - §Verifying: shorten the private-bucket callout to one sentence.
The torch install in Step 1 is prerequisite for the GPU-visibility verify cell below, not for running Jupyter on a GPU queue (the queue picks the queue; the kernel doesn't need torch to launch). Reframe: - Step 1: "For GPU work, also install PyTorch now" → "To run the GPU-visibility verify cell below, also install PyTorch". - Step 2: drop the "For GPU work, the venv already has torch" note (misleading — a GPU job doesn't require torch). Keep the pointer to Using GPUs for allocation behaviour.
Post-review pass. Content preserved, ~90 lines of redundancy removed. - §5.3 "Invalid credentials from ldap*": deleted — it was a one-liner redirecting to §2.1. Folded the redirect into §2.1's admin-password fallback callout so a reader who mistypes at a -W prompt still sees the recovery path in situ. - §5.4/§5.5 renumbered to §5.3/§5.4. - §6 "How it works": collapsed the ASCII diagram plus the separate "tag-based discovery" sub-section into one bullet list plus a login-node-replacement recovery snippet. Roughly a third of the previous size, same operational information. - §8 "UID / GID conventions": absorbed into §3.1 (Add a user), which was already pointing at it for the allocation policy; the table now sits next to the invocation it informs. - §9 "Template structure": deleted — its ASCII diagram restated §1 "modular deployment" (which already documents DirectoryRole / IamProfileArn per CNG type). - §10 "Upgrading to AWS Simple AD (future)": deleted — placeholder, no operational content, still tracked in ROADMAP.md.
- Using GPUs: put "How the GPU allocation behaves" before the verify cell (allocation is the frame, verify is the check). Collapse the bullets: --gres is the only knob (merges the old "Slurm enforces" and "Multi-GPU works" points), drop scontrol-example detail, drop the p5.48xlarge measurement note, keep containerized-kernel pointer as one line. - Notes section removed. The Multi-user/token bullet duplicated the Step 2 comment on the token being the user boundary; the Multiple-clusters bullet was stale (the Step 3 lookup no longer uses Name=*login); the SSH-tunnel alternative and /fsx-execve caveat are out of the load-bearing flow. "Do not run Jupyter on the login node" moved up into Prerequisites where readers will hit it before submitting.
…r + verify section Post-review pass, ~80 more lines out. - Table: collapse "Template" and "Launch" into one Launch column; the Launch button already resolves to the same YAML, listing the file path next to it was redundant. Move "Can do" into the Role cell so the table stays two-column. - §Deploying the policies (CLI example) removed. The Launch buttons in the table cover the deploy path; the parameter-choice notes (AttachUsers, AttachImageBuilderPolicy, ClusterStackName) fit in one paragraph under the table. - §What the cluster user can do (~25-line CLI example) removed. The same session / port-forward / Grafana-password recipes live in README §6 / §8.2, and this doc's job is the policies, not a copy of the user runbook. - §Considerations: drop the "Combined CRUD is intentional" paragraph (internal design detail; only matters when tweaking the admin policy, in which case reading the template beats reading prose). - §Refining to least-privilege via CloudTrail removed. It's an advanced follow-on with its own tooling, not a policy-reference concern. - §Verifying the policies removed. The one-liner pointer to iam-test.md was thin, and the private-bucket note gets absorbed into §Considerations. - §Not covered by these policies: fold into §Considerations as a short bullet list.
- Drop "Login-node access is scoped to one stack": the exact-match ssm:resourceTag condition + operator-mutable Name caveat is already documented in cluster-user-iam.yaml's Description and inline comments where a reader tightening the policy will actually read it. - Drop "The admin policy is split into core + Image Builder": internal reason (IAM 6,144-char limit) doesn't lead to a reader action — AttachImageBuilderPolicy handling is already noted under the table. - Drop "Private template bucket": edge case for private-bucket hosting; those users hit the missing s3:GetObject themselves and it's unrelated to reference-policy design. Keeps: sample-grade disclaimer + AWS reference link, AWS-managed pairing hints (with the AmazonEC2FullAccess warning), and the list of roles this template doesn't cover.
Document every published x86_64 PCS-Ready DLAMI build with its PCS Agent, Slurm, base DLAMI, driver/CUDA, DCGM, EFA and containerd versions, so readers can pick a build (and know whether node lifecycle actions, which need agent 1.5.0-1, are available).
…EADME §7 Adding a short pointer so the srun example in §7 does not silently fail on clusters deployed with AccountingPolicyEnforcement set — the submitter (including ubuntu) has to be registered in sacctmgr first. Details stay in USER-MANAGEMENT.md §4.
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 1/5 — Slurm Accounting Runbooks (USER-MANAGEMENT §4 + multi-user-test Part F)
This is a strong docs refresh overall — the §2 first-user walkthrough and the JUPYTER step fixes are exactly the kind of friction removal these docs needed, and the live-verifiable claims I could check all held up (details in the final batch). The one area that doesn't meet the PR's own "verbatim-verified" bar is the new accounting material: Part F and USER-MANAGEMENT §4.2 each contain commands whose documented expected output is unreachable as committed — details in the 7 inline comments of this batch. The common thread is that Part F cannot have completed as written, so I'd suggest re-running it once these are fixed and noting the run in the test plan.
| ```bash | ||
| sudo LDAP_ADMIN_PASSWORD="$ADMIN_PW" ldap-add-user.sh carol 10003 3000 | ||
|
|
||
| sudo sacctmgr -i add account proj_physics Description="Physics group" |
There was a problem hiding this comment.
Part F's sudo sacctmgr invocations fail under secure_path — the section's own sibling doc explains why (blocking)
F1, F3, and F5 invoke bare sudo sacctmgr (lines 620–626, 654, 667, 692–696 — including cleanup). USER-MANAGEMENT §4 — added in this same PR — explains precisely why that form fails: Ubuntu's sudo resets PATH via secure_path, so "sudo sacctmgr from a shell where you only export PATH=… still fails to find the binary", and §4.1 accordingly uses sudo $S -i … with the absolute path throughout. As committed, every accounting mutation in Part F — including the F5 cleanup — dies with sudo: sacctmgr: command not found, which also means Part F can't have run verbatim in this form. I'd suggest opening Part F by binding S=/opt/aws/pcs/scheduler/slurm-25.11/bin/sacctmgr (as §4 does) and using sudo $S -i … in F1/F3/F5. Could you re-run Part F after the fix and confirm?
|
|
||
| ```bash | ||
| sudo su - alice -c 'export PATH=/opt/aws/pcs/scheduler/slurm-25.11/bin:$PATH; \ | ||
| echo "hostname; id" > /home/alice/smalljob.sh && chmod +x /home/alice/smalljob.sh; \ |
There was a problem hiding this comment.
F2's batch script has no shebang — sbatch rejects it before submission (blocking)
echo "hostname; id" > /home/alice/smalljob.sh produces a script whose first line is not #!…. sbatch hard-rejects such scripts at submit time — src/sbatch/sbatch.c in the Slurm source: "This does not look like a batch script. The first line must start with #! followed by the path to an interpreter." So F2's expected Submitted batch job <n> / COMPLETED output is unreachable. The existing C3 sidesteps this with --wrap — either form works:
| echo "hostname; id" > /home/alice/smalljob.sh && chmod +x /home/alice/smalljob.sh; \ | |
| printf '#!/bin/bash\nhostname; id\n' > /home/alice/smalljob.sh && chmod +x /home/alice/smalljob.sh; \ |
| ### F1. Project accounts and quota | ||
|
|
||
| ```bash | ||
| sudo LDAP_ADMIN_PASSWORD="$ADMIN_PW" ldap-add-user.sh carol 10003 3000 |
There was a problem hiding this comment.
F1 creates carol with UID 10003 — already taken by charlie, so the PR's own new guard aborts the scenario (blocking)
Part E (line 580) creates charlie 10003 3000. F1 then unconditionally runs ldap-add-user.sh carol 10003 3000 — and the duplicate-uidNumber guard this very PR adds to ldap-add-user.sh now (correctly!) refuses it: uidNumber 10003 is already used by 'charlie'. On a directory where Part E ran, Part F aborts at its first command. The prerequisites line ("add carol here if only alice/bob exist") also conflicts with the unconditional add — if carol exists, the username guard trips instead. I'd pick the next free UID and make the add conditional:
| sudo LDAP_ADMIN_PASSWORD="$ADMIN_PW" ldap-add-user.sh carol 10003 3000 | |
| getent passwd carol >/dev/null || sudo LDAP_ADMIN_PASSWORD="$ADMIN_PW" ldap-add-user.sh carol 10004 3000 |
| ```bash | ||
| srun -N 1 -n 1 -p cpu1 bash -c 'sudo sss_cache -E; sleep 2; getent passwd alice' | ||
| # As alice | ||
| sbatch --account=proj_physics --nodes=1 --ntasks-per-node=8 --time=30:00 myjob.sh |
There was a problem hiding this comment.
§4.2's hold demo can't trigger: the scripts don't exist, -p is missing, and 240 < 6000 (blocking)
Three independent problems make the documented squeue output (PD AssocGrpCPURunMinutesLimit) unreachable as written:
- Neither
myjob.shnorsmalljob.shis created by any prior step — the firstsbatchfails withUnable to open file myjob.sh. - Neither
sbatchpasses--partition, and this repo's own docs (JUPYTER.md Step 2, README §7) are emphatic that PCS clusters have no default partition — a baresbatchis rejected with "invalid partition specified". (The expectedsqueueoutput even showscpu1.) - The quota math: the limit just set is
cpu=6000CPU-minutes, but the "over-budget" job requests 8 CPUs × 30 min = 240 CPU-minutes — it fits with room to spare, so no hold occurs. The AWS blog this section reproduces triggers the hold only because ~95 CPU-hours had already been consumed in its narrative; that precondition doesn't hold here. Part F's own F3 gets this right by tightening the limit tocpu=60first.
I'd suggest mirroring F3: --wrap='sleep 1000', -p cpu1, and either tighten the limit below the request or state the prior-usage precondition explicitly.
|
|
||
| Registering users in accounting is optional unless the cluster was | ||
| deployed with `AccountingPolicyEnforcement=associations,limits,safe` | ||
| (or the stricter `associations,limits`) — with either enforcement on, |
There was a problem hiding this comment.
§4's "the stricter associations,limits" mode isn't deployable from these templates (should fix)
§4 intro and the §4.2 callout both describe AccountingPolicyEnforcement=associations,limits (no safe) as the stricter alternative that rejects at submit time, and multi-user-test F3 repeats it. But both templates only allow none and 'associations,limits,safe' — see the AccountingPolicyEnforcement AllowedValues in assets/cluster.yaml and assets/pcs-ml-cluster-deploy-all.yaml — so a reader following this doc into a deploy hits CloudFormation parameter validation failure, and the claimed submit-time-rejection behavior of the strict mode can't have been verified on a cluster from these templates. I'd either scope the text to what the templates offer ("the strict variant exists in Slurm but isn't offered by these templates") or add the AllowedValue and verify the behavior before documenting it.
|
|
||
| sudo sacctmgr -i modify user alice set GrpTRESRunMins=cpu=6000 | ||
|
|
||
| sacctmgr show user alice bob carol format=User,Account,DefaultAccount,GrpTRESRunMins |
There was a problem hiding this comment.
F1's sacctmgr show omits WithAssoc — the very flag §4.1 explains is required (nit)
format=…Account,DefaultAccount,GrpTRESRunMins requests association-level fields, and USER-MANAGEMENT §4.1 (this PR) adds the sentence "WithAssoc joins the user's association row so per-account attributes actually appear in the output" — but F1's own show command drops the flag, so the expected verification ("alice's GrpTRESRunMins column shows cpu=6000") won't render:
| sacctmgr show user alice bob carol format=User,Account,DefaultAccount,GrpTRESRunMins | |
| sacctmgr show user alice bob carol WithAssoc format=User,Account,DefaultAccount,GrpTRESRunMins |
|
|
||
| # Monthly cluster utilization by account/user | ||
| sreport cluster AccountUtilizationByUser \ | ||
| start=2026-04-01 end=2026-04-30 -t percent \ |
There was a problem hiding this comment.
sreport end dates exclude the final day — "monthly" reports drop April 30 / March 31 (nit)
sreport date-only times default to 00:00:00 of that day (https://slurm.schedmd.com/sreport.html), so end=2026-04-30 cuts off at midnight entering Apr 30 — the last day of the month is excluded (same for end=2026-03-31 on line 472, and for end=$(date +%Y-%m-%d) in multi-user-test F4, which excludes today's just-submitted jobs — precisely the ones the test wants to see). Use the first day of the next month / end=now:
| start=2026-04-01 end=2026-04-30 -t percent \ | |
| start=2026-04-01 end=2026-05-01 -t percent \ |
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 2/5 — LDAP Test Sequencing & Version Paths
Three sequencing/consistency issues in the new test sections and the operations doc — each breaks a runbook when followed top-to-bottom. Details inline.
| ldapmodify -x -H ldap://localhost -D "cn=admin,dc=cluster,dc=internal" -W -f /tmp/m.ldif | ||
|
|
||
| sudo sss_cache -E && sleep 2 | ||
| id testuser2 |
There was a problem hiding this comment.
B8 verifies membership for testuser2 — deleted two sections earlier in B6 (should fix)
B6 (line ~149) ldapdeletes testuser2 and verifies getent returns nothing. New B8 then adds memberUid: testuser2 to ml-team (LDAP accepts the dangling string — memberUid is unconstrained) and expects id testuser2 to print uid=10002(testuser2) … 3001(ml-team) — impossible for a deleted user, so B8's expected output is unreachable in document order. I'd use testuser1 for the member-add half, or move B6's deletion after B8.
| # Generate a keypair and install its public key on user creation: | ||
| ssh-keygen -t ed25519 -N "" -f ~/.ssh/pcs-testuser1 -C "testuser1@laptop" | ||
| sudo -E ldap-add-user.sh testuser1 10001 3000 "$(cat ~/.ssh/pcs-testuser1.pub)" | ||
| # (Skip if B1 already created testuser1 without a key — add it with: |
There was a problem hiding this comment.
B9's opening block mixes laptop and login-node commands, always fails under the new duplicate guard, and its ldapmodify fallback can't work (should fix)
Three issues in the setup preamble:
- The section header says "From an operator laptop", but
sudo -E ldap-add-user.sh …must run on the login node (the helper lives at/usr/local/binthere and bindsldap://localhost). - In document order B1 has always already created
testuser1, so this add now always exits 1 under the duplicate-username guard added by this same PR — the "(Skip if B1 already created testuser1…)" parenthetical is the mainline case, not the exception. - The suggested fallback "add it with: ldapmodify" can't work: the helper's own comment (
assets/scripts/ldap-add-user.sh, SSH-key block) states keys are intentionally not stored in LDAP — they go to/home/<user>/.ssh/authorized_keys. The working login-node fallback is appending the pubkey there, e.g.echo "<pubkey>" | sudo tee -a /home/testuser1/.ssh/authorized_keys(plus thechmod/chownthe helper does).
I'd restructure: generate the keypair on the laptop, install the key on the login node (helper for a fresh user, authorized_keys append for an existing one), then run the SSH-over-SSM block from the laptop.
|
|
||
| ```bash | ||
| SACCTMGR=/opt/aws/pcs/scheduler/slurm-25.11/bin/sacctmgr | ||
| S=/opt/aws/pcs/scheduler/slurm-25.11/bin/sacctmgr # 25.05 if SlurmVersion=25.05 |
There was a problem hiding this comment.
slurm-25.11 paths are now hard-coded with only a trailing comment — the old version-switch note was deleted (should fix)
The previous doc opened with a prominent blockquote ("Slurm path matches SlurmVersion … replace slurm-25.11 with slurm-25.05 in every path"). This PR removes it; what remains is one # 25.05 if SlurmVersion=25.05 comment on this line — but §3.3's sudo /opt/aws/pcs/scheduler/slurm-25.11/bin/sacctmgr and the export PATH=…slurm-25.11… line carry no caveat at all, so a 25.05 deployment gets No such file or directory with no pointer to why. Cheapest fix that covers every occurrence: derive it once, e.g. SLURM_BIN=/opt/aws/pcs/scheduler/$(ls /opt/aws/pcs/scheduler | grep '^slurm-')/bin, or restore a one-line note at the top of §3.
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 3/5 — Cross-Doc Consistency (anchors & sibling drift)
Five inline comments: two broken anchors, two places where new text contradicts a sibling doc, and one wording nit. Plus one finding in a file this PR doesn't touch:
Complementary fix: tests/jupyter-notebook-test.md still teaches the old Values=*login discovery this PR removes from JUPYTER.md (should fix)
JUPYTER.md Step 3 now resolves the login node via the PCS API — great. Its regression companion tests/jupyter-notebook-test.md:119 still uses --filters "Name=tag:Name,Values=*login", so after this PR the test doc no longer matches the doc it tests (and *login matches any cluster's login node in multi-cluster accounts — the problem the PCS-API recipe exists to avoid). Note tests/lint-docs.sh's banned-pattern only matches the Values=PCS- spelling, so this form passes lint. One hunk to port the same PCS-API snippet there would finish the migration.
| └─────────────────────────────────────────────────────────┘ | ||
| ``` | ||
| 1. Open the deploy-all template's **Launch Stack** link in the | ||
| [README](../README.md#quick-start) (or navigate to CloudFormation |
There was a problem hiding this comment.
../README.md#quick-start — README's heading is "3. Quick Start", so the anchor doesn't resolve (nit)
README.md's heading is ## 3. Quick Start → anchor #3-quick-start; #quick-start opens the README at the top without navigating. (tests/lint-docs.sh's anchor check only covers README-internal anchors, which is why this passed lint — extending it to cross-file ../ anchors would catch both this and the Part F one.)
| [README](../README.md#quick-start) (or navigate to CloudFormation | |
| [README](../README.md#3-quick-start) (or navigate to CloudFormation |
| the blog demonstrates: two project accounts, a per-user cap, `--account=` | ||
| job attribution, quota-based rejection, and the blog's own reporting | ||
| commands. Regression guard for the walkthrough in | ||
| [USER-MANAGEMENT.md §4.1](../docs/USER-MANAGEMENT.md#41-worked-example-project-accounts--per-user-quota--reports). |
There was a problem hiding this comment.
Part F links a USER-MANAGEMENT §4.1 heading that doesn't exist (nit)
The anchor #41-worked-example-project-accounts--per-user-quota--reports matches no heading in this PR's USER-MANAGEMENT.md — §4.1 is "Register users to accounts" (#41-register-users-to-accounts). Looks like a leftover from an earlier draft of the section title:
| [USER-MANAGEMENT.md §4.1](../docs/USER-MANAGEMENT.md#41-worked-example-project-accounts--per-user-quota--reports). | |
| [USER-MANAGEMENT.md §4.1](../docs/USER-MANAGEMENT.md#41-register-users-to-accounts). |
| PCS-Ready DLAMI ships CUDA 12.x, so pin the matching wheel: | ||
|
|
||
| ```bash | ||
| $HOME/jupyter-env/bin/pip install torch --index-url https://download.pytorch.org/whl/cu124 |
There was a problem hiding this comment.
"Ships CUDA 12.x → pin cu124" contradicts this PR's own DLAMI table (CUDA 13.2) — the tested wheel is cu130 (should fix)
PCS-READY-DLAMI.md (added in this same PR) lists Default CUDA 13.2 with a 12.8/12.9/13.0/13.2 stack — 12.4 isn't on the image at all — and the sibling tests/jupyter-notebook-test.md records the verified environment as torch 2.12.1+cu130, driver 595.71.05 / CUDA 13.2. Two things worth fixing: (a) the premise — a venv-installed torch wheel bundles its own CUDA runtime, so what matters is the NVIDIA driver (595.71.05 supports all current wheels), not the image's CUDA toolkit; (b) the pick — since the sibling test already validated cu130, pin that (all of cu124/cu126/cu128/cu129/cu130 exist on download.pytorch.org — verified live, 2026-08-04):
| $HOME/jupyter-env/bin/pip install torch --index-url https://download.pytorch.org/whl/cu124 | |
| $HOME/jupyter-env/bin/pip install torch --index-url https://download.pytorch.org/whl/cu130 |
I'd also reword the preceding sentence — "The PCS-Ready DLAMI's driver (595.xx) supports current CUDA wheels; the cu130 build matches the image's CUDA 13.2 stack" — rather than "ships CUDA 12.x, so pin the matching wheel".
| ``` | ||
|
|
||
| Expected: `torch.cuda.device_count()` **equals the `--gres=gpu:N` you | ||
| requested**. If `nvidia-smi` shows more devices than |
There was a problem hiding this comment.
"File a bug" advice labels the documented-normal GPU visibility as broken (should fix)
tests/jupyter-notebook-test.md documents the verified behavior on these clusters: "the full GPU table for the node (all physical GPUs listed … allocation limits are enforced per-process via CUDA_VISIBLE_DEVICES, not in nvidia-smi)". The new text tells readers that exact state means "Slurm's cgroup isolation is not restricting them — file a bug against the CNG configuration". As written, every reader on the documented configuration is told to file a bug. The reliable gate is the one the cell already checks — torch.cuda.device_count() equals the --gres count; I'd replace the nvidia-smi sentence with "seeing all physical GPUs in nvidia-smi is expected — allocation is enforced per-process via CUDA_VISIBLE_DEVICES".
|
|
||
| The token file (and the `openssl rand` line above it) is no longer needed | ||
| and can be dropped. Connect to `http://localhost:8888/lab` — no token | ||
| required. Nothing else in Step 3 changes. |
There was a problem hiding this comment.
Token-less mode: "Nothing else in Step 3 changes" isn't quite true (nit)
Step 3's sub-step 2 is "read the token" (cat ~/.jupyter-token-<jobid>) — gone in this mode — and the sbatch banner's token = run cat $TOKEN_FILE … line goes stale/empty once the token lines are dropped. Tiny fix: "Steps 1 and 3 of Step 3 are unchanged; skip sub-step 2, and drop the token line from the banner heredoc.
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 4/5 — Security Posture
Four inline comments: one on the token-less Jupyter mode (with a live-verified one-line fix), two on IAM.md claims vs what the policy enforces, and one hardening gap in the new duplicate guard.
| ```bash | ||
| exec jupyter lab --no-browser --ip="$NODE_IP" --port="$PORT" \ | ||
| --ServerApp.token='' --ServerApp.password='' \ | ||
| --ServerApp.disable_check_xsrf=True \ |
There was a problem hiding this comment.
Drop --ServerApp.disable_check_xsrf=True — it's unnecessary for token-less use and removes the last browser-side guard (should fix)
Verified live (JupyterLab 4.6.2, --ServerApp.token='' --ServerApp.password='', XSRF left enabled, 2026-08-04): GET /lab → 200 and sets the _xsrf cookie; the Lab UI's own cookie+header flow then works (POST /api/kernels → 201); a naive cross-site POST without the header → 403 '_xsrf' argument missing. So the token-less recipe works without this flag — and with it, any webpage open in the operator's browser while the tunnel is up can POST to http://localhost:8888 and execute code on the compute node, which the (otherwise excellent) multi-user warning below doesn't cover. Jupyter's security docs already frame no-auth mode as needing another protective layer (https://jupyter-server.readthedocs.io/en/latest/operators/security.html); XSRF is the one layer that costs nothing here:
| --ServerApp.disable_check_xsrf=True \ |
One wording nit in the warning while you're there: "single-user" should mean "every IAM principal who can reach the VPC/run jobs is trusted as the same person" — DirectoryService=none by itself doesn't guarantee a single principal.
| | Role | Launch | | ||
| |---|---| | ||
| | **Cluster admin** — deploys/updates/deletes clusters. Full CRUD on CloudFormation, PCS, EC2 (VPC/SG/launch templates/placement groups/NAT/EIP), FSx, scoped IAM, SSM Parameter Store, KMS, Secrets Manager, and (optionally) Image Builder. **Broad — do not hand to every engineer.** | [](https://console.aws.amazon.com/cloudformation/home#/stacks/quickcreate?templateUrl=https://awsome-distributed-ai.s3.amazonaws.com/templates/aws-pcs/cluster-admin-iam.yaml&stackName=pcs-cluster-admins) | | ||
| | **Cluster user** — engineers running jobs on an existing cluster. SSM session to the **login node only**, port-forward Grafana, read the Grafana password, read PCS cluster/queue status. Cannot create, modify, or delete anything, and cannot shell into compute nodes. **Safe to hand out widely.** | [](https://console.aws.amazon.com/cloudformation/home#/stacks/quickcreate?templateUrl=https://awsome-distributed-ai.s3.amazonaws.com/templates/aws-pcs/cluster-user-iam.yaml&stackName=pcs-cluster-users) | |
There was a problem hiding this comment.
"Cannot create, modify, or delete anything … Safe to hand out widely" overstates what the user policy enforces (should fix)
The stock cluster-user-iam.yaml grants ssm:StartSession on the SSM-SessionManagerRunShell document, and a plain Session Manager shell lands as ssm-user, which has passwordless sudo by default (https://docs.aws.amazon.com/systems-manager/latest/userguide/session-manager-getting-started-ssm-user-permissions.html). Root on the login node reaches the instance role's ssm:GetParameter on /pcs/<id>/ldap/* — i.e. the LDAP admin password — plus shared /home and Slurm accounting. The policy itself is untouched by this PR (worth a follow-up: drop RunShell from the user policy, or configure Session Manager Run As), but this rewrite is where the claim gets its strongest wording, so I'd soften the row to what the policy actually enforces: no AWS-API mutations and no compute-node sessions — the login-node shell it grants is root-equivalent on the shared directory until RunShell is removed.
| ``` | ||
| (`pcs-ready-dlami-with-enroot-pyxis.yaml`). The cluster-user template | ||
| requires `ClusterStackName` and scopes SSM session access to that one | ||
| cluster's login node — deploy one stack of it per cluster. |
There was a problem hiding this comment.
The one-cluster scoping claim lost its "Name tag is operator-mutable" caveat (should fix)
The deleted IAM.md section carried an explicit warning that the ssm:resourceTag/Name = <ClusterStackName>-login condition keys on the operator-mutable Name tag (re-tag ⇒ the scoping silently changes). This rewrite keeps the "scopes SSM session access to that one cluster's login node" claim but drops the caveat, leaving the claim unqualified. One sentence restores it: "Scoping keys on the EC2 Name tag, which is operator-mutable — re-tagging a login node changes who can reach it (fails closed for this cluster, but a matching tag elsewhere in the account would satisfy the condition).
| # existing account is still a mistake). ldapsearch here is unauthenticated | ||
| # and read-only; a lookup failure (LDAP down) still lets us continue and | ||
| # surface the real error at ldapadd time. | ||
| if command -v ldapsearch >/dev/null 2>&1; then |
There was a problem hiding this comment.
The new duplicate guard checks LDAP only — a local/NSS name like ubuntu still passes, with a destructive follow-on (should fix)
The guard is a real improvement (and B5a locks it in — nice). One gap: it searches only ldap://localhost, so ldap-add-user.sh ubuntu 10001 3000 "<key>" passes both checks (the username regex admits ubuntu; UID 1000's owner isn't in LDAP), creates a shadowing LDAP ubuntu, appends the key to the real /home/ubuntu/.ssh/authorized_keys, and then chown -R 10001:3000 /home/ubuntu — breaking the DLAMI default account's home. Same for UIDs of local users. Since the shared /home is exactly where the damage lands, I'd check the full NSS view and the documented range before touching LDAP or the filesystem:
if getent passwd "${USERNAME}" >/dev/null; then echo "…exists (local or LDAP)…" >&2; exit 1; fi
if getent passwd "${USER_UID}" >/dev/null; then echo "…uid in use…" >&2; exit 1; fi
if [ "${USER_UID}" -lt 10001 ] || [ "${USER_UID}" -gt 59999 ]; then echo "…outside LDAP range 10001-59999…" >&2; exit 1; fi(getent goes through SSSD, so it also sees LDAP — the two ldapsearch probes could then become the fallback for the SSSD-cold-cache window, keeping your documented fail-open behavior.)
KeitaW
left a comment
KeitaW
left a comment
There was a problem hiding this comment.
Review Batch 5/5 — PCS-Ready DLAMI Doc, Pre-existing Notes & Positives
Two inline comments on the new DLAMI doc, then two pre-existing observations and the things that verified clean.
Pre-existing, out of this diff (informational — no action needed for this PR)
- Helper passes secrets in argv:
ldap-add-user.shinvokesldapadd -w "${LDAP_ADMIN_PASSWORD}"andldappasswd -w … -s "${INITIAL_PW}"— both visible topson the multi-user login node while running. §2.1's new "never lands on the command line" sentence is true for the operator's shell, but a follow-up switching the helper to-y <root-only-password-file>would make it true end-to-end. - Jupyter port collisions move the server silently: the sbatch script notes modulo-1000 port collisions but doesn't set
--ServerApp.port_retries=0; jupyter-server retries ~50 ports on conflict, so a collided server starts on$PORT+1while the banner and tunnel still say$PORT. Adding--ServerApp.port_retries=0makes the collision loud instead.
Things That Look Great
- The duplicate-UID/username guard closes a real foot-gun — LDAP happily accepts two entries with one
uidNumber, and on shared/home+/fsxthat's one POSIX principal. The guard's fail-open-when-LDAP-down rationale is documented in-line, and B5a pins the behavior with exit codes and expected stderr. - PCS-API login discovery is now consistent across JUPYTER.md Step 3, USER-MANAGEMENT §2.3, IAM.md, and the new B9 — the last stale
Name=*loginfilter indocs/is gone (one remains intests/jupyter-notebook-test.md, noted in Batch 3). - The DLAMI table is accurate where I could check it. I verified 3 of 13 AMI IDs live in us-east-2 (
ami-0631f0a4211880f98,ami-0b0605bcf9ffffa63, plus the current latest): names, build dates, and the full Description strings (PCS Agent / Slurm / EFS Utils versions) match the table exactly. The SNS topic ARN and SSM parameter path match the official PCS docs verbatim. - §2.1's IMDS-based recovery is a genuinely nice touch — no hand-copied cluster ID; and the prerequisites hold: every
add-cng*.yamllaunch template setsInstanceMetadataTags: enabled, and IMDS permits:in exposed tag keys. - The
-W/-Sswitch across all rawldap*operator commands is a real hygiene improvement over the old-w "$ADMIN_PW"/-s NEWPASSforms — nothing sensitive in operator argv or history any more. - The README §7 accounting callout is exactly the kind of fix that only comes from actually running the walkthrough — and its cross-reference anchor resolves correctly.
- The GPU verify cell is well-designed — env vars, torch's view, and an actual kernel launch, in one paste.
- The single-user-default reframing of USER-MANAGEMENT is the right call — most deployments never need LDAP, and the old doc buried that.
- The IAM.md trim loses no operational content — the Grafana port-forward flow lives in README §8.2, and
tests/iam-test.mdremains linked from the tests README.
Sources
- Slurm: sbatch shebang check — https://github.com/SchedMD/slurm/blob/master/src/sbatch/sbatch.c ("This does not look like a batch script"); sreport time semantics — https://slurm.schedmd.com/sreport.html
- AWS PCS: PCS-Ready DLAMI guide (SNS ARN, SSM path, describe-images lookup) — https://docs.aws.amazon.com/pcs/latest/userguide/working-with_ami_pcs-ready-dlami.html; IMDS instance tags — https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/work-with-tags-in-IMDS.html; ssm-user sudo — https://docs.aws.amazon.com/systems-manager/latest/userguide/session-manager-getting-started-ssm-user-permissions.html
- Jupyter: security model / no-auth guidance — https://jupyter-server.readthedocs.io/en/latest/operators/security.html
- Verified live, 2026-08-04:
aws ssm get-parameter /aws/service/pcs/ami/dlami-base-ubuntu2404/x86_64/latest/ami-id(us-east-2) →ami-083d41c7d9d7d10a3;aws ec2 describe-imageson 3 table AMI IDs → Description strings match;curl download.pytorch.org/whl/{cu124,cu126,cu128,cu129,cu130}/torch/→ all 200; JupyterLab 4.6.2 token-less + XSRF-enabled: GET /lab 200, cross-site POST 403, cookie+header POST 201 - Repo files:
assets/cluster.yaml+assets/pcs-ml-cluster-deploy-all.yaml(AccountingPolicyEnforcementAllowedValues:none,associations,limits,safe);assets/cluster-user-iam.yaml(RunShell grant; CFN/PCS/EC2 read grants backing the JUPYTER/USER-MANAGEMENT IAM notes);assets/scripts/ldap-add-user.sh(authorized_keys install, argv password forms);tests/jupyter-notebook-test.md(expected nvidia-smi behavior,torch 2.12.1+cu130record,Values=*loginremnant);add-cng*.yaml(InstanceMetadataTags: enabled)
| ## PCS-Ready DLAMI x86\_64 | ||
|
|
||
| Every published x86\_64 PCS-Ready DLAMI, newest first. AMI IDs are region-scoped | ||
| and shown for **us-east-2**; use the `describe-images` command below the table |
There was a problem hiding this comment.
"Use the describe-images command below the table" — there is no command below the table (should fix)
The file ends at the release-notes link; the promised cross-region lookup command was never added. The official PCS docs page already ships the right one (https://docs.aws.amazon.com/pcs/latest/userguide/working-with_ami_pcs-ready-dlami.html) — adding it below the table with a build-date filter closes the gap:
aws ec2 describe-images --region <region> --owners amazon \
--filters 'Name=name,Values=aws-pcs-ready-dlami-base-ubuntu2404-x86_64-<PCS build>*' \
'Name=state,Values=available' \
--query 'Images[0].[Name,ImageId]' --output text|
|
||
| | PCS build | AMI ID (us-east-2) | Base DLAMI | PCS Agent | Slurm | EFS Utils | Kernel | NVIDIA driver | Default CUDA | CUDA stack | DCGM | EFA | OFI-NCCL | containerd | | ||
| | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | | ||
| | 2026-07-27 | `ami-0631f0a4211880f98` | 20260724 | 1.5.0-1 | 24.11.7-2, 25.05.8-2, 25.11.6-2 | 2.4.2 | 6.17.0-1019 | 595.71.05 | 13.2 | 12.8, 12.9, 13.0, 13.2 | 4.6.0 | 1.47.0 | 1.18.0 | v2.2.6 | |
There was a problem hiding this comment.
The table is already one build behind at review time (nit)
Verified live (2026-08-04, us-east-2): the SSM parameter /aws/service/pcs/ami/dlami-base-ubuntu2404/x86_64/latest/ami-id now resolves to ami-083d41c7d9d7d10a3 — build 2026-07-29 (created 2026-08-04), same component versions as this row. Inherent to a static table, but worth (a) adding the 2026-07-29 row before merge and (b) framing the intro as a dated snapshot ("as of YYYY-MM-DD") rather than "Every published … DLAMI", so staleness reads as expected rather than as an error. The lookup command above is what lets readers self-serve past the snapshot.
…ould-fix + nit) USER-MANAGEMENT.md: - §3 note on Slurm binary path swap when SlurmVersion=25.05 - §4.1 sacctmgr uses $S alias to absolute path (secure_path bypass) - §4.2 hold demo now creates myjob.sh/smalljob.sh, adds --partition, tightens the cap so 4-task job trips AssocGrpCPURunMinutesLimit - §4.2 use --ntasks-per-node=4 to match sinfo-reported CPUs on cpu1 - §4 accounting-enforcement wording: templates offer ,safe only - sreport end date off-by-one (2026-04-30 → 2026-05-01, ...-03-31 → -04-01) - README anchor #quick-start → #3-quick-start multi-user-test.md: - Part B: reorder — new B7 group add uses testuser2 still present, B9 deletes testuser2 at the end - Part B8 (was B9) restructures SSH-over-SSM into three explicit steps (laptop keygen → login-node authorized_keys append → laptop ssh) - Part F1 use sudo $S with absolute path, add carol UID 10004 - Part F2 batch script gets a shebang (printf '#!/bin/bash\n...\n') - Part F3 --ntasks-per-node=4 matches sinfo CPUs on cpu1 - Part F3 anchor awslabs#41-worked-example... → awslabs#41-register-users-to-accounts - Part F WithAssoc on F1 sacctmgr show; sreport end=now JUPYTER.md: - Step 1 pin PyTorch cu130 (matches DLAMI's CUDA 13.2 stack) - Tokenless mode drop --ServerApp.disable_check_xsrf=True; note Step 3 sub-step 2 is skipped and the banner heredoc's token line removed - Multi-user warning reworded to trusted-same-principal formulation - Verify GPU cell no longer flags all-visible nvidia-smi as a bug IAM.md: - Cluster-user row narrows claim: no AWS-API mutations, no compute-node sessions; login-node SSM shell is ssm-user with passwordless sudo - Restore Name-tag operator-mutable caveat on the scoping description PCS-READY-DLAMI.md: - Reframe intro as a dated snapshot (as of 2026-08-05); drop the "describe-images below" reference (command was never added) assets/scripts/ldap-add-user.sh: - Duplicate-detection extended: getent passwd for username and UID (catches local /etc/passwd shadowing, e.g. `ubuntu` UID 1000), plus hard-reject UID outside the 10001-59999 LDAP-user range Verified end-to-end via pcs-ml-cluster-deploy-all.yaml on a fresh Tokyo cluster (DirectoryService=OpenLDAP-LoginNode + ManagedAccounting= enabled + AccountingPolicyEnforcement=associations,limits,safe): §2.1 password recovery, §2.2 add first user (alice), §4.1 register users, §4.2 hold demo (limit trips), Part F1/F2/F3 all pass verbatim.
|
Thanks for the thorough review! All 21 comments addressed in Blocking (4) — all adopted:
Should-fix (11) — all adopted:
Nit (6) — all adopted (some inline with the above):
Two takeaways for me: run every section as-is on a fresh deploy-all cluster (not the modular chain), and every fenced block that appears in the doc has to run — even "trivial-looking" ones (F2's |
2 participants
Summary
Refresh of the customer-facing docs (
USER-MANAGEMENT.md,JUPYTER.md,IAM.md) after end-to-end verbatim verification on a live Tokyo cluster, plus a related script bug fix, a cluster.yaml default tweak, regression tests, and a new PCS-Ready DLAMI version-history doc.USER-MANAGEMENT.mdopens with the single-user default and treats multi-user as opt-in. Adds a §2 walkthrough that takes a first-time admin from recovering the admin password through adding a user, logging in, and running ansrunon a compute node, then reorganises the remaining operations and Slurm accounting for day-to-day use.JUPYTER.mdfixes the friction in Steps 1–3, adds a notebook cell that verifies GPU visibility, and documents a token-less mode for single-user clusters only.IAM.mdis trimmed to a role table plus a short Considerations section.PCS-READY-DLAMI.mdis new: version history of the AWS-published PCS-Ready DLAMI (Ubuntu 24.04).Test plan
Deployed a fresh cluster in
ap-northeast-1withDirectoryService=OpenLDAP-LoginNode,ManagedAccounting=enabled,AccountingPolicyEnforcement=associations,limits,safe, andMonitoringStack=Prometheus-LoginNode. Ran the fenced blocks in README §6/§7/§8.2, USER-MANAGEMENT §2/§3/§4, JUPYTER Step 1–3, and the load-bearing blocks inmulti-user-test.mdverbatim.lint-docs.shPASS.One gap surfaced during verification and fixed in this PR: on clusters with
AccountingPolicyEnforcementset, the README §7srunexample is rejected because the submitter must be registered in Slurm accounting first. Added a short callout in §7 that points to USER-MANAGEMENT §4 for the details.