backupccl: add feature flag support for BACKUP, RESTORE #56533

angelapwen · 2020-11-10T23:23:26Z

Follow-up to the RFC at #55778. This addresses the SRE use case mentioned in #51643 — instead of moving forward with a global denylist as the RFC indicated, we are prototyping feature flags via cluster settings to turn on/off requested features. The first part of the prototype will be done on BACKUP and RESTORE commands.

See this doc for further details.

Note that the logic test under ccl/backupccl/testdata/backup-restore/feature-flags can be tested with the command make test PKG=./pkg/ccl/backupccl TESTS='TestBackupRestoreDataDriven'

— Commit message below —

Adds a cluster setting to toggle a feature flag for the BACKUP and
RESTORE commands off and on. The feature is being introduced to
address a Cockroach Cloud SRE use case: needing to disable certain
categories of features in case of cluster failure.

Release note (enterprise change): Adds cluster settings to enable/
disable the BACKUP and RESTORE commands. If a user attempts to use
these features while they are disabled, an error indicating that
the database administrator has disabled the feature is surfaced.

Example usage for the database administrator:
SET CLUSTER SETTING feature.backup.enabled = FALSE;
SET CLUSTER SETTING feature.backup.enabled = TRUE;
SET CLUSTER SETTING feature.restore.enabled = FALSE;
SET CLUSTER SETTING feature.restore.enabled = TRUE;

cockroach-teamcity · 2020-11-10T23:23:33Z

This change is

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen and @pbardea)

pkg/sql/opt/memo/memo.go, line 134 at r1 (raw file):

	// The following are selected fields from SessionData which can affect
	// planning. We need to cross-check these before reusing a cached memo.
	// TODO(angelaw): Is it necesssary to include feature flags here?

We shouldn't need it here, these are only fields that can cause the same statement to be planned differently by the optimizer

angelapwen · 2020-11-10T23:58:08Z

pkg/sql/opt/memo/memo.go, line 134 at r1 (raw file):

Previously, RaduBerinde wrote…

We shouldn't need it here, these are only fields that can cause the same statement to be planned differently by the optimizer

Thanks @RaduBerinde , I'm making some changes now from comments @otan gave me and will update the PR as soon as I test!

angelapwen · 2020-11-11T00:05:19Z

Note: will not merge until I have fixed the release note category

pbardea

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @otan, and @pbardea)

pkg/ccl/backupccl/backup_planning.go, line 514 at r2 (raw file):

	if !featureBackupEnabled.Get(&p.ExecCfg().Settings.SV) {
		return nil, nil, nil, false,
			pgerror.Newf(pgcode.OperatorIntervention, "BACKUP feature was disabled by the database administrator.")

nit: I think that we typically format error messages without a "." at the end.

angelapwen

Will update the release note with the "enterprise change" category per @pbardea and also look to adding support for the RESTORE command here as well, considering the change was simpler than I thought.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @pbardea)

pkg/ccl/backupccl/backup_planning.go, line 514 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

nit: I think that we typically format error messages without a "." at the end.

Thank you! Will fix this.

angelapwen · 2020-11-11T23:31:42Z

Added support and testing for the RESTORE feature.

I have also added hierarchical cluster settings for these features, eg. feature.bulkio.backup.enabled and feature.bulkio.restore.enabled so that feature.bulkio.enabled will be a general category for both BACKUP and RESTORE.

The category name can of course be edited and all feedback is welcomed!

otan

LG -- i don't think we should worry about hierarchy yet (because of naming, and if we end up doing hierarchy we can simplify the logic a bit as well). just feature.backup.enabled would work for me!

otan · 2020-11-12T03:39:05Z

pkg/ccl/backupccl/backup_planning.go

+var featureBackupEnabled = settings.RegisterPublicBoolSetting(
+	"feature.bulkio.backup.enabled",
+	"set to true to enable backups, false to disable; default is true",
+	true /* enabled by default */)


nit: this should just be the varname, i.e. true /* defaultValue */,

even better if we can define a const FeatureFlagEnabledDefault = true somewhere -- not opposed to creating a pkg/featureflag for this const (for now)

Good idea! Do you ever see a case where that const is set to false? 🤔 Or is the idea of the constant just for clarity?

clarity -- see also "avoid naked parameters" in https://wiki.crdb.io/wiki/spaces/CRDB/pages/181371303/Go+coding+guidelines

angelapwen · 2020-11-12T07:32:15Z

LG -- i don't think we should worry about hierarchy yet (because of naming, and if we end up doing hierarchy we can simplify the logic a bit as well). just feature.backup.enabled would work for me!

Gotcha — I can keep just the feature.backup.enabled and feature.restore.enabled for this PR

Adds a cluster setting to toggle a feature flag for the BACKUP and RESTORE commands off and on. The feature is being introduced to address a Cockroach Cloud SRE use case: needing to disable certain categories of features in case of cluster failure. Release note (enterprise change): Adds cluster settings to enable/ disable the BACKUP and RESTORE commands. If a user attempts to use these features while they are disabled, an error indicating that the database administrator has disabled the feature is surfaced. Example usage for the database administrator: SET CLUSTER SETTING feature.backup.enabled = FALSE; SET CLUSTER SETTING feature.backup.enabled = TRUE; SET CLUSTER SETTING feature.restore.enabled = FALSE; SET CLUSTER SETTING feature.restore.enabled = TRUE;

angelapwen · 2020-11-12T20:06:35Z

bors r=otan

craig · 2020-11-12T21:07:50Z

Build failed (retrying...):

GitHub CI (Cockroach)

55325: ui: Transactions link between Sessions and Statements r=dhartunian a=elkmaster Resolves: #55131, #56244 Release note (admin ui change): Link to the Transactions page is now shown between the Sessions and Statements links in the left hand navigation. This more clearly reflects the hierarchy between the 3 concepts. 56346: testcluster: minor logging improvements r=andreimatei a=andreimatei Log when TestCluster quiescing starts, and add a node log tag to each node's quiescing ctx so messages from different nodes can be disambiguated. Release note: None 56437: cli, ui: dismiss release notes signup banner per environment variable r=knz,dhartunian a=nkodali Previously, the signup banner could only be dismissed manually. For internal testing purposes, this banner is unnecessary. This change provides a way to dismiss the signup banner upon start of a cluster via the cli by setting the environment variable COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED=true. Resolves #46998 Release note: none 56533: backupccl: add feature flag support for BACKUP, RESTORE r=otan a=angelapwen Follow-up to the RFC at #55778. This addresses the SRE use case mentioned in #51643 — instead of moving forward with a global denylist as the RFC indicated, we are prototyping feature flags via cluster settings to turn on/off requested features. The first part of the prototype will be done on `BACKUP` and `RESTORE` commands. See [this doc](https://docs.google.com/document/d/1nZSdcK7YprL0P4TAuseY-mvlYnd82IaJ_ptAQDoWB6o/edit?) for further details. Note that the logic test under `ccl/backupccl/testdata/backup-restore/feature-flags` can be tested with the command `make test PKG=./pkg/ccl/backupccl TESTS='TestBackupRestoreDataDriven'` — Commit message below — Adds a cluster setting to toggle a feature flag for the BACKUP and RESTORE commands off and on; as well as a broad category for Bulk IO commands. Currently disabling the cluster setting for Bulk IO will only disable BACKUP and RESTORE jobs, but other types may be included in this category in the future.. The feature is being introduced to address a Cockroach Cloud SRE use case: needing to disable certain categories of features in case of cluster failure. Release note (enterprise change): Adds cluster settings to enable/ disable the BACKUP and RESTORE commands. If a user attempts to use these features while they are disabled, an error indicating that the database administrator has disabled the feature is surfaced. Example usage for the database administrator: SET CLUSTER SETTING feature.bulkio.backup.enabled = FALSE; SET CLUSTER SETTING feature.bulkio.backup.enabled = TRUE; SET CLUSTER SETTING feature.bulkio.restore.enabled = FALSE; SET CLUSTER SETTING feature.bulkio.restore.enabled = TRUE; SET CLUSTER SETTING feature.bulkio.enabled = FALSE; SET CLUSTER SETTING feature.bulkio.enabled = TRUE; 56591: ui: fix Overview screen in OSS builds r=dhartunian a=davepacheco Previously, when using OSS builds (created with `make buildoss`), when you loading the DB Console in your browser, you'd get "Page Not Found". The route for the overview page was missing the leading '/'. This bug appears to have been introduced in 722c932. Release note (admin ui change): This fixes a bug where users of the OSS builds of CockroachDB would see "Page Not Found" when loading the Console. 56600: roachpb: remove SetInner in favor of MustSetInner r=nvanbenschoten a=tbg As of a recent commit, `ErrorDetail.SetInner` became unused, and we can switch to a `MustSetInner` pattern for `ErrorDetail`. Since the codegen involved is shared with {Request,Response}Union, those lose the `SetInner` setter as well; we were always asserting on the returned bool there anyway so this isn't changing anything. Release note: None Co-authored-by: Vlad Los <[email protected]> Co-authored-by: Andrei Matei <[email protected]> Co-authored-by: Namrata Kodali <[email protected]> Co-authored-by: angelapwen <[email protected]> Co-authored-by: Joshua M. Clulow <[email protected]> Co-authored-by: Tobias Grieger <[email protected]>

craig · 2020-11-12T23:00:58Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2020-11-13T02:25:20Z

