Merge pull request #47 from huffmanca/enable_volume_expansion

Bug 1751641: Enables updating of StorageClass if a change is detected.

pkg/controller/clusterstorage/clusterstorage_controller.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"reflect"
 
 	configv1 "github.com/openshift/api/config/v1"
 	"github.com/openshift/cluster-storage-operator/pkg/generated"
@@ -145,7 +146,7 @@ func (r *ReconcileClusterStorage) Reconcile(request reconcile.Request) (reconcil
 	}
 
 	// Define a new StorageClass object
-	sc, err := newStorageClassForCluster(instance)
+	newSCFromFile, err := newStorageClassForCluster(instance)
 	if err != nil {
 		_ = r.syncStatus(clusterOperatorInstance, err)
 		// requeue only if platform is supported
@@ -154,22 +155,22 @@ func (r *ReconcileClusterStorage) Reconcile(request reconcile.Request) (reconcil
 		}
 		return reconcile.Result{}, nil
 	}
-	clusterOperatorInstance.Status.RelatedObjects = getRelatedObjects(sc)
+	clusterOperatorInstance.Status.RelatedObjects = getRelatedObjects(newSCFromFile)
 
 	// Set the clusteroperator to be the owner of the SC
-	ocontroller.EnsureOwnerRef(sc, metav1.OwnerReference{
+	ocontroller.EnsureOwnerRef(newSCFromFile, metav1.OwnerReference{
 		APIVersion: "v1",
 		Kind:       "clusteroperator",
 		Name:       clusterOperatorName,
 		UID:        instance.GetUID(),
 	})
 
 	// Check if this StorageClass already exists
-	found := &storagev1.StorageClass{}
-	err = r.client.Get(context.TODO(), types.NamespacedName{Name: sc.Name, Namespace: corev1.NamespaceAll}, found)
+	existingSC := &storagev1.StorageClass{}
+	err = r.client.Get(context.TODO(), types.NamespacedName{Name: newSCFromFile.Name, Namespace: corev1.NamespaceAll}, existingSC)
 	if err != nil && apierrors.IsNotFound(err) {
-		reqLogger.Info("Creating a new StorageClass", "StorageClass.Name", sc.Name)
-		err = r.client.Create(context.TODO(), sc)
+		reqLogger.Info("Creating a new StorageClass", "StorageClass.Name", newSCFromFile.Name)
+		err = r.client.Create(context.TODO(), newSCFromFile)
 		if err != nil {
 			_ = r.syncStatus(clusterOperatorInstance, err)
 			return reconcile.Result{}, err
@@ -183,8 +184,34 @@ func (r *ReconcileClusterStorage) Reconcile(request reconcile.Request) (reconcil
 		return reconcile.Result{}, err
 	}
 
-	// StorageClass already exists - don't requeue
-	reqLogger.Info("Skip reconcile: StorageClass already exists", "StorageClass.Name", found.Name)
+	// Check to see if modifications have been made to the StorageClass attributes
+	comparisonSC := newSCFromFile.DeepCopy()
+	// Copy over the ObjectMeta, which includes the annotations and labels
+	comparisonSC.ObjectMeta = existingSC.ObjectMeta
+
+	// Define a default ReclaimPolicy for comparison
+	if comparisonSC.ReclaimPolicy == nil {
+		deletePolicy := corev1.PersistentVolumeReclaimDelete
+		comparisonSC.ReclaimPolicy = &deletePolicy
+	}
+
+	// If a change has been detected, update the StorageClass.
+	if !reflect.DeepEqual(comparisonSC, existingSC) {
+		reqLogger.Info("StorageClass already exists and needs to be updated", "StorageClass.Name", existingSC.Name)
+
+		err = r.client.Update(context.TODO(), comparisonSC)
+		if err != nil {
+			_ = r.syncStatus(clusterOperatorInstance, err)
+			return reconcile.Result{}, err
+		}
+
+		// StorageClass updated successfully - don't requeue
+		_ = r.syncStatus(clusterOperatorInstance, nil)
+		return reconcile.Result{}, nil
+	}
+
+	// StorageClass already exists and doesn't need to be updated - don't requeue
+	reqLogger.Info("Skip reconcile: StorageClass already exists", "StorageClass.Name", existingSC.Name)
 	_ = r.syncStatus(clusterOperatorInstance, nil)
 	return reconcile.Result{}, nil
 }

pkg/controller/clusterstorage/clusterstorage_controller_test.go

@@ -3,15 +3,19 @@ package clusterstorage
 import (
 	"context"
 	"os"
+	"reflect"
+	"strconv"
 	"testing"
 
 	configv1 "github.com/openshift/api/config/v1"
 	"github.com/openshift/cluster-storage-operator/pkg/apis"
 	corev1 "k8s.io/api/core/v1"
+	storagev1 "k8s.io/api/storage/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
 func TestSetStatusProgressing(t *testing.T) {
@@ -93,3 +97,113 @@ func TestSetStatusProgressing(t *testing.T) {
 		})
 	}
 }
+
+func TestReconcile(t *testing.T) {
+
+	platform := configv1.AWSPlatformType
+
+	tests := []struct {
+		name                 string
+		existingStorageClass *storagev1.StorageClass
+		newStorageClass      *storagev1.StorageClass
+		expectedStorageClass *storagev1.StorageClass
+	}{
+		{
+			name:                 "StorageClass needs to be updated. Annotations must be kept intact.",
+			existingStorageClass: getFakeStorageClass(false, false, ""),
+			newStorageClass:      getFakeStorageClass(false, true, ""),
+			expectedStorageClass: getFakeStorageClass(true, false, "kubernetes.io/aws-ebs"),
+		},
+		{
+			name:                 "StorageClass matches and does not need to be updated",
+			existingStorageClass: getFakeStorageClass(true, true, "kubernetes.io/aws-ebs"),
+			newStorageClass:      getFakeStorageClass(true, true, "kubernetes.io/aws-ebs"),
+			expectedStorageClass: getFakeStorageClass(true, true, "kubernetes.io/aws-ebs"),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			clusterOperator := &configv1.ClusterOperator{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "storage",
+					Namespace: corev1.NamespaceAll,
+				},
+			}
+			infrastructure := &configv1.Infrastructure{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "storage",
+					Namespace: corev1.NamespaceAll,
+				},
+				Status: configv1.InfrastructureStatus{
+					Platform: platform,
+				},
+			}
+			scheme := runtime.NewScheme()
+			if err := apis.AddToScheme(scheme); err != nil {
+				t.Errorf("apis.AddToScheme: %v", err)
+			}
+			if err := configv1.AddToScheme(scheme); err != nil {
+				t.Errorf("configv1.AddToScheme: %v", err)
+			}
+			if err := storagev1.AddToScheme(scheme); err != nil {
+				t.Errorf("storagev1.AddToScheme: %v", err)
+			}
+			client := fake.NewFakeClientWithScheme(scheme, clusterOperator, test.existingStorageClass, infrastructure)
+			reconciler := &ReconcileClusterStorage{client: client, scheme: scheme}
+
+			request := reconcile.Request{
+				NamespacedName: types.NamespacedName{
+					Name:      "storage",
+					Namespace: corev1.NamespaceAll,
+				},
+			}
+			_, err := reconciler.Reconcile(request)
+			if err != nil {
+				t.Errorf("Reconcile: %v", err)
+			}
+
+			err = client.Get(context.TODO(), types.NamespacedName{Name: test.newStorageClass.Name, Namespace: corev1.NamespaceAll}, test.newStorageClass)
+			if err != nil {
+				t.Errorf("Get: %v", err)
+			}
+			// Manually update the OwnerReferences to match the Operator added values
+			test.newStorageClass.OwnerReferences = test.expectedStorageClass.OwnerReferences
+
+			if !reflect.DeepEqual(test.newStorageClass, test.expectedStorageClass) {
+				t.Errorf("StorageClass doesn't match expected result: %v", test.newStorageClass)
+			}
+		})
+	}
+}
+
+func getFakeStorageClass(allowVolumeExpansion, isDefaultClass bool, provisioner string) *storagev1.StorageClass {
+	deletePolicy := corev1.PersistentVolumeReclaimDelete
+	volumeBindingMode := storagev1.VolumeBindingWaitForFirstConsumer
+
+	sc := &storagev1.StorageClass{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "StorageClass",
+			APIVersion: "storage.k8s.io/v1",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "gp2",
+			Annotations: map[string]string{
+				"storageclass.kubernetes.io/is-default-class": strconv.FormatBool(isDefaultClass),
+			},
+		},
+		AllowVolumeExpansion: &allowVolumeExpansion,
+		VolumeBindingMode:    &volumeBindingMode,
+		Parameters: map[string]string{
+			"encrypted": "true",
+			"type":      "gp2",
+		},
+		ReclaimPolicy: &deletePolicy,
+	}
+
+	if provisioner != "" {
+		sc.Provisioner = provisioner
+	}
+
+	return sc
+}