Merge pull request #385 from suleymanakbas91/fix-node-capacity-check

OCPBUGS-17805: Do not error during capacity check if annotation does not exist on the node
openshift · Aug 17, 2023 · f5d1358 · f5d1358
2 parents fe456ff + 064bab7
commit f5d1358
Show file tree

Hide file tree

Showing 2 changed files with 103 additions and 15 deletions.
diff --git 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
@@ -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
 				}
-
 			}
 		})
 	}