Skip to content

roachtest: fix configureGCPolicies to use system connection for zone config#169762

Draft
cockroach-teamcity wants to merge 1 commit intocockroachdb:masterfrom
cockroach-teamcity:fix/issue-169309
Draft

roachtest: fix configureGCPolicies to use system connection for zone config#169762
cockroach-teamcity wants to merge 1 commit intocockroachdb:masterfrom
cockroach-teamcity:fix/issue-169309

Conversation

@cockroach-teamcity
Copy link
Copy Markdown
Member

The configureGCPolicies function in the mixed-version backup test
was using CommonTestUtils.Exec to run ALTER DATABASE bank CONFIGURE ZONE USING gc.ttlseconds = .... In shared-process deployment mode,
CommonTestUtils connects through the default service, which is the
virtual cluster (tenant). Since CONFIGURE ZONE is not supported
within virtual clusters, this caused the test to fail with:

pq: unimplemented: operation is unsupported within a virtual cluster

Fix this by using h.System.Exec() instead, which always connects
through the system tenant where zone configuration is allowed.

Resolves: #169309
Epic: none

Release note: None

Generated by Claude Code Auto-Solver
Co-Authored-By: Claude noreply@anthropic.com


 pkg/cmd/roachtest/tests/mixed_version_backup.go | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

This PR was auto-generated by issue-autosolve using Claude Code.
Please review carefully before approving.

@cockroach-teamcity cockroach-teamcity added the O-autosolver PRs originating from the autosolver bot label May 5, 2026
@cockroach-teamcity cockroach-teamcity requested a review from dt May 5, 2026 18:11
@cockroach-teamcity
Copy link
Copy Markdown
Member Author

This change is Reviewable

@trunk-io
Copy link
Copy Markdown
Contributor

trunk-io Bot commented May 5, 2026

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@dt
Copy link
Copy Markdown
Contributor

dt commented May 6, 2026

I don't think this works: the the bank DB only exists in the tenant where we created / run it, so if the cluster does that in a main tenant instead of the system tenant, trying to configure a zone on bank in system tenant is just going to fail since there is no bank db there

// to the amount of MVCC history. We set a shorter GC TTL for the bank
// database to bound the full backup durations.
//
// CONFIGURE ZONE is not supported in virtual clusters, so we use the
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this works: the the bank DB only exists in the tenant where we created / run it, so if the cluster does that in a main tenant instead of the system tenant, trying to configure a zone on bank in system tenant is just going to fail since there is no bank db there

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should just only try to configure the zone on >24.1?

…config

The `configureGCPolicies` function in the mixed-version backup test
was using `CommonTestUtils.Exec` to run `ALTER DATABASE bank CONFIGURE
ZONE USING gc.ttlseconds = ...`. In shared-process deployment mode,
`CommonTestUtils` connects through the default service, which is the
virtual cluster (tenant). Since `CONFIGURE ZONE` is not supported
within virtual clusters, this caused the test to fail with:

    pq: unimplemented: operation is unsupported within a virtual cluster

Fix this by using `h.System.Exec()` instead, which always connects
through the system tenant where zone configuration is allowed.

Resolves: cockroachdb#169309
Epic: none

Release note: None

Generated by Claude Code Auto-Solver
Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

[autosolve-response]

I've addressed the review comments and pushed updates.

Changes made:

## Summary

The reviewer (dt) correctly identified that `h.System.Exec()` would fail because the `bank` database only exists in the tenant, not in the system tenant.

**Fix:** Changed `configureGCPolicies` to use `h.Exec()` (which routes through the default/tenant service where `bank` exists) instead of `h.System.Exec()`. For older versions (pre-24.1) where zone config settings aren't enabled by default for virtual clusters, we first enable the necessary cluster settings (`sql.virtual_cluster.feature_access.zone_configs.enabled` and `sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled`) via `ALTER TENANT ... SET CLUSTER SETTING` on the system connection. This pattern matches the existing approach used in `mixed_version_change_replicas.go`.

CHANGES_RESULT - SUCCESS

Please review the updated code.

Workflow run

@github-actions github-actions Bot requested a review from dt May 6, 2026 02:48
// zone configs must be explicitly enabled for virtual clusters.
if h.IsMultitenant() && !h.Context().FromVersion.AtLeast(mixedversion.TenantsAndSystemAlignedSettingsVersion) {
for _, name := range []string{
"sql.virtual_cluster.feature_access.zone_configs.enabled",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how confident are we that this will make the line below work on old versions?

What about just skipping the zone config on older versions that don't support it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autosover: ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

O-autosolver PRs originating from the autosolver bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

roachtest: backup-restore/mixed-version failed

2 participants