Skip to content

Commit

Permalink
Fix for migtools#87 - Don't update NAB Spec from Velero Backup Spec
Browse files Browse the repository at this point in the history 
Change to remove update of the NAB Spec from the Velero Backup Spec
during reconcile loop.

Small nit change to add velerov1 api to import.

Signed-off-by: Michal Pryc <[email protected]>
  • Loading branch information
@mpryc
mpryc committed Sep 25, 2024
1 parent f50ab81 commit 78a11b6
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 109 deletions.
51 changes: 14 additions & 37 deletions internal/controller/nonadminbackup_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque
return ctrl.Result{}, nil
}

reconcileExit, reconcileRequeue, reconcileErr = r.UpdateSpecStatus(ctx, logger, &nab)
reconcileExit, reconcileRequeue, reconcileErr = r.SyncVeleroBackupWithNonAdminBackup(ctx, logger, &nab)
if reconcileRequeue {
return ctrl.Result{Requeue: true}, reconcileErr
} else if reconcileExit && reconcileErr != nil {
Expand Down Expand Up @@ -213,19 +213,17 @@ func (r *NonAdminBackupReconciler) ValidateSpec(ctx context.Context, logrLogger
return false, false, nil
}

// UpdateSpecStatus updates the Spec and Status from the NonAdminBackup.
// SyncVeleroBackupWithNonAdminBackup ensures the VeleroBackup associated with the given NonAdminBackup resource
// is created, updated, and its status is reconciled. If the VeleroBackup does not exist, it creates a new one.
// The function also updates the status and conditions of the NonAdminBackup resource to reflect the state
// of the VeleroBackup.
//
// Parameters:
//
// ctx: Context for the request.
// log: Logger instance for logging messages.
// logrLogger: Logger instance for logging messages.
// nab: Pointer to the NonAdminBackup object.
//
// The function generates the name for the Velero Backup object based on the provided namespace and name.
// It then checks if a Velero Backup object with that name already exists. If it does not exist, it creates a new one
// and updates NonAdminBackup Status. Otherwise, updates NonAdminBackup VeleroBackup Spec and Status based on Velero Backup object Spec and Status.
// The function returns boolean values indicating whether the reconciliation loop should exit or requeue
func (r *NonAdminBackupReconciler) UpdateSpecStatus(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) {
func (r *NonAdminBackupReconciler) SyncVeleroBackupWithNonAdminBackup(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) {
logger := logrLogger

veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name)
Expand Down Expand Up @@ -308,33 +306,22 @@ func (r *NonAdminBackupReconciler) UpdateSpecStatus(ctx context.Context, logrLog
return false, true, nil
}

// We should not update already created VeleroBackup object.
// The VeleroBackup within NonAdminBackup will
// be reverted back to the previous state - the state which created VeleroBackup
// in a first place, so they will be in sync.
veleroBackupLogger.Info("VeleroBackup already exists, checking if NonAdminBackup VeleroBackupSpec and VeleroBackupStatus needs update")
// Ensure that the NonAdminBackup's NonAdminBackupStatus is in sync
// with the VeleroBackup. Any required updates to the NonAdminBackup
// Status will be applied based on the current state of the VeleroBackup.
veleroBackupLogger.Info("VeleroBackup already exists, verifying if NonAdminBackup Status requires update")
updated = updateNonAdminBackupVeleroBackupStatus(&nab.Status, &veleroBackup)
if updated {
if err := r.Status().Update(ctx, nab); err != nil {
veleroBackupLogger.Error(err, "NonAdminBackup BackupStatus - Failed to update")
veleroBackupLogger.Error(err, "Failed to update NonAdminBackup Status after VeleroBackup reconciliation")
return true, false, err
}

logger.V(1).Info("NonAdminBackup - Requeue after Status Update")
logger.V(1).Info("NonAdminBackup Status updated successfully, requeuing")
return false, true, nil
}
updated = updateNonAdminBackupVeleroBackupSpec(&nab.Spec, &veleroBackup)
if updated {
if err := r.Update(ctx, nab); err != nil {
veleroBackupLogger.Error(err, "NonAdminBackup BackupSpec - Failed to update")
return true, false, err
}

logger.V(1).Info("NonAdminBackup - Requeue after Spec Update")
return false, true, nil
}

logger.V(1).Info("NonAdminBackup VeleroBackupSpec and VeleroBackupStatus already up to date")
logger.V(1).Info("NonAdminBackup Status is already up to date")
return false, false, nil
}

Expand Down Expand Up @@ -379,13 +366,3 @@ func updateNonAdminBackupVeleroBackupStatus(status *nacv1alpha1.NonAdminBackupSt
}
return false
}

