feat(backingimage): support v2 backing image #168
Conversation
ref: longhorn/longhorn 6341 Signed-off-by: Jack Lin <[email protected]>
WalkthroughThis pull request introduces several modifications primarily focused on the logical volume resize functionality. The command Changes
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: 3
🧹 Outside diff range and nitpick comments (2)
app/cmd/nvmecli/nvmecli.go (1)
Line range hint
1-307: Well-structured CLI implementation with clear separation of concerns.The code demonstrates good architectural patterns:
- Clear separation between CLI handling and business logic
- Consistent command structure and error handling
- Proper use of the urfave/cli package features
Consider adding the following improvements:
- Add command-level validation functions to centralize input validation
- Consider implementing command aliases for commonly used commands
- Add command-level documentation that includes examples
app/cmd/basic/bdev_lvol.go (1)
399-401: Add usage description for size-in-mib flag.For consistency with other commands and better clarity, add a usage description specifying that the size is in MiB.
cli.Uint64Flag{ Name: "size-in-mib", + Usage: "Specify the new size in MiB", Required: true, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/cmd/basic/bdev_lvol.go(2 hunks)app/cmd/nvmecli/nvmecli.go(6 hunks)pkg/spdk/client/basic.go(1 hunks)pkg/spdk/types/lvol.go(1 hunks)
🔇 Additional comments (4)
pkg/spdk/types/lvol.go (1)
133-134: LGTM! Field name change improves clarity.
The change from Size to SizeInMib makes the unit of measurement explicit and maintains consistency with other structs like BdevLvolCreateRequest.
app/cmd/nvmecli/nvmecli.go (1)
108-108: LGTM! Consistent access to global flag.
The changes correctly update the retrieval of the "host-proc" parameter from c.String to c.GlobalString. This is the proper way to access flags defined at the root command level, ensuring consistent behavior across all subcommands.
Let's verify that the "host-proc" flag is only defined at the root level and not redefined in subcommands:
Also applies to: 134-134, 170-170, 219-219, 260-260, 292-292
✅ Verification successful
Let me analyze the output and generate another verification script to check if the "host-proc" flag is defined in any subcommands of nvmecli:
From the previous output, we can see that the "host-proc" flag is defined only once in nvmecli.go at the root level. Let me verify one more aspect - checking if it's used consistently with GlobalString across all usages:
LGTM! The host-proc flag is correctly defined and used.
The verification confirms that:
- The "host-proc" flag is defined once at the root level
- It's consistently accessed using
c.GlobalString("host-proc")across all functions - All seven functions (discover, connect, disconnect, get, start, stop, and flush) use the correct method to access this global flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any duplicate definitions of the host-proc flag
# Expected: Only one definition at the root level
# Search for host-proc flag definitions
ast-grep --pattern 'cli.StringFlag{
Name: "host-proc",
$$$
}'
Length of output: 27847
Script:
#!/bin/bash
# Search for any subcommand definitions in nvmecli.go that might redefine the host-proc flag
ast-grep --pattern 'cli.Command{
$$$
Flags: []cli.Flag{
$$$
cli.StringFlag{
Name: "host-proc",
$$$
},
$$$
},
$$$
}'
Length of output: 153
Script:
#!/bin/bash
# Search for any usage of host-proc in nvmecli.go
rg "host-proc" app/cmd/nvmecli/nvmecli.go -B 1 -A 1
Length of output: 1118
app/cmd/basic/bdev_lvol.go (1)
403-403: LGTM!
The usage string has been correctly updated to reflect the new flag name.
pkg/spdk/client/basic.go (1)
417-419: LGTM! Verify callers of this method
The parameter rename from size to sizeInMib improves clarity about the unit of measurement. The implementation correctly uses the renamed field.
Let's verify all callers of this method have been updated:
✅ Verification successful
All callers have been updated correctly to use size-in-mib
The verification shows that:
- The
BdevLvolResizemethod is called only once inapp/cmd/basic/bdev_lvol.go - The caller correctly uses
size-in-mibparameter from CLI flags - No other callers or size-related assignments were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of BdevLvolResize to ensure they're using the correct parameter name
# Test: Search for function calls. Expect: All callers use sizeInMib parameter
# Search for direct function calls
rg -U "BdevLvolResize.*\(" -A 2
# Search for variable assignments that will be used in the function call
rg "(?i)size.*:=.*(?i)mib"
Length of output: 909
| Name string `json:"name"` | ||
| SizeInMib uint64 `json:"size_in_mib"` |
There was a problem hiding this comment.
💡 Codebase verification
API compatibility issue confirmed - action required
The codebase search reveals that the JSON field rename from "size" to "size_in_mib" will cause compatibility issues:
- The
BdevLvolGetFragmapRequeststruct inpkg/spdk/types/lvol.gostill usesjson:"size"for itsSizefield - The CLI commands in
app/cmd/basic/bdev_lvol.gouse size parameters in different contexts (both MiB and bytes) - The client documentation in
pkg/spdk/client/basic.goreferences the"size"field in the API documentation
This change requires coordinated updates across these locations to maintain consistency and avoid breaking API compatibility.
🔗 Analysis chain
Verify API compatibility for JSON field rename.
The JSON field has been renamed from "size" to "size_in_mib". Please ensure this change is compatible with existing API consumers or that appropriate migration steps are in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of the old JSON field name
rg -l '"size"' | while read -r file; do
if [[ "$file" =~ \.json$ ]] || [[ "$file" =~ \.ya?ml$ ]] || [[ "$file" =~ _test\.(go|py|js)$ ]]; then
echo "=== $file ==="
rg -A 2 -B 2 '"size"' "$file"
fi
done
# Search for any struct field references
ast-grep --pattern 'Size uint64'
Length of output: 1218
Script:
#!/bin/bash
# Let's check the specific files that might be using the JSON field
rg -A 5 -B 5 '"size"' app/cmd/basic/bdev_lvol.go pkg/spdk/types/lvol.go pkg/spdk/client/basic.go
# Also check for any test files that might be using this field
fd -e test.go -x rg -l '"size"' {}
# Look for any JSON struct tags with 'size'
rg 'json:"size"'
# Check the git history for this specific file to see when this change was made
git log -p pkg/spdk/types/lvol.go | rg -A 5 -B 5 'size.*json'
Length of output: 4102
| @@ -420,7 +420,7 @@ func bdevLvolResize(c *cli.Context) error { | |||
| name = c.String("uuid") | |||
| } | |||
|
|
|||
| resized, err := spdkCli.BdevLvolResize(name, c.Uint64("size")) | |||
| resized, err := spdkCli.BdevLvolResize(name, c.Uint64("size-in-mib")) | |||
There was a problem hiding this comment.
💡 Codebase verification
Size parameter naming is inconsistent across lvol operations
The verification revealed inconsistencies in size parameter naming:
BdevLvolCreateusessizeparameter and flagBdevLvolResizeusessize-in-mibflag andsizeInMibparameterBdevLvolGetFragmapusessizeparameter (but represents bytes)
The usage descriptions also show inconsistency:
- Create: "Specify bdev lvol size in MiB"
- Resize: No usage description for size-in-mib flag
- GetFragmap: "Size in bytes of the specific segment"
🔗 Analysis chain
LGTM! Verify consistent parameter naming across the codebase.
The client call has been correctly updated to use the new parameter name.
Let's verify that all size-related parameters in lvol operations consistently use the MiB suffix:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in size parameter naming across the codebase
# Expected results: All size parameters in lvol operations should use MiB suffix
# Check for size parameters in function signatures and struct fields
ast-grep --pattern 'func $$$BdevLvol$$$($$$ size $$$)'
# Check for size-related CLI flags
rg --type go "cli\.(Uint64|Int64|Float64)Flag.*Name.*size" -A 2
Length of output: 122
Script:
#!/bin/bash
# Let's try a more comprehensive search for size-related parameters in lvol operations
# Search for lvol-related function calls with size parameters
rg --type go "BdevLvol.*size" -B 2 -A 2
# Search for CLI flags related to size in lvol commands
rg --type go "Name:\s+\"size" -B 2 -A 2
# Search for struct fields or parameters containing both lvol and size
ast-grep --pattern 'type $_ struct {
$$$
size $$$
$$$
}'
# Look for specific lvol resize calls
rg --type go "BdevLvolResize.*size" -B 2 -A 2
Length of output: 7824
| // "sizeInMib": Required. Desired size of the logical volume in bytes. | ||
| func (c *Client) BdevLvolResize(name string, sizeInMib uint64) (resized bool, err error) { |
There was a problem hiding this comment.
Fix documentation inconsistency
The documentation states the size is "in bytes" but the parameter name suggests it's in MiB. Please update the documentation to be consistent with the parameter name.
Apply this diff to fix the documentation:
-// "sizeInMib": Required. Desired size of the logical volume in bytes.
+// "sizeInMib": Required. Desired size of the logical volume in mebibytes (MiB).📝 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.
| // "sizeInMib": Required. Desired size of the logical volume in bytes. | |
| func (c *Client) BdevLvolResize(name string, sizeInMib uint64) (resized bool, err error) { | |
| // "sizeInMib": Required. Desired size of the logical volume in mebibytes (MiB). | |
| func (c *Client) BdevLvolResize(name string, sizeInMib uint64) (resized bool, err error) { |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #168 +/- ##
==========================================
+ Coverage 22.89% 22.95% +0.06%
==========================================
Files 34 34
Lines 4813 4813
==========================================
+ Hits 1102 1105 +3
+ Misses 3537 3535 -2
+ Partials 174 173 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ref: longhorn/longhorn#6341
resizecommand: https://spdk.io/doc/jsonrpc.htmlnvmeclito usec.GlobalStringinstead ofc.String