feat(lvol): add snapshot checksum APIs #169
Conversation
WalkthroughThe pull request introduces new functionality for managing snapshot checksums in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 4
🧹 Outside diff range and nitpick comments (4)
pkg/spdk/types/lvol.go (2)
107-110: LGTM! Consider adding field documentation.The new
EnableAddUpdateXattrsfield is well-structured and follows Go conventions. Its purpose is clear from the name, but consider adding a godoc comment to explain when this flag should be used, especially its interaction with theXattrsfield.type BdevLvolSnapshotRequest struct { + // LvolName is the name of the logical volume to snapshot LvolName string `json:"lvol_name"` + // SnapshotName is the name to give the new snapshot SnapshotName string `json:"snapshot_name"` + // Xattrs are extended attributes to set on the snapshot Xattrs map[string]string `json:"xattrs,omitempty"` + // EnableAddUpdateXattrs controls whether extended attributes can be added or updated during snapshot creation EnableAddUpdateXattrs bool `json:"enable_add_update_xattrs,omitempty"` }
154-164: LGTM! Consider adding validation and documentation.The new checksum-related types are well-structured and consistent with the codebase. Consider these enhancements:
- Add validation methods for the request types
- Document the checksum algorithm or format used
+// BdevLvolRegisterSnapshotChecksumRequest represents a request to register a checksum for a snapshot type BdevLvolRegisterSnapshotChecksumRequest struct { + // Name is the name of the snapshot Name string `json:"name"` } + +func (r *BdevLvolRegisterSnapshotChecksumRequest) Validate() error { + if r.Name == "" { + return fmt.Errorf("snapshot name cannot be empty") + } + return nil +} +// BdevLvolGetSnapshotChecksumRequest represents a request to retrieve a snapshot's checksum type BdevLvolGetSnapshotChecksumRequest struct { + // Name is the name of the snapshot Name string `json:"name"` } + +func (r *BdevLvolGetSnapshotChecksumRequest) Validate() error { + if r.Name == "" { + return fmt.Errorf("snapshot name cannot be empty") + } + return nil +} +// BdevLvolSnapshotChecksum represents a snapshot's checksum value +// The checksum is calculated using [specify algorithm here] type BdevLvolSnapshotChecksum struct { + // Checksum is the calculated checksum value of the snapshot Checksum uint64 `json:"checksum"` }app/cmd/basic/bdev_lvol.go (1)
662-674: Document checksum implementation detailsConsider adding documentation in the command usage that specifies:
- The checksum algorithm being used
- Format of the checksum output
- Best practices for when to register and verify checksums
- Any performance implications of checksum operations
Also applies to: 704-716
pkg/spdk/client/basic.go (1)
506-542: Consider adding unit tests for new snapshot checksum APIsThe newly added functions
BdevLvolRegisterSnapshotChecksumandBdevLvolGetSnapshotChecksumenhance snapshot management capabilities. To ensure their correctness and reliability, consider adding corresponding unit tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/cmd/basic/bdev_lvol.go(2 hunks)pkg/spdk/client/basic.go(2 hunks)pkg/spdk/types/lvol.go(2 hunks)
🔇 Additional comments (4)
app/cmd/basic/bdev_lvol.go (2)
36-37: LGTM: Command registration follows established patterns
The new snapshot checksum commands are properly integrated into the existing command structure.
694-695: Verify SPDK client implementation
Ensure that the corresponding SPDK client methods are properly implemented:
BdevLvolRegisterSnapshotChecksumBdevLvolGetSnapshotChecksum
Also applies to: 736-737
✅ Verification successful
SPDK client methods are properly implemented
The verification shows that both methods are fully implemented in pkg/spdk/client/basic.go:
BdevLvolRegisterSnapshotChecksumis implemented with proper request handlingBdevLvolGetSnapshotChecksumis implemented with proper request handling
Both methods are properly integrated with:
- Request types defined in
pkg/spdk/types/lvol.go - Client struct defined in
pkg/spdk/client/client.go - JSON-RPC communication layer through
jsonCli.SendCommandWithLongTimeout
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the client method implementations
ast-grep --pattern 'func (client *Client) BdevLvolRegisterSnapshotChecksum($$$) {
$$$
}'
ast-grep --pattern 'func (client *Client) BdevLvolGetSnapshotChecksum($$$) {
$$$
}'
Length of output: 180
Script:
#!/bin/bash
# Let's try a broader search for these method names using ripgrep
rg "BdevLvolRegisterSnapshotChecksum|BdevLvolGetSnapshotChecksum" -A 5
# Also search for the SPDK client interface/struct definition
rg "type.*Client.*struct" -A 10
Length of output: 6114
pkg/spdk/client/basic.go (2)
313-315: Enable extended attributes during snapshot creation
The addition of EnableAddUpdateXattrs: true in the BdevLvolSnapshot request enables adding or updating extended attributes during snapshot creation, which is necessary for the snapshot checksum functionality.
509-520: Implementation of BdevLvolRegisterSnapshotChecksum is correct
The BdevLvolRegisterSnapshotChecksum function correctly constructs the request and handles the response, following the established patterns in the client. It effectively computes and stores the checksum of snapshot data.
DamiaSan
force-pushed
the
snapshot_checksum
branch
from
b5a686e to
db380dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/cmd/basic/bdev_lvol.go (1)
659-751: Consider performance and storage implicationsThe checksum implementation should consider:
- Performance impact:
- Consider implementing async checksum computation for large snapshots
- Add progress tracking for long-running computations
- Storage requirements:
- Document the checksum storage location
- Consider cleanup mechanisms for orphaned checksums
- Consistency guarantees:
- Document the behavior when a snapshot is modified after checksum computation
- Consider adding a timestamp to the checksum metadata
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/cmd/basic/bdev_lvol.go(2 hunks)pkg/spdk/client/basic.go(2 hunks)pkg/spdk/types/lvol.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/spdk/client/basic.go
- pkg/spdk/types/lvol.go
🔇 Additional comments (3)
app/cmd/basic/bdev_lvol.go (3)
36-37: LGTM: Command registration follows established patterns
The new commands are properly integrated into the existing command set.
705-751: LGTM: Proper validation and error handling
The implementation includes:
- Input validation for alias/UUID
- Protection against nil pointer dereference
- Descriptive error messages
660-703: Consider additional validation for snapshot registration
While the implementation is solid, consider these improvements:
- Verify that the target is actually a snapshot before attempting to register its checksum
- Add a dry-run flag for validation without computation
func BdevLvolRegisterSnapshotChecksumCmd() cli.Command {
return cli.Command{
Name: "register-snapshot-checksum",
Flags: []cli.Flag{
+ cli.BoolFlag{
+ Name: "dry-run",
+ Usage: "Validate input without computing checksum",
+ },
cli.StringFlag{
Name: "alias",
Usage: "The alias of a snapshot is <LVSTORE NAME>/<SNAPSHOT NAME>. Specify this or uuid",
},
cli.StringFlag{
Name: "uuid",
Usage: "Specify this or alias",
},
},
|
@DamiaSan Ready for review? |
It depends on SPDK PR longhorn/spdk#38. Otherwise CI build will fail. |
DamiaSan
force-pushed
the
snapshot_checksum
branch
from
db380dc to
6f82fcd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #169 +/- ##
==========================================
- Coverage 22.88% 22.41% -0.48%
==========================================
Files 34 34
Lines 4842 4944 +102
==========================================
Hits 1108 1108
- Misses 3557 3659 +102
Partials 177 177
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@DamiaSan It's good to go after updating the vendor. |
Longhorn 9709 Signed-off-by: Damiano Cipriani <[email protected]>
DamiaSan
force-pushed
the
snapshot_checksum
branch
from
6f82fcd to
0b7d4ff
Compare
|
SPDK part has been merged, so now this PR is ready for review and to be merged. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/cmd/basic/bdev_lvol.go (1)
660-703: Consider additional validation for snapshot existenceWhile the implementation is solid, consider adding validation to ensure the target is actually a snapshot before attempting to register its checksum.
func bdevLvolRegisterSnapshotChecksum(c *cli.Context) error { spdkCli, err := client.NewClient(context.Background()) if err != nil { return err } name := c.String("alias") if name == "" { name = c.String("uuid") } if name == "" { return fmt.Errorf("either alias or uuid must be provided") } + // Verify target is a snapshot + info, err := spdkCli.BdevLvolGet(name, 0) + if err != nil { + return fmt.Errorf("failed to get info for %q: %v", name, err) + } + if len(info) == 0 { + return fmt.Errorf("snapshot %q not found", name) + } + if !info[0].DriverSpecific.Lvol.Snapshot { + return fmt.Errorf("%q is not a snapshot", name) + } + registered, err := spdkCli.BdevLvolRegisterSnapshotChecksum(name) if err != nil { return fmt.Errorf("failed to register checksum for snapshot %q: %v", name, err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfile.dapper(1 hunks)app/cmd/basic/bdev_lvol.go(2 hunks)pkg/spdk/client/basic.go(1 hunks)pkg/spdk/types/lvol.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/spdk/types/lvol.go
🔇 Additional comments (5)
Dockerfile.dapper (1)
20-20: Verify the SPDK commit dependency
The SPDK commit update is aligned with the PR dependency mentioned. Let's verify the commit exists and contains the required changes.
✅ Verification successful
SPDK commit dependency verified successfully
The specified SPDK commit 002cd0679259c4e4b782d9540441a46e1228d484 exists in the longhorn/spdk repository, confirming the validity of the dependency update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SPDK commit exists and contains required changes
# Check if the commit exists in longhorn/spdk repository
gh api repos/longhorn/spdk/commits/002cd0679259c4e4b782d9540441a46e1228d484 --jq '.sha'
Length of output: 128
app/cmd/basic/bdev_lvol.go (2)
36-37: LGTM: Commands properly registered
The new checksum commands are correctly added to the subcommands list.
705-751: LGTM: Robust implementation with proper error handling
The implementation includes:
- Proper input validation
- Descriptive error messages
- Nil check for checksum
- Consistent error handling pattern
pkg/spdk/client/basic.go (2)
505-519: LGTM: Proper timeout handling for long-running operation
The implementation correctly uses SendCommandWithLongTimeout for the potentially long-running checksum computation operation.
521-541: LGTM: Clean implementation with proper type handling
The implementation:
- Uses proper SPDK types
- Correctly unmarshals the response
- Returns a pointer to allow nil for non-existent checksums
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9709
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context
This PR depends on the PR longhorn/spdk#38, so it is still a draft. Will convert to a regular PR when the SPDK PR will be merged.