From 7fe97886a82a4e19af17dfd399b5626073d5f2a9 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Wed, 16 Jan 2019 13:25:24 +0800 Subject: [PATCH] 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) {