feat(backup): multiple backup stores support #2182

mantissahz · 2023-09-28T09:40:42Z

Support multiple backup stores to create, delete, and update them.
Adding a flag ReadOnly in BackupTarget CR spec prevents backups from being created and stored on this remote backup target.
And provide HTTP endpoints to create, delete, update, and list secrets.

Ref: longhorn/longhorn#5411

mergify · 2023-10-25T06:20:18Z

This pull request is now in conflicts. Could you fix it @mantissahz? 🙏

mergify · 2023-10-27T01:51:46Z

This pull request is now in conflicts. Could you fix it @mantissahz? 🙏

innobead · 2023-11-22T00:47:05Z

Is this ready for review?

mantissahz · 2023-11-22T01:23:17Z

Is this ready for review?

@innobead Yes, but LEP longhorn/longhorn#6630 is still in review.

james-munson

Still reviewing, but one question already.

api/model.go

controller/backup_target_controller.go

webhook/resources/backup/mutator.go

james-munson

Just a couple of minor things.

controller/backup_target_controller.go

webhook/resources/backuptarget/validator.go

List backup volume CRs using a backup target name. List backup volume CRs using a volume name. Get a backup volume using a backup target name and a volume name. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Add the backup target mutator and add the finalizer into the backupTarget CR when mutating. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Add the volume mutation and validation to handle the field `Spec.BackupTargetName`. Add a new label "backup-target". ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Add fields `Spec.BackingImage` and `Spec.BackupTargetName` in the BackupBackingImage CRD ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

1. Add the deleting function. 2. Modify the synchronizing backup volumes and backup backing images from a backup target methods. 3. `sets.String` is deprecated and use generic `Set` instead ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Move backupTimer from setting controller to backup target controller Move AWS IAM Role Annotation logic to datastore/kubernetes.go ref: longhorn/longhorn 5411, 6947 Signed-off-by: James Lu <james.lu@suse.com>

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Remove adding `backup-target` back when node is updated. Remove backup targets when uninstalling. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Introduce a new filed `BackupTargetName` in `Volume.Spec` to point out where the volume will be backed up to. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Modify backup volume controller to support multiple backup stores support. Synchronize and handle the backup volumes information from different backup targets with the same backup volume name or not. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Modify backup controller to support multiple backup stores support. Synchronize and handle the backup information from different backup targets with the same backup volume name or not. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

coderabbitai

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (19)

webhook/resources/backupbackingimage/mutator.go (1)

44-47: Enhance type assertion error message

The error message could be more specific to help with debugging.

-		return nil, werror.NewInvalidError(fmt.Sprintf("%v is not a *longhorn.BackupBackingImage", newObj), "")
+		return nil, werror.NewInvalidError(fmt.Sprintf("expected *longhorn.BackupBackingImage but got %T", newObj), "")

k8s/pkg/apis/longhorn/v1beta2/backupbackingimage.go (2)

7-9: Enhance field documentation for BackingImage.

While the documentation is clear, consider adding more context about the backing image's purpose and relationship with the backup system.

Consider expanding the comment to:

-// The backing image name.
+// The name of the backing image to backup. This backing image must exist
+// in the same namespace as this backup resource.

7-13: Consider architectural implications of optional BackupTargetName.

The addition of BackupTargetName field aligns well with the PR's objective of supporting multiple backup stores. However, there are a few architectural considerations to address:

How will the system behave when BackupTargetName is not specified? Will it use a default backup target?
How does this interact with the ReadOnly flag mentioned in the PR objectives?
Should we document the relationship between BackupBackingImage and BackupTarget CRs?

Consider adding a comment in the spec documentation explaining these relationships and default behaviors.

webhook/resources/backingimage/mutator.go (1)

172-195: Improve error message clarity.

The error message could be more specific about what was searched and not found.

Consider this improvement:

-    return "", errors.Errorf("no matching backup found for URL %v and backing image %v", backupTargetURL, backingImageName)
+    return "", errors.Errorf("no backup target found matching URL %v and backing image %v in existing backup images", backupTargetURL, backingImageName)

manager/engine.go (1)

354-407: Add documentation for new backup target management methods.

Consider adding documentation for these methods to explain their purpose, parameters, and return values.

Add documentation like this:

+// CreateBackupTarget creates a new backup target if it doesn't exist
+// Parameters:
+//   backupTargetName: name of the backup target
+//   backupTargetSpec: specification for the backup target
+// Returns:
+//   The created or existing backup target and any error encountered
 func (m *VolumeManager) CreateBackupTarget(backupTargetName string, backupTargetSpec *longhorn.BackupTargetSpec) (*longhorn.BackupTarget, error) {

Implementation looks good!

The backup target management methods are well-implemented with proper:

Input validation
Error handling
Change detection
Spec updates

controller/backup_volume_controller.go (1)

277-285: Consider enhancing error messages with more context.

While the error handling is functional, the error messages could be more descriptive by including the backup target name and URL in the log messages.

-		log.WithError(err).Error("Failed to list backups from backup target")
+		log.WithError(err).WithFields(logrus.Fields{
+			"backupTarget": backupTargetClient.URL,
+			"volumeName":   remoteBackupVolumeName,
+		}).Error("Failed to list backups from backup target")

webhook/resources/volume/validator.go (1)

494-505: Consider improving error handling and documentation.

The implementation could benefit from the following improvements:

Add method documentation to explain its purpose and parameters.
Make the error message more specific about when empty values are not allowed.
Improve error wrapping with more context.

Consider applying these improvements:

+// validateBackupTarget ensures that the backup target exists and is valid.
+// It prevents empty backup targets during volume creation and validates target existence during updates.
+// Parameters:
+//   oldBackupTarget: The current backup target name (empty during volume creation)
+//   newBackupTarget: The new backup target name to validate
 func (v *volumeValidator) validateBackupTarget(oldBackupTarget, newBackupTarget string) error {
 	if newBackupTarget == "" {
-		return fmt.Errorf("backup target name cannot be empty when creating a volume or updating from an existing backup target")
+		if oldBackupTarget == "" {
+			return fmt.Errorf("backup target name is required when creating a volume")
+		}
+		return fmt.Errorf("backup target name cannot be cleared when updating from %q", oldBackupTarget)
 	}
 	if oldBackupTarget == newBackupTarget {
 		return nil
 	}
 	if _, err := v.ds.GetBackupTargetRO(newBackupTarget); err != nil {
-		return errors.Wrapf(err, "failed to get backup target %v", newBackupTarget)
+		return errors.Wrapf(err, "invalid backup target %q: target does not exist or is not accessible", newBackupTarget)
 	}
 	return nil
 }

controller/backup_target_controller.go (2)

385-399: Consider adding error handling for timer initialization.

While the timer initialization looks good, consider handling potential errors that could occur during timer startup to prevent silent failures.

 if btc.bsTimerMap[name] == nil && backupTarget.Spec.PollInterval.Duration != time.Duration(0) && backupTarget.Spec.BackupTargetURL != "" {
     ctx, cancel := context.WithCancel(context.Background())
     btc.bsTimerMap[name] = &BackupStoreTimer{
         logger:       log.WithField("component", "backup-store-timer"),
         controllerID: btc.controllerID,
         ds:           btc.ds,
         btName:       name,
         pollInterval: backupTarget.Spec.PollInterval.Duration,
         ctx:          ctx,
         cancel:       cancel,
     }
-    go btc.bsTimerMap[name].Start()
+    go func() {
+        defer func() {
+            if r := recover(); r != nil {
+                log.WithField("panic", r).Error("Timer startup failed")
+                cancel()
+                delete(btc.bsTimerMap, name)
+            }
+        }()
+        btc.bsTimerMap[name].Start()
+    }()
 }

537-579: Consider adding method documentation.

The syncBackupVolume method would benefit from godoc-style documentation explaining its purpose, parameters, and return values.

+// syncBackupVolume synchronizes backup volumes between the backup store and the cluster.
+// It creates backup volumes that exist in the store but not in the cluster,
+// removes ones that exist in the cluster but not in the store,
+// and triggers updates for existing ones.
+// Parameters:
+//   - backupTarget: the backup target being synchronized
+//   - backupStoreBackupVolumeNames: list of volume names from the backup store
+//   - syncTime: the time of synchronization
+//   - log: logger for recording operations
+// Returns an error if any operation fails
 func (btc *BackupTargetController) syncBackupVolume(backupTarget *longhorn.BackupTarget, backupStoreBackupVolumeNames []string, syncTime metav1.Time, log logrus.FieldLogger) error {

controller/backup_controller.go (1)

389-390: Consider enhancing error logging.

The error logging could be more descriptive to aid in troubleshooting.

-				log.Warnf("failed to sync backup volume %v for backup target %v", remoteBackupVolumeName, backupTargetName)
+				log.WithError(err).Errorf("Failed to sync backup volume %v for backup target %v", remoteBackupVolumeName, backupTargetName)

controller/volume_controller_test.go (1)

1387-1394: Fix indentation inconsistency

The indentation of these lines is inconsistent with the surrounding code. The labels and spec fields should align with the rest of the struct definition.

-					Labels: map[string]string{
-						types.LonghornLabelBackupTarget: TestBackupTargetName,
-						types.LonghornLabelBackupVolume: bvName,
-					},
-				},
-				Spec: longhorn.BackupVolumeSpec{
-					BackupTargetName: TestBackupTargetName,
-					VolumeName:       bvName,
+				Labels: map[string]string{
+					types.LonghornLabelBackupTarget: TestBackupTargetName,
+					types.LonghornLabelBackupVolume: bvName,
+				},
+			},
+			Spec: longhorn.BackupVolumeSpec{
+				BackupTargetName: TestBackupTargetName,
+				VolumeName:       bvName,

controller/setting_controller.go (1)

Line range hint 404-457: Implementation looks solid with room for minor improvements.

The implementation correctly handles the default backup target creation and updates. Consider these improvements:

Add more specific error handling for the update operation
Add logging for create/update operations to aid debugging

 if _, err = sc.ds.UpdateBackupTarget(backupTarget); err != nil {
+    if apierrors.IsConflict(err) {
+        // Log conflicts at debug level since they're expected in concurrent scenarios
+        sc.logger.WithError(err).Debug("Conflict updating backup target, will retry")
+    } else {
+        // Log other errors as errors since they indicate real problems
+        sc.logger.WithError(err).Error("Failed to update backup target")
+    }
     return err
 }
+sc.logger.Infof("Successfully updated backup target %v", backupTarget.Name)

controller/engine_controller.go (1)

1422-1427: Maintain consistent format specifiers in error messages.

For consistency, use the same format specifier throughout the error messages.

Apply this change:

 if apierrors.IsNotFound(err) {
-    return errors.Wrapf(err, "cannot find the backup target %s", backupVolume.Spec.BackupTargetName)
+    return errors.Wrapf(err, "cannot find the backup target %v", backupVolume.Spec.BackupTargetName)
 }
-return errors.Wrapf(err, "failed to get backup target %s", backupVolume.Spec.BackupTargetName)
+return errors.Wrapf(err, "failed to get backup target %v", backupVolume.Spec.BackupTargetName)

k8s/crds.yaml (3)

887-890: Consider enhancing the backupTargetName field description

While the field definition is correct, consider making the description more informative by specifying:

What happens when the field is not set
The relationship with the system-wide default backup target

Apply this change:

              backupTargetName:
-                description: The backup target name.
+                description: The name of the backup target that this backup will use. When not specified, the system-wide default backup target will be used.
                nullable: true
                type: string

4223-4226: Enhance the backupTargetName field description in VolumeSpec

The field definition is correct, but the description could be more comprehensive.

Apply this change:

              backupTargetName:
-                description: The backup target name that the volume will be backed up to or is synced.
+                description: The name of the backup target that this volume will use for backup operations and backup listing synchronization. When not specified, the system-wide default backup target will be used.
                type: string

Line range hint 657-4226: Consider documenting the backup target resolution precedence

The introduction of backupTargetName across multiple CRDs creates a flexible system for backup target management. However, this flexibility requires clear documentation about:

The precedence order when backup targets are specified at different levels
The synchronization behavior between different resources using the same backup target
The implications of the readOnly flag on backup operations

Consider adding this information to the API documentation to help users understand the backup target resolution logic.

controller/volume_controller.go (1)

3627-3636: Improve error handling for backup volume info retrieval

The error message should include more context about the backup volume name and backup target name to help with debugging.

-			return nil, errors.Wrapf(err, "failed to get backup volume information for restoring volume %v", v.Name)
+			return nil, errors.Wrapf(err, "failed to get backup volume information for restoring volume %v (backup volume: %v, backup target: %v)", v.Name, backupVolumeName, v.Spec.BackupTargetName)

datastore/longhorn.go (2)

4351-4425: Consider adding pagination support for list operations

The list operations could benefit from pagination support for large clusters with many backup volumes.

Line range hint 5830-5894: Consider adding batch operations for backup backing images

The implementation could benefit from batch operations for listing and filtering backup backing images to improve performance.

Consider adding batch list/filter operations:

+func (s *DataStore) BatchListBackupBackingImagesWithBackupTargetNameRO(backupTargetNames []string) (map[string]map[string]*longhorn.BackupBackingImage, error) {
+    result := map[string]map[string]*longhorn.BackupBackingImage{}
+    for _, name := range backupTargetNames {
+        items, err := s.ListBackupBackingImagesWithBackupTargetNameRO(name) 
+        if err != nil {
+            return nil, err
+        }
+        result[name] = items
+    }
+    return result, nil
+}

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7a1e122 and 438a842.

📒 Files selected for processing (54)

api/backup.go (2 hunks)
api/backupbackingimage.go (1 hunks)
api/model.go (10 hunks)
api/router.go (2 hunks)
api/snapshot.go (1 hunks)
api/volume.go (1 hunks)
app/recurring_job.go (3 hunks)
client/generated_backup.go (1 hunks)
client/generated_backup_target.go (1 hunks)
client/generated_backup_volume.go (2 hunks)
client/generated_volume.go (1 hunks)
controller/backing_image_data_source_controller.go (1 hunks)
controller/backup_backing_image_controller.go (4 hunks)
controller/backup_controller.go (13 hunks)
controller/backup_target_controller.go (14 hunks)
controller/backup_volume_controller.go (7 hunks)
controller/controller_test.go (1 hunks)
controller/engine_controller.go (2 hunks)
controller/setting_controller.go (3 hunks)
controller/system_backup_controller.go (2 hunks)
controller/uninstall_controller.go (2 hunks)
controller/utils.go (1 hunks)
controller/volume_controller.go (5 hunks)
controller/volume_controller_test.go (2 hunks)
csi/controller_server.go (4 hunks)
datastore/kubernetes.go (1 hunks)
datastore/longhorn.go (13 hunks)
engineapi/backup_backing_image.go (4 hunks)
engineapi/types.go (3 hunks)
k8s/crds.yaml (9 hunks)
k8s/pkg/apis/longhorn/v1beta2/backingimagedatasource.go (1 hunks)
k8s/pkg/apis/longhorn/v1beta2/backup.go (1 hunks)
k8s/pkg/apis/longhorn/v1beta2/backupbackingimage.go (1 hunks)
k8s/pkg/apis/longhorn/v1beta2/backuptarget.go (1 hunks)
k8s/pkg/apis/longhorn/v1beta2/backupvolume.go (1 hunks)
k8s/pkg/apis/longhorn/v1beta2/volume.go (1 hunks)
manager/backupbackingimage.go (2 hunks)
manager/engine.go (6 hunks)
manager/volume.go (6 hunks)
types/types.go (7 hunks)
upgrade/util/util.go (5 hunks)
upgrade/v17xto180/upgrade.go (2 hunks)
util/util.go (3 hunks)
webhook/resources/backingimage/mutator.go (3 hunks)
webhook/resources/backup/mutator.go (1 hunks)
webhook/resources/backup/validator.go (3 hunks)
webhook/resources/backupbackingimage/mutator.go (2 hunks)
webhook/resources/backuptarget/mutator.go (1 hunks)
webhook/resources/backuptarget/validator.go (1 hunks)
webhook/resources/backupvolume/mutator.go (1 hunks)
webhook/resources/volume/mutator.go (4 hunks)
webhook/resources/volume/validator.go (3 hunks)
webhook/server/mutation.go (2 hunks)
webhook/server/validation.go (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (28)

api/backupbackingimage.go
api/snapshot.go
api/volume.go
client/generated_backup.go
client/generated_backup_target.go
client/generated_backup_volume.go
client/generated_volume.go
controller/backing_image_data_source_controller.go
controller/backup_backing_image_controller.go
controller/controller_test.go
controller/system_backup_controller.go
controller/uninstall_controller.go
controller/utils.go
engineapi/backup_backing_image.go
engineapi/types.go
k8s/pkg/apis/longhorn/v1beta2/backingimagedatasource.go
k8s/pkg/apis/longhorn/v1beta2/backup.go
k8s/pkg/apis/longhorn/v1beta2/backuptarget.go
k8s/pkg/apis/longhorn/v1beta2/backupvolume.go
k8s/pkg/apis/longhorn/v1beta2/volume.go
manager/backupbackingimage.go
util/util.go
webhook/resources/backup/mutator.go
webhook/resources/backup/validator.go
webhook/resources/backuptarget/validator.go
webhook/resources/volume/mutator.go
webhook/server/mutation.go
webhook/server/validation.go

🧰 Additional context used

📓 Learnings (18)

api/backup.go (4)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:107-112
Timestamp: 2024-10-29T08:29:47.475Z
Learning: In the file `api/backup.go`, for the `BackupTargetUpdate` function, prefer to keep the use of `RetryOnConflictCause` without adding a context with timeout.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:92-105
Timestamp: 2024-10-28T15:01:34.436Z
Learning: The method `s.m.UpdateBackupTarget` checks if the backup target exists before updating.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:123-128
Timestamp: 2024-10-28T13:21:51.980Z
Learning: In `api/backup.go`, the method `s.m.DeleteBackupTarget` already checks for the existence of the backup target and handles errors. Therefore, it's unnecessary to add an existence check before calling this method.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:85-90
Timestamp: 2024-10-28T14:55:39.140Z
Learning: Validation of `BackupTargetURL` format is handled in the `backupTargetValidator` in `webhook/resources/backuptarget/validator.go`. Therefore, adding validation in `api/backup.go` is unnecessary.

api/model.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/model.go:914-917
Timestamp: 2024-10-28T15:26:50.887Z
Learning: In `api/model.go`, within the `backupTargetUpdate` action, the input type "BackupTarget" refers to the struct `type BackupTarget struct` and is correctly capitalized as intended.

controller/backup_controller.go (10)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:310-310
Timestamp: 2024-10-29T08:51:32.394Z
Learning: In `controller/backup_controller.go`, when retrieving `backupVolume` using `GetBackupVolumeWithBackupTargetAndVolume`, it can be `nil` without being an error, and the code should handle this case appropriately.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:804-804
Timestamp: 2024-10-24T11:31:44.721Z
Learning: In the `checkMonitor` method in `controller/backup_controller.go`, `backupTarget` is checked for `nil` at the beginning of the method, so accessing `backupTarget.Name` is safe.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/volume_controller.go:4765-4768
Timestamp: 2024-10-24T12:38:46.899Z
Learning: In the `controller/volume_controller.go`, `volume.Spec.BackupTargetName` is guaranteed to be non-empty by the volume mutator. Therefore, it's unnecessary to check if it's empty before using it.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:325-326
Timestamp: 2024-10-24T12:04:44.187Z
Learning: In `controller/backup_controller.go`, `backupTargetClient` is guaranteed to be non-`nil` when there is no error, so additional `nil` checks before its usage are unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:618-626
Timestamp: 2024-10-29T09:06:52.799Z
Learning: In the `getBackupTarget` function in `controller/backup_controller.go`, we should keep the check for `backup.DeletionTimestamp == nil` when `backupTarget` is `nil`, to handle cases where the backup target is deleted and backup CRs are deleting.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:701-717
Timestamp: 2024-10-28T01:22:59.032Z
Learning: The `bbi mutator` ensures that `backupTargetName` is valid before it's used in labels when creating `BackupBackingImage` objects in `controller/backup_controller.go`.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/engine.go:354-376
Timestamp: 2024-10-29T08:27:34.455Z
Learning: When creating a `BackupTarget` in the `CreateBackupTarget` function, it's unnecessary to validate the `backupTargetName` parameter because an error will be returned when creating the `BackupTarget` CR if the name is invalid or empty.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:404-404
Timestamp: 2024-10-29T08:47:14.968Z
Learning: In `controller/backup_controller.go`, be aware that after setting the backup status to error when a volume is not found, the code already includes `return nil`, so no additional return statement is needed.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_volume_controller.go:240-242
Timestamp: 2024-10-29T04:23:30.091Z
Learning: In `controller/backup_volume_controller.go`, the `BackupVolumeDelete` method of `backupTargetClient` returns `nil` if the backup volume is not found, so it's unnecessary to check for 'not found' errors after calling this method.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/volume/validator.go:494-505
Timestamp: 2024-10-28T15:15:07.305Z
Learning: In the `validateBackupTarget` function in `webhook/resources/volume/validator.go`, when creating a volume, `oldBackupTarget` will be empty, and `newBackupTarget` cannot be empty. Therefore, the validation logic must ensure that `newBackupTarget` is not empty during volume creation.

controller/backup_target_controller.go (5)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:537-579
Timestamp: 2024-10-29T07:10:30.855Z
Learning: Prefer to keep the `syncBackupVolume` and `syncBackupBackingImage` functions separate, rather than refactoring them into a generic sync function.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:361-366
Timestamp: 2024-10-30T06:52:14.797Z
Learning: `btc.bsTimerMap` is always initialized when the backup target controller is created and will not be `nil` in `stopTimer`, so nil checks on `btc.bsTimerMap` are unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:884-897
Timestamp: 2024-10-29T06:24:24.031Z
Learning: In `controller/backup_target_controller.go`, maintain the original error handling in the polling function within `BackupStoreTimer.Start()` as per the team's preference.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:382-399
Timestamp: 2024-10-29T04:28:29.104Z
Learning: In the backup target controller, `backupTarget.Spec.PollInterval` will not be smaller than 0, and the maximum polling interval should not be limited as it can be set to days.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:382-399
Timestamp: 2024-10-29T07:18:29.563Z
Learning: In `controller/backup_target_controller.go`, the logging related to the backup store timer is handled within `BackupStoreTimer.Start()`, so additional logging during timer initialization is unnecessary.

controller/backup_volume_controller.go (3)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_volume_controller.go:340-352
Timestamp: 2024-10-29T07:56:06.720Z
Learning: When creating backups in the `reconcile` method of `BackupVolumeController` (in `controller/backup_volume_controller.go`), the backup should not already exist, so error handling for existing backups is not necessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_volume_controller.go:240-242
Timestamp: 2024-10-29T04:23:30.091Z
Learning: In `controller/backup_volume_controller.go`, the `BackupVolumeDelete` method of `backupTargetClient` returns `nil` if the backup volume is not found, so it's unnecessary to check for 'not found' errors after calling this method.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:537-579
Timestamp: 2024-10-29T07:10:30.855Z
Learning: Prefer to keep the `syncBackupVolume` and `syncBackupBackingImage` functions separate, rather than refactoring them into a generic sync function.

controller/setting_controller.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/setting_controller.go:245-247
Timestamp: 2024-10-28T01:27:09.346Z
Learning: In `api/model.go`, the field `SyncBackupTarget` is different from the method `syncDefaultBackupTarget` in `controller/setting_controller.go`, and they should not be considered the same.

csi/controller_server.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: csi/controller_server.go:1070-1071
Timestamp: 2024-10-30T07:07:32.174Z
Learning: In the method `cleanupBackupVolume` in `csi/controller_server.go`, errors are returned directly and handled at the upper-level method `DeleteSnapshot`. Therefore, it's acceptable to return errors directly in such helper methods without wrapping them.

datastore/kubernetes.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: datastore/kubernetes.go:895-918
Timestamp: 2024-10-29T03:01:18.811Z
Learning: In the `HandleSecretsForAWSIAMRoleAnnotation` function in `datastore/kubernetes.go`, when the old and new secret names are the same and the backup target URL changes, it's unnecessary to handle this scenario in the function, because updating the backup target URL in the secret's annotation will trigger the Kubernetes secret controller to reconcile the secret and handle the AWS IAM Role ARN of the pods.

datastore/longhorn.go (3)

<retrieved_learning>
Learnt from: mantissahz
PR: #2182
File: datastore/longhorn.go:4233-4267
Timestamp: 2024-10-29T07:28:51.100Z
Learning: When reviewing the URL validation in checkBackupTargetURLFormat, prefer to keep the original implementation without refactoring.
</retrieved_learning>

<retrieved_learning>
Learnt from: mantissahz
PR: #2182
File: datastore/longhorn.go:4254-4267
Timestamp: 2024-10-29T06:28:32.626Z
Learning: In datastore/longhorn.go, for the checkBackupTargetURLFormat function, avoid adding global variables for regex patterns unless they are used in multiple places.
</retrieved_learning>

<retrieved_learning>
Learnt from: mantissahz
PR: #2182
File: datastore/longhorn.go:4178-4181
Timestamp: 2024-10-24T13:30:06.074Z
Learning: The function types.GetLonghornAnnotationKey does not exist in the codebase.
</retrieved_learning>

k8s/crds.yaml (2)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: k8s/crds.yaml:4223-4226
Timestamp: 2024-10-29T04:46:47.777Z
Learning: The `crds.yaml` file is automatically generated by the script `k8s/generate_code.sh`.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: k8s/crds.yaml:4223-4226
Timestamp: 2024-10-29T08:32:45.912Z
Learning: The file `k8s/crds.yaml` is automatically generated by the `k8s/generate_code.sh` script; manual edits to this file will be overwritten.

manager/engine.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/engine.go:354-404
Timestamp: 2024-10-29T10:39:23.381Z
Learning: In API methods intended for users or UI, return errors directly and allow the users to handle retry logic; avoid handling conflicts internally.

manager/volume.go (2)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/volume.go:1203-1205
Timestamp: 2024-10-29T04:34:12.107Z
Learning: The 'default' backup target concept is not being removed in the PR for multiple backup stores support.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/volume.go:1180-1182
Timestamp: 2024-10-29T12:51:34.772Z
Learning: Ensure that when reviewing the `restoreBackingImage` function in `manager/volume.go`, verify that variables referenced in error messages are actually undefined before suggesting corrections.

upgrade/util/util.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/util/util.go:555-573
Timestamp: 2024-10-29T07:21:21.625Z
Learning: The team prefers to keep the original code style and convention in functions like `ListAndUpdateBackupTargetsInProvidedCache`, even if it leads to code duplication.

upgrade/v17xto180/upgrade.go (4)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:174-180
Timestamp: 2024-10-29T14:05:05.010Z
Learning: Ensure that code suggestions provide meaningful improvements over the existing implementation and avoid suggesting changes that match the current code.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:20-23
Timestamp: 2024-10-29T13:13:50.924Z
Learning: In `upgrade/v17xto180/upgrade.go`, the `Upgrade` method performs `namespace` validation at the beginning of the upgrade path, so it's unnecessary to check `namespace` again in `UpgradeResources`.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:25-42
Timestamp: 2024-10-29T08:29:16.196Z
Learning: The team prefers not to add logs during the upgrade process.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:195-195
Timestamp: 2024-10-29T03:58:48.533Z
Learning: The term `BackupImagesDataSources` is the correct resource name in error messages within `upgrade/v17xto180/upgrade.go` and should not be flagged as a typo.

webhook/resources/backingimage/mutator.go (8)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backingimage/mutator.go:172-195
Timestamp: 2024-10-29T13:15:37.849Z
Learning: In the `findBackupTargetName` function, the loop `for _, bbi := range bbis {...}` handles the case when `bbis` is empty, so an additional early return is unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backingimage/mutator.go:197-213
Timestamp: 2024-10-29T07:04:42.102Z
Learning: In future code reviews, maintain the existing switch statements for URL scheme handling if different handlers for different types might be added later.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:107-112
Timestamp: 2024-10-29T08:29:47.475Z
Learning: In the file `api/backup.go`, for the `BackupTargetUpdate` function, prefer to keep the use of `RetryOnConflictCause` without adding a context with timeout.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backingimage/mutator.go:96-102
Timestamp: 2024-10-30T06:48:50.235Z
Learning: Since `getBackupTargetURLAndBackingImageName` will return an error when `backupURL` is empty, additional validation for `backupURL` is unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:85-90
Timestamp: 2024-10-28T14:55:39.140Z
Learning: Validation of `BackupTargetURL` format is handled in the `backupTargetValidator` in `webhook/resources/backuptarget/validator.go`. Therefore, adding validation in `api/backup.go` is unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/engine.go:0-0
Timestamp: 2024-10-29T07:30:27.089Z
Learning: In the `CreateBackupTarget` function within `manager/engine.go`, the `BackupTargetSpec.BackupTargetURL` field can be empty, so validation should allow empty values for it.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backing_image_data_source_controller.go:714-714
Timestamp: 2024-10-28T01:25:02.898Z
Learning: In `controller/backing_image_data_source_controller.go`, the method `GetBackupTargetRO` already handles error checking for `bids.Spec.Parameters[longhorn.DataSourceTypeRestoreParameterBackupTargetName]`, so explicit checks before calling it are unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backupbackingimage/mutator.go:68-80
Timestamp: 2024-10-29T10:24:27.093Z
Learning: In the `Create` method of `backupBackingImageMutator` in `webhook/resources/backupbackingimage/mutator.go`, it's acceptable not to add validation to prevent user overwriting of `LonghornLabelBackupTarget` and `LonghornLabelBackingImage` labels, because these labels are assigned values later in the code.

webhook/resources/backupbackingimage/mutator.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backupbackingimage/mutator.go:68-80
Timestamp: 2024-10-29T10:24:27.093Z
Learning: In the `Create` method of `backupBackingImageMutator` in `webhook/resources/backupbackingimage/mutator.go`, it's acceptable not to add validation to prevent user overwriting of `LonghornLabelBackupTarget` and `LonghornLabelBackingImage` labels, because these labels are assigned values later in the code.

webhook/resources/backupvolume/mutator.go (2)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/volume/mutator.go:115-117
Timestamp: 2024-10-28T15:09:08.570Z
Learning: In `webhook/resources/volume/mutator.go`, within the `Create` method of `volumeMutator`, it's acceptable for `bv` to be `nil` after calling `v.ds.GetBackupVolumeWithBackupTargetAndVolumeRO`, as a `nil` `bv` is handled by the volume and engine controllers. Explicit `nil` checks in this context are not necessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backupbackingimage/mutator.go:68-80
Timestamp: 2024-10-29T10:24:27.093Z
Learning: In the `Create` method of `backupBackingImageMutator` in `webhook/resources/backupbackingimage/mutator.go`, it's acceptable not to add validation to prevent user overwriting of `LonghornLabelBackupTarget` and `LonghornLabelBackingImage` labels, because these labels are assigned values later in the code.

webhook/resources/volume/validator.go (3)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/volume/validator.go:494-505
Timestamp: 2024-10-28T15:15:07.305Z
Learning: In the `validateBackupTarget` function in `webhook/resources/volume/validator.go`, when creating a volume, `oldBackupTarget` will be empty, and `newBackupTarget` cannot be empty. Therefore, the validation logic must ensure that `newBackupTarget` is not empty during volume creation.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:85-90
Timestamp: 2024-10-28T14:55:39.140Z
Learning: Validation of `BackupTargetURL` format is handled in the `backupTargetValidator` in `webhook/resources/backuptarget/validator.go`. Therefore, adding validation in `api/backup.go` is unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backup/validator.go:55-75
Timestamp: 2024-10-22T12:57:02.630Z
Learning: In `webhook/resources/backup/validator.go`, the `BackupTargetName` field is populated by the backup mutator, so it is guaranteed to be set when the validator runs.

🔇 Additional comments (67)

webhook/resources/backupvolume/mutator.go (1)

Line range hint 72-77: LGTM! Error handling and label management look good.

The implementation correctly:

Processes all backups in the collection
Provides good error context
Uses the appropriate datastore method for label management

webhook/resources/backupbackingimage/mutator.go (3)

9-9: LGTM: Import changes are appropriate

The new imports are properly organized and necessary for the added functionality.

Also applies to: 14-14

68-80: LGTM: Label management implementation

The label management implementation looks good. As per the learnings, it's acceptable not to validate the labels since they are assigned values later in the code.

57-66: Verify backup target's ReadOnly status

Since the PR objectives mention a new ReadOnly flag for backup targets, we should validate that the target is not read-only before allowing backup creation.

Consider adding validation:

 backupTarget, err := b.ds.GetBackupTargetRO(backupBackingImage.Spec.BackupTargetName)
 if apierrors.IsNotFound(err) {
     backupTarget, err = b.ds.GetDefaultBackupTargetRO()
     if err != nil {
         return nil, werror.NewInvalidError(errors.Wrapf(err, "failed to get default backup target").Error(), "")
     }
+    if backupTarget.Spec.ReadOnly {
+        return nil, werror.NewInvalidError(fmt.Sprintf("cannot create backup on read-only backup target %s", backupTarget.Name), "")
+    }
     patchOps = append(patchOps, fmt.Sprintf(`{"op": "replace", "path": "/spec/backupTargetName", "value": "%s"}`, backupTarget.Name))
 } else if err != nil {
     return nil, werror.NewInvalidError(errors.Wrapf(err, "failed to get backup target of backup backing image").Error(), "")
+} else if backupTarget.Spec.ReadOnly {
+    return nil, werror.NewInvalidError(fmt.Sprintf("cannot create backup on read-only backup target %s", backupTarget.Name), "")
 }

k8s/pkg/apis/longhorn/v1beta2/backupbackingimage.go (1)

10-13: Verify upgrade path for BackupTargetName field.

The implementation looks good. The field is properly marked as optional and nullable, which is correct for backward compatibility. However, as mentioned in the PR comments by ChanYiLin, we need to ensure proper upgrade handling.

Let's verify the CRD upgrade handling:

✅ Verification successful

Let me gather more context about the upgrade handling code.

Upgrade path for BackupTargetName field is properly handled

The upgrade path is well-implemented in upgrade/v17xto180/upgrade.go. During upgrade:

Empty backupTargetName fields are set to the default value using ensureBackupTargetSet
The field is properly marked as optional and nullable in the CRD
Labels are added for better resource tracking

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if there are any existing BackupBackingImage CRs that might be affected during upgrade

# Look for existing BackupBackingImage CRs
rg -l "kind:\s*BackupBackingImage" --type yaml

# Check for any upgrade-related code
rg -A 5 "BackupBackingImage.*[Uu]pgrade"

Length of output: 583

Script:

#!/bin/bash
# Check the upgrade implementation details
cat upgrade/v17xto180/upgrade.go

# Look for any CRD validation or conversion code
rg -A 5 "BackupBackingImage.*[Cc]onvert"

# Check the CRD definition
rg -A 20 "kind: BackupBackingImage" k8s/crds.yaml

Length of output: 7960

upgrade/v17xto180/upgrade.go (4)

68-73: LGTM: Well-structured helper functions

The helper functions ensureBackupTargetSet and ensureLabel are well-implemented, promoting code reuse and consistent behavior across different resource types.

Also applies to: 102-110

Line range hint 11-183: LGTM: Well-implemented upgrade path

The implementation provides a robust upgrade path from v1.7.x to v1.8.0 with:

Consistent error handling across resource types
Proper resource validation and updates
Reusable helper functions
Clear upgrade sequence

174-181: 🛠️ Refactor suggestion

Consider consistent parameter initialization

The parameter initialization in upgradeBackingImageDataSources could be simplified to match the pattern used in other upgrade functions:

 for _, bids := range backingImageDataSourcesMap {
-    if bids.Spec.Parameters == nil {
-        bids.Spec.Parameters = map[string]string{longhorn.DataSourceTypeRestoreParameterBackupTargetName: types.DefaultBackupTargetName}
-    } else {
-        backupTargetName := bids.Spec.Parameters[longhorn.DataSourceTypeRestoreParameterBackupTargetName]
-        bids.Spec.Parameters[longhorn.DataSourceTypeRestoreParameterBackupTargetName] = ensureBackupTargetSet(backupTargetName)
-    }
+    if bids.Spec.Parameters == nil {
+        bids.Spec.Parameters = make(map[string]string)
+    }
+    targetName := bids.Spec.Parameters[longhorn.DataSourceTypeRestoreParameterBackupTargetName]
+    bids.Spec.Parameters[longhorn.DataSourceTypeRestoreParameterBackupTargetName] = ensureBackupTargetSet(targetName)
 }

⛔ Skipped due to learnings

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backing_image_data_source_controller.go:714-714
Timestamp: 2024-10-28T01:25:02.898Z
Learning: In `controller/backing_image_data_source_controller.go`, the method `GetBackupTargetRO` already handles error checking for `bids.Spec.Parameters[longhorn.DataSourceTypeRestoreParameterBackupTargetName]`, so explicit checks before calling it are unnecessary.

20-43: Consider adding error context for upgrade steps

The sequential upgrade process is well-structured, but error messages could be more descriptive to aid in debugging when multiple upgrades are involved. Consider enhancing error context in a future update.

api/backup.go (1)

283-289: LGTM!

The backup listing implementation is clean and consistent with the codebase patterns. The error handling is appropriate and the method uses the correct sorting function.

webhook/resources/backingimage/mutator.go (3)

6-6: LGTM!

The addition of the net/url import is appropriate for the new URL parsing functionality.

96-102: LGTM!

The logic for handling backup target name in restore operations is well-implemented with proper error handling.

197-213: LGTM! Consider verifying URL handling.

The URL parsing implementation is robust and handles different backup store types correctly. The switch statement is well-structured for future extensibility.

Let's verify the URL handling with different schemes:

✅ Verification successful

Based on the comprehensive analysis of the URL handling implementation across the codebase, I can now provide a final response:

URL handling implementation is robust and consistent

The implementation in getBackupTargetURLAndBackingImageName follows the established patterns in the codebase:

Proper error handling for URL parsing failures
Consistent scheme validation against defined constants in types.go
Safe query parameter handling with proper error messages
Query parameter cleanup before returning the URL

The supported schemes (CIFS, NFS, AZBlob, S3) are properly defined as constants and consistently used across the codebase, particularly in backup-related operations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify URL handling for different backup store types
# Test: Search for URLs in the codebase to ensure consistent handling

# Check URL formats across the codebase
echo "Checking URL formats for different backup store types..."
rg -A 2 'BackupStoreType(CIFS|NFS|AZBlob|S3).*=.*"[^"]*"'

# Check URL parameter handling
echo "Checking backingImage parameter usage..."
rg -A 2 '[?&]backingImage='

Length of output: 1025

Script:

#!/bin/bash
# Let's verify the URL handling implementation more thoroughly

# Check for URL parsing and validation across the codebase
echo "Checking URL parsing and validation..."
rg -A 5 'url\.Parse\(' 

# Check for any test cases related to backup URL handling
echo "Checking test cases for URL handling..."
fd -e go -x rg -l 'Test.*URL'

# Look for any other URL scheme validations
echo "Checking URL scheme validations..."
rg -A 3 '\.Scheme\s*==' 

# Check for any error cases related to URL handling
echo "Checking URL-related error handling..."
rg -A 2 'errors.*URL'

Length of output: 50443

manager/engine.go (5)

13-18: LGTM!

Import reordering changes.

Line range hint 257-277: LGTM! BackupSnapshot changes look good.

The changes correctly integrate the new backupTargetName parameter into both the backup labels and spec, maintaining consistency with the multiple backup stores support.

Line range hint 331-343: LGTM! Good use of read-only operations.

The change to use ListBackupTargetsRO is appropriate for this listing operation.

347-352: LGTM! Good use of read-only operations.

The change to use GetBackupTargetRO is appropriate for this retrieval operation.

484-488: LGTM! Good use of read-only operations.

The changes to use GetVolumeRO and ListBackupsWithBackupVolumeNameRO are appropriate for these retrieval operations.

controller/backup_volume_controller.go (4)

215-221: LGTM! Well-structured backup target validation.

The code properly validates the backup target's existence and handles error cases appropriately, preventing orphaned backup volumes when not in deletion state.

240-242: LGTM! Proper error handling in backup volume deletion.

The error handling is correct, and the function properly handles the case when the backup volume doesn't exist in the remote target, as BackupVolumeDelete returns nil for not found cases.

340-352: LGTM! Proper backup creation with labels.

The implementation correctly sets the backup target name in labels and handles owner references. The error handling is appropriate since backups should not exist at this point in the reconciliation process.

403-415: LGTM! Well-implemented owner references management.

The code properly checks for existing owner references before adding new ones, preventing duplicates. The implementation is thorough and handles all edge cases correctly.

api/router.go (3)

125-126: LGTM! The backup target update action is well-integrated.

The implementation follows the established pattern of using forwarding handlers and is consistent with other similar actions in the codebase.

131-133: LGTM! The backup target CRUD endpoints are properly implemented.

The endpoints follow RESTful conventions and are consistent with the existing patterns in the codebase.

Note: A previous review comment about adding a PUT endpoint for updates is still valid.

262-265: LGTM! The backup target WebSocket stream is properly implemented.

The WebSocket stream implementation follows the established pattern used for other resources.

Note: Previous review comments about implementing filtering capabilities are still valid and acknowledged for future improvement.

webhook/resources/volume/validator.go (2)

148-150: LGTM: Backup target validation in Create method.

The implementation correctly validates the backup target during volume creation, with proper error field path.

332-334: LGTM: Backup target validation in Update method.

The implementation correctly validates the backup target during volume updates, with proper error field path.

app/recurring_job.go (1)

704-722: LGTM! Well-structured implementation with proper error handling.

The implementation:

Correctly filters backup volumes using appropriate labels
Properly handles edge cases (no results and multiple results)
Uses clear error messages
Returns early on error conditions
Follows Go best practices for error handling and return values

controller/backup_target_controller.go (1)

875-909: LGTM! Timer management implementation looks solid.

The implementation includes proper nil checks, clear logging, and follows the team's preferred error handling pattern in the polling function.

controller/backup_controller.go (6)

280-288: LGTM! Proper error handling for backup target retrieval.

The code correctly retrieves and validates the backup target, with appropriate error handling and nil checks.

Line range hint 306-326: LGTM! Proper handling of backup deletion prerequisites.

The code correctly handles attachment ticket deletion, backup volume retrieval, and backup target client initialization. The error handling follows established patterns.

617-627: LGTM! Proper implementation of backup target retrieval.

The method correctly handles error cases and includes the necessary check for deletion timestamp as required.

689-717: LGTM! Proper handling of backup backing image creation.

The code correctly handles backup target name and creates backup backing image with appropriate labels. The validation is handled by the bbi mutator as expected.

Line range hint 864-882: LGTM! Well-structured backup volume sync implementation.

The method properly handles both backup volume and backup target synchronization with appropriate error handling.

Line range hint 807-817: LGTM! Proper implementation of backup listing and validation.

The code correctly lists and validates backups, with appropriate error handling and state checking.

manager/volume.go (3)

157-158: LGTM: Volume creation changes for backup target support

The changes correctly integrate the backup target name into the volume creation process, including proper error handling and spec field addition.

Also applies to: 195-195

431-435: LGTM: Improved backup target handling in sync trigger

The changes properly use the BackupTargetName from volume spec instead of labels, with appropriate error handling and improved logging.

Also applies to: 446-446

1225-1227: LGTM: Proper integration of backup target in source parameters

The changes correctly add the backup target name to the source parameters with consistent parameter naming.

types/types.go (4)

31-34: LGTM: New constants align with backup target feature.

The new constants are well-organized and follow the existing naming conventions. They properly support the multiple backup stores feature.

Also applies to: 136-137

548-553: LGTM: Label functions follow established patterns.

The new label functions are well-designed and consistent with existing label generation patterns in the codebase.

Also applies to: 582-593

739-750: LGTM: Name generation functions are well-structured.

The backup resource name generation functions are well-implemented:

Common logic properly extracted into a helper function
Consistent with the pattern suggested in past review comments
Ensures unique name generation through checksum and random ID combination

31-34: Verify backup target integration across codebase.

Let's verify the consistent usage of backup target functionality:

✅ Verification successful

Based on the results, I can generate the final response as we have sufficient information about the backup target integration across the codebase.

Backup target integration appears properly implemented with consistent error handling and validation

The verification shows:

Proper error handling in backup target operations with appropriate logging and condition updates
Consistent usage of backup target labels across mutators (volume, backup, backing image)
Comprehensive validation of backup target URLs and credential secrets
Well-structured error propagation through webhook validators

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify backup target integration
# Test 1: Check for proper error handling in backup target operations
rg "BackupTarget.*Error" -A 5

# Test 2: Verify consistent usage of backup target labels
rg "LonghornLabelBackupTarget.*=" -A 2

# Test 3: Check for proper validation of backup target names
rg "ValidateBackupTarget.*" -A 5

Length of output: 3927

upgrade/util/util.go (3)

Line range hint 535-613: Implementation follows established patterns correctly.

The new List functions for backup resources maintain consistency with existing patterns and properly handle caching and error cases.

809-816: Switch cases properly integrated for backup resources.

The new cases in UpdateResources maintain consistency with the existing pattern and correctly map to their respective update functions.

Line range hint 929-1028: Consider potential race conditions in update operations.

While the implementation follows the established pattern, there's a potential race condition between the List and Update operations. If resources are modified between these operations, it could lead to unexpected behavior.

Consider adding a note in the function documentation about this limitation, or if critical, implement optimistic locking using resource versions.

csi/controller_server.go (3)

130-130: LGTM!

The usage of getBackupVolume in CreateVolume follows CSI spec requirements with proper error handling and clear error messages.

1070-1071: LGTM!

The direct error return in cleanupBackupVolume is appropriate since errors are handled at the upper-level method DeleteSnapshot, as noted in the learnings.

1286-1286: LGTM!

The usage of getBackupVolume in getBackup follows CSI spec requirements with proper error handling using appropriate error codes.

controller/volume_controller_test.go (1)

1140-1140: LGTM: BackupTargetName field addition

The addition of BackupTargetName to VolumeSpec aligns with the PR objective of supporting multiple backup stores.

controller/setting_controller.go (1)

245-247: LGTM! Method rename clarifies its purpose.

The rename from syncBackupTarget to syncDefaultBackupTarget better reflects that this method specifically handles the default backup target, which aligns with the new multiple backup stores support.

api/model.go (4)

64-64: LGTM: New backup target fields added to structs

The new BackupTargetName fields are consistently added across relevant structs to support multiple backup stores functionality. The field names and types are appropriate.

Also applies to: 143-144, 177-177, 194-195

880-907: LGTM: Backup target schema properly defined

The backup target schema is well-defined with:

Appropriate HTTP methods for collection and resource operations
Proper field validations and defaults
Required fields marked correctly
Update action properly configured

Also applies to: 914-917

1870-1871: LGTM: Resource conversion functions properly updated

The conversion functions are consistently updated to handle the new backup target fields, ensuring proper mapping between API types and internal types.

Also applies to: 1921-1922, 1978-1978

880-907: Verify upgrade path for backup target CRDs

As mentioned in the PR comments by @ChanYiLin, please ensure there is a proper upgrade plan for the new backup target CRDs, particularly for users upgrading from previous versions.

controller/engine_controller.go (2)

1417-1420: Existing error handling is acceptable.

The current error handling approach aligns with the maintainer's preference to return raw errors.

1448-1456: Variable naming discussion already addressed.

The current variable naming has been discussed in previous reviews.

k8s/crds.yaml (2)

657-665: LGTM: BackupBackingImage CRD schema changes

The schema changes for BackupBackingImageSpec look correct:

Required backingImage field is properly defined
Optional backupTargetName field is properly marked as nullable

1128-1131: LGTM: BackupTarget CRD schema changes

The addition of the readOnly field to BackupTargetSpec is well-defined and properly documented.

controller/volume_controller.go (2)

3652-3669: LGTM! Good refactoring of backup volume info retrieval

The extracted getBackupVolumeInfo method improves code organization by encapsulating the backup volume info retrieval logic.

4776-4778: 🛠️ Refactor suggestion

Verify backup target existence before accessing backup volume

The code should verify that the backup target exists and is valid before attempting to retrieve the backup volume.

Add backup target validation:

+	if volume.Spec.BackupTargetName == "" {
+		return nil
+	}
+	bt, err := c.ds.GetBackupTargetRO(volume.Spec.BackupTargetName)
+	if err != nil {
+		return errors.Wrapf(err, "failed to get backup target %v", volume.Spec.BackupTargetName) 
+	}
+	if bt == nil {
+		return nil
+	}
 	bv, err := c.ds.GetBackupVolumeWithBackupTargetAndVolumeRO(volume.Spec.BackupTargetName, backupVolumeName)

datastore/longhorn.go (3)

921-932: LGTM: Owner references for backup target

The implementation follows the standard pattern for owner references in Kubernetes controllers.

934-945: LGTM: Owner references for backup volume

The implementation follows the standard pattern for owner references in Kubernetes controllers.

4427-4432: LGTM: Label selector implementation

The label selector implementation follows Kubernetes best practices for resource filtering.

Also applies to: 4433-4448

webhook/resources/backuptarget/mutator.go (1)

1-67: Code is well-structured and correctly implements the mutator logic

The implementation of the backupTargetMutator struct, along with its associated methods and the mutate function, is clear and follows the established patterns in the codebase. Type assertions and error handling are appropriately managed, ensuring robust operation of the mutator.

datastore/kubernetes.go (3)

893-918: Well-structured handling of AWS IAM Role annotations

The HandleSecretsForAWSIAMRoleAnnotation function correctly manages AWS IAM Role annotations when backup targets are created or updated. The logic appropriately handles scenarios where secret names and backup target URLs change, incorporating previous feedback effectively.

920-957: Properly updating secret annotations in updateSecretForAWSIAMRoleAnnotation

The function effectively updates secret annotations and handles existence checks for the AWS IAM Role ARN. Error handling is appropriately managed, and nil checks for secret.Data and secret.Annotations are correctly implemented, enhancing the robustness of the code.

959-988: Ensure all pods are processed even if updating a pod fails

The concern about ensuring all pods are processed even if updating a pod fails remains valid. Previous feedback on this issue still applies.

webhook/resources/backupvolume/mutator.go

upgrade/v17xto180/upgrade.go

api/backup.go

csi/controller_server.go

datastore/longhorn.go

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Add the method to get the correct BackupVolume with the volume name and backup target name for the recurring job and related data strucurtes of APIs. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Add more parameters in creating BackupVolume for testing volume_controller.go ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

…he restoring type ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Upgrade CRs for Volume, BackupVolume, Backup, BackupBackingImage, and BackingImageDataSource. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

Ref: 5411 Signed-off-by: James Lu <james.lu@suse.com>

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

coderabbitai

Actionable comments posted: 9

🧹 Outside diff range and nitpick comments (10)

webhook/resources/backup/validator.go (1)

55-63: Consider implementing transaction or optimistic locking

The sequential database queries for snapshot and volume could lead to race conditions if the resources are modified between queries. Consider:

Using a transaction if supported by the datastore
Implementing optimistic locking with resource versions
Adding version validation in the admission webhook

api/backup.go (1)

62-64: Improve error handling consistency in request parsing.

The error from apiContext.Read() should be wrapped with context about the operation being performed, similar to other error handling in the file.

Apply this improvement:

 	if err := apiContext.Read(&input); err != nil {
-		return err
+		return errors.Wrap(err, "failed to parse backup target input")
 	}

Also applies to: 96-98

controller/backup_controller.go (1)

389-390: Consider standardizing error message formatting

The error message formatting in syncBackupVolume could be improved to match the style used elsewhere in the file.

-				log.Warnf("failed to sync backup volume %v for backup target %v", remoteBackupVolumeName, backupTargetName)
+				log.WithError(err).Errorf("Failed to sync backup volume %v for backup target %v", remoteBackupVolumeName, backupTargetName)

csi/controller_server.go (1)

268-291: Consider standardizing error message format.

While the error handling is solid, the error message format could be more consistent:

Some messages use "failed to" prefix
Others use "found multiple" or different formats

Consider standardizing to always use "failed to" prefix:

-		return nil, fmt.Errorf("found multiple backup volumes for backup target %s and volume %s", vol.BackupTargetName, volumeName)
+		return nil, fmt.Errorf("failed to get unique backup volume: multiple volumes found for backup target %s and volume %s", vol.BackupTargetName, volumeName)

controller/volume_controller_test.go (1)

Line range hint 1-1600: Consider adding test cases for backup target operations

While the test file has comprehensive coverage for volume lifecycle operations, consider adding specific test cases for:

Volume operations with invalid backup target names
Backup target switching scenarios
Error cases when backup target is not available

datastore/longhorn.go (2)

4233-4267: Consider optimizing URL validation regex compilation

The regex pattern compilation in checkBackupTargetURLFormat is done on each function call. While the current implementation works and matches the learnings, consider adding a comment explaining why we're not pre-compiling the regex patterns.

4636-4640: Consider adding batch size limit for backup deletion

The DeleteAllBackupsForBackupVolumeWithBackupTarget function could potentially delete a large number of backups in one operation. Consider adding a batch size limit to prevent potential timeout issues.

controller/backup_target_controller.go (3)

553-560: Address TODO: Add unit tests for synchronization logic

There are TODO comments indicating that unit tests need to be added for the functions pullBackupVolumeFromBackupTarget and cleanupBackupVolumeNotExistOnBackupTarget. Adding unit tests will enhance test coverage and help ensure the correctness of these synchronization operations.

Would you like assistance in creating unit tests for these functions, or should I open a GitHub issue to track this task?

618-619: Typographical error in log message

The log message contains a typographical error: "Can not" should be "Cannot" in "Cannot find the BackupVolume CR in the cluster backup volume map".

Apply this diff to fix the typo:

-log.WithField("volume", volumeName).Warnf("Can not find the BackupVolume CR in the cluster backup volume map")
+log.WithField("volume", volumeName).Warnf("Cannot find the BackupVolume CR in the cluster backup volume map")

685-686: Typographical error in log message

The log message contains a typographical error: "Can not" should be "Cannot" in "Cannot find the BackupBackingImage CR in the cluster backup backing image map".

Apply this diff to fix the typo:

-log.WithField("backing image", backingImageName).Warnf("Can not find the BackupBackingImage CR in the cluster backup backing image map")
+log.WithField("backing image", backingImageName).Warnf("Cannot find the BackupBackingImage CR in the cluster backup backing image map")

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 438a842 and 97553ea.

📒 Files selected for processing (32)

api/backup.go (2 hunks)
api/model.go (10 hunks)
api/router.go (2 hunks)
api/snapshot.go (1 hunks)
app/recurring_job.go (3 hunks)
client/generated_backup.go (1 hunks)
client/generated_backup_target.go (1 hunks)
client/generated_backup_volume.go (2 hunks)
controller/backing_image_data_source_controller.go (1 hunks)
controller/backup_backing_image_controller.go (4 hunks)
controller/backup_controller.go (13 hunks)
controller/backup_target_controller.go (14 hunks)
controller/controller_test.go (1 hunks)
controller/engine_controller.go (2 hunks)
controller/system_backup_controller.go (2 hunks)
controller/volume_controller.go (5 hunks)
controller/volume_controller_test.go (2 hunks)
csi/controller_server.go (4 hunks)
datastore/longhorn.go (13 hunks)
engineapi/backup_backing_image.go (4 hunks)
engineapi/types.go (3 hunks)
k8s/crds.yaml (9 hunks)
k8s/pkg/apis/longhorn/v1beta2/backingimagedatasource.go (1 hunks)
manager/backupbackingimage.go (2 hunks)
manager/engine.go (6 hunks)
manager/volume.go (6 hunks)
types/types.go (7 hunks)
upgrade/util/util.go (5 hunks)
upgrade/v17xto180/upgrade.go (2 hunks)
webhook/resources/backingimage/mutator.go (3 hunks)
webhook/resources/backup/mutator.go (1 hunks)
webhook/resources/backup/validator.go (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (13)

api/snapshot.go
app/recurring_job.go
client/generated_backup.go
client/generated_backup_target.go
client/generated_backup_volume.go
controller/backing_image_data_source_controller.go
controller/backup_backing_image_controller.go
controller/controller_test.go
controller/system_backup_controller.go
engineapi/backup_backing_image.go
engineapi/types.go
k8s/pkg/apis/longhorn/v1beta2/backingimagedatasource.go
webhook/resources/backup/mutator.go

🧰 Additional context used

📓 Learnings (13)

api/backup.go (5)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:79-90
Timestamp: 2024-10-30T08:14:47.421Z
Learning: In `api/backup.go`, the `newBackupTarget` function does not need to validate the `PollInterval` input for empty or negative values because this validation is performed by the backup target validator.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:382-399
Timestamp: 2024-10-29T04:28:29.104Z
Learning: In the backup target controller, `backupTarget.Spec.PollInterval` will not be smaller than 0, and the maximum polling interval should not be limited as it can be set to days.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:92-105
Timestamp: 2024-10-28T15:01:34.436Z
Learning: The method `s.m.UpdateBackupTarget` checks if the backup target exists before updating.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:123-128
Timestamp: 2024-10-28T13:21:51.980Z
Learning: In `api/backup.go`, the method `s.m.DeleteBackupTarget` already checks for the existence of the backup target and handles errors. Therefore, it's unnecessary to add an existence check before calling this method.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:85-90
Timestamp: 2024-10-28T14:55:39.140Z
Learning: Validation of `BackupTargetURL` format is handled in the `backupTargetValidator` in `webhook/resources/backuptarget/validator.go`. Therefore, adding validation in `api/backup.go` is unnecessary.

api/model.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/model.go:914-917
Timestamp: 2024-10-28T15:26:50.887Z
Learning: In `api/model.go`, within the `backupTargetUpdate` action, the input type "BackupTarget" refers to the struct `type BackupTarget struct` and is correctly capitalized as intended.

controller/backup_controller.go (11)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:310-310
Timestamp: 2024-10-29T08:51:32.394Z
Learning: In `controller/backup_controller.go`, when retrieving `backupVolume` using `GetBackupVolumeWithBackupTargetAndVolume`, it can be `nil` without being an error, and the code should handle this case appropriately.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:804-804
Timestamp: 2024-10-24T11:31:44.721Z
Learning: In the `checkMonitor` method in `controller/backup_controller.go`, `backupTarget` is checked for `nil` at the beginning of the method, so accessing `backupTarget.Name` is safe.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/volume_controller.go:4765-4768
Timestamp: 2024-10-24T12:38:46.899Z
Learning: In the `controller/volume_controller.go`, `volume.Spec.BackupTargetName` is guaranteed to be non-empty by the volume mutator. Therefore, it's unnecessary to check if it's empty before using it.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:325-326
Timestamp: 2024-10-24T12:04:44.187Z
Learning: In `controller/backup_controller.go`, `backupTargetClient` is guaranteed to be non-`nil` when there is no error, so additional `nil` checks before its usage are unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:618-626
Timestamp: 2024-10-29T09:06:52.799Z
Learning: In the `getBackupTarget` function in `controller/backup_controller.go`, we should keep the check for `backup.DeletionTimestamp == nil` when `backupTarget` is `nil`, to handle cases where the backup target is deleted and backup CRs are deleting.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:701-717
Timestamp: 2024-10-28T01:22:59.032Z
Learning: The `bbi mutator` ensures that `backupTargetName` is valid before it's used in labels when creating `BackupBackingImage` objects in `controller/backup_controller.go`.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/engine.go:354-376
Timestamp: 2024-10-29T08:27:34.455Z
Learning: When creating a `BackupTarget` in the `CreateBackupTarget` function, it's unnecessary to validate the `backupTargetName` parameter because an error will be returned when creating the `BackupTarget` CR if the name is invalid or empty.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_controller.go:404-404
Timestamp: 2024-10-29T08:47:14.968Z
Learning: In `controller/backup_controller.go`, be aware that after setting the backup status to error when a volume is not found, the code already includes `return nil`, so no additional return statement is needed.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_volume_controller.go:240-242
Timestamp: 2024-10-29T04:23:30.091Z
Learning: In `controller/backup_volume_controller.go`, the `BackupVolumeDelete` method of `backupTargetClient` returns `nil` if the backup volume is not found, so it's unnecessary to check for 'not found' errors after calling this method.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/volume/mutator.go:115-117
Timestamp: 2024-10-28T15:09:08.570Z
Learning: In `webhook/resources/volume/mutator.go`, within the `Create` method of `volumeMutator`, it's acceptable for `bv` to be `nil` after calling `v.ds.GetBackupVolumeWithBackupTargetAndVolumeRO`, as a `nil` `bv` is handled by the volume and engine controllers. Explicit `nil` checks in this context are not necessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/volume/validator.go:494-505
Timestamp: 2024-10-28T15:15:07.305Z
Learning: In the `validateBackupTarget` function in `webhook/resources/volume/validator.go`, when creating a volume, `oldBackupTarget` will be empty, and `newBackupTarget` cannot be empty. Therefore, the validation logic must ensure that `newBackupTarget` is not empty during volume creation.

controller/backup_target_controller.go (5)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:537-579
Timestamp: 2024-10-29T07:10:30.855Z
Learning: Prefer to keep the `syncBackupVolume` and `syncBackupBackingImage` functions separate, rather than refactoring them into a generic sync function.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:361-366
Timestamp: 2024-10-30T06:52:14.797Z
Learning: `btc.bsTimerMap` is always initialized when the backup target controller is created and will not be `nil` in `stopTimer`, so nil checks on `btc.bsTimerMap` are unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:884-897
Timestamp: 2024-10-29T06:24:24.031Z
Learning: In `controller/backup_target_controller.go`, maintain the original error handling in the polling function within `BackupStoreTimer.Start()` as per the team's preference.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:382-399
Timestamp: 2024-10-29T04:28:29.104Z
Learning: In the backup target controller, `backupTarget.Spec.PollInterval` will not be smaller than 0, and the maximum polling interval should not be limited as it can be set to days.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backup_target_controller.go:382-399
Timestamp: 2024-10-29T07:18:29.563Z
Learning: In `controller/backup_target_controller.go`, the logging related to the backup store timer is handled within `BackupStoreTimer.Start()`, so additional logging during timer initialization is unnecessary.

csi/controller_server.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: csi/controller_server.go:1070-1071
Timestamp: 2024-10-30T07:07:32.174Z
Learning: In the method `cleanupBackupVolume` in `csi/controller_server.go`, errors are returned directly and handled at the upper-level method `DeleteSnapshot`. Therefore, it's acceptable to return errors directly in such helper methods without wrapping them.

datastore/longhorn.go (3)

<retrieved_learning>
Learnt from: mantissahz
PR: #2182
File: datastore/longhorn.go:4233-4267
Timestamp: 2024-10-29T07:28:51.100Z
Learning: When reviewing the URL validation in checkBackupTargetURLFormat, prefer to keep the original implementation without refactoring.
</retrieved_learning>

<retrieved_learning>
Learnt from: mantissahz
PR: #2182
File: datastore/longhorn.go:4254-4267
Timestamp: 2024-10-29T06:28:32.626Z
Learning: In datastore/longhorn.go, for the checkBackupTargetURLFormat function, avoid adding global variables for regex patterns unless they are used in multiple places.
</retrieved_learning>

<retrieved_learning>
Learnt from: mantissahz
PR: #2182
File: datastore/longhorn.go:4178-4181
Timestamp: 2024-10-24T13:30:06.074Z
Learning: The function types.GetLonghornAnnotationKey does not exist in the codebase.
</retrieved_learning>

k8s/crds.yaml (2)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: k8s/crds.yaml:4223-4226
Timestamp: 2024-10-29T04:46:47.777Z
Learning: The `crds.yaml` file is automatically generated by the script `k8s/generate_code.sh`.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: k8s/crds.yaml:4223-4226
Timestamp: 2024-10-29T08:32:45.912Z
Learning: The file `k8s/crds.yaml` is automatically generated by the `k8s/generate_code.sh` script; manual edits to this file will be overwritten.

manager/backupbackingimage.go (4)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/backupbackingimage.go:86-88
Timestamp: 2024-10-29T06:46:30.159Z
Learning: In the Longhorn codebase, validation for creating backup backing images is performed by the backup backing image mutator, not within the `CreateBackupBackingImage` function in `manager/backupbackingimage.go`.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/backupbackingimage.go:65-69
Timestamp: 2024-10-29T10:51:51.770Z
Learning: In `manager/backupbackingimage.go`, the `CreateBackupBackingImage` method should retain the existence check for the backing image because the backup backing image mutator does not verify if the backing image exists.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/backupbackingimage.go:44-61
Timestamp: 2024-10-29T06:49:19.112Z
Learning: In `manager/backupbackingimage.go`, the backup target validation is performed by the backing image mutator before calling `restoreBackingImage`.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/volume.go:1180-1182
Timestamp: 2024-10-29T12:51:34.772Z
Learning: Ensure that when reviewing the `restoreBackingImage` function in `manager/volume.go`, verify that variables referenced in error messages are actually undefined before suggesting corrections.

manager/engine.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/engine.go:354-404
Timestamp: 2024-10-29T10:39:23.381Z
Learning: In API methods intended for users or UI, return errors directly and allow the users to handle retry logic; avoid handling conflicts internally.

manager/volume.go (2)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/volume.go:1203-1205
Timestamp: 2024-10-29T04:34:12.107Z
Learning: The 'default' backup target concept is not being removed in the PR for multiple backup stores support.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/volume.go:1180-1182
Timestamp: 2024-10-29T12:51:34.772Z
Learning: Ensure that when reviewing the `restoreBackingImage` function in `manager/volume.go`, verify that variables referenced in error messages are actually undefined before suggesting corrections.

upgrade/util/util.go (1)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/util/util.go:555-573
Timestamp: 2024-10-29T07:21:21.625Z
Learning: The team prefers to keep the original code style and convention in functions like `ListAndUpdateBackupTargetsInProvidedCache`, even if it leads to code duplication.

upgrade/v17xto180/upgrade.go (5)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:52-58
Timestamp: 2024-10-30T08:14:26.801Z
Learning: In the `upgrade/v17xto180/upgrade.go` file, prefer to keep individual error handling in each resource upgrade function instead of consolidating them into a shared helper function.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:174-180
Timestamp: 2024-10-29T14:05:05.010Z
Learning: Ensure that code suggestions provide meaningful improvements over the existing implementation and avoid suggesting changes that match the current code.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:20-23
Timestamp: 2024-10-29T13:13:50.924Z
Learning: In `upgrade/v17xto180/upgrade.go`, the `Upgrade` method performs `namespace` validation at the beginning of the upgrade path, so it's unnecessary to check `namespace` again in `UpgradeResources`.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:25-42
Timestamp: 2024-10-29T08:29:16.196Z
Learning: The team prefers not to add logs during the upgrade process.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: upgrade/v17xto180/upgrade.go:195-195
Timestamp: 2024-10-29T03:58:48.533Z
Learning: The term `BackupImagesDataSources` is the correct resource name in error messages within `upgrade/v17xto180/upgrade.go` and should not be flagged as a typo.

webhook/resources/backingimage/mutator.go (8)

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backingimage/mutator.go:172-195
Timestamp: 2024-10-29T13:15:37.849Z
Learning: In the `findBackupTargetName` function, the loop `for _, bbi := range bbis {...}` handles the case when `bbis` is empty, so an additional early return is unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backingimage/mutator.go:197-213
Timestamp: 2024-10-29T07:04:42.102Z
Learning: In future code reviews, maintain the existing switch statements for URL scheme handling if different handlers for different types might be added later.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:107-112
Timestamp: 2024-10-29T08:29:47.475Z
Learning: In the file `api/backup.go`, for the `BackupTargetUpdate` function, prefer to keep the use of `RetryOnConflictCause` without adding a context with timeout.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backingimage/mutator.go:96-102
Timestamp: 2024-10-30T06:48:50.235Z
Learning: Since `getBackupTargetURLAndBackingImageName` will return an error when `backupURL` is empty, additional validation for `backupURL` is unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:85-90
Timestamp: 2024-10-28T14:55:39.140Z
Learning: Validation of `BackupTargetURL` format is handled in the `backupTargetValidator` in `webhook/resources/backuptarget/validator.go`. Therefore, adding validation in `api/backup.go` is unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: manager/engine.go:0-0
Timestamp: 2024-10-29T07:30:27.089Z
Learning: In the `CreateBackupTarget` function within `manager/engine.go`, the `BackupTargetSpec.BackupTargetURL` field can be empty, so validation should allow empty values for it.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: controller/backing_image_data_source_controller.go:714-714
Timestamp: 2024-10-28T01:25:02.898Z
Learning: In `controller/backing_image_data_source_controller.go`, the method `GetBackupTargetRO` already handles error checking for `bids.Spec.Parameters[longhorn.DataSourceTypeRestoreParameterBackupTargetName]`, so explicit checks before calling it are unnecessary.

Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backupbackingimage/mutator.go:68-80
Timestamp: 2024-10-29T10:24:27.093Z
Learning: In the `Create` method of `backupBackingImageMutator` in `webhook/resources/backupbackingimage/mutator.go`, it's acceptable not to add validation to prevent user overwriting of `LonghornLabelBackupTarget` and `LonghornLabelBackingImage` labels, because these labels are assigned values later in the code.

🔇 Additional comments (56)

manager/backupbackingimage.go (3)

44-61: LGTM! The changes improve backup restoration flow.

The modifications correctly implement the multiple backup stores support by:

Deriving the backing image name from the backup backing image spec
Properly checking for existing backing images before restoration
Using the correct backup target name for restoration

64-64: LGTM! Method signature properly updated.

The signature change appropriately supports multiple backup stores by explicitly accepting the backing image and backup target names.

86-88: LGTM! Spec initialization correctly includes new fields.

The initialization properly sets up the backup backing image with the required fields for multiple backup stores support.

webhook/resources/backup/validator.go (2)

29-36: LGTM: Resource configuration is well-structured

The resource configuration correctly defines the backup validator scope and operations.

46-48: LGTM: Proper name validation implemented

The name validation ensures compliance with Kubernetes naming conventions.

upgrade/v17xto180/upgrade.go (1)

47-183: Verify upgrade paths for all resource types

The upgrade functions handle various resource types and their fields. Please ensure:

All existing resources are properly upgraded with the new fields
Default values are appropriate for each resource type
Label migrations preserve existing metadata

✅ Verification successful

Based on the results, I can now generate the final response:

Upgrade paths for resources are properly implemented

The verification confirms:

Empty BackupTargetName handling is consistent across resources and defaults to types.DefaultBackupTargetName
Label migrations are consistent and preserve metadata with proper key-value pairs
Parameter handling for BackupTargetName in BackingImageDataSource is properly implemented with fallback to default values

The implementation follows a consistent pattern across all resource types and handles edge cases appropriately:

Proper label keys defined in types.go
Consistent label application in upgrade functions
Proper parameter handling in data source upgrades

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify resource types and their fields

# Test 1: Check for existing resources without BackupTargetName
echo "Checking for resources without BackupTargetName..."
rg -l "BackupTargetName.*\"\"" 

# Test 2: Verify label usage consistency
echo "Checking label usage patterns..."
rg "LonghornLabel(BackupTarget|BackupVolume|BackingImage)"

# Test 3: Check for potential parameter conflicts
echo "Checking parameter conflicts..."
rg "DataSourceTypeRestoreParameterBackupTargetName"

Length of output: 8076

api/backup.go (3)

6-24: LGTM! Imports and constants are well-organized.

The new imports support the added functionality, and the default poll interval of 300 seconds (5 minutes) is a reasonable choice for backup target polling.

283-289: LGTM! Backup listing implementation is clear and consistent.

The function uses the appropriate sorting method and includes specific error messaging.

26-128: Verify backup target implementation aligns with multiple stores requirement.

The implementation looks solid, but let's verify that it fully supports the multiple backup stores requirement from PR #5411.

✅ Verification successful

Let me gather more information about the backup target implementation to verify multiple stores support.

Based on the gathered information, I can now provide a final response about the backup target implementation's alignment with multiple stores support.

Multiple backup stores support is properly implemented

The implementation fully supports multiple backup stores as evidenced by:

BackupTarget CRD properly defines independent specs for each target with unique URLs and credentials
No artificial limitations on the number of backup targets in ListBackupTargets operations
Proper concurrent handling with conflict resolution in status updates
Independent polling intervals per target allowing parallel operations

The code aligns well with PR #5411's objective of supporting multiple backup stores.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the backup target implementation supports multiple stores
# Test 1: Check if the backup target CRD supports multiple stores
rg -A 5 "kind: BackupTarget" 

# Test 2: Check for any potential race conditions in backup target handling
rg -A 5 "UpdateBackupTarget.*Status"

Length of output: 1846

Script:

#!/bin/bash
# Check the BackupTarget CRD spec and implementation details
rg -A 10 "type BackupTargetSpec struct"

# Check if there are any limitations on the number of backup targets
rg -A 5 "ListBackupTargets"

# Look for backup target URL validation
rg -A 5 "BackupTargetURL.*validate"

Length of output: 6068

webhook/resources/backingimage/mutator.go (4)

6-6: LGTM!

The net/url import is correctly placed and necessary for URL parsing functionality.

172-195: LGTM! Comprehensive error handling and logging.

The function is well-implemented with:

Proper error handling for listing backup images
Clear error messages
Appropriate logging for URL parsing failures
Correct matching logic for backup target URLs and backing image names

197-213: LGTM! Well-structured URL parsing with extensibility.

The function is well-implemented with:

Robust URL parsing using the standard library
Clear validation of required parameters
Maintainable switch statement structure that allows for future type-specific handling
Support for multiple backup store types (CIFS, NFS, AZBlob, S3)

96-102: Verify integration with backup target functionality.

The logic for finding and setting the backup target name looks correct. However, let's verify that this change integrates properly with the backup target functionality.

✅ Verification successful

Let me gather more specific information about the backup target name handling.

Let me check one final aspect about the backup target validation and error handling.

Based on the gathered information, I can now provide the final response:

Integration with backup target functionality is properly implemented

The code changes are well-integrated with the backup target functionality:

The findBackupTargetName implementation correctly handles backup URL parsing and lookup
The backup target name is properly validated in the data source controller
The parameter is correctly used in volume operations for restoration
Error handling is comprehensive across the workflow

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the integration with backup target functionality
# Test: Check if there are any direct references to BackupTargetName in other files
rg -l "DataSourceTypeRestoreParameterBackupTargetName"

# Test: Check for any potential backup target related validation or handling
rg -A 5 "BackupTarget.*Validate|BackupTarget.*Handler"

Length of output: 2368

Script:

#!/bin/bash
# Check the findBackupTargetName implementation
ast-grep --pattern 'func $_ findBackupTargetName($_) $_ {
  $$$
}'

# Check for validation of BackupTargetName in the data source controller
rg -A 10 "DataSourceTypeRestoreParameterBackupTargetName" controller/backing_image_data_source_controller.go

# Check the BackupTargetName usage in volume operations
rg -A 10 "DataSourceTypeRestoreParameterBackupTargetName" manager/volume.go

Length of output: 1428

Script:

#!/bin/bash
# Check the implementation of findBackupTargetName in the mutator
rg -A 15 "func.*findBackupTargetName" webhook/resources/backingimage/mutator.go

# Check BackupTarget validation in the API
rg -A 10 "Validate.*BackupTarget" k8s/pkg/apis/longhorn/v1beta2/

# Look for any BackupTarget related errors or validation in the webhook
rg "BackupTarget.*Error|BackupTarget.*Valid" webhook/resources/backingimage/

Length of output: 907

manager/engine.go (7)

Line range hint 257-277: LGTM! BackupSnapshot changes align with multiple backup stores support.

The changes correctly integrate the new backupTargetName parameter into both the labels and spec of the backup CR.

Line range hint 331-345: LGTM! Consistent use of read-only operations.

The change to use ListBackupTargetsRO aligns with the pattern of using read-only operations for list operations.

347-352: LGTM! Consistent use of read-only operations.

The change to use GetBackupTargetRO aligns with the pattern of using read-only operations for get operations.

354-376: LGTM! CreateBackupTarget implementation is correct.

The method properly validates input and handles the "not found" case appropriately.

378-404: LGTM! UpdateBackupTarget and helper implementation are correct.

The implementation:

Properly validates input
Uses DeepCopy for safe spec updates
Correctly identifies changes via the helper function
Returns errors directly as expected for API methods

406-408: LGTM! DeleteBackupTarget implementation is correct.

The method correctly delegates to the datastore layer.

484-488: LGTM! ListBackupsForVolume changes support multiple backup stores.

The changes correctly use read-only operations and properly integrate the backup target name in the listing operation.

api/router.go (3)

131-133: LGTM! Well-structured RESTful endpoints.

The implementation follows REST principles and maintains consistency with other resource types in the API.

262-265: LGTM! WebSocket implementation follows established patterns.

The WebSocket stream implementation for backup targets is consistent with other resources in the system, supporting both regular and periodic streams.

125-126: 🛠️ Refactor suggestion

Consider consolidating backup target update operations.

The backupTargetUpdate action is implemented as a POST with query parameter, while standard REST practices suggest using PUT for updates. Since you've already added a dedicated PUT endpoint for backup targets (line 132), consider consolidating the update operations to maintain API consistency.

controller/backup_controller.go (6)

280-288: LGTM! Proper backup target validation

The implementation correctly retrieves and validates the backup target, with appropriate error handling and nil checks.

291-292: LGTM! Clear variable naming

The use of remoteBackupVolumeName improves code clarity by better describing the purpose of the variable.

Line range hint 306-336: LGTM! Comprehensive deletion handling

The implementation properly handles the backup deletion workflow, including:

Attachment ticket cleanup
Remote data cleanup verification
Backup volume sync coordination

617-627: LGTM! Well-implemented backup target retrieval

The method properly handles backup target retrieval with appropriate error handling and correctly maintains the deletion timestamp check for handling backup target deletion cases.

689-717: LGTM! Proper backup backing image handling

The implementation correctly handles backup backing image creation with appropriate defaulting and labeling. The code relies on the bbi mutator for label validation.

Line range hint 864-875: LGTM! Well-structured sync implementation

The method properly handles both backup volume and backup target synchronization with appropriate error handling and conflict resolution.

manager/volume.go (3)

157-158: LGTM: Changes align with multiple backup stores support

The addition of backupTargetName parameter and spec field properly implements the multiple backup stores feature.

Also applies to: 195-195

431-435: LGTM: Improved backup target handling

The code now correctly uses volume.Spec.BackupTargetName instead of labels, and provides better error messages.

Also applies to: 446-446

1180-1182: LGTM: Proper implementation of backup target handling in restoreBackingImage

The changes correctly:

Handle secret validation with proper error messages
Maintain backward compatibility with default backup target
Pass backup target information in source parameters

Also applies to: 1203-1205, 1225-1227

types/types.go (4)

31-34: LGTM: New constants follow conventions.

The new constants for backup targets and operations are well-organized and follow the existing naming conventions.

Also applies to: 136-137

548-553: LGTM: Label functions are well-designed.

The new label functions follow consistent patterns and properly combine labels for backup targets with their associated resources.

Also applies to: 582-593

739-749: LGTM: Name generation functions are well-structured.

The backup resource name generation functions are well-organized:

Common logic is properly extracted into a helper function
Consistent name generation pattern using checksum and random ID
Follows the DRY principle

167-168: LGTM: Label constants follow conventions.

The new label constants for backup targets and volumes are properly grouped with other label constants and follow the established naming pattern.

csi/controller_server.go (3)

Line range hint 130-150: LGTM! Error handling follows CSI spec.

The error handling is well implemented with appropriate CSI status codes and clear error messages. The code properly validates backup volume availability before proceeding with backup retrieval.

1070-1071: LGTM! Using centralized backup volume retrieval.

The change properly utilizes the new getBackupVolume helper method. Direct error return is appropriate here as these errors are handled at the upper-level DeleteSnapshot method, as noted in the learnings.

1286-1286: LGTM! Consistent with backup volume retrieval pattern.

The change properly integrates with the centralized backup volume retrieval pattern and maintains appropriate error handling using CSI status codes.

controller/volume_controller_test.go (2)

1140-1140: LGTM: BackupTargetName field added to VolumeSpec

The addition of BackupTargetName field to the VolumeSpec aligns with the PR objective of supporting multiple backup stores.

1387-1394: Verify backup target name consistency

The BackupVolume resource is correctly updated to include the backup target name in both labels and spec. However, we should ensure that the backup target name is consistently used across related operations.

✅ Verification successful

Based on the extensive search results, I can now generate the final response:

Backup target name usage is consistent and properly validated

The codebase shows consistent handling of backup target names through:

Volume mutator ensures backup target name is set to types.DefaultBackupTargetName if empty
Volume validator validates backup target name before use
Consistent label propagation in both specs and labels across backup-related resources
Proper validation and synchronization between volume and backup target resources

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify consistent usage of backup target name across the codebase
echo "Checking BackupTarget usage in volume and backup related code..."
rg --type go "BackupTarget.*Name" -B 2 -A 2

# Check for potential validation of backup target name
ast-grep --pattern 'func $_($_, $_, $_) {
  $$$
  BackupTarget
  $$$
}'

Length of output: 70127

api/model.go (4)

64-64: LGTM: New backup target fields added to structs

The new BackupTargetName fields added to various structs are consistent with the PR objective of supporting multiple backup stores. The field names and types are appropriate.

Also applies to: 143-144, 177-177, 194-195

880-907: LGTM: BackupTarget schema properly configured

The BackupTarget schema is well-defined with:

Appropriate CRUD methods
Required field validation
Sensible defaults (especially for pollInterval)
ReadOnly flag implementation as per PR objectives

1837-1837: LGTM: Resource conversion functions properly updated

The conversion functions correctly map the new backup target related fields between API models and resource representations.

Also applies to: 1870-1871, 1921-1922, 1978-1978

914-917: Verify the input type for backupTargetUpdate action

The input type "BackupTarget" is capitalized differently from other input types in the codebase. However, based on the learnings from the user's previous feedback, this is intentional as it refers to the struct type BackupTarget struct.

Let's verify the consistency of input type usage:

✅ Verification successful

Let me gather more information about the BackupTarget struct to ensure our verification is complete.

Input type "BackupTarget" is correctly capitalized

Based on the codebase analysis and the previous learning, the capitalization of "BackupTarget" as the input type is correct. The struct is consistently defined as BackupTarget across multiple packages in the codebase, including api/model.go, engineapi/types.go, and client/generated_backup_target.go. This matches the input type specification in the action definition.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the consistency of input type usage in the codebase
# Look for other action input type definitions to compare

# Search for Input field assignments in action definitions
rg -U 'Input:\s+"[^"]+"' --type go

Length of output: 2072

Script:

#!/bin/bash
# Search for BackupTarget struct definition and its usage
ast-grep --pattern 'type BackupTarget struct {
  $$$
}'

Length of output: 6867

controller/engine_controller.go (2)

1422-1427: LGTM! Well-structured error handling.

The error handling for backup target retrieval is well-implemented with:

Clear distinction between "not found" and other API errors
Descriptive error messages with proper context
Consistent use of errors.Wrapf

1448-1456: LGTM! Clear version-specific error handling.

The implementation:

Correctly handles restore errors based on CLI API version
Uses descriptive variable names
Has clear error handling logic

k8s/crds.yaml (5)

657-665: LGTM: BackupBackingImage CRD backupTargetName field definition

The field is properly defined with:

Nullable property allowing optional usage
Clear description
Consistent with the multiple backup stores feature objective

887-890: LGTM: Backup CRD backupTargetName field definition

The field is properly defined with:

Nullable property allowing optional usage
Clear description
Consistent with other CRDs

1128-1131: LGTM: BackupTarget CRD readOnly field definition

The field is properly defined with:

Boolean type
Clear description explaining its purpose for preventing backup creation
Aligns with the PR objective for read-only backup targets

4223-4226: ⚠️ Potential issue

Inconsistency: Volume CRD backupTargetName field missing nullable property

The backupTargetName field in the Volume CRD is missing the nullable: true property that is present in other CRDs (BackupBackingImage, Backup). This inconsistency could cause issues with null value handling.

Add the nullable property:

  backupTargetName:
    description: The backup target name that the volume will be backed
      up to or is synced.
+   nullable: true
    type: string

Likely invalid or redundant comment.

Line range hint 1-4500: Verify upgrade path for new CRD fields

As mentioned in the PR comments by @ChanYiLin, please ensure there is a proper upgrade plan for the new CRD fields (backupTargetName, readOnly) when users upgrade from previous versions.

Run the following script to check for any existing usage of these fields that might be affected during upgrade:

datastore/longhorn.go (4)

921-945: LGTM: Owner reference functions are well-implemented

The owner reference functions for backup targets and backup volumes follow consistent patterns and best practices:

Proper type definitions
Consistent naming
Clear separation of concerns

4128-4144: LGTM: ListBackupTargetsRO implementation

The read-only list function is well implemented with:

Proper error handling
Informative logging
Efficient map allocation

4351-4425: LGTM: Backup volume listing functions are well structured

The functions handle:

Proper selector creation
Efficient error handling
Clear error messages for duplicate entries

4535-4556: LGTM: Backup listing functions follow best practices

The implementation:

Reuses selector logic
Provides both RO and mutable versions
Has proper error handling

webhook/resources/backup/validator.go

upgrade/v17xto180/upgrade.go

upgrade/util/util.go

controller/volume_controller.go

mantissahz self-assigned this Sep 28, 2023

mantissahz force-pushed the issue5411 branch from d3e7c73 to 872a5bc Compare October 5, 2023 03:47

mantissahz force-pushed the issue5411 branch from 872a5bc to ec2bf08 Compare October 25, 2023 06:19

mantissahz force-pushed the issue5411 branch from ec2bf08 to a7f38a2 Compare October 26, 2023 07:56

mantissahz force-pushed the issue5411 branch from a7f38a2 to 9207d10 Compare November 3, 2023 14:43

mantissahz force-pushed the issue5411 branch 3 times, most recently from a782b93 to 142535f Compare November 21, 2023 01:02

mantissahz marked this pull request as ready for review November 22, 2023 01:23

mantissahz requested a review from a team as a code owner November 22, 2023 01:23

mantissahz force-pushed the issue5411 branch 3 times, most recently from c1e1178 to 4cdb679 Compare November 23, 2023 16:48

mantissahz marked this pull request as draft November 30, 2023 02:10

mantissahz force-pushed the issue5411 branch from 4cdb679 to 4e67c1e Compare December 4, 2023 00:57

mantissahz marked this pull request as ready for review December 4, 2023 03:19

james-munson reviewed Dec 4, 2023

View reviewed changes

api/model.go Outdated Show resolved Hide resolved

api/model.go Outdated Show resolved Hide resolved

james-munson reviewed Dec 8, 2023

View reviewed changes

controller/backup_target_controller.go Outdated Show resolved Hide resolved

james-munson reviewed Dec 8, 2023

View reviewed changes

webhook/resources/backup/mutator.go Outdated Show resolved Hide resolved

james-munson requested changes Dec 8, 2023

View reviewed changes

mantissahz force-pushed the issue5411 branch from 4e67c1e to fe4d30c Compare December 12, 2023 07:10

mantissahz requested review from a team and ChanYiLin December 12, 2023 07:10

ChanYiLin requested changes Dec 12, 2023

View reviewed changes

mantissahz requested review from ChanYiLin and a team December 12, 2023 09:08

mantissahz added 12 commits October 30, 2024 15:38

feat(backup): methods to get backup volume CRs

f37caff

List backup volume CRs using a backup target name. List backup volume CRs using a volume name. Get a backup volume using a backup target name and a volume name. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): backup target mutation

