From 87e569e09986d3f1faad1f53a397c457b9d00742 Mon Sep 17 00:00:00 2001 From: Ryotaro Banno Date: Tue, 22 Oct 2024 01:45:50 +0000 Subject: [PATCH 1/6] separate Reconcile into 3 (standalone, primary, secondary) functions Signed-off-by: Ryotaro Banno --- .../controller/mantlebackup_controller.go | 55 ++++++++++++------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/internal/controller/mantlebackup_controller.go b/internal/controller/mantlebackup_controller.go index 89a4971e..5e952af0 100644 --- a/internal/controller/mantlebackup_controller.go +++ b/internal/controller/mantlebackup_controller.go @@ -283,18 +283,10 @@ func (r *MantleBackupReconciler) expire(ctx context.Context, backup *mantlev1.Ma // // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.16.3/pkg/reconcile -// -// Reconcile is the main component of mantle-controller, so let's admit that Reconcile can be complex by `nolint:gocyclo` -// -//nolint:gocyclo func (r *MantleBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) var backup mantlev1.MantleBackup - if r.role == RoleSecondary { - return ctrl.Result{}, nil - } - err := r.Get(ctx, req.NamespacedName, &backup) if aerrors.IsNotFound(err) { logger.Info("MantleBackup is not found", "error", err) @@ -305,14 +297,29 @@ func (r *MantleBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, err } - if isCreatedWhenMantleControllerWasSecondary(&backup) { + switch r.role { + case RoleStandalone: + return r.ReconcileAsStandalone(ctx, &backup) + case RolePrimary: + return r.ReconcileAsPrimary(ctx, &backup) + case RoleSecondary: + return r.ReconcileAsSecondary(ctx, &backup) + } + + panic("unreachable") +} + +func (r *MantleBackupReconciler) ReconcileAsStandalone(ctx context.Context, backup *mantlev1.MantleBackup) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + if isCreatedWhenMantleControllerWasSecondary(backup) { logger.Info( "skipping to reconcile the MantleBackup created by a remote mantle-controller to prevent accidental data loss", ) return ctrl.Result{}, nil } - target, result, getSnapshotTargetErr := r.getSnapshotTarget(ctx, &backup) + target, result, getSnapshotTargetErr := r.getSnapshotTarget(ctx, backup) switch { case getSnapshotTargetErr == errSkipProcessing: return ctrl.Result{}, nil @@ -327,32 +334,32 @@ func (r *MantleBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request } if !backup.ObjectMeta.DeletionTimestamp.IsZero() { - return r.finalize(ctx, &backup, target, isErrTargetPVCNotFound(getSnapshotTargetErr)) + return r.finalize(ctx, backup, target, isErrTargetPVCNotFound(getSnapshotTargetErr)) } if getSnapshotTargetErr != nil { return ctrl.Result{}, getSnapshotTargetErr } - if !controllerutil.ContainsFinalizer(&backup, MantleBackupFinalizerName) { - controllerutil.AddFinalizer(&backup, MantleBackupFinalizerName) - err = r.Update(ctx, &backup) - if err != nil { + if !controllerutil.ContainsFinalizer(backup, MantleBackupFinalizerName) { + controllerutil.AddFinalizer(backup, MantleBackupFinalizerName) + + if err := r.Update(ctx, backup); err != nil { logger.Error(err, "failed to add finalizer", "finalizer", MantleBackupFinalizerName) return ctrl.Result{}, err } - err := r.updateStatusCondition(ctx, &backup, metav1.Condition{Type: mantlev1.BackupConditionReadyToUse, Status: metav1.ConditionFalse, Reason: mantlev1.BackupReasonNone}) + err := r.updateStatusCondition(ctx, backup, metav1.Condition{Type: mantlev1.BackupConditionReadyToUse, Status: metav1.ConditionFalse, Reason: mantlev1.BackupReasonNone}) if err != nil { return ctrl.Result{}, err } return ctrl.Result{Requeue: true}, nil } - if err := r.expire(ctx, &backup); err != nil { + if err := r.expire(ctx, backup); err != nil { return ctrl.Result{}, err } - if err := r.provisionRBDSnapshot(ctx, &backup, target); err != nil { + if err := r.provisionRBDSnapshot(ctx, backup, target); err != nil { return ctrl.Result{}, err } @@ -361,10 +368,18 @@ func (r *MantleBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, nil } - if r.role == RolePrimary { - return r.replicate(ctx, &backup) + return ctrl.Result{}, nil +} + +func (r *MantleBackupReconciler) ReconcileAsPrimary(ctx context.Context, backup *mantlev1.MantleBackup) (ctrl.Result, error) { + result, err := r.ReconcileAsStandalone(ctx, backup) + if err != nil || !result.IsZero() { + return result, err } + return r.replicate(ctx, backup) +} +func (r *MantleBackupReconciler) ReconcileAsSecondary(_ context.Context, _ *mantlev1.MantleBackup) (ctrl.Result, error) { return ctrl.Result{}, nil } From 2dddfa36810565ed128440c4c454ff937bb4ec2d Mon Sep 17 00:00:00 2001 From: Ryotaro Banno Date: Tue, 22 Oct 2024 05:05:40 +0000 Subject: [PATCH 2/6] implement ReconcileAsSecondary Signed-off-by: Ryotaro Banno --- .../controller/mantlebackup_controller.go | 192 +++++++++++++++++- test/e2e/multik8s/suite_test.go | 7 +- 2 files changed, 190 insertions(+), 9 deletions(-) diff --git a/internal/controller/mantlebackup_controller.go b/internal/controller/mantlebackup_controller.go index 5e952af0..64db84ec 100644 --- a/internal/controller/mantlebackup_controller.go +++ b/internal/controller/mantlebackup_controller.go @@ -334,7 +334,7 @@ func (r *MantleBackupReconciler) ReconcileAsStandalone(ctx context.Context, back } if !backup.ObjectMeta.DeletionTimestamp.IsZero() { - return r.finalize(ctx, backup, target, isErrTargetPVCNotFound(getSnapshotTargetErr)) + return r.finalizeStandalone(ctx, backup, target, isErrTargetPVCNotFound(getSnapshotTargetErr)) } if getSnapshotTargetErr != nil { @@ -379,8 +379,46 @@ func (r *MantleBackupReconciler) ReconcileAsPrimary(ctx context.Context, backup return r.replicate(ctx, backup) } -func (r *MantleBackupReconciler) ReconcileAsSecondary(_ context.Context, _ *mantlev1.MantleBackup) (ctrl.Result, error) { - return ctrl.Result{}, nil +func (r *MantleBackupReconciler) ReconcileAsSecondary(ctx context.Context, backup *mantlev1.MantleBackup) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + if !isCreatedWhenMantleControllerWasSecondary(backup) { + logger.Info( + "skipping to reconcile the MantleBackup created by a different mantle-controller to prevent accidental data loss", + ) + return ctrl.Result{}, nil + } + + target, result, getSnapshotTargetErr := r.getSnapshotTarget(ctx, backup) + switch { + case getSnapshotTargetErr == errSkipProcessing: + return ctrl.Result{}, nil + case isErrTargetPVCNotFound(getSnapshotTargetErr): + // deletion logic may run. + case getSnapshotTargetErr == nil: + default: + return ctrl.Result{}, getSnapshotTargetErr + } + if result.Requeue { + return result, nil + } + + if !backup.ObjectMeta.DeletionTimestamp.IsZero() { + return r.finalizeSecondary(ctx, backup, target, isErrTargetPVCNotFound(getSnapshotTargetErr)) + } + + if getSnapshotTargetErr != nil { + return ctrl.Result{}, getSnapshotTargetErr + } + + if !meta.IsStatusConditionTrue(backup.Status.Conditions, mantlev1.BackupConditionReadyToUse) { + result, err := r.startImport(ctx, backup, target) + if err != nil || !result.IsZero() { + return result, err + } + } + + return r.secondaryCleanup(ctx, backup) } func scheduleExpire(_ context.Context, evt event.GenericEvent, q workqueue.RateLimitingInterface) { @@ -589,7 +627,7 @@ func isCreatedWhenMantleControllerWasSecondary(backup *mantlev1.MantleBackup) bo return ok } -func (r *MantleBackupReconciler) finalize( +func (r *MantleBackupReconciler) finalizeStandalone( ctx context.Context, backup *mantlev1.MantleBackup, target *snapshotTarget, @@ -600,6 +638,8 @@ func (r *MantleBackupReconciler) finalize( return ctrl.Result{Requeue: true}, nil } + // primaryClean() is called in finalizeStandalone() to delete resources for + // exported and uploaded snapshots in both standalone and primary Mantle. result, err := r.primaryCleanup(ctx, backup) if err != nil || result != (ctrl.Result{}) { return result, err @@ -625,6 +665,42 @@ func (r *MantleBackupReconciler) finalize( return ctrl.Result{}, nil } +func (r *MantleBackupReconciler) finalizeSecondary( + ctx context.Context, + backup *mantlev1.MantleBackup, + target *snapshotTarget, + targetPVCNotFound bool, +) (ctrl.Result, error) { + logger := log.FromContext(ctx) + if _, ok := backup.GetAnnotations()[annotDiffTo]; ok { + return ctrl.Result{Requeue: true}, nil + } + + result, err := r.secondaryCleanup(ctx, backup) + if err != nil || result != (ctrl.Result{}) { + return result, err + } + + if !controllerutil.ContainsFinalizer(backup, MantleBackupFinalizerName) { + return ctrl.Result{}, nil + } + + if !targetPVCNotFound { + err := r.removeRBDSnapshot(ctx, target.poolName, target.imageName, backup.Name) + if err != nil { + return ctrl.Result{}, err + } + } + + controllerutil.RemoveFinalizer(backup, MantleBackupFinalizerName) + if err := r.Update(ctx, backup); err != nil { + logger.Error(err, "failed to remove finalizer", "finalizer", MantleBackupFinalizerName) + return ctrl.Result{}, err + } + + return ctrl.Result{}, nil +} + type dataSyncPrepareResult struct { isIncremental bool // NOTE: The value is forcibly set to false if isSecondaryMantleBackupReadyToUse is true. isSecondaryMantleBackupReadyToUse bool @@ -780,6 +856,107 @@ func (r *MantleBackupReconciler) export( return ctrl.Result{}, nil } +func (r *MantleBackupReconciler) startImport( + ctx context.Context, + backup *mantlev1.MantleBackup, + target *snapshotTarget, +) (ctrl.Result, error) { //nolint:unparam + if result, err := r.isExportDataAlreadyUploaded(ctx, backup); err != nil || !result.IsZero() { + return result, err + } + + // Requeue if the PV is smaller than the PVC. (This may be the case if pvc-autoresizer is used.) + if isPVSmallerThanPVC(target.pv, target.pvc) { + return ctrl.Result{Requeue: true}, nil + } + + if err := r.updateStatusManifests(ctx, backup, target.pv, target.pvc); err != nil { + return ctrl.Result{}, err + } + + if result, err := r.reconcileDiscardJob(ctx, backup, target); err != nil || !result.IsZero() { + return result, err + } + + if result, err := r.reconcileImportJob(ctx, backup, target); err != nil || !result.IsZero() { + return result, err + } + + return ctrl.Result{}, nil +} + +func (r *MantleBackupReconciler) isExportDataAlreadyUploaded( + _ context.Context, + _ *mantlev1.MantleBackup, +) (ctrl.Result, error) { //nolint:unparam + return ctrl.Result{}, nil +} + +func isPVSmallerThanPVC( + pv *corev1.PersistentVolume, + pvc *corev1.PersistentVolumeClaim, +) bool { + return pv.Spec.Capacity.Storage().Cmp(*pvc.Spec.Resources.Requests.Storage()) == -1 +} + +func (r *MantleBackupReconciler) updateStatusManifests( + ctx context.Context, + backup *mantlev1.MantleBackup, + pv *corev1.PersistentVolume, + pvc *corev1.PersistentVolumeClaim, +) error { + if backup.Status.PVManifest != "" || backup.Status.PVCManifest != "" { + return nil + } + return updateStatus(ctx, r.Client, backup, func() error { + pvJSON, err := json.Marshal(*pv) + if err != nil { + return err + } + backup.Status.PVManifest = string(pvJSON) + + pvcJSON, err := json.Marshal(*pvc) + if err != nil { + return err + } + backup.Status.PVCManifest = string(pvcJSON) + + return nil + }) +} + +func (r *MantleBackupReconciler) reconcileDiscardJob( + _ context.Context, + backup *mantlev1.MantleBackup, + _ *snapshotTarget, +) (ctrl.Result, error) { //nolint:unparam + if backup.GetAnnotations()[annotSyncMode] != syncModeFull { + return ctrl.Result{}, nil + } + + // FIXME: implement here later + + return ctrl.Result{}, nil +} + +func (r *MantleBackupReconciler) reconcileImportJob( + ctx context.Context, + backup *mantlev1.MantleBackup, + _ *snapshotTarget, +) (ctrl.Result, error) { //nolint:unparam + // FIXME: implement here later + + if err := r.updateStatusCondition(ctx, backup, metav1.Condition{ + Type: mantlev1.BackupConditionReadyToUse, + Status: metav1.ConditionTrue, + Reason: mantlev1.BackupReasonNone, + }); err != nil { + return ctrl.Result{}, err + } + + return ctrl.Result{}, nil +} + func (r *MantleBackupReconciler) primaryCleanup( ctx context.Context, backup *mantlev1.MantleBackup, @@ -799,3 +976,10 @@ func (r *MantleBackupReconciler) primaryCleanup( return ctrl.Result{}, nil } + +func (r *MantleBackupReconciler) secondaryCleanup( + _ context.Context, + _ *mantlev1.MantleBackup, +) (ctrl.Result, error) { // nolint:unparam + return ctrl.Result{}, nil +} diff --git a/test/e2e/multik8s/suite_test.go b/test/e2e/multik8s/suite_test.go index c0b366b1..b7c7637c 100644 --- a/test/e2e/multik8s/suite_test.go +++ b/test/e2e/multik8s/suite_test.go @@ -151,11 +151,8 @@ func replicationTestSuite() { if secondaryMB.Status.SnapID != nil { return errors.New(".Status.SapID is incorrectly populated") } - if secondaryMB.Status.PVManifest != "" { - return errors.New(".Status.PVManifest is incorrectly populated") - } - if secondaryMB.Status.PVCManifest != "" { - return errors.New(".Status.PVCManifest is incorrectly populated") + if !meta.IsStatusConditionTrue(secondaryMB.Status.Conditions, "ReadyToUse") { + return errors.New("ReadyToUse of .Status.Conditions is not True") } return nil From d9410582046b01ca774dcc063bdf2225d4f63a6a Mon Sep 17 00:00:00 2001 From: Ryotaro Banno Date: Tue, 22 Oct 2024 06:31:17 +0000 Subject: [PATCH 3/6] empty .spec.volumeName of PVC when it's sent to secondary Mantle Signed-off-by: Ryotaro Banno --- internal/controller/mantlebackup_controller.go | 1 + test/e2e/multik8s/suite_test.go | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/internal/controller/mantlebackup_controller.go b/internal/controller/mantlebackup_controller.go index 64db84ec..eb8efdd1 100644 --- a/internal/controller/mantlebackup_controller.go +++ b/internal/controller/mantlebackup_controller.go @@ -508,6 +508,7 @@ func (r *MantleBackupReconciler) replicateManifests( annotRemoteUID: string(pvc.GetUID()), }) pvcSent.Spec = pvc.Spec + pvcSent.Spec.VolumeName = "" // The VolumeName should be blank. pvcSentJson, err := json.Marshal(pvcSent) if err != nil { return ctrl.Result{}, err diff --git a/test/e2e/multik8s/suite_test.go b/test/e2e/multik8s/suite_test.go index b7c7637c..4d94d221 100644 --- a/test/e2e/multik8s/suite_test.go +++ b/test/e2e/multik8s/suite_test.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" mantlev1 "github.com/cybozu-go/mantle/api/v1" + corev1 "k8s.io/api/core/v1" ) func TestMtest(t *testing.T) { @@ -105,9 +106,15 @@ func replicationTestSuite() { pvc.Annotations["mantle.cybozu.io/remote-uid"] != string(primaryPVC.GetUID()) { return errors.New("invalid remote-uid annotation") } + primaryPVC.Spec.VolumeName = "" + pvc.Spec.VolumeName = "" if !reflect.DeepEqual(primaryPVC.Spec, pvc.Spec) { return errors.New("spec not equal") } + if pvc.Status.Phase != corev1.ClaimBound { + return errors.New("pvc not bound") + } + return nil }).Should(Succeed()) From 2dd1da5eb84b577baf04889469b8ebb4afcdfd38 Mon Sep 17 00:00:00 2001 From: Ryotaro Banno Date: Wed, 23 Oct 2024 07:22:57 +0000 Subject: [PATCH 4/6] rename ReconcileAs* to reconcileAs* --- internal/controller/mantlebackup_controller.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/controller/mantlebackup_controller.go b/internal/controller/mantlebackup_controller.go index eb8efdd1..a352e472 100644 --- a/internal/controller/mantlebackup_controller.go +++ b/internal/controller/mantlebackup_controller.go @@ -299,17 +299,17 @@ func (r *MantleBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request switch r.role { case RoleStandalone: - return r.ReconcileAsStandalone(ctx, &backup) + return r.reconcileAsStandalone(ctx, &backup) case RolePrimary: - return r.ReconcileAsPrimary(ctx, &backup) + return r.reconcileAsPrimary(ctx, &backup) case RoleSecondary: - return r.ReconcileAsSecondary(ctx, &backup) + return r.reconcileAsSecondary(ctx, &backup) } panic("unreachable") } -func (r *MantleBackupReconciler) ReconcileAsStandalone(ctx context.Context, backup *mantlev1.MantleBackup) (ctrl.Result, error) { +func (r *MantleBackupReconciler) reconcileAsStandalone(ctx context.Context, backup *mantlev1.MantleBackup) (ctrl.Result, error) { logger := log.FromContext(ctx) if isCreatedWhenMantleControllerWasSecondary(backup) { @@ -371,15 +371,15 @@ func (r *MantleBackupReconciler) ReconcileAsStandalone(ctx context.Context, back return ctrl.Result{}, nil } -func (r *MantleBackupReconciler) ReconcileAsPrimary(ctx context.Context, backup *mantlev1.MantleBackup) (ctrl.Result, error) { - result, err := r.ReconcileAsStandalone(ctx, backup) +func (r *MantleBackupReconciler) reconcileAsPrimary(ctx context.Context, backup *mantlev1.MantleBackup) (ctrl.Result, error) { + result, err := r.reconcileAsStandalone(ctx, backup) if err != nil || !result.IsZero() { return result, err } return r.replicate(ctx, backup) } -func (r *MantleBackupReconciler) ReconcileAsSecondary(ctx context.Context, backup *mantlev1.MantleBackup) (ctrl.Result, error) { +func (r *MantleBackupReconciler) reconcileAsSecondary(ctx context.Context, backup *mantlev1.MantleBackup) (ctrl.Result, error) { logger := log.FromContext(ctx) if !isCreatedWhenMantleControllerWasSecondary(backup) { From 1243f1f142b959bd87b7f40b04063615df49eb46 Mon Sep 17 00:00:00 2001 From: Ryotaro Banno Date: Wed, 23 Oct 2024 07:32:53 +0000 Subject: [PATCH 5/6] expire MantleBackups when role is secondary --- internal/controller/mantlebackup_controller.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/controller/mantlebackup_controller.go b/internal/controller/mantlebackup_controller.go index a352e472..2874c924 100644 --- a/internal/controller/mantlebackup_controller.go +++ b/internal/controller/mantlebackup_controller.go @@ -411,6 +411,10 @@ func (r *MantleBackupReconciler) reconcileAsSecondary(ctx context.Context, backu return ctrl.Result{}, getSnapshotTargetErr } + if err := r.expire(ctx, backup); err != nil { + return ctrl.Result{}, err + } + if !meta.IsStatusConditionTrue(backup.Status.Conditions, mantlev1.BackupConditionReadyToUse) { result, err := r.startImport(ctx, backup, target) if err != nil || !result.IsZero() { From 024e131aea77e5fa7c266accbed41260606b6dca Mon Sep 17 00:00:00 2001 From: Ryotaro Banno Date: Thu, 24 Oct 2024 04:37:01 +0000 Subject: [PATCH 6/6] check finalizers before calling {primary,secondary}Cleanup Signed-off-by: Ryotaro Banno --- internal/controller/mantlebackup_controller.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/controller/mantlebackup_controller.go b/internal/controller/mantlebackup_controller.go index 2874c924..c687687c 100644 --- a/internal/controller/mantlebackup_controller.go +++ b/internal/controller/mantlebackup_controller.go @@ -643,6 +643,10 @@ func (r *MantleBackupReconciler) finalizeStandalone( return ctrl.Result{Requeue: true}, nil } + if !controllerutil.ContainsFinalizer(backup, MantleBackupFinalizerName) { + return ctrl.Result{}, nil + } + // primaryClean() is called in finalizeStandalone() to delete resources for // exported and uploaded snapshots in both standalone and primary Mantle. result, err := r.primaryCleanup(ctx, backup) @@ -650,10 +654,6 @@ func (r *MantleBackupReconciler) finalizeStandalone( return result, err } - if !controllerutil.ContainsFinalizer(backup, MantleBackupFinalizerName) { - return ctrl.Result{}, nil - } - if !targetPVCNotFound { err := r.removeRBDSnapshot(ctx, target.poolName, target.imageName, backup.Name) if err != nil { @@ -681,15 +681,15 @@ func (r *MantleBackupReconciler) finalizeSecondary( return ctrl.Result{Requeue: true}, nil } + if !controllerutil.ContainsFinalizer(backup, MantleBackupFinalizerName) { + return ctrl.Result{}, nil + } + result, err := r.secondaryCleanup(ctx, backup) if err != nil || result != (ctrl.Result{}) { return result, err } - if !controllerutil.ContainsFinalizer(backup, MantleBackupFinalizerName) { - return ctrl.Result{}, nil - } - if !targetPVCNotFound { err := r.removeRBDSnapshot(ctx, target.poolName, target.imageName, backup.Name) if err != nil {