diff --git a/Makefile b/Makefile index e9bd192bc..e0a67f56d 100644 --- a/Makefile +++ b/Makefile @@ -190,9 +190,6 @@ test-util: generate manifests envtest ## Run util tests. test-util-pvc: generate manifests envtest ## Run util-pvc tests. go test ./internal/controller/util -coverprofile cover.out -ginkgo.focus PVCS_Util -test-kubeobjects: ## Run kubeobjects tests. - go test ./internal/controller/kubeobjects -coverprofile cover.out -ginkgo.focus Kubeobjects - test-drenv: ## Run drenv tests. $(MAKE) -C test diff --git a/internal/controller/drplacementcontrol_controller.go b/internal/controller/drplacementcontrol_controller.go index 099a52b83..a6d192d76 100644 --- a/internal/controller/drplacementcontrol_controller.go +++ b/internal/controller/drplacementcontrol_controller.go @@ -119,7 +119,7 @@ func (r *DRPlacementControlReconciler) SetupWithManager(mgr ctrl.Manager) error // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.7.0/pkg/reconcile // -//nolint:funlen,gocognit,gocyclo,cyclop +//nolint:funlen,gocognit,gocyclo,cyclop,nestif func (r *DRPlacementControlReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { logger := r.Log.WithValues("DRPC", req.NamespacedName, "rid", uuid.New()) @@ -166,13 +166,21 @@ func (r *DRPlacementControlReconciler) Reconcile(ctx context.Context, req ctrl.R // then the DRPC should be deleted as well. The least we should do here is to clean up DPRC. err := r.processDeletion(ctx, drpc, placementObj, logger) if err != nil { - logger.Info(fmt.Sprintf("Error in deleting DRPC: (%v)", err)) - statusErr := r.setDeletionStatusAndUpdate(ctx, drpc) if statusErr != nil { err = fmt.Errorf("drpc deletion failed: %w and status update failed: %w", err, statusErr) + + return ctrl.Result{}, err } + // Is this an expected condition? + if rmnutil.IsOperationInProgress(err) { + logger.Info("Deleting DRPC in progress", "reason", err) + + return ctrl.Result{Requeue: true}, nil + } + + // Unexpected error. return ctrl.Result{}, err } @@ -724,19 +732,19 @@ func (r *DRPlacementControlReconciler) cleanupVRGs( return fmt.Errorf("failed to retrieve VRGs. We'll retry later. Error (%w)", err) } - if !ensureVRGsManagedByDRPC(r.Log, mwu, vrgs, drpc, vrgNamespace) { - return fmt.Errorf("VRG adoption in progress") + // We have to ensure the seconrary VRG is deleted before deleting the primary VRG. This will fail until there + // is no secondary VRG in the vrgs list. + if err := r.ensureVRGsDeleted(mwu, vrgs, drpc, vrgNamespace, rmn.Secondary); err != nil { + return err } - // delete VRG manifestwork - for _, drClusterName := range rmnutil.DRPolicyClusterNames(drPolicy) { - if err := mwu.DeleteManifestWork(mwu.BuildManifestWorkName(rmnutil.MWTypeVRG), drClusterName); err != nil { - return fmt.Errorf("%w", err) - } + // This will fail until there is no primary VRG in the vrgs list. + if err := r.ensureVRGsDeleted(mwu, vrgs, drpc, vrgNamespace, rmn.Primary); err != nil { + return err } if len(vrgs) != 0 { - return fmt.Errorf("waiting for VRGs count to go to zero") + return rmnutil.OperationInProgress("waiting for VRGs count to go to zero") } // delete MCVs @@ -747,6 +755,38 @@ func (r *DRPlacementControlReconciler) cleanupVRGs( return nil } +// ensureVRGsDeleted ensure that seconrary or primary VRGs are deleted. Return an error if a vrg could not be deleted, +// or deletion is in progress. Return nil if vrg of specified type was not found. +func (r *DRPlacementControlReconciler) ensureVRGsDeleted( + mwu rmnutil.MWUtil, + vrgs map[string]*rmn.VolumeReplicationGroup, + drpc *rmn.DRPlacementControl, + vrgNamespace string, + replicationState rmn.ReplicationState, +) error { + var inProgress bool + + for cluster, vrg := range vrgs { + if vrg.Spec.ReplicationState == replicationState { + if !ensureVRGsManagedByDRPC(r.Log, mwu, vrgs, drpc, vrgNamespace) { + return rmnutil.OperationInProgress(fmt.Sprintf("%s VRG adoption in progress", replicationState)) + } + + if err := mwu.DeleteManifestWork(mwu.BuildManifestWorkName(rmnutil.MWTypeVRG), cluster); err != nil { + return fmt.Errorf("failed to delete %s VRG manifestwork for cluster %q: %w", replicationState, cluster, err) + } + + inProgress = true + } + } + + if inProgress { + return rmnutil.OperationInProgress(fmt.Sprintf("%s VRG manifestwork deletion in progress", replicationState)) + } + + return nil +} + func (r *DRPlacementControlReconciler) deleteAllManagedClusterViews( drpc *rmn.DRPlacementControl, clusterNames []string, ) error { diff --git a/internal/controller/kubeobjects/requests.go b/internal/controller/kubeobjects/requests.go index f9b437037..73b5f845c 100644 --- a/internal/controller/kubeobjects/requests.go +++ b/internal/controller/kubeobjects/requests.go @@ -43,8 +43,6 @@ func RequestsMapKeyedByName(requestsStruct Requests) map[string]Request { return requests } -type RequestProcessingError struct{ string } - type CaptureSpec struct { //+optional Name string `json:"name,omitempty"` @@ -138,16 +136,6 @@ type Operation struct { InverseOp string `json:"inverseOp,omitempty"` } -func RequestProcessingErrorCreate(s string) RequestProcessingError { return RequestProcessingError{s} } -func (e RequestProcessingError) Error() string { return e.string } - -// Called by errors.Is() to match target. -func (RequestProcessingError) Is(target error) bool { - _, ok := target.(RequestProcessingError) - - return ok -} - type RequestsManager interface { ProtectsPath() string RecoversPath() string diff --git a/internal/controller/kubeobjects/requests_test.go b/internal/controller/kubeobjects/requests_test.go deleted file mode 100644 index 21a1cc4ae..000000000 --- a/internal/controller/kubeobjects/requests_test.go +++ /dev/null @@ -1,30 +0,0 @@ -// SPDX-FileCopyrightText: The RamenDR authors -// SPDX-License-Identifier: Apache-2.0 - -package kubeobjects_test - -import ( - "errors" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - - "github.com/ramendr/ramen/internal/controller/kubeobjects" -) - -var _ = Describe("kubeobjects", func() { - Context("comparing errors", func() { - err := kubeobjects.RequestProcessingErrorCreate("error") - other := kubeobjects.RequestProcessingErrorCreate("other") - - It("is not equal", func() { - Expect(other == err).To(Equal(false)) - }) - It("is same error", func() { - Expect(errors.Is(other, err)).To(Equal(true)) - }) - It("is not same error", func() { - Expect(errors.Is(err, errors.New("error"))).To(Equal(false)) - }) - }) -}) diff --git a/internal/controller/kubeobjects/velero/requests.go b/internal/controller/kubeobjects/velero/requests.go index 0542a694e..18ff77f73 100644 --- a/internal/controller/kubeobjects/velero/requests.go +++ b/internal/controller/kubeobjects/velero/requests.go @@ -17,6 +17,7 @@ import ( "github.com/go-logr/logr" pkgerrors "github.com/pkg/errors" "github.com/ramendr/ramen/internal/controller/kubeobjects" + "github.com/ramendr/ramen/internal/controller/util" velero "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -249,11 +250,11 @@ func backupDummyStatusProcessAndRestore( velero.BackupPhaseUploading, velero.BackupPhaseUploadingPartialFailure, velero.BackupPhaseDeleting: - return nil, kubeobjects.RequestProcessingErrorCreate("backup" + string(backup.Status.Phase)) + return nil, util.OperationInProgress("backup" + string(backup.Status.Phase)) case velero.BackupPhaseFailedValidation: return nil, errors.New("backup" + string(backup.Status.Phase)) default: - return nil, kubeobjects.RequestProcessingErrorCreate("backup.status.phase absent") + return nil, util.OperationInProgress("backup.status.phase absent") } } @@ -283,13 +284,13 @@ func restoreStatusProcess( return nil case velero.RestorePhaseNew, velero.RestorePhaseInProgress: - return kubeobjects.RequestProcessingErrorCreate("restore" + string(restore.Status.Phase)) + return util.OperationInProgress("restore" + string(restore.Status.Phase)) case velero.RestorePhaseFailed, velero.RestorePhaseFailedValidation, velero.RestorePhasePartiallyFailed: return errors.New("restore" + string(restore.Status.Phase)) default: - return kubeobjects.RequestProcessingErrorCreate("restore.status.phase absent") + return util.OperationInProgress("restore.status.phase absent") } } @@ -399,13 +400,13 @@ func backupRealStatusProcess( velero.BackupPhaseUploading, velero.BackupPhaseUploadingPartialFailure, velero.BackupPhaseDeleting: - return kubeobjects.RequestProcessingErrorCreate("backup" + string(backup.Status.Phase)) + return util.OperationInProgress("backup" + string(backup.Status.Phase)) case velero.BackupPhaseFailedValidation, velero.BackupPhasePartiallyFailed, velero.BackupPhaseFailed: return errors.New("backup" + string(backup.Status.Phase)) default: - return kubeobjects.RequestProcessingErrorCreate("backup.status.phase absent") + return util.OperationInProgress("backup.status.phase absent") } } diff --git a/internal/controller/util/errors.go b/internal/controller/util/errors.go new file mode 100644 index 000000000..a751bff18 --- /dev/null +++ b/internal/controller/util/errors.go @@ -0,0 +1,24 @@ +// SPDX-FileCopyrightText: The RamenDR authors +// SPDX-License-Identifier: Apache-2.0package util + +package util + +import "errors" + +// OperationInProgress error is return when an operation is in progress and we wait for the desired state. The error +// string should describe the operation for logging the error. +type OperationInProgress string + +func (e OperationInProgress) Error() string { return string(e) } + +// Called by errors.Is() to match target. +func (OperationInProgress) Is(target error) bool { + _, ok := target.(OperationInProgress) + + return ok +} + +// IsOperationInProgress returns true if err or error wrapped by it is an OperationInProgress error. +func IsOperationInProgress(err error) bool { + return errors.Is(err, OperationInProgress("")) +} diff --git a/internal/controller/util/errors_test.go b/internal/controller/util/errors_test.go new file mode 100644 index 000000000..3274daa6b --- /dev/null +++ b/internal/controller/util/errors_test.go @@ -0,0 +1,33 @@ +// SPDX-FileCopyrightText: The RamenDR authors +// SPDX-License-Identifier: Apache-2.0 + +package util_test + +import ( + "errors" + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/ramendr/ramen/internal/controller/util" +) + +var _ = Describe("OperationInProgress", func() { + Context("comparing errors", func() { + err := util.OperationInProgress("this operation") + + It("is not equal", func() { + Expect(err == util.OperationInProgress("other operation")).To(Equal(false)) + }) + It("match error", func() { + Expect(util.IsOperationInProgress(err)).To(Equal(true)) + }) + It("match wrapped error", func() { + wrapped := fmt.Errorf("wrapping operation in progress: %w", err) + Expect(util.IsOperationInProgress(wrapped)).To(Equal(true)) + }) + It("does not match other errors", func() { + Expect(util.IsOperationInProgress(errors.New("other error"))).To(Equal(false)) + }) + }) +}) diff --git a/internal/controller/vrg_kubeobjects.go b/internal/controller/vrg_kubeobjects.go index 08c2ce091..89a7ffe5d 100644 --- a/internal/controller/vrg_kubeobjects.go +++ b/internal/controller/vrg_kubeobjects.go @@ -361,7 +361,7 @@ func (v *VRGInstance) kubeObjectsGroupCapture( continue } - if errors.Is(err, kubeobjects.RequestProcessingError{}) { + if util.IsOperationInProgress(err) { log1.Info("Kube objects group capturing", "state", err.Error()) continue @@ -695,7 +695,7 @@ func (v *VRGInstance) executeRecoverGroup(result *ctrl.Result, s3StoreAccessor s } } - if errors.Is(err, kubeobjects.RequestProcessingError{}) { + if util.IsOperationInProgress(err) { log1.Info("Kube objects group recovering", "state", err.Error()) return err