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

vfs: make Unwrap part of FS, fix metamorphic restart op #3887

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

RaduBerinde
Copy link
Member

The metamorphic restart op never resolved the "base" MemFS because
diskHealthCheckingFS does not implement Unwrap. The code path
where we discard writes done by Close was inactive.

This change makes Unwrap part of FS and implements it correctly in
diskHealthCheckingFS.

In the metamorphic test, we now support the restartDB operation even
when the FS is not strict. When it is strict, we discard unsynced data
prior to Close. We no longer silently tolerate Root returning
something other than *MemFS in strictFS mode.

@RaduBerinde RaduBerinde requested a review from jbowens August 24, 2024 17:03
@RaduBerinde RaduBerinde requested a review from a team as a code owner August 24, 2024 17:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jbowens and @RaduBerinde)


metamorphic/test.go line 276 at r1 (raw file):

	var memFS *vfs.MemFS
	if !t.testOpts.strictFS {

Shouldn't this say if t.testOpts.strictFS?
I am surprised this did not panic when calling SetIgnoreSyncs because of it not being a strict MemFS.

Oh, this "worked" but is actually broken because we return earlier in this function due to

if !t.testOpts.strictFS {
		return nil
	}

And because of this new if-block we will never do ignore syncs.

Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @jbowens and @sumeerbhola)


metamorphic/test.go line 276 at r1 (raw file):

Previously, sumeerbhola wrote…

Shouldn't this say if t.testOpts.strictFS?
I am surprised this did not panic when calling SetIgnoreSyncs because of it not being a strict MemFS.

Oh, this "worked" but is actually broken because we return earlier in this function due to

if !t.testOpts.strictFS {
		return nil
	}

And because of this new if-block we will never do ignore syncs.

faceslap Fixed it.. but now I get meta failures.. I'll have to investigate.

Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @jbowens and @sumeerbhola)


metamorphic/test.go line 276 at r1 (raw file):

Previously, RaduBerinde wrote…

faceslap Fixed it.. but now I get meta failures.. I'll have to investigate.

Oh, I see. Without strictFS, we are using NoSync for write options, so we can't really restart the database and expect a deterministic outcome.

Changing it back to only run this op in strictFS mode. I'm still running into some failures that I'm investigating.

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @jbowens)

The metamorphic restart op never resolved the "base" `MemFS` because
`diskHealthCheckingFS` does not implement `Unwrap`. The code path
where we discard writes done by Close was inactive.

This change makes `Unwrap` part of `FS` and implements it correctly in
`diskHealthCheckingFS`.

In the meta test, we no longer silently tolerate `Root` returning
something other than `*MemFS` in `strictFS` mode.
Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 7 files reviewed, 1 unresolved discussion (waiting on @jbowens and @sumeerbhola)


metamorphic/test.go line 276 at r1 (raw file):

Previously, RaduBerinde wrote…

Oh, I see. Without strictFS, we are using NoSync for write options, so we can't really restart the database and expect a deterministic outcome.

Changing it back to only run this op in strictFS mode. I'm still running into some failures that I'm investigating.

Added a couple more exceptions and a fix for write-after-EOF in MemFS, seems to work now.

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.

:lgtm:

Reviewed 4 of 6 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sumeerbhola)

@RaduBerinde
Copy link
Member Author

TFTR!

@RaduBerinde RaduBerinde merged commit 9b79e82 into cockroachdb:master Aug 26, 2024
11 checks passed
@RaduBerinde RaduBerinde deleted the fix-meta-restart branch August 26, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants