Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage/engine: improve cluster version handling #42653

Closed
petermattis opened this issue Nov 21, 2019 · 5 comments · Fixed by #97054
Closed

storage/engine: improve cluster version handling #42653

petermattis opened this issue Nov 21, 2019 · 5 comments · Fixed by #97054
Assignees
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. E-quick-win Likely to be a quick win for someone experienced. T-storage Storage Team

Comments

@petermattis
Copy link
Collaborator

petermattis commented Nov 21, 2019

The cluster version is currently stored inside of RocksDB/Pebble by Store.WriteClusterVersion. At startup, a node will refuse to start if the cluster version is newer than what the node supports. This prevents accidental downgrades. Unfortunately, there is a hole in this strategy. Because the cluster version is stored in RocksDB, we have to open the RocksDB instance in order to read it. Opening the RocksDB instance causes the RocksDB WAL to be replayed and replaying the WAL itself can callback into CRDB code which is cluster version specific. It looks like this exact scenario caused problems for a user who upgraded to 19.1 and then restarted a node with 2.0.

There are two possibilities for improvement here. We could open the RocksDB instance read-only in order to check the cluster version, then open it again as writable. This should work, but is mildly unfortunate as we'd end up replaying the WAL twice.

Another option is to move the cluster version storage out of RocksDB. We already have a COCKROACHDB_VERSION. We could extend this file, or add another file, that stores the cluster version.

Jira issue: CRDB-5336

@github-actions
Copy link

github-actions bot commented Jun 4, 2021

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
5 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

@petermattis petermattis added A-storage Relating to our storage engine (Pebble) on-disk storage. and removed no-issue-activity labels Jun 4, 2021
@jlinder jlinder added the T-storage Storage Team label Jun 16, 2021
@jbowens
Copy link
Collaborator

jbowens commented Dec 29, 2021

We ran into this issue when making backwards incompatible encryption-at-rest changes and began moving the cluster version out of Pebble. A new file STORAGE_MIN_VERSION was introduced in #68619. The STORAGE_MIN_VERSION file contains a roachpb.Version protobuf. In #69203, kvserver.WriteClusterVersion was updated to write to both the key within Pebble and this new file STORAGE_MIN_VERSION. This was included in the 21.2 release. In 22.1, we can switch remaining keys.StoreClusterVersionKey readers over to reading the cluster version through this file, and remove the keys.StoreClusterVersionKey altogether.

@nicktrav
Copy link
Collaborator

Doing some backlog triage.

My understanding of this issue is that we have implemented the "out of DB" version handling, and all that remains is some cleanup:

In 22.1, we can switch remaining keys.StoreClusterVersionKey readers over to reading the cluster version through this file, and remove the keys.StoreClusterVersionKey altogether.

@nicktrav nicktrav added the C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. label Jul 11, 2022
@nicktrav nicktrav added the E-quick-win Likely to be a quick win for someone experienced. label Feb 10, 2023
@nicktrav
Copy link
Collaborator

@RaduBerinde - does any of your recent work in #96394 help here?

@RaduBerinde RaduBerinde self-assigned this Feb 10, 2023
@RaduBerinde
Copy link
Member

Yes, I am working on the cleanup mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. E-quick-win Likely to be a quick win for someone experienced. T-storage Storage Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants