Skip to content

Commit

Permalink
Merge #123084
Browse files Browse the repository at this point in the history 
123084: upgrade: automatically bump the system DB version r=rafiss a=rafiss

Rather than making authors of an upgrade remember that they should bump
the version of the system DB schema, now it will be bumped automatically
after any upgrade step that has a migration or if upgrading to the final
cluster version for a release. Two tests are added:

- TestSystemDatabaseSchemaBootstrapVersionBumped makes sure the
  SystemDatabaseSchemaBootstrapVersion is incremented whenever
  clusterversion.Latest changes AND if that version has an upgrade
  migration or is the final version in a release.
- TestUpgradeHappensAfterMigrations is augmented to check that after all
  upgrades run, the actual system database schema version is equal to
  the SystemDatabaseSchemaBootstrapVersion.

Taken together, these tests ensure that the system database bootstrap
version is manually incremented in the code when it should be, and that
the automatic upgrade process correctly increments the version.

This change results in more work, since it means that the version will
be bumped when it is not strictly necessary to do so. But since it
makes it harder to forget a manual step, it seems worth doing.

fixes #121914
Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
  • Loading branch information
@rafiss
craig[bot] and rafiss committed May 3, 2024
2 parents b1d7489 + 614ce3e commit 6ff7eca
Show file tree
Hide file tree
Showing 25 changed files with 166 additions and 95 deletions.
30 changes: 10 additions & 20 deletions pkg/clusterversion/cockroach_versions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,28 +197,13 @@ const (
// options lagging_ranges_threshold and lagging_ranges_polling_interval.
TODODelete_V23_2_ChangefeedLaggingRangesOpts

// ***************************************************************************
// WHERE TO ADD VERSION GATES DURING 23.2 STABILITY?
// ---------------------------------------------------------------------------
// If version gate is for 23.2 (to be backported to release-23.2):
// Then add new gate above this comment (immediately above this comment).
// If version gate is for 24.1 (upcoming 24.1 development):
// Then add new gate at the end (immediately above the "Add new versions
// here" comment).
// ***************************************************************************

// V23_2 is CockroachDB v23.2. It's used for all v23.2.x patch releases.
V23_2

// V24_1Start demarcates the start of cluster versions stepped through during
// the process of upgrading from 23.2 to 24.1.
V24_1Start

// *************************************************
// Step (1) Add new versions here.
// Do not add new versions to a patch release.
// *************************************************

// V24_1_DropPayloadAndProgressFromSystemJobsTable drop the unused payload and
// progress columns from system.jobs table.
V24_1_DropPayloadAndProgressFromSystemJobsTable
Expand Down Expand Up @@ -268,6 +253,11 @@ const (
// the system tenant to ensure it is a superset of secondary tenants.
V24_1_AddSpanCounts

// *************************************************
// Step (1) Add new versions above this comment.
// Do not add new versions to a patch release.
// *************************************************

numKeys
)

Expand Down Expand Up @@ -308,11 +298,6 @@ var versionTable = [numKeys]roachpb.Version{
// v24.1 versions. Internal versions must be even.
V24_1Start: {Major: 23, Minor: 2, Internal: 2},

// *************************************************
// Step (2): Add new versions here.
// Do not add new versions to a patch release.
// *************************************************

V24_1_DropPayloadAndProgressFromSystemJobsTable: {Major: 23, Minor: 2, Internal: 4},

V24_1_MigrateOldStylePTSRecords: {Major: 23, Minor: 2, Internal: 6},
Expand All @@ -327,6 +312,11 @@ var versionTable = [numKeys]roachpb.Version{
V24_1_EstimatedMVCCStatsInSplit: {Major: 23, Minor: 2, Internal: 22},
V24_1_ReplicatedLockPipelining: {Major: 23, Minor: 2, Internal: 24},
V24_1_AddSpanCounts: {Major: 23, Minor: 2, Internal: 26},

// *************************************************
// Step (2): Add new versions above this comment.
// Do not add new versions to a patch release.
// *************************************************
}

// Latest is always the highest version key. This is the maximum logical cluster
Expand Down
2 changes: 2 additions & 0 deletions pkg/server/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,8 @@ go_test(
"//pkg/spanconfig",
"//pkg/sql",
"//pkg/sql/appstatspb",
"//pkg/sql/catalog/descs",
"//pkg/sql/catalog/systemschema",
"//pkg/sql/execinfrapb",
"//pkg/sql/isql",
"//pkg/sql/roleoption",
Expand Down
48 changes: 47 additions & 1 deletion pkg/server/migration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@ import (

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/server/serverpb"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
"github.com/cockroachdb/cockroach/pkg/storage"
"github.com/cockroachdb/cockroach/pkg/storage/fs"
"github.com/cockroachdb/cockroach/pkg/testutils"
Expand Down Expand Up @@ -318,7 +321,9 @@ func TestMigrationPurgeOutdatedReplicas(t *testing.T) {

// TestUpgradeHappensAfterMigration is a regression test to ensure that
// upgrades run prior to attempting to upgrade the cluster to the current
// version.
// version. It will also verify that any migrations that modify the system
// database schema properly update the SystemDatabaseSchemaVersion on the
// system database descriptor.
func TestUpgradeHappensAfterMigrations(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
Expand Down Expand Up @@ -349,6 +354,27 @@ func TestUpgradeHappensAfterMigrations(t *testing.T) {
},
},
})

internalDB := s.ApplicationLayer().InternalDB().(descs.DB)
err := internalDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error {
systemDBDesc, err := txn.Descriptors().ByID(txn.KV()).Get().Database(ctx, keys.SystemDatabaseID)
if err != nil {
return err
}
systemDBVersion := systemDBDesc.DatabaseDesc().GetSystemDatabaseSchemaVersion()
// NB: When MinSupported is changed to 24_2, this check can change to
// an equality check. This is because 24.2 is the first release where
// https://github.com/cockroachdb/cockroach/issues/121914 has been
// resolved.
require.False(
t, clusterversion.MinSupported.Version().Less(*systemDBVersion),
"before upgrade, expected system database version (%v) to be less than or equal to clusterversion.MinSupported (%v)",
*systemDBVersion, clusterversion.MinSupported.Version(),
)
return nil
})
require.NoError(t, err)

close(automaticUpgrade)
sr := sqlutils.MakeSQLRunner(db)

Expand All @@ -359,5 +385,25 @@ func TestUpgradeHappensAfterMigrations(t *testing.T) {
SELECT version = crdb_internal.node_executable_version()
FROM [SHOW CLUSTER SETTING version]`,
[][]string{{"true"}})

// After the upgrade, make sure the new system database version is equal to
// the SystemDatabaseSchemaBootstrapVersion. This serves two purposes:
// - reminder to bump SystemDatabaseSchemaBootstrapVersion.
// - ensure that upgrades have run and updated the system database version.
err = internalDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error {
systemDBDesc, err := txn.Descriptors().ByID(txn.KV()).Get().Database(ctx, keys.SystemDatabaseID)
if err != nil {
return err
}
systemDBVersion := systemDBDesc.DatabaseDesc().GetSystemDatabaseSchemaVersion()
require.Equalf(
t, systemschema.SystemDatabaseSchemaBootstrapVersion, *systemDBVersion,
"after upgrade, expected system database version (%v) to be equal to systemschema.SystemDatabaseSchemaBootstrapVersion (%v)",
*systemDBVersion, systemschema.SystemDatabaseSchemaBootstrapVersion,
)
return nil
})
require.NoError(t, err)

s.Stopper().Stop(context.Background())
}
1 change: 0 additions & 1 deletion pkg/sql/catalog/bootstrap/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ go_test(
"//pkg/security/securityassets",
"//pkg/security/securitytest",
"//pkg/server",
"//pkg/sql/catalog/systemschema",
"//pkg/testutils",
"//pkg/testutils/datapathutils",
"//pkg/testutils/serverutils",
Expand Down
28 changes: 0 additions & 28 deletions pkg/sql/catalog/bootstrap/bootstrap_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/config/zonepb"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
Expand Down Expand Up @@ -140,30 +139,3 @@ func makeMetadataSchema(tenantID uint64) MetadataSchema {
}
return MakeMetadataSchema(codec, zonepb.DefaultZoneConfigRef(), zonepb.DefaultSystemZoneConfigRef())
}

// TestSystemDatabaseSchemaBootstrapVersionBumped serves as a reminder to bump
// systemschema.SystemDatabaseSchemaBootstrapVersion whenever a new upgrade
// creates or modifies the schema of system tables. We unfortunately cannot
// programmatically determine if an upgrade should bump the version so by
// adding a test failure when the initial values change, the programmer and
// code reviewers are reminded to manually check whether the version should
// be bumped.
func TestSystemDatabaseSchemaBootstrapVersionBumped(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

// If you need to update this value (i.e. failed this test), check whether
// you need to bump systemschema.SystemDatabaseSchemaBootstrapVersion too.
const prevSystemHash = "cc22bc73bd41333a8706272d157bb1760d34c9dbbda112856a44f950637aacdb"
_, curSystemHash := GetAndHashInitialValuesToString(0 /* tenantID */)

if prevSystemHash != curSystemHash {
t.Fatalf(
`Check whether you need to bump systemschema.SystemDatabaseSchemaBootstrapVersion
and then update prevSystemHash to %q.
The current value of SystemDatabaseSchemaBootstrapVersion is %s.`,
curSystemHash,
systemschema.SystemDatabaseSchemaBootstrapVersion,
)
}
}
8 changes: 4 additions & 4 deletions pkg/sql/catalog/bootstrap/testdata/testdata
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
system hash=cc22bc73bd41333a8706272d157bb1760d34c9dbbda112856a44f950637aacdb
system hash=17ebb12f8ac77327e6ab3af4dba91106b240ac9b2c889ac9f23f69b85539ca3c
----
[{"key":"8b"}
,{"key":"8b89898a89","value":"0312450a0673797374656d10011a250a0d0a0561646d696e1080101880100a0c0a04726f6f7410801018801012046e6f646518032200280140004a006a0a08d7843d10021800200e"}
,{"key":"8b89898a89","value":"0312450a0673797374656d10011a250a0d0a0561646d696e1080101880100a0c0a04726f6f7410801018801012046e6f646518032200280140004a006a0a08d7843d10021800201a"}
,{"key":"8b898b8a89","value":"030a8e030a0a64657363726970746f721803200128013a0042270a02696410011a0c08011040180030005014600020003000680070007800800100880100980100422f0a0a64657363726970746f7210021a0c08081000180030005011600020013000680070007800800100880100980100480352710a077072696d61727910011801220269642a0a64657363726970746f72300140004a10080010001a00200028003000380040005a0070027a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060026a210a0b0a0561646d696e102018200a0a0a04726f6f741020182012046e6f64651803800101880103980100b201130a077072696d61727910001a02696420012800b201240a1066616d5f325f64657363726970746f7210021a0a64657363726970746f7220022802b80103c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880302a80300b00300d00300d80300e00300"}
,{"key":"8b898c8a89","value":"030ac7050a0575736572731804200128013a00422d0a08757365726e616d6510011a0c0807100018003000501960002000300068007000780080010088010098010042330a0e68617368656450617373776f726410021a0c0808100018003000501160002001300068007000780080010088010098010042320a066973526f6c6510031a0c08001000180030005010600020002a0566616c73653000680070007800800100880100980100422c0a07757365725f696410041a0c080c100018003000501a60002000300068007000780080010088010098010048055290010a077072696d617279100118012208757365726e616d652a0e68617368656450617373776f72642a066973526f6c652a07757365725f6964300140004a10080010001a00200028003000380040005a007002700370047a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00102e00100e90100000000000000005a740a1175736572735f757365725f69645f696478100218012207757365725f69643004380140004a10080010001a00200028003000380040005a007a0408002000800100880100900103980100a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060036a250a0d0a0561646d696e10e00318e0030a0c0a04726f6f7410e00318e00312046e6f64651803800101880103980100b201240a077072696d61727910001a08757365726e616d651a07757365725f6964200120042804b2012c0a1466616d5f325f68617368656450617373776f726410021a0e68617368656450617373776f726420022802b2011c0a0c66616d5f335f6973526f6c6510031a066973526f6c6520032803b80104c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880303a80300b00300d00300d80300e00300"}
,{"key":"8b898d8a89","value":"030afd020a057a6f6e65731805200128013a0042270a02696410011a0c08011040180030005014600020003000680070007800800100880100980100422b0a06636f6e66696710021a0c080810001800300050116000200130006800700078008001008801009801004803526d0a077072696d61727910011801220269642a06636f6e666967300140004a10080010001a00200028003000380040005a0070027a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060026a250a0d0a0561646d696e10e00318e0030a0c0a04726f6f7410e00318e00312046e6f64651803800101880103980100b201130a077072696d61727910001a02696420012800b2011c0a0c66616d5f325f636f6e66696710021a06636f6e66696720022802b80103c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880302a80300b00300d00300d80300e00300"}
Expand Down Expand Up @@ -194,10 +194,10 @@ system hash=cc22bc73bd41333a8706272d157bb1760d34c9dbbda112856a44f950637aacdb
,{"key":"ca"}
]

tenant hash=f20ed7fb713c784083c3fdf946f6e794f6a74a05a91e5804518775017314c358
tenant hash=064f1382f676795b02b278e4fd0d7d3b64954095ba874eca71470be88fad4715
----
[{"key":""}
,{"key":"8b89898a89","value":"0312450a0673797374656d10011a250a0d0a0561646d696e1080101880100a0c0a04726f6f7410801018801012046e6f646518032200280140004a006a0a08d7843d10021800200e"}
,{"key":"8b89898a89","value":"0312450a0673797374656d10011a250a0d0a0561646d696e1080101880100a0c0a04726f6f7410801018801012046e6f646518032200280140004a006a0a08d7843d10021800201a"}
,{"key":"8b898b8a89","value":"030a8e030a0a64657363726970746f721803200128013a0042270a02696410011a0c08011040180030005014600020003000680070007800800100880100980100422f0a0a64657363726970746f7210021a0c08081000180030005011600020013000680070007800800100880100980100480352710a077072696d61727910011801220269642a0a64657363726970746f72300140004a10080010001a00200028003000380040005a0070027a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060026a210a0b0a0561646d696e102018200a0a0a04726f6f741020182012046e6f64651803800101880103980100b201130a077072696d61727910001a02696420012800b201240a1066616d5f325f64657363726970746f7210021a0a64657363726970746f7220022802b80103c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880302a80300b00300d00300d80300e00300"}
,{"key":"8b898c8a89","value":"030ac7050a0575736572731804200128013a00422d0a08757365726e616d6510011a0c0807100018003000501960002000300068007000780080010088010098010042330a0e68617368656450617373776f726410021a0c0808100018003000501160002001300068007000780080010088010098010042320a066973526f6c6510031a0c08001000180030005010600020002a0566616c73653000680070007800800100880100980100422c0a07757365725f696410041a0c080c100018003000501a60002000300068007000780080010088010098010048055290010a077072696d617279100118012208757365726e616d652a0e68617368656450617373776f72642a066973526f6c652a07757365725f6964300140004a10080010001a00200028003000380040005a007002700370047a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00102e00100e90100000000000000005a740a1175736572735f757365725f69645f696478100218012207757365725f69643004380140004a10080010001a00200028003000380040005a007a0408002000800100880100900103980100a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060036a250a0d0a0561646d696e10e00318e0030a0c0a04726f6f7410e00318e00312046e6f64651803800101880103980100b201240a077072696d61727910001a08757365726e616d651a07757365725f6964200120042804b2012c0a1466616d5f325f68617368656450617373776f726410021a0e68617368656450617373776f726420022802b2011c0a0c66616d5f335f6973526f6c6510031a066973526f6c6520032803b80104c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880303a80300b00300d00300d80300e00300"}
,{"key":"8b898d8a89","value":"030afd020a057a6f6e65731805200128013a0042270a02696410011a0c08011040180030005014600020003000680070007800800100880100980100422b0a06636f6e66696710021a0c080810001800300050116000200130006800700078008001008801009801004803526d0a077072696d61727910011801220269642a06636f6e666967300140004a10080010001a00200028003000380040005a0070027a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060026a250a0d0a0561646d696e10e00318e0030a0c0a04726f6f7410e00318e00312046e6f64651803800101880103980100b201130a077072696d61727910001a02696420012800b2011c0a0c66616d5f325f636f6e66696710021a06636f6e66696720022802b80103c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880302a80300b00300d00300d80300e00300"}
Expand Down
3 changes: 2 additions & 1 deletion pkg/sql/catalog/systemschema/system.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1208,7 +1208,8 @@ const SystemDatabaseName = catconstants.SystemDatabaseName
// SystemDatabaseSchemaBootstrapVersion is the system database schema version
// that should be used during bootstrap. It should be bumped up alongside any
// upgrade that creates or modifies the schema of a system table.
var SystemDatabaseSchemaBootstrapVersion = clusterversion.V24_1_SessionBasedLeasingUpgradeDescriptor.Version()
// NB: Don't set this to clusterversion.Latest; use a specific version instead.
var SystemDatabaseSchemaBootstrapVersion = clusterversion.V24_1_AddSpanCounts.Version()

// MakeSystemDatabaseDesc constructs a copy of the system database
// descriptor.
Expand Down
Loading

0 comments on commit 6ff7eca

Please sign in to comment.