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.

Sign up for GitHub

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

Jump to bottom

tests: fix rpc limit tests #12364

Merged
merged 3 commits into from
Mar 24, 2022
Merged

tests: fix rpc limit tests #12364

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7135ff8
tests: fix rpc limit tests
lgfa29 Mar 24, 2022
f9d1ef4
test: bind server to private IP when testing connection rate limit
lgfa29 Mar 24, 2022
a6abf94
GHA u up?
lgfa29 Mar 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions nomad/rpc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,12 @@ func TestRPC_Limits_OK(t *testing.T) {
}
c.RPCHandshakeTimeout = tc.timeout
c.RPCMaxConnsPerClient = tc.limit

// With Raft v3 Autopilot makes a lot of RPC requests to
// Status.RaftStats using the StatsFetcher. This makes it very
// likely that there will be a concurrent request that will
// mess with the request limit enforcer.
c.RaftConfig.ProtocolVersion = 2
Copy link
Member

Choose a reason for hiding this comment

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

Rather than switching to v2, do we have any configuration that could throttle-down (or turn off) the autopilot library for this test? Given that we're going to be running with v3 by default I'd rather not switch this here if we can avoid it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not without modifying the current code I think. I tried to call s.autopilot.Stop() but the requests kept happening.

What did work was to have a config flag to prevent it from starting in the first place:

diff --git a/nomad/config.go b/nomad/config.go
index 6ea8cfd09..4ff57f443 100644
--- a/nomad/config.go
+++ b/nomad/config.go
@@ -349,6 +349,8 @@ type Config struct {
 	// DeploymentQueryRateLimit is in queries per second and is used by the
 	// DeploymentWatcher to throttle the amount of simultaneously deployments
 	DeploymentQueryRateLimit float64
+
+	disableAutopilot bool
 }
 
 // DefaultConfig returns the default configuration. Only used as the basis for
diff --git a/nomad/leader.go b/nomad/leader.go
index 7db6cc57e..4a2affa7c 100644
--- a/nomad/leader.go
+++ b/nomad/leader.go
@@ -288,8 +288,10 @@ func (s *Server) establishLeadership(stopCh chan struct{}) error {
 	s.handlePausableWorkers(true)
 
 	// Initialize and start the autopilot routine
-	s.getOrCreateAutopilotConfig()
-	s.autopilot.Start()
+	if !s.config.disableAutopilot {
+		s.getOrCreateAutopilotConfig()
+		s.autopilot.Start()
+	}
 
 	// Initialize scheduler configuration
 	s.getOrCreateSchedulerConfig()
diff --git a/nomad/rpc_test.go b/nomad/rpc_test.go
index 070829c00..539d01678 100644
--- a/nomad/rpc_test.go
+++ b/nomad/rpc_test.go
@@ -873,7 +873,7 @@ func TestRPC_Limits_OK(t *testing.T) {
 				// Status.RaftStats using the StatsFetcher. This makes it very
 				// likely that there will be a concurrent request that will
 				// mess with the request limit enforcer.
-				c.RaftConfig.ProtocolVersion = 2
+				c.disableAutopilot = true
 			})
 			defer func() {
 				cleanup()

Would this be a better approach?

Copy link
Member

Choose a reason for hiding this comment

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

That looks good to me! (Probably worth adding a comment on the bool to let folks know why it's there.)

})
defer func() {
cleanup()
Expand Down