From 50a2a22b7442c6996e14c5071b658953ee0d71ce Mon Sep 17 00:00:00 2001
From: sirkrypt0 <22522058+sirkrypt0@users.noreply.github.com>
Date: Tue, 29 Jun 2021 09:11:21 +0200
Subject: [PATCH] Only create exactly one new runner when one runner is claimed

Previously we would create as much runners as needed based on the
local idleRunnersCount and the desiredIdleRunnersCount. This is
problematic if two runners are claimed shortly after one another.
As we only add a runner to the idleRunners list once we get the
event from Nomad, the second runner claim in a short timeframe
would create two new runners. This has been fixed now.
---
 runner/manager.go      |  8 ++++----
 runner/manager_test.go | 10 ++++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/runner/manager.go b/runner/manager.go
index 22e1dbd..7e37096 100644
--- a/runner/manager.go
+++ b/runner/manager.go
@@ -171,11 +171,11 @@ func (m *NomadRunnerManager) updateRunnerSpecs(environmentID EnvironmentID, temp
 }
 
 func (m *NomadRunnerManager) Claim(environmentID EnvironmentID, duration int) (Runner, error) {
-	job, ok := m.environments.Get(environmentID)
+	environment, ok := m.environments.Get(environmentID)
 	if !ok {
 		return nil, ErrUnknownExecutionEnvironment
 	}
-	runner, ok := job.idleRunners.Sample()
+	runner, ok := environment.idleRunners.Sample()
 	if !ok {
 		return nil, ErrNoRunnersAvailable
 	}
@@ -187,9 +187,9 @@ func (m *NomadRunnerManager) Claim(environmentID EnvironmentID, duration int) (R
 
 	runner.SetupTimeout(time.Duration(duration) * time.Second)
 
-	err = m.scaleEnvironment(environmentID)
+	err = m.createRunner(environment)
 	if err != nil {
-		log.WithError(err).WithField("environmentID", environmentID).Error("Couldn't scale environment")
+		log.WithError(err).WithField("environmentID", environmentID).Error("Couldn't create new runner for claimed one")
 	}
 
 	return runner, nil
diff --git a/runner/manager_test.go b/runner/manager_test.go
index 2782761..3e83302 100644
--- a/runner/manager_test.go
+++ b/runner/manager_test.go
@@ -133,6 +133,16 @@ func (s *ManagerTestSuite) TestClaimAddsRunnerToUsedRunners() {
 	s.Equal(savedRunner, receivedRunner)
 }
 
+func (s *ManagerTestSuite) TestTwoClaimsAddExactlyTwoRunners() {
+	s.AddIdleRunnerForDefaultEnvironment(s.exerciseRunner)
+	s.AddIdleRunnerForDefaultEnvironment(NewRunner(tests.AnotherRunnerID, s.nomadRunnerManager))
+	_, err := s.nomadRunnerManager.Claim(defaultEnvironmentID, defaultInactivityTimeout)
+	s.Require().NoError(err)
+	_, err = s.nomadRunnerManager.Claim(defaultEnvironmentID, defaultInactivityTimeout)
+	s.Require().NoError(err)
+	s.apiMock.AssertNumberOfCalls(s.T(), "RegisterRunnerJob", 2)
+}
+
 func (s *ManagerTestSuite) TestGetReturnsRunnerIfRunnerIsUsed() {
 	s.nomadRunnerManager.usedRunners.Add(s.exerciseRunner)
 	savedRunner, err := s.nomadRunnerManager.Get(s.exerciseRunner.Id())