Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(lvol): add snapshot checksum APIs #169

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions app/cmd/basic/bdev_lvol.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
BdevLvolGetXattrCmd(),
BdevLvolGetFragmapCmd(),
BdevLvolRenameCmd(),
BdevLvolRegisterSnapshotChecksumCmd(),
BdevLvolGetSnapshotChecksumCmd(),

Check warning on line 37 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L36-L37

Added lines #L36 - L37 were not covered by tests
},
}
}
Expand Down Expand Up @@ -654,3 +656,96 @@

return util.PrintObject(renamed)
}

func BdevLvolRegisterSnapshotChecksumCmd() cli.Command {
return cli.Command{
Name: "register-snapshot-checksum",
Flags: []cli.Flag{
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",
},
},
Usage: "compute and store checksum of snapshot's data: \"register-snapshot-checksum --alias <LVSTORE NAME>/<LVOL NAME>\"," +
" or \"register-snapshot-checksum --uuid <LVOL UUID>\"",
Action: func(c *cli.Context) {
if err := bdevLvolRegisterSnapshotChecksum(c); err != nil {
logrus.WithError(err).Fatalf("Failed to run register snapshot checksum command")
}

Check warning on line 678 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L660-L678

Added lines #L660 - L678 were not covered by tests
},
}
}

func bdevLvolRegisterSnapshotChecksum(c *cli.Context) error {
spdkCli, err := client.NewClient(context.Background())
if err != nil {
return err
}

Check warning on line 687 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L683-L687

Added lines #L683 - L687 were not covered by tests

name := c.String("alias")
if name == "" {
name = c.String("uuid")
}
if name == "" {
return fmt.Errorf("either alias or uuid must be provided")
}

Check warning on line 695 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L689-L695

Added lines #L689 - L695 were not covered by tests

registered, err := spdkCli.BdevLvolRegisterSnapshotChecksum(name)
if err != nil {
return fmt.Errorf("failed to register checksum for snapshot %q: %v", name, err)
}

Check warning on line 700 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L697-L700

Added lines #L697 - L700 were not covered by tests

return util.PrintObject(registered)

Check warning on line 702 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L702

Added line #L702 was not covered by tests
}

func BdevLvolGetSnapshotChecksumCmd() cli.Command {
return cli.Command{
Name: "get-snapshot-checksum",
Flags: []cli.Flag{
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",
},
},
Usage: "get checksum of snapshot's data: \"get-snapshot-checksum --alias <LVSTORE NAME>/<LVOL NAME>\"," +
" or \"get-snapshot-checksum --uuid <LVOL UUID>\"",
Action: func(c *cli.Context) {
if err := bdevLvolGetSnapshotChecksum(c); err != nil {
logrus.WithError(err).Fatalf("Failed to run get snapshot checksum command")
}

Check warning on line 723 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L705-L723

Added lines #L705 - L723 were not covered by tests
},
}
}

func bdevLvolGetSnapshotChecksum(c *cli.Context) error {
spdkCli, err := client.NewClient(context.Background())
if err != nil {
return err
}

Check warning on line 732 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L728-L732

Added lines #L728 - L732 were not covered by tests

name := c.String("alias")
if name == "" {
name = c.String("uuid")
}
if name == "" {
return fmt.Errorf("either alias or uuid must be provided")
}

Check warning on line 740 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L734-L740

Added lines #L734 - L740 were not covered by tests

checksum, err := spdkCli.BdevLvolGetSnapshotChecksum(name)
if err != nil {
return fmt.Errorf("failed to get checksum for snapshot %q: %v", name, err)
}
if checksum == nil {
return fmt.Errorf("no checksum found for snapshot %q", name)
}

Check warning on line 748 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L742-L748

Added lines #L742 - L748 were not covered by tests

return util.PrintObject(*checksum)

Check warning on line 750 in app/cmd/basic/bdev_lvol.go

View check run for this annotation

Codecov / codecov/patch

app/cmd/basic/bdev_lvol.go#L750

Added line #L750 was not covered by tests
}
38 changes: 38 additions & 0 deletions pkg/spdk/client/basic.go
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,44 @@
return &result, nil
}

// BdevLvolRegisterSnapshotChecksum compute and store checksum of snapshot's data. Overwrite old checksum if already registered.
//
// "name": Required. UUID or alias of the snapshot. The alias of a snapshot is <LVSTORE NAME>/<SNAPSHOT NAME>.
func (c *Client) BdevLvolRegisterSnapshotChecksum(name string) (registered bool, err error) {
req := spdktypes.BdevLvolRegisterSnapshotChecksumRequest{
Name: name,
}

cmdOutput, err := c.jsonCli.SendCommandWithLongTimeout("bdev_lvol_register_snapshot_checksum", req)
if err != nil {
return false, err
}

Check warning on line 516 in pkg/spdk/client/basic.go

View check run for this annotation

Codecov / codecov/patch

pkg/spdk/client/basic.go#L508-L516

Added lines #L508 - L516 were not covered by tests

return registered, json.Unmarshal(cmdOutput, &registered)

Check warning on line 518 in pkg/spdk/client/basic.go

View check run for this annotation

Codecov / codecov/patch

pkg/spdk/client/basic.go#L518

Added line #L518 was not covered by tests
}

