upgrademanager: defer retrieval of cluster ID #113382
Merged
Conversation
This file contains 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
|
This change is |
stevendanna
force-pushed
the
deferred-cluster-id
branch
from
ea48bb0 to
0e48843
Compare
adityamaru
approved these changes
Oct 31, 2023
stevendanna
added
the
backport-23.2.x
|
bors r=adityamaru |
|
🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like. |
This changes more of the helper functions in the test to take a testing.T so that failures inside the function are correctly called on the subtests rather than the parent test. Epic: none Release note: None
Previously, the upgrade manager's constructor took the cluster ID directly. However, the cluster ID may not be initialised at the point of constructing the manager. This isn't typically a problem because in most cases, the only upgrade that _require_ the cluster ID doesn't actually take the cluster ID from the manager. Rather, it gets the cluster ID from the execution context of the migration job on resumption. But, tests such as TestRetriesWithExponentialBackoff set the DontUseJobs testing hook. In this case, the migration manager runs the upgrades directly, passing its own copy of the cluster ID, which is unitialized. To fix this, we pass the cluster ID container rather than the cluster ID so that we can defer retrieving it until we actually run the migrations, at which point it should be set. Informs cockroachdb#112763 Release note: none
stevendanna
force-pushed
the
deferred-cluster-id
branch
from
0e48843 to
6c4c8b5
Compare
|
bors r=adityamaru |
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Dec 6, 2023
113494: jobs: deflake TestRetriesWithExponentialBackoff r=adityamaru a=stevendanna This attempts to deflake TestRetriesWithExponentialBackoff. The majority of the problems with this test are the result of pause clearing the backoff state and thus needing to take a different path through most of this test. If this PR doesn't solve the flakes, I recommend that we just remove those two test cases and write a new test for them. The only reason I haven't done that here is that this test has revealed interesting job system behaviour in the past since it is so involved. See individual commits for more details. Fixes #112763 First two commits are form #113382 Co-authored-by: Steven Danna <[email protected]>
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.
Previously, the upgrade manager's constructor took the cluster ID
directly.
However, the cluster ID may not be initialised at the point of
constructing the manager. This isn't typically a problem because in
most cases, the only upgrade that require the cluster ID doesn't
actually take the cluster ID from the manager. Rather, it gets the
cluster ID from the execution context of the migration job on
resumption.
But, tests such as TestRetriesWithExponentialBackoff set the
DontUseJobs testing hook. In this case, the migration manager runs the
upgrades directly, passing its own copy of the cluster ID, which is
unitialized.
To fix this, we pass the cluster ID container rather than the cluster
ID so that we can defer retrieving it until we actually run the
migrations, at which point it should be set.
Informs #112763
Release note: none