-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
roachtest: port multitenant-multiregion to new APIs #124795
roachtest: port multitenant-multiregion to new APIs #124795
Conversation
This commit ports the `multitenant-multiregion` roachtest to use the recently introduced roachprod API to manage virtual clusters. It also removes the previous brittle logic for log checking. Fixes: cockroachdb#124029 Release note: None
119a704
to
5f509a2
Compare
require.NoError(t, err) | ||
configStmts := []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want all of these cluster settings back in, but especially that first one:
sql.virtual_cluster.feature_access.multiregion.enabled='true'
Without that, I don't think it will let you create a multitenant + multiregion cluster
install.MakeClusterSettings(), | ||
) | ||
|
||
virtualClusterURLs := func() []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't change once the cluster is started, so this can just be a var virtualClusterURLs []string
instead of invoking a function to fetch it every time.
for i, node := range c.All() { | ||
region := regions[i] | ||
regionInfo := fmt.Sprintf("cloud=%s,region=%s,zone=%s", c.Cloud(), regionOnly(region), region) | ||
tenant := deprecatedCreateTenantNode(ctx, t, c, c.All(), tenantID, node, tenantHTTPPort, tenantSQLPort, createTenantRegion(regionInfo)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the old test was creating a tenant on every node whereas here you're just creating one tenant overall. I think we would want to call c.StartServiceForVirtualCluster
for every tenant?
mkStmt("SET CLUSTER SETTING sql.region_liveness.enabled='yes'"), | ||
) | ||
verifySQL(t, tenants[0].pgURL, | ||
verifySQL(t, virtualClusterURLs[0], | ||
mkStmt(fmt.Sprintf(`ALTER DATABASE system SET PRIMARY REGION '%s'`, regionOnly(regions[0]))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work because the tenant user doesn't have access to modify the system
db 🤔
This might not be an issue with your test set up? It should be creating the default user and granting it ADMIN
permissions, and in fact I see the comment in the logs saying: log into DB console for virtual cluster multiregion-tenant with user=roachprod password=cockroachdb
.
Would need to take a deeper look to see why it's not working. Maybe the pgurl
isn't correct?
Anyway, if it is unrelated you can pass in Auth: install.AuthRootCert
to c.ExternalPGUrl()
as a temporary unblocker, although in the end we should get this working with non root.
This commit ports the
acceptance/multitenant-multiregion
roachtest to use the recently introduced roachprod API to manage virtual clusters.It also removes the previous brittle logic for log checking.
Fixes: #124029
Release note: None