fix: add integration tests for NAB #73

mateusoliveira43 · 2024-08-28T13:54:36Z

Why the changes were made

Add Integration tests for NAB, so we can trust our code base more

Fixes #61

Also

removed duplication in logs
removed unnecessary validations
made each reconcile only execute one update call per object, to avoid race conditions
removed terminal error from wrong places

Can help on refactor of #62

How to test the changes made

To run integration tests (and also unit tests), run make simulation-test

Check Test logs to see what is being tested

To check code coverage, run go tool cover -html=cover.out

Also check #73 (comment)

openshift-ci · 2024-08-28T13:54:41Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: Mateus Oliveira <[email protected]>

go.mod

Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 · 2024-09-06T16:14:17Z

internal/controller/nonadminbackup_controller_test.go

I suggest running these test against master branch to see the current problems

To do so, in master branch run

git restore -s fix/add-integration-tests-nab hack/extra-crds/velero.io_backups.yaml git restore -s fix/add-integration-tests-nab internal/controller/suite_test.go git restore -s fix/add-integration-tests-nab internal/controller/nonadminbackup_controller_test.go git restore -s fix/add-integration-tests-nab cmd/main.go git restore -s fix/add-integration-tests-nab Makefile git apply test-master.diff make simulation-test

where test-master.diff is

diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index dcec107..840c4ce 100644 --- a/internal/common/function/function_test.go +++ b/internal/common/function/function_test.go @@ -22,6 +22,7 @@ import ( "reflect" "testing" + "github.com/onsi/ginkgo/v2" "github.com/stretchr/testify/assert" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -35,6 +36,8 @@ import ( "github.com/migtools/oadp-non-admin/internal/common/constant" ) +var _ = ginkgo.Describe("PLACEHOLDER", func() {}) + func TestMergeMaps(t *testing.T) { const ( d = "d" diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 85456c2..d05590b 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -20,7 +20,6 @@ package controller import ( "context" "errors" - "time" "github.com/go-logr/logr" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -43,8 +42,9 @@ import ( // NonAdminBackupReconciler reconciles a NonAdminBackup object type NonAdminBackupReconciler struct { client.Client - Scheme *runtime.Scheme - Context context.Context + Scheme *runtime.Scheme + Context context.Context + OADPNamespace string } const ( @@ -89,7 +89,7 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque reconcileExit, reconcileRequeue, reconcileErr := r.InitNonAdminBackup(ctx, rLog, &nab) if reconcileRequeue { - return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + return ctrl.Result{Requeue: true}, reconcileErr } else if reconcileExit && reconcileErr != nil { return ctrl.Result{}, reconcile.TerminalError(reconcileErr) } else if reconcileExit { @@ -98,7 +98,7 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque reconcileExit, reconcileRequeue, reconcileErr = r.ValidateVeleroBackupSpec(ctx, rLog, &nab) if reconcileRequeue { - return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + return ctrl.Result{Requeue: true}, reconcileErr } else if reconcileExit && reconcileErr != nil { return ctrl.Result{}, reconcile.TerminalError(reconcileErr) } else if reconcileExit { @@ -107,7 +107,7 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque reconcileExit, reconcileRequeue, reconcileErr = r.CreateVeleroBackupSpec(ctx, rLog, &nab) if reconcileRequeue { - return ctrl.Result{Requeue: true, RequeueAfter: requeueTimeSeconds * time.Second}, reconcileErr + return ctrl.Result{Requeue: true}, reconcileErr } else if reconcileExit && reconcileErr != nil { return ctrl.Result{}, reconcile.TerminalError(reconcileErr) } else if reconcileExit { @@ -235,7 +235,7 @@ func (r *NonAdminBackupReconciler) CreateVeleroBackupSpec(ctx context.Context, l } veleroBackup := velerov1api.Backup{} - err := r.Get(ctx, client.ObjectKey{Namespace: constant.OadpNamespace, Name: veleroBackupName}, &veleroBackup) + err := r.Get(ctx, client.ObjectKey{Namespace: r.OADPNamespace, Name: veleroBackupName}, &veleroBackup) if err != nil && apierrors.IsNotFound(err) { // Create VeleroBackup @@ -255,7 +255,7 @@ func (r *NonAdminBackupReconciler) CreateVeleroBackupSpec(ctx context.Context, l veleroBackup = velerov1api.Backup{ ObjectMeta: metav1.ObjectMeta{ Name: veleroBackupName, - Namespace: constant.OadpNamespace, + Namespace: r.OADPNamespace, }, Spec: *backupSpec, } @@ -325,7 +325,7 @@ func (r *NonAdminBackupReconciler) SetupWithManager(mgr ctrl.Manager) error { WithEventFilter(predicate.CompositePredicate{ NonAdminBackupPredicate: predicate.NonAdminBackupPredicate{}, VeleroBackupPredicate: predicate.VeleroBackupPredicate{ - OadpVeleroNamespace: constant.OadpNamespace, + OadpVeleroNamespace: r.OADPNamespace, }, Context: r.Context, }).

internal/controller/nonadminbackup_controller.go

Signed-off-by: Mateus Oliveira <[email protected]>

internal/common/function/function.go

internal/controller/nonadminbackup_controller.go

shubham-pampattiwar · 2024-09-11T06:56:19Z

internal/controller/nonadminbackup_controller.go

-// It then checks if a Velero Backup object with that name already exists. If it does not exist, it creates a new one.
+// 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.


We should not update the NAB Velerospec from Velero backup object spec here, its fine if Velero adds some default values, its not the responsibility of the controller to update spec, specs are user defined or by external entity, controller's job is to get the object to the desired state and provide updates on status in doing so. The spec update to NAB object will also generate an event to trigger reconcile which we don't want.

Discuss in meeting

shubham-pampattiwar · 2024-09-11T06:57:16Z

internal/controller/nonadminbackup_controller.go

 // The function returns boolean values indicating whether the reconciliation loop should exit or requeue
-func (r *NonAdminBackupReconciler) CreateVeleroBackupSpec(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) {
-	logger := logrLogger.WithValues("CreateVeleroBackupSpec", nab.Namespace)
+func (r *NonAdminBackupReconciler) UpdateSpecStatus(ctx context.Context, logrLogger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (exitReconcile bool, requeueReconcile bool, errorReconcile error) {


Lets changes the function name to some thing like ProcessNABSpec or something similar

The original function name was CreateVeleroBackupSpec which I think was correct. It reflected we are creating it not updating nor Processing.

It did update NAB when VeleroBackup updates

oadp-non-admin/internal/controller/nonadminbackup_controller.go

Lines 265 to 281 in 4b2f57f

} else {

// 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.

logger.Info("Backup already exists, updating NonAdminBackup status", nameField, veleroBackupName)

updatedNab, errBackupUpdate := function.UpdateNonAdminBackupFromVeleroBackup(ctx, r.Client, logger, nab, &veleroBackup)

// Regardless if the status was updated or not, we should not

// requeue here as it was only status update.

if errBackupUpdate != nil {

return true, false, errBackupUpdate

} else if updatedNab {

logger.V(1).Info("NonAdminBackup CR - Rqueue after Status Update")

return false, true, nil

}

return true, false, nil

}

we should break this function into smaller ones, it does too many things. But I tried to changed as little as possible in this PR

I suggest follow up with break up of function. Do you agree?

internal/controller/nonadminbackup_controller.go

internal/predicate/velerobackup_predicate.go

shubham-pampattiwar · 2024-09-11T07:25:24Z

OK at the high level this is starting to look like:

We have a main Reconcile function for NAB controller
This controller has a bunch of tasks: Init, ValidateSpec and so on...
Now each task returns exitReconcile bool, requeueReconcile bool, errorReconcile error
IMO as I had said before too we could structure this similar to the OADP DPA reconciler
It wont be the exact same but refactoring would be similar.
In our NAB controller case, in addition to what the controller tasks currently return exitReconcile bool, requeueReconcile bool, errorReconcile error, we could add one more struct called NABUpdate This struct would consist of the phase and condition that needs to be updated for NAB object in that particular reconcile loop and the status update call could be executed at the end of the reconcile loop in a consolidated manner.
All the above changes would simplify the code workflow a lot, would be easier to understand/debug and make the code maintainable as well.

Signed-off-by: Mateus Oliveira <[email protected]>

internal/controller/nonadminbackup_controller_test.go

shubham-pampattiwar · 2024-09-16T21:00:17Z

internal/controller/nonadminbackup_controller_test.go

+		gomega.Expect(deleteTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed())
+	})
+
+	ginkgo.DescribeTable("Reconcile called by NonAdminBackup Delete event",


Lets change this test describe, we can say some thing like "NAB object does not exist case"

Is not better to keep as it is for consistency?

the other test describe is Reconcile triggered by NonAdminBackup Create/Update events and by Requeue

internal/controller/nonadminbackup_controller_test.go

shubham-pampattiwar · 2024-09-16T23:56:22Z

internal/controller/nonadminbackup_controller_test.go

+			},
+			result: reconcile.Result{Requeue: true},
+		}),
+		ginkgo.Entry("When called by Requeue(update NonAdminBackup Condition to Queued True), should update NonAdminBackup VeleroBackupStatus and Requeue", nonAdminBackupSingleReconcileScenario{


This test is failing when executed independently/focused. (For Velero Backup name counter seems incorrect, expects a 6th reconcile instead of first) I am assuming these tests should always be executed together ?

I think I fixed the problem in next commit, please test when possible

internal/controller/nonadminbackup_controller_test.go

shubham-pampattiwar · 2024-09-16T23:59:23Z

internal/controller/nonadminbackup_controller_test.go

+			},
+			result: reconcile.Result{Requeue: true},
+		}),
+		ginkgo.Entry("When called by Requeue(update NonAdminBackup VeleroBackupStatus), should update NonAdminBackup spec BackupSpec and Requeue", nonAdminBackupSingleReconcileScenario{


Lets hold off on adding this test as we are gonna remove this task from NAB reconcile loop.

internal/controller/nonadminbackup_controller_test.go

shubham-pampattiwar · 2024-09-17T00:09:04Z

internal/controller/nonadminbackup_controller_test.go

+			gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.status)).To(gomega.Succeed())
+			if scenario.priorStatus != nil {
+				if len(scenario.priorStatus.VeleroBackupName) > 0 {
+					gomega.Expect(reflect.DeepEqual(


Is this something incomplete or residual/debug code ?
We are comparing the NAB.spec.backupSpec with VeleroBackupSpec with just IncludedNS. Let me know if am missing something here.

just to confirm NAB spec was updated by reconcile loop

Signed-off-by: Mateus Oliveira <[email protected]>

mpryc

Tested on a cluster, looks ok, there is one gotcha which is different then previous workflow:

Create NAB 'mynab' -> Backup is created -> Delete NAB -> Create NAB 'mynab'

With this PR the NAB get's the accepted / created state with an old Backup being in place.

Previously those were final on the Backup object where UUID was pointing to the deleted NAB:

  annotations:
    openshift.io/oadp-nab-origin-name: example
    openshift.io/oadp-nab-origin-namespace: default
    openshift.io/oadp-nab-origin-uuid: 463057c2-6d57-4be5-9a71-fc12723f7cb4

Currently it's the same, but the NAB object gets updated to point to Backup which now mismatches the uuid. Previously the NAB was not updated to get into accepted state and was left on New state.

Signed-off-by: Mateus Oliveira <[email protected]>

mpryc

/lgtm

mateusoliveira43 · 2024-09-24T13:58:26Z

/hold

shubham-pampattiwar · 2024-09-24T14:01:21Z

Can follow up rest of the updates

openshift-ci · 2024-09-24T14:01:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43, mpryc, shubham-pampattiwar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mateusoliveira43,mpryc,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

shubham-pampattiwar · 2024-09-24T14:01:47Z

/unhold

openshift-ci bot added the do-not-merge/work-in-progress label Aug 28, 2024

openshift-ci bot added the approved label Aug 28, 2024

mateusoliveira43 force-pushed the fix/add-integration-tests-nab branch from b8aa9c2 to 8082cb3 Compare August 28, 2024 20:57

mateusoliveira43 added 5 commits September 3, 2024 09:46

fix: add integration tests for NAB

01fce67

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: add integration tests for NAB

3d34406

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: add integration tests for NAB

0544117

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: add integration tests for NAB

a5427ef

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: add integration tests for NAB

698144a

Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 force-pushed the fix/add-integration-tests-nab branch from ab565eb to 698144a Compare September 3, 2024 12:47

fixup! fix: add integration tests for NAB

2a6dd14

Signed-off-by: Mateus Oliveira <[email protected]>

mpryc reviewed Sep 4, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

mateusoliveira43 added 5 commits September 4, 2024 10:54

fixup! fix: add integration tests for NAB

2c2fb3d

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: add integration tests for NAB

492a54f

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: add integration tests for NAB

cc03f93

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: add integration tests for NAB

4fd01fa

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: add integration tests for NAB

cbcffa4

Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 marked this pull request as ready for review September 6, 2024 15:52

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 6, 2024

openshift-ci bot requested review from shubham-pampattiwar and sseago September 6, 2024 15:52

mateusoliveira43 commented Sep 6, 2024

View reviewed changes

internal/controller/nonadminbackup_controller.go Show resolved Hide resolved

mateusoliveira43 added 2 commits September 8, 2024 14:18

fixup! fix: add integration tests for NAB

6b1b570

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: add integration tests for NAB

aa02b5f

Signed-off-by: Mateus Oliveira <[email protected]>

shubham-pampattiwar reviewed Sep 11, 2024

View reviewed changes

fixup! fix: add integration tests for NAB

1a4f4ce

Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 mentioned this pull request Sep 11, 2024

Consider simplifying logic in reconcile loop for NAB. #62

Closed

fixup! fix: add integration tests for NAB

55a34e6

Signed-off-by: Mateus Oliveira <[email protected]>

shubham-pampattiwar reviewed Sep 17, 2024

View reviewed changes

fixup! fix: add integration tests for NAB

03e4b9f

Signed-off-by: Mateus Oliveira <[email protected]>

mpryc previously approved these changes Sep 23, 2024

View reviewed changes

mateusoliveira43 mentioned this pull request Sep 24, 2024

Reject NAB CR creation calls for orphaned Velero Backup objects #84

Open

fixup! fix: add integration tests for NAB

94dfd83

Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 dismissed mpryc’s stale review via 94dfd83 September 24, 2024 13:36

mpryc approved these changes Sep 24, 2024

View reviewed changes

openshift-ci bot assigned mpryc Sep 24, 2024

openshift-ci bot added the lgtm label Sep 24, 2024

openshift-ci bot added the do-not-merge/hold label Sep 24, 2024

shubham-pampattiwar approved these changes Sep 24, 2024

View reviewed changes

openshift-ci bot removed the do-not-merge/hold label Sep 24, 2024

openshift-merge-bot bot merged commit f50ab81 into migtools:master Sep 24, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add integration tests for NAB #73

fix: add integration tests for NAB #73

mateusoliveira43 commented Aug 28, 2024 •

edited

Loading

openshift-ci bot commented Aug 28, 2024

mateusoliveira43 Sep 6, 2024 •

edited

Loading

shubham-pampattiwar Sep 11, 2024

mateusoliveira43 Sep 11, 2024

shubham-pampattiwar Sep 11, 2024

mpryc Sep 11, 2024

mateusoliveira43 Sep 11, 2024

shubham-pampattiwar commented Sep 11, 2024

shubham-pampattiwar Sep 16, 2024

mateusoliveira43 Sep 24, 2024

shubham-pampattiwar Sep 16, 2024

mateusoliveira43 Sep 20, 2024

shubham-pampattiwar Sep 16, 2024

shubham-pampattiwar Sep 17, 2024

mateusoliveira43 Sep 20, 2024

mpryc left a comment •

edited

Loading

mpryc left a comment

mateusoliveira43 commented Sep 24, 2024

shubham-pampattiwar commented Sep 24, 2024

openshift-ci bot commented Sep 24, 2024

shubham-pampattiwar commented Sep 24, 2024

	} else {
	// 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.
	logger.Info("Backup already exists, updating NonAdminBackup status", nameField, veleroBackupName)
	updatedNab, errBackupUpdate := function.UpdateNonAdminBackupFromVeleroBackup(ctx, r.Client, logger, nab, &veleroBackup)
	// Regardless if the status was updated or not, we should not
	// requeue here as it was only status update.
	if errBackupUpdate != nil {
	return true, false, errBackupUpdate
	} else if updatedNab {
	logger.V(1).Info("NonAdminBackup CR - Rqueue after Status Update")
	return false, true, nil
	}
	return true, false, nil
	}

fix: add integration tests for NAB #73

fix: add integration tests for NAB #73

Conversation

mateusoliveira43 commented Aug 28, 2024 • edited Loading

Why the changes were made

How to test the changes made

openshift-ci bot commented Aug 28, 2024

mateusoliveira43 Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham-pampattiwar commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpryc left a comment • edited Loading

Choose a reason for hiding this comment

mpryc left a comment

Choose a reason for hiding this comment

mateusoliveira43 commented Sep 24, 2024

shubham-pampattiwar commented Sep 24, 2024

openshift-ci bot commented Sep 24, 2024

shubham-pampattiwar commented Sep 24, 2024

mateusoliveira43 commented Aug 28, 2024 •

edited

Loading

mateusoliveira43 Sep 6, 2024 •

edited

Loading

mpryc left a comment •

edited

Loading