roachtest: ensure valid suspect duration in mixed version decommission#108408
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Aug 10, 2023
Merged
Conversation
In cockroachdb#106859, the `decommission/mixed-versions` test was updated to properly support the decommission pre-checks introduced in 23.1, however in doing so there was an inadvertent bug introduced in the test due to the `server.time_after_store_suspect` setting. While this setting can be used to shorten the time a store is considered suspect after node restart, there exists a discrepency in this setting between 23.1 (the current predecessor major version) and 23.2, as 23.2 requires the setting to have a minimum of 10s, otherwise reverting to the default of 30s, despite the fact that this validation is not performed when the setting is actually overridden on the predecessor version. This change corrects that mistake, setting the value to the correct minimum version and waiting out the "suspect" time after restart before attempting decommission. Fixes: cockroachdb#107150. Release note: None
Member
|
This change is |
| // NB: The suspect duration must be at least 10s, as versions 23.2 and | ||
| // beyond will reset to the default of 30s if it fails validation, even if | ||
| // set by a previous version. | ||
| const suspectDuration = 10 * time.Second |
There was a problem hiding this comment.
nit: You can use the constant liveness.minTimeUntilNodeSuspect - although you would have to make that a public const. It might be easier to see where this comes from and if it changes later.
Contributor
Author
There was a problem hiding this comment.
I had sort of assumed we had a preference against importing from internal CRDB packages in roachtests... is that not the case?
andrewbaptist
approved these changes
Aug 9, 2023
Contributor
Author
|
bors r+ |
Contributor
|
Build succeeded: |
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.
In #106859, the
decommission/mixed-versionstest was updated toproperly support the decommission pre-checks introduced in 23.1, however
in doing so there was an inadvertent bug introduced in the test due to
the
server.time_after_store_suspectsetting. While this setting can beused to shorten the time a store is considered suspect after node
restart, there exists a discrepency in this setting between 23.1 (the
current predecessor major version) and 23.2, as 23.2 requires the
setting to have a minimum of 10s, otherwise reverting to the default of
30s, despite the fact that this validation is not performed when the
setting is actually overridden on the predecessor version.
This change corrects that mistake, setting the value to the correct
minimum version and waiting out the "suspect" time after restart before
attempting decommission.
Fixes: #107150.
Release note: None