// updateNonAdminBackupVeleroBackupSpec sets the BackupSpec in NonAdminBackup object spec and returns true
// if the BackupSpec is changed by this call.
func updateNonAdminBackupVeleroBackupSpec(spec *nacv1alpha1.NonAdminBackupSpec, veleroBackup *velerov1api.Backup) bool {
if !reflect.DeepEqual(spec.BackupSpec, &veleroBackup.Spec) {
spec.BackupSpec = veleroBackup.Spec.DeepCopy()
return true
}
return false
}
90 changes: 18 additions & 72 deletions internal/controller/nonadminbackup_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (

"github.com/onsi/ginkgo/v2"
"github.com/onsi/gomega"
"github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
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/apimachinery/pkg/types"
Expand Down Expand Up @@ -209,12 +209,12 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
gomega.Expect(k8sClient.Create(ctx, nonAdminBackup)).To(gomega.Succeed())

if scenario.createVeleroBackup {
veleroBackup := &v1.Backup{
veleroBackup := &velerov1.Backup{
ObjectMeta: metav1.ObjectMeta{
Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName),
Namespace: oadpNamespaceName,
},
Spec: v1.BackupSpec{
Spec: velerov1.BackupSpec{
IncludedNamespaces: []string{nonAdminNamespaceName},
},
}
Expand Down Expand Up @@ -268,7 +268,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
if len(scenario.priorStatus.VeleroBackupName) > 0 {
gomega.Expect(reflect.DeepEqual(
nonAdminBackup.Spec.BackupSpec,
&v1.BackupSpec{
&velerov1.BackupSpec{
IncludedNamespaces: []string{
nonAdminNamespaceName,
},
Expand All @@ -290,7 +290,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
}),
ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new), should update NonAdminBackup Condition to Accepted True and Requeue", nonAdminBackupSingleReconcileScenario{
spec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &v1.BackupSpec{},
BackupSpec: &velerov1.BackupSpec{},
},
priorStatus: &nacv1alpha1.NonAdminBackupStatus{
Phase: nacv1alpha1.NonAdminBackupPhaseNew,
Expand All @@ -310,7 +310,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
}),
ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True), should update NonAdminBackup phase to created and Requeue", nonAdminBackupSingleReconcileScenario{
spec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &v1.BackupSpec{},
BackupSpec: &velerov1.BackupSpec{},
},
priorStatus: &nacv1alpha1.NonAdminBackupStatus{
Phase: nacv1alpha1.NonAdminBackupPhaseNew,
Expand Down Expand Up @@ -341,7 +341,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase created; Conditions Accepted True), should update NonAdminBackup Condition to Queued True and Requeue", nonAdminBackupSingleReconcileScenario{
createVeleroBackup: true,
spec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &v1.BackupSpec{},
BackupSpec: &velerov1.BackupSpec{},
},
priorStatus: &nacv1alpha1.NonAdminBackupStatus{
Phase: nacv1alpha1.NonAdminBackupPhaseCreated,
Expand Down Expand Up @@ -378,7 +378,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase created; Conditions Accepted True, Queued True), should update NonAdminBackup VeleroBackupStatus and Requeue", nonAdminBackupSingleReconcileScenario{
createVeleroBackup: true,
spec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &v1.BackupSpec{},
BackupSpec: &velerov1.BackupSpec{},
},
priorStatus: &nacv1alpha1.NonAdminBackupStatus{
Phase: nacv1alpha1.NonAdminBackupPhaseCreated,
Expand All @@ -403,7 +403,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
Phase: nacv1alpha1.NonAdminBackupPhaseCreated,
VeleroBackupName: placeholder,
VeleroBackupNamespace: placeholder,
VeleroBackupStatus: &v1.BackupStatus{},
VeleroBackupStatus: &velerov1.BackupStatus{},
Conditions: []metav1.Condition{
{
Type: "Accepted",
Expand All @@ -421,59 +421,9 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
},
result: reconcile.Result{Requeue: true},
}),
ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase created; Conditions Accepted True, Queued True; VeleroBackupStatus), should update NonAdminBackup spec BackupSpec and Requeue", nonAdminBackupSingleReconcileScenario{
createVeleroBackup: true,
spec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &v1.BackupSpec{},
},
priorStatus: &nacv1alpha1.NonAdminBackupStatus{
Phase: nacv1alpha1.NonAdminBackupPhaseCreated,
VeleroBackupName: placeholder,
VeleroBackupNamespace: placeholder,
VeleroBackupStatus: &v1.BackupStatus{},
Conditions: []metav1.Condition{
{
Type: "Accepted",
Status: metav1.ConditionTrue,
Reason: "BackupAccepted",
Message: "backup accepted",
LastTransitionTime: metav1.NewTime(time.Now()),
},
{
Type: "Queued",
Status: metav1.ConditionTrue,
Reason: "BackupScheduled",
Message: "Created Velero Backup object",
LastTransitionTime: metav1.NewTime(time.Now()),
},
},
},
ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{
Phase: nacv1alpha1.NonAdminBackupPhaseCreated,
VeleroBackupName: placeholder,
VeleroBackupNamespace: placeholder,
VeleroBackupStatus: &v1.BackupStatus{},
Conditions: []metav1.Condition{
{
Type: "Accepted",
Status: metav1.ConditionTrue,
Reason: "BackupAccepted",
Message: "backup accepted",
},
{
Type: "Queued",
Status: metav1.ConditionTrue,
Reason: "BackupScheduled",
Message: "Created Velero Backup object",
},
},
},
// TODO should not exit?
result: reconcile.Result{Requeue: true},
}),
ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new) [invalid spec], should update NonAdminBackup phase to BackingOff and Requeue", nonAdminBackupSingleReconcileScenario{
spec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &v1.BackupSpec{
BackupSpec: &velerov1.BackupSpec{
IncludedNamespaces: []string{"not-valid"},
},
},
Expand All @@ -488,7 +438,7 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func
ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase BackingOff), should update NonAdminBackup Condition to Accepted False and stop with terminal error", nonAdminBackupSingleReconcileScenario{
// TODO this validates spec again...
spec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &v1.BackupSpec{
BackupSpec: &velerov1.BackupSpec{
IncludedNamespaces: []string{"not-valid"},
},
},
Expand Down Expand Up @@ -583,15 +533,11 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller",
ginkgo.By("Validating NonAdminBackup Spec")
gomega.Expect(reflect.DeepEqual(
nonAdminBackup.Spec.BackupSpec,
&v1.BackupSpec{
IncludedNamespaces: []string{
nonAdminNamespaceName,
},
},
&velerov1.BackupSpec{},
)).To(gomega.BeTrue())

