From 72f6d25b4815dce61883746ee39bbe85f8ada006 Mon Sep 17 00:00:00 2001 From: Anthony ESTEBE Date: Tue, 27 Nov 2018 13:05:01 +0700 Subject: [PATCH 1/3] fix issue when many containers are created but only one is deleted --- container/service.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/container/service.go b/container/service.go index 914c873e2..25750d483 100644 --- a/container/service.go +++ b/container/service.go @@ -51,14 +51,24 @@ func (c *DockerContainer) StartService(options ServiceOptions) (serviceID string // StopService stops a docker service. func (c *DockerContainer) StopService(namespace []string) (err error) { + ctx, cancel := context.WithTimeout(context.Background(), c.callTimeout) + defer cancel() + if err := c.client.ServiceRemove(ctx, c.Namespace(namespace)); err != nil && !docker.IsErrNotFound(err) { + return err + } + c.deletePendingContainer(namespace) + return c.waitForStatus(namespace, STOPPED) +} + +func (c *DockerContainer) deletePendingContainer(namespace []string) error { ctx, cancel := context.WithTimeout(context.Background(), c.callTimeout) defer cancel() container, err := c.FindContainer(namespace) if err != nil && !docker.IsErrNotFound(err) { return err } - if err := c.client.ServiceRemove(ctx, c.Namespace(namespace)); err != nil && !docker.IsErrNotFound(err) { - return err + if docker.IsErrNotFound(err) { + return nil } // TOFIX: Hack to force Docker to remove the containers. // Sometime, the ServiceRemove function doesn't remove the associated containers. @@ -69,7 +79,7 @@ func (c *DockerContainer) StopService(namespace []string) (err error) { c.client.ContainerStop(ctx, container.ID, &timeout) c.client.ContainerRemove(ctx, container.ID, types.ContainerRemoveOptions{}) } - return c.waitForStatus(namespace, STOPPED) + return c.deletePendingContainer(namespace) } // ServiceLogs returns the logs of a service. From e6ab9e2279121c500b09e3fe3b350f526b9c6ec5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=CC=87lker=20Go=CC=88ktug=CC=86=20O=CC=88ztu=CC=88rk?= Date: Thu, 29 Nov 2018 09:23:55 +0300 Subject: [PATCH 2/3] container: fix missing error check in StopService() --- container/service.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/container/service.go b/container/service.go index 25750d483..9f50610b4 100644 --- a/container/service.go +++ b/container/service.go @@ -56,7 +56,9 @@ func (c *DockerContainer) StopService(namespace []string) (err error) { if err := c.client.ServiceRemove(ctx, c.Namespace(namespace)); err != nil && !docker.IsErrNotFound(err) { return err } - c.deletePendingContainer(namespace) + if err := c.deletePendingContainer(namespace); err != nil { + return err + } return c.waitForStatus(namespace, STOPPED) } @@ -64,12 +66,12 @@ func (c *DockerContainer) deletePendingContainer(namespace []string) error { ctx, cancel := context.WithTimeout(context.Background(), c.callTimeout) defer cancel() container, err := c.FindContainer(namespace) - if err != nil && !docker.IsErrNotFound(err) { + if err != nil { + if docker.IsErrNotFound(err) { + return nil + } return err } - if docker.IsErrNotFound(err) { - return nil - } // TOFIX: Hack to force Docker to remove the containers. // Sometime, the ServiceRemove function doesn't remove the associated containers. // This hack for Docker to stop and then remove the container. From 8079cf499e0e417cf702c83c76d92e222a46dbb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=CC=87lker=20Go=CC=88ktug=CC=86=20O=CC=88ztu=CC=88rk?= Date: Thu, 29 Nov 2018 12:17:35 +0300 Subject: [PATCH 3/3] container: prepare a base for testing with mockery, convert TestStopService test to use mockery --- container/container_test.go | 19 +++++++++++++ container/service_test.go | 56 ++++++++++++++++++++++--------------- container/wait_test.go | 36 ++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 22 deletions(-) diff --git a/container/container_test.go b/container/container_test.go index 13db8f2f2..6daeead48 100644 --- a/container/container_test.go +++ b/container/container_test.go @@ -10,9 +10,28 @@ import ( "github.com/mesg-foundation/core/config" "github.com/mesg-foundation/core/container/dockertest" + "github.com/mesg-foundation/core/utils/docker/mocks" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) +func newTesting(t *testing.T) (*DockerContainer, *mocks.CommonAPIClient) { + m := &mocks.CommonAPIClient{} + mockNew(m) + + c, err := New(ClientOption(m)) + require.NoError(t, err) + + return c, m +} + +func mockNew(m *mocks.CommonAPIClient) { + m.On("NegotiateAPIVersion", mock.Anything).Once().Return() + m.On("Info", mock.Anything).Once().Return(types.Info{Swarm: swarm.Info{NodeID: "1"}}, nil) + m.On("NetworkInspect", mock.Anything, "core", types.NetworkInspectOptions{}).Once(). + Return(types.NetworkResource{ID: "1"}, nil) +} + func TestNew(t *testing.T) { dt := dockertest.New() c, err := New(ClientOption(dt.Client())) diff --git a/container/service_test.go b/container/service_test.go index e098f895a..7387a0907 100644 --- a/container/service_test.go +++ b/container/service_test.go @@ -9,6 +9,7 @@ import ( "github.com/docker/docker/api/types/filters" "github.com/docker/docker/api/types/swarm" "github.com/mesg-foundation/core/container/dockertest" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -35,30 +36,41 @@ func TestStartService(t *testing.T) { } func TestStopService(t *testing.T) { - namespace := []string{"namespace"} - dt := dockertest.New() - c, _ := New(ClientOption(dt.Client())) - containerID := "1" - - dt.ProvideContainerList([]types.Container{ - {ID: containerID}, - }, nil) - dt.ProvideContainerInspect(types.ContainerJSON{ - ContainerJSONBase: &types.ContainerJSONBase{ID: containerID}, - }, nil) - - dt.ProvideContainerList(nil, dockertest.NotFoundErr{}) - dt.ProvideServiceInspectWithRaw(swarm.Service{}, nil, dockertest.NotFoundErr{}) - - require.Nil(t, c.StopService(namespace)) - require.Equal(t, c.Namespace(namespace), (<-dt.LastServiceRemove()).ServiceID) + var ( + c, m = newTesting(t) + containerID = "1" + namespace = []string{"2"} + fullNamespace = c.Namespace(namespace) + ) + + m.On("ServiceRemove", mock.Anything, fullNamespace).Once().Return(nil) + m.On("ContainerList", mock.AnythingOfType("*context.timerCtx"), types.ContainerListOptions{ + Filters: filters.NewArgs(filters.KeyValuePair{ + Key: "label", + Value: "com.docker.stack.namespace=" + fullNamespace, + }), + Limit: 1, + }).Once(). + Return([]types.Container{{ID: containerID}}, nil) + m.On("ContainerInspect", mock.AnythingOfType("*context.timerCtx"), containerID).Once(). + Return(types.ContainerJSON{ + ContainerJSONBase: &types.ContainerJSONBase{ID: containerID}, + }, nil) + m.On("ContainerStop", mock.Anything, containerID, mock.AnythingOfType("*time.Duration")).Once().Return(nil) + m.On("ContainerRemove", mock.Anything, containerID, types.ContainerRemoveOptions{}).Once().Return(nil) + m.On("ContainerList", mock.AnythingOfType("*context.timerCtx"), types.ContainerListOptions{ + Filters: filters.NewArgs(filters.KeyValuePair{ + Key: "label", + Value: "com.docker.stack.namespace=" + fullNamespace, + }), + Limit: 1, + }).Once(). + Return(nil, dockertest.NotFoundErr{}) + mockWaitForStatus(t, m, fullNamespace, STOPPED) - ls := <-dt.LastContainerStop() - require.Equal(t, containerID, ls.Container) + require.NoError(t, c.StopService(namespace)) - lr := <-dt.LastContainerRemove() - require.Equal(t, containerID, lr.Container) - require.Equal(t, types.ContainerRemoveOptions{}, lr.Options) + m.AssertExpectations(t) } func TestStopNotExistingService(t *testing.T) { diff --git a/container/wait_test.go b/container/wait_test.go index 50396c8b9..dbcede2b1 100644 --- a/container/wait_test.go +++ b/container/wait_test.go @@ -4,11 +4,47 @@ import ( "testing" "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/filters" "github.com/docker/docker/api/types/swarm" "github.com/mesg-foundation/core/container/dockertest" + "github.com/mesg-foundation/core/utils/docker/mocks" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) +// TODO: support all status types. +func mockWaitForStatus(t *testing.T, m *mocks.CommonAPIClient, namespace string, wantedStatus StatusType) { + var ( + containerID = "1" + ) + + m.On("TaskList", mock.Anything, types.TaskListOptions{ + Filters: filters.NewArgs(filters.KeyValuePair{ + Key: "label", + Value: "com.docker.stack.namespace=" + namespace, + }), + }).Once(). + Return([]swarm.Task{}, nil) + m.On("ContainerList", mock.AnythingOfType("*context.timerCtx"), types.ContainerListOptions{ + Filters: filters.NewArgs(filters.KeyValuePair{ + Key: "label", + Value: "com.docker.stack.namespace=" + namespace, + }), + Limit: 1, + }).Once(). + Return([]types.Container{{ID: containerID}}, nil) + + containerInspect := m.On("ContainerInspect", mock.AnythingOfType("*context.timerCtx"), containerID).Once() + serviceInspect := m.On("ServiceInspectWithRaw", mock.Anything, namespace, types.ServiceInspectOptions{}).Once() + switch wantedStatus { + case STOPPED: + containerInspect.Return(types.ContainerJSON{}, dockertest.NotFoundErr{}) + serviceInspect.Return(swarm.Service{}, nil, dockertest.NotFoundErr{}) + default: + t.Errorf("unhandled status %v", wantedStatus) + } +} + func TestWaitForStatusRunning(t *testing.T) { namespace := []string{"namespace"} containerID := "1"