fix(backup): modify backup volume/volume APIs and naming #3360
Conversation
WalkthroughThis pull request introduces a series of modifications across multiple files in the Longhorn project, focusing on backup volume management and naming conventions. The changes primarily involve renaming variables related to backup volumes, adding new methods for updating backup targets, and enhancing routing and API functionality. The modifications aim to improve code clarity, introduce new API capabilities, and standardize naming conventions across different components of the backup and volume management system. Changes
Sequence DiagramsequenceDiagram
participant API as API Layer
participant Controller as Volume Controller
participant Manager as Volume Manager
participant Storage as Storage System
API->>Controller: Update Backup Target Name
Controller->>Manager: VolumeUpdateBackupTargetName
Manager->>Storage: Update Volume Backup Target
Storage-->>Manager: Confirmation
Manager-->>Controller: Updated Volume
Controller-->>API: Return Updated Volume
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
mantissahz
force-pushed
the
issue5411v1217
branch
from
edd7dd4 to
c24900e
Compare
mantissahz
force-pushed
the
issue5411v1217
branch
from
c24900e to
6d836b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
api/backup.go (4)
246-247: Validate backupVolumeName for empty string earlier.You're checking if backupVolumeName is empty and returning an error. As a minor improvement, consider validating it earlier (e.g. immediately after reading the request) to fail fast.
268-270: Check for potential concurrency or usage conflicts.Deleting a backup volume might collide with other operations referencing it (like listing, restoring, etc.). Consider verifying whether you need concurrency control or usage checks prior to deletion.
318-325: Improve not found behavior.When a backup is not found, we log a warning and return HTTP 404. This is good practice, but consider providing a more descriptive message or recommended action so that users know how to proceed.
345-360: Enhance partial cleanup logic.After deleting a backup, the code retrieves the backup volume and returns its state. In case the volume is also no longer present, you handle that scenario gracefully by returning an error. You might want to add an explicit check or log message if the volume is missing, so the user knows it was fully cleaned up.
api/model.go (1)
1990-1992: Consider using a constant for the label keyThe string literal
types.LonghornLabelBackupVolumeshould be used instead of directly accessing the label to maintain consistency and make future updates easier.-volumeName, ok := b.Labels[types.LonghornLabelBackupVolume] +volumeName, ok := b.Labels[types.LonghornLabelBackupVolume]api/volume.go (1)
839-859: LGTM! Consider adding input validation.The implementation follows the established pattern for volume update methods and includes proper error handling. However, consider adding validation for the
backupTargetNameinput to ensure it meets any required format or constraints.func (s *Server) VolumeUpdateBackupTargetName(rw http.ResponseWriter, req *http.Request) error { var input UpdateBackupTargetInput id := mux.Vars(req)["name"] apiContext := api.GetApiContext(req) if err := apiContext.Read(&input); err != nil { return errors.Wrap(err, "failed to read BackupTarget input") } + // Validate backup target name + if input.BackupTargetName == "" { + return fmt.Errorf("backup target name cannot be empty") + } + obj, err := util.RetryOnConflictCause(func() (interface{}, error) { return s.m.UpdateVolumeBackupTarget(id, input.BackupTargetName) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/backup.go(6 hunks)api/model.go(6 hunks)api/router.go(2 hunks)api/volume.go(1 hunks)controller/backup_controller.go(1 hunks)controller/uninstall_controller.go(2 hunks)manager/engine.go(3 hunks)manager/kubernetes.go(2 hunks)manager/volume.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- controller/backup_controller.go
🧰 Additional context used
📓 Learnings (2)
api/volume.go (1)
Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: k8s/pkg/apis/longhorn/v1beta2/volume.go:305-307
Timestamp: 2024-11-10T16:45:04.898Z
Learning: In Longhorn Manager, volume-related API handlers handle the new `BackupTargetName` field through the volume mutator.
api/model.go (1)
Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/model.go:914-917
Timestamp: 2024-11-10T16:45:04.898Z
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.
🔇 Additional comments (18)
manager/kubernetes.go (1)
39-50: Check for nil backupVolume before setting storageClassName.
Currently, if the backup volume is nil, we simply skip assigning a storage class without surfacing a warning or error. Consider improving the visibility of this condition, either by logging a warning message or returning an error, depending on your desired user experience.
You can run the script below to verify if there are any references or checks against a nil backup volume in the rest of your codebase:
api/backup.go (3)
203-207: Renaming to backupVolumeName clarifies intent.
This rename from “volName” to “backupVolumeName” improves clarity. Be sure all related variables and error messages reflect this naming consistently.
251-259: Potential range check for backup volume existence.
Here you retrieve the backup volume and log a “failed to get backup” if not found. Once obtained, the code proceeds to sync it only if input.SyncBackupVolume is true. If the backup volume retrieval fails due to transient or undetermined reasons, be sure you’re handling partial failures or retries as appropriate for your environment.
286-293: Ensure consistent naming for “BackupListByBackupVolume”.
Renaming the function to “BackupListByBackupVolume” is clear. Verify all references (router, documentation, logs) have been updated to use this new naming convention.
✅ Verification successful
Function name is consistently used across the codebase
The function BackupListByBackupVolume is correctly referenced in both its implementation (api/backup.go) and router registration (api/router.go). No inconsistencies were found in the naming convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check references to the old BackupListByVolumeName
rg 'BackupListByVolumeName'
Length of output: 27
Script:
#!/bin/bash
# Let's check for any references to the current function name and related patterns
rg -i "backuplistbybackupvolume|backuplistbyvolume"
# Let's also check the router setup to see how this endpoint is registered
ast-grep --pattern 'router.$_($_, $_, $_)'
Length of output: 379
manager/engine.go (3)
437-444: Good fallback approach for missing BackupVolume CR.
Returning a new BackupVolume object when the CR is not found ensures backward compatibility with certain CSI operations. This approach looks robust.
505-515: New “ListBackupsForBackupVolumeSorted” method looks consistent.
Introducing this method centralizes logic for listing backups by the backup volume name. Ensure your code handles the empty list case and logs or reacts accordingly.
461-462: Confirm no references remain to the old volumeName parameter.
Deleting the backup volume now relies on backupVolumeName. Searching your codebase for the old parameter name could uncover possible missed references.
✅ Verification successful
No issues found with the parameter rename
The codebase search shows that all internal calls to DeleteBackupVolume consistently use backupVolumeName as the parameter name. The only occurrence of volumeName is in the vendor package backupstore, which is an external dependency and not part of the main codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
rg 'DeleteBackupVolume(.*volumeName' -A 3
Length of output: 144
Script:
#!/bin/bash
# Search for DeleteBackupVolume function calls with any parameter name
rg "DeleteBackupVolume\([^)]*\)" -A 3
# Also search for any references to volumeName near DeleteBackupVolume
rg "volumeName.*DeleteBackupVolume|DeleteBackupVolume.*volumeName" -A 3
Length of output: 2286
api/router.go (4)
92-92: New "updateBackupTargetName" action is aligned with naming changes.
This addition matches your updated naming convention for backup volume interactions. Great consistency!
137-138: Renaming path parameter to {name} is clearer.
Changing “{volName}” to “{name}” for backup volumes standardizes your routes. Ensure you’ve updated any documentation or references that mention “{volName}.”
140-140: Switching “backupList” to “BackupListByBackupVolume.”
This is a logical rename that clearly conveys the resource context. Validate that all route calls are updated.
146-146: Review query-based actions on “{name}” path.
Line 146 changes the parameter to “{name}” for backup volumes. Verify that the handler logic (and any scripts referencing it) accommodates this new path parameter.
✅ Verification successful
Based on the search results, I can now generate the final response:
The path parameter change from {volName} to {name} is consistent with the codebase patterns.
The codebase consistently uses {name} as the path parameter across all endpoints, and the handlers correctly extract this parameter using mux.Vars(req)["name"]. The backup volume related handlers in api/backup.go already use backupVolumeName := mux.Vars(req)["name"] to extract the parameter, showing they're aligned with this change. This is a safe modification that follows the established API conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
rg '/v1/backupvolumes/{volName}'
Length of output: 163
Script:
#!/bin/bash
# Search for backup volume related API paths and handlers
rg -A 5 "/v1/backupvolumes"
# Search for backup volume related handler functions
ast-grep --pattern 'func $_($_, $_, $_) {
$$$
volName := $$$
$$$
}'
Length of output: 2131
Script:
#!/bin/bash
# Search for backup volume handlers to check parameter usage
ast-grep --pattern 'func $_BackupVolume$_(w http.ResponseWriter, req *http.Request) error {
$$$
}'
# Search for action handlers related to backup volumes
ast-grep --pattern 'func $_Backup$_(w http.ResponseWriter, req *http.Request) error {
$$$
}'
# Look for gorilla mux variable usage
rg "mux.Vars" -A 3
Length of output: 21168
api/model.go (4)
400-402: LGTM: New input type follows Go conventions
The UpdateBackupTargetInput struct is well-defined with proper JSON tags and follows the existing pattern for input types.
1105-1107: LGTM: Action registration is consistent
The "updateBackupTarget" action is properly registered with the correct input type.
1682-1682: LGTM: Action properly added for detached volumes
The "updateBackupTarget" action is correctly added to the allowed actions for detached volumes.
1714-1714: LGTM: Action properly added for attached volumes
The "updateBackupTarget" action is correctly added to the allowed actions for attached volumes.
manager/volume.go (2)
436-458: LGTM! Error message improvements and variable renaming.
The changes improve error message clarity and maintain consistent variable naming.
1277-1301: LGTM! Well-structured implementation with proper error handling.
The implementation follows the established pattern for volume update methods with:
- Proper error wrapping
- Idempotency check
- Appropriate logging
controller/uninstall_controller.go (1)
Line range hint 773-782: LGTM! Improved error handling and method call update.
The changes enhance error handling by:
- Adding a check for missing backup volume label
- Using direct deletion when appropriate
- Updating to the new method signature with backup target name
mantissahz
force-pushed
the
issue5411v1217
branch
from
6d836b4 to
741ed09
Compare
mantissahz
force-pushed
the
issue5411v1217
branch
from
741ed09 to
0fe44bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/router.go (1)
Line range hint
259-266: Consider addressing the WebSocket filtering TODO.Since this PR is modifying backup volume APIs, it might be a good opportunity to address the TODO comment about WebSocket filtering for backup volumes. This would improve the efficiency of the WebSocket endpoint by reducing unnecessary traffic.
Would you like me to help design a solution for implementing the WebSocket filtering functionality? I can create a GitHub issue to track this enhancement.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/backup.go(6 hunks)api/model.go(6 hunks)api/router.go(2 hunks)controller/backup_controller.go(1 hunks)controller/uninstall_controller.go(2 hunks)manager/engine.go(3 hunks)manager/kubernetes.go(2 hunks)manager/volume.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- controller/backup_controller.go
- controller/uninstall_controller.go
- manager/kubernetes.go
- api/model.go
- manager/engine.go
- api/backup.go
🔇 Additional comments (3)
manager/volume.go (1)
436-458: LGTM! Error handling improvements look good.
The changes improve error message clarity and maintain consistent error handling patterns.
api/router.go (2)
92-92: LGTM! Verify handler implementation.
The new action "updateBackupTargetName" follows the established naming convention and integrates well with existing volume actions.
Let's verify the handler implementation:
✅ Verification successful
Handler implementation verified and properly structured
The handler VolumeUpdateBackupTargetName is correctly implemented in api/volume.go with proper request handling:
- Takes standard http.ResponseWriter and *http.Request parameters
- Uses mux for route parameter extraction
- Includes input validation through API context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the handler implementation exists
# Expected: Find the implementation of VolumeUpdateBackupTargetName
ast-grep --pattern 'func (*Server) VolumeUpdateBackupTargetName'
Length of output: 66
Script:
#!/bin/bash
# Let's try a broader search to find the handler implementation
# First, let's search for any mention of VolumeUpdateBackupTargetName
rg "VolumeUpdateBackupTargetName" -A 5
# Also search for similar patterns in case it's named slightly differently
rg "UpdateBackupTargetName" -A 5
Length of output: 1535
137-138:
Breaking Change: API parameter renamed.
The route parameter change from {volName} to {name} is a breaking change that could affect API clients. While this change improves consistency with REST API conventions, it requires proper documentation and migration guidance.
Let's verify if this change is documented:
mantissahz
force-pushed
the
issue5411v1217
branch
from
56020e2 to
2610bac
Compare
mantissahz
force-pushed
the
issue5411v1217
branch
from
2610bac to
33a88aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
datastore/longhorn.go (2)
304-304: Consider streamlining backup target validation to avoid duplication.Retrieved learnings mention that backup target URL format is already validated in the webhook. Keeping a second validation path here may lead to duplicated logic. Consider consolidating checks in one place to maintain consistency and reduce maintenance overhead.
4320-4320: Review behavior for empty backupTargetName.If the “backupTargetName” parameter is empty, the current logic appears to skip duplication checks for the currently processed resource. Consider guarding against that corner case or clarifying that behavior within the method’s documentation to avoid potential confusion.
csi/controller_server.go (2)
284-289: LGTM! Consider using a constant for clarityThe changes improve error handling by distinguishing between "not found" and error cases. The debug logging helps with troubleshooting.
Consider extracting the magic number into a constant for better maintainability:
+const maxBackupVolumesPerTarget = 1 -if len(list.Data) > 1 { +if len(list.Data) > maxBackupVolumesPerTarget { return nil, fmt.Errorf("found multiple backup volumes for backup target %s and volume %s", vol.BackupTargetName, volumeName) }
1292-1295: LGTM! Consider fixing indentationThe nil check for backupVolume is consistent with the error handling pattern established earlier.
Fix the indentation for better readability:
if backupVolume == nil { - return nil, nil + return nil, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
api/backup.go(8 hunks)api/model.go(6 hunks)api/router.go(2 hunks)api/volume.go(1 hunks)controller/backup_controller.go(1 hunks)controller/uninstall_controller.go(2 hunks)csi/controller_server.go(6 hunks)datastore/longhorn.go(5 hunks)manager/engine.go(3 hunks)manager/kubernetes.go(2 hunks)manager/volume.go(2 hunks)webhook/resources/backuptarget/validator.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- manager/kubernetes.go
- manager/volume.go
- controller/backup_controller.go
- controller/uninstall_controller.go
- api/volume.go
- webhook/resources/backuptarget/validator.go
- api/model.go
🧰 Additional context used
📓 Learnings (1)
datastore/longhorn.go (3)
Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:85-90
Timestamp: 2024-11-10T16:45:04.898Z
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: datastore/longhorn.go:4233-4267
Timestamp: 2024-11-10T16:45:04.898Z
Learning: When reviewing the URL validation in `checkBackupTargetURLFormat`, prefer to keep the original implementation without refactoring.
Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backuptarget/validator.go:102-105
Timestamp: 2024-11-10T16:45:04.898Z
Learning: In `webhook/resources/backuptarget/validator.go`, the method `ValidateBackupTargetURL` in `DataStore` checks if `BackupTargetURL` is an empty string, so additional checks for empty strings are unnecessary during backup target validation.
🔇 Additional comments (19)
datastore/longhorn.go (3)
4284-4284: Ensure consistency with the webhook validator.
While this method is essential for checking backup target URLs, confirm that the logic aligns with the existing webhook-based validation. Otherwise, updates in one place could inadvertently desynchronize validations between the DataStore and the webhook.
4296-4296: Looks good.
This call to “validateBackupTargetURLExisting” correctly encapsulates the logic for ensuring unique backup target URLs without collisions. No immediate issues detected.
4343-4343: Clear and concise collision detection.
This ensures that two distinct backup target CRs cannot claim the same URL path. The condition checks appear correct and consistent with the “strings.ToLower()” usage for collision prevention.
api/backup.go (7)
Line range hint 26-31: LGTM! Improved variable naming
The change from name to backupTargetName enhances code clarity by using a more descriptive variable name.
203-209: LGTM! Consistent naming and error message formatting
The changes improve code consistency by:
- Using the more explicit
backupVolumeNameinstead ofvolName - Using single quotes in error messages
246-259: LGTM! Consistent error handling and naming
The changes maintain consistency in both variable naming and error message formatting.
268-271: LGTM! Consistent naming
The change maintains consistency with the new naming convention.
286-295: Add test coverage for BackupListByBackupVolume
While the renaming improves clarity, test coverage is needed for this handler.
Line range hint 318-329: LGTM! Consistent error handling
The changes maintain consistency in both variable naming and error message formatting.
345-362: LGTM! Consistent error handling
The changes maintain consistency in both variable naming and error message formatting.
manager/engine.go (3)
437-444: LGTM! Good compatibility handling
The changes improve API consistency while maintaining compatibility with the Longhorn CSI plugin by returning an empty backup volume when not found.
461-462: LGTM! Consistent naming
The change maintains consistency with the new naming convention.
505-515: LGTM! Clean implementation with proper error handling
The new method follows existing patterns and properly handles the NotFound case by returning an empty list.
api/router.go (5)
92-92: LGTM! Consistent action definition
The new action follows the established pattern for volume actions.
123-127: LGTM! Well-structured RESTful routes
The new backup target routes follow RESTful conventions and are properly wrapped with error handling.
138-139: LGTM! Consistent route parameters
The route parameter update maintains consistency with the new naming convention.
141-141: LGTM! More explicit handler name
The handler name change better describes its functionality.
147-147: LGTM! Consistent route parameters
The route parameter update maintains consistency with the new naming convention.
csi/controller_server.go (1)
Line range hint 1250-1277: LGTM! Nil check prevents potential panic
The added nil check for backup.SnapshotCreated prevents potential nil pointer dereference and aligns with the function's purpose of waiting for backup controller sync.
mantissahz
force-pushed
the
issue5411v1217
branch
2 times, most recently
from
ed67ae5 to
0231a62
Compare
Add an API to update the field `BackupTargetname` of a volume. Modify backup volume list API to get information by backup volume name. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <[email protected]>
And correct the way to get the BackupVolume CR. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <[email protected]>
ref: longhorn/longhorn 5411, 9987 Signed-off-by: James Lu <[email protected]>
ref: longhorn/longhorn 5411, 9987 Signed-off-by: James Lu <[email protected]>
derekbit
force-pushed
the
issue5411v1217
branch
from
0231a62 to
a711e4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/volume.go (1)
839-859: LGTM with a suggestion for input validation.The implementation follows the established patterns for volume update handlers and includes proper error handling with retries. Consider adding validation for the backup target name input to ensure it's not empty or malformed.
Add input validation before processing:
func (s *Server) VolumeUpdateBackupTargetName(rw http.ResponseWriter, req *http.Request) error { var input UpdateBackupTargetInput id := mux.Vars(req)["name"] apiContext := api.GetApiContext(req) if err := apiContext.Read(&input); err != nil { return errors.Wrap(err, "failed to read BackupTarget input") } + + if input.BackupTargetName == "" { + return errors.New("backup target name cannot be empty") + } obj, err := util.RetryOnConflictCause(func() (interface{}, error) { return s.m.UpdateVolumeBackupTarget(id, input.BackupTargetName) })csi/controller_server.go (2)
287-292: Consider adding trace logging for multiple backup volumes case.While the condition change from
>= 2to> 1is correct, consider adding trace logging when multiple backup volumes are found to help with debugging.if len(list.Data) > 1 { + log.Tracef("found %d backup volumes for backup target %s and volume %s: %v", + len(list.Data), vol.BackupTargetName, volumeName, + list.Data) return nil, fmt.Errorf("found multiple backup volumes for backup target %s and volume %s", vol.BackupTargetName, volumeName) }
1295-1298: Consider adding debug logging for nil backup volume case.For better observability, consider adding debug logging when backup volume is nil.
if backupVolume == nil { + log.Debugf("backup volume not found for volume %s", volumeName) return nil, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
api/backup.go(8 hunks)api/model.go(6 hunks)api/router.go(2 hunks)api/volume.go(1 hunks)controller/backup_controller.go(1 hunks)controller/uninstall_controller.go(2 hunks)csi/controller_server.go(6 hunks)datastore/longhorn.go(5 hunks)manager/engine.go(3 hunks)manager/kubernetes.go(2 hunks)manager/volume.go(2 hunks)webhook/resources/backuptarget/validator.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- manager/volume.go
- manager/kubernetes.go
- webhook/resources/backuptarget/validator.go
- controller/backup_controller.go
- api/model.go
- controller/uninstall_controller.go
🧰 Additional context used
📓 Learnings (2)
api/volume.go (1)
Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: k8s/pkg/apis/longhorn/v1beta2/volume.go:305-307
Timestamp: 2024-11-10T16:45:04.898Z
Learning: In Longhorn Manager, volume-related API handlers handle the new `BackupTargetName` field through the volume mutator.
datastore/longhorn.go (4)
Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: api/backup.go:85-90
Timestamp: 2024-11-10T16:45:04.898Z
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: datastore/longhorn.go:4233-4267
Timestamp: 2024-11-10T16:45:04.898Z
Learning: When reviewing the URL validation in `checkBackupTargetURLFormat`, prefer to keep the original implementation without refactoring.
Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backuptarget/validator.go:102-105
Timestamp: 2024-11-10T16:45:04.898Z
Learning: In `webhook/resources/backuptarget/validator.go`, the method `ValidateBackupTargetURL` in `DataStore` checks if `BackupTargetURL` is an empty string, so additional checks for empty strings are unnecessary during backup target validation.
Learnt from: mantissahz
PR: longhorn/longhorn-manager#2182
File: webhook/resources/backuptarget/validator.go:0-0
Timestamp: 2024-11-10T16:45:04.898Z
Learning: In the `validateDRVolume` function within `webhook/resources/backuptarget/validator.go`, prefer comparing backup target names via labels instead of URLs when validating DR volumes to account for potential URL formatting differences.
🔇 Additional comments (11)
datastore/longhorn.go (4)
304-305: LGTM: Improved backup target URL validation
The addition of backupTargetName parameter allows proper validation by excluding the current backup target when checking for duplicate URLs.
Line range hint 4292-4304: LGTM: Proper handling of backup target URL validation
The implementation correctly validates backup target URLs while avoiding false positives by excluding the current backup target from duplicate checks.
Line range hint 4328-4351: LGTM: Robust backup target URL validation logic
The implementation:
- Properly normalizes and compares backup target paths
- Handles different URL schemes correctly
- Provides clear error messages
- Correctly excludes current backup target from duplicate checks
4351-4354: LGTM: Clear error handling for duplicate URLs
The implementation provides clear error messages when duplicate backup target URLs are detected.
api/backup.go (1)
Line range hint 26-32: LGTM! Consistent naming improves code clarity.
The renaming changes from name/volName to backupTargetName/backupVolumeName improve code clarity and maintain consistency across the codebase.
Tests needed for BackupListByBackupVolume handler
The handler BackupListByBackupVolume lacks test coverage.
Also applies to: 203-210, 246-262, 286-296, 318-364
manager/engine.go (2)
Line range hint 437-450: LGTM! Improved error handling for compatibility.
The function now properly handles NotFound errors by returning an empty BackupVolume object, maintaining compatibility with the Longhorn CSI plugin.
505-515: LGTM! Well-structured new function.
The new ListBackupsForBackupVolumeSorted function:
- Properly handles NotFound case by returning empty list
- Reuses existing functionality via
ListBackupsForVolumeSorted - Maintains consistent error handling patterns
api/router.go (2)
123-127: LGTM! Well-structured API routes.
The changes maintain RESTful patterns and provide consistent naming across endpoints:
- Complete CRUD operations for backup targets
- Consistent use of
backupVolumeNameparameter - Proper handler mappings with forwarding setup
Also applies to: 138-139, 141-141, 147-147
92-92: LGTM! New volume action handler.
The addition of updateBackupTargetName action maintains consistency with other volume actions.
csi/controller_server.go (2)
132-134: Improved error handling for backup volume retrieval.
The error handling has been separated into distinct cases for better error reporting and debugging, as discussed in previous reviews.
1253-1253: Verify backup state before accessing fields.
The added nil check for backup before accessing SnapshotCreated field prevents potential nil pointer dereferences.
Also applies to: 1278-1278
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#5411
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context