settings: delete StateMachineSetting, introduce VersionSetting

[irfansharif]

We introduced the custom StateMachineSetting type in #17216 to power the
underlying machinery for version upgrades:

SET CLUSTER SETTING version = <major>-<minor>;

At the time we left it generalizable enough to make room for future
settings with arbitrary internal transitions. For cluster versions this
meant only allowing transitions from one major version to the immediate
next, all the while making sure that each binary in the cluster was able
to activate the targeted version (the proposed version fell within the
allowable range ["binary min supported version", "binary version"]).

In the three years since we haven't added any state-machine validated
settings that fit the bill, and the layers of abstractions to get to
validated cluster version updates are one too many for (my) comfort.
This PR introduces a new VersionSetting type that is more tightly
coupled with the ClusterVersion type itself.

VersionSetting uses language that only appropriate for cluster versions,
hopefully clarifying things as we go. We'll depend on this clarification
in future PRs when we remove the use of gossip in disseminating cluster
version bumps.

Release note (sql/cli change): The underlying type for the version
cluster setting has been changed. Previously it was of an internal type
representing "state machine", but now it's simply "version". This has no
operational implications, but it does reflect differently in a few
spots:

• The Type column in cockroach gen settings-list will now show
  "version" instead of "custom validation"
• The setting_type column for version in SHOW CLUSTER SETTINGS
  will now show a "v" instead of an "m"
• The valueType column for version in system.settings will now
  show a "v" instead of an "m"

[cockroach-teamcity]

This change is 

[knz]

Reviewed 1 of 1 files at r1, 14 of 14 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @tbg)

---

pkg/settings/setting.go, line 220 at r2 (raw file):

	// called on the goroutine which handles all settings updates.
	SetOnChange(sv *Values, fn func())
	ErrorHint() (bool, string)

I applaud you for stepping out of your way to add these comments! Maybe ErrorHint() can get one too?

---

pkg/settings/settings_test.go, line 29 at r2 (raw file):

type dummyVersion struct {
	// Each dummyVersion has a msg1 prefix, and a growsbyone component that

Pull this comment above the struct.

[tbg]

Looks good. I also think you're on to some additional (deeper?) cleanup regarding the initialization of the setting and perhaps even the Opaque thing.

Reviewed 1 of 1 files at r1, 14 of 14 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)

---

pkg/settings/version.go, line 71 at r2 (raw file):

// cyclical dependency structure.
type ClusterVersionImpl interface {
	ClusterVersionImpl()

Do we still need Values.opaque after this change?

---

pkg/settings/version.go, line 79 at r2 (raw file):

// MakeVersionSetting instantiates a version setting instance. See
// VersionSetting for additional commentary.jhk

jhk

---

pkg/settings/version.go, line 186 at r2 (raw file):

// time.
//
// TODO(irfansharif): Is this true? Shouldn't the default here just the the

Yes, I think there is something there. We call .setToDefault in two places:

1. in SettingsValues.Init, at which point the cluster version is generally initialized
2. in the settings updater, which is even later

I think we could pass the default through the (*Settings).Values and use setToDefault here then. I.e. this code

cockroach/pkg/settings/cluster/cluster_settings.go

Lines 150 to 153 in 7bed111

	sv := &s.SV
	s.Version = clusterversion.MakeVersionHandleWithOverride(
	&s.SV, binaryVersion, binaryMinSupportedVersion)
	sv.Init(s.Version)

would do something like

ClusterVersionSetting.Override(&st.SV, "19.2-1")
sv.Init(handle)

Also look at this code, it's basically setting the version like any other setting, really looks like everything here should go away and the setting should look like any other:

cockroach/pkg/settings/cluster/settings.go

Lines 135 to 150 in 6ae15a7

	// InitializeVersion initializes the Version field of this setting. Before this
	// method has been called, usage of the Version field is illegal and leads to a
	// fatal error.
	func (s *Settings) InitializeVersion(cv ClusterVersion) error {
	b, err := protoutil.Marshal(&cv)
	if err != nil {
	return err
	}
	// Note that we don't call `updater.ResetRemaining()`.
	updater := settings.NewUpdater(&s.SV)
	if err := updater.Set(KeyVersionSetting, string(b), version.Typ()); err != nil {
	return err
	}
	s.Version.baseVersion.Store(&cv)
	return nil
	}

[irfansharif]

Thanks for reviewing! Like Tobi mentioned, this paves the way for more deep cleansing here. I have them sitting aside as separate commits that already touch the areas identified below (which is where to TODOs came from). I'll send those out in future PRs. We'll also be able to remove this whole SetBeforeChange callback business and shave the interfaces down even further.

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

---

pkg/settings/setting.go, line 220 at r2 (raw file):

Previously, knz (kena) wrote…

I applaud you for stepping out of your way to add these comments! Maybe ErrorHint() can get one too?

Done.

---

pkg/settings/settings_test.go, line 29 at r2 (raw file):

Previously, knz (kena) wrote…

Pull this comment above the struct.

Done.

---

pkg/settings/version.go, line 71 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Do we still need Values.opaque after this change?

Nope, see top-level comment.

---

pkg/settings/version.go, line 79 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

jhk

imap jhk <Esc>l

Done

---

pkg/settings/version.go, line 186 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Yes, I think there is something there. We call .setToDefault in two places:

1. in SettingsValues.Init, at which point the cluster version is generally initialized
2. in the settings updater, which is even later

I think we could pass the default through the (*Settings).Values and use setToDefault here then. I.e. this code

cockroach/pkg/settings/cluster/cluster_settings.go

Lines 150 to 153 in 7bed111

	sv := &s.SV
	s.Version = clusterversion.MakeVersionHandleWithOverride(
	&s.SV, binaryVersion, binaryMinSupportedVersion)
	sv.Init(s.Version)

would do something like

ClusterVersionSetting.Override(&st.SV, "19.2-1")
sv.Init(handle)

Also look at this code, it's basically setting the version like any other setting, really looks like everything here should go away and the setting should look like any other:

cockroach/pkg/settings/cluster/settings.go

Lines 135 to 150 in 6ae15a7

	// InitializeVersion initializes the Version field of this setting. Before this
	// method has been called, usage of the Version field is illegal and leads to a
	// fatal error.
	func (s *Settings) InitializeVersion(cv ClusterVersion) error {
	b, err := protoutil.Marshal(&cv)
	if err != nil {
	return err
	}
	// Note that we don't call `updater.ResetRemaining()`.
	updater := settings.NewUpdater(&s.SV)
	if err := updater.Set(KeyVersionSetting, string(b), version.Typ()); err != nil {
	return err
	}
	s.Version.baseVersion.Store(&cv)
	return nil
	}

See top-level comment, slated for future PRs that changes even more things about cluster settings.

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[irfansharif]

bors r-

[craig]

Canceled.

[irfansharif]

bors r+

[craig]

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[irfansharif]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)