PMM-15227 Remove stale HA replicas from Inventory - #5738
Conversation
Render
Destroyed
Render
Destroyed
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5738 +/- ##
==========================================
+ Coverage 43.59% 45.29% +1.70%
==========================================
Files 415 217 -198
Lines 43134 27893 -15241
==========================================
- Hits 18804 12635 -6169
+ Misses 22454 13879 -8575
+ Partials 1876 1379 -497
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughHA setup now removes eligible stale PMM Server nodes after migrations when trusted peer data identifies scaled-down replicas. It preserves active replicas, the default server node, and nodes with monitored services. Tests cover detection, removal, rollback, and invalid peer data. Documentation describes Inventory behavior. ChangesHA stale-node cleanup
Sequence Diagram(s)sequenceDiagram
participant SetupDB
participant Cleanup as Stale HA cleanup
participant Nodes as PMM node records
participant Agents as PMM agent records
SetupDB->>Cleanup: Pass configured HA peers
Cleanup->>Nodes: Find obsolete PMM Server nodes
Cleanup->>Agents: Check monitored services
Cleanup->>Nodes: Remove eligible nodes and agents
Cleanup-->>SetupDB: Log cleanup result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@documentation/docs/install-pmm/install-HA-clustered.md`:
- Line 832: Revise the statement about removed replica Nodes in the HA cluster
cleanup documentation to say that only eligible stale Nodes disappear after
remaining pods restart. Mention that removal is skipped when peer data is
untrusted or the Node still monitors Services, and instruct operators to move
those Services to a running replica before removal.
In `@managed/models/node_helpers_test.go`:
- Around line 272-275: Replace the live PostgreSQL setup in the
RemoveStaleHANodes unit test with a go-sqlmock database, configuring only the
SQL expectations needed by RemoveStaleHANodes. Keep testdb.Open out of this unit
test; move the coverage to integration tests only if the behavior cannot be
validated with mocked database interactions.
In `@managed/models/node_helpers.go`:
- Around line 375-394: Exclude the legacy PMM Server node from stale-replica
removal by adding a check for node.NodeID == PMMServerNodeID in the loop before
removeNode is called. Preserve the existing handling for eligible HA replicas
and retain the legacy node without attempting removal.
- Around line 411-420: Update the peer parsing helper around the existing
strings.TrimSpace/strings.Cut logic to call net.ParseIP on the full trimmed peer
entry before splitting at “:”, rejecting unbracketed IPv6 addresses instead of
treating their first segment as a node label. Preserve the existing host and
label validation for non-IP entries, and add an unbracketed IPv6 case to the
untrusted-peer tests.
- Around line 359-399: Replace the package-level logrus.Warnf and logrus.Infof
calls in the stale HA cleanup flow with structured *logrus.Entry logging. Attach
relevant fields such as node_id, node_name, peer, and ha_node_id to each event
while preserving the existing messages and control flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0769d83-7fa0-483d-b09c-be3789de1b70
📒 Files selected for processing (4)
documentation/docs/install-pmm/install-HA-clustered.mdmanaged/models/database.gomanaged/models/node_helpers.gomanaged/models/node_helpers_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
ademidoff
left a comment
ademidoff
left a comment
There was a problem hiding this comment.
Review of the HA stale-node cleanup, against b2cbb72. Findings are inline; the first four are the ones I'd act on before merge, the rest are hardening, efficiency and test/doc notes.
The two headline items: the pre-HA pmm-server Node is classified as stale (either a permanent unactionable warning or, in the tail case, a PermissionDenied that aborts the migration and crash-loops the replica), and the cleanup runs at migration altitude where any failure keeps the server from booting.
Verified locally: make test-models RUN=TestRemoveStaleHANodes passes, golangci-lint is clean on the changed files, and the PermissionDenied path was reproduced with a throwaway test (since deleted). Everything else is reasoned from the code and labelled as such.
Nice fix on the unbracketed IPv6 peer in haPeerNodeName — that one is closed.
removeNode's ban and the stale-HA-node skip both compared against PMMServerNodeID, which setupPMMServerHAAgents reassigns to the replica's generated Node ID. pmm-managed retries SetupDB in a loop, so a commit that failed after registration reopened both guards on the next attempt and would cascade-delete a pre-HA "pmm-server" Node. Compare against defaultPMMServerNodeID instead; PMMServerNodeID keeps its runtime meaning for the callers that need it.
RemoveStaleHANodes ran inside the schema-migration transaction, so any error it returned aborted the migration and pmm-managed eventually gave up with "Could not migrate DB: timeout". A NotFound from the monitored- services pre-check is enough to trigger that whenever replicas restart together. Split it into StaleHANodes, which picks the Nodes to drop, and RemoveStaleHANode, which drops one. migrateDB runs the sweep after the migration commits, gives each Node a transaction of its own, and logs failures instead of returning them: a Node that can't be removed is rolled back whole and skipped, and the rest of the sweep continues. Extract setupFixtures to keep migrateDB under the gocognit limit.
Call the sweep from SetupDB instead of migrateDB, which had no business rewriting Inventory and only grew a setupFixtures extraction to stay under the gocognit limit; migrateDB is back to its original form. Splitting selection from removal left the monitored-services check in a different transaction than the delete, so a service bound to the replica in between would be cascade-removed, and left RemoveStaleHANode willing to remove any Node handed to it with the PMM Server ban lifted. Re-check inside the removal, which closes both, at a negligible cost of three queries per removed Node during startup.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
managed/models/node_helpers.go (1)
109-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
idx++increment instead of commenting it out.Line 112 leaves
idx++as dead code. If a maintainer adds a third filter below this block, that filter reuses placeholder index2and binds the wrong argument. Keep the increment so the counter stays correct by construction. The Go compiler does not complain about an assigned-but-unusedidxhere becauseidxis read earlier.♻️ Proposed fix
if filters.IsPMMServerNode != nil { conditions = append(conditions, "is_pmm_server_node = "+q.Placeholder(idx)) args = append(args, *filters.IsPMMServerNode) - // idx++ + idx++ } + _ = idxAn alternative is to drop the
_ = idxline and instead keepidxincremented in every branch, adding the blank assignment only if a linter reports the final value as unused.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed/models/node_helpers.go` around lines 109 - 113, Restore the idx++ increment in the filters.IsPMMServerNode block so each appended condition advances the placeholder counter. Remove the commented-out increment and preserve the existing conditions and args handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@managed/models/node_helpers.go`:
- Around line 109-113: Restore the idx++ increment in the
filters.IsPMMServerNode block so each appended condition advances the
placeholder counter. Remove the commented-out increment and preserve the
existing conditions and args handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 616706a6-3f5f-413d-96d4-0c565849bd56
📒 Files selected for processing (7)
documentation/docs/install-pmm/install-HA-clustered.mdmanaged/models/agent_helpers.gomanaged/models/agent_helpers_test.gomanaged/models/database.gomanaged/models/node_helpers.gomanaged/models/node_helpers_test.gomanaged/models/node_model.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
🚧 Files skipped from review as they are similar to previous changes (3)
- managed/models/node_model.go
- managed/models/database.go
- documentation/docs/install-pmm/install-HA-clustered.md
The warning and the HA scaling doc both told the operator to re-add the services and then delete the Node from Inventory. Every user-facing delete path goes through RemoveNode, which refuses PMM Server Nodes - that refusal is what this ticket works around - so the advice ended at the wall the feature exists to remove. Point at the next restart's sweep instead, and say the same thing in both places. Two doc comments overstated the cascade: removeNode deletes Services attached to the Node, but for Services whose exporters merely run on it only the exporters go, and RemoveStaleHANode can never delete a Service because its own re-check refuses such a Node. Keep the error on the "already removed by another replica" branch: a NotFound from a child agent mid-cascade lands there too, and that case rolls back and leaves the Node in Inventory.
haPeerNodeName decides the whole sweep: an entry it reads no name from stops the cleanup and keeps every Node, while a name it does read is trusted as a live replica. Until now it was only reachable through DB-backed subtests asserting on the sweep's result, where a failure says "elements differ" rather than naming the input. The table goes in models_test.go, the package's existing internal test, so no database is involved and each case gets its own subtest. Three shapes the DB-backed table never reached are covered: bare IPv4 without a port, a bracketed IPv6 without a port, and memberlist's name/address form carrying an IPv6 address.
| return nil, err | ||
| } | ||
|
|
||
| removeStaleHANodes(ctx, db, params) |
There was a problem hiding this comment.
I doubt about the place where it should be running. I think that HA Leader is a good candidate for this.
|
Re-reviewed at
Two things you added that I hadn't asked for and that close real gaps: Verified locally at this SHA:
One note on @maxkondr's suggestions for Unrelated to this PR, but found while running the suite: From my side the findings are closed. Happy to turn this into a formal approval if that's useful. |
Signed-off-by: Ante Gulin <[email protected]>
Signed-off-by: Ante Gulin <[email protected]>
Rename StaleHANodes to FindStaleHANodes, matching the verb-prefixed finders around it. Rename its haNodeID argument to localHANodeID: it is the calling replica's own PMM_HA_NODE_ID, not the leader's, and it doubles as the Node name that replica registers itself under. Return an error when PMM_HA_PEERS names no peers or omits this replica. Both contradict the way the chart renders the list, so reporting them as "nothing is stale" left the caller unable to tell a misconfiguration from an empty result. A list of bare addresses stays a warning: that is valid configuration which simply carries no names. Move the sweep out of SetupDB into an exported RemoveStaleHANodes so it can be driven by a caller rather than by database setup. Signed-off-by: Ante Gulin <[email protected]>
Signed-off-by: Ante Gulin <[email protected]>
OnNodeID and PMMAgentIDs replace FindAgentsOnNode and FindAgentsByPMMAgentIDs. An empty PMMAgentIDs matches everything, so haNodeMonitoredServices guards the call. Signed-off-by: Ante Gulin <[email protected]>
PMM_HA_PEERS is fixed into a process's environment at startup and cannot be re-read, so only then is it guaranteed to describe the current cluster. A leader service can run much later: a rolling update recreates the leader, forcing an election that a not-yet-updated pod can win, and that pod sweeps with a peer list older than the cluster. On a 2 -> 3 scale-up this deleted the new replica's Node and Agents 12 seconds after it registered them, leaving a running replica with no Inventory Node. Scaling down to one replica also never swept at all, because a lone survivor cannot reach quorum to become leader. Signed-off-by: Ante Gulin <[email protected]>
ademidoff
left a comment
ademidoff
left a comment
There was a problem hiding this comment.
Re-reviewed the delta since 34de666 (6 commits). Nothing blocking — two nits inline, both documentation-only.
Checked in particular that folding the finders into AgentFilters kept the semantics: OnNodeID still expands to (runs_on_node_id = $n OR node_id = $n+1), so the services.node_id fix is intact, and I traced the placeholder arithmetic through all eight filters — idx advances correctly past the IN list and the two-placeholder OnNodeID. Also confirmed the new q.Context().Err() check can't panic: reform initialises Querier.ctx to context.Background() and clone() propagates it, so Context() is never nil.
Special mention for the leader revert in f76164c. Trying @maxkondr's suggestion, reproducing the failure on a live 2 -> 3 scale-up, and writing the reason down at managed/models/database.go:1526-1533 is a better outcome than either taking or refusing the suggestion on argument alone.
Verified locally at this SHA: go build ./managed/... clean; TestFindStaleHANodes (12), TestRemoveStaleHANode (6), TestHAPeerNodeName (15), TestAgentHelpers (17) and TestNodeHelpers (5) all pass; golangci-lint flags nothing in this diff. All 11 findings from my first review are still fixed after the refactor — I re-checked each rather than assuming the rename carried them over.
| type AgentFilters struct { | ||
| // Return only Agents started by this pmm-agent. | ||
| PMMAgentID string | ||
| // Return only Agents started by any of these pmm-agents. An empty slice is not a filter. |
There was a problem hiding this comment.
Two asymmetries in the new filters are worth spelling out in the FindAgents doc comment as well, not only on the fields — a new caller reads the function, not the struct:
- An empty
PMMAgentIDsmatches everything, not nothing. You've documented it here and guarded the one caller atmanaged/models/node_helpers.go:499, withReportsScaledDownReplicaWithNoAgentsto keep it honest, so nothing is broken today. But the guard is load-bearing and lives at the call site, and the next caller that builds the slice withappendin a loop getsnilon the empty path — exactly the shape that silently returns the whole inventory. I don't think it can be fixed inside the filter-struct pattern (nil, empty and unset are indistinguishable), which is precisely why it belongs in the function's own doc comment. PMMAgentIDprobes,PMMAgentIDsdoesn't. The singular runsFindAgentByIDfirst and returnsNotFoundfor an unknown ID; the plural returns an empty result. Someone switching a call from one to the other loses that error without noticing.
Both are one line each — no code change intended.
| return models.RemoveStaleHANode(tx.Querier, "ha-node-2") | ||
| })) | ||
|
|
||
| // what a replica sees when another one won the race; RemoveStaleHANodes reads this as |
There was a problem hiding this comment.
Stale name: RemoveStaleHANodes was renamed back to unexported removeStaleHANodes in f76164c when the leader service was reverted. Left over from that revert.
4 participants
PMM-15227
Percona-Lab/pmm-submodules#4513
Problem
Every PMM Server replica in an HA deployment registers its own inventory Node at first boot,
and nothing ever removed it. After scaling the deployment down, the departed replica kept
appearing under Inventory > Nodes with status Unknown.
Scope
HA deployments only, and inventory rows only. No API or schema change; metrics already
written to VictoriaMetrics are left alone.
Solution
At startup, remove PMM Server Nodes whose name is absent from the configured
PMM_HA_PEERS.The peer list is the signal because the chart regenerates it from the replica count and
recreates every pod when it changes, whereas a member missing from the memberlist cluster may
simply be restarting.
Cleanup is skipped when the peer list cannot be trusted (an entry that carries no node name,
or a list that omits this replica), and a departing replica that still monitors Services is
kept with a warning instead of having those exporters cascade-deleted.