From 7b8b8e8ad95411011d15701d560300437ae72b73 Mon Sep 17 00:00:00 2001 From: Simon Aronsson Date: Sat, 11 Jan 2020 00:28:27 +0100 Subject: [PATCH] #387 fix: switch to image id map and add additional tests --- go.mod | 6 +- go.sum | 2 + internal/actions/actions_suite_test.go | 124 +++++++------------------ internal/actions/check.go | 2 +- internal/actions/mocks/client.go | 72 ++++++++++++++ internal/actions/mocks/container.go | 28 ++++++ internal/actions/update.go | 9 +- internal/actions/update_test.go | 84 +++++++++++++++++ pkg/container/client.go | 19 ++-- 9 files changed, 244 insertions(+), 102 deletions(-) create mode 100644 internal/actions/mocks/client.go create mode 100644 internal/actions/mocks/container.go create mode 100644 internal/actions/update_test.go diff --git a/go.mod b/go.mod index a45b228..9bdd550 100644 --- a/go.mod +++ b/go.mod @@ -53,11 +53,11 @@ require ( github.com/theupdateframework/notary v0.6.1 // indirect github.com/zmap/zlint v1.0.2 // indirect golang.org/x/net v0.0.0-20190522155817-f3200d17e092 + golang.org/x/sys v0.0.0-20190412213103-97732733099d + google.golang.org/genproto v0.0.0-20190404172233-64821d5d2107 + google.golang.org/grpc v1.21.0 gopkg.in/dancannon/gorethink.v3 v3.0.5 // indirect gopkg.in/fatih/pool.v2 v2.0.0 // indirect gopkg.in/gorethink/gorethink.v3 v3.0.5 // indirect - golang.org/x/sys v0.0.0-20190403152447-81d4e9dc473e - google.golang.org/genproto v0.0.0-20190401181712-f467c93bbac2 - google.golang.org/grpc v1.21.0 gotest.tools v2.2.0+incompatible // indirect ) diff --git a/go.sum b/go.sum index 160a1c2..f96232a 100644 --- a/go.sum +++ b/go.sum @@ -298,6 +298,7 @@ golang.org/x/sys v0.0.0-20181107165924-66b7b1311ac8/go.mod h1:STP8DvDyc/dI5b8T5h golang.org/x/sys v0.0.0-20181116152217-5ac8a444bdc5/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20181122145206-62eef0e2fa9b/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.0.0-20190403152447-81d4e9dc473e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20190412213103-97732733099d h1:+R4KGOnez64A81RvjARKc4UT5/tI9ujCIVX+P5KiHuI= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg= @@ -319,6 +320,7 @@ google.golang.org/appengine v1.4.0 h1:/wp5JvzpHIxhs/dumFmF7BXTf3Z+dd4uXta4kVyO50 google.golang.org/appengine v1.4.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7/EB5XEv4= google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc= google.golang.org/genproto v0.0.0-20190307195333-5fe7a883aa19/go.mod h1:VzzqZJRnGkLBvHegQrXjBqPurQTc5/KpmUdxsrq26oE= +google.golang.org/genproto v0.0.0-20190401181712-f467c93bbac2/go.mod h1:VzzqZJRnGkLBvHegQrXjBqPurQTc5/KpmUdxsrq26oE= google.golang.org/genproto v0.0.0-20190404172233-64821d5d2107 h1:xtNn7qFlagY2mQNFHMSRPjT2RkOV4OXM7P5TVy9xATo= google.golang.org/genproto v0.0.0-20190404172233-64821d5d2107/go.mod h1:VzzqZJRnGkLBvHegQrXjBqPurQTc5/KpmUdxsrq26oE= google.golang.org/grpc v1.17.0/go.mod h1:6QZJwpn2B+Zp71q/5VxRsJ6NXXVCE5NRUHRo+f3cWCs= diff --git a/internal/actions/actions_suite_test.go b/internal/actions/actions_suite_test.go index 76d2be5..2c9b0c8 100644 --- a/internal/actions/actions_suite_test.go +++ b/internal/actions/actions_suite_test.go @@ -1,18 +1,16 @@ package actions_test import ( - "errors" "github.com/containrrr/watchtower/internal/actions" "testing" "time" "github.com/containrrr/watchtower/pkg/container" "github.com/containrrr/watchtower/pkg/container/mocks" - "github.com/docker/docker/api/types" - t "github.com/containrrr/watchtower/pkg/types" cli "github.com/docker/docker/client" + . "github.com/containrrr/watchtower/internal/actions/mocks" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -24,7 +22,7 @@ func TestActions(t *testing.T) { var _ = Describe("the actions package", func() { var dockerClient cli.CommonAPIClient - var client mockClient + var client MockClient BeforeSuite(func() { server := mocks.NewMockAPIServer() dockerClient, _ = cli.NewClientWithOpts( @@ -32,12 +30,15 @@ var _ = Describe("the actions package", func() { cli.WithHTTPClient(server.Client())) }) BeforeEach(func() { - client = mockClient{ - api: dockerClient, - pullImages: false, - removeVolumes: false, - TestData: &TestData{}, - } + pullImages := false + removeVolumes := false + + client = CreateMockClient( + &TestData {}, + dockerClient, + pullImages, + removeVolumes, + ) }) Describe("the check prerequisites method", func() { @@ -51,7 +52,7 @@ var _ = Describe("the actions package", func() { When("given an array of one", func() { It("should not do anything", func() { client.TestData.Containers = []container.Container{ - createMockContainer( + CreateMockContainer( "test-container", "test-container", "watchtower", @@ -63,27 +64,30 @@ var _ = Describe("the actions package", func() { }) When("given multiple containers", func() { BeforeEach(func() { - client = mockClient{ - api: dockerClient, - pullImages: false, - removeVolumes: false, - TestData: &TestData{ + pullImages := false + removeVolumes := false + client = CreateMockClient( + &TestData{ NameOfContainerToKeep: "test-container-02", Containers: []container.Container{ - createMockContainer( + CreateMockContainer( "test-container-01", "test-container-01", "watchtower", time.Now().AddDate(0, 0, -1)), - createMockContainer( + CreateMockContainer( "test-container-02", "test-container-02", "watchtower", time.Now()), }, }, - } + dockerClient, + pullImages, + removeVolumes, + ) }) + It("should stop all but the latest one", func() { err := actions.CheckForMultipleWatchtowerInstances(client, false) Expect(err).NotTo(HaveOccurred()) @@ -91,96 +95,40 @@ var _ = Describe("the actions package", func() { }) When("deciding whether to cleanup images", func() { BeforeEach(func() { - client = mockClient{ - api: dockerClient, - pullImages: false, - removeVolumes: false, - TestData: &TestData{ + pullImages := false + removeVolumes := false + + client = CreateMockClient( + &TestData{ Containers: []container.Container{ - createMockContainer( + CreateMockContainer( "test-container-01", "test-container-01", "watchtower", time.Now().AddDate(0, 0, -1)), - createMockContainer( + CreateMockContainer( "test-container-02", "test-container-02", "watchtower", time.Now()), }, }, - } + dockerClient, + pullImages, + removeVolumes, + ) }) It("should try to delete the image if the cleanup flag is true", func() { err := actions.CheckForMultipleWatchtowerInstances(client, true) Expect(err).NotTo(HaveOccurred()) - Expect(client.TestData.TriedToRemoveImage).To(BeTrue()) + Expect(client.TestData.TriedToRemoveImage()).To(BeTrue()) }) It("should not try to delete the image if the cleanup flag is false", func() { err := actions.CheckForMultipleWatchtowerInstances(client, false) Expect(err).NotTo(HaveOccurred()) - Expect(client.TestData.TriedToRemoveImage).To(BeFalse()) + Expect(client.TestData.TriedToRemoveImage()).To(BeFalse()) }) }) }) }) -func createMockContainer(id string, name string, image string, created time.Time) container.Container { - content := types.ContainerJSON{ - ContainerJSONBase: &types.ContainerJSONBase{ - ID: id, - Image: image, - Name: name, - Created: created.String(), - }, - } - return *container.NewContainer(&content, nil) -} - -type mockClient struct { - TestData *TestData - api cli.CommonAPIClient - pullImages bool - removeVolumes bool -} - -type TestData struct { - TriedToRemoveImage bool - NameOfContainerToKeep string - Containers []container.Container -} - -func (client mockClient) ListContainers(f t.Filter) ([]container.Container, error) { - return client.TestData.Containers, nil -} - -func (client mockClient) StopContainer(c container.Container, d time.Duration) error { - if c.Name() == client.TestData.NameOfContainerToKeep { - return errors.New("tried to stop the instance we want to keep") - } - return nil -} -func (client mockClient) StartContainer(c container.Container) (string, error) { - panic("Not implemented") -} - -func (client mockClient) RenameContainer(c container.Container, s string) error { - panic("Not implemented") -} - -func (client mockClient) RemoveImage(c container.Container) error { - client.TestData.TriedToRemoveImage = true - return nil -} - -func (client mockClient) GetContainer(containerID string) (container.Container, error) { - return container.Container{}, nil -} - -func (client mockClient) ExecuteCommand(containerID string, command string) error { - return nil -} - -func (client mockClient) IsContainerStale(c container.Container) (bool, error) { - panic("Not implemented") -} diff --git a/internal/actions/check.go b/internal/actions/check.go index 8574300..b60aec5 100644 --- a/internal/actions/check.go +++ b/internal/actions/check.go @@ -51,7 +51,7 @@ func cleanupExcessWatchtowers(containers []container.Container, client container } if cleanup { - if err := client.RemoveImage(c); err != nil { + if err := client.RemoveImageByID(c.ImageID()); err != nil { // logging the original here as we're just returning a count logrus.Error(err) cleanupErrors++ diff --git a/internal/actions/mocks/client.go b/internal/actions/mocks/client.go new file mode 100644 index 0000000..e01043a --- /dev/null +++ b/internal/actions/mocks/client.go @@ -0,0 +1,72 @@ +package mocks + +import ( + "errors" + "github.com/containrrr/watchtower/pkg/container" + "time" + + t "github.com/containrrr/watchtower/pkg/types" + cli "github.com/docker/docker/client" +) + +type MockClient struct { + TestData *TestData + api cli.CommonAPIClient + pullImages bool + removeVolumes bool +} + +type TestData struct { + TriedToRemoveImageCount int + NameOfContainerToKeep string + Containers []container.Container +} + +func (testdata *TestData) TriedToRemoveImage() bool { + return testdata.TriedToRemoveImageCount > 0 +} + +func CreateMockClient(data *TestData, api cli.CommonAPIClient, pullImages bool, removeVolumes bool) MockClient { + return MockClient { + data, + api, + pullImages, + removeVolumes, + } +} + +func (client MockClient) ListContainers(f t.Filter) ([]container.Container, error) { + return client.TestData.Containers, nil +} + +func (client MockClient) StopContainer(c container.Container, d time.Duration) error { + if c.Name() == client.TestData.NameOfContainerToKeep { + return errors.New("tried to stop the instance we want to keep") + } + return nil +} +func (client MockClient) StartContainer(c container.Container) (string, error) { + return "", nil +} + +func (client MockClient) RenameContainer(c container.Container, s string) error { + return nil +} + +func (client MockClient) RemoveImageByID(id string) error { + client.TestData.TriedToRemoveImageCount++ + return nil +} + +func (client MockClient) GetContainer(containerID string) (container.Container, error) { + return container.Container{}, nil +} + +func (client MockClient) ExecuteCommand(containerID string, command string) error { + return nil +} + +func (client MockClient) IsContainerStale(c container.Container) (bool, error) { + return true, nil +} + diff --git a/internal/actions/mocks/container.go b/internal/actions/mocks/container.go new file mode 100644 index 0000000..4a56106 --- /dev/null +++ b/internal/actions/mocks/container.go @@ -0,0 +1,28 @@ +package mocks + +import ( + "github.com/containrrr/watchtower/pkg/container" + "github.com/docker/docker/api/types" + container2 "github.com/docker/docker/api/types/container" + "time" +) + +func CreateMockContainer(id string, name string, image string, created time.Time) container.Container { + content := types.ContainerJSON{ + ContainerJSONBase: &types.ContainerJSONBase{ + ID: id, + Image: image, + Name: name, + Created: created.String(), + }, + Config: &container2.Config{ + Labels: make(map[string]string), + }, + } + return *container.NewContainer( + &content, + &types.ImageInspect{ + ID: image, + }, + ) +} diff --git a/internal/actions/update.go b/internal/actions/update.go index 37fb22e..5461f35 100644 --- a/internal/actions/update.go +++ b/internal/actions/update.go @@ -73,17 +73,18 @@ func stopStaleContainer(container container.Container, client container.Client, } func restartContainersInSortedOrder(containers []container.Container, client container.Client, params UpdateParams) { - toDelete := make(map[container.Container]bool) + imageIDs := make(map[string]bool) + for _, container := range containers { if !container.Stale { continue } restartStaleContainer(container, client, params) - toDelete[container] = true + imageIDs[container.ImageID()] = true } if params.Cleanup { - for cont := range toDelete { - if err := client.RemoveImage(cont); err != nil { + for imageID := range imageIDs { + if err := client.RemoveImageByID(imageID); err != nil { log.Error(err) } } diff --git a/internal/actions/update_test.go b/internal/actions/update_test.go new file mode 100644 index 0000000..4ab6c8a --- /dev/null +++ b/internal/actions/update_test.go @@ -0,0 +1,84 @@ +package actions_test + +import ( + "github.com/containrrr/watchtower/internal/actions" + "github.com/containrrr/watchtower/pkg/container" + "github.com/containrrr/watchtower/pkg/container/mocks" + cli "github.com/docker/docker/client" + "time" + + . "github.com/containrrr/watchtower/internal/actions/mocks" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + + +var _ = Describe("the update action", func() { + var dockerClient cli.CommonAPIClient + var client MockClient + + BeforeEach(func() { + server := mocks.NewMockAPIServer() + dockerClient, _ = cli.NewClientWithOpts( + cli.WithHost(server.URL), + cli.WithHTTPClient(server.Client())) + }) + + + When("watchtower has been instructed to clean up", func() { + BeforeEach(func() { + pullImages := false + removeVolumes := false + client = CreateMockClient( + &TestData{ + NameOfContainerToKeep: "test-container-02", + Containers: []container.Container{ + CreateMockContainer( + "test-container-01", + "test-container-01", + "fake-image:latest", + time.Now().AddDate(0, 0, -1)), + CreateMockContainer( + "test-container-02", + "test-container-02", + "fake-image:latest", + time.Now()), + CreateMockContainer( + "test-container-02", + "test-container-02", + "fake-image:latest", + time.Now()), + }, + }, + dockerClient, + pullImages, + removeVolumes, + ) + }) + + When("there are multiple containers using the same image", func() { + It("should only try to remove the image once", func() { + + err := actions.Update(client, actions.UpdateParams{ Cleanup: true }) + Expect(err).NotTo(HaveOccurred()) + Expect(client.TestData.TriedToRemoveImageCount).To(Equal(1)) + }) + }) + When("there are multiple containers using different images", func() { + It("should try to remove each of them", func() { + client.TestData.Containers = append( + client.TestData.Containers, + CreateMockContainer( + "unique-test-container", + "unique-test-container", + "unique-fake-image:latest", + time.Now(), + ), + ) + err := actions.Update(client, actions.UpdateParams{ Cleanup: true }) + Expect(err).NotTo(HaveOccurred()) + Expect(client.TestData.TriedToRemoveImageCount).To(Equal(2)) + }) + }) + }) +}) diff --git a/pkg/container/client.go b/pkg/container/client.go index 5877eb4..ab69d40 100644 --- a/pkg/container/client.go +++ b/pkg/container/client.go @@ -29,7 +29,8 @@ type Client interface { RenameContainer(Container, string) error IsContainerStale(Container) (bool, error) ExecuteCommand(containerID string, command string) error - RemoveImage(Container) error + RemoveImageByID(string) error + } // NewClient returns a new Client instance which can be used to interact with @@ -156,7 +157,7 @@ func (client dockerClient) StopContainer(c Container, timeout time.Duration) err // Wait for container to be removed. In this case an error is a good thing if err := client.waitForStopOrTimeout(c, timeout); err == nil { - return fmt.Errorf("Container %s (%s) could not be removed", c.Name(), c.ID()) + return fmt.Errorf("container %s (%s) could not be removed", c.Name(), c.ID()) } return nil @@ -279,10 +280,16 @@ func (client dockerClient) IsContainerStale(c Container) (bool, error) { return false, nil } -func (client dockerClient) RemoveImage(c Container) error { - imageID := c.ImageID() - log.Infof("Removing image %s", imageID) - _, err := client.api.ImageRemove(context.Background(), imageID, types.ImageRemoveOptions{Force: true}) +func (client dockerClient) RemoveImageByID(id string) error { + log.Infof("Removing image %s", id) + + _, err := client.api.ImageRemove( + context.Background(), + id, + types.ImageRemoveOptions{ + Force: true, + }) + return err }