From 5d13798e5c33ce02834823cdaf58a7c319cb2e1a Mon Sep 17 00:00:00 2001 From: Jonathan Basseri Date: Tue, 8 May 2018 16:00:37 -0700 Subject: [PATCH] Change the return of EquivalenceClass.lookupResult. This makes the lookup behave like a normal map lookup, so it is easier for readers to follow the logic. It also inverts the "invalid" bool to an "ok" bool because `!invalid` is a double negative. --- pkg/scheduler/core/equivalence_cache.go | 24 ++++------- pkg/scheduler/core/equivalence_cache_test.go | 45 +++++++++++--------- 2 files changed, 33 insertions(+), 36 deletions(-) diff --git a/pkg/scheduler/core/equivalence_cache.go b/pkg/scheduler/core/equivalence_cache.go index 6e080a256f..b2d56ff2a0 100644 --- a/pkg/scheduler/core/equivalence_cache.go +++ b/pkg/scheduler/core/equivalence_cache.go @@ -85,9 +85,9 @@ func (ec *EquivalenceCache) RunPredicate( return false, []algorithm.PredicateFailureReason{}, fmt.Errorf("nodeInfo is nil or node is invalid") } - fit, reasons, invalid := ec.lookupResult(pod.GetName(), nodeInfo.Node().GetName(), predicateKey, equivClassInfo.hash) - if !invalid { - return fit, reasons, nil + result, ok := ec.lookupResult(pod.GetName(), nodeInfo.Node().GetName(), predicateKey, equivClassInfo.hash) + if ok { + return result.Fit, result.FailReasons, nil } fit, reasons, err := pred(pod, meta, nodeInfo) if err != nil { @@ -139,26 +139,18 @@ func (ec *EquivalenceCache) updateResult( glog.V(5).Infof("Updated cached predicate: %v for pod: %v on node: %s, with item %v", predicateKey, podName, nodeName, predicateItem) } -// lookupResult returns cached predicate results: -// 1. if pod fit -// 2. reasons if pod did not fit -// 3. if cache item is not found +// lookupResult returns cached predicate results and a bool saying whether a +// cache entry was found. func (ec *EquivalenceCache) lookupResult( podName, nodeName, predicateKey string, equivalenceHash uint64, -) (bool, []algorithm.PredicateFailureReason, bool) { +) (value predicateResult, ok bool) { ec.mu.RLock() defer ec.mu.RUnlock() glog.V(5).Infof("Begin to calculate predicate: %v for pod: %s on node: %s based on equivalence cache", predicateKey, podName, nodeName) - if result, exist := ec.cache[nodeName][predicateKey][equivalenceHash]; exist { - if result.Fit { - return true, []algorithm.PredicateFailureReason{}, false - } - return false, result.FailReasons, false - } - // is invalid - return false, []algorithm.PredicateFailureReason{}, true + value, ok = ec.cache[nodeName][predicateKey][equivalenceHash] + return value, ok } // InvalidateCachedPredicateItem marks all items of given predicateKeys, of all pods, on the given node as invalid diff --git a/pkg/scheduler/core/equivalence_cache_test.go b/pkg/scheduler/core/equivalence_cache_test.go index efbcf43e9a..b745f1c81a 100644 --- a/pkg/scheduler/core/equivalence_cache_test.go +++ b/pkg/scheduler/core/equivalence_cache_test.go @@ -287,14 +287,14 @@ func TestRunPredicate(t *testing.T) { if !test.expectCacheHit && test.pred.callCount == 0 { t.Errorf("Predicate should be called") } - _, _, invalid := ecache.lookupResult(pod.Name, node.Node().Name, "testPredicate", equivClass.hash) - if invalid && test.expectCacheWrite { + _, ok := ecache.lookupResult(pod.Name, node.Node().Name, "testPredicate", equivClass.hash) + if !ok && test.expectCacheWrite { t.Errorf("Cache write should happen") } - if !test.expectCacheHit && test.expectCacheWrite && invalid { + if !test.expectCacheHit && test.expectCacheWrite && !ok { t.Errorf("Cache write should happen") } - if !test.expectCacheHit && !test.expectCacheWrite && !invalid { + if !test.expectCacheHit && !test.expectCacheWrite && ok { t.Errorf("Cache write should not happen") } }) @@ -396,8 +396,8 @@ func TestLookupResult(t *testing.T) { equivalenceHashForUpdatePredicate uint64 equivalenceHashForCalPredicate uint64 cachedItem predicateItemType - expectedInvalidPredicateKey bool - expectedInvalidEquivalenceHash bool + expectedPredicateKeyMiss bool + expectedEquivalenceHashMiss bool expectedPredicateItem predicateItemType cache schedulercache.Cache }{ @@ -412,7 +412,7 @@ func TestLookupResult(t *testing.T) { fit: false, reasons: []algorithm.PredicateFailureReason{predicates.ErrPodNotFitsHostPorts}, }, - expectedInvalidPredicateKey: true, + expectedPredicateKeyMiss: true, expectedPredicateItem: predicateItemType{ fit: false, reasons: []algorithm.PredicateFailureReason{}, @@ -429,7 +429,7 @@ func TestLookupResult(t *testing.T) { cachedItem: predicateItemType{ fit: true, }, - expectedInvalidPredicateKey: false, + expectedPredicateKeyMiss: false, expectedPredicateItem: predicateItemType{ fit: true, reasons: []algorithm.PredicateFailureReason{}, @@ -447,7 +447,7 @@ func TestLookupResult(t *testing.T) { fit: false, reasons: []algorithm.PredicateFailureReason{predicates.ErrPodNotFitsHostPorts}, }, - expectedInvalidPredicateKey: false, + expectedPredicateKeyMiss: false, expectedPredicateItem: predicateItemType{ fit: false, reasons: []algorithm.PredicateFailureReason{predicates.ErrPodNotFitsHostPorts}, @@ -465,8 +465,8 @@ func TestLookupResult(t *testing.T) { fit: false, reasons: []algorithm.PredicateFailureReason{predicates.ErrPodNotFitsHostPorts}, }, - expectedInvalidPredicateKey: false, - expectedInvalidEquivalenceHash: true, + expectedPredicateKeyMiss: false, + expectedEquivalenceHashMiss: true, expectedPredicateItem: predicateItemType{ fit: false, reasons: []algorithm.PredicateFailureReason{}, @@ -490,27 +490,32 @@ func TestLookupResult(t *testing.T) { node, ) // if we want to do invalid, invalid the cached item - if test.expectedInvalidPredicateKey { + if test.expectedPredicateKeyMiss { predicateKeys := sets.NewString() predicateKeys.Insert(test.predicateKey) ecache.InvalidateCachedPredicateItem(test.nodeName, predicateKeys) } // calculate predicate with equivalence cache - fit, reasons, invalid := ecache.lookupResult(test.podName, + result, ok := ecache.lookupResult(test.podName, test.nodeName, test.predicateKey, test.equivalenceHashForCalPredicate, ) - // returned invalid should match expectedInvalidPredicateKey or expectedInvalidEquivalenceHash + fit, reasons := result.Fit, result.FailReasons + // returned invalid should match expectedPredicateKeyMiss or expectedEquivalenceHashMiss if test.equivalenceHashForUpdatePredicate != test.equivalenceHashForCalPredicate { - if invalid != test.expectedInvalidEquivalenceHash { - t.Errorf("Failed: %s, expected invalid: %v, but got: %v", - test.name, test.expectedInvalidEquivalenceHash, invalid) + if ok && test.expectedEquivalenceHashMiss { + t.Errorf("Failed: %s, expected (equivalence hash) cache miss", test.name) + } + if !ok && !test.expectedEquivalenceHashMiss { + t.Errorf("Failed: %s, expected (equivalence hash) cache hit", test.name) } } else { - if invalid != test.expectedInvalidPredicateKey { - t.Errorf("Failed: %s, expected invalid: %v, but got: %v", - test.name, test.expectedInvalidPredicateKey, invalid) + if ok && test.expectedPredicateKeyMiss { + t.Errorf("Failed: %s, expected (predicate key) cache miss", test.name) + } + if !ok && !test.expectedPredicateKeyMiss { + t.Errorf("Failed: %s, expected (predicate key) cache hit", test.name) } } // returned predicate result should match expected predicate item