htwkmooc/poseidon
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix Goroutine Leak of Environment Get

Browse Source 
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.
This commit is contained in:
Maximilian Paß
2023-09-11 10:22:19 +02:00
parent 460b8b2065
commit 59da36303c
8 changed files with 48 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
internal/environment/abstract_manager.go
View File

@ -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

2
internal/environment/aws_environment.go
View File

@ -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
}

4
internal/environment/nomad_environment.go
View File

@ -219,8 +219,9 @@ func (n *NomadEnvironment) Register() error {
return nil
}
func (n *NomadEnvironment) Delete() error {
func (n *NomadEnvironment) Delete(local bool) error {
n.cancel()
if !local {
err := n.removeRunners()
if err != nil {
return err
@ -229,6 +230,7 @@ func (n *NomadEnvironment) Delete() error {
if err != nil {
return fmt.Errorf("couldn't delete environment job: %w", err)
}
}
return nil
}

14
internal/environment/nomad_environment_test.go
View File

@ -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())
}

6
internal/environment/nomad_manager.go
View File

@ -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)
}

24
internal/environment/nomad_manager_test.go
View File

@ -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)
})

3
internal/runner/execution_environment.go
View File

@ -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.

12
internal/runner/execution_environment_mock.go
View File

@ -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)
}