history: rewrite mappings

Rewrite the backend for displaying the history of an image to simplify
the code and be closer to docker's behaviour.  Instead of driving
index-based heuristics, create a reverse mapping from top-layers to the
corresponding image IDs and lookup the layers on-demand.  Also use the
uncompressed layer size to be closer to Docker's behaviour.

Note that intermediate images from local builds are not considered for
the ID lookups anymore.

Fixes: #3359
Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

libpod/image/image.go

@@ -765,109 +765,65 @@ func (i *Image) History(ctx context.Context) ([]*History, error) {
 		return nil, err
 	}
 
-	// Use our layers list to find images that use any of them (or no
-	// layer, since every base layer is derived from an empty layer) as its
-	// topmost layer.
-	interestingLayers := make(map[string]bool)
-	var layer *storage.Layer
-	if i.TopLayer() != "" {
-		if layer, err = i.imageruntime.store.Layer(i.TopLayer()); err != nil {
-			return nil, err
+	// Build a mapping from top-layer to image ID.
+	images, err := i.imageruntime.GetImages()
+	if err != nil {
+		return nil, err
+	}
+	topLayerMap := make(map[string]string)
+	for _, image := range images {
+		if _, exists := topLayerMap[image.TopLayer()]; !exists {
+			topLayerMap[image.TopLayer()] = image.ID()
 		}
 	}
-	interestingLayers[""] = true
-	for layer != nil {
-		interestingLayers[layer.ID] = true
-		if layer.Parent == "" {
-			break
-		}
-		layer, err = i.imageruntime.store.Layer(layer.Parent)
+
+	var allHistory []*History
+	var layer *storage.Layer
+
+	// Check if we have an actual top layer to prevent lookup errors.
+	if i.TopLayer() != "" {
+		layer, err = i.imageruntime.store.Layer(i.TopLayer())
 		if err != nil {
 			return nil, err
 		}
 	}
 
-	// Get the IDs of the images that share some of our layers.  Hopefully
-	// this step means that we'll be able to avoid reading the
-	// configuration of every single image in local storage later on.
-	images, err := i.imageruntime.GetImages()
-	if err != nil {
-		return nil, errors.Wrapf(err, "error getting images from store")
-	}
-	interestingImages := make([]*Image, 0, len(images))
-	for i := range images {
-		if interestingLayers[images[i].TopLayer()] {
-			interestingImages = append(interestingImages, images[i])
-		}
-	}
+	// Iterate in reverse order over the history entries, and lookup the
+	// corresponding image ID, size and get the next later if needed.
+	numHistories := len(oci.History) - 1
+	for x := numHistories; x >= 0; x-- {
+		var size int64
 
-	// Build a list of image IDs that correspond to our history entries.
-	historyImages := make([]*Image, len(oci.History))
-	if len(oci.History) > 0 {
-		// The starting image shares its whole history with itself.
-		historyImages[len(historyImages)-1] = i
-		for i := range interestingImages {
-			image, err := images[i].ociv1Image(ctx)
-			if err != nil {
-				return nil, errors.Wrapf(err, "error getting image configuration for image %q", images[i].ID())
+		id := "<missing>"
+		if x == numHistories {
+			id = i.ID()
+		} else if layer != nil {
+			if !oci.History[x].EmptyLayer {
+				size = layer.UncompressedSize
 			}
-			// If the candidate has a longer history or no history
-			// at all, then it doesn't share the portion of our
-			// history that we're interested in matching with other
-			// images.
-			if len(image.History) == 0 || len(image.History) > len(historyImages) {
-				continue
-			}
-			// If we don't include all of the layers that the
-			// candidate image does (i.e., our rootfs didn't look
-			// like its rootfs at any point), then it can't be part
-			// of our history.
-			if len(image.RootFS.DiffIDs) > len(oci.RootFS.DiffIDs) {
-				continue
-			}
-			candidateLayersAreUsed := true
-			for i := range image.RootFS.DiffIDs {
-				if image.RootFS.DiffIDs[i] != oci.RootFS.DiffIDs[i] {
-					candidateLayersAreUsed = false
-					break
-				}
-			}
-			if !candidateLayersAreUsed {
-				continue
-			}
-			// If the candidate's entire history is an initial
-			// portion of our history, then we're based on it,
-			// either directly or indirectly.
-			sharedHistory := historiesMatch(oci.History, image.History)
-			if sharedHistory == len(image.History) {
-				historyImages[sharedHistory-1] = images[i]
+			if imageID, exists := topLayerMap[layer.ID]; exists {
+				id = imageID
+				// Delete the entry to avoid reusing it for following history items.
+				delete(topLayerMap, layer.ID)
 			}
 		}
-	}
 
-	var (
-		size       int64
-		sizeCount  = 1
-		allHistory []*History
-	)
-
-	for i := len(oci.History) - 1; i >= 0; i-- {
-		imageID := "<missing>"
-		if historyImages[i] != nil {
-			imageID = historyImages[i].ID()
-		}
-		if !oci.History[i].EmptyLayer {
-			size = img.LayerInfos()[len(img.LayerInfos())-sizeCount].Size
-			sizeCount++
-		}
 		allHistory = append(allHistory, &History{
-			ID:        imageID,
-			Created:   oci.History[i].Created,
-			CreatedBy: oci.History[i].CreatedBy,
+			ID:        id,
+			Created:   oci.History[x].Created,
+			CreatedBy: oci.History[x].CreatedBy,
 			Size:      size,
-			Comment:   oci.History[i].Comment,
+			Comment:   oci.History[x].Comment,
 		})
+
+		if layer != nil && layer.Parent != "" && !oci.History[x].EmptyLayer {
+			layer, err = i.imageruntime.store.Layer(layer.Parent)
+			if err != nil {
+				return nil, err
+			}
+		}
 	}
+
 	return allHistory, nil
 }
 

test/e2e/images_test.go

@@ -360,7 +360,6 @@ LABEL "com.example.vendor"="Example Vendor"
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 		output = session.OutputToString()
-		Expect(output).To(Not(MatchRegexp("<missing>")))
 		Expect(output).To(Not(MatchRegexp("error")))
 
 		session = podmanTest.Podman([]string{"history", "--quiet", "foo"})