Skip to content

Commit

Permalink
cli,server: explain in comment why tenant version must be initialized
Browse files Browse the repository at this point in the history
For the longest time, we had a comment that suggested we weren't sure
about it. We are now.

Release note: None
  • Loading branch information
knz committed Apr 5, 2023
1 parent ff2c4a3 commit b4f737c
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 17 deletions.
21 changes: 9 additions & 12 deletions pkg/cli/mt_start_sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,16 @@ func runStartSQL(cmd *cobra.Command, args []string) error {
if err := serverCfg.InitSQLServer(ctx); err != nil {
return err
}
// This value is injected in order to have something populated during startup.
// In the initial 20.2 release of multi-tenant clusters, no version state was
// ever populated in the version cluster setting. A value is populated during
// the activation of 21.1. See the documentation attached to the TenantCluster
// in upgrade/upgradecluster for more details on the tenant upgrade flow.
// Note that the value of 21.1 is populated when a tenant cluster is created
// during 21.1 in crdb_internal.create_tenant.
//
// Note that the tenant will read the value in the system.settings table
// before accepting SQL connections.

// We need a value in the version setting prior to the update
// coming from the system.settings table. This value must be valid
// and compatible with the state of the tenant's keyspace.
//
// TODO(knz): Check if this special initialization can be removed.
// See: https://github.com/cockroachdb/cockroach/issues/90831
// Since we don't know at which binary version the tenant
// keyspace was initialized, we must be conservative and
// assume it was created a long time ago; and that we may
// have to run all known migrations since then. So initialize
// the version setting to the minimum supported version.
st := serverCfg.BaseConfig.Settings
return clusterversion.Initialize(
ctx, st.Version.BinaryMinSupportedVersion(), &st.SV,
Expand Down
13 changes: 8 additions & 5 deletions pkg/server/server_controller_new_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,12 +193,15 @@ func makeSharedProcessTenantServerConfig(
) (baseCfg BaseConfig, sqlCfg SQLConfig, err error) {
st := cluster.MakeClusterSettings()

// This version initialization is copied from cli/mt_start_sql.go.
// We need a value in the version setting prior to the update
// coming from the system.settings table. This value must be valid
// and compatible with the state of the tenant's keyspace.
//
// TODO(knz): Why is this even useful? The comment refers to v21.1
// compatibility, yet if we don't do this, the server panics with
// "version not initialized". This might be related to:
// https://github.com/cockroachdb/cockroach/issues/84587
// Since we don't know at which binary version the tenant
// keyspace was initialized, we must be conservative and
// assume it was created a long time ago; and that we may
// have to run all known migrations since then. So initialize
// the version setting to the minimum supported version.
if err := clusterversion.Initialize(
ctx, st.Version.BinaryMinSupportedVersion(), &st.SV,
); err != nil {
Expand Down

0 comments on commit b4f737c

Please sign in to comment.