Verified Commit 26611aef authored by Tomasz Maczukin's avatar Tomasz Maczukin 💬
Browse files

Fix Cleanup() handling for volumes manager

parent cbf1fb6b
......@@ -3,6 +3,7 @@ package volumes
import (
"context"
"fmt"
"sync"
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/container"
......@@ -22,7 +23,7 @@ type containerClient interface {
type CacheContainersManager interface {
FindOrCleanExisting(containerName string, containerPath string) string
Create(containerName string, containerPath string) (string, error)
Remove(ctx context.Context, id string) error
Cleanup(ctx context.Context, ids []string) chan bool
}
type cacheContainerManager struct {
......@@ -138,6 +139,31 @@ func (m *cacheContainerManager) startCacheContainer(containerID string) error {
return nil
}
func (m *cacheContainerManager) Remove(ctx context.Context, id string) error {
return m.containerClient.RemoveContainer(ctx, id)
func (m *cacheContainerManager) Cleanup(ctx context.Context, ids []string) chan bool {
done := make(chan bool, 1)
ids = append(m.failedContainerIDs, ids...)
go func() {
wg := new(sync.WaitGroup)
wg.Add(len(ids))
for _, id := range ids {
m.remove(ctx, wg, id)
}
wg.Wait()
done <- true
}()
return done
}
func (m *cacheContainerManager) remove(ctx context.Context, wg *sync.WaitGroup, id string) {
go func() {
err := m.containerClient.RemoveContainer(ctx, id)
if err != nil {
m.logger.Debugln(fmt.Sprintf("Error while removing the container: %v", err))
}
wg.Done()
}()
}
......@@ -222,18 +222,35 @@ func TestCacheContainerManager_CreateCacheContainer(t *testing.T) {
}
}
func TestCacheContainerManager_RemoveCacheContainer(t *testing.T) {
cClient := new(mockContainerClient)
defer cClient.AssertExpectations(t)
func TestCacheContainerManager_Cleanup(t *testing.T) {
ctx := context.Background()
containerClientMock := new(mockContainerClient)
defer containerClientMock.AssertExpectations(t)
loggerMock := new(mockDebugLogger)
defer loggerMock.AssertExpectations(t)
cClient.On("RemoveContainer", mock.Anything, "container-id").
containerClientMock.On("RemoveContainer", ctx, "failed-container-1").
Return(nil).
Once()
containerClientMock.On("RemoveContainer", ctx, "container-1-with-remove-error").
Return(errors.New("test-error")).
Once()
containerClientMock.On("RemoveContainer", ctx, "container-1").
Return(nil).
Once()
loggerMock.On("Debugln", "Error while removing the container: test-error").
Once()
m := &cacheContainerManager{
containerClient: cClient,
containerClient: containerClientMock,
logger: loggerMock,
failedContainerIDs: []string{"failed-container-1", "container-1-with-remove-error"},
}
err := m.Remove(context.Background(), "container-id")
assert.NoError(t, err)
done := m.Cleanup(ctx, []string{"container-1"})
<-done
}
......@@ -7,7 +7,6 @@ import (
"path"
"path/filepath"
"strings"
"sync"
"gitlab.com/gitlab-org/gitlab-runner/common"
)
......@@ -188,25 +187,5 @@ func (m *manager) ContainerIDs() []string {
}
func (m *manager) Cleanup(ctx context.Context) chan bool {
done := make(chan bool, 1)
remove := func(wg *sync.WaitGroup, containerID string) {
wg.Add(1)
go func() {
_ = m.cacheContainersManager.Remove(ctx, containerID)
wg.Done()
}()
}
go func() {
wg := new(sync.WaitGroup)
for _, id := range m.tmpContainerIDs {
remove(wg, id)
}
wg.Wait()
done <- true
}()
return done
return m.cacheContainersManager.Cleanup(ctx, m.tmpContainerIDs)
}
......@@ -463,8 +463,13 @@ func TestDefaultManager_Cleanup(t *testing.T) {
ccManager := new(MockCacheContainersManager)
defer ccManager.AssertExpectations(t)
ccManager.On("Remove", mock.Anything, "container-1").
Return(nil).
doneCh := make(chan bool, 1)
ccManager.On("Cleanup", mock.Anything, []string{"container-1"}).
Run(func(_ mock.Arguments) {
close(doneCh)
}).
Return(doneCh).
Once()
m := &manager{
......
......@@ -10,6 +10,22 @@ type MockCacheContainersManager struct {
mock.Mock
}
// Cleanup provides a mock function with given fields: ctx, ids
func (_m *MockCacheContainersManager) Cleanup(ctx context.Context, ids []string) chan bool {
ret := _m.Called(ctx, ids)
var r0 chan bool
if rf, ok := ret.Get(0).(func(context.Context, []string) chan bool); ok {
r0 = rf(ctx, ids)
} else {
if ret.Get(0) != nil {
r0 = ret.Get(0).(chan bool)
}
}
return r0
}
// Create provides a mock function with given fields: containerName, containerPath
func (_m *MockCacheContainersManager) Create(containerName string, containerPath string) (string, error) {
ret := _m.Called(containerName, containerPath)
......@@ -44,17 +60,3 @@ func (_m *MockCacheContainersManager) FindOrCleanExisting(containerName string,
return r0
}
// Remove provides a mock function with given fields: ctx, id
func (_m *MockCacheContainersManager) Remove(ctx context.Context, id string) error {
ret := _m.Called(ctx, id)
var r0 error
if rf, ok := ret.Get(0).(func(context.Context, string) error); ok {
r0 = rf(ctx, id)
} else {
r0 = ret.Error(0)
}
return r0
}
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment