From 18a8184dcecbefb824f971fac50acaba4202adaf Mon Sep 17 00:00:00 2001
From: Jonathan Basseri <misterikkit@google.com>
Date: Tue, 15 May 2018 16:01:12 -0700
Subject: [PATCH] Add warnings about cache invalidation.

Part of https://github.com/kubernetes/kubernetes/pull/63040 is the
assumption that scheduler cache updates must happen before equivalence
cache updates for any given informer event.

The reason for this is that the equivalence cache implementation checks
the main cache for staleness while holding the equiv. cache write lock.

case 1: If an informer invalidates an equiv. cache entry before the
staleness check, then we know that the main cache update completed.

case 2: If an informer blocks trying to grab the equiv. cache lock, then
invalidation will occur right after the potentially stale update is
written.

This patch adds a note to places where we invalidate the equivalence
cache so that hopefully nobody violates this invariant.
---
 pkg/scheduler/factory/factory.go | 15 +++++++++++++++
 pkg/scheduler/scheduler.go       |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/pkg/scheduler/factory/factory.go b/pkg/scheduler/factory/factory.go
index 0c7dee77134..fb5cc22fcbe 100644
--- a/pkg/scheduler/factory/factory.go
+++ b/pkg/scheduler/factory/factory.go
@@ -634,6 +634,9 @@ func (c *configFactory) updatePodInCache(oldObj, newObj interface{}) {
 		return
 	}
 
+	// NOTE: Because the scheduler uses snapshots of schedulerCache and the live
+	// version of equivalencePodCache, updates must be written to schedulerCache
+	// before invalidating equivalencePodCache.
 	if err := c.schedulerCache.UpdatePod(oldPod, newPod); err != nil {
 		glog.Errorf("scheduler cache UpdatePod failed: %v", err)
 	}
@@ -720,6 +723,9 @@ func (c *configFactory) deletePodFromCache(obj interface{}) {
 		glog.Errorf("cannot convert to *v1.Pod: %v", t)
 		return
 	}
+	// NOTE: Because the scheduler uses snapshots of schedulerCache and the live
+	// version of equivalencePodCache, updates must be written to schedulerCache
+	// before invalidating equivalencePodCache.
 	if err := c.schedulerCache.RemovePod(pod); err != nil {
 		glog.Errorf("scheduler cache RemovePod failed: %v", err)
 	}
@@ -776,6 +782,9 @@ func (c *configFactory) updateNodeInCache(oldObj, newObj interface{}) {
 		return
 	}
 
+	// NOTE: Because the scheduler uses snapshots of schedulerCache and the live
+	// version of equivalencePodCache, updates must be written to schedulerCache
+	// before invalidating equivalencePodCache.
 	if err := c.schedulerCache.UpdateNode(oldNode, newNode); err != nil {
 		glog.Errorf("scheduler cache UpdateNode failed: %v", err)
 	}
@@ -869,6 +878,9 @@ func (c *configFactory) deleteNodeFromCache(obj interface{}) {
 		glog.Errorf("cannot convert to *v1.Node: %v", t)
 		return
 	}
+	// NOTE: Because the scheduler uses snapshots of schedulerCache and the live
+	// version of equivalencePodCache, updates must be written to schedulerCache
+	// before invalidating equivalencePodCache.
 	if err := c.schedulerCache.RemoveNode(node); err != nil {
 		glog.Errorf("scheduler cache RemoveNode failed: %v", err)
 	}
@@ -1297,6 +1309,9 @@ func (c *configFactory) MakeDefaultErrorFunc(backoff *util.PodBackoff, podQueue
 					_, err := c.client.CoreV1().Nodes().Get(nodeName, metav1.GetOptions{})
 					if err != nil && errors.IsNotFound(err) {
 						node := v1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}
+						// NOTE: Because the scheduler uses snapshots of schedulerCache and the live
+						// version of equivalencePodCache, updates must be written to schedulerCache
+						// before invalidating equivalencePodCache.
 						c.schedulerCache.RemoveNode(&node)
 						// invalidate cached predicate for the node
 						if c.enableEquivalenceClassCache {
diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go
index a3109599911..98beabe5c57 100644
--- a/pkg/scheduler/scheduler.go
+++ b/pkg/scheduler/scheduler.go
@@ -373,6 +373,9 @@ func (sched *Scheduler) assume(assumed *v1.Pod, host string) error {
 	// If the binding fails, scheduler will release resources allocated to assumed pod
 	// immediately.
 	assumed.Spec.NodeName = host
+	// NOTE: Because the scheduler uses snapshots of SchedulerCache and the live
+	// version of Ecache, updates must be written to SchedulerCache before
+	// invalidating Ecache.
 	if err := sched.config.SchedulerCache.AssumePod(assumed); err != nil {
 		glog.Errorf("scheduler cache AssumePod failed: %v", err)