PMM-15360: Gate OpenManager's service on the switch - #5852
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## PMM-15360-om-switch-flag #5852 +/- ##
============================================================
- Coverage 46.94% 46.90% -0.05%
============================================================
Files 430 430
Lines 45409 45478 +69
============================================================
+ Hits 21318 21331 +13
- Misses 22030 22084 +54
- Partials 2061 2063 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8a4d7f9 to
ff8c156
Compare
a7edb05 to
38d343d
Compare
38d343d to
2ec32ca
Compare
2ec32ca to
2f0a73e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThe OM service now reports whether OpenManager is enabled and whether SEP Inventory is available. Scheduled collection skips disabled ticks. The server receives the OM service through Merge Risk: 🔵 Low · up to The change gates OpenManager RPCs and collection on the persisted switch, validates SEP before enabling, and starts an initial collection. It is mergeable with owner awareness because repeated updates can fail during temporary SEP unavailability, while configuration or initial collection failures may leave the switch enabled despite an error response; failure logging should also preserve structured error context. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the implementation, validation behavior, immediate collection, deferred scope, ticket, stacked PRs, and feature-build status. It does not include the template's API documentation checkbox, but the description states that no proto or per-method changes were made, so this is non-critical. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 782889cc-c850-4ecb-bd31-b1430e5860f9
📒 Files selected for processing (4)
managed/cmd/pmm-managed/main.gomanaged/services/om/service.gomanaged/services/server/deps.gomanaged/services/server/server.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
Implements Enabled() bool on om.Service, so every /v1/om/* RPC and the scheduled topology collection refuse while OpenManager is off, via the same generic gRPC-service-enabled interceptor BackupService and the other preview features already use -- no proto or per-method changes needed. Adds IsAvailable(), a read against SEP's om_inventory app (the same call the scheduled collection already makes), and wires it into ChangeSettings so turning OpenManager on is refused unless the app is actually reachable -- rather than accepting a setting that would do nothing. Turning it on also kicks an immediate topology collection (extracted into its own method: golangci-lint's gocognit check flags ChangeSettings once this is inlined alongside Advisors' and telemetry's own transition handling). The generated swagger client's field names are OMEnabled/EnableOM here (not OmEnabled/EnableOm, as in the settings-flag commit this stacks on): this branch's api/Makefile, via PMM-15326-om-api already merged into PMM-15326-om-backend, registers "om" as an additional-initialism for the swagger client generator. That commit hasn't reached the epic yet, so the settings-flag PR's own branch (based on the epic directly) correctly generates the un-capitalized form; this is the make gen output for this branch specifically. Does not touch SEP's om_inventory app in any way -- turning the switch off does not stop its periodic sweep. That's deferred to a follow-up. Signed-off-by: Pawel Lebioda <pawel.lebioda@percona.com>
2f0a73e to
bdf0a85
Compare
ChangeSettings now tells SEP whether OpenManager is on, on either transition of the switch: patchConfig and triggerRun are two small additions to sep_client.go's existing (previously read-only) client, and Service.SyncInventoryEnabled uses them to PATCH the app's ENABLED field and, on enabling, kick an immediate sweep rather than leaving the estate to wait out SCHEDULE's own interval. ENABLED rather than SCHEDULE, because the app's own configured cadence is an operator setting independent of whether OpenManager is turned on; PATCHing SCHEDULE here would silently overwrite a customized interval every time the switch is toggled off and back on. See OmInventorySettings in SEP (PMM-15326) for the other half. Both calls are best-effort, matching the existing triggerOMCollectionIfJustEnabled: a stale write or a sweep that does not fire means SEP is briefly out of step with the switch, not a broken settings change, so failure is logged rather than returned to the caller. No new permission needed on SEP's side: its existing --sep-token credential already clears the admin gate on the endpoints this uses, by identity, the same way scheduled inventory sync and scheduled execution already do. Verified end-to-end against a live dev stack through the real PUT /v1/server/settings API: disabling/enabling OpenManager there flips SEP's ENABLED accordingly, a custom SCHEDULE survives the round trip untouched, and an immediate sweep fires on enabling instead of waiting out SCHEDULE's own interval. Signed-off-by: Pawel Lebioda <pawel.lebioda@percona.com>
| if s.probe == nil || s.probe.app.client == nil { | ||
| return | ||
| } | ||
| if err := s.probe.app.patchConfig(ctx, map[string]any{"ENABLED": enabled}); err != nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
| if !enabled { | ||
| return | ||
| } | ||
| if err := s.probe.app.triggerRun(ctx); err != nil { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
avoid inline error handling using if err := ...; err != nil; use plain assignment err := ... (noinlineerr)
|
The failing Evidence it's environmental, not this diff:
Looks like the shared CI base image ( |
2 participants
What
Implements
Enabled() boolonom.Service, so every/v1/om/*RPC and the scheduled topology collection refuse while OpenManager is off, via the same generic gRPC-service-enabled interceptorBackupServiceand the other preview features already use — no proto or per-method changes needed.Adds
IsAvailable(), a read against SEP'som_inventoryapp (the same call the scheduled collection already makes), and wires it intoChangeSettingsso turning OpenManager on is refused unless the app is actually reachable — rather than accepting a setting that would do nothing. Turning it on also kicks an immediate topology collection so the page is not empty on first view.Does not touch SEP's
om_inventoryapp in any way — turning the switch off does not stop its periodic sweep. That's deferred to a follow-up.Ticket: PMM-15360
Stacking
Second of three PRs for PMM-15360. Stacks on
PMM-15326-om-backend(#5816, needsom.Service) and #5851 (needsSettings.OpenManager.Enabled) — both are merged in here since there's no single upstream branch with both yet, so this diff includes #5851's changes inline until that one merges. Only the last commit ("Gate OpenManager's service on the switch") is net-new here.Feature build: N/A on its own, same reasoning as #5851 — the UI half that would make this visible lands in the sibling nav/page PR.