Reload cluster discovery settings from remote_servers without restart. - #2197
Reload cluster discovery settings from remote_servers without restart.#2197ianton-ru wants to merge 22 commits into
Conversation
Previously ClusterDiscovery read user/password/path and related fields only at startup, so XML changes had no effect until restart. Apply discovery config diffs in place on reload, including add/remove of discovery and multicluster paths. Co-authored-by: Cursor <cursoragent@cursor.com>
Serialize start()/ensureWorkerStarted() so concurrent config reload cannot double-assign the worker thread, and remove the ephemeral ZK node when a participant is reloaded as an observer. Co-authored-by: Cursor <cursoragent@cursor.com>
Invisible-only config updates now schedule an upsert, and invisible upserts clear the published cluster so visibility toggles take effect immediately. Co-authored-by: Cursor <cursoragent@cursor.com>
When a multicluster-discovered cluster already occupies a name, adding a static <path> entry previously left watches/callbacks inconsistent. Remove the dynamic entry first so static config wins cleanly. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
ianton-ru
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90d6e81a0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (finished) | ||
| break; | ||
|
|
||
| consumePendingConfigUpdate(); |
There was a problem hiding this comment.
When the initial update repeatedly fails against a Keeper path that the reload removes or corrects, the worker calls initialUpdate before reaching this pending-config consumer, so every retry continues using the obsolete configuration and the corrective reload is never applied. Process the pending update before retrying initialization so the advertised restart-free recovery works even while the old path remains unavailable.
Useful? React with 👍 / 👎.
| if (registration_changed || invisible_changed) | ||
| clusters_to_update->set(info.name); | ||
| else | ||
| rebuildClusterObject(info); |
There was a problem hiding this comment.
When a reload changes <my_hostname> or <shard> without changing the node UUID set, this only schedules upsertCluster: registerInZk updates the existing znode data, but needUpdate compares only UUIDs and the subsequent rebuild uses the old nodes_info payload. The local cluster therefore retains the old address or shard, and peers are not notified because setting child data does not trigger their children watches; invalidate and reread the payloads or otherwise notify all observers.
Useful? React with 👍 / 👎.
| catch (...) | ||
| { | ||
| tryLogCurrentException(log, "Error while unregistering node from cluster '" + info.name + "'"); |
There was a problem hiding this comment.
If Keeper is temporarily unavailable while a participant cluster is removed or its path is replaced, swallowing this exception lets removeStaticCluster erase all local tracking and treat the reload as applied even though the old ephemeral registration may remain for the live session. Other servers can consequently keep routing queries or writes to a node that was removed from that cluster, with no later retry; fail the update and retry unregistration instead.
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
Registration changes only update ephemeral payload data, which does not fire children watches, and needUpdate skipped getNodes when UUIDs were unchanged. Clear nodes_info on registration change, recreate the ephemeral when payload differs so peers are notified, and always refresh payloads in upsertCluster. Co-authored-by: Cursor <cursoragent@cursor.com>
Previously, the first ClusterDiscovery constructed on config reload skipped updateFromConfig and never called start(), so enabling discovery post-startup left registration and watches inactive until restart. Start the worker when the server is already up, including the allow-flag-only reload path where remote_servers is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Invalid discovery XML (e.g. both password and secret) used to update clusters_config first and only then fail in updateFromConfig, leaving Clusters and discovery out of sync. Parse/validate discovery before mutating shared cluster state so SYSTEM RELOAD CONFIG fails closed. Co-authored-by: Cursor <cursoragent@cursor.com>
Unrelated config reloads were always calling notifyHostIDsUpdated after the early-return path was removed for allow-flag discovery start. Notify only when clusters changed, discovery was updated, or discovery was just created. Co-authored-by: Cursor <cursoragent@cursor.com>
An existing ClusterDiscovery is still updated when allow_experimental_cluster_discovery is disabled, so invalid discovery XML could commit Clusters and then fail in updateFromConfig. Validate whenever remote_servers changes and discovery is enabled or already constructed. Co-authored-by: Cursor <cursoragent@cursor.com>
If initialUpdate keeps failing, the worker never reached consumePendingConfigUpdate in the main loop, so a corrective remote_servers reload stayed stuck. Drain the pending update before each init attempt (and before startImpl's first init). Co-authored-by: Cursor <cursoragent@cursor.com>
Keep applying the local cluster removal when Keeper cleanup fails, queue the ephemeral path for worker retry, and cover it with a failpoint integration test instead of failing the whole reload. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop queued ZK cleanup for a path on participant re-add/register, and skip retries while an active participant owns that path, so a delayed unregister cannot remove a freshly re-registered ephemeral. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop redundant cancel helpers and fold unregister into one function; keep skipping retries when a participant owns the path again, and merge the failpoint coverage into a single test. Co-authored-by: Cursor <cursoragent@cursor.com>
Also stop marking Flags::cv mutable; only the mutex needs mutable for isStopped. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cde1440d06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clusters_to_update->remove(name); | ||
| get_nodes_callbacks.erase(name); | ||
| clusters_info.erase(it); |
There was a problem hiding this comment.
When a static observer cluster has replaced a same-named cluster discovered through a still-configured multicluster_root_path, removing that static entry erases the name here without marking the root's need_update. The generic wake from updateFromConfig is insufficient because findDynamicClusters skips roots whose flag is false, and the worker uses an untimed wait, so with no Keeper child event the dynamic cluster can remain absent indefinitely. Mark the remaining multicluster roots for rescan when removing a static shadow.
Useful? React with 👍 / 👎.
When a static cluster that shadowed a same-named dynamic entry is removed, mark remaining multicluster roots for update so findDynamicClusters rediscovers it without waiting for the force-refresh interval. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Erase impl when a name enters discovery and drop automatic_clusters when it returns to static or is removed, so Context no longer prefers a stale static Cluster after ownership transitions. Co-authored-by: Cursor <cursoragent@cursor.com>
Flags: :wait and multicluster/register bits were cleared before Keeper I/O, so a one-shot throw left the worker blocked until an unrelated event; restore consumed signals and re-arm wakeup for retry. Co-authored-by: Cursor <cursoragent@cursor.com>
…isabled. Reloading the allow flag from 1 to 0 left the worker registered and clusters published; tear down ClusterDiscovery with synchronous unregister so reload matches a restart with discovery off. Co-authored-by: Cursor <cursoragent@cursor.com>
Participant aliases on the same Keeper path share one ephemeral; skip unregister (and observer tryRemove) while another non-observer still owns that path so peers do not temporarily lose the retained replica. Co-authored-by: Cursor <cursoragent@cursor.com>
Watch callbacks used Flags::set, which could reinsert a removed cluster name and cause perpetual Unknown cluster scans; use setIfPresent and drop unknown keys in the worker loop. Co-authored-by: Cursor <cursoragent@cursor.com>
mkmkme
left a comment
mkmkme
left a comment
There was a problem hiding this comment.
lgtm in general, left three comments
There was a problem hiding this comment.
Not sure if this test actually makes sense to add. IIUC you changed the behaviour of repeating start() / updateFromConfig() so it doesn't abort anymore. Does this really need to be always checked in the unit test suite?
|
|
||
| auto & info = it->second; | ||
| auto zk = context->getDefaultOrAuxiliaryZooKeeper(info.zk_name); | ||
| registerInZk(zk, info); |
There was a problem hiding this comment.
It seems that the first thing upsertCluster does is registerInZk so the lines 1061-1062 seem to be redundant
There was a problem hiding this comment.
I'm also not sure about this unit test tbh. It has a any_need_update field which is not used (but I assume it's just taken from ClusterDiscovery::Flags it's mirroring).
But if we have a look at the rest of it, this test:
- Has its own class to test and doesn't test any of the actual production code, so any change that would break the class it mirrors would not be caught by the test
- It basically just tests the underlying
std::unordered_mapwhich we can assume to work just fine since it's part of the standard C++ library.
2 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Reload cluster discovery settings from remote_servers without restart.
Documentation entry for user-facing changes
Previously ClusterDiscovery read user/password/path and related fields only at startup, so XML changes had no effect until restart. Apply discovery config diffs in place on reload, including add/remove of discovery and multicluster paths.
CI/CD Options
Exclude tests:
Regression jobs to run: