Repository navigation
PMM-15260 Make pmm-agent Node registration idempotent - #5890
Conversation
`pmm-agent setup` registered the Node on every run, and a forced registration makes PMM Server drop the Node together with every Service on it. That is harmless for the database operators, whose sidecars re-add their own services on each container start, but it would silently delete user-added services from a long-lived PMM Client pod, which is what the pmm-ha Helm chart is about to pre-provision as monitoring delegates. Registration is now skipped when the Agent already holds an ID and is being set up against the same PMM Server it is registered with. `--force` still registers the Node again, and pointing an Agent at a different PMM Server registers it there as before.
…gent Skipping registration for an Agent which already holds an ID strands it whenever PMM Server no longer knows that ID, e.g. after the server was reinstalled or restored from a backup taken before the Agent was registered. pmm-agent then loops on "No Agent with ID" while the Node monitors nothing. `pmm-agent setup` now asks the server whether it still knows the Agent and registers the Node again when it does not. Credentials the server rejects count as an answer too: registering reports that with an actionable message, whereas skipping it would fail silently later on. An unreachable server is not an answer: a failed check keeps the registration, so an Agent still starts while PMM Server has no leader elected yet.
|
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:
WalkthroughThe setup flow loads configuration, normalizes the server address, and queries inventory with one deadline. It classifies gRPC error payloads, detects Node conflicts and type mismatches, retries with setup credentials when required, and controls registration and configuration writes. Node removal and unregistration delete Grafana service accounts with bounded cleanup. Foreign service tokens produce warnings. The inventory API and command return those warnings. Sequence Diagram(s)sequenceDiagram
participant Setup
participant InventoryClient
participant PMMServer
Setup->>InventoryClient: Query Agent and Node with one deadline
InventoryClient->>PMMServer: GetAgent and GetNode
PMMServer-->>InventoryClient: Node data or classified error
InventoryClient-->>Setup: Registration state
Setup-->>Setup: Register, preserve, or stop
Setup->>PMMServer: Remove service account during node cleanup
PMMServer-->>Setup: Warning or error
Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Setup can wait nearly twice its intended timeout and omit a warning for discarded credentials. More importantly, a Grafana outage can hold node-removal database locks and connections for the cleanup timeout, so this should be resolved before merge. 🚥 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: c516639e-a555-4ae5-a838-b1650f616a79
⛔ Files ignored due to path filters (1)
agent/packages.dotis excluded by!**/*.dot
📒 Files selected for processing (7)
agent/commands/clients.goagent/commands/clients_test.goagent/commands/setup.goagent/commands/setup_test.goagent/config/config.goagent/config/config_test.goagent/config/encryption_test.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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5890 +/- ##
==========================================
+ Coverage 43.59% 46.34% +2.75%
==========================================
Files 415 418 +3
Lines 43134 44115 +981
==========================================
+ Hits 18804 20447 +1643
+ Misses 22454 21675 -779
- Partials 1876 1993 +117 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The GetAgent request built from a bare GetAgentParams literal carried no timeout, so a hung PMM Server could stall `pmm-agent setup` forever. Build the params with the generated constructor, which applies the runtime default, and cover the hanging server in the test. Use the Apache-2.0 header in agent/commands/setup_test.go, as every Go file under agent/ does. Signed-off-by: Alex Demidoff <a@demidoff.me>
Removing a Node through the inventory API left the Grafana service account of its pmm-agent behind, together with a valid admin token. The next registration of a Node with the same name then failed with "service account already exists" after the Node had already been committed, and every retry hit "Node already exists". The UI deletes Nodes through this endpoint, so the automatic re-registration added in this PR ran into it on every Node deleted from the UI. Delete the service account once the removal is committed, the same way UnregisterNode does. A failure only logs a warning: Nodes which pmm-agent never registered have no account. Signed-off-by: Alex Demidoff <a@demidoff.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
managed/services/inventory/nodes.go (1)
370-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured fields for the cleanup warning.
Warnfembedsnode.NodeNameanderrin the message. Use fields onlogger.Get(ctx)so log consumers can filter the Node and failure consistently.As per coding guidelines, use structured logging with fields instead of formatted values.
Proposed change
- logger.Get(ctx).Warnf("Failed to delete the service account of node %s: %s", node.NodeName, err) + logger.Get(ctx). + WithField("node_name", node.NodeName). + WithError(err). + Warn("Failed to delete the service account")Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: e89f182f-2843-4f4e-be87-386dbeaa4577
📒 Files selected for processing (7)
.mockery.yamlmanaged/cmd/pmm-managed/main.gomanaged/services/inventory/deps.gomanaged/services/inventory/mock_grafana_client_test.gomanaged/services/inventory/nodes.gomanaged/services/inventory/nodes_test.gomanaged/services/inventory/services_test.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.
The Grafana client has no HTTP timeout, so once the Node is removed an unresponsive Grafana could hold the RemoveNode request until the proxy gives up. Give the cleanup its own deadline, derived from the request context to keep the auth headers, and have the tests check for it. Signed-off-by: Alex Demidoff <a@demidoff.me>
The cleanup context was derived from the request, so a client giving up right after the Node was removed cancelled it, and the service account survived. Detach it from the request cancellation while keeping the auth headers, and log the warning Grafana returns when it keeps an account with custom tokens. Signed-off-by: Alex Demidoff <a@demidoff.me>
`pmm-admin config` runs `pmm-agent setup` without --config-file, so the Agent ID was never loaded and every run registered again. Load the file the running pmm-agent reports, and decide about the registration from it. While keeping a registration, keep the stored service token too: the credentials given to setup only serve to register, and used to be replaced by the token. Compare the PMM Server address of the file in its normalized form, and register again when PMM Server has the Agent on a Node with another name, which is what a configuration file copied from another host looks like. Tell the reasons apart in the output: rejected credentials, a server which could not be asked, and Node settings which only take effect when the Node is registered. Treat an ID PMM Server rejects as unknown to it. Signed-off-by: Alex Demidoff <a@demidoff.me>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
managed/services/inventory/nodes.go (1)
314-316: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the
RemoveNotFoundgRPC contract.
models.FindNodeByIDreturnsreform.ErrNoRowsfor an unknown ID, and this branch returns it directly. The existingmanaged/services/inventory/nodes_test.gocontract expectscodes.NotFoundwithNode with ID %q not found.. Handlereform.ErrNoRowsexplicitly before returning other errors.Proposed fix
node, err := models.FindNodeByID(s.db.Querier, id) if err != nil { + if errors.Is(err, reform.ErrNoRows) { + return status.Errorf(codes.NotFound, `Node with ID %q not found.`, id) + } return err }As per coding guidelines, check
reform.ErrNoRowsexplicitly for “not found”.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 71d2c61a-c85d-4eae-99c3-02b4839e5413
📒 Files selected for processing (7)
admin/commands/config.goagent/commands/clients.goagent/commands/clients_test.goagent/commands/setup.goagent/commands/setup_test.goagent/config/config.gomanaged/services/inventory/nodes.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.
A rejected or missing Node in the second lookup returned as unknown and kept the registration. Map its status the same way as the Agent lookup, so that rejected credentials and a missing Node register the Node again. Signed-off-by: Alex Demidoff <a@demidoff.me>
Three ways the idempotent setup could still lose the Services on a Node, or leave the Agent unable to reach PMM Server. A Node name which does not match the one PMM Server has registered the Agent on made setup register again. That does not replace the registered Node: without --region the address is not checked for uniqueness, so PMM Server creates a second Node under the given name and leaves the first one behind with every Service on it, monitored by nothing. The Node name defaults to the hostname, so any Node registered under a name of its own took this path on every plain re-run. Setup now stops instead and names both Nodes, --node-name to keep the registered one and --force to replace it. The registration was checked with the credentials given to setup, and then those were dropped in favour of the stored service token, which nothing had verified. A token PMM Server no longer accepts left setup reporting success and the Agent looping on a rejected connection, with no way back short of --force. Ask the server with the credentials the Agent runs with, so that a confirmed registration means the Agent's own credentials still work, and a rejected token registers the Node again with the credentials given to setup. A configuration file which exists but cannot be read - wrong encryption key, truncated YAML - was reported and then treated as no file at all, which means registering the Node. Whether the Agent is registered cannot be told from such a file, so stop, unless --force asks for exactly that. The Node address is only set when the Node is registered and nothing updates it afterwards, so a kept registration keeps the address it was registered with. Report it when it no longer matches: in pull metrics mode that is the address PMM Server scrapes. The Node lookup now returns the Node rather than its name alone, and checkRegistration takes the lookup as an argument, so the states it maps the answers of PMM Server to are covered by tests. Signed-off-by: Alex Demidoff <a@demidoff.me>
RemoveNode and UnregisterNode both delete the Grafana service account named after the Node, so that the token its pmm-agent authenticates with does not outlive the Node. Only the inventory copy was given a context of its own: UnregisterNode still passed the request context, so a client which gave up right after the Node was removed left the account behind, and an unresponsive Grafana held the request until the proxy gave up - the two failures this PR fixed for the other endpoint. Move the cleanup to services.RemoveNodeServiceAccount, which detaches the context from the request while keeping the auth headers Grafana needs, and bounds it. Both endpoints now behave the same way, and the contract is covered by a test of its own rather than only through the inventory service. Signed-off-by: Alex Demidoff <a@demidoff.me>
18d609e to
7342135
Compare
The service account cleanup of UnregisterNode now runs on a context of its own, so the mock no longer sees the request context it expected and the Register/Unregister subtest failed on an unexpected call. Match a context which carries a deadline, the way the inventory test already does, so that the test asserts what the cleanup has to do rather than which context object it is handed. Signed-off-by: Alex Demidoff <a@demidoff.me>
|
I stress-tested this against a live PMM built from your Feature Build and wanted to share what I found before it merges. The core fix works: a re-run of What follows is what sits around the fix. Most of it is on the paths an operator reaches by following the tool's own on-screen instructions. How this was tested. Critical1.
|
The message for an Agent registered on another Node offered --force to replace that Node together with its Services. RegisterNode resolves the Node to cascade-delete by the requested name, and the uniqueness check on the address is skipped without a region, so nothing is removed there: a second Node appears and the registered one is left behind, monitored by nothing. It also named --node-name, which is a positional argument in both CLIs, not a flag. The address-change message is the one place where --force does remove the Node with every Service on it, and the only one which did not say so. The two --force help strings disagreed with each other; align them and the documentation with what the flag does. Signed-off-by: Alex Demidoff <a@demidoff.me>
Reading the ID from the configuration file before the guard let `pmm-agent setup --skip-registration` through where it used to stop. That command skips the whole registration switch, so nothing restores the credentials the Agent runs with, and it stored a configuration assembled from the flags and the defaults alone: an empty server address and no service token, which was the only copy of it. Signed-off-by: Alex Demidoff <a@demidoff.me>
`pmm-agent setup` now reads the configuration file to tell whether this Node is already registered, and an encrypted file reads exactly like a damaged one without the key: LoadFromFile skips Decrypt when no key file is set and hands ciphertext to the YAML parser, so setup exits 1. pmm-admin forwarded neither key flag, so there was no supported way to run `pmm-admin config` on an encrypted installation - the env variable the message suggests is the systemd unit's, not the operator's shell. Forward both, defaulting to the same variables pmm-agent reads. Signed-off-by: Alex Demidoff <a@demidoff.me>
The check compared the registered Node against cfg.Setup.NodeName, which both `pmm-agent setup` and `pmm-admin config` fall back to the hostname and always pass on. A Node registered under any name which is not the hostname therefore failed every routine re-run: `pmm-admin config --metrics-mode=pull <addr> generic` exited 1 on a Node named db-prod-1, and a pmm-client container with PMM_AGENT_SETUP=1, a persisted config volume and no PMM_AGENT_SETUP_NODE_NAME refused to start, because the entrypoint exits on a non-zero setup. Classify the name while the flags are parsed: a name is the operator's when it is not the one it falls back to. kingpin fires no action for a value which came from the environment - which is how a container names its Node - so the classification happens after parsing rather than during it. A name which is only this host's is now reported the way a registered address or type already is, and the registration stands. Signed-off-by: Alex Demidoff <a@demidoff.me>
Keeping a registration refused to write the configuration file at all, because what setup assembled holds nothing the file alone carries and storing it would drop the ports range, the /proc/mounts path and every other file-only setting. `pmm-admin config` runs setup without --config-file, so every one of its invocations took that branch: it printed that the file was left unchanged and exited 0 having applied neither --paths-base nor --listen-port, --log-level, --log-lines-count or --server-insecure-tls, none of which it even named. The same branch swallowed `pmm-agent setup --skip-registration`, which exists to do nothing but write. Merge the flags onto the file the Agent runs with instead, which is the merge Get already performs for --config-file: the flags given win, and everything the file alone holds survives. The credentials stay the ones the Agent runs with, since those given to setup only serve to register and nothing was registered. Get's exit hook becomes applyDefaults so that the merged configuration reaches the same derived paths and defaults as a loaded one. Signed-off-by: Alex Demidoff <a@demidoff.me>
setServerTransport maps HTMLMime, TextMime and DefaultMime to a consumer which returns nginxError, and go-swagger drops the response it was reading into whenever a consumer fails. The typed answer went with it, and with the answer the HTTP status - the only thing which says a proxy in front of PMM Server demanded credentials of its own. serverStatus therefore returned 0 for every real proxy 401, so serverRefused was false and the registration was never checked again with the credentials given to setup. Take the body as the message for the two registration lookups and keep the typed answer, whose gRPC code stays zero because nothing PMM Server sent set one. The management client still gets nginxError, which is what register reports as "check pmm-managed logs". The test asserted the proxy case while serving application/json on every response, which no proxy does for its own error page, so it passed on a path the code never took. Serve the content type each case describes. Signed-off-by: Alex Demidoff <a@demidoff.me>
Registering returned the assembled configuration, so `--force` still wrote a file built from flags and defaults alone and dropped the ports range, the paths and the listening port with it. Those describe this host's installation rather than the Node on PMM Server: --force asks for the Node to be registered again, not for the Agent to be reconfigured from scratch. Merge onto the file there as well, and take the ID and the credentials from the assembled configuration, which after a registration holds the ones it just returned. Keep a log line count the file holds. --log-lines-count is the only stored setting whose flag carries a default, so it was the only one kingpin put that default over when the flags were re-parsed - a file saying 512 came back as 1024. Neither an action nor a default tells a value which was given from one which stood in, so the flag records that it fired and the variable is read directly. Signed-off-by: Alex Demidoff <a@demidoff.me>
serverNodeOfAgent built its own deadline, so the second lookup - the one made when PMM Server refuses the credentials the Agent runs with - got a fresh 30 seconds of its own. An operator promised one wait could face two: measured at 560ms against a 300ms budget. The caller owns the deadline now, and checkRegistrationOnServer sets it once for the whole check. The comment on registrationCheckTimeout said it bounded the check as a whole, which stopped being true when the retry landed; it is true again. Also report a username which was not used. keepRegistration restores both credentials from the file, but unappliedCredentials compared only passwords, so a username given beside the password the Agent already runs with was discarded without being named - the one thing that stops it passing for an accepted one. Signed-off-by: Alex Demidoff <a@demidoff.me>
A configuration file holds every path, derived or not, so parsing --paths-base over it moved the base and nothing else: the exporters, the tools, the temporary directory and the Nomad data directory kept pointing at the base the file came from. Before the flags are merged onto the file this was flags and defaults alone, where every path derived from the base given. Empty the paths the old base derived before the defaults are applied again, so that they follow the new base. A path an operator set does not match the old derivation and stays where it is. Signed-off-by: Alex Demidoff <a@demidoff.me>
Storing the whole Server struct of the assembled configuration put back exactly what merging the flags onto the file exists to avoid. The assembled configuration carries the flags and the defaults, so a file which alone holds `insecure-tls: true` was rewritten with it off, and `without-tls`, which no flag carries, could never survive a re-run. On a self-signed installation that leaves an Agent which can no longer reach PMM Server. Store only what setup settles, the ID and the credentials. The address and the TLS settings are the ones the merge already applied. Signed-off-by: Alex Demidoff <a@demidoff.me>
The lookup took the credentials from the file and the TLS settings from the flags. PMM Server ships a self-signed certificate, so a re-run which did not repeat --server-insecure-tls failed the lookup on certificate verification and left the registration unverified, saying nothing about a registration which is there. Carry the setting the Agent runs with into the lookup. A flag which asks to skip the check still stands over a file which does not. Signed-off-by: Alex Demidoff <a@demidoff.me>
`pmm-admin config` runs `pmm-agent setup` with c.Output(), which keeps stdout and drops stderr for a command that exits zero. The warning that nothing about the registration was verified went to stderr, so the run read as a confirmed registration. Write it where the rest of what setup says already goes. Signed-off-by: Alex Demidoff <a@demidoff.me>
Forwarding --config-file-key-password to `pmm-agent setup` put it in an argv every local user can read with ps, and in the line which logs the command under --debug. Hand it to the child in the environment, which pmm-agent reads it from anyway. The key file stays on the argv; it is a path, not a secret. Signed-off-by: Alex Demidoff <a@demidoff.me>
PMM Server answers a token Grafana failed to look up exactly like one it rejects, so a rejected token says nothing about the registration - the rule #5890 already applies to a refused token. The previous commit handled every errCredentialsRejected that way instead, which: - registered the Node again when setup had only the Agent's own token, so a brief Grafana failure could exit 1 or add a second Node; - hid a mistyped password: a rejected password was asked about again and ended unverified, where #5890 reports it and exits 1; - relied on errors.New(err.Error()) not matching the sentinel, and printed only "credentials rejected". withGivenCredentials now turns errCredentialsRejected into errTokenRejected only when the Agent runs with a token, and treats it like a refusal through a single predicate. A rejected password takes the #5890 path again, so the lookupError comment is back to its original text. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com>
* PMM-15692 Report a rejected service-account token as Unauthenticated. Grafana's /api/auth/serviceaccount has no sign-in middleware, so a token it rejects (wrong, expired, revoked, or of a disabled service account) gets 400 "Auth method is not service account token" instead of 401. pmm-managed mapped that 400 to codes.Internal. Since PMM-15186 the CLIs pick their hint from the code, so pmm-admin told users to check PMM Server logs instead of their credentials. Before PMM-13463 that 400 fell through to the API-key check, which answered 401; the live "Token auth - fail" test was switched to Internal when that fallback went away. Map 400 to Unauthenticated alongside 401 and 403, and restore the test's original expectation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * PMM-15692 Keep the Node when a token is rejected pmm-managed now answers a service token Grafana rejects with codes.Unauthenticated instead of codes.Internal. pmm-agent setup read Unauthenticated as errCredentialsRejected and registered the Node again without asking with the credentials given to setup. With the Node still registered that failed on its name ("already exists", exit 1, which stops a container running setup on start) or added a second Node under the hostname. A brief Grafana DB failure ends in the same 400, so it could trigger this too. withGivenCredentials now treats a rejection like a refusal: it asks again with the credentials given to setup and only registers when PMM Server does not know the Agent; otherwise the registration is left unverified and kept, as before. The agent tests modelled a refused token as 401 with codes.Internal; they now cover 401 with codes.Unauthenticated, end to end through checkRegistrationOnServer. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> * PMM-15692 Shorten the rejected-token comment AGENTS.md allows a comment one line, two at most; the explanation of Grafana's 400 lives in the ticket and the first commit's message. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> * PMM-15692 Test the rejected-token answer end to end The test called retrieveRole directly, so it checked the authError but not what a client receives. It now goes through ServeHTTP and asserts the HTTP status and the JSON body, the "code" the CLIs pick their hint from included. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> * PMM-15692 Map only the token lookup's 400 The 400 Grafana gives a token it did not authenticate is specific to /api/auth/serviceaccount, which has no sign-in middleware. Mapping every 400 in retrieveRole also covered /api/user and /api/user/orgs, so a future 400 on the basic-auth or cookie path would have read as bad credentials. getRoleForServiceToken now marks its 400 with errServiceTokenRejected, and retrieveRole maps only that, alongside 401 and 403 as before. The log keeps Grafana's real status. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> * PMM-15692 Name the Grafana error test for what it covers The table holds a server error and a basic-auth 400 next to the rejected credentials, so "RejectedCredentials" and its doc comment described only part of it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> * PMM-15692 Treat a rejected token as a refusal PMM Server answers a token Grafana failed to look up exactly like one it rejects, so a rejected token says nothing about the registration - the rule #5890 already applies to a refused token. The previous commit handled every errCredentialsRejected that way instead, which: - registered the Node again when setup had only the Agent's own token, so a brief Grafana failure could exit 1 or add a second Node; - hid a mistyped password: a rejected password was asked about again and ended unverified, where #5890 reports it and exits 1; - relied on errors.New(err.Error()) not matching the sentinel, and printed only "credentials rejected". withGivenCredentials now turns errCredentialsRejected into errTokenRejected only when the Agent runs with a token, and treats it like a refusal through a single predicate. A rejected password takes the #5890 path again, so the lookupError comment is back to its original text. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> * PMM-15692 Say which Node a rejected token belongs to With the service account disabled and the Node still there, the credentials given to setup find the Node, but that answer was thrown away: setup said "did not confirm that pmm-agent is registered", told the operator to re-run once PMM Server answers, and offered --force, which removes the Node with every Service on it. For a rejected token, the Node the second lookup finds is now the answer. Setup says that PMM Server has the Agent on that Node but does not accept its token, points to the Node's service account, and spells out what --force would remove; the registration is then confirmed and kept. A refused (401 without Unauthenticated) answer keeps the first answer as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> * PMM-15692 Ask again the way the Agent reaches PMM Server The second lookup set up its transport with the --server-insecure-tls given to setup only, ignoring insecure-tls from the configuration file that runningServer merges in for the first lookup. With the flag left off, it failed PMM Server's self-signed certificate instead of finding the Node, or finding it gone, and setup ended unverified. It now uses the merged setting, as the first lookup does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> * PMM-15692 Bring the registration doc comments up to date serverNodeOfAgent and runningServer still said that a rejected token registers the Node again, and serverRefused named a 401 without Unauthenticated as the only grounds for asking again. A caller trusting them would re-register on errCredentialsRejected, which is what withGivenCredentials now guards against for a token. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> * PMM-15692 Cover the rejected-token branches setup had no test for A rejected token with no PMM Server address to ask again, and a second lookup that fails for something other than the credentials - a deadline or credentials that cannot read the inventory - all have to keep the registration rather than register the Node again. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> --------- Signed-off-by: Jiří Čtvrtka <j.ctvrtka@gmail.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ticket number: PMM-15260
Feature build: Percona-Lab/pmm-submodules#4558
pmm-agent setupregistered the Node on every run. A forced registration makes PMM Server drop theNode together with every Service on it, because
RegisterNoderemoves the existing Node withRemoveCascade. That is harmless for the database operators, whose sidecars keep their configurationin an ephemeral file and re-add their own Services on each container start. It silently deletes
user-added Services from a long-lived PMM Client, which is what PMM-14989 reports and what the
pmm-ha Helm chart needs in order to pre-provision Client pods as monitoring delegates.
This PR covers points 1 and 2 of the ticket:
ID and which is set up against the same PMM Server keeps its Node and its Services.
--forcestillregisters the Node again exactly as before, and pointing an Agent at a different PMM Server
registers it there. The address is compared against the configuration file rather than the merged
configuration, which is what keeps
pmm-admin configagainst a new server working.stranded whenever PMM Server no longer has its ID, e.g. after the server was reinstalled or
restored from a backup predating the registration, and pmm-agent loops on "No Agent with ID" while
monitoring nothing. Setup now asks the server and registers again when the answer is no. 404, 401
and 403 all count as an answer, since registering either succeeds or reports a credentials problem
with an actionable message. An unreachable server is not an answer: a failed check keeps the
existing registration, so an Agent still starts while PMM Server has no elected leader.
Both commits are the prototypes from
PMM-14665-skip-non-eligible-nodes-from-add-service, reverted in9af8db5 to keep that ticket to its own scope. They are cherry-picked onto current
mainhere withno conflicts, and carry their unit tests.
Not in this PR
Points 3 and 4 of the ticket are deliberately left out and stay open:
failing with AlreadyExists. It weakens a guard that matters outside Kubernetes, where two hosts can
share a hostname, and the ticket's open question on gating it needs settling first.
would turn a systemd-managed install into a restart loop, and in Kubernetes a container restart does
not re-run init containers, so it does not by itself help the case above.
The release note required by the ticket's acceptance criteria is also not here and still needs
writing:
pmm-agent setupis now idempotent, and--forceremains the way to force re-registration.No API endpoint is added, removed or altered. The registration check consumes the existing
GET /v1/inventory/agents/{agent_id}.If this PR is related to other PRs, contributions, or ongoing work in this or other repositories, please reference them here:
PMM-HA-GAbranch of percona/percona-helm-charts pre-provisions PMM Client pods and currentlygates registration on the configuration file being absent. Once this ships, that gate can be
dropped so a Client whose Node the server has forgotten recovers on a pod restart.