Build succeeded:

GitHub CI (Cockroach)

angelapwen added the do-not-merge bors won't merge a PR with this label. label Nov 10, 2020

angelapwen requested a review from a team November 10, 2020 23:23

angelapwen requested a review from a team as a code owner November 10, 2020 23:23

angelapwen self-assigned this Nov 10, 2020

angelapwen requested review from pbardea and removed request for a team November 10, 2020 23:23

angelapwen force-pushed the add-backup-feature-flag branch 3 times, most recently from edb76a3 to dcd0e52 Compare November 10, 2020 23:38

RaduBerinde reviewed Nov 10, 2020

View reviewed changes

angelapwen force-pushed the add-backup-feature-flag branch 3 times, most recently from e9de358 to 6aa747e Compare November 11, 2020 00:04

angelapwen changed the title ~~[WIP] backupccl: add feature flag support for backup command~~ backupccl: add feature flag support for backup command Nov 11, 2020

angelapwen requested a review from otan November 11, 2020 00:04

angelapwen force-pushed the add-backup-feature-flag branch 3 times, most recently from b8626e3 to d16438f Compare November 11, 2020 19:47

pbardea approved these changes Nov 11, 2020

View reviewed changes

angelapwen commented Nov 11, 2020

View reviewed changes

angelapwen force-pushed the add-backup-feature-flag branch 2 times, most recently from 63fc0c6 to 6523b66 Compare November 11, 2020 23:29

angelapwen changed the title ~~backupccl: add feature flag support for backup command~~ backupccl: add feature flag support for BACKUP, RESTORE Nov 11, 2020

angelapwen removed the do-not-merge bors won't merge a PR with this label. label Nov 11, 2020

angelapwen force-pushed the add-backup-feature-flag branch from 6523b66 to 04699e2 Compare November 12, 2020 00:43

otan reviewed Nov 12, 2020

View reviewed changes

angelapwen force-pushed the add-backup-feature-flag branch from 04699e2 to 86d57b1 Compare November 12, 2020 18:45

blathers-crl bot requested a review from otan November 12, 2020 18:45

angelapwen force-pushed the add-backup-feature-flag branch from 86d57b1 to 2e2a44f Compare November 12, 2020 18:46

otan approved these changes Nov 12, 2020

View reviewed changes

angelapwen force-pushed the add-backup-feature-flag branch from 2e2a44f to 2a3f6ba Compare November 12, 2020 19:24

craig bot merged commit e640435 into cockroachdb:master Nov 13, 2020

jseldess mentioned this pull request Dec 9, 2020

backupccl: add feature flag support for BACKUP, RESTORE cockroachdb/docs#9084

Closed

angelapwen deleted the add-backup-feature-flag branch February 5, 2021 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backupccl: add feature flag support for BACKUP, RESTORE #56533

backupccl: add feature flag support for BACKUP, RESTORE #56533

angelapwen commented Nov 10, 2020 •

edited

Loading

cockroach-teamcity commented Nov 10, 2020

RaduBerinde left a comment

angelapwen commented Nov 10, 2020

angelapwen commented Nov 11, 2020

pbardea left a comment

angelapwen left a comment

angelapwen commented Nov 11, 2020 •

edited

Loading

otan left a comment

otan Nov 12, 2020

angelapwen Nov 12, 2020

otan Nov 12, 2020

angelapwen commented Nov 12, 2020

angelapwen commented Nov 12, 2020

craig bot commented Nov 12, 2020

craig bot commented Nov 12, 2020

craig bot commented Nov 13, 2020

backupccl: add feature flag support for BACKUP, RESTORE #56533

backupccl: add feature flag support for BACKUP, RESTORE #56533

Conversation

angelapwen commented Nov 10, 2020 • edited Loading

cockroach-teamcity commented Nov 10, 2020

RaduBerinde left a comment

Choose a reason for hiding this comment

angelapwen commented Nov 10, 2020

angelapwen commented Nov 11, 2020

pbardea left a comment

Choose a reason for hiding this comment

angelapwen left a comment

Choose a reason for hiding this comment

angelapwen commented Nov 11, 2020 • edited Loading

otan left a comment

Choose a reason for hiding this comment

otan Nov 12, 2020

Choose a reason for hiding this comment

angelapwen Nov 12, 2020

Choose a reason for hiding this comment

otan Nov 12, 2020

Choose a reason for hiding this comment

angelapwen commented Nov 12, 2020

angelapwen commented Nov 12, 2020

craig bot commented Nov 12, 2020

craig bot commented Nov 12, 2020

craig bot commented Nov 13, 2020

angelapwen commented Nov 10, 2020 •

edited

Loading

angelapwen commented Nov 11, 2020 •

edited

Loading