PMM-15326: Add the OpenManager Inventory app - #1395
Draft
plebioda wants to merge 10 commits into
Draft
Conversation
…entation
SEP's settings router is admin-gated, and not every caller of an app's configuration
is an admin. The case in hand is a deployment-level shared secret with no person
behind it - PMM's `--sep-token` resolves to the synthetic `sep-service` user, built
`is_admin=False` deliberately - which has to be able to read and change the
configuration of the app it drives, and nothing else.
The app could serve its own `/config`, and an app *should* be able to: a schedule
change scoped to one app has no business requiring SEP-wide administrative access.
What an app must not do is reimplement what a settings PATCH means. "Validate the
whole batch before writing any of it, persist atomically, republish the proxy
snapshot inline, fire the rebind callbacks for the keys that changed" is four
invariants deep, and a second copy of it drifts into a second set of validation
rules - which is how one endpoint starts accepting what the other rejects.
So the two halves of `PATCH /{setting_class}` and `DELETE /{setting_class}/{key}`
that are *not* routing come out as `apply_class_overrides` and
`clear_class_override`, and the router's own handlers call them. Pure extraction:
same phases, same errors, same order, no behaviour change, and the existing settings
tests cover it unchanged.
`clear_class_override` comes out with it rather than being left behind, because
without a way to remove an override an operator who once set a value can only ever
set another one: "no override" stops being a reachable state and the value the
deployment shipped becomes unrecoverable through the API.
Two things ride along, both about the same failure mode - a setting that changes and
nothing acts on it:
- `build_sep_override_callbacks` is lifted out of `sep_overrides_lifespan` so the
registry is a value a test can assert on. Every entry in it is a change with an
effect *outside* the settings snapshot: a client rebound, a logging config
re-applied, a beat row rewritten. A missing entry fails silently - the API reports
the new value and nothing happens until the process restarts - and a literal buried
inside a context manager is not something a test can look at.
- The reseed callback now re-applies app gating. `init_periodic_tasks_db` preserves
`enabled` only on its *update* path, and a schedule an app may set to `None` -
which is how an app-owned periodic task is turned off - contributes no task while
it is null, so the orphan cleanup deletes its beat row. Setting it again takes the
*create* path, which builds a fresh row at the model's default `enabled`. A
disabled app would start running on the next beat tick. `init_sep_db` already runs
`init_periodic_tasks_db` and `sync_app_periodic_task_gating` as a pair at startup;
this makes the hot path do the same.
One test is retargeted rather than extended: the settings-proxy list assertion
indexed the last three classes by position, and app-owned groups are appended, so
every app that declares a settings class shifted a fixed index. It now asserts the
ordering it actually cares about - core, then remote, then app-owned - which is the
contract, and which no future app can break by existing.
Nothing here has a caller yet in this branch. The first is the OpenManager Inventory
app, which serves its own `/config` off these two functions, but they are the app
framework's, not that app's: any app whose configuration a non-admin principal must
reach needs exactly this and should not write it again.
OpenManager needs tables of its own, and the first question is where they go. A separate database was priced and rejected: another connection pool, another set of credentials to provision, another backup story, and no join to SEP's inventory - for tables whose whole purpose is to be joined against it. Table prefixes were the other option, and they make the isolation a naming convention that the next person breaks. So a **schema** inside the existing SEP database, and the mechanism is what this commit adds. The tables declare a *symbolic* schema, `om_schema`, which is never a real name anywhere; the engine translates it, per bind: PostgreSQL -> `SEP.OM.SCHEMA`, `om` by default SQLite -> the default schema, since SQLite has no schemas MySQL -> the default schema, since MySQL's "schema" is a database A literal name would make the tables uncreatable on SQLite, which is what `settings.yaml` ships, and the real-PostgreSQL test lane routes every table into a per-xdist-worker schema that a hard-coded `om` would escape and then collide across workers. The translation goes on the **engine**, not the call sites, because both paths that reach these tables come from there: the routes through `SessionDep` and the Celery task through `get_async_session_maker`. One option covers the HTTP path and the background path together, which is the whole reason a schema was affordable where a second database was not. Alembic needs the same treatment and does not inherit it: `env.py` puts the map on the connection it migrates with, or a migration naming `om_schema` creates a literal `om_schema` schema on PostgreSQL and fails outright on SQLite. Offline mode has no connection to carry a map, so a script generated with `--sql` names the token literally and has to be edited before it is run; that is recorded in the docstring rather than worked around, because `make migrate` uses online mode. The suite gets the same problem from the other side. It builds every service's metadata in one in-memory SQLite database, which is the one place a table called `service` in the app's schema and SEP inventory's own `service` can collide. Rather than translate the token in some thirty engine constructions - each one that forgot would fail with "unknown database om_schema" - a connect-time listener satisfies it: SQLite has no schemas but it has `ATTACH`, and an attached in-memory database *is* a schema as far as SQL is concerned. Registered on the `Engine` class so it covers engines that do not exist yet, and it fires before any statement, so `create_all` already sees it. No table declares the token yet - the next commit is the first.
Two tables for what the estate *is* and one for what a sweep *did*, which is the split the rest of the app is built on: om.host one row per node PMM knows, database or no database om.service one row per MongoDB service PMM has registered om.inventory_run one row per sweep, with its counters and its receipt A host row is written **whether or not any MongoDB was found on it**. That is what makes "which machines have no database" a query rather than an absence, and it is the only way a machine that has never run one appears at all - which is the case an install decision is about. Its services are separate rows because a host may carry several, and because a host can be reachable while a mongod on it is not. What was collected lives in a JSONB `observed` document rather than in columns. The attribute set will change more than once before this settles, and every change would otherwise be a migration on a table two products read. The freshness columns are the part worth reading closely, because they encode rules that are cheap here and expensive to discover later: - `failing_since` is set only when unset. Overwriting it on every failure turns "since" into "most recent failure", so the duration is always about one schedule interval and the column stops being worth reading. - A failure never touches `observed`. What a host was running when it was last reachable is exactly what is wanted while it is not. - `last_attempt_at` moves on every attempt, `last_success_at` only on success. The gap between them is the answer to "how stale is this". - The caller decides what counts as an attempt, and `upsert_*` takes `attempted` separately for it. Seeing an entity and probing it are different things: a host with no executor is seen every sweep and probed by none of them, so its identity refreshes while its failure history stays put - which is what keeps "unreachable for three days" from silently becoming "unreachable since the last sweep". `delete_host` removes the host's services itself rather than trusting `ON DELETE CASCADE`. SQLite enforces no foreign key without `PRAGMA foreign_keys=ON`, SEP sets it nowhere, and SQLite is the shipped default - so on a default deployment the cascade is decoration and forgetting a host would leave service rows pointing at nothing. The constraint stays as the backstop it is on PostgreSQL; what changes is that the promise no longer depends on which database someone configured. The upserts are written attribute by attribute on purpose. Nothing in these tables is user-writable *yet*; the moment one field is - an assigned name, a label, a suppression flag - a blanket "update every column" upsert wipes it on the next sweep, and the test that catches that has to exist before the field does. One migration, hand-written, and it has to stay hand-written: Alembic applies no `schema_translate_map` during autogenerate and `include_schemas` is off, so `--autogenerate` proposes creating a literal `om_schema` and re-creating these tables on every run. An autogenerated OM diff is a bug. Its tests arrive with the sweep that drives them: the lifecycle above is asserted through `_persist_estate`, because a rule about what one sweep may touch is only testable against a sweep. The package carries no `__init__.py` yet. The framework's convention is that it re-exports the `BaseApp` object, which pulls in the router, the schema and the settings class - so it lands with the registration, and until then this is an implicit namespace package whose modules import on their own.
`OmInventorySettings` under `SEP.OM_INVENTORY`: the schedule, the probe's timeouts and concurrency, the run retention, the repository URL the reachability check fetches, and the path to the node-side credentials file. Every knob an operator may want to change while the app is running is a `hot_field`, so it takes effect through the override machinery rather than a restart - `SCHEDULE` above all, since a ten-minute sweep of an estate that turns out to be larger than expected is the first thing anyone will want to slow down. **`CREDENTIALS_PATH` is deliberately not hot.** It names a file the probe payload reads on every database host and hands to a driver as a URI, so making it settable at runtime would widen "configure this app" into "read a chosen file across the estate". It stays a deployment-level setting, changed where the deployment is described. `SCHEDULE` is nullable, and that is the app's off switch: with no schedule the estate only changes when someone asks for a refresh. The periodic-task contribution reads it through a thunk rather than a literal so that turning it back on re-registers the task on the next registry rebuild - which is also why the reseed callback in the previous commit's PR has to re-apply app gating. `app_owned_settings.py` is what the framework collects to expose this class on SEP's admin settings API *and* to let the app serve its own `/config` off the helpers extracted in #1393. It is a list rather than a bare entry because the registry collects only activated apps: a SEP deployment without OM has no such section at all, rather than an empty one to be confused by. The `SettingClassEnum` member comes with three Alembic revisions, one per consumer track (sep, tasks, inventory), because `settingoverride.setting_class` uses `native_enum=False` - the allowed values live in a CHECK constraint rather than a PostgreSQL type, so each track's constraint has to be widened separately. They persist member *names* (`OM_INVENTORY_SETTINGS`), not values (`OmInventorySettings`); a constraint built from the values rejects every row the model writes, which is the trap the enum's own docstring records.
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.
This is the half of discovery PMM cannot do for itself. Everything it can derive -
identity, running version, replica-set state, reachability, load - it reads from its own
inventory and VictoriaMetrics. What is left needs a process on the database host: the
command line a mongod was started with, the config file it read, and above all the
*installed* binary version as against the *running* server the metrics report. Their
divergence is the upgraded-but-not-restarted case, and no metric anywhere carries it.
The payload rides the pre-seeded system `run-python` task rather than a task of its own -
`POST /execute/run-python` with `meta.target` naming the executor host, `meta.config`
carrying the payload's JSON, and `payload` a `file://` URI - which is how the topology
app dispatches its collector shards. Results come back over **stdout**, streamed from
the task-log chunk store: that channel has no total size cap, unlike the 16 KB
`.sep-run-result.json` file, which is silently discarded above it.
One dispatch per executor host, carrying every service that host serves. The payload
collects host-level facts once and reuses them across its targets, so batching by host
is both fewer Nomad jobs and less duplicated work - and a host with **no** services is
dispatched to as well, with an empty target list, because a machine carrying a PMM client
and no database is exactly the one an install decision is about and it has no service to
be reached through.
Four decisions in here that were each paid for once:
- **Every target is attempted even when an earlier one fails**, and the payload exits 0
regardless. A per-target error becomes an `error` field on that target's record; a
non-zero exit is reserved for "the payload could not start at all", which the
orchestrator reports as a dispatch failure rather than a probe failure.
- **An abandoned dispatch is released.** Giving up on a probe ends *this sweep's* wait and
does nothing to the queue item, which stays `RUNNING` with nothing left to advance it -
and the Tasks layer refuses a queue item identical to one already in flight. Since every
sweep dispatches the same `run-python` to the same host with the same config, one
abandoned run makes that host answer `409` forever while the sweep reports itself merely
partial. Measured on the sandbox: seven such rows blocked their hosts for over an hour.
The stop is best-effort - it can legitimately fail when the allocation is already gone,
which is the case most likely to have caused the abandonment - and the reason comes back
so the sweep can say the item was left in flight, because that is the one thing here
that needs a human.
- **Process facts are matched to a target by port.** A host may run several mongods, or a
mongos beside a mongod, and the first `ps` line is not everyone's. Sharing it gave every
service on such a host the same argv, config path, uptime and installed version while
its database facts came from its own port - so the one comparison this exists to make
was wrong for every service but one. A single server process is attributed without a
port match, because a mongod started with no explicit port names it nowhere the payload
can read and that is the commonest host in any estate.
- **Repository reachability is an HTTPS GET whose body is checked**, not a ping. The
failures worth catching all live above that layer: DNS that resolves nowhere useful, a
TLS interception appliance with an untrusted certificate, a proxy that allows CONNECT
but blocks this origin, a transparent cache serving 403. Every one of those passes a
ping and fails `yum install`. The packaging key's PGP header is the marker, so a captive
portal answering 200 with its own HTML is reported unreachable rather than healthy. The
proxy in effect is reported beside the result - without it "connection refused" from a
host with no proxy and from a host behind a broken one are the same string and different
problems - with its credentials redacted, since the value is stored and served.
Mongods PMM has no service for are reported as host observations rather than invented
service rows: there is no service id to key one on. An arbiter is the ordinary case - it
holds no data, therefore no user documents, therefore SCRAM cannot authenticate and
`pmm-admin add mongodb` fails for it - so any estate with arbiters and authentication has
them.
The payload's one third-party import is `pymongo`, declared as the task's pip
requirements and imported lazily so a run with `probe_database` false works on a host
without it. Every value is bound to a name before any f-string, and a test asserts that:
the Tasks layer runs this file through `python-minifier`, which normalises inner string
quotes to double, turning `{target['host']}` into PEP 701 syntax that parses only on
Python 3.12+. The payload runs on whatever Python a monitored host happens to have, and
this workspace's own pmm-server carries 3.9, where every probe failed with
`SyntaxError: f-string: expecting '}'` while the database hosts on 3.12 were fine.
The lifecycle: create the run row, map, dispatch, collect, write the estate, close the run. Only this function ever writes a terminal status, and the row exists before the work starts so a caller can be answered with an id immediately. The counters are the 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. They 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 receipt is host-oriented**, one entry per host with its services nested. A flat list of services - which this was - cannot show a machine carrying a PMM client and no database, however many times it is probed, and that machine is the case OM most exists to describe: a reader looking for `pmm-client-node00` found the sweep counted it and could not see it. One dispatch covers every service on a host, so the host owns the timing and the failure; its services carry only what is theirs. That also stops the duration being repeated identically across a host's services, which read as several measurements when it was one. Facts are keyed by **PMM's** service UUID, not SEP's inventory id. That translation is why `build_facts` exists rather than the caller storing records: the consumer joins facts against its own services table, where SEP's integer key means nothing. A service with no `external_id` contributes none - they would be unjoinable, and storing unjoinable facts only makes a run look more productive than it was. Host attributes and service attributes are lifted into separate documents from the payload's separate records. That is what stops `repo.reachable` being stored three times on a host running three mongods, and those three copies being free to disagree once a partial sweep updates some and not others. `installed_version` is on both: for a service it is the upgrade check, and for a host with no service at all it is the whole install decision. A scoped refresh narrows *everything downstream of enumeration*, never the enumeration itself - hosts are still listed, because that is how a scoped id is recognised as a host at all - and nothing outside the scope is written. An entity this run did not attempt keeps every timestamp it had, which is what stops refreshing one host from making the rest of the estate look failed. Measured on a scoped refresh of `standalone-node00`. Three judgements the terminal status has to get right: - A host with an executor and **no database** that answered is a `SUCCESS`. Judging on services alone reported `FAILED` for a run that did exactly what it was asked. - **Orphans do not count against a run.** A service whose node runs no healthy executor is a fact about the estate, not a failure of the sweep. - **Nothing answering is `FAILED`, however much was attempted.** Reserving `FAILED` for having attempted nothing left the one status that says "look at OM itself" unreachable in the case that most needs it, and reported a total outage as "some of the estate is fine" to anything automating against it. Single-flight is enforced here as well as at the endpoint, because **the schedule does not go through the endpoint**: beat calls this task directly, so with the check in the handler alone a scheduled sweep would start on top of one already dispatching, both would enqueue the same job for the same host, the Tasks layer would refuse the duplicate, and the loser would record a 409 against a host that is perfectly healthy - moving its failure timestamps for a race rather than a fault. Measured on this workspace's sandbox: two full sweeps 31 seconds apart, four healthy hosts marked unanswered. Conflict is judged **per host**: two runs collide only when they would touch the same host, a full refresh collides with everything, and a run older than `STALE_RUN_AFTER` is reaped rather than honoured, because a crashed worker leaves a row nothing else advances and it would otherwise wedge the app permanently. Where a run is asking on its own behalf it yields only to the run that claimed the hosts first, by an order both sides compute from the same rows - otherwise two racers each see the other and *both* skip, which is the one outcome worse than either of them proceeding. The estate is written before the run reaches a terminal status, so a reader that sees a finished run always finds the rows it produced. Retention prunes finished runs only: retention is by `started_at`, an in-flight sweep is the oldest row while it runs, and pruning it would leave the worker finalising a row that no longer exists. A refused connection to SEP's *own* API is waited out rather than recorded as a sweep failure. Beat sets "last run" to now on startup and fires on its first tick, seconds before uvicorn has bound - measured here at six seconds - and the run that lost that race wrote a terminal `FAILED` carrying `Cannot connect to host localhost:8000` on an estate where nothing was wrong. Both consumers lead with the newest run, so a healthy estate presented as a failed sweep for the next ten minutes. `celery.py` is the entry point beat and the trigger endpoint share; `@owned_by` tags it so the app-drain reconciler counts it toward this app rather than treating it as core.
The API the consumer polls, and the `BaseApp` registration that makes the app exist.
`GET /hosts` and `GET /services` are the estate as OM last saw it, with the freshness
columns beside every row, so a caller can tell "this is current" from "this is what it
looked like before the host stopped answering" without a second request. Filters for the
questions asked in practice: hosts with or without an executor, services by host, and
either kind by whether it is currently failing. `DELETE` on a row exists because an
estate the operator cannot correct is one they stop trusting - a decommissioned host that
lingers as permanently failing teaches everyone to ignore the column.
`POST /runs` is the on-demand refresh, and its point is latency. A full sweep is bounded
by its slowest host - a minute and a half in this sandbox - and "I just did something to
this host, is it healthy now" should not cost that. The scope is node ids, which is what
PMM already holds, so its trigger passes them through untranslated. An id OM does not
hold is answered 404 by name rather than by running a refresh that would quietly do
nothing: SEP's inventory copy can lag PMM's, so that is a real case rather than a typo
guard. A host already being refreshed is a 409 naming the run that holds it.
`GET /runs` and `GET /runs/{id}` are the history. The detail shape is kept apart from the
list shape on purpose: a sweep's facts run to a few hundred records, so returning them
for every row of a 25-run history would make the list an order of magnitude larger to
serve a page that shows one run at a time.
**The app serves its own `/config`** off the helpers extracted in #1393, rather than
pointing the caller at `/api/sep/admin/settings`. That router is admin-gated and PMM's
principal is not an admin: the `--sep-token` bearer resolves to the synthetic
`sep-service` user, built `is_admin=False` deliberately, since it is a deployment-level
shared secret with no person behind it. An app-owned endpoint keeps a schedule change
scoped to this app instead of requiring SEP-wide administrative access. Every field is
listed, not only the overridden ones, each carrying whether an override is in effect - so
"why is it sweeping every ten minutes" is answerable without also reading the deployment's
YAML - and `DELETE /config/{key}` puts a field back to what the deployment configured,
because without it "no override" stops being a reachable state.
The trust model this implies is worth an explicit decision rather than an inferred one:
any `IsApiAuthenticated` principal, not only the service token, can change `SCHEDULE`,
`MAX_CONCURRENT_PROBES` or `PROBE_DATABASE`. Deliberate - PMM has to be able to configure
the app it drives - but it means an interactive user can disable sweeps or raise Nomad
load, and a narrower capability may be wanted before this ships. `CREDENTIALS_PATH` is
excluded from `hot_field` for exactly this reason and stays deployment-level.
`sidebar=False` and no `react_route`: there is nothing to navigate to. The consumer is
PMM's OM service polling these routes, and the run history is for diagnosis through the
API. Registering as an app is still what earns the Celery module inclusion, the app-drain
ownership tag, the `SCHEDULE` rebind callback, and the switch on the Apps page.
Activated in the default profile, because an app that ships inactive is dead weight and
the framework's own switch is what turns it off. The side-car profile is the next commit.
`__init__.py` arrives here rather than with the tables, because the framework's convention
is that it re-exports the `BaseApp` object - which pulls in the router, the schema and the
settings class, none of which existed until now.
Deployment, in its own commit, because a reviewer reading a feature commit should not have to decide whether an activation entry is part of the feature. **Which topology this serves:** the PMM-embedded side-car, `sidecar/settings.yaml`, the baked profile a `pmm-server` image runs SEP from. That profile activates a deliberately reduced app set, and OM belongs in it because OM is what the profile exists for: the app sweeps the MongoDB estate over Nomad for the facts no metric carries and serves them at `/api/apps/om_inventory/services`, which `pmm-managed` pulls. The topology and health half lives in `pmm-managed`, so a PMM that ships this app also needs a build carrying `managed/services/om`. `ALLOWED_HOSTS` gains `host.docker.internal` in the **default** profile, and that one is not cosmetic: `pmm-managed` reaches SEP from inside the `pmm-server` container, where the host is only addressable by that name, and without it `TrustedHostMiddleware` answers 400 to every cross-container request whatever the credentials it carries. It is in the default profile rather than the side-car's because the local dev loop hits the same wall - both halves of the pair run in containers there too. `REDUCED_ACTIVATION` in the SEP test conftest moves with it, not after it: `tests/sidecar/test_embedded_settings.py` asserts the constant mirrors the baked profile exactly, and that constant is what the SEP subtree's tests stand the profile up from. A divergence makes those tests assert against a deployment that does not exist - which is how an activation-gated artifact-download failure once stayed invisible to the whole suite. The three settings-list assertions ride here for the same reason rather than with the app: each one enumerates the classes an *activated* app contributes, and `collect_app_owned_settings_classes(REDUCED_ACTIVATION)` is what fills them. They are true the moment the profile lists the app and not before. Nothing here can be removed later on a schedule: it is the activation itself, so it stays until the app does.
Nothing hand-written. Its own commit so every commit before it is code a reviewer has
to read and this one is not.
Two CI-enforced reasons it cannot be deferred to a follow-up: `test_openapi_specs_fresh`
runs `scripts/dump_openapi.py --check` and fails the moment a route exists that the
committed spec does not carry, and the snapshot tests compare the app's `GET /schema`
and its OpenAPI fragment against per-app goldens.
make regen-specs, minus the form-DSL goldens no form here touches:
SEP_UPDATE_SNAPSHOTS=1 pytest test_schema_snapshot test_openapi_snapshot
scripts/dump_openapi.py
pnpm --filter @sep/api codegen && oxfmt --write src/generated
`specs/tasks.json` and `generated/tasks.ts` move by three lines each: the settings-class
enum gains `OmInventorySettings`, which appears in every service's spec because every
service serves the settings API. The bulk is `sep.json` and `generated/sep.ts`, which
gain the app's routes.
Codegen needs Node >= 22.22; the default `node` here is 20.19 and nvm carries 22.23.2,
which is enough. The same PATH makes `make run-pre-commit` run its `oxfmt` and `oxlint`
hooks rather than aborting them.
plebioda
commented
Aug 21, 2026
|
|
||
|
|
||
| @router.patch("/config", response_model=list[SettingResponse]) | ||
| async def patch_config( |
Collaborator
Author
There was a problem hiding this comment.
For the SEP team: is a per-app, non-admin /config endpoint acceptable at all? Implemented one way, and I would like to be talked out of it if that is the wrong way. I am new to this part of SEP, so please correct whatever I have got wrong below rather than assuming I checked it.
Why not just register the settings class and use the admin router. That was the intent - the DB-backed overrides are exactly the right facility, with alerts as the worked example. But /api/sep/admin/settings is gated on IsApiAdmin (app/sep/api/routes/settings.py:86), and the caller that needs to read and write this app's configuration is pmm-managed, not a browser. It arrives with the bearer from its --sep-token flag, which is SEP_INTERNAL_TOKEN; that short-circuits authentication at app/api/deps.py:95 and returns the synthetic sep-service principal built at app/api/deps.py:49 with no is_admin argument, so it takes the default False (app/core/auth/models.py:201). IsApiAdmin raises on exactly that. So PMM gets a 403 from the settings router - not a sandbox misconfiguration, it follows from how the principal is constructed.
The two ways out, and why I picked the second.
-
Make the service principal an admin. One line, and every app's settings become reachable by PMM at once. I did not do it: that token is a deployment-level shared secret with no person behind it, and in the default configuration it is derived from
SECRET_KEYrather than separately provisioned. Making it an admin means a leakedSECRET_KEYgrants administrative access to all of SEP rather than to the settings of the one app that needed it. That is a much larger change than the problem justifies, and not one to make quietly. -
Let the app serve its own
/config- what this PR does. Gated by the/api/appsmount (IsApiAuthenticated, plus a bearer on unsafe methods) rather than by admin. The settings router keeps its admin gate and itsOmInventorySettingsgroup; both surfaces read and write the same override rows through the same manager, so they cannot disagree.
The consequence to weigh: any logged-in SEP user, not only the service token, can then change SCHEDULE, MAX_CONCURRENT_PROBES or PROBE_DATABASE - so disable sweeps, or raise Nomad load. The tests assert regular_user on purpose, so this is the behaviour as designed rather than an accident.
What I would like confirmed, or pushed back on:
- Is a per-app, non-admin config endpoint acceptable at all? My argument that it changes little: the same principal can already
POST /runsto trigger sweeps andDELETE /hosts/{node_id}to drop inventory rows on this same router. Changing a sweep interval seems no more privileged than either. But "seems" is doing a lot of work in that sentence, and you know the threat model better than I do. - Should the real gate be on PMM's side? Every OpenManager page reaches SEP through pmm-managed rather than holding a SEP bearer in the browser, so the user-facing authorization for a schedule change would be a Grafana role in PMM's
auth_server.go- which is where a PMM operator would look for it. Is "PMM gates the human, SEP trusts the service token" the right division, or does SEP want its own say? - Is
sep-serviceeven the right identity for PMM→SEP calls? The bigger question, and the reason I am asking rather than merging. Today every such call is one shared secret with one all-or-nothing identity. If the answer to (1) is "config changes need an admin", the answer is probably a per-deployment credential with its own permissions rather than promoting the shared one - and that is a SEP-wide decision well outside this ticket. - Should this be framework rather than per-app wiring? The two helpers this endpoint stands on (
apply_class_overrides,clear_class_override, extracted in PMM-15326: Let an app own its settings without a second config implementation #1393) are most of what a second app would need.
If the answer is "no", what changes: this commit's /config handlers and their tests. #1393 is useful either way - it removes a duplicate implementation of what a settings PATCH means, whoever is allowed to call it. Arranged that way on purpose, so reversing this is one commit rather than a rewrite.
One detail worth keeping whatever is decided: CREDENTIALS_PATH is deliberately the one field that is not runtime-settable. It names a file the probe payload reads on every database host and hands to a driver as a URI, so an overridable one would turn "configure this app" into "read a chosen file across the estate" - which really is a different permission from the one argued for above.
8 tasks
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
The OpenManager Inventory app: it sweeps the MongoDB estate over Nomad for the facts no metric carries, stores them, and serves them at
/api/apps/om_inventory/*forpmm-managedto pull.Everything PMM can derive for itself - identity, running version, replica-set state, reachability, load - it reads from its own inventory and VictoriaMetrics. What is left needs a process on the database host: the command line a mongod was started with, the config file it read, whether the repository is reachable, what else is running that PMM has no service for, and above all the installed binary version as against the running server the metrics report. Their divergence is the upgraded-but-not-restarted case, and no metric anywhere carries it.
Read it commit by commit
Nine commits in dependency order, each one a self-contained argument. The last is machine-generated and can be skipped entirely.
schema_translate_mapon the SEP engine, and a connect-timeATTACHlistener in the test conftestom.hostandom.serviceattemptedis separate from seenCREDENTIALS_PATHis not/configtrust model~10k lines to read, ~10.5k to skip. The generated half cannot be deferred to a follow-up PR:
test_openapi_specs_freshrunsdump_openapi.py --check, and the snapshot tests compare per-app goldens.The design decisions worth arguing about
om_schema) and translated per bind, because a literal name is uncreatable on the SQLite thatsettings.yamlships.observeddocument. The attribute set will change more than once before this settles, and every change would otherwise be a migration on a table two products read.pmm-admin add mongodbfails for it.Wanted: an explicit decision on the
/configtrust modelThe app serves its own
/configbecause SEP's settings router is admin-gated and PMM's principal is not an admin - the--sep-tokenbearer resolves to the syntheticsep-serviceuser, builtis_admin=Falsedeliberately. The consequence is that anyIsApiAuthenticatedprincipal, not only that token, can changeSCHEDULE,MAX_CONCURRENT_PROBESorPROBE_DATABASE: an interactive user can disable sweeps or raise Nomad load.CREDENTIALS_PATHis excluded fromhot_fieldfor exactly that reason, since it names a file the payload reads on every database host.That is a product call, not an oversight, and it is the open question from this ticket's discussion. If a narrower capability is wanted before this ships, say so and it goes in here.
Three unshipped migrations became one
a3f1c8d24b71→e7c4a1b9d3f2→b4d8e2a1c9f7were three revisions while this branch was written; the host counters and theSKIPPEDstatus are folded into the create revision, which its own docstring already argued for. None has shipped, so there is no data a move migration would preserve. A dev database that ran the old revisions has to be dropped (./om reset data, then bootstrap) - that cost is local and was already the case for this branch.Why this is its own PR
Third of the PRs replacing the single 21k-line draft #1371, which stays open until the replacements are up and gets closed by hand.
get_executor_statesraises on a backend that does not serve/hosts/states/rather than silently degrading./configimportsapply_class_overridesandclear_class_override.PMM-15326-om-inventoryis cut on top of it, so the first commit in this PR's list is PMM-15326: Let an app own its settings without a second config implementation #1393's - review fromPMM-15326: Let an app's tables live in a schema of its ownonward. A rebase after PMM-15326: Let an app own its settings without a second config implementation #1393 merges makes it disappear.tests/sidecar/test_embedded_settings.pyassertsREDUCED_ACTIVATIONmirrors the baked profile exactly, and three settings-list assertions are only true once the profile lists the app - so splitting them would have left one PR carrying assertions that pass only after the other merges. It is still a separate commit, which is what this workspace's rule about deployment changes actually asks for.Base is
PMM-15299-open-manager, the integration branch for the epic.The split is provably lossless
PR-A ∪ PR-B ∪ PR-Chas the same tree as the original branch rebased onto the baseline, except for the two intended differences: the migration collapse, and one test docstring reworded to state the rule rather than the app that first hit it.Tested
CI does not run here -
.github/workflows/ci.ymltriggers onpull_requestwithbranches: [main], so a PR based on this integration branch gets no test job. Everything below was run locally on this branch.venv/bin/python -m pytest tests -q -n 8- the whole suite, app and side-cartests/app/sep/apps/om_inventory/- 184 tests: enumeration across executor states, estate upserts and the freshness rules, the API with its filters, scoped refresh and single-flight, the probe payload (process attribution, repo reachability, unregistered mongods, release-on-timeout), the config API, and the minification guardtests/app/sep/migrations,tests/app/tasks/migrations,tests/app/inventory- the collapsed revision and the three enum-extension revisionstests/sidecar- the profile mirror and the image app-set assertionsmake run-pre-commit- all 22 hooks,oxfmtandoxlintincludedpmm-client-node00,standalone-node00) checked individuallyChecklist
make test)make run-pre-commit)make makemigrations) - hand-written rather than generated, deliberately: Alembic applies noschema_translate_mapduring autogenerate andinclude_schemasis off, so--autogenerateproposes creating a literalom_schemaand re-creating these tables on every run. An autogenerated OM diff is a bug, and the revision says so.sidebar=False, noreact_route) because the consumer ispmm-managedsettings.yamlcarriesSEP.OM.SCHEMAandSEP.OM_INVENTORYwith comments;GET /configlists every field with its originchangelog.d/- cannot be, and it needs a decision.scripts/changelog.py'sFRAGMENT_REaccepts onlySEP-<n>.<section>.mdand raisesFragmentErroron anything else, so no PMM-ticketed fragment can exist and this feature would ship with no SEP release note. Either the SEP half of this work gets aSEP-xxxxfor changelog purposes - one multi-line fragment would then cover the app and PMM-15326: Report the executor fleet, not only the part of it that works #1390's endpoint, since assembly emits one bullet per line - or the note comes from PMM's side only. Flagged the same way on PMM-15326: Report the executor fleet, not only the part of it that works #1390 and PMM-15326: Let an app own its settings without a second config implementation #1393.