fix: missing tags on images with multiple tags for minikube image list

pkg/minikube/machine/cache_image_test.go

@@ -0,0 +1,257 @@
+/*
+Copyright 2023 The Kubernetes Authors All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package machine
+
+import (
+	"reflect"
+	"sort"
+	"testing"
+
+	"k8s.io/minikube/pkg/minikube/cruntime"
+)
+
+type CacheImageTestCase struct {
+	description string
+	images      [][]cruntime.ListImage
+	expected    []cruntime.ListImage
+}
+
+func TestMergeImageLists(t *testing.T) {
+	// test case 0: from #16556
+	// e.g. on node 1 image1 have an item with k8s.gcr.io/image1:v1.0.0 tag
+	// and another item with registry.k8s.io/image1:v1.0.0 tag too
+	testCase0 := CacheImageTestCase{
+		description: "same images with multiple tags appear on one node",
+		images: [][]cruntime.ListImage{
+			[]cruntime.ListImage{
+				{
+					ID:          "image_id_1",
+					RepoDigests: []string{"image_digest_1"},
+					RepoTags:    []string{"k8s.gcr.io/image1:v1.0.0"},
+					Size:        "1",
+				},
+				{
+					ID:          "image_id_2",
+					RepoDigests: []string{"image_digest_2"},
+					RepoTags:    []string{"registry.k8s.io/image2:v1.0.0"},
+					Size:        "1",
+				},
+
+				{
+					ID:          "image_id_1",
+					RepoDigests: []string{"image_digest_1"},
+					RepoTags:    []string{"registry.k8s.io/image1:v1.0.0"},
+					Size:        "1",
+				},
+				{
+					ID:          "image_id_2",
+					RepoDigests: []string{"image_digest_2"},
+					RepoTags:    []string{"k8s.gcr.io/image2:v1.0.0"},
+					Size:        "1",
+				},
+			},
+		},
+		expected: []cruntime.ListImage{
+			{
+				ID:          "image_id_1",
+				RepoDigests: []string{"image_digest_1"},
+				RepoTags:    []string{"k8s.gcr.io/image1:v1.0.0", "registry.k8s.io/image1:v1.0.0"},
+				Size:        "1",
+			},
+			{
+				ID:          "image_id_2",
+				RepoDigests: []string{"image_digest_2"},
+				RepoTags:    []string{"k8s.gcr.io/image2:v1.0.0", "registry.k8s.io/image2:v1.0.0"},
+				Size:        "1",
+			},
+		},
+	}
+	// test case 1: from #16557
+	// e.g. image1 have k8s.gcr.io/image1:v1.0.0 tag on node 1 and registry.k8s.io/image1:v1.0.0 on node 2
+	testCase1 := CacheImageTestCase{
+		description: "same images with multiple tags appear on two node",
+		images: [][]cruntime.ListImage{
+			{
+				{
+					ID:          "image_id_1",
+					RepoDigests: []string{"image_digest_1"},
+					RepoTags:    []string{"k8s.gcr.io/image1:v1.0.0"},
+					Size:        "1",
+				},
+				{
+					ID:          "image_id_2",
+					RepoDigests: []string{"image_digest_2"},
+					RepoTags:    []string{"registry.k8s.io/image2:v1.0.0"},
+					Size:        "1",
+				},
+			},
+			{
+				{
+					ID:          "image_id_1",
+					RepoDigests: []string{"image_digest_1"},
+					RepoTags:    []string{"registry.k8s.io/image1:v1.0.0"},
+					Size:        "1",
+				},
+				{
+					ID:          "image_id_2",
+					RepoDigests: []string{"image_digest_2"},
+					RepoTags:    []string{"k8s.gcr.io/image2:v1.0.0"},
+					Size:        "1",
+				},
+			},
+		},
+		expected: []cruntime.ListImage{
+			{
+				ID:          "image_id_1",
+				RepoDigests: []string{"image_digest_1"},
+				RepoTags:    []string{"k8s.gcr.io/image1:v1.0.0", "registry.k8s.io/image1:v1.0.0"},
+				Size:        "1",
+			},
+			{
+				ID:          "image_id_2",
+				RepoDigests: []string{"image_digest_2"},
+				RepoTags:    []string{"k8s.gcr.io/image2:v1.0.0", "registry.k8s.io/image2:v1.0.0"},
+				Size:        "1",
+			},
+		},
+	}
+	// test case 2: from #16557
+	// e.g. image1 have k8s.gcr.io/image1:v1.0.0 tag on node 1
+	// and both k8s.gcr.io/image1:v1.0.0 and registry.k8s.io/image1:v1.0.0 on node 2
+	testCase2 := CacheImageTestCase{
+		description: "image has tag1 on node1 and both tag1 & tag2 on node 2",
+		images: [][]cruntime.ListImage{
+			{
+				{
+					ID:          "image_id_1",
+					RepoDigests: []string{"image_digest_1"},
+					RepoTags:    []string{"k8s.gcr.io/image1:v1.0.0"},
+					Size:        "1",
+				},
+				{
+					ID:          "image_id_2",
+					RepoDigests: []string{"image_digest_2"},
+					RepoTags:    []string{"registry.k8s.io/image2:v1.0.0", "k8s.gcr.io/image2:v1.0.0"},
+					Size:        "1",
+				},
+			},
+			{
+				{
+					ID:          "image_id_1",
+					RepoDigests: []string{"image_digest_1"},
+					RepoTags:    []string{"registry.k8s.io/image1:v1.0.0", "k8s.gcr.io/image1:v1.0.0"},
+					Size:        "1",
+				},
+				{
+					ID:          "image_id_2",
+					RepoDigests: []string{"image_digest_2"},
+					RepoTags:    []string{"k8s.gcr.io/image2:v1.0.0"},
+					Size:        "1",
+				},
+			},
+		},
+		expected: []cruntime.ListImage{
+			{
+				ID:          "image_id_1",
+				RepoDigests: []string{"image_digest_1"},
+				RepoTags:    []string{"k8s.gcr.io/image1:v1.0.0", "registry.k8s.io/image1:v1.0.0"},
+				Size:        "1",
+			},
+			{
+				ID:          "image_id_2",
+				RepoDigests: []string{"image_digest_2"},
+				RepoTags:    []string{"k8s.gcr.io/image2:v1.0.0", "registry.k8s.io/image2:v1.0.0"},
+				Size:        "1",
+			},
+		},
+	}
+	testCase3 := CacheImageTestCase{
+		description: "normal case",
+		images: [][]cruntime.ListImage{
+			{
+				{
+					ID:          "image_id_1",
+					RepoDigests: []string{"image_digest_1"},
+					RepoTags:    []string{"k8s.gcr.io/image1:v1.0.0"},
+					Size:        "1",
+				},
+				{
+					ID:          "image_id_2",
+					RepoDigests: []string{"image_digest_2"},
+					RepoTags:    []string{"registry.k8s.io/image2:v1.0.0"},
+					Size:        "1",
+				},
+			},
+			{
+				{
+					ID:          "image_id_3",
+					RepoDigests: []string{"image_digest_3"},
+					RepoTags:    []string{"registry.k8s.io/image3:v1.0.0"},
+					Size:        "1",
+				},
+				{
+					ID:          "image_id_4",
+					RepoDigests: []string{"image_digest_4"},
+					RepoTags:    []string{"k8s.gcr.io/image4:v1.0.0"},
+					Size:        "1",
+				},
+			},
+		},
+		expected: []cruntime.ListImage{
+			{
+				ID:          "image_id_1",
+				RepoDigests: []string{"image_digest_1"},
+				RepoTags:    []string{"k8s.gcr.io/image1:v1.0.0"},
+				Size:        "1",
+			},
+			{
+				ID:          "image_id_2",
+				RepoDigests: []string{"image_digest_2"},
+				RepoTags:    []string{"registry.k8s.io/image2:v1.0.0"},
+				Size:        "1",
+			},
+			{
+				ID:          "image_id_3",
+				RepoDigests: []string{"image_digest_3"},
+				RepoTags:    []string{"registry.k8s.io/image3:v1.0.0"},
+				Size:        "1",
+			},
+			{
+				ID:          "image_id_4",
+				RepoDigests: []string{"image_digest_4"},
+				RepoTags:    []string{"k8s.gcr.io/image4:v1.0.0"},
+				Size:        "1",
+			},
+		},
+	}
+	testCases := []CacheImageTestCase{testCase0, testCase1, testCase2, testCase3}
+
+	for _, testCase := range testCases {
+		actualResult := mergeImageLists(testCase.images)
+		sort.Slice(actualResult, func(i, j int) bool {
+			return actualResult[i].ID < actualResult[j].ID
+		})
+		for _, img := range actualResult {
+			sort.Slice(img.RepoTags, func(i, j int) bool {
+				return img.RepoTags[i] < img.RepoTags[j]
+			})
+		}
+		if ok := reflect.DeepEqual(actualResult, testCase.expected); !ok {
+			t.Errorf("test case %s failed,\n actual %v,\n expected %v", testCase.description, actualResult, testCase.expected)
+		}
+	}
+}

pkg/minikube/machine/cache_images.go

@@ -686,7 +686,7 @@ func ListImages(profile *config.Profile, format string) error {
 		return errors.Wrapf(err, "error loading config for profile :%v", pName)
 	}
 
-	images := map[string]cruntime.ListImage{}
+	imageListsFromNodes := [][]cruntime.ListImage{}
 	for _, n := range c.Nodes {
 		m := config.MachineName(*c, n)
 
@@ -715,19 +715,12 @@ func ListImages(profile *config.Profile, format string) error {
 				klog.Warningf("Failed to list images for profile %s %v", pName, err.Error())
 				continue
 			}
+			imageListsFromNodes = append(imageListsFromNodes, list)
 
-			for _, img := range list {
-				if _, ok := images[img.ID]; !ok {
-					images[img.ID] = img
-				}
-			}
 		}
 	}
 
-	uniqueImages := []cruntime.ListImage{}
-	for _, img := range images {
-		uniqueImages = append(uniqueImages, img)
-	}
+	uniqueImages := mergeImageLists(imageListsFromNodes)
 
 	switch format {
 	case "table":
@@ -770,6 +763,42 @@ func ListImages(profile *config.Profile, format string) error {
 	return nil
 }
 
+// mergeImageLists merges image lists from different nodes into a single list
+// all the repo tags of the same image will be preserved and grouped in one image item
+func mergeImageLists(lists [][]cruntime.ListImage) []cruntime.ListImage {
+	images := map[string]*cruntime.ListImage{}
+	// key of this set is img.ID+img.repoTag
+	// we use this set to detect whether a tag of a image is included in images map
+	imageTagAndIDSet := map[string]struct{}{}
+	for _, list := range lists {
+		for _, img := range list {
+			img := img
+			if _, ok := images[img.ID]; !ok {
+				images[img.ID] = &img
+				for _, repoTag := range img.RepoTags {
+					imageTagAndIDSet[img.ID+repoTag] = struct{}{}
+				}
+				continue
+			}
+			for _, repoTag := range img.RepoTags {
+				if _, ok := imageTagAndIDSet[img.ID+repoTag]; ok {
+					continue
+				}
+				// if there is any repo tag which is not included in the map's corresponding item
+				// add it to the repo tag list
+				imageTagAndIDSet[img.ID+repoTag] = struct{}{}
+				images[img.ID].RepoTags = append(images[img.ID].RepoTags, repoTag)
+			}
+		}
+
+	}
+	uniqueImages := []cruntime.ListImage{}
+	for _, img := range images {
+		uniqueImages = append(uniqueImages, *img)
+	}
+	return uniqueImages
+}
+
 // parseRepoTag splits input string for two parts: image name and image tag
 func parseRepoTag(repoTag string) (string, string) {
 	idx := strings.LastIndex(repoTag, ":")