From ab0f274a6fa7ebc4986cc2b501edf66821c0738d Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Mon, 14 Feb 2022 17:55:55 -0500 Subject: [PATCH 1/3] kubelet/stats: update cadvisor stats provider with new log location in https://github.com/kubernetes/kubernetes/pull/74441, the namespace and name were added to the pod log location. However, cAdvisor stats provider wasn't correspondingly updated. since CRI-O uses cAdvisor stats provider by default, despite being a CRI implementation, eviction with ephemeral storage and container logs doesn't work as expected, until now! Signed-off-by: Peter Hunt --- pkg/kubelet/stats/cadvisor_stats_provider.go | 12 +++++++++++- pkg/kubelet/stats/helper.go | 3 +++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/kubelet/stats/cadvisor_stats_provider.go b/pkg/kubelet/stats/cadvisor_stats_provider.go index 6e4655fdcde..738f240c2d3 100644 --- a/pkg/kubelet/stats/cadvisor_stats_provider.go +++ b/pkg/kubelet/stats/cadvisor_stats_provider.go @@ -125,7 +125,17 @@ func (p *cadvisorStatsProvider) ListPodStats() ([]statsapi.PodStats, error) { // the user and has network stats. podStats.Network = cadvisorInfoToNetworkStats(&cinfo) } else { - podStats.Containers = append(podStats.Containers, *cadvisorInfoToContainerStats(containerName, &cinfo, &rootFsInfo, &imageFsInfo)) + containerStat := cadvisorInfoToContainerStats(containerName, &cinfo, &rootFsInfo, &imageFsInfo) + // NOTE: This doesn't support the old pod log path, `/var/log/pods/UID`. For containers + // using old log path, they will be populated by cadvisorInfoToContainerStats. + podUID := types.UID(podStats.PodRef.UID) + logs, err := p.hostStatsProvider.getPodContainerLogStats(podStats.PodRef.Namespace, podStats.PodRef.Name, podUID, containerName, &rootFsInfo) + if err != nil { + klog.ErrorS(err, "Unable to fetch container log stats", "containerName", containerName) + } else { + containerStat.Logs = logs + } + podStats.Containers = append(podStats.Containers, *containerStat) } } diff --git a/pkg/kubelet/stats/helper.go b/pkg/kubelet/stats/helper.go index 6af80ce3d97..da32fb11eed 100644 --- a/pkg/kubelet/stats/helper.go +++ b/pkg/kubelet/stats/helper.go @@ -94,6 +94,9 @@ func cadvisorInfoToContainerStats(name string, info *cadvisorapiv2.ContainerInfo result.CPU = cpu result.Memory = memory + // NOTE: if they can be found, log stats will be overwritten + // by the caller, as it knows more information about the pod, + // which is needed to determine log size. if rootFs != nil { // The container logs live on the node rootfs device result.Logs = buildLogsStats(cstat, rootFs) From 1c3357db76687ded50fbcbda3881576bfa958581 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 15 Feb 2022 16:28:17 -0500 Subject: [PATCH 2/3] kubelet/stats: take container log stats into account when checking ephemeral stats this commit updates checkEphemeralStorage to be able to add container log stats, if applicable. It also updates the old check when container log stats aren't found to be more accurate. Specifically, this check previously worked because of a fluke programming accident: according to this block in pkg/kubelet/stats/helper.go:113 ``` if result.Rootfs != nil { rootfsUsage := *cfs.BaseUsageBytes result.Rootfs.UsedBytes = &rootfsUsage } ``` BaseUsageBytes should be the value added, not TotalUsageBytes. However, since in this case one also needs to account for the calculated log size, which is TotalUsageBytes - BaseUsageBytes using TotalUsageBytes value accidentally worked. Updating the case to use the correct value AND log offset fixes this accident and makes the behavior more in line with what happens when calculating ephemeral storage. Signed-off-by: Peter Hunt --- .../stats/cadvisor_stats_provider_test.go | 2 +- pkg/kubelet/stats/provider_test.go | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/pkg/kubelet/stats/cadvisor_stats_provider_test.go b/pkg/kubelet/stats/cadvisor_stats_provider_test.go index 303d62526ba..96a3f81abfc 100644 --- a/pkg/kubelet/stats/cadvisor_stats_provider_test.go +++ b/pkg/kubelet/stats/cadvisor_stats_provider_test.go @@ -270,7 +270,7 @@ func TestCadvisorListPodStats(t *testing.T) { assert.EqualValues(t, p0Time.Unix(), ps.StartTime.Time.Unix()) checkNetworkStats(t, "Pod0", seedPod0Infra, ps.Network) - checkEphemeralStats(t, "Pod0", []int{seedPod0Container0, seedPod0Container1}, []int{seedEphemeralVolume1, seedEphemeralVolume2}, ps.EphemeralStorage) + checkEphemeralStats(t, "Pod0", []int{seedPod0Container0, seedPod0Container1}, []int{seedEphemeralVolume1, seedEphemeralVolume2}, nil, ps.EphemeralStorage) if ps.CPU != nil { checkCPUStats(t, "Pod0", seedPod0Infra, ps.CPU) } diff --git a/pkg/kubelet/stats/provider_test.go b/pkg/kubelet/stats/provider_test.go index 3bbc8334ba0..220d000e937 100644 --- a/pkg/kubelet/stats/provider_test.go +++ b/pkg/kubelet/stats/provider_test.go @@ -37,6 +37,7 @@ import ( kubepodtest "k8s.io/kubernetes/pkg/kubelet/pod/testing" serverstats "k8s.io/kubernetes/pkg/kubelet/server/stats" kubetypes "k8s.io/kubernetes/pkg/kubelet/types" + "k8s.io/kubernetes/pkg/volume" ) const ( @@ -699,16 +700,27 @@ func checkFsStats(t *testing.T, label string, seed int, stats *statsapi.FsStats) assert.EqualValues(t, seed+offsetFsInodesFree, *stats.InodesFree, label+".InodesFree") } -func checkEphemeralStats(t *testing.T, label string, containerSeeds []int, volumeSeeds []int, stats *statsapi.FsStats) { +func checkEphemeralStats(t *testing.T, label string, containerSeeds []int, volumeSeeds []int, containerLogStats []*volume.Metrics, stats *statsapi.FsStats) { var usedBytes, inodeUsage int for _, cseed := range containerSeeds { - usedBytes = usedBytes + cseed + offsetFsTotalUsageBytes + usedBytes += cseed + offsetFsBaseUsageBytes inodeUsage += cseed + offsetFsInodeUsage + // If containerLogStats is nil, then the log stats calculated from cAdvisor + // information is used. Since it's Total - Base, and these values are + // set to the offset, we can use the calculated difference in the offset + // to account for this. + if containerLogStats == nil { + usedBytes += offsetFsTotalUsageBytes - offsetFsBaseUsageBytes + } } for _, vseed := range volumeSeeds { - usedBytes = usedBytes + vseed + offsetFsUsage + usedBytes += vseed + offsetFsUsage inodeUsage += vseed + offsetFsInodeUsage } + for _, logStats := range containerLogStats { + usedBytes += int(logStats.Used.Value()) + inodeUsage += int(logStats.InodesUsed.Value()) + } assert.EqualValues(t, usedBytes, int(*stats.UsedBytes), label+".UsedBytes") assert.EqualValues(t, inodeUsage, int(*stats.InodesUsed), label+".InodesUsed") } From 0b7629d2cc293d804c0b7d6ea0df54c8cb4f2c9c Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 15 Feb 2022 16:29:12 -0500 Subject: [PATCH 3/3] kubelet/stats: add unit test for when container logs are found Signed-off-by: Peter Hunt --- .../stats/cadvisor_stats_provider_test.go | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/pkg/kubelet/stats/cadvisor_stats_provider_test.go b/pkg/kubelet/stats/cadvisor_stats_provider_test.go index 96a3f81abfc..9cc560d6a89 100644 --- a/pkg/kubelet/stats/cadvisor_stats_provider_test.go +++ b/pkg/kubelet/stats/cadvisor_stats_provider_test.go @@ -30,9 +30,12 @@ import ( cadvisortest "k8s.io/kubernetes/pkg/kubelet/cadvisor/testing" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" containertest "k8s.io/kubernetes/pkg/kubelet/container/testing" + kubecontainertest "k8s.io/kubernetes/pkg/kubelet/container/testing" + "k8s.io/kubernetes/pkg/kubelet/kuberuntime" "k8s.io/kubernetes/pkg/kubelet/leaky" serverstats "k8s.io/kubernetes/pkg/kubelet/server/stats" statustest "k8s.io/kubernetes/pkg/kubelet/status/testing" + "k8s.io/kubernetes/pkg/volume" ) func TestFilterTerminatedContainerInfoAndAssembleByPodCgroupKey(t *testing.T) { @@ -510,3 +513,106 @@ func TestCadvisorImagesFsStats(t *testing.T) { assert.Equal(imageFsInfo.Inodes, stats.Inodes) assert.Equal(*imageFsInfo.Inodes-*imageFsInfo.InodesFree, *stats.InodesUsed) } + +func TestCadvisorListPodStatsWhenContainerLogFound(t *testing.T) { + const ( + namespace0 = "test0" + ) + const ( + seedRoot = 0 + seedRuntime = 100 + seedKubelet = 200 + seedMisc = 300 + seedPod0Infra = 1000 + seedPod0Container0 = 0 + seedPod0Container1 = 0 + ) + const ( + pName0 = "pod0" + ) + const ( + cName00 = "c0" + cName01 = "c1" + ) + const ( + rootfsCapacity = uint64(10000000) + rootfsAvailable = uint64(5000000) + rootfsInodesFree = uint64(1000) + rootfsInodes = uint64(2000) + imagefsCapacity = uint64(20000000) + imagefsAvailable = uint64(8000000) + imagefsInodesFree = uint64(2000) + imagefsInodes = uint64(4000) + ) + + prf0 := statsapi.PodReference{Name: pName0, Namespace: namespace0, UID: "UID" + pName0} + infos := map[string]cadvisorapiv2.ContainerInfo{ + "/": getTestContainerInfo(seedRoot, "", "", ""), + "/docker-daemon": getTestContainerInfo(seedRuntime, "", "", ""), + "/kubelet": getTestContainerInfo(seedKubelet, "", "", ""), + "/system": getTestContainerInfo(seedMisc, "", "", ""), + // Pod0 - Namespace0 + "/pod0-i": getTestContainerInfo(seedPod0Infra, pName0, namespace0, leaky.PodInfraContainerName), + "/pod0-c0": getTestContainerInfo(seedPod0Container0, pName0, namespace0, cName00), + "/pod0-c1": getTestContainerInfo(seedPod0Container1, pName0, namespace0, cName01), + } + + containerLogStats0 := makeFakeLogStats(0) + containerLogStats1 := makeFakeLogStats(0) + fakeStats := map[string]*volume.Metrics{ + kuberuntime.BuildContainerLogsDirectory(prf0.Namespace, prf0.Name, types.UID(prf0.UID), cName00): containerLogStats0, + kuberuntime.BuildContainerLogsDirectory(prf0.Namespace, prf0.Name, types.UID(prf0.UID), cName01): containerLogStats1, + } + fakeStatsSlice := []*volume.Metrics{containerLogStats0, containerLogStats1} + fakeOS := &kubecontainertest.FakeOS{} + + freeRootfsInodes := rootfsInodesFree + totalRootfsInodes := rootfsInodes + rootfs := cadvisorapiv2.FsInfo{ + Capacity: rootfsCapacity, + Available: rootfsAvailable, + InodesFree: &freeRootfsInodes, + Inodes: &totalRootfsInodes, + } + + freeImagefsInodes := imagefsInodesFree + totalImagefsInodes := imagefsInodes + imagefs := cadvisorapiv2.FsInfo{ + Capacity: imagefsCapacity, + Available: imagefsAvailable, + InodesFree: &freeImagefsInodes, + Inodes: &totalImagefsInodes, + } + + options := cadvisorapiv2.RequestOptions{ + IdType: cadvisorapiv2.TypeName, + Count: 2, + Recursive: true, + } + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockCadvisor := cadvisortest.NewMockInterface(mockCtrl) + mockCadvisor.EXPECT().ContainerInfoV2("/", options).Return(infos, nil) + mockCadvisor.EXPECT().RootFsInfo().Return(rootfs, nil) + mockCadvisor.EXPECT().ImagesFsInfo().Return(imagefs, nil) + + mockRuntime := containertest.NewMockRuntime(mockCtrl) + mockRuntime.EXPECT().ImageStats().Return(&kubecontainer.ImageStats{TotalStorageBytes: 123}, nil).AnyTimes() + + volumeStats := serverstats.PodVolumeStats{} + p0Time := metav1.Now() + mockStatus := statustest.NewMockPodStatusProvider(mockCtrl) + mockStatus.EXPECT().GetPodStatus(types.UID("UID"+pName0)).Return(v1.PodStatus{StartTime: &p0Time}, true) + + resourceAnalyzer := &fakeResourceAnalyzer{podVolumeStats: volumeStats} + + p := NewCadvisorStatsProvider(mockCadvisor, resourceAnalyzer, nil, nil, mockRuntime, mockStatus, NewFakeHostStatsProviderWithData(fakeStats, fakeOS)) + pods, err := p.ListPodStats() + assert.NoError(t, err) + + assert.Equal(t, 1, len(pods)) + // Validate Pod0 Results + checkEphemeralStats(t, "Pod0", []int{seedPod0Container0, seedPod0Container1}, nil, fakeStatsSlice, pods[0].EphemeralStorage) +}