// BdevLvolGetSnapshotChecksum gets snapshot's stored checksum. The checksum must has been previously registered.
//
// "name": Required. UUID or alias of the snapshot. The alias of a snapshot is <LVSTORE NAME>/<SNAPSHOT NAME>.
func (c *Client) BdevLvolGetSnapshotChecksum(name string) (checksum *uint64, err error) {
req := spdktypes.BdevLvolGetSnapshotChecksumRequest{
Name: name,
}

cmdOutput, err := c.jsonCli.SendCommandWithLongTimeout("bdev_lvol_get_snapshot_checksum", req)
if err != nil {
return nil, err
}

Check warning on line 532 in pkg/spdk/client/basic.go

View check run for this annotation

Codecov / codecov/patch

pkg/spdk/client/basic.go#L524-L532

Added lines #L524 - L532 were not covered by tests

var snapshotChecksum spdktypes.BdevLvolSnapshotChecksum
err = json.Unmarshal(cmdOutput, &snapshotChecksum)
if err != nil {
return nil, err
}

Check warning on line 538 in pkg/spdk/client/basic.go

View check run for this annotation

Codecov / codecov/patch

pkg/spdk/client/basic.go#L534-L538

Added lines #L534 - L538 were not covered by tests

return &snapshotChecksum.Checksum, nil

Check warning on line 540 in pkg/spdk/client/basic.go

View check run for this annotation

Codecov / codecov/patch

pkg/spdk/client/basic.go#L540

Added line #L540 was not covered by tests
}

// BdevLvolRename renames a logical volume.
//
// "oldName": Required. UUID or alias of the existing logical volume.
Expand Down
12 changes: 12 additions & 0 deletions pkg/spdk/types/lvol.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,18 @@ type BdevLvolRenameRequest struct {
NewName string `json:"new_name"`
}

type BdevLvolRegisterSnapshotChecksumRequest struct {
Name string `json:"name"`
}

type BdevLvolGetSnapshotChecksumRequest struct {
Name string `json:"name"`
}

type BdevLvolSnapshotChecksum struct {
Checksum uint64 `json:"checksum"`
}
Comment on lines +153 to +163
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Let me generate the final response based on the verification results.

Implementation appears consistent but lacks test coverage

The implementation shows proper integration:

  • The new types are correctly used in the client implementation
  • RPC method names match between types and client calls
  • JSON request/response structures are properly handled

However, there are no test files found for these new snapshot checksum features.

  • pkg/spdk/client/basic.go: Add unit tests for BdevLvolRegisterSnapshotChecksum and BdevLvolGetSnapshotChecksum methods
  • pkg/spdk/types/lvol.go: Consider adding validation tests for the new types
🔗 Analysis chain

Verify integration with dependent PR.

Since this implementation depends on PR #38 from longhorn/spdk, let's verify the consistency between these changes and the dependent PR.


Let me analyze the results and generate additional verification scripts to check the implementation details.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any existing usage of the new types and verify consistency

# Check for any existing usage of the new types
echo "Checking for existing usage of new types..."
rg "BdevLvolRegisterSnapshotChecksumRequest|BdevLvolGetSnapshotChecksumRequest|BdevLvolSnapshotChecksum" --type go

# Look for any related command implementations
echo "Checking for command implementations..."
rg "RegisterSnapshotChecksum|GetSnapshotChecksum" --type go

# Look for any test files
echo "Checking for test coverage..."
fd -e go -e test

Length of output: 33142


Script:

#!/bin/bash
# Check the implementation of the snapshot checksum methods in client

# Look for the client implementation details
echo "Checking client implementation..."
rg -A 10 "BdevLvolRegisterSnapshotChecksum|BdevLvolGetSnapshotChecksum" pkg/spdk/client/basic.go

# Check for any RPC method definitions
echo "Checking RPC method definitions..."
rg "bdev_lvol_register_snapshot_checksum|bdev_lvol_get_snapshot_checksum" --type go

# Look for any test files related to snapshot checksum
echo "Checking test files..."
rg -l "TestBdevLvolRegisterSnapshotChecksum|TestBdevLvolGetSnapshotChecksum" --type go

Length of output: 1990


func GetLvolAlias(lvsName, lvolName string) string {
return fmt.Sprintf("%s/%s", lvsName, lvolName)
}
Expand Down
Loading