support replication of MantleBackup and PVC objects #37

ushitora-anqou · 2024-09-05T08:58:54Z

No description provided.

This commit changes the type of MantleBackup's SnapID field from int to *int so that we can check if the value is missing or is zero. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

This commit avoid skipping MantleBackup's reconciliation early when ReadyToUse is true, so that we can make sure that replication process is performed even when the reconciliation is retried for some reasons after ReadyToUse becomes true. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

The current implementation updates MantleBackup's status without getting its present content. Thus, if we try to update MantleBackup's status several times in one reconciliation iteration, we fail to do so because its resourceVersion field is invalid in the second update. In order to resolve the problem above, this commit introduces a new function named updateMantleBackupStatus. It not only gets the status every time before updating it, but also allows us to update it in a similiar manner to ctrl.CreateOrUpdate. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

…hotting Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

satoru-takeuchi

I reviewd 6 commits. I'll let you know after finishing the remaining review of two commits.

satoru-takeuchi · 2024-09-12T01:01:50Z

internal/controller/mantlebackup_controller.go

@@ -105,8 +113,10 @@ func executeCommandImpl(command []string, input io.Reader) ([]byte, error) {
 var executeCommand = executeCommandImpl

 func (r *MantleBackupReconciler) updateStatus(ctx context.Context, backup *mantlev1.MantleBackup, condition metav1.Condition) error {


How about renaming this function to updateStatusCondition and updateMantleBackupStatus to updateStatus? It's a bit complicated to understand why both updateStatus() and updateMantleBackupStatus are for updating MantleBackup.status and the former calls the latter inside it. By the above mentioned renaming, the purpose of both function will be clear. The new UpdateStatus is to update any fields of status. On the other hand, updateStatusCondition is only to update status.conditions.

I fixed this in the following commits:
4c40f2b
7e21cad

satoru-takeuchi · 2024-09-12T01:17:47Z

internal/controller/mantlebackup_controller.go

@@ -332,10 +332,6 @@ func (r *MantleBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		return ctrl.Result{Requeue: true}, nil
 	}

-	if cond := meta.FindStatusCondition(backup.Status.Conditions, mantlev1.BackupConditionReadyToUse); cond != nil && cond.Status == metav1.ConditionTrue {


How about create a new function for snapshot creation and call this only if conditions["ReadyToUse"] is "True"? It improved the readbility of Reconcile. In addition, we can reduce the number of k8s API calls and RBD command executions.

Fixed in the following commits:
8a27991
eda8e4c

satoru-takeuchi

I read all commits in your first version.

satoru-takeuchi · 2024-09-12T01:31:02Z

internal/controller/mantlebackup_controller.go

+	labelLocalBackupTargetPVCUID  = "local-backup-target-pvc-uid"
+	labelRemoteBackupTargetPVCUID = "remote-backup-target-pvc-uid"


Could you add the prefix, "mantle.cybozu.io/", before both label names (like "mantle.cybozu.io/local-backup-target-pvc-uid")? It is to apply unique name to the mantle-related labels.

Fixed in the following commits:

0b44995

satoru-takeuchi · 2024-09-12T01:47:36Z

internal/controller/mantlebackup_controller.go

+
+	// Call CreateOrUpdatePVC
+	client := r.primarySettings.Client
+	respPVC, err := client.CreateOrUpdatePVC(


Suggested change

respPVC, err := client.CreateOrUpdatePVC(

resp, err := client.CreateOrUpdatePVC(

Because the response of CreateOrUpdatePVC does not contain PVC's full definition. It only contains uid.

Fixed in 0b44995

satoru-takeuchi · 2024-09-12T02:08:09Z

test/e2e/multik8s/suite_test.go

+					return errors.New("finalizer not found")
+				}
+				if mb.Labels == nil ||
+					mb.Labels["local-backup-target-pvc-uid"] != string(secondaryPVC.GetUID()) &&


Suggested change

mb.Labels["local-backup-target-pvc-uid"] != string(secondaryPVC.GetUID()) &&

mb.Labels["local-backup-target-pvc-uid"] != string(secondaryPVC.GetUID()) ||

Fixed in 0b44995

satoru-takeuchi · 2024-09-12T02:09:48Z

test/e2e/multik8s/suite_test.go

+					return err
+				}
+
+				mb, err := getMB(secondaryK8sCluster, namespace, backupName)


Suggested change

mb, err := getMB(secondaryK8sCluster, namespace, backupName)

secondaryMB, err := getMB(secondaryK8sCluster, namespace, backupName)

Fixed in 0b44995

satoru-takeuchi · 2024-09-12T02:19:40Z

internal/controller/replication.go

+	}
+
+	if backupReceived.Labels == nil || backupReceived.Annotations == nil {
+		return nil, fmt.Errorf("received MantleBackup is invalid: %s: %s",


Suggested change

return nil, fmt.Errorf("received MantleBackup is invalid: %s: %s",

return nil, fmt.Errorf("both labels and annotations must not be nil in the received MantleBackup: %s: %s",

Fixed in 0b44995

satoru-takeuchi · 2024-09-12T02:25:21Z

internal/controller/replication.go

+			// Make sure the remote-uids are equal.
+			errMsg := ""
+			if pvc.Annotations == nil {
+				errMsg = "annotation is nil in pvc"


Suggested change

errMsg = "annotation is nil in pvc"

errMsg = "annotations field is nil in pvc"

Fixed in 0b44995

satoru-takeuchi · 2024-09-12T02:25:45Z

internal/controller/replication.go

+		if !backup.CreationTimestamp.IsZero() {
+			errMsg := ""
+			if backup.Labels == nil {
+				errMsg = "label not found"


Suggested change

errMsg = "label not found"

errMsg = "labes field is in in backup"

Fixed in 0b44995

satoru-takeuchi · 2024-09-12T02:26:12Z

internal/controller/replication.go

+			}
+
+			if backup.Annotations == nil {
+				errMsg = "annotation is nil in backup"


Suggested change

errMsg = "annotation is nil in backup"

errMsg = "annotation field is nil in backup"

Fixed in 0b44995

satoru-takeuchi · 2024-09-12T02:47:41Z

internal/controller/mantlebackup_controller.go

+		}
+	}
+
+	// Attach local-backup-target-pvc-uid label


Could you enrich the comment to let reads to know this CreateOrUpdate is to assure that snapshot corresponding to the MantleBackup only exists iff local-backup-target-pvc-uid is applied.

Fixed in 95d7d5c

satoru-takeuchi · 2024-09-12T02:51:38Z

internal/controller/mantlebackup_controller.go

+		_, ok2 := backup.Labels[labelRemoteBackupTargetPVCUID]
+		if ok1 && ok2 {
+			logger.Warn(
+				"skipping to reconcile the MantleBackup because it has both local/remote-backup-target-pvc-uid labels",


Suggested change

"skipping to reconcile the MantleBackup because it has both local/remote-backup-target-pvc-uid labels",

"skipping to reconcile the MantleBackup created by a remote mantle-controller to prevent accidental data loss",

Fixed in 6cb0005

ushitora-anqou · 2024-09-12T05:37:10Z

@satoru-takeuchi I've fixed your points, so could you review this PR again? Note that we need to squash my commits before merging this PR, so please do not merge this even if there are no flaws.

satoru-takeuchi

LTGM. Please squash commits.

This commit introduces a functionality of replicating PVC and MantleBackup resources. Also, it adds some e2e tests to make sure that the replication works correctly. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

In the current implementation, the ctrl.Result value returned from MantleBackupReconciler.createRBDSnapshot is useless, because: - the ctrl.Result value is used only if the err value is not nil. - the ctrl.Result value equals ctrl.Result{} whenever the err value is not nil. This commit stops returning the value. We can use ctrl.Result{} instead. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

ushitora-anqou force-pushed the support-repl-of-mb-and-pvc-objects branch from 5aa69ce to 07addee Compare September 6, 2024 02:49

ushitora-anqou added 5 commits September 10, 2024 02:12

change type of MantleBackup's SnapID field to *int

c867217

This commit changes the type of MantleBackup's SnapID field from int to *int so that we can check if the value is missing or is zero. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

attach local-backup-target-pvc-uid label to MantleBackup before snaps…

abbdb30

…hotting Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

move MantleBackup's replication process into a new function

2d0e5b9

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

ushitora-anqou force-pushed the support-repl-of-mb-and-pvc-objects branch 2 times, most recently from de0cbae to b4a15d8 Compare September 10, 2024 06:14

ushitora-anqou requested a review from satoru-takeuchi September 10, 2024 07:24

ushitora-anqou marked this pull request as ready for review September 10, 2024 07:24

satoru-takeuchi requested changes Sep 12, 2024

View reviewed changes

ushitora-anqou force-pushed the support-repl-of-mb-and-pvc-objects branch from f0ad64a to 95d7d5c Compare September 12, 2024 04:58

ushitora-anqou requested a review from satoru-takeuchi September 12, 2024 05:37

satoru-takeuchi approved these changes Sep 12, 2024

View reviewed changes

ushitora-anqou added 8 commits September 12, 2024 07:07

support replication of PVC and MantleBackup resources

88fc179

This commit introduces a functionality of replicating PVC and MantleBackup resources. Also, it adds some e2e tests to make sure that the replication works correctly. Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

make sure reconciled MantleBackup is created by primary Mantle

cbb22aa

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

add Service for secondary mantle to mantle chart

f7297bb

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

rename MantleBackupReconciler.updateStatus to updateStatusCondition

63b2c82

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

generalize updateMantleBackupStatus and rename it to updateStatus

958ade2

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

try to create RBD snapshot only when ReadyToUse is not True

a7f5d1f

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

chore: improve comments

17ea4f2

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>

ushitora-anqou force-pushed the support-repl-of-mb-and-pvc-objects branch from 95d7d5c to 17ea4f2 Compare September 12, 2024 07:07

satoru-takeuchi merged commit ced1d71 into main Sep 12, 2024
3 checks passed

satoru-takeuchi deleted the support-repl-of-mb-and-pvc-objects branch September 12, 2024 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support replication of MantleBackup and PVC objects #37

support replication of MantleBackup and PVC objects #37

ushitora-anqou commented Sep 5, 2024

satoru-takeuchi left a comment

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024 •

edited

Loading

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024 •

edited

Loading

satoru-takeuchi left a comment

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024 •

edited

Loading

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024 •

edited

Loading

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024

satoru-takeuchi Sep 12, 2024

ushitora-anqou Sep 12, 2024

ushitora-anqou commented Sep 12, 2024

satoru-takeuchi left a comment

		@@ -105,8 +113,10 @@ func executeCommandImpl(command []string, input io.Reader) ([]byte, error) {
		var executeCommand = executeCommandImpl

		func (r MantleBackupReconciler) updateStatus(ctx context.Context, backup mantlev1.MantleBackup, condition metav1.Condition) error {

		labelLocalBackupTargetPVCUID = "local-backup-target-pvc-uid"
		labelRemoteBackupTargetPVCUID = "remote-backup-target-pvc-uid"

	respPVC, err := client.CreateOrUpdatePVC(
	resp, err := client.CreateOrUpdatePVC(

	mb.Labels["local-backup-target-pvc-uid"] != string(secondaryPVC.GetUID()) &&
	mb.Labels["local-backup-target-pvc-uid"] != string(secondaryPVC.GetUID()) \|\|

	mb, err := getMB(secondaryK8sCluster, namespace, backupName)
	secondaryMB, err := getMB(secondaryK8sCluster, namespace, backupName)

	return nil, fmt.Errorf("received MantleBackup is invalid: %s: %s",
	return nil, fmt.Errorf("both labels and annotations must not be nil in the received MantleBackup: %s: %s",

	errMsg = "annotation is nil in pvc"
	errMsg = "annotations field is nil in pvc"

	errMsg = "label not found"
	errMsg = "labes field is in in backup"

	errMsg = "annotation is nil in backup"
	errMsg = "annotation field is nil in backup"

	"skipping to reconcile the MantleBackup because it has both local/remote-backup-target-pvc-uid labels",
	"skipping to reconcile the MantleBackup created by a remote mantle-controller to prevent accidental data loss",

support replication of MantleBackup and PVC objects #37

support replication of MantleBackup and PVC objects #37

Conversation

ushitora-anqou commented Sep 5, 2024

satoru-takeuchi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushitora-anqou Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushitora-anqou Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

satoru-takeuchi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushitora-anqou Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushitora-anqou Sep 12, 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

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

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

ushitora-anqou commented Sep 12, 2024

satoru-takeuchi left a comment

Choose a reason for hiding this comment

ushitora-anqou Sep 12, 2024 •

edited

Loading

ushitora-anqou Sep 12, 2024 •

edited

Loading

ushitora-anqou Sep 12, 2024 •

edited

Loading

ushitora-anqou Sep 12, 2024 •

edited

Loading