port setNodeStatusImages to Setter abstraction, add test

pkg/kubelet/BUILD

@@ -178,6 +178,7 @@ go_test(
         "//pkg/kubelet/lifecycle:go_default_library",
         "//pkg/kubelet/logs:go_default_library",
         "//pkg/kubelet/network/dns:go_default_library",
+        "//pkg/kubelet/nodestatus:go_default_library",
         "//pkg/kubelet/pleg:go_default_library",
         "//pkg/kubelet/pod:go_default_library",
         "//pkg/kubelet/pod/testing:go_default_library",

pkg/kubelet/kubelet_node_status.go

@@ -43,12 +43,6 @@ import (
 	volutil "k8s.io/kubernetes/pkg/volume/util"
 )
 
-const (
-	// maxNamesPerImageInNodeStatus is max number of names per image stored in
-	// the node status.
-	maxNamesPerImageInNodeStatus = 5
-)
-
 // registerWithAPIServer registers the node with the cluster master. It is safe
 // to call multiple times, but not concurrently (kl.registrationCompleted is
 // not locked).
@@ -444,37 +438,6 @@ func (kl *Kubelet) recordEvent(eventType, event, message string) {
 	kl.recorder.Eventf(kl.nodeRef, eventType, event, message)
 }
 
-// Set images list for the node
-func (kl *Kubelet) setNodeStatusImages(node *v1.Node) {
-	// Update image list of this node
-	var imagesOnNode []v1.ContainerImage
-	containerImages, err := kl.imageManager.GetImageList()
-	if err != nil {
-		glog.Errorf("Error getting image list: %v", err)
-		node.Status.Images = imagesOnNode
-		return
-	}
-	// sort the images from max to min, and only set top N images into the node status.
-	if int(kl.nodeStatusMaxImages) > -1 &&
-		int(kl.nodeStatusMaxImages) < len(containerImages) {
-		containerImages = containerImages[0:kl.nodeStatusMaxImages]
-	}
-
-	for _, image := range containerImages {
-		names := append(image.RepoDigests, image.RepoTags...)
-		// Report up to maxNamesPerImageInNodeStatus names per image.
-		if len(names) > maxNamesPerImageInNodeStatus {
-			names = names[0:maxNamesPerImageInNodeStatus]
-		}
-		imagesOnNode = append(imagesOnNode, v1.ContainerImage{
-			Names:     names,
-			SizeBytes: image.Size,
-		})
-	}
-
-	node.Status.Images = imagesOnNode
-}
-
 // Set the GOOS and GOARCH for this node
 func (kl *Kubelet) setNodeStatusGoRuntime(node *v1.Node) {
 	node.Status.NodeInfo.OperatingSystem = goruntime.GOOS
@@ -545,7 +508,7 @@ func (kl *Kubelet) defaultNodeStatusFuncs() []func(*v1.Node) error {
 			kl.containerManager.GetDevicePluginResourceCapacity, kl.containerManager.GetNodeAllocatableReservation, kl.recordEvent),
 		nodestatus.VersionInfo(kl.cadvisor.VersionInfo, kl.containerRuntime.Type, kl.containerRuntime.Version),
 		nodestatus.DaemonEndpoints(kl.daemonEndpoints),
-		withoutError(kl.setNodeStatusImages),
+		nodestatus.Images(kl.nodeStatusMaxImages, kl.imageManager.GetImageList),
 		withoutError(kl.setNodeStatusGoRuntime),
 	)
 	if utilfeature.DefaultFeatureGate.Enabled(features.AttachVolumeLimit) {

pkg/kubelet/kubelet_node_status_test.go

@@ -51,6 +51,7 @@ import (
 	cadvisortest "k8s.io/kubernetes/pkg/kubelet/cadvisor/testing"
 	"k8s.io/kubernetes/pkg/kubelet/cm"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
+	"k8s.io/kubernetes/pkg/kubelet/nodestatus"
 	"k8s.io/kubernetes/pkg/kubelet/util/sliceutils"
 	"k8s.io/kubernetes/pkg/version"
 	"k8s.io/kubernetes/pkg/volume/util"
@@ -85,7 +86,7 @@ func makeExpectedImageList(imageList []kubecontainer.Image, maxImages int) []v1.
 	var expectedImageList []v1.ContainerImage
 	for _, kubeImage := range imageList {
 		apiImage := v1.ContainerImage{
-			Names:     kubeImage.RepoTags[0:maxNamesPerImageInNodeStatus],
+			Names:     kubeImage.RepoTags[0:nodestatus.MaxNamesPerImageInNodeStatus],
 			SizeBytes: kubeImage.Size,
 		}
 
@@ -100,9 +101,9 @@ func makeExpectedImageList(imageList []kubecontainer.Image, maxImages int) []v1.
 
 func generateImageTags() []string {
 	var tagList []string
-	// Generate > maxNamesPerImageInNodeStatus tags so that the test can verify
+	// Generate > MaxNamesPerImageInNodeStatus tags so that the test can verify
-	// that kubelet report up to maxNamesPerImageInNodeStatus tags.
+	// that kubelet report up to MaxNamesPerImageInNodeStatus tags.
-	count := rand.IntnRange(maxNamesPerImageInNodeStatus+1, maxImageTagsForTest+1)
+	count := rand.IntnRange(nodestatus.MaxNamesPerImageInNodeStatus+1, maxImageTagsForTest+1)
 	for ; count > 0; count-- {
 		tagList = append(tagList, "k8s.gcr.io:v"+strconv.Itoa(count))
 	}

pkg/kubelet/nodestatus/BUILD

@@ -49,12 +49,15 @@ go_test(
         "//pkg/kubelet/container:go_default_library",
         "//pkg/kubelet/container/testing:go_default_library",
         "//pkg/kubelet/events:go_default_library",
+        "//pkg/kubelet/util/sliceutils:go_default_library",
         "//pkg/version:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/rand:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library",
         "//vendor/github.com/google/cadvisor/info/v1:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
         "//vendor/github.com/stretchr/testify/require:go_default_library",

pkg/kubelet/nodestatus/setters.go

@@ -43,6 +43,12 @@ import (
 	"github.com/golang/glog"
 )
 
+const (
+	// MaxNamesPerImageInNodeStatus is max number of names
+	// per image stored in the node status.
+	MaxNamesPerImageInNodeStatus = 5
+)
+
 // Setter modifies the node in-place, and returns an error if the modification failed.
 // Setters may partially mutate the node before returning an error.
 type Setter func(node *v1.Node) error
@@ -331,6 +337,45 @@ func DaemonEndpoints(daemonEndpoints *v1.NodeDaemonEndpoints) Setter {
 	}
 }
 
+// Images returns a Setter that updates the images on the node.
+// imageListFunc is expected to return a list of images sorted in descending order by image size.
+// nodeStatusMaxImages is ignored if set to -1.
+func Images(nodeStatusMaxImages int32,
+	imageListFunc func() ([]kubecontainer.Image, error), // typically Kubelet.imageManager.GetImageList
+) Setter {
+	return func(node *v1.Node) error {
+		// Update image list of this node
+		var imagesOnNode []v1.ContainerImage
+		containerImages, err := imageListFunc()
+		if err != nil {
+			// TODO(mtaufen): consider removing this log line, since returned error will be logged
+			glog.Errorf("Error getting image list: %v", err)
+			node.Status.Images = imagesOnNode
+			return fmt.Errorf("error getting image list: %v", err)
+		}
+		// we expect imageListFunc to return a sorted list, so we just need to truncate
+		if int(nodeStatusMaxImages) > -1 &&
+			int(nodeStatusMaxImages) < len(containerImages) {
+			containerImages = containerImages[0:nodeStatusMaxImages]
+		}
+
+		for _, image := range containerImages {
+			names := append(image.RepoDigests, image.RepoTags...)
+			// Report up to MaxNamesPerImageInNodeStatus names per image.
+			if len(names) > MaxNamesPerImageInNodeStatus {
+				names = names[0:MaxNamesPerImageInNodeStatus]
+			}
+			imagesOnNode = append(imagesOnNode, v1.ContainerImage{
+				Names:     names,
+				SizeBytes: image.Size,
+			})
+		}
+
+		node.Status.Images = imagesOnNode
+		return nil
+	}
+}
+
 // ReadyCondition returns a Setter that updates the v1.NodeReady condition on the node.
 func ReadyCondition(
 	nowFunc func() time.Time, // typically Kubelet.clock.Now

pkg/kubelet/nodestatus/setters_test.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"net"
 	"sort"
+	"strconv"
 	"testing"
 	"time"
 
@@ -30,11 +31,14 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
+	"k8s.io/apimachinery/pkg/util/rand"
+	"k8s.io/apimachinery/pkg/util/uuid"
 	fakecloud "k8s.io/kubernetes/pkg/cloudprovider/providers/fake"
 	"k8s.io/kubernetes/pkg/kubelet/cm"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
 	kubecontainertest "k8s.io/kubernetes/pkg/kubelet/container/testing"
 	"k8s.io/kubernetes/pkg/kubelet/events"
+	"k8s.io/kubernetes/pkg/kubelet/util/sliceutils"
 	"k8s.io/kubernetes/pkg/version"
 
 	"github.com/stretchr/testify/assert"
@@ -704,6 +708,86 @@ func TestVersionInfo(t *testing.T) {
 	}
 }
 
+func TestImages(t *testing.T) {
+	const (
+		minImageSize = 23 * 1024 * 1024
+		maxImageSize = 1000 * 1024 * 1024
+	)
+
+	cases := []struct {
+		desc           string
+		maxImages      int32
+		imageList      []kubecontainer.Image
+		imageListError error
+		expectError    error
+	}{
+		{
+			desc:      "max images enforced",
+			maxImages: 1,
+			imageList: makeImageList(2, 1, minImageSize, maxImageSize),
+		},
+		{
+			desc:      "no max images cap for -1",
+			maxImages: -1,
+			imageList: makeImageList(2, 1, minImageSize, maxImageSize),
+		},
+		{
+			desc:      "max names per image enforced",
+			maxImages: -1,
+			imageList: makeImageList(1, MaxNamesPerImageInNodeStatus+1, minImageSize, maxImageSize),
+		},
+		{
+			desc:      "images are sorted by size, descending",
+			maxImages: -1,
+			// makeExpectedImageList will sort them for expectedNode when the test case is run
+			imageList: []kubecontainer.Image{{Size: 3}, {Size: 1}, {Size: 4}, {Size: 2}},
+		},
+		{
+			desc:      "repo digests and tags both show up in image names",
+			maxImages: -1,
+			// makeExpectedImageList will use both digests and tags
+			imageList: []kubecontainer.Image{
+				{
+					RepoDigests: []string{"foo", "bar"},
+					RepoTags:    []string{"baz", "quux"},
+				},
+			},
+		},
+		{
+			desc:           "error getting image list, image list on node is reset to empty",
+			maxImages:      -1,
+			imageListError: fmt.Errorf("foo"),
+			expectError:    fmt.Errorf("error getting image list: foo"),
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.desc, func(t *testing.T) {
+			imageListFunc := func() ([]kubecontainer.Image, error) {
+				// today, imageListFunc is expected to return a sorted list,
+				// but we may choose to sort in the setter at some future point
+				// (e.g. if the image cache stopped sorting for us)
+				sort.Sort(sliceutils.ByImageSize(tc.imageList))
+				return tc.imageList, tc.imageListError
+			}
+			// construct setter
+			setter := Images(tc.maxImages, imageListFunc)
+			// call setter on node
+			node := &v1.Node{}
+			err := setter(node)
+			require.Equal(t, tc.expectError, err)
+			// check expected node, image list should be reset to empty when there is an error
+			expectNode := &v1.Node{}
+			if err == nil {
+				expectNode.Status.Images = makeExpectedImageList(tc.imageList, tc.maxImages, MaxNamesPerImageInNodeStatus)
+			}
+			assert.True(t, apiequality.Semantic.DeepEqual(expectNode, node),
+				"Diff: %s", diff.ObjectDiff(expectNode, node))
+		})
+	}
+
+}
+
 func TestReadyCondition(t *testing.T) {
 	now := time.Now()
 	before := now.Add(-time.Second)
@@ -1309,6 +1393,52 @@ type testEvent struct {
 	message   string
 }
 
+// makeImageList randomly generates a list of images with the given count
+func makeImageList(numImages, numTags, minSize, maxSize int32) []kubecontainer.Image {
+	images := make([]kubecontainer.Image, numImages)
+	for i := range images {
+		image := &images[i]
+		image.ID = string(uuid.NewUUID())
+		image.RepoTags = makeImageTags(numTags)
+		image.Size = rand.Int63nRange(int64(minSize), int64(maxSize+1))
+	}
+	return images
+}
+
+func makeExpectedImageList(imageList []kubecontainer.Image, maxImages, maxNames int32) []v1.ContainerImage {
+	// copy the imageList, we do not want to mutate it in-place and accidentally edit a test case
+	images := make([]kubecontainer.Image, len(imageList))
+	copy(images, imageList)
+	// sort images by size
+	sort.Sort(sliceutils.ByImageSize(images))
+	// convert to []v1.ContainerImage and truncate the list of names
+	expectedImages := make([]v1.ContainerImage, len(images))
+	for i := range images {
+		image := &images[i]
+		expectedImage := &expectedImages[i]
+		names := append(image.RepoDigests, image.RepoTags...)
+		if len(names) > int(maxNames) {
+			names = names[0:maxNames]
+		}
+		expectedImage.Names = names
+		expectedImage.SizeBytes = image.Size
+	}
+	// -1 means no limit, truncate result list if necessary.
+	if maxImages > -1 &&
+		int(maxImages) < len(expectedImages) {
+		return expectedImages[0:maxImages]
+	}
+	return expectedImages
+}
+
+func makeImageTags(num int32) []string {
+	tags := make([]string, num)
+	for i := range tags {
+		tags[i] = "k8s.gcr.io:v" + strconv.Itoa(i)
+	}
+	return tags
+}
+
 func makeReadyCondition(ready bool, message string, transition, heartbeat time.Time) *v1.NodeCondition {
 	if ready {
 		return &v1.NodeCondition{