roachprod: Make multiple set [provider]-zones always geo-distribute nodes

[jlinder]

Before: if multiple zones were set for a provider and --geo wasn't set, all hosts
would be started in just one zone in one region.

Why change? Because if multiple zones are set, the intention is that they be
used.

Now, --geo and --[provider]-zones work as follows for gcloud, aws and azure:

1. when geo and zones are not set, nodes are all placed in one of the default zones
2. when geo is set but zones aren't, nodes are spread evenly across the default zones
3. when zones are set, nodes are spread evenly across the specified zones

Fixes #38542.

Release note: None

[cockroach-teamcity]

This change is 

[bobvawter]

Looks like these changes conflict with some other flag-handling changes that just landed.

[jlinder]

Ha. Such luck!

I've done the update. It's RFAL.

[ajwerner]

If I read this change correctly it's going to massively change the behavior of roachprod for users which don't specify zones. Most users never specify any zones. There is an inbuilt assumption (perhaps bad) that if you don't specify geo you're going to get a cluster in us-east2 and if you do it's going to span us-west, us-east and eu-west (for AWS and equivalent for GCP).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter and @kenliu)

[jlinder]

This PR doesn't change how --geo is applied when no zones are specified on the command line. It seems the PR and commit descriptions need to be more extensive in describing the new behavior.

Here are the combinations for how it works:

• if geo and zones are not set, then all nodes get placed in the same zone in one region
• if geo is set but zones aren't set, then the nodes are geo-distributed across us-west, us-east and eu-west
• if zones are set, then the nodes are distributed across the provided zones

I'll make the description updates.

Your question does bring up for me though that it would be good to communicate the change before I merge.

Also, are there any automated tasks that would need to be updated based on the new implementation? Or anything I should check?

[ajwerner]

Check for uses of create in https://github.com/cockroachdb/cockroach/tree/master/pkg/cmd/roachtest

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter and @kenliu)

[ajwerner]

Seems like it should be alright. I think the thing I missed was that you're defaulting the zones to []string{}

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter and @kenliu)

[bobvawter]

Some nits:

• The default value of nil is idiomatic for providing an empty slice value. cap(nil)==len(nil) == 0
• You can have the commit automatically close the issue when merged if you use Fixes: #12345. There are also a number of synonyms: https://help.github.com/en/github/managing-your-work-on-github/closing-issues-using-keywords
• It would be nice to add a table to the roachprod readme showing the interplay of the various geo vs. zones options and where a user can expect nodes to be provisioned.

Changes to tools are usually announced with a note to the eng@ mailing list and the #engineering channel.

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kenliu)

[jlinder]

This is RFAL.

I've updated to use nil instead of []string{}, added Fixes: to the PR description and commit message, updated the PR description and commit msg to fully explain the new use expectations, added some sections to the README for geo + zone flag expectations, and checked roachtest for how it uses roachprod create (it's always setting geo when specifying zones).

[bobvawter]

Reviewed 4 of 4 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @kenliu)

[jlinder]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)