From 6573a8ab1dab8bc1cfffea186c6565e6548bca9d Mon Sep 17 00:00:00 2001 From: Maximilian Pass <22845248+mpass99@users.noreply.github.com> Date: Fri, 27 Sep 2024 11:34:46 +0200 Subject: [PATCH 1/2] Fix provideRunner with no managers enabled. If only AWS was enabled, a 500 error instead of a 404 error was returned. If both Nomad and AWS was disabled, the requests returned with a 200 response but an empty body. A silent nil-dereference was triggered. --- cmd/poseidon/main.go | 12 ++++++++++-- internal/environment/abstract_manager.go | 8 ++++++++ internal/runner/abstract_manager.go | 4 ++-- internal/runner/nomad_manager.go | 14 +++++++++----- internal/runner/nomad_manager_test.go | 2 +- 5 files changed, 30 insertions(+), 10 deletions(-) diff --git a/cmd/poseidon/main.go b/cmd/poseidon/main.go index 28eed601..ffc33e6e 100644 --- a/cmd/poseidon/main.go +++ b/cmd/poseidon/main.go @@ -341,6 +341,13 @@ func createManagerHandler(ctx context.Context, handler managerCreator, enabled b return runnerManager, environmentManager } +func createAbstractManager(ctx context.Context) ( + runnerManager runner.Manager, environmentManager environment.ManagerHandler, +) { + runnerManager = runner.NewAbstractManager(ctx) + return runnerManager, environment.NewAbstractManager(runnerManager) +} + func createNomadManager(ctx context.Context) (runner.Manager, environment.ManagerHandler) { // API initialization nomadAPIClient, err := nomad.NewExecutorAPI(ctx, &config.Config.Nomad) @@ -400,8 +407,9 @@ func createAWSManager(ctx context.Context) ( // initRouter builds a router that serves the API with the chain of responsibility for multiple managers. func initRouter(ctx context.Context) *mux.Router { - runnerManager, environmentManager := createManagerHandler(ctx, createNomadManager, config.Config.Nomad.Enabled, - nil, nil) + runnerManager, environmentManager := createManagerHandler(ctx, createAbstractManager, true, nil, nil) + runnerManager, environmentManager = createManagerHandler(ctx, createNomadManager, config.Config.Nomad.Enabled, + runnerManager, environmentManager) runnerManager, environmentManager = createManagerHandler(ctx, createAWSManager, config.Config.AWS.Enabled, runnerManager, environmentManager) diff --git a/internal/environment/abstract_manager.go b/internal/environment/abstract_manager.go index 558de015..ee2687f1 100644 --- a/internal/environment/abstract_manager.go +++ b/internal/environment/abstract_manager.go @@ -15,6 +15,14 @@ type AbstractManager struct { runnerManager runner.Manager } +// NewAbstractManager creates a new abstract runner manager that keeps track of all environments of one kind. +func NewAbstractManager(runnerManager runner.Manager) *AbstractManager { + return &AbstractManager{ + nextHandler: nil, + runnerManager: runnerManager, + } +} + func (n *AbstractManager) SetNextHandler(next ManagerHandler) { n.nextHandler = next } diff --git a/internal/runner/abstract_manager.go b/internal/runner/abstract_manager.go index fa475e98..214bf725 100644 --- a/internal/runner/abstract_manager.go +++ b/internal/runner/abstract_manager.go @@ -12,7 +12,7 @@ import ( "github.com/openHPI/poseidon/pkg/storage" ) -var ErrNullObject = errors.New("functionality not available for the null object") +var ErrUnknownExecutionEnvironment = errors.New("execution environment not found") // AbstractManager is used to have a fallback runner manager in the chain of responsibility // following the null object pattern. @@ -99,7 +99,7 @@ func (n *AbstractManager) EnvironmentStatistics() map[dto.EnvironmentID]*dto.Sta } func (n *AbstractManager) Claim(_ dto.EnvironmentID, _ int) (Runner, error) { - return nil, ErrNullObject + return nil, ErrUnknownExecutionEnvironment } func (n *AbstractManager) Get(runnerID string) (Runner, error) { diff --git a/internal/runner/nomad_manager.go b/internal/runner/nomad_manager.go index ea1df3bc..88d485ef 100644 --- a/internal/runner/nomad_manager.go +++ b/internal/runner/nomad_manager.go @@ -24,10 +24,9 @@ import ( const environmentMigrationDelay = time.Minute var ( - log = logging.GetLogger("runner") - ErrUnknownExecutionEnvironment = errors.New("execution environment not found") - ErrNoRunnersAvailable = errors.New("no runners available for this execution environment") - ErrRunnerNotFound = errors.New("no runner found with this id") + log = logging.GetLogger("runner") + ErrNoRunnersAvailable = errors.New("no runners available for this execution environment") + ErrRunnerNotFound = errors.New("no runner found with this id") ) type alertData struct { @@ -50,8 +49,13 @@ func NewNomadRunnerManager(ctx context.Context, apiClient nomad.ExecutorAPI) *No func (m *NomadRunnerManager) Claim(environmentID dto.EnvironmentID, duration int) (Runner, error) { environment, ok := m.GetEnvironment(environmentID) if !ok { - return nil, ErrUnknownExecutionEnvironment + r, err := m.NextHandler().Claim(environmentID, duration) + if err != nil { + return nil, fmt.Errorf("nomad wrapped: %w", err) + } + return r, nil } + runner, ok := environment.Sample() if !ok { return nil, ErrNoRunnersAvailable diff --git a/internal/runner/nomad_manager_test.go b/internal/runner/nomad_manager_test.go index 1caac853..d3ceb2eb 100644 --- a/internal/runner/nomad_manager_test.go +++ b/internal/runner/nomad_manager_test.go @@ -117,7 +117,7 @@ func (s *ManagerTestSuite) TestSetEnvironmentAddsNewEnvironment() { func (s *ManagerTestSuite) TestClaimReturnsNotFoundErrorIfEnvironmentNotFound() { runner, err := s.nomadRunnerManager.Claim(anotherEnvironmentID, defaultInactivityTimeout) s.Nil(runner) - s.Equal(ErrUnknownExecutionEnvironment, err) + s.ErrorIs(err, ErrUnknownExecutionEnvironment) } func (s *ManagerTestSuite) TestClaimReturnsRunnerIfAvailable() { From 11b18aa7408c4cfaa6d63a11de0ae84ea6c24bcb Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Sat, 28 Sep 2024 11:41:15 +0200 Subject: [PATCH 2/2] Fix createOrUpdate environment with no manager enabled Previously, the abstract manager would still return a successful response of 204, even if the environment was not stored at all. Now, we return a "proper" 500 code if no environment manager is able to handle the request. --- internal/api/environments.go | 1 + internal/environment/abstract_manager.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/api/environments.go b/internal/api/environments.go index ce834384..dee41a9b 100644 --- a/internal/api/environments.go +++ b/internal/api/environments.go @@ -128,6 +128,7 @@ func (e *EnvironmentController) createOrUpdate(writer http.ResponseWriter, reque }) if err != nil { writeInternalServerError(request.Context(), writer, err, dto.ErrorUnknown) + return } if created { diff --git a/internal/environment/abstract_manager.go b/internal/environment/abstract_manager.go index ee2687f1..4d58dcfa 100644 --- a/internal/environment/abstract_manager.go +++ b/internal/environment/abstract_manager.go @@ -49,7 +49,7 @@ func (n *AbstractManager) Get(_ context.Context, _ dto.EnvironmentID, _ bool) (r func (n *AbstractManager) CreateOrUpdate(_ context.Context, _ dto.EnvironmentID, _ dto.ExecutionEnvironmentRequest) ( bool, error, ) { - return false, nil + return false, dto.ErrNotSupported } func (n *AbstractManager) Delete(environmentID dto.EnvironmentID) (bool, error) {