fix(aws-pcs): wait for dpkg lock before monitoring install

[DaisukeMiyamoto]

What

The upstream aws-parallelcluster-monitoring/post-install.sh starts with a
raw apt-get install ca-certificates curl gnupg jq bc — no dpkg lock
wait. If unattended-upgrades, apt-daily, or the tail-end of this
node's own first-boot post-install is still holding the lock at that
moment, the installer aborts with:

Reading package lists...
++ apt-get install -y ca-certificates curl gnupg jq bc
E: Could not get lock /var/lib/dpkg/lock-frontend. It is held by process 7762 (apt)
E: Unable to acquire the dpkg frontend lock (/var/lib/dpkg/lock-frontend), is another process using it?

The current UserData wraps the invocation in a 3×15s retry, but that
drains in 45 s — a longer-running lock holder eats the whole budget on
first boot. Symptom on the login node: docker ps returns empty
(headers only), Grafana is unreachable, and the monitoring stack is
just… not there. Compute nodes behave the same when they land the
monitoring role.

Fix

Wrap the upstream call with a POSIX-clean wait_for_apt_lock helper
that polls the four apt/dpkg lock files for up to 5 minutes and
returns the moment they clear. Call it before the initial invocation
AND before each retry, so a longer-running lock holder doesn't burn
the retry budget with fixed sleeps. The idempotent 3× retry stays as a
safety net for genuinely transient issues (network hiccup, shared
/home still settling, etc.).

install-enroot-pyxis.sh already ships the same helper on the
post-install path (fixed in 01ecc90 upstream / this repo main). This
is the counterpart on the monitoring path.

While here: add-cng-p5.yaml still had the pre-retry monitoring block
(… | tee /var/log/monitoring-install.log, no rc capture, no retry).
Normalize it to the same shape as the other three so the four
hand-maintained UserData copies are byte-identical.

Also add a tests/lint-docs.sh check that asserts the monitoring
block is byte-identical across add-cng / add-cng-p5 /
add-cng-p6-b200 / add-cng-p6-b300 — same idea as the existing
needrestart-guard lock-step check.

An upstream fix to add wait_for_apt_lock to
aws-parallelcluster-monitoring/post-install.sh itself is planned as a
separate PR so other consumers benefit too; this PR is the defensive
wrapper at the deploy layer so PCS clusters don't have to wait for it.

Testing

• bash tests/lint-docs.sh passes on the fixed tree.
• Negative test: a one-word edit in add-cng-p5.yaml's monitoring
  block makes the lint FAIL with a diff on the exact line.
• aws cloudformation validate-template passes on all four CNG
  templates.
• wait_for_apt_lock runs POSIX-clean under dash (cloud-init
  runcmd shell): returns 0 immediately when no lock is held.
• Regression check on live clusters (Tokyo, DirectoryService=OpenLDAP-LoginNode,
  static CPU compute): both the pre-fix and post-fix templates produced a
  healthy monitoring stack (6 containers Up, exit 0, retry attempt 1).
  The wait path never fired because no lock contention happened on my
  side — matching that customer's reported symptom is reproduction-shy
  and depends on P5 + concurrent apt-mirror pressure that this local
  test does not exercise. The fix's negative case (lock held → wait →
  proceed) is defended by the POSIX-clean check above; the positive
  case (unchanged behaviour when no lock) is what the live clusters
  demonstrated.

[KeitaW]

Shell robustness & consistency — three notes on the monitoring wait/lock path. Nothing here blocks; every template ends up strictly better than main (three had retry-without-wait, p5 had single-shot-without-wait — all four now wait+retry). Details inline.

[KeitaW]

This inline fuser poll is now the third implementation of wait_for_apt_lock in architectures/aws-pcs/, each doing the same job differently: assets/scripts/install-enroot-pyxis.sh:81 (while fuser …; sleep 1, and its apt_get wrapper uses apt-get -o DPkg::Lock::Timeout=300), assets/scripts/setup-directory.sh:59 (sleep 10, three lock files), and this one (for i in $(seq 1 60); sleep 5, returns 0/1). The PR description says this "ships the same helper" as install-enroot-pyxis.sh — it's actually a fourth behavioural variant, so that line is worth correcting.

Two practical consequences of the fuser-poll approach:

1. Check-then-act race. wait_for_apt_lock returns the moment the frontend lock is momentarily free, but the child apt-get install inside post-install.sh runs a step later — unattended-upgrades can re-take the lock in that gap. Your 3× retry loop does cover this in practice (attempt 2 re-waits), which is why this is a suggestion, not a blocker.
2. Worst-case first-boot time. The child apt-get fails fast on a held lock, so each failed attempt re-runs the full 5-minute poll — a genuinely stuck lock costs ~3 × 5 min ≈ 15 min of first-boot delay, 3× the single 300 s cap the sibling helpers use.

