From 13052fa02138dbb4b84028860bfb645c4d50131f Mon Sep 17 00:00:00 2001
From: Jan-Eric Hellenberg <janeric.hellenberg@gmail.com>
Date: Fri, 7 May 2021 09:26:02 +0200
Subject: [PATCH] Throw fatal on wrong data type in runnerpool

---
 environment/execution_environment.go |  7 +-----
 environment/runner_pool.go           |  9 ++++----
 environment/runner_pool_test.go      | 34 ++++++++++++++++++++--------
 store/entity_store.go                |  2 +-
 4 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/environment/execution_environment.go b/environment/execution_environment.go
index e27d6a4..9992879 100644
--- a/environment/execution_environment.go
+++ b/environment/execution_environment.go
@@ -74,12 +74,7 @@ func (environment *NomadExecutionEnvironment) Refresh() {
 		for _, r := range environment.unusedRunners(runners) {
 			// ToDo: Listen on Nomad event stream
 			log.Printf("Adding allocation %+v", 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.allRunners.Add(r)
 			environment.availableRunners <- r
 		}
 		jobScale, err := environment.nomadApiClient.GetJobScale(environment.jobId)
diff --git a/environment/runner_pool.go b/environment/runner_pool.go
index 44dc89e..64b15cc 100644
--- a/environment/runner_pool.go
+++ b/environment/runner_pool.go
@@ -1,7 +1,6 @@
 package environment
 
 import (
-	"errors"
 	"gitlab.hpi.de/codeocean/codemoon/poseidon/runner"
 	"gitlab.hpi.de/codeocean/codemoon/poseidon/store"
 	"sync"
@@ -27,15 +26,17 @@ func NewLocalRunnerPool() *localRunnerPool {
 	}
 }
 
-func (pool *localRunnerPool) Add(r store.Entity) (err error) {
+func (pool *localRunnerPool) Add(r store.Entity) {
 	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.")
+		log.
+			WithField("pool", pool).
+			WithField("entity", r).
+			Fatal("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) {
diff --git a/environment/runner_pool_test.go b/environment/runner_pool_test.go
index 36d893a..8fcf254 100644
--- a/environment/runner_pool_test.go
+++ b/environment/runner_pool_test.go
@@ -1,6 +1,8 @@
 package environment
 
 import (
+	"github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/suite"
 	"gitlab.hpi.de/codeocean/codemoon/poseidon/runner"
 	"testing"
@@ -27,19 +29,31 @@ func (suite *RunnerPoolTestSuite) SetupTest() {
 	suite.runner = CreateTestRunner()
 }
 
-func (suite *RunnerPoolTestSuite) TestAddInvalidEntityTypeReturnsError() {
+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{}
-	err := suite.runnerPool.Add(dummyEntity)
-	suite.Error(err)
+	suite.runnerPool.Add(dummyEntity)
+	suite.Equal(logrus.FatalLevel, hook.LastEntry().Level)
+	suite.Equal(dummyEntity, hook.LastEntry().Data["entity"])
 }
 
-func (suite *RunnerPoolTestSuite) TestAddValidEntityReturnsNoError() {
-	err := suite.runnerPool.Add(suite.runner)
-	suite.NoError(err)
+func (suite *RunnerPoolTestSuite) TestAddValidEntityThrowsFatal() {
+	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)
+	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)
@@ -50,15 +64,15 @@ func (suite *RunnerPoolTestSuite) TestRunnerWithSameIdOverwritesOldOne() {
 	// assure runner is actually different
 	suite.NotEqual(suite.runner, otherRunnerWithSameId)
 
-	_ = suite.runnerPool.Add(suite.runner)
-	_ = suite.runnerPool.Add(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.Add(suite.runner)
 	suite.runnerPool.Delete(suite.runner.Id())
 	retrievedRunner, ok := suite.runnerPool.Get(suite.runner.Id())
 	suite.Nil(retrievedRunner)
diff --git a/store/entity_store.go b/store/entity_store.go
index 207c0a5..7e63aaa 100644
--- a/store/entity_store.go
+++ b/store/entity_store.go
@@ -5,7 +5,7 @@ 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)
+	Add(entity Entity)
 
 	// Get returns a entity from the store.
 	// If the entity does not exist in the store, ok will be false.