diff --git a/environment/execution_environment.go b/environment/execution_environment.go index 176ef02..e27d6a4 100644 --- a/environment/execution_environment.go +++ b/environment/execution_environment.go @@ -2,7 +2,6 @@ package environment import ( "errors" - "gitlab.hpi.de/codeocean/codemoon/poseidon/environment/pool" "gitlab.hpi.de/codeocean/codemoon/poseidon/logging" "gitlab.hpi.de/codeocean/codemoon/poseidon/nomad" "gitlab.hpi.de/codeocean/codemoon/poseidon/runner" @@ -28,7 +27,7 @@ type NomadExecutionEnvironment struct { id int jobId string availableRunners chan runner.Runner - allRunners pool.RunnerPool + allRunners RunnerPool nomadApiClient nomad.ExecutorApi } @@ -36,7 +35,7 @@ var executionEnvironment ExecutionEnvironment // DebugInit initializes one execution environment so that its runners can be provided. // ToDo: This should be replaced by a create Execution Environment route -func DebugInit(runnersPool pool.RunnerPool, nomadApi nomad.ExecutorApi) { +func DebugInit(runnersPool RunnerPool, nomadApi nomad.ExecutorApi) { executionEnvironment = &NomadExecutionEnvironment{ id: 0, jobId: "python", @@ -75,7 +74,12 @@ func (environment *NomadExecutionEnvironment) Refresh() { for _, r := range environment.unusedRunners(runners) { // ToDo: Listen on Nomad event stream log.Printf("Adding allocation %+v", r) - environment.allRunners.AddRunner(r) + if err := environment.allRunners.Add(r); err != nil { + log. + WithError(err). + WithField("runner", r). + Fatal("Invalid storage implementation used for object of type") + } environment.availableRunners <- r } jobScale, err := environment.nomadApiClient.GetJobScale(environment.jobId) @@ -98,7 +102,7 @@ func (environment *NomadExecutionEnvironment) Refresh() { func (environment *NomadExecutionEnvironment) unusedRunners(fetchedRunnerIds []string) (newRunners []runner.Runner) { newRunners = make([]runner.Runner, 0) for _, runnerId := range fetchedRunnerIds { - _, ok := environment.allRunners.GetRunner(runnerId) + _, ok := environment.allRunners.Get(runnerId) if !ok { newRunners = append(newRunners, runner.NewExerciseRunner(runnerId)) } diff --git a/environment/execution_environment_test.go b/environment/execution_environment_test.go index f9a2367..43ba142 100644 --- a/environment/execution_environment_test.go +++ b/environment/execution_environment_test.go @@ -4,14 +4,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" - "gitlab.hpi.de/codeocean/codemoon/poseidon/environment/pool" "gitlab.hpi.de/codeocean/codemoon/poseidon/mocks" "gitlab.hpi.de/codeocean/codemoon/poseidon/runner" "testing" "time" ) -const runnerId = "s0m3-r4nd0m-1d" const anotherRunnerId = "4n0th3r-1d" const jobId = "4n0th3r-1d" @@ -28,9 +26,9 @@ type GetNextRunnerTestSuite struct { func (suite *GetNextRunnerTestSuite) SetupTest() { suite.nomadExecutionEnvironment = &NomadExecutionEnvironment{ availableRunners: make(chan runner.Runner, 50), - allRunners: pool.NewLocalRunnerPool(), + allRunners: NewLocalRunnerPool(), } - suite.exerciseRunner = runner.NewExerciseRunner(runnerId) + suite.exerciseRunner = CreateTestRunner() } func (suite *GetNextRunnerTestSuite) TestGetNextRunnerReturnsRunnerIfAvailable() { @@ -64,7 +62,7 @@ func (suite *GetNextRunnerTestSuite) TestGetNextRunnerThrowsAnErrorIfNoRunnersAv } func TestRefreshFetchRunners(t *testing.T) { - apiMock, environment := newRefreshMock([]string{runnerId}, pool.NewLocalRunnerPool()) + apiMock, environment := newRefreshMock([]string{RunnerId}, NewLocalRunnerPool()) // ToDo: Terminate Refresh when test finished (also in other tests) go environment.Refresh() _, _ = environment.NextRunner() @@ -72,14 +70,14 @@ func TestRefreshFetchRunners(t *testing.T) { } func TestRefreshFetchesRunnersIntoChannel(t *testing.T) { - _, environment := newRefreshMock([]string{runnerId}, pool.NewLocalRunnerPool()) + _, environment := newRefreshMock([]string{RunnerId}, NewLocalRunnerPool()) go environment.Refresh() availableRunner, _ := environment.NextRunner() - assert.Equal(t, availableRunner.Id(), runnerId) + assert.Equal(t, availableRunner.Id(), RunnerId) } func TestRefreshScalesJob(t *testing.T) { - apiMock, environment := newRefreshMock([]string{runnerId}, pool.NewLocalRunnerPool()) + apiMock, environment := newRefreshMock([]string{RunnerId}, NewLocalRunnerPool()) go environment.Refresh() _, _ = environment.NextRunner() time.Sleep(100 * time.Millisecond) // ToDo: Be safe this test is not flaky @@ -87,16 +85,16 @@ func TestRefreshScalesJob(t *testing.T) { } func TestRefreshAddsRunnerToPool(t *testing.T) { - runnersInUse := pool.NewLocalRunnerPool() - _, environment := newRefreshMock([]string{runnerId}, runnersInUse) + runnersInUse := NewLocalRunnerPool() + _, environment := newRefreshMock([]string{RunnerId}, runnersInUse) go environment.Refresh() availableRunner, _ := environment.NextRunner() - poolRunner, ok := runnersInUse.GetRunner(availableRunner.Id()) - assert.True(t, ok, "The requested runner is added to the pool") - assert.Equal(t, availableRunner, poolRunner, "The requested runner equals the runner added to the pool") + poolRunner, ok := runnersInUse.Get(availableRunner.Id()) + assert.True(t, ok) + assert.Equal(t, availableRunner, poolRunner) } -func newRefreshMock(returnedRunnerIds []string, allRunners pool.RunnerPool) (apiClient *mocks.ExecutorApi, environment *NomadExecutionEnvironment) { +func newRefreshMock(returnedRunnerIds []string, allRunners RunnerPool) (apiClient *mocks.ExecutorApi, environment *NomadExecutionEnvironment) { apiClient = &mocks.ExecutorApi{} apiClient.On("LoadRunners", jobId).Return(returnedRunnerIds, nil) apiClient.On("GetJobScale", jobId).Return(len(returnedRunnerIds), nil) diff --git a/environment/pool/pool.go b/environment/pool/pool.go deleted file mode 100644 index 3a5e169..0000000 --- a/environment/pool/pool.go +++ /dev/null @@ -1,51 +0,0 @@ -package pool - -import ( - "gitlab.hpi.de/codeocean/codemoon/poseidon/runner" - "sync" -) - -// RunnerPool is the interface for storing runners -// ToDo: Create interface implementation that use some persistent storage like a database -type RunnerPool interface { - // AddRunner adds a provided runner to the pool storage - AddRunner(runner.Runner) - // GetRunner returns a runner from the pool storage - // If the requested runner is not stored 'ok' will be false - GetRunner(id string) (r runner.Runner, ok bool) - // DeleteRunner deletes the runner with the passed id from the pool storage - DeleteRunner(id string) -} - -// localRunnerPool stores runner objects in the local application memory -type localRunnerPool struct { - sync.RWMutex - runners map[string]runner.Runner -} - -// NewLocalRunnerPool responds with a RunnerPool implementation -// This implementation stores the data thread-safe in the local application memory -func NewLocalRunnerPool() *localRunnerPool { - return &localRunnerPool{ - runners: make(map[string]runner.Runner), - } -} - -func (pool *localRunnerPool) AddRunner(r runner.Runner) { - pool.Lock() - defer pool.Unlock() - pool.runners[r.Id()] = r -} - -func (pool *localRunnerPool) GetRunner(id string) (r runner.Runner, ok bool) { - pool.RLock() - defer pool.RUnlock() - r, ok = pool.runners[id] - return -} - -func (pool *localRunnerPool) DeleteRunner(id string) { - pool.Lock() - defer pool.Unlock() - delete(pool.runners, id) -} diff --git a/environment/pool/pool_test.go b/environment/pool/pool_test.go deleted file mode 100644 index 3f71c36..0000000 --- a/environment/pool/pool_test.go +++ /dev/null @@ -1,24 +0,0 @@ -package pool - -import ( - "github.com/stretchr/testify/assert" - "gitlab.hpi.de/codeocean/codemoon/poseidon/runner" - "testing" -) - -const runnerId = "s0m3-r4nd0m-1d" - -func TestAddedRunnerCanBeRetrieved(t *testing.T) { - runnerPool := NewLocalRunnerPool() - runnerPool.AddRunner(runner.NewExerciseRunner(runnerId)) - _, ok := runnerPool.GetRunner(runnerId) - assert.True(t, ok, "A saved runner should be retrievable") -} - -func TestDeletedRunnersAreNotAccessible(t *testing.T) { - pool := NewLocalRunnerPool() - pool.AddRunner(runner.NewExerciseRunner(runnerId)) - pool.DeleteRunner(runnerId) - _, ok := pool.GetRunner(runnerId) - assert.False(t, ok, "A deleted runner should not be accessible") -} diff --git a/environment/runner_pool.go b/environment/runner_pool.go new file mode 100644 index 0000000..44dc89e --- /dev/null +++ b/environment/runner_pool.go @@ -0,0 +1,52 @@ +package environment + +import ( + "errors" + "gitlab.hpi.de/codeocean/codemoon/poseidon/runner" + "gitlab.hpi.de/codeocean/codemoon/poseidon/store" + "sync" +) + +// RunnerPool is a type of entity store that should store runner entities. +type RunnerPool interface { + store.EntityStore +} + +// 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.Runner +} + +// NewLocalRunnerPool responds with a RunnerPool implementation +// This implementation stores the data thread-safe in the local application memory +func NewLocalRunnerPool() *localRunnerPool { + return &localRunnerPool{ + runners: make(map[string]runner.Runner), + } +} + +func (pool *localRunnerPool) Add(r store.Entity) (err error) { + pool.Lock() + defer pool.Unlock() + runnerEntity, ok := r.(runner.Runner) + if !ok { + return errors.New("Entity of type runner.Runner was expected, but wasn't given.") + } + pool.runners[r.Id()] = runnerEntity + return nil +} + +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) +} diff --git a/environment/runner_pool_test.go b/environment/runner_pool_test.go new file mode 100644 index 0000000..36d893a --- /dev/null +++ b/environment/runner_pool_test.go @@ -0,0 +1,66 @@ +package environment + +import ( + "github.com/stretchr/testify/suite" + "gitlab.hpi.de/codeocean/codemoon/poseidon/runner" + "testing" +) + +type DummyEntity struct{} + +func (DummyEntity) Id() string { + return "" +} + +func TestRunnerPoolTestSuite(t *testing.T) { + suite.Run(t, new(RunnerPoolTestSuite)) +} + +type RunnerPoolTestSuite struct { + suite.Suite + runnerPool *localRunnerPool + runner runner.Runner +} + +func (suite *RunnerPoolTestSuite) SetupTest() { + suite.runnerPool = NewLocalRunnerPool() + suite.runner = CreateTestRunner() +} + +func (suite *RunnerPoolTestSuite) TestAddInvalidEntityTypeReturnsError() { + dummyEntity := DummyEntity{} + err := suite.runnerPool.Add(dummyEntity) + suite.Error(err) +} + +func (suite *RunnerPoolTestSuite) TestAddValidEntityReturnsNoError() { + err := suite.runnerPool.Add(suite.runner) + suite.NoError(err) +} + +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 := runner.NewExerciseRunner(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") +} diff --git a/environment/test_constants.go b/environment/test_constants.go new file mode 100644 index 0000000..fd94c1a --- /dev/null +++ b/environment/test_constants.go @@ -0,0 +1,9 @@ +package environment + +import "gitlab.hpi.de/codeocean/codemoon/poseidon/runner" + +const RunnerId = "s0m3-r4nd0m-1d" + +func CreateTestRunner() runner.Runner { + return runner.NewExerciseRunner(RunnerId) +} diff --git a/main.go b/main.go index 46d31c4..8e37dce 100644 --- a/main.go +++ b/main.go @@ -5,7 +5,6 @@ import ( "gitlab.hpi.de/codeocean/codemoon/poseidon/api" "gitlab.hpi.de/codeocean/codemoon/poseidon/config" "gitlab.hpi.de/codeocean/codemoon/poseidon/environment" - "gitlab.hpi.de/codeocean/codemoon/poseidon/environment/pool" "gitlab.hpi.de/codeocean/codemoon/poseidon/logging" "gitlab.hpi.de/codeocean/codemoon/poseidon/nomad" "net/http" diff --git a/runner/runner.go b/runner/runner.go index baa5978..a815a4c 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -5,6 +5,7 @@ import ( "encoding/json" "github.com/google/uuid" "gitlab.hpi.de/codeocean/codemoon/poseidon/api/dto" + "gitlab.hpi.de/codeocean/codemoon/poseidon/store" "sync" ) @@ -28,15 +29,14 @@ const ( ) type Runner interface { + store.Entity + // SetStatus sets the status of the runner. SetStatus(Status) // Status gets the status of the runner. Status() Status - // Id returns the id of the runner. - Id() string - // Execution looks up an ExecutionId for the runner and returns the associated RunnerRequest. // If this request does not exit, ok is false, else true. Execution(ExecutionId) (request dto.ExecutionRequest, ok bool) diff --git a/store/entity_store.go b/store/entity_store.go new file mode 100644 index 0000000..207c0a5 --- /dev/null +++ b/store/entity_store.go @@ -0,0 +1,21 @@ +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) (err error) + + // 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) +} + +type Entity interface { + // Id returns the id of the given entity. + Id() string +}