5dfebd2

Add the backup target mutator and add the finalizer into the backupTarget CR when mutating. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): volume mutation and validation for mutiple backup targets

dab50a4

Add the volume mutation and validation to handle the field `Spec.BackupTargetName`. Add a new label "backup-target". ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): new fields of BackupBackingImage CRD

44c2241

Add fields `Spec.BackingImage` and `Spec.BackupTargetName` in the BackupBackingImage CRD ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): move out backup target logic from setting controller

32bfa63

Move backupTimer from setting controller to backup target controller Move AWS IAM Role Annotation logic to datastore/kubernetes.go ref: longhorn/longhorn 5411, 6947 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): add backup target validator

92abea1

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): modify setting and uninstall controllers

6a49632

Remove adding `backup-target` back when node is updated. Remove backup targets when uninstalling. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): new filed of Volume CR

1d35163

Introduce a new filed `BackupTargetName` in `Volume.Spec` to point out where the volume will be backed up to. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): add backup target label when creating a backup

5b53968

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

mantissahz force-pushed the issue5411 branch from 7a1e122 to 438a842 Compare October 30, 2024 07:38

coderabbitai bot reviewed Oct 30, 2024

View reviewed changes

mantissahz added 14 commits October 30, 2024 16:35

feat(backup): handle the restoration

