fix: backup progress should not add block failed to upload to successful count #209
Conversation
WalkthroughThe changes in this pull request enhance error handling mechanisms in both the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BackupSystem
participant BackupStoreDriver
User->>BackupSystem: Initiate backup
BackupSystem->>BackupStoreDriver: Write block data
BackupStoreDriver-->>BackupSystem: Return result (success/error)
BackupSystem->>User: Report success/error
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)backupbackingimage/backupbackingimage.go (1)
The modification ensures that failed block uploads are not counted towards the successful backup progress by making the error handling more explicit. When a block write fails, the deferred function in 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| err = bsDriver.Write(blkFile, rs) | ||
| if err != nil { |
There was a problem hiding this comment.
💡 Codebase verification
Fix Approved for deltablock.go:428.
However, similar error handling issues were found in backupbackingimage.go:267 where the error from bsDriver.Write is not assigned. Please ensure all bsDriver.Write calls properly handle errors to avoid counting failed uploads as successful.
- backupbackingimage/backupbackingimage.go:267
🔗 Analysis chain
LGTM! Fix properly captures Write errors.
The change correctly captures and propagates errors from the Write operation, ensuring failed block uploads are not counted as successful.
Let's verify if there are similar patterns in the codebase that might need the same fix:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for similar patterns where Write errors might not be captured
# Test: Search for direct Write calls without error assignment
ast-grep --pattern 'bsDriver.Write($_, $_)
if err != nil'
Length of output: 59
Script:
#!/bin/bash
# Description: Look for bsDriver.Write calls without error assignment
# Test: Search for bsDriver.Write calls that do not assign the returned error to a variable
ast-grep --pattern 'bsDriver.Write($_, $_)' --filter '!(parent(expr_stmt) and parent(expr_stmt).expr = "err = bsDriver.Write($_, $_)")'
Length of output: 325
Script:
#!/bin/bash
# Description: Look for bsDriver.Write calls without error assignment
# Test: Search for bsDriver.Write calls not assigned to the err variable
rg -n 'bsDriver\.Write\s*\([^)]*\)\s*(?!\s*err\s*=)'
Length of output: 332
Script:
#!/bin/bash
# Description: Look for bsDriver.Write calls without error assignment
# Test: Search for bsDriver.Write calls not assigned to the err variable using PCRE2
rg -n --pcre2 'bsDriver\.Write\s*\([^)]*\)\s*(?!\s*err\s*=)'
Length of output: 196
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #209 +/- ##
=========================================
- Coverage 8.08% 8.08% -0.01%
=========================================
Files 23 23
Lines 2535 2536 +1
=========================================
Hits 205 205
- Misses 2303 2304 +1
Partials 27 27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ful count Longhorn 9791 Signed-off-by: Derek Su <[email protected]>
| @@ -264,7 +264,8 @@ | |||
| return err | |||
| } | |||
|
|
|||
| return bsDriver.Write(blkFile, rs) | |||
| err = bsDriver.Write(blkFile, rs) | |||
| return err | |||
There was a problem hiding this comment.
nit: consider using a returned err variable instead, so updates to the variable won't be missed.
longhorn/longhorn#9791
Which issue(s) this PR fixes:
Issue #
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit