Merge pull request #16578 from ComradeProgrammer/image_tags

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

pkg/minikube/machine/cache_image_test.go

@@ -0,0 +1,259 @@
+/*
+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) {
+	testCases := []CacheImageTestCase{
+		// 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
+		{
+			description: "same images with multiple tags appear on one 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 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
+		{
+			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
+		{
+			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",
+				},
+			},
+		},
+
+		{
+			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",
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		got := mergeImageLists(tc.images)
+		sort.Slice(got, func(i, j int) bool {
+			return got[i].ID < got[j].ID
+		})
+		for _, img := range got {
+			sort.Slice(img.RepoTags, func(i, j int) bool {
+				return img.RepoTags[i] < img.RepoTags[j]
+			})
+		}
+		if ok := reflect.DeepEqual(got, tc.expected); !ok {
+			t.Errorf("%s:\nmergeImageLists() = %+v;\nwant %+v", tc.description, got, tc.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,39 @@ 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
+			_, existingImg := images[img.ID]
+			if !existingImg {
+				images[img.ID] = &img
+			}
+			for _, repoTag := range img.RepoTags {
+				if _, ok := imageTagAndIDSet[img.ID+repoTag]; ok {
+					continue
+				}
+				imageTagAndIDSet[img.ID+repoTag] = struct{}{}
+				if existingImg {
+					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, ":")