The failure you're defending against is the dpkg frontend lock (per the error in your PR body). apt-get's native DPkg::Lock::Timeout makes apt block on exactly that lock itself — no check-then-act gap — and install-enroot-pyxis.sh:96 already uses it in this tree. Since the monitoring installer is an upstream script you invoke rather than edit, you can set it globally right before the call so its own apt-get inherits it:

echo 'DPkg::Lock::Timeout "300";' > /etc/apt/apt.conf.d/99lock-timeout

That makes the child apt-get wait once, race-free, typically succeed on attempt 1 — collapsing the poll and most of the retry budget, and avoiding a third helper variant. Keep the 3× retry as the net for genuinely transient (non-lock) failures. I'd apply this identically to all four copies so the new lint stays green.

[KeitaW]

When the poll exhausts its budget the function return 1s, but the caller ignores the return and proceeds to install anyway (the right best-effort behaviour). The gap is only observability: on the exact case this PR targets — a lock held long enough to matter — monitoring-install.log shows no sign the wait ever timed out. Both sibling helpers log here (install-enroot-pyxis.sh:86 "still held after ${max}s; proceeding"; setup-directory.sh:65 the same). A matching echo before return 1 would make a stuck-lock boot self-explanatory in the log. Minor.

Note: this block is byte-identical across all four files and the new lint enforces that, so this edit — like any here — has to land in all four copies at once.

[KeitaW]

monitoring_extract normalizes leading whitespace with sed -E 's/^[[:space:]]+//' (needed — the four templates legitimately nest the block at different depths). The side effect is that the check can't see relative indentation drift inside the block — e.g. an indented heredoc terminator, or <<'EOF' switched to the tab-stripping <<-'EOF' in one copy — which would change deployed behaviour while still comparing as identical. The neighbouring guard_extract documents exactly this in a five-line comment (tests/lint-docs.sh:71-75); monitoring_extract copied the technique but not the caveat, and both the block comment and the FAIL message say "byte-identical." I'd mirror the guard_extract comment here so "byte-identical" isn't read literally. Trivial.

[KeitaW]

Maintainability (follow-up, non-blocking)

The recurring shape in the aws-pcs assets is a UserData block hand-copied across add-cng / -p5 / -p6-b200 / -p6-b300, now policed by a second byte-identical lint (after the needrestart guard). The lint is the right stopgap and I'm glad it's here. But note the asymmetry with the sibling path: the enroot install is a single fetched script (assets/scripts/install-enroot-pyxis.sh, pulled at boot), whereas the monitoring wrapper is inlined four times. The drift-proof version of this change is to give the monitoring wrapper the same treatment — extract it to one assets/scripts/install-monitoring.sh fetched at boot — so there's one copy instead of four-plus-a-lint. I'd treat that as a follow-up, not a blocker; you've clearly reasoned about the deploy-layer wrapper and flagged the upstream post-install.sh fix as its own PR, which is a sound split.

---

Things that look great

• The add-cng-p5.yaml normalization is real, not just claimed — I extracted all four monitoring blocks on the branch and they hash identically (sha256 first-12 450177747a72, 49 lines each). The one file still on the old pre-retry shape is now in lock-step.
• The p5 change quietly fixes a latent exit-code-masking bug, not just "normalizes shape." The line it replaces was bash /tmp/post-install.sh … 2>&1 | tee /var/log/monitoring-install.log followed by an unconditional echo "Monitoring installation complete" — so p5 ran the installer once, and even a captured $? would have resolved to tee's exit status, not the installer's, logging success regardless. The new >> …log 2>&1; rc=$? … (exit $rc) shape captures the real exit code. Worth calling out in the PR description as a fix, not just a cleanup.
• The new lint check is the right pattern — I confirmed it PASSes and actually compares the four blocks, extending the existing needrestart lock-step check rather than inventing a new mechanism.
• The added helper is POSIX/dash-clean — no local, no arrays, no [[ ]] — genuinely more portable than the two .sh siblings (both use bash local). Correct call for cloud-init UserData. I ran it under dash both ways: no-lock returns immediately; lock-held waits one poll cycle then proceeds when the lock clears.
• The testing section is honest about what it did and didn't exercise — explicitly stating the lock-contention repro is shy and depends on P5 + concurrent apt-mirror pressure, rather than overclaiming.

---

Sources

• Repo precedent — native apt lock-timeout already in this tree: architectures/aws-pcs/assets/scripts/install-enroot-pyxis.sh:81-96 (apt-get -o DPkg::Lock::Timeout=300).
• Repo precedent — third wait_for_apt_lock variant: architectures/aws-pcs/assets/scripts/setup-directory.sh:59.
• Verified live (2026-07-12): all four monitoring blocks byte-identical (450177747a72); bash tests/lint-docs.sh PASS; wait_for_apt_lock correct under dash (no-lock fast-return + lock-held wait/proceed); aws cloudformation validate-template VALID on all four CNG templates.

[KeitaW]

Re-review (round 2) — resolution scoreboard

