roachprod: rewrite `roachprod start` #51790

irfansharif · 2020-07-22T19:54:21Z

This is quite the workhorse, and does a lot and has to be compatible
with a lot of existing CRDB versions. It's grown organically as a result
and I'm finding it a bit difficult to maintain, breaking it down a bit
makes it clearer what the structure of it all is and would've perhaps
prevented me introducing bugs like #51497.

Do scrutinize the PR closely, we use roachprod start everywhere. It's
mostly mindless code movement but it's pretty fragile code.

Release note: None

cockroach-teamcity · 2020-07-22T19:54:28Z

This change is

irfansharif · 2020-07-22T19:55:39Z

I've tested this with a few versions manually, but I also did that before and things still slipped through the crack. How do I make sure this is not breaking things any further?

irfansharif · 2020-07-22T20:04:18Z

Feel free to reject this PR, I'm more than happy to send out a more targeted patch and leave things as is if this is too controversial.

petermattis

Feel free to reject this PR, I'm more than happy to send out a more targeted patch and leave things as is if this is too controversial.

I'm not going to have the time to properly review this patch this week. So if there is something that needs to be fixed soon, a more targeted patch would be useful. That said, start has evolved over time and I can see the benefit in doing a clean slate rewrite.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @petermattis)

irfansharif · 2020-07-22T21:34:29Z

Thinking about this a bit more, the targeted patch would have the same code block movements, this just packages it better, so I'm happy waiting (perhaps @knz may disagree). Tagging @jlinder for dev-inf eyes.

knz · 2020-07-23T14:26:24Z

I'm good with reviewing this (even the complex form), just today I'm a bit swamped. Will get to this soon.

nvanbenschoten · 2020-07-23T15:50:17Z

This is pretty tough to review. Is there really not a more targetted patch that we could pull out that addresses the issue without rewriting everything at the same time? Or can we split the rewrite into a separate commit so that we aren't "sneaking in the fix for #51497"? I have a hard time even finding where that logic change is in this.

irfansharif · 2020-07-23T16:07:51Z

Hm, ok, let me re-work this. I'll break this up so it's clearer where the code movement is happening.

This will come in handy when understanding future commits. Release note: None

