testcluster: clone Settings for each server#112456
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Oct 18, 2023
Merged
testcluster: clone Settings for each server#112456craig[bot] merged 2 commits intocockroachdb:masterfrom
craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
aa97820 to
3649355
Compare
3649355 to
fce396f
Compare
fce396f to
598efb5
Compare
There is no reason these should be embedded. Epic: none Release note: None
Currently there are many tests where multi-node test clusters share the same `*Settings` object. This is problematic - on one hand, it can make some test failures very hard to debug, and on the other hand, we are missing out on testing more realistic scenarios where nodes in a cluster don't magically have their settings in sync. This change adds test-only code to clone the Settings object for each server. Fixes cockroachdb#112395 Release note: None
598efb5 to
0073091
Compare
Member
Author
|
Friendly ping for whomever has a bit of time to review this. I have a few PRs on top that I hope to merge before I leave on vacation Thursday afternoon. Many thanks!! |
mgartner
approved these changes
Oct 18, 2023
Contributor
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @dt and @srosenberg)
Member
Author
|
TFTR! bors r+ |
Contributor
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Oct 18, 2023
112385: bootstrap: fix and clean up bootstrapping at older versions r=RaduBerinde a=RaduBerinde Note: this PR is only for the top commit, the rest is #112456. ---- This commit fixes the logic around bootstrapping clusters at earlier versions. Currently there is some special casing around the minimum supported version which makes that work, but other backwards compatible versions don't work correctly. Specifically, after bumping the version to 24.1, a `local-mixed-23.2` config for logic tests does not work. The core problem is that we are initializing the stores with a version and then populating the initial system values at another version. I am reasonably confident this was the root cause for #104994, where we tried adding a 23.1 config while the min supported version was 22.2. This fix makes the logic clearer and simpler. We can only bootstrap the cluster at specific versions (the current version, and compatible released versions). When the bootstrap version is overridden, we find the latest version that we can initialize the cluster and use that for both the stores and the initial values. Then we can upgrade to the desired version from there. This simplifies the tests: we no longer need `BootstrapVersionKeyOverride` (which was confusing, given that we also have `BinaryVersionOverride`). Epic: none Release note: None Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
andrewbaptist
pushed a commit
to andrewbaptist/cockroach
that referenced
this pull request
Mar 26, 2024
Previously the test TestRangeMigration was using artifical versions. This no longer works after cockroachdb#112456. This commit cleans up the test to validate upgrades work from `PreviousRelease` to `Latest` Epic: none Release note: None
craig bot
pushed a commit
that referenced
this pull request
Mar 26, 2024
121118: kvserver: clean up version logic r=kvoli a=andrewbaptist Previously the test TestRangeMigration was using artifical versions. This no longer works after #112456. This commit cleans up the test to validate upgrades work from `PreviousRelease` to `Latest` Epic: none Release note: None Co-authored-by: Andrew Baptist <baptist@cockroachlabs.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
3 participants
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.
testserver: unembed Settings, RaftConfig
There is no reason these should be embedded.
Epic: none
Release note: None
testcluster: clone Settings for each server
Currently there are many tests where multi-node test clusters share
the same
*Settingsobject. This is problematic - on one hand, it canmake some test failures very hard to debug, and on the other hand, we
are missing out on testing more realistic scenarios where nodes in a
cluster don't magically have their settings in sync.
This change adds some test-only code to clone the Settings object.
Fixes #112395
Release note: None