server,settingswatcher: fix the local persisted cache

[knz]

There's two commits here, fixing 2 separate issues.
Epic: CRDB-6671

server,settingswatcher: properly evict entries from the local persisted cache

Fixes #70567.
Supersedes #101472.

(For context, on each node there is a local persisted cache of cluster
setting customizations. This exists to ensure that configured values
can be used even before a node has fully started up and can start
reading customizations from system.settings.)

Prior to this patch, entries were never evicted from the local
persisted cache: when a cluster setting was reset, any previously
saved entry in the cache would remain there.

This is a very old bug, which was long hidden and was recently
revealed when commit 2f5d717 was
merged. In a nutshell, before this recent commit the code responsible
to load the entries from the cache didn't fully work and so the stale
entries were never restored from the cache. That commit fixed the
loader code, and so the stale entries became active, which made the
old bug visible.

To fix the old bug, this present commit modifies the settings watcher
to preserve KV deletion events, and propagates them to the persisted
cache.

(There is no release note because there is no user-facing release where
the bug was visible.)

settingswatcher: write-through to the persisted cache

Fixes #111422.
Fixes #111328.

Prior to this patch, the rangefeed watcher over system.settings was
updating the in-RAM value store before it propagated the updates to
the persisted local cache.

In fact, the update to the persisted local cache was lagging quite a
bit behind, because the rangefeed watcher would buffer updates and
only flush them after a while.

As a result, the following sequence was possible:

1. client updates a cluster setting.
2. server is immediately shut down. The persisted cache
   has not been updated yet.
3. server is restarted. For a short while (until the settings watcher
   has caught up), the old version of the setting remains active.

This recall of ghost values of a setting was simply a bug. This patch
fixes that, by ensuring that the persisted cache is written through
before the in-RAM value store.

By doing this, we give up on batching updates to the persisted local
store. This is deemed acceptable because cluster settings are not
updated frequently.

[cockroach-teamcity]

This change is 

[stevendanna]

Thanks for digging into this situation. Overall this looks good to me. Left a couple of minor comments for your consideration.

Reviewed 1 of 1 files at r1, 6 of 6 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @knz)

---

pkg/server/settingswatcher/settings_watcher.go line 296 at r3 (raw file):

	// updates in the onUpdate rangefeed function.
	if s.storage != nil {
		s.snapshot = rangefeedbuffer.MergeKVsKeepingDeletions(s.snapshot, []roachpb.KeyValue{rkv})

I wondered for a bit whether it was possible to make the merging code more efficient given that we only ever update 1 value at a time. But, ultimately decided that, like you mentioned in one of the commit messages, settings are updated infrequently.

Another thing we could consider if this becomes an issue is returning some indicator about whether anything was actually updated to avoid extra KV traffic if it is unnecessary. For instance, if the rangefeed restarts under the hood such that we see a flurry of duplicates.

But again, let's handle that if we see it become an issue. This traffic should be trivial overall.

---

pkg/kv/kvclient/rangefeed/rangefeedbuffer/kvs_test.go line 107 at r2 (raw file):

// TestMergeKVs tests the logic of MergeKVsKeepingDeletions and the
// logic of to extract KVs from a slice of events.

[nit] missing "EventToKVs" between "of" and "to" in the comment I think.

---

pkg/kv/kvclient/rangefeed/rangefeedbuffer/kvs_test.go line 168 at r2 (raw file):

				{"c", 4, ""},
				{"d", 4, ""},
			},

Should we add some out of order keys in this update (I see the updates for b are out of timestamp order, but I mean something like reversing d and c).

Perhaps also a key at a lower timestamp than what we have in the existing snapshot. Which I don't think can happen easily in the first commit but can definitely happen when we move this to being used in the translateEvent callback.

Both of these cases should work given your current implementation, so the test is just to prevent regressions there.

---

pkg/settings/integration_tests/settings_test.go line 292 at r3 (raw file):

}

func TestSettingsPersistenceEndToEnd(t *testing.T) {

I wonder if we want a test that adds some testing hook into server startup so that we can test accessing settings in a way that guarantees we are relying on the version of the on-disk cache. I don't feel strongly about this suggestion.

[aliher1911]

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

---

pkg/server/settingswatcher/settings_watcher.go line 296 at r3 (raw file):

Previously, stevendanna (Steven Danna) wrote…

I wondered for a bit whether it was possible to make the merging code more efficient given that we only ever update 1 value at a time. But, ultimately decided that, like you mentioned in one of the commit messages, settings are updated infrequently.

Another thing we could consider if this becomes an issue is returning some indicator about whether anything was actually updated to avoid extra KV traffic if it is unnecessary. For instance, if the rangefeed restarts under the hood such that we see a flurry of duplicates.

But again, let's handle that if we see it become an issue. This traffic should be trivial overall.

We must be only writing to a local store, not even under proper mvcc so the only downside must be pebble compaction/iteration over garbage which I think is negligible here. At least that's how I understand this setting storage here.

---

pkg/server/settings_cache_test.go line 185 at r3 (raw file):

	testutils.SucceedsSoon(t, func() error {
		store, err := ts.GetStores().(*kvserver.Stores).GetStore(1)
		if err != nil {

We can require.NoError(t, err, "failed to get store") here and below to avoid retry in cases where we can only get fatal errors. Also less lines to read.

[stevendanna]

Backfilling some commentary from slack. An issue we might want to consider is what happens when:

1. What happens if the following happens:
2. Write cluster setting a
3. See range feed update for write
4. Update cache
5. Reset cluster setting a
6. Crash before range feed sees update

One edge case to think through.

[knz]

The special case mentioned above is now handled via DeleteRange.

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

---

pkg/server/settingswatcher/settings_watcher.go line 296 at r3 (raw file):

Previously, aliher1911 (Oleg) wrote…

We must be only writing to a local store, not even under proper mvcc so the only downside must be pebble compaction/iteration over garbage which I think is negligible here. At least that's how I understand this setting storage here.

I went for the DeleteRange route. It's so much easier to read and understand.

---

pkg/server/settings_cache_test.go line 185 at r3 (raw file):

Previously, aliher1911 (Oleg) wrote…

We can require.NoError(t, err, "failed to get store") here and below to avoid retry in cases where we can only get fatal errors. Also less lines to read.

Done.

---

pkg/settings/integration_tests/settings_test.go line 292 at r3 (raw file):

Previously, stevendanna (Steven Danna) wrote…

I wonder if we want a test that adds some testing hook into server startup so that we can test accessing settings in a way that guarantees we are relying on the version of the on-disk cache. I don't feel strongly about this suggestion.

Done.

[knz]

PTAL

[knz]

TFYRs!

bors r=stevendanna,aliher1911

[knz]

bors r-

the kv testsuite is not happy yet

[craig]

Canceled.

[knz]

unrelated flake: #111426

[knz]

bors r=stevendanna,aliher1911

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[RaduBerinde]

LGTM with one question

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/server/settingswatcher/settings_watcher.go line 297 at r5 (raw file):

	if s.storage != nil {
		s.snapshot = rangefeedbuffer.MergeKVs(s.snapshot, []roachpb.KeyValue{rkv})
		s.storage.SnapshotKVs(ctx, s.snapshot)

SnapshotKV runs the update asynchronously... If we're not waiting for it, we reduced the window but it's still there. Maybe that's ok but then the comment should be honest about that, currently it suggests a clear sequencing.

[knz]

SnapshotKV runs the update asynchronously... If we're not waiting for it, we reduced the window but it's still there.

😱 i did not see this.