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

roachtest: use local SSDs for disk-stall failover tests #99963

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

nicktrav
Copy link
Collaborator

The disk-stalled roachtests were updated in #99747 to use local SSDs. This change broke the failover/*/disk-stall tests, which look for /dev/sdb on GCE (the used for GCE Persistent Disks), but the tests still create clusters with local SSDs (the roachtest default).

Fix #99902.
Fix #99926.
Fix #99930.

Touches #97968.

Release note: None.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nicktrav nicktrav added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 29, 2023
@nicktrav
Copy link
Collaborator Author

I took this for a spin. Each of the three variants passed:

@nicktrav nicktrav requested review from andrewbaptist, a team and sumeerbhola March 29, 2023 20:29
@nicktrav nicktrav marked this pull request as ready for review March 29, 2023 20:29
@nicktrav nicktrav requested a review from a team as a code owner March 29, 2023 20:29
@nicktrav nicktrav requested review from herkolategan and smg260 and removed request for a team March 29, 2023 20:29
Copy link
Collaborator

@andrewbaptist andrewbaptist left a comment

Choose a reason for hiding this comment

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

:lgtm:

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 1 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @nicktrav, and @smg260)


-- commits line 4 at r1:
nit: to not use?


-- commits line 6 at r1:
nit: something missing/odd in "the used for ..."


pkg/cmd/roachtest/tests/disk_stall.go line 51 at r1 (raw file):

		s := r.MakeClusterSpec(4, spec.ReuseNone())
		// Use local SSDs in an attempt to work around flakes encountered when using
		// SSDs. See #97968.

Confused here too. #99747 says switch to PD.


pkg/cmd/roachtest/tests/failover.go line 52 at r1 (raw file):

			if failureMode == failureModeDiskStall {
				// Use local SSDs in an attempt to work around flakes encountered when
				// using SSDs. See #97968.

ditto

@nicktrav nicktrav force-pushed the nickt.failover-disk-stall branch 2 times, most recently from 08b5ef6 to fce87cd Compare March 29, 2023 22:38
Copy link
Collaborator Author

@nicktrav nicktrav 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: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @herkolategan, @smg260, and @sumeerbhola)


-- commits line 4 at r1:

Previously, sumeerbhola wrote…

nit: to not use?

Woops. Yes, confusing myself. Fixed.


-- commits line 6 at r1:

Previously, sumeerbhola wrote…

nit: something missing/odd in "the used for ..."

Fixed.


pkg/cmd/roachtest/tests/disk_stall.go line 51 at r1 (raw file):

Previously, sumeerbhola wrote…

Confused here too. #99747 says switch to PD.

Done.


pkg/cmd/roachtest/tests/failover.go line 52 at r1 (raw file):

Previously, sumeerbhola wrote…

ditto

Done.

The disk-stalled roachtests were updated in cockroachdb#99747 to use PDs in favor
of local SSDs. This change broke the `failover/*/disk-stall` tests,
which look for `/dev/sdb` on GCE (the device name used for GCE
Persistent Disks), but the tests still create clusters with local SSDs
(the roachtest default).

Fix cockroachdb#99902.
Fix cockroachdb#99926.
Fix cockroachdb#99930.

Touches cockroachdb#97968.

Release note: None.
@nicktrav
Copy link
Collaborator Author

Rebasing around some CI flakes.

@nicktrav
Copy link
Collaborator Author

TFTRs!

bors r=andrewbaptist

@craig
Copy link
Contributor

craig bot commented Mar 30, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
4 participants