Merge pull request #63603 from resouer/clean-cache

Automatic merge from submit-queue (batch tested with PRs 63603, 63557, 62015). 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>.

Clean up equiv cache with a simple implementation instead of LRU

**What this PR does / why we need it**:

The original version of equiv cache use pod hash as cache key, also, the predicate order is not fixed. So I used a LRU cache to improve hit rate.

While now we've already refactored it to use predicates as keys, and its order was also fixed in scheduler, we can use a simplest cache instead now.

**Special notes for your reviewer**:

The question is brought up by @misterikkit

**Release note**:

```release-note
NONE
```

pkg/scheduler/core/BUILD

@@ -57,7 +57,6 @@ go_library(
         "//pkg/scheduler/volumebinder:go_default_library",
         "//pkg/util/hash:go_default_library",
         "//vendor/github.com/golang/glog:go_default_library",
-        "//vendor/github.com/golang/groupcache/lru:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/api/policy/v1beta1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",

pkg/scheduler/core/equivalence_cache.go

@@ -28,12 +28,8 @@ import (
 	hashutil "k8s.io/kubernetes/pkg/util/hash"
 
 	"github.com/golang/glog"
-	"github.com/golang/groupcache/lru"
 )
 
-// We use predicate names as cache's key, its count is limited
-const maxCacheEntries = 100
-
 // EquivalenceCache holds:
 // 1. a map of AlgorithmCache with node name as key
 // 2. function to get equivalence pod
@@ -42,11 +38,8 @@ type EquivalenceCache struct {
 	algorithmCache map[string]AlgorithmCache
 }
 
-// The AlgorithmCache stores PredicateMap with predicate name as key
-type AlgorithmCache struct {
-	// Only consider predicates for now
-	predicatesCache *lru.Cache
-}
+// The AlgorithmCache stores PredicateMap with predicate name as key, PredicateMap as value.
+type AlgorithmCache map[string]PredicateMap
 
 // PredicateMap stores HostPrediacte with equivalence hash as key
 type PredicateMap map[uint64]HostPredicate
@@ -57,12 +50,6 @@ type HostPredicate struct {
 	FailReasons []algorithm.PredicateFailureReason
 }
 
-func newAlgorithmCache() AlgorithmCache {
-	return AlgorithmCache{
-		predicatesCache: lru.New(maxCacheEntries),
-	}
-}
-
 // NewEquivalenceCache returns EquivalenceCache to speed up predicates by caching
 // result from previous scheduling.
 func NewEquivalenceCache() *EquivalenceCache {
@@ -109,22 +96,21 @@ func (ec *EquivalenceCache) updateResult(
 	equivalenceHash uint64,
 ) {
 	if _, exist := ec.algorithmCache[nodeName]; !exist {
-		ec.algorithmCache[nodeName] = newAlgorithmCache()
+		ec.algorithmCache[nodeName] = AlgorithmCache{}
 	}
 	predicateItem := HostPredicate{
 		Fit:         fit,
 		FailReasons: reasons,
 	}
 	// if cached predicate map already exists, just update the predicate by key
-	if v, ok := ec.algorithmCache[nodeName].predicatesCache.Get(predicateKey); ok {
-		predicateMap := v.(PredicateMap)
+	if predicateMap, ok := ec.algorithmCache[nodeName][predicateKey]; ok {
 		// maps in golang are references, no need to add them back
 		predicateMap[equivalenceHash] = predicateItem
 	} else {
-		ec.algorithmCache[nodeName].predicatesCache.Add(predicateKey,
+		ec.algorithmCache[nodeName][predicateKey] =
 			PredicateMap{
 				equivalenceHash: predicateItem,
-			})
+			}
 	}
 	glog.V(5).Infof("Updated cached predicate: %v for pod: %v on node: %s, with item %v", predicateKey, podName, nodeName, predicateItem)
 }
@@ -139,20 +125,13 @@ func (ec *EquivalenceCache) lookupResult(
 ) (bool, []algorithm.PredicateFailureReason, bool) {
 	glog.V(5).Infof("Begin to calculate predicate: %v for pod: %s on node: %s based on equivalence cache",
 		predicateKey, podName, nodeName)
-	if algorithmCache, exist := ec.algorithmCache[nodeName]; exist {
-		if cachePredicate, exist := algorithmCache.predicatesCache.Get(predicateKey); exist {
-			predicateMap := cachePredicate.(PredicateMap)
-			// TODO(resouer) Is it possible a race that cache failed to update immediately?
-			if hostPredicate, ok := predicateMap[equivalenceHash]; ok {
-				if hostPredicate.Fit {
-					return true, []algorithm.PredicateFailureReason{}, false
-				}
-				return false, hostPredicate.FailReasons, false
-			}
-			// is invalid
-			return false, []algorithm.PredicateFailureReason{}, true
+	if hostPredicate, exist := ec.algorithmCache[nodeName][predicateKey][equivalenceHash]; exist {
+		if hostPredicate.Fit {
+			return true, []algorithm.PredicateFailureReason{}, false
 		}
+		return false, hostPredicate.FailReasons, false
 	}
+	// is invalid
 	return false, []algorithm.PredicateFailureReason{}, true
 }
 
@@ -163,10 +142,8 @@ func (ec *EquivalenceCache) InvalidateCachedPredicateItem(nodeName string, predi
 	}
 	ec.mu.Lock()
 	defer ec.mu.Unlock()
-	if algorithmCache, exist := ec.algorithmCache[nodeName]; exist {
-		for predicateKey := range predicateKeys {
-			algorithmCache.predicatesCache.Remove(predicateKey)
-		}
+	for predicateKey := range predicateKeys {
+		delete(ec.algorithmCache[nodeName], predicateKey)
 	}
 	glog.V(5).Infof("Done invalidating cached predicates: %v on node: %s", predicateKeys, nodeName)
 }
@@ -181,8 +158,7 @@ func (ec *EquivalenceCache) InvalidateCachedPredicateItemOfAllNodes(predicateKey
 	// algorithmCache uses nodeName as key, so we just iterate it and invalid given predicates
 	for _, algorithmCache := range ec.algorithmCache {
 		for predicateKey := range predicateKeys {
-			// just use keys is enough
-			algorithmCache.predicatesCache.Remove(predicateKey)
+			delete(algorithmCache, predicateKey)
 		}
 	}
 	glog.V(5).Infof("Done invalidating cached predicates: %v on all node", predicateKeys)

pkg/scheduler/core/equivalence_cache_test.go

@@ -345,14 +345,14 @@ func TestUpdateResult(t *testing.T) {
 	for _, test := range tests {
 		ecache := NewEquivalenceCache()
 		if test.expectPredicateMap {
-			ecache.algorithmCache[test.nodeName] = newAlgorithmCache()
+			ecache.algorithmCache[test.nodeName] = AlgorithmCache{}
 			predicateItem := HostPredicate{
 				Fit: true,
 			}
-			ecache.algorithmCache[test.nodeName].predicatesCache.Add(test.predicateKey,
+			ecache.algorithmCache[test.nodeName][test.predicateKey] =
 				PredicateMap{
 					test.equivalenceHash: predicateItem,
-				})
+				}
 		}
 		ecache.mu.Lock()
 		ecache.updateResult(
@@ -365,12 +365,11 @@ func TestUpdateResult(t *testing.T) {
 		)
 		ecache.mu.Unlock()
 
-		value, ok := ecache.algorithmCache[test.nodeName].predicatesCache.Get(test.predicateKey)
+		cachedMapItem, ok := ecache.algorithmCache[test.nodeName][test.predicateKey]
 		if !ok {
 			t.Errorf("Failed: %s, can't find expected cache item: %v",
 				test.name, test.expectCacheItem)
 		} else {
-			cachedMapItem := value.(PredicateMap)
 			if !reflect.DeepEqual(cachedMapItem[test.equivalenceHash], test.expectCacheItem) {
 				t.Errorf("Failed: %s, expected cached item: %v, but got: %v",
 					test.name, test.expectCacheItem, cachedMapItem[test.equivalenceHash])
@@ -693,7 +692,7 @@ func TestInvalidateCachedPredicateItemOfAllNodes(t *testing.T) {
 	// there should be no cached predicate any more
 	for _, test := range tests {
 		if algorithmCache, exist := ecache.algorithmCache[test.nodeName]; exist {
-			if _, exist := algorithmCache.predicatesCache.Get(testPredicate); exist {
+			if _, exist := algorithmCache[testPredicate]; exist {
 				t.Errorf("Failed: cached item for predicate key: %v on node: %v should be invalidated",
 					testPredicate, test.nodeName)
 				break