[oadp-1.5] updates for 1.4 compat

internal/controller/garbagecollector_controller.go

@@ -29,6 +29,7 @@
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -278,6 +279,6 @@
 		WithLogConstructor(func(_ *reconcile.Request) logr.Logger {
 			return logr.New(ctrl.Log.GetSink().WithValues("controller", "nonadmingarbagecollector"))
 		}).
-		WatchesRawSource(&source.PeriodicalSource{Frequency: r.Frequency}).
+		WatchesRawSource(&source.PeriodicalSource{Frequency: r.Frequency}, &handler.EnqueueRequestForObject{}).
 		Complete(r)
 }

internal/controller/garbagecollector_controller_test.go

@@ -27,11 +27,9 @@ import (
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/config"
 
 	nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1"
 	"github.com/migtools/oadp-non-admin/internal/common/constant"
@@ -148,9 +146,6 @@ var _ = ginkgo.Describe("Test full reconcile loop of GarbageCollector Controller
 			gomega.Expect(restoresInOADPNamespace.Items).To(gomega.HaveLen(scenario.restores + scenario.orphanRestores))
 
 			k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
-				Controller: config.Controller{
-					SkipNameValidation: ptr.To(true),
-				},
 				Scheme: k8sClient.Scheme(),
 				Cache: cache.Options{
 					DefaultNamespaces: map[string]cache.Config{

internal/controller/nonadminbackup_controller.go

@@ -34,6 +34,7 @@
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/util/retry"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -194,7 +195,7 @@
 		},
 	)
 	if updatedPhase || updatedCondition {
-		if err := r.Status().Update(ctx, nab); err != nil {
+		if err := r.updateStatusWithRetry(ctx, logger, nab); err != nil {
 			logger.Error(err, statusUpdateError)
 			return false, err
 		}
@@ -238,7 +239,7 @@
 		},
 	)
 	if updatedPhase || updatedCondition {
-		if err := r.Status().Update(ctx, nab); err != nil {
+		if err := r.updateStatusWithRetry(ctx, logger, nab); err != nil {
 			logger.Error(err, statusUpdateError)
 			return false, err
 		}
@@ -350,7 +351,7 @@
 	// Status will be applied based on the current state of the DeleteBackupRequest.
 	updated := updateNonAdminBackupDeleteBackupRequestStatus(&nab.Status, deleteBackupRequest)
 	if updated {
-		if err := r.Status().Update(ctx, nab); err != nil {
+		if err := r.updateStatusWithRetry(ctx, logger, nab); err != nil {
 			logger.Error(err, "Failed to update NonAdminBackup Status after DeleteBackupRequest reconciliation")
 			return false, err
 		}
@@ -457,9 +458,10 @@
 func (r *NonAdminBackupReconciler) removeNabFinalizerUponVeleroBackupDeletion(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) {
 	logger.V(1).Info("VeleroBackup deleted, removing NonAdminBackup finalizer")
 
+	patch := client.MergeFrom(nab.DeepCopy())
 	controllerutil.RemoveFinalizer(nab, constant.NabFinalizerName)
 
-	if err := r.Update(ctx, nab); err != nil {
+	if err := r.Patch(ctx, nab, patch); err != nil {
 		logger.Error(err, "Failed to remove finalizer from NonAdminBackup")
 		return false, err
 	}
@@ -490,7 +492,7 @@
 
 	// Set phase to New
 	if updated := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminPhaseNew); updated {
-		if err := r.Status().Update(ctx, nab); err != nil {
+		if err := r.updateStatusWithRetry(ctx, logger, nab); err != nil {
 			logger.Error(err, statusUpdateError)
 			return false, err
 		}
@@ -527,7 +529,7 @@
 			},
 		)
 		if updatedPhase || updatedCondition {
-			if updateErr := r.Status().Update(ctx, nab); updateErr != nil {
+			if updateErr := r.updateStatusWithRetry(ctx, logger, nab); updateErr != nil {
 				logger.Error(updateErr, statusUpdateError)
 				return false, updateErr
 			}
@@ -548,7 +550,7 @@
 		},
 	)
 	if updated {
-		if err := r.Status().Update(ctx, nab); err != nil {
+		if err := r.updateStatusWithRetry(ctx, logger, nab); err != nil {
 			logger.Error(err, statusUpdateError)
 			return false, err
 		}
@@ -590,7 +592,7 @@
 			Namespace: r.OADPNamespace,
 			Name:      veleroBackupNACUUID,
 		}
-		if err := r.Status().Update(ctx, nab); err != nil {
+		if err := r.updateStatusWithRetry(ctx, logger, nab); err != nil {
 			logger.Error(err, statusUpdateError)
 			return false, err
 		}
@@ -606,8 +608,9 @@
 	// to ensure we won't risk having orphant Velero Backup resource, due to an unexpected error
 	// while adding finalizer after creatign Velero Backup
 	if !controllerutil.ContainsFinalizer(nab, constant.NabFinalizerName) {
+		patch := client.MergeFrom(nab.DeepCopy())
 		controllerutil.AddFinalizer(nab, constant.NabFinalizerName)
-		if err := r.Update(ctx, nab); err != nil {
+		if err := r.Patch(ctx, nab, patch); err != nil {
 			logger.Error(err, "Failed to add finalizer")
 			return false, err
 		}
@@ -663,7 +666,7 @@
 				},
 			)
 			if updatedPhase || updatedCondition {
-				if updateErr := r.Status().Update(ctx, nab); updateErr != nil {
+				if updateErr := r.updateStatusWithRetry(ctx, logger, nab); updateErr != nil {
 					logger.Error(updateErr, nonAdminRestoreStatusUpdateFailureMessage)
 					return false, updateErr
 				}
@@ -798,7 +801,7 @@
 	updatedDataUploadStatus := updateNonAdminBackupDataUploadStatus(&nab.Status, dataUploads)
 
 	if updated || updatedPhase || updatedCondition || updatedQueueInfo || updatedPodVolumeBackupStatus || updatedDataUploadStatus {
-		if err := r.Status().Update(ctx, nab); err != nil {
+		if err := r.updateStatusWithRetry(ctx, logger, nab); err != nil {
 			logger.Error(err, statusUpdateError)
 			return false, err
 		}
@@ -810,6 +813,40 @@
 	return false, nil
 }
 
+// updateStatusWithRetry updates the NonAdminBackup status with retry logic to handle resource version conflicts
+func (r *NonAdminBackupReconciler) updateStatusWithRetry(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) error {
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		// Always fetch the latest version before updating status
+		current := &nacv1alpha1.NonAdminBackup{}
+		if err := r.Get(ctx, client.ObjectKeyFromObject(nab), current); err != nil {
+			return err
+		}
+
+		// Preserve existing conditions and merge in new status updates
+		// This prevents losing conditions that were set by other reconcile steps
+		current.Status.Phase = nab.Status.Phase
+		current.Status.VeleroBackup = nab.Status.VeleroBackup
+		current.Status.QueueInfo = nab.Status.QueueInfo
+		current.Status.VeleroDeleteBackupRequest = nab.Status.VeleroDeleteBackupRequest
+		current.Status.DataMoverDataUploads = nab.Status.DataMoverDataUploads
+		current.Status.FileSystemPodVolumeBackups = nab.Status.FileSystemPodVolumeBackups
+
+		// Merge conditions instead of replacing them
+		for _, newCondition := range nab.Status.Conditions {
+			meta.SetStatusCondition(&current.Status.Conditions, newCondition)
+		}
+
+		// Attempt status update on fresh resource version
+		if err := r.Status().Update(ctx, current); err != nil {
+			logger.V(1).Info("Status update conflict, retrying...", "error", err.Error())
+			return err
+		}
+
+		logger.V(1).Info("Status update successful")
+		return nil
+	})
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *NonAdminBackupReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
@@ -832,16 +869,16 @@
 			},
 		}).
 		// handler runs after predicate
 		Watches(&velerov1.Backup{}, &handler.VeleroBackupHandler{}).
 		Watches(&velerov1.Backup{}, &handler.VeleroBackupQueueHandler{
 			Client:        r.Client,
 			OADPNamespace: r.OADPNamespace,
 		}).
 		Watches(&velerov1.PodVolumeBackup{}, &handler.VeleroPodVolumeBackupHandler{
 			Client:        r.Client,
 			OADPNamespace: r.OADPNamespace,
 		}).
 		Watches(&velerov2alpha1.DataUpload{}, &handler.VeleroDataUploadHandler{
 			Client:        r.Client,
 			OADPNamespace: r.OADPNamespace,
 		}).

internal/controller/nonadminbackup_controller_test.go

@@ -36,7 +36,6 @@ import (
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
-	"sigs.k8s.io/controller-runtime/pkg/config"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -123,21 +122,27 @@ func checkTestNonAdminBackupStatus(nonAdminBackup *nacv1alpha1.NonAdminBackup, e
 		}
 	}
 
-	if len(nonAdminBackup.Status.Conditions) != len(expectedStatus.Conditions) {
-		return fmt.Errorf("NonAdminBackup Status has %v Condition(s), expected to have %v", len(nonAdminBackup.Status.Conditions), len(expectedStatus.Conditions))
-	}
-	for index := range nonAdminBackup.Status.Conditions {
-		if nonAdminBackup.Status.Conditions[index].Type != expectedStatus.Conditions[index].Type {
-			return fmt.Errorf("NonAdminBackup Status Conditions [%v] Type %v is not equal to expected %v", index, nonAdminBackup.Status.Conditions[index].Type, expectedStatus.Conditions[index].Type)
-		}
-		if nonAdminBackup.Status.Conditions[index].Status != expectedStatus.Conditions[index].Status {
-			return fmt.Errorf("NonAdminBackup Status Conditions [%v] Status %v is not equal to expected %v", index, nonAdminBackup.Status.Conditions[index].Status, expectedStatus.Conditions[index].Status)
-		}
-		if nonAdminBackup.Status.Conditions[index].Reason != expectedStatus.Conditions[index].Reason {
-			return fmt.Errorf("NonAdminBackup Status Conditions [%v] Reason %v is not equal to expected %v", index, nonAdminBackup.Status.Conditions[index].Reason, expectedStatus.Conditions[index].Reason)
+	// Check that all expected conditions are present with correct values
+	// More robust than exact array matching - allows for additional conditions or different ordering
+	for _, expectedCondition := range expectedStatus.Conditions {
+		found := false
+		for _, actualCondition := range nonAdminBackup.Status.Conditions {
+			if actualCondition.Type == expectedCondition.Type {
+				found = true
+				if actualCondition.Status != expectedCondition.Status {
+					return fmt.Errorf("NonAdminBackup Status Condition Type %v has Status %v, expected %v", expectedCondition.Type, actualCondition.Status, expectedCondition.Status)
+				}
+				if actualCondition.Reason != expectedCondition.Reason {
+					return fmt.Errorf("NonAdminBackup Status Condition Type %v has Reason %v, expected %v", expectedCondition.Type, actualCondition.Reason, expectedCondition.Reason)
+				}
+				if !strings.Contains(actualCondition.Message, expectedCondition.Message) {
+					return fmt.Errorf("NonAdminBackup Status Condition Type %v has Message %v, expected to contain %v", expectedCondition.Type, actualCondition.Message, expectedCondition.Message)
+				}
+				break
+			}
 		}
-		if !strings.Contains(nonAdminBackup.Status.Conditions[index].Message, expectedStatus.Conditions[index].Message) {
-			return fmt.Errorf("NonAdminBackup Status Conditions [%v] Message %v does not contain expected message %v", index, nonAdminBackup.Status.Conditions[index].Message, expectedStatus.Conditions[index].Message)
+		if !found {
+			return fmt.Errorf("NonAdminBackup Status missing expected Condition Type %v", expectedCondition.Type)
 		}
 	}
 
@@ -1145,9 +1150,6 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller",
 			gomega.Expect(createTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed())
 
 			k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
-				Controller: config.Controller{
-					SkipNameValidation: ptr.To(true),
-				},
 				Scheme: k8sClient.Scheme(),
 				Cache: cache.Options{
 					DefaultNamespaces: map[string]cache.Config{
@@ -1581,9 +1583,6 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller",
 			gomega.Expect(k8sClient.Update(ctx, veleroBackup)).To(gomega.Succeed())
 
 			k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
-				Controller: config.Controller{
-					SkipNameValidation: ptr.To(true),
-				},
 				Scheme: k8sClient.Scheme(),
 				Cache: cache.Options{
 					DefaultNamespaces: map[string]cache.Config{

internal/controller/nonadminbackupstoragelocation_controller.go

@@ -53,7 +53,6 @@
 
 	"github.com/go-logr/logr"
 	oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
-	oadpcommon "github.com/openshift/oadp-operator/pkg/common"
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
 	corev1 "k8s.io/api/core/v1"
@@ -244,9 +243,9 @@
 				},
 				NonAdminBslSecretPredicate: predicate.NonAdminBslSecretPredicate{},
 			}).
 		Watches(&velerov1.BackupStorageLocation{}, &handler.VeleroBackupStorageLocationHandler{}).
 		Watches(&nacv1alpha1.NonAdminBackupStorageLocationRequest{}, &handler.NonAdminBackupStorageLocationRequestHandler{}).
 		Watches(&corev1.Secret{}, &handler.NonAdminBslSecretHandler{
 			Client: r.Client,
 		}).
 		Complete(r)
@@ -892,12 +891,8 @@
 
 	enforcedBSLSpec := getEnforcedBSLSpec(nabsl, r.EnforcedBslSpec)
 
-	err = oadpcommon.UpdateBackupStorageLocation(veleroBsl, *enforcedBSLSpec)
-
-	if err != nil {
-		logger.Error(err, "Failed to update VeleroBackupStorageLocation spec")
-		return false, err
-	}
+	// Update the VeleroBackupStorageLocation spec
+	veleroBsl.Spec = *enforcedBSLSpec
 
 	// NaBSL/BSL must have a unique prefix for proper function of the non-admin backup sync controller
 	// 1. Check if user has specified the prefix as "foo" in NaBSL creation, then prefix used would be <non-admin-ns>/foo

internal/controller/nonadminbackupstoragelocation_controller_test.go

@@ -51,9 +51,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
-	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
-	"sigs.k8s.io/controller-runtime/pkg/config"
 
 	nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1"
 	"github.com/migtools/oadp-non-admin/internal/common/constant"
@@ -232,9 +230,6 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackupStorageLocati
 			}
 
 			k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
-				Controller: config.Controller{
-					SkipNameValidation: ptr.To(true),
-				},
 				Scheme: k8sClient.Scheme(),
 			})
 			gomega.Expect(err).ToNot(gomega.HaveOccurred())

internal/controller/nonadminbackupsynchronizer_controller.go

@@ -31,6 +31,7 @@
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -169,6 +170,6 @@
 		WithLogConstructor(func(_ *reconcile.Request) logr.Logger {
 			return logr.New(ctrl.Log.GetSink().WithValues("controller", "nonadminbackupsynchronizer"))
 		}).
-		WatchesRawSource(&source.PeriodicalSource{Frequency: r.SyncPeriod}).
+		WatchesRawSource(&source.PeriodicalSource{Frequency: r.SyncPeriod}, &handler.EnqueueRequestForObject{}).
 		Complete(r)
 }

internal/controller/nonadminbackupsynchronizer_controller_test.go

@@ -26,11 +26,9 @@ import (
 	"github.com/onsi/gomega"
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/config"
 
 	nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1"
 	"github.com/migtools/oadp-non-admin/internal/common/constant"
@@ -160,9 +158,6 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Synchronizer
 			gomega.Expect(nonAdminBackupsInNonAminNamespace.Items).To(gomega.BeEmpty())
 
 			k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
-				Controller: config.Controller{
-					SkipNameValidation: ptr.To(true),
-				},
 				Scheme: k8sClient.Scheme(),
 				Cache: cache.Options{
 					DefaultNamespaces: map[string]cache.Config{

internal/controller/nonadmindownloadrequest_controller.go

@@ -226,10 +226,10 @@
 func (r *NonAdminDownloadRequestReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&nacv1alpha1.NonAdminDownloadRequest{}, builder.WithPredicates(ctrlpredicate.Funcs{
-			CreateFunc: func(_ event.TypedCreateEvent[client.Object]) bool {
+			CreateFunc: func(_ event.CreateEvent) bool {
 				return true // required fields are set via velero validation markers
 			},
-			UpdateFunc: func(tue event.TypedUpdateEvent[client.Object]) bool {
+			UpdateFunc: func(tue event.UpdateEvent) bool {
 				// only process update on spec change
 				if tue.ObjectNew.GetGeneration() == tue.ObjectOld.GetGeneration() {
 					return false
@@ -239,16 +239,16 @@
 				}
 				return false
 			},
-			DeleteFunc: func(_ event.TypedDeleteEvent[client.Object]) bool {
+			DeleteFunc: func(_ event.DeleteEvent) bool {
 				return true
 			}, // we process delete events by deleting corresponding velero download requests if found
-			GenericFunc: func(_ event.TypedGenericEvent[client.Object]) bool {
+			GenericFunc: func(_ event.GenericEvent) bool {
 				return false
 			},
 		})).
 		Named("nonadmindownloadrequest").
 		Watches(&velerov1.DownloadRequest{}, handler.Funcs{
-			UpdateFunc: func(ctx context.Context, tue event.TypedUpdateEvent[client.Object], rli workqueue.TypedRateLimitingInterface[reconcile.Request]) {
+			UpdateFunc: func(ctx context.Context, tue event.UpdateEvent, rli workqueue.RateLimitingInterface) {
 				if dr, ok := tue.ObjectNew.(*velerov1.DownloadRequest); ok &&
 					dr.Status.Phase == velerov1.DownloadRequestPhaseProcessed { // only reconcile on updates when downloadrequests is processed
 					log := function.GetLogger(ctx, dr, "VeleroDownloadRequestHandler")