PMM-15326: OpenManager initial implementation - #1371
Draft
plebioda wants to merge 23 commits into
Draft
Conversation
POM's read path lives in pmm-managed, which derives the topology document from PMM's inventory and VictoriaMetrics. What belongs in SEP is the half that needs a process on a database host, and this app is that half. pom_discovery sweeps the estate over Nomad on its own schedule: list the MongoDB services from SEP's inventory, resolve each strictly to the executor host its probe must run on, dispatch one payload per host, record what came back. An unmatched service is recorded as orphaned rather than probed somewhere wrong, because probing the wrong host reports confidently wrong facts about it. It collects what no metric carries -- the installed binary version above all, whose divergence from the running one is the upgraded-but-not-restarted case. GET /api/apps/pom_discovery/facts serves them for PMM to pull. That endpoint never probes: it answers from the last completed sweep and says how old it is, which keeps the consumer's collection sub-second against a source whose own work takes tens of seconds. A run records what it saw service by service, not four counters: executor host, how it was matched, whether it answered, the host's wall-clock, its fact count, its error. The duration is measured around the whole dispatch rather than read off the task history, because a host queued behind a busy client costs the sweep as much as a slow payload. It is monotonic and taken in a finally, so a host that failed instantly is still timed -- 0.28s says the dispatch was refused, which is a different fault from a host that took a minute. A dispatch that fails after reaching the queue is stopped. Otherwise the queue item stays RUNNING, the tasks API refuses an identical item, and every later sweep to that host answers 409 while reporting itself partial -- which reads as an unreachable node rather than a queue needing a clear. The release is best-effort, because it legitimately fails when the allocation is already gone; the reason is folded into the host's error, naming the id that will block the next probe. host.docker.internal joins ALLOWED_HOSTS: pmm-managed reaches this app from inside the pmm-server container, where the host is only addressable by that name.
settings.embedded.yaml's SEP.APPS is what image-sidecar-embedded strips the app set down to, so without an entry the app is absent from the image however the rest is configured -- restrict_apps.py deletes the package, and because the restricted image can only narrow the list, no environment variable or bind mount brings it back. POM is split across both products, so a PMM shipping this side-car also needs a build carrying managed/services/pom. That is unlike atw, whose PMM half is already on the branch.
Agreed on 2026-08-12: POM's SEP-side tables live in the existing `sep` database, isolated by a dedicated PostgreSQL schema. Not a separate database, which would make every call site choose a connection and need a second migration mechanism; not a table-name prefix, which isolates nothing. No behaviour change. This is the cheapest first step and it de-risks the rest: the tables that follow arrive into a schema that already works. The mechanism is SQLAlchemy's *symbolic* schema, copied from what SEP already does for the Celery beat tables. Models declare the token `pom_schema` and the engine translates it per bind -- to `pom` on PostgreSQL, to the default schema anywhere else. One set of table definitions therefore works on a deployment that wants the isolation and on SQLite, which has no schemas at all, with no branching in the models. app/sep/pom/ is new and holds what every POM app shares rather than what this one owns: discovery is the first app, and restart, configuration change, upgrade and installation are meant to follow into the same schema. An app that later ships without the others must not take the schema definition with it. Nothing in it touches sep_settings at import time -- the bind is passed in by the caller, because reaching for the lazy settings proxy mid-construction is what the Celery config already carries a warning about. Resolved to the default schema on everything except PostgreSQL. SQLite has no schemas short of an ATTACH, and MySQL's "schema" *is* a database, so honouring the setting there would scatter POM's tables into a second database nothing provisions. The table is `discovery_run`, not `pom_discovery_run`. The prefix was the only thing saying what the table was while it sat in the default schema; inside `pom` it is stutter. The migration is rewritten in place rather than followed by a move migration, because nothing has shipped -- there is no deployment whose data would be preserved. The cost is local and worth stating: a machine that already ran the old revision has to drop the old default-schema `pom_discovery_run` and delete the `a3f1c8d24b71` row from `alembic_version_sep` by hand, since the migration that would have moved it deliberately does not exist. Once POM ships, that file freezes and changes become new revisions. Three things in it are easy to get wrong: - CREATE SCHEMA cannot be symbolic. Raw DDL is not translated, so the migration asks pom_schema() for the real name and skips the statement where the bind has no schemas. Everything after it stays symbolic. - The version table stays `alembic_version_sep` in the default schema. Splitting head bookkeeping for a shared target across two schemas would be worse than the isolation is worth, and every other app on that target lives there. - Do not autogenerate these. Alembic applies no schema_translate_map during autogenerate and include_schemas is off, so --autogenerate proposes creating a literal `pom_schema` and re-creating the tables on every run. Treat an autogenerated POM diff as a bug. The test harness needed one change, and it is not the obvious one. This suite creates every service's metadata in a single in-memory SQLite database, so a POM table called `service` would be SEP inventory's `service` -- production never collides, since PostgreSQL separates by schema and SQLite keeps the two services in separate database files. Rather than translate the token in each of the ~30 places the suite builds an engine, the root conftest attaches an in-memory database under the token's own name: SQLite has no schemas but it has ATTACH, and an attached database is a schema as far as SQL is concerned. So `pom_schema.service` resolves untranslated and no fixture needs to know. The PostgreSQL and MySQL fixtures do still map it, into their per-worker schema, because neither has an ATTACH to lean on. No PMM-side provisioning change: postgres-sep already creates the `sep` role and database, the migration runs as that role, and the role therefore owns the schema it creates. Grants only become work if a future POM process connects as its own role.
Agreed on 2026-08-13: POM's inventory covers both host and service. A host may carry no service; a host may carry more than one, ideally one. Two tables, upserted, each a few real columns for identity and freshness plus one JSONB document of everything probed. Changing what is collected is then a payload change rather than a migration -- this is the start of the project and the attribute set will move more than once. The cost is that fleet-wide questions become JSONB queries; promoting a field to a column later is easy and the reverse is not. Why a host is an entity rather than an attribute of a service: it has state of its own, roughly half of what is collected belongs to it, it is what a future install or restart targets, and -- the decisive part -- a host with no database has to be representable. Against a service-keyed table it simply is not there. That case is measured, not assumed. This sandbox runs three hosts carrying a PMM client and nothing else, beside two arbiters running a mongod PMM has no service for, and **PMM's inventory describes the two identically**: same node type, zero services, the same four agents, the same null distro. Two entirely different situations -- "there is a database here PMM cannot authenticate against" and "there is no database here at all" -- and no field tells them apart. Only probing the host does, which is what the host document is for. Keyed on PMM's own ids, node_id and service_id, so the tables and the API speak the ids every consumer already holds and nothing needs translating anywhere, including a scoped refresh triggered with the node_id PMM has. Stored as text, not uuid: PMM's ids are usually UUIDs but not always -- the PMM server's own node is the literal string `pmm-server` in every deployment, and a uuid column would reject the one node every installation has. No POM-minted surrogate key. It only pays off if POM can recognise a re-registered machine and keep one row, and there is no natural key to do it with: `machine_id` is inherited from the container image, so most of this sandbox reports one shared value and the rest report an empty string. Matching on it would merge unrelated hosts. The consequence to accept is that a re-registered node gets a second row until retention prunes it. Enumeration changes source. The app listed services and could therefore only ever produce hosts that run a database; hosts now come from SEP's inventory nodes -- filled by PMMSyncer independently of services -- crossed with the Nomad executor list. A node with neither a MongoDB service nor an executor is some other machine PMM monitors and is left out; PMM's own server node is one. Executor matching reuses mapping.py's name-then-address order at the host level, where it belongs, and keeps its refusal to fall back to an arbitrary host: that would mean probing one machine and recording the answers against another. The freshness lifecycle is where the care went, because every rule reads as an implementation detail and none is. failing_since is set with COALESCE, so it keeps saying "failing for three days" rather than "failed a minute ago". A failed probe does not erase `observed` or `role` -- what a host was running when it was last reachable is exactly what is wanted while it is not. And an entity a run did not target keeps its columns untouched, which is what makes a scoped refresh possible later without marking the rest of the estate failed. That last rule has two teeth, both found against live data rather than reasoned out: - An orphaned service still gets a row. It is a service PMM knows about, and hiding it reports a healthier estate than exists -- the PoC measured 17 of 18 services unreachable in one run. What it does not get is an *attempt*. - A host with an executor and no service is seen every sweep and probed by none, because dispatch is driven by targets. Counting that as a failed attempt would have exactly the pmm-client hosts accrue a failure every ten minutes, forever, for a condition that is not a failure. The upsert is written column-explicitly. Nothing here is user-writable yet, so there is nothing to clobber -- but a blanket "update every column" wipes the first field that ever is, and the test for it cannot be added after the fact because by then the data is gone. It stands in on first_seen_at, the one column no attempt may move. pom.service.node_id is a real foreign key, so hosts are written first in the same transaction. Across apps the pom schema takes no foreign keys at all: each app's migrations are an independent branch, an image that strips an app removes its versions/ directory, and there is no ordering between branches, so a cross-app FK can reference a table that legitimately vanishes. Verified against the live sandbox: 17 host rows (4 with executors) and 14 service rows, of which the one running database is probed and carries installed_version 7.0.39-21; the three pmm-client hosts are seen with their executors recorded, no attempt and no failures; the 13 unreachable services are rows with their freshness columns untouched. Two things this deliberately does not do yet. Mongods PMM has not registered are scoped out as rows and will be recorded on the host's document, which needs the payload to report processes outside its target list. And a host with no database gets an empty document, because dispatch still needs a target -- probing a host for its own sake is the next step, and it is what fills in the OS, package manager and repo reachability the install case will want.
Dispatch was driven strictly by resolved services, so the machines POM could say
the least about were exactly the ones an install decision is about: a host
carrying a PMM client and no database has no service to be reached through. Its
row existed and its `observed` document was empty, permanently.
Three changes, one idea.
The payload now prints a record for the *host* -- OS, kernel, the installed
binary -- before any target records, and does so whether or not there are
targets. It used to exit 1 on an empty target list, treating "nothing to probe
here" as a misconfiguration when it is a legitimate and interesting state: it is
what a machine looks like before anything is installed on it.
The consumer tells the two apart by `service`: null on the host's record, always
a name on a service's. That is the whole discriminator, and it means the host
line cannot be mistaken for a service however a service is named.
probe_all takes the executor hosts of the whole estate as well as those serving
a resolved service, and dispatches to every one of them -- with that host's
services as targets where it has any, and an empty target list where it does
not.
The host's document now comes from the host's own record rather than from
whichever service record happened to answer. That was always a workaround: the
attributes are collected once per dispatch and belong to the host, so reading
them off a service was fine only while every host had a service to read them
off. It stops being fine the moment a host has none.
A failed dispatch that produced only the host record is no longer counted as a
failure. On a host with no database that record is the *only* thing the dispatch
had to produce, so treating "no service records" as failure would fail every
empty host by construction.
Verified against the sandbox: the three pmm-client hosts now carry
`{"os": "Ubuntu 24.04.3 LTS", "kernel": "6.17.0-35-generic", "collected_at": ...}`
and count as probed, where before they were seen with an empty document. 17 host
rows, 4 probed -- the four with a live executor -- and 14 service rows unchanged.
This is also the precondition for the two things that follow: recording mongods
PMM has not registered (the arbiter case) needs the payload to report processes
outside its target list, and the rest of the §11 attribute set -- package
manager, free space, repo reachability -- is per-host and now has a record to
land in.
A service row is a service PMM knows -- that is what keying on `service_id` means. But the probe finds mongod processes PMM has no service for, and dropping them let the estate view call a host empty while a database ran on it. They go on the **host's** `observed` document as `unregistered_mongods`, each with its program, pid, port, config path and command line. No identity to invent, no schema commitment, and the information survives. Promoting them to rows is the change whenever the discovery work needs it, and it needs a key -- which is where `(node_id, port)` comes back. The cause is worth knowing before anyone decides this is rare. An arbiter holds no data, therefore no user documents, therefore SCRAM cannot authenticate, therefore `pmm-admin add mongodb` fails for it -- so **any** estate with arbiters and authentication enabled has mongods PMM has no service for. This sandbox has two, and neither has ever been in PMM's service list. The payload gains three pieces. `collect_server_processes` reports every mongod and mongos, where `collect_process_facts` deliberately reports only the first -- a probe of one service wants one answer, and "what is actually running here" wants all of them. `parse_port` reads the port from the command line, falling back to the configuration file, because every node in this sandbox is started as `mongod --config <file>` with the port set inside it; an argv-only reading would find nothing on any of them and report every registered service as a stranger. `find_unregistered` subtracts the targets by port. A process whose port cannot be determined is reported rather than dropped. It cannot be matched to a target, and silently discarding a running database is exactly the dishonesty this list exists to prevent -- better a visible entry with a null port. A target with no port registers nothing, for the same reason: treating null as a wildcard would hide every stranger on the host. Verified against the sandbox with the sharded cluster running. Both arbiters report their mongod on 27018, PMM has no service for either, and every host whose services are registered reports none. Two things this turned up, neither of them new: - **Application changes need the backend restarted; payload changes do not.** The payload is read off disk per dispatch, so an edit reaches the next sweep; the sweep's own code is whatever the worker imported at start. This cost an hour of looking at a node that was reporting correctly into a sweep that was not reading the field. - **Stale host rows are real.** `standalone-node00` and the pmm-client hosts each have two rows: PMM re-registered them under new node ids when their pmm-agents were restarted, and POM keeps the old row because nothing prunes. PMM itself has exactly one node per name. This is the consequence §5.3 accepts openly in exchange for using PMM's ids, and it is the concrete argument for the retention question §15 leaves open.
… forgotten
The tables have been filling since `9a43b115` with nothing but psql to read them.
This is the read surface, plus the two deletes.
GET /hosts every host, its services nested
GET /hosts/{node_id} one host
GET /services flat, for consumers that work in services
GET /services/{service_id} one service
DELETE /hosts/{node_id} forget a host and what was on it
DELETE /services/{service_id} forget one service
`GET /hosts?has_service=false` is the question the host table exists to answer:
which machines carry a PMM client and no database. A filter rather than an
endpoint of its own, because the same list with the filter inverted is the
ordinary estate view and one list contract is enough to learn. `failing` and
`executor` filter the same way.
Both item paths take **PMM's** ids, which is the whole benefit of keying the
tables that way: the path is the id every consumer already holds, with no lookup
step on either side. There is deliberately no `/hosts/{id}/services` -- a host
already carries its services, so that would be a second spelling of the same
list, and `/services?node_id=` covers wanting them without the host.
DELETE is here because stale rows are not hypothetical. Restarting a node's
pmm-agent runs `setup --force`, which replaces the node in PMM and mints a new
id; POM keys on that id, so it gains a row and keeps the old one, and nothing
prunes. Today this sandbox had eleven such rows -- every one of them created by
restarts made while fixing something else. Without a delete the only cure is
psql against a schema an operator should never need to know exists.
It is not suppression. An entity PMM still knows about returns on the next
sweep, because POM's job is to describe what PMM says exists, not to hold an
opinion about it. And it is not retention: automatic pruning of entities that
vanish is still open, and this is what makes its absence survivable meanwhile.
**Deleting a host deletes its services explicitly, and that is not redundant.**
`pom.service.node_id` is `ON DELETE CASCADE`, so the DDL says the database will
do it -- but SQLite enforces no foreign key unless `PRAGMA foreign_keys=ON` is
set per connection, SEP sets it nowhere, and SQLite is the shipped default in
settings.yaml. On a default deployment the constraint is decoration, and
deleting a host would have left services pointing at a host that no longer
existed. The test caught it; reading the DDL would not have. The constraint
stays as the backstop it genuinely is on PostgreSQL.
Verified against the live sandbox, on the actual duplicates rather than
fixtures: `DELETE` on a stale `pmm-client-node00` row returned 204, a second
returned 404, and the host was left with exactly one row. Then every host row
whose node id PMM no longer knows was forgotten through the API -- eleven of
them -- leaving 18 hosts, 12 services and no duplicates. That is precisely what
retention will need to do automatically, done by hand to prove the surface can.
Three test failures that had been standing since the app was written go with
this, because they were never about this change:
- `tests/app/sep/snapshots/{openapi,schema}/pom_discovery.json` did not exist.
The app was added without generating them, so four snapshot tests failed on
every run. `make regen-specs` writes them; they now cover the new routes too.
- `test_contract.py::test_list_200` asserted a `GET /` list route this app has
never had, having been written for the task-contract scaffold rather than a
declared router. Removed rather than repaired: `test_schema_200` already proves
the router is mounted behind the auth guard, without needing a database, and
what the routes serve is pinned against a real session in the new tests.
The app's suite is green: 77 passed, none failing.
`POST /runs` takes `{"node_ids": [...]}`. Absent or empty still means everything,
which is what the scheduled sweep passes.
The two questions are different sizes. "What does the estate look like" is a
Nomad job per executor host: measured at 90-101 seconds across this sandbox's
fifteen. "I just did something to this host, is it healthy now" is one job, and
measured at 44 seconds - it should not have to wait on the other fourteen, and it
is the question every action PMM grows will ask next. The scope is node ids,
which PMM already holds, so its trigger passes them through untranslated.
**Conflict is judged per host.** The old guard refused any run while any other
was in flight, which with a ten-minute schedule means a scoped refresh is
usually refused exactly when someone wants one. Two runs now collide only when
they would touch the same host; a full refresh collides with everything,
including another full refresh. The stale-run reaper stays, because a crashed
worker still leaves a row nothing else advances - one was sitting at 900 seconds
while this was being written.
**Nothing outside the scope is written.** The narrowing happens in one place,
after enumeration and before any dispatch, so §5.4's "only a run that attempted
an entity touches its timestamps" stops being a principle and starts being the
thing that keeps a one-host refresh from marking the estate failed. An id that
names no enumerated host narrows to nothing rather than to everything, which is
the failure worth being paranoid about.
Three bugs found by building it, two of them older than this change:
- **A full-estate run stored the JSON scalar `null`, not SQL NULL.** SQLAlchemy's
JSON types default to `none_as_null=False`, so `WHERE scope IS NULL` matched no
full sweeps at all. The Python side reads back `None` either way, so nothing
complains until someone asks the database a question about scope. Exactly the
trap `bugs/sep-autojson-drops-none-as-null.md` records, walked into while
hand-rolling a Column specifically to avoid it. There is a test that asserts
against SQL rather than the response, because the response cannot tell.
- **Aging a run raised `TypeError` on SQLite.** `utc_now() - run.started_at`
subtracts an aware datetime from a naive one, because SQLite stores no timezone
- and SQLite is the shipped default. The stale-run guard would have 500'd on
precisely the request meant to recover from a crashed worker. Pre-existing;
no test had ever put a RUNNING row in front of the trigger. `get_facts` aged
the same way and is fixed with it.
- **A run that probed a host and no services reported FAILED.** `_terminal_status`
judged on services alone, which was right while every dispatch existed to reach
one. It stopped being right when a host became probeable for its own sake: a
scoped refresh of a pmm-client host resolves no services, because there are
none, and was reported as a failure for doing exactly what it was asked. It now
counts dispatches and services together. Measured on standalone-node00 before
and after: FAILED, then SUCCESS, same work.
Verified against the sandbox: a scoped refresh of a host with no database
completes in 44 seconds and reports SUCCESS, beside full sweeps of 90 and 101
seconds; conflicts answer 409 naming the run that holds the host.
Worth recording for whoever optimises next: the 44 seconds is almost entirely the
run-python job template building a venv and pip-installing pymongo on the host
before the payload runs. The probe itself is milliseconds. Scoping avoids paying
that fifteen times; not paying it at all is a bigger win still.
The estate replaces it. `/facts` answered "what did the last completed sweep
collect", which is a question about a run; `/services` answers "what is on this
service", which is the question the consumer was always asking. The difference is
only visible when a sweep fails: `/facts` dropped a service the newest sweep could
not reach, so PMM lost every probe field for it, while an upserted row keeps
reporting what it last saw and how old that is.
Nothing shipped this, so there is no compatibility window to keep. pmm-managed moved
to `/services` in the same change (pmm `5599e4876`), and `./om discovery show|facts`
with it - `facts` now reads the estate through the API, which makes a disagreement
between it and `./om discovery estate`, which reads the tables, a serialisation bug
worth finding rather than two views of different things.
`latest_terminal_run` goes with the endpoint: its whole reason for existing was
skipping a sweep in flight so the merge would not see an empty fact set for the
duration, and rows do not empty out while a run is going. `FACTS_MAX_AGE` goes too -
staleness is now per row, carried on the row, and there is nothing left to judge it
against globally. `GET /runs/{id}` keeps serving the run's own fact list, which is
still the honest record of what one sweep did.
plebioda
force-pushed
the
PMM-15326-pom-inventory
branch
from
August 19, 2026 22:57
860d724 to
ff40b01
Compare
…entation
REVIEW NOTE: this commit is deliberately on its own because it touches shared
authorization and configuration plumbing. **It needs sign-off from the PMM and SEP
teams before the app on top of it is merged.** The open question is written up in
PMM-15326's plan, §10; the short version is below.
**The question.** SEP's settings API (`/api/sep/admin/settings`) requires an admin.
PMM talks to SEP with a shared deployment token that resolves to the `sep-service`
account, and that account is not an admin - it is constructed with `is_admin` left at
its default (`app/api/deps.py:49`), so PMM gets a 403 from that router. Either the
account becomes an admin, or an app that PMM must configure serves its own endpoint.
**What this enables, and why.** The second option. Making the shared token an admin
would mean a leaked `SECRET_KEY` grants administrative access to all of SEP rather
than to one app's settings, which is a much larger blast radius than the problem
needs. A reviewer who disagrees should say so here rather than after the UI is built
on it.
**What actually changes.** Nothing about who can reach the existing router: it keeps
its admin gate, and no caller gains a permission. The local-class bodies of
`PATCH /{setting_class}` and `DELETE /{setting_class}/{key}` move out to
`apply_class_overrides` and `clear_class_override` so an app-owned endpoint can call
them instead of carrying its own copy. Two copies of "validate the batch, write it
atomically, republish the snapshot, rebind" drift into two different sets of
validation rules, and that is the failure this refactor exists to prevent. The
handlers now resolve the class, branch on remote, and delegate; the 353 tests over
this package pass unmodified.
`clear_class_override` is exported for a reason and not for symmetry: without a
revert, an app's own endpoint can set a value but never unset one, so "no override"
stops being reachable and the YAML the deployment ships becomes unrecoverable through
the API.
Also here, because both are shared files rather than app files:
- `POM_DISCOVERY_SETTINGS` joins `SettingClassEnum`, with the CHECK-constraint
migration on all three tracks. The member list is kept converged even though the
class is app-owned and collected only on the SEP side, so its rows only ever land
in the SEP database - the same reasoning that put `ALERTS_SETTINGS` and
`INVENTORY_SETTINGS` on tracks that never write them. Diverging constraints would
make "which members are allowed" a question about which database you are looking at.
- The web process's rebind-callback registry moves out of the lifespan into
`build_sep_override_callbacks`, and gains the entry that re-seeds beat when POM's
schedule changes. Every entry in that map is a change with an effect *outside* the
settings snapshot - a client rebound, a logging config re-applied, a beat row
rewritten - and a missing one is invisible from the handler code: the setting
changes, the API reports the new value, and nothing acts on it. Extracting it is
what makes the map assertable, which the next commit's tests rely on.
`SCHEDULE` is the setting that matters: nothing else refreshes the estate, so "how
often does POM look" is an operational question, and answering it today means editing
a file and restarting. `PomDiscoverySettings` becomes an overridable app-owned class
with `GET` / `PATCH` / `DELETE /config` on the app itself.
Builds on the previous commit, which carries the shared plumbing and the open
authorization question this depends on. In short: the app serves its own config
endpoint because SEP's settings router requires an admin and PMM's principal is not
one. **That decision is not settled - see the previous commit and the plan's §10.**
**`CREDENTIALS_PATH` is the one field that stays YAML/env only.** It names a file the
payload reads on every database *host* and hands to a driver as a URI, so making it
settable over the API would turn "configure this app" into "read a chosen file across
the estate" - a different permission from the one being argued for. Everything else
is hot.
Two registration steps beyond the obvious ones, both of which fail *silently*. The
endpoint answers correctly without either, which is why each has a test:
- **`APP_OWNED_SETTINGS_CLASSES` has to be re-exported from the package `__init__`.**
The collector reads it off the app package, so a declaration living only in
`app_owned_settings.py` is never found. Every request still works - `GET` reads
rows straight from the database, `PATCH` republishes the snapshot inline - and then
the refresher never republishes, so an override silently reverts to YAML on
restart. Measured: a 25-minute sweep back at 10 after `./om restart sep-backend`.
- **`SCHEDULE` has to be wired to the beat re-seed callback.** The proxy holding a new
interval is not the same as beat running on one; beat reads `celery_periodictask`,
which only changes when that callback fires. Without it the API reports a changed
schedule and the sweep keeps firing on the old one, which is the worst shape a
configuration bug can take. The entry itself is in the previous commit, since it
lives in shared wiring.
Turning the sweep off works because the `SCHEDULE is None` check sits inside the
`periodic_task_schedules` thunk rather than guarding a list at import: `null`
unregisters the task and an interval re-registers it. That was already true and is
now documented, since a literal would have decided once and stayed decided.
Verified end to end against the sandbox: `PATCH {"SCHEDULE__every": 25}` put 25 on the
`celery_intervalschedule` row `sep__run_pom_probe` points at with nothing restarted,
`DELETE` put it back to 10, and both survived a restart in the right direction.
Worth knowing before building the UI: `GET` and `PATCH` do not share a key set. The
LIST projection expands a nested model into leaves, so `GET` returns `SCHEDULE__every`
and `SCHEDULE__period` and never `SCHEDULE`, while `PATCH` accepts either - and only
the parent form can express `null`. A form built from the GET key set cannot discover
the one key it needs to turn the sweep off.
Three existing tests change, all of them closed-set assertions over the wired settings
classes: a new app-owned class is exactly what they exist to notice. The ordering
assertion in `test_settings_proxy` is rewritten to compare positions rather than fixed
negative indices, so the next app to declare a class does not break it again.
Generated output, in its own commit so the feature diffs stay readable. `tests/app/test_openapi_specs_fresh.py` has been failing on this branch for several commits: every POM endpoint added since the app landed is missing from `frontend/packages/api/specs/sep.json`, which is what the frontend codegen reads. Nine paths appear here and none disappear, all of them `pom_discovery`'s - `/hosts`, `/services`, `/runs`, `/schema` from earlier commits, `/config` from this one. `inventory.json` and `tasks.json` move by five lines each: `SettingClassEnum` gained `PomDiscoverySettings`, and every sub-app's settings API publishes that enum. The rest of the churn in `sep.json` is key ordering. The generator re-serialises the whole document, so the diff is far larger than the nine paths that actually changed; compared as parsed JSON, nothing else moved.
`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. Two 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. 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 POM, which has to describe hosts it cannot probe (PMM-15326 §11), but nothing here is POM-specific.
§11's orphan split. "Nothing can run on this machine" was one outcome and is really three, and they need three different people to fix them: registered: false never onboarded - go and set it up reachable: false onboarded, agent down - go and start it driver_healthy: false up but broken - go and read its logs All three arrived here as the same thing: absence from `GET /hosts/`, whose three filter conditions collapse into one bit. A host that failed any of them was simply not in the list, so POM could not describe it and could not say why. Enumeration now reads `GET /hosts/states/` (previous commit) and matches nodes against **every** known executor rather than the usable subset. Dispatch still works from the usable ones, via `usable_executor_hosts`, so nothing is dispatched anywhere new. Two consequences worth stating, because both change 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` now 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. The executor facts are written on **every** sweep whether or not the host was probed, so `upsert_host` takes them separately from `observed`. They are not probe output - SEP knows them without running anything - and the hosts nothing can run on are exactly the ones whose document would otherwise be empty with no explanation. Merged after the attempt rather than before, since a successful probe replaces `observed` wholesale and would drop them from the hosts that did answer. Verified against the sandbox, on rows that previously read identically: pmm-client-node00 registered: true, reachable: false replicaset-single-node00 registered: false, reachable: false The first has a stopped container and a Nomad client that still knows about it; the second has no client at all. One sweep, one query, two different answers.
§11's `repo.*`. Install and upgrade readiness, and the one precondition POM could not previously report: a host may be up, probeable and running a healthy database and still be unable to fetch a package. An HTTPS GET of `PERCONA-PACKAGING-KEY` rather than a ping or a TCP connect, because every failure worth catching lives above that layer. DNS that resolves nowhere useful, a TLS interception appliance with a certificate the host does not trust, a proxy that allows CONNECT but blocks this origin, a captive portal serving its own page - all four pass a ping and fail `yum install`. The key is about 3 KB, stable, and load-bearing: an unreachable packaging key is a real blocker rather than a synthetic reachability check. **The body is checked, not only the status.** A proxy answering 200 with an authentication page is the failure a status-code check reports as success, and it is the likeliest one in exactly the corporate networks this exists to describe. Reading the response is also what a package manager does. **The proxy in effect is reported whether or not one is set.** Without it the result cannot be explained: "connection refused" from a host with no proxy and from a host behind a broken one are the same string and completely different jobs. All four environment spellings are read, since urllib honours all four. No attempt is made to redact a proxy URL that carries credentials - it is reported as configured, and pretending otherwise would be worse than saying so. `REPO_URL` and `REPO_TIMEOUT` are hot settings. An air-gapped estate mirrors the repository somewhere else, and checking the public one there would report every host as broken. The timeout is 8 seconds and deliberately short: a repository slow enough to exceed that is not usable by a package manager either, so waiting longer only delays the same answer. The whole document is kept on the host row rather than flattened to a boolean. "Unreachable" is not actionable on its own; the status code, the latency and the proxy are what say whether to go and fix DNS, a certificate, or an allow-list. Collected once per dispatch, like the OS facts - it describes the host, and asking once per service would multiply the wait by the services on it. It can never fail a sweep: a repository check that raised would cost the caller the OS, process and version facts collected beside it. Verified on `pmm-client-node00`: reachable in 141 ms with no proxy, then `URLError: Name or service not known` after pointing `REPO_URL` at an invalid host, both landing in the row and both readable from `./om discovery estate`.
A sweep has attempted hosts as well as services since a host became probeable for its own sake, but the receipt only ever counted services. So a refresh of a pmm-client host - the case POM most exists to describe - reported "0 of 0 services", which is exactly what a run that did nothing looks like. Three counters, and the pair is the point: `hosts_total` against `hosts_probeable` puts a number on the estate nothing can be dispatched to, which is a fact about onboarding rather than a failed sweep. `hosts_answered` is what actually came back. The same reasoning as the existing resolved-vs-answered split, one level up. Forward migration rather than editing the create: the table is already applied wherever POM has run, and three columns defaulting to zero are cheaper than asking anyone to rebuild a database to read a new counter. Existing rows read as "this run predates host counting" rather than as NULL, which every consumer would otherwise special-case. Also extracts `_finalise`. The counters are a run's whole receipt and every one of them is derived, so a mistake is invisible until someone reads a history and draws the wrong conclusion; as a block inside the sweep there was no way to assert them without dispatching Nomad jobs. Both sets are taken from the same lists the estate was written from rather than counted independently, so the receipt and the rows cannot disagree about what happened.
**The schedule does not go through the endpoint.** Celery beat calls `run_probe` directly, so the conflict guard living in `POST /runs` protected a user from starting an overlapping refresh and did nothing at all about the ten-minute schedule starting one. Found by reading a receipt, not by reading code. Two full sweeps 31 seconds apart on this workspace's sandbox: the second enqueued the same probe job for the same host as the first, the Tasks layer refused the duplicate, and the loser recorded `HTTPConflictException: 409: Identical queue item already running` against four replicaset hosts that were perfectly healthy. That is worse than a confusing receipt. `_apply_attempt` treats the 409 as a genuine failed attempt, so it moves `failing_since` and increments `consecutive_failures` - hosts accrue a failure history for a race between two sweeps rather than for anything wrong with them. It did no lasting damage this time only because the good sweep happened to finish *last* and overwrite the failure; reverse the order and four fine hosts read as failing until the next success. So the check moves into `conflicting_run` in `crud.py`, where both callers reach it, and `run_probe` makes it before doing any work. Two details that are easy to get wrong: - **A run must not refuse itself.** The endpoint creates the row *before* dispatching, so the task finds its own `RUNNING` row; without excluding it, every manually triggered refresh would skip itself. - **The reaper had to move with it**, rather than being left in the endpoint. Two copies of "how old is abandoned" drift, and the one in the path beat uses is the one that matters - a crashed worker wedging the schedule is exactly the case a reaper exists for. **A refused sweep records a run with status `skipped`**, naming what held it, rather than returning silently. A ten-minute schedule that quietly does nothing leaves a gap in the history that reads exactly like it having fired and found nothing. `skipped` is neither a failure nor a success - it did nothing, deliberately - so the UI gives it a neutral chip rather than a red one, or a red row would appear every time the schedule met a manual refresh. The migration widens the status CHECK constraint, and does it with the enum member **names**. `EnumField` persists `ProbeRunStatus.RUNNING` as `"RUNNING"` while its value is `"running"`, so the first attempt built the constraint from the values and was rejected by every row already in the table. `SettingClassEnum`'s docstring records that trap; this walked into it anyway, which is why the migration now says so in a comment. Verified against the sandbox by racing it deliberately: a manual refresh during another answers 409 naming the run, and `run_probe` invoked directly - exactly what beat does - records `SKIPPED` instead of dispatching. No 409s in any subsequent run, and the replicaset hosts hold zero failures. Five tests cover what went unnoticed: a full sweep blocks another full sweep, a run does not refuse itself, disjoint scopes do not block each other, an abandoned run is reaped rather than honoured, and the message names the hosts that are held.
Unfolding a refresh on the Discovery page showed a table of services, so a machine carrying a PMM client and no database appeared nowhere in it - however many times the refresh had probed it. The counters said three hosts answered; the receipt could name one. That host is the case POM most exists to describe: it is where a database can be installed, and it has no service to be listed through. A sweep attempts **hosts**. It has since a host became probeable for its own sake, and the receipt was the last part still shaped as though services were the unit of work. `nodes` is now one entry per host - node id, name, executor host, how it was matched, whether the host answered, how long it took, its error - with the services on it nested underneath, carrying only what is theirs. Two things fall out of that shape rather than being decided separately: - **A host's duration is reported once.** It used to be copied onto every service the host served, which read as several measurements of several things when it was one measurement of one dispatch. - **"Answered" stops being ambiguous.** Whether the *host* answered and whether its *services* did are different questions, and a host with no database answers perfectly well while having no services at all. Flattened together there was no way to say that. `services: []` is a meaningful answer and renders as "none" rather than as an empty cell, for the same reason `pom.host` keeps rows for hosts with no service. Still outcomes and never observations: what the probe *found* belongs to the estate, which is upserted and stays current. A receipt carrying the attributes as well would be a second copy going stale on the next refresh. The sweep tests move with it, and one of them changes meaning: it used to assert the duration was repeated across a host's services, which is precisely the behaviour being removed. It now asserts the opposite, and a new test covers the case that could not be expressed before - a probed host with no database appearing in the receipt at all. Regenerated with `buf generate --path pom/v1/pom.proto` rather than `make gen`, which rewrites 56 unrelated API files from a different protoc-gen-go.
…ally has
`pmm-server` had failed its probe 183 consecutive times with
`SyntaxError: f-string: expecting '}'`, while every database host answered fine.
The Tasks layer runs each payload through `python-minifier` before dispatch, and the
minifier normalises inner string quotes to double. So
f"mongodb://{userinfo}{target['host']}:{target['port']}/?{'&'.join(options)}"
is dispatched as `f"...{"&".join(E)}"` - a double quote nested inside a double-quoted
f-string. That is PEP 701, and it parses only on **Python 3.12 and later**.
The hosts do not agree on Python. This workspace's database containers carry 3.12.3
and `pmm-server` carries 3.9.25, so the identical payload was valid on nine hosts and
a syntax error on the tenth. The source line was legal on every Python there has ever
been; the bug existed only after minification, and only on some machines. It is
invisible in review, invisible in a local run, and invisible on most of the estate.
So every value is bound to a name before the f-string. That is a rule with a reason
rather than a style preference: the payload runs on whatever Python a monitored host
happens to have, which is not ours to choose, and keeping expressions out of
f-strings is what stops the minifier being *able* to emit the construct.
The regression test minifies the payload and asserts no f-string carries its own quote
character inside an expression. Worth knowing how it was validated, because the
obvious check is wrong: reintroducing `target['host']` does **not** reproduce the bug -
the minifier hoists subscript literals into constants. The trigger is a method call on
a literal, `'&'.join(...)`, and the test was confirmed to fail on that and pass once
reverted. Checking only the subscript form would have suggested a broken test.
Verified on the host that was failing: a scoped refresh of `pmm-server` now records a
success and clears its error.
Rebasing onto the current `pmm` changed what `make regen-specs` emits, in two unrelated ways. The specs and the POM OpenAPI snapshot pick up the new base. `MessagesSettings` is gone from `SettingClassEnum` and `HealthReportSettings` is in it, and SEP-1708's `options` field and its `SettingOption` schema now appear on every settings response, POM's included. `sep.json` also loses `ServiceResponse` and `PaginatedResponse_ServiceResponse_`: the endpoints returning them were retired earlier in this branch, but the base had renamed the paginated wrapper in the same hunk, so the conflict was resolved toward the base rather than guessing. Regenerating is what actually drops them. The generated TypeScript client is regenerated for the first time on this branch. No commit here had ever touched `frontend/packages/api/src/generated/`, so the committed client lagged this branch's own spec changes: `tasks.ts` was missing `/hosts/states/`, and neither `tasks.ts` nor `inventory.ts` knew `PomDiscoverySettings`. The specs were committed without regenerating the client that is derived from them. Regenerated with the three steps `make regen-specs` runs. `tests/app/test_openapi_specs_fresh.py` passes and a second codegen run is a no-op, so the artifacts are at a fixed point.
Rebasing onto the current `pmm` moved three things POM depends on, and git flagged none of them: the files that broke are new in this branch, so there was no conflict to resolve. `app.sep.deps.get_api_authenticated_user` and `validate_csrf` are gone, removed with the legacy Jinja SSR layer in SEP-1687. Four POM test modules imported them and failed at collection, taking 49 tests down with them. Three of the four already overrode `get_current_user` alongside the removed guard, so dropping the dead override there is a no-op; `test_contract.py` overrode only the removed one, so it moves to `get_current_user` (what `IsApiAuthenticated` now wraps) or the guard it mounts is never satisfied and every case 401s. CSRF on mutations is `require_bearer_for_unsafe_methods`, which these fixtures already override. `REDUCED_ACTIVATION` mirrors the baked side-car profile, and that profile activates `pom_discovery` as of this branch, so the mirror gains it too. `tests/sidecar/test_embedded_settings.py` is what compares the two. Also pins POM's routing under `SEP.ROOT_PATH`. SEP-1794 landed in the base while this branch was out, and it is what retires the nginx prefix-stripping this branch used to carry: `sidecar/settings.yaml` sets `ROOT_PATH: /sep` and activates `pom_discovery` together, and pmm-managed pulls `GET /services` through that prefix. POM builds no self-referential URLs, so the prefix is a routing concern only; this asserts the routes resolve under it instead of leaving it to inference. The side-car comment naming `/api/apps/pom_discovery/facts` is corrected to `/services`. `GET /facts` was retired earlier in this branch, so the comment named an endpoint that no longer exists.
SEP-1728 added `.github/labeler.yml` after this branch was cut. Its app section is generated from the apps on disk by `scripts/sync_labeler_apps.py`, and `tests/scripts/test_sync_labeler_apps.py` asserts the committed file matches, so a new app has to appear there or CI fails. Only the `app:pom_discovery` entry is added, and it is written by hand rather than by running the sync script: the script walks `app/sep/apps/*` on disk, so it also picks up any stale directory a local checkout happens to carry, and running it would commit labels for apps that are not in the repository.
plebioda
force-pushed
the
PMM-15326-pom-inventory
branch
from
August 20, 2026 09:21
ff40b01 to
d364b39
Compare
The product is OpenManager, not PSMDB OpenManager, so `pom` becomes `om` throughout. The app's own half of the name follows what it actually serves: an estate of hosts and services that PMM presents as Inventory, not a run of discovery. `POM Discovery` in the app registry is `OpenManager Inventory`. So: `app/sep/apps/pom_discovery/` -> `app/sep/apps/om_inventory/`, the shared `app/sep/pom/` -> `app/sep/om/`, `SEP.POM` -> `SEP.OM` and `SEP.POM_DISCOVERY` -> `SEP.OM_INVENTORY`, `PomDiscoverySettings` -> `OmInventorySettings`, `PomHost`/`PomService` -> `OmHost`/`OmService`, the Celery task `sep__run_pom_probe` -> `sep__run_om_probe`, and the `settingclass` enum value `pom_discovery` -> `om_inventory` in all three alembic branches. The database moves with it: schema `pom` -> `om`, so `om.host`, `om.service` and - discovery being inventory here too - `om.inventory_run`. The symbolic token the models declare is `om_schema`, and the indexes are `ix_om_*`. The three migrations that create and alter those tables are edited in place rather than joined by a rename migration. None of them has shipped: the branch is unmerged, and a rename migration would leave the old names in the tree permanently for the sake of databases that only exist on our own laptops. The cost is that an existing dev database still holds the `pom` schema and will not migrate across - drop it and re-run (`./om reset data`, then bootstrap). Derived artifacts are regenerated, not edited: the `GET /schema` and OpenAPI snapshot goldens, and `frontend/packages/api/specs/*.json` via `scripts/dump_openapi.py`. The generated TS client under `frontend/packages/api/src/generated/` is renamed by hand, because `pnpm codegen` wants a newer Node than this checkout has; it is verified against the regenerated spec instead - same 169 operation ids, same path and schema ordering, so it is what codegen would have produced. `pom_worker` and `pom_api` were cited in a docstring as where some of this code came from. Those apps were deleted, and the commits that held them will be squashed away, so the citation goes rather than being renamed to something no git history contains. Verified: 9512 tests pass, ruff clean, `alembic check` fails exactly as it did before the rename (the symbolic schema cannot be compared on SQLite).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tested
Checklist
make test)make run-pre-commit)make makemigrations)changelog.d/if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)