The ServerNodes API is a bit confusing to work with (see earlier commit improving it's documentation), and given it's iterable an index `n` into the returned list is either identifying the n-th node in the list, or node n within the cluster. To this end, using the name `nodeIdx` to refer to the former and `node` to refer to the latter, should hopefully clarify which one it is that we're referring to. Also, these loop bodies are pretty large, we shouldn't be using `i` so liberally anyway (especially when it's not a simple loop iteration index). Release note: None

irfansharif · 2020-07-23T21:57:30Z

PTAL, I'm keeping this PR focused on just the re-write, and outlining in comments what the current problems are without actually addressing them. I'll get to those after.

nvanbenschoten

thanks for splitting this up! It made it much easier to review.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 1 of 1 files at r11.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @irfansharif, @jlinder, and @knz)

pkg/cmd/roachprod/install/cockroach.go, line 520 at r4 (raw file):

	for i, vpc := range h.c.VPCs {
		if i > 0 && vpc != h.c.VPCs[0] {
			advertisePublicIP = true

return true and return false below?

And add a few helper methods to start thinning down `roachprod start` code (this commit pulls out `useStartSingleNode`, `shouldAdvertisePublicIP`, `getEnvVars`, `distributeCerts`). Release note: None

The code here is a bit duplicated with the generation of `encryptArgs`, but in future commits we'll be separating out the generation of args provided to `cockroach start`, and this, though related, does not apply. Release note: None

These helpers focus on generating the raw commands that are used when initializing a cluster, and setting cluster settings following. Release note: None

These helpers actually initialize the cluster, and set cluster settings following. Release note: None

This helper generates arguments provided to `cockroach start`. We improve comments while here. Release note: None

This helper, in the same style as `generateClusterSettingCmd` and `generateInitCmd`, generates the raw command used to start a given node. Release note: None

This helper is responsible for actually starting a given cockroach node. Release note: None

This will be addressed in follow-on PRs. Release note: None

irfansharif

Thanks for the quick turn around! Going to just blaze ahead to catch nightly failures tonight (hopefully this adds nothing more than already present).

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jlinder, @knz, and @nvanbenschoten)

craig · 2020-07-23T23:04:06Z

This PR was included in a batch that was canceled, it will be automatically retried

irfansharif · 2020-07-24T00:05:12Z

bors r+

craig · 2020-07-24T00:05:14Z

Already running a review

craig · 2020-07-24T01:21:46Z

Build succeeded:

GitHub CI (Cockroach)

..and the setting of cluster settings for single node clusters. `roachprod start --sequential` was broken in cockroachdb#51329, and the broken-ness outlined in TODOs in cockroachdb#51790. This PR just addresses those TODOs. Fixes cockroachdb#51497 Fixes cockroachdb#51721 Fixes cockroachdb#51738 Fixes cockroachdb#51768 Fixes cockroachdb#51769 Fixes cockroachdb#51776 Release note: None

51893: roachprod: fixup `roachprod --sequential` r=irfansharif a=irfansharif ..and the setting of cluster settings for single node clusters. `roachprod start --sequential` was broken in #51329, and the broken-ness outlined in TODOs in #51790. This PR just addresses those TODOs. Fixes #51497 Fixes #51721 Fixes #51738 Fixes #51768 Fixes #51769 Fixes #51776 Release note: None Co-authored-by: irfan sharif <[email protected]>

irfansharif requested review from knz and petermattis July 22, 2020 19:54

petermattis reviewed Jul 22, 2020

View reviewed changes

irfansharif force-pushed the 200722.roachprod-sequential branch 2 times, most recently from 44ff1c6 to 72c9b0f Compare July 22, 2020 21:15

irfansharif requested a review from jlinder July 22, 2020 21:34

irfansharif force-pushed the 200722.roachprod-sequential branch from 72c9b0f to 5d14709 Compare July 22, 2020 21:54

This was referenced Jul 23, 2020

release: 20.2.0-alpha.2 #51035

Closed

roachtest: gossip/chaos/nodes=9 failed #51721

Closed

irfansharif added 2 commits July 23, 2020 17:21

roachprod: improve comments around ServerNodes,Start

4cfa19f

This will come in handy when understanding future commits. Release note: None

irfansharif force-pushed the 200722.roachprod-sequential branch from 5d14709 to 64bb390 Compare July 23, 2020 21:56

nvanbenschoten approved these changes Jul 23, 2020

View reviewed changes

irfansharif added 8 commits July 23, 2020 18:13

roachprod: introduce a crdbStartHelper struct

d724fba

And add a few helper methods to start thinning down `roachprod start` code (this commit pulls out `useStartSingleNode`, `shouldAdvertisePublicIP`, `getEnvVars`, `distributeCerts`). Release note: None

roachprod: pull out generateKeyCmd

6edf032

The code here is a bit duplicated with the generation of `encryptArgs`, but in future commits we'll be separating out the generation of args provided to `cockroach start`, and this, though related, does not apply. Release note: None

roachprod: pull out generate{Init,ClusterSetting}Cmd

cd7e565

These helpers focus on generating the raw commands that are used when initializing a cluster, and setting cluster settings following. Release note: None

roachprod: pull out initializeCluster and setClusterSettings

220b987

These helpers actually initialize the cluster, and set cluster settings following. Release note: None

roachprod: pull out generateStartArgs

e457797

This helper generates arguments provided to `cockroach start`. We improve comments while here. Release note: None

roachprod: pull out generateStartCmd

dfec700

This helper, in the same style as `generateClusterSettingCmd` and `generateInitCmd`, generates the raw command used to start a given node. Release note: None

roachprod: pull out startNode

cedbab9

This helper is responsible for actually starting a given cockroach node. Release note: None

roachprod: add some TODOs around present broken-ness

e769049

This will be addressed in follow-on PRs. Release note: None

irfansharif force-pushed the 200722.roachprod-sequential branch from 64bb390 to e769049 Compare July 23, 2020 22:13

irfansharif commented Jul 23, 2020

View reviewed changes

craig bot merged commit c07859c into cockroachdb:master Jul 24, 2020

irfansharif deleted the 200722.roachprod-sequential branch July 24, 2020 07:22

irfansharif mentioned this pull request Jul 24, 2020

roachprod: fixup roachprod --sequential #51893

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachprod: rewrite `roachprod start` #51790

roachprod: rewrite `roachprod start` #51790

irfansharif commented Jul 22, 2020 •

edited

Loading

cockroach-teamcity commented Jul 22, 2020

irfansharif commented Jul 22, 2020

irfansharif commented Jul 22, 2020

petermattis left a comment

irfansharif commented Jul 22, 2020

knz commented Jul 23, 2020

nvanbenschoten commented Jul 23, 2020

irfansharif commented Jul 23, 2020

irfansharif commented Jul 23, 2020

nvanbenschoten left a comment

irfansharif left a comment

craig bot commented Jul 23, 2020

irfansharif commented Jul 24, 2020

craig bot commented Jul 24, 2020

craig bot commented Jul 24, 2020

roachprod: rewrite roachprod start #51790

roachprod: rewrite roachprod start #51790

Conversation

irfansharif commented Jul 22, 2020 • edited Loading

cockroach-teamcity commented Jul 22, 2020

irfansharif commented Jul 22, 2020

irfansharif commented Jul 22, 2020

petermattis left a comment

Choose a reason for hiding this comment

irfansharif commented Jul 22, 2020

knz commented Jul 23, 2020

nvanbenschoten commented Jul 23, 2020

irfansharif commented Jul 23, 2020

irfansharif commented Jul 23, 2020

nvanbenschoten left a comment

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

craig bot commented Jul 23, 2020

irfansharif commented Jul 24, 2020

craig bot commented Jul 24, 2020

craig bot commented Jul 24, 2020

roachprod: rewrite `roachprod start` #51790

roachprod: rewrite `roachprod start` #51790

irfansharif commented Jul 22, 2020 •

edited

Loading