fix: add data engine option to restore backing image api #3392
Conversation
WalkthroughThis pull request introduces modifications to the backing image backup and restore functionality across multiple files. The changes primarily focus on adding support for a new Changes
Assessment against linked issues
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
manager/volume.go (2)
1200-1202: Fallback to DataEngineTypeV1 is correct.Providing a default (v1) if no engine is specified helps maintain backward compatibility.
1204-1207: Ensure further data engine type validation.The explicit check for v1 or v2 is sufficient. In the future, consider a single source of truth for permissible engine types if more types are introduced.
controller/system_backup_controller.go (1)
990-993: Skipping data engine v2 backing images.A single comment clarifies the unimplemented backup logic for v2. If upcoming releases will support v2 backups, consider adding a future-proof structure or TODO next to this code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/backupbackingimage.go(3 hunks)api/model.go(1 hunks)client/generated_backing_image_restore_input.go(1 hunks)controller/system_backup_controller.go(1 hunks)manager/backupbackingimage.go(2 hunks)manager/volume.go(4 hunks)
🧰 Additional context used
📓 Learnings (1)
manager/volume.go (1)
Learnt from: derekbit
PR: longhorn/longhorn-manager#3295
File: controller/backing_image_data_source_controller.go:404-407
Timestamp: 2024-12-13T07:06:57.033Z
Learning: In the `syncBackingImage` function in `controller/backing_image_data_source_controller.go`, when adding a new entry to `bi.Spec.DiskFileSpecMap`, the `DataEngine` is set to `DataEngineTypeV1` because the backing image data source always prepares the backing image on the v1 disk.
🪛 GitHub Check: CodeFactor
manager/volume.go
[notice] 1192-1260: manager/volume.go#L1192-L1260
Complex Method
🔇 Additional comments (9)
manager/volume.go (4)
1192-1192: New function signature for restoreBackingImage looks good.
The added dataEngine parameter is consistent with the feature requirement. No immediate syntax or logic issues are spotted at the function declaration level.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 1192-1260: manager/volume.go#L1192-L1260
Complex Method
1219-1221: Logically consistent mismatch check.
Rejecting a pre-existing backing image with a different data engine prevents inconsistent states. Ensure the error message covers potential user confusion about partial restore attempts.
1242-1242: Setting the backing image data engine type.
Assigning the data engine for the newly restored backing image is correct. This aligns with the added logic earlier, ensuring the correct metadata is stored.
161-161: Verify backing image existence before restore.
Here, a restore is initiated unconditionally. Although the function internally handles empty or non-existent backing images, a lightweight existence check for spec.BackingImage beforehand could improve clarity and reduce function calls.
api/backupbackingimage.go (2)
75-77: Streamlined naming pattern for backup backing images.
Using types.GetBackupBackingImageNameFromBIName standardizes naming across the system. This also helps avoid potential collisions.
60-60: Properly forwarding the DataEngine parameter.
Including 'input.DataEngine' ensures that the new engine type is passed along for restoration. Verify that the API surfaces any erroneous or empty dataEngine input clearly to the user.
✅ Verification successful
Let me gather information about how the DataEngine parameter is handled in the codebase.
Let me check how the DataEngine parameter is validated and handled in the RestoreBackupBackingImage method.
Let me check the manager's implementation of RestoreBackupBackingImage and related validation code.
Let me check for any validation of DataEngine in the backing image and volume code, as well as the complete implementation of RestoreBackupBackingImage.
Based on the code analysis, I can now generate the final response:
DataEngine parameter is properly handled with default fallback and validation.
The implementation shows proper handling of the DataEngine parameter:
- In CreateBackingImage, empty DataEngine defaults to "v1"
- IsDataEngineV1/V2 functions validate the engine type
- RestoreBackupBackingImage forwards the parameter to restoreBackingImage for actual restoration
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the implementation of RestoreBackupBackingImage method
ast-grep --pattern 'func ($_, $_) RestoreBackupBackingImage($$$) {
$$$
}'
# Search for validation or error handling of DataEngine parameter
rg -A 5 "DataEngine.*error|error.*DataEngine"
# Look for DataEngine type definition and valid values
rg "type.*DataEngine|DataEngine.*string"
Length of output: 49825
Script:
#!/bin/bash
# Look for RestoreBackupBackingImage implementation
ast-grep --pattern 'func ($_, $_) RestoreBackupBackingImage($$$) {
$$$
}'
# Check for validation of DataEngine in backing image related code
rg -A 5 "RestoreBackupBackingImage.*DataEngine|DataEngine.*RestoreBackupBackingImage"
# Look for backing image restore input validation
rg -A 5 "BackingImageRestoreInput.*validate|validate.*BackingImageRestoreInput"
Length of output: 653
Script:
#!/bin/bash
# Look for RestoreBackupBackingImage implementation in manager
rg -A 10 "func.*RestoreBackupBackingImage" manager/
# Check for DataEngine validation in backing image related code
rg -A 5 "ValidateDataEngine|validateDataEngine"
# Look for backing image restore validation in webhook
rg -A 10 "backupbackingimage.*validate" webhook/
Length of output: 847
Script:
#!/bin/bash
# Get the complete RestoreBackupBackingImage implementation
rg -A 30 "func.*RestoreBackupBackingImage" manager/
# Check for DataEngine validation in types
rg -A 5 "IsDataEngineV1|IsDataEngineV2|ValidateDataEngine" types/
# Look for backing image creation with DataEngine
rg -A 10 "CreateBackingImage.*DataEngine" manager/
Length of output: 3940
manager/backupbackingimage.go (1)
65-65: Reuses existing method restoreBackingImage.
Passing the new dataEngine parameter is seamless and aligns well with the updated function signature in volume.go.
api/model.go (1)
298-298: DataEngine field in BackingImageRestoreInput recognized.
The new DataEngine string field extends the restore request payload, aligning with the rest of the code changes. Consider validating or enumerating allowed engine types on the API level.
client/generated_backing_image_restore_input.go (1)
10-11: LGTM! Field addition follows proper patterns.
The new DataEngine field is correctly implemented with appropriate JSON/YAML tags and follows the existing naming conventions. The omitempty tag ensures backward compatibility with existing clients.
Let's verify the generator configuration to ensure this addition is properly reflected:
✅ Verification successful
Field addition is correctly synchronized with API model
The verification shows that the DataEngine field is properly defined in both:
api/model.go: Core API model definitionclient/generated_backing_image_restore_input.go: Generated client code
The field definitions are consistent across both files with matching types and JSON tags, confirming this is a valid generated client implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the generator configuration and related schema definitions
# Look for generator configuration files
fd -t f -e yaml -e json 'schema|swagger|openapi' | while read -r file; do
echo "=== Checking $file for DataEngine field definition ==="
cat "$file"
done
# Look for related type definitions that might reference this
rg -A 5 -B 5 'type BackingImageRestoreInput struct' --type go
Length of output: 46686
| @@ -41,7 +41,7 @@ func (m *VolumeManager) DeleteBackupBackingImage(name string) error { | |||
| return m.ds.DeleteBackupBackingImage(name) | |||
| } | |||
|
|
|||
| func (m *VolumeManager) RestoreBackupBackingImage(name string, secret, secretNamespace string) error { | |||
| func (m *VolumeManager) RestoreBackupBackingImage(name string, secret, secretNamespace string, dataEngine string) error { | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Add validation for 'dataEngine' parameter
The code already has a guard for empty 'name' parameter, but lacks validation for 'dataEngine'. Looking at the codebase:
- Empty name check exists:
if name == "" { return fmt.Errorf("restore backing image name is not given") } - Other parts of the codebase validate dataEngine with messages like "invalid data engine %v"
- Similar function in manager/volume.go validates dataEngine:
if dataEngine == "" { ... }
Add validation for dataEngine parameter to fail fast:
if dataEngine == "" {
return fmt.Errorf("data engine is not specified")
}🔗 Analysis chain
Add guards around empty 'name' or 'dataEngine' parameters.
When name is empty, it fails fast. Similarly, consider quickly checking if 'dataEngine' has an invalid format rather than relying solely on the deeper restore logic.
Let me gather information about the parameter validation in the codebase by checking the implementation and related functions.
Let me check the implementation of RestoreBackupBackingImage to see how dataEngine is validated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the implementation of RestoreBackupBackingImage and its parameter validation
ast-grep --pattern 'func (m *VolumeManager) RestoreBackupBackingImage($$$) {
$$$
}'
# Look for similar functions in the same file to understand validation patterns
rg -A 5 "func.*VolumeManager.*" manager/backupbackingimage.go
# Search for validation patterns across the codebase
rg "if.*name\s*==\s*\"\""
rg "if.*dataEngine\s*==\s*\"\""
Length of output: 7490
Script:
#!/bin/bash
# Get the full implementation of RestoreBackupBackingImage
rg -A 20 "func \(m \*VolumeManager\) RestoreBackupBackingImage" manager/backupbackingimage.go
# Check for dataEngine validation patterns in the codebase
rg "dataEngine.*validation" -A 5
rg "invalid.*dataEngine"
Length of output: 7521
| "github.com/gorilla/mux" | ||
| "github.com/longhorn/longhorn-manager/types" | ||
| "github.com/pkg/errors" |
| @@ -41,7 +41,7 @@ func (m *VolumeManager) DeleteBackupBackingImage(name string) error { | |||
| return m.ds.DeleteBackupBackingImage(name) | |||
| } | |||
|
|
|||
| func (m *VolumeManager) RestoreBackupBackingImage(name string, secret, secretNamespace string) error { | |||
| func (m *VolumeManager) RestoreBackupBackingImage(name string, secret, secretNamespace string, dataEngine string) error { | |||
There was a problem hiding this comment.
| func (m *VolumeManager) RestoreBackupBackingImage(name string, secret, secretNamespace string, dataEngine string) error { | |
| func (m *VolumeManager) RestoreBackupBackingImage(name string, secret, secretNamespace, dataEngine string) error { |
|
@mergify backport v1.8.x |
derekbit
force-pushed
the
LH6341_v2_backing_image_restore_api
branch
from
ee091b3 to
c5effab
Compare
🟠 Waiting for conditions to match
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
manager/volume.go (2)
1192-1207: LGTM! Consider using a constant for the default data engine value.The parameter validation and error handling look good. However, consider using
string(longhorn.DataEngineTypeV1)directly in the default value assignment for consistency.- dataEngine = string(longhorn.DataEngineTypeV1) + dataEngine = string(longhorn.DataEngineTypeV1)🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 1192-1260: manager/volume.go#L1192-L1260
Complex Method
Line range hint
1192-1260: Consider breaking down the complex method into smaller functions.While the method is well-structured, it could benefit from being split into smaller, more focused functions to improve readability and maintainability:
validateBackingImageSecretvalidateDataEnginecheckExistingBackingImagerestoreFromBackupExample refactor for the first two functions:
func (m *VolumeManager) validateBackingImageSecret(secret, secretNamespace string) error { if secret != "" || secretNamespace != "" { if _, err := m.ds.GetSecretRO(secretNamespace, secret); err != nil { return errors.Wrapf(err, "failed to get secret %v in namespace %v", secret, secretNamespace) } } return nil } func (m *VolumeManager) validateDataEngine(dataEngine string) (string, error) { if dataEngine == "" { dataEngine = string(longhorn.DataEngineTypeV1) } if longhorn.DataEngineType(dataEngine) != longhorn.DataEngineTypeV1 && longhorn.DataEngineType(dataEngine) != longhorn.DataEngineTypeV2 { return "", fmt.Errorf("invalid data engine type %v", dataEngine) } return dataEngine, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/backupbackingimage.go(3 hunks)api/model.go(1 hunks)client/generated_backing_image_restore_input.go(1 hunks)controller/system_backup_controller.go(1 hunks)manager/backupbackingimage.go(2 hunks)manager/volume.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- client/generated_backing_image_restore_input.go
- api/model.go
- manager/backupbackingimage.go
- controller/system_backup_controller.go
- api/backupbackingimage.go
🧰 Additional context used
🪛 GitHub Check: CodeFactor
manager/volume.go
[notice] 1192-1260: manager/volume.go#L1192-L1260
Complex Method
🔇 Additional comments (2)
manager/volume.go (2)
1219-1221: LGTM! Good validation for data engine type compatibility.
The validation ensures that an existing backing image's data engine type matches the requested type, preventing potential inconsistencies.
1242-1242: LGTM! Data engine type correctly set in backing image spec.
The data engine type is properly propagated to the new backing image during restoration.
ref: longhorn 6341 Signed-off-by: Jack Lin <[email protected]>
derekbit
force-pushed
the
LH6341_v2_backing_image_restore_api
branch
from
c5effab to
97779ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
manager/volume.go (1)
Line range hint
1192-1260: Consider breaking down the complex methodThe
restoreBackingImagemethod is flagged as complex by static analysis. Consider breaking it down into smaller, focused functions:
validateBackingImageSecretvalidateAndDefaultDataEnginecheckExistingBackingImagecreateRestoreBackingImageExample refactor:
func (m *VolumeManager) restoreBackingImage(backupTargetName, biName, secret, secretNamespace, dataEngine string) error { + if err := m.validateBackingImageSecret(secret, secretNamespace); err != nil { + return err + } + + dataEngine, err := m.validateAndDefaultDataEngine(dataEngine) + if err != nil { + return err + } + + if biName == "" { + return nil + } + + if err := m.checkExistingBackingImage(biName, dataEngine); err != nil { + return err + } + + return m.createRestoreBackingImage(backupTargetName, biName, secret, secretNamespace, dataEngine) } +func (m *VolumeManager) validateBackingImageSecret(secret, secretNamespace string) error { + if secret != "" || secretNamespace != "" { + if _, err := m.ds.GetSecretRO(secretNamespace, secret); err != nil { + return errors.Wrapf(err, "failed to get secret %v in namespace %v", secret, secretNamespace) + } + } + return nil } +func (m *VolumeManager) validateAndDefaultDataEngine(dataEngine string) (string, error) { + if dataEngine == "" { + dataEngine = string(longhorn.DataEngineTypeV1) + } + if longhorn.DataEngineType(dataEngine) != longhorn.DataEngineTypeV1 && + longhorn.DataEngineType(dataEngine) != longhorn.DataEngineTypeV2 { + return "", fmt.Errorf("invalid data engine type %v", dataEngine) + } + return dataEngine, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/backupbackingimage.go(3 hunks)api/model.go(1 hunks)client/generated_backing_image_restore_input.go(1 hunks)controller/system_backup_controller.go(1 hunks)manager/backupbackingimage.go(2 hunks)manager/volume.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- client/generated_backing_image_restore_input.go
- controller/system_backup_controller.go
- api/model.go
- manager/backupbackingimage.go
- api/backupbackingimage.go
🧰 Additional context used
📓 Learnings (1)
manager/volume.go (1)
Learnt from: derekbit
PR: longhorn/longhorn-manager#3295
File: controller/backing_image_data_source_controller.go:404-407
Timestamp: 2024-12-13T07:06:57.033Z
Learning: In the `syncBackingImage` function in `controller/backing_image_data_source_controller.go`, when adding a new entry to `bi.Spec.DiskFileSpecMap`, the `DataEngine` is set to `DataEngineTypeV1` because the backing image data source always prepares the backing image on the v1 disk.
🪛 GitHub Check: CodeFactor
manager/volume.go
[notice] 1192-1260: manager/volume.go#L1192-L1260
Complex Method
🔇 Additional comments (3)
manager/volume.go (3)
161-161: LGTM: Data engine parameter added correctly
The addition of the data engine parameter to the restoreBackingImage call is consistent with the PR objectives and properly converts the DataEngine type to string.
1200-1206: LGTM: Proper data engine validation with default fallback
The code correctly:
- Defaults to DataEngineTypeV1 when no engine is specified
- Validates that only V1 or V2 engines are allowed
- Provides clear error messaging
1219-1221: LGTM: Proper data engine compatibility check
The code correctly prevents restoring a backing image when there's a data engine type mismatch with an existing backing image.
| @@ -71,8 +72,9 @@ func (s *Server) BackupBackingImageCreate(w http.ResponseWriter, req *http.Reque | |||
| } | |||
|
|
|||
| backingImageName := mux.Vars(req)["name"] | |||
| if err := s.m.CreateBackupBackingImage(input.Name, backingImageName, input.BackupTargetName); err != nil { | |||
| return errors.Wrapf(err, "failed to create backup backing image '%s'", input.Name) | |||
| backupBackingImageName := types.GetBackupBackingImageNameFromBIName(backingImageName) | |||
There was a problem hiding this comment.
- Can we check if
input.Nameis equal tobackingImageNameor an empty string, and then generate a random UUID name? - Or we won't allow users to customize the backup backing image name from the API when creating it.
ref:
Fix:
DataEngineoption in restoring backup backing imagecc @derekbit @mantissahz @houhoucoop