Repository navigation
[tests] Wait until users, roles and grants reach all cluster replicas - #3277
Closed
polyglotAI-bot wants to merge 1 commit into
Closed
polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
On the 3-replica test cluster, users, roles, settings profiles and grants
are kept in the replicated user directory. Each replica updates its own
copy from Keeper asynchronously, and nginx sends each request to any
replica. A login, GRANT or DROP ... IF EXISTS right after a change can go
to a replica that does not have the change yet: AUTHENTICATION_FAILED,
UNKNOWN_ROLE, or ACCESS_ENTITY_ALREADY_EXISTS.
Add BaseIntegrationTest.waitForAllReplicas(). It checks system.<table>
on every replica through clusterAllReplicas('test_cluster', ...) with a
limited number of attempts, and does nothing outside cluster mode. Call
it after each access change that a later request depends on, and give
each test its own user, role and profile names, dropped in finally.
Part of: #3267
Fixes: #3237
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Client V2 CoverageCoverage Report
Class Coverage
|
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
6 tasks done
chernser
reviewed
Oct 30, 2026
| * | ||
| * @throws IOException when the request fails or the server returns an error | ||
| */ | ||
| public static String queryForResult(String sql) throws IOException { |
Contributor
There was a problem hiding this comment.
oh, no - we should use own client - it is capable enough for this. this code just add extra to tests and hard to maintain.
In V1 move tests to http client tests - this will be much easier to use V1 within tests
In V2 use V2 instance of client - if required create aux instance just for tests queries.
Contributor
There was a problem hiding this comment.
@polyglotAI-bot please fix:
- we should use own client - it is capable enough for this. this code just add extra to tests and hard to maintain.
In V1 move tests to http client tests - this will be much easier to use V1 within tests
In V2 use V2 instance of client - if required create aux instance just for tests queries.
chernser
requested changes
Oct 30, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
move v1 tests to http client module tests
use own client instead of url connection
4 tasks
Contributor
|
Closing as another implementation do not use URLConnection #3285 |
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
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.
Description
Part of #3267 (PR 1 of the plan in #3267 (comment), changed as requested: a check with limited attempts instead of an administrative command, and unique names). Fixes #3237.
Root cause: on the 3-replica cluster, users, roles, settings profiles and grants are in the
replicateduser directory. Each replica keeps its own copy and updates it from Keeper asynchronously. Login,GRANTandDROP ... IF EXISTSread only the local copy, and nginx sends each request to any replica. So a request right after a change can go to a replica that does not have the change yet:AUTHENTICATION_FAILEDGRANT ... TO <new user>→UNKNOWN_ROLE("There is no rolesome_user")DROP USER IF EXISTS some_userdoes nothing on a stale replica, thenCREATE USERcollides in Keeper →ACCESS_ENTITY_ALREADY_EXISTS(the error in [tests] Fix cluster tests on main #3267)Changes
BaseIntegrationTest.waitForAllReplicas(table, condition, expectedRows): runsSELECT hostName() AS host, countIf(<condition>) AS matching_rows FROM clusterAllReplicas('test_cluster', system.<table>) GROUP BY hostup to 100 times, 100 ms apart, and returns when all 3 replicas reportexpectedRows. It also requires 3 rows, because a replica with an empty table (for examplesystem.role_grants) is not in theGROUP BYresult. On failure it shows the last per-host result. It does nothing outside cluster mode.ClickHouseServerForTest.queryForResult(sql): runs one HTTP query and returns the body.ClientTests,HttpTransportTests; jdbc-v2StatementTest,ReadonlyProfileTest,ShowStatementDatabaseMetaDataTest):GRANT, profiles beforeCREATE USER ... SETTINGS PROFILE, and the final grant state before the assertions (granted_role_is_default = 0afterSET DEFAULT ROLE NONE);finally.testRuntimeCredentialChange: the 20 × 250 ms sleep-and-retry loops are replaced by the same check.Not changed:
default(ascloud.ymluses). The Cloud workflow does not run on PRs, so I could not verify it. Enabling it is a one-line change inwaitForAllReplicas.Test
Local cluster made by the repo's own
ClusterClickHouseContainerStarter(TEST_CLICKHOUSE_CONTAINER=cluster, CH 26.10.3.7). The 3 replicas were limited to 0.15 CPU each withdocker update, to reproduce the timing of a busy GitHub runner.mainHttpTransportTests(3 auth tests),ClientTestsrole testsACCESS_ENTITY_ALREADY_EXISTS,AUTHENTICATION_FAILED,UNKNOWN_ROLE)testRuntimeCredentialChange)StatementTest.testSettingRole,ReadonlyProfileTest,ShowStatementDatabaseMetaDataTest.testGetColumnsForReadonlyUserWithColumnGrantsUNKNOWN_ROLEonGRANT)testDDLStatements)The same tests also pass in single-server mode (
TEST_CLICKHOUSE_CONTAINER=single).Pre-PR validation gate
main, pass with the changeAGENTS.md(test-only, no workflow changes, Java 8 compatible)🤖 Generated with Claude Code
Note
Low Risk
Changes are limited to integration test helpers and test cases; production client/JDBC code is untouched.
Overview
Fixes flaky cluster integration tests where nginx can hit a replica whose replicated user directory has not caught up yet, causing
AUTHENTICATION_FAILED,UNKNOWN_ROLE, andACCESS_ENTITY_ALREADY_EXISTS.Test harness: Adds
waitForAllReplicasonBaseIntegrationTest, which pollsclusterAllReplicasagainstsystem.*tables (up to 100 × 100 ms) until every node reports the expected row count; it is a no-op outside cluster mode. AddsClickHouseServerForTest.queryForResultto read TabSeparated HTTP responses for that check.Affected tests: client-v2 and jdbc-v2 tests that create users, roles, profiles, or grants now use unique suffixed names, call
waitForAllReplicasafter each DDL/grant step that later steps depend on, and tear down infinally. Role tests share acreateUserWithRoleshelper; credential-change tests drop manual retry loops in favor of the same wait.Reviewed by Cursor Bugbot for commit 0ff1558. Bugbot is set up for automated code reviews on this repo. Configure here.