From 064bab731e8a3d55f8b5180bb804794ee4bcdb93 Mon Sep 17 00:00:00 2001 From: Suleyman Akbas Date: Wed, 16 Aug 2023 17:39:34 +0200 Subject: [PATCH] fix: do not error during capacity check if annotation does not exist on the node --- .../persistent-volume-claim/controller.go | 12 +- .../controller_test.go | 106 ++++++++++++++++-- 2 files changed, 103 insertions(+), 15 deletions(-) diff --git a/controllers/persistent-volume-claim/controller.go b/controllers/persistent-volume-claim/controller.go index 9d3b4a209..2ff69ba96 100644 --- a/controllers/persistent-volume-claim/controller.go +++ b/controllers/persistent-volume-claim/controller.go @@ -21,7 +21,7 @@ import ( ) const ( - capacityAnnotation = "capacity.topolvm.io/00default" + CapacityAnnotation = "capacity.topolvm.io/" ) // PersistentVolumeClaimReconciler reconciles a PersistentVolumeClaim object @@ -67,7 +67,9 @@ func (r *PersistentVolumeClaimReconciler) Reconcile(ctx context.Context, req ctr return ctrl.Result{}, nil } - if !strings.HasPrefix(*pvc.Spec.StorageClassName, controllers.StorageClassPrefix) { + // Skip if StorageClassName does not contain the lvms prefix + lvmsPrefix, deviceClass, exists := strings.Cut(*pvc.Spec.StorageClassName, "-") + if !exists || fmt.Sprintf("%s-", lvmsPrefix) != controllers.StorageClassPrefix { logger.Info("skipping pvc as the storageClassName does not contain desired prefix", "desired-prefix", controllers.StorageClassPrefix) return ctrl.Result{}, nil @@ -90,11 +92,9 @@ func (r *PersistentVolumeClaimReconciler) Reconcile(ctx context.Context, req ctr requestedStorage := pvc.Spec.Resources.Requests.Storage() var nodeMessage []string for _, node := range nodeList.Items { - capacity, ok := node.Annotations[capacityAnnotation] + capacity, ok := node.Annotations[CapacityAnnotation+deviceClass] if !ok { - errMessage := fmt.Sprintf("could not find capacity annotation on the node %s", node.Name) - logger.Error(fmt.Errorf(errMessage), errMessage) - return ctrl.Result{}, nil + continue } capacityQuantity, err := resource.ParseQuantity(capacity) diff --git a/controllers/persistent-volume-claim/controller_test.go b/controllers/persistent-volume-claim/controller_test.go index 4ffb18474..9190ba54c 100644 --- a/controllers/persistent-volume-claim/controller_test.go +++ b/controllers/persistent-volume-claim/controller_test.go @@ -11,6 +11,7 @@ import ( persistentvolumeclaim "github.com/openshift/lvm-operator/controllers/persistent-volume-claim" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -107,6 +108,90 @@ func TestPersistentVolumeClaimReconciler_Reconcile(t *testing.T) { }, expectNoStorageAvailable: true, }, + { + name: "testing PVC requesting more storage than capacity in the node", + req: controllerruntime.Request{NamespacedName: types.NamespacedName{ + Namespace: defaultNamespace, + Name: "test-pending-PVC", + }}, + objs: []client.Object{ + &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNamespace, Name: "test-pending-PVC"}, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: pointer.String(controllers.StorageClassPrefix + "bla"), + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceStorage: *resource.NewQuantity(100, resource.DecimalSI), + }, + }, + }, + Status: v1.PersistentVolumeClaimStatus{ + Phase: v1.ClaimPending, + }, + }, + &v1.Node{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{persistentvolumeclaim.CapacityAnnotation + "bla": "10"}}, + }, + }, + expectNoStorageAvailable: true, + }, + { + name: "testing PVC requesting less storage than capacity in the node", + req: controllerruntime.Request{NamespacedName: types.NamespacedName{ + Namespace: defaultNamespace, + Name: "test-pending-PVC", + }}, + objs: []client.Object{ + &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNamespace, Name: "test-pending-PVC"}, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: pointer.String(controllers.StorageClassPrefix + "bla"), + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceStorage: *resource.NewQuantity(10, resource.DecimalSI), + }, + }, + }, + Status: v1.PersistentVolumeClaimStatus{ + Phase: v1.ClaimPending, + }, + }, + &v1.Node{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{persistentvolumeclaim.CapacityAnnotation + "bla": "100"}}, + }, + }, + expectNoStorageAvailable: false, + }, + { + name: "testing PVC requesting less storage than capacity in one node, having another node without annotation", + req: controllerruntime.Request{NamespacedName: types.NamespacedName{ + Namespace: defaultNamespace, + Name: "test-pending-PVC", + }}, + objs: []client.Object{ + &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNamespace, Name: "test-pending-PVC"}, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: pointer.String(controllers.StorageClassPrefix + "bla"), + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceStorage: *resource.NewQuantity(10, resource.DecimalSI), + }, + }, + }, + Status: v1.PersistentVolumeClaimStatus{ + Phase: v1.ClaimPending, + }, + }, + &v1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "annotated", Annotations: map[string]string{persistentvolumeclaim.CapacityAnnotation + "bla": "100"}}, + }, + &v1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "idle"}, + }, + }, + expectNoStorageAvailable: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -125,18 +210,21 @@ func TestPersistentVolumeClaimReconciler_Reconcile(t *testing.T) { return } - if tt.expectNoStorageAvailable { - select { - case event := <-recorder.Events: - if !strings.Contains(event, "NotEnoughCapacity") { - t.Errorf("event was captured but it did not contain the reason NotEnoughCapacity") - return - } - case <-time.After(100 * time.Millisecond): + select { + case event := <-recorder.Events: + if !strings.Contains(event, "NotEnoughCapacity") { + t.Errorf("event was captured but it did not contain the reason NotEnoughCapacity") + return + } + if !tt.expectNoStorageAvailable { + t.Errorf("event was captured but was not expected") + return + } + case <-time.After(100 * time.Millisecond): + if tt.expectNoStorageAvailable { t.Errorf("wanted event that no storage is available but none was sent") return } - } }) }