diff --git a/pkg/controller/volume/persistentvolume/scheduler_binder.go b/pkg/controller/volume/persistentvolume/scheduler_binder.go index c39dd5e2668..8fb159e59a6 100644 --- a/pkg/controller/volume/persistentvolume/scheduler_binder.go +++ b/pkg/controller/volume/persistentvolume/scheduler_binder.go @@ -142,31 +142,19 @@ func (b *volumeBinder) GetBindingsCache() PodBindingCache { return b.podBindingCache } +func podHasClaims(pod *v1.Pod) bool { + for _, vol := range pod.Spec.Volumes { + if vol.PersistentVolumeClaim != nil { + return true + } + } + return false +} + // FindPodVolumes caches the matching PVs and PVCs to provision per node in podBindingCache. // This method intentionally takes in a *v1.Node object instead of using volumebinder.nodeInformer. // That's necessary because some operations will need to pass in to the predicate fake node objects. func (b *volumeBinder) FindPodVolumes(pod *v1.Pod, node *v1.Node) (unboundVolumesSatisfied, boundVolumesSatisfied bool, err error) { - var ( - matchedClaims []*bindingInfo - provisionedClaims []*v1.PersistentVolumeClaim - ) - defer func() { - // We recreate bindings for each new schedule loop. - // Although we do not distinguish nil from empty in this function, for - // easier testing, we normalize empty to nil. - if len(matchedClaims) == 0 { - matchedClaims = nil - } - if len(provisionedClaims) == 0 { - provisionedClaims = nil - } - // TODO merge into one atomic function - // Mark cache with all the matches for each PVC for this node - b.podBindingCache.UpdateBindings(pod, node.Name, matchedClaims) - // Mark cache with all the PVCs that need provisioning for this node - b.podBindingCache.UpdateProvisionedPVCs(pod, node.Name, provisionedClaims) - }() - podName := getPodName(pod) // Warning: Below log needs high verbosity as it can be printed several times (#60933). @@ -183,6 +171,34 @@ func (b *volumeBinder) FindPodVolumes(pod *v1.Pod, node *v1.Node) (unboundVolume } }() + if !podHasClaims(pod) { + // Fast path + return unboundVolumesSatisfied, boundVolumesSatisfied, nil + } + + var ( + matchedClaims []*bindingInfo + provisionedClaims []*v1.PersistentVolumeClaim + ) + defer func() { + // We recreate bindings for each new schedule loop. + if len(matchedClaims) == 0 && len(provisionedClaims) == 0 { + // Clear cache if no claims to bind or provision for this node. + b.podBindingCache.ClearBindings(pod, node.Name) + return + } + // Although we do not distinguish nil from empty in this function, for + // easier testing, we normalize empty to nil. + if len(matchedClaims) == 0 { + matchedClaims = nil + } + if len(provisionedClaims) == 0 { + provisionedClaims = nil + } + // Mark cache with all matched and provisioned claims for this node + b.podBindingCache.UpdateBindings(pod, node.Name, matchedClaims, provisionedClaims) + }() + // The pod's volumes need to be processed in one call to avoid the race condition where // volumes can get bound/provisioned in between calls. boundClaims, claimsToBind, unboundClaimsImmediate, err := b.getPodVolumes(pod) @@ -318,8 +334,7 @@ func (b *volumeBinder) AssumePodVolumes(assumedPod *v1.Pod, nodeName string) (al // Update cache with the assumed pvcs and pvs // Even if length is zero, update the cache with an empty slice to indicate that no // operations are needed - b.podBindingCache.UpdateBindings(assumedPod, nodeName, newBindings) - b.podBindingCache.UpdateProvisionedPVCs(assumedPod, nodeName, newProvisionedPVCs) + b.podBindingCache.UpdateBindings(assumedPod, nodeName, newBindings, newProvisionedPVCs) return } diff --git a/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go b/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go index f32c1269b54..f67644f8914 100644 --- a/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go +++ b/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go @@ -28,18 +28,16 @@ import ( type PodBindingCache interface { // UpdateBindings will update the cache with the given bindings for the // pod and node. - UpdateBindings(pod *v1.Pod, node string, bindings []*bindingInfo) + UpdateBindings(pod *v1.Pod, node string, bindings []*bindingInfo, provisionings []*v1.PersistentVolumeClaim) + + // ClearBindings will clear the cached bindings for the given pod and node. + ClearBindings(pod *v1.Pod, node string) // GetBindings will return the cached bindings for the given pod and node. // A nil return value means that the entry was not found. An empty slice // means that no binding operations are needed. GetBindings(pod *v1.Pod, node string) []*bindingInfo - // UpdateProvisionedPVCs will update the cache with the given provisioning decisions - // for the pod and node. - UpdateProvisionedPVCs(pod *v1.Pod, node string, provisionings []*v1.PersistentVolumeClaim) - - // GetProvisionedPVCs will return the cached provisioning decisions for the given pod and node. // A nil return value means that the entry was not found. An empty slice // means that no provisioning operations are needed. GetProvisionedPVCs(pod *v1.Pod, node string) []*v1.PersistentVolumeClaim @@ -98,7 +96,7 @@ func (c *podBindingCache) DeleteBindings(pod *v1.Pod) { } } -func (c *podBindingCache) UpdateBindings(pod *v1.Pod, node string, bindings []*bindingInfo) { +func (c *podBindingCache) UpdateBindings(pod *v1.Pod, node string, bindings []*bindingInfo, pvcs []*v1.PersistentVolumeClaim) { c.rwMutex.Lock() defer c.rwMutex.Unlock() @@ -111,11 +109,13 @@ func (c *podBindingCache) UpdateBindings(pod *v1.Pod, node string, bindings []*b decision, ok := decisions[node] if !ok { decision = nodeDecision{ - bindings: bindings, + bindings: bindings, + provisionings: pvcs, } VolumeBindingRequestSchedulerBinderCache.WithLabelValues("add").Inc() } else { decision.bindings = bindings + decision.provisionings = pvcs } decisions[node] = decision } @@ -136,27 +136,6 @@ func (c *podBindingCache) GetBindings(pod *v1.Pod, node string) []*bindingInfo { return decision.bindings } -func (c *podBindingCache) UpdateProvisionedPVCs(pod *v1.Pod, node string, pvcs []*v1.PersistentVolumeClaim) { - c.rwMutex.Lock() - defer c.rwMutex.Unlock() - - podName := getPodName(pod) - decisions, ok := c.bindingDecisions[podName] - if !ok { - decisions = nodeDecisions{} - c.bindingDecisions[podName] = decisions - } - decision, ok := decisions[node] - if !ok { - decision = nodeDecision{ - provisionings: pvcs, - } - } else { - decision.provisionings = pvcs - } - decisions[node] = decision -} - func (c *podBindingCache) GetProvisionedPVCs(pod *v1.Pod, node string) []*v1.PersistentVolumeClaim { c.rwMutex.RLock() defer c.rwMutex.RUnlock() @@ -172,3 +151,15 @@ func (c *podBindingCache) GetProvisionedPVCs(pod *v1.Pod, node string) []*v1.Per } return decision.provisionings } + +func (c *podBindingCache) ClearBindings(pod *v1.Pod, node string) { + c.rwMutex.Lock() + defer c.rwMutex.Unlock() + + podName := getPodName(pod) + decisions, ok := c.bindingDecisions[podName] + if !ok { + return + } + delete(decisions, node) +} diff --git a/pkg/controller/volume/persistentvolume/scheduler_binder_cache_test.go b/pkg/controller/volume/persistentvolume/scheduler_binder_cache_test.go index 65086274cc3..f8499e752da 100644 --- a/pkg/controller/volume/persistentvolume/scheduler_binder_cache_test.go +++ b/pkg/controller/volume/persistentvolume/scheduler_binder_cache_test.go @@ -48,6 +48,14 @@ func TestUpdateGetBindings(t *testing.T) { getPod: "pod1", getNode: "node2", }, + "binding-nil": { + updatePod: "pod1", + updateNode: "node1", + updateBindings: nil, + updateProvisionings: nil, + getPod: "pod1", + getNode: "node1", + }, "binding-exists": { updatePod: "pod1", updateNode: "node1", @@ -65,8 +73,7 @@ func TestUpdateGetBindings(t *testing.T) { // Perform updates updatePod := &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: scenario.updatePod, Namespace: "ns"}} - cache.UpdateBindings(updatePod, scenario.updateNode, scenario.updateBindings) - cache.UpdateProvisionedPVCs(updatePod, scenario.updateNode, scenario.updateProvisionings) + cache.UpdateBindings(updatePod, scenario.updateNode, scenario.updateBindings, scenario.updateProvisionings) // Verify updated bindings bindings := cache.GetBindings(updatePod, scenario.updateNode) @@ -116,8 +123,7 @@ func TestDeleteBindings(t *testing.T) { cache.DeleteBindings(pod) // Perform updates - cache.UpdateBindings(pod, "node1", initialBindings) - cache.UpdateProvisionedPVCs(pod, "node1", initialProvisionings) + cache.UpdateBindings(pod, "node1", initialBindings, initialProvisionings) // Get bindings and provisionings bindings = cache.GetBindings(pod, "node1") diff --git a/pkg/controller/volume/persistentvolume/scheduler_binder_test.go b/pkg/controller/volume/persistentvolume/scheduler_binder_test.go index a3792ecec7c..c2450210ac1 100644 --- a/pkg/controller/volume/persistentvolume/scheduler_binder_test.go +++ b/pkg/controller/volume/persistentvolume/scheduler_binder_test.go @@ -328,8 +328,6 @@ func (env *testEnv) assumeVolumes(t *testing.T, name, node string, pod *v1.Pod, } } - env.internalBinder.podBindingCache.UpdateBindings(pod, node, bindings) - pvcCache := env.internalBinder.pvcCache for _, pvc := range provisionings { if err := pvcCache.Assume(pvc); err != nil { @@ -337,14 +335,12 @@ func (env *testEnv) assumeVolumes(t *testing.T, name, node string, pod *v1.Pod, } } - env.internalBinder.podBindingCache.UpdateProvisionedPVCs(pod, node, provisionings) + env.internalBinder.podBindingCache.UpdateBindings(pod, node, bindings, provisionings) } func (env *testEnv) initPodCache(pod *v1.Pod, node string, bindings []*bindingInfo, provisionings []*v1.PersistentVolumeClaim) { cache := env.internalBinder.podBindingCache - cache.UpdateBindings(pod, node, bindings) - - cache.UpdateProvisionedPVCs(pod, node, provisionings) + cache.UpdateBindings(pod, node, bindings, provisionings) } func (env *testEnv) validatePodCache(t *testing.T, name, node string, pod *v1.Pod, expectedBindings []*bindingInfo, expectedProvisionings []*v1.PersistentVolumeClaim) {