Skip to content

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Aug 15, 2025

storage: move store version checks into fs pkg

Move functions to perform pebble compatibility checks into the fs pkg.

storage: add cluster version to env config

storage, fs: move pebble cluster version compat check to InitEnv

Previously, we performed the CRDB version compatibility check for pebble and the
currently running cluster in the pebble constructor. This commit moves that
check to the file system environment initialization so that we can colocate
with fs encyrption initialization.

We also extend the version checking to validate that pebble's minversion is not
too high for the current cockroach version, to guard against backwards
incompatible changes.

Fixes: #148213

Epic: none
Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz force-pushed the move-version-check branch 4 times, most recently from deb890e to 662d064 Compare August 15, 2025 19:48
@xinhaoz xinhaoz changed the title wip wip: storage, fs: move pebble cluster version compat check to InitEnv Aug 15, 2025
@xinhaoz xinhaoz force-pushed the move-version-check branch 10 times, most recently from 6dc486f to b64a3d4 Compare August 18, 2025 17:50
@xinhaoz xinhaoz changed the title wip: storage, fs: move pebble cluster version compat check to InitEnv storage, fs: move pebble cluster version compat check to InitEnv Aug 18, 2025
@xinhaoz xinhaoz marked this pull request as ready for review August 18, 2025 18:15
@xinhaoz xinhaoz requested review from a team as code owners August 18, 2025 18:15
@xinhaoz xinhaoz requested a review from annrpom August 18, 2025 18:15
@xinhaoz xinhaoz force-pushed the move-version-check branch 3 times, most recently from 14683fd to d5e8f96 Compare August 18, 2025 19:31
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(still reading; flushing comments on first commit)

Reviewed 10 of 10 files at r1, 1 of 19 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @annrpom)


pkg/storage/minversion/min_version.go line 26 at r1 (raw file):

// MinVersionIsAtLeastTargetVersion returns whether the min version recorded
// on disk is at least the target version.
func MinVersionIsAtLeastTargetVersion(

nit: This is moot if we end up moving this into storage/fs, but if these are in a minversion package I think we should strip the MinVersion prefix from these names to avoid the stutter.


pkg/storage/min_version.go line 37 at r1 (raw file):

	}
	filename := atomicRenameFS.PathJoin(dir, minversion.MinVersionFilename)
	if err := fs.SafeWriteToUnencryptedFile(atomicRenameFS, dir, filename, b, fs.UnspecifiedWriteCategory); err != nil {

Maybe we should move the min version stuff into the fs package itself? it seems like the func to write the min version file (writeMinVersionFile) should be in the minversion package, but that's not possible because of the cycle that motivated this move right?

@xinhaoz xinhaoz force-pushed the move-version-check branch from d5e8f96 to c2d3f7b Compare August 19, 2025 20:13
Move functions to perform pebble compatibility checks into the fs pkg.
@xinhaoz xinhaoz force-pushed the move-version-check branch from c2d3f7b to 3f06293 Compare August 19, 2025 21:29
Previously, we performed the CRDB version compatibility check for pebble and the
currently running cluster in the pebble constructor. This commit moves that
check to the file system environment initialization so that we can  colocate
with fs encyrption initialization.

We also extend the version checking to validate that pebble's minversion is not
too high for the current cockroach version, to guard against backwards
incompatible changes.
Fixes: cockroachdb#148213

Epic: none
Release note: None
@xinhaoz xinhaoz force-pushed the move-version-check branch from 3f06293 to ab81bf1 Compare August 19, 2025 21:34
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

:lgtm:

@jbowens reviewed 25 of 25 files at r4, 8 of 22 files at r5, 2 of 22 files at r6, 15 of 19 files at r7, 6 of 6 files at r8, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @annrpom)

@xinhaoz
Copy link
Member Author

xinhaoz commented Aug 20, 2025

tftr!
bors r+

@craig
Copy link
Contributor

craig bot commented Aug 20, 2025

@craig craig bot merged commit 19606f2 into cockroachdb:master Aug 20, 2025
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage: avoid opening store with too new of a cluster version
3 participants