From 8e217f7102262c7e70d4f500842a960e3c5dac53 Mon Sep 17 00:00:00 2001 From: Michael Taufen Date: Thu, 28 Jun 2018 17:11:18 -0700 Subject: [PATCH] port setNodeStatusImages to Setter abstraction, add test --- pkg/kubelet/BUILD | 1 + pkg/kubelet/kubelet_node_status.go | 39 +------ pkg/kubelet/kubelet_node_status_test.go | 9 +- pkg/kubelet/nodestatus/BUILD | 3 + pkg/kubelet/nodestatus/setters.go | 45 ++++++++ pkg/kubelet/nodestatus/setters_test.go | 130 ++++++++++++++++++++++++ 6 files changed, 185 insertions(+), 42 deletions(-) diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index c6f168426a9..0c4e7a8189f 100644 --- a/pkg/kubelet/BUILD +++ b/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", diff --git a/pkg/kubelet/kubelet_node_status.go b/pkg/kubelet/kubelet_node_status.go index 553e28ba7f4..5a83e91f9ea 100644 --- a/pkg/kubelet/kubelet_node_status.go +++ b/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) { diff --git a/pkg/kubelet/kubelet_node_status_test.go b/pkg/kubelet/kubelet_node_status_test.go index 622f21b4e40..6085f043d40 100644 --- a/pkg/kubelet/kubelet_node_status_test.go +++ b/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 - // that kubelet report up to maxNamesPerImageInNodeStatus tags. - count := rand.IntnRange(maxNamesPerImageInNodeStatus+1, maxImageTagsForTest+1) + // Generate > MaxNamesPerImageInNodeStatus tags so that the test can verify + // that kubelet report up to MaxNamesPerImageInNodeStatus tags. + count := rand.IntnRange(nodestatus.MaxNamesPerImageInNodeStatus+1, maxImageTagsForTest+1) for ; count > 0; count-- { tagList = append(tagList, "k8s.gcr.io:v"+strconv.Itoa(count)) } diff --git a/pkg/kubelet/nodestatus/BUILD b/pkg/kubelet/nodestatus/BUILD index 00b50925cba..84930d6c2b2 100644 --- a/pkg/kubelet/nodestatus/BUILD +++ b/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", diff --git a/pkg/kubelet/nodestatus/setters.go b/pkg/kubelet/nodestatus/setters.go index 1e0a21200b6..69ff9bbfdee 100644 --- a/pkg/kubelet/nodestatus/setters.go +++ b/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 diff --git a/pkg/kubelet/nodestatus/setters_test.go b/pkg/kubelet/nodestatus/setters_test.go index 8042f89adae..c07330cfb7c 100644 --- a/pkg/kubelet/nodestatus/setters_test.go +++ b/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{