Merge pull request #59598 from Random-Liu/remove-unnecessary-summary-call

Automatic merge from submit-queue (batch tested with PRs 59344, 59595, 59598). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove unnecessary summary api call.

Summary API call is not as cheap as we think. Especially for CRI container runtime, it means:
1) Extra cgroups parsing (because cpu/memory are considered to be on demand);
2) Extra grpc encoding/decoding `ListPodSandboxes`, `ListContainers`, `ListContainerStats`;

I don't think it is necessary to call summary twice inside the same function.
/cc @kubernetes/sig-node-pr-reviews @dashpole @jingxu97 

Signed-off-by: Lantao Liu <lantaol@google.com>

**Release note**:

```release-note
none
```

pkg/kubelet/eviction/eviction_manager.go

@@ -232,12 +232,15 @@ func (m *managerImpl) synchronize(diskInfoProvider DiskInfoProvider, podFunc Act
 	}
 
 	activePods := podFunc()
-	// make observations and get a function to derive pod usage stats relative to those observations.
-	observations, statsFunc, err := makeSignalObservations(m.summaryProvider, capacityProvider, activePods)
+	updateStats := true
+	summary, err := m.summaryProvider.Get(updateStats)
 	if err != nil {
-		glog.Errorf("eviction manager: unexpected err: %v", err)
+		glog.Errorf("eviction manager: failed to get get summary stats: %v", err)
 		return nil
 	}
+
+	// make observations and get a function to derive pod usage stats relative to those observations.
+	observations, statsFunc := makeSignalObservations(summary, capacityProvider, activePods)
 	debugLogObservations("observations", observations)
 
 	// attempt to create a threshold notifier to improve eviction response time
