From 619cd40fb64f398b28c61838ba90e36109c5c300 Mon Sep 17 00:00:00 2001 From: Jan-Eric Hellenberg Date: Thu, 20 May 2021 09:26:17 +0200 Subject: [PATCH] Refactor EntityStore interface to multiple interfaces of specific type --- api/runners.go | 2 +- runner/constants_test.go | 16 +--- runner/job_store.go | 57 -------------- runner/job_store_test.go | 101 ------------------------- runner/manager.go | 34 ++++----- runner/manager_mock.go | 46 ++++++------ runner/manager_test.go | 49 ++++++------ runner/nomad_job_storage.go | 61 +++++++++++++++ runner/nomad_job_storage_test.go | 77 +++++++++++++++++++ runner/pool.go | 70 ----------------- runner/pool_test.go | 125 ------------------------------- runner/runner.go | 4 +- runner/storage.go | 74 ++++++++++++++++++ runner/storage_test.go | 101 +++++++++++++++++++++++++ store/entity_store.go | 24 ------ tests/test_constants.go | 10 +++ 16 files changed, 393 insertions(+), 458 deletions(-) delete mode 100644 runner/job_store.go delete mode 100644 runner/job_store_test.go create mode 100644 runner/nomad_job_storage.go create mode 100644 runner/nomad_job_storage_test.go delete mode 100644 runner/pool.go delete mode 100644 runner/pool_test.go create mode 100644 runner/storage.go create mode 100644 runner/storage_test.go delete mode 100644 store/entity_store.go create mode 100644 tests/test_constants.go diff --git a/api/runners.go b/api/runners.go index e616ba4..a358c7e 100644 --- a/api/runners.go +++ b/api/runners.go @@ -107,7 +107,7 @@ func (r *RunnerController) findRunnerMiddleware(next http.Handler) http.Handler writeNotFound(writer, err) return } - ctx := runner.NewContext(request.Context(), targetRunner.(runner.Runner)) + ctx := runner.NewContext(request.Context(), targetRunner) requestWithRunner := request.WithContext(ctx) next.ServeHTTP(writer, requestWithRunner) }) diff --git a/runner/constants_test.go b/runner/constants_test.go index 3e38cb0..2605a41 100644 --- a/runner/constants_test.go +++ b/runner/constants_test.go @@ -1,16 +1,8 @@ package runner +import "gitlab.hpi.de/codeocean/codemoon/poseidon/tests" + const ( - defaultRunnerId = "s0m3-r4nd0m-1d" - anotherRunnerId = "4n0th3r-runn3r-1d" - defaultEnvironmentId = EnvironmentId(0) - anotherEnvironmentId = EnvironmentId(42) - defaultJobId = "s0m3-j0b-1d" - anotherJobId = "4n0th3r-j0b-1d" + defaultEnvironmentId = EnvironmentId(tests.DefaultEnvironmentIdAsInteger) + anotherEnvironmentId = EnvironmentId(tests.AnotherEnvironmentIdAsInteger) ) - -type DummyEntity struct{} - -func (DummyEntity) Id() string { - return "" -} diff --git a/runner/job_store.go b/runner/job_store.go deleted file mode 100644 index d67d5a5..0000000 --- a/runner/job_store.go +++ /dev/null @@ -1,57 +0,0 @@ -package runner - -import ( - "gitlab.hpi.de/codeocean/codemoon/poseidon/store" - "sync" -) - -// JobStore is a type of entity store that should store job entities. -type JobStore interface { - store.EntityStore -} - -// nomadJobStore stores NomadJob objects in the local application memory. -type nomadJobStore struct { - sync.RWMutex - jobs map[string]*NomadJob -} - -// NewNomadJobStore responds with a Pool implementation. -// This implementation stores the data thread-safe in the local application memory. -func NewNomadJobStore() *nomadJobStore { - return &nomadJobStore{ - jobs: make(map[string]*NomadJob), - } -} - -func (pool *nomadJobStore) Add(nomadJob store.Entity) { - pool.Lock() - defer pool.Unlock() - jobEntity, ok := nomadJob.(*NomadJob) - if !ok { - log. - WithField("pool", pool). - WithField("entity", nomadJob). - Fatal("Entity of type NomadJob was expected, but wasn't given.") - } - pool.jobs[nomadJob.Id()] = jobEntity -} - -func (pool *nomadJobStore) Get(id string) (nomadJob store.Entity, ok bool) { - pool.RLock() - defer pool.RUnlock() - nomadJob, ok = pool.jobs[id] - return -} - -func (pool *nomadJobStore) Delete(id string) { - pool.Lock() - defer pool.Unlock() - delete(pool.jobs, id) -} - -func (pool *nomadJobStore) Len() int { - pool.RLock() - defer pool.RUnlock() - return len(pool.jobs) -} diff --git a/runner/job_store_test.go b/runner/job_store_test.go deleted file mode 100644 index 4bd478f..0000000 --- a/runner/job_store_test.go +++ /dev/null @@ -1,101 +0,0 @@ -package runner - -import ( - "github.com/sirupsen/logrus" - "github.com/sirupsen/logrus/hooks/test" - "github.com/stretchr/testify/suite" - "testing" -) - -func TestJobStoreTestSuite(t *testing.T) { - suite.Run(t, new(JobStoreTestSuite)) -} - -type JobStoreTestSuite struct { - suite.Suite - jobStore *nomadJobStore - job *NomadJob -} - -func (suite *JobStoreTestSuite) SetupTest() { - suite.jobStore = NewNomadJobStore() - suite.job = &NomadJob{environmentId: defaultEnvironmentId, jobId: defaultJobId} -} - -func (suite *JobStoreTestSuite) TestAddInvalidEntityTypeThrowsFatal() { - var hook *test.Hook - logger, hook := test.NewNullLogger() - // don't terminate program on fatal log entry - logger.ExitFunc = func(int) {} - log = logger.WithField("pkg", "environment") - - dummyEntity := DummyEntity{} - suite.jobStore.Add(dummyEntity) - suite.Equal(logrus.FatalLevel, hook.LastEntry().Level) - suite.Equal(dummyEntity, hook.LastEntry().Data["entity"]) -} - -func (suite *JobStoreTestSuite) TestAddValidEntityDoesNotThrowFatal() { - var hook *test.Hook - logger, hook := test.NewNullLogger() - log = logger.WithField("pkg", "environment") - - suite.jobStore.Add(suite.job) - // currently, the Add method does not log anything else. adjust if necessary - suite.Nil(hook.LastEntry()) -} - -func (suite *JobStoreTestSuite) TestAddedJobCanBeRetrieved() { - suite.jobStore.Add(suite.job) - retrievedJob, ok := suite.jobStore.Get(suite.job.Id()) - suite.True(ok, "A saved runner should be retrievable") - suite.Equal(suite.job, retrievedJob) -} - -func (suite *JobStoreTestSuite) TestJobWithSameIdOverwritesOldOne() { - otherJobWithSameId := &NomadJob{environmentId: defaultEnvironmentId} - // assure runner is actually different - otherJobWithSameId.jobId = anotherJobId - suite.NotEqual(suite.job, otherJobWithSameId) - - suite.jobStore.Add(suite.job) - suite.jobStore.Add(otherJobWithSameId) - retrievedJob, _ := suite.jobStore.Get(suite.job.Id()) - suite.NotEqual(suite.job, retrievedJob) - suite.Equal(otherJobWithSameId, retrievedJob) -} - -func (suite *JobStoreTestSuite) TestDeletedJobIsNotAccessible() { - suite.jobStore.Add(suite.job) - suite.jobStore.Delete(suite.job.Id()) - retrievedRunner, ok := suite.jobStore.Get(suite.job.Id()) - suite.Nil(retrievedRunner) - suite.False(ok, "A deleted runner should not be accessible") -} - -func (suite *JobStoreTestSuite) TestLenOfEmptyPoolIsZero() { - suite.Equal(0, suite.jobStore.Len()) -} - -func (suite *JobStoreTestSuite) TestLenChangesOnStoreContentChange() { - suite.Run("len increases when job is added", func() { - suite.jobStore.Add(suite.job) - suite.Equal(1, suite.jobStore.Len()) - }) - - suite.Run("len does not increase when job with same id is added", func() { - suite.jobStore.Add(suite.job) - suite.Equal(1, suite.jobStore.Len()) - }) - - suite.Run("len increases again when different job is added", func() { - anotherJob := &NomadJob{environmentId: anotherEnvironmentId} - suite.jobStore.Add(anotherJob) - suite.Equal(2, suite.jobStore.Len()) - }) - - suite.Run("len decreases when job is deleted", func() { - suite.jobStore.Delete(suite.job.Id()) - suite.Equal(1, suite.jobStore.Len()) - }) -} diff --git a/runner/manager.go b/runner/manager.go index 2e5add9..ffd1eb1 100644 --- a/runner/manager.go +++ b/runner/manager.go @@ -15,9 +15,6 @@ var ( ) type EnvironmentId int -func (e EnvironmentId) toString() string { - return string(rune(e)) -} type NomadJobId string @@ -41,45 +38,45 @@ type Manager interface { type NomadRunnerManager struct { apiClient nomad.ExecutorApi - jobs JobStore - usedRunners Pool + jobs NomadJobStorage + usedRunners Storage } func NewNomadRunnerManager(apiClient nomad.ExecutorApi) *NomadRunnerManager { return &NomadRunnerManager{ apiClient, - NewNomadJobStore(), - NewLocalRunnerPool(), + NewLocalNomadJobStorage(), + NewLocalRunnerStorage(), } } type NomadJob struct { environmentId EnvironmentId jobId NomadJobId - idleRunners Pool + idleRunners Storage desiredIdleRunnersCount int } -func (j *NomadJob) Id() string { - return j.environmentId.toString() +func (j *NomadJob) Id() EnvironmentId { + return j.environmentId } func (m *NomadRunnerManager) RegisterEnvironment(environmentId EnvironmentId, nomadJobId NomadJobId, desiredIdleRunnersCount int) { m.jobs.Add(&NomadJob{ environmentId, nomadJobId, - NewLocalRunnerPool(), + NewLocalRunnerStorage(), desiredIdleRunnersCount, }) go m.refreshEnvironment(environmentId) } func (m *NomadRunnerManager) Claim(environmentId EnvironmentId) (Runner, error) { - job, ok := m.jobs.Get(environmentId.toString()) + job, ok := m.jobs.Get(environmentId) if !ok { return nil, ErrUnknownExecutionEnvironment } - runner, ok := job.(*NomadJob).idleRunners.Sample() + runner, ok := job.idleRunners.Sample() if !ok { return nil, ErrNoRunnersAvailable } @@ -92,7 +89,7 @@ func (m *NomadRunnerManager) Get(runnerId string) (Runner, error) { if !ok { return nil, ErrRunnerNotFound } - return runner.(Runner), nil + return runner, nil } func (m *NomadRunnerManager) Return(r Runner) (err error) { @@ -106,12 +103,11 @@ func (m *NomadRunnerManager) Return(r Runner) (err error) { // Refresh Big ToDo: Improve this function!! State out that it also rescales the job; Provide context to be terminable... func (m *NomadRunnerManager) refreshEnvironment(id EnvironmentId) { - jobEntity, ok := m.jobs.Get(id.toString()) + job, ok := m.jobs.Get(id) if !ok { // this environment does not exist return } - job := jobEntity.(*NomadJob) lastJobScaling := -1 for { runners, err := m.apiClient.LoadRunners(string(job.jobId)) @@ -130,7 +126,7 @@ func (m *NomadRunnerManager) refreshEnvironment(id EnvironmentId) { log.WithError(err).Printf("Failed get allocation count") break } - neededRunners := job.desiredIdleRunnersCount - job.idleRunners.Len() + 1 + neededRunners := job.desiredIdleRunnersCount - job.idleRunners.Length() + 1 runnerCount := jobScale + neededRunners time.Sleep(50 * time.Millisecond) if runnerCount != lastJobScaling { @@ -147,7 +143,7 @@ func (m *NomadRunnerManager) refreshEnvironment(id EnvironmentId) { func (m *NomadRunnerManager) unusedRunners(environmentId EnvironmentId, fetchedRunnerIds []string) (newRunners []Runner) { newRunners = make([]Runner, 0) - jobEntity, ok := m.jobs.Get(environmentId.toString()) + job, ok := m.jobs.Get(environmentId) if !ok { // the environment does not exist, so it won't have any unused runners return @@ -155,7 +151,7 @@ func (m *NomadRunnerManager) unusedRunners(environmentId EnvironmentId, fetchedR for _, runnerId := range fetchedRunnerIds { _, ok := m.usedRunners.Get(runnerId) if !ok { - _, ok = jobEntity.(*NomadJob).idleRunners.Get(runnerId) + _, ok = job.idleRunners.Get(runnerId) if !ok { newRunners = append(newRunners, NewRunner(runnerId)) } diff --git a/runner/manager_mock.go b/runner/manager_mock.go index af5bdba..8ad7e50 100644 --- a/runner/manager_mock.go +++ b/runner/manager_mock.go @@ -9,6 +9,29 @@ type ManagerMock struct { mock.Mock } +// Claim provides a mock function with given fields: id +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 { + r0 = rf(id) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(Runner) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(EnvironmentId) error); ok { + r1 = rf(id) + } 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) @@ -50,26 +73,3 @@ func (_m *ManagerMock) Return(r Runner) error { return r0 } - -// Use provides a mock function with given fields: id -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 { - r0 = rf(id) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(Runner) - } - } - - var r1 error - if rf, ok := ret.Get(1).(func(EnvironmentId) error); ok { - r1 = rf(id) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} diff --git a/runner/manager_test.go b/runner/manager_test.go index 553d20b..a501554 100644 --- a/runner/manager_test.go +++ b/runner/manager_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" "gitlab.hpi.de/codeocean/codemoon/poseidon/nomad" + "gitlab.hpi.de/codeocean/codemoon/poseidon/tests" "testing" "time" ) @@ -27,7 +28,7 @@ type ManagerTestSuite struct { func (suite *ManagerTestSuite) SetupTest() { suite.apiMock = &nomad.ExecutorApiMock{} suite.nomadRunnerManager = NewNomadRunnerManager(suite.apiMock) - suite.exerciseRunner = NewRunner(defaultRunnerId) + suite.exerciseRunner = NewRunner(tests.DefaultRunnerId) suite.mockRunnerQueries([]string{}) suite.registerDefaultEnvironment() } @@ -35,18 +36,18 @@ func (suite *ManagerTestSuite) SetupTest() { func (suite *ManagerTestSuite) mockRunnerQueries(returnedRunnerIds []string) { // reset expected calls to allow new mocked return values suite.apiMock.ExpectedCalls = []*mock.Call{} - suite.apiMock.On("LoadRunners", defaultJobId).Return(returnedRunnerIds, nil) - suite.apiMock.On("JobScale", defaultJobId).Return(len(returnedRunnerIds), nil) - suite.apiMock.On("SetJobScale", defaultJobId, mock.AnythingOfType("int"), "Runner Requested").Return(nil) + suite.apiMock.On("LoadRunners", tests.DefaultJobId).Return(returnedRunnerIds, nil) + suite.apiMock.On("JobScale", tests.DefaultJobId).Return(len(returnedRunnerIds), nil) + suite.apiMock.On("SetJobScale", tests.DefaultJobId, mock.AnythingOfType("int"), "Runner Requested").Return(nil) } func (suite *ManagerTestSuite) registerDefaultEnvironment() { - suite.nomadRunnerManager.RegisterEnvironment(defaultEnvironmentId, defaultJobId, defaultDesiredRunnersCount) + suite.nomadRunnerManager.RegisterEnvironment(defaultEnvironmentId, tests.DefaultJobId, defaultDesiredRunnersCount) } func (suite *ManagerTestSuite) AddIdleRunnerForDefaultEnvironment(r Runner) { - jobEntity, _ := suite.nomadRunnerManager.jobs.Get(defaultEnvironmentId.toString()) - jobEntity.(*NomadJob).idleRunners.Add(r) + job, _ := suite.nomadRunnerManager.jobs.Get(defaultEnvironmentId) + job.idleRunners.Add(r) } func (suite *ManagerTestSuite) waitForRunnerRefresh() { @@ -54,10 +55,10 @@ func (suite *ManagerTestSuite) waitForRunnerRefresh() { } func (suite *ManagerTestSuite) TestRegisterEnvironmentAddsNewJob() { - suite.nomadRunnerManager.RegisterEnvironment(anotherEnvironmentId, defaultJobId, defaultDesiredRunnersCount) - jobEntity, ok := suite.nomadRunnerManager.jobs.Get(defaultEnvironmentId.toString()) + suite.nomadRunnerManager.RegisterEnvironment(anotherEnvironmentId, tests.DefaultJobId, defaultDesiredRunnersCount) + job, ok := suite.nomadRunnerManager.jobs.Get(defaultEnvironmentId) suite.True(ok) - suite.NotNil(jobEntity) + suite.NotNil(job) } func (suite *ManagerTestSuite) TestClaimReturnsNotFoundErrorIfEnvironmentNotFound() { @@ -89,7 +90,7 @@ func (suite *ManagerTestSuite) TestClaimReturnsNoRunnerOfDifferentEnvironment() func (suite *ManagerTestSuite) TestClaimDoesNotReturnTheSameRunnerTwice() { suite.AddIdleRunnerForDefaultEnvironment(suite.exerciseRunner) - suite.AddIdleRunnerForDefaultEnvironment(NewRunner(anotherRunnerId)) + suite.AddIdleRunnerForDefaultEnvironment(NewRunner(tests.AnotherRunnerId)) firstReceivedRunner, _ := suite.nomadRunnerManager.Claim(defaultEnvironmentId) secondReceivedRunner, _ := suite.nomadRunnerManager.Claim(defaultEnvironmentId) @@ -103,7 +104,7 @@ func (suite *ManagerTestSuite) TestClaimThrowsAnErrorIfNoRunnersAvailable() { } func (suite *ManagerTestSuite) TestClaimAddsRunnerToUsedRunners() { - suite.mockRunnerQueries([]string{defaultRunnerId}) + suite.mockRunnerQueries([]string{tests.DefaultRunnerId}) suite.waitForRunnerRefresh() receivedRunner, _ := suite.nomadRunnerManager.Claim(defaultEnvironmentId) savedRunner, ok := suite.nomadRunnerManager.usedRunners.Get(receivedRunner.Id()) @@ -119,7 +120,7 @@ func (suite *ManagerTestSuite) TestGetReturnsRunnerIfRunnerIsUsed() { } func (suite *ManagerTestSuite) TestGetReturnsErrorIfRunnerNotFound() { - savedRunner, err := suite.nomadRunnerManager.Get(defaultRunnerId) + savedRunner, err := suite.nomadRunnerManager.Get(tests.DefaultRunnerId) suite.Nil(savedRunner) suite.Error(err) } @@ -147,33 +148,33 @@ func (suite *ManagerTestSuite) TestReturnReturnsErrorWhenApiCallFailed() { } func (suite *ManagerTestSuite) TestRefreshFetchesRunners() { - suite.mockRunnerQueries([]string{defaultRunnerId}) + suite.mockRunnerQueries([]string{tests.DefaultRunnerId}) suite.waitForRunnerRefresh() - suite.apiMock.AssertCalled(suite.T(), "LoadRunners", defaultJobId) + suite.apiMock.AssertCalled(suite.T(), "LoadRunners", tests.DefaultJobId) } func (suite *ManagerTestSuite) TestNewRunnersFoundInRefreshAreAddedToIdleRunners() { - suite.mockRunnerQueries([]string{defaultRunnerId}) + suite.mockRunnerQueries([]string{tests.DefaultRunnerId}) suite.waitForRunnerRefresh() - jobEntity, _ := suite.nomadRunnerManager.jobs.Get(defaultEnvironmentId.toString()) - _, ok := jobEntity.(*NomadJob).idleRunners.Get(defaultRunnerId) + job, _ := suite.nomadRunnerManager.jobs.Get(defaultEnvironmentId) + _, ok := job.idleRunners.Get(tests.DefaultRunnerId) suite.True(ok) } func (suite *ManagerTestSuite) TestRefreshScalesJob() { - suite.mockRunnerQueries([]string{defaultRunnerId}) + suite.mockRunnerQueries([]string{tests.DefaultRunnerId}) suite.waitForRunnerRefresh() // use one runner to necessitate rescaling _, _ = suite.nomadRunnerManager.Claim(defaultEnvironmentId) suite.waitForRunnerRefresh() - suite.apiMock.AssertCalled(suite.T(), "SetJobScale", defaultJobId, defaultDesiredRunnersCount+1, "Runner Requested") + suite.apiMock.AssertCalled(suite.T(), "SetJobScale", tests.DefaultJobId, defaultDesiredRunnersCount+1, "Runner Requested") } func (suite *ManagerTestSuite) TestRefreshAddsRunnerToPool() { - suite.mockRunnerQueries([]string{defaultRunnerId}) + suite.mockRunnerQueries([]string{tests.DefaultRunnerId}) suite.waitForRunnerRefresh() - jobEntity, _ := suite.nomadRunnerManager.jobs.Get(defaultEnvironmentId.toString()) - poolRunner, ok := jobEntity.(*NomadJob).idleRunners.Get(defaultRunnerId) + job, _ := suite.nomadRunnerManager.jobs.Get(defaultEnvironmentId) + poolRunner, ok := job.idleRunners.Get(tests.DefaultRunnerId) suite.True(ok) - suite.Equal(defaultRunnerId, poolRunner.Id()) + suite.Equal(tests.DefaultRunnerId, poolRunner.Id()) } diff --git a/runner/nomad_job_storage.go b/runner/nomad_job_storage.go new file mode 100644 index 0000000..0c7e7a0 --- /dev/null +++ b/runner/nomad_job_storage.go @@ -0,0 +1,61 @@ +package runner + +import ( + "sync" +) + +// NomadJobStorage is an interface for storing NomadJobs. +type NomadJobStorage interface { + // Add adds a job to the storage. + // It overwrites the old job if one with the same id was already stored. + Add(job *NomadJob) + + // Get returns a job from the storage. + // Iff the job does not exist in the store, ok will be false. + Get(id EnvironmentId) (job *NomadJob, ok bool) + + // Delete deletes the job with the passed id from the storage. It does nothing if no job with the id is present in the storage. + Delete(id EnvironmentId) + + // Length returns the number of currently stored jobs in the storage. + Length() int +} + +// localNomadJobStorage stores NomadJob objects in the local application memory. +type localNomadJobStorage struct { + sync.RWMutex + jobs map[EnvironmentId]*NomadJob +} + +// NewLocalNomadJobStorage responds with an empty localNomadJobStorage. +// This implementation stores the data thread-safe in the local application memory. +func NewLocalNomadJobStorage() *localNomadJobStorage { + return &localNomadJobStorage{ + jobs: make(map[EnvironmentId]*NomadJob), + } +} + +func (s *localNomadJobStorage) Add(job *NomadJob) { + s.Lock() + defer s.Unlock() + s.jobs[job.Id()] = job +} + +func (s *localNomadJobStorage) Get(id EnvironmentId) (job *NomadJob, ok bool) { + s.RLock() + defer s.RUnlock() + job, ok = s.jobs[id] + return +} + +func (s *localNomadJobStorage) Delete(id EnvironmentId) { + s.Lock() + defer s.Unlock() + delete(s.jobs, id) +} + +func (s *localNomadJobStorage) Length() int { + s.RLock() + defer s.RUnlock() + return len(s.jobs) +} diff --git a/runner/nomad_job_storage_test.go b/runner/nomad_job_storage_test.go new file mode 100644 index 0000000..f8ce8a8 --- /dev/null +++ b/runner/nomad_job_storage_test.go @@ -0,0 +1,77 @@ +package runner + +import ( + "github.com/stretchr/testify/suite" + "gitlab.hpi.de/codeocean/codemoon/poseidon/tests" + "testing" +) + +func TestJobStoreTestSuite(t *testing.T) { + suite.Run(t, new(JobStoreTestSuite)) +} + +type JobStoreTestSuite struct { + suite.Suite + jobStorage *localNomadJobStorage + job *NomadJob +} + +func (suite *JobStoreTestSuite) SetupTest() { + suite.jobStorage = NewLocalNomadJobStorage() + suite.job = &NomadJob{environmentId: defaultEnvironmentId, jobId: tests.DefaultJobId} +} + +func (suite *JobStoreTestSuite) TestAddedJobCanBeRetrieved() { + suite.jobStorage.Add(suite.job) + retrievedJob, ok := suite.jobStorage.Get(suite.job.Id()) + suite.True(ok, "A saved runner should be retrievable") + suite.Equal(suite.job, retrievedJob) +} + +func (suite *JobStoreTestSuite) TestJobWithSameIdOverwritesOldOne() { + otherJobWithSameId := &NomadJob{environmentId: defaultEnvironmentId} + // assure runner is actually different + otherJobWithSameId.jobId = tests.AnotherJobId + suite.NotEqual(suite.job, otherJobWithSameId) + + suite.jobStorage.Add(suite.job) + suite.jobStorage.Add(otherJobWithSameId) + retrievedJob, _ := suite.jobStorage.Get(suite.job.Id()) + suite.NotEqual(suite.job, retrievedJob) + suite.Equal(otherJobWithSameId, retrievedJob) +} + +func (suite *JobStoreTestSuite) TestDeletedJobIsNotAccessible() { + suite.jobStorage.Add(suite.job) + suite.jobStorage.Delete(suite.job.Id()) + retrievedRunner, ok := suite.jobStorage.Get(suite.job.Id()) + suite.Nil(retrievedRunner) + suite.False(ok, "A deleted runner should not be accessible") +} + +func (suite *JobStoreTestSuite) TestLenOfEmptyPoolIsZero() { + suite.Equal(0, suite.jobStorage.Length()) +} + +func (suite *JobStoreTestSuite) TestLenChangesOnStoreContentChange() { + suite.Run("len increases when job is added", func() { + suite.jobStorage.Add(suite.job) + suite.Equal(1, suite.jobStorage.Length()) + }) + + suite.Run("len does not increase when job with same id is added", func() { + suite.jobStorage.Add(suite.job) + suite.Equal(1, suite.jobStorage.Length()) + }) + + suite.Run("len increases again when different job is added", func() { + anotherJob := &NomadJob{environmentId: anotherEnvironmentId} + suite.jobStorage.Add(anotherJob) + suite.Equal(2, suite.jobStorage.Length()) + }) + + suite.Run("len decreases when job is deleted", func() { + suite.jobStorage.Delete(suite.job.Id()) + suite.Equal(1, suite.jobStorage.Length()) + }) +} diff --git a/runner/pool.go b/runner/pool.go deleted file mode 100644 index d1c8050..0000000 --- a/runner/pool.go +++ /dev/null @@ -1,70 +0,0 @@ -package runner - -import ( - "gitlab.hpi.de/codeocean/codemoon/poseidon/store" - "sync" -) - -// Pool is a type of entity store that should store runner entities. -type Pool interface { - store.EntityStore - - // Sample returns and removes an arbitrary entity from the pool. - // ok is true iff a runner was returned. - Sample() (r Runner, ok bool) -} - -// localRunnerPool stores runner objects in the local application memory. -// ToDo: Create implementation that use some persistent storage like a database -type localRunnerPool struct { - sync.RWMutex - runners map[string]Runner -} - -// NewLocalRunnerPool responds with a Pool implementation. -// This implementation stores the data thread-safe in the local application memory -func NewLocalRunnerPool() *localRunnerPool { - return &localRunnerPool{ - runners: make(map[string]Runner), - } -} - -func (pool *localRunnerPool) Add(r store.Entity) { - pool.Lock() - defer pool.Unlock() - runnerEntity, ok := r.(Runner) - if !ok { - log. - WithField("pool", pool). - WithField("entity", r). - Fatal("Entity of type runner.Runner was expected, but wasn't given.") - } - pool.runners[r.Id()] = runnerEntity -} - -func (pool *localRunnerPool) Get(id string) (r store.Entity, ok bool) { - pool.RLock() - defer pool.RUnlock() - r, ok = pool.runners[id] - return -} - -func (pool *localRunnerPool) Delete(id string) { - pool.Lock() - defer pool.Unlock() - delete(pool.runners, id) -} - -func (pool *localRunnerPool) Sample() (Runner, bool) { - pool.Lock() - defer pool.Unlock() - for _, runner := range pool.runners { - delete(pool.runners, runner.Id()) - return runner, true - } - return nil, false -} - -func (pool *localRunnerPool) Len() int { - return len(pool.runners) -} diff --git a/runner/pool_test.go b/runner/pool_test.go deleted file mode 100644 index 2d9f28a..0000000 --- a/runner/pool_test.go +++ /dev/null @@ -1,125 +0,0 @@ -package runner - -import ( - "github.com/sirupsen/logrus" - "github.com/sirupsen/logrus/hooks/test" - "github.com/stretchr/testify/suite" - "testing" -) - -func TestRunnerPoolTestSuite(t *testing.T) { - suite.Run(t, new(RunnerPoolTestSuite)) -} - -type RunnerPoolTestSuite struct { - suite.Suite - runnerPool *localRunnerPool - runner Runner -} - -func (suite *RunnerPoolTestSuite) SetupTest() { - suite.runnerPool = NewLocalRunnerPool() - suite.runner = NewRunner(defaultRunnerId) -} - -func (suite *RunnerPoolTestSuite) TestAddInvalidEntityTypeThrowsFatal() { - var hook *test.Hook - logger, hook := test.NewNullLogger() - // don't terminate program on fatal log entry - logger.ExitFunc = func(int) {} - log = logger.WithField("pkg", "environment") - - dummyEntity := DummyEntity{} - suite.runnerPool.Add(dummyEntity) - suite.Equal(logrus.FatalLevel, hook.LastEntry().Level) - suite.Equal(dummyEntity, hook.LastEntry().Data["entity"]) -} - -func (suite *RunnerPoolTestSuite) TestAddValidEntityDoesNotThrowFatal() { - var hook *test.Hook - logger, hook := test.NewNullLogger() - log = logger.WithField("pkg", "environment") - - suite.runnerPool.Add(suite.runner) - // currently, the Add method does not log anything else. adjust if necessary - suite.Nil(hook.LastEntry()) -} - -func (suite *RunnerPoolTestSuite) TestAddedRunnerCanBeRetrieved() { - suite.runnerPool.Add(suite.runner) - retrievedRunner, ok := suite.runnerPool.Get(suite.runner.Id()) - suite.True(ok, "A saved runner should be retrievable") - suite.Equal(suite.runner, retrievedRunner) -} - -func (suite *RunnerPoolTestSuite) TestRunnerWithSameIdOverwritesOldOne() { - otherRunnerWithSameId := NewRunner(suite.runner.Id()) - // assure runner is actually different - suite.NotEqual(suite.runner, otherRunnerWithSameId) - - suite.runnerPool.Add(suite.runner) - suite.runnerPool.Add(otherRunnerWithSameId) - retrievedRunner, _ := suite.runnerPool.Get(suite.runner.Id()) - suite.NotEqual(suite.runner, retrievedRunner) - suite.Equal(otherRunnerWithSameId, retrievedRunner) -} - -func (suite *RunnerPoolTestSuite) TestDeletedRunnersAreNotAccessible() { - suite.runnerPool.Add(suite.runner) - suite.runnerPool.Delete(suite.runner.Id()) - retrievedRunner, ok := suite.runnerPool.Get(suite.runner.Id()) - suite.Nil(retrievedRunner) - suite.False(ok, "A deleted runner should not be accessible") -} - -func (suite *RunnerPoolTestSuite) TestSampleReturnsRunnerWhenOneIsAvailable() { - suite.runnerPool.Add(suite.runner) - sampledRunner, ok := suite.runnerPool.Sample() - suite.NotNil(sampledRunner) - suite.True(ok) -} - -func (suite *RunnerPoolTestSuite) TestSampleReturnsFalseWhenNoneIsAvailable() { - sampledRunner, ok := suite.runnerPool.Sample() - suite.Nil(sampledRunner) - suite.False(ok) -} - -func (suite *RunnerPoolTestSuite) TestSampleRemovesRunnerFromPool() { - suite.runnerPool.Add(suite.runner) - sampledRunner, _ := suite.runnerPool.Sample() - _, ok := suite.runnerPool.Get(sampledRunner.Id()) - suite.False(ok) -} - -func (suite *RunnerPoolTestSuite) TestLenOfEmptyPoolIsZero() { - suite.Equal(0, suite.runnerPool.Len()) -} - -func (suite *RunnerPoolTestSuite) TestLenChangesOnStoreContentChange() { - suite.Run("len increases when runner is added", func() { - suite.runnerPool.Add(suite.runner) - suite.Equal(1, suite.runnerPool.Len()) - }) - - suite.Run("len does not increase when runner with same id is added", func() { - suite.runnerPool.Add(suite.runner) - suite.Equal(1, suite.runnerPool.Len()) - }) - - suite.Run("len increases again when different runner is added", func() { - anotherRunner := NewRunner(anotherRunnerId) - suite.runnerPool.Add(anotherRunner) - suite.Equal(2, suite.runnerPool.Len()) - }) - - suite.Run("len decreases when runner is deleted", func() { - suite.runnerPool.Delete(suite.runner.Id()) - suite.Equal(1, suite.runnerPool.Len()) - }) - - suite.Run("len decreases when runner is sampled", func() { - _, _ = suite.runnerPool.Sample() - suite.Equal(0, suite.runnerPool.Len()) - }) -} diff --git a/runner/runner.go b/runner/runner.go index 4fd5b13..34de519 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -5,7 +5,6 @@ import ( "encoding/json" "github.com/google/uuid" "gitlab.hpi.de/codeocean/codemoon/poseidon/api/dto" - "gitlab.hpi.de/codeocean/codemoon/poseidon/store" "sync" ) @@ -21,7 +20,8 @@ const ( ) type Runner interface { - store.Entity + // Id returns the id of the runner. + Id() string // AddExecution saves the supplied ExecutionRequest for the runner and returns an ExecutionId to retrieve it again. AddExecution(dto.ExecutionRequest) (ExecutionId, error) diff --git a/runner/storage.go b/runner/storage.go new file mode 100644 index 0000000..be0e86d --- /dev/null +++ b/runner/storage.go @@ -0,0 +1,74 @@ +package runner + +import ( + "sync" +) + +// Storage is an interface for storing runners. +type Storage interface { + // Add adds an runner to the storage. + // It overwrites the old runner if one with the same id was already stored. + Add(Runner) + + // Get returns a runner from the storage. + // Iff the runner does not exist in the storage, ok will be false. + Get(id string) (r Runner, ok bool) + + // Delete deletes the runner with the passed id from the storage. It does nothing if no runner with the id is present in the store. + Delete(id string) + + // Length returns the number of currently stored runners in the storage. + Length() int + + // Sample returns and removes an arbitrary runner from the storage. + // ok is true iff a runner was returned. + Sample() (r Runner, ok bool) +} + +// localRunnerStorage stores runner objects in the local application memory. +// ToDo: Create implementation that use some persistent storage like a database +type localRunnerStorage struct { + sync.RWMutex + runners map[string]Runner +} + +// NewLocalRunnerStorage responds with a Storage implementation. +// This implementation stores the data thread-safe in the local application memory +func NewLocalRunnerStorage() *localRunnerStorage { + return &localRunnerStorage{ + runners: make(map[string]Runner), + } +} + +func (s *localRunnerStorage) Add(r Runner) { + s.Lock() + defer s.Unlock() + s.runners[r.Id()] = r +} + +func (s *localRunnerStorage) Get(id string) (r Runner, ok bool) { + s.RLock() + defer s.RUnlock() + r, ok = s.runners[id] + return +} + +func (s *localRunnerStorage) Delete(id string) { + s.Lock() + defer s.Unlock() + delete(s.runners, id) +} + +func (s *localRunnerStorage) Sample() (Runner, bool) { + s.Lock() + defer s.Unlock() + for _, runner := range s.runners { + delete(s.runners, runner.Id()) + return runner, true + } + return nil, false +} + +func (s *localRunnerStorage) Length() int { + return len(s.runners) +} diff --git a/runner/storage_test.go b/runner/storage_test.go new file mode 100644 index 0000000..34f69cc --- /dev/null +++ b/runner/storage_test.go @@ -0,0 +1,101 @@ +package runner + +import ( + "github.com/stretchr/testify/suite" + "gitlab.hpi.de/codeocean/codemoon/poseidon/tests" + "testing" +) + +func TestRunnerPoolTestSuite(t *testing.T) { + suite.Run(t, new(RunnerPoolTestSuite)) +} + +type RunnerPoolTestSuite struct { + suite.Suite + runnerStorage *localRunnerStorage + runner Runner +} + +func (suite *RunnerPoolTestSuite) SetupTest() { + suite.runnerStorage = NewLocalRunnerStorage() + suite.runner = NewRunner(tests.DefaultRunnerId) +} + +func (suite *RunnerPoolTestSuite) TestAddedRunnerCanBeRetrieved() { + suite.runnerStorage.Add(suite.runner) + retrievedRunner, ok := suite.runnerStorage.Get(suite.runner.Id()) + suite.True(ok, "A saved runner should be retrievable") + suite.Equal(suite.runner, retrievedRunner) +} + +func (suite *RunnerPoolTestSuite) TestRunnerWithSameIdOverwritesOldOne() { + otherRunnerWithSameId := NewRunner(suite.runner.Id()) + // assure runner is actually different + suite.NotEqual(suite.runner, otherRunnerWithSameId) + + suite.runnerStorage.Add(suite.runner) + suite.runnerStorage.Add(otherRunnerWithSameId) + retrievedRunner, _ := suite.runnerStorage.Get(suite.runner.Id()) + suite.NotEqual(suite.runner, retrievedRunner) + suite.Equal(otherRunnerWithSameId, retrievedRunner) +} + +func (suite *RunnerPoolTestSuite) TestDeletedRunnersAreNotAccessible() { + suite.runnerStorage.Add(suite.runner) + suite.runnerStorage.Delete(suite.runner.Id()) + retrievedRunner, ok := suite.runnerStorage.Get(suite.runner.Id()) + suite.Nil(retrievedRunner) + suite.False(ok, "A deleted runner should not be accessible") +} + +func (suite *RunnerPoolTestSuite) TestSampleReturnsRunnerWhenOneIsAvailable() { + suite.runnerStorage.Add(suite.runner) + sampledRunner, ok := suite.runnerStorage.Sample() + suite.NotNil(sampledRunner) + suite.True(ok) +} + +func (suite *RunnerPoolTestSuite) TestSampleReturnsFalseWhenNoneIsAvailable() { + sampledRunner, ok := suite.runnerStorage.Sample() + suite.Nil(sampledRunner) + suite.False(ok) +} + +func (suite *RunnerPoolTestSuite) TestSampleRemovesRunnerFromPool() { + suite.runnerStorage.Add(suite.runner) + sampledRunner, _ := suite.runnerStorage.Sample() + _, ok := suite.runnerStorage.Get(sampledRunner.Id()) + suite.False(ok) +} + +func (suite *RunnerPoolTestSuite) TestLenOfEmptyPoolIsZero() { + suite.Equal(0, suite.runnerStorage.Length()) +} + +func (suite *RunnerPoolTestSuite) TestLenChangesOnStoreContentChange() { + suite.Run("len increases when runner is added", func() { + suite.runnerStorage.Add(suite.runner) + suite.Equal(1, suite.runnerStorage.Length()) + }) + + suite.Run("len does not increase when runner with same id is added", func() { + suite.runnerStorage.Add(suite.runner) + suite.Equal(1, suite.runnerStorage.Length()) + }) + + suite.Run("len increases again when different runner is added", func() { + anotherRunner := NewRunner(tests.AnotherRunnerId) + suite.runnerStorage.Add(anotherRunner) + suite.Equal(2, suite.runnerStorage.Length()) + }) + + suite.Run("len decreases when runner is deleted", func() { + suite.runnerStorage.Delete(suite.runner.Id()) + suite.Equal(1, suite.runnerStorage.Length()) + }) + + suite.Run("len decreases when runner is sampled", func() { + _, _ = suite.runnerStorage.Sample() + suite.Equal(0, suite.runnerStorage.Length()) + }) +} diff --git a/store/entity_store.go b/store/entity_store.go deleted file mode 100644 index 8cd6a20..0000000 --- a/store/entity_store.go +++ /dev/null @@ -1,24 +0,0 @@ -package store - -// EntityStore is the general interface for storing different entity types. -type EntityStore interface { - // Add adds an entity to the store. - // It overwrites the old entity if one with the same id was already stored. - // Returns an error if the entity is of invalid type for the concrete implementation. - Add(entity Entity) - - // Get returns a entity from the store. - // If the entity does not exist in the store, ok will be false. - Get(id string) (entity Entity, ok bool) - - // Delete deletes the entity with the passed id from the store. - Delete(id string) - - // Len returns the number of currently stored entities in the store. - Len() int -} - -type Entity interface { - // Id returns the id of the given entity. - Id() string -} diff --git a/tests/test_constants.go b/tests/test_constants.go new file mode 100644 index 0000000..6317586 --- /dev/null +++ b/tests/test_constants.go @@ -0,0 +1,10 @@ +package tests + +const ( + DefaultEnvironmentIdAsInteger = 0 + AnotherEnvironmentIdAsInteger = 42 + DefaultJobId = "s0m3-j0b-1d" + AnotherJobId = "4n0th3r-j0b-1d" + DefaultRunnerId = "s0m3-r4nd0m-1d" + AnotherRunnerId = "4n0th3r-runn3r-1d" +)