roachprod: fix roachprod init.
#88514
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
When a cluster is started with the `--skip-init` option, the caller can run `roachprod init` at any time to initialize the cluster. Unfortunately, the code used to initialize the cluster was duplicated: one copy existed in the `start` path, and another in the `init` path. Since the latter is used far less frequently, it had a bug that went unnoticed: it hardcoded the first node index as `0`, when node indices start at 1. This commit fixes the issue by updating the constant and sharing code between `init `and `start`. Fixes cockroachdb#88226. Release note: None
|
This change is |
srosenberg
approved these changes
Sep 23, 2022
erikgrinaker
approved these changes
Sep 23, 2022
|
bors r=srosenberg,erikgrinaker TFTR! |
|
Build succeeded: |
|
I think this change might have had an unintended side effect - we no longer set the test license, which causes failures like #88665. |
This was referenced Sep 25, 2022
Closed
renatolabs
added a commit
to renatolabs/cockroach
that referenced
this pull request
Sep 26, 2022
In cockroachdb#88514, the cluster start logic was refactored to reuse the same code across `init` and `start`, fixing a bug in the former. However, the refactoring overlooked the fact that we previously always set the default cluster settings when there's more than one node in the cluster. This fixes that by setting the default cluster settings in that case; one particularly important cluster setting is the license key, necessary for some roachtests. Fixes cockroachdb#88660, cockroachdb#88665, cockroachdb#88666. Release note: None
renatolabs
added a commit
to renatolabs/cockroach
that referenced
this pull request
Sep 26, 2022
In cockroachdb#88514, the cluster start logic was refactored to reuse the same code across `init` and `start`, fixing a bug in the former. However, the refactoring overlooked the fact that we previously always set the default cluster settings when there's more than one node in the cluster. This fixes that by setting the default cluster settings in that case; one particularly important cluster setting is the license key, necessary for some roachtests. Fixes cockroachdb#88660. Fixes cockroachdb#88665. Fixes cockroachdb#88666. Fixes cockroachdb#88710. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Sep 26, 2022
88252: storage: add gc benchmarks with multiple range tombstones r=erikgrinaker a=aliher1911 Previously we only had tests that verify GC performance with no range tombstones or with a single range tombstone under all the data. This commit extends the benchmark to have many tombstones interleaving data at different keys. Release note: None Fixes #87417 88295: sql: add assertion for txn passed to internal executor r=ajwerner a=ZhouXing19 If an internal executor is created with a txn binding to it, it should not be used to execute a statement with a nil txn. Release note: None 88670: schemachanger/rel: fix race due to failure to clone constraint slots r=ajwerner a=ajwerner The fundamental race here is that while the slots themselves were being copied by value, the "any" clauses which are a slice were not. The second bug here is that the "inline" values were not being properly reset. That bug could lead to problems when the query was run again in the context of a different element set. We need to reset those inline values too. Fixes #88628 Release note: None 88716: roachprod: set default cluster settings when starting r=yuzefovich,srosenberg,smg260 a=renatolabs In #88514, the cluster start logic was refactored to reuse the same code across `init` and `start`, fixing a bug in the former. However, the refactoring overlooked the fact that we previously always set the default cluster settings when there's more than one node in the cluster. This fixes that by setting the default cluster settings in that case; one particularly important cluster setting is the license key, necessary for some roachtests. Fixes #88660 Fixes #88665 Fixes #88666 Fixes #88710. Release note: None Co-authored-by: Oleg Afanasyev <oleg@cockroachlabs.com> Co-authored-by: Jane Xing <zhouxing@uchicago.edu> Co-authored-by: Andrew Werner <awerner32@gmail.com> Co-authored-by: Renato Costa <renato@cockroachlabs.com>
smg260
pushed a commit
to smg260/cockroach
that referenced
this pull request
Jan 18, 2023
In cockroachdb#88514, the cluster start logic was refactored to reuse the same code across `init` and `start`, fixing a bug in the former. However, the refactoring overlooked the fact that we previously always set the default cluster settings when there's more than one node in the cluster. This fixes that by setting the default cluster settings in that case; one particularly important cluster setting is the license key, necessary for some roachtests. Fixes cockroachdb#88660. Fixes cockroachdb#88665. Fixes cockroachdb#88666. Fixes cockroachdb#88710. Release note: None
smg260
pushed a commit
to smg260/cockroach
that referenced
this pull request
Jan 18, 2023
In cockroachdb#88514, the cluster start logic was refactored to reuse the same code across `init` and `start`, fixing a bug in the former. However, the refactoring overlooked the fact that we previously always set the default cluster settings when there's more than one node in the cluster. This fixes that by setting the default cluster settings in that case; one particularly important cluster setting is the license key, necessary for some roachtests. Fixes cockroachdb#88660. Fixes cockroachdb#88665. Fixes cockroachdb#88666. Fixes cockroachdb#88710. Release note: None
smg260
pushed a commit
to smg260/cockroach
that referenced
this pull request
Mar 7, 2023
In cockroachdb#88514, the cluster start logic was refactored to reuse the same code across `init` and `start`, fixing a bug in the former. However, the refactoring overlooked the fact that we previously always set the default cluster settings when there's more than one node in the cluster. This fixes that by setting the default cluster settings in that case; one particularly important cluster setting is the license key, necessary for some roachtests. Fixes cockroachdb#88660. Fixes cockroachdb#88665. Fixes cockroachdb#88666. Fixes cockroachdb#88710. Release note: None
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.
When a cluster is started with the
--skip-initoption, the caller can runroachprod initat any time to initialize the cluster. Unfortunately, the code used to initialize the cluster was duplicated: one copy existed in thestartpath, and another in theinitpath. Since the latter is used far less frequently, it had a bug that went unnoticed: it hardcoded the first node index as0, when node indices start at 1.This commit fixes the issue by updating the constant and sharing code between
initandstart.Fixes #88226.
Release note: None