Skip to content

server: storeCachedSettingsKVs does not remove unset settings values #70567

Closed
#111475
Closed
server: storeCachedSettingsKVs does not remove unset settings values#70567
#111475
Assignees
knz
Labels
A-configurabilityPertains to cluster settings, CLI flags, env vars etcC-bugCode not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.T-multitenantIssues owned by the multi-tenant virtual teambranch-masterFailures and bugs on the master branch.branch-release-23.2Used to mark GA and release blockers, technical advisories, and bugs for 23.2release-blockerIndicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.
@ajwerner

Description

@ajwerner
ajwerner
opened on Sep 22, 2021

Is your feature request related to a problem? Please describe.

In settingsworker.go we store on disk in the system tenant a cache of settings. This allows us to have some access to all settings before the KV layer becomes available (e.g. during node restarts).

When the settings watcher finds a rangefeed update, we write it

cockroach/pkg/server/settingsworker.go

Line 60 in 9c8aa87

return errors.Wrap(storeCachedSettingsKVs(ctx, eng, settingsKVs), "while storing settings kvs")

The issue is, when we write it, we don't clear out the values which are now unset. See

cockroach/pkg/server/settings_cache.go

Lines 24 to 38 in 9c8aa87

// storeCachedSettingsKVs stores or caches node's settings locally.
// This helps in restoring the node restart with the at least the same settings with which it died.
func storeCachedSettingsKVs(ctx context.Context, eng storage.Engine, kvs []roachpb.KeyValue) error {
batch := eng.NewBatch()
defer batch.Close()
for _, kv := range kvs {
kv.Value.Timestamp = hlc.Timestamp{} // nb: Timestamp is not part of checksum
if err := storage.MVCCPut(
ctx, batch, nil, keys.StoreCachedSettingsKey(kv.Key), hlc.Timestamp{}, kv.Value, nil,
); err != nil {
return err
}
}
return batch.Commit(false /* sync */)
}

Describe the solution you'd like
We should clear the keys which do exist and are not in the set we want to write.

Additional context

This can be very annoying as we might retain values which are no longer valid. See #68335.

Jira issue: CRDB-10124
Epic: CRDB-6671

Metadata

Metadata

Assignees

Labels

A-configurabilityPertains to cluster settings, CLI flags, env vars etcC-bugCode not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.T-multitenantIssues owned by the multi-tenant virtual teambranch-masterFailures and bugs on the master branch.branch-release-23.2Used to mark GA and release blockers, technical advisories, and bugs for 23.2release-blockerIndicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions