settings: assert that SystemOnly settings are not accessed in virtual clusters

[knz]

Fixes #91825.
Epic: CRDB-6671
First commit from #110947

TLDR: tests will now fail (with a panic) if code running in virtual clusters mistakenly attempts to access a SystemOnly cluster setting.

---

Prior to this patch, it was possible for code (e.g. in SQL) running inside a virtual cluster to access a SystemOnly setting. This resulted in silently incorrect behavior: the default value of the setting would be observed always, without any relationship to values explicitly set in the storage layer.

When the setting mechanisms for virtual clusters had been implemented orignally, this concern was known. We even had implemented a mechanism on settings.Values (SetNonSystemTenant()) by which we could cause further .Get() calls to fail with an error in tests if accessing a SystemOnly setting from a virtual cluster.

Sadly, this mechanism was never hooked up anywhere.

This patch closes that loop by annotating the settings.Values properly during server initialization.

Since the property is stored inside the settings.Values, this means it is not any more possible to share a single settings.Values instance (nor cluster.Settings) between the system tenant and a secondary tenant.

---

The following bugs were fixed as a result of this change:

• certain settings didn't have the right class, including:

  • kv.bulk_io_write.concurrent_export_requests
  • kv.closed_timestamp.follower_reads.enabled
  • kv.range_split.by_load_merge_delay
  • kv.rangefeed.enabled
  • sql.hash_sharded_range_pre_split.max
  • storage.max_sync_duration.fatal.enabled
  • storage.value_blocks.enabled
  • ui.display_timezone
  • kv.bulk_sst.target_size

• the API endpoint that lists cluster settings was incorrectly
  trying to retrieve SystemOnly settings when accessed
  from a virtual cluster.

[cockroach-teamcity]

This change is 

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @stevendanna, and @yuzefovich)

---

pkg/server/testserver.go line 600 at r10 (raw file):

Or perhaps do that only when / if such need arises?

I like that better too.

---

pkg/settings/values.go line 251 at r10 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

We just discussed this - we need this to propagate.
Alas the other work in #110758 is only going to work with persisted changes (they will be picked up by rangefed). So we need a propagate here for the .Override changes.

Done.

[yuzefovich]

Last commit

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt, @knz, and @stevendanna)

---

pkg/settings/values.go line 251 at r10 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Done.

nit: update the comment too to mention RO.

---

pkg/storage/pebble.go line 83 at r14 (raw file):

// MaxSyncDuration fatal the Cockroach process. Defaults to true.
var MaxSyncDurationFatalOnExceeded = settings.RegisterBoolSetting(
	settings.TenantWritable, // used for temp storage in virtual cluster servers.

nit: usually for inline comments like this we start without a capital and do not have a period in the end.

[knz]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt, @stevendanna, and @yuzefovich)

---

pkg/settings/values.go line 251 at r10 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: update the comment too to mention RO.

Done.

---

pkg/storage/pebble.go line 83 at r14 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: usually for inline comments like this we start without a capital and do not have a period in the end.

Fixed.

[knz]

bors r=yuzefovich

[yuzefovich]

Second commit should be removed, right?

bors r-

[craig]

Canceled.

[knz]

Second commit should be removed, right?

🤦

Done

bors r=yuzefovich

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)