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

Author: piksel

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,14 +2,15 @@ package actions
 
 import (
 	"errors"
+	"strings"
+
 	"github.com/containrrr/watchtower/internal/util"
 	"github.com/containrrr/watchtower/pkg/container"
 	"github.com/containrrr/watchtower/pkg/lifecycle"
 	"github.com/containrrr/watchtower/pkg/session"
 	"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)
 		}