From 0020590c96f81b843c103ddb33baf7d450c2a078 Mon Sep 17 00:00:00 2001 From: sirkrypt0 <22522058+sirkrypt0@users.noreply.github.com> Date: Tue, 8 Jun 2021 16:48:38 +0200 Subject: [PATCH] Update all runners when updating environment Previously only the default job would be updated to the newest specs. Now all Nomad jobs that belong to the given environment are updated accordingly. --- environment/manager.go | 16 ++++------- environment/manager_test.go | 31 +++++++++----------- runner/manager.go | 56 +++++++++++++++++++++++++++++++------ runner/manager_mock.go | 40 +++++++++++++------------- runner/manager_test.go | 21 ++------------ 5 files changed, 88 insertions(+), 76 deletions(-) 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) -}