feat: support v2 backing image download file from sync service #442
Conversation
WalkthroughThe changes in this pull request enhance the file download functionality in the application. The HTTP route for downloading files at Changes
Poem
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
CodeRabbit Configuration File (
|
ChanYiLin
force-pushed
the
LH6341_v2_backing_image
branch
from
9b45cd0
to
abb24df
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: 3
🧹 Outside diff range and nitpick comments (2)
pkg/sync/router.go (1)
20-20: Consider API versioning strategy for v2 features.The endpoint
/v1/files/{id}/downloadis being enhanced with v2 functionality, but remains under the/v1/path. Consider if this v2 backing image feature should be exposed through a/v2/endpoint instead, maintaining clear API versioning.Options to consider:
- Create a new
/v2/files/{id}/downloadendpoint specifically for v2 backing images- Use query parameters to distinguish v2 functionality (current approach)
- Use content negotiation through Accept headers
The choice depends on your API versioning strategy and backward compatibility requirements.
pkg/sync/service.go (1)
224-225: Parse 'forV2Creation' query parameter as a booleanCurrently,
'forV2Creation'is retrieved as a string and compared to"true". To improve code clarity and handle different representations of true values, consider parsing it as a boolean usingstrconv.ParseBool.Here's how you might adjust the code:
queryParams := request.URL.Query() - forV2Creation := queryParams.Get("forV2Creation") + forV2CreationStr := queryParams.Get("forV2Creation") + forV2Creation, err := strconv.ParseBool(forV2CreationStr) + if err != nil { + err = fmt.Errorf("invalid forV2Creation parameter: %v", err) + return + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/sync/router.go(1 hunks)pkg/sync/service.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/sync/service.go
256-256: ineffectual assignment to err
(ineffassign)
263-263: ineffectual assignment to err
(ineffassign)
276-276: ineffectual assignment to err
(ineffassign)
281-281: SA5001: should check error returned from os.Open() before deferring src.Close()
(staticcheck)
🔇 Additional comments (1)
pkg/sync/router.go (1)
20-20: LGTM! Adding HEAD method support is a good practice.
The addition of the HEAD method alongside GET for the download endpoint follows HTTP standards and allows clients to efficiently check file metadata without downloading the content. This is particularly useful for large backing images where checking existence or metadata first can save bandwidth.
Let's verify the consistency of download-related endpoints:
ChanYiLin
force-pushed
the
LH6341_v2_backing_image
branch
from
abb24df
to
8b65294
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (8)
pkg/types/types.go (1)
72-74: LGTM! Consider adding documentation comments.The new constants are well-placed and follow the existing naming conventions. Consider adding documentation comments to explain their purpose and usage.
+// DataSourceTypeParameterDataEngine is used to specify the data engine version for operations DataSourceTypeParameterDataEngine = "data-engine" +// DataEnginev1 represents the v1 data engine DataEnginev1 = "v1" +// DataEnginev2 represents the v2 data engine DataEnginev2 = "v2"pkg/client/sync_client.go (1)
Line range hint
267-294: Fix incorrect error message.The error message doesn't match the operation being performed.
Apply this fix:
- return fmt.Errorf("download from URL failed, err: %s", err) + return fmt.Errorf("restore from backup URL failed, err: %s", err)pkg/datasource/service.go (1)
Line range hint
203-351: Consider enhancing error handling for v2 data engine operationsWhile the dataEngine parameter integration is solid, consider adding comprehensive error handling specific to v2 data engine operations, especially for QCOW2 format handling and conversion failures.
Consider:
- Adding specific error types for v2 data engine operations
- Implementing detailed logging for QCOW2 conversion steps
- Adding metrics for tracking conversion success/failure rates
pkg/manager/service.go (1)
343-343: Consider RPC protocol updates for v2 backing imagesThe current change adds the data engine parameter, but the RPC protocol (
rpc.SyncRequest) might need updates to support v2 backing image functionality.Consider:
- Adding a field to
SyncRequestto indicate v2 backing images- Including qcow2-specific metadata in the request
- Updating the sync service to handle the new fields
This would require changes to the protocol definition and potentially other components in the system.
pkg/sync/server_test.go (2)
106-106: Consider extracting data engine constantThe
types.DataEnginev1value is repeated across multiple test cases. Consider extracting it to a test-level constant for better maintainability.const ( TestSyncingFileUUID = "test-sf-uuid" TestDiskUUID = "test-disk-uuid" + testDataEngine = types.DataEnginev1 )Also applies to: 221-221, 317-317, 353-353, 525-525, 533-533, 537-537, 539-539, 611-611
525-541: Enhance test case descriptions in TestDuplicateCallsThe test verifies duplicate call handling but could benefit from clearer documentation of the expected behavior for each scenario.
Add comments to clarify the test scenarios:
func (s *SyncTestSuite) TestDuplicateCalls(c *C) { + // Test cases: + // 1. Initial successful download + // 2. Duplicate download attempt (should fail) + // 3. Duplicate upload attempt (should fail) + // 4. Duplicate receive attempt (should fail) + // 5. Download attempt with different path but same UUID (should fail) + // 6. Multiple delete/forget calls (should succeed) logrus.Debugf("Testing sync server: TestDuplicateCalls")pkg/sync/service.go (1)
223-225: Add Comments to Explain theforV2CreationParameterThe introduction of the
forV2Creationquery parameter modifies the download logic but lacks inline documentation. Adding comments will improve code readability and help future maintainers understand its purpose and impact on the download process.pkg/sync/sync_file.go (1)
Line range hint
871-967: Consider refactoringfinishProcessingto reduce complexityThe
finishProcessingmethod spans nearly 100 lines and contains multiple nested conditional blocks, making it difficult to read and maintain. Refactoring this method into smaller, well-defined helper functions can improve readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
pkg/client/sync_client.go(8 hunks)pkg/datasource/service.go(6 hunks)pkg/manager/service.go(1 hunks)pkg/sync/router.go(1 hunks)pkg/sync/server_test.go(6 hunks)pkg/sync/service.go(13 hunks)pkg/sync/sync_file.go(13 hunks)pkg/types/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/sync/router.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/sync/service.go
251-251: ineffectual assignment to err
(ineffassign)
255-255: ineffectual assignment to err
(ineffassign)
266-266: ineffectual assignment to err
(ineffassign)
256-256: SA5001: should check error returned from os.Open() before deferring src.Close()
(staticcheck)
🪛 GitHub Check: CodeFactor
pkg/sync/sync_file.go
[notice] 871-967: pkg/sync/sync_file.go#L871-L967
Complex Method
🔇 Additional comments (19)
pkg/types/types.go (2)
72-74: Verify the usage of new data engine constants.
Let's ensure these constants are properly utilized across the codebase, particularly in the sync service and data source handling.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
The data engine constants are properly integrated across the codebase
The verification shows that:
- The constants are correctly used in sync service (
pkg/sync/sync_file.go,pkg/sync/service.go) - Data source handling properly implements fallback to
DataEnginev1when not specified (pkg/datasource/service.go) - Test coverage exists for both data engine versions (
pkg/sync/server_test.go) - The qcow2 format detection is properly integrated with data engine v2 handling (
pkg/sync/sync_file.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of data engine constants in the codebase
# Check for direct usage of the constants
echo "Checking direct usage of data engine constants..."
rg -A 3 "DataSourceTypeParameterDataEngine|DataEnginev1|DataEnginev2"
# Check for hardcoded values that should use these constants
echo "Checking for hardcoded values that should use the constants..."
rg -A 3 '"data-engine"|"v1"|"v2"'
# Check for related qcow2 handling as mentioned in PR objectives
echo "Checking qcow2 related code that might need data engine parameter..."
rg -A 5 "qcow2.*download|download.*qcow2"
Length of output: 10891
72-74: Verify integration between data engine and file type handling.
Since the PR objectives mention qcow2 to raw conversion for v2 backing images, ensure proper integration between the new data engine constants and existing file type constants (SyncingFileTypeRaw, SyncingFileTypeQcow2).
✅ Verification successful
Let me gather more information to verify the integration between data engine versions and file type handling.
Based on the search results, I can see that there is proper integration between data engine versions and file type handling. The code shows:
- Data engine v2 specific handling in
pkg/sync/sync_file.go:
if dataEngine == types.DataEnginev2 {
imgInfo, qemuErr := util.GetQemuImgInfo(sf.tmpFilePath)
...
}- File type conversion utilities in
pkg/util/util.go:
func ConvertFromRawToQcow2(filePath string)
func ConvertFromQcow2ToRaw(sourcePath, targetPath string)- File type validation in
pkg/sync/service.go:
if fileType != types.SyncingFileTypeRaw && fileType != types.SyncingFileTypeQcow2 {
return fmt.Errorf("")
}Integration between data engine and file types is properly implemented.
- Data engine v2 checks file format using
GetQemuImgInfo - Conversion utilities exist for both raw-to-qcow2 and qcow2-to-raw transformations
- File type validation ensures only supported formats are used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the relationship between data engine and file type handling
# Check for code that handles both data engine and file type together
echo "Checking for data engine and file type handling..."
ast-grep --pattern 'if $_ == types.DataEnginev2 {
$$$
$file_type
$$$
}'
# Check for conversion logic between qcow2 and raw formats
echo "Checking for format conversion logic..."
rg -A 5 "SyncingFileTypeQcow2.*SyncingFileTypeRaw|SyncingFileTypeRaw.*SyncingFileTypeQcow2"
Length of output: 699
Script:
#!/bin/bash
# Search for usage of DataEnginev2 constant
echo "Checking DataEnginev2 usage..."
rg -A 5 "DataEnginev2"
# Search for file type handling in sync-related code
echo "Checking file type handling in sync operations..."
rg -A 5 "fileType.*SyncingFileType|SyncingFileType.*fileType"
# Search for any conversion or transformation logic
echo "Checking for conversion logic..."
rg -A 5 "Convert|Transform|Format.*Change"
Length of output: 110315
pkg/client/sync_client.go (3)
Line range hint 224-246: LGTM! Clean implementation.
The dataEngine parameter is correctly added while maintaining existing security practices for credential handling.
Line range hint 369-386: LGTM! Consistent implementation.
The dataEngine parameter is properly integrated while maintaining the existing robust error handling patterns.
Line range hint 189-204: LGTM! Verify dataEngine parameter usage.
The addition of the dataEngine parameter is well-implemented and follows consistent patterns. The parameter is correctly propagated to the query parameters.
Let's verify the dataEngine parameter usage across the codebase:
✅ Verification successful
All DownloadFromURL calls consistently include the dataEngine parameter
The verification shows that all calls to DownloadFromURL across the codebase properly include the dataEngine parameter:
- In
pkg/datasource/service.go: Correctly passes dataEngine parameter - In
pkg/sync/server_test.go: All test cases usetypes.DataEnginev1 - The implementation in
pkg/sync/sync_file.goand related handlers properly propagate the parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent dataEngine parameter usage
# Expected: All calls to DownloadFromURL should include dataEngine parameter
# Search for DownloadFromURL method calls
rg -A 2 "DownloadFromURL\("
Length of output: 3498
pkg/datasource/service.go (5)
203-207: LGTM: Proper handling of dataEngine parameter
The implementation correctly retrieves and defaults the dataEngine parameter, maintaining consistency with the existing parameter handling pattern.
Also applies to: 222-222
235-239: LGTM: Consistent implementation of dataEngine parameter
The implementation maintains consistency with the established pattern for parameter handling.
Also applies to: 240-240
279-283: LGTM: Proper integration with sync client
The dataEngine parameter is correctly handled and properly integrated with the sync client's Receive method.
Also applies to: 305-305
346-351: LGTM: Proper query parameter handling
The dataEngine parameter is correctly added to the URL query parameters for the sync server request.
248-251: Consider adding QCOW2 format validation
While the dataEngine parameter handling is correct, given the PR objectives mentioning QCOW2 image handling, consider adding validation for image format when dataEngine is v2.
Consider adding format validation:
if dataEngine == "" {
dataEngine = types.DataEnginev1
}
+ // For v2 data engine, validate QCOW2 format if specified
+ if dataEngine == types.DataEnginev2 {
+ if format, exists := parameters[types.DataSourceTypeFileType]; exists && format == "qcow2" {
+ s.log.Infof("QCOW2 format detected for v2 data engine")
+ }
+ }Also applies to: 253-253
pkg/manager/service.go (1)
Line range hint 343-347: Potential port resource leak in error handling
If syncClient.Receive fails, we should ensure the port is released before returning the error. Currently, the port release depends on the goroutine receiving the channel message.
Consider restructuring the error handling:
- if err := m.syncClient.Receive(biFilePath, req.Spec.Uuid, m.diskUUID, req.Spec.Checksum, "", int(port), req.Spec.Size, types.DataEnginev1); err != nil {
- portReleaseChannel <- nil
- return nil, err
- }
+ if err := m.syncClient.Receive(biFilePath, req.Spec.Uuid, m.diskUUID, req.Spec.Checksum, "", int(port), req.Spec.Size, types.DataEnginev1); err != nil {
+ if releaseErr := m.releasePorts(port, port+1); releaseErr != nil {
+ log.WithError(releaseErr).Error("Failed to release ports during error handling")
+ }
+ return nil, err
+ }pkg/sync/service.go (8)
467-467: Validate dataEngine Parameter in Cloning
In doCloneFromBackingImage, the dataEngine parameter is retrieved but not validated. Ensure that the dataEngine value is checked against expected values and handle any invalid inputs to prevent unexpected behavior.
539-540: Ensure Proper Handling of dataEngine in Restoration
The dataEngine parameter is used in the restore process without validation. Validate this parameter and handle any invalid or unexpected values to avoid runtime errors.
597-598: Validate dataEngine Parameter in Upload
In doUploadFromRequest, the dataEngine parameter is extracted but not validated. Ensure that you validate dataEngine against expected values and handle cases where it may be missing or invalid.
682-682: Validate dataEngine Parameter in Peer Reception
In doReceiveFromPeer, the dataEngine parameter is obtained from the query parameters but not validated. Ensure that it is checked against expected values and handle any invalid inputs appropriately.
247-269:
Proper Error Handling and Resource Management
Several issues need attention in this block:
-
Ineffectual Assignments to
err:- At lines 251 and 266,
erris assigned but not used effectively because the function returns immediately after. There's no need to reassignerrin these cases.
- At lines 251 and 266,
-
Unchecked Errors Before Using Resources:
- At line 255,
errreturned fromos.Open(sf.filePath)is not checked before deferringsrc.Close(). This can lead to a panic ifsrcisnil.
- At line 255,
-
Deferring Resource Closure Without Checking Method:
- The
defer src.Close()should only be called if the method isGET, as inHEADrequests,srcmight not be utilized.
- The
Apply the following fixes:
// Open the file and check for errors
src, err := os.Open(sf.filePath)
if err != nil {
err = errors.Wrapf(err, "failed to open the download file %v", sf.filePath)
return
}
defer src.Close()
// Handle HEAD method separately
if request.Method == http.MethodHead {
return
}
// Copy data from src to the response writer
if _, ioErr := io.Copy(writer, src); ioErr != nil {
err = ioErr
return
}🧰 Tools
🪛 golangci-lint (1.62.2)
251-251: ineffectual assignment to err
(ineffassign)
255-255: ineffectual assignment to err
(ineffassign)
266-266: ineffectual assignment to err
(ineffassign)
256-256: SA5001: should check error returned from os.Open() before deferring src.Close()
(staticcheck)
255-257:
Check Error Before Deferring src.Close()
Immediately after opening the file with os.Open(sf.filePath), the error err should be checked before deferring src.Close(). If os.Open fails, src will be nil, and calling src.Close() would cause a panic.
Apply this fix:
src, err := os.Open(sf.filePath)
if err != nil {
err = errors.Wrapf(err, "failed to open the download file %v", sf.filePath)
return
}
defer src.Close()🧰 Tools
🪛 golangci-lint (1.62.2)
255-255: ineffectual assignment to err
(ineffassign)
256-256: SA5001: should check error returned from os.Open() before deferring src.Close()
(staticcheck)
266-267:
Handle the Error Returned by io.Copy
At line 266, the error from io.Copy(writer, src) is assigned to err but not acted upon. Since the function is about to return, the error should be handled appropriately.
Modify the code as follows:
if _, ioErr := io.Copy(writer, src); ioErr != nil {
err = ioErr
return
}🧰 Tools
🪛 golangci-lint (1.62.2)
266-266: ineffectual assignment to err
(ineffassign)
403-403: Validate and Handle the dataEngine Parameter
The dataEngine parameter is extracted from the query without validation. Ensure that dataEngine is validated against expected values and handle cases where it might be missing or invalid.
Verification Script:
| @@ -340,7 +340,7 @@ func (m *Manager) Sync(ctx context.Context, req *rpc.SyncRequest) (resp *rpc.Bac | |||
| }() | |||
|
|
|||
| biFilePath := types.GetBackingImageFilePath(m.diskPath, req.Spec.Name, req.Spec.Uuid) | |||
| if err := m.syncClient.Receive(biFilePath, req.Spec.Uuid, m.diskUUID, req.Spec.Checksum, "", int(port), req.Spec.Size); err != nil { | |||
| if err := m.syncClient.Receive(biFilePath, req.Spec.Uuid, m.diskUUID, req.Spec.Checksum, "", int(port), req.Spec.Size, types.DataEnginev1); err != nil { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add UUID validation for security
The req.Spec.Uuid is used in file path construction without validation. This could potentially lead to path traversal if the UUID contains special characters.
Add UUID validation before using it in file paths:
+ if !isValidUUID(req.Spec.Uuid) {
+ return nil, status.Errorf(codes.InvalidArgument, "invalid UUID format")
+ }
if err := m.syncClient.Receive(biFilePath, req.Spec.Uuid, m.diskUUID, req.Spec.Checksum, "", int(port), req.Spec.Size, types.DataEnginev1); err != nil {Consider adding a helper function:
func isValidUUID(uuid string) bool {
r := regexp.MustCompile("^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-4[a-fA-F0-9]{3}-[8|9|aA|bB][a-fA-F0-9]{3}-[a-fA-F0-9]{12}$")
return r.MatchString(uuid)
}Implementation doesn't fully support v2 backing images
The current implementation hardcodes types.DataEnginev1, but according to the PR objectives, we need to support v2 backing images and handle qcow2 transformation. Consider:
- Adding logic to determine the appropriate data engine version
- Implementing the qcow2 to raw format transformation for v2 backing images
Here's a suggested approach:
- if err := m.syncClient.Receive(biFilePath, req.Spec.Uuid, m.diskUUID, req.Spec.Checksum, "", int(port), req.Spec.Size, types.DataEnginev1); err != nil {
+ dataEngine := types.DataEnginev1
+ if req.Spec.IsV2BackingImage {
+ dataEngine = types.DataEnginev2
+ }
+ if err := m.syncClient.Receive(biFilePath, req.Spec.Uuid, m.diskUUID, req.Spec.Checksum, "", int(port), req.Spec.Size, dataEngine); err != nil {Committable suggestion skipped: line range outside the PR's diff.
| @@ -379,7 +415,7 @@ func (s *Service) doDownloadFromURL(request *http.Request) (err error) { | |||
| return | |||
| } | |||
|
|
|||
| if _, err := sf.DownloadFromURL(url); err != nil { | |||
| if _, err := sf.DownloadFromURL(url, dataEngine); err != nil { | |||
There was a problem hiding this comment.
Update sf State on Download Failure
When sf.DownloadFromURL(url, dataEngine) returns an error, the error is logged but sf's state may not be updated to reflect the failure. Ensure that the state of sf is appropriately set to Failed to prevent any inconsistent state.
| @@ -446,7 +483,7 @@ | |||
| return | |||
| } | |||
|
|
|||
| if _, err := sf.CloneToFileWithEncryption(sourceBackingImage, sourceBackingImageUUID, encryption, credential); err != nil { | |||
| if _, err := sf.CloneToFileWithEncryption(sourceBackingImage, sourceBackingImageUUID, encryption, credential, dataEngine); err != nil { | |||
There was a problem hiding this comment.
Properly Update sf State on Clone Failure
If an error occurs during sf.CloneToFileWithEncryption(...), the error is only logged. Ensure that sf's state is updated to reflect the failure, which is crucial for the synchronization process.
| @@ -510,7 +549,7 @@ | |||
| return | |||
| } | |||
|
|
|||
| if err := sf.RestoreFromBackupURL(backupURL, credential, concurrentLimit); err != nil { | |||
| if err := sf.RestoreFromBackupURL(backupURL, credential, concurrentLimit, dataEngine); err != nil { | |||
There was a problem hiding this comment.
Update sf State on Restore Failure
When sf.RestoreFromBackupURL(...) fails, make sure to update sf's state to Failed. This ensures that any dependent processes are aware of the failure and can handle it accordingly.
| @@ -589,7 +630,7 @@ | |||
| return err | |||
| } | |||
|
|
|||
| if _, err := sf.IdleTimeoutCopyToFile(p); err != nil { | |||
| if _, err := sf.IdleTimeoutCopyToFile(p, dataEngine); err != nil { | |||
There was a problem hiding this comment.
Properly Handle Errors During File Upload
When calling sf.IdleTimeoutCopyToFile(p, dataEngine), any errors returned should result in the sf state being updated to Failed. This helps maintain consistency and allows for proper error handling upstream.
| @@ -652,7 +694,7 @@ | |||
| return | |||
| } | |||
|
|
|||
| if err := sf.Receive(int(port), fileType); err != nil { | |||
| if err := sf.Receive(int(port), fileType, dataEngine); err != nil { | |||
There was a problem hiding this comment.
Update sf State on Receive Failure
If an error occurs during sf.Receive(int(port), fileType, dataEngine), ensure that the sf state is set to Failed. This is important for error propagation and system stability.
| sf.lock.RLock() | ||
| if sf.state != types.StateReady { | ||
| sf.lock.RUnlock() | ||
| err = fmt.Errorf("cannot get the reader for a non-ready file, current state %v", sf.state) | ||
| return | ||
| } | ||
| sf.lock.RUnlock() | ||
|
|
There was a problem hiding this comment.
Ensure Thread-Safe Access to sf.state
The read lock on sf.lock is released before checking the state and returning an error if the state is not types.StateReady. This could lead to a race condition if another goroutine modifies sf.state between releasing the lock and the state check.
Consider the following refactor:
sf.lock.RLock()
defer sf.lock.RUnlock()
if sf.state != types.StateReady {
err = fmt.Errorf("cannot get the reader for a non-ready file, current state %v", sf.state)
return
}By deferring the unlock, you ensure that the state remains consistent during the check.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sf.lock.RLock() | |
| if sf.state != types.StateReady { | |
| sf.lock.RUnlock() | |
| err = fmt.Errorf("cannot get the reader for a non-ready file, current state %v", sf.state) | |
| return | |
| } | |
| sf.lock.RUnlock() | |
| sf.lock.RLock() | |
| defer sf.lock.RUnlock() | |
| if sf.state != types.StateReady { | |
| err = fmt.Errorf("cannot get the reader for a non-ready file, current state %v", sf.state) | |
| return | |
| } | |
| if convertErr := util.ConvertFromQcow2ToRaw(sf.tmpFilePath, tmpRawFile); convertErr != nil { | ||
| finalErr = errors.Wrapf(convertErr, "failed to create raw image from qcow2 image %v", sf.tmpFilePath) | ||
| return | ||
| } | ||
| os.RemoveAll(sf.tmpFilePath) | ||
| if renameErr := os.Rename(tmpRawFile, sf.tmpFilePath); renameErr != nil { | ||
| finalErr = errors.Wrapf(renameErr, "failed to rename tmp raw file %v to file %v", tmpRawFile, sf.tmpFilePath) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential errors from os.RemoveAll
In the conversion process from qcow2 to raw format, the call to os.RemoveAll(sf.tmpFilePath) does not check for errors. If the removal fails, it may lead to data inconsistencies or unexpected behavior when renaming the converted file. Consider handling the error returned by os.RemoveAll to ensure file operations are successful.
Apply this diff to handle the error:
if imgInfo.Format == "qcow2" {
if convertErr := util.ConvertFromQcow2ToRaw(sf.tmpFilePath, tmpRawFile); convertErr != nil {
finalErr = errors.Wrapf(convertErr, "failed to create raw image from qcow2 image %v", sf.tmpFilePath)
return
}
- os.RemoveAll(sf.tmpFilePath)
+ if removeErr := os.RemoveAll(sf.tmpFilePath); removeErr != nil {
+ finalErr = errors.Wrapf(removeErr, "failed to remove tmp file %v", sf.tmpFilePath)
+ return
+ }
if renameErr := os.Rename(tmpRawFile, sf.tmpFilePath); renameErr != nil {
finalErr = errors.Wrapf(renameErr, "failed to rename tmp raw file %v to file %v", tmpRawFile, sf.tmpFilePath)
return
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if convertErr := util.ConvertFromQcow2ToRaw(sf.tmpFilePath, tmpRawFile); convertErr != nil { | |
| finalErr = errors.Wrapf(convertErr, "failed to create raw image from qcow2 image %v", sf.tmpFilePath) | |
| return | |
| } | |
| os.RemoveAll(sf.tmpFilePath) | |
| if renameErr := os.Rename(tmpRawFile, sf.tmpFilePath); renameErr != nil { | |
| finalErr = errors.Wrapf(renameErr, "failed to rename tmp raw file %v to file %v", tmpRawFile, sf.tmpFilePath) | |
| return | |
| } | |
| } | |
| if convertErr := util.ConvertFromQcow2ToRaw(sf.tmpFilePath, tmpRawFile); convertErr != nil { | |
| finalErr = errors.Wrapf(convertErr, "failed to create raw image from qcow2 image %v", sf.tmpFilePath) | |
| return | |
| } | |
| if removeErr := os.RemoveAll(sf.tmpFilePath); removeErr != nil { | |
| finalErr = errors.Wrapf(removeErr, "failed to remove tmp file %v", sf.tmpFilePath) | |
| return | |
| } | |
| if renameErr := os.Rename(tmpRawFile, sf.tmpFilePath); renameErr != nil { | |
| finalErr = errors.Wrapf(renameErr, "failed to rename tmp raw file %v to file %v", tmpRawFile, sf.tmpFilePath) | |
| return | |
| } | |
| } |
ref: longhorn/longhorn 6341 Signed-off-by: Jack Lin <[email protected]>
ChanYiLin
force-pushed
the
LH6341_v2_backing_image
branch
from
8b65294
to
f54b860
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
pkg/datasource/service.go (1)
Line range hint
203-351: Consider documenting dataEngine parameter usage.The implementation of the dataEngine parameter is consistent across all methods, but consider adding documentation to explain:
- The purpose and impact of the dataEngine parameter
- The significance of the default value
DataEnginev1- How it relates to the qcow2 image handling requirements mentioned in the PR objectives
This will help future maintainers understand the design decisions and requirements.
pkg/sync/server_test.go (1)
Line range hint
525-611: Enhance TestDownloadToDst to verify v2 backing image requirementsThe PR objectives mention that v2 backing images require qcow2 to raw format conversion. The current test only verifies qcow2 format preservation. Consider adding test cases to verify:
- Correct handling of qcow2 to raw conversion for v2 backing images
- Checksum and size calculations after format conversion
- Error cases when conversion fails
Consider extending the test with v2 scenarios:
func (s *SyncTestSuite) TestDownloadToDst(c *C) { + // Test cases for v2 backing image + testCases := []struct { + name string + dataEngine string + wantFormat string + }{ + {"v1-preserves-qcow2", types.DataEnginev1, "qcow2"}, + {"v2-converts-to-raw", types.DataEnginev2, "raw"}, + } + + for _, tc := range testCases { + c.Run(tc.name, func() { + downloadFilePath := filepath.Join(s.dir, "sync-download-dst-"+tc.name) + err = cli.DownloadToDst(curPath, downloadFilePath, tc.dataEngine) + c.Assert(err, IsNil) + + fileInfo, err := util.GetQemuImgInfo(downloadFilePath) + c.Assert(err, IsNil) + c.Assert(fileInfo.Format, Equals, tc.wantFormat) + }) + } }pkg/sync/sync_file.go (1)
Line range hint
871-967: RefactorfinishProcessingmethod to reduce complexityThe
finishProcessingmethod spans over 90 lines and includes multiple nested conditional statements and error handling blocks, making it difficult to read and maintain. Consider refactoring this method into smaller, more focused helper functions to improve readability and reduce complexity.Possible refactoring steps:
- Extract the QCOW2 to raw conversion logic: Move the code handling the conversion from QCOW2 to raw format into a separate method, for example,
convertQcow2ToRaw().- Separate checksum validation: Isolate the checksum calculation and validation into its own function to simplify the main method flow.
- Modularize error handling: Create helper functions for repetitive error handling patterns to reduce nested structures.
This refactoring will enhance maintainability and make future modifications easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
pkg/client/sync_client.go(8 hunks)pkg/datasource/service.go(6 hunks)pkg/manager/service.go(1 hunks)pkg/sync/router.go(1 hunks)pkg/sync/server_test.go(6 hunks)pkg/sync/service.go(13 hunks)pkg/sync/sync_file.go(13 hunks)pkg/types/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/client/sync_client.go
- pkg/manager/service.go
- pkg/sync/router.go
- pkg/types/types.go
🧰 Additional context used
🪛 GitHub Check: CodeFactor
pkg/sync/sync_file.go
[notice] 871-967: pkg/sync/sync_file.go#L871-L967
Complex Method
🔇 Additional comments (12)
pkg/datasource/service.go (5)
235-240: LGTM! Parameter handling is consistent.
The dataEngine parameter handling follows the established pattern and is properly integrated with the existing parameter validation logic.
279-283: LGTM! Parameter handling is well-integrated.
The dataEngine parameter is properly extracted and passed to the sync client's Receive method, maintaining consistency with the overall implementation pattern.
Also applies to: 305-305
346-351: LGTM! RESTful parameter handling is correct.
The dataEngine parameter is properly added to the URL query parameters, following RESTful practices and maintaining consistency with the implementation pattern.
248-253: Verify qcow2 image handling requirements.
While the dataEngine parameter is properly added, the PR objectives mention requirements for handling qcow2 images and transforming them to raw format. Please verify if additional logic is needed in this method or if this is handled by the sync client.
Let's check if qcow2 handling is implemented in the sync client:
✅ Verification successful
Let me gather more specific information about the sync client's DownloadFromURL implementation:
Based on the search results, I can now generate the final response:
The qcow2 image handling is properly implemented in the sync client layer
The sync client already handles qcow2 image processing appropriately:
- The sync package contains comprehensive qcow2 handling in
sync_file.go - It automatically detects qcow2 format and performs necessary conversions
- For v2 data engine, it transforms qcow2 to raw format as required
- The conversion logic is implemented in the
finishProcessingmethod which is called after download
No additional logic is needed in the downloadFromURL method as the qcow2 handling is correctly encapsulated in the sync client layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for qcow2 image handling implementation
# Look for qcow2 related code in the codebase
rg -i "qcow2|image.*format|format.*conversion" --type go
Length of output: 6347
Script:
#!/bin/bash
# Check the implementation of DownloadFromURL in sync client
ast-grep --pattern 'func ($receiver) DownloadFromURL($$$) {
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Let's try with ripgrep to find DownloadFromURL implementation
rg -A 20 "func.*DownloadFromURL" --type go
Length of output: 5735
203-207: LGTM! Verify dataEngine parameter handling in sync client.
The addition of the dataEngine parameter with a default fallback to DataEnginev1 is implemented correctly. The parameter is properly passed to the sync client.
Let's verify the parameter handling in the sync client:
Also applies to: 222-222
✅ Verification successful
Parameter handling for dataEngine is properly implemented across the codebase
The verification shows that:
- The
dataEngineparameter is correctly passed through the sync client'sCloneFromBackingImagemethod - It's properly added to the request URL query parameters as
data-engine - The constants
DataEnginev1andDataEnginev2are defined in the types package - The parameter handling is consistent across different operations (clone, download, restore, receive)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dataEngine parameter handling in sync client
# Check if CloneFromBackingImage method in sync client properly handles the dataEngine parameter
ast-grep --pattern 'func $_($$) {
$$$
CloneFromBackingImage($$$, dataEngine)
$$$
}'
Length of output: 91
Script:
#!/bin/bash
# Let's search for CloneFromBackingImage method definition and its usages
rg "CloneFromBackingImage" -A 5
Length of output: 2242
Script:
#!/bin/bash
# Let's also check the interface or type definition that includes this method
ast-grep --pattern 'type $_ interface {
$$$
CloneFromBackingImage($$$)
$$$
}'
Length of output: 85
Script:
#!/bin/bash
# Search for any struct implementations of this method
ast-grep --pattern 'func ($_ $_) CloneFromBackingImage($$$) {
$$$
}'
Length of output: 473
Script:
#!/bin/bash
# Let's check how dataEngine parameter is handled in the sync client's implementation
rg -A 10 "CloneFromBackingImage.*dataEngine" pkg/client/sync_client.go
Length of output: 589
Script:
#!/bin/bash
# Let's also check the request construction to see how dataEngine is passed
rg -B 5 -A 15 "doCloneFromBackingImage" pkg/sync/service.go
Length of output: 992
Script:
#!/bin/bash
# Let's check how query parameters are constructed in the sync client
rg -A 10 'req.URL.Query\(\)' pkg/client/sync_client.go
Length of output: 2618
Script:
#!/bin/bash
# Let's also check the types package for any dataEngine related constants
rg "DataEngine" pkg/types/
Length of output: 254
pkg/sync/service.go (6)
239-246:
Ensure Thread-Safe Access to sf.state
The read lock on sf.lock is released before checking the state, which may lead to a race condition if another goroutine modifies sf.state between releasing the lock and the state check.
Consider deferring the unlock to ensure consistent state during the check:
sf.lock.RLock()
defer sf.lock.RUnlock()
if sf.state != types.StateReady {
err = fmt.Errorf("cannot get the reader for a non-ready file, current state %v", sf.state)
return
}423-423:
Update sf State on Download Failure
When sf.DownloadFromURL(url, dataEngine) returns an error, it's logged but sf's state isn't updated to reflect the failure. This can lead to inconsistent state management.
Ensure that sf.state is set to types.StateFailed upon error:
if _, err := sf.DownloadFromURL(url, dataEngine); err != nil {
s.log.Errorf("Sync Service: failed to download sync file %v: %v", filePath, err)
+ sf.SetState(types.StateFailed)
return
}491-491:
Properly Update sf State on Clone Failure
If an error occurs during sf.CloneToFileWithEncryption(...), the state of sf should be updated to reflect the failure to maintain consistency.
Set sf.state to types.StateFailed when an error is encountered:
if _, err := sf.CloneToFileWithEncryption(sourceBackingImage, sourceBackingImageUUID, encryption, credential, dataEngine); err != nil {
s.log.Errorf("Sync Service: failed to clone sync file %v: %v", filePath, err)
+ sf.SetState(types.StateFailed)
return
}557-557:
Update sf State on Restore Failure
When sf.RestoreFromBackupURL(...) fails, the state of sf should be set to types.StateFailed to accurately represent the file's status.
Modify the error handling to update the state:
if err := sf.RestoreFromBackupURL(backupURL, credential, concurrentLimit, dataEngine); err != nil {
s.log.Errorf("Sync Service: failed to download sync file %v: %v", filePath, err)
+ sf.SetState(types.StateFailed)
return
}638-638:
Handle Errors During File Upload Properly
If sf.IdleTimeoutCopyToFile(p, dataEngine) returns an error, the state of sf should be updated to types.StateFailed to ensure proper error propagation.
Include state updating in the error handling:
if _, err := sf.IdleTimeoutCopyToFile(p, dataEngine); err != nil {
+ sf.SetState(types.StateFailed)
return err
}702-702:
Update sf State on Receive Failure
Ensure that when sf.Receive(int(port), fileType, dataEngine) encounters an error, sf.state is updated to reflect the failure.
Adjust the error handling to set the state:
if err := sf.Receive(int(port), fileType, dataEngine); err != nil {
s.log.Errorf("Sync Service: failed to receive sync file %v: %v", filePath, err)
+ sf.SetState(types.StateFailed)
return
}pkg/sync/sync_file.go (1)
920-920:
Handle potential errors from os.RemoveAll
The call to os.RemoveAll(sf.tmpFilePath) does not check for errors. If the removal fails, it could lead to data inconsistencies or unexpected behavior when renaming the converted file. Consider handling the error returned by os.RemoveAll to ensure file operations are successful.
Apply this diff to handle the error:
if imgInfo.Format == "qcow2" {
if convertErr := util.ConvertFromQcow2ToRaw(sf.tmpFilePath, tmpRawFile); convertErr != nil {
finalErr = errors.Wrapf(convertErr, "failed to create raw image from qcow2 image %v", sf.tmpFilePath)
return
}
- os.RemoveAll(sf.tmpFilePath)
+ if removeErr := os.RemoveAll(sf.tmpFilePath); removeErr != nil {
+ finalErr = errors.Wrapf(removeErr, "failed to remove tmp file %v", sf.tmpFilePath)
+ return
+ }
if renameErr := os.Rename(tmpRawFile, sf.tmpFilePath); renameErr != nil {
finalErr = errors.Wrapf(renameErr, "failed to rename tmp raw file %v to file %v", tmpRawFile, sf.tmpFilePath)
return
}
}| @@ -103,7 +103,7 @@ func (s *SyncTestSuite) BenchmarkMultipleDownload(c *C) { | |||
| Remote: s.addr, | |||
| } | |||
|
|
|||
| err := cli.DownloadFromURL("http://test-download-from-url.io", curPath, curUUID, TestDiskUUID, "") | |||
| err := cli.DownloadFromURL("http://test-download-from-url.io", curPath, curUUID, TestDiskUUID, "", types.DataEnginev1) | |||
There was a problem hiding this comment.
Add test coverage for v2 data engine scenarios
The PR adds support for v2 backing images, but all modified test cases are using types.DataEnginev1. We should add test cases that verify the v2 functionality, particularly:
- Downloading qcow2 images with v2 data engine
- Verifying the conversion from qcow2 to raw format
- Testing size and checksum calculations for v2 backing images
Consider adding new test cases or extending existing ones like this:
func (s *SyncTestSuite) TestDownloadToDst(c *C) {
+ // Test v2 data engine with qcow2 to raw conversion
+ downloadFilePathV2 := downloadFilePath + "-v2"
+ err = cli.DownloadToDst(curPath, downloadFilePathV2, types.DataEnginev2)
+ c.Assert(err, IsNil)
+
+ // Verify raw format conversion
+ downloadFileInfoV2, err := util.GetQemuImgInfo(downloadFilePathV2)
+ c.Assert(err, IsNil)
+ c.Assert(downloadFileInfoV2.Format, Equals, "raw")
}Also applies to: 221-221, 317-317, 353-353, 525-525, 533-533, 537-537, 539-539, 611-611
| if openErr != nil { | ||
| err = errors.Wrapf(openErr, "failed to stat the download file %v", sf.filePath) | ||
| return |
There was a problem hiding this comment.
Correct the Error Message for File Opening Failure
When os.Open() fails, the error message incorrectly states "failed to stat the download file." It should reflect the actual operation performed.
Apply this diff to fix the error message:
if openErr != nil {
- err = errors.Wrapf(openErr, "failed to stat the download file %v", sf.filePath)
+ err = errors.Wrapf(openErr, "failed to open the download file %v", sf.filePath)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if openErr != nil { | |
| err = errors.Wrapf(openErr, "failed to stat the download file %v", sf.filePath) | |
| return | |
| if openErr != nil { | |
| err = errors.Wrapf(openErr, "failed to open the download file %v", sf.filePath) | |
| return |
ref: longhorn/longhorn#6341
TODO:
dataEngineto the datasource podSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes