Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

roachprod: set default cluster settings when starting #88716

Merged

Conversation

renatolabs
Copy link
Contributor

@renatolabs renatolabs commented Sep 26, 2022

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

@renatolabs renatolabs requested a review from a team as a code owner September 26, 2022 14:46
@renatolabs renatolabs requested review from herkolategan and removed request for a team September 26, 2022 14:46
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thanks!

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan and @renatolabs)


-- commits line 2 at r1:
nit: no trailing periods in the commit message titles.


-- commits line 14 at r1:
nit: this will only close the first issue - you need to put each issue on a separate line with its own "Fixes" prefix.

Copy link
Contributor

@smg260 smg260 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,

nit: I notice initializeCluster is followed by setClusterSettings twice - if this is a common pattern, might it make sense to abstract away into a function which does both?

@srosenberg srosenberg self-requested a review September 26, 2022 18:41
@srosenberg
Copy link
Member

srosenberg commented Sep 26, 2022

nit: I notice initializeCluster is followed by setClusterSettings twice - if this is a common pattern, might it make sense to abstract away into a function which does both?

Right. Technically, it's called twice only on the first node (i.e., n1), when numNodes > 1. I think setClusterSettings is idempotent and side-effect free (?).

@srosenberg
Copy link
Member

nit: I notice initializeCluster is followed by setClusterSettings twice - if this is a common pattern, might it make sense to abstract away into a function which does both?

Right. Technically, it's called twice only on the first node (i.e., n1), when numNodes > 1. I think setClusterSettings is idempotent and side-effect free (?).

Nope. It's invoked at most once on each node. In case of single-node clusters which are started via start-single-node, initializeCluster is skipped because that's done elsewhere, but setClusterSettings is still needed.

@renatolabs
Copy link
Contributor Author

Yes. clusterSynced.Init is the function that calls them both. Start's behavior a little tricky:

  • don't initialize nor set default cluster settings if --skip-init was passed
  • don't initialize but set cluster settings if starting with more than one node (not start-single-node)
  • do both otherwise.

#88514 broke that because of I misread the meaning of the shouldInit local variable. We should have restored the previous behavior now.

@renatolabs renatolabs force-pushed the fix-cluster-start-set-cluster-settings branch from fbdf786 to bc362ba Compare September 26, 2022 19:02
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
@renatolabs renatolabs force-pushed the fix-cluster-start-set-cluster-settings branch from bc362ba to ed443e1 Compare September 26, 2022 19:12
@renatolabs
Copy link
Contributor Author

bors r=yuzefovich,srosenberg,smg260

TFTRs!

@yuzefovich yuzefovich changed the title roachprod: set default cluster settings when starting. roachprod: set default cluster settings when starting Sep 26, 2022
@craig
Copy link
Contributor

craig bot commented Sep 26, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants