roachtest: add ability to set default virtual cluster

[renatolabs]

This commit adds a SetDefaultVirtualCluster function to the cluster interface. This function's primary use case is to simulate the behaviour of the server.controller.default_target_cluster cluster setting, but for separate-process tenants. When this function is called, every subsequent call to Conn will connect to the provided virtual cluster (instead of system). Similary, expansions provided by roachprod (such as {pgurl}) will also use the new default virtual cluster. This functionality will enable the mixedversion framework to continue to provide the existing API when testing separate-process tenant upgrades. It could also be useful if roachtest itself wanted to provide this kind of randomized deployment model in each run.

Epic: none

Release note: None

[cockroach-teamcity]

This change is 

[renatolabs]

Requesting another review pass on this PR.

I wanted to change ExternalAdminUIAddr to be able to take an optional "virtual cluster" parameter (it would hardcode system previously). However, that turned out to hit a wall: we already have option.VirtualClusterName defined in the roachtest option package, but that function is defined as an option for c.Conn{,E}. Ideally, we would use the same option.VirtualClusterName API for the ExernalAdminUIAddr function as well.

A couple of approaches we could consider:

• have separate option functions for each cluster function: so we could have option.AdminAddrVirtualCluster, which is both awkward and error prone (why can't I use option.VirtualCluster?)
• give up on the "functional options" pattern and make virtualCluster a required parameter to ExternalAdminUIAddr. This could work, but it's inconvenient to have to pass install.SystemInterfaceName to every call of that function, even in cases where there's no multi tenancy at all.

I pushed a commit that uses some reflection to achieve a more ergonomic API: option.VirtualClusterName can now be used for both c.Conn and c.ExternalAdminUIAddr. Reflection always (rightfully) raises some eyebrows but I think in this case it's the lesser of the evils.

Open to thoughts and if anyone thinks this should be handled in a different way, I'm all ears.

[srosenberg]

However, that turned out to hit a wall: we already have option.VirtualClusterName defined in the roachtest option package, but that function is defined as an option for c.Conn{,E}.

Could we just pretend that ConnOption is a union type and pass it to ExternalAdminUIAddr? i.e.,

ExternalAdminUIAddr(ctx context.Context, l *logger.Logger, node option.NodeListOption, opts ...func(*option.ConnOption)) ([]string, error)

and call c.ExternalAdminUIAddr(ctx, l, nodes, option.VirtualClusterName("foo")). I have a feeling I'm missing something...

[renatolabs]

Couuld we just pretend that ConnOption is a union type and pass it to ExternalAdminUIAddr?

We could do something like this. I considered that option, but the reason I didn't like it as much is that then you give up on any kind of option validation -- one could pass ExternalAdminUIAddr(..., option.AuthOption(...)) and the code would compile, but the option would be silently ignored because it doesn't apply. Dead code would be undetectable and confusing. It would also be harder to tell what options are applicable in each function that takes these options (assuming we will over time desire to have more functions that take options, which I think is a likely possibility.)

[srosenberg]

We could do something like this. I considered that option, but the reason I didn't like it as much is that then you give up on any kind of option validation -- one could pass ExternalAdminUIAddr(..., option.AuthOption(...)) and the code would compile, but the option would be silently ignored because it doesn't apply. Dead code would be undetectable and confusing. It would also be harder to tell what options are applicable in each function that takes these options (assuming we will over time desire to have more functions that take options, which I think is a likely possibility.)

Yep, that's indeed the problem with "duck typing" :) Lack of subtypes is at the root of the problem here. I am not opposed to your workaround, but it didn't strike me as being particularly intuitive; on the first reading, it took > 5 mins to "unparse" everything. Maybe other folks could comment on the pattern of CustomOption; my main worry is mixing different APIs of passing options. @DarrylWong @herkolategan Thoughts?

Note that we could also stick with ConnOption and use reflection, but only to validate that the passed opts ...func(*option.ConnOption) doesn't set any fields outside of option.VirtualClusterOptions.

[DarrylWong]

I agree its a bit confusing but of the given solutions, CustomOptions approach seems fine to me.

Note that we could also stick with ConnOption and use reflection, but only to validate

I like this solution more, but would this fall apart if we ever need to use these options in a non connection context? i.e. if we add function that takes in DBName or VirtualClusterName but isn't a connection, then we're back to the same problem?

[srosenberg]

I like this solution more, but would this fall apart if we ever need to use these options in a non connection context? i.e. if we add function that takes in DBName or VirtualClusterName but isn't a connection, then we're back to the same problem?

Yep, here you can't overload VirtualClusterName constructor, so you're stuck with ConnOption, i.e.,

func VirtualClusterName(name string) func(*ConnOption) {
	return func(option *ConnOption) {
		option.VirtualClusterName = name
	}
}

Thus, anywhere you want to pass VirtualClusterName, you have to use ConnOption and validate. :(

[renatolabs]

use reflection, but only to validate that the passed opts

I pushed a variation of the initial approach to implement something like this suggestion (although I suspect it might not have been exactly what you had in mind) -- see last commit. The option definitions are a lot simpler and without the applyFunc business which is a big win. The validation function got a little more complex, but I don't think by much.

Preferences? I think I like this approach slightly more.

[srosenberg]

The option definitions are a lot simpler and without the applyFunc business which is a big win.

I concur, definitely a big win; the user-side is a lot more intuitive now!

The validation function got a little more complex, but I don't think by much.

Yep. Not the prettiest function, but it does what needs to be done :)

Preferences? I think I like this approach slightly more.

I'm definitely a lot more enthusiastic about this approach. Thanks more making it easier to grok for the mere-mortals like us! :)

[renatolabs]

Ran about 100 tests on this branch here, and nothing seems broken.

I don't think there's a high risk of this breaking existing behaviour, but I'll keep an eye out and we'll see how this performs in the runs over the weekend.

TFTRs

bors r=srosenberg,DarrylWong

[craig[bot]]

Build succeeded:

• macos_amd64_build
• lint
• windows_build
• local_roachtest_fips
• acceptance
• local_roachtest
• unit_tests
• docker_image_amd64
• examples_orms
• linux_amd64_build
• linux_arm64_build
• check_generated_code
• macos_arm64_build
• linux_amd64_fips_build