Minor fixes in import populator

* Fixes WFFC handling in import populator
* Fix indexer so it's shared between populators
* Improve populator source detection
* Modify common reconciler so we don't wait for pvcPrime to be in lost phase
* Adds license information in populator-related files

Signed-off-by: Alvaro Romero <alromero@redhat.com>

cmd/cdi-controller/controller.go

@@ -262,6 +262,11 @@ func start(ctx context.Context, cfg *rest.Config) {
 		klog.Errorf("Unable to setup datasource controller: %v", err)
 		os.Exit(1)
 	}
+	// Populator controllers and indexes
+	if err := populators.CreateCommonPopulatorIndexes(mgr); err != nil {
+		klog.Errorf("Unable to create common populator indexes: %v", err)
+		os.Exit(1)
+	}
 	if _, err := populators.NewImportPopulator(ctx, mgr, log, installerLabels); err != nil {
 		klog.Errorf("Unable to setup import populator: %v", err)
 		os.Exit(1)

pkg/controller/common/util.go

@@ -87,9 +87,6 @@ const (
 	AnnExternalPopulation = AnnAPIGroup + "/externalPopulation"
 	// AnnOwnedByDataVolume annotation has the owner DataVolume name
 	AnnOwnedByDataVolume = AnnAPIGroup + "/ownedByDataVolume"
-	// AnnSelectedNodeName annotation is added to the PVC' to specify the node used when creating the populator pod for
-	// dynamic provisioning
-	AnnSelectedNodeName = AnnAPIGroup + "/storage.selected.node"
 
 	// AnnDeleteAfterCompletion is PVC annotation for deleting DV after completion
 	AnnDeleteAfterCompletion = AnnAPIGroup + "/storage.deleteAfterCompletion"
@@ -222,6 +219,10 @@ const (
 	// AnnImmediateBinding provides a const to indicate whether immediate binding should be performed on the PV (overrides global config)
 	AnnImmediateBinding = AnnAPIGroup + "/storage.bind.immediate.requested"
 
+	// AnnSelectedNode annotation is added to a PVC that has been triggered by scheduler to
+	// be dynamically provisioned. Its value is the name of the selected node.
+	AnnSelectedNode = "volume.kubernetes.io/selected-node"
+
 	// CloneUniqueID is used as a special label to be used when we search for the pod
 	CloneUniqueID = "cdi.kubevirt.io/storage.clone.cloneUniqeId"
 
@@ -571,10 +572,10 @@ func GetPriorityClass(pvc *v1.PersistentVolumeClaim) string {
 	return anno[AnnPriorityClassName]
 }
 
-// GetSelectedNodename gets the node name specified in a PVC for dynamic provisioning
-func GetSelectedNodename(pvc *v1.PersistentVolumeClaim) string {
+// GetSelectedNodeName gets the node name specified in a PVC for dynamic provisioning
+func GetSelectedNodeName(pvc *v1.PersistentVolumeClaim) string {
 	anno := pvc.GetAnnotations()
-	return anno[AnnSelectedNodeName]
+	return anno[AnnSelectedNode]
 }
 
 // ShouldDeletePod returns whether the PVC workload pod should be deleted

pkg/controller/import-controller.go

@@ -523,7 +523,7 @@ func (r *ImportReconciler) createImporterPod(pvc *corev1.PersistentVolumeClaim)
 		scratchPvcName:    scratchPvcName,
 		vddkImageName:     vddkImageName,
 		priorityClassName: cc.GetPriorityClass(pvc),
-		selectedNodeName:  cc.GetSelectedNodename(pvc),
+		selectedNodeName:  cc.GetSelectedNodeName(pvc),
 	}
 
 	pod, err := createImporterPod(r.log, r.client, podArgs, r.installerLabels)

pkg/controller/populators/import-populator.go

@@ -1,3 +1,19 @@
+/*
+Copyright 2023 The CDI Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
 package populators
 
 import (
@@ -131,8 +147,8 @@ func (r *ImportPopulatorReconciler) reconcileTargetPVC(pvc, pvcPrime *corev1.Per
 // Import-specific implementation of updateAnnotations
 func (r *ImportPopulatorReconciler) updateAnnotations(pvc *corev1.PersistentVolumeClaim, source client.Object) {
 	importSource := source.(*cdiv1.ImportSource)
-
 	annotations := pvc.Annotations
+	annotations[AnnPopulatorKind] = importKind
 	annotations[cc.AnnContentType] = cc.GetContentType(string(importSource.Spec.ContentType))
 	annotations[cc.AnnPreallocationRequested] = strconv.FormatBool(cc.GetPreallocation(r.client, importSource.Spec.Preallocation))
 

pkg/controller/populators/populator-base.go

@@ -1,8 +1,23 @@
+/*
+Copyright 2023 The CDI Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
 package populators
 
 import (
 	"context"
-	"time"
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
@@ -39,6 +54,10 @@ type ReconcilerBase struct {
 	installerLabels map[string]string
 }
 
+const (
+	dataSourceRefField = "spec.dataSourceRef"
+)
+
 // Interface to store populator-specific methods
 type populatorController interface {
 	// Returns the specific populator CR
@@ -69,25 +88,9 @@ func addCommonPopulatorsWatches(mgr manager.Manager, c controller.Controller, lo
 		return err
 	}
 
-	const dataSourceRefField = "spec.dataSourceRef"
-
-	indexKeyFunc := func(namespace, name string) string {
-		return namespace + "/" + name
-	}
-
-	if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &corev1.PersistentVolumeClaim{}, dataSourceRefField, func(obj client.Object) []string {
-		pvc := obj.(*corev1.PersistentVolumeClaim)
-		if IsPVCDataSourceRefKind(pvc, sourceKind) && pvc.Spec.DataSourceRef.Name != "" {
-			return []string{indexKeyFunc(obj.GetNamespace(), pvc.Spec.DataSourceRef.Name)}
-		}
-		return nil
-	}); err != nil {
-		return err
-	}
-
 	mapDataSourceRefToPVC := func(obj client.Object) (reqs []reconcile.Request) {
 		var pvcs corev1.PersistentVolumeClaimList
-		matchingFields := client.MatchingFields{dataSourceRefField: indexKeyFunc(obj.GetNamespace(), obj.GetName())}
+		matchingFields := client.MatchingFields{dataSourceRefField: getPopulatorIndexKey(obj.GetNamespace(), sourceKind, obj.GetName())}
 		if err := mgr.GetClient().List(context.TODO(), &pvcs, matchingFields); err != nil {
 			log.Error(err, "Unable to list PVCs", "matchingFields", matchingFields)
 			return reqs
@@ -107,6 +110,22 @@ func addCommonPopulatorsWatches(mgr manager.Manager, c controller.Controller, lo
 	return nil
 }
 
+// CreateCommonPopulatorIndexes creates indexes used by all populators
+func CreateCommonPopulatorIndexes(mgr manager.Manager) error {
+	if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &corev1.PersistentVolumeClaim{}, dataSourceRefField, func(obj client.Object) []string {
+		pvc := obj.(*corev1.PersistentVolumeClaim)
+		dataSourceRef := pvc.Spec.DataSourceRef
+		if dataSourceRef != nil && dataSourceRef.APIGroup != nil &&
+			*dataSourceRef.APIGroup == cc.AnnAPIGroup && dataSourceRef.Name != "" {
+			return []string{getPopulatorIndexKey(obj.GetNamespace(), pvc.Spec.DataSourceRef.Kind, pvc.Spec.DataSourceRef.Name)}
+		}
+		return nil
+	}); err != nil {
+		return err
+	}
+	return nil
+}
+
 func (r *ReconcilerBase) getPVCPrime(pvc *corev1.PersistentVolumeClaim) (*corev1.PersistentVolumeClaim, error) {
 	pvcPrime := &corev1.PersistentVolumeClaim{}
 	pvcPrimeKey := types.NamespacedName{Namespace: pvc.Namespace, Name: PVCPrimeName(pvc)}
@@ -148,7 +167,7 @@ func (r *ReconcilerBase) handleStorageClass(pvc *corev1.PersistentVolumeClaim) (
 
 	if storageClass.VolumeBindingMode != nil && *storageClass.VolumeBindingMode == storagev1.VolumeBindingWaitForFirstConsumer {
 		waitForFirstConsumer = true
-		nodeName := pvc.Annotations[cc.AnnSelectedNodeName]
+		nodeName := pvc.Annotations[cc.AnnSelectedNode]
 		if nodeName == "" {
 			// Wait for the PVC to get a node name before continuing
 			return false, waitForFirstConsumer, nil
@@ -165,7 +184,7 @@ func (r *ReconcilerBase) createPVCPrime(pvc *corev1.PersistentVolumeClaim, sourc
 	annotations := make(map[string]string)
 	annotations[cc.AnnImmediateBinding] = ""
 	if waitForFirstConsumer {
-		annotations[cc.AnnSelectedNodeName] = pvc.Annotations[cc.AnnSelectedNodeName]
+		annotations[cc.AnnSelectedNode] = pvc.Annotations[cc.AnnSelectedNode]
 	}
 	pvcPrimeName := PVCPrimeName(pvc)
 	pvcPrime := &corev1.PersistentVolumeClaim{
@@ -308,11 +327,6 @@ func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, popu
 
 func (r *ReconcilerBase) reconcileCleanup(pvcPrime *corev1.PersistentVolumeClaim) (reconcile.Result, error) {
 	if pvcPrime != nil {
-		// Wait for the bind controller to rebind the PV
-		if pvcPrime.Status.Phase != corev1.ClaimLost {
-			return reconcile.Result{RequeueAfter: 2 * time.Second}, nil
-		}
-
 		if err := r.client.Delete(context.TODO(), pvcPrime); err != nil {
 			return reconcile.Result{}, err
 		}

pkg/controller/populators/util.go

@@ -1,3 +1,19 @@
+/*
+Copyright 2023 The CDI Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
 package populators
 
 import (
@@ -20,16 +36,21 @@ const (
 	// createdPVCPrimeSuccessfully provides a const to indicate we created PVC prime for population
 	createdPVCPrimeSuccessfully = "CreatedPVCPrimeSuccessfully"
 
-	// AnnSelectedNode annotation is added to a PVC that has been triggered by scheduler to
-	// be dynamically provisioned. Its value is the name of the selected node.
-	AnnSelectedNode = "volume.kubernetes.io/selected-node"
+	// AnnPopulatorKind annotation is added to the PVC' to specify the population kind, so it's later
+	// checked by the common populator watches.
+	AnnPopulatorKind = cc.AnnAPIGroup + "/storage.populator.kind"
 
 	// annMigratedTo annotation is added to a PVC and PV that is supposed to be
 	// dynamically provisioned/deleted by by its corresponding CSI driver
 	// through the CSIMigration feature flags. When this annotation is set the
 	// Kubernetes components will "stand-down" and the external-provisioner will
 	// act on the objects
 	annMigratedTo = "pv.kubernetes.io/migrated-to"
+
+	// importKind is used to specify that the PVC' is meant to be populated by the import flow
+	importKind = "import"
+	// uploadKind is used to specify that the PVC' is meant to be populated by the upload flow
+	uploadKind = "upload"
 )
 
 // IsPVCDataSourceRefKind returns if the PVC has DataSourceRef that
@@ -44,15 +65,14 @@ func isPVCPrimeDataSourceRefKind(pvc *corev1.PersistentVolumeClaim, kind string)
 	if owner == nil || owner.Kind != "PersistentVolumeClaim" {
 		return false
 	}
-	_, ok := pvc.Annotations[cc.AnnUploadRequest]
-	if ok {
-		return kind == cdiv1.UploadSourceRef
-	}
-	// TODO: Maybe use a more descriptive annotation?
-	_, ok = pvc.Annotations[cc.AnnSource]
-	if ok {
+	populatorKind, _ := pvc.Annotations[AnnPopulatorKind]
+	switch populatorKind {
+	case importKind:
 		return kind == cdiv1.ImportSourceRef
+	case uploadKind:
+		return kind == cdiv1.UploadSourceRef
 	}
+
 	return false
 }
 
@@ -61,6 +81,10 @@ func PVCPrimeName(targetPVC *corev1.PersistentVolumeClaim) string {
 	return fmt.Sprintf("%s-%s", primePvcPrefix, targetPVC.UID)
 }
 
+func getPopulatorIndexKey(namespace, kind, name string) string {
+	return namespace + "/" + kind + "/" + name
+}
+
 func isPVCOwnedByDataVolume(pvc *corev1.PersistentVolumeClaim) bool {
 	owner := metav1.GetControllerOf(pvc)
 	return (owner != nil && owner.Kind == "DataVolume") || cc.HasAnnOwnedByDataVolume(pvc)