Merge pull request #104287 from jsturtevant/windows-stats

Reduce the number of expensive calls in the Windows stats queries for dockershim

pkg/kubelet/dockershim/docker_image_linux.go

@@ -25,20 +25,12 @@ import (
 	"path/filepath"
 	"time"
 
-	"k8s.io/klog/v2"
-
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 )
 
 // ImageFsInfo returns information of the filesystem that is used to store images.
 func (ds *dockerService) ImageFsInfo(_ context.Context, _ *runtimeapi.ImageFsInfoRequest) (*runtimeapi.ImageFsInfoResponse, error) {
-	info, err := ds.client.Info()
+	bytes, inodes, err := dirSize(filepath.Join(ds.dockerRootDir, "image"))
-	if err != nil {
-		klog.ErrorS(err, "Failed to get docker info")
-		return nil, err
-	}
-
-	bytes, inodes, err := dirSize(filepath.Join(info.DockerRootDir, "image"))
 	if err != nil {
 		return nil, err
 	}
@@ -48,7 +40,7 @@ func (ds *dockerService) ImageFsInfo(_ context.Context, _ *runtimeapi.ImageFsInf
 			{
 				Timestamp: time.Now().Unix(),
 				FsId: &runtimeapi.FilesystemIdentifier{
-					Mountpoint: info.DockerRootDir,
+					Mountpoint: ds.dockerRootDir,
 				},
 				UsedBytes: &runtimeapi.UInt64Value{
 					Value: uint64(bytes),

pkg/kubelet/dockershim/docker_image_windows.go

@@ -31,16 +31,10 @@ import (
 
 // ImageFsInfo returns information of the filesystem that is used to store images.
 func (ds *dockerService) ImageFsInfo(_ context.Context, _ *runtimeapi.ImageFsInfoRequest) (*runtimeapi.ImageFsInfoResponse, error) {
-	info, err := ds.client.Info()
-	if err != nil {
-		klog.ErrorS(err, "Failed to get docker info")
-		return nil, err
-	}
-
 	statsClient := &winstats.StatsClient{}
-	fsinfo, err := statsClient.GetDirFsInfo(info.DockerRootDir)
+	fsinfo, err := statsClient.GetDirFsInfo(ds.dockerRootDir)
 	if err != nil {
-		klog.ErrorS(err, "Failed to get fsInfo for dockerRootDir", "path", info.DockerRootDir)
+		klog.ErrorS(err, "Failed to get fsInfo for dockerRootDir", "path", ds.dockerRootDir)
 		return nil, err
 	}
 
@@ -49,7 +43,7 @@ func (ds *dockerService) ImageFsInfo(_ context.Context, _ *runtimeapi.ImageFsInf
 			Timestamp: time.Now().UnixNano(),
 			UsedBytes: &runtimeapi.UInt64Value{Value: fsinfo.Usage},
 			FsId: &runtimeapi.FilesystemIdentifier{
-				Mountpoint: info.DockerRootDir,
+				Mountpoint: ds.dockerRootDir,
 			},
 		},
 	}

pkg/kubelet/dockershim/docker_service.go

@@ -257,16 +257,17 @@ func NewDockerService(config *ClientConfig, podSandboxImage string, streamingCon
 	ds.network = network.NewPluginManager(plug)
 	klog.InfoS("Docker cri networking managed by the network plugin", "networkPluginName", plug.Name())
 
+	dockerInfo, err := ds.client.Info()
+	if err != nil {
+		return nil, fmt.Errorf("Failed to execute Info() call to the Docker client")
+	}
+	klog.InfoS("Docker Info", "dockerInfo", dockerInfo)
+	ds.dockerRootDir = dockerInfo.DockerRootDir
+
 	// skipping cgroup driver checks for Windows
 	if runtime.GOOS == "linux" {
-		// NOTE: cgroup driver is only detectable in docker 1.11+
 		cgroupDriver := defaultCgroupDriver
-		dockerInfo, err := ds.client.Info()
+		if len(dockerInfo.CgroupDriver) == 0 {
-		klog.InfoS("Docker Info", "dockerInfo", dockerInfo)
-		if err != nil {
-			klog.ErrorS(err, "Failed to execute Info() call to the Docker client")
-			klog.InfoS("Falling back to use the default driver", "cgroupDriver", cgroupDriver)
-		} else if len(dockerInfo.CgroupDriver) == 0 {
 			klog.InfoS("No cgroup driver is set in Docker")
 			klog.InfoS("Falling back to use the default driver", "cgroupDriver", cgroupDriver)
 		} else {
@@ -314,6 +315,9 @@ type dockerService struct {
 	// the docker daemon every time we need to do such checks.
 	versionCache *cache.ObjectCache
 
+	// docker root directory
+	dockerRootDir string
+
 	// containerCleanupInfos maps container IDs to the `containerCleanupInfo` structs
 	// needed to clean up after containers have been removed.
 	// (see `applyPlatformSpecificDockerConfig` and `performPlatformSpecificContainerCleanup`

pkg/kubelet/dockershim/docker_service_test.go

@@ -85,6 +85,7 @@ func newTestDockerService() (*dockerService, *libdocker.FakeDockerClient, *testi
 		network:           pm,
 		checkpointManager: ckm,
 		networkReady:      make(map[string]bool),
+		dockerRootDir:     "/docker/root/dir",
 	}, c, fakeClock
 }
 

pkg/kubelet/dockershim/docker_stats.go

@@ -22,6 +22,7 @@ package dockershim
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 )
@@ -29,8 +30,18 @@ import (
 var ErrNotImplemented = errors.New("Not implemented")
 
 // ContainerStats returns stats for a container stats request based on container id.
-func (ds *dockerService) ContainerStats(_ context.Context, r *runtimeapi.ContainerStatsRequest) (*runtimeapi.ContainerStatsResponse, error) {
+func (ds *dockerService) ContainerStats(ctx context.Context, r *runtimeapi.ContainerStatsRequest) (*runtimeapi.ContainerStatsResponse, error) {
-	stats, err := ds.getContainerStats(r.ContainerId)
+	filter := &runtimeapi.ContainerFilter{
+		Id: r.ContainerId,
+	}
+	listResp, err := ds.ListContainers(ctx, &runtimeapi.ListContainersRequest{Filter: filter})
+	if err != nil {
+		return nil, err
+	}
+	if len(listResp.Containers) != 1 {
+		return nil, fmt.Errorf("container with id %s not found", r.ContainerId)
+	}
+	stats, err := ds.getContainerStats(listResp.Containers[0])
 	if err != nil {
 		return nil, err
 	}
@@ -55,7 +66,7 @@ func (ds *dockerService) ListContainerStats(ctx context.Context, r *runtimeapi.L
 
 	var stats []*runtimeapi.ContainerStats
 	for _, container := range listResp.Containers {
-		containerStats, err := ds.getContainerStats(container.Id)
+		containerStats, err := ds.getContainerStats(container)
 		if err != nil {
 			return nil, err
 		}

pkg/kubelet/dockershim/docker_stats_linux.go

@@ -20,42 +20,30 @@ limitations under the License.
 package dockershim
 
 import (
-	"context"
 	"time"
 
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 )
 
-func (ds *dockerService) getContainerStats(containerID string) (*runtimeapi.ContainerStats, error) {
+func (ds *dockerService) getContainerStats(c *runtimeapi.Container) (*runtimeapi.ContainerStats, error) {
-	info, err := ds.client.Info()
+	statsJSON, err := ds.client.GetContainerStats(c.Id)
 	if err != nil {
 		return nil, err
 	}
 
-	statsJSON, err := ds.client.GetContainerStats(containerID)
+	containerJSON, err := ds.client.InspectContainerWithSize(c.Id)
 	if err != nil {
 		return nil, err
 	}
 
-	containerJSON, err := ds.client.InspectContainerWithSize(containerID)
-	if err != nil {
-		return nil, err
-	}
-
-	statusResp, err := ds.ContainerStatus(context.Background(), &runtimeapi.ContainerStatusRequest{ContainerId: containerID})
-	if err != nil {
-		return nil, err
-	}
-	status := statusResp.GetStatus()
-
 	dockerStats := statsJSON.Stats
 	timestamp := time.Now().UnixNano()
 	containerStats := &runtimeapi.ContainerStats{
 		Attributes: &runtimeapi.ContainerAttributes{
-			Id:          containerID,
+			Id:          c.Id,
-			Metadata:    status.Metadata,
+			Metadata:    c.Metadata,
-			Labels:      status.Labels,
+			Labels:      c.Labels,
-			Annotations: status.Annotations,
+			Annotations: c.Annotations,
 		},
 		Cpu: &runtimeapi.CpuUsage{
 			Timestamp:            timestamp,
@@ -67,7 +55,7 @@ func (ds *dockerService) getContainerStats(containerID string) (*runtimeapi.Cont
 		},
 		WritableLayer: &runtimeapi.FilesystemUsage{
 			Timestamp: timestamp,
-			FsId:      &runtimeapi.FilesystemIdentifier{Mountpoint: info.DockerRootDir},
+			FsId:      &runtimeapi.FilesystemIdentifier{Mountpoint: ds.dockerRootDir},
 			UsedBytes: &runtimeapi.UInt64Value{Value: uint64(*containerJSON.SizeRw)},
 		},
 	}

pkg/kubelet/dockershim/docker_stats_test.go

@@ -23,12 +23,14 @@ import (
 	"testing"
 
 	dockertypes "github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/container"
 	"github.com/stretchr/testify/assert"
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 	"k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker"
 )
 
 func TestContainerStats(t *testing.T) {
+	labels := map[string]string{containerTypeLabelKey: containerTypeLabelContainer}
 	tests := map[string]struct {
 		containerID    string
 		container      *libdocker.FakeContainer
@@ -36,22 +38,33 @@ func TestContainerStats(t *testing.T) {
 		calledDetails  []libdocker.CalledDetail
 	}{
 		"container exists": {
-			"fake_container",
+			"k8s_fake_container",
-			&libdocker.FakeContainer{ID: "fake_container"},
+			&libdocker.FakeContainer{
+				ID:   "k8s_fake_container",
+				Name: "k8s_fake_container_1_2_1",
+				Config: &container.Config{
+					Labels: labels,
+				},
+			},
 			&dockertypes.StatsJSON{},
 			[]libdocker.CalledDetail{
+				libdocker.NewCalledDetail("list", nil),
 				libdocker.NewCalledDetail("get_container_stats", nil),
 				libdocker.NewCalledDetail("inspect_container_withsize", nil),
-				libdocker.NewCalledDetail("inspect_container", nil),
-				libdocker.NewCalledDetail("inspect_image", nil),
 			},
 		},
 		"container doesn't exists": {
-			"nonexistant_fake_container",
+			"k8s_nonexistant_fake_container",
-			&libdocker.FakeContainer{ID: "fake_container"},
+			&libdocker.FakeContainer{
+				ID:   "k8s_fake_container",
+				Name: "k8s_fake_container_1_2_1",
+				Config: &container.Config{
+					Labels: labels,
+				},
+			},
 			&dockertypes.StatsJSON{},
 			[]libdocker.CalledDetail{
-				libdocker.NewCalledDetail("get_container_stats", nil),
+				libdocker.NewCalledDetail("list", nil),
 			},
 		},
 	}

pkg/kubelet/dockershim/docker_stats_unsupported.go

@@ -25,6 +25,6 @@ import (
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 )
 
-func (ds *dockerService) getContainerStats(containerID string) (*runtimeapi.ContainerStats, error) {
+func (ds *dockerService) getContainerStats(c *runtimeapi.Container) (*runtimeapi.ContainerStats, error) {
 	return nil, fmt.Errorf("not implemented")
 }

pkg/kubelet/dockershim/docker_stats_windows.go

@@ -20,7 +20,6 @@ limitations under the License.
 package dockershim
 
 import (
-	"context"
 	"strings"
 	"time"
 
@@ -29,26 +28,21 @@ import (
 	"k8s.io/klog/v2"
 )
 
-func (ds *dockerService) getContainerStats(containerID string) (*runtimeapi.ContainerStats, error) {
+func (ds *dockerService) getContainerStats(c *runtimeapi.Container) (*runtimeapi.ContainerStats, error) {
-	info, err := ds.client.Info()
+	hcsshimContainer, err := hcsshim.OpenContainer(c.Id)
-	if err != nil {
-		return nil, err
-	}
-
-	hcsshimContainer, err := hcsshim.OpenContainer(containerID)
 	if err != nil {
 		// As we moved from using Docker stats to hcsshim directly, we may query HCS with already exited container IDs.
 		// That will typically happen with init-containers in Exited state. Docker still knows about them but the HCS does not.
 		// As we don't want to block stats retrieval for other containers, we only log errors.
 		if !hcsshim.IsNotExist(err) && !hcsshim.IsAlreadyStopped(err) {
-			klog.V(4).InfoS("Error opening container (stats will be missing)", "containerID", containerID, "err", err)
+			klog.V(4).InfoS("Error opening container (stats will be missing)", "containerID", c.Id, "err", err)
 		}
 		return nil, nil
 	}
 	defer func() {
 		closeErr := hcsshimContainer.Close()
 		if closeErr != nil {
-			klog.ErrorS(closeErr, "Error closing container", "containerID", containerID)
+			klog.ErrorS(closeErr, "Error closing container", "containerID", c.Id)
 		}
 	}()
 
@@ -61,30 +55,19 @@ func (ds *dockerService) getContainerStats(containerID string) (*runtimeapi.Cont
 			// These hcs errors do not have helpers exposed in public package so need to query for the known codes
 			// https://github.com/microsoft/hcsshim/blob/master/internal/hcs/errors.go
 			// PR to expose helpers in hcsshim: https://github.com/microsoft/hcsshim/pull/933
-			klog.V(4).InfoS("Container is not in a state that stats can be accessed. This occurs when the container is created but not started.", "containerID", containerID, "err", err)
+			klog.V(4).InfoS("Container is not in a state that stats can be accessed. This occurs when the container is created but not started.", "containerID", c.Id, "err", err)
 			return nil, nil
 		}
 		return nil, err
 	}
 
-	containerJSON, err := ds.client.InspectContainerWithSize(containerID)
-	if err != nil {
-		return nil, err
-	}
-
-	statusResp, err := ds.ContainerStatus(context.Background(), &runtimeapi.ContainerStatusRequest{ContainerId: containerID})
-	if err != nil {
-		return nil, err
-	}
-	status := statusResp.GetStatus()
-
 	timestamp := time.Now().UnixNano()
 	containerStats := &runtimeapi.ContainerStats{
 		Attributes: &runtimeapi.ContainerAttributes{
-			Id:          containerID,
+			Id:          c.Id,
-			Metadata:    status.Metadata,
+			Metadata:    c.Metadata,
-			Labels:      status.Labels,
+			Labels:      c.Labels,
-			Annotations: status.Annotations,
+			Annotations: c.Annotations,
 		},
 		Cpu: &runtimeapi.CpuUsage{
 			Timestamp: timestamp,
@@ -97,8 +80,11 @@ func (ds *dockerService) getContainerStats(containerID string) (*runtimeapi.Cont
 		},
 		WritableLayer: &runtimeapi.FilesystemUsage{
 			Timestamp: timestamp,
-			FsId:      &runtimeapi.FilesystemIdentifier{Mountpoint: info.DockerRootDir},
+			FsId:      &runtimeapi.FilesystemIdentifier{Mountpoint: ds.dockerRootDir},
-			UsedBytes: &runtimeapi.UInt64Value{Value: uint64(*containerJSON.SizeRw)},
+			// used bytes from image are not implemented on Windows
+			// don't query for it since it is expensive to call docker over named pipe
+			// https://github.com/moby/moby/blob/1ba54a5fd0ba293db3bea46cd67604b593f2048b/daemon/images/image_windows.go#L11-L14
+			UsedBytes: &runtimeapi.UInt64Value{Value: 0},
 		},
 	}
 	return containerStats, nil

pkg/kubelet/dockershim/libdocker/fake_client.go

@@ -35,7 +35,7 @@ import (
 	dockercontainer "github.com/docker/docker/api/types/container"
 	dockerimagetypes "github.com/docker/docker/api/types/image"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/utils/clock"
 )
 
@@ -226,6 +226,7 @@ func convertFakeContainer(f *FakeContainer) *dockertypes.ContainerJSON {
 	if f.HostConfig == nil {
 		f.HostConfig = &dockercontainer.HostConfig{}
 	}
+	fakeRWSize := int64(40)
 	return &dockertypes.ContainerJSON{
 		ContainerJSONBase: &dockertypes.ContainerJSONBase{
 			ID:    f.ID,
@@ -240,6 +241,7 @@ func convertFakeContainer(f *FakeContainer) *dockertypes.ContainerJSON {
 			},
 			Created:    dockerTimestampToString(f.CreatedAt),
 			HostConfig: f.HostConfig,
+			SizeRw:     &fakeRWSize,
 		},
 		Config:          f.Config,
 		NetworkSettings: &dockertypes.NetworkSettings{},