Clean adoption of the round-1 headline: the hand-rolled fuser-poll wait_for_apt_lock is gone, replaced by a native echo 'DPkg::Lock::Timeout "300";' > /etc/apt/apt.conf.d/99lock-timeout drop-in, with the 3× retry re-scoped to non-lock transients. Verified live on the branch: all four monitoring blocks byte-identical (sha256 first-12 635e8a615cdf, 40 lines), lint-docs.sh PASSes, apt-config parses the drop-in on apt 2.4, and install-enroot-pyxis.sh:96 uses the same 300. Nothing blocks.

#	Round-1 finding	Status
1	third divergent wait_for_apt_lock fuser-poll; prefer native DPkg::Lock::Timeout	✅ Resolved — drop-in as suggested; verified live. The real win: it's inherited by the apt-get calls inside the unedited post-install.sh (where "Could not get lock" fired), which the old outer-only poll never covered. TOCTOU gone; 3rd helper copy gone.
2	log a WARN when the wait times out	➖ Moot — no fuser-poll, so no wait-timeout branch.
3	lint monitoring_extract "byte-identical" is really "modulo indentation"	❌ Still open — lint-docs.sh unchanged; carry-over 🟢 (round-1 inline stands).
B	monitoring install inline-duplicated 4× (fetched-script follow-up)	➖ Unchanged / informational — four copies (now a smaller drop-in), lint enforces identity.

Two 🟢 comments this round — one inline (below), one on the PR description.

[KeitaW]

The drop-in is a persistent, node-wide apt setting — worth saying so, and it's no longer a "helper"

The mechanism is right, and I like that it names the install-enroot-pyxis.sh precedent. Two small accuracy points on the narration:

• Unlike enroot's per-call -o DPkg::Lock::Timeout=300 (transient, scoped to that one apt-get), this writes a persistent /etc/apt/apt.conf.d/99lock-timeout that applies node-wide to every future apt-get on the instance. Benign-to-beneficial here, but broader and lasting — and it's a drop-in, not a "sibling helper." (Precision aside: 300 matches the runtime/enroot path, but isn't literally the repo-wide value — the DLAMI bake path at pcs-ready-dlami-with-enroot-pyxis.yaml:267 uses =120.)
• Worth stating if you want it: on a genuinely stuck lock the worst case is unchanged from the old poll — apt-get waits up to 300 s, fails, sleep 15, ×3 ≈ 15.5 min (and boot never gates on it — no exit $rc, so a failing monitoring install just logs complete (exit N) and the node still comes up). The drop-in's value isn't a shorter worst case; it's correctness — it protects the installer's internal apt-get, atomically, no check-then-act gap. The retry loop is genuinely complementary, not redundant: it also catches a 300 s wait that wasn't long enough (a first-boot unattended-upgrades pulling large CUDA/driver packages can hold the lock past 5 min).

[KeitaW]

The PR description still narrates the removed wait_for_apt_lock helper

The code now ships the drop-in, but the description still describes the old approach — "Wrap the upstream call with a POSIX-clean wait_for_apt_lock helper" and "wait_for_apt_lock runs POSIX-clean under dash". A reader comparing the description to the diff will hunt for a wait_for_apt_lock function that isn't there. Worth refreshing the What / Fix / Testing sections to describe the DPkg::Lock::Timeout drop-in you actually shipped — it's a cleaner story anyway (native apt mechanism, no custom shell).

---

Things that look great (round 2)

• You took the round-1 recommendation and implemented it exactly right — the drop-in at the same 300 as the enroot path, with a comment explaining why it's a global apt.conf.d file (the installer runs unedited, so a per-call -o isn't reachable). That reasoning is the crux of why this beats the poll.
• The retry loop was correctly re-scoped, not just left in place — the comment now frames the 3× retry as covering non-lock transients, since the lock case is handled atomically by the drop-in. Right mental model (and it still catches a lock held beyond 300 s as a bonus).
• add-cng-p5.yaml stays in lock-step — it took the larger diff (still on the old single-shot shape) and lands byte-identical with the other three (635e8a615cdf); the enforcing lint passes.
• No new CFN foot-guns — the block uses only brace-free $attempt / $rc / $?, so !Sub passes them through untouched; the only ${…} are the intended MonitoringVersion / MonitoringRepo. (Had the retry vars been ${attempt}, CFN would reject the template — correctly avoided.)

---

Sources

• Verified live (2026-07-13, branch head 0329142): four monitoring blocks byte-identical (sha256 first-12 635e8a615cdf, 40 lines); bash tests/lint-docs.sh → PASS; apt-config dump parses DPkg::Lock::Timeout "300"; on apt 2.4.14; install-enroot-pyxis.sh:96 uses -o DPkg::Lock::Timeout=300 (same value).
• DPkg::Lock::Timeout is honored by every apt-get, including the unedited installer's child calls — apt.conf(5); requires apt ≥ 2.0 (Ubuntu 20.04+); PCS AMIs are 22.04/24.04 (apt 2.4).