testutils: add `GrantTenantCapabilities` to test interfaces by shubhamdhama · Pull Request #141490 · cockroachdb/cockroach

shubhamdhama · 2025-02-14T11:49:32Z

Many tests require granting a capability when using external-process mode. Since this pattern is repeated in multiple places, it makes sense to provide it as a utility.

As part of this cleanup, I've also adjusted when it runs: it now applies only to external-process tenants, as shared-process tenants always have all capabilities.

Informs: #138912
Epic: CRDB-38970
Release note: None

cockroach-teamcity · 2025-02-14T11:49:49Z

This change is

stevendanna

This looks like a nice improvement. I do wonder if we want to go even further. We could add this to TestServerArgs and then handle them as part of this code:

cockroach/pkg/server/testserver.go

Lines 731 to 780 in 9598211

    
           func (ts *testServer) grantDefaultTenantCapabilities( 
        
           	ctx context.Context, tenantID roachpb.TenantID, skipTenantCheck bool, 
        
           ) error { 
        
           	ie := ts.InternalExecutor().(*sql.InternalExecutor) 
        
           	for _, setting := range []settings.Setting{ 
        
           		sql.SecondaryTenantScatterEnabled, 
        
           		sql.SecondaryTenantSplitAtEnabled, 
        
           		sqlclustersettings.SecondaryTenantZoneConfigsEnabled, 
        
           		sql.SecondaryTenantsMultiRegionAbstractionsEnabled, 
        
           	} { 
        
           		// Update the override for this setting. We need to do this 
        
           		// instead of calling .Override() on the setting directly: certain 
        
           		// tests expect to be able to change the value afterwards using 
        
           		// another ALTER VC SET CLUSTER SETTING statement, which is not 
        
           		// possible with regular overrides. 
        
           		_, err := ie.Exec(ctx, "testserver-alter-tenant-cap", nil, 
        
           			fmt.Sprintf("ALTER VIRTUAL CLUSTER [$1] SET CLUSTER SETTING %s = true", setting.Name()), tenantID.ToUint64()) 
        
           		if err != nil { 
        
           			if skipTenantCheck { 
        
           				log.Infof(ctx, "ignoring error changing setting because SkipTenantCheck is true: %v", err) 
        
           			} else { 
        
           				return err 
        
           			} 
        
           		} 
        
           	} 
        
           	// Waiting for capabilities can take time. To avoid paying this cost in all 
        
           	// cases, we only set the nodelocal storage capability if the caller has 
        
           	// configured an ExternalIODir since nodelocal storage only works with that 
        
           	// configured. 
        
           	shouldGrantNodelocalCap := ts.params.ExternalIODir != "" 
        
           	if shouldGrantNodelocalCap { 
        
           		_, err := ie.Exec(ctx, "testserver-alter-tenant-cap", nil, 
        
           			"ALTER TENANT [$1] GRANT CAPABILITY can_use_nodelocal_storage", tenantID.ToUint64()) 
        
           		if err != nil { 
        
           			if skipTenantCheck { 
        
           				log.Infof(ctx, "ignoring error granting capability because SkipTenantCheck is true: %v", err) 
        
           			} else { 
        
           				return err 
        
           			} 
        
           		} else { 
        
           			if err := ts.WaitForTenantCapabilities(ctx, tenantID, map[tenantcapabilities.ID]string{ 
        
           				tenantcapabilities.CanUseNodelocalStorage: "true", 
        
           			}, ""); err != nil { 
        
           				return err 
        
           			} 
        
           		} 
        
           	} 
        
           	return nil 
        
           }

To avoid waiting for capabilities longer than we have to. Don't let this suggestion get in the way of this improvement though, I think both functions are useful.

pkg/backup/backup_test.go

pkg/server/testserver.go

shubhamdhama · 2025-02-18T12:36:55Z

This looks like a nice improvement. I do wonder if we want to go even further. We could add this to TestServerArgs and then handle them as part of this code:

It's a great suggestion. I'm working on it as a follow-up.

cthumuluru-crdb

Except for one or two minor comments, this looks great!

pkg/server/storage_api/nodes_test.go

pkg/server/testserver.go

pkg/testutils/testcluster/testcluster.go

pkg/backup/backup_test.go

cthumuluru-crdb

Except for one or two minor comments, this looks great!

pkg/testutils/testcluster/testcluster.go

Many tests require granting a capability when using external-process mode. Since this pattern is repeated in multiple places, it makes sense to provide it as a utility. As part of this cleanup, I've also adjusted when it runs: it now applies only to external-process tenants, as shared-process tenants always have all capabilities. Informs: cockroachdb#138912 Epic: CRDB-38970 Release note: None

shubhamdhama · 2025-02-21T12:01:09Z

TFTRs!

bors r=stevendanna,cthumuluru-crdb

craig · 2025-02-21T12:29:35Z

Build succeeded:

