Fixed review comments

Author: sseago

kubevirt-datamover-plugin/pvc/backup.go

@@ -93,11 +93,7 @@ func (p *BackupPlugin) Execute(
 
 	p.Log.Infof("[pvc-backup] Processing PersistentVolumeClaim %s/%s", pvc.Namespace, pvc.Name)
 	// Get VMs for this volume, if any
-	crClient, err := clients.CRClient()
-	if err != nil {
-		return item, nil, "", nil, fmt.Errorf("failed to get controller-runtime client: %w", err)
-	}
-	pvcs, err := p.getPVCList(crClient, pvc.Namespace)
+	pvcs, err := p.getPVCList(p.crClient, pvc.Namespace)
 	if err != nil {
 		return item, nil, "", nil, fmt.Errorf("failed to get PVC list: %w", err)
 	}
@@ -110,7 +106,7 @@ func (p *BackupPlugin) Execute(
 	vmNames := pvcs[pvc.Name]
 	for _, vmName := range vmNames {
 		vm := new(kvcore.VirtualMachine)
-		err := crClient.Get(context.Background(), crclient.ObjectKey{Name: vmName, Namespace: pvc.Namespace}, vm)
+		err := p.crClient.Get(context.Background(), crclient.ObjectKey{Name: vmName, Namespace: pvc.Namespace}, vm)
 		if err != nil {
 			return item, nil, "", nil, fmt.Errorf("failed to get VM %s: %w", vmName, err)
 		}

kubevirt-datamover-plugin/pvc/backup_test.go

@@ -306,7 +306,6 @@ func createTestPV(name string, pvc *corev1.PersistentVolumeClaim) *corev1.Persis
 }
 func getFakeClient() crclient.Client {
 	scheme := runtime.NewScheme()
-	//_ = velerov2alpha1.AddToScheme(scheme)
 	_ = kvcore.AddToScheme(scheme)
 	builder := fake.NewClientBuilder().
 		WithScheme(scheme)

kubevirt-datamover-plugin/vm/backup.go

@@ -146,7 +146,7 @@ func (p *BackupPlugin) Execute(
 	p.Log.Infof("[vm-backup] Generated operation ID: %s", operationID)
 
 	// Get the first PVC with skip volume policy for SourcePVC (kubevirt datamover trigger)
-	sourcePVC, err := p.getFirstKubevirtPVC(vm, backup)
+	sourcePVC, err := p.getFirstKubevirtPVC(vm, backup, vh)
 	if err != nil {
 		return nil, nil, "", nil, fmt.Errorf("failed to get source PVC: %w", err)
 	}
@@ -305,11 +305,8 @@ func CheckPreconditions(vm *kvcore.VirtualMachine, backup *velerov1.Backup, log
 	}
 
 	// Check 4: Volume policy check
-	// - At least one PVC must have "skip" action (triggers kubevirt datamover)
+	// - At least one PVC must have "custom" action with "datamover=kubevirt" parameter (triggers kubevirt datamover)
 	// - No PVC can have "snapshot" action (would conflict with kubevirt datamover)
-	// TODO(https://github.com/migtools/kubevirt-datamover-plugin/issues/4): Once upstream
-	// Velero changes are merged, update this to check for "custom" action type and inspect
-	// the action parameters map to verify it's for kubevirt.
 	hasKubevirtPolicy, hasConflictingPolicy, err := checkVolumePolicies(vm, backup, log, vh)
 	if err != nil {
 		return false, "", fmt.Errorf("failed to check volume policies: %w", err)
@@ -416,7 +413,7 @@ func checkVolumePolicies(vm *kvcore.VirtualMachine, backup *velerov1.Backup, log
 // Velero changes merge, update this to return the first PVC with "custom" action type
 // and kubevirt-specific parameter set. The volumehelper API will need to expose the
 // actual action type (not just shouldSnapshot boolean) for this check.
-func (p *BackupPlugin) getFirstKubevirtPVC(vm *kvcore.VirtualMachine, backup *velerov1.Backup) (*corev1.PersistentVolumeClaim, error) {
+func (p *BackupPlugin) getFirstKubevirtPVC(vm *kvcore.VirtualMachine, backup *velerov1.Backup, vh vhutil.VolumeHelper) (*corev1.PersistentVolumeClaim, error) {
 	pvcNames := controllercommon.GetVolumesForVm(vm)
 	if len(pvcNames) == 0 {
 		return nil, fmt.Errorf("no PVCs found for VirtualMachine %s/%s", vm.Namespace, vm.Name)
@@ -427,11 +424,6 @@ func (p *BackupPlugin) getFirstKubevirtPVC(vm *kvcore.VirtualMachine, backup *ve
 		return nil, fmt.Errorf("failed to get core client: %w", err)
 	}
 
-	crClient, err := clients.CRClient()
-	if err != nil {
-		return nil, fmt.Errorf("failed to get controller-runtime client: %w", err)
-	}
-
 	for _, pvcName := range pvcNames {
 		pvc, err := coreClient.PersistentVolumeClaims(vm.Namespace).Get(context.Background(), pvcName, metav1.GetOptions{})
 		if err != nil {
@@ -451,20 +443,18 @@ func (p *BackupPlugin) getFirstKubevirtPVC(vm *kvcore.VirtualMachine, backup *ve
 		}
 		pvcUnstructured := &unstructured.Unstructured{Object: pvcMap}
 
-		// Check if this PVC has snapshot policy
-		shouldSnapshot, err := volumehelper.ShouldPerformSnapshotWithBackup(
+		// Check if this PVC has custom kubevirt datamover policy
+		shouldUseKubevirtDm, err := vh.ShouldPerformCustomAction(
 			pvcUnstructured,
 			kuberesource.PersistentVolumeClaims,
-			*backup,
-			crClient,
-			p.Log,
+			kubevirtCustomPolicy,
 		)
 		if err != nil {
 			return nil, fmt.Errorf("failed to check volume policy for PVC %s: %w", pvcName, err)
 		}
 
-		// Return first PVC without snapshot policy
-		if !shouldSnapshot {
+		// Return first PVC matching kubevirt dm custom policy action
+		if shouldUseKubevirtDm {
 			return pvc, nil
 		}
 	}

kubevirt-datamover-plugin/vm/backup_test.go

@@ -490,7 +490,7 @@ func TestBackupPlugin_checkPreconditions(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			vh, err := plugin.pluginPVCPodCache.GetOrCreateVolumeHelper(tc.backup, getFakeClient(), plugin.Log)
+			vh, err := plugin.pluginPVCPodCache.GetOrCreateVolumeHelper(tc.backup, plugin.crClient, plugin.Log)
 			require.NoError(t, err)
 			eligible, reason, err := CheckPreconditions(tc.vm, tc.backup, plugin.Log, vh)
 			require.NoError(t, err)