feat(backingimage): support spdk backing image #728
Conversation
WalkthroughThe changes introduce a new API for managing backing images, enhancing the existing structures and methods across multiple files. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyClient
participant BackingImageService
Client->>ProxyClient: SPDKBackingImageCreate(name, UUID, checksum, size)
ProxyClient->>BackingImageService: CreateBackingImageRequest
BackingImageService-->>ProxyClient: CreateBackingImageResponse
ProxyClient-->>Client: BackingImageResponse
sequenceDiagram
participant Client
participant ProxyClient
participant BackingImageService
Client->>ProxyClient: SPDKBackingImageList()
ProxyClient->>BackingImageService: ListBackingImagesRequest
BackingImageService-->>ProxyClient: ListBackingImagesResponse
ProxyClient-->>Client: BackingImageListResponse
Warning Rate limit exceeded@ChanYiLin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package qcow: could not load export data: no export data for "github.com/longhorn/longhorn-engine/pkg/qcow"" 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
|
ref: longhorn/longhorn 6341 Signed-off-by: Jack Lin <[email protected]>
ChanYiLin
force-pushed
the
LH6341_v2_backing_image
branch
from
64e2a12
to
18ed15a
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: 8
🧹 Outside diff range and nitpick comments (7)
pkg/api/backing_image.go (1)
8-16: Add documentation and field validation.The struct looks well-designed, but could benefit from:
- GoDoc comments explaining the purpose of the struct and each field
- Validation logic for required fields (Name, BackingImageUUID, Size)
Consider adding documentation and validation like this:
+// BackingImage represents a SPDK backing image with its properties and current status type BackingImage struct { + // Name is the unique identifier of the backing image Name string `json:"name"` + // BackingImageUUID is the UUID assigned to this backing image BackingImageUUID string `json:"backing_image_uuid"` // ... (add comments for other fields) } +// Validate ensures all required fields are present +func (bi *BackingImage) Validate() error { + if bi.Name == "" { + return fmt.Errorf("backing image name is required") + } + if bi.BackingImageUUID == "" { + return fmt.Errorf("backing image UUID is required") + } + if bi.Size == 0 { + return fmt.Errorf("backing image size must be greater than 0") + } + return nil +}pkg/meta/version.go (1)
83-92: Consider future refactoring to reduce duplication.The version management pattern (Version, GitCommit, BuildDate) is duplicated across multiple structs (
VersionOutput,DiskServiceVersionOutput,BackingImageServiceVersionOutput). Consider introducing a common base struct in a future refactoring to follow the DRY principle.Example approach:
type BaseVersionInfo struct { Version string `json:"version"` GitCommit string `json:"gitCommit"` BuildDate string `json:"buildDate"` } type BackingImageServiceVersionOutput struct { BaseVersionInfo InstanceManagerBackingImageServiceAPIVersion int `json:"instanceManagerBackingImageServiceAPIVersion"` InstanceManagerBackingImageServiceAPIMinVersion int `json:"instanceManagerBackingImageServiceAPIMinVersion"` }pkg/proxy/proxy.go (2)
64-66: Consider adding cleanup for SPDK clientWhile the field additions are appropriate, consider implementing a cleanup method for the
spdkLocalClientto ensure proper resource release. This could be done by extending the existing context cancellation handling instartMonitoring().func (p *Proxy) startMonitoring() { <-p.ctx.Done() + if p.spdkLocalClient != nil { + if err := p.spdkLocalClient.Close(); err != nil { + logrus.WithError(err).Error("Failed to close SPDK client") + } + } logrus.Infof("%s: stopped monitoring due to the context done", types.ProxyGRPCService) }
Line range hint
128-144: Fix error handling in getSPDKClientFromAddressThe error check after
net.JoinHostPortis unnecessary as this function only returns an error when given invalid input, which isn't possible here since we've already validated the host throughSplitHostPort. Additionally, the error isn't being used in the condition.func getSPDKClientFromAddress(address string) (*spdkclient.SPDKClient, error) { host, _, err := net.SplitHostPort(address) if err != nil { return nil, err } spdkServiceAddress := net.JoinHostPort(host, strconv.Itoa(types.InstanceManagerSpdkServiceDefaultPort)) - if err != nil { - return nil, err - } return spdkclient.NewSPDKClient(spdkServiceAddress) }pkg/client/instance.go (1)
98-101: Add documentation and validation for BackingImageName fieldWhile the field addition follows the struct's pattern, consider:
- Adding field documentation to explain its purpose and requirements
- Adding validation logic in the InstanceCreate method to ensure BackingImageName meets any format/length requirements when specified
type ReplicaCreateRequest struct { + // DiskName is the name of the disk where the replica will be created DiskName string + // DiskUUID is the unique identifier of the disk DiskUUID string + // ExposeRequired indicates if the replica needs to be exposed ExposeRequired bool + // BackingImageName specifies the name of the SPDK backing image to use BackingImageName string }pkg/client/proxy_backing_image.go (1)
33-34: Ensure consistent error wrapping for improved error contextIn the methods
SPDKBackingImageCreate,SPDKBackingImageDelete, andSPDKBackingImageGet, errors are returned directly without additional context. However, inSPDKBackingImageListandSPDKBackingImageWatch, errors are wrapped usingerrors.Wrap. For consistency and better error tracing, consider wrapping errors in all methods.Apply this diff to wrap errors consistently:
@@ -33,7 +33,7 @@ if err != nil { - return nil, err + return nil, errors.Wrap(err, "failed to create backing image") } @@ -52,7 +52,7 @@ - return err + return errors.Wrap(err, "failed to delete backing image") @@ -69,7 +69,7 @@ if err != nil { - return nil, err + return nil, errors.Wrap(err, "failed to get backing image") }Also applies to: 52-52, 69-70
pkg/proxy/backing_image.go (1)
38-38: Correct the casing of 'SPDK' in log messagesThe term "SPDK" should be fully capitalized in log messages. Currently, it's written as "SPDk".
Apply the following diff to correct the log messages:
- log.Info("Backing Image Server: Creating SPDk Backing Image") + log.Info("Backing Image Server: Creating SPDK Backing Image") - log.Info("Backing Image Server: Deleting SPDk Backing Image") + log.Info("Backing Image Server: Deleting SPDK Backing Image") - log.Info("Backing Image Server: Get SPDk Backing Image") + log.Info("Backing Image Server: Getting SPDK Backing Image") - logrus.Info("Backing Image Server: List SPDk Backing Image") + logrus.Info("Backing Image Server: Listing SPDK Backing Images")Also applies to: 56-56, 68-68, 80-80
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (192)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/bits-and-blooms/bitset/README.mdis excluded by!vendor/**vendor/github.com/bits-and-blooms/bitset/bitset.gois excluded by!vendor/**vendor/github.com/bits-and-blooms/bitset/leading_zeros_18.gois excluded by!vendor/**vendor/github.com/bits-and-blooms/bitset/leading_zeros_19.gois excluded by!vendor/**vendor/github.com/bits-and-blooms/bitset/select.gois excluded by!vendor/**vendor/github.com/coreos/go-systemd/v22/LICENSEis excluded by!vendor/**vendor/github.com/coreos/go-systemd/v22/NOTICEis excluded by!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/dbus.gois excluded by!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/methods.gois excluded by!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/properties.gois excluded by!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/set.gois excluded by!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/subscription.gois excluded by!vendor/**vendor/github.com/coreos/go-systemd/v22/dbus/subscription_set.gois excluded by!vendor/**vendor/github.com/gammazero/deque/README.mdis excluded by!vendor/**vendor/github.com/gammazero/deque/deque.gois excluded by!vendor/**vendor/github.com/gammazero/deque/doc.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/CONTRIBUTING.mdis excluded by!vendor/**vendor/github.com/godbus/dbus/v5/LICENSEis excluded by!vendor/**vendor/github.com/godbus/dbus/v5/MAINTAINERSis excluded by!vendor/**vendor/github.com/godbus/dbus/v5/README.mdis excluded by!vendor/**vendor/github.com/godbus/dbus/v5/auth.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/auth_anonymous.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/auth_external.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/auth_sha1.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/call.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/conn.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/conn_darwin.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/conn_other.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/conn_unix.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/conn_windows.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/dbus.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/decoder.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/default_handler.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/doc.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/encoder.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/escape.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/export.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/homedir.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/match.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/message.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/object.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/sequence.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/sequential_handler.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/server_interfaces.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/sig.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_darwin.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_generic.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_nonce_tcp.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_tcp.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_unix.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_unixcred_dragonfly.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_unixcred_freebsd.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_unixcred_linux.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_unixcred_netbsd.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_unixcred_openbsd.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/transport_zos.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/variant.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/variant_lexer.gois excluded by!vendor/**vendor/github.com/godbus/dbus/v5/variant_parser.gois excluded by!vendor/**vendor/github.com/longhorn/go-spdk-helper/pkg/spdk/client/basic.gois excluded by!vendor/**vendor/github.com/longhorn/go-spdk-helper/pkg/spdk/types/lvol.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/api/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/client/client.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/backing_image.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/backup.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/replica.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/server.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/util.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/types/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/util/http_handler.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/util/util.gois excluded by!vendor/**vendor/github.com/longhorn/types/pkg/generated/imrpc/instance.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/github.com/longhorn/types/pkg/generated/imrpc/proxy.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/github.com/longhorn/types/pkg/generated/imrpc/proxy_grpc.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk_grpc.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/github.com/matttproud/golang_protobuf_extensions/v2/NOTICEis excluded by!vendor/**vendor/github.com/matttproud/golang_protobuf_extensions/v2/pbutil/.gitignoreis excluded by!vendor/**vendor/github.com/matttproud/golang_protobuf_extensions/v2/pbutil/Makefileis excluded by!vendor/**vendor/github.com/matttproud/golang_protobuf_extensions/v2/pbutil/decode.gois excluded by!vendor/**vendor/github.com/matttproud/golang_protobuf_extensions/v2/pbutil/doc.gois excluded by!vendor/**vendor/github.com/matttproud/golang_protobuf_extensions/v2/pbutil/encode.gois excluded by!vendor/**vendor/github.com/moby/sys/mountinfo/mounted_linux.gois excluded by!vendor/**vendor/github.com/moby/sys/userns/LICENSEis excluded by!vendor/**vendor/github.com/moby/sys/userns/userns.gois excluded by!vendor/**vendor/github.com/moby/sys/userns/userns_linux.gois excluded by!vendor/**vendor/github.com/moby/sys/userns/userns_linux_fuzzer.gois excluded by!vendor/**vendor/github.com/moby/sys/userns/userns_unsupported.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/NOTICEis excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/blkio_device.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/cgroup_linux.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/cgroup_unsupported.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/config.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/config_linux.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/configs_fuzzer.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/hugepage_limit.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/intelrdt.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/interface_priority_map.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/mount.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/namespaces.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/namespaces_linux.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall_unsupported.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/namespaces_unsupported.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/network.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/configs/rdma.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/devices/device.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/devices/device_unix.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/user/lookup_unix.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/user/user.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/user/user_fuzzer.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/userns/userns.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/userns/userns_deprecated.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/userns/userns_fuzzer.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/userns/userns_linux.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/userns/userns_maps.cis excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/userns/userns_maps_linux.gois excluded by!vendor/**vendor/github.com/opencontainers/runc/libcontainer/userns/userns_unsupported.gois excluded by!vendor/**vendor/github.com/opencontainers/runtime-spec/LICENSEis excluded by!vendor/**vendor/github.com/opencontainers/runtime-spec/specs-go/config.gois excluded by!vendor/**vendor/github.com/opencontainers/runtime-spec/specs-go/state.gois excluded by!vendor/**vendor/github.com/opencontainers/runtime-spec/specs-go/version.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/NOTICEis excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/go_collector.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/go_collector_latest.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/histogram.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/internal/go_collector_options.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/metric.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/process_collector.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/process_collector_other.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/registry.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/summary.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/vec.gois excluded by!vendor/**vendor/github.com/prometheus/client_model/go/metrics.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/github.com/prometheus/common/expfmt/decode.gois excluded by!vendor/**vendor/github.com/prometheus/common/expfmt/encode.gois excluded by!vendor/**vendor/github.com/prometheus/common/expfmt/expfmt.gois excluded by!vendor/**vendor/github.com/prometheus/common/expfmt/openmetrics_create.gois excluded by!vendor/**vendor/github.com/prometheus/common/expfmt/text_create.gois excluded by!vendor/**vendor/github.com/prometheus/common/expfmt/text_parse.gois excluded by!vendor/**vendor/github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg/README.txtis excluded by!vendor/**vendor/github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg/autoneg.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/alert.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/labels.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/labelset.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/labelset_string.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/metadata.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/metric.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/signature.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/silence.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/value.gois excluded by!vendor/**vendor/github.com/prometheus/common/model/value_float.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/.golangci.ymlis excluded by!vendor/**vendor/github.com/prometheus/procfs/MAINTAINERS.mdis excluded by!vendor/**vendor/github.com/prometheus/procfs/Makefile.commonis excluded by!vendor/**vendor/github.com/prometheus/procfs/arp.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/buddyinfo.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/crypto.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/fscache.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/ipvs.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/loadavg.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/mdstat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/meminfo.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/mountinfo.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/mountstats.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_conntrackstat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_ip_socket.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_sockstat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_softnet.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_tls_stat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_unix.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_wireless.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_limits.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_ns.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_psi.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_smaps.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_stat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_status.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_sys.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/softirqs.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/stat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/swaps.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/thread.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/zoneinfo.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (7)
pkg/api/backing_image.go(1 hunks)pkg/client/instance.go(2 hunks)pkg/client/proxy_backing_image.go(1 hunks)pkg/instance/instance.go(1 hunks)pkg/meta/version.go(2 hunks)pkg/proxy/backing_image.go(1 hunks)pkg/proxy/proxy.go(2 hunks)
🔇 Additional comments (9)
pkg/api/backing_image.go (1)
1-6: LGTM!
The package name and imports are appropriate for the functionality being implemented.
pkg/meta/version.go (3)
15-18: LGTM! API version constants are well-defined.
The new backing image service API version constants follow the established pattern and are appropriately initialized to version 1 for a new service.
74-81: LGTM! Version output struct is well-defined.
The BackingImageServiceVersionOutput struct follows the established pattern and includes appropriate JSON tags for serialization.
15-18: Verify version compatibility checks.
Let's ensure the new version constants are properly integrated into compatibility checks across the codebase.
Also applies to: 74-92
pkg/proxy/proxy.go (3)
9-13: LGTM: Import additions are appropriate
The new imports for error handling and gRPC status codes are necessary for the SPDK client integration and follow Go conventions.
76-79: LGTM: Robust error handling for SPDK client initialization
The error handling is well-implemented:
- Uses appropriate gRPC status codes
- Provides good context through error wrapping
- Returns meaningful error messages to the client
86-88: LGTM: Clean initialization of SPDK fields
The initialization of the SPDK-related fields is clean and follows the established pattern in the struct initialization.
pkg/client/instance.go (1)
156-160: Verify backward compatibility and existing instance handling
The addition of BackingImageName to spdkInstanceSpec needs verification:
- Ensure existing instances without BackingImageName continue to work
- Verify if any default value is needed when BackingImageName is not specified
- Check if this change requires API version bump
✅ Verification successful
Based on the search results, I can now provide a definitive response about the BackingImageName field addition:
BackingImageName field is safely handled with proper backward compatibility
The codebase analysis shows that:
- BackingImageName is an optional field - the code checks for empty string before using it (
if r.BackingImageName != "") - Existing instances without BackingImageName continue to work as the field defaults to empty string
- The field is properly integrated into the SPDK instance creation path with appropriate error handling
- No API version bump is required as this is an additive, backward-compatible change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing usage patterns and potential breaking changes
# Look for existing replica creation patterns
rg -A 5 "InstanceCreate.*InstanceTypeReplica" --type go
# Check for version handling in the codebase
ast-grep --pattern 'const.*Version.*=.*'
Length of output: 100
Script:
#!/bin/bash
# Let's try a different approach to analyze the changes
# Search for BackingImageName usage in the codebase
rg "BackingImageName" --type go -B 2 -A 2
# Look for SPDK instance related structs and interfaces
rg "type.*Instance" --type go -A 5
# Check for version related constants specifically in API packages
fd -e go -E 'vendor/*' --exec grep -l "Version" {} \; | xargs cat
Length of output: 113200
pkg/instance/instance.go (1)
160-163: LGTM! Implementation correctly adds backing image support.
The modification properly integrates the backing image name parameter into the ReplicaCreate call while maintaining consistent error handling.
Let's verify the consistency of backing image handling across the codebase:
✅ Verification successful
Let me gather more specific information about the ReplicaCreate implementation and usage.
Let me check the ReplicaCreateRequest definition to verify the backing image parameter handling.
Based on the gathered information, I can now provide the final response:
Verified: BackingImageName parameter is properly integrated
The modification to include BackingImageName in the ReplicaCreate call is correctly implemented and consistent with the codebase architecture:
- The parameter is properly defined in the SPDK RPC protocol (
ReplicaCreateRequeststruct) - The backing image integration is consistently handled across the client, instance, and SPDK components
- The change maintains compatibility with existing interfaces while extending functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify backing image integration across the codebase
# Check for other occurrences of ReplicaCreate to ensure consistent parameter usage
ast-grep --pattern 'ReplicaCreate($$$)'
# Check for related backing image handling
rg -l "BackingImage" --type go
Length of output: 2097
Script:
#!/bin/bash
# Check for ReplicaCreate implementation and interface definition
rg "ReplicaCreate.*\(" -A 5 --type go
# Check for backing image related structs and types
ast-grep --pattern 'type $_ struct {
$$$
BackingImage$_ $_
$$$
}'
Length of output: 41608
Script:
#!/bin/bash
# Check ReplicaCreateRequest definition in SPDK RPC
rg "type ReplicaCreateRequest struct" -A 10 --type go
# Check for backing image parameter in the SPDK server implementation
rg "BackingImageName.*string" --type go
Length of output: 6322
| func RPCToBackingImage(obj *rpc.SPDKBackingImageResponse) *BackingImage { | ||
| return &BackingImage{ | ||
| Name: obj.Spec.Name, | ||
| BackingImageUUID: obj.Spec.BackingImageUuid, | ||
| DiskUUID: obj.Spec.DiskUuid, | ||
| Size: obj.Spec.Size, | ||
| ExpectedChecksum: obj.Spec.Checksum, | ||
|
|
||
| Status: BackingImageStatus{ | ||
| Progress: int(obj.Status.Progress), | ||
| State: obj.Status.State, | ||
| CurrentChecksum: obj.Status.Checksum, | ||
| ErrorMsg: obj.Status.ErrorMsg, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling.
The conversion function should validate its input and the resulting BackingImage.
Consider this implementation:
-func RPCToBackingImage(obj *rpc.SPDKBackingImageResponse) *BackingImage {
+func RPCToBackingImage(obj *rpc.SPDKBackingImageResponse) (*BackingImage, error) {
+ if obj == nil {
+ return nil, fmt.Errorf("cannot convert nil SPDKBackingImageResponse")
+ }
+ if obj.Spec == nil {
+ return nil, fmt.Errorf("backing image spec is nil")
+ }
+
bi := &BackingImage{
Name: obj.Spec.Name,
BackingImageUUID: obj.Spec.BackingImageUuid,
DiskUUID: obj.Spec.DiskUuid,
Size: obj.Spec.Size,
ExpectedChecksum: obj.Spec.Checksum,
Status: BackingImageStatus{
Progress: int(obj.Status.Progress),
State: obj.Status.State,
CurrentChecksum: obj.Status.Checksum,
ErrorMsg: obj.Status.ErrorMsg,
},
}
+
+ if err := bi.Validate(); err != nil {
+ return nil, fmt.Errorf("invalid backing image after conversion: %v", err)
+ }
+
- return bi
+ return bi, nil
}Committable suggestion skipped: line range outside the PR's diff.
| func RPCToBackingImageList(obj *rpc.SPDKBackingImageListResponse) map[string]*BackingImage { | ||
| ret := map[string]*BackingImage{} | ||
| for name, bi := range obj.BackingImages { | ||
| ret[name] = RPCToBackingImage(bi) | ||
| } | ||
| return ret | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider memory optimization.
The list conversion should handle errors and optimize memory allocation.
Consider this implementation:
-func RPCToBackingImageList(obj *rpc.SPDKBackingImageListResponse) map[string]*BackingImage {
+func RPCToBackingImageList(obj *rpc.SPDKBackingImageListResponse) (map[string]*BackingImage, error) {
+ if obj == nil {
+ return nil, fmt.Errorf("cannot convert nil SPDKBackingImageListResponse")
+ }
+
- ret := map[string]*BackingImage{}
+ ret := make(map[string]*BackingImage, len(obj.BackingImages))
for name, bi := range obj.BackingImages {
- ret[name] = RPCToBackingImage(bi)
+ converted, err := RPCToBackingImage(bi)
+ if err != nil {
+ return nil, fmt.Errorf("failed to convert backing image %s: %v", name, err)
+ }
+ ret[name] = converted
}
- return ret
+ return ret, nil
}Committable suggestion skipped: line range outside the PR's diff.
| type BackingImageStatus struct { | ||
| Progress int `json:"progress"` | ||
| State string `json:"state"` | ||
| CurrentChecksum string `json:"currentChecksum"` | ||
| ErrorMsg string `json:"errorMsg"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using enum for State and fix JSON tag inconsistency.
Two suggestions for improvement:
- The
Statefield should be an enumerated type for type safety - The
CurrentChecksumJSON tag doesn't follow the snake_case convention used by other fields
Consider this implementation:
+// BackingImageState represents the possible states of a backing image
+type BackingImageState string
+
+const (
+ BackingImageStateInitial BackingImageState = "initial"
+ BackingImageStateReady BackingImageState = "ready"
+ BackingImageStateError BackingImageState = "error"
+ // Add other states as needed
+)
type BackingImageStatus struct {
Progress int `json:"progress"`
- State string `json:"state"`
+ State BackingImageState `json:"state"`
CurrentChecksum string `json:"currentChecksum"`
+ CurrentChecksum string `json:"current_checksum"`
ErrorMsg string `json:"errorMsg"`
+ ErrorMsg string `json:"error_msg"`
}Committable suggestion skipped: line range outside the PR's diff.
| type BackingImageStream struct { | ||
| stream rpc.ProxyEngineService_SPDKBackingImageWatchClient | ||
| } | ||
|
|
||
| func NewBackingImageStream(stream rpc.ProxyEngineService_SPDKBackingImageWatchClient) *BackingImageStream { | ||
| return &BackingImageStream{ | ||
| stream, | ||
| } | ||
| } | ||
|
|
||
| func (s *BackingImageStream) Recv() (*emptypb.Empty, error) { | ||
| return s.stream.Recv() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance stream management with context and cleanup.
The stream implementation should handle context cancellation and provide proper cleanup.
Consider this implementation:
+// BackingImageStream wraps the SPDK backing image watch stream
type BackingImageStream struct {
stream rpc.ProxyEngineService_SPDKBackingImageWatchClient
+ done chan struct{}
}
func NewBackingImageStream(stream rpc.ProxyEngineService_SPDKBackingImageWatchClient) *BackingImageStream {
return &BackingImageStream{
stream: stream,
+ done: make(chan struct{}),
}
}
-func (s *BackingImageStream) Recv() (*emptypb.Empty, error) {
+func (s *BackingImageStream) Recv(ctx context.Context) (*emptypb.Empty, error) {
+ select {
+ case <-ctx.Done():
+ return nil, ctx.Err()
+ case <-s.done:
+ return nil, fmt.Errorf("stream closed")
+ default:
return s.stream.Recv()
+ }
+}
+
+// Close closes the backing image stream
+func (s *BackingImageStream) Close() error {
+ close(s.done)
+ return nil
}📝 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.
| type BackingImageStream struct { | |
| stream rpc.ProxyEngineService_SPDKBackingImageWatchClient | |
| } | |
| func NewBackingImageStream(stream rpc.ProxyEngineService_SPDKBackingImageWatchClient) *BackingImageStream { | |
| return &BackingImageStream{ | |
| stream, | |
| } | |
| } | |
| func (s *BackingImageStream) Recv() (*emptypb.Empty, error) { | |
| return s.stream.Recv() | |
| } | |
| // BackingImageStream wraps the SPDK backing image watch stream | |
| type BackingImageStream struct { | |
| stream rpc.ProxyEngineService_SPDKBackingImageWatchClient | |
| done chan struct{} | |
| } | |
| func NewBackingImageStream(stream rpc.ProxyEngineService_SPDKBackingImageWatchClient) *BackingImageStream { | |
| return &BackingImageStream{ | |
| stream: stream, | |
| done: make(chan struct{}), | |
| } | |
| } | |
| func (s *BackingImageStream) Recv(ctx context.Context) (*emptypb.Empty, error) { | |
| select { | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| case <-s.done: | |
| return nil, fmt.Errorf("stream closed") | |
| default: | |
| return s.stream.Recv() | |
| } | |
| } | |
| // Close closes the backing image stream | |
| func (s *BackingImageStream) Close() error { | |
| close(s.done) | |
| return nil | |
| } |
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to create spdk backing image due to parameters missing") | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message in SPDKBackingImageGet
The error message in SPDKBackingImageGet incorrectly states "failed to create spdk backing image due to parameters missing", but this function is for retrieving backing images. Update the error message to accurately reflect the get operation.
Apply the following diff to correct the error message:
- return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to create spdk backing image due to parameters missing")
+ return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to get spdk backing image due to missing parameters")📝 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.
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to create spdk backing image due to parameters missing") | |
| } | |
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to get spdk backing image due to missing parameters") | |
| } |
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to create spdk backing image due to parameters missing") | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message in SPDKBackingImageDelete
The error message in SPDKBackingImageDelete incorrectly states "failed to create spdk backing image due to parameters missing", but this function is responsible for deleting backing images. Update the error message to accurately reflect the delete operation.
Apply the following diff to correct the error message:
- return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to create spdk backing image due to parameters missing")
+ return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to delete spdk backing image due to missing parameters")📝 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.
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to create spdk backing image due to parameters missing") | |
| } | |
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "failed to delete spdk backing image due to missing parameters") | |
| } |
| func (p *Proxy) watchSPDKBackingImage(ctx context.Context, req *emptypb.Empty, client *spdkclient.SPDKClient, notifyChan chan struct{}) error { | ||
| logrus.Info("Start watching SPDK replicas") | ||
|
|
||
| notifier, err := client.BackingImageWatch(context.Background()) |
There was a problem hiding this comment.
Use the provided context instead of context.Background()
In watchSPDKBackingImage, the call to client.BackingImageWatch uses context.Background(), which ignores cancellation and timeout signals from the parent context. This can lead to resource leaks if the parent context is canceled.
Replace context.Background() with the provided ctx to ensure proper context propagation.
- notifier, err := client.BackingImageWatch(context.Background())
+ notifier, err := client.BackingImageWatch(ctx)This change ensures that the BackingImageWatch operation respects cancellations and timeouts, preventing potential resource leaks.
📝 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.
| notifier, err := client.BackingImageWatch(context.Background()) | |
| notifier, err := client.BackingImageWatch(ctx) |
| done <- struct{}{} | ||
| return grpcstatus.Error(grpccodes.Internal, errors.Wrapf(err, "failed to create SPDK client").Error()) |
There was a problem hiding this comment.
Prevent potential deadlock when spdkClient creation fails
In SPDKBackingImageWatch, if an error occurs during the creation of spdkClient and done <- struct{}{} is called, it may cause a deadlock because the goroutine that reads from done hasn't started yet. Since done is an unbuffered channel, the send operation will block indefinitely.
To fix this issue, rearrange the code to start the goroutine before attempting to create spdkClient, and avoid sending to done when an error occurs.
Apply the following changes:
done := make(chan struct{})
+ go func() {
+ <-done
+ logrus.Info("Stopped clients for watching SPDK backing image")
+ if spdkClient != nil {
+ spdkClient.Close()
+ }
+ close(done)
+ }()
// Create a client for watching SPDK backing image
spdkClient, err := spdkclient.NewSPDKClient(p.spdkServiceAddress)
if err != nil {
- done <- struct{}{}
return grpcstatus.Error(grpccodes.Internal, errors.Wrapf(err, "failed to create SPDK client").Error())
}This ensures the goroutine is ready to receive from done before any potential send operation. Also, we check if spdkClient is not nil before calling spdkClient.Close().
Committable suggestion skipped: line range outside the PR's diff.
|
This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏 |
ref: longhorn/longhorn#6341