diff --git a/environment/manager.go b/environment/manager.go index 947fdbd..389474b 100644 --- a/environment/manager.go +++ b/environment/manager.go @@ -48,8 +48,6 @@ func (m *NomadEnvironmentManager) CreateOrUpdate( if err != nil { return false, err } - exists := m.runnerManager.EnvironmentExists(runner.EnvironmentID(idInt)) - err = m.registerDefaultJob(id, request.PrewarmingPoolSize, request.CPULimit, request.MemoryLimit, request.Image, request.NetworkAccess, request.ExposedPorts) @@ -58,15 +56,11 @@ func (m *NomadEnvironmentManager) CreateOrUpdate( return false, err } - // TODO: If already exists, make sure to update all existing runners as well - if !exists { - err = m.runnerManager.RegisterEnvironment( - runner.EnvironmentID(idInt), request.PrewarmingPoolSize) - if err != nil { - return false, err - } + created, err := m.runnerManager.CreateOrUpdateEnvironment(runner.EnvironmentID(idInt), request.PrewarmingPoolSize) + if err != nil { + return created, err } - return !exists, nil + return created, nil } func (m *NomadEnvironmentManager) Delete(id string) { @@ -75,7 +69,7 @@ func (m *NomadEnvironmentManager) Delete(id string) { func (m *NomadEnvironmentManager) Load() { // ToDo: remove create default execution environment for debugging purposes - err := m.runnerManager.RegisterEnvironment(runner.EnvironmentID(0), 5) + _, err := m.runnerManager.CreateOrUpdateEnvironment(runner.EnvironmentID(0), 5) if err != nil { return } diff --git a/environment/manager_test.go b/environment/manager_test.go index 8834e82..7bd9278 100644 --- a/environment/manager_test.go +++ b/environment/manager_test.go @@ -28,7 +28,7 @@ func TestCreateOrUpdateTestSuite(t *testing.T) { func (s *CreateOrUpdateTestSuite) SetupTest() { s.runnerManagerMock = runner.ManagerMock{} - s.runnerManagerMock.On("RegisterEnvironment", mock.Anything, mock.Anything).Return(nil) + s.runnerManagerMock.On("registerEnvironment", mock.Anything, mock.Anything).Return(nil) s.apiMock = nomad.ExecutorAPIMock{} s.request = dto.ExecutionEnvironmentRequest{ @@ -53,10 +53,10 @@ func (s *CreateOrUpdateTestSuite) mockEnvironmentExists(exists bool) { s.runnerManagerMock.On("EnvironmentExists", mock.AnythingOfType("EnvironmentID")).Return(exists) } -func (s *CreateOrUpdateTestSuite) mockRegisterEnvironment() *mock.Call { - return s.runnerManagerMock.On("RegisterEnvironment", +func (s *CreateOrUpdateTestSuite) mockCreateOrUpdateEnvironment(exists bool) *mock.Call { + return s.runnerManagerMock.On("CreateOrUpdateEnvironment", mock.AnythingOfType("EnvironmentID"), mock.AnythingOfType("uint")). - Return() + Return(!exists, nil) } func (s *CreateOrUpdateTestSuite) createJobForRequest() *nomadApi.Job { @@ -77,7 +77,7 @@ func (s *CreateOrUpdateTestSuite) TestFailsOnTooLargeID() { } func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentExistsRegistersCorrectJob() { - s.mockEnvironmentExists(true) + s.mockCreateOrUpdateEnvironment(true) expectedJob := s.createJobForRequest() created, err := s.manager.CreateOrUpdate(tests.DefaultEnvironmentIDAsString, s.request) @@ -87,7 +87,7 @@ func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentExistsRegistersCorrectJob() } func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentExistsOccurredErrorIsPassed() { - s.mockEnvironmentExists(true) + s.mockCreateOrUpdateEnvironment(true) s.registerNomadJobMockCall.Return("", tests.ErrDefault) created, err := s.manager.CreateOrUpdate(tests.DefaultEnvironmentIDAsString, s.request) @@ -96,7 +96,7 @@ func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentExistsOccurredErrorIsPassed } func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentExistsReturnsFalse() { - s.mockEnvironmentExists(true) + s.mockCreateOrUpdateEnvironment(true) created, err := s.manager.CreateOrUpdate(tests.DefaultEnvironmentIDAsString, s.request) s.NoError(err) @@ -104,8 +104,7 @@ func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentExistsReturnsFalse() { } func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentDoesNotExistRegistersCorrectJob() { - s.mockEnvironmentExists(false) - s.mockRegisterEnvironment() + s.mockCreateOrUpdateEnvironment(false) expectedJob := s.createJobForRequest() @@ -116,24 +115,20 @@ func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentDoesNotExistRegistersCorrec } func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentDoesNotExistRegistersCorrectEnvironment() { - s.mockEnvironmentExists(false) - s.mockRegisterEnvironment() + s.mockCreateOrUpdateEnvironment(false) created, err := s.manager.CreateOrUpdate(tests.DefaultEnvironmentIDAsString, s.request) s.True(created) s.NoError(err) - s.runnerManagerMock.AssertCalled(s.T(), "RegisterEnvironment", - runner.EnvironmentID(tests.DefaultEnvironmentIdAsInteger), - s.request.PrewarmingPoolSize) + s.runnerManagerMock.AssertCalled(s.T(), "CreateOrUpdateEnvironment", + runner.EnvironmentID(tests.DefaultEnvironmentIdAsInteger), s.request.PrewarmingPoolSize) } func (s *CreateOrUpdateTestSuite) TestWhenEnvironmentDoesNotExistOccurredErrorIsPassedAndNoEnvironmentRegistered() { - s.mockEnvironmentExists(false) - s.mockRegisterEnvironment() + s.mockCreateOrUpdateEnvironment(false) s.registerNomadJobMockCall.Return("", tests.ErrDefault) created, err := s.manager.CreateOrUpdate(tests.DefaultEnvironmentIDAsString, s.request) s.False(created) - s.Equal(tests.ErrDefault, err) - s.runnerManagerMock.AssertNotCalled(s.T(), "RegisterEnvironment") + s.Equal(tests.DefaultError, err) } diff --git a/runner/manager.go b/runner/manager.go index 42408ce..1e3d405 100644 --- a/runner/manager.go +++ b/runner/manager.go @@ -9,6 +9,7 @@ import ( "gitlab.hpi.de/codeocean/codemoon/poseidon/logging" "gitlab.hpi.de/codeocean/codemoon/poseidon/nomad" "strconv" + "strings" "time" ) @@ -32,11 +33,9 @@ type NomadJobID string // Manager keeps track of the used and unused runners of all execution environments in order to provide unused // runners to new clients and ensure no runner is used twice. type Manager interface { - // RegisterEnvironment adds a new environment that should be managed. - RegisterEnvironment(id EnvironmentID, desiredIdleRunnersCount uint) error - - // EnvironmentExists returns whether the environment with the given id exists. - EnvironmentExists(id EnvironmentID) bool + // CreateOrUpdateEnvironment creates the given environment if it does not exist. Otherwise, it updates + // the existing environment and all runners. + CreateOrUpdateEnvironment(environmentID EnvironmentID, desiredIdleRunnersCount uint) (bool, error) // Claim returns a new runner. // It makes sure that the runner is not in use yet and returns an error if no runner could be provided. @@ -81,7 +80,15 @@ func (j *NomadEnvironment) ID() EnvironmentID { return j.environmentID } -func (m *NomadRunnerManager) RegisterEnvironment(environmentID EnvironmentID, desiredIdleRunnersCount uint) error { +func (m *NomadRunnerManager) CreateOrUpdateEnvironment(id EnvironmentID, desiredIdleRunnersCount uint) (bool, error) { + _, ok := m.environments.Get(id) + if !ok { + return true, m.registerEnvironment(id, desiredIdleRunnersCount) + } + return false, m.updateEnvironment(id, desiredIdleRunnersCount) +} + +func (m *NomadRunnerManager) registerEnvironment(environmentID EnvironmentID, desiredIdleRunnersCount uint) error { templateJob, err := m.apiClient.LoadTemplateJob(environmentID.toString()) if err != nil { return fmt.Errorf("couldn't register environment: %w", err) @@ -100,9 +107,40 @@ func (m *NomadRunnerManager) RegisterEnvironment(environmentID EnvironmentID, de return nil } -func (m *NomadRunnerManager) EnvironmentExists(id EnvironmentID) (ok bool) { - _, ok = m.environments.Get(id) - return +func (m *NomadRunnerManager) updateEnvironment(id EnvironmentID, desiredIdleRunnersCount uint) error { + environment, ok := m.environments.Get(id) + if !ok { + return ErrUnknownExecutionEnvironment + } + environment.desiredIdleRunnersCount = desiredIdleRunnersCount + + templateJob, err := m.apiClient.LoadTemplateJob(id.toString()) + if err != nil { + return fmt.Errorf("update environment couldn't load template job: %w", err) + } + environment.templateJob = templateJob + + runners, err := m.apiClient.LoadRunners(id.toString()) + if err != nil { + return fmt.Errorf("update environment couldn't load runners: %w", err) + } + var occurredErrors []string + for _, id := range runners { + // avoid taking the address of the loop variable + runnerID := id + updatedRunnerJob := *environment.templateJob + updatedRunnerJob.ID = &runnerID + updatedRunnerJob.Name = &runnerID + _, err := m.apiClient.RegisterNomadJob(&updatedRunnerJob) + if err != nil { + occurredErrors = append(occurredErrors, err.Error()) + } + } + if len(occurredErrors) > 0 { + errorResult := strings.Join(occurredErrors, "\n") + return fmt.Errorf("%d errors occurred when updating environment: %s", len(occurredErrors), errorResult) + } + return nil } func (m *NomadRunnerManager) Claim(environmentID EnvironmentID) (Runner, error) { diff --git a/runner/manager_mock.go b/runner/manager_mock.go index df850ff..de7b942 100644 --- a/runner/manager_mock.go +++ b/runner/manager_mock.go @@ -10,11 +10,11 @@ type ManagerMock struct { } // Claim provides a mock function with given fields: id -func (_m *ManagerMock) Claim(id EnvironmentID) (Runner, error) { +func (_m *ManagerMock) Claim(id EnvironmentId) (Runner, error) { ret := _m.Called(id) var r0 Runner - if rf, ok := ret.Get(0).(func(EnvironmentID) Runner); ok { + if rf, ok := ret.Get(0).(func(EnvironmentId) Runner); ok { r0 = rf(id) } else { if ret.Get(0) != nil { @@ -23,7 +23,7 @@ func (_m *ManagerMock) Claim(id EnvironmentID) (Runner, error) { } var r1 error - if rf, ok := ret.Get(1).(func(EnvironmentID) error); ok { + if rf, ok := ret.Get(1).(func(EnvironmentId) error); ok { r1 = rf(id) } else { r1 = ret.Error(1) @@ -32,27 +32,34 @@ func (_m *ManagerMock) Claim(id EnvironmentID) (Runner, error) { return r0, r1 } -// EnvironmentExists provides a mock function with given fields: id -func (_m *ManagerMock) EnvironmentExists(id EnvironmentID) bool { - ret := _m.Called(id) +// CreateOrUpdateEnvironment provides a mock function with given fields: environmentId, desiredIdleRunnersCount +func (_m *ManagerMock) CreateOrUpdateEnvironment(environmentId EnvironmentId, desiredIdleRunnersCount uint) (bool, error) { + ret := _m.Called(environmentId, desiredIdleRunnersCount) var r0 bool - if rf, ok := ret.Get(0).(func(EnvironmentID) bool); ok { - r0 = rf(id) + if rf, ok := ret.Get(0).(func(EnvironmentId, uint) bool); ok { + r0 = rf(environmentId, desiredIdleRunnersCount) } else { r0 = ret.Get(0).(bool) } - return r0 + var r1 error + if rf, ok := ret.Get(1).(func(EnvironmentId, uint) error); ok { + r1 = rf(environmentId, desiredIdleRunnersCount) + } else { + r1 = ret.Error(1) + } + + return r0, r1 } -// Get provides a mock function with given fields: runnerID -func (_m *ManagerMock) Get(runnerID string) (Runner, error) { - ret := _m.Called(runnerID) +// Get provides a mock function with given fields: runnerId +func (_m *ManagerMock) Get(runnerId string) (Runner, error) { + ret := _m.Called(runnerId) var r0 Runner if rf, ok := ret.Get(0).(func(string) Runner); ok { - r0 = rf(runnerID) + r0 = rf(runnerId) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(Runner) @@ -61,7 +68,7 @@ func (_m *ManagerMock) Get(runnerID string) (Runner, error) { var r1 error if rf, ok := ret.Get(1).(func(string) error); ok { - r1 = rf(runnerID) + r1 = rf(runnerId) } else { r1 = ret.Error(1) } @@ -69,11 +76,6 @@ func (_m *ManagerMock) Get(runnerID string) (Runner, error) { return r0, r1 } -// RegisterEnvironment provides a mock function with given fields: id, nomadJobID, desiredIdleRunnersCount -func (_m *ManagerMock) RegisterEnvironment(id EnvironmentID, nomadJobID NomadJobID, desiredIdleRunnersCount uint) { - _m.Called(id, nomadJobID, desiredIdleRunnersCount) -} - // Return provides a mock function with given fields: r func (_m *ManagerMock) Return(r Runner) error { ret := _m.Called(r) diff --git a/runner/manager_test.go b/runner/manager_test.go index 219891c..0b55460 100644 --- a/runner/manager_test.go +++ b/runner/manager_test.go @@ -7,7 +7,6 @@ import ( "github.com/sirupsen/logrus" "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "gitlab.hpi.de/codeocean/codemoon/poseidon/nomad" "gitlab.hpi.de/codeocean/codemoon/poseidon/tests" @@ -59,7 +58,7 @@ func mockRunnerQueries(apiMock *nomad.ExecutorAPIMock, returnedRunnerIds []strin } func (s *ManagerTestSuite) registerDefaultEnvironment() { - err := s.nomadRunnerManager.RegisterEnvironment(defaultEnvironmentId, 0) + err := s.nomadRunnerManager.registerEnvironment(defaultEnvironmentId, 0) s.Require().NoError(err) } @@ -73,7 +72,7 @@ func (s *ManagerTestSuite) waitForRunnerRefresh() { } func (s *ManagerTestSuite) TestRegisterEnvironmentAddsNewJob() { - err := s.nomadRunnerManager.RegisterEnvironment(anotherEnvironmentId, defaultDesiredRunnersCount) + err := s.nomadRunnerManager.registerEnvironment(anotherEnvironmentId, defaultDesiredRunnersCount) s.Require().NoError(err) job, ok := s.nomadRunnerManager.environments.Get(defaultEnvironmentId) s.True(ok) @@ -251,19 +250,3 @@ func modifyMockedCall(apiMock *nomad.ExecutorAPIMock, method string, modifier fu } } } - -func (s *ManagerTestSuite) TestWhenEnvironmentDoesNotExistEnvironmentExistsReturnsFalse() { - id := anotherEnvironmentId - _, ok := s.nomadRunnerManager.environments.Get(id) - require.False(s.T(), ok) - - s.False(s.nomadRunnerManager.EnvironmentExists(id)) -} - -func (s *ManagerTestSuite) TestWhenEnvironmentExistsEnvironmentExistsReturnsTrue() { - id := anotherEnvironmentId - s.nomadRunnerManager.environments.Add(&NomadEnvironment{environmentId: id}) - - exists := s.nomadRunnerManager.EnvironmentExists(id) - s.True(exists) -}