From 59da36303ceed465101071b2ca0f6de2e2c5fe93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Pa=C3=9F?= <22845248+mpass99@users.noreply.github.com> Date: Mon, 11 Sep 2023 10:22:19 +0200 Subject: [PATCH] Fix Goroutine Leak of Environment Get that was caused by creating an intermediate environment `fetchedEnvironment` when fetching the environments but not removing it in case that we just copy its configuration to the existing environment. --- internal/environment/abstract_manager.go | 2 +- internal/environment/aws_environment.go | 2 +- internal/environment/nomad_environment.go | 18 +++++++------- .../environment/nomad_environment_test.go | 14 +++++++++-- internal/environment/nomad_manager.go | 6 ++++- internal/environment/nomad_manager_test.go | 24 +++++++++---------- internal/runner/execution_environment.go | 3 ++- internal/runner/execution_environment_mock.go | 12 +++++----- 8 files changed, 48 insertions(+), 33 deletions(-) diff --git a/internal/environment/abstract_manager.go b/internal/environment/abstract_manager.go index 0400bee..28b7705 100644 --- a/internal/environment/abstract_manager.go +++ b/internal/environment/abstract_manager.go @@ -58,7 +58,7 @@ func (n *AbstractManager) Delete(id dto.EnvironmentID) (bool, error) { } n.runnerManager.DeleteEnvironment(id) - if err := e.Delete(); err != nil { + if err := e.Delete(false); err != nil { return true, fmt.Errorf("could not delete environment: %w", err) } return true, nil diff --git a/internal/environment/aws_environment.go b/internal/environment/aws_environment.go index 4ba1124..7d864c9 100644 --- a/internal/environment/aws_environment.go +++ b/internal/environment/aws_environment.go @@ -45,7 +45,7 @@ func (a *AWSEnvironment) SetImage(awsEndpoint string) { a.awsEndpoint = awsEndpoint } -func (a *AWSEnvironment) Delete() error { +func (a *AWSEnvironment) Delete(_ bool) error { return nil } diff --git a/internal/environment/nomad_environment.go b/internal/environment/nomad_environment.go index cf7754b..777fbe6 100644 --- a/internal/environment/nomad_environment.go +++ b/internal/environment/nomad_environment.go @@ -219,15 +219,17 @@ func (n *NomadEnvironment) Register() error { return nil } -func (n *NomadEnvironment) Delete() error { +func (n *NomadEnvironment) Delete(local bool) error { n.cancel() - err := n.removeRunners() - if err != nil { - return err - } - err = n.apiClient.DeleteJob(*n.job.ID) - if err != nil { - return fmt.Errorf("couldn't delete environment job: %w", err) + if !local { + err := n.removeRunners() + if err != nil { + return err + } + err = n.apiClient.DeleteJob(*n.job.ID) + if err != nil { + return fmt.Errorf("couldn't delete environment job: %w", err) + } } return nil } diff --git a/internal/environment/nomad_environment_test.go b/internal/environment/nomad_environment_test.go index 6da16f4..883b248 100644 --- a/internal/environment/nomad_environment_test.go +++ b/internal/environment/nomad_environment_test.go @@ -165,7 +165,7 @@ func (s *MainTestSuite) TestParseJob() { environment, err := NewNomadEnvironment(tests.DefaultEnvironmentIDAsInteger, apiMock, templateEnvironmentJobHCL) s.NoError(err) s.NotNil(environment.job) - s.NoError(environment.Delete()) + s.NoError(environment.Delete(false)) }) s.Run("returns error when given wrong job", func() { @@ -216,7 +216,7 @@ func (s *MainTestSuite) TestSampleDoesNotSetForcePullFlag() { _, job := helpers.CreateTemplateJob() environment := &NomadEnvironment{apiMock, templateEnvironmentJobHCL, job, - storage.NewLocalStorage[runner.Runner](), context.Background(), nil} + storage.NewLocalStorage[runner.Runner](), s.TestCtx, nil} runner1 := &runner.RunnerMock{} runner1.On("ID").Return(tests.DefaultRunnerID) environment.AddRunner(runner1) @@ -225,3 +225,13 @@ func (s *MainTestSuite) TestSampleDoesNotSetForcePullFlag() { s.Require().True(ok) <-time.After(tests.ShortTimeout) // New Runners are requested asynchronously } + +func (s *MainTestSuite) TestNomadEnvironment_DeleteLocally() { + apiMock := &nomad.ExecutorAPIMock{} + environment, err := NewNomadEnvironment(tests.DefaultEnvironmentIDAsInteger, apiMock, templateEnvironmentJobHCL) + s.Require().NoError(err) + + err = environment.Delete(true) + s.NoError(err) + apiMock.AssertExpectations(s.T()) +} diff --git a/internal/environment/nomad_manager.go b/internal/environment/nomad_manager.go index f344387..7fa9cfe 100644 --- a/internal/environment/nomad_manager.go +++ b/internal/environment/nomad_manager.go @@ -72,6 +72,10 @@ func (m *NomadEnvironmentManager) Get(id dto.EnvironmentID, fetch bool) ( ok = true default: executionEnvironment.SetConfigFrom(fetchedEnvironment) + err = fetchedEnvironment.Delete(true) + if err != nil { + log.WithError(err).Warn("Failed to remove environment locally") + } } } @@ -98,7 +102,7 @@ func (m *NomadEnvironmentManager) CreateOrUpdate( if isExistingEnvironment { // Remove existing environment to force downloading the newest Docker image. // See https://github.com/openHPI/poseidon/issues/69 - err = environment.Delete() + err = environment.Delete(false) if err != nil { return false, fmt.Errorf("failed to remove the environment: %w", err) } diff --git a/internal/environment/nomad_manager_test.go b/internal/environment/nomad_manager_test.go index 09af223..daaf798 100644 --- a/internal/environment/nomad_manager_test.go +++ b/internal/environment/nomad_manager_test.go @@ -127,7 +127,7 @@ func (s *MainTestSuite) TestNewNomadEnvironmentManager() { s.NotNil(m) s.Equal(templateJobHCL, m.templateEnvironmentHCL) - s.NoError(environment.Delete()) + s.NoError(environment.Delete(false)) }) s.Run("returns error if template file is invalid", func() { @@ -145,8 +145,6 @@ func (s *MainTestSuite) TestNewNomadEnvironmentManager() { } func (s *MainTestSuite) TestNomadEnvironmentManager_Get() { - s.T().Skip("ToDo: Get does not delete the replaced environment") // ToDo - disableRecovery, cancel := context.WithCancel(context.Background()) cancel() @@ -179,7 +177,7 @@ func (s *MainTestSuite) TestNomadEnvironmentManager_Get() { s.NoError(err) s.Equal(expectedEnvironment, environment) - err = environment.Delete() + err = environment.Delete(false) s.Require().NoError(err) }) @@ -213,11 +211,11 @@ func (s *MainTestSuite) TestNomadEnvironmentManager_Get() { s.NoError(err) s.Equal(fetchedEnvironment.Image(), environment.Image()) - err = fetchedEnvironment.Delete() + err = fetchedEnvironment.Delete(false) s.Require().NoError(err) - err = environment.Delete() + err = environment.Delete(false) s.Require().NoError(err) - err = localEnvironment.Delete() + err = localEnvironment.Delete(false) s.Require().NoError(err) }) runnerManager.DeleteEnvironment(tests.DefaultEnvironmentIDAsInteger) @@ -239,9 +237,9 @@ func (s *MainTestSuite) TestNomadEnvironmentManager_Get() { s.NoError(err) s.Equal(fetchedEnvironment.Image(), environment.Image()) - err = fetchedEnvironment.Delete() + err = fetchedEnvironment.Delete(false) s.Require().NoError(err) - err = environment.Delete() + err = environment.Delete(false) s.Require().NoError(err) }) }) @@ -281,7 +279,7 @@ func (s *MainTestSuite) TestNomadEnvironmentManager_List() { s.Equal(1, len(environments)) s.Equal(localEnvironment, environments[0]) - err = localEnvironment.Delete() + err = localEnvironment.Delete(false) s.Require().NoError(err) }) runnerManager.DeleteEnvironment(tests.DefaultEnvironmentIDAsInteger) @@ -308,9 +306,9 @@ func (s *MainTestSuite) TestNomadEnvironmentManager_List() { s.True(ok) s.Equal(fetchedEnvironment.job, nomadEnvironment.job) - err = fetchedEnvironment.Delete() + err = fetchedEnvironment.Delete(false) s.Require().NoError(err) - err = nomadEnvironment.Delete() + err = nomadEnvironment.Delete(false) s.Require().NoError(err) }) } @@ -340,7 +338,7 @@ func (s *MainTestSuite) TestNomadEnvironmentManager_Load() { s.Require().True(ok) s.Equal("python:latest", environment.Image()) - err = environment.Delete() + err = environment.Delete(false) s.Require().NoError(err) }) diff --git a/internal/runner/execution_environment.go b/internal/runner/execution_environment.go index fc236fe..9d77a40 100644 --- a/internal/runner/execution_environment.go +++ b/internal/runner/execution_environment.go @@ -38,7 +38,8 @@ type ExecutionEnvironment interface { // Register saves this environment at the executor. Register() error // Delete removes this environment and all it's runner from the executor and Poseidon itself. - Delete() error + // Iff local the environment is just removed from Poseidon without external escalation. + Delete(local bool) error // Sample returns and removes an arbitrary available runner. // ok is true iff a runner was returned. diff --git a/internal/runner/execution_environment_mock.go b/internal/runner/execution_environment_mock.go index bee1cb4..9562f91 100644 --- a/internal/runner/execution_environment_mock.go +++ b/internal/runner/execution_environment_mock.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.33.1. DO NOT EDIT. +// Code generated by mockery v2.33.2. DO NOT EDIT. package runner @@ -45,13 +45,13 @@ func (_m *ExecutionEnvironmentMock) CPULimit() uint { return r0 } -// Delete provides a mock function with given fields: -func (_m *ExecutionEnvironmentMock) Delete() error { - ret := _m.Called() +// Delete provides a mock function with given fields: local +func (_m *ExecutionEnvironmentMock) Delete(local bool) error { + ret := _m.Called(local) var r0 error - if rf, ok := ret.Get(0).(func() error); ok { - r0 = rf() + if rf, ok := ret.Get(0).(func(bool) error); ok { + r0 = rf(local) } else { r0 = ret.Error(0) }