fix: correctly handle non-stale restarts (#1220)

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

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,
+	)
+}

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

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