From e9c83af533b4cf763c8b66c652c0a80dbeacbd75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Mon, 18 Apr 2022 19:36:38 +0200 Subject: [PATCH] fix: correctly handle non-stale restarts (#1220) --- internal/actions/mocks/client.go | 14 ++- internal/actions/mocks/container.go | 51 +++++++-- internal/actions/update.go | 27 ++++- internal/actions/update_test.go | 167 ++++++++++++++++------------ 4 files changed, 166 insertions(+), 93 deletions(-) diff --git a/internal/actions/mocks/client.go b/internal/actions/mocks/client.go index 4844a55..2afc43c 100644 --- a/internal/actions/mocks/client.go +++ b/internal/actions/mocks/client.go @@ -3,9 +3,10 @@ package mocks import ( "errors" "fmt" - "github.com/containrrr/watchtower/pkg/container" "time" + "github.com/containrrr/watchtower/pkg/container" + t "github.com/containrrr/watchtower/pkg/types" ) @@ -21,6 +22,7 @@ type TestData struct { TriedToRemoveImageCount int NameOfContainerToKeep string Containers []container.Container + Staleness map[string]bool } // TriedToRemoveImage is a test helper function to check whether RemoveImageByID has been called @@ -85,9 +87,13 @@ func (client MockClient) ExecuteCommand(_ t.ContainerID, command string, _ int) } } -// IsContainerStale is always true for the mock client -func (client MockClient) IsContainerStale(_ container.Container) (bool, t.ImageID, error) { - return true, "", nil +// IsContainerStale is true if not explicitly stated in TestData for the mock client +func (client MockClient) IsContainerStale(cont container.Container) (bool, t.ImageID, error) { + stale, found := client.TestData.Staleness[cont.Name()] + if !found { + stale = true + } + return stale, "", nil } // WarnOnHeadPullFailed is always true for the mock client diff --git a/internal/actions/mocks/container.go b/internal/actions/mocks/container.go index 167d571..3272d63 100644 --- a/internal/actions/mocks/container.go +++ b/internal/actions/mocks/container.go @@ -2,14 +2,15 @@ package mocks import ( "fmt" + "strconv" + "strings" + "time" + "github.com/containrrr/watchtower/pkg/container" wt "github.com/containrrr/watchtower/pkg/types" "github.com/docker/docker/api/types" dockerContainer "github.com/docker/docker/api/types/container" "github.com/docker/go-connections/nat" - "strconv" - "strings" - "time" ) // CreateMockContainer creates a container substitute valid for testing @@ -32,15 +33,20 @@ func CreateMockContainer(id string, name string, image string, created time.Time } return *container.NewContainer( &content, - &types.ImageInspect{ - ID: image, - RepoDigests: []string{ - image, - }, - }, + CreateMockImageInfo(image), ) } +// CreateMockImageInfo returns a mock image info struct based on the passed image +func CreateMockImageInfo(image string) *types.ImageInspect { + return &types.ImageInspect{ + ID: image, + RepoDigests: []string{ + image, + }, + } +} + // CreateMockContainerWithImageInfo should only be used for testing func CreateMockContainerWithImageInfo(id string, name string, image string, created time.Time, imageInfo types.ImageInspect) container.Container { return CreateMockContainerWithImageInfoP(id, name, image, created, &imageInfo) @@ -93,9 +99,7 @@ func CreateMockContainerWithConfig(id string, name string, image string, running } return *container.NewContainer( &content, - &types.ImageInspect{ - ID: image, - }, + CreateMockImageInfo(image), ) } @@ -114,3 +118,26 @@ func CreateContainerForProgress(index int, idPrefix int, nameFormat string) (con c := CreateMockContainerWithConfig(contID, contName, oldImgID, true, false, time.Now(), config) return c, wt.ImageID(newImgID) } + +// CreateMockContainerWithLinks should only be used for testing +func CreateMockContainerWithLinks(id string, name string, image string, created time.Time, links []string, imageInfo *types.ImageInspect) container.Container { + content := types.ContainerJSON{ + ContainerJSONBase: &types.ContainerJSONBase{ + ID: id, + Image: image, + Name: name, + Created: created.String(), + HostConfig: &dockerContainer.HostConfig{ + Links: links, + }, + }, + Config: &dockerContainer.Config{ + Image: image, + Labels: make(map[string]string), + }, + } + return *container.NewContainer( + &content, + imageInfo, + ) +} diff --git a/internal/actions/update.go b/internal/actions/update.go index e0f7065..bd3791f 100644 --- a/internal/actions/update.go +++ b/internal/actions/update.go @@ -2,6 +2,8 @@ package actions import ( "errors" + "strings" + "github.com/containrrr/watchtower/internal/util" "github.com/containrrr/watchtower/pkg/container" "github.com/containrrr/watchtower/pkg/lifecycle" @@ -9,7 +11,6 @@ import ( "github.com/containrrr/watchtower/pkg/sorter" "github.com/containrrr/watchtower/pkg/types" log "github.com/sirupsen/logrus" - "strings" ) // Update looks at the running Docker containers to see if any of the images @@ -108,8 +109,10 @@ func performRollingRestart(containers []container.Container, client container.Cl } else { if err := restartStaleContainer(containers[i], client, params); err != nil { failed[containers[i].ID()] = err + } else if containers[i].Stale { + // Only add (previously) stale containers' images to cleanup + cleanupImageIDs[containers[i].ImageID()] = true } - cleanupImageIDs[containers[i].ImageID()] = true } } } @@ -127,7 +130,8 @@ func stopContainersInReversedOrder(containers []container.Container, client cont if err := stopStaleContainer(containers[i], client, params); err != nil { failed[containers[i].ID()] = err } else { - stopped[containers[i].ImageID()] = true + // NOTE: If a container is restarted due to a dependency this might be empty + stopped[containers[i].SafeImageID()] = true } } @@ -143,6 +147,14 @@ func stopStaleContainer(container container.Container, client container.Client, if !container.ToRestart() { return nil } + + // Perform an additional check here to prevent us from stopping a linked container we cannot restart + if container.LinkedToRestarting { + if err := container.VerifyConfiguration(); err != nil { + return err + } + } + if params.LifecycleHooks { skipUpdate, err := lifecycle.ExecutePreUpdateCommand(client, container) if err != nil { @@ -171,11 +183,13 @@ func restartContainersInSortedOrder(containers []container.Container, client con if !c.ToRestart() { continue } - if stoppedImages[c.ImageID()] { + if stoppedImages[c.SafeImageID()] { if err := restartStaleContainer(c, client, params); err != nil { failed[c.ID()] = err + } else if c.Stale { + // Only add (previously) stale containers' images to cleanup + cleanupImageIDs[c.ImageID()] = true } - cleanupImageIDs[c.ImageID()] = true } } @@ -188,6 +202,9 @@ func restartContainersInSortedOrder(containers []container.Container, client con func cleanupImages(client container.Client, imageIDs map[types.ImageID]bool) { for imageID := range imageIDs { + if imageID == "" { + continue + } if err := client.RemoveImageByID(imageID); err != nil { log.Error(err) } diff --git a/internal/actions/update_test.go b/internal/actions/update_test.go index 7d392d7..eb540b1 100644 --- a/internal/actions/update_test.go +++ b/internal/actions/update_test.go @@ -1,55 +1,76 @@ package actions_test import ( + "time" + "github.com/containrrr/watchtower/internal/actions" "github.com/containrrr/watchtower/pkg/container" "github.com/containrrr/watchtower/pkg/types" + dockerTypes "github.com/docker/docker/api/types" dockerContainer "github.com/docker/docker/api/types/container" "github.com/docker/go-connections/nat" - "time" . "github.com/containrrr/watchtower/internal/actions/mocks" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) -var _ = Describe("the update action", func() { - var client MockClient +func getCommonTestData(keepContainer string) *TestData { + return &TestData{ + NameOfContainerToKeep: keepContainer, + 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()), + }, + } +} + +func getLinkedTestData(withImageInfo bool) *TestData { + staleContainer := CreateMockContainer( + "test-container-01", + "/test-container-01", + "fake-image1:latest", + time.Now().AddDate(0, 0, -1)) + + var imageInfo *dockerTypes.ImageInspect + if withImageInfo { + imageInfo = CreateMockImageInfo("test-container-02") + } + linkingContainer := CreateMockContainerWithLinks( + "test-container-02", + "/test-container-02", + "fake-image2:latest", + time.Now(), + []string{staleContainer.Name()}, + imageInfo) + + return &TestData{ + Staleness: map[string]bool{linkingContainer.Name(): false}, + Containers: []container.Container{ + staleContainer, + linkingContainer, + }, + } +} +var _ = Describe("the update action", func() { When("watchtower has been instructed to clean up", func() { - BeforeEach(func() { - pullImages := false - removeVolumes := false - //goland:noinspection GoBoolExpressions - 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()), - }, - }, - pullImages, - removeVolumes, - ) - }) - When("there are multiple containers using the same image", func() { It("should only try to remove the image once", func() { - + client := CreateMockClient(getCommonTestData(""), false, false) _, err := actions.Update(client, types.UpdateParams{Cleanup: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(1)) @@ -57,8 +78,9 @@ var _ = Describe("the update action", func() { }) 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, + testData := getCommonTestData("") + testData.Containers = append( + testData.Containers, CreateMockContainer( "unique-test-container", "unique-test-container", @@ -66,25 +88,46 @@ var _ = Describe("the update action", func() { time.Now(), ), ) + client := CreateMockClient(testData, false, false) _, err := actions.Update(client, types.UpdateParams{Cleanup: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(2)) }) }) + When("there are linked containers being updated", func() { + It("should not try to remove their images", func() { + client := CreateMockClient(getLinkedTestData(true), false, false) + _, err := actions.Update(client, types.UpdateParams{Cleanup: true}) + Expect(err).NotTo(HaveOccurred()) + Expect(client.TestData.TriedToRemoveImageCount).To(Equal(1)) + }) + }) When("performing a rolling restart update", func() { It("should try to remove the image once", func() { - + client := CreateMockClient(getCommonTestData(""), false, false) _, err := actions.Update(client, types.UpdateParams{Cleanup: true, RollingRestart: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(1)) }) }) + When("updating a linked container with missing image info", func() { + It("should gracefully fail", func() { + client := CreateMockClient(getLinkedTestData(false), false, false) + + report, err := actions.Update(client, types.UpdateParams{}) + Expect(err).NotTo(HaveOccurred()) + // Note: Linked containers that were skipped for recreation is not counted in Failed + // If this happens, an error is emitted to the logs, so a notification should still be sent. + Expect(report.Updated()).To(HaveLen(1)) + Expect(report.Fresh()).To(HaveLen(1)) + }) + }) }) When("watchtower has been instructed to monitor only", func() { When("certain containers are set to monitor only", func() { - BeforeEach(func() { - client = CreateMockClient( + It("should not update those containers", func() { + client := CreateMockClient( &TestData{ NameOfContainerToKeep: "test-container-02", Containers: []container.Container{ @@ -110,9 +153,6 @@ var _ = Describe("the update action", func() { false, false, ) - }) - - It("should not update those containers", func() { _, err := actions.Update(client, types.UpdateParams{Cleanup: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(1)) @@ -120,8 +160,8 @@ var _ = Describe("the update action", func() { }) When("monitor only is set globally", func() { - BeforeEach(func() { - client = CreateMockClient( + It("should not update any containers", func() { + client := CreateMockClient( &TestData{ Containers: []container.Container{ CreateMockContainer( @@ -139,9 +179,6 @@ var _ = Describe("the update action", func() { false, false, ) - }) - - It("should not update any containers", func() { _, err := actions.Update(client, types.UpdateParams{MonitorOnly: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(0)) @@ -152,9 +189,9 @@ var _ = Describe("the update action", func() { When("watchtower has been instructed to run lifecycle hooks", func() { - When("prupddate script returns 1", func() { - BeforeEach(func() { - client = CreateMockClient( + When("pre-update script returns 1", func() { + It("should not update those containers", func() { + client := CreateMockClient( &TestData{ //NameOfContainerToKeep: "test-container-02", Containers: []container.Container{ @@ -177,9 +214,7 @@ var _ = Describe("the update action", func() { false, false, ) - }) - It("should not update those containers", func() { _, err := actions.Update(client, types.UpdateParams{Cleanup: true, LifecycleHooks: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(0)) @@ -188,8 +223,8 @@ var _ = Describe("the update action", func() { }) When("prupddate script returns 75", func() { - BeforeEach(func() { - client = CreateMockClient( + It("should not update those containers", func() { + client := CreateMockClient( &TestData{ //NameOfContainerToKeep: "test-container-02", Containers: []container.Container{ @@ -212,9 +247,6 @@ var _ = Describe("the update action", func() { false, false, ) - }) - - It("should not update those containers", func() { _, err := actions.Update(client, types.UpdateParams{Cleanup: true, LifecycleHooks: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(0)) @@ -223,8 +255,8 @@ var _ = Describe("the update action", func() { }) When("prupddate script returns 0", func() { - BeforeEach(func() { - client = CreateMockClient( + It("should update those containers", func() { + client := CreateMockClient( &TestData{ //NameOfContainerToKeep: "test-container-02", Containers: []container.Container{ @@ -247,9 +279,6 @@ var _ = Describe("the update action", func() { false, false, ) - }) - - It("should update those containers", func() { _, err := actions.Update(client, types.UpdateParams{Cleanup: true, LifecycleHooks: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(1)) @@ -305,8 +334,8 @@ var _ = Describe("the update action", func() { }) When("container is not running", func() { - BeforeEach(func() { - client = CreateMockClient( + It("skip running preupdate", func() { + client := CreateMockClient( &TestData{ //NameOfContainerToKeep: "test-container-02", Containers: []container.Container{ @@ -329,9 +358,6 @@ var _ = Describe("the update action", func() { false, false, ) - }) - - It("skip running preupdate", func() { _, err := actions.Update(client, types.UpdateParams{Cleanup: true, LifecycleHooks: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(1)) @@ -340,8 +366,8 @@ var _ = Describe("the update action", func() { }) When("container is restarting", func() { - BeforeEach(func() { - client = CreateMockClient( + It("skip running preupdate", func() { + client := CreateMockClient( &TestData{ //NameOfContainerToKeep: "test-container-02", Containers: []container.Container{ @@ -364,9 +390,6 @@ var _ = Describe("the update action", func() { false, false, ) - }) - - It("skip running preupdate", func() { _, err := actions.Update(client, types.UpdateParams{Cleanup: true, LifecycleHooks: true}) Expect(err).NotTo(HaveOccurred()) Expect(client.TestData.TriedToRemoveImageCount).To(Equal(1))