(Stacked on cockroachdb#141490.) This PR continues the work from cockroachdb#140447, replacing occurrences of `createTestServerParams` with `createTestServerParamsAllowTenants` to enable tenant testing in these tests. Informs: cockroachdb#140446 Epic: CRDB-38970 Release note: None

shubhamdhama requested review from a team as code owners February 14, 2025 11:49

shubhamdhama requested review from DrewKimball, angles-n-daemons, asg0451, golgeek, kev-cao and srosenberg and removed request for a team February 14, 2025 11:49

shubhamdhama requested review from cthumuluru-crdb and stevendanna February 14, 2025 12:04

stevendanna reviewed Feb 17, 2025

View reviewed changes

pkg/backup/backup_test.go Outdated Show resolved Hide resolved

pkg/server/testserver.go Outdated Show resolved Hide resolved

shubhamdhama mentioned this pull request Feb 18, 2025

sql: increase tenant testing coverage #141604

Merged

shubhamdhama force-pushed the add-grant-capability-utility branch from 33fd527 to 3fed02c Compare February 18, 2025 08:18

shubhamdhama requested a review from stevendanna February 18, 2025 10:10

shubhamdhama mentioned this pull request Feb 20, 2025

multitenant: move tenantcapabilities.IDs to tenantcapabilitiespb #141774

Merged

stevendanna approved these changes Feb 20, 2025

View reviewed changes

cthumuluru-crdb approved these changes Feb 20, 2025

View reviewed changes

pkg/server/storage_api/nodes_test.go Outdated Show resolved Hide resolved

pkg/server/testserver.go Outdated Show resolved Hide resolved

pkg/testutils/testcluster/testcluster.go Outdated Show resolved Hide resolved

pkg/backup/backup_test.go Outdated Show resolved Hide resolved

cthumuluru-crdb approved these changes Feb 20, 2025

View reviewed changes

asg0451 removed their request for review February 20, 2025 20:31

shubhamdhama commented Feb 21, 2025

View reviewed changes

pkg/testutils/testcluster/testcluster.go Outdated Show resolved Hide resolved

shubhamdhama force-pushed the add-grant-capability-utility branch from 3fed02c to 2ea4166 Compare February 21, 2025 08:27

craig bot merged commit fbcabe0 into cockroachdb:master Feb 21, 2025
24 checks passed

celeste-cockroachdb bot added the target-release-25.2.0 label Feb 21, 2025

shubhamdhama mentioned this pull request Feb 21, 2025

server: Implement 'gossip' endpoint in multitenant setup #141212

Merged

shubhamdhama changed the title ~~testutils: add GrantTenantCapability to test interfaces~~ testutils: add GrantTenantCapabilities to test interfaces Feb 21, 2025

shubhamdhama deleted the add-grant-capability-utility branch February 21, 2025 12:54

celeste-cockroachdb bot added v25.2.0-prerelease and removed target-release-25.2.0 labels Mar 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testutils: add `GrantTenantCapabilities` to test interfaces#141490

testutils: add `GrantTenantCapabilities` to test interfaces#141490
craig[bot] merged 1 commit intocockroachdb:masterfrom
shubhamdhama:add-grant-capability-utility

shubhamdhama commented Feb 14, 2025

Uh oh!

cockroach-teamcity commented Feb 14, 2025

Uh oh!

stevendanna left a comment

Uh oh!

Uh oh!

Uh oh!

shubhamdhama commented Feb 18, 2025

Uh oh!

cthumuluru-crdb left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cthumuluru-crdb left a comment

Uh oh!

Uh oh!

shubhamdhama commented Feb 21, 2025

Uh oh!

craig bot commented Feb 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	func (ts *testServer) grantDefaultTenantCapabilities(
	ctx context.Context, tenantID roachpb.TenantID, skipTenantCheck bool,
	) error {
	ie := ts.InternalExecutor().(*sql.InternalExecutor)
	for _, setting := range []settings.Setting{
	sql.SecondaryTenantScatterEnabled,
	sql.SecondaryTenantSplitAtEnabled,
	sqlclustersettings.SecondaryTenantZoneConfigsEnabled,
	sql.SecondaryTenantsMultiRegionAbstractionsEnabled,
	} {
	// Update the override for this setting. We need to do this
	// instead of calling .Override() on the setting directly: certain
	// tests expect to be able to change the value afterwards using
	// another ALTER VC SET CLUSTER SETTING statement, which is not
	// possible with regular overrides.
	_, err := ie.Exec(ctx, "testserver-alter-tenant-cap", nil,
	fmt.Sprintf("ALTER VIRTUAL CLUSTER [$1] SET CLUSTER SETTING %s = true", setting.Name()), tenantID.ToUint64())
	if err != nil {
	if skipTenantCheck {
	log.Infof(ctx, "ignoring error changing setting because SkipTenantCheck is true: %v", err)
	} else {
	return err
	}
	}
	}

	// Waiting for capabilities can take time. To avoid paying this cost in all
	// cases, we only set the nodelocal storage capability if the caller has
	// configured an ExternalIODir since nodelocal storage only works with that
	// configured.
	shouldGrantNodelocalCap := ts.params.ExternalIODir != ""
	if shouldGrantNodelocalCap {
	_, err := ie.Exec(ctx, "testserver-alter-tenant-cap", nil,
	"ALTER TENANT [$1] GRANT CAPABILITY can_use_nodelocal_storage", tenantID.ToUint64())
	if err != nil {
	if skipTenantCheck {
	log.Infof(ctx, "ignoring error granting capability because SkipTenantCheck is true: %v", err)
	} else {
	return err
	}
	} else {
	if err := ts.WaitForTenantCapabilities(ctx, tenantID, map[tenantcapabilities.ID]string{
	tenantcapabilities.CanUseNodelocalStorage: "true",
	}, ""); err != nil {
	return err
	}
	}
	}
	return nil
	}

Conversation

shubhamdhama commented Feb 14, 2025

Uh oh!

cockroach-teamcity commented Feb 14, 2025

Uh oh!

stevendanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shubhamdhama commented Feb 18, 2025

Uh oh!

cthumuluru-crdb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cthumuluru-crdb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shubhamdhama commented Feb 21, 2025

Uh oh!

craig bot commented Feb 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants