PMM-15326: Report the executor fleet, not only the part of it that works by plebioda · Pull Request #1390 · percona/pmm-extensions

plebioda · 2026-08-21T13:17:25Z

Summary

Adds GET /api/tasks/hosts/states/: one entry per host the executor backend knows about, usable or not, with reachable and driver_healthy reported separately.

GET /hosts/ answers "where can a job be placed". It gets there by filtering on three conditions at once - Status == ready, raw_exec present in Drivers, and that driver's Healthy flag - and returning a name-to-address mapping. That is the right answer for a dispatcher and all a dispatcher needs.

It is the wrong answer for anything reporting on the fleet, because the three conditions collapse into one bit and every failure lands in the same place: absence from the mapping. A machine that is missing may never have been onboarded, or be onboarded and down, or be up with a broken driver - three problems for three different people, and the caller cannot tell which, or even that the machine exists.

Nothing about /hosts/ changes and no caller is moved. get_host_states is concrete on BaseExecutor, defaulting to "everything get_hosts returns, reachable and healthy", so a backend with no notion of an unusable host says nothing (CeleryExecutor runs the work in-process); Nomad overrides it.

Three details worth a reviewer's attention:

The unfiltered node list is fetched without resources=True. The stub entries already carry Status and Drivers, and the detail fetch is one request per node against a Nomad that may have hundreds.
A missing raw_exec key reads as unhealthy, not as absent-so-fine. Nomad omits drivers it has not detected, so a never-onboarded host has no key at all, and treating that as healthy would report the emptiest case as the best one.
detail carries the driver's HealthDescription only when it is a problem - Nomad sets it to the literal "Healthy" on a working driver, and a field whose job is to explain failures must not be full of the word "Healthy". For an unreachable node it comes from the node stub's StatusDescription instead, since the driver fields there are a stale pre-disconnect snapshot (added in review).

The raw_exec driver name and the ready status are now named constants shared by the dispatch filter and the reporting, so the two cannot drift into disagreeing about what "healthy" means.

Worth calling out for review: this endpoint is broader than /hosts/. It returns every registered client's name, address and driver detail to any authenticated tasks user, where /hosts/ returns only the placeable ones. That is deliberate - "why can nothing run on this machine" is unanswerable otherwise - but it is a disclosure change and should be an explicit decision rather than an inferred one. Happy to admin-gate it if that is the preference.

Why this is its own PR

Wanted by OpenManager, which has to describe the hosts it cannot probe, but nothing here is OpenManager-specific: no OM code, no OM imports, and the OM app reaches this over HTTP like any other client. It is the first of four PRs replacing the single 21k-line draft #1371, which stays open until the replacements are up. The others are the core app-owned-settings extraction, the om_inventory app itself, and the side-car activation.

Base is main - this PR was retargeted here after starting against PMM-15299-open-manager.

The first two commits are the original submission: the code, then the regenerated OpenAPI spec and TS client on their own so the first commit was all a reviewer had to read. Everything after that responds to review: the down-node detail gap, moving ExecutorHostState next to its Tasks-service siblings and dropping the dead usable property, hardening a couple of tests, and the changelog fragment. See the inline replies on each thread for which commit addresses it.

Tested

CI is green on this branch (python / test, frontend, precommit-*, typecheck_diff, etc.) now that it targets main and the label-gate has qa not required.
venv/bin/python -m pytest tests/app -q -n 8 locally: 14479 passed, 501 skipped, plus 3 failures in tests/app/test_host_payloads.py that are pre-existing and unrelated to this change (Python-stdlib-version checks; CI's own python / test (3.12.3) job passes clean, so this looks like a local sandbox quirk, not a real regression).
tests/app/tasks/execution/executors/nomad/test_models.py - get_host_states over a ready host, a down host with a StatusDescription, a host with an unhealthy driver, a host with no raw_exec key at all, a host with a malformed (non-boolean) Healthy value, and the HealthDescription passthrough; get_hosts's exact filter_ expression is now pinned too
tests/app/tasks/execution/executors/celery/test_models.py - the base default, that a backend which does not override it reports its hosts as reachable and healthy
tests/app/tasks/test_routes.py - the route, its 502 mapping for both a connection failure and a non-JSON body, and that /hosts/ is unchanged
tests/app/test_openapi_specs_fresh.py - the committed spec matches scripts/dump_openapi.py
make run-pre-commit scoped to the changed files - ruff, ruff-format, oxfmt, oxlint and the changelog-fragment hook all pass

Checklist

New/modified functions have type hints and rST docstrings
New tests added for new features or bug fixes
All tests pass locally (make test)
Pre-commit hooks pass (make run-pre-commit)
Database migrations generated if models changed (make makemigrations) - N/A, no model changes
User-facing changes documented (README, inline help, UI text) - N/A. The two docs that name /api/tasks/hosts/ (README.md on DEFAULT_EXECUTOR_HOST, docs/customer/nomad-driver-deployment.md on choosing a dispatch target) both describe where a job can be placed, which this does not change. The new endpoint carries its own OpenAPI description.
Configuration changes documented with examples - N/A, no configuration changes
Changelog fragment added under changelog.d/ - changelog.d/PMM-15326.added.md, now that TICKET_PROJECTS accepts PMM tickets directly (SEP-2083: Accept PMM ticket keys in changelog fragments #1536); no separate SEP-xxxx ticket needed.

Three reads that answer "what is out there and where can a probe run", before anything is dispatched. Services come from SEP's inventory, filtered to MongoDB. Hosts come from SEP's **nodes**, not from those services - which is the whole point. Enumerating from services can only ever produce hosts that already run a database, and the case worth catching is the one where none does. Crossing nodes with the executor list gives four states: has executor no executor has MongoDB service normal: probeable monitored, not actionable no MongoDB service **nothing installed yet** monitored only The bottom-left cell is the valuable one, not something to filter out: a reachable host with no database is where a database can be installed. What *is* filtered out is the bottom-right - a node with neither a MongoDB service nor an executor is some other machine PMM happens to monitor, and PMM's own server node is one of them. Matching a node to its executor host reuses the order `BaseTaskSyncer.get_task_target` uses per service - name first, then address - but at the **host** level, where it belongs: every service on a host resolves to the same executor, so asking once per host is both cheaper and impossible to answer inconsistently. What it deliberately does not copy is that method's fallback. With `strict_executor_matching` off, an unmatched node resolves to `next(iter(available_hosts))` - an arbitrary unrelated host - and the probe would run there and report facts about a mongod that is not on that box. Here an unmatched service is `ORPHANED` and is not probed at all. That case is the norm, not an edge: an inventory row routinely outlives the executor that served it. Hosts resolve against **every** known executor rather than the usable subset, because a host served by a registered-but-broken client has to resolve or its row reports "no executor" and sends the reader after an onboarding problem that is not there. Dispatch still works from the usable ones, so nothing is dispatched anywhere new. Two consequences worth stating, because both decide what ends up in the estate: - **A host does not leave the estate when its agent stops.** Scope is decided on whether an executor matched, not on whether it works. Deciding it on usability would drop a machine at exactly the moment someone starts looking for it - and for a host with no database, drop it with no service to bring it back. - **`has_executor` means *usable*, not *matched*.** It is what decides whether a probe is dispatched, and a matched-but-down executor answering true there produces a dispatch that waits out its timeout instead of a row that explains itself. `executor_document` is emitted for every host whether or not anything ran on it, so "why can OM not probe this machine" is answered by the row rather than by its absence: `registered: false` says onboard the machine, `registered: true` with `driver_healthy: false` says go and look at the agent. It reads the fleet endpoint from #1390 for that; on a Tasks backend that predates it, the sweep raises rather than quietly reporting every host as fine. Duplicate registrations of one name collapse, preferring the usable one. Restarting a host's agent leaves the old registration behind as `down` beside the new one, so a plain dict comprehension keeps whichever came last and calls a running machine unreachable - measured on this workspace's sandbox, where `pmm-client-node00` was registered once ready and twice down and the sweep refused to dispatch to a host that was up. `get_hosts` never had to care, because everything in it was usable by construction. A node with no `external_id` is skipped and logged: PMM's node id is the key, and a row that cannot be keyed cannot be joined, triggered or updated. The cause is an inventory sync that has not caught up rather than anything about the host, which is why it is logged rather than silently dropped.

Copilot

Pull request overview

Adds executor fleet-state reporting while preserving the existing dispatch-oriented host endpoint.

Changes:

Adds /hosts/states/ and the ExecutorHostState contract.
Implements detailed Nomad state reporting with a default executor fallback.
Adds tests and regenerates OpenAPI/TypeScript clients.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`app/tasks/routes.py`	Exposes the authenticated host-state endpoint.
`app/tasks/execution/models.py`	Defines host-state data and default behavior.
`app/tasks/execution/executors/nomad/models.py`	Reports all Nomad client states.
`tests/app/tasks/test_routes.py`	Tests endpoint responses and failures.
`tests/app/tasks/execution/executors/nomad/test_models.py`	Covers Nomad host-state scenarios.
`tests/app/tasks/execution/executors/celery/test_models.py`	Covers the default executor behavior.
`frontend/packages/api/specs/tasks.json`	Regenerates the Tasks OpenAPI specification.
`frontend/packages/api/src/generated/tasks.ts`	Regenerates TypeScript API types.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Rebuilt, not updated, per convention for this branch: om-inventory (which already carries app-settings as its base), fleet-states, and app-settings' own new commits, all now with their #1395/#1390/#1393 review fixes applied. om-inventory: 2dba51f -> 1bec897 (11 commits) fleet-states: 082739b -> a4e0a0e (3 commits) app-settings: a276651 -> 31c17c3 (4 commits) # Conflicts: # tests/app/sep/api/routes/test_settings_proxy.py

Pulls in SEP-1860 (default inventory-sync schedule for the embedded profile, #1407) plus everything else that landed on main since the last merge, so the three PMM-15326 app branches (#1390, #1393, #1395) can rebase onto a base that carries it.

Three reads that answer "what is out there and where can a probe run", before anything is dispatched. Services come from SEP's inventory, filtered to MongoDB. Hosts come from SEP's **nodes**, not from those services - which is the whole point. Enumerating from services can only ever produce hosts that already run a database, and the case worth catching is the one where none does. Crossing nodes with the executor list gives four states: has executor no executor has MongoDB service normal: probeable monitored, not actionable no MongoDB service **nothing installed yet** monitored only The bottom-left cell is the valuable one, not something to filter out: a reachable host with no database is where a database can be installed. What *is* filtered out is the bottom-right - a node with neither a MongoDB service nor an executor is some other machine PMM happens to monitor, and PMM's own server node is one of them. Matching a node to its executor host reuses the order `BaseTaskSyncer.get_task_target` uses per service - name first, then address - but at the **host** level, where it belongs: every service on a host resolves to the same executor, so asking once per host is both cheaper and impossible to answer inconsistently. What it deliberately does not copy is that method's fallback. With `strict_executor_matching` off, an unmatched node resolves to `next(iter(available_hosts))` - an arbitrary unrelated host - and the probe would run there and report facts about a mongod that is not on that box. Here an unmatched service is `ORPHANED` and is not probed at all. That case is the norm, not an edge: an inventory row routinely outlives the executor that served it. Hosts resolve against **every** known executor rather than the usable subset, because a host served by a registered-but-broken client has to resolve or its row reports "no executor" and sends the reader after an onboarding problem that is not there. Dispatch still works from the usable ones, so nothing is dispatched anywhere new. Two consequences worth stating, because both decide what ends up in the estate: - **A host does not leave the estate when its agent stops.** Scope is decided on whether an executor matched, not on whether it works. Deciding it on usability would drop a machine at exactly the moment someone starts looking for it - and for a host with no database, drop it with no service to bring it back. - **`has_executor` means *usable*, not *matched*.** It is what decides whether a probe is dispatched, and a matched-but-down executor answering true there produces a dispatch that waits out its timeout instead of a row that explains itself. `executor_document` is emitted for every host whether or not anything ran on it, so "why can OM not probe this machine" is answered by the row rather than by its absence: `registered: false` says onboard the machine, `registered: true` with `driver_healthy: false` says go and look at the agent. It reads the fleet endpoint from #1390 for that; on a Tasks backend that predates it, the sweep raises rather than quietly reporting every host as fine. Duplicate registrations of one name collapse, preferring the usable one. Restarting a host's agent leaves the old registration behind as `down` beside the new one, so a plain dict comprehension keeps whichever came last and calls a running machine unreachable - measured on this workspace's sandbox, where `pmm-client-node00` was registered once ready and twice down and the sweep refused to dispatch to a host that was up. `get_hosts` never had to care, because everything in it was usable by construction. A node with no `external_id` is skipped and logged: PMM's node id is the key, and a row that cannot be keyed cannot be joined, triggered or updated. The cause is an inventory sync that has not caught up rather than anything about the host, which is why it is logged rather than silently dropped.

Rebuilt (disposable by convention, not updated) as a merge of the two branches stacked on PMM-15299-open-manager plus SEP-1825/SEP-1860 from main: fleet-states (#1390) and om-inventory (#1395). app-settings (#1393) is no longer one of the parents -- SEP-1825 already shipped the same 'app owns its settings class' mechanism it was building, so om-inventory now registers OmInventorySettings the same way Alerts/Report/Inventory do (AppOwnedClassEntry, string-keyed, no SettingClassEnum member, no CHECK-constraint migration), and #1393's own commit was dropped from om-inventory's history rather than merged in.

percona/pmm-extensions#1395's review fix (SCHEMA_TRANSLATE_MAP moved off core, into a DatabaseOptions field apps declare) landed on PMM-15326-om-inventory and needed carrying through the harness overlay too (previous commit) -- without it every ./om inventory run against PMM's embedded Postgres failed with the om schema unresolved. Re-merges the fixed om-inventory (percona/pmm-extensions#1395) with fleet-states (percona/pmm-extensions#1390, unaffected by the schema change, remerges cleanly).

percona/pmm-extensions#1395's review fix (SCHEMA_TRANSLATE_MAP moved off core, into a DatabaseOptions field apps declare) landed on PMM-15326-om-inventory and needed carrying through the harness overlay too (previous commit) -- without it every ./om inventory run against PMM's embedded Postgres failed with the om schema unresolved. Re-merges the fixed om-inventory (percona/pmm-extensions#1395), fleet-states (percona/pmm-extensions#1390), and om-switch-gate (percona/pmm-extensions#1437, rebased onto the fixed om-inventory tip separately).

percona/pmm-extensions#1395's review fix (SCHEMA_TRANSLATE_MAP moved off core, into a DatabaseOptions field apps declare) landed on PMM-15326-om-inventory and needed carrying through the harness overlay too (previous commit) -- without it every ./om inventory run against PMM's embedded Postgres failed with the om schema unresolved. Re-merges the fixed om-inventory (percona/pmm-extensions#1395), fleet-states (percona/pmm-extensions#1390), om-switch-gate (percona/pmm-extensions#1437), and this branch's own PMM-15347 bootstrap-payload commits, rebased onto the fixed integration tip.

percona/pmm-extensions#1395's follow-up fix: yyyyyyyan's minifier-collision comment on find_unregistered didn't reproduce at the pinned python-minifier 2.11.3 (confirmed independently), so the function reverts to its natural comprehension form with a real regression guard in its place rather than the unsupported rationale. No behavior change, comment/test only. Re-merges the fixed om-inventory with fleet-states (percona/pmm-extensions#1390, unaffected, remerges cleanly).

percona/pmm-extensions#1395's follow-up fix: yyyyyyyan's minifier-collision comment on find_unregistered didn't reproduce at the pinned python-minifier 2.11.3 (confirmed independently), so the function reverts to its natural comprehension form with a real regression guard in its place rather than the unsupported rationale. No behavior change, comment/test only. Re-merges the fixed om-inventory, fleet-states (percona/pmm-extensions#1390), and om-switch-gate (percona/pmm-extensions#1437, rebased onto the fixed tip separately).

percona/pmm-extensions#1395's follow-up fix: yyyyyyyan's minifier-collision comment on find_unregistered didn't reproduce at the pinned python-minifier 2.11.3 (confirmed independently), so the function reverts to its natural comprehension form with a real regression guard in its place rather than the unsupported rationale. No behavior change, comment/test only. Re-merges the fixed om-inventory, fleet-states (percona/pmm-extensions#1390), om-switch-gate (percona/pmm-extensions#1437), and this branch's own PMM-15347 bootstrap-payload commits, rebased onto the fixed integration tip.

Three reads that answer "what is out there and where can a probe run", before anything is dispatched. Services come from SEP's inventory, filtered to MongoDB. Hosts come from SEP's **nodes**, not from those services - which is the whole point. Enumerating from services can only ever produce hosts that already run a database, and the case worth catching is the one where none does. Crossing nodes with the executor list gives four states: has executor no executor has MongoDB service normal: probeable monitored, not actionable no MongoDB service **nothing installed yet** monitored only The bottom-left cell is the valuable one, not something to filter out: a reachable host with no database is where a database can be installed. What *is* filtered out is the bottom-right - a node with neither a MongoDB service nor an executor is some other machine PMM happens to monitor, and PMM's own server node is one of them. Matching a node to its executor host reuses the order `BaseTaskSyncer.get_task_target` uses per service - name first, then address - but at the **host** level, where it belongs: every service on a host resolves to the same executor, so asking once per host is both cheaper and impossible to answer inconsistently. What it deliberately does not copy is that method's fallback. With `strict_executor_matching` off, an unmatched node resolves to `next(iter(available_hosts))` - an arbitrary unrelated host - and the probe would run there and report facts about a mongod that is not on that box. Here an unmatched service is `ORPHANED` and is not probed at all. That case is the norm, not an edge: an inventory row routinely outlives the executor that served it. Hosts resolve against **every** known executor rather than the usable subset, because a host served by a registered-but-broken client has to resolve or its row reports "no executor" and sends the reader after an onboarding problem that is not there. Dispatch still works from the usable ones, so nothing is dispatched anywhere new. Two consequences worth stating, because both decide what ends up in the estate: - **A host does not leave the estate when its agent stops.** Scope is decided on whether an executor matched, not on whether it works. Deciding it on usability would drop a machine at exactly the moment someone starts looking for it - and for a host with no database, drop it with no service to bring it back. - **`has_executor` means *usable*, not *matched*.** It is what decides whether a probe is dispatched, and a matched-but-down executor answering true there produces a dispatch that waits out its timeout instead of a row that explains itself. `executor_document` is emitted for every host whether or not anything ran on it, so "why can OM not probe this machine" is answered by the row rather than by its absence: `registered: false` says onboard the machine, `registered: true` with `driver_healthy: false` says go and look at the agent. It reads the fleet endpoint from #1390 for that; on a Tasks backend that predates it, the sweep raises rather than quietly reporting every host as fine. Duplicate registrations of one name collapse, preferring the usable one. Restarting a host's agent leaves the old registration behind as `down` beside the new one, so a plain dict comprehension keeps whichever came last and calls a running machine unreachable - measured on this workspace's sandbox, where `pmm-client-node00` was registered once ready and twice down and the sweep refused to dispatch to a host that was up. `get_hosts` never had to care, because everything in it was usable by construction. A node with no `external_id` is skipped and logged: PMM's node id is the key, and a row that cannot be keyed cannot be joined, triggered or updated. The cause is an inventory sync that has not caught up rather than anything about the host, which is why it is logged rather than silently dropped.

marcuscruz-percona · 2026-09-17T19:16:48Z

Review verdict: 👍 close to ready — a few small follow-ups

Read this together with #1395 and #1437 as a stack. This one is the cleanest of the three: the split between reachable and driver_healthy is the right shape, the constants shared between dispatch and reporting are a nice touch, and the generated spec/client are fresh. Thanks for the careful PR description — it made verification easy.

Things worth doing before merge

Changelog fragment is now possible. scripts/changelog.py accepts PMM keys since SEP-2083: Accept PMM ticket keys in changelog fragments #1536 (TICKET_PROJECTS = ("SEP", "PMM")), and changelog.d/README.md even uses TICKET=PMM-15326 as its example. make changelog-add TICKET=PMM-15326 SECTION=added MSG='...' works today, so the blocker noted in the checklist is gone.
A down node arrives with no reason. detail is only filled from the driver (app/tasks/execution/executors/nomad/models.py:1129), so a host whose agent is down reports reachable=false, driver_healthy=true, detail=null. Nomad's node StatusDescription would fit there. Related: driver_healthy=true on a down node is a stale fingerprint presented as current (:1116,1122, pinned in test_models.py:1456-1466) — either gate it on reachable or say "last known" in the docstring.
reachable = Status == "ready" (:1121) treats an initializing client as unreachable, which reads differently from the field doc at app/tasks/execution/models.py:63-64.

Small things

app/tasks/routes.py:839-844 is a byte-for-byte copy of :809-814; a tiny wrapper would let the docstring's "wrapped the same way" be literally true.
ExecutorHostState.usable (models.py:84-90) has no production callers and doesn't serialize — peter-o-addo's computed_field question still stands.
The -- in models.py:55-56 still lands verbatim in tasks.json / tasks.ts.
Test gaps: the identity-check commit claims a malformed Healthy ("false", 1) reads unhealthy, but nothing asserts it; no JSONDecodeError case for /hosts/states/ like /hosts/ has; TestGetHosts doesn't assert the filter string, so a typo in the new constants would pass.
Nomad keeps a down ghost after an agent restart, so one name can appear more than once. The consumer in PMM-15326: Add the OpenManager Inventory app #1395 dedups; worth a sentence in the endpoint docstring.

Stack note: #1395 calls this endpoint and fails every sweep on a backend without it, so this needs to land first. 🚀

Three reads that answer "what is out there and where can a probe run", before anything is dispatched. Services come from SEP's inventory, filtered to MongoDB. Hosts come from SEP's **nodes**, not from those services - which is the whole point. Enumerating from services can only ever produce hosts that already run a database, and the case worth catching is the one where none does. Crossing nodes with the executor list gives four states: has executor no executor has MongoDB service normal: probeable monitored, not actionable no MongoDB service **nothing installed yet** monitored only The bottom-left cell is the valuable one, not something to filter out: a reachable host with no database is where a database can be installed. What *is* filtered out is the bottom-right - a node with neither a MongoDB service nor an executor is some other machine PMM happens to monitor, and PMM's own server node is one of them. Matching a node to its executor host reuses the order `BaseTaskSyncer.get_task_target` uses per service - name first, then address - but at the **host** level, where it belongs: every service on a host resolves to the same executor, so asking once per host is both cheaper and impossible to answer inconsistently. What it deliberately does not copy is that method's fallback. With `strict_executor_matching` off, an unmatched node resolves to `next(iter(available_hosts))` - an arbitrary unrelated host - and the probe would run there and report facts about a mongod that is not on that box. Here an unmatched service is `ORPHANED` and is not probed at all. That case is the norm, not an edge: an inventory row routinely outlives the executor that served it. Hosts resolve against **every** known executor rather than the usable subset, because a host served by a registered-but-broken client has to resolve or its row reports "no executor" and sends the reader after an onboarding problem that is not there. Dispatch still works from the usable ones, so nothing is dispatched anywhere new. Two consequences worth stating, because both decide what ends up in the estate: - **A host does not leave the estate when its agent stops.** Scope is decided on whether an executor matched, not on whether it works. Deciding it on usability would drop a machine at exactly the moment someone starts looking for it - and for a host with no database, drop it with no service to bring it back. - **`has_executor` means *usable*, not *matched*.** It is what decides whether a probe is dispatched, and a matched-but-down executor answering true there produces a dispatch that waits out its timeout instead of a row that explains itself. `executor_document` is emitted for every host whether or not anything ran on it, so "why can OM not probe this machine" is answered by the row rather than by its absence: `registered: false` says onboard the machine, `registered: true` with `driver_healthy: false` says go and look at the agent. It reads the fleet endpoint from #1390 for that; on a Tasks backend that predates it, the sweep raises rather than quietly reporting every host as fine. Duplicate registrations of one name collapse, preferring the usable one. Restarting a host's agent leaves the old registration behind as `down` beside the new one, so a plain dict comprehension keeps whichever came last and calls a running machine unreachable - measured on this workspace's sandbox, where `pmm-client-node00` was registered once ready and twice down and the sweep refused to dispatch to a host that was up. `get_hosts` never had to care, because everything in it was usable by construction. A node with no `external_id` is skipped and logged: PMM's node id is the key, and a row that cannot be keyed cannot be joined, triggered or updated. The cause is an inventory sync that has not caught up rather than anything about the host, which is why it is logged rather than silently dropped.

yyyyyyyan

@plebioda — the reachable / driver_healthy split is the right decomposition, and the two shared constants genuinely stop dispatch and reporting drifting apart — I evaluated both strings and the refactored filter is byte-for-byte the previous literal, so nothing changed underneath /hosts/. Skipping resources=True to avoid an N-request fan-out and keeping HealthDescription out of detail on a healthy driver are both details most implementations get wrong.

A host whose agent is down reports no reason at all — app/tasks/execution/executors/nomad/models.py:1129. detail is only ever sourced from the driver's HealthDescription, so a down host comes back reachable=false, driver_healthy=true, detail=null, and that driver_healthy=true is a stale pre-disconnect fingerprint reported as current. This is the one case the endpoint most needs to explain: the broken-driver path gets a sentence, the down path gets nothing, and the operator goes back to Nomad's API anyway. Nomad's node stub carries StatusDescription for exactly this and nothing in the repo reads it yet. Filling detail from it when the node is unreachable, and saying in driver_healthy's docstring that the value is last-known when reachable is false, closes it — detail has no consumer in the repo and no new field is added, so no spec or client regeneration.

The changelog fragment is owed, and the blocker the checklist cites is gone — no file. scripts/changelog.py:64 reads TICKET_PROJECTS: tuple[str, ...] = ("SEP", "PMM") as of #1536 (merged 2026-09-15), changelog.d/README.md:20 uses TICKET=PMM-15326 as its own worked example, and changelog.d/PMM-15455.changed.md is already on main. So make changelog-add TICKET=PMM-15326 SECTION=added MSG='...' works today and no SEP-xxxx is needed — which also resolves the open thread on routes.py:820 and the note peter-o-addo left about filing a separate ticket.

CI is red, and the description says it does not run here — no file. ci.yml:5-14 carries no branches: filter and the base is now main, so CI does run: label-gate fails with "PR requires 'qa passed' or 'qa not required' label to merge", and python, frontend, precommit-python, precommit-frontend, typecheck_diff and build all report skipping because each needs: label-gate. The effect is that no CI job has validated this diff while the description says none was expected. Adding qa not required (or qa passed) lets the suite run. The rebase also left the base branch, the "two commits" line and the changelog item stale. Locally I got tests/app/tasks/ 1936 passed / 0 failed / 26 skipped, 403 passed across the executors plus test_openapi_specs_fresh.py, and a clean ty on the added lines; ruff and the frontend linters are deny-listed for me, so those lanes are still unverified.

Seven further comments are inline — a dead usable property, three test gaps, and three smaller accuracy and placement notes.

Happy to re-review once the down-node detail lands.

plebioda · 2026-09-22T10:42:10Z

Pushed fixes for both reviews:

The blocking issue (@yyyyyyyan): a down node's detail now comes from the node stub's StatusDescription instead of reporting nothing - 256ffc665.
Moved ExecutorHostState into app/tasks/models.py next to its siblings and dropped the dead usable property - 85b8bccdc.
Pinned the dispatch filter's exact expression and added regression coverage for non-boolean Healthy values - d848dc1ce.
Added the missing non-JSON-response test for /hosts/states/ - eb45ec497.
Regenerated the spec/client for the docstring changes - 79af0cdc9.
Added the changelog fragment now that TICKET_PROJECTS accepts PMM directly - e1648bc4f.

Replied inline on each thread with the specific commit. CI is green on this push now that it's targeting main with qa not required set.

peter-o-addo's driver_present suggestion I left as-is for now - the down-node fix and status/detail together already cover the distinction OpenManager needs; easy to add if that changes.

Three reads that answer "what is out there and where can a probe run", before anything is dispatched. Services come from SEP's inventory, filtered to MongoDB. Hosts come from SEP's **nodes**, not from those services - which is the whole point. Enumerating from services can only ever produce hosts that already run a database, and the case worth catching is the one where none does. Crossing nodes with the executor list gives four states: has executor no executor has MongoDB service normal: probeable monitored, not actionable no MongoDB service **nothing installed yet** monitored only The bottom-left cell is the valuable one, not something to filter out: a reachable host with no database is where a database can be installed. What *is* filtered out is the bottom-right - a node with neither a MongoDB service nor an executor is some other machine PMM happens to monitor, and PMM's own server node is one of them. Matching a node to its executor host reuses the order `BaseTaskSyncer.get_task_target` uses per service - name first, then address - but at the **host** level, where it belongs: every service on a host resolves to the same executor, so asking once per host is both cheaper and impossible to answer inconsistently. What it deliberately does not copy is that method's fallback. With `strict_executor_matching` off, an unmatched node resolves to `next(iter(available_hosts))` - an arbitrary unrelated host - and the probe would run there and report facts about a mongod that is not on that box. Here an unmatched service is `ORPHANED` and is not probed at all. That case is the norm, not an edge: an inventory row routinely outlives the executor that served it. Hosts resolve against **every** known executor rather than the usable subset, because a host served by a registered-but-broken client has to resolve or its row reports "no executor" and sends the reader after an onboarding problem that is not there. Dispatch still works from the usable ones, so nothing is dispatched anywhere new. Two consequences worth stating, because both decide what ends up in the estate: - **A host does not leave the estate when its agent stops.** Scope is decided on whether an executor matched, not on whether it works. Deciding it on usability would drop a machine at exactly the moment someone starts looking for it - and for a host with no database, drop it with no service to bring it back. - **`has_executor` means *usable*, not *matched*.** It is what decides whether a probe is dispatched, and a matched-but-down executor answering true there produces a dispatch that waits out its timeout instead of a row that explains itself. `executor_document` is emitted for every host whether or not anything ran on it, so "why can OM not probe this machine" is answered by the row rather than by its absence: `registered: false` says onboard the machine, `registered: true` with `driver_healthy: false` says go and look at the agent. It reads the fleet endpoint from #1390 for that; on a Tasks backend that predates it, the sweep raises rather than quietly reporting every host as fine. Duplicate registrations of one name collapse, preferring the usable one. Restarting a host's agent leaves the old registration behind as `down` beside the new one, so a plain dict comprehension keeps whichever came last and calls a running machine unreachable - measured on this workspace's sandbox, where `pmm-client-node00` was registered once ready and twice down and the sweep refused to dispatch to a host that was up. `get_hosts` never had to care, because everything in it was usable by construction. A node with no `external_id` is skipped and logged: PMM's node id is the key, and a row that cannot be keyed cannot be joined, triggered or updated. The cause is an inventory sync that has not caught up rather than anything about the host, which is why it is logged rather than silently dropped.

yyyyyyyan

@plebioda — all eight of my comments are addressed and hold up at the current head; LGTM.

`GET /hosts/` answers "where can I place a job". `NomadExecutor.get_hosts` produces it by filtering on three conditions at once - `Status == ready`, `raw_exec` present in `Drivers`, and its `Healthy` flag - and returning a name-to-address mapping. That is the right answer for a dispatcher and all it needs. It is the wrong answer for anything reporting on the fleet, because the three conditions collapse into one bit and the failures land in the same place: absence. A machine missing from that mapping may never have been onboarded, or be onboarded and down, or be up with a broken driver. Those need three different people to fix them, and a caller looking at the mapping cannot tell which it is - or even that the machine exists. So `GET /hosts/states/` alongside it, returning one `ExecutorHostState` per host the backend knows about, with `reachable` and `driver_healthy` reported separately and the driver's own `HealthDescription` carried along. Nothing about `/hosts/` changes; no caller is moved. `get_host_states` is concrete on `BaseExecutor` rather than abstract, defaulting to "everything `get_hosts` returns, reachable and healthy". That is true by construction for any backend, and it means a backend with no notion of an unusable host does not have to say so - `CeleryExecutor` runs the work in-process and has nothing to add. Nomad overrides it. Making it abstract would have edited every implementation and every test double to say nothing. Three details worth keeping: - The unfiltered node list is fetched without `resources=True`. The stub entries already carry `Status` and `Drivers`, and the detail fetch is one request per node against a Nomad that may have hundreds. - A missing `raw_exec` key reads as unhealthy, not as absent-so-fine. Nomad omits drivers it has not detected, so the never-onboarded host has no key at all - treating that as healthy would report the emptiest case as the best one. - `detail` carries the driver's `HealthDescription` only when it is a problem. Nomad sets it to the literal "Healthy" on a working driver, and a field whose job is to explain failures must not be full of the word "Healthy". The driver name and the ready status are now named constants shared by the dispatch filter and the reporting, so the two cannot drift into disagreeing about what "healthy" means. Wanted by OpenManager, which has to describe the hosts it cannot probe, but nothing here is OpenManager-specific: "why can nothing run on this machine" is a question the tasks service is the only thing able to answer, and it will outlive the app that asked first.

…ates/ Derived, not authored: `tests/app/test_openapi_specs_fresh.py` runs `scripts/dump_openapi.py --check` and fails the moment a route exists that the committed spec does not carry, so the regeneration has to ride in the same change that adds the route. Kept as its own commit so the previous one is only the code a reviewer has to read. Both files are what the tooling produces, not what a hand wrote: `specs/tasks.json` from `scripts/dump_openapi.py` and `src/generated/tasks.ts` from `pnpm --filter @sep/api codegen` followed by `oxfmt --write src/generated`, which is `make regen-specs` minus the snapshot goldens no route here touches. 103 lines added to the spec and 101 to the client, none removed: the path, the `ExecutorHostState` schema, and the operation type. The other two clients and the other two specs are untouched, which is the check that this branch carries only its own share of the contract.

… fix

The prior commit regenerated frontend/packages/api/specs/tasks.json but missed the derived src/generated/tasks.ts, so the generated-client freshness check (git diff --exit-code -- packages/api/src/generated/) still failed CI.

ExecutionEvent, FileMetadata and TaskLog all live in app/tasks/models.py as the response shapes BaseExecutor's methods return; ExecutorHostState was defined locally in app/tasks/execution/models.py instead, so a reader looking for the Tasks service's response shapes found three in one place and the fourth somewhere else. Also drops the usable property: a plain @Property on a pydantic model does not serialize, so it never reached the OpenAPI schema, the generated client, or any caller - grepping the repo turns up no consumer. Test assertions against it now assert reachable/driver_healthy directly, which also reports more precisely which half of a failing case actually failed.

detail was sourced only from the driver, so an unreachable node came back reachable=false, driver_healthy=true, detail=null: the down case, the one this endpoint most needs to explain, got nothing, and driver_healthy was a stale pre-disconnect reading presented as current. Nomad's node stub carries StatusDescription for exactly this and the code had never read it. Fill detail from StatusDescription once the node is unreachable, keep the existing HealthDescription branch for reachable-but-broken, and read Name/Address defensively like every other field in the loop already does - this is the one path that drops get_hosts's filter, so it is also the only one that can see a partially-provisioned node.

test_get_hosts asserted the call happened but never looked at filter_, so a dropped clause or flipped operator in the expression that gates all job placement changed nothing any test observed. Separately, f478d98 switched get_host_states from bool(driver.get("Healthy")) to an identity check, but no case asserted the regression that commit was written to prevent: reverting the check would have kept the whole suite green. Add cases for a string "false" and a truthy int.

…dy does Both routes share the same except requests.exceptions.RequestException clause, but only /hosts/ had a test pinning the JSONDecodeError path; /hosts/states/ only had ConnectionError coverage. Mirror the existing /hosts/ test for parity.

…ring fixes ExecutorHostState's docstring is embedded verbatim in the schema description, so the reachable/driver_healthy wording and the :meth: target fixed above need the fixture and generated client to follow.

The fragment was blocked on filing a SEP ticket for the endpoint, since scripts/changelog.py only accepted a SEP or PMM ticket the project already recognized. #1536 widened TICKET_PROJECTS to accept PMM tickets directly, now merged to main, so no separate ticket is needed.

plebioda · 2026-09-22T20:58:11Z

Rebased onto `main` after #1395 squash-merged - the only conflict was both PRs independently adding `changelog.d/PMM-15326.added.md` as a new file. Resolved by keeping all four bullets (the three om_inventory ones plus the host-states one). Everything else (the actual code, routes, spec/client, tests) merged clean with zero overlap. Force-pushed; PR is mergeable again.

github-actions Bot assigned plebioda Aug 21, 2026

plebioda force-pushed the PMM-15326-fleet-states branch from d7fdc56 to 39fb30a Compare August 21, 2026 13:40

This was referenced Aug 21, 2026

PMM-15326: Let an app own its settings without a second config implementation #1393

Closed

PMM-15326: Add the OpenManager Inventory app #1395

Merged

plebioda marked this pull request as ready for review August 21, 2026 20:42

plebioda requested review from marcuscruz-percona, nachodd, peter-o-addo and yyyyyyyan as code owners August 21, 2026 20:42

plebioda force-pushed the PMM-15326-fleet-states branch from 39fb30a to 082739b Compare August 24, 2026 19:05

yyyyyyyan requested a balanced review from Copilot August 25, 2026 13:55

Copilot started reviewing on behalf of yyyyyyyan August 25, 2026 13:56 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Comment thread app/tasks/routes.py

Comment thread app/tasks/execution/executors/nomad/models.py Outdated

Comment thread tests/app/tasks/execution/executors/celery/test_models.py Outdated

Comment thread app/tasks/execution/models.py Outdated

plebioda force-pushed the PMM-15326-fleet-states branch from a4e0a0e to 16bf706 Compare September 1, 2026 07:07

plebioda mentioned this pull request Sep 11, 2026

Temporarily run CI on PRs staged against PMM-15299-open-manager #1508

Closed

plebioda added question Further information is requested and removed question Further information is requested labels Sep 11, 2026

plebioda added the skip-test label Sep 11, 2026

github-actions Bot added the svc:tasks PR touches the tasks service (app/tasks/) label Sep 15, 2026

marcuscruz-percona mentioned this pull request Sep 17, 2026

PMM-15326: Let PMM's OpenManager switch turn the estate sweep on and off #1437

Merged

plebioda changed the base branch from PMM-15299-open-manager to main September 21, 2026 20:11

plebioda requested a review from a team as a code owner September 21, 2026 20:11

plebioda force-pushed the PMM-15326-fleet-states branch from bc4c1f2 to 540bd29 Compare September 21, 2026 20:17

yyyyyyyan requested changes Sep 22, 2026

View reviewed changes

plebioda added the qa not required Merge without a QA sign-off: substitutes for 'qa passed' in label-gate. Does not skip any test job. label Sep 22, 2026

plebioda requested a review from a team as a code owner September 22, 2026 10:30

yyyyyyyan approved these changes Sep 22, 2026

View reviewed changes

plebioda added 12 commits September 22, 2026 22:54

PMM-15326: Use an identity check for driver health, not truthiness

0ce1cf5

PMM-15326: Match the file's em dash convention in a test docstring

7997ed2

PMM-15326: Regenerate the tasks OpenAPI spec for the :meth: docstring…

233df59

… fix

plebioda force-pushed the PMM-15326-fleet-states branch from e1648bc to 53c3a59 Compare September 22, 2026 20:57

yyyyyyyan merged commit cd041e1 into main Sep 23, 2026
23 checks passed

yyyyyyyan deleted the PMM-15326-fleet-states branch September 23, 2026 18:16

Navigation Menu

PMM-15326: Report the executor fleet, not only the part of it that works - #1390

PMM-15326: Report the executor fleet, not only the part of it that works#1390
yyyyyyyan merged 12 commits into
mainfrom
PMM-15326-fleet-states

plebioda commented Aug 21, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcuscruz-percona commented Sep 17, 2026

Uh oh!

yyyyyyyan left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plebioda commented Sep 22, 2026

Uh oh!

yyyyyyyan left a comment

Uh oh!

plebioda commented Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

plebioda commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why this is its own PR

Tested

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcuscruz-percona commented Sep 17, 2026

Review verdict: 👍 close to ready — a few small follow-ups

Uh oh!

yyyyyyyan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plebioda commented Sep 22, 2026

Uh oh!

yyyyyyyan left a comment

Choose a reason for hiding this comment

Uh oh!

plebioda commented Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

plebioda commented Aug 21, 2026 •

edited

Loading