Merge pull request red-hat-storage#87 from leelavg/fix-label

controllers: use annotations for claims and client mapping

controllers/storageclassclaim_controller.go

@@ -195,8 +195,6 @@ func (r *StorageClassClaimReconciler) reconcilePhases() (reconcile.Result, error
 	// Close client-side connections.
 	defer providerClient.Close()
 
-	addAnnotation(r.storageClassClaim, storageClientLabel, r.storageClient.Name)
-
 	cc := csi.ClusterConfig{
 		Client:    r.Client,
 		Namespace: r.OperatorNamespace,
@@ -241,13 +239,20 @@ func (r *StorageClassClaimReconciler) reconcilePhases() (reconcile.Result, error
 		// Configuration phase.
 		r.storageClassClaim.Status.Phase = v1alpha1.StorageClassClaimConfiguring
 
+		updateStorageClassClaim := false
 		// Check if finalizers are present, if not, add them.
 		if !contains(r.storageClassClaim.GetFinalizers(), storageClassClaimFinalizer) {
-			storageClassClaimRef := klog.KRef(r.storageClassClaim.Name, r.storageClassClaim.Namespace)
-			r.log.Info("Finalizer not found for StorageClassClaim. Adding finalizer.", "StorageClassClaim", storageClassClaimRef)
+			r.log.Info("Finalizer not found for StorageClassClaim. Adding finalizer.", "StorageClassClaim", r.storageClassClaim.Name)
 			r.storageClassClaim.SetFinalizers(append(r.storageClassClaim.GetFinalizers(), storageClassClaimFinalizer))
+			updateStorageClassClaim = true
+		}
+		if addAnnotation(r.storageClassClaim, storageClientAnnotationKey, client.ObjectKeyFromObject(r.storageClient).String()) {
+			updateStorageClassClaim = true
+		}
+
+		if updateStorageClassClaim {
 			if err := r.update(r.storageClassClaim); err != nil {
-				return reconcile.Result{}, fmt.Errorf("failed to update StorageClassClaim [%v] with finalizer: %s", storageClassClaimRef, err)
+				return reconcile.Result{}, fmt.Errorf("failed to update StorageClassClaim %q: %v", r.storageClassClaim.Name, err)
 			}
 		}
 
@@ -572,14 +577,18 @@ func (r *StorageClassClaimReconciler) own(resource metav1.Object) error {
 	return controllerutil.SetOwnerReference(r.storageClassClaim, resource, r.Scheme)
 }
 
-// addAnnotation add a annotation to a resource metadata
-func addAnnotation(obj metav1.Object, key string, value string) {
+// addAnnotation adds an annotation to a resource metadata, returns true if added else false
+func addAnnotation(obj metav1.Object, key string, value string) bool {
 	annotations := obj.GetAnnotations()
 	if annotations == nil {
 		annotations = map[string]string{}
 		obj.SetAnnotations(annotations)
 	}
-	annotations[key] = value
+	if oldValue, exist := annotations[key]; !exist || oldValue != value {
+		annotations[key] = value
+		return true
+	}
+	return false
 }
 
 func (r *StorageClassClaimReconciler) createOrReplaceVolumeSnapshotClass(volumeSnapshotClass *snapapi.VolumeSnapshotClass) error {

controllers/storageclient_controller.go

@@ -57,11 +57,15 @@ const (
 	GetStorageConfig      = "GetStorageConfig"
 	AcknowledgeOnboarding = "AcknowledgeOnboarding"
 
-	storageClientLabel          = "ocs.openshift.io/storageclient"
+	storageClientAnnotationKey  = "ocs.openshift.io/storageclient"
 	storageClientNameLabel      = "ocs.openshift.io/storageclient.name"
 	storageClientNamespaceLabel = "ocs.openshift.io/storageclient.namespace"
 	storageClientFinalizer      = "storageclient.ocs.openshift.io"
 
+	// indexes for caching
+	storageProviderEndpointIndexName = "index:storageProviderEndpoint"
+	storageClientAnnotationIndexName = "index:storageClientAnnotation"
+
 	csvPrefix = "ocs-client-operator"
 )
 
@@ -78,18 +82,26 @@ type StorageClientReconciler struct {
 
 // SetupWithManager sets up the controller with the Manager.
 func (s *StorageClientReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	ctx := context.Background()
 	// Index should be registered before cache start.
 	// IndexField is used to filter out the objects that already exists with
 	// status.phase != failed This will help in blocking
 	// the new storageclient creation if there is already with one with same
 	// provider endpoint with status.phase != failed
-	_ = mgr.GetCache().IndexField(context.TODO(), &v1alpha1.StorageClient{}, "spec.storageProviderEndpoint", func(o client.Object) []string {
+	_ = mgr.GetCache().IndexField(ctx, &v1alpha1.StorageClient{}, storageProviderEndpointIndexName, func(o client.Object) []string {
 		res := []string{}
 		if o.(*v1alpha1.StorageClient).Status.Phase != v1alpha1.StorageClientFailed {
 			res = append(res, o.(*v1alpha1.StorageClient).Spec.StorageProviderEndpoint)
 		}
 		return res
 	})
+
+	if err := mgr.GetCache().IndexField(ctx, &v1alpha1.StorageClassClaim{}, storageClientAnnotationIndexName, func(obj client.Object) []string {
+		return []string{obj.GetAnnotations()[storageClientAnnotationKey]}
+	}); err != nil {
+		return fmt.Errorf("unable to set up FieldIndexer for storageclient annotation: %v", err)
+	}
+
 	enqueueStorageClientRequest := handler.EnqueueRequestsFromMapFunc(
 		func(_ context.Context, obj client.Object) []reconcile.Request {
 			annotations := obj.GetAnnotations()
@@ -158,7 +170,7 @@ func (s *StorageClientReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 
 func (s *StorageClientReconciler) reconcilePhases(instance *v1alpha1.StorageClient) (ctrl.Result, error) {
 	storageClientListOption := []client.ListOption{
-		client.MatchingFields{"spec.storageProviderEndpoint": instance.Spec.StorageProviderEndpoint},
+		client.MatchingFields{storageProviderEndpointIndexName: instance.Spec.StorageProviderEndpoint},
 	}
 
 	storageClientList := &v1alpha1.StorageClientList{}
@@ -341,21 +353,21 @@ func (s *StorageClientReconciler) offboardConsumer(instance *v1alpha1.StorageCli
 func (s *StorageClientReconciler) verifyNoStorageClassClaimsExist(instance *v1alpha1.StorageClient) error {
 
 	storageClassClaims := &v1alpha1.StorageClassClaimList{}
-	err := s.Client.List(s.ctx, storageClassClaims)
+	err := s.Client.List(s.ctx,
+		storageClassClaims,
+		client.MatchingFields{storageClientAnnotationIndexName: client.ObjectKeyFromObject(instance).String()},
+		client.Limit(1),
+	)
 	if err != nil {
 		return fmt.Errorf("failed to list storageClassClaims: %v", err)
 	}
 
-	for i := range storageClassClaims.Items {
-		storageClassClaim := &storageClassClaims.Items[i]
-		sc := storageClassClaim.Labels[storageClientLabel]
-		if sc == instance.Name {
-			err = fmt.Errorf("Failed to cleanup resources. storageClassClaims are present." +
-				"Delete all storageClassClaims for the cleanup to proceed")
-			s.recorder.ReportIfNotPresent(instance, corev1.EventTypeWarning, "Cleanup", err.Error())
-			s.Log.Error(err, "Waiting for all storageClassClaims to be deleted.")
-			return err
-		}
+	if len(storageClassClaims.Items) != 0 {
+		err = fmt.Errorf("Failed to cleanup resources. storageClassClaims are present."+
+			"Delete all storageClassClaims corresponding to storageclient %q for the cleanup to proceed", client.ObjectKeyFromObject(instance))
+		s.recorder.ReportIfNotPresent(instance, corev1.EventTypeWarning, "Cleanup", err.Error())
+		s.Log.Error(err, "Waiting for all storageClassClaims to be deleted.")
+		return err
 	}
 
 	return nil