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: add admission-overload/follower-overload #111070

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

irfansharif
Copy link
Contributor

@irfansharif irfansharif commented Sep 21, 2023

This is just resuscitating #81516, tests we added when developing
follower pausing but never checked in. Some of this work is applicable
in the context of replication admission control, when we apply flow
control for regular writes. The tests add an IO nemesis on n3/s3,
restricting disk bandwidth to 20MiB/s. They then observe what happens
when remote nodes n1 and n2 issue follower write traffic to it, either
across many ranges or just 1 (potentially triggering the per-replica
proposal quota pool). We can observe LSM state on n3/s3 under such
conditions, and also what would happen if the concentrated follower node
also serves some foreground load.

Part of #89208.

Release note: None

@irfansharif irfansharif requested a review from a team as a code owner September 21, 2023 17:59
@irfansharif irfansharif requested review from rachitgsrivastava and smg260 and removed request for a team September 21, 2023 17:59
@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.

:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @aadityasondhi, @bananabrick, @irfansharif, @rachitgsrivastava, and @smg260)


-- commits line 7 at r1:
nit: The tests add an ...


-- commits line 13 at r1:
nit: if the concentrated follower node also serves some ...


pkg/cmd/roachtest/tests/admission_control_follower_overload.go line 119 at r1 (raw file):

	// cluster every time.
	const dev = true
	if dev {

nit: why this if-block if dev is always true?

@aadityasondhi aadityasondhi force-pushed the 230921.ac-follower-overload branch 3 times, most recently from 937348d to 7e01e05 Compare October 25, 2023 14:40
Copy link
Collaborator

@aadityasondhi aadityasondhi 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 @bananabrick, @rachitgsrivastava, @smg260, and @sumeerbhola)


-- commits line 7 at r1:

Previously, sumeerbhola wrote…

nit: The tests add an ...

Fixed.


-- commits line 13 at r1:

Previously, sumeerbhola wrote…

nit: if the concentrated follower node also serves some ...

Fixed.


pkg/cmd/roachtest/tests/admission_control_follower_overload.go line 119 at r1 (raw file):

Previously, sumeerbhola wrote…

nit: why this if-block if dev is always true?

I'm not sure; it came from the original PR. I removed it.

@aadityasondhi
Copy link
Collaborator

bors r=sumeerbhola

@craig
Copy link
Contributor

craig bot commented Oct 25, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Oct 25, 2023

Build failed (retrying...):

@aadityasondhi
Copy link
Collaborator

bors r-

@craig
Copy link
Contributor

craig bot commented Oct 25, 2023

Canceled.

@aadityasondhi aadityasondhi force-pushed the 230921.ac-follower-overload branch 2 times, most recently from baacc66 to 1e716fd Compare October 25, 2023 18:14
This is just resuscitating cockroachdb#81516, tests we added when developing
follower pausing but never checked in. Some of this work is applicable
in the context of replication admission control, when we apply flow
control for regular writes. The tests add an IO nemesis on n3/s3,
restricting disk bandwidth to 20MiB/s. They then observe what happens
when remote nodes n1 and n2 issue follower write traffic to it, either
across many ranges or just 1 (potentially triggering the per-replica
proposal quota pool). We can observe LSM state on n3/s3 under such
conditions, and also what would happen if the concentrated follower node
also serves some foreground load.

Part of cockroachdb#89208.

Release note: None
@aadityasondhi aadityasondhi force-pushed the 230921.ac-follower-overload branch from 1e716fd to ca1a6d6 Compare October 25, 2023 18:31
@aadityasondhi
Copy link
Collaborator

bors r=sumeerbhola

@craig
Copy link
Contributor

craig bot commented Oct 25, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Oct 25, 2023

Build succeeded:

@craig craig bot merged commit c8ee614 into cockroachdb:master Oct 25, 2023
3 checks passed
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