From 7fe97886a82a4e19af17dfd399b5626073d5f2a9 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Wed, 16 Jan 2019 13:25:24 +0800 Subject: [PATCH 1/3] Merge UpdateProvisionedPVCs with UpdateBindings. This simplifies code and saves a lock. --- .../persistentvolume/scheduler_binder.go | 10 ++---- .../scheduler_binder_cache.go | 34 +++---------------- .../scheduler_binder_cache_test.go | 14 +++++--- .../persistentvolume/scheduler_binder_test.go | 8 ++--- 4 files changed, 20 insertions(+), 46 deletions(-) diff --git a/pkg/controller/volume/persistentvolume/scheduler_binder.go b/pkg/controller/volume/persistentvolume/scheduler_binder.go index c39dd5e2668..b285d801ed6 100644 --- a/pkg/controller/volume/persistentvolume/scheduler_binder.go +++ b/pkg/controller/volume/persistentvolume/scheduler_binder.go @@ -160,11 +160,8 @@ func (b *volumeBinder) FindPodVolumes(pod *v1.Pod, node *v1.Node) (unboundVolume 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) + // Mark cache with all matched and provisioned claims for this node + b.podBindingCache.UpdateBindings(pod, node.Name, matchedClaims, provisionedClaims) }() podName := getPodName(pod) @@ -318,8 +315,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..e5bb3a6ea1a 100644 --- a/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go +++ b/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go @@ -28,18 +28,13 @@ 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) // 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 +93,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 +106,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 +133,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() 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) { From c2d25e08d79c071c3b8405e98c71251a6dc6fce4 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Wed, 16 Jan 2019 14:20:40 +0800 Subject: [PATCH 2/3] Skip if pod does not have claims. --- .../persistentvolume/scheduler_binder.go | 46 ++++++++++++------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/pkg/controller/volume/persistentvolume/scheduler_binder.go b/pkg/controller/volume/persistentvolume/scheduler_binder.go index b285d801ed6..9cbf315d899 100644 --- a/pkg/controller/volume/persistentvolume/scheduler_binder.go +++ b/pkg/controller/volume/persistentvolume/scheduler_binder.go @@ -142,10 +142,40 @@ 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) { + podName := getPodName(pod) + + // Warning: Below log needs high verbosity as it can be printed several times (#60933). + klog.V(5).Infof("FindPodVolumes for pod %q, node %q", podName, node.Name) + + // Initialize to true for pods that don't have volumes + unboundVolumesSatisfied = true + boundVolumesSatisfied = true + start := time.Now() + defer func() { + VolumeSchedulingStageLatency.WithLabelValues("predicate").Observe(time.Since(start).Seconds()) + if err != nil { + VolumeSchedulingStageFailed.WithLabelValues("predicate").Inc() + } + }() + + if !podHasClaims(pod) { + // Fast path + return unboundVolumesSatisfied, boundVolumesSatisfied, nil + } + var ( matchedClaims []*bindingInfo provisionedClaims []*v1.PersistentVolumeClaim @@ -164,22 +194,6 @@ func (b *volumeBinder) FindPodVolumes(pod *v1.Pod, node *v1.Node) (unboundVolume b.podBindingCache.UpdateBindings(pod, node.Name, matchedClaims, provisionedClaims) }() - podName := getPodName(pod) - - // Warning: Below log needs high verbosity as it can be printed several times (#60933). - klog.V(5).Infof("FindPodVolumes for pod %q, node %q", podName, node.Name) - - // Initialize to true for pods that don't have volumes - unboundVolumesSatisfied = true - boundVolumesSatisfied = true - start := time.Now() - defer func() { - VolumeSchedulingStageLatency.WithLabelValues("predicate").Observe(time.Since(start).Seconds()) - if err != nil { - VolumeSchedulingStageFailed.WithLabelValues("predicate").Inc() - } - }() - // 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) From dbd80460de04fd1097065dee646b667c0468f2fd Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Wed, 16 Jan 2019 21:21:36 +0800 Subject: [PATCH 3/3] Clear cache instead of saving nils if no claims to bind or provision --- .../volume/persistentvolume/scheduler_binder.go | 5 +++++ .../persistentvolume/scheduler_binder_cache.go | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/pkg/controller/volume/persistentvolume/scheduler_binder.go b/pkg/controller/volume/persistentvolume/scheduler_binder.go index 9cbf315d899..8fb159e59a6 100644 --- a/pkg/controller/volume/persistentvolume/scheduler_binder.go +++ b/pkg/controller/volume/persistentvolume/scheduler_binder.go @@ -182,6 +182,11 @@ func (b *volumeBinder) FindPodVolumes(pod *v1.Pod, node *v1.Node) (unboundVolume ) 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 { diff --git a/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go b/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go index e5bb3a6ea1a..f67644f8914 100644 --- a/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go +++ b/pkg/controller/volume/persistentvolume/scheduler_binder_cache.go @@ -30,6 +30,9 @@ type PodBindingCache interface { // pod and node. 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. @@ -148,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) +}