test: resolve CCM server flavor per cluster instead of from global properties - #984
Draft
nikagra wants to merge 7 commits into
Draft
test: resolve CCM server flavor per cluster instead of from global properties#984nikagra wants to merge 7 commits into
nikagra wants to merge 7 commits into
Conversation
…rs them driver-core has no Failsafe plugin binding (only bound in driver-tests/osgi/*), and Surefire's default includes never match *IT.java, so these CCM integration tests were silently never executed by `mvn verify -Pshort`/`-Plong`. Renaming to *Test.java matches Surefire's default discovery pattern, mirroring the fix already applied to DriverConfigReportingCcmIT in scylladb#973. Renamed: TabletsIT, ZeroTokenNodesIT, LWTLoadBalancingIT, SchemaBuilderIT. Now that LWTLoadBalancingTest actually runs, it surfaced a real (previously undetected) bug: both test methods constructed a SimpleStatement with bound values and then passed it to session.prepare(), which rejects statements carrying values. Fixed by preparing the value-free statement and binding values only on the resulting PreparedStatement, as the tests already intended. All classes verified live against ScyllaDB 2026.1.0: Tablets (3), ZeroTokenNodes (7), and LWTLoadBalancing (2) tests pass. SchemaBuilderTest's 6 methods remain pre-existing enabled=false, unrelated to this fix. Fixes scylladb#981. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CCMBridge.add(int, int) unconditionally included `-t <thriftItf>` in the `ccm add` command, but scylla-ccm's `add` command has no Thrift option at all (Scylla never had a Thrift interface) -- passing it makes the whole command fail with "ccm: error: no such option: -t". Confirmed against scylladb/scylla-ccm's actual ClusterAddCmd parser (ccmlib/cmds/cluster_cmds.py, master). ZeroTokenNodesTest is the only caller of this method, and it never ran before the scylladb#981 rename fix, so this was never caught. All three "Scylla ITs" CI matrix legs on scylladb#982 failed with the identical error once the rename made the test actually execute. Verified locally against a venv with the real `scylla-ccm` (master, same as CI's `make install-scylla-ccm`) installed: all 7 ZeroTokenNodesTest methods pass, plus a full regression of the other 3 renamed classes (12/12). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review follow-up on scylladb#982. Renaming these classes made them execute for the first time, but several of their assertions could not fail. Each fix below was verified live against ScyllaDB 2026.1.0. TabletsTest, two independent false-passes: - `removeTableMappings(KEYSPACE_NAME)` passed the mixed-case "tabletsTest" while TabletMap keys arrive lowercased from the server and are matched with an exact equals(), so the "empty out tablets information" step silently cleared nothing and an iteration could be satisfied by state learned in a previous one. Lowercased, as the three sibling call sites already do. - `executeOnAllHostsAndReturnIfResultHasTabletsInfo` pins the statement via setHost() and never clears it, so checkIfRoutedProperly re-executed a pinned statement and always observed exactly one coordinator -- `nodes.size() <= REPLICATION_FACTOR` could not fail. The pin is now cleared before the routing check. - Additionally, checkIfRoutedProperly now clears Statement.getLastHost() per iteration. PagingOptimizingLoadBalancingPolicy returns that host ahead of the real query plan and PagingOptimizingLatencyTracker sets it after every successful BoundStatement execution, which pinned the loop to its first coordinator for the bound-statement half of the matrix. LWTLoadBalancingTest could not distinguish PRESERVE_REPLICA_ORDER from RANDOM, for two reasons: - The framework's default keyspace is hardcoded to RF=1, so "the first replica" was trivially unique and hasSize(1) held under REGULAR routing too. initTestKeyspace() is now overridden to create an RF=3 keyspace (tablets disabled on Scylla, as elsewhere for replica-placement tests), following SingleTokenIntegrationTest's template. - Both tests re-executed one BoundStatement instance, so the paging optimisation described above pinned the coordinator after the first query -- hasSize(1) was guaranteed by that, not by the LWT path. Coordinator collection now binds a fresh statement per execution. - Added should_spread_non_serial_select_across_replicas as the control: same statement, same table, same policy, non-serial consistency level, asserting the coordinator does vary. Verified that it fails (1 coordinator) if REPLICATION_FACTOR is dropped back to 1, so the two hasSize(1) assertions are now load-bearing. ZeroTokenNodesTest asserted on a HashSet with containsExactly, which is order-sensitive; HashSet iteration order is hash-derived, so this was a latent flake. Switched to containsOnly, already used everywhere else in the file (AssertJ 1.7.1, which this module pins, has no containsExactlyInAnyOrder; for a Set containsOnly is equivalent). CCMBridge derived the instance's `isScylla` from the global scylla.version property instead of the constructor's scyllaVersion, unlike the sibling `isDSE = dseVersion != null` one line above. Now derived from the instance. Behaviour-neutral today -- Builder.scylla already defaults to the global, withScylla() has no callers, and every bridge that is actually built takes build()'s !versionConfigured branch where scyllaVersion *is* the global -- so this removes a footgun rather than fixing a live bug. Verified: TabletsTest 3/3, ZeroTokenNodesTest 7/7, LWTLoadBalancingTest 3/3 against ScyllaDB 2026.1.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`CCMBridge.Builder.build()` resolves an explicitly configured version into a Scylla version, a DSE version or a Cassandra version, but `buildCreateCommand()` only ever received the Cassandra and DSE ones. For Scylla the Cassandra version is hardcoded to 3.0.8 (what Scylla reports in `system.local`), so `withScylla(true).withVersion(...)` silently created an Apache Cassandra 3.0.8 cluster, while `add()` and `getScyllaVersion()` treated it as Scylla. Pass the whole resolution to `buildCreateCommand()` and emit `--scylla -v release:<version>`, the same shape the globally configured Scylla version already produces. The version resolution moves into `resolveVersions()` so it can be asserted without starting a cluster. The two remaining Scylla checks that have a per-instance equivalent (`jmxAddressOfNode()` and the Scylla-only yaml ports) now use it instead of the global version. `withSSL()`/`withAuth()` keep reading the global one: they run before the flavor is resolved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`SCYLLA_PRODUCT` was put into `ENVIRONMENT_MAP` once, in the static initializer, from the global `scylla.version` property. Since that map is immutable and shared by every cluster, a builder configuring its own Scylla version got the wrong repository in both directions: an explicit Enterprise version installed from the OSS repository, and an explicit OSS version inherited `SCYLLA_PRODUCT=enterprise` from a global Enterprise run. Split the map into a flavor-independent `BASE_ENVIRONMENT_MAP` plus a per-cluster environment derived from `ResolvedVersions`, threaded through the `CCMBridge` instance so every ccm command it issues sees it. The globally configured path keeps `ENVIRONMENT_MAP` verbatim: that one is derived from the raw property string, which may be a branch spec whose resolved version number looks like an Enterprise one without being installed as such. Not reachable from CI — `withScylla()` has no callers and no short-group test configures a version while creating a cluster — so this is footgun removal rather than a live bug fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two remaining reads of global state from per-instance code paths in CCMBridge, both in the same family as the previous rounds. `BASE_ENVIRONMENT_MAP` copied the process environment verbatim, so a SCYLLA_PRODUCT inherited from the surrounding shell reached every cluster that configures its own version — installing an explicitly configured OSS version from the Enterprise repository. It is now stripped there, leaving `withScyllaEnterprise` as the only way the variable is set for a version actually resolved as Enterprise. The globally configured version keeps the inherited value: a non-numeric `scylla.version` (a branch spec) can't be recognised as Enterprise, and exporting the variable is the only way to select the repository in that case. `withSSL()`/`withAuth()` picked JKS keystore vs PEM certificate settings from the global `scylla.version` at builder-configuration time, before the flavor is resolved, so an explicitly versioned cluster got the other flavor's TLS yaml in both directions. They now only record the request; `buildClientEncryptionOptions(ResolvedVersions)` derives the yaml at build time, sibling to `buildCreateCommand` and `buildEnvironmentMap`. Explicitly configured entries still win over these defaults, as they did when `withCassandraConfiguration()` ran after `withSSL()`. Because ssl/auth no longer show up in `cassandraConfiguration` at configuration time, `Builder.equals`/`hashCode` have to compare them directly — `CCMCache` keys cached clusters on the builder and would otherwise hand an encrypted cluster to a test that asked for a plaintext one. `hashCode` also picks up `scylla`, which `equals` already compared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
The two remaining review findings on this branch. Both are pre-existing scylla-3.x behavior rather than regressions from the commits before this one, and neither is reachable from CI, so this is footgun removal. SCYLLA_PRODUCT could still be inherited for the globally configured version. The previous commit stripped an inherited value from BASE_ENVIRONMENT_MAP, so no cluster configuring its own version could pick one up, but ENVIRONMENT_MAP was copied before that strip and kept whatever the surrounding shell exported. That is what scylla-3.x always did -- it has a single env map and no strip at all -- yet the comment justifying it only covered a branch spec, while the code applied it to a numeric OSS version, a pure Cassandra run, and no configured version too. In each of those the repository is already known from the resolved flavor, so a stale SCYLLA_PRODUCT=enterprise left in the shell would install an OSS version from the Enterprise repository. The assembly moved into buildGlobalEnvironmentMap, which now strips the inherited value unless the version is a branch spec -- the one case isScyllaEnterpriseVersion cannot classify, where exporting the variable is the only way to select the repository. Extracting it is what makes the decision testable: it is otherwise reachable only through a static initializer reading the live process environment, which is why the existing global-path test could assert nothing but isSameAs. @CCMConfig could not declare Scylla. Builder.scylla defaults to GLOBAL_SCYLLA_VERSION_NUMBER != null and withVersion() does not touch it, so under a Scylla run an explicit Cassandra version is resolved as a Scylla release. Base silently installed C* 3.0.8 and discarded the requested version; since buildCreateCommand started honouring the flag it fails loudly instead. Neither is right, and a test could not fix itself: @CCMConfig forwards dse but had no Scylla equivalent, and version()'s javadoc still called itself "the C* or DSE version to use". Added the scylla attribute, forwarded it from CCMTestsSupport alongside dse and ahead of withVersion, and documented on both withVersion() and @CCMConfig.version that the flavor flags decide what the version names. The two call sites that configure a version without a flavor now say which they mean: ProtocolVersionRenegotiationTest's 2.1.16 and RecommissionedNodeTest's 2.1.20 are both Cassandra versions. Testing: CCMBridgeCreateCommandTest grows to 13 cases, covering all four SCYLLA_PRODUCT paths against a synthetic inherited environment, so they hold regardless of the surrounding run. 13/13 with and without -Dscylla.version=2026.1.0 and with SCYLLA_PRODUCT=enterprise exported. Falsified by making buildGlobalEnvironmentMap always keep the inherited value: exactly the two strip cases fail. Full driver-core unit group green, 640 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes #983.
Why this is a separate PR
#982 exists to unblock
driver-core's four CCM tests that Surefire never discovered. Reviewing it surfaced thatCCMBridgeresolves the server flavor from global system properties throughout, so a cluster configuring its own version gets the surrounding run's flavor instead of its own. Three fixes for that were written and live-verified on #982's branch, but they're a refactor of the CCM test harness rather than part of unblocking the tests — so they moved here, verbatim, and #982 went back to being the rename plus theccm addfix it made necessary.None of this is reachable from CI today (see #983 for why), so it is footgun removal that makes the harness behave as its API claims, not a live CI failure. Sibling to #800, the same bug class in 4.x.
099a169abf— pass--scyllatoccm createwhen Scylla mode is explicitbuildCreateCommandonly ever received the resolved Cassandra and DSE versions, so itsversionConfiguredbranch emitted-v <cassandraVersion>and never--scylla. For Scylla that Cassandra version is hardcoded to3.0.8— what Scylla reports insystem.local, not an install target — sowithVersion(...)under-Dscylla.version=...had CCM build an Apache Cassandra 3.0.8 cluster whileadd()passed--scyllaandgetScyllaVersion()reported Scylla. The requested version was discarded outright.buildCreateCommandnow takes the whole resolution and emits--scylla -v release:<version>, the same shape the globally configured Scylla version already produces viaCASSANDRA_INSTALL_ARGS.build()intoresolveVersions()returning aResolvedVersionsholder — a straight extraction, no behaviour change, but it makes the flavor decision assertable without starting a cluster.jmxAddressOfNode()and the Scylla-only yaml ports (prometheus_port,api_port,native_shard_aware_transport_port) now use the per-instance version.9f34037f3b—SCYLLA_PRODUCTfollows the version each cluster installsENVIRONMENT_MAPwas built once in the static initializer and, being immutable and shared by every cluster, gave a builder that configures its own version the wrong repository in both directions:global OSS + explicit Enterprise version →
SCYLLA_PRODUCTunset → Enterprise version installed from the OSS repository.global Enterprise + explicit OSS version →
SCYLLA_PRODUCT=enterpriseleaks in → OSS version looked up in the Enterprise repository.The map is split into a flavor-independent
BASE_ENVIRONMENT_MAPand a per-cluster environment built bybuildEnvironmentMap(ResolvedVersions), sibling tobuildCreateCommand. It is threaded through theCCMBridgeinstance, so every ccm command a bridge issues —create,add,start— sees its own environment, not just the create.The 4-digit-year Enterprise check is now the shared
isScyllaEnterpriseVersionhelper, used by both the static initializer and the per-cluster path.withScyllaEnterprisecopies into aHashMaprather than usingImmutableMap.Builder: the inherited process environment may already defineSCYLLA_PRODUCT, and duplicate keys would makebuild()throw.58bf916fc6— client encryption follows the flavor,SCYLLA_PRODUCTcan't be inheritedSCYLLA_PRODUCTinherited from the surrounding shell. The previous commit derived the variable from the resolved version, butBASE_ENVIRONMENT_MAPstill copied the process environment verbatim — so an exportedSCYLLA_PRODUCT=enterprisereached every cluster configuring its own version, installing an explicitly configured OSS version from the Enterprise repository. It is now stripped there, leavingwithScyllaEnterpriseas the only way the variable gets set.Client encryption picked the wrong flavor's TLS yaml.
withSSL()/withAuth()chose between Cassandra's JKS keystore/truststore and Scylla's PEMcertificate/keyfile/truststore by reading the globalscylla.versionat builder-configuration time — before the flavor is resolved, and beforewithVersion()has any effect. Both directions were wrong.buildClientEncryptionOptions(ResolvedVersions)derives the yaml at build time, sibling tobuildCreateCommandandbuildEnvironmentMap.withCassandraConfiguration()ran afterwithSSL()— this matters forSSLSessionTicketsTest, the one test that sets aclient_encryption_options.*key of its own.Builder.equals/hashCodenow comparessl/authdirectly. This is required, not cosmetic: they no longer show up incassandraConfigurationat configuration time, andCCMCachekeys cached clusters on the builder, so without it an encrypted cluster could be handed to a test that asked for a plaintext one.hashCodealso picks upscylla, whichequalsalready compared.Unlike the other two, this one is reachable without a
withScylla()call:CCMTestsSupport.ccmBuildercallswithVersion()beforewithSSL(), so@CCMConfig(version = ..., ssl = true)is enough. It still isn't reachable from CI, because the only SSL tests areSSLTestBasesubclasses, which use@CCMConfig(ssl = true, createCluster = false)and never configure a version.Testing
Carried over from #982, where these commits were originally verified:
CCMBridgeCreateCommandTest(new,unitgroup, no CCM) pins the explicit-version branches ofccm create— Scylla gets--scylla -v release:<version>and never3.0.8, Cassandra gets a bare-v, DSE gets--dse— plus the execution environment and the flavor-specific client-encryption yaml. Each case sets the flavor explicitly so it doesn't depend on the surrounding run's system properties. 9/9 pass, with and without-Dscylla.version=2026.1.0and with or withoutSCYLLA_PRODUCTexported.buildEnvironmentMapreturnENVIRONMENT_MAPunconditionally fails the Enterprise case under a global OSS run, and the OSS + Cassandra cases under-Dscylla.version=2026.1.0— one failure per leak direction.SCYLLA_PRODUCTstrip makes the twodoesNotContainKeyassertions fail underSCYLLA_PRODUCT=enterprise.buildClientEncryptionOptionsfails the Scylla case under a Cassandra run and the Cassandra case under-Dscylla.version=2026.1.0.ssl/authcomparison fromequalsfails the cache-key test.short-group SSL tests against Scylla 2026.1.0 —SSLEncryptionTest+SSLAuthenticatedEncryptionTest, 9/9 pass. These are the tests CI actually exercises through this code, so this confirms the PEM path is unchanged end-to-end. The JKS path is left to the Cassandra IT legs; local CCM can't start Apache Cassandra here.ZeroTokenNodesTestlive (7 real clusters, plaintext): 7/7 — a regression check on the builder-equality change.driver-coreunitgroup green, so theBuilder.equals/hashCodechange breaks nothing else.099a169abfand9f34037f3beach passed all 10 checks including the four IT legs.82028f1425— the last two flavor decisionsThe two findings raised against the commits above. Both are pre-existing
scylla-3.xbehavior rather than regressions from them, and neither is reachable from CI.SCYLLA_PRODUCTcould still be inherited for the globally configured version.9f34037f3bstripped an inherited value fromBASE_ENVIRONMENT_MAP, so no cluster configuring its own version could pick one up, butENVIRONMENT_MAPwas copied before that strip and kept whatever the surrounding shell exported. That is whatscylla-3.xalways did — one env map, no strip at all — but the comment justifying it only covered a branch spec, while the code also applied it to a numeric OSS version, a pure Cassandra run, and no configured version. In each of those the repository is already known from the resolved flavor, so a staleSCYLLA_PRODUCT=enterprisewould install an OSS version from the Enterprise repository.The assembly moved into
buildGlobalEnvironmentMap, which strips the inherited value unless the version is a branch spec — the one caseisScyllaEnterpriseVersioncannot classify, where exporting the variable is the only way to select the repository. Extracting it is what makes the decision testable at all: it is otherwise reachable only through a static initializer reading the live process environment, which is why the previous global-path test could assert nothing butisSameAs.@CCMConfigcould not declare Scylla.Builder.scylladefaults toGLOBAL_SCYLLA_VERSION_NUMBER != nullandwithVersion()doesn't touch it, so under a Scylla run an explicit Cassandra version resolves as a Scylla release. Base silently installed C* 3.0.8 and discarded the requested version; since099a169abfstarted honouring the flag it fails loudly instead. Neither is right — and a test could not fix itself, because@CCMConfigforwardsdsebut had no Scylla equivalent, andversion()'s javadoc still called itself "the C* or DSE version to use".Added the
scyllaattribute, forwarded it fromCCMTestsSupportalongsidedseand ahead ofwithVersion, and documented on bothwithVersion()and@CCMConfig.version()that the flavor flags decide what the version names. The two call sites that configure a version without a flavor now say which they mean —ProtocolVersionRenegotiationTest's2.1.16andRecommissionedNodeTest's2.1.20are both Cassandra versions.Testing:
CCMBridgeCreateCommandTestgrows to 13 cases, covering all fourSCYLLA_PRODUCTpaths against a synthetic inherited environment, so they hold regardless of the surrounding run. 13/13 with and without-Dscylla.version=2026.1.0and withSCYLLA_PRODUCT=enterpriseexported.buildGlobalEnvironmentMapalways keep the inherited value: exactly the two strip cases fail, the Enterprise and branch-spec cases still pass.driver-coreunitgroup green — 640 tests, 0 failures — so the@CCMConfigaddition breaks nothing.🤖 Generated with Claude Code