@@ -314,7 +317,7 @@ func (m *managerImpl) synchronize(diskInfoProvider DiskInfoProvider, podFunc Act
 	// evict pods if there is a resource usage violation from local volume temporary storage
 	// If eviction happens in localStorageEviction function, skip the rest of eviction action
 	if utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) {
-		if evictedPods := m.localStorageEviction(activePods); len(evictedPods) > 0 {
+		if evictedPods := m.localStorageEviction(summary, activePods); len(evictedPods) > 0 {
 			return evictedPods
 		}
 	}
@@ -454,15 +457,7 @@ func (m *managerImpl) reclaimNodeLevelResources(resourceToReclaim v1.ResourceNam
 
 // localStorageEviction checks the EmptyDir volume usage for each pod and determine whether it exceeds the specified limit and needs
 // to be evicted. It also checks every container in the pod, if the container overlay usage exceeds the limit, the pod will be evicted too.
-func (m *managerImpl) localStorageEviction(pods []*v1.Pod) []*v1.Pod {
-	// do not update node-level stats as local storage evictions do not utilize them.
-	forceStatsUpdate := false
-	summary, err := m.summaryProvider.Get(forceStatsUpdate)
-	if err != nil {
-		glog.Errorf("Could not get summary provider")
-		return nil
-	}
-
+func (m *managerImpl) localStorageEviction(summary *statsapi.Summary, pods []*v1.Pod) []*v1.Pod {
 	statsFunc := cachedStatsFunc(summary.Pods)
 	evicted := []*v1.Pod{}
 	for _, pod := range pods {

pkg/kubelet/eviction/helpers.go

@@ -30,7 +30,6 @@ import (
 	"k8s.io/kubernetes/pkg/features"
 	statsapi "k8s.io/kubernetes/pkg/kubelet/apis/stats/v1alpha1"
 	evictionapi "k8s.io/kubernetes/pkg/kubelet/eviction/api"
-	"k8s.io/kubernetes/pkg/kubelet/server/stats"
 	kubetypes "k8s.io/kubernetes/pkg/kubelet/types"
 	schedulerutils "k8s.io/kubernetes/pkg/scheduler/util"
 )
@@ -715,12 +714,7 @@ func (a byEvictionPriority) Less(i, j int) bool {
 }
 
 // makeSignalObservations derives observations using the specified summary provider.
-func makeSignalObservations(summaryProvider stats.SummaryProvider, capacityProvider CapacityProvider, pods []*v1.Pod) (signalObservations, statsFunc, error) {
-	updateStats := true
-	summary, err := summaryProvider.Get(updateStats)
-	if err != nil {
-		return nil, nil, err
-	}
+func makeSignalObservations(summary *statsapi.Summary, capacityProvider CapacityProvider, pods []*v1.Pod) (signalObservations, statsFunc) {
 	// build the function to work against for pod stats
 	statsFunc := cachedStatsFunc(summary.Pods)
 	// build an evaluation context for current eviction signals
@@ -787,7 +781,7 @@ func makeSignalObservations(summaryProvider stats.SummaryProvider, capacityProvi
 		glog.Errorf("Could not find capacity information for resource %v", v1.ResourceMemory)
 	}
 
-	return result, statsFunc, nil
+	return result, statsFunc
 }
 
 // thresholdsMet returns the set of thresholds that were met independent of grace period

pkg/kubelet/eviction/helpers_test.go

@@ -993,9 +993,6 @@ func TestMakeSignalObservations(t *testing.T) {
 		},
 		Pods: []statsapi.PodStats{},
 	}
-	provider := &fakeSummaryProvider{
-		result: fakeStats,
-	}
 	pods := []*v1.Pod{
 		podMaker("pod1", "ns1", "uuid1", 1),
 		podMaker("pod1", "ns2", "uuid2", 1),
@@ -1011,10 +1008,7 @@ func TestMakeSignalObservations(t *testing.T) {
 	if res.CmpInt64(int64(allocatableMemoryCapacity)) != 0 {
 		t.Errorf("Expected Threshold %v to be equal to value %v", res.Value(), allocatableMemoryCapacity)
 	}
-	actualObservations, statsFunc, err := makeSignalObservations(provider, capacityProvider, pods)
-	if err != nil {
-		t.Errorf("Unexpected err: %v", err)
-	}
+	actualObservations, statsFunc := makeSignalObservations(fakeStats, capacityProvider, pods)
 	allocatableMemQuantity, found := actualObservations[evictionapi.SignalAllocatableMemoryAvailable]
 	if !found {
 		t.Errorf("Expected allocatable memory observation, but didnt find one")
@@ -1027,7 +1021,7 @@ func TestMakeSignalObservations(t *testing.T) {
 	}
 	memQuantity, found := actualObservations[evictionapi.SignalMemoryAvailable]
 	if !found {
-		t.Errorf("Expected available memory observation: %v", err)
+		t.Error("Expected available memory observation")
 	}
 	if expectedBytes := int64(nodeAvailableBytes); memQuantity.available.Value() != expectedBytes {
 		t.Errorf("Expected %v, actual: %v", expectedBytes, memQuantity.available.Value())
@@ -1037,7 +1031,7 @@ func TestMakeSignalObservations(t *testing.T) {
 	}
 	nodeFsQuantity, found := actualObservations[evictionapi.SignalNodeFsAvailable]
 	if !found {
-		t.Errorf("Expected available nodefs observation: %v", err)
+		t.Error("Expected available nodefs observation")
 	}
 	if expectedBytes := int64(nodeFsAvailableBytes); nodeFsQuantity.available.Value() != expectedBytes {
 		t.Errorf("Expected %v, actual: %v", expectedBytes, nodeFsQuantity.available.Value())
@@ -1047,7 +1041,7 @@ func TestMakeSignalObservations(t *testing.T) {
 	}
 	nodeFsInodesQuantity, found := actualObservations[evictionapi.SignalNodeFsInodesFree]
 	if !found {
-		t.Errorf("Expected inodes free nodefs observation: %v", err)
+		t.Error("Expected inodes free nodefs observation")
 	}
 	if expected := int64(nodeFsInodesFree); nodeFsInodesQuantity.available.Value() != expected {
 		t.Errorf("Expected %v, actual: %v", expected, nodeFsInodesQuantity.available.Value())
@@ -1057,7 +1051,7 @@ func TestMakeSignalObservations(t *testing.T) {
 	}
 	imageFsQuantity, found := actualObservations[evictionapi.SignalImageFsAvailable]
 	if !found {
-		t.Errorf("Expected available imagefs observation: %v", err)
+		t.Error("Expected available imagefs observation")
 	}
 	if expectedBytes := int64(imageFsAvailableBytes); imageFsQuantity.available.Value() != expectedBytes {
 		t.Errorf("Expected %v, actual: %v", expectedBytes, imageFsQuantity.available.Value())
@@ -1067,7 +1061,7 @@ func TestMakeSignalObservations(t *testing.T) {
 	}
 	imageFsInodesQuantity, found := actualObservations[evictionapi.SignalImageFsInodesFree]
 	if !found {
-		t.Errorf("Expected inodes free imagefs observation: %v", err)
+		t.Error("Expected inodes free imagefs observation")
 	}
 	if expected := int64(imageFsInodesFree); imageFsInodesQuantity.available.Value() != expected {
 		t.Errorf("Expected %v, actual: %v", expected, imageFsInodesQuantity.available.Value())