87d4702

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): handle triggering the backup volume synchronization

2770fcf

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): recurringjob for multiple backupstores

3863ecf

Add the method to get the correct BackupVolume with the volume name and backup target name for the recurring job and related data strucurtes of APIs. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): add backup target APIs

a7e9e1c

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): backup validation

6036180

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): modify volume controller unit tests for backup targets

ffab803

Add more parameters in creating BackupVolume for testing volume_controller.go ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): modify getting BackupVolume for csi backup

f5d6cc3

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): modify controllers related to backup backing image

f190b0c

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): modify backing image data source controller

789bf78

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): add backup target name when mutating BackingImage for t…

4a9652c

…he restoring type ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): modify system backup controller

ef8e0b3

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): upgrade CRs for multiple backup store

1cf1bda

Upgrade CRs for Volume, BackupVolume, Backup, BackupBackingImage, and BackingImageDataSource. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): add websocket for backup targets

39595e1

Ref: 5411 Signed-off-by: James Lu <james.lu@suse.com>

feat(backup): update k8s/crds.yaml

97553ea

ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>

mantissahz force-pushed the issue5411 branch from 438a842 to 97553ea Compare October 30, 2024 08:50

coderabbitai bot reviewed Oct 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backup): multiple backup stores support #2182

feat(backup): multiple backup stores support #2182

mantissahz commented Sep 28, 2023 •

edited

Loading

mergify bot commented Oct 25, 2023

mergify bot commented Oct 27, 2023

innobead commented Nov 22, 2023

mantissahz commented Nov 22, 2023

james-munson left a comment

james-munson left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

feat(backup): multiple backup stores support #2182

Are you sure you want to change the base?

feat(backup): multiple backup stores support #2182

Conversation

mantissahz commented Sep 28, 2023 • edited Loading

mergify bot commented Oct 25, 2023

mergify bot commented Oct 27, 2023

innobead commented Nov 22, 2023

mantissahz commented Nov 22, 2023

james-munson left a comment

Choose a reason for hiding this comment

james-munson left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

mantissahz commented Sep 28, 2023 •

edited

Loading