From 5a708a7ff0d377664b4f8e5dd3c066da025f77e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=83=A1=E7=8E=AE=E6=96=87?= Date: Fri, 12 Sep 2025 15:03:47 +0800 Subject: [PATCH] scheduler/volumebinding: remove Get[API]{PV,PVC} should be replaced by generic Get[APIObj] --- .../plugins/volumebinding/assume_cache.go | 16 ------- .../volumebinding/assume_cache_test.go | 42 +++++++++---------- .../framework/plugins/volumebinding/binder.go | 12 +++--- .../plugins/volumebinding/binder_test.go | 24 +++++------ 4 files changed, 39 insertions(+), 55 deletions(-) diff --git a/pkg/scheduler/framework/plugins/volumebinding/assume_cache.go b/pkg/scheduler/framework/plugins/volumebinding/assume_cache.go index b4e6063b149..d62ca6e5543 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/assume_cache.go +++ b/pkg/scheduler/framework/plugins/volumebinding/assume_cache.go @@ -57,14 +57,6 @@ func NewPVAssumeCache(logger klog.Logger, informer informer) (PVAssumeCache, err return PVAssumeCache{cache}, err } -func (c PVAssumeCache) GetPV(pvName string) (*v1.PersistentVolume, error) { - return c.Get(pvName) -} - -func (c PVAssumeCache) GetAPIPV(pvName string) (*v1.PersistentVolume, error) { - return c.GetAPIObj(pvName) -} - func (c PVAssumeCache) ListPVs(storageClassName string) ([]*v1.PersistentVolume, error) { // This works because we will never change the storage class in scheduler // Assumed PVs needs to be included here to ensure the same PVC will not be bound to another PV in the next scheduling cycle. @@ -82,11 +74,3 @@ func NewPVCAssumeCache(logger klog.Logger, informer informer) (PVCAssumeCache, e cache, err := newAssumeCache[*v1.PersistentVolumeClaim](logger, informer, schema.GroupResource{Resource: "persistentvolumeclaims"}) return PVCAssumeCache{cache}, err } - -func (c PVCAssumeCache) GetPVC(pvName string) (*v1.PersistentVolumeClaim, error) { - return c.Get(pvName) -} - -func (c PVCAssumeCache) GetAPIPVC(pvName string) (*v1.PersistentVolumeClaim, error) { - return c.GetAPIObj(pvName) -} diff --git a/pkg/scheduler/framework/plugins/volumebinding/assume_cache_test.go b/pkg/scheduler/framework/plugins/volumebinding/assume_cache_test.go index 8af8e024e02..21e2d00bdb2 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/assume_cache_test.go +++ b/pkg/scheduler/framework/plugins/volumebinding/assume_cache_test.go @@ -97,12 +97,12 @@ func verifyListPVs(t *testing.T, cache PVAssumeCache, expectedPVs map[string]*v1 } func verifyPV(cache PVAssumeCache, name string, expectedPV *v1.PersistentVolume) error { - pv, err := cache.GetPV(name) + pv, err := cache.Get(name) if err != nil { return err } if pv != expectedPV { - return fmt.Errorf("GetPV() returned %p, expected %p", pv, expectedPV) + return fmt.Errorf("Get() returned %p, expected %p", pv, expectedPV) } return nil } @@ -168,7 +168,7 @@ func TestAssumePV(t *testing.T) { t.Errorf("Test %q failed: Assume() returned success but expected error", name) } - // Check that GetPV returns correct PV + // Check that Get returns correct PV expectedPV := scenario.newPV if !scenario.shouldSucceed { expectedPV = scenario.oldPV @@ -192,13 +192,13 @@ func TestRestorePV(t *testing.T) { // Add oldPV to cache informer.add(oldPV) if err := verifyPV(cache, oldPV.Name, oldPV); err != nil { - t.Fatalf("Failed to GetPV() after initial update: %v", err) + t.Fatalf("Failed to Get() after initial update: %v", err) } // Restore PV cache.Restore(oldPV) if err := verifyPV(cache, oldPV.Name, oldPV); err != nil { - t.Fatalf("Failed to GetPV() after initial restore: %v", err) + t.Fatalf("Failed to Get() after initial restore: %v", err) } // Assume newPV @@ -206,13 +206,13 @@ func TestRestorePV(t *testing.T) { t.Fatalf("Assume() returned error %v", err) } if err := verifyPV(cache, oldPV.Name, newPV); err != nil { - t.Fatalf("Failed to GetPV() after Assume: %v", err) + t.Fatalf("Failed to Get() after Assume: %v", err) } // Restore PV cache.Restore(newPV) if err := verifyPV(cache, oldPV.Name, oldPV); err != nil { - t.Fatalf("Failed to GetPV() after restore: %v", err) + t.Fatalf("Failed to Get() after restore: %v", err) } } @@ -220,12 +220,12 @@ func TestBasicPVCache(t *testing.T) { informer, cache := newTestPVCache(t) // Get object that doesn't exist - pv, err := cache.GetPV("nothere") + pv, err := cache.Get("nothere") if err == nil { - t.Errorf("GetPV() returned unexpected success") + t.Errorf("Get() returned unexpected success") } if pv != nil { - t.Errorf("GetPV() returned unexpected PV %q", pv.Name) + t.Errorf("Get() returned unexpected PV %q", pv.Name) } // Add a bunch of PVs @@ -339,12 +339,12 @@ func makeClaim(name, version, namespace string) *v1.PersistentVolumeClaim { } func verifyPVC(cache PVCAssumeCache, pvcKey string, expectedPVC *v1.PersistentVolumeClaim) error { - pvc, err := cache.GetPVC(pvcKey) + pvc, err := cache.Get(pvcKey) if err != nil { return err } if pvc != expectedPVC { - return fmt.Errorf("GetPVC() returned %p, expected %p", pvc, expectedPVC) + return fmt.Errorf("Get() returned %p, expected %p", pvc, expectedPVC) } return nil } @@ -407,7 +407,7 @@ func TestAssumePVC(t *testing.T) { // Add oldPVC to cache informer.add(scenario.oldPVC) if err := verifyPVC(cache, getPVCName(scenario.oldPVC), scenario.oldPVC); err != nil { - t.Fatalf("Failed to GetPVC() after initial update: %v", err) + t.Fatalf("Failed to Get() after initial update: %v", err) } // Assume newPVC @@ -419,13 +419,13 @@ func TestAssumePVC(t *testing.T) { t.Errorf("Test %q failed: Assume() returned success but expected error", name) } - // Check that GetPVC returns correct PVC + // Check that Get returns correct PVC expectedPV := scenario.newPVC if !scenario.shouldSucceed { expectedPV = scenario.oldPVC } if err := verifyPVC(cache, getPVCName(scenario.oldPVC), expectedPV); err != nil { - t.Errorf("Failed to GetPVC() after initial update: %v", err) + t.Errorf("Failed to Get() after initial update: %v", err) } }) } @@ -443,13 +443,13 @@ func TestRestorePVC(t *testing.T) { // Add oldPVC to cache informer.add(oldPVC) if err := verifyPVC(cache, getPVCName(oldPVC), oldPVC); err != nil { - t.Fatalf("Failed to GetPVC() after initial update: %v", err) + t.Fatalf("Failed to Get() after initial update: %v", err) } // Restore PVC cache.Restore(oldPVC) if err := verifyPVC(cache, getPVCName(oldPVC), oldPVC); err != nil { - t.Fatalf("Failed to GetPVC() after initial restore: %v", err) + t.Fatalf("Failed to Get() after initial restore: %v", err) } // Assume newPVC @@ -457,13 +457,13 @@ func TestRestorePVC(t *testing.T) { t.Fatalf("Assume() returned error %v", err) } if err := verifyPVC(cache, getPVCName(oldPVC), newPVC); err != nil { - t.Fatalf("Failed to GetPVC() after Assume: %v", err) + t.Fatalf("Failed to Get() after Assume: %v", err) } // Restore PVC cache.Restore(newPVC) if err := verifyPVC(cache, getPVCName(oldPVC), oldPVC); err != nil { - t.Fatalf("Failed to GetPVC() after restore: %v", err) + t.Fatalf("Failed to Get() after restore: %v", err) } } @@ -480,7 +480,7 @@ func TestConcurrentAssumePVC(t *testing.T) { t.Fatalf("Assume() returned error %v", err) } if err := verifyPVC(cache, getPVCName(pvc1Update), pvc1Update); err != nil { - t.Fatalf("Failed to GetPVC() after Assume: %v", err) + t.Fatalf("Failed to Get() after Assume: %v", err) } pvc2 := makeClaim("pvc1", "7", "ns1") @@ -496,7 +496,7 @@ func TestConcurrentAssumePVC(t *testing.T) { cache.Restore(pvc1Update) // Should still have pvc 2 in cache if err := verifyPVC(cache, getPVCName(pvc2Update), pvc2Update); err != nil { - t.Fatalf("Failed to GetPVC() after restore: %v", err) + t.Fatalf("Failed to Get() after restore: %v", err) } } diff --git a/pkg/scheduler/framework/plugins/volumebinding/binder.go b/pkg/scheduler/framework/plugins/volumebinding/binder.go index 738718f19e1..f4c9a22fc1f 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/binder.go +++ b/pkg/scheduler/framework/plugins/volumebinding/binder.go @@ -625,12 +625,12 @@ func (b *volumeBinder) checkBindings(logger klog.Logger, pod *v1.Pod, bindings [ } for _, binding := range bindings { - pv, err := b.pvCache.GetAPIPV(binding.pv.Name) + pv, err := b.pvCache.GetAPIObj(binding.pv.Name) if err != nil { return false, fmt.Errorf("failed to check binding: %w", err) } - pvc, err := b.pvcCache.GetAPIPVC(getPVCName(binding.pvc)) + pvc, err := b.pvcCache.GetAPIObj(getPVCName(binding.pvc)) if err != nil { return false, fmt.Errorf("failed to check binding: %w", err) } @@ -663,7 +663,7 @@ func (b *volumeBinder) checkBindings(logger klog.Logger, pod *v1.Pod, bindings [ } for _, claim := range claimsToProvision { - pvc, err := b.pvcCache.GetAPIPVC(getPVCName(claim)) + pvc, err := b.pvcCache.GetAPIObj(getPVCName(claim)) if err != nil { return false, fmt.Errorf("failed to check provisioning pvc: %w", err) } @@ -688,7 +688,7 @@ func (b *volumeBinder) checkBindings(logger klog.Logger, pod *v1.Pod, bindings [ // If the PVC is bound to a PV, check its node affinity if pvc.Spec.VolumeName != "" { - pv, err := b.pvCache.GetAPIPV(pvc.Spec.VolumeName) + pv, err := b.pvCache.GetAPIObj(pvc.Spec.VolumeName) if err != nil { if apierrors.IsNotFound(err) { // We tolerate NotFound error here, because PV is possibly @@ -754,7 +754,7 @@ func (b *volumeBinder) isPVCBound(logger klog.Logger, namespace, pvcName string) }, } pvcKey := getPVCName(claim) - pvc, err := b.pvcCache.GetPVC(pvcKey) + pvc, err := b.pvcCache.Get(pvcKey) if err != nil || pvc == nil { return false, nil, fmt.Errorf("error getting PVC %q: %v", pvcKey, err) } @@ -845,7 +845,7 @@ func (b *volumeBinder) checkBoundClaims(logger klog.Logger, claims []*v1.Persist for _, pvc := range claims { pvName := pvc.Spec.VolumeName - pv, err := b.pvCache.GetPV(pvName) + pv, err := b.pvCache.Get(pvName) if err != nil { if apierrors.IsNotFound(err) { err = nil diff --git a/pkg/scheduler/framework/plugins/volumebinding/binder_test.go b/pkg/scheduler/framework/plugins/volumebinding/binder_test.go index 5fb2bb26944..1273c10a6f7 100644 --- a/pkg/scheduler/framework/plugins/volumebinding/binder_test.go +++ b/pkg/scheduler/framework/plugins/volumebinding/binder_test.go @@ -336,7 +336,7 @@ func (env *testEnv) updateVolumes(ctx context.Context, pvs []*v1.PersistentVolum } return wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 3*time.Second, false, func(ctx context.Context) (bool, error) { for _, pv := range pvs { - pvInCache, err := env.internalBinder.pvCache.GetAPIPV(pv.Name) + pvInCache, err := env.internalBinder.pvCache.GetAPIObj(pv.Name) if pvInCache == nil || err != nil { return false, nil } @@ -358,7 +358,7 @@ func (env *testEnv) updateClaims(ctx context.Context, pvcs []*v1.PersistentVolum } return wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 3*time.Second, false, func(ctx context.Context) (bool, error) { for _, pvc := range pvcs { - pvcInCache, err := env.internalBinder.pvcCache.GetAPIPVC(getPVCName(pvc)) + pvcInCache, err := env.internalBinder.pvcCache.GetAPIObj(getPVCName(pvc)) if pvcInCache == nil || err != nil { return false, nil } @@ -454,9 +454,9 @@ func (env *testEnv) validateAssume(t *testing.T, pod *v1.Pod, bindings []*Bindin // Check pv cache pvCache := env.internalBinder.pvCache for _, b := range bindings { - pv, err := pvCache.GetPV(b.pv.Name) + pv, err := pvCache.Get(b.pv.Name) if err != nil { - t.Errorf("GetPV %q returned error: %v", b.pv.Name, err) + t.Errorf("Get PV %q returned error: %v", b.pv.Name, err) continue } if pv.Spec.ClaimRef == nil { @@ -475,9 +475,9 @@ func (env *testEnv) validateAssume(t *testing.T, pod *v1.Pod, bindings []*Bindin pvcCache := env.internalBinder.pvcCache for _, p := range provisionings { pvcKey := getPVCName(p) - pvc, err := pvcCache.GetPVC(pvcKey) + pvc, err := pvcCache.Get(pvcKey) if err != nil { - t.Errorf("GetPVC %q returned error: %v", pvcKey, err) + t.Errorf("Get PVC %q returned error: %v", pvcKey, err) continue } if pvc.Annotations[volume.AnnSelectedNode] != nodeLabelValue { @@ -502,9 +502,9 @@ func (env *testEnv) validateCacheRestored(t *testing.T, pod *v1.Pod, bindings [] pvcCache := env.internalBinder.pvcCache for _, p := range provisionings { pvcKey := getPVCName(p) - pvc, err := pvcCache.GetPVC(pvcKey) + pvc, err := pvcCache.Get(pvcKey) if err != nil { - t.Errorf("GetPVC %q returned error: %v", pvcKey, err) + t.Errorf("Get PVC %q returned error: %v", pvcKey, err) continue } if pvc.Annotations[volume.AnnSelectedNode] != "" { @@ -522,9 +522,9 @@ func (env *testEnv) validateBind( // Check pv cache pvCache := env.internalBinder.pvCache for _, pv := range expectedPVs { - cachedPV, err := pvCache.GetPV(pv.Name) + cachedPV, err := pvCache.Get(pv.Name) if err != nil { - t.Errorf("GetPV %q returned error: %v", pv.Name, err) + t.Errorf("Get PV %q returned error: %v", pv.Name, err) } // Cache may be overridden by API object with higher version, compare but ignore resource version. newCachedPV := cachedPV.DeepCopy() @@ -549,9 +549,9 @@ func (env *testEnv) validateProvision( // Check pvc cache pvcCache := env.internalBinder.pvcCache for _, pvc := range expectedPVCs { - cachedPVC, err := pvcCache.GetPVC(getPVCName(pvc)) + cachedPVC, err := pvcCache.Get(getPVCName(pvc)) if err != nil { - t.Errorf("GetPVC %q returned error: %v", getPVCName(pvc), err) + t.Errorf("Get PVC %q returned error: %v", getPVCName(pvc), err) } // Cache may be overridden by API object with higher version, compare but ignore resource version. newCachedPVC := cachedPVC.DeepCopy()