addressed review comments

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

pkg/controller/clone/common.go

@@ -26,35 +26,6 @@ func IsDataSourcePVC(kind string) bool {
 	return kind == "PersistentVolumeClaim"
 }
 
-// IsDataSourceVolumeClone checks for VolumeCloneSourceRef source kind
-func IsDataSourceVolumeClone(dataSource *corev1.TypedObjectReference) bool {
-	return dataSource != nil && dataSource.Kind == cdiv1.VolumeCloneSourceRef
-}
-
-// GetVolumeCloneSource returns the VolumeCloneSource dataSourceRef
-func GetVolumeCloneSource(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (*cdiv1.VolumeCloneSource, error) {
-	if !IsDataSourceVolumeClone(pvc.Spec.DataSourceRef) {
-		return nil, fmt.Errorf("invalid dataSourceRef")
-	}
-
-	ns := pvc.Namespace
-	if pvc.Spec.DataSourceRef.Namespace != nil {
-		ns = *pvc.Spec.DataSourceRef.Namespace
-	}
-
-	obj := &cdiv1.VolumeCloneSource{}
-	exists, err := getResource(ctx, c, ns, pvc.Spec.DataSourceRef.Name, obj)
-	if err != nil {
-		return nil, err
-	}
-
-	if !exists {
-		return nil, nil
-	}
-
-	return obj, nil
-}
-
 // AddCommonLabels adds common labels to a resource
 func AddCommonLabels(obj metav1.Object) {
 	if obj.GetLabels() == nil {
@@ -144,20 +115,6 @@ func GetStorageClassForClaim(ctx context.Context, c client.Client, pvc *corev1.P
 	return nil, nil
 }
 
-// MustGetStorageClassForClaim returns the storageclass for a PVC
-func MustGetStorageClassForClaim(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (*storagev1.StorageClass, error) {
-	sc, err := GetStorageClassForClaim(ctx, c, pvc)
-	if err != nil {
-		return nil, err
-	}
-
-	if sc == nil {
-		return nil, fmt.Errorf("no storageclass for pvc")
-	}
-
-	return sc, nil
-}
-
 // GetDriverFromVolume returns the CSI driver name for a PVC
 func GetDriverFromVolume(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (*string, error) {
 	if pvc.Spec.VolumeName == "" {
@@ -186,7 +143,7 @@ func GetDriverFromVolume(ctx context.Context, c client.Client, pvc *corev1.Persi
 	return &pv.Spec.CSI.Driver, nil
 }
 
-// GetCompatibleVolumeSnapshotClass returns a VolumeSNapshotClass name that works for all PVCs
+// GetCompatibleVolumeSnapshotClass returns a VolumeSnapshotClass name that works for all PVCs
 func GetCompatibleVolumeSnapshotClass(ctx context.Context, c client.Client, pvcs ...*corev1.PersistentVolumeClaim) (*string, error) {
 	var drivers []string
 	for _, pvc := range pvcs {
@@ -293,8 +250,8 @@ func checkQuotaExceeded(r record.EventRecorder, owner client.Object, err error)
 	}
 }
 
-func claimBoundOrWFFC(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (bool, error) {
-	if pvc.Spec.VolumeName != "" {
+func IsClaimBoundOrWFFC(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (bool, error) {
+	if cc.IsBound(pvc) {
 		return true, nil
 	}
 

pkg/controller/clone/csi-clone.go

@@ -59,7 +59,7 @@ func (p *CSIClonePhase) Reconcile(ctx context.Context) (*reconcile.Result, error
 		}
 	}
 
-	done, err := claimBoundOrWFFC(ctx, p.Client, pvc)
+	done, err := IsClaimBoundOrWFFC(ctx, p.Client, pvc)
 	if err != nil {
 		return nil, err
 	}

pkg/controller/clone/planner.go

@@ -25,6 +25,20 @@ import (
 	"kubevirt.io/containerized-data-importer/pkg/util"
 )
 
+const (
+	// CloneValidationFailed reports that a clone wasn't admitted by our validation mechanism (reason)
+	CloneValidationFailed = "CloneValidationFailed"
+
+	// MessageCloneValidationFailed reports that a clone wasn't admitted by our validation mechanism (message)
+	MessageCloneValidationFailed = "The clone doesn't meet the validation requirements"
+
+	// CloneWithoutSource reports that the source of a clone doesn't exists (reason)
+	CloneWithoutSource = "CloneWithoutSource"
+
+	// MessageCloneWithoutSource reports that the source of a clone doesn't exists (message)
+	MessageCloneWithoutSource = "The source %s %s doesn't exist"
+)
+
 // Planner plans clone operations
 type Planner struct {
 	RootObjectType  client.ObjectList
@@ -42,7 +56,7 @@ type Planner struct {
 	watchMutex        sync.Mutex
 }
 
-// Phase is the interface implemeented by all clone phases
+// Phase is the interface implemented by all clone phases
 type Phase interface {
 	Name() string
 	Reconcile(context.Context) (*reconcile.Result, error)
@@ -96,7 +110,7 @@ type ChooseStrategyArgs struct {
 func (p *Planner) ChooseStrategy(ctx context.Context, args *ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) {
 	if IsDataSourcePVC(args.DataSource.Spec.Source.Kind) {
 		args.Log.V(3).Info("Getting strategy for PVC source")
-		return p.strategyForSourcePVC(ctx, args)
+		return p.computeStrategyForSourcePVC(ctx, args)
 	}
 
 	return nil, fmt.Errorf("unsupported datasource")
@@ -217,7 +231,7 @@ func (p *Planner) watchOwned(log logr.Logger, obj client.Object) error {
 	return nil
 }
 
-func (p *Planner) strategyForSourcePVC(ctx context.Context, args *ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) {
+func (p *Planner) computeStrategyForSourcePVC(ctx context.Context, args *ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) {
 	if ok, err := p.validateTargetStorageClassAssignment(ctx, args); !ok || err != nil {
 		return nil, err
 	}
@@ -229,21 +243,14 @@ func (p *Planner) strategyForSourcePVC(ctx context.Context, args *ChooseStrategy
 	}
 
 	if !exists {
-		// TODO EVENT
-		//Event{
-		//	eventType: corev1.EventTypeWarning,
-		//	reason:    CloneWithoutSource,
-		//	message:   fmt.Sprintf(MessageCloneWithoutSource, "pvc", datavolume.Spec.Source.PVC.Name),
-		//})
-
+		message := fmt.Sprintf(MessageCloneWithoutSource, "pvc", args.DataSource.Spec.Source.Name)
+		p.Recorder.Event(args.TargetClaim, corev1.EventTypeWarning, CloneWithoutSource, message)
 		args.Log.V(3).Info("Source PVC does not exist, cannot compute strategy")
 		return nil, nil
 	}
 
 	if err = p.validateSourcePVC(args, sourceClaim); err != nil {
-		// TODO EVENT
-		//r.recorder.Event(datavolume, corev1.EventTypeWarning, CloneValidationFailed, MessageCloneValidationFailed)
-		//return false, err
+		p.Recorder.Event(args.TargetClaim, corev1.EventTypeWarning, CloneValidationFailed, MessageCloneValidationFailed)
 		args.Log.V(1).Info("Validation failed", "target", args.TargetClaim, "source", sourceClaim)
 		return nil, err
 	}
@@ -309,11 +316,16 @@ func (p *Planner) validateTargetStorageClassAssignment(ctx context.Context, args
 		return false, fmt.Errorf("claim has emptystring storageclass, will not work")
 	}
 
-	_, err := MustGetStorageClassForClaim(ctx, p.Client, args.TargetClaim)
+	sc, err := GetStorageClassForClaim(ctx, p.Client, args.TargetClaim)
 	if err != nil {
 		return false, err
 	}
 
+	if sc == nil {
+		args.Log.V(3).Info("Target PVC has no storage class, cannot compute strategy")
+		return false, fmt.Errorf("target storage class not found")
+	}
+
 	return true, nil
 }
 
@@ -331,11 +343,16 @@ func (p *Planner) validateAdvancedClonePVC(ctx context.Context, args *ChooseStra
 		return false, nil
 	}
 
-	sc, err := MustGetStorageClassForClaim(ctx, p.Client, args.TargetClaim)
+	sc, err := GetStorageClassForClaim(ctx, p.Client, args.TargetClaim)
 	if err != nil {
 		return false, err
 	}
 
+	if sc == nil {
+		args.Log.V(3).Info("target storage class not found")
+		return false, fmt.Errorf("target storage class not found")
+	}
+
 	srcCapacity, hasSrcCapacity := sourceClaim.Status.Capacity[corev1.ResourceStorage]
 	targetRequest, hasTargetRequest := args.TargetClaim.Spec.Resources.Requests[corev1.ResourceStorage]
 	allowExpansion := sc.AllowVolumeExpansion != nil && *sc.AllowVolumeExpansion

pkg/controller/clone/planner_test.go

@@ -18,6 +18,7 @@ package clone
 
 import (
 	"context"
+	"strings"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -180,6 +181,18 @@ var _ = Describe("Planner test", func() {
 		}
 	}
 
+	expectEvent := func(planner *Planner, event string) {
+		close(planner.Recorder.(*record.FakeRecorder).Events)
+		found := false
+		for e := range planner.Recorder.(*record.FakeRecorder).Events {
+			if strings.Contains(e, event) {
+				found = true
+			}
+		}
+		planner.Recorder = nil
+		Expect(found).To(BeTrue())
+	}
+
 	Context("ChooseStrategy tests", func() {
 
 		It("should error if unsupported kind", func() {
@@ -233,7 +246,7 @@ var _ = Describe("Planner test", func() {
 			planner := createPlanner()
 			strategy, err := planner.ChooseStrategy(context.Background(), args)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(Equal("no storageclass for pvc"))
+			Expect(err.Error()).To(Equal("target storage class not found"))
 			Expect(strategy).To(BeNil())
 		})
 
@@ -247,6 +260,7 @@ var _ = Describe("Planner test", func() {
 			strategy, err := planner.ChooseStrategy(context.Background(), args)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(strategy).To(BeNil())
+			expectEvent(planner, CloneWithoutSource)
 		})
 
 		It("should fail target smaller", func() {
@@ -263,6 +277,7 @@ var _ = Describe("Planner test", func() {
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(Equal("target resources requests storage size is smaller than the source"))
 			Expect(strategy).To(BeNil())
+			expectEvent(planner, CloneValidationFailed)
 		})
 
 		It("should return host assisted with no volumesnapshotclass", func() {

pkg/controller/clone/prep-claim.go

@@ -66,7 +66,7 @@ func (p *PrepClaimPhase) Reconcile(ctx context.Context) (*reconcile.Result, erro
 	p.Log.V(3).Info("Expand sizes", "req", requestedSize, "cur", currentSize, "act", actualSize)
 
 	if !hasActual {
-		if actualClaim.Spec.VolumeName != "" {
+		if cc.IsBound(actualClaim) {
 			return nil, fmt.Errorf("actual PVC size missing")
 		}
 

pkg/controller/clone/snap-clone.go

@@ -66,7 +66,7 @@ func (p *SnapshotClonePhase) Reconcile(ctx context.Context) (*reconcile.Result,
 		}
 	}
 
-	done, err := claimBoundOrWFFC(ctx, p.Client, pvc)
+	done, err := IsClaimBoundOrWFFC(ctx, p.Client, pvc)
 	if err != nil {
 		return nil, err
 	}

pkg/controller/common/util.go

@@ -1301,9 +1301,14 @@ func InflateSizeWithOverhead(ctx context.Context, c client.Client, imgSize int64
 	return returnSize, nil
 }
 
+// IsBound returns if the pvc is bound
+func IsBound(pvc *corev1.PersistentVolumeClaim) bool {
+	return pvc.Spec.VolumeName != ""
+}
+
 // IsUnbound returns if the pvc is not bound yet
 func IsUnbound(pvc *corev1.PersistentVolumeClaim) bool {
-	return pvc.Spec.VolumeName == ""
+	return !IsBound(pvc)
 }
 
 // IsImageStream returns true if registry source is ImageStream