ginkgo.By("Simulating VeleroBackup update to finished state")
veleroBackup := &v1.Backup{}
veleroBackup := &velerov1.Backup{}
gomega.Expect(k8sClient.Get(
ctx,
types.NamespacedName{
Expand All @@ -600,7 +546,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller",
},
veleroBackup,
)).To(gomega.Succeed())
veleroBackup.Status.Phase = v1.BackupPhaseCompleted
veleroBackup.Status.Phase = velerov1.BackupPhaseCompleted
// TODO can not call .Status().Update() for veleroBackup object: backups.velero.io "name..." not found error
gomega.Expect(k8sClient.Update(ctx, veleroBackup)).To(gomega.Succeed())

Expand All @@ -616,7 +562,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller",
if err != nil {
return false, err
}
return nonAdminBackup.Status.VeleroBackupStatus.Phase == v1.BackupPhaseCompleted, nil
return nonAdminBackup.Status.VeleroBackupStatus.Phase == velerov1.BackupPhaseCompleted, nil
}, 5*time.Second, 1*time.Second).Should(gomega.BeTrue())
}

Expand All @@ -626,13 +572,13 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller",
},
ginkgo.Entry("Should update NonAdminBackup until VeleroBackup completes and then delete it", nonAdminBackupFullReconcileScenario{
spec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &v1.BackupSpec{},
BackupSpec: &velerov1.BackupSpec{},
},
status: nacv1alpha1.NonAdminBackupStatus{
Phase: nacv1alpha1.NonAdminBackupPhaseCreated,
VeleroBackupName: placeholder,
VeleroBackupNamespace: placeholder,
VeleroBackupStatus: &v1.BackupStatus{},
VeleroBackupStatus: &velerov1.BackupStatus{},
Conditions: []metav1.Condition{
{
Type: "Accepted",
Expand All @@ -651,7 +597,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller",
}),
ginkgo.Entry("Should update NonAdminBackup until it invalidates and then delete it", nonAdminBackupFullReconcileScenario{
spec: nacv1alpha1.NonAdminBackupSpec{
BackupSpec: &v1.BackupSpec{
BackupSpec: &velerov1.BackupSpec{
IncludedNamespaces: []string{"not-valid"},
},
},
Expand Down

0 comments on commit 78a11b6

Please sign in to comment.