From 4e916a4b00e7bd4aa37311b03a488ae2ec1a803f Mon Sep 17 00:00:00 2001 From: Ryan Curran Date: Wed, 19 Aug 2026 08:32:26 -0400 Subject: [PATCH] group-parity, and split the two resources add-to-group was straddling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both of these came out of reviewing the m4 refresh path before the next wave, and both are about the same thing: a wave that looks fine right up until a host is already in production. group-parity answers a question nothing in the toolchain could: do the hosts in a group get the profiles a working production host gets? The m4-214 incident turned on this — a host missing Skip Setup Assistant and the FDA SSH Keygen Wrapper hung at first boot and presented as "Safari automation is broken", costing most of a day. The postmortem's advice was to diff `profiles show` by hand against a known-good host, which needs SSH to a box that by definition may not be reachable. This asks SimpleMDM instead, before a wave starts. Two findings shaped the implementation, both verified against the live account: - Assignment-group records carry no `profiles` relationship at all; the link lives on the profile side as `relationships.groups`. So the comparison is per-device, via GET /devices/{id}/profiles. - It has to be per-device for correctness, not just convenience. Diffing the assignment groups reports Skip Setup Assistant and the FDA wrapper as missing from the bootstrap group — true, and irrelevant: its devices get both from the additive DEP Enrollment group. Crying wolf on exactly the pair from the postmortem would train operators to ignore the check. A test pins this against a well-meaning simplification. The baseline is the intersection of several sampled reference devices rather than one sampled host, so an atypical prod box can't drag a profile in and the result doesn't depend on which device the API listed first. A profile diff alone under-reports, though, so there is a second peer-wise pass: a group that two thirds of the devices are in but some are not means those were moved rather than added. That found three hosts missing Relops Public SSH Key, Sudoers, Enable SSH and DEP Enrollment — no admin key, no passwordless sudo, no sshd after their next wipe. Two were also in the prod group, so they still received the profiles and the profile diff said nothing about them. Devices are labelled with serial and name because a bare device id is not identifying: a matching enrollment date was enough to mistake one for m4-214. The second change: `add-to-group --quarantine-on-register` coupled an action bound by the SimpleMDM API (three calls per host, no SSH to pace it) to a ~30-minute wait bound by Taskcluster, with one --concurrency serving both. Raise it for wall-clock and you hammer SimpleMDM; lower it for SimpleMDM and 33 hosts serialise into five and a half hours. At -j12 this looked like five hosts failing when twelve had already been added — the POST succeeded and the follow-up push_apps 429'd — so killing the batch orphaned twelve live, autonomous bootstraps with nothing watching them. They would have reached production unvalidated. The runbook has carried a two-command workaround ever since; this is that workaround, built in. The add phase is now clamped to simplemdm_max_concurrent whatever -j says, and warns rather than silently obeying. The watch phase then runs every host at once, because a watcher is an idle poll loop. A host whose add FAILED is still watched, since that is precisely the case where it is in the group anyway. The bootstrap-spanning budget is passed as --max-wait-seconds, so operators no longer need to know to export a 5400s override — a budget that expires early puts the host live unheld, which is the failure the flag exists to prevent. Both phases log under one batch directory, and Ctrl-C writes added.txt naming every host bootstrapping unwatched, with the command to re-attach. Co-Authored-By: Claude Opus 5 (1M context) --- docs/RUNBOOK.md | 67 ++++- orchestrator/README.md | 20 +- orchestrator/orchestrator/batch.py | 214 +++++++++++++- orchestrator/orchestrator/cli.py | 57 +++- .../orchestrator/clients/simplemdm.py | 50 ++++ orchestrator/orchestrator/config.py | 18 ++ orchestrator/orchestrator/workflow.py | 224 +++++++++++++++ orchestrator/tests/test_batch_phases.py | 184 ++++++++++++ orchestrator/tests/test_group_parity.py | 270 ++++++++++++++++++ 9 files changed, 1077 insertions(+), 27 deletions(-) create mode 100644 orchestrator/tests/test_batch_phases.py create mode 100644 orchestrator/tests/test_group_parity.py diff --git a/docs/RUNBOOK.md b/docs/RUNBOOK.md index 5a5cd98..a64f122 100644 --- a/docs/RUNBOOK.md +++ b/docs/RUNBOOK.md @@ -16,12 +16,13 @@ because that's the order you meet them in. cd ~/git/relops-bootstrap/orchestrator HOSTS=~/Desktop/wave.txt # one short hostname per line, '#' comments ok +uv run reprovision group-parity # ← do the hosts get prod's profiles? uv run reprovision batch $HOSTS --action mint -j3 # SecureToken (idempotent, often a no-op) uv run reprovision batch $HOSTS --action os-update -j3 # in-place upgrade to target OS # ⏳ wait ~35 min: ~14GB download, then startosinstall, then a reboot uv run reprovision batch $HOSTS --action preflight --allow-sip-enabled -j3 -uv run reprovision batch $HOSTS --action add-to-group --quarantine-on-register -j2 -# ⏳ blocks ~30 min per host: this is the bootstrap, and the watch that holds each host +uv run reprovision batch $HOSTS --action add-to-group --quarantine-on-register +# ⏳ two phases, automatically: a paced SimpleMDM add, then every watcher at once (~30 min) uv run reprovision batch $HOSTS --action validate -j3 # ← the gate. Do not skip. uv run reprovision unquarantine macmini-m4-XXX # only what validate passed ``` @@ -55,24 +56,48 @@ bound by a different resource. | `mint` | 3–4 | SSH | fast; often a no-op when the token already exists | | `os-update` | **irrelevant** ⚠️ | the mirror | **launch-and-return** — `-j` paces only the *launches*. The **hosts-file length is the real concurrency.** 33 hosts = 33 simultaneous 14GB pulls | | `preflight` | 3–4 | SSH | read-only | -| `add-to-group` | **2** 🚨 | **SimpleMDM API** | 3 API calls per host. At `-j12` the 429 retry budget blew and hosts failed | +| `add-to-group` | **2** 🚨 | **SimpleMDM API** | 3 API calls per host. At `-j12` the 429 retry budget blew and hosts failed. Clamped to `REPROVISION_SIMPLEMDM_MAX_CONCURRENT` (2) regardless of `-j` | +| `quarantine-on-register` | all of them | local process count | an idle poll loop; `-j` does not apply | | `validate` | 3 | SSH | read-only | | `provision` | 3 | MDC1 imaging throughput | matches the runner's `RUNNER_MAX_CONCURRENT` | -### 🪤 The `-j` trap that bit us hardest +### 🪤 The `-j` trap that bit us hardest — now fixed -`add-to-group --quarantine-on-register` couples a **SimpleMDM-bound action** to a -**30-minute Taskcluster-bound wait**, so one `-j` has to serve both. Raise it for wall-clock -and you hammer SimpleMDM; lower it for SimpleMDM and 33 hosts serialize into ~5½ hours. +`add-to-group --quarantine-on-register` used to couple a **SimpleMDM-bound action** to a +**30-minute Taskcluster-bound wait**, so one `-j` had to serve both. Raise it for wall-clock and +you hammered SimpleMDM; lower it for SimpleMDM and 33 hosts serialized into ~5½ hours. + +**It now splits itself into two phases**, so the command below is all you run: + +```bash +uv run reprovision batch $HOSTS --action add-to-group --quarantine-on-register +``` + +Phase 1 adds hosts at the SimpleMDM cap (2), clamping `-j` with a warning if you asked for more. +Phase 2 then watches every host at once, with the bootstrap-spanning budget passed explicitly — +no `REPROVISION_QUARANTINE_ON_REGISTER_MAX_WAIT_SECONDS` export needed. Both phases log under one +batch directory, and a `Ctrl-C` writes `added.txt` naming every host that is bootstrapping +unwatched, plus the command to re-attach: + +```bash +uv run reprovision batch ~/.local/state/reprovision/batch-/added.txt \ + --action quarantine-on-register +``` + +A host whose add *failed* is still watched — the add can succeed while the follow-up `push_apps` +429s, which is exactly what happened on 2026-08-14. + +
+The old manual workaround, for reference At `-j12` on 2026-08-14 it looked like 5 hosts failed. What actually happened: **12 hosts had already been added to the group** (the add succeeded, the follow-up `push_apps` 429'd), so killing the batch orphaned 12 live bootstraps with no watcher. They'd have gone into production unvalidated. -**Workaround until this is fixed** — split it along the resource boundary: +Split it along the resource boundary by hand: ```bash # 1. the SimpleMDM half, gently @@ -90,6 +115,8 @@ grep '^macmini' $HOSTS | xargs -P 33 -I{} \ > (~30 min). Too short and the watch expires before there's anything to quarantine — and the > host goes live unheld, i.e. exactly the failure the flag exists to prevent. +
+ --- ## 🔬 Never trust `ok`. Verify the thing itself. @@ -259,6 +286,30 @@ looping over many hosts by hand. - ⚠️ **Membership does not prove the PKG was pushed.** The already-a-member path skips `push_apps`, so a host added by hand in the UI can sit in the group with nothing installed. `add-to-group` warns when the payload is missing. +- 🧬 **Check profile parity before a wave**, not after the first host hangs: + + ```bash + uv run reprovision group-parity # the whole bootstrap group + uv run reprovision group-parity --host macmini-m4-241 # one host + ``` + + Read-only, API-only, no SSH unless you pass `--host`. It builds a baseline from the profiles + *every* sampled production device shares, then names anything a target device lacks — with the + m4-214 story attached for the profiles that caused it. + + It also runs a second, **peer-wise membership pass**: a group that two thirds of these devices + are in but some are not means those devices were *moved* rather than added. This catches strictly + more than the profile diff, because the groups a mis-clicked host loses are mostly app-bearing — + on 2026-08-19 three hosts were missing `Relops Public SSH Key`, `Sudoers`, `Enable SSH` and + `DEP Enrollment`, i.e. no admin key, no passwordless sudo and no sshd after their next wipe. Two + of them were also in the prod group, so they still received the profiles and a profile-only diff + said nothing. + + It compares **effective per-device** profile sets, deliberately. Diffing the assignment groups + themselves reports Skip Setup Assistant and the FDA SSH Keygen Wrapper as missing from the + bootstrap group — true, and irrelevant: its devices get both from the additive DEP Enrollment + group. Don't "simplify" it back to a group-level diff; it would cry wolf on the exact pair from + the postmortem. --- diff --git a/orchestrator/README.md b/orchestrator/README.md index 67269c9..4e272b8 100644 --- a/orchestrator/README.md +++ b/orchestrator/README.md @@ -202,10 +202,11 @@ second trip to the rack, and **SIP stays enabled** — no Recovery visit require ```bash HOSTS=~/Desktop/wave.txt +reprovision group-parity # ⓪ profile parity, before anything reprovision batch $HOSTS --action mint -j3 # ① SecureToken (idempotent) reprovision batch $HOSTS --action os-update -j3 # ② in-place upgrade, ~35 min reprovision batch $HOSTS --action preflight --allow-sip-enabled -j3 # ③ read-only gate -reprovision batch $HOSTS --action add-to-group --quarantine-on-register -j2 # ④ the GO +reprovision batch $HOSTS --action add-to-group --quarantine-on-register # ④ the GO (2 phases) reprovision batch $HOSTS --action validate -j3 # ⑤ fitness — do not skip reprovision unquarantine macmini-m4-XXX # ⑥ release what passed ``` @@ -293,12 +294,14 @@ hosts offline simultaneously on the 2026-05-12 batch. > - **`os-update` ignores `-j` entirely.** It's launch-and-return (~10s/host), so `-j` paces only > the launches while the ~14GB download runs detached. **The hosts-file length is the real > concurrency.** 33 hosts means 33 simultaneous pulls no matter what you pass. -> - **`add-to-group` wants `-j2`.** It makes 3 SimpleMDM calls per host, and the API's rate -> limiter is unforgiving. At `-j12` the 429 retry budget was exhausted in ~64s. +> - **`add-to-group` is capped at 2 for you.** It makes 3 SimpleMDM calls per host and the API's +> rate limiter is unforgiving — at `-j12` the 429 retry budget was exhausted in ~64s. A larger +> `-j` is clamped, with a warning. +> - **`add-to-group --quarantine-on-register` runs as two phases**, because a SimpleMDM-bound add +> and a 30-minute Taskcluster-bound watch cannot share one `-j`. The add is paced; the watchers +> all start at once. > -> The full table, plus why `add-to-group --quarantine-on-register` couples two different -> bottlenecks to one `-j` and how to split them: -> [Runbook → Concurrency](../docs/RUNBOOK.md#concurrency). +> The full table: [Runbook → Concurrency](../docs/RUNBOOK.md#concurrency). > 🔬 **`ok` means the command succeeded, not that the work happened.** For `os-update`, `ok` > means *the upgrade launched* — verify with `pgrep -x curl` plus growth of @@ -358,6 +361,7 @@ direct `REPROVISION_*` value always wins over its `_REF`. | `reprovision run ` | Full pipeline, **including an EACS wipe**. `--unquarantine` returns it to service at the end. | | `reprovision provision ` | Fresh DEP host → prod. **No wipe in this path.** `--no-wait` stops after the BST escrow. | | `reprovision preflight ` | Read-only readiness check (OS version, SIP, SecureToken, BST). Needs no SimpleMDM/TC credential. | +| 🧬 `reprovision group-parity` | Read-only: do a group's hosts get the profiles a working production host gets? Builds a baseline from the profiles every sampled prod device shares and names what a target lacks. Compares **effective per-device** sets, not assignment groups — the bootstrap group carries neither m4-214 profile yet its devices hold both, via the additive DEP Enrollment group. Also flags **membership outliers** — a device missing a group two thirds of its peers are in was moved, not added, and the groups it lost are mostly app-bearing (admin key, sudo, sshd), which a profile diff cannot see. `--host` checks one box; needs only the SimpleMDM API key otherwise. | | 🚀 `reprovision add-to-group ` | **ADD** the host to the SimpleMDM bootstrap group — the action that triggers the whole bootstrap. Additive only, never a move. Idempotent. Refuses production groups. `--quarantine-on-register` starts the registration watch here, where it belongs. | | ✅ `reprovision validate ` | Read-only **fitness** check on a bootstrapped host — display mode (60Hz), last puppet run, worker. **Run this before every unquarantine.** Exit 2 = not bootstrapped yet, exit 1 = bootstrapped but UNFIT. | | `reprovision quarantine-on-register ` | Watch for a fresh worker to register, then quarantine it on sight. | @@ -387,7 +391,9 @@ skipped separately from failed, which is what makes "38 ok, 15 not ready, 2 brok | `-j` / `REPROVISION_BATCH_MAX_CONCURRENT` | `3` | | `REPROVISION_PREFLIGHT_SSHD_WAIT_SECONDS` | `60` | | `REPROVISION_QUARANTINE_ON_REGISTER_POLL_SECONDS` | `5` (this interval *is* the exposure window) | -| `REPROVISION_QUARANTINE_ON_REGISTER_MAX_WAIT_SECONDS` | `900` — ⚠️ sized for a watch started *after* bootstrap. Driving the watch by hand from group-add needs **5400**, or it expires before there is anything to quarantine | +| `REPROVISION_QUARANTINE_ON_REGISTER_MAX_WAIT_SECONDS` | `900` — sized for a watch started *after* bootstrap. Every path that starts the watch earlier now passes a bootstrap-spanning budget explicitly (`--max-wait-seconds`), so you should not need to export this | +| `REPROVISION_SIMPLEMDM_MAX_CONCURRENT` | `2` — fan-out cap for SimpleMDM-bound batch work, independent of `-j` | +| `REPROVISION_REFERENCE_GROUP_ID` | `2017918` — the production group `group-parity` measures against (read-only) | | `REPROVISION_VALIDATE_EXPECTED_REFRESH_HZ` | `60.0` — matches what mozharness itself enforces, so `validate` agrees with CI rather than inventing a second standard | | `REPROVISION_BOOTSTRAP_GROUP_ID` | `2417981` (`gecko-t-osx-1500-m4-bootstrap`). Production groups are separately blocked in `clients.simplemdm.PROTECTED_GROUP_IDS`, which this **cannot** override | | `REPROVISION_BOOTSTRAP_PKG_MAX_WAIT_SECONDS` | `300` — how long to wait for the PKG before calling it a group problem | diff --git a/orchestrator/orchestrator/batch.py b/orchestrator/orchestrator/batch.py index 083067b..fb1336d 100644 --- a/orchestrator/orchestrator/batch.py +++ b/orchestrator/orchestrator/batch.py @@ -45,7 +45,15 @@ EXIT_FAILED = 1 EXIT_NOT_READY = 2 -ACTIONS = ("preflight", "mint", "os-update", "add-to-group", "validate", "provision") +ACTIONS = ( + "preflight", + "mint", + "os-update", + "add-to-group", + "quarantine-on-register", + "validate", + "provision", +) @dataclass @@ -112,6 +120,7 @@ def _child_cmd( allow_sip_enabled: bool, wait: bool, quarantine_on_register: bool = False, + watch_max_wait_seconds: int = 0, ) -> list[str]: exe = _reprovision_exe() if action == "preflight": @@ -131,6 +140,15 @@ def _child_cmd( # Read-only fitness check; no gate flags, and it inspects the RUNNING host rather than # gating on a target version, so --expected-os doesn't apply. return [exe, "validate", host] + elif action == "quarantine-on-register": + # Pure Taskcluster polling: no SSH, no SimpleMDM, no gate flags. The budget is passed + # explicitly because the default is sized for a watch that starts AFTER bootstrap; a watch + # started at group-add has to span the whole thing, and getting that wrong is not a + # harmless timeout — the host goes live UNHELD, the exact failure the watch prevents. + cmd = [exe, "quarantine-on-register", host] + if watch_max_wait_seconds: + cmd += ["--max-wait-seconds", str(watch_max_wait_seconds)] + return cmd elif action == "add-to-group": # No gate flags: nothing here inspects the OS version or SIP state. It does need SSH, # though — the host's serial is the only join key to its SimpleMDM device record, since a @@ -252,6 +270,141 @@ def _mmss(seconds: float) -> str: return f"{s // 60}:{s % 60:02d}" +def _write_roster(log_dir: Path, hosts: list[str]) -> Path: + """Record which hosts are (or may be) in the group, so the watch phase is re-runnable. + + A file rather than a printed list: the resume command is then a real command an operator can + paste at 6pm without re-deriving 49 hostnames from scrollback. + """ + roster = log_dir / "added.txt" + roster.write_text( + "# hosts added to the bootstrap group by this batch — they are bootstrapping NOW.\n" + "# Re-attach watchers: reprovision batch --action quarantine-on-register\n" + + "".join(f"{h}\n" for h in hosts) + ) + return roster + + +def _add_to_group_then_watch( + hosts: list[str], + *, + concurrency: int = 0, + expected_os: str = "", + allow_sip_enabled: bool = False, + per_host_timeout: int = 0, + dry_run: bool = False, +) -> int: + """Run the SimpleMDM add and the Taskcluster registration watch as two separate phases. + + `add-to-group --quarantine-on-register` couples an action bound by the SimpleMDM API (three + calls per host, no SSH to pace it) to a ~30-minute wait bound by Taskcluster. One + `--concurrency` cannot serve both: raise it for wall-clock and you hammer SimpleMDM, lower it + for SimpleMDM and 33 hosts serialise into five and a half hours. + + On 2026-08-14 at `-j12` this looked like 5 hosts failing. What actually happened: **12 hosts + had already been added to the group** — the POST succeeded and the follow-up `push_apps` + 429'd — so killing the batch orphaned 12 live, autonomous bootstraps with no watcher. They + would have registered and started claiming production work unvalidated. The runbook has + carried a two-command workaround for this ever since; this is that workaround, built in. + + Phase 1 is clamped to `simplemdm_max_concurrent`, independent of `--concurrency`. Phase 2 runs + every host at once: a watcher is an idle poll loop, so the binding resource is neither the API + nor the network but local process count. + """ + s = get_settings() + asked = concurrency or s.batch_max_concurrent + add_j = min(asked, s.simplemdm_max_concurrent) + watch_budget = s.bootstrap_max_wait_seconds + s.quarantine_on_register_max_wait_seconds + + ui.step("BATCH", f"add-to-group then watch × {len(hosts)} host(s), in two phases") + ui.info( + f"phase 1: add to the group, {add_j} at a time (SimpleMDM-bound) · " + f"phase 2: watch for registration, all {len(hosts)} at once (Taskcluster-bound)" + ) + if asked > add_j: + ui.warn( + f"-j {asked} ignored for the add phase: it is SimpleMDM-bound, clamped to {add_j}. " + "At -j12 the 429 retry budget blew and left 12 hosts added-but-unwatched " + "(2026-08-14). Raise REPROVISION_SIMPLEMDM_MAX_CONCURRENT if you really mean it." + ) + + # One directory for both phases, resolved before either starts: an interrupt during phase 1 + # still has to be able to write the resume roster somewhere findable. + root = None if dry_run else _log_dir() + + added: dict[str, HostResult] = {} + try: + add_failed = run_batch( + hosts, + action="add-to-group", + concurrency=add_j, + expected_os=expected_os, + allow_sip_enabled=allow_sip_enabled, + quarantine_on_register=False, + per_host_timeout=per_host_timeout, + dry_run=dry_run, + results_out=added, + log_dir=None if root is None else root / "add", + watch_follows=True, + ) + except KeyboardInterrupt: + _report_orphans(hosts, added, root) + raise + + # Watch every host that MIGHT now be in the group — "ok" and "failed" alike. A failed add is + # precisely the 2026-08-14 case: the add landed and the push 429'd, so the host is in the group + # and bootstrapping while the batch called it a failure. Only "skipped" (exit 2 — e.g. no SSH, + # so its serial was never read) means it was definitely never added. Over-watching wastes an + # idle poll loop; under-watching puts an unvalidated host into production. + watch = list(hosts) if dry_run else [ + h for h in hosts if h in added and added[h].state != "skipped" + ] + if not watch: + ui.warn("no host was added to the group — nothing to watch") + return add_failed + + if root is not None: + ui.info(f"roster: {_write_roster(root, watch)}") + + try: + watch_failed = run_batch( + watch, + action="quarantine-on-register", + concurrency=len(watch), + per_host_timeout=per_host_timeout or watch_budget + 300, + dry_run=dry_run, + watch_max_wait_seconds=watch_budget, + log_dir=None if root is None else root / "watch", + ) + except KeyboardInterrupt: + _report_orphans(watch, added, root) + raise + + return add_failed + watch_failed + + +def _report_orphans( + hosts: list[str], added: dict[str, HostResult], log_dir: Path | None +) -> None: + """On interrupt, say which hosts are bootstrapping unwatched — and how to re-attach. + + Silence here is what turned a Ctrl-C into 12 unheld production workers. + """ + maybe = [h for h in hosts if h not in added or added[h].state != "skipped"] + if not maybe: + return + ui.err( + f"INTERRUPTED with {len(maybe)} host(s) possibly already in the bootstrap group. The " + "bootstrap is autonomous: they will finish, register, and claim production work with " + "nothing holding them." + ) + if log_dir is None: + ui.err("re-attach watchers now: reprovision batch --action quarantine-on-register") + return + roster = _write_roster(log_dir, maybe) + ui.err(f"re-attach watchers now: reprovision batch {roster} --action quarantine-on-register") + + def run_batch( hosts: list[str], *, @@ -263,6 +416,10 @@ def run_batch( quarantine_on_register: bool = False, per_host_timeout: int = 0, dry_run: bool = False, + watch_max_wait_seconds: int = 0, + results_out: dict[str, HostResult] | None = None, + log_dir: Path | None = None, + watch_follows: bool = False, ) -> int: """Drive `action` across `hosts`. Returns the number of hosts that FAILED (not skipped). @@ -272,9 +429,26 @@ def run_batch( if action not in ACTIONS: raise ReprovisionError(f"unknown batch action {action!r} — one of {', '.join(ACTIONS)}") + # One --concurrency cannot serve both halves of this action; split it. See the function. + if action == "add-to-group" and quarantine_on_register: + return _add_to_group_then_watch( + hosts, + concurrency=concurrency, + expected_os=expected_os, + allow_sip_enabled=allow_sip_enabled, + per_host_timeout=per_host_timeout, + dry_run=dry_run, + ) + s = get_settings() concurrency = concurrency or s.batch_max_concurrent expected_os = expected_os or s.provision_expected_os + if action == "quarantine-on-register" and not watch_max_wait_seconds: + # The child's own default (900s) assumes the watch starts once bootstrap has finished. + # Reached through a batch it never does — the operator is attaching watchers to hosts that + # are mid-bootstrap — so size it for the whole thing here instead of making the runbook + # tell people to export REPROVISION_QUARANTINE_ON_REGISTER_MAX_WAIT_SECONDS=5400. + watch_max_wait_seconds = s.bootstrap_max_wait_seconds + s.quarantine_on_register_max_wait_seconds def _cmd_for(host: str) -> list[str]: return _child_cmd( host, @@ -283,6 +457,7 @@ def _cmd_for(host: str) -> list[str]: allow_sip_enabled=allow_sip_enabled, wait=wait, quarantine_on_register=quarantine_on_register, + watch_max_wait_seconds=watch_max_wait_seconds, ) # A provision blocks on the bootstrap sentinel, so give the child the sentinel budget plus @@ -297,6 +472,10 @@ def _cmd_for(host: str) -> list[str]: # Launch-and-return: staging the script plus the started-cleanly check. The ~14GB # download runs detached and outlives this call by design. per_host_timeout = 600 + elif action == "quarantine-on-register": + # Must outlast the child's own watch budget, or the batch kills the watcher and the + # host it was holding goes live unheld. + per_host_timeout = watch_max_wait_seconds + 300 elif action == "validate": per_host_timeout = 300 elif action == "add-to-group": @@ -318,6 +497,8 @@ def _cmd_for(host: str) -> list[str]: # passed, when mint is handed neither flag and checks neither thing. if action == "validate": gate_note = "read-only fitness check on already-bootstrapped hosts" + elif action == "quarantine-on-register": + gate_note = "Taskcluster-bound watch only — no SimpleMDM call, no SSH, no OS/SIP gate" elif action in ("mint", "add-to-group"): gate_note = f"no OS/SIP gate — {action} doesn't inspect the running OS" elif action == "os-update": @@ -333,14 +514,23 @@ def _cmd_for(host: str) -> list[str]: ui.info("launches the upgrade and returns; hosts reboot on their own — sweep with --action preflight after") if action == "validate": ui.info("exit 2 = not bootstrapped yet (skipped) · exit 1 = bootstrapped but UNFIT to take work") - if action == "add-to-group": - ui.info("ADD only, never a move; already-member hosts are skipped — then wait for the pkg to land") + if action == "quarantine-on-register": ui.info( - "watching for registration and quarantining on sight (blocks for the whole bootstrap)" - if quarantine_on_register - else "NOT quarantining: the bootstrap is autonomous, so these hosts will go live and " - "claim production work unvalidated — pass --quarantine-on-register for fresh hardware" + f"watch budget {_mmss(watch_max_wait_seconds)} per host — sized to span a whole " + "bootstrap, not just the registration gap" ) + if action == "add-to-group": + ui.info("ADD only, never a move; already-member hosts are skipped — then wait for the pkg to land") + if quarantine_on_register: + ui.info("watching for registration and quarantining on sight (blocks for the whole bootstrap)") + elif watch_follows: + # Don't cry wolf: the operator DID ask to hold these hosts, and phase 2 does it. + ui.info("watchers are attached in phase 2, immediately after this phase completes") + else: + ui.info( + "NOT quarantining: the bootstrap is autonomous, so these hosts will go live and " + "claim production work unvalidated — pass --quarantine-on-register for fresh hardware" + ) if action == "provision": ui.info( "hosts will be quarantined on registration" @@ -354,7 +544,10 @@ def _cmd_for(host: str) -> list[str]: ui.wire(" ".join(_cmd_for(host))) return 0 - log_dir = _log_dir() + # An explicit dir lets a multi-phase run keep every phase under one batch directory, so the + # operator has a single place to look and the resume roster sits beside the logs. + log_dir = log_dir or _log_dir() + log_dir.mkdir(parents=True, exist_ok=True) ui.info(f"logs: {log_dir}") # Resolve secrets once, here, before any child starts. See _prewarm_secret_env: N children @@ -363,7 +556,10 @@ def _cmd_for(host: str) -> list[str]: if secret_env: ui.info(f"pre-warmed {len(secret_env)} credential(s) — children won't call 1Password") - results: dict[str, HostResult] = {} + # The caller's dict when given one, populated as each host completes rather than at the end: + # _add_to_group_then_watch needs to know which hosts are already in the group even if the + # operator Ctrl-Cs mid-phase, because those hosts keep bootstrapping either way. + results: dict[str, HostResult] = results_out if results_out is not None else {} started = time.monotonic() done = 0 total = len(hosts) diff --git a/orchestrator/orchestrator/cli.py b/orchestrator/orchestrator/cli.py index 15cd545..8c4ca18 100644 --- a/orchestrator/orchestrator/cli.py +++ b/orchestrator/orchestrator/cli.py @@ -74,14 +74,28 @@ def provision( @_app.command() -def quarantine_on_register(hostname: str) -> None: +def quarantine_on_register( + hostname: str, + max_wait_seconds: int = typer.Option( + 0, + "--max-wait-seconds", + help="Watch budget. Default (900s) assumes bootstrap is already finished. Starting the " + "watch at group-add needs ~30 min of budget — pass it explicitly rather than exporting " + "REPROVISION_QUARANTINE_ON_REGISTER_MAX_WAIT_SECONDS.", + ), +) -> None: """Wait for a fresh worker to appear in Taskcluster, then quarantine it on sight. A worker that isn't registered yet can't be quarantined (`quarantineWorker` 404s), so this watches for it. Narrows the window between registration and the first claimed task to seconds — it does not eliminate it. Use standalone for hosts already mid-bootstrap. + + A budget that expires before the worker registers is not a harmless timeout: the host then + goes live UNHELD, which is the exact failure this command exists to prevent. """ - workflow.step_quarantine_on_register(workflow.resolve_offline(hostname)) + workflow.step_quarantine_on_register( + workflow.resolve_offline(hostname), max_wait_seconds=max_wait_seconds or None + ) @_app.command() @@ -122,7 +136,8 @@ def batch( action: str = typer.Option( "provision", "--action", - help="What to run per host: preflight | mint | os-update | add-to-group | validate | provision.", + help="What to run per host: preflight | mint | os-update | add-to-group | " + "quarantine-on-register | validate | provision.", ), concurrency: int = typer.Option( 0, "--concurrency", "-j", help="How many hosts in flight (default 3 — MDC1 throughput, not CPU)." @@ -240,6 +255,42 @@ def add_to_group( ) +@_app.command() +def group_parity( + group_id: int = typer.Option( + 0, "--group-id", help="Group to check (default: settings.bootstrap_group_id)." + ), + reference_group_id: int = typer.Option( + 0, "--reference-group-id", help="Group to measure against (default: settings.reference_group_id)." + ), + reference_sample: int = typer.Option( + 0, "--reference-sample", help="Reference devices to intersect for the baseline (default 5)." + ), + max_devices: int = typer.Option( + 0, "--max-devices", help="Check only the first N devices of the group (default: all)." + ), + host: str = typer.Option( + "", "--host", help="Check one host instead of the whole group (needs SSH, to read its serial)." + ), +) -> None: + """Do this group's hosts get the profiles a working production host gets? (read-only) + + Run this BEFORE a wave. A group that receives freshly-erased hosts must carry Skip Setup + Assistant and the FDA SSH Keygen Wrapper or every host hangs at the Wi-Fi pane — and that + failure presents as "Safari automation is broken", not as a missing profile (m4-214). + + Compares effective per-device profile sets, so it does not flag profiles that reach the hosts + by another additive path. Needs only the SimpleMDM API key; writes nothing. + """ + workflow.step_group_parity( + group_id=group_id or None, + reference_group_id=reference_group_id or None, + reference_sample=reference_sample or None, + max_devices=max_devices, + hostname=host or None, + ) + + @_app.command() def validate( hostname: str, diff --git a/orchestrator/orchestrator/clients/simplemdm.py b/orchestrator/orchestrator/clients/simplemdm.py index d44d9aa..ead5a1e 100644 --- a/orchestrator/orchestrator/clients/simplemdm.py +++ b/orchestrator/orchestrator/clients/simplemdm.py @@ -108,6 +108,56 @@ def assignment_group_device_ids(group_id: int) -> list[int]: return [int(d["id"]) for d in rel.get("devices", {}).get("data", [])] +def _paginated(path: str, *, limit: int = 100) -> list[dict]: + """Every page of a SimpleMDM list endpoint, following has_more/starting_after. + + Not optional for correctness: /custom_configuration_profiles returns has_more=True at the + default page size on this account, so a single unpaginated GET silently reports a partial + profile set — and a parity check built on a partial set reports missing profiles that are + simply on page two. + """ + out: list[dict] = [] + params: dict[str, object] = {"limit": limit} + while True: + page_json = _request("GET", path, params=params).json() + page = page_json.get("data", []) + out += page + if not page_json.get("has_more") or not page: + return out + params["starting_after"] = page[-1]["id"] + + +def device_profiles(device_id: int) -> dict[int, str]: + """Configuration profiles SimpleMDM considers assigned to this device, {id: name}. + + This is the *effective* set, and that is the whole point. Profiles reach a device by several + independent paths — assignment groups, device groups — and only this endpoint composes them. + Comparing assignment groups to each other instead is actively misleading: the bootstrap group + is NOT attached to "Skip Setup Assistant - All Screens" or the FDA "SSH Keygen Wrapper", yet + its devices hold both, because a DEP arrival is still in the additive DEP Enrollment group. + A group-level diff therefore flags exactly the two profiles from the m4-214 postmortem as + missing when they are in fact delivered — the one false alarm this check cannot afford. + + NB: assignment-group records carry no `profiles` relationship at all (verified 2026-08-19: + the keys are apps / device_groups / devices / media). The link lives on the profile side, as + `relationships.groups`. + """ + return { + int(p["id"]): p.get("attributes", {}).get("name", f"profile {p['id']}") + for p in _paginated(f"/devices/{device_id}/profiles") + } + + +def assignment_groups() -> list[dict]: + """Every assignment group, with its device membership. One paginated sweep. + + Cheaper and more useful than asking per device: a device record does not list the assignment + groups it belongs to, so the only way to answer "what groups is this host in?" is to invert + this. 47 groups on this account as of 2026-08-19. + """ + return _paginated("/assignment_groups") + + def add_device_to_assignment_group(group_id: int, device_id: int) -> None: """ADD a device to an assignment group. Purely additive — never moves or unassigns. diff --git a/orchestrator/orchestrator/config.py b/orchestrator/orchestrator/config.py index 8b84318..70434ce 100644 --- a/orchestrator/orchestrator/config.py +++ b/orchestrator/orchestrator/config.py @@ -101,6 +101,24 @@ class Settings(BaseSettings): # override. bootstrap_group_id: int = Field(default=2417981) + # The group `group-parity` measures against: a live production group whose devices are known + # to work. Read-only here — PROTECTED_GROUP_IDS blocks WRITES to 2017918, and reading it is + # precisely how we learn what a working host is supposed to have. + reference_group_id: int = Field(default=2017918) + # How many reference-group devices to sample for the baseline. The baseline is the + # INTERSECTION of their profile sets, so a handful is enough and more only costs API calls: + # one atypical prod host can't drag a profile into the baseline, and a profile every sampled + # host has is one the fleet genuinely standardises on. + group_parity_reference_sample: int = Field(default=5) + + # Fan-out cap for SimpleMDM-bound batch work, independent of --concurrency. `add-to-group` + # makes three API calls per host with no SSH to pace it, so the API is the binding resource, + # not MDC1 throughput. At -j12 on 2026-08-14 the 429 retry budget blew: 5 hosts reported + # failure while 12 had ALREADY been added to the group (the add succeeded, the follow-up + # push_apps 429'd), so killing the batch orphaned 12 live bootstraps with no watcher and they + # would have gone into production unvalidated. + simplemdm_max_concurrent: int = Field(default=2) + # `validate` refuses a host whose main display isn't at this refresh rate. 60.0 because that is # what mozharness's own pre-test check enforces — matching it means validate agrees with what CI # will decide, rather than inventing a second standard. A KVM presenting 75Hz made m4-242 fail diff --git a/orchestrator/orchestrator/workflow.py b/orchestrator/orchestrator/workflow.py index ffd148c..d72cdf8 100644 --- a/orchestrator/orchestrator/workflow.py +++ b/orchestrator/orchestrator/workflow.py @@ -360,6 +360,230 @@ def step_add_to_group( ) +# Profiles whose absence has already cost real debugging time. Matched on a name SUBSTRING, not +# an id, so the warning survives a profile being rebuilt (which changes its id) or renamed around +# the stem. Anything missing from the baseline is reported; these get the story attached. +_LOAD_BEARING_PROFILES: tuple[tuple[str, str], ...] = ( + ( + "Skip Setup Assistant", + "a freshly-erased host then stops on the 'Select your Wi-Fi network' pane, which nothing " + "ever dismisses on an ethernet-only DC machine — and it presents as 'Safari automation is " + "broken', not as a stuck Setup Assistant (m4-214, cost most of a day)", + ), + ( + "SSH Keygen Wrapper", + "Full Disk Access for the SSH keygen wrapper — lost in the same m4-214 incident", + ), + ( + "CI Worker Support Binaries", + "the PPPC profile granting system-level TCC to the worker support binaries. Under SIP a " + "profile is the ONLY way those grants can land, so a SIP-on host without it diverges from " + "the prod fleet silently", + ), +) + + +# What share of a group's devices must belong to some OTHER group before a device that doesn't is +# treated as an outlier rather than as normal variation. Two thirds: high enough that a legitimate +# split (half a wave earmarked for a different role) isn't flagged, low enough to catch the single +# mis-clicked host among 40. +_MEMBERSHIP_QUORUM = 2 / 3 + + +def _membership_outliers( + gid: int, target_ids: list[int] +) -> dict[int, tuple[str, list[int]]]: + """Devices missing a group that almost all of their peers are in — i.e. moved, not added. + + Profile parity alone under-reports this. A device record does not list its assignment groups, + so this inverts the group->devices relationship once and asks the question peer-wise, with no + reference group involved: 39 of 40 hosts are in DEP Enrollment, so the 40th is the anomaly. + + Catches strictly more than the profile diff, because the groups a mis-clicked host loses are + not only profile-bearing. Found live on 2026-08-19: one bootstrap-group device was in that + group ALONE, having lost DEP Enrollment (Skip Setup Assistant, FDA), Relops Public SSH Key, + Sudoers and Enable SSH. Without the admin key the orchestrator cannot reach it at all — every + step is SSH — so the host is unprovisionable, and no profile-only check would say why. + """ + quorum = len(target_ids) * _MEMBERSHIP_QUORUM + out: dict[int, tuple[str, list[int]]] = {} + for group in simplemdm.assignment_groups(): + other = int(group["id"]) + if other == gid: + continue + ids = {int(d["id"]) for d in group.get("relationships", {}).get("devices", {}).get("data", [])} + if len([d for d in target_ids if d in ids]) < quorum: + continue + missing = [d for d in target_ids if d not in ids] + if missing: + out[other] = (group.get("attributes", {}).get("name", "?"), missing) + return out + + +def _device_label(device_id: int) -> str: + """id + serial + name. A bare device id is not identifying enough to act on. + + A fresh DEP arrival is named "Mac mini", so on 2026-08-19 a device id alone was mistaken for + macmini-m4-214 on nothing more than a matching enrollment date. The serial is what an operator + can actually search for in the SimpleMDM UI. + """ + try: + a = simplemdm.get_device(device_id).get("attributes", {}) + except ReprovisionError: + return f"device {device_id}" + name, serial = a.get("name") or "?", a.get("serial_number") or "?" + return f"device {device_id} (serial {serial}, named {name!r})" + + +def step_group_parity( + *, + group_id: int | None = None, + reference_group_id: int | None = None, + reference_sample: int | None = None, + max_devices: int = 0, + hostname: str | None = None, +) -> None: + """Do the hosts in the bootstrap group get the profiles a working prod host gets? + + Read-only, API-only: no SSH, no writes, safe on a live fleet. Nothing else in the toolchain + answers this, and it is the one question the m4-214 incident turned on — a host missing Skip + Setup Assistant and the FDA SSH Keygen Wrapper hung at first boot and presented as a Safari + fault. The postmortem's advice was to diff `profiles show -type configuration` against a + known-good host by hand, which needs SSH to a box that by definition may not be reachable yet. + This asks SimpleMDM instead, before a wave starts. + + The baseline is the INTERSECTION of the profile sets of several sampled reference-group + devices, not one sampled host: an intersection can't be skewed by a single atypical prod box, + and a profile that every sampled host carries is one the fleet genuinely standardises on. + + Deliberately compares EFFECTIVE per-device sets rather than the groups themselves — see + simplemdm.device_profiles. A group-level diff flags the two m4-214 profiles as missing from + the bootstrap group, which is true and irrelevant: its devices receive both from the additive + DEP Enrollment group. Crying wolf on that exact pair would train the operator to ignore this. + + Raises ReprovisionError (exit 1) when target devices lack baseline profiles. + """ + s = get_settings() + gid = group_id or s.bootstrap_group_id + ref_gid = reference_group_id or s.reference_group_id + n_sample = reference_sample or s.group_parity_reference_sample + + ui.step("GROUP PARITY", "do these hosts get the profiles a working prod host gets? (read-only)") + + if gid == ref_gid: + raise ReprovisionError( + f"group and reference group are both {gid} — nothing to compare. Pass " + "--reference-group-id to measure against a different group." + ) + + ref_name = simplemdm.get_assignment_group(ref_gid).get("attributes", {}).get("name", "?") + ref_devices = simplemdm.assignment_group_device_ids(ref_gid)[:n_sample] + if not ref_devices: + raise ReprovisionError( + f"reference group {ref_gid} ({ref_name}) has no devices — nothing to build a baseline " + "from. Point --reference-group-id at a populated production group." + ) + + baseline: dict[int, str] | None = None + for did in ref_devices: + profiles = simplemdm.device_profiles(did) + baseline = profiles if baseline is None else {i: n for i, n in baseline.items() if i in profiles} + assert baseline is not None + ui.info( + f"baseline: {len(baseline)} profile(s) common to {len(ref_devices)} device(s) " + f"in {ref_gid} ({ref_name})" + ) + if not baseline: + raise ReprovisionError( + f"the {len(ref_devices)} sampled devices in {ref_gid} share no profiles at all — that " + "group is too heterogeneous to be a baseline. Sample fewer, or pick another group." + ) + + # Targets: one named host, or the group's own membership. + if hostname: + device = _resolve_mdm_device(resolve_offline(hostname)) + targets = [(hostname, int(device["id"]))] + ui.info(f"checking {hostname} (device {targets[0][1]})") + else: + name = simplemdm.get_assignment_group(gid).get("attributes", {}).get("name", "?") + ids = simplemdm.assignment_group_device_ids(gid) + if not ids: + raise ReprovisionError(f"group {gid} ({name}) has no devices to check") + if max_devices: + ids = ids[:max_devices] + # A DEP arrival is named "Mac mini" in SimpleMDM, so device ids are the only stable label + # here. Not worth a GET per device to print a name they mostly don't have yet. + # Labelled lazily: naming 40 devices up front costs 40 GETs to print ids the operator + # mostly doesn't need. Only devices with a gap get resolved to a serial. + targets = [(f"device {i}", i) for i in ids] + ui.info(f"checking {len(targets)} device(s) in {gid} ({name})") + + # profile id -> (name, devices lacking it) + gaps: dict[int, tuple[str, list[str]]] = {} + for label, did in targets: + have = simplemdm.device_profiles(did) + for pid, pname in baseline.items(): + if pid not in have: + gaps.setdefault(pid, (pname, []))[1].append(label) + + total = len(targets) + if gaps: + ui.warn(f"{len(gaps)} profile(s) missing from at least one device") + else: + ui.ok(f"every checked device has all {len(baseline)} baseline profile(s)") + + # Second, independent question: is any device missing a GROUP its peers are all in? Catches + # the mis-clicked move, including the app-bearing groups a profile diff cannot see. + outliers = _membership_outliers(gid, [did for _label, did in targets]) if not hostname else {} + if outliers: + ui.warn(f"{len(outliers)} group(s) that most of these devices are in, some are not") + elif not hostname: + ui.ok("group membership is consistent across the group") + + if not gaps and not outliers: + ui.ok("parity: no gap against the prod fleet") + return + + sections: list[str] = [] + + if gaps: + lines = [] + for pid, (pname, lacking) in sorted(gaps.items(), key=lambda kv: -len(kv[1][1])): + why = next((story for stem, story in _LOAD_BEARING_PROFILES if stem in pname), "") + line = f"{pname} (profile {pid}) — missing on {len(lacking)}/{total} device(s)" + if len(lacking) <= 3: + line += "\n " + "\n ".join( + _device_label(d) for _l, d in targets if _l in lacking + ) + if why: + line += f"\n ^ {why}" + lines.append(line) + sections.append( + f"profile parity gap against {ref_gid} ({ref_name}):\n - " + "\n - ".join(lines) + ) + + if outliers: + lines = [] + for other, (oname, missing) in sorted(outliers.items(), key=lambda kv: -len(kv[1][1])): + lines.append( + f"{oname} ({other}) — {total - len(missing)}/{total} of these devices are in it, " + f"{len(missing)} are not:\n " + + "\n ".join(_device_label(d) for d in missing[:5]) + ) + sections.append( + "group membership outliers — these look MOVED rather than ADDED:\n - " + + "\n - ".join(lines) + ) + + raise ReprovisionError( + "\n\n".join(sections) + + "\n\nFix by attaching the profile to the group in SimpleMDM (a profile reaches devices " + "via its own `groups` relationship), or by ADDING the device to the groups it is missing. " + "Never MOVE it — a move strips the source group's profiles and apps, which is how this " + "state arises in the first place." + ) + + def step_validate(ctx: HostContext, *, expected_refresh_hz: float | None = None) -> None: """Read-only fitness check on a bootstrapped host: is it actually able to run tasks? diff --git a/orchestrator/tests/test_batch_phases.py b/orchestrator/tests/test_batch_phases.py new file mode 100644 index 0000000..7b39332 --- /dev/null +++ b/orchestrator/tests/test_batch_phases.py @@ -0,0 +1,184 @@ +"""`batch --action add-to-group --quarantine-on-register` must not be one `-j` serving two resources. + +The add is bound by the SimpleMDM API (three calls per host, no SSH to pace it); the watch is a +~30-minute wait bound by Taskcluster. Sharing one concurrency knob between them is what produced +the worst near-miss of wave 1: at `-j12` on 2026-08-14 five hosts *reported* failure while twelve +had already been added to the group, so killing the batch orphaned twelve autonomous bootstraps +with nothing watching them. They would have gone into production unvalidated. + +These tests pin the properties that stop that recurring, not the implementation: +concurrency is clamped per resource, a *failed* add is still watched, and an interrupt leaves a +resume roster on disk. +""" + +from __future__ import annotations + +import subprocess +import threading +from pathlib import Path +from unittest.mock import patch + +import pytest + +from orchestrator import batch +from orchestrator.config import get_settings + + +@pytest.fixture(autouse=True) +def _no_prewarm(): + with patch("orchestrator.batch._prewarm_secret_env", return_value={}): + yield + + +HOSTS = [f"macmini-m4-2{n:02d}" for n in range(40, 52)] # 12 hosts — the -j12 run + + +class _Recorder: + """Stands in for subprocess.run, recording every child invocation by phase.""" + + def __init__(self, codes: dict[tuple[str, str], int] | None = None): + self.codes = codes or {} + self.calls: list[dict] = [] + self.peak: dict[str, int] = {} + self._in_flight: dict[str, int] = {} + self._lock = threading.Lock() + + def __call__(self, cmd, stdout=None, stderr=None, timeout=None, check=False, env=None): + action = cmd[1] + host = next(c for c in cmd if c.startswith("macmini-")) + with self._lock: + self._in_flight[action] = self._in_flight.get(action, 0) + 1 + self.peak[action] = max(self.peak.get(action, 0), self._in_flight[action]) + self.calls.append({"action": action, "host": host, "cmd": cmd, "timeout": timeout}) + try: + threading.Event().wait(0.01) + finally: + with self._lock: + self._in_flight[action] -= 1 + return subprocess.CompletedProcess( + args=cmd, returncode=self.codes.get((action, host), 0), stdout=None, stderr=None + ) + + def hosts_for(self, action: str) -> list[str]: + return [c["host"] for c in self.calls if c["action"] == action] + + +def _run(rec: _Recorder, tmp_path: Path, hosts=None, **kw): + with patch("orchestrator.batch.subprocess.run", side_effect=rec), \ + patch("orchestrator.batch._log_dir", return_value=tmp_path), \ + patch("orchestrator.batch.ui.batch_summary"): + return batch.run_batch( + hosts or HOSTS, action="add-to-group", quarantine_on_register=True, **kw + ) + + +def test_the_coupled_invocation_runs_as_two_distinct_phases(tmp_path): + rec = _Recorder() + _run(rec, tmp_path) + # Sorted: both phases run concurrently, so completion order is not deterministic. + assert sorted(rec.hosts_for("add-to-group")) == sorted(HOSTS) + assert sorted(rec.hosts_for("quarantine-on-register")) == sorted(HOSTS) + # and the add child is no longer asked to do the watching itself + for call in rec.calls: + if call["action"] == "add-to-group": + assert "--quarantine-on-register" not in call["cmd"] + + +def test_the_add_phase_is_clamped_to_the_simplemdm_cap_whatever_j_says(tmp_path): + """-j is the knob operators reach for to speed up the watch. It must not reach SimpleMDM.""" + rec = _Recorder() + _run(rec, tmp_path, concurrency=12) + assert rec.peak["add-to-group"] <= get_settings().simplemdm_max_concurrent + + +def test_the_watch_phase_is_not_throttled_by_the_simplemdm_cap(tmp_path): + """A watcher is an idle poll loop; serialising 12 of them at -j2 costs hours for nothing.""" + rec = _Recorder() + _run(rec, tmp_path, concurrency=12) + assert rec.peak["quarantine-on-register"] > get_settings().simplemdm_max_concurrent + + +def test_a_host_whose_add_FAILED_is_still_watched(tmp_path): + """The 2026-08-14 case exactly: the POST landed, the follow-up push_apps 429'd. + + The host is in the group and bootstrapping regardless of what the batch called it, so not + watching it is how an unvalidated worker reaches production. + """ + rec = _Recorder(codes={("add-to-group", "macmini-m4-241"): 1}) + _run(rec, tmp_path) + assert "macmini-m4-241" in rec.hosts_for("quarantine-on-register") + + +def test_a_host_that_was_never_ready_is_not_watched(tmp_path): + """Exit 2 means the add never happened (no SSH, so its serial was never read).""" + rec = _Recorder(codes={("add-to-group", "macmini-m4-241"): batch.EXIT_NOT_READY}) + _run(rec, tmp_path) + assert "macmini-m4-241" not in rec.hosts_for("quarantine-on-register") + + +def test_the_watch_child_gets_a_bootstrap_spanning_budget_explicitly(tmp_path): + """The child's 900s default assumes bootstrap already finished; here it hasn't even started. + + Passed as an argument rather than left to REPROVISION_QUARANTINE_ON_REGISTER_MAX_WAIT_SECONDS, + because a budget that expires before the worker registers puts the host live UNHELD. + """ + rec = _Recorder() + _run(rec, tmp_path) + s = get_settings() + watch = next(c for c in rec.calls if c["action"] == "quarantine-on-register") + assert "--max-wait-seconds" in watch["cmd"] + budget = int(watch["cmd"][watch["cmd"].index("--max-wait-seconds") + 1]) + assert budget >= s.bootstrap_max_wait_seconds + # the subprocess timeout must outlast the child's own budget, or the batch kills the watcher + assert watch["timeout"] > budget + + +def test_an_interrupt_mid_add_leaves_a_resume_roster_on_disk(tmp_path): + """Silence on Ctrl-C is what turned an interrupt into twelve unheld production workers.""" + rec = _Recorder() + calls: list[str] = [] + + def _boom(cmd, **kw): + host = next(c for c in cmd if c.startswith("macmini-")) + calls.append(host) + if len(calls) > 2: + raise KeyboardInterrupt + return rec(cmd, **kw) + + errs: list[str] = [] + with patch("orchestrator.batch.subprocess.run", side_effect=_boom), \ + patch("orchestrator.batch._log_dir", return_value=tmp_path), \ + patch("orchestrator.batch.ui.batch_summary"), \ + patch("orchestrator.batch.ui.err", side_effect=errs.append), \ + pytest.raises(KeyboardInterrupt): + batch.run_batch(HOSTS, action="add-to-group", quarantine_on_register=True, concurrency=2) + + roster = tmp_path / "added.txt" + assert roster.exists(), "no resume roster written — the operator has no way back" + listed = [ln for ln in roster.read_text().splitlines() if not ln.startswith("#")] + assert listed, "roster names no hosts" + assert any("quarantine-on-register" in e for e in errs), "no resume command surfaced" + + +def test_each_phase_keeps_its_own_logs_under_one_batch_directory(tmp_path): + # Both phases run the same hostnames; sharing one flat directory would have phase 2 overwrite + # phase 1's log and lose the record of what the add actually did. + rec = _Recorder() + _run(rec, tmp_path, hosts=HOSTS[:2]) + assert (tmp_path / "add" / "macmini-m4-240.log").exists() + assert (tmp_path / "watch" / "macmini-m4-240.log").exists() + + +def test_dry_run_shows_both_phases_and_touches_nothing(tmp_path): + wires: list[str] = [] + with patch("orchestrator.batch.subprocess.run") as run, \ + patch("orchestrator.batch._log_dir") as log_dir, \ + patch("orchestrator.batch.ui.batch_summary"), \ + patch("orchestrator.batch.ui.wire", side_effect=wires.append): + assert batch.run_batch( + HOSTS[:2], action="add-to-group", quarantine_on_register=True, dry_run=True + ) == 0 + run.assert_not_called() + log_dir.assert_not_called() + joined = "\n".join(wires) + assert "add-to-group" in joined and "quarantine-on-register" in joined diff --git a/orchestrator/tests/test_group_parity.py b/orchestrator/tests/test_group_parity.py new file mode 100644 index 0000000..bb13ee1 --- /dev/null +++ b/orchestrator/tests/test_group_parity.py @@ -0,0 +1,270 @@ +"""`reprovision group-parity` — do a group's hosts get the profiles a working prod host gets? + +This is the check the m4-214 incident (2026-08-12) needed and nobody had. A host missing +"Skip Setup Assistant - All Screens" and the FDA "SSH Keygen Wrapper" came up on the Wi-Fi pane +at first boot, and because a modal Setup Assistant holds focus it presented as *Safari automation +is broken* — hours went into Safari versions, TCC grants and puppet ordering before the actual +cause surfaced. The postmortem's advice was to diff `profiles show -type configuration` against a +known-good host by hand, which needs SSH to a box that by definition may not be reachable. + +The load-bearing design decision, pinned below: compare EFFECTIVE per-device profile sets, never +assignment groups to each other. Verified against the live account on 2026-08-19 — the bootstrap +group is not attached to either m4-214 profile, yet its devices hold both, because a DEP arrival +is still in the additive DEP Enrollment group. A group-level diff reports exactly those two as +missing when they are in fact delivered, which would train an operator to ignore this check. +""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from orchestrator import workflow +from orchestrator.errors import ReprovisionError + +BOOTSTRAP_GID = 2417981 +PROD_GID = 2017918 + +SKIP_SETUP = (197155, "System Settings - Skip Setup Assistant - All Screens") +FDA_KEYGEN = (197148, "System Settings - Full Disk Access - SSH Keygen Wrapper") +PPPC = (222765, "TCC - CI Worker Support Binaries (PPPC)") +COMMON = (161696, "System Settings - Desktop & Dock - Disable Click to Show Desktop") + + +_GROUP_NAMES = { + PROD_GID: "gecko-t-osx-1500-m4", + BOOTSTRAP_GID: "gecko-t-osx-1500-m4-bootstrap", +} + + +def _group(gid): + return {"id": gid, "attributes": {"name": _GROUP_NAMES.get(gid, f"group-{gid}")}} + + +def _assignment_group(gid, name, device_ids): + return { + "id": gid, + "attributes": {"name": name}, + "relationships": {"devices": {"data": [{"id": d} for d in device_ids]}}, + } + + +def _parity(*, ref_devices, target_devices, profiles, hostname=None, all_groups=None, **kw): + """Drive step_group_parity against a fake fleet. `profiles` maps device id -> {pid: name}. + + `all_groups` is the account-wide assignment-group list the membership pass inverts; default + empty, so profile-only tests stay about profiles. + """ + def _ids(gid): + return ref_devices if gid == PROD_GID else target_devices + + def _dev(did): + return {"id": did, "attributes": {"name": "Mac mini", "serial_number": f"SER{did}"}} + + with patch("orchestrator.workflow.simplemdm.get_assignment_group", side_effect=_group), \ + patch("orchestrator.workflow.simplemdm.assignment_group_device_ids", side_effect=_ids), \ + patch("orchestrator.workflow.simplemdm.assignment_groups", return_value=all_groups or []), \ + patch("orchestrator.workflow.simplemdm.get_device", side_effect=_dev), \ + patch("orchestrator.workflow.simplemdm.device_profiles", + side_effect=lambda did: dict(profiles[did])) as dp: + workflow.step_group_parity( + group_id=BOOTSTRAP_GID, reference_group_id=PROD_GID, hostname=hostname, **kw + ) + return dp + + +# --- the baseline --- + + +def test_the_baseline_is_the_intersection_not_one_sampled_host(): + """One atypical prod box must not be able to drag a profile into the baseline. + + A union would flag every target host for a profile that only one reference host happens to + carry; a single sample makes the whole check depend on which device the API listed first. + """ + profiles = { + 1: dict([COMMON, SKIP_SETUP]), # reference hosts... + 2: dict([COMMON]), # ...disagree about SKIP_SETUP + 9: dict([COMMON]), # target lacks it too — but it isn't in the baseline + } + _parity(ref_devices=[1, 2], target_devices=[9], profiles=profiles) # must not raise + + +def test_a_profile_every_reference_host_has_and_the_target_lacks_is_a_gap(): + profiles = { + 1: dict([COMMON, PPPC]), + 2: dict([COMMON, PPPC]), + 9: dict([COMMON]), + } + with pytest.raises(ReprovisionError, match="CI Worker Support Binaries"): + _parity(ref_devices=[1, 2], target_devices=[9], profiles=profiles) + + +def test_full_parity_passes_quietly(): + profiles = {1: dict([COMMON, PPPC]), 9: dict([COMMON, PPPC])} + _parity(ref_devices=[1], target_devices=[9], profiles=profiles) + + +# --- the false-alarm this check cannot afford --- + + +def test_a_profile_delivered_by_another_additive_path_is_NOT_flagged(): + """The bootstrap group carries neither m4-214 profile; its devices get both from DEP Enrollment. + + Because the comparison is per-device and effective, that is correctly a non-event. If this ever + starts failing, someone has reimplemented the check against assignment-group attachment and it + will cry wolf on the exact pair from the postmortem. + """ + profiles = { + 1: dict([COMMON, SKIP_SETUP, FDA_KEYGEN]), + 9: dict([COMMON, SKIP_SETUP, FDA_KEYGEN]), # via DEP Enrollment, not the bootstrap group + } + _parity(ref_devices=[1], target_devices=[9], profiles=profiles) + + +def test_it_reads_the_effective_set_of_every_target_device(): + profiles = {1: dict([COMMON]), 9: dict([COMMON]), 10: dict([COMMON]), 11: dict([COMMON])} + dp = _parity(ref_devices=[1], target_devices=[9, 10, 11], profiles=profiles) + assert {c.args[0] for c in dp.call_args_list} == {1, 9, 10, 11} + + +# --- the report --- + + +def test_a_gap_reports_how_many_devices_are_affected(): + profiles = { + 1: dict([COMMON, PPPC]), + 9: dict([COMMON]), + 10: dict([COMMON, PPPC]), + 11: dict([COMMON]), + } + with pytest.raises(ReprovisionError, match=r"missing on 2/3 device"): + _parity(ref_devices=[1], target_devices=[9, 10, 11], profiles=profiles) + + +@pytest.mark.parametrize( + "profile,expected", + [ + (SKIP_SETUP, "Wi-Fi network"), # the symptom, not the cause — that's how you meet it + (FDA_KEYGEN, "m4-214"), + (PPPC, "SIP"), + ], +) +def test_a_known_load_bearing_profile_carries_its_story(profile, expected): + """A bare profile name doesn't tell an operator why they should care, or what it'll look like.""" + profiles = {1: dict([COMMON, profile]), 9: dict([COMMON])} + with pytest.raises(ReprovisionError, match=expected): + _parity(ref_devices=[1], target_devices=[9], profiles=profiles) + + +def test_the_fix_it_suggests_never_says_move(): + """A move strips the source group's profiles — the m4-214 root cause. It must not be advice.""" + profiles = {1: dict([COMMON, PPPC]), 9: dict([COMMON])} + with pytest.raises(ReprovisionError) as e: + _parity(ref_devices=[1], target_devices=[9], profiles=profiles) + assert "ADDING" in str(e.value) + assert "Never MOVE" in str(e.value) + + +# --- guards --- + + +def test_max_devices_caps_the_work(): + profiles = {1: dict([COMMON]), 9: dict([COMMON]), 10: dict([COMMON])} + dp = _parity(ref_devices=[1], target_devices=[9, 10], profiles=profiles, max_devices=1) + assert {c.args[0] for c in dp.call_args_list} == {1, 9} + + +def test_comparing_a_group_against_itself_is_refused(): + with pytest.raises(ReprovisionError, match="nothing to compare"): + workflow.step_group_parity(group_id=PROD_GID, reference_group_id=PROD_GID) + + +def test_an_empty_reference_group_is_refused(): + with pytest.raises(ReprovisionError, match="no devices"): + _parity(ref_devices=[], target_devices=[9], profiles={9: dict([COMMON])}) + + +def test_a_reference_group_with_nothing_in_common_is_refused(): + """An intersection of zero isn't "parity achieved" — it means the baseline is meaningless.""" + profiles = {1: dict([COMMON]), 2: dict([PPPC]), 9: {}} + with pytest.raises(ReprovisionError, match="share no profiles"): + _parity(ref_devices=[1, 2], target_devices=[9], profiles=profiles) + + +def test_single_host_mode_resolves_the_device_by_serial(): + """A DEP arrival is named "Mac mini" in SimpleMDM, so the serial is the only join key.""" + profiles = {1: dict([COMMON, PPPC]), 555: dict([COMMON])} + with patch("orchestrator.workflow.ssh.platform_serial", return_value="W4LT930Y9Q"), \ + patch("orchestrator.workflow.simplemdm.find_device_by_serial", return_value={"id": 555}), \ + pytest.raises(ReprovisionError, match="CI Worker Support Binaries"): + _parity( + ref_devices=[1], target_devices=[], profiles=profiles, hostname="macmini-m4-241" + ) + + +# --- group membership outliers: the mis-clicked MOVE, which a profile diff under-reports --- + +DEP_GID = 2017921 +SSHKEY_GID = 1514391 + + +def _clean(pids=(COMMON,)): + return dict(pids) + + +def test_a_device_missing_a_group_all_its_peers_are_in_is_flagged(): + """Found live 2026-08-19: one device sat in the bootstrap group ALONE. + + It had lost DEP Enrollment, Relops Public SSH Key, Sudoers and Enable SSH — so it was missing + the admin key too, and every orchestrator step is SSH. A profile-only check reported two + missing profiles and could not say the host was unreachable by design. + """ + targets = [9, 10, 11, 12] + profiles = {1: _clean(), **{d: _clean() for d in targets}} + groups = [ + _assignment_group(DEP_GID, "DEP Enrollment", [9, 10, 11]), # 12 is missing + _assignment_group(SSHKEY_GID, "Relops Public SSH Key", [9, 10, 11]), + ] + with pytest.raises(ReprovisionError) as e: + _parity(ref_devices=[1], target_devices=targets, profiles=profiles, all_groups=groups) + msg = str(e.value) + assert "DEP Enrollment" in msg and "Relops Public SSH Key" in msg + assert "MOVED rather than ADDED" in msg + assert "SER12" in msg, "the outlier must be identified by serial, not just an id" + + +def test_a_group_only_a_minority_share_is_not_an_outlier(): + """Half a wave earmarked for a different role is normal variation, not a mis-click.""" + targets = [9, 10, 11, 12] + profiles = {1: _clean(), **{d: _clean() for d in targets}} + groups = [_assignment_group(4242, "tart-vm-hosts", [9, 10])] # 50% < quorum + _parity(ref_devices=[1], target_devices=targets, profiles=profiles, all_groups=groups) + + +def test_the_group_being_checked_is_not_reported_against_itself(): + targets = [9, 10, 11] + profiles = {1: _clean(), **{d: _clean() for d in targets}} + groups = [_assignment_group(BOOTSTRAP_GID, "gecko-t-osx-1500-m4-bootstrap", targets)] + _parity(ref_devices=[1], target_devices=targets, profiles=profiles, all_groups=groups) + + +def test_consistent_membership_and_full_profiles_passes(): + targets = [9, 10, 11] + profiles = {1: _clean(), **{d: _clean() for d in targets}} + groups = [_assignment_group(DEP_GID, "DEP Enrollment", targets)] + _parity(ref_devices=[1], target_devices=targets, profiles=profiles, all_groups=groups) + + +def test_single_host_mode_skips_the_membership_pass(): + """One host has no peers to be an outlier against; the comparison would be meaningless.""" + profiles = {1: _clean(), 555: _clean()} + with patch("orchestrator.workflow.ssh.platform_serial", return_value="W4LT930Y9Q"), \ + patch("orchestrator.workflow.simplemdm.find_device_by_serial", return_value={"id": 555}), \ + patch("orchestrator.workflow.simplemdm.assignment_groups") as ag: + _parity( + ref_devices=[1], target_devices=[], profiles=profiles, hostname="macmini-m4-241", + all_groups=[_assignment_group(DEP_GID, "DEP Enrollment", [1])], + ) + ag.assert_not_called()