Avoid redundant HEAD requests for identical layers on push

pushV2Tag already deduplicates layers, but the scope of this
deduplication is only for a particular tag. If we are pushing all tags
in a repository, we may check layers several times. Fix this by moving
the layersSeen map from the pushV2Tag function to the v2Pusher struct.

In addition to avoiding some useless round-trips, this makes the "docker
push" output less confusing. It formerly could contain many repeated
lines like:

    124e2127157f: Image already exists
    124e2127157f: Image already exists
    ...

Add test coverage based on the "docker push" output: a hash should not
appear multiple times when pushing multiple tags.

Fixes #14873

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

graph/push.go

@@ -29,12 +29,13 @@ func (s *TagStore) NewPusher(endpoint registry.APIEndpoint, localRepo Repository
 	switch endpoint.Version {
 	case registry.APIVersion2:
 		return &v2Pusher{
-			TagStore:  s,
-			endpoint:  endpoint,
-			localRepo: localRepo,
-			repoInfo:  repoInfo,
-			config:    imagePushConfig,
-			sf:        sf,
+			TagStore:   s,
+			endpoint:   endpoint,
+			localRepo:  localRepo,
+			repoInfo:   repoInfo,
+			config:     imagePushConfig,
+			sf:         sf,
+			layersSeen: make(map[string]bool),
 		}, nil
 	case registry.APIVersion1:
 		return &v1Pusher{

graph/push_v2.go

@@ -26,6 +26,11 @@ type v2Pusher struct {
 	config    *ImagePushConfig
 	sf        *streamformatter.StreamFormatter
 	repo      distribution.Repository
+
+	// layersSeen is the set of layers known to exist on the remote side.
+	// This avoids redundant queries when pushing multiple tags that
+	// involve the same layers.
+	layersSeen map[string]bool
 }
 
 func (p *v2Pusher) Push() (fallback bool, err error) {
@@ -86,8 +91,6 @@ func (p *v2Pusher) pushV2Tag(tag string) error {
 		return fmt.Errorf("tag does not exist: %s", tag)
 	}
 
-	layersSeen := make(map[string]bool)
-
 	layer, err := p.graph.Get(layerId)
 	if err != nil {
 		return err
@@ -116,7 +119,7 @@ func (p *v2Pusher) pushV2Tag(tag string) error {
 			return err
 		}
 
-		if layersSeen[layer.ID] {
+		if p.layersSeen[layer.ID] {
 			break
 		}
 
@@ -171,7 +174,7 @@ func (p *v2Pusher) pushV2Tag(tag string) error {
 		m.FSLayers = append(m.FSLayers, manifest.FSLayer{BlobSum: dgst})
 		m.History = append(m.History, manifest.History{V1Compatibility: string(jsonData)})
 
-		layersSeen[layer.ID] = true
+		p.layersSeen[layer.ID] = true
 	}
 
 	logrus.Infof("Signed manifest for %s:%s using daemon's key: %s", p.repo.Name(), tag, p.trustKey.KeyID())

integration-cli/docker_cli_push_test.go

@@ -60,7 +60,32 @@ func (s *DockerRegistrySuite) TestPushMultipleTags(c *check.C) {
 
 	dockerCmd(c, "tag", "busybox", repoTag2)
 
-	dockerCmd(c, "push", repoName)
+	out, _ := dockerCmd(c, "push", repoName)
+
+	// There should be no duplicate hashes in the output
+	imageSuccessfullyPushed := ": Image successfully pushed"
+	imageAlreadyExists := ": Image already exists"
+	imagePushHashes := make(map[string]struct{})
+	outputLines := strings.Split(out, "\n")
+	for _, outputLine := range outputLines {
+		if strings.Contains(outputLine, imageSuccessfullyPushed) {
+			hash := strings.TrimSuffix(outputLine, imageSuccessfullyPushed)
+			if _, present := imagePushHashes[hash]; present {
+				c.Fatalf("Duplicate image push: %s", outputLine)
+			}
+			imagePushHashes[hash] = struct{}{}
+		} else if strings.Contains(outputLine, imageAlreadyExists) {
+			hash := strings.TrimSuffix(outputLine, imageAlreadyExists)
+			if _, present := imagePushHashes[hash]; present {
+				c.Fatalf("Duplicate image push: %s", outputLine)
+			}
+			imagePushHashes[hash] = struct{}{}
+		}
+	}
+
+	if len(imagePushHashes) == 0 {
+		c.Fatal(`Expected at least one line containing "Image successfully pushed"`)
+	}
 }
 
 func (s *DockerRegistrySuite) TestPushInterrupt(c *check.C) {
@@ -79,8 +104,7 @@ func (s *DockerRegistrySuite) TestPushInterrupt(c *check.C) {
 		c.Fatalf("Failed to kill push process: %v", err)
 	}
 	if out, _, err := dockerCmdWithError(c, "push", repoName); err == nil {
-		str := string(out)
-		if !strings.Contains(str, "already in progress") {
+		if !strings.Contains(out, "already in progress") {
 			c.Fatalf("Push should be continued on daemon side, but seems ok: %v, %s", err, out)
 		}
 	}