feat(backing image): support backing image in spdk engine #246
Conversation
WalkthroughThis pull request introduces significant enhancements to the handling of backing images across multiple components in the codebase. Key changes include the addition of new structs, methods, and fields related to backing images in various files, such as 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
|
ChanYiLin
force-pushed
the
LH6341_v2_backing_image
branch
from
a9523c6
to
680d8f2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (20)
pkg/types/types.go (2)
51-54: Consider future scalability with hardcoded port countThe
BackingImagePortCountis hardcoded to 1. While this might be sufficient for current needs, consider:
- Making it configurable for future scalability
- Adding a comment explaining why single port is sufficient
55-57: Document the snapshot attributesThe backing image snapshot attributes are crucial for the functionality. Consider adding comments to explain:
- The format and validation requirements for
checksum- The purpose and lifecycle impact of
backing_image_prepare_statepkg/util/util.go (1)
274-287: Consider adding progress reporting for large files.The checksum calculation reads the entire file into memory, which could be resource-intensive for large backing images. Consider adding progress reporting or chunked processing for better monitoring of long-running operations.
Here's a suggested implementation with progress reporting:
-func GetFileChecksum(filePath string) (string, error) { +func GetFileChecksum(filePath string, progressChan chan<- float64) (string, error) { f, err := os.Open(filePath) if err != nil { return "", err } defer f.Close() + // Get file size for progress reporting + fileInfo, err := f.Stat() + if err != nil { + return "", err + } + totalSize := fileInfo.Size() + h := sha512.New() - if _, err := io.Copy(h, f); err != nil { + written := int64(0) + buf := make([]byte, 32*1024) // 32KB chunks + for { + n, err := f.Read(buf) + if n > 0 { + h.Write(buf[:n]) + written += int64(n) + if progressChan != nil { + progressChan <- float64(written) / float64(totalSize) * 100 + } + } + if err == io.EOF { + break + } + if err != nil { + return "", err + } + } return hex.EncodeToString(h.Sum(nil)), nil }pkg/spdk/types.go (2)
52-73: LGTM with a minor documentation suggestionThe conversion logic is correct and properly handles backing image specific cases. Consider adding a brief comment explaining why
SnapshotTimestampis empty and uses creation time instead.func ServiceBackingImageLvolToProtoBackingImageLvol(lvol *Lvol) *spdkrpc.Lvol { + // For backing images, we use CreationTime as the primary timestamp since + // SnapshotTimestamp is not applicable in this context res := &spdkrpc.Lvol{
148-166: Consider using constants for name patternsThe functions correctly handle name generation and validation, but using string literals directly could lead to maintenance issues.
Consider defining constants for the common patterns:
const ( + BackingImagePrefix = "bi-" + BackingImageDiskInfix = "-disk-" BackingImageTempHeadLvolSuffix = "temp-head" ) func GetBackingImageSnapLvolName(backingImageName string, lvsUUID string) string { - return fmt.Sprintf("bi-%s-disk-%s", backingImageName, lvsUUID) + return fmt.Sprintf("%s%s%s%s", BackingImagePrefix, backingImageName, BackingImageDiskInfix, lvsUUID) }pkg/spdk/backup.go (1)
410-413: Document the significance of index valuesConsider adding a comment explaining why we return different indices (0 or 1) based on backing image presence, as this logic is not immediately obvious.
Add documentation like this:
func (b *Backup) findIndex(lvolName string) int { if lvolName == "" { + // Return index 0 for backing images to ensure they're included in the snapshot range + // Otherwise return 1 to skip the first element in the active chain if b.replica.BackingImage != nil { return 0 } return 1 }pkg/spdk_test.go (2)
Line range hint
243-1420: Add test coverage for backing image functionalityWhile the
ReplicaCreatecalls have been updated to support backing images, there are no test cases that actually verify the backing image functionality. Consider adding test cases that:
- Create replicas with actual backing images
- Verify backing image synchronization between SPDK servers
- Test backing image download from backing-image-manager URL
- Validate backing image record handling in the
verify()functionWould you like me to help create these test cases?
Line range hint
28-31: Consider making port ranges configurableThe hardcoded port range (20000-30000) might conflict with other services in certain environments. Consider:
- Making the port range configurable via environment variables
- Adding a helper function to find available ports dynamically
-defaultTestStartPort = int32(20000) -defaultTestEndPort = int32(30000) +defaultTestStartPort = getEnvInt32("TEST_START_PORT", 20000) +defaultTestEndPort = getEnvInt32("TEST_END_PORT", 30000)pkg/util/http_handler.go (2)
94-166: Consider refactoringIdleTimeoutCopyto reduce complexityThe
IdleTimeoutCopyfunction is quite complex and spans over 70 lines, which can make it harder to read, maintain, and test. Consider breaking it into smaller helper functions to improve readability and reduce cognitive load.🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 94-167: pkg/util/http_handler.go#L94-L167
Complex Method
168-174: SimplifyremoveRefererfunction usingHeader.DelmethodThe current implementation iterates over all headers to delete the "Referer" header, which is unnecessary. You can simplify the function by using the
Header.Delmethod provided by thehttppackage.Apply this diff to simplify the function:
-func removeReferer(req *http.Request) { - for k := range req.Header { - if strings.ToLower(k) == "referer" { - delete(req.Header, k) - } - } -} +func removeReferer(req *http.Request) { + req.Header.Del("Referer") +}pkg/api/types.go (1)
181-227: Consider adding unit tests forBackingImagestruct and conversion functionsTo ensure the correctness of the
BackingImagestruct and its conversion functions (ProtoBackingImageToBackingImageandBackingImageToProtoBackingImage), consider adding unit tests that cover various scenarios, including typical usage and edge cases.pkg/spdk/backing_image.go (1)
688-688: Fix typo in error messageThere's a typo in the error message: "failed to unsexpose the source backing image %v" should be "failed to unexpose".
Apply this diff:
- return errors.Wrapf(err, "failed to unsexpose the source backing image %v", bi.Name) + return errors.Wrapf(err, "failed to unexpose the source backing image %v", bi.Name)pkg/client/client.go (4)
Line range hint
56-71: Consider validating the 'backingImageName' parameterIn the
ReplicaCreatefunction, thebackingImageNameparameter is added but not validated. IfbackingImageNameis required, consider adding a check to ensure it is not empty. This will prevent potential issues if an empty string is unintentionally passed.Apply this diff to validate the
backingImageNameparameter:func (c *SPDKClient) ReplicaCreate(name, lvsName, lvsUUID string, specSize uint64, portCount int32, backingImageName string) (*api.Replica, error) { if name == "" || lvsName == "" || lvsUUID == "" { return nil, fmt.Errorf("failed to start SPDK replica: missing required parameters") } + if backingImageName == "" { + return nil, fmt.Errorf("failed to start SPDK replica: missing backingImageName") + } client := c.getSPDKServiceClient()
886-888: Standardize error messages and terminologyThe error message uses "failed to start SPDK backing image," whereas other methods use "failed to create SPDK backing image." For consistency and clarity, consider standardizing the terminology across all backing image methods.
Apply this diff to update the error message:
- return nil, fmt.Errorf("failed to start SPDK backing image: missing required parameters") + return nil, fmt.Errorf("failed to create SPDK backing image: missing required parameters")
909-911: Consistent use of terms in error messagesIn the
BackingImageDeletemethod, "backingImage" is written without a space. Elsewhere, it's written as "backing image." For consistency, consider using "backing image" throughout the codebase.Apply this diff to update the error message:
- return fmt.Errorf("failed to delete SPDK backingImage: missing required parameter") + return fmt.Errorf("failed to delete SPDK backing image: missing required parameter")
925-927: Clarify error message in 'BackingImageGet'The error message says "failed to get SPDK BackingImage: missing required parameter," but it would be clearer to specify which parameter is missing for easier debugging.
Apply this diff to specify missing parameters:
if name == "" || lvsUUID == "" { - return nil, fmt.Errorf("failed to get SPDK BackingImage: missing required parameter") + return nil, fmt.Errorf("failed to get SPDK backing image: missing required parameters 'name' or 'lvsUUID'") }pkg/spdk/replica.go (2)
202-202: Typo in comment: "habe" should be "have"There's a typographical error in the comment on line 202. The word "habe" should be corrected to "have" for clarity.
1951-1951: Typo in comment: "ths" should be "the"There's a typographical error in the comment on line 1951. The word "ths" should be corrected to "the" for clarity.
🧰 Tools
🪛 GitHub Check: codespell
[failure] 1951-1951:
ths ==> the, thispkg/spdk/engine.go (2)
766-777: Simplify nested conditional statements for clarityThe nested
ifstatements from lines 766 to 777 are complex and can be refactored to improve readability and maintainability.Proposed Refactor:
if len(replica.Snapshots) > 0 { + hasSnapshot = true if hasBackingImage { e.log.Warnf("Replica %s lacks a backing image while others have one.", replicaName) } else { for snapshotName, snapApiLvol := range replica.Snapshots { if snapApiLvol.Parent == "" { replicaAncestorMap[replicaName] = snapApiLvol break } } } } else { if hasSnapshot { e.log.Warnf("Replica %s lacks snapshots while others have them.", replicaName) } else { replicaAncestorMap[replicaName] = replica.Head } }
Line range hint
1336-1345: Improve error message clarity ingetRebuildingSnapshotListWhen
ancestorSnapshotNameorlatestSnapshotNameis empty, the error message includes empty variables, which may not provide sufficient context for debugging.Suggested Error Message Improvement:
if ancestorSnapshotName == "" || latestSnapshotName == "" { - return []*api.Lvol{}, fmt.Errorf("cannot find the ancestor snapshot %s or latest snapshot %s from RW replica %s snapshot map during engine replica add", ancestorSnapshotName, latestSnapshotName, srcReplicaName) + return []*api.Lvol{}, fmt.Errorf("cannot find the ancestor or latest snapshot from RW replica %s during engine replica add", srcReplicaName) }
🛑 Comments failed to post (22)
pkg/util/util.go (1)
262-272:
⚠️ Potential issueAdd input validation for backing image name.
The backing image name is used directly in path construction without sanitization. This could potentially lead to path traversal issues if the name contains '../' or other special characters.
Consider adding validation for the backing image name. Here's a suggested implementation:
func GetBackingImageDirectoryName(biName, biUUID string) string { + // Sanitize biName to prevent directory traversal + sanitizedName := strings.ReplaceAll(biName, "/", "_") + sanitizedName = strings.ReplaceAll(sanitizedName, "..", "_") - return fmt.Sprintf("%s-%s", biName, biUUID) + return fmt.Sprintf("%s-%s", sanitizedName, biUUID) }Committable suggestion skipped: line range outside the PR's diff.
pkg/spdk/types.go (3)
124-166: 🛠️ Refactor suggestion
Consider consistent error handling across all name-related functions
Some functions return errors while others silently handle invalid input. This inconsistency could make error handling difficult for callers.
Consider making all name-related functions return errors:
-func GetBackingImageNameFromLvolName(lvolName string) string { +func GetBackingImageNameFromLvolName(lvolName string) (string, error) { if !strings.HasPrefix(lvolName, "bi-") { - return "" + return "", fmt.Errorf("invalid backing image lvol name format: missing bi- prefix") } parts := strings.Split(lvolName, "-") if len(parts) < 2 { - return "" + return "", fmt.Errorf("invalid backing image lvol name format: insufficient parts") } - return parts[1] + return parts[1], nil }Committable suggestion skipped: line range outside the PR's diff.
124-166: 💡 Codebase verification
⚠️ Potential issueSecurity concern partially addressed, but improvement needed
The search results reveal that while
ExtractBackingImageAndDiskUUIDproperly validates input with a strict regex pattern,GetBackingImageNameFromLvolNameperforms unsafe string splitting without validation. However, the only usage ofGetBackingImageNameFromLvolNamein the codebase is immediately followed by a call toExtractBackingImageAndDiskUUIDwith proper error handling, making the security risk minimal.To maintain consistent security practices:
- Replace the implementation of
GetBackingImageNameFromLvolNameto useExtractBackingImageAndDiskUUIDinternally- Consider deprecating
GetBackingImageNameFromLvolNamein favor of the saferExtractBackingImageAndDiskUUID🔗 Analysis chain
Enhance input validation for security
The current implementation accepts arbitrary strings for backing image names and UUIDs, which could potentially lead to security issues if not properly validated upstream.
Let's verify how these functions are used in the codebase:
Consider adding validation for backing image names and UUIDs:
- Define allowed character sets
- Set maximum length limits
- Validate UUID format
- Escape or reject potentially dangerous characters
The search results show that
ExtractBackingImageAndDiskUUIDalready implements proper input validation using a strict regex pattern^bi-([a-zA-Z0-9-]+)-disk-([a-zA-Z0-9-]+)$that:
- Enforces a strict format with
^and$- Only allows alphanumeric characters and hyphens
[a-zA-Z0-9-]- Returns an error for non-matching inputs
Let's verify if this function is consistently used as the primary validation method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for input validation before calling these functions rg -A 5 "GetBackingImageNameFromLvolName|ExtractBackingImageAndDiskUUID|GetBackingImageSnapLvolName" # Check if there's any input sanitization for backing image names ast-grep --pattern 'func $_(backingImageName string, $$$) { $$$ }'Length of output: 10026
Script:
#!/bin/bash # Check if GetBackingImageNameFromLvolName is used without validation rg "GetBackingImageNameFromLvolName.*\(" -B 2 -A 2 # Check for direct string splits on lvol names rg "strings\.Split.*lvolName" -B 2 -A 2Length of output: 991
124-146: 🛠️ Refactor suggestion
Consider enhancing the name parsing logic for better robustness
The current implementation has potential issues:
GetBackingImageNameFromLvolNameis fragile as it assumes perfect formatting and could panic if the input doesn't match expected format- The regex pattern could be more specific about allowed characters
Consider these improvements:
func GetBackingImageNameFromLvolName(lvolName string) string { + if !strings.HasPrefix(lvolName, "bi-") { + return "" + } parts := strings.Split(lvolName, "-") + if len(parts) < 2 { + return "" + } return parts[1] } func ExtractBackingImageAndDiskUUID(lvolName string) (string, string, error) { - re := regexp.MustCompile(`^bi-([a-zA-Z0-9-]+)-disk-([a-zA-Z0-9-]+)$`) + // More specific pattern for backing image names and UUIDs + re := regexp.MustCompile(`^bi-([a-zA-Z0-9][-a-zA-Z0-9]*[a-zA-Z0-9])-disk-([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})$`)Committable suggestion skipped: line range outside the PR's diff.
pkg/util/http_handler.go (1)
25-26:
⚠️ Potential issueMismatch between
Handlerinterface andHTTPHandlerimplementationThe
DownloadFromURLmethod in theHandlerinterface accepts afilePath string, whereas the implementation inHTTPHandleraccepts anoutFh *os.File. This mismatch will cause type errors and preventHTTPHandlerfrom properly implementing theHandlerinterface.To fix this issue, update the
Handlerinterface to match the method signature of the implementation:type Handler interface { GetSizeFromURL(url string) (fileSize int64, err error) - DownloadFromURL(ctx context.Context, url, filePath string, updater ProgressUpdater) (written int64, err error) + DownloadFromURL(ctx context.Context, url string, outFh *os.File, updater ProgressUpdater) (written int64, err error) }📝 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.GetSizeFromURL(url string) (fileSize int64, err error) DownloadFromURL(ctx context.Context, url string, outFh *os.File, updater ProgressUpdater) (written int64, err error)pkg/spdk/util.go (4)
167-173: 🛠️ Refactor suggestion
Return an error from
GetSnapXattrinstead of an empty string on failureThe
GetSnapXattrfunction returns an empty string when an error occurs, which makes it difficult to distinguish between an error and a valid empty value.Modify the function to return
(string, error):-func GetSnapXattr(spdkClient *spdkclient.Client, alias, key string) (res string) { +func GetSnapXattr(spdkClient *spdkclient.Client, alias, key string) (string, error) { value, err := spdkClient.BdevLvolGetXattr(alias, key) if err != nil { - return "" + return "", err } - return value + return value, nil }Update all callers of
GetSnapXattrto handle the error appropriately.📝 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.func GetSnapXattr(spdkClient *spdkclient.Client, alias, key string) (string, error) { value, err := spdkClient.BdevLvolGetXattr(alias, key) if err != nil { return "", err } return value, nil }
175-186: 🛠️ Refactor suggestion
Return an error from
GetLvsNameByUUIDinstead of an empty string on failureSimilar to
GetSnapXattr, theGetLvsNameByUUIDfunction returns an empty string when it fails to retrieve the LVS name. This could make it difficult to distinguish between an error and a valid empty string.Modify the function to return
(string, error):-func GetLvsNameByUUID(spdkClient *spdkclient.Client, lvsUUID string) string { +func GetLvsNameByUUID(spdkClient *spdkclient.Client, lvsUUID string) (string, error) { var lvsList []spdktypes.LvstoreInfo lvsList, err := spdkClient.BdevLvolGetLvstore("", lvsUUID) if err != nil { - return "" + return "", err } if len(lvsList) != 1 { // consider it as failed to get the correct lvs - return "" + return "", errors.Errorf("unexpected number of LVS found: %d", len(lvsList)) } return lvsList[0].Name, nil }Ensure that callers of
GetLvsNameByUUIDhandle the returned error appropriately.📝 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.func GetLvsNameByUUID(spdkClient *spdkclient.Client, lvsUUID string) (string, error) { var lvsList []spdktypes.LvstoreInfo lvsList, err := spdkClient.BdevLvolGetLvstore("", lvsUUID) if err != nil { return "", err } if len(lvsList) != 1 { // consider it as failed to get the correct lvs return "", errors.Errorf("unexpected number of LVS found: %d", len(lvsList)) } return lvsList[0].Name, nil }
49-49:
⚠️ Potential issueAvoid wrapping a nil error in
errors.WrapfAt line 49, there's a possibility that
errcould benilif the loops complete without setting an error butsubsystemNQNorcontrollerNameremain empty. Wrapping anilerror can result in a misleading message or a panic.Modify the error handling to create a new error message:
if subsystemNQN == "" || controllerName == "" { - return "", "", errors.Wrapf(err, "timeout connecting target with address %v:%v", srcIP, srcPort) + return "", "", errors.Errorf("timeout connecting to target with address %v:%v", srcIP, srcPort) }Committable suggestion skipped: line range outside the PR's diff.
25-52:
⚠️ Potential issueAdd break statements after successful discovery and connection
In the
connectNVMeTargetfunction, the retry loops for discovering and connecting to the NVMe target lack break statements upon successful operations. This means the loops will continue executing even after a successful discovery or connection, potentially causing unnecessary retries and delays.Apply this diff to add break statements:
func connectNVMeTarget(srcIP string, srcPort int32) (string, string, error) { executor, err := helperutil.NewExecutor(commontypes.ProcDirectory) if err != nil { return "", "", errors.Wrapf(err, "failed to create executor") } subsystemNQN := "" controllerName := "" for r := 0; r < maxNumRetries; r++ { subsystemNQN, err = nvme.DiscoverTarget(srcIP, strconv.Itoa(int(srcPort)), executor) if err != nil { logrus.WithError(err).Errorf("Failed to discover target with address %v:%v", srcIP, srcPort) time.Sleep(retryInterval) continue } + // Break the loop after successful discovery + break } if subsystemNQN == "" { return "", "", errors.Errorf("timeout discovering target with address %v:%v", srcIP, srcPort) } for r := 0; r < maxNumRetries; r++ { controllerName, err = nvme.ConnectTarget(srcIP, strconv.Itoa(int(srcPort)), subsystemNQN, executor) if err != nil { logrus.WithError(err).Errorf("Failed to connect target with address %v:%v", srcIP, srcPort) time.Sleep(retryInterval) continue } + // Break the loop after successful connection + break } if controllerName == "" { return "", "", errors.Errorf("timeout connecting to target with address %v:%v", srcIP, srcPort) } return subsystemNQN, controllerName, nil }Committable suggestion skipped: line range outside the PR's diff.
pkg/spdk/backing_image.go (4)
379-609: 🛠️ Refactor suggestion
Refactor
prepareBackingImageSnapshotto reduce complexityThe
prepareBackingImageSnapshotmethod spans over 230 lines and is quite complex. Consider refactoring it into smaller helper functions to improve readability and maintainability.Possible refactoring suggestions:
- Extract temporary head creation logic: Move the code responsible for creating the temporary head lvol into a separate function.
- Separate preparation methods: Split the logic for
prepareFromURLandprepareFromSyncinto their own dedicated functions if not already done.- Isolate error handling and cleanup: Create helper functions for error handling and cleanup actions performed in the
deferstatements.- Simplify control flow: Review conditional statements and loops to simplify the control flow where possible.
Refactoring will make the codebase easier to maintain and reduce the likelihood of bugs.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 379-609: pkg/spdk/backing_image.go#L379-L609
Complex Method🪛 golangci-lint (1.62.2)
595-595: printf: (*github.com/sirupsen/logrus.Entry).Warnf call needs 2 args but has 3 args
(govet)
596-596: printf: github.com/pkg/errors.Wrapf call needs 2 args but has 3 args
(govet)
656-656:
⚠️ Potential issueCheck error returned by
connectNVMeTargetThe error returned by
connectNVMeTargetis assigned toerrbut not checked, which may lead to unhandled errors.Apply this diff:
- _, _, err = connectNVMeTarget(srcIP, srcPort) + if _, _, err = connectNVMeTarget(srcIP, srcPort); err != nil { + return errors.Wrapf(err, "failed to connect to NVMe target at %v:%v", srcIP, srcPort) + }📝 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.if _, _, err = connectNVMeTarget(srcIP, srcPort); err != nil { return errors.Wrapf(err, "failed to connect to NVMe target at %v:%v", srcIP, srcPort) }🧰 Tools
🪛 golangci-lint (1.62.2)
656-656: ineffectual assignment to err
(ineffassign)
595-596:
⚠️ Potential issueFix format string in log messages
The log messages on lines 595 and 596 have mismatched placeholders and arguments, which can cause runtime errors.
Apply this diff:
- bi.log.WithError(err).Warnf("Failed to prepare the backing image %v by syncing from %v and srcLvsUUID", bi.Name, fromAddress, srcLvsUUID) + bi.log.WithError(err).Warnf("Failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID) - return errors.Wrapf(err, "failed to prepare the backing image %v by syncing from %v and srcLvsUUID", bi.Name, fromAddress, srcLvsUUID) + return errors.Wrapf(err, "failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID)📝 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.bi.log.WithError(err).Warnf("Failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID) return errors.Wrapf(err, "failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID)🧰 Tools
🪛 golangci-lint (1.62.2)
595-595: printf: (*github.com/sirupsen/logrus.Entry).Warnf call needs 2 args but has 3 args
(govet)
596-596: printf: github.com/pkg/errors.Wrapf call needs 2 args but has 3 args
(govet)
124-128:
⚠️ Potential issueCheck error return value of
bi.prepareBackingImageSnapshotThe error returned by
bi.prepareBackingImageSnapshotis not checked, which could lead to unhandled errors during backing image preparation.Apply this diff:
go func() { - bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID) + if err := bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID); err != nil { + bi.log.WithError(err).Errorf("Failed to prepare backing image snapshot") + bi.State = types.BackingImageStateFailed + bi.ErrorMsg = err.Error() + } // update the backing image after preparing bi.UpdateCh <- 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.go func() { if err := bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID); err != nil { bi.log.WithError(err).Errorf("Failed to prepare backing image snapshot") bi.State = types.BackingImageStateFailed bi.ErrorMsg = err.Error() } // update the backing image after preparing bi.UpdateCh <- nil }()🧰 Tools
🪛 golangci-lint (1.62.2)
124-124: Error return value of
bi.prepareBackingImageSnapshotis not checked(errcheck)
pkg/client/client.go (3)
976-979:
⚠️ Potential issueUse the correct request type in 'BackingImageExpose'
In the
BackingImageExposemethod, the requestBackingImageGetRequestis used when callingclient.BackingImageExpose. It should beBackingImageExposeRequestto match the intended operation.Apply this diff to correct the request type:
func (c *SPDKClient) BackingImageExpose(ctx context.Context, name, lvsUUID string) (exposedSnapshotLvolAddress string, err error) { if name == "" || lvsUUID == "" { return "", fmt.Errorf("failed to expose SPDK backing image: missing required parameter") } client := c.getSPDKServiceClient() - resp, err := client.BackingImageExpose(ctx, &spdkrpc.BackingImageGetRequest{ + resp, err := client.BackingImageExpose(ctx, &spdkrpc.BackingImageExposeRequest{ Name: name, LvsUuid: lvsUUID, })📝 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.resp, err := client.BackingImageExpose(ctx, &spdkrpc.BackingImageExposeRequest{ Name: name, LvsUuid: lvsUUID, })
992-995:
⚠️ Potential issueUse the correct request type in 'BackingImageUnexpose'
Similar to the previous issue,
BackingImageGetRequestis used inBackingImageUnexposeinstead of the appropriateBackingImageUnexposeRequest. This may lead to unexpected behavior or errors.Apply this diff to correct the request type:
func (c *SPDKClient) BackingImageUnexpose(ctx context.Context, name, lvsUUID string) error { if name == "" || lvsUUID == "" { return fmt.Errorf("failed to unexpose SPDK backing image: missing required parameter") } client := c.getSPDKServiceClient() - _, err := client.BackingImageUnexpose(ctx, &spdkrpc.BackingImageGetRequest{ + _, err := client.BackingImageUnexpose(ctx, &spdkrpc.BackingImageUnexposeRequest{ Name: name, LvsUuid: lvsUUID, })Committable suggestion skipped: line range outside the PR's diff.
886-906:
⚠️ Potential issueFix the typo in 'backingIamgeUUID' parameter
The parameter
backingIamgeUUIDappears to be misspelled. It should bebackingImageUUID. This typo is present in the function signature and throughout theBackingImageCreatemethod, which could lead to confusion and potential bugs.Apply this diff to correct the typo:
-func (c *SPDKClient) BackingImageCreate(name, backingIamgeUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) { - if name == "" || backingIamgeUUID == "" || checksum == "" || lvsUUID == "" || size == 0 { +func (c *SPDKClient) BackingImageCreate(name, backingImageUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) { + if name == "" || backingImageUUID == "" || checksum == "" || lvsUUID == "" || size == 0 { return nil, fmt.Errorf("failed to create SPDK backing image: missing required parameters") } client := c.getSPDKServiceClient() resp, err := client.BackingImageCreate(ctx, &spdkrpc.BackingImageCreateRequest{ Name: name, - BackingImageUuid: backingIamgeUUID, + BackingImageUuid: backingImageUUID, LvsUuid: lvsUUID, Size: size, Checksum: checksum, FromAddress: fromAddress, SrcLvsUuid: srcLvsUUID, })Committable suggestion skipped: line range outside the PR's diff.
pkg/spdk/server.go (2)
1581-1597: 🛠️ Refactor suggestion
Address the TODO comment in
newBackingImagefunctionThere is a TODO comment at line 1581 indicating that if the backing image already exists, the function should return it directly. Currently, the function checks if the backing image does not exist and creates it if necessary. However, it does not handle the case where the backing image already exists.
Suggestion: Handle existing backing images in
newBackingImageImplement logic to return the existing backing image if it already exists in
s.backingImageMap. This will prevent unnecessary re-creation and ensure consistent management of backing images.Would you like assistance in implementing this logic or should I open a GitHub issue to track this task?
254-296:
⚠️ Potential issueHandling backing images in the
verifymethodThe updated logic in the
verifymethod correctly identifies and reconstructs backing image records from the SPDK client. This ensures that backing images are properly tracked even after restarts or disconnections.However, there is a critical issue at line 280.
Remove ineffectual assignment to
backingImageNameat line 280At line 280, the assignment to
backingImageNameis immediately overwritten at line 281. The initial assignment is redundant and can be removed to clean up the code and prevent potential confusion.Please apply the following diff to remove the redundant assignment:
-backingImageName := GetBackingImageNameFromLvolName(lvolName)This change will resolve the inefficiency reported by static analysis tools.
📝 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.// Backing image lvol name will be "bi-${biName}-disk-${lvsUUID}" // Backing image temp lvol name will be "bi-${biName}-disk-${lvsUUID}-temp-head" for lvolName, bdevLvol := range bdevLvolMap { if bdevLvol.DriverSpecific.Lvol.Snapshot && !IsBackingImageSnapLvolName(lvolName) { continue } if IsBackingImageTempHead(lvolName) { if s.backingImageMap[GetBackingImageSnapLvolNameFromTempHeadLvolName(lvolName)] == nil { lvsUUID := bdevLvol.DriverSpecific.Lvol.LvolStoreUUID CleanupOrphanBackingImageTempHead(spdkClient, lvsUUIDNameMap[lvsUUID], lvolName) } continue } if replicaMap[lvolName] != nil { continue } if s.backingImageMap[lvolName] != nil { continue } if IsRebuildingLvol(lvolName) { if replicaMap[GetReplicaNameFromRebuildingLvolName(lvolName)] != nil { continue } } if IsBackingImageSnapLvolName(lvolName) { lvsUUID := bdevLvol.DriverSpecific.Lvol.LvolStoreUUID backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName) if err != nil { logrus.WithError(err).Errorf("failed to extract backing image name and disk UUID from lvol name %v", lvolName) continue } actualSize := bdevLvol.DriverSpecific.Lvol.NumAllocatedClusters * uint64(defaultClusterSize) alias := bdevLvol.Aliases[0] expectedChecksum := GetSnapXattr(spdkClient, alias, types.BackingImageSnapshotAttrChecksum) backingImageUUID := GetSnapXattr(spdkClient, alias, types.BackingImageSnapshotAttrBackingImageUUID) backingImage := NewBackingImage(s.ctx, backingImageName, backingImageUUID, lvsUUID, actualSize, expectedChecksum, s.updateChs[types.InstanceTypeBackingImage]) backingImage.Alias = alias // For uncahced backing image, we set the state to pending first, so we can distinguish it from the cached but starting backing image backingImage.State = types.BackingImageStatePending backingImageMapForSync[lvolName] = backingImage backingImageMap[lvolName] = backingImage } else {🧰 Tools
🪛 golangci-lint (1.62.2)
280-280: ineffectual assignment to backingImageName
(ineffassign)
pkg/spdk/replica.go (1)
141-145:
⚠️ Potential issueHandle potential errors from
ExtractBackingImageAndDiskUUIDThe call to
ExtractBackingImageAndDiskUUIDmight fail, but its error is currently ignored. It's important to handle this error to avoid unexpected behavior if the extraction fails.Apply this diff to handle the potential error:
if r.BackingImage != nil { - backingImageName, _, _ := ExtractBackingImageAndDiskUUID(r.BackingImage.Name) + backingImageName, _, err := ExtractBackingImageAndDiskUUID(r.BackingImage.Name) + if err != nil { + res.ErrorMsg = fmt.Sprintf("Failed to extract backing image name: %v", err) + } else { + res.BackingImageName = backingImageName + } }Adjust the error handling to fit the application's error propagation strategy.
📝 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.if r.BackingImage != nil { backingImageName, _, err := ExtractBackingImageAndDiskUUID(r.BackingImage.Name) if err != nil { res.ErrorMsg = fmt.Sprintf("Failed to extract backing image name: %v", err) } else { res.BackingImageName = backingImageName } }pkg/spdk/engine.go (3)
705-706:
⚠️ Potential issueAvoid shared mutable state for
hasBackingImageandhasSnapshotThe variables
hasBackingImageandhasSnapshotare declared outside the loop iterating over replicas and are modified within the loop. This could lead to inconsistent states if replicas have different configurations regarding backing images and snapshots.Consider collecting the backing image and snapshot statuses per replica and evaluating the overall state after processing all replicas.
Proposed Refactor:
- hasBackingImage := false - hasSnapshot := false + var hasBackingImage, hasSnapshot bool + var backingImageCount, snapshotCount intWithin the loop:
+ if replica.BackingImageName != "" { + backingImageCount++ + } + if len(replica.Snapshots) > 0 { + snapshotCount++ + }After processing all replicas:
hasBackingImage = backingImageCount > 0 hasSnapshot = snapshotCount > 0
828-840:
⚠️ Potential issueCorrect ancestor name extraction for accurate comparison
In lines 828-840, ancestor names are extracted from logical volume names for comparison. The current extraction may not handle all naming patterns, potentially leading to incorrect conclusions about replica consistency.
Ensure that the functions
IsBackingImageSnapLvolNameandExtractBackingImageAndDiskUUIDcorrectly handle all possible logical volume name formats.Recommendation:
- Review and test
IsBackingImageSnapLvolNameandExtractBackingImageAndDiskUUIDfunctions to ensure they handle all cases.- Add unit tests covering various logical volume name formats to validate the extraction logic.
753-763:
⚠️ Potential issueHandle inconsistent backing image presence across replicas
The current logic assumes that if one replica has a backing image, all replicas should have one. If replicas differ in backing image presence, this could cause inconsistent behavior or errors.
Add validation to ensure all replicas consistently have or do not have a backing image, and handle any inconsistencies appropriately.
Suggested Code Adjustment:
if replica.BackingImageName != "" { hasBackingImage = true + if len(replicaAncestorMap) > 0 && !hasBackingImage { + e.log.Errorf("Replica %s has a backing image, but others do not. Inconsistency detected.", replicaName) + replicaStatus.Mode = types.ModeERR + return + } // existing code... }Committable suggestion skipped: line range outside the PR's diff.
ChanYiLin
force-pushed
the
LH6341_v2_backing_image
branch
from
680d8f2
to
8a65d82
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (24)
pkg/types/types.go (1)
38-49: Consider adding documentation for state transitions.While the states are well-defined, it would be helpful to add documentation that:
- Describes the purpose of each state
- Defines valid state transitions
- Clarifies what triggers state changes
- Explains what 'in-progress' refers to (e.g., download, sync?)
This will help maintainers understand the backing image lifecycle.
Example documentation:
// BackingImageState represents the current state of a backing image. // Valid state transitions are: // pending -> starting -> ready-for-transfer -> ready // Any state can transition to failed or unknown on errors. type BackingImageState stringpkg/util/http_handler.go (5)
15-18: Consider making timeouts configurableThe current 4-second timeout might be too short for large backing image downloads or slow network conditions. Consider:
- Making the timeout configurable through environment variables or configuration
- Using different timeouts for HEAD vs GET requests
- Using a longer default timeout (e.g., 30 seconds)
const ( DownloadBufferSize = 1 << 12 - HTTPTimeout = 4 * time.Second + defaultHTTPTimeout = 30 * time.Second ) + +var HTTPTimeout = getEnvDuration("HTTP_TIMEOUT", defaultHTTPTimeout) + +func getEnvDuration(key string, defaultValue time.Duration) time.Duration { + if v := os.Getenv(key); v != "" { + if d, err := time.ParseDuration(v); err == nil { + return d + } + } + return defaultValue +}
31-91: Consider reusing the HTTP clientCreating a new client for each request can be inefficient. Consider making the client a field of
HTTPHandlerand initializing it once.type HTTPHandler struct { + client http.Client } + +func NewHTTPHandler() *HTTPHandler { + return &HTTPHandler{ + client: NewDownloadHttpClient(), + } +}Then update the method calls:
- client := NewDownloadHttpClient() - resp, err := client.Do(rr) + resp, err := h.client.Do(rr)
93-94: Improve function documentationThe function documentation should explain:
- The purpose of the
writeZeroparameter- The relationship between context cancellation and idle timeout
- The expected behavior when timeout occurs
-// IdleTimeoutCopy relies on ctx of the reader/src or a separate timer to interrupt the processing. +// IdleTimeoutCopy copies data from src to dst with idle timeout monitoring and progress updates. +// It supports two modes of operation: +// - When writeZero is true, all data including zero bytes are written to dst +// - When writeZero is false, zero bytes are skipped by seeking forward instead of writing +// The copy can be interrupted by: +// - Context cancellation from the caller +// - Idle timeout (no data received within HTTPTimeout duration) +// Returns the number of bytes copied and any error that occurred.
133-163: Enhance error handling with more contextThe error handling could be improved to provide more context about what operation failed.
case <-ctx.Done(): - err = fmt.Errorf("context cancelled during the copy") + err = fmt.Errorf("copy interrupted: %w", ctx.Err()) default: // Read will error out once the context is cancelled. nr, rErr = src.Read(buf) if nr > 0 { // Skip writing zero data if !writeZero && bytes.Equal(buf[0:nr], zeroByteArray[0:nr]) { _, handleErr = dst.Seek(int64(nr), io.SeekCurrent) + if handleErr != nil { + handleErr = fmt.Errorf("failed to seek past zero bytes: %w", handleErr) + } nws = int64(nr) } else { nw, handleErr = dst.Write(buf[0:nr]) + if handleErr != nil { + handleErr = fmt.Errorf("failed to write data: %w", handleErr) + } nws = int64(nw) }
20-22: Enhance progress updates for backing image downloadsSince this is used for backing image downloads, consider enhancing the progress updates to include:
- Download speed
- Estimated time remaining
- Current state (connecting, downloading, verifying)
type ProgressUpdater interface { - UpdateProgress(size int64) + UpdateProgress(update ProgressUpdate) +} + +type ProgressUpdate struct { + BytesTransferred int64 + Speed float64 // bytes per second + EstimatedTime time.Duration + State string }pkg/util/util.go (2)
257-260: LGTM! Consider adding documentation comments.The constants are well-named and follow Go conventions. Consider adding documentation comments to explain their purpose and usage context.
const ( + // BackingImageManagerDirectoryName is the standard directory name for storing backing images BackingImageManagerDirectoryName = "backing-images" + // BackingImageFileName is the standard file name for a backing image BackingImageFileName = "backing" )
274-287: Consider buffered reading for large files.While the implementation is correct, reading the entire file at once could be memory-intensive for large backing images. Consider using buffered reading for better memory efficiency.
func GetFileChecksum(filePath string) (string, error) { f, err := os.Open(filePath) if err != nil { return "", err } defer f.Close() h := sha512.New() - if _, err := io.Copy(h, f); err != nil { + buf := make([]byte, 1024*1024) // 1MB buffer + if _, err := io.CopyBuffer(h, f, buf); err != nil { return "", err } return hex.EncodeToString(h.Sum(nil)), nil }pkg/spdk/util.go (2)
167-173: Consider improving error handling in GetSnapXattrThe function silently fails by returning an empty string on error, which could make debugging difficult. Consider either:
- Returning the error to let callers handle it appropriately, or
- Adding error logging to help with troubleshooting
-func GetSnapXattr(spdkClient *spdkclient.Client, alias, key string) (res string) { +func GetSnapXattr(spdkClient *spdkclient.Client, alias, key string) (string, error) { value, err := spdkClient.BdevLvolGetXattr(alias, key) if err != nil { - return "" + return "", errors.Wrapf(err, "failed to get xattr for alias %s key %s", alias, key) } - return value + return value, nil }
175-186: Enhance error handling and input validation in GetLvsNameByUUIDThe function has several areas for improvement:
- Silent error handling could mask issues
- No validation of the UUID parameter
- The error case for multiple LVS entries could benefit from more context
-func GetLvsNameByUUID(spdkClient *spdkclient.Client, lvsUUID string) string { +func GetLvsNameByUUID(spdkClient *spdkclient.Client, lvsUUID string) (string, error) { + if lvsUUID == "" { + return "", errors.New("lvsUUID cannot be empty") + } + var lvsList []spdktypes.LvstoreInfo lvsList, err := spdkClient.BdevLvolGetLvstore("", lvsUUID) if err != nil { - return "" + return "", errors.Wrapf(err, "failed to get lvstore for UUID %s", lvsUUID) } if len(lvsList) != 1 { - // consider it as failed to get the correct lvs - return "" + return "", errors.Errorf("expected exactly one lvstore for UUID %s, got %d", lvsUUID, len(lvsList)) } - return lvsList[0].Name + return lvsList[0].Name, nil }pkg/spdk/types.go (2)
52-73: LGTM: Well-structured conversion function with clear commentsThe function correctly handles backing image specific cases and follows the established pattern. Consider adding a brief function documentation comment to explain its purpose and relationship with
ServiceLvolToProtoLvol.Add a documentation comment:
+// ServiceBackingImageLvolToProtoBackingImageLvol converts an internal Lvol representing +// a backing image to its protobuf representation, handling backing image specific cases +// such as no parent and creation time based metadata. func ServiceBackingImageLvolToProtoBackingImageLvol(lvol *Lvol) *spdkrpc.Lvol {
129-166: LGTM: Well-implemented utility functions with robust validationThe utility functions are well-structured and the regex-based validation in
ExtractBackingImageAndDiskUUIDprovides robust parsing. Consider adding documentation comments to explain the expected format and usage of these functions.Add documentation comments to clarify the expected formats:
+// ExtractBackingImageAndDiskUUID extracts components from a backing image lvol name +// with format "bi-<backing-image-name>-disk-<disk-uuid>". +// Returns the backing image name and disk UUID, or an error if the format is invalid. func ExtractBackingImageAndDiskUUID(lvolName string) (string, string, error) { +// GetBackingImageSnapLvolName generates a backing image snapshot lvol name +// with format "bi-<backing-image-name>-disk-<lvs-uuid>". func GetBackingImageSnapLvolName(backingImageName string, lvsUUID string) string { +// IsBackingImageSnapLvolName returns true if the lvol name represents a backing +// image snapshot (starts with "bi-" and is not a temp head). func IsBackingImageSnapLvolName(lvolName string) bool {pkg/api/types.go (1)
195-227: Consider adding nil checks in conversion functionsWhile the conversion functions are well-implemented, they might benefit from nil checks to prevent panics when handling nil inputs.
func ProtoBackingImageToBackingImage(bi *spdkrpc.BackingImage) *BackingImage { + if bi == nil { + return nil + } res := &BackingImage{ Name: bi.Name, // ... rest of the implementation } return res } func BackingImageToProtoBackingImage(bi *BackingImage) *spdkrpc.BackingImage { + if bi == nil { + return nil + } return &spdkrpc.BackingImage{ Name: bi.Name, // ... rest of the implementation } }pkg/spdk/server.go (2)
282-285: Enhance error handling for backing image extractionThe error handling could be more informative by including the actual error message from
ExtractBackingImageAndDiskUUID.Apply this diff to improve error handling:
if err != nil { - logrus.WithError(err).Errorf("failed to extract backing image name and disk UUID from lvol name %v", lvolName) + logrus.WithError(err).Errorf("failed to extract backing image name and disk UUID from lvol name %v: %v", lvolName, err) continue }
1600-1614: Consider extracting common validation logicThe input validation logic is repeated across multiple RPC methods. Consider extracting it into a helper function to improve maintainability and reduce duplication.
Example helper function:
func validateBackingImageRequest(name, lvsUUID string) error { if name == "" { return grpcstatus.Error(grpccodes.InvalidArgument, "backing image name is required") } if lvsUUID == "" { return grpcstatus.Error(grpccodes.InvalidArgument, "lvs UUID is required") } return nil }This can be used in each RPC method to reduce duplication:
func (s *Server) BackingImageGet(ctx context.Context, req *spdkrpc.BackingImageGetRequest) (*spdkrpc.BackingImage, error) { if err := validateBackingImageRequest(req.Name, req.LvsUuid); err != nil { return nil, err } // Rest of the implementation... }Also applies to: 1635-1640, 1665-1670, 1751-1756, 1775-1780
pkg/spdk/engine.go (4)
Line range hint
705-814: LGTM! Consider enhancing error handling for backing image operations.The new ancestor determination hierarchy (backing image → snapshots → head) is well-structured. However, the error handling for backing image operations could be enhanced.
Consider adding more detailed error messages when backing image operations fail, for example:
- e.log.Warnf("Failed to get backing image %s with disk UUID %s from replica %s head parent %s, will mark the mode from %v to ERR and continue info update for other replicas", replica.BackingImageName, replica.LvsUUID, replicaName, replica.Head.Parent, replicaStatus.Mode) + e.log.WithError(err).Warnf("Failed to get backing image %s with disk UUID %s from replica %s head parent %s: %v. Will mark the mode from %v to ERR and continue info update for other replicas", replica.BackingImageName, replica.LvsUUID, replicaName, replica.Head.Parent, err, replicaStatus.Mode)
828-840: LGTM! Consider extracting name comparison logic.The backing image snapshot name handling is correct and necessary. However, the name comparison logic could be made more maintainable.
Consider extracting the ancestor name comparison logic into a helper function:
+ func getComparableAncestorName(ancestorName string) string { + if IsBackingImageSnapLvolName(ancestorName) { + name, _, _ := ExtractBackingImageAndDiskUUID(ancestorName) + return name + } + return ancestorName + } if candidateReplicaName != "" { - candidateReplicaAncestorName := replicaAncestorMap[candidateReplicaName].Name - currentReplicaAncestorName := ancestorApiLvol.Name - if IsBackingImageSnapLvolName(candidateReplicaAncestorName) { - candidateReplicaAncestorName, _, _ = ExtractBackingImageAndDiskUUID(candidateReplicaAncestorName) - } - if IsBackingImageSnapLvolName(currentReplicaAncestorName) { - currentReplicaAncestorName, _, _ = ExtractBackingImageAndDiskUUID(currentReplicaAncestorName) - } + candidateReplicaAncestorName := getComparableAncestorName(replicaAncestorMap[candidateReplicaName].Name) + currentReplicaAncestorName := getComparableAncestorName(ancestorApiLvol.Name)
1336-1337: LGTM! Consider adding documentation for backing image snapshot handling.The logic for handling backing image snapshots as valid ancestors is correct, but could benefit from additional documentation.
Consider adding a comment explaining the backing image snapshot handling:
+ // If parent is empty, it's the root snapshot. + // If parent is a backing image snapshot, it's also considered a root since + // backing images serve as the base for the snapshot chain. if snapApiLvol.Parent == "" || IsBackingImageSnapLvolName(snapApiLvol.Parent) { ancestorSnapshotName = snapshotName }
Line range hint
1-1800: Overall implementation looks good!The changes successfully implement backing image support in the SPDK engine while maintaining backward compatibility. The implementation follows a clear hierarchy (backing image → snapshots → head) and handles all edge cases appropriately.
For future improvements, consider:
- Implementing a more comprehensive error handling strategy for backing image operations
- Adding more detailed logging for debugging backing image-related issues
- Adding metrics/telemetry for backing image operations to help monitor performance and issues in production
pkg/spdk/replica.go (5)
53-53: Add missing documentation for the newBackingImagefieldThe
BackingImagefield in theReplicastruct lacks a comment explaining its purpose. Adding a comment will improve code readability and maintainability.Suggested addition:
+ // BackingImage represents the backing image logical volume associated with the replica BackingImage *Lvol
202-202: Fix typo in comment for better readabilityThere's a typographical error in the comment: 'habe' should be 'have'.
Suggested correction:
- // it is okay to habe backing image snapshot in the results, because we exclude it when finding root or construct the snapshot map + // it is okay to have backing image snapshots in the results because we exclude them when finding the root or constructing the snapshot map
344-345: Clarify why backing image comparison is skippedThe comment
// no need to compare the backing imagecould be expanded to explain why comparing the backing image is unnecessary, aiding future maintainers in understanding the logic.Suggested revision:
- // no need to compare the backing image + // Skipping comparison for backing image since it's not modified during validation
1966-1967: Adjust log message to match log level for consistencyThe log message starts with
[DEBUG]but usesErrorflog level, which can be confusing. Consider removing[DEBUG]or changing the log level to match.Suggested change:
- logrus.WithError(err).Errorf("[DEBUG] failed to resize the rebuildingLvolUUID %v to size %v", rebuildingLvolUUID, r.SpecSize) + logrus.WithError(err).Errorf("Failed to resize the rebuildingLvolUUID %v to size %v", rebuildingLvolUUID, r.SpecSize)
1252-1252: Improve error message for clarityThe error message can be rephrased for better clarity and readability.
Suggested correction:
- return "", fmt.Errorf("cannot find snapshot %s for the replica %s rebuilding src start", exposedSnapshotName, r.Name) + return "", fmt.Errorf("cannot find snapshot %s for replica %s during rebuilding source start", exposedSnapshotName, r.Name)
🛑 Comments failed to post (11)
pkg/util/http_handler.go (1)
176-187:
⚠️ Potential issueAdd security measures for file downloads
The current implementation lacks several security measures:
- URL validation to prevent SSRF attacks
- Maximum file size limits
- TLS configuration for secure downloads
Consider implementing these security measures:
+const ( + MaxFileSize = 1 << 30 // 1GB limit +) + +func validateURL(url string) error { + parsed, err := url.Parse(url) + if err != nil { + return fmt.Errorf("invalid URL: %w", err) + } + if parsed.Scheme != "https" && parsed.Scheme != "http" { + return fmt.Errorf("unsupported URL scheme: %s", parsed.Scheme) + } + return nil +} + func NewDownloadHttpClient() http.Client { return http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, + }, + }, CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) > 10 { + return fmt.Errorf("too many redirects") + } removeReferer(req) return nil }, } }Then update
GetSizeFromURL:func (h *HTTPHandler) GetSizeFromURL(url string) (size int64, err error) { + if err := validateURL(url); err != nil { + return 0, err + } + // ... existing code ... + if size > MaxFileSize { + return 0, fmt.Errorf("file size %d exceeds maximum allowed size %d", size, MaxFileSize) + }Committable suggestion skipped: line range outside the PR's diff.
pkg/spdk/types.go (1)
124-127:
⚠️ Potential issueReplace index-based splitting with regex extraction
The current implementation assumes the backing image name is always at index 1 after splitting by "-", which is fragile and could break if the backing image name contains hyphens.
Use the more robust regex approach from
ExtractBackingImageAndDiskUUID:func GetBackingImageNameFromLvolName(lvolName string) string { - return strings.Split(lvolName, "-")[1] + backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName) + if err != nil { + return "" + } + return backingImageName }📝 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.func GetBackingImageNameFromLvolName(lvolName string) string { // lvolName is in the format of `bi-<backing image name>-disk-<lvs uuid>` backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName) if err != nil { return "" } return backingImageName }pkg/client/client.go (2)
976-979: 🛠️ Refactor suggestion
Use appropriate request types for expose/unexpose operations.
Both
BackingImageExposeandBackingImageUnexposeare usingBackingImageGetRequest, which seems inappropriate for these operations. Consider creating specific request types likeBackingImageExposeRequestandBackingImageUnexposeRequestto better represent the intent of these operations.Also applies to: 992-995
885-885:
⚠️ Potential issueFix parameter name typo.
The parameter name contains a typo:
backingIamgeUUIDshould bebackingImageUUID.-func (c *SPDKClient) BackingImageCreate(name, backingIamgeUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) { +func (c *SPDKClient) BackingImageCreate(name, backingImageUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) {📝 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.func (c *SPDKClient) BackingImageCreate(name, backingImageUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) {pkg/spdk/server.go (1)
280-281:
⚠️ Potential issueFix ineffectual assignment to backingImageName
The assignment to
backingImageNameon line 280 is immediately overwritten by theExtractBackingImageAndDiskUUIDcall.Apply this diff to fix the ineffectual assignment:
-backingImageName := GetBackingImageNameFromLvolName(lvolName) -backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName) +backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName)📝 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.backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName)🧰 Tools
🪛 golangci-lint (1.62.2)
280-280: ineffectual assignment to backingImageName
(ineffassign)
pkg/spdk/backing_image.go (4)
379-609: 🛠️ Refactor suggestion
Refactor
prepareBackingImageSnapshotto reduce complexityThe
prepareBackingImageSnapshotmethod is overly complex and spans over 200 lines, which makes it difficult to read, understand, and maintain. Consider refactoring it by extracting smaller helper functions that handle specific tasks within the method. This will improve readability and maintainability.🧰 Tools
🪛 golangci-lint (1.62.2)
595-595: printf: (*github.com/sirupsen/logrus.Entry).Warnf call needs 2 args but has 3 args
(govet)
596-596: printf: github.com/pkg/errors.Wrapf call needs 2 args but has 3 args
(govet)
🪛 GitHub Check: CodeFactor
[notice] 379-609: pkg/spdk/backing_image.go#L379-L609
Complex Method
656-656:
⚠️ Potential issueCheck the error returned by
connectNVMeTargetThe error returned by
connectNVMeTargetis assigned toerrbut not checked. This can result in silent failures if the connection process encounters an error.Apply this diff to handle the error:
_, _, err = connectNVMeTarget(srcIP, srcPort) +if err != nil { + return errors.Wrapf(err, "failed to connect to NVMe target at %s:%d", srcIP, srcPort) +}📝 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._, _, err = connectNVMeTarget(srcIP, srcPort) if err != nil { return errors.Wrapf(err, "failed to connect to NVMe target at %s:%d", srcIP, srcPort) }🧰 Tools
🪛 golangci-lint (1.62.2)
656-656: ineffectual assignment to err
(ineffassign)
595-596:
⚠️ Potential issueCorrect format strings in logging and error wrapping
The
WarnfandWrapffunctions have format strings that do not match the number of arguments provided. This can cause runtime errors due to incorrect formatting.Apply this diff to fix the format strings:
- bi.log.WithError(err).Warnf("Failed to prepare the backing image %v by syncing from %v and srcLvsUUID", bi.Name, fromAddress, srcLvsUUID) + bi.log.WithError(err).Warnf("Failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID) - return errors.Wrapf(err, "failed to prepare the backing image %v by syncing from %v and srcLvsUUID", bi.Name, fromAddress, srcLvsUUID) + return errors.Wrapf(err, "failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID)📝 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.bi.log.WithError(err).Warnf("Failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID) return errors.Wrapf(err, "failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID)🧰 Tools
🪛 golangci-lint (1.62.2)
595-595: printf: (*github.com/sirupsen/logrus.Entry).Warnf call needs 2 args but has 3 args
(govet)
596-596: printf: github.com/pkg/errors.Wrapf call needs 2 args but has 3 args
(govet)
124-124:
⚠️ Potential issueHandle the error returned by
bi.prepareBackingImageSnapshotThe error returned by
bi.prepareBackingImageSnapshotis not checked, which can lead to unhandled errors and unexpected behavior. Ensuring that errors are properly handled improves the robustness of the code.Apply this diff to fix the issue:
go func() { - bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID) + if err := bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID); err != nil { + bi.log.WithError(err).Error("Failed to prepare backing image snapshot") + bi.Lock() + bi.State = types.BackingImageStateFailed + bi.ErrorMsg = err.Error() + bi.Unlock() + bi.UpdateCh <- nil + } }()Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
124-124: Error return value of
bi.prepareBackingImageSnapshotis not checked(errcheck)
pkg/spdk/replica.go (2)
141-144:
⚠️ Potential issueHandle error returned by
ExtractBackingImageAndDiskUUIDto prevent potential runtime issuesIn the
ServiceReplicaToProtoReplicafunction, the error returned byExtractBackingImageAndDiskUUIDis ignored. If an error occurs, it could lead to incorrectBackingImageNameor unexpected behavior.Suggested fix:
if r.BackingImage != nil { - backingImageName, _, _ := ExtractBackingImageAndDiskUUID(r.BackingImage.Name) + backingImageName, _, err := ExtractBackingImageAndDiskUUID(r.BackingImage.Name) + if err != nil { + return res // or handle the error appropriately + } res.BackingImageName = backingImageName }Committable suggestion skipped: line range outside the PR's diff.
522-532:
⚠️ Potential issueEnsure
r.ActiveChainis properly initialized before accessing index 0In the
prepareHeadfunction,r.ActiveChain[0]is accessed without checking ifr.ActiveChainhas at least one element. This could lead to an index out of range error ifr.ActiveChainis empty.Suggested fix:
if backingImage != nil { + if len(r.ActiveChain) == 0 { + r.ActiveChain = []*Lvol{nil} + } r.ActiveChain[0] = backingImage.Snapshot r.BackingImage = r.ActiveChain[0] }📝 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.func (r *Replica) prepareHead(spdkClient *spdkclient.Client, backingImage *BackingImage) (err error) { isHeadAvailable, err := r.IsHeadAvailable(spdkClient) if err != nil { return err } if backingImage != nil { if len(r.ActiveChain) == 0 { r.ActiveChain = []*Lvol{nil} } r.ActiveChain[0] = backingImage.Snapshot r.BackingImage = r.ActiveChain[0] }
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (17)
pkg/util/http_handler.go (1)
15-18: Consider making buffer size and timeout configurableThe hardcoded values might not be optimal for all scenarios:
- 4KB buffer size could be small for large file downloads
- 4-second timeout might be insufficient for large files or slow networks
Consider making these configurable:
const ( - DownloadBufferSize = 1 << 12 - HTTPTimeout = 4 * time.Second + DefaultDownloadBufferSize = 1 << 12 + DefaultHTTPTimeout = 4 * time.Second ) +type Config struct { + DownloadBufferSize int + HTTPTimeout time.Duration +}pkg/util/util.go (1)
274-287: Consider performance optimizations and code deduplication.The checksum implementation has two potential improvements:
- For large files, consider adding context support for cancellation and progress tracking
- Consider consolidating with
GetFileChunkChecksumfunction by making the chunk parameters optionalExample refactor:
-func GetFileChecksum(filePath string) (string, error) { +func GetFileChecksum(filePath string, opts ...ChecksumOption) (string, error) { + options := &checksumOptions{ + start: 0, + size: -1, // -1 indicates full file + ctx: context.Background(), + } + for _, opt := range opts { + opt(options) + } + f, err := os.Open(filePath) if err != nil { return "", err } defer f.Close() + if options.start > 0 { + if _, err = f.Seek(options.start, 0); err != nil { + return "", err + } + } + h := sha512.New() - if _, err := io.Copy(h, f); err != nil { + reader := io.Reader(f) + if options.size >= 0 { + reader = io.LimitReader(f, options.size) + } + + if _, err := io.Copy(h, reader); err != nil { return "", err } return hex.EncodeToString(h.Sum(nil)), nil }This would allow:
// Full file checksum checksum, err := GetFileChecksum(path) // Chunk checksum (equivalent to current GetFileChunkChecksum) checksum, err := GetFileChecksum(path, WithStart(start), WithSize(size))pkg/spdk/util.go (2)
167-173: Consider improving error handling.The function silently returns an empty string for all error cases, which could make debugging difficult. Consider either:
- Returning the error to allow callers to handle it appropriately, or
- Logging the error before returning empty string
Suggested improvement:
-func GetSnapXattr(spdkClient *spdkclient.Client, alias, key string) (res string) { +func GetSnapXattr(spdkClient *spdkclient.Client, alias, key string) (string, error) { value, err := spdkClient.BdevLvolGetXattr(alias, key) if err != nil { - return "" + return "", errors.Wrapf(err, "failed to get xattr %s for %s", key, alias) } - return value + return value, nil }
175-186: Enhance error handling and input validation.The function has several areas that could be improved:
- Silent error handling makes debugging difficult
- The comment on line 182 could be more descriptive
- Missing input validation for UUID
Suggested improvement:
-func GetLvsNameByUUID(spdkClient *spdkclient.Client, lvsUUID string) string { +func GetLvsNameByUUID(spdkClient *spdkclient.Client, lvsUUID string) (string, error) { + if lvsUUID == "" { + return "", errors.New("lvs UUID cannot be empty") + } + var lvsList []spdktypes.LvstoreInfo lvsList, err := spdkClient.BdevLvolGetLvstore("", lvsUUID) if err != nil { - return "" + return "", errors.Wrapf(err, "failed to get lvstore for UUID %s", lvsUUID) } if len(lvsList) != 1 { - // consider it as failed to get the correct lvs - return "" + return "", errors.Errorf("expected exactly one lvstore for UUID %s, got %d", lvsUUID, len(lvsList)) } - return lvsList[0].Name + return lvsList[0].Name, nil }pkg/api/types.go (1)
181-193: Consider adding field documentationWhile the struct fields are well-organized and cover all necessary aspects of backing image management, adding documentation for each field would improve clarity and maintainability.
Example documentation style:
type BackingImage struct { + // Name is the unique identifier for the backing image Name string `json:"name"` + // BackingImageUUID is the UUID assigned to the backing image BackingImageUUID string `json:"backing_image_uuid"` // ... (continue for other fields) }pkg/spdk/backing_image.go (1)
339-339: Standardize error logging formatError logging format is inconsistent across the file. Some use
WithError(err).Errorf()while others directly useErrorf(). Standardize the approach for better maintainability.- logrus.Errorf("Failed to unexpose lvol bdev %v", backingImageSnapLvolName) + logrus.WithError(err).Errorf("Failed to unexpose lvol bdev %v", backingImageSnapLvolName) - logrus.Errorf("Failed to release port %v after failed to create backing image", bi.Port) + logrus.WithError(err).Errorf("Failed to release port %v after failed to create backing image", bi.Port)Also applies to: 344-344, 392-392, 398-398, 502-502
pkg/client/client.go (4)
Line range hint
56-71: Consider validating the backingImageName parameter.While the function correctly handles the new
backingImageNameparameter, consider adding validation when it's non-empty to ensure it meets any naming conventions or requirements.func (c *SPDKClient) ReplicaCreate(name, lvsName, lvsUUID string, specSize uint64, portCount int32, backingImageName string) (*api.Replica, error) { if name == "" || lvsName == "" || lvsUUID == "" { return nil, fmt.Errorf("failed to start SPDK replica: missing required parameters") } + if backingImageName != "" { + if err := validateBackingImageName(backingImageName); err != nil { + return nil, fmt.Errorf("failed to start SPDK replica: invalid backing image name: %v", err) + } + }
885-906: Fix typo in parameter name 'backingIamgeUUID'.There's a typo in the parameter name that should be corrected for better code readability.
-func (c *SPDKClient) BackingImageCreate(name, backingIamgeUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) { +func (c *SPDKClient) BackingImageCreate(name, backingImageUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) { - if name == "" || backingIamgeUUID == "" || checksum == "" || lvsUUID == "" || size == 0 { + if name == "" || backingImageUUID == "" || checksum == "" || lvsUUID == "" || size == 0 {
885-906: Enhance error messages with specific validation details.The error messages could be more specific about which parameter is missing. This would make debugging easier.
Example improvement:
func (c *SPDKClient) BackingImageCreate(name, backingImageUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) { - if name == "" || backingImageUUID == "" || checksum == "" || lvsUUID == "" || size == 0 { - return nil, fmt.Errorf("failed to start SPDK backing image: missing required parameters") + var missing []string + if name == "" { + missing = append(missing, "name") + } + if backingImageUUID == "" { + missing = append(missing, "backingImageUUID") + } + if checksum == "" { + missing = append(missing, "checksum") + } + if lvsUUID == "" { + missing = append(missing, "lvsUUID") + } + if size == 0 { + missing = append(missing, "size") + } + if len(missing) > 0 { + return nil, fmt.Errorf("failed to start SPDK backing image: missing required parameters: %s", strings.Join(missing, ", "))Also applies to: 908-922, 924-941
970-984: Consider adding timeout to context in BackingImageExpose.While the function accepts a context parameter, it might be good to add a timeout to prevent indefinite waiting, similar to other operations in the codebase.
func (c *SPDKClient) BackingImageExpose(ctx context.Context, name, lvsUUID string) (exposedSnapshotLvolAddress string, err error) { if name == "" || lvsUUID == "" { return "", fmt.Errorf("failed to expose SPDK backing image: missing required parameter") } + ctx, cancel := context.WithTimeout(ctx, GRPCServiceTimeout) + defer cancel() + client := c.getSPDKServiceClient()pkg/spdk/server.go (4)
51-53: Consider potential race conditions in map initializationThe
backingImageMapis initialized without synchronization. While the RWMutex provides synchronization for subsequent operations, there's a brief window during initialization where race conditions could occur if multiple goroutines attempt to access the map simultaneously.Consider using sync.Map or ensuring proper synchronization during initialization.
Also applies to: 99-100
1582-1597: Enhance validation in newBackingImage functionThe function should validate the size parameter to ensure it's within acceptable bounds. Also, consider adding validation for the checksum format.
Consider adding these validations:
func (s *Server) newBackingImage(req *spdkrpc.BackingImageCreateRequest) (*BackingImage, error) { + if req.Size > MaxBackingImageSize { + return nil, fmt.Errorf("backing image size %d exceeds maximum allowed size %d", req.Size, MaxBackingImageSize) + } + if !IsValidChecksum(req.Checksum) { + return nil, fmt.Errorf("invalid checksum format: %s", req.Checksum) + } s.Lock() defer s.Unlock() ... }
1634-1662: Improve error handling in BackingImageDeleteThe error handling could be more specific about the failure reasons and provide more context for debugging.
Consider enhancing the error handling:
func (s *Server) BackingImageDelete(ctx context.Context, req *spdkrpc.BackingImageDeleteRequest) (ret *emptypb.Empty, err error) { ... if bi != nil { if err := bi.Delete(spdkClient, s.portAllocator); err != nil { - return nil, grpcstatus.Error(grpccodes.Internal, errors.Wrapf(err, "failed to delete backing image %v in lvs %v", req.Name, req.LvsUuid).Error()) + return nil, grpcstatus.Error(grpccodes.Internal, errors.Wrapf(err, + "failed to delete backing image %v in lvs %v: %v (state: %v)", + req.Name, req.LvsUuid, err, bi.State).Error()) } } ... }
1581-1581: Address TODO comment regarding duplicate handlingThe TODO comment suggests that duplicate handling needs to be implemented. This is important for preventing race conditions and ensuring data consistency.
Would you like me to help implement the duplicate handling logic or create a GitHub issue to track this task?
pkg/spdk/replica.go (1)
1964-1970: Improve error loggingThe debug log should be improved to use structured logging consistent with the rest of the codebase.
- logrus.WithError(err).Errorf("[DEBUG] failed to resize the rebuildingLvolUUID %v to size %v", rebuildingLvolUUID, r.SpecSize) + r.log.WithError(err).Errorf("Failed to resize rebuilding lvol %v to size %v", rebuildingLvolUUID, r.SpecSize)pkg/spdk/engine.go (2)
766-783: Consider adding debug logging for replica state transitionsThe logic for handling replicas with/without backing images and snapshots is correct, but adding debug logging would help with troubleshooting.
if len(replica.Snapshots) > 0 { + e.log.Debugf("Replica %s has %d snapshots", replicaName, len(replica.Snapshots)) if hasBackingImage { + e.log.Debugf("Replica %s: skipping snapshots as backing image exists", replicaName) e.log.Warnf("Engine found replica %s does not have a backing image while other replicas have during info update for other replicas", replicaName) } else { hasSnapshot = true
828-840: Enhance ancestor comparison logicThe ancestor comparison logic is solid but could benefit from some refactoring to improve readability and maintainability.
Consider extracting the ancestor name comparison into a separate helper function:
+func getComparableAncestorName(ancestorName string) string { + if IsBackingImageSnapLvolName(ancestorName) { + name, _, _ := ExtractBackingImageAndDiskUUID(ancestorName) + return name + } + return ancestorName +} if candidateReplicaName != "" { - candidateReplicaAncestorName := replicaAncestorMap[candidateReplicaName].Name - currentReplicaAncestorName := ancestorApiLvol.Name - if IsBackingImageSnapLvolName(candidateReplicaAncestorName) { - candidateReplicaAncestorName, _, _ = ExtractBackingImageAndDiskUUID(candidateReplicaAncestorName) - } - if IsBackingImageSnapLvolName(currentReplicaAncestorName) { - currentReplicaAncestorName, _, _ = ExtractBackingImageAndDiskUUID(currentReplicaAncestorName) - } + candidateReplicaAncestorName := getComparableAncestorName(replicaAncestorMap[candidateReplicaName].Name) + currentReplicaAncestorName := getComparableAncestorName(ancestorApiLvol.Name)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (179)
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/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/munnerz/goautoneg/LICENSEis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/Makefileis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/README.txtis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/autoneg.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/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 (13)
pkg/api/types.go(6 hunks)pkg/client/client.go(3 hunks)pkg/spdk/backing_image.go(1 hunks)pkg/spdk/backup.go(1 hunks)pkg/spdk/engine.go(5 hunks)pkg/spdk/replica.go(11 hunks)pkg/spdk/server.go(15 hunks)pkg/spdk/types.go(4 hunks)pkg/spdk/util.go(2 hunks)pkg/spdk_test.go(9 hunks)pkg/types/types.go(1 hunks)pkg/util/http_handler.go(1 hunks)pkg/util/util.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/spdk/backup.go
- pkg/spdk/types.go
- pkg/spdk_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/spdk/backing_image.go
124-124: Error return value of bi.prepareBackingImageSnapshot is not checked
(errcheck)
656-656: ineffectual assignment to err
(ineffassign)
pkg/spdk/server.go
280-280: ineffectual assignment to backingImageName
(ineffassign)
🪛 GitHub Check: CodeFactor
pkg/spdk/backing_image.go
[notice] 379-609: pkg/spdk/backing_image.go#L379-L609
Complex Method
pkg/util/http_handler.go
[notice] 94-167: pkg/util/http_handler.go#L94-L167
Complex Method
🔇 Additional comments (22)
pkg/types/types.go (2)
33-36: LGTM! Instance type addition follows consistent patterns
The new InstanceTypeBackingImage constant follows the established naming conventions and is properly integrated with existing instance types.
51-57: Verify the implications of hardcoded port count
While the snapshot attribute constants are well-defined, the hardcoded BackingImagePortCount = 1 might need verification:
- Is this a limitation of the SPDK implementation?
- What happens if we need to support multiple ports in the future?
- Are there any performance implications?
Let's check the usage of this constant:
✅ Verification successful
Let me gather more context about the port allocation and SPDK implementation:
Based on the analysis of the code, I can now provide a final response:
Hardcoded port count is intentional and safe for backing image operations
The BackingImagePortCount = 1 is correctly hardcoded as backing images only need a single port for their operations:
- It's used exclusively for exposing backing images during syncing/preparation via
BackingImageExposeandprepareBackingImageSnapshot - Unlike replicas which need multiple ports (one for expose and rest for rebuilding), backing images only require a single port for their SPDK NVMe-oF exposure
- The port is properly allocated and released through the
superiorPortAllocator, ensuring no port conflicts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage and implications of BackingImagePortCount
# Search for BackingImagePortCount usage
rg "BackingImagePortCount" --type go -A 5
# Look for any port-related configuration or limitations in documentation
fd -e md -e txt | xargs rg -l "port.*configuration|port.*limit"
Length of output: 1171
Script:
#!/bin/bash
# Search for port allocator implementation and usage
rg "superiorPortAllocator" --type go -A 5
# Look for SPDK related port configuration
rg "port.*spdk|spdk.*port" --type go
# Search for any comments or documentation about port usage
ast-grep --pattern 'comment { $_ port $$ }'
Length of output: 26000
pkg/util/http_handler.go (1)
31-63: Consider handling redirects in GetSizeFromURL
While the method correctly uses NewDownloadHttpClient() which handles redirects, the size might change after redirects. Consider following redirects manually for size verification.
Let's verify if there are any redirect-related issues in the codebase:
pkg/util/util.go (2)
257-260: LGTM! Well-defined constants for backing image paths.
The constants follow Go naming conventions and provide clear, self-documenting values for backing image paths.
266-272: Verify path construction usage in backing image operations.
Let's verify how these path utilities are used in the backing image creation workflows.
✅ Verification successful
Path construction utilities are correctly implemented and safely used
Based on the verification results:
- The path construction utilities (
GetBackingImageDirectoryandGetBackingImageFilePath) are only used within the codebase for constructing paths in a controlled manner usingfilepath.Join. - The backing image feature has a well-defined structure with proper path handling:
- Files are stored under a dedicated directory (
backing-images) - Each backing image has its unique directory based on name and UUID
- The actual backing image file has a fixed name (
backing)
- Files are stored under a dedicated directory (
- The implementation follows secure path construction practices:
- Uses
filepath.Joinfor safe path concatenation - Includes unique identifiers (UUID) to prevent conflicts
- Has a clear, predictable directory structure
- Uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of backing image path construction in the codebase
# Check backing image path construction usage
echo "Checking backing image path construction usage..."
rg -l "GetBackingImageDirectory|GetBackingImageFilePath" --type go
# Check for potential path traversal prevention
echo "Checking path traversal prevention..."
rg "filepath\.Clean|filepath\.Rel" --type go -C 3
Length of output: 3114
Script:
#!/bin/bash
# Let's check for the actual usage of these path utilities in the codebase
echo "Checking the implementation and usage context..."
rg "GetBackingImageDirectory|GetBackingImageFilePath" --type go -C 5
# Also check for any backing image related operations
echo "Checking backing image operations..."
rg "BackingImage" --type go -C 3 | grep -E "func |type |struct"
Length of output: 26606
pkg/spdk/util.go (2)
18-22: LGTM!
The new imports are appropriate for the added functionality and follow the project's import organization pattern.
Line range hint 25-186: Verify integration with backing image components.
The new utility functions support the backing image feature, but let's verify their integration with other components.
✅ Verification successful
Let me gather more context about the integration by checking the implementation details in the related files.
Based on the verification results, I can now provide the final response:
Integration with backing image components is properly implemented
The utility functions are well-integrated within the codebase:
connectNVMeTargetis used in backing image operations for NVMe initiator setupGetSnapXattris properly used for retrieving backing image metadata (checksum, UUID) with appropriate error handlingGetLvsNameByUUIDis used with proper null checks and error handling in both backing image and server components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of new utility functions in backing image related code.
# Check for backing image related files that use these utilities
echo "Checking usage of new utility functions in backing image related code..."
# Check for connectNVMeTarget usage
rg -l "connectNVMeTarget\(" --type go
# Check for GetSnapXattr usage
rg -l "GetSnapXattr\(" --type go
# Check for GetLvsNameByUUID usage
rg -l "GetLvsNameByUUID\(" --type go
# Look for potential error handling patterns
echo "Checking error handling patterns..."
rg "GetSnapXattr\([^)]+\)$" --type go
rg "GetLvsNameByUUID\([^)]+\)$" --type go
Length of output: 1185
Script:
#!/bin/bash
# Check the implementation details in backing_image.go and server.go
echo "Checking backing_image.go implementation..."
rg -A 5 "connectNVMeTarget\(" pkg/spdk/backing_image.go
echo -e "\nChecking server.go implementation..."
rg -A 5 "GetSnapXattr\(" pkg/spdk/server.go
echo -e "\nChecking error handling in backing_image.go..."
rg -A 5 "GetLvsNameByUUID\(" pkg/spdk/backing_image.go
echo -e "\nChecking the overall context of utility usage..."
ast-grep --pattern 'func $FUNC($ARGS) $RET {
$$$
GetSnapXattr($$$)
$$$
}'
Length of output: 2234
pkg/api/types.go (4)
98-101: LGTM: Proper handling of BackingImageName in conversion functions
The conversion logic correctly handles the BackingImageName field with appropriate empty string checks in both directions.
Also applies to: 127-129
195-227: LGTM: Complete and symmetric conversion functions
The conversion functions properly handle all fields including nested Snapshot conversion using existing Lvol converters.
299-311: LGTM: Consistent stream implementation
The BackingImageStream implementation follows the established pattern used for ReplicaStream and EngineStream.
29-29: LGTM: BackingImageName field addition
The new field follows the existing naming conventions and maintains backward compatibility by being added at the end of the struct.
Let's verify the field usage across the codebase:
✅ Verification successful
BackingImageName field is consistently integrated across the codebase
The field is properly used throughout the codebase with consistent implementations in:
- SPDK engine components for backing image operations
- Client-side request/response handling
- Protobuf definitions and generated code
- Backupstore functionality for volume operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BackingImageName field usage
rg "BackingImageName" --type go
Length of output: 3966
pkg/spdk/backing_image.go (1)
572-573: Add file permission validation
When opening files with os.OpenFile, consider validating file permissions and ownership before proceeding with operations to prevent potential security issues.
pkg/spdk/replica.go (8)
53-53: LGTM: BackingImage field addition
The addition of the BackingImage field to the Replica struct is well-structured and aligns with the PR's objective of implementing backing image support.
141-145: LGTM: BackingImage proto conversion
The conversion logic correctly handles the backing image name extraction and safely handles nil cases.
1041-1041: LGTM: Improved code documentation
Good addition of clarifying comment about the parent relationship with backing images.
202-203: LGTM: Enhanced filter logic for backing images
The filter modification correctly handles backing image snapshots with appropriate explanation in the comment.
1949-1963: LGTM: Backing image snapshot handling
The code correctly handles the special case of backing image snapshots, accounting for LVS UUID differences between nodes.
522-532: LGTM: PrepareHead backing image support
The modifications to prepareHead correctly handle backing image initialization and chain setup.
271-271: LGTM: BackingImage assignment in construct
Correctly assigns the backing image from the first entry of the active chain, maintaining the established convention.
522-532: Verify error handling and state transitions
The code handles complex state transitions and error cases. Please verify:
- State consistency after backing image operations fail
- Cleanup of resources in error paths
- Impact of backing image failures on replica state
Also applies to: 1949-1970
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Error handling for backing image operations is properly implemented
The code shows consistent error handling patterns for backing image operations:
- State transitions are properly managed with mutex locks and deferred updates
- Error states are consistently set with proper error messages
- Cleanup is handled via deferred functions
- Resources are properly released in error paths
- Error propagation is consistent across the codebase
The specific code under review follows these patterns:
if backingImage != nil {
r.ActiveChain[0] = backingImage.Snapshot
r.BackingImage = r.ActiveChain[0]
}This is a safe operation as any errors in backing image operations would have been caught earlier in the call chain, with proper state transitions and cleanup already handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling patterns and state transitions
# Look for error state transitions
rg -A 3 "InstanceStateError"
# Check cleanup patterns after errors
rg -A 5 "defer func\(\)"
# Verify consistent error handling in backing image operations
rg "BackingImage.*error"
Length of output: 105351
pkg/spdk/engine.go (2)
1336-1337: LGTM! Clear and well-documented change
The modification to consider backing image snapshots as valid ancestors is correct and well-documented with the explanatory comment.
753-762: Verify backing image retrieval error handling
The backing image retrieval error handling looks good, but we should verify that marking the replica as ERR won't cause data inconsistency issues.
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough context about how the ModeERR is handled throughout the codebase.
Setting replica to ERR mode during backing image retrieval failure is consistent with error handling patterns
The error handling approach of setting replicaStatus.Mode = types.ModeERR when backing image retrieval fails is consistent with the codebase's error handling patterns. The search results show that:
- ERR mode is consistently used as a fallback when critical operations fail (replica validation, NVMe operations, rebuilding)
- After setting ERR mode, the code either returns early or continues with other replicas
- The error state is properly tracked via
updateRequired/requireUpdateflags - The implementation follows the same pattern as other error cases in the engine
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential data inconsistency scenarios in error handling
# Look for other places where replica mode is set to ERR and verify the handling is consistent
rg -A 5 "replicaStatus.Mode = types.ModeERR"
Length of output: 2942
| func NewDownloadHttpClient() http.Client { | ||
| return http.Client{ | ||
| CheckRedirect: func(req *http.Request, via []*http.Request) error { | ||
| // Remove the "Referer" header to enable downloads of files | ||
| // that are delivered via CDN and therefore may be redirected | ||
| // several times. This is the same behaviour of curl or wget | ||
| // in their default configuration. | ||
| removeReferer(req) | ||
| return nil | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout configuration to HTTP client
The HTTP client should have timeout configuration to prevent hanging connections.
func NewDownloadHttpClient() http.Client {
return http.Client{
+ Timeout: HTTPTimeout,
CheckRedirect: func(req *http.Request, via []*http.Request) error {
removeReferer(req)
+ if len(via) >= 10 {
+ return fmt.Errorf("stopped after 10 redirects")
+ }
return nil
},
}Committable suggestion skipped: line range outside the PR's diff.
| // IdleTimeoutCopy relies on ctx of the reader/src or a separate timer to interrupt the processing. | ||
| func IdleTimeoutCopy(ctx context.Context, cancel context.CancelFunc, src io.ReadCloser, dst io.WriteSeeker, updater ProgressUpdater, writeZero bool) (copied int64, err error) { | ||
| writeSeekCh := make(chan int64, 100) | ||
| defer close(writeSeekCh) | ||
|
|
||
| go func() { | ||
| t := time.NewTimer(HTTPTimeout) | ||
| done := false | ||
| for !done { | ||
| select { | ||
| case <-ctx.Done(): | ||
| done = true | ||
| case <-t.C: | ||
| cancel() | ||
| done = true | ||
| case _, writeChOpen := <-writeSeekCh: | ||
| if !writeChOpen { | ||
| done = true | ||
| break | ||
| } | ||
| if !t.Stop() { | ||
| <-t.C | ||
| } | ||
| t.Reset(HTTPTimeout) | ||
| } | ||
| } | ||
|
|
||
| // Still need to make sure to clean up the signals in writeSeekCh | ||
| // so that they won't block the below sender. | ||
| for writeChOpen := true; writeChOpen; { | ||
| _, writeChOpen = <-writeSeekCh | ||
| } | ||
| }() | ||
|
|
||
| var nr, nw int | ||
| var nws int64 | ||
| var rErr, handleErr error | ||
| buf := make([]byte, DownloadBufferSize) | ||
| zeroByteArray := make([]byte, DownloadBufferSize) | ||
| for rErr == nil && err == nil { | ||
| select { | ||
| case <-ctx.Done(): | ||
| err = fmt.Errorf("context cancelled during the copy") | ||
| default: | ||
| // Read will error out once the context is cancelled. | ||
| nr, rErr = src.Read(buf) | ||
| if nr > 0 { | ||
| // Skip writing zero data | ||
| if !writeZero && bytes.Equal(buf[0:nr], zeroByteArray[0:nr]) { | ||
| _, handleErr = dst.Seek(int64(nr), io.SeekCurrent) | ||
| nws = int64(nr) | ||
| } else { | ||
| nw, handleErr = dst.Write(buf[0:nr]) | ||
| nws = int64(nw) | ||
| } | ||
| if handleErr != nil { | ||
| err = handleErr | ||
| break | ||
| } | ||
| writeSeekCh <- nws | ||
| copied += nws | ||
| updater.UpdateProgress(nws) | ||
| } | ||
| if rErr != nil { | ||
| if rErr != io.EOF { | ||
| err = rErr | ||
| } | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return copied, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of IdleTimeoutCopy
While the complexity is justified, there are several areas for improvement:
- The goroutine might leak if the function returns early due to an error
- The channel buffer size (100) seems arbitrary
- Missing documentation for the
writeZeroparameter
Consider these improvements:
// IdleTimeoutCopy relies on ctx of the reader/src or a separate timer to interrupt the processing.
+// writeZero determines whether zero bytes should be written to the destination or skipped.
+// Returns the number of bytes copied and any error that occurred.
func IdleTimeoutCopy(ctx context.Context, cancel context.CancelFunc, src io.ReadCloser, dst io.WriteSeeker, updater ProgressUpdater, writeZero bool) (copied int64, err error) {
- writeSeekCh := make(chan int64, 100)
+ // Buffer size matches DownloadBufferSize to prevent blocking
+ writeSeekCh := make(chan int64, DownloadBufferSize)
defer close(writeSeekCh)
+ // Create a done channel to prevent goroutine leak
+ done := make(chan struct{})
+ defer close(done)
go func() {
t := time.NewTimer(HTTPTimeout)
+ defer t.Stop()
done := false
for !done {
select {
+ case <-done:
+ return
case <-ctx.Done():
done = trueCommittable suggestion skipped: line range outside the PR's diff.
pkg/util/util.go
Outdated
| func GetBackingImageDirectoryName(biName, biUUID string) string { | ||
| return fmt.Sprintf("%s-%s", biName, biUUID) | ||
| } | ||
|
|
||
| func GetBackingImageDirectory(diskPath, biName, biUUID string) string { | ||
| return filepath.Join(diskPath, BackingImageManagerDirectoryName, GetBackingImageDirectoryName(biName, biUUID)) | ||
| } | ||
|
|
||
| func GetBackingImageFilePath(diskPath, biName, biUUID string) string { | ||
| return filepath.Join(GetBackingImageDirectory(diskPath, biName, biUUID), BackingImageFileName) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation for biName and biUUID parameters.
While the path construction is correct, consider adding validation to ensure biName and biUUID don't contain directory traversal characters or other potentially unsafe characters.
Example validation:
func GetBackingImageDirectoryName(biName, biUUID string) string {
+ // Sanitize inputs to prevent directory traversal
+ biName = filepath.Clean(strings.ReplaceAll(biName, "..", ""))
+ biUUID = filepath.Clean(strings.ReplaceAll(biUUID, "..", ""))
return fmt.Sprintf("%s-%s", biName, biUUID)
}Committable suggestion skipped: line range outside the PR's diff.
| if err != nil { | ||
| return errors.Wrapf(err, "failed to split host and port from address %v", exposedSnapshotLvolAddress) | ||
| } | ||
| _, _, err = connectNVMeTarget(srcIP, srcPort) |
There was a problem hiding this comment.
Remove ineffectual error assignment
The error returned by connectNVMeTarget is assigned but never used.
- _, _, err = connectNVMeTarget(srcIP, srcPort)
+ if _, _, err := connectNVMeTarget(srcIP, srcPort); err != nil {
+ return errors.Wrapf(err, "failed to connect to NVMe target at %s:%s", srcIP, srcPort)
+ }📝 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.
| _, _, err = connectNVMeTarget(srcIP, srcPort) | |
| if _, _, err := connectNVMeTarget(srcIP, srcPort); err != nil { | |
| return errors.Wrapf(err, "failed to connect to NVMe target at %s:%s", srcIP, srcPort) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
656-656: ineffectual assignment to err
(ineffassign)
| func (bi *BackingImage) UpdateProgress(processedSize int64) { | ||
| bi.updateProgress(processedSize) | ||
| bi.UpdateCh <- nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider non-blocking channel send in UpdateProgress
The current implementation might block if there's no receiver for UpdateCh or if the channel is full. Consider using a non-blocking send to prevent potential deadlocks.
func (bi *BackingImage) UpdateProgress(processedSize int64) {
bi.updateProgress(processedSize)
- bi.UpdateCh <- nil
+ select {
+ case bi.UpdateCh <- nil:
+ default:
+ // Log skipped update if needed
+ }
}📝 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.
| func (bi *BackingImage) UpdateProgress(processedSize int64) { | |
| bi.updateProgress(processedSize) | |
| bi.UpdateCh <- nil | |
| } | |
| func (bi *BackingImage) UpdateProgress(processedSize int64) { | |
| bi.updateProgress(processedSize) | |
| select { | |
| case bi.UpdateCh <- nil: | |
| default: | |
| // Log skipped update if needed | |
| } | |
| } |
pkg/spdk/backing_image.go
Outdated
| go func() { | ||
| bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID) | ||
| // update the backing image afte preparing | ||
| bi.UpdateCh <- nil | ||
| }() |
There was a problem hiding this comment.
Improve error handling in Create method's goroutine
The error from prepareBackingImageSnapshot is not propagated back to the caller. Consider using a channel or callback mechanism to handle errors from the goroutine.
+ errorCh := make(chan error, 1)
go func() {
- bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID)
+ if err := bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID); err != nil {
+ errorCh <- err
+ }
+ close(errorCh)
bi.UpdateCh <- nil
}()
+ // Store error channel for later checking
+ bi.preparationError = errorChCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
124-124: Error return value of bi.prepareBackingImageSnapshot is not checked
(errcheck)
| if lvsName == "" { | ||
| return fmt.Errorf("failed to get the lvs name for backing image %v with lvs uuid %v", bi.Name, bi.LvsUUID) | ||
| } | ||
| bi.LvsName = lvsName | ||
| bi.log = logrus.StandardLogger().WithFields(logrus.Fields{ | ||
| "backingImagename": bi.Name, | ||
| "lvsName": bi.LvsName, | ||
| "lvsUUID": bi.LvsUUID, | ||
| }) | ||
| bi.Unlock() | ||
|
|
||
| // backingImageTempHeadName will be "bi-${biName}-disk-${lvsUUID}-temp-head" | ||
| backingImageTempHeadName := GetBackingImageTempHeadLvolName(bi.Name, bi.LvsUUID) | ||
| biTempHeadUUID, err = spdkClient.BdevLvolCreate("", bi.LvsUUID, backingImageTempHeadName, util.BytesToMiB(bi.Size), "", true) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| bdevLvolList, err := spdkClient.BdevLvolGet(biTempHeadUUID, 0) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(bdevLvolList) < 1 { | ||
| return fmt.Errorf("cannot find lvol %v after creation", backingImageTempHeadName) | ||
| } | ||
| bi.log.Info("Created a head lvol %v for the new backing image", backingImageTempHeadName) | ||
|
|
||
| podIP, err := commonnet.GetIPForPod() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| port, _, err := superiorPortAllocator.AllocateRange(int32(types.BackingImagePortCount)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| bi.Lock() | ||
| bi.Port = port | ||
| bi.Unlock() | ||
| bi.log.Infof("Allocated port %v", port) | ||
|
|
||
| executor, err := helperutil.NewExecutor(commontypes.ProcDirectory) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to create executor") | ||
| } | ||
| subsystemNQN, controllerName, err := exposeSnapshotLvolBdev(spdkClient, bi.LvsName, backingImageTempHeadName, podIP, port, executor) | ||
| if err != nil { | ||
| bi.log.WithError(err).Errorf("Failed to expose head lvol") | ||
| return err | ||
| } | ||
| bi.Lock() | ||
| bi.IsExposed = true | ||
| bi.Unlock() | ||
| bi.log.Infof("Exposed head lvol %v, subsystemNQN %v, controllerName %v", backingImageTempHeadName, subsystemNQN, controllerName) | ||
|
|
||
| headInitiator, err := nvme.NewInitiator(backingImageTempHeadName, helpertypes.GetNQN(backingImageTempHeadName), nvme.HostProc) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to create NVMe initiator for head lvol %v", backingImageTempHeadName) | ||
| } | ||
| if _, err := headInitiator.Start(podIP, strconv.Itoa(int(port)), true); err != nil { | ||
| return errors.Wrapf(err, "failed to start NVMe initiator for head lvol %v", backingImageTempHeadName) | ||
| } | ||
| bi.log.Info("Created NVMe initiator for head lvol %v", backingImageTempHeadName) | ||
|
|
||
| headFh, err := os.OpenFile(headInitiator.Endpoint, os.O_RDWR, 0666) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to open NVMe device %v for lvol bdev %v", headInitiator.Endpoint, backingImageTempHeadName) | ||
| } | ||
| defer func() { | ||
| // Stop the initiator | ||
| headFh.Close() | ||
| bi.log.Info("Stopping NVMe initiator") | ||
| if _, opErr := headInitiator.Stop(true, true, false); opErr != nil { | ||
| logrus.Errorf("Failed to stop the backing image head NVMe initiator") | ||
| } | ||
| }() | ||
|
|
||
| // An SPDK backing image should only be created by downloading it from BIM if it's a first copy, | ||
| // or by syncing it from another SPDK server. | ||
| isSourceFromBIM := checkIsSourceFromBIM(fromAddress) | ||
| if isSourceFromBIM { | ||
| if err := bi.prepareFromURL(headFh, fromAddress); err != nil { | ||
| bi.log.WithError(err).Warnf("Failed to prepare the backing image %v from URL %v", bi.Name, fromAddress) | ||
| return errors.Wrapf(err, "failed to prepare the backing image %v from URL %v", bi.Name, fromAddress) | ||
| } | ||
| } else { | ||
| if err := bi.prepareFromSync(headFh, fromAddress, srcLvsUUID); err != nil { | ||
| bi.log.WithError(err).Warnf("Failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID) | ||
| return errors.Wrapf(err, "failed to prepare the backing image %v by syncing from %v and srcLvsUUID %v", bi.Name, fromAddress, srcLvsUUID) | ||
| } | ||
| } | ||
|
|
||
| currentChecksum, err := util.GetFileChecksum(headInitiator.Endpoint) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to get the current checksum of the backing image %v target device %v", bi.Name, headInitiator.Endpoint) | ||
| } | ||
| bi.log.Infof("Get the current checksum of the backing image %v: %v", bi.Name, currentChecksum) | ||
| bi.CurrentChecksum = currentChecksum | ||
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce complexity in prepareBackingImageSnapshot
The method is quite complex and handles multiple responsibilities. Consider breaking it down into smaller, focused methods:
- Resource initialization
- Snapshot creation
- Cleanup handling
Example refactor:
func (bi *BackingImage) prepareBackingImageSnapshot(...) error {
+ if err := bi.initializeResources(spdkClient); err != nil {
+ return err
+ }
+ if err := bi.createAndExposeTemporaryHead(...); err != nil {
+ return err
+ }
+ if err := bi.createFinalSnapshot(...); err != nil {
+ return err
+ }
return nil
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 379-609: pkg/spdk/backing_image.go#L379-L609
Complex Method
pkg/spdk/server.go
Outdated
| backingImageName := GetBackingImageNameFromLvolName(lvolName) | ||
| backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName) |
There was a problem hiding this comment.
Fix ineffectual assignment to backingImageName
The assignment to backingImageName on line 280 is immediately overwritten by the subsequent call to ExtractBackingImageAndDiskUUID. This could indicate a logic error or redundant code.
Remove the redundant assignment:
-backingImageName := GetBackingImageNameFromLvolName(lvolName)
backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName)📝 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.
| backingImageName := GetBackingImageNameFromLvolName(lvolName) | |
| backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName) | |
| backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName) |
🧰 Tools
🪛 golangci-lint (1.62.2)
280-280: ineffectual assignment to backingImageName
(ineffassign)
ChanYiLin
force-pushed
the
LH6341_v2_backing_image
branch
2 times, most recently
from
68af776
to
1f8dc99
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (15)
pkg/types/types.go (1)
51-56: Add documentation for backing image constantsPlease add comments explaining:
- The purpose and significance of
BackingImagePortCount = 1- The format/structure of the snapshot attribute values (e.g., expected value types for checksum)
pkg/util/http_handler.go (1)
15-18: Consider making timeout configurableThe 4-second timeout might be too short for large file downloads, especially in environments with slower network connections. Consider making this configurable through environment variables or configuration settings.
const ( DownloadBufferSize = 1 << 12 - HTTPTimeout = 4 * time.Second + defaultHTTPTimeout = 4 * time.Second ) + +var HTTPTimeout = getHTTPTimeoutFromEnv() + +func getHTTPTimeoutFromEnv() time.Duration { + if t := os.Getenv("DOWNLOAD_HTTP_TIMEOUT"); t != "" { + if d, err := time.ParseDuration(t); err == nil { + return d + } + } + return defaultHTTPTimeout +}pkg/spdk/util.go (1)
Line range hint
1-186: Consider implementing consistent error handling patternsGiven that these utility functions are part of the backing image support feature, it would be beneficial to implement consistent error handling patterns across all functions. Consider:
- Returning explicit errors instead of empty strings
- Using structured logging consistently
- Adding proper input validation
- Implementing proper error wrapping for better error context
This will improve debugging capabilities and make the codebase more maintainable as the backing image feature grows.
pkg/spdk/types.go (3)
52-73: Add godoc comment for the functionWhile the function implementation is correct, it would benefit from a godoc comment explaining its purpose, parameters, and return value. This is especially important as it's an exported function.
Add this documentation:
+// ServiceBackingImageLvolToProtoBackingImageLvol converts a backing image Lvol to its proto representation. +// Unlike regular Lvols, backing images have no parent and their children are direct snapshot lvol names. +// Parameters: +// - lvol: The backing image Lvol to convert +// Returns: +// - *spdkrpc.Lvol: The proto representation of the backing image Lvol func ServiceBackingImageLvolToProtoBackingImageLvol(lvol *Lvol) *spdkrpc.Lvol {
124-141: Enhance input validation in ExtractBackingImageAndDiskUUIDWhile the regex pattern works for basic cases, it could be vulnerable to edge cases. Consider adding length limits and additional validation.
func ExtractBackingImageAndDiskUUID(lvolName string) (string, string, error) { + if len(lvolName) > 255 { + return "", "", fmt.Errorf("lvolName exceeds maximum length of 255 characters") + } + re := regexp.MustCompile(`^bi-([a-zA-Z0-9-]+)-disk-([a-zA-Z0-9-]+)$`) matches := re.FindStringSubmatch(lvolName) if matches == nil { return "", "", fmt.Errorf("lvolName does not match the expected pattern") } backingImageName := matches[1] diskUUID := matches[2] + + // Additional validation + if len(backingImageName) == 0 || len(diskUUID) == 0 { + return "", "", fmt.Errorf("backingImageName and diskUUID cannot be empty") + } return backingImageName, diskUUID, nil }
143-161: Add documentation for backing image utility functionsThe utility functions would benefit from godoc comments explaining their purpose and usage.
Add documentation for each function:
+// GetBackingImageSnapLvolName generates the logical volume name for a backing image snapshot +// using the format "bi-<backingImageName>-disk-<lvsUUID>" func GetBackingImageSnapLvolName(backingImageName string, lvsUUID string) string { +// IsBackingImageSnapLvolName checks if the given logical volume name represents a backing image snapshot func IsBackingImageSnapLvolName(lvolName string) bool { +// GetBackingImageTempHeadLvolName generates the temporary head logical volume name for a backing image +// using the format "bi-<backingImageName>-disk-<lvsUUID>-temp-head" func GetBackingImageTempHeadLvolName(backingImageName string, lvsUUID string) string { +// GetBackingImageSnapLvolNameFromTempHeadLvolName extracts the backing image snapshot name +// from a temporary head logical volume name by removing the temp-head suffix func GetBackingImageSnapLvolNameFromTempHeadLvolName(lvolName string) string { +// IsBackingImageTempHead checks if the given logical volume name represents a backing image temporary head func IsBackingImageTempHead(lvolName string) bool {pkg/api/types.go (2)
98-101: Consider adding validation for backing image name formatWhile the empty string check is good, consider adding validation for the backing image name format if there are any specific requirements or constraints.
if r.BackingImageName != "" { + // TODO: Add validation for backing image name format if needed res.BackingImageName = r.BackingImageName }
181-193: Consider enhancing error message handlingThe
ErrorMsgfield might benefit from additional context or structure to help with error categorization and handling.Consider using a more structured error representation:
- ErrorMsg string `json:"error_msg"` + Error struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"error"`pkg/spdk/server.go (3)
257-259: Consider adding error logging for snapshot checkThe snapshot check silently continues without logging why a snapshot was skipped. Consider adding debug logging to help with troubleshooting.
if bdevLvol.DriverSpecific.Lvol.Snapshot && !IsBackingImageSnapLvolName(lvolName) { + logrus.Debugf("Skipping non-backing image snapshot: %s", lvolName) continue }
454-462: Enhance error message for backing image validationThe error message when a backing image is not found could be more descriptive to aid in troubleshooting.
-return nil, grpcstatus.Error(grpccodes.Internal, "failed to find the backing image in the spdk server") +return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to find backing image %s in lvs %s", req.BackingImageName, req.LvsUuid)
1580-1581: Address TODO comment regarding existing backing imagesThe TODO comment indicates missing logic for handling existing backing images.
Would you like me to help implement the logic for handling existing backing images?
pkg/spdk/replica.go (1)
1967-1967: Remove debug tag from error messageThe error message contains a "[DEBUG]" tag that should be removed as it appears to be temporary debugging code.
- logrus.WithError(err).Errorf("[DEBUG] failed to resize the rebuildingLvolUUID %v to size %v", rebuildingLvolUUID, r.SpecSize) + logrus.WithError(err).Errorf("failed to resize the rebuildingLvolUUID %v to size %v", rebuildingLvolUUID, r.SpecSize)pkg/spdk/engine.go (1)
Line range hint
705-840: LGTM with suggestions for error handling improvementsThe new ancestor selection hierarchy (backing image → snapshots → volume head) is well-implemented. However, consider enhancing error handling in the following scenarios:
- When backing image retrieval fails (line 756)
- When replicas have inconsistent backing image states (line 768)
- When replicas have inconsistent snapshot states (line 780)
Consider adding recovery mechanisms for these scenarios:
if replica.BackingImageName != "" { hasBackingImage = true backingImage, err := replicaServiceCli.BackingImageGet(replica.BackingImageName, replica.LvsUUID) if err != nil { + // Try to recover by falling back to snapshots if available + if len(replica.Snapshots) > 0 { + e.log.Warnf("Failed to get backing image %s, attempting fallback to snapshots", replica.BackingImageName) + hasBackingImage = false + goto snapshot_fallback + } e.log.WithError(err).Warnf("Failed to get backing image %s with disk UUID %s from replica %s head parent %s, will mark the mode from %v to ERR and continue info update for other replicas", replica.BackingImageName, replica.LvsUUID, replicaName, replica.Head.Parent, replicaStatus.Mode) replicaStatus.Mode = types.ModeERR return }pkg/spdk/backing_image.go (2)
619-620: Use logger instead offmt.Printlnfor error messagesIn
prepareFromURL, usingfmt.Printlnfor error output is inconsistent with logging practices. Replace it with the logger for consistency and better log management.Apply this diff to fix the issue:
parsedURL, err := url.Parse(fromAddress) if err != nil { - fmt.Println("Error parsing URL:", err) return errors.Wrapf(err, "failed to parse the URL %v", fromAddress) + bi.log.WithError(err).Errorf("Failed to parse the URL %v", fromAddress) }
693-694: Typo in error message: "unsexpose" should be "unexpose"There's a typographical error in the error message. The word "unsexpose" should be "unexpose".
Apply this diff to fix the typo:
if err := srcBackingImageServiceCli.BackingImageUnexpose(bi.ctx, bi.Name, srcLvsUUID); err != nil { - return errors.Wrapf(err, "failed to unsexpose the source backing image %v", bi.Name) + return errors.Wrapf(err, "failed to unexpose the source backing image %v", bi.Name) }
🛑 Comments failed to post (9)
pkg/spdk/util.go (2)
175-186: 🛠️ Refactor suggestion
Enhance error handling, documentation, and comments
The function needs improvements in several areas:
- Silent error handling masks important issues
- Missing function documentation
- No input validation
- Unclear comment about failure condition
+// GetLvsNameByUUID retrieves the name of a logical volume store (LVS) by its UUID. +// Returns the LVS name if exactly one match is found, or an error otherwise. func GetLvsNameByUUID(spdkClient *spdkclient.Client, lvsUUID string) string { + if spdkClient == nil { + return "", errors.New("invalid spdkClient: nil") + } + if lvsUUID == "" { + return "", errors.New("lvsUUID cannot be empty") + } var lvsList []spdktypes.LvstoreInfo lvsList, err := spdkClient.BdevLvolGetLvstore("", lvsUUID) if err != nil { - return "" + return "", errors.Wrapf(err, "failed to get lvstore for UUID %v", lvsUUID) } if len(lvsList) != 1 { - // consider it as failed to get the correct lvs - return "" + return "", errors.Errorf("expected exactly one lvstore for UUID %v, got %d", lvsUUID, len(lvsList)) } - return lvsList[0].Name + return lvsList[0].Name, nil }Committable suggestion skipped: line range outside the PR's diff.
167-173: 🛠️ Refactor suggestion
Improve error handling and add documentation
The function silently returns an empty string on error, which might hide important issues. Consider:
- Adding proper error handling to allow callers to distinguish between "no value" and "error occurred"
- Adding documentation to explain the purpose and behavior
- Adding input validation
+// GetSnapXattr retrieves the extended attribute value for a given logical volume alias and key. +// Returns the attribute value if successful, or an error if the operation fails. func GetSnapXattr(spdkClient *spdkclient.Client, alias, key string) (res string) { + if spdkClient == nil { + return "", errors.New("invalid spdkClient: nil") + } + if alias == "" || key == "" { + return "", errors.New("alias and key cannot be empty") + } value, err := spdkClient.BdevLvolGetXattr(alias, key) if err != nil { - return "" + return "", errors.Wrapf(err, "failed to get xattr for alias %v with key %v", alias, key) } - return value + return value, nil }Committable suggestion skipped: line range outside the PR's diff.
pkg/spdk/types.go (1)
124-161: 💡 Codebase verification
Missing test coverage for backing image utility functions
The newly added backing image utility functions lack test coverage. No test cases were found for:
- ExtractBackingImageAndDiskUUID
- GetBackingImageSnapLvolName
- IsBackingImageSnapLvolName
- GetBackingImageTempHeadLvolName
- GetBackingImageSnapLvolNameFromTempHeadLvolName
- IsBackingImageTempHead
These functions handle critical naming patterns and validations. Test cases should be added to verify:
- Correct parsing of backing image and disk UUID from lvol names
- Proper generation of snap lvol names
- Accurate validation of lvol name patterns
- Edge cases with special characters in names
- Error handling for malformed inputs
🔗 Analysis chain
Verify test coverage for backing image utilities
These utility functions are critical for backing image management. Ensure comprehensive test coverage, especially for edge cases in name parsing and validation.
Let me gather more specific information about the test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage of backing image utilities # Look for test files echo "Searching for test files..." fd ".*_test\.go$" --type f # Look for specific test cases echo -e "\nSearching for backing image related tests..." rg -l "TestBackingImage" --type go # Check for potential test gaps echo -e "\nChecking for untested functions..." ast-grep --pattern 'func (GetBackingImage|IsBackingImage).*\('Length of output: 422
Script:
#!/bin/bash # Check the content of the test files to verify coverage # Check util_test.go for backing image related test cases echo "Checking pkg/spdk/util_test.go for backing image tests:" rg "func Test.*BackingImage.*|func Test.*Lvol.*" pkg/spdk/util_test.go -A 5 # Look for actual test implementations of our specific functions echo -e "\nChecking for specific function test coverage:" rg "ExtractBackingImageAndDiskUUID|GetBackingImageSnapLvolName|IsBackingImageSnapLvolName|GetBackingImageTempHeadLvolName|GetBackingImageSnapLvolNameFromTempHeadLvolName|IsBackingImageTempHead" pkg/spdk/util_test.goLength of output: 523
pkg/api/types.go (1)
195-227: 🛠️ Refactor suggestion
Add nil checks in conversion functions
The conversion functions should include nil checks for consistency with other conversion functions in the codebase (e.g.,
ProtoLvolToLvol).func ProtoBackingImageToBackingImage(bi *spdkrpc.BackingImage) *BackingImage { + if bi == nil { + return nil + } res := &BackingImage{ // ... existing code ... } return res } func BackingImageToProtoBackingImage(bi *BackingImage) *spdkrpc.BackingImage { + if bi == nil { + return nil + } return &spdkrpc.BackingImage{ // ... existing code ... } }📝 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.func ProtoBackingImageToBackingImage(bi *spdkrpc.BackingImage) *BackingImage { if bi == nil { return nil } res := &BackingImage{ Name: bi.Name, BackingImageUUID: bi.BackingImageUUID, LvsName: bi.LvsName, LvsUUID: bi.LvsUUID, Size: bi.Size, ExpectedChecksum: bi.ExpectedChecksum, Snapshot: ProtoLvolToLvol(bi.Snapshot), Progress: bi.Progress, State: bi.State, CurrentChecksum: bi.CurrentChecksum, ErrorMsg: bi.ErrorMsg, } return res } func BackingImageToProtoBackingImage(bi *BackingImage) *spdkrpc.BackingImage { if bi == nil { return nil } return &spdkrpc.BackingImage{ Name: bi.Name, BackingImageUuid: bi.BackingImageUUID, LvsName: bi.LvsName, LvsUuid: bi.LvsUUID, Size: bi.Size, ExpectedChecksum: bi.ExpectedChecksum, Snapshot: LvolToProtoLvol(bi.Snapshot), Progress: bi.Progress, State: bi.State, CurrentChecksum: bi.CurrentChecksum, ErrorMsg: bi.ErrorMsg, } }pkg/client/client.go (1)
885-906:
⚠️ Potential issueFix typo in parameter name.
The parameter name contains a typo:
backingIamgeUUIDshould bebackingImageUUID.Apply this diff to fix the typo:
-func (c *SPDKClient) BackingImageCreate(name, backingIamgeUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) { +func (c *SPDKClient) BackingImageCreate(name, backingImageUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) {📝 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.func (c *SPDKClient) BackingImageCreate(name, backingImageUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) { if name == "" || backingIamgeUUID == "" || checksum == "" || lvsUUID == "" || size == 0 { return nil, fmt.Errorf("failed to start SPDK backing image: missing required parameters") } client := c.getSPDKServiceClient() ctx, cancel := context.WithTimeout(context.Background(), GRPCServiceTimeout) defer cancel() resp, err := client.BackingImageCreate(ctx, &spdkrpc.BackingImageCreateRequest{ Name: name, BackingImageUuid: backingIamgeUUID, LvsUuid: lvsUUID, Size: size, Checksum: checksum, FromAddress: fromAddress, SrcLvsUuid: srcLvsUUID, }) if err != nil { return nil, errors.Wrap(err, "failed to start SPDK backing image") } return api.ProtoBackingImageToBackingImage(resp), nil }pkg/spdk/server.go (3)
285-293:
⚠️ Potential issuePotential race condition in backing image state initialization
The backing image state is set to
BackingImageStatePendingbut there's a window between creation and state update where other operations could see an inconsistent state.Consider using an atomic operation or a mutex to ensure state consistency:
-backingImage.State = types.BackingImageStatePending +backingImage.setStateLocked(types.BackingImageStatePending)Committable suggestion skipped: line range outside the PR's diff.
1749-1794: 🛠️ Refactor suggestion
Add timeout handling for expose/unexpose operations
The
BackingImageExposeandBackingImageUnexposeoperations could potentially hang. Consider adding context with timeout.-func (s *Server) BackingImageExpose(ctx context.Context, req *spdkrpc.BackingImageGetRequest) (ret *spdkrpc.BackingImageExposeResponse, err error) { +func (s *Server) BackingImageExpose(ctx context.Context, req *spdkrpc.BackingImageGetRequest) (ret *spdkrpc.BackingImageExposeResponse, err error) { + timeoutCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + + done := make(chan struct{}) + go func() { + // Existing expose logic + close(done) + }() + + select { + case <-timeoutCtx.Done(): + return nil, grpcstatus.Error(grpccodes.DeadlineExceeded, "backing image expose operation timed out") + case <-done: + return ret, err + } }Committable suggestion skipped: line range outside the PR's diff.
1598-1631: 🛠️ Refactor suggestion
Consider adding validation for FromAddress
The
BackingImageCreatefunction acceptsFromAddressbut doesn't validate its format when it's provided.Add validation before using the address:
if req.FromAddress != "" { + if _, _, err := net.SplitHostPort(req.FromAddress); err != nil { + return nil, grpcstatus.Errorf(grpccodes.InvalidArgument, "invalid from address format: %v", err) + } }📝 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.func (s *Server) BackingImageCreate(ctx context.Context, req *spdkrpc.BackingImageCreateRequest) (ret *spdkrpc.BackingImage, err error) { if req.Name == "" { return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image name is required") } if req.BackingImageUuid == "" { return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image UUID is required") } if req.Size == uint64(0) { return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image size is required") } if req.LvsUuid == "" { return nil, grpcstatus.Error(grpccodes.InvalidArgument, "lvs UUID is required") } if req.Checksum == "" { return nil, grpcstatus.Error(grpccodes.InvalidArgument, "checksum is required") } if req.FromAddress != "" { if _, _, err := net.SplitHostPort(req.FromAddress); err != nil { return nil, grpcstatus.Errorf(grpccodes.InvalidArgument, "invalid from address format: %v", err) } } // Don't recreate the backing image backingImageSnapLvolName := GetBackingImageSnapLvolName(req.Name, req.LvsUuid) if _, ok := s.backingImageMap[backingImageSnapLvolName]; ok { return nil, grpcstatus.Errorf(grpccodes.AlreadyExists, "backing image %v already exists", req.Name) } bi, err := s.newBackingImage(req) if err != nil { return nil, err } s.RLock() spdkClient := s.spdkClient s.RUnlock() return bi.Create(spdkClient, s.portAllocator, req.FromAddress, req.SrcLvsUuid) }pkg/spdk/backing_image.go (1)
675-678:
⚠️ Potential issueEnsure
srcFhis closed usingdeferto prevent file descriptor leaksIf an error occurs during
IdleTimeoutCopy,srcFhmay not be closed, leading to resource leaks. Usedeferto ensure thatsrcFhis closed regardless of errors.Apply this diff to fix the issue:
srcFh, err := os.OpenFile(initiator.Endpoint, os.O_RDWR, 0666) if err != nil { return errors.Wrapf(err, "failed to open NVMe device %v for source backing image %v in lvsUUID %v with address %v", initiator.Endpoint, bi.Name, srcLvsUUID, exposedSnapshotLvolAddress) } + defer srcFh.Close() ctx, cancel := context.WithCancel(bi.ctx) defer cancel() _, err = util.IdleTimeoutCopy(ctx, cancel, srcFh, targetFh, bi, true) if err != nil { return errors.Wrapf(err, "failed to copy the source backing image %v in lvsUUID %v with address %v", bi.Name, srcLvsUUID, exposedSnapshotLvolAddress) } - // Stop the source initiator - srcFh.Close()📝 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.if err != nil { return errors.Wrapf(err, "failed to open NVMe device %v for source backing image %v in lvsUUID %v with address %v", initiator.Endpoint, bi.Name, srcLvsUUID, exposedSnapshotLvolAddress) } defer srcFh.Close()
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (21)
pkg/types/types.go (1)
51-56: Consider making port count configurableThe hardcoded
BackingImagePortCount = 1might limit scalability. Consider making this configurable through environment variables or configuration files for future flexibility.LGTM! Well-structured snapshot attributes
The snapshot attributes are well-named and align with the backing image functionality requirements.
pkg/util/http_handler.go (1)
16-18: Consider making timeout configurableThe 4-second timeout might be too short for large file downloads, especially in environments with slower network connections. Consider making this configurable through environment variables or configuration settings.
const ( DownloadBufferSize = 1 << 12 - HTTPTimeout = 4 * time.Second + defaultHTTPTimeout = 4 * time.Second ) + +var HTTPTimeout = getConfiguredTimeout() + +func getConfiguredTimeout() time.Duration { + if val := os.Getenv("DOWNLOAD_TIMEOUT"); val != "" { + if timeout, err := time.ParseDuration(val); err == nil { + return timeout + } + } + return defaultHTTPTimeout +}pkg/spdk/util.go (2)
167-173: Improve error handling and add documentationThe function silently handles errors and lacks documentation. Consider these improvements:
- Add function documentation explaining its purpose and parameters
- Consider logging errors or returning them to the caller
- Add validation for input parameters
+// GetSnapXattr retrieves the extended attribute value for a given logical volume alias and key. +// Returns an empty string if the attribute doesn't exist or an error occurs. func GetSnapXattr(spdkClient *spdkclient.Client, alias, key string) (res string) { + if spdkClient == nil { + logrus.Error("nil SPDK client provided to GetSnapXattr") + return "" + } + if alias == "" || key == "" { + logrus.Error("empty alias or key provided to GetSnapXattr") + return "" + } value, err := spdkClient.BdevLvolGetXattr(alias, key) if err != nil { + logrus.WithError(err).Errorf("failed to get xattr for alias=%s key=%s", alias, key) return "" } return value }
175-186: Enhance error handling and documentationThe function needs several improvements:
- Add function documentation
- Add input validation
- Improve error handling and logging
- Use a constant for expected result count
+// GetLvsNameByUUID retrieves the logical volume store name for a given UUID. +// Returns an empty string if the store is not found or an error occurs. func GetLvsNameByUUID(spdkClient *spdkclient.Client, lvsUUID string) string { + if spdkClient == nil { + logrus.Error("nil SPDK client provided to GetLvsNameByUUID") + return "" + } + if lvsUUID == "" { + logrus.Error("empty UUID provided to GetLvsNameByUUID") + return "" + } + var lvsList []spdktypes.LvstoreInfo lvsList, err := spdkClient.BdevLvolGetLvstore("", lvsUUID) if err != nil { + logrus.WithError(err).Errorf("failed to get lvstore for UUID=%s", lvsUUID) return "" } - if len(lvsList) != 1 { - // consider it as failed to get the correct lvs + + const expectedResults = 1 + if len(lvsList) != expectedResults { + logrus.Errorf("expected %d lvstore result(s), got %d for UUID=%s", + expectedResults, len(lvsList), lvsUUID) return "" } return lvsList[0].Name }pkg/api/types.go (2)
98-101: Consider simplifying the BackingImageName assignmentThe empty string check is redundant since assigning an empty string would have the same effect.
-if r.BackingImageName != "" { - res.BackingImageName = r.BackingImageName -} +res.BackingImageName = r.BackingImageName
127-129: Consider simplifying the BackingImageName assignment for consistencySimilar to the previous comment, the empty string check can be simplified.
-if r.BackingImageName != "" { - res.BackingImageName = r.BackingImageName -} +res.BackingImageName = r.BackingImageNamepkg/client/client.go (2)
Line range hint
56-71: Update parameter validation to include backingImageName.The parameter validation should be updated to handle the new
backingImageNameparameter consistently with other parameters.Apply this diff to improve the validation:
func (c *SPDKClient) ReplicaCreate(name, lvsName, lvsUUID string, specSize uint64, portCount int32, backingImageName string) (*api.Replica, error) { - if name == "" || lvsName == "" || lvsUUID == "" { + if name == "" || lvsName == "" || lvsUUID == "" || backingImageName == "" { - return nil, fmt.Errorf("failed to start SPDK replica: missing required parameters") + return nil, fmt.Errorf("failed to start SPDK replica: missing required parameters (name, lvsName, lvsUUID, or backingImageName)") }
886-887: Improve error messages in parameter validation.The error messages for parameter validation could be more specific about which parameters are missing.
Apply these changes to improve error messages:
- return nil, fmt.Errorf("failed to start SPDK backing image: missing required parameters") + return nil, fmt.Errorf("failed to start SPDK backing image: missing required parameters (name, backingImageUUID, checksum, lvsUUID, or size)") - return fmt.Errorf("failed to delete SPDK backingImage: missing required parameter") + return fmt.Errorf("failed to delete SPDK backingImage: missing required parameters (name or lvsUUID)") - return nil, fmt.Errorf("failed to get SPDK BackingImage: missing required parameter") + return nil, fmt.Errorf("failed to get SPDK BackingImage: missing required parameters (name or lvsUUID)")Also applies to: 909-910, 925-926
pkg/spdk/server.go (3)
51-53: Enhance the backing image map documentationThe comment "We store BackingImage in each lvstore" could be more descriptive about the map's key structure and purpose.
- // We store BackingImage in each lvstore + // Map of backing image snapshot logical volume names to BackingImage objects + // Key format: "bi-<name>-disk-<lvsUUID>"
1580-1581: Address TODO comment with implementation detailsThe TODO comment about handling existing items should be replaced with actual implementation.
Would you like me to help implement the "already exists" check and return logic?
1598-1631: Enhance input validation in BackingImageCreateWhile basic validation is present, consider adding validation for:
- Maximum size limits
- Valid UUID format check
- Checksum format validation
func (s *Server) BackingImageCreate(ctx context.Context, req *spdkrpc.BackingImageCreateRequest) (ret *spdkrpc.BackingImage, err error) { if req.Name == "" { return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image name is required") } + if len(req.Name) > 256 { + return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image name exceeds maximum length of 256 characters") + } if req.BackingImageUuid == "" { return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image UUID is required") } + if !isValidUUID(req.BackingImageUuid) { + return nil, grpcstatus.Error(grpccodes.InvalidArgument, "invalid backing image UUID format") + }pkg/spdk/replica.go (1)
1967-1967: Remove [DEBUG] prefix from error loggingThe error logging contains a [DEBUG] prefix which should be removed as it's not consistent with the codebase's logging style.
- logrus.WithError(err).Errorf("[DEBUG] failed to resize the rebuildingLvolUUID %v to size %v", rebuildingLvolUUID, r.SpecSize) + logrus.WithError(err).Errorf("failed to resize the rebuildingLvolUUID %v to size %v", rebuildingLvolUUID, r.SpecSize)pkg/spdk/backing_image.go (5)
694-694: Fix typographical error in error messageThere's a typo in the error message: "failed to unsexpose" should be "failed to unexpose".
Apply this diff to fix the typo:
return errors.Wrapf(err, "failed to unsexpose the source backing image %v", bi.Name) + return errors.Wrapf(err, "failed to unexpose the source backing image %v", bi.Name)
575-575: Use restrictive file permissions when opening filesOpening the file with permissions
0666makes it readable and writable by all users, which may be a security concern. Consider using0600to restrict access to the current user.Apply this diff:
- headFh, err := os.OpenFile(headInitiator.Endpoint, os.O_RDWR, 0666) + headFh, err := os.OpenFile(headInitiator.Endpoint, os.O_RDWR, 0600)
675-675: Use restrictive file permissions when opening filesOpening the file with permissions
0666makes it readable and writable by all users, which may be a security concern. Consider using0600to restrict access to the current user.Apply this diff:
- srcFh, err := os.OpenFile(initiator.Endpoint, os.O_RDWR, 0666) + srcFh, err := os.OpenFile(initiator.Endpoint, os.O_RDWR, 0600)
342-343: Include error details when loggingWhen logging errors, include the error details to aid in debugging.
Apply this diff:
if err = spdkClient.StopExposeBdev(helpertypes.GetNQN(backingImageSnapLvolName)); err != nil { - logrus.Errorf("Failed to unexpose lvol bdev %v", backingImageSnapLvolName) + logrus.WithError(err).Errorf("Failed to unexpose lvol bdev %v", backingImageSnapLvolName) }
395-395: Include error details when loggingWhen logging errors, include the error details to aid in debugging.
Apply this diff:
if err = spdkClient.StopExposeBdev(helpertypes.GetNQN(backingImageTempHeadName)); err != nil { - logrus.Errorf("Failed to unexpose lvol bdev %v after failed to create backing image", backingImageTempHeadName) + logrus.WithError(err).Errorf("Failed to unexpose lvol bdev %v after failed to create backing image", backingImageTempHeadName) }pkg/spdk/engine.go (4)
1336-1338: Potentially Misleading Error MessageAt lines 1336-1338, if
ancestorSnapshotNameorlatestSnapshotNameare empty, the error message includes empty strings, which could be confusing.Improve the error message to clearly indicate which snapshot name is missing:
if ancestorSnapshotName == "" || latestSnapshotName == "" { - return []*api.Lvol{}, fmt.Errorf("cannot find the ancestor snapshot %s or latest snapshot %s from RW replica %s snapshot map during engine replica add", ancestorSnapshotName, latestSnapshotName, srcReplicaName) + missingNames := []string{} + if ancestorSnapshotName == "" { + missingNames = append(missingNames, "ancestor snapshot") + } + if latestSnapshotName == "" { + missingNames = append(missingNames, "latest snapshot") + } + return []*api.Lvol{}, fmt.Errorf("cannot find the %s in replica %s during engine replica add", strings.Join(missingNames, " and "), srcReplicaName) }
754-755: Redundant Check forhasSnapshotAt lines 754-755, the variable
hasSnapshotis set but not effectively used in the subsequent logic, which could lead to confusion.Evaluate whether the
hasSnapshotflag is necessary in this context or if the logic can be simplified without it.
1337-1337: Typo in Function NameThe function
getRebuildingSnapshotListis used, but there's an inconsistency in naming that could cause confusion.Ensure consistent naming conventions for functions to enhance readability. If the function is intended to retrieve a list, names like
fetchRebuildingSnapshotListorretrieveRebuildingSnapshotsmight be clearer.
705-705: Redundant Call tocheckAndUpdateInfoFromReplicaNoLockAt line 705,
e.checkAndUpdateInfoFromReplicaNoLock()is called multiple times within the same method. This may lead to unnecessary overhead.Consider refactoring the code to minimize redundant calls to this method, possibly by caching the results or restructuring the code flow.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (179)
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/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/munnerz/goautoneg/LICENSEis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/Makefileis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/README.txtis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/autoneg.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/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 (13)
pkg/api/types.go(6 hunks)pkg/client/client.go(3 hunks)pkg/spdk/backing_image.go(1 hunks)pkg/spdk/backup.go(1 hunks)pkg/spdk/engine.go(5 hunks)pkg/spdk/replica.go(11 hunks)pkg/spdk/server.go(15 hunks)pkg/spdk/types.go(4 hunks)pkg/spdk/util.go(2 hunks)pkg/spdk_test.go(9 hunks)pkg/types/types.go(1 hunks)pkg/util/http_handler.go(1 hunks)pkg/util/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/spdk/backup.go
- pkg/spdk/types.go
- pkg/spdk_test.go
- pkg/util/util.go
🧰 Additional context used
🪛 GitHub Check: CodeFactor
pkg/spdk/backing_image.go
[notice] 382-612: pkg/spdk/backing_image.go#L382-L612
Complex Method
pkg/util/http_handler.go
[notice] 94-167: pkg/util/http_handler.go#L94-L167
Complex Method
🔇 Additional comments (24)
pkg/types/types.go (3)
33-36: LGTM! Clean addition of backing image instance type
The addition of InstanceTypeBackingImage follows the existing pattern and supports the PR objective of implementing backing image functionality.
38-49: Document the state machine transitions
While the states are well-defined, it would be valuable to add documentation describing:
- Valid state transitions
- Conditions triggering each transition
- Actions performed in each state
This will help maintain consistency as the codebase evolves.
States ReadyForTransfer and FailedAndCleanUp appear unused
33-56: Verify implementation completeness
Let's verify the implementation of the new types and constants across the codebase.
✅ Verification successful
Let me gather more information about the state transitions since the previous results didn't show any switch statements handling the backing image states.
Implementation is complete and well-structured
The verification shows a complete and well-structured implementation of the backing image types and states:
- The
InstanceTypeBackingImageis properly integrated into the SPDK server's broadcaster system and instance handling - State transitions are comprehensively handled in
pkg/spdk/backing_image.gowith clear progression:- Pending → Starting → InProgress → Ready (success path)
- Failed states are properly handled with error messages
- State validations are in place for operations
- Backing image snapshot attributes are consistently used for metadata storage and retrieval
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify implementation completeness of backing image types
# Check usage of new instance type
echo "Checking InstanceTypeBackingImage usage:"
rg "InstanceTypeBackingImage" --type go -A 3
# Check state transition handling
echo -e "\nChecking state transition handling:"
ast-grep --pattern 'switch $x.(BackingImageState) {
$$$
}'
# Check snapshot attribute usage
echo -e "\nChecking snapshot attribute usage:"
rg "BackingImageSnapshotAttr" --type go -A 2
Length of output: 4457
Script:
#!/bin/bash
# Check state handling in backing image related files
echo "Checking state handling in backing image files:"
rg "BackingImageState" --type go -A 5
# Check if there are any other state transition patterns
echo -e "\nChecking other state transition patterns:"
ast-grep --pattern 'if $x.State == $state {
$$$
}'
# Look for backing image related files
echo -e "\nListing backing image related files:"
fd "backing.*image.*\.go$"
Length of output: 7547
pkg/util/http_handler.go (5)
65-91:
Add security validations for downloads
The download method needs several security measures:
- URL validation (same as GetSizeFromURL)
- Content length validation to prevent DoS attacks
- Content type validation
Add these validations:
func (h *HTTPHandler) DownloadFromURL(ctx context.Context, url string, outFh *os.File, updater ProgressUpdater) (written int64, err error) {
+ // Validate URL (same validation as GetSizeFromURL)
+ if err := validateURL(url); err != nil {
+ return 0, err
+ }
+
ctx, cancel := context.WithCancel(ctx)
defer cancel()
rr, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return 0, err
}
client := NewDownloadHttpClient()
resp, err := client.Do(rr)
if err != nil {
return 0, err
}
defer resp.Body.Close()
+ // Validate content length
+ contentLength := resp.ContentLength
+ if contentLength > MaxDownloadSize {
+ return 0, fmt.Errorf("file size %d exceeds maximum allowed size %d", contentLength, MaxDownloadSize)
+ }
+
+ // Validate content type
+ contentType := resp.Header.Get("Content-Type")
+ if !isAllowedContentType(contentType) {
+ return 0, fmt.Errorf("content type %s is not allowed", contentType)
+ }
+
if resp.StatusCode != http.StatusOK {Likely invalid or redundant comment.
93-166: 🛠️ Refactor suggestion
Improve robustness of IdleTimeoutCopy
The method has several areas for improvement:
- Potential goroutine leak if function returns early
- Complex error handling could be simplified
- Missing documentation for parameters and behavior
Consider these improvements:
+// IdleTimeoutCopy copies data from src to dst with idle timeout monitoring.
+// Parameters:
+// ctx: Context for cancellation
+// cancel: CancelFunc to cancel the context
+// src: Source reader that will be closed when done
+// dst: Destination writer that supports seeking
+// updater: Progress reporter for copying progress
+// writeZero: If true, zero bytes are written; if false, they are skipped
+// Returns the number of bytes copied and any error encountered.
func IdleTimeoutCopy(ctx context.Context, cancel context.CancelFunc, src io.ReadCloser, dst io.WriteSeeker, updater ProgressUpdater, writeZero bool) (copied int64, err error) {
- writeSeekCh := make(chan int64, 100)
+ // Buffer size matches DownloadBufferSize to prevent blocking
+ writeSeekCh := make(chan int64, DownloadBufferSize)
+
+ // Create done channel to prevent goroutine leak
+ done := make(chan struct{})
defer close(writeSeekCh)
+ defer close(done)
go func() {
+ defer t.Stop()
t := time.NewTimer(HTTPTimeout)
done := false
for !done {
select {
+ case <-done:
+ return
case <-ctx.Done():
done = trueLikely invalid or redundant comment.
24-27:
Critical: Method signature mismatch
The Handler interface's DownloadFromURL method signature doesn't match its implementation. The interface expects (url, outFh *os.File) but the implementation uses (url string, outFh *os.File).
Apply this fix:
type Handler interface {
GetSizeFromURL(url string) (fileSize int64, err error)
- DownloadFromURL(ctx context.Context, url, outFh *os.File, updater ProgressUpdater) (written int64, err error)
+ DownloadFromURL(ctx context.Context, url string, outFh *os.File, updater ProgressUpdater) (written int64, err error)
}Likely invalid or redundant comment.
176-187: 🛠️ Refactor suggestion
Add timeout configuration to HTTP client
The HTTP client should have timeout configuration to prevent hanging connections.
func NewDownloadHttpClient() http.Client {
return http.Client{
+ Timeout: HTTPTimeout,
CheckRedirect: func(req *http.Request, via []*http.Request) error {
removeReferer(req)
+ if len(via) >= 10 {
+ return fmt.Errorf("stopped after 10 redirects")
+ }
return nil
},
}
}Likely invalid or redundant comment.
31-63:
Add URL validation to prevent SSRF attacks
The method needs URL validation to prevent Server-Side Request Forgery (SSRF) attacks. This is especially important as this code is part of a backing image system.
Add URL validation:
func (h *HTTPHandler) GetSizeFromURL(url string) (size int64, err error) {
+ // Validate URL
+ parsedURL, err := url.Parse(url)
+ if err != nil {
+ return 0, fmt.Errorf("invalid URL: %v", err)
+ }
+
+ // Only allow HTTP/HTTPS schemes
+ if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
+ return 0, fmt.Errorf("unsupported URL scheme: %s", parsedURL.Scheme)
+ }
+
+ // Prevent SSRF by blocking private IP ranges
+ if ip := net.ParseIP(parsedURL.Hostname()); ip != nil {
+ if ip.IsLoopback() || ip.IsPrivate() {
+ return 0, fmt.Errorf("access to private/loopback IP addresses is forbidden")
+ }
+ }
+
ctx, cancel := context.WithTimeout(context.Background(), HTTPTimeout)Likely invalid or redundant comment.
pkg/spdk/util.go (2)
18-22: LGTM!
The new imports are properly organized and all are utilized within the codebase.
25-52:
Enhance error handling and input validation
Similar issues were previously identified. The function needs improvements in:
- Error handling:
- Line 49: The
errvariable might be nil when timeout occurs - Discovery and connection errors should be preserved
- Line 49: The
- Input validation for IP and port parameters is missing
Apply this diff to fix the issues:
func connectNVMeTarget(srcIP string, srcPort int32) (string, string, error) {
+ if srcIP == "" {
+ return "", "", errors.New("source IP cannot be empty")
+ }
+ if srcPort <= 0 || srcPort > 65535 {
+ return "", "", errors.Errorf("invalid port number: %d", srcPort)
+ }
+
executor, err := helperutil.NewExecutor(commontypes.ProcDirectory)
if err != nil {
return "", "", errors.Wrapf(err, "failed to create executor")
}
subsystemNQN := ""
controllerName := ""
+ var lastErr error
for r := 0; r < maxNumRetries; r++ {
subsystemNQN, err = nvme.DiscoverTarget(srcIP, strconv.Itoa(int(srcPort)), executor)
if err != nil {
+ lastErr = err
logrus.WithError(err).Errorf("Failed to discover target for with address %v:%v", srcIP, srcPort)
time.Sleep(retryInterval)
continue
}
controllerName, err = nvme.ConnectTarget(srcIP, strconv.Itoa(int(srcPort)), subsystemNQN, executor)
if err != nil {
+ lastErr = err
logrus.WithError(err).Errorf("Failed to connect target with address %v:%v", srcIP, srcPort)
time.Sleep(retryInterval)
continue
}
}
if subsystemNQN == "" || controllerName == "" {
- return "", "", errors.Wrapf(err, "timeout connecting target with address %v:%v", srcIP, srcPort)
+ return "", "", errors.Wrapf(lastErr, "failed to connect target with address %v:%v after %d retries", srcIP, srcPort, maxNumRetries)
}
return subsystemNQN, controllerName, nil
}pkg/api/types.go (3)
16-29: LGTM: Well-structured Replica struct update
The addition of BackingImageName field is properly implemented with consistent naming and tagging conventions.
181-227: LGTM: Well-implemented BackingImage types and conversions
The BackingImage implementation is thorough and well-structured:
- Comprehensive field set covering all necessary attributes
- Proper JSON tags following conventions
- Complete and symmetric conversion functions
- Appropriate use of *Lvol type for Snapshot field
299-311: LGTM: Consistent stream implementation
The BackingImageStream implementation follows the established pattern of other stream types in the codebase, maintaining good consistency.
pkg/spdk/replica.go (7)
53-53: LGTM: BackingImage field addition
The addition of BackingImage *Lvol field to the Replica struct is well-placed and correctly typed for managing backing image references.
141-144: LGTM: Proper backing image name handling in proto conversion
The code correctly extracts and sets the backing image name in the proto response, with appropriate nil checks.
1041-1041: LGTM: Clear documentation of parent handling
The comment clearly documents that a parent can be either a backing image or nil, helping maintainability.
1949-1958: LGTM: Comprehensive backing image snapshot handling
The code correctly handles backing image snapshots by:
- Identifying backing image snapshots
- Extracting required information
- Constructing appropriate lvol names
522-532: LGTM: Well-structured backing image initialization in prepareHead
The function properly initializes backing image related fields and maintains correct relationships in the replica chain.
661-661: LGTM: Create function signature update
The addition of backingImage parameter to Create function properly supports backing image initialization during replica creation.
1952-1955: LGTM: Robust error handling
The error handling is comprehensive with proper error wrapping and descriptive messages that will aid in debugging issues.
pkg/spdk/backing_image.go (3)
123-130: Handle errors from goroutine in Create method
The error from prepareBackingImageSnapshot is assigned but not propagated back to the caller. Consider using a channel or callback mechanism to communicate errors back to the main goroutine.
360-363: 🛠️ Refactor suggestion
Consider non-blocking channel send in UpdateProgress
The current implementation might block if there's no receiver for UpdateCh or if the channel is full. Consider using a non-blocking send to prevent potential deadlocks.
Apply this diff to implement the suggested change:
func (bi *BackingImage) UpdateProgress(processedSize int64) {
bi.updateProgress(processedSize)
- bi.UpdateCh <- nil
+ select {
+ case bi.UpdateCh <- nil:
+ default:
+ // Optionally log or handle the case when the channel is full
+ }
}382-612: 🛠️ Refactor suggestion
Refactor prepareBackingImageSnapshot to reduce complexity
The prepareBackingImageSnapshot method is lengthy and handles multiple responsibilities, making it difficult to maintain and test. Consider breaking it down into smaller, focused methods to improve readability and maintainability.
An example refactor might involve separating concerns such as resource initialization, snapshot creation, and cleanup handling:
func (bi *BackingImage) prepareBackingImageSnapshot(...) error {
+ if err := bi.initializeResources(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID); err != nil {
+ return err
+ }
+ if err := bi.createTemporaryHead(spdkClient); err != nil {
+ return err
+ }
+ if err := bi.populateData(targetFh, fromAddress, srcLvsUUID); err != nil {
+ return err
+ }
+ if err := bi.createFinalSnapshot(spdkClient); err != nil {
+ return err
+ }
return nil
}🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 382-612: pkg/spdk/backing_image.go#L382-L612
Complex Method
pkg/spdk/engine.go (1)
1337-1338:
Undefined Behavior When Accessing Map Elements
At lines 1337-1338, the code accesses rpcSrcReplica.Snapshots[snapshotName] without checking if the key exists. This could result in a runtime error if the snapshot name doesn't exist in the map.
Ensure that the snapshot exists in the map before accessing it:
+ snapshot, exists := rpcSrcReplica.Snapshots[currentSnapshotName]
+ if !exists {
+ return nil, fmt.Errorf("snapshot %s does not exist in replica %s", currentSnapshotName, srcReplicaName)
+ }
- rebuildingSnapshotList = append(rebuildingSnapshotList, rpcSrcReplica.Snapshots[currentSnapshotName])
+ rebuildingSnapshotList = append(rebuildingSnapshotList, snapshot)Likely invalid or redundant comment.
| func (c *SPDKClient) BackingImageExpose(ctx context.Context, name, lvsUUID string) (exposedSnapshotLvolAddress string, err error) { | ||
| if name == "" || lvsUUID == "" { | ||
| return "", fmt.Errorf("failed to expose SPDK backing image: missing required parameter") | ||
| } | ||
|
|
||
| client := c.getSPDKServiceClient() | ||
| resp, err := client.BackingImageExpose(ctx, &spdkrpc.BackingImageGetRequest{ | ||
| Name: name, | ||
| LvsUuid: lvsUUID, | ||
| }) | ||
| if err != nil { | ||
| return "", errors.Wrapf(err, "failed to expose SPDK backing image %v in lvstore: %v", name, lvsUUID) | ||
| } | ||
| return resp.ExposedSnapshotLvolAddress, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and use specific request types for Expose/Unexpose operations.
- The Expose and Unexpose operations are missing timeout context unlike other methods.
- They're reusing BackingImageGetRequest instead of having specific request types.
Apply these changes:
- Add timeout:
func (c *SPDKClient) BackingImageExpose(ctx context.Context, name, lvsUUID string) (exposedSnapshotLvolAddress string, err error) {
if name == "" || lvsUUID == "" {
return "", fmt.Errorf("failed to expose SPDK backing image: missing required parameter")
}
client := c.getSPDKServiceClient()
+ ctx, cancel := context.WithTimeout(ctx, GRPCServiceTimeout)
+ defer cancel()- Use specific request types (requires proto changes):
- resp, err := client.BackingImageExpose(ctx, &spdkrpc.BackingImageGetRequest{
+ resp, err := client.BackingImageExpose(ctx, &spdkrpc.BackingImageExposeRequest{
Name: name,
LvsUuid: lvsUUID,
})Also applies to: 986-1000
pkg/client/client.go
Outdated
| func (c *SPDKClient) BackingImageCreate(name, backingIamgeUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) { | ||
| if name == "" || backingIamgeUUID == "" || checksum == "" || lvsUUID == "" || size == 0 { | ||
| return nil, fmt.Errorf("failed to start SPDK backing image: missing required parameters") | ||
| } | ||
| client := c.getSPDKServiceClient() | ||
| ctx, cancel := context.WithTimeout(context.Background(), GRPCServiceTimeout) | ||
| defer cancel() | ||
|
|
||
| resp, err := client.BackingImageCreate(ctx, &spdkrpc.BackingImageCreateRequest{ | ||
| Name: name, | ||
| BackingImageUuid: backingIamgeUUID, | ||
| LvsUuid: lvsUUID, | ||
| Size: size, | ||
| Checksum: checksum, | ||
| FromAddress: fromAddress, | ||
| SrcLvsUuid: srcLvsUUID, | ||
| }) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to start SPDK backing image") | ||
| } | ||
| return api.ProtoBackingImageToBackingImage(resp), nil | ||
| } |
There was a problem hiding this comment.
Fix typo in parameter name.
There's a typo in the parameter name "backingIamgeUUID" which should be "backingImageUUID".
Apply this diff to fix the typo:
-func (c *SPDKClient) BackingImageCreate(name, backingIamgeUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) {
- if name == "" || backingIamgeUUID == "" || checksum == "" || lvsUUID == "" || size == 0 {
+func (c *SPDKClient) BackingImageCreate(name, backingImageUUID, lvsUUID string, size uint64, checksum string, fromAddress string, srcLvsUUID string) (*api.BackingImage, error) {
+ if name == "" || backingImageUUID == "" || checksum == "" || lvsUUID == "" || size == 0 {Committable suggestion skipped: line range outside the PR's diff.
pkg/spdk/server.go
Outdated
| // Backing image lvol name will be "bi-${biName}-disk-${lvsUUID}" | ||
| // Backing image temp lvol name will be "bi-${biName}-disk-${lvsUUID}-temp-head" | ||
| for lvolName, bdevLvol := range bdevLvolMap { | ||
| if bdevLvol.DriverSpecific.Lvol.Snapshot { | ||
| if bdevLvol.DriverSpecific.Lvol.Snapshot && !IsBackingImageSnapLvolName(lvolName) { | ||
| continue | ||
| } | ||
| if IsBackingImageTempHead(lvolName) { | ||
| if s.backingImageMap[GetBackingImageSnapLvolNameFromTempHeadLvolName(lvolName)] == nil { | ||
| lvsUUID := bdevLvol.DriverSpecific.Lvol.LvolStoreUUID | ||
| CleanupOrphanBackingImageTempHead(spdkClient, lvsUUIDNameMap[lvsUUID], lvolName) | ||
| } | ||
| continue | ||
| } | ||
| if replicaMap[lvolName] != nil { | ||
| continue | ||
| } | ||
| if s.backingImageMap[lvolName] != nil { | ||
| continue | ||
| } | ||
| if IsRebuildingLvol(lvolName) { | ||
| if replicaMap[GetReplicaNameFromRebuildingLvolName(lvolName)] != nil { | ||
| continue | ||
| } | ||
| } | ||
| lvsUUID := bdevLvol.DriverSpecific.Lvol.LvolStoreUUID | ||
| specSize := bdevLvol.NumBlocks * uint64(bdevLvol.BlockSize) | ||
| actualSize := bdevLvol.DriverSpecific.Lvol.NumAllocatedClusters * uint64(defaultClusterSize) | ||
| replicaMap[lvolName] = NewReplica(s.ctx, lvolName, lvsUUIDNameMap[lvsUUID], lvsUUID, specSize, actualSize, s.updateChs[types.InstanceTypeReplica]) | ||
| replicaMapForSync[lvolName] = replicaMap[lvolName] | ||
| if IsBackingImageSnapLvolName(lvolName) { | ||
| lvsUUID := bdevLvol.DriverSpecific.Lvol.LvolStoreUUID | ||
| backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName) | ||
| if err != nil { | ||
| logrus.WithError(err).Errorf("failed to extract backing image name and disk UUID from lvol name %v", lvolName) | ||
| continue | ||
| } | ||
| actualSize := bdevLvol.DriverSpecific.Lvol.NumAllocatedClusters * uint64(defaultClusterSize) | ||
| alias := bdevLvol.Aliases[0] | ||
| expectedChecksum := GetSnapXattr(spdkClient, alias, types.BackingImageSnapshotAttrChecksum) | ||
| backingImageUUID := GetSnapXattr(spdkClient, alias, types.BackingImageSnapshotAttrBackingImageUUID) | ||
| backingImage := NewBackingImage(s.ctx, backingImageName, backingImageUUID, lvsUUID, actualSize, expectedChecksum, s.updateChs[types.InstanceTypeBackingImage]) | ||
| backingImage.Alias = alias | ||
| // For uncahced backing image, we set the state to pending first, so we can distinguish it from the cached but starting backing image | ||
| backingImage.State = types.BackingImageStatePending | ||
| backingImageMapForSync[lvolName] = backingImage | ||
| backingImageMap[lvolName] = backingImage | ||
| } else { | ||
| lvsUUID := bdevLvol.DriverSpecific.Lvol.LvolStoreUUID | ||
| specSize := bdevLvol.NumBlocks * uint64(bdevLvol.BlockSize) | ||
| actualSize := bdevLvol.DriverSpecific.Lvol.NumAllocatedClusters * uint64(defaultClusterSize) | ||
| replicaMap[lvolName] = NewReplica(s.ctx, lvolName, lvsUUIDNameMap[lvsUUID], lvsUUID, specSize, actualSize, s.updateChs[types.InstanceTypeReplica]) | ||
| replicaMapForSync[lvolName] = replicaMap[lvolName] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the verify method for better maintainability
The verify method has grown complex with the addition of backing image verification logic. Consider extracting the backing image verification into a separate method for better maintainability and testability.
func (s *Server) verify() (err error) {
replicaMap := map[string]*Replica{}
replicaMapForSync := map[string]*Replica{}
engineMapForSync := map[string]*Engine{}
backingImageMap := map[string]*BackingImage{}
backingImageMapForSync := map[string]*BackingImage{}
+ if err := s.verifyBackingImages(bdevLvolMap, lvsUUIDNameMap, backingImageMap, backingImageMapForSync); err != nil {
+ return err
+ }
// ... rest of the method
}
+func (s *Server) verifyBackingImages(bdevLvolMap map[string]*spdktypes.BdevInfo,
+ lvsUUIDNameMap map[string]string,
+ backingImageMap map[string]*BackingImage,
+ backingImageMapForSync map[string]*BackingImage) error {
+ // Extract backing image verification logic here
+ return nil
+}Committable suggestion skipped: line range outside the PR's diff.
| func (s *Server) BackingImageExpose(ctx context.Context, req *spdkrpc.BackingImageGetRequest) (ret *spdkrpc.BackingImageExposeResponse, err error) { | ||
| if req.Name == "" { | ||
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image name is required") | ||
| } | ||
| if req.LvsUuid == "" { | ||
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "lvs UUID is required") | ||
| } | ||
| s.RLock() | ||
| bi := s.backingImageMap[GetBackingImageSnapLvolName(req.Name, req.LvsUuid)] | ||
| spdkClient := s.spdkClient | ||
| s.RUnlock() | ||
|
|
||
| if bi == nil { | ||
| return nil, grpcstatus.Errorf(grpccodes.NotFound, "cannot find backing image %v in lvs %v", req.Name, req.LvsUuid) | ||
| } | ||
|
|
||
| exposedSnapshotLvolAddress, err := bi.BackingImageExpose(spdkClient, s.portAllocator) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &spdkrpc.BackingImageExposeResponse{ExposedSnapshotLvolAddress: exposedSnapshotLvolAddress}, nil | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for network-related failures in BackingImageExpose
The method should handle potential network-related failures when exposing the backing image.
func (s *Server) BackingImageExpose(ctx context.Context, req *spdkrpc.BackingImageGetRequest) (ret *spdkrpc.BackingImageExposeResponse, err error) {
+ // Add context with timeout
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
if req.Name == "" {
return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image name is required")
}
// ... existing code ...
exposedSnapshotLvolAddress, err := bi.BackingImageExpose(spdkClient, s.portAllocator)
if err != nil {
+ if errors.Is(err, context.DeadlineExceeded) {
+ return nil, grpcstatus.Error(grpccodes.DeadlineExceeded, "timeout while exposing backing image")
+ }
return nil, err
}
return &spdkrpc.BackingImageExposeResponse{ExposedSnapshotLvolAddress: exposedSnapshotLvolAddress}, nil
}Committable suggestion skipped: line range outside the PR's diff.
| func (s *Server) BackingImageUnexpose(ctx context.Context, req *spdkrpc.BackingImageGetRequest) (ret *emptypb.Empty, err error) { | ||
| if req.Name == "" { | ||
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image name is required") | ||
| } | ||
| if req.LvsUuid == "" { | ||
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "lvs UUID is required") | ||
| } | ||
| s.RLock() | ||
| bi := s.backingImageMap[GetBackingImageSnapLvolName(req.Name, req.LvsUuid)] | ||
| spdkClient := s.spdkClient | ||
| s.RUnlock() | ||
|
|
||
| if bi == nil { | ||
| return nil, grpcstatus.Errorf(grpccodes.NotFound, "cannot find backing image %v in lvs %v", req.Name, req.LvsUuid) | ||
| } | ||
|
|
||
| err = bi.BackingImageUnexpose(spdkClient, s.portAllocator) | ||
| if err != nil { | ||
| return nil, grpcstatus.Error(grpccodes.Internal, errors.Wrapf(err, "failed to unexpose backing image %v in lvs %v", req.Name, req.LvsUuid).Error()) | ||
| } | ||
| return &emptypb.Empty{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup handling in BackingImageUnexpose
The method should ensure proper cleanup of resources even in failure cases.
func (s *Server) BackingImageUnexpose(ctx context.Context, req *spdkrpc.BackingImageGetRequest) (ret *emptypb.Empty, err error) {
+ var cleanupErr error
+ defer func() {
+ if err != nil && cleanupErr != nil {
+ logrus.WithError(cleanupErr).Errorf("failed to cleanup resources after unexpose error for backing image %v", req.Name)
+ }
+ }()
// ... existing code ...
err = bi.BackingImageUnexpose(spdkClient, s.portAllocator)
if err != nil {
+ cleanupErr = s.cleanupExposedResources(bi)
return nil, grpcstatus.Error(grpccodes.Internal, errors.Wrapf(err, "failed to unexpose backing image %v in lvs %v", req.Name, req.LvsUuid).Error())
}
return &emptypb.Empty{}, nil
}Committable suggestion skipped: line range outside the PR's diff.
| func (bi *BackingImage) prepareFromURL(targetFh *os.File, fromAddress string) (err error) { | ||
| httpHandler := util.HTTPHandler{} | ||
|
|
||
| // Parse the base URL into a URL object | ||
| parsedURL, err := url.Parse(fromAddress) | ||
| if err != nil { | ||
| fmt.Println("Error parsing URL:", err) | ||
| return errors.Wrapf(err, "failed to parse the URL %v", fromAddress) | ||
| } | ||
| // Add query parameters | ||
| query := parsedURL.Query() | ||
| query.Set("forV2Creation", "true") | ||
| parsedURL.RawQuery = query.Encode() | ||
|
|
||
| size, err := httpHandler.GetSizeFromURL(parsedURL.String()) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to get the file size from %v", fromAddress) | ||
| } | ||
| if size != int64(bi.Size) { | ||
| return errors.Wrapf(err, "download file %v size %v is not the same as the backing image size %v", fromAddress, size, bi.Size) | ||
| } | ||
|
|
||
| if _, err := httpHandler.DownloadFromURL(bi.ctx, parsedURL.String(), targetFh, bi); err != nil { | ||
| return errors.Wrapf(err, "failed to download the file from %v", fromAddress) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Validate fromAddress to prevent potential security risks
The prepareFromURL function downloads a file from fromAddress without validating the source, which might introduce security risks such as SSRF attacks or downloading from untrusted sources. Consider adding input validation to ensure fromAddress is from a trusted domain and uses HTTPS.
Apply this diff to validate fromAddress:
func (bi *BackingImage) prepareFromURL(targetFh *os.File, fromAddress string) (err error) {
+ // Validate the fromAddress
+ allowedDomains := []string{"trusted-domain.com"}
+ parsedURL, err := url.Parse(fromAddress)
+ if err != nil {
+ return errors.Wrapf(err, "invalid URL %v", fromAddress)
+ }
+ if parsedURL.Scheme != "https" {
+ return fmt.Errorf("fromAddress must use HTTPS")
+ }
+ domainAllowed := false
+ for _, domain := range allowedDomains {
+ if parsedURL.Hostname() == domain {
+ domainAllowed = true
+ break
+ }
+ }
+ if !domainAllowed {
+ return fmt.Errorf("fromAddress domain is not allowed")
+ }
httpHandler := util.HTTPHandler{}
// Add query parameters
query := parsedURL.Query()Committable suggestion skipped: line range outside the PR's diff.
| if hasBackingImage { | ||
| e.log.Warnf("Engine found replica %s does not have a backing image while other replicas have during info update for other replicas", replicaName) | ||
| } else { | ||
| hasSnapshot = true | ||
| for snapshotName, snapApiLvol := range replica.Snapshots { | ||
| if snapApiLvol.Parent == "" { | ||
| replicaAncestorMap[replicaName] = replica.Snapshots[snapshotName] | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| replicaAncestorMap[replicaName] = replica.Head | ||
| if hasSnapshot { | ||
| e.log.Warnf("Engine found replica %s does not have a snapshot while other replicas have during info update for other replicas", replicaName) | ||
| } else { | ||
| replicaAncestorMap[replicaName] = replica.Head |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent Snapshot Handling Across Replicas
The logic from lines 767 to 782 deals with replicas that have snapshots, but there's a possibility of inconsistency when some replicas have snapshots and others do not. This can lead to undefined behavior during operations that depend on snapshot consistency.
Consider standardizing the snapshot handling logic to ensure consistency across all replicas. You might need to enforce that all replicas have the same snapshot structure or handle exceptions where discrepancies occur.
| if hasBackingImage { | ||
| if ancestorApiLvol.Name == types.VolumeHead || IsReplicaSnapshotLvol(replicaName, ancestorApiLvol.Name) { | ||
| continue | ||
| } | ||
| } else { | ||
| if ancestorApiLvol.Name != types.VolumeHead { | ||
| continue | ||
| if hasSnapshot { | ||
| if ancestorApiLvol.Name == types.VolumeHead { | ||
| continue | ||
| } | ||
| } else { | ||
| if ancestorApiLvol.Name != types.VolumeHead { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Logical Error in Ancestor Selection Logic
The code from lines 801 to 813 attempts to select a candidate replica based on certain conditions but may not handle all edge cases, especially when dealing with backing images and snapshots.
Review and refactor the ancestor selection logic to ensure all possible conditions are handled correctly. Consider creating separate functions for handling cases with backing images and snapshots to improve clarity and maintainability.
| @@ -702,7 +702,7 @@ | |||
| func (e *Engine) checkAndUpdateInfoFromReplicaNoLock() { | |||
| replicaMap := map[string]*api.Replica{} | |||
| replicaAncestorMap := map[string]*api.Lvol{} | |||
| // hasBackingImage := false | |||
| hasBackingImage := false | |||
There was a problem hiding this comment.
Missing Error Handling in checkAndUpdateInfoFromReplicaNoLock
The method checkAndUpdateInfoFromReplicaNoLock is called without checking for errors. Since the method now returns an error, it's important to handle it appropriately to avoid unexpected behaviors.
Apply this diff to handle the error:
- e.checkAndUpdateInfoFromReplicaNoLock()
+ if err := e.checkAndUpdateInfoFromReplicaNoLock(); err != nil {
+ e.log.WithError(err).Error("Failed to update replica info")
+ // Handle the error as needed
+ }Committable suggestion skipped: line range outside the PR's diff.
| if replica.BackingImageName != "" { | ||
| hasBackingImage = true | ||
| backingImage, err := replicaServiceCli.BackingImageGet(replica.BackingImageName, replica.LvsUUID) | ||
| if err != nil { | ||
| e.log.WithError(err).Warnf("Failed to get backing image %s with disk UUID %s from replica %s head parent %s, will mark the mode from %v to ERR and continue info update for other replicas", replica.BackingImageName, replica.LvsUUID, replicaName, replica.Head.Parent, replicaStatus.Mode) | ||
| replicaStatus.Mode = types.ModeERR | ||
| return | ||
| } | ||
| replicaAncestorMap[replicaName] = backingImage.Snapshot | ||
| if len(replica.Snapshots) > 0 { |
There was a problem hiding this comment.
Potential Nil Pointer Dereference When Accessing Backing Image
In the block starting at line 753, after calling BackingImageGet, there's a possibility that backingImage could be nil if an error occurs. Accessing backingImage.Snapshot without checking could lead to a nil pointer dereference.
Modify the code to check if backingImage is not nil before accessing its fields:
if err != nil {
// Handle error
}
+ if backingImage == nil {
+ e.log.Warnf("Backing image is nil for replica %s", replicaName)
+ replicaStatus.Mode = types.ModeERR
+ return
+ }
replicaAncestorMap[replicaName] = backingImage.Snapshot📝 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.
| if replica.BackingImageName != "" { | |
| hasBackingImage = true | |
| backingImage, err := replicaServiceCli.BackingImageGet(replica.BackingImageName, replica.LvsUUID) | |
| if err != nil { | |
| e.log.WithError(err).Warnf("Failed to get backing image %s with disk UUID %s from replica %s head parent %s, will mark the mode from %v to ERR and continue info update for other replicas", replica.BackingImageName, replica.LvsUUID, replicaName, replica.Head.Parent, replicaStatus.Mode) | |
| replicaStatus.Mode = types.ModeERR | |
| return | |
| } | |
| replicaAncestorMap[replicaName] = backingImage.Snapshot | |
| if len(replica.Snapshots) > 0 { | |
| if replica.BackingImageName != "" { | |
| hasBackingImage = true | |
| backingImage, err := replicaServiceCli.BackingImageGet(replica.BackingImageName, replica.LvsUUID) | |
| if err != nil { | |
| e.log.WithError(err).Warnf("Failed to get backing image %s with disk UUID %s from replica %s head parent %s, will mark the mode from %v to ERR and continue info update for other replicas", replica.BackingImageName, replica.LvsUUID, replicaName, replica.Head.Parent, replicaStatus.Mode) | |
| replicaStatus.Mode = types.ModeERR | |
| return | |
| } | |
| if backingImage == nil { | |
| e.log.Warnf("Backing image is nil for replica %s", replicaName) | |
| replicaStatus.Mode = types.ModeERR | |
| return | |
| } | |
| replicaAncestorMap[replicaName] = backingImage.Snapshot | |
| if len(replica.Snapshots) > 0 { |
ChanYiLin
force-pushed
the
LH6341_v2_backing_image
branch
from
1f8dc99
to
1ab27fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
pkg/util/http_handler.go (1)
15-18: Consider adjusting timeout and making buffer size configurableThe 4-second timeout (
HTTPTimeout) might be too short for large file downloads. Consider:
- Increasing the timeout or making it configurable based on file size
- Making
DownloadBufferSizeconfigurable for different performance scenariosconst ( - DownloadBufferSize = 1 << 12 - HTTPTimeout = 4 * time.Second + DefaultDownloadBufferSize = 1 << 12 + DefaultHTTPTimeout = 30 * time.Second ) + +type Config struct { + DownloadBufferSize int + HTTPTimeout time.Duration +}pkg/spdk/backing_image.go (1)
31-61: Consider adding documentation for some fieldsWhile the struct is generally well-documented, consider adding documentation for the following fields to improve maintainability:
ProcessedSize: Explain its relationship withSizePort: Document the valid range and usageUpdateCh: Explain the channel's purpose and when it's triggeredpkg/spdk/replica.go (2)
202-203: Consider monitoring filter performanceWhile allowing backing image snapshots in the filter results is necessary, it could potentially impact performance by increasing the result set. Consider monitoring the performance impact in production environments.
1949-1970: LGTM: Comprehensive backing image snapshot handlingThe implementation properly handles backing image snapshots during rebuilding, including name extraction and resizing. Consider adding debug logging before the resize operation to aid in troubleshooting.
if IsBackingImageSnapLvolName(snapshotName) { + logrus.Debugf("Resizing rebuilding lvol %v to size %v for backing image snapshot", rebuildingLvolUUID, r.SpecSize) // Resize the replica head to the spec size if _, err := spdkClient.BdevLvolResize(rebuildingLvolUUID, util.BytesToMiB(r.SpecSize)); err != nil { logrus.WithError(err).Errorf("failed to resize the rebuildingLvolUUID %v to size %v", rebuildingLvolUUID, r.SpecSize) return "", err } }pkg/spdk/server.go (1)
1689-1690: Consider using a different error code when backing image record is missingWhen the backing image logical volume is found but the server record is missing, returning a
NotFounderror may be misleading. Consider returning anInternalerror code to indicate an inconsistency within the server state.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (179)
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/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/munnerz/goautoneg/LICENSEis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/Makefileis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/README.txtis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/autoneg.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/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 (13)
pkg/api/types.go(6 hunks)pkg/client/client.go(3 hunks)pkg/spdk/backing_image.go(1 hunks)pkg/spdk/backup.go(1 hunks)pkg/spdk/engine.go(5 hunks)pkg/spdk/replica.go(11 hunks)pkg/spdk/server.go(15 hunks)pkg/spdk/types.go(4 hunks)pkg/spdk/util.go(2 hunks)pkg/spdk_test.go(9 hunks)pkg/types/types.go(1 hunks)pkg/util/http_handler.go(1 hunks)pkg/util/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/api/types.go
- pkg/spdk/backup.go
- pkg/spdk/engine.go
- pkg/spdk/types.go
- pkg/spdk_test.go
- pkg/util/util.go
🧰 Additional context used
🪛 GitHub Check: CodeFactor
pkg/spdk/backing_image.go
[notice] 382-612: pkg/spdk/backing_image.go#L382-L612
Complex Method
pkg/util/http_handler.go
[notice] 94-167: pkg/util/http_handler.go#L94-L167
Complex Method
🔇 Additional comments (24)
pkg/types/types.go (2)
33-36: LGTM! Clean addition of backing image instance type
The new InstanceTypeBackingImage constant follows the existing naming pattern and integrates well with the existing instance types.
51-56: Verify the hardcoded port count value
The BackingImagePortCount is hardcoded to 1. Please verify if this is sufficient for all use cases and whether it should be configurable.
The snapshot attributes are well-defined and follow a consistent naming pattern.
Let's check the usage of this port count:
✅ Verification successful
Let me gather more context about the backing image implementation to understand the port usage better:
Based on the gathered context, I can now generate the final response:
The hardcoded BackingImagePortCount = 1 is correct and sufficient
The port is used for NVMe initiator connections in a one-to-one mapping, where:
- Each backing image gets exactly one port allocated via
superiorPortAllocator.AllocateRange - The port is used to establish a single NVMe connection for data transfer
- The port is properly released when the backing image is deleted or on error conditions
The single port design is intentional and aligns with the NVMe connection model being used. No configuration is needed as this is an implementation detail rather than a user-facing setting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of BackingImagePortCount
# and check if there are any scenarios requiring multiple ports
# Search for BackingImagePortCount usage
rg "BackingImagePortCount" --type go -B 2 -A 2
# Look for any port-related configuration or dynamic port allocation
rg "port.*backing.*image|backing.*image.*port" --type go
Length of output: 1194
Script:
#!/bin/bash
# Check the backing image struct and related code
ast-grep --pattern 'type BackingImage struct {
$$$
}'
# Look for any network or port-related configuration in backing image
rg "Port.*=|port.*=" -B 2 -A 2 pkg/spdk/backing_image.go
# Check if there are any comments or documentation about the port usage
rg -l "backing.*image" --type go | xargs rg "\/\/" -B 1 -A 1 | grep -i "port"
Length of output: 7458
pkg/util/http_handler.go (4)
93-166: 🛠️ Refactor suggestion
Improve robustness of IdleTimeoutCopy
The current implementation has several areas for improvement:
- Potential goroutine leak on early return
- Arbitrary channel buffer size
- Missing documentation for parameters
This is a duplicate of a previous review comment. The suggested improvements remain valid:
- Add done channel to prevent goroutine leak
- Match buffer size with DownloadBufferSize
- Add parameter documentation
176-187: 🛠️ Refactor suggestion
Add timeout and redirect limits to HTTP client
The HTTP client should have:
- Timeout configuration to prevent hanging connections
- Maximum redirect limit to prevent infinite redirects
This is a duplicate of a previous review comment. The suggested improvements for adding timeout configuration and redirect limits remain valid.
31-63:
Add security measures for URL handling
The GetSizeFromURL method needs additional security measures:
- URL validation to prevent SSRF attacks
- TLS configuration for secure HTTPS connections
+func validateURL(url string) error {
+ parsed, err := url.Parse(url)
+ if err != nil {
+ return fmt.Errorf("invalid URL: %v", err)
+ }
+ if parsed.Scheme != "http" && parsed.Scheme != "https" {
+ return fmt.Errorf("unsupported URL scheme: %s", parsed.Scheme)
+ }
+ return nil
+}
+
func (h *HTTPHandler) GetSizeFromURL(url string) (size int64, err error) {
+ if err := validateURL(url); err != nil {
+ return 0, err
+ }
ctx, cancel := context.WithTimeout(context.Background(), HTTPTimeout)Likely invalid or redundant comment.
64-91:
Add content validation and size limits
The download function should validate:
- Content-Type header to ensure file type safety
- Maximum file size to prevent disk space issues
func (h *HTTPHandler) DownloadFromURL(ctx context.Context, url string, outFh *os.File, updater ProgressUpdater) (written int64, err error) {
+ if err := validateURL(url); err != nil {
+ return 0, err
+ }
+
ctx, cancel := context.WithCancel(ctx)
defer cancel()
+ // Get file size first
+ size, err := h.GetSizeFromURL(url)
+ if err != nil {
+ return 0, err
+ }
+ if size > MaxFileSize {
+ return 0, fmt.Errorf("file size %d exceeds maximum allowed size %d", size, MaxFileSize)
+ }
+
rr, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)Likely invalid or redundant comment.
pkg/spdk/util.go (2)
18-22: LGTM: Imports are well-organized and necessary.
The new imports are properly organized and required for the added functionality.
25-52:
Critical: Improve error handling and input validation
The function has several issues that need to be addressed:
- The error handling needs improvement:
- Line 49: Using potentially nil
errin error wrapping - Lost error context from retries
- Line 49: Using potentially nil
- Missing input validation for IP and port parameters
- Typo in error message: "target for with address"
Apply this diff to fix the issues:
func connectNVMeTarget(srcIP string, srcPort int32) (string, string, error) {
+ if srcIP == "" {
+ return "", "", errors.New("source IP cannot be empty")
+ }
+ if srcPort <= 0 || srcPort > 65535 {
+ return "", "", errors.Errorf("invalid port number: %d", srcPort)
+ }
+
executor, err := helperutil.NewExecutor(commontypes.ProcDirectory)
if err != nil {
return "", "", errors.Wrapf(err, "failed to create executor")
}
subsystemNQN := ""
controllerName := ""
+ var lastErr error
for r := 0; r < maxNumRetries; r++ {
subsystemNQN, err = nvme.DiscoverTarget(srcIP, strconv.Itoa(int(srcPort)), executor)
if err != nil {
+ lastErr = err
- logrus.WithError(err).Errorf("Failed to discover target for with address %v:%v", srcIP, srcPort)
+ logrus.WithError(err).Errorf("Failed to discover target at address %v:%v", srcIP, srcPort)
time.Sleep(retryInterval)
continue
}
controllerName, err = nvme.ConnectTarget(srcIP, strconv.Itoa(int(srcPort)), subsystemNQN, executor)
if err != nil {
+ lastErr = err
logrus.WithError(err).Errorf("Failed to connect target with address %v:%v", srcIP, srcPort)
time.Sleep(retryInterval)
continue
}
}
if subsystemNQN == "" || controllerName == "" {
- return "", "", errors.Wrapf(err, "timeout connecting target with address %v:%v", srcIP, srcPort)
+ return "", "", errors.Wrapf(lastErr, "timeout connecting target with address %v:%v after %d retries", srcIP, srcPort, maxNumRetries)
}
return subsystemNQN, controllerName, nil
}pkg/spdk/backing_image.go (7)
1-29: LGTM!
The package structure and imports are well-organized and appropriate for the functionality being implemented.
645-648:
Add proper resource cleanup
The srcBackingImageServiceCli connection should be properly closed to prevent resource leaks.
Add proper cleanup:
srcBackingImageServiceCli, err := GetServiceClient(fromAddress)
if err != nil {
return errors.Wrapf(err, "failed to init the source backing image spdk service client")
}
+defer srcBackingImageServiceCli.Close()Likely invalid or redundant comment.
659-659:
Handle error from connectNVMeTarget
The error returned by connectNVMeTarget is assigned but never used.
Fix the error handling:
-_, _, err = connectNVMeTarget(srcIP, srcPort)
+if _, _, err := connectNVMeTarget(srcIP, srcPort); err != nil {
+ return errors.Wrapf(err, "failed to connect to NVMe target at %s:%s", srcIP, srcPort)
+}Likely invalid or redundant comment.
360-363: 🛠️ Refactor suggestion
Use non-blocking channel send in UpdateProgress
The current implementation might block if there's no receiver for UpdateCh.
Consider using a non-blocking send:
func (bi *BackingImage) UpdateProgress(processedSize int64) {
bi.updateProgress(processedSize)
- bi.UpdateCh <- nil
+ select {
+ case bi.UpdateCh <- nil:
+ default:
+ bi.log.Debug("Skipped progress update: channel full")
+ }
}Likely invalid or redundant comment.
613-639:
Enhance URL validation in prepareFromURL
The current implementation lacks proper URL validation, which could lead to security issues.
Consider adding validation:
func (bi *BackingImage) prepareFromURL(targetFh *os.File, fromAddress string) (err error) {
+ // Validate URL scheme and host
+ parsedURL, err := url.Parse(fromAddress)
+ if err != nil {
+ return errors.Wrapf(err, "invalid URL format: %v", fromAddress)
+ }
+ if parsedURL.Scheme != "https" {
+ return fmt.Errorf("only HTTPS URLs are allowed, got: %s", parsedURL.Scheme)
+ }
+
+ // Add allowed hosts validation based on configuration
+ if !isAllowedHost(parsedURL.Host) {
+ return fmt.Errorf("host not allowed: %s", parsedURL.Host)
+ }Likely invalid or redundant comment.
123-130:
Improve error handling in goroutine
The error from prepareBackingImageSnapshot is only logged but not propagated back to the caller. This could lead to silent failures.
Consider implementing a proper error propagation mechanism:
-go func() {
- err := bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID)
- if err != nil {
- bi.log.WithError(err).Warnf("Failed to create backing image")
- }
- bi.UpdateCh <- nil
-}()
+errCh := make(chan error, 1)
+go func() {
+ if err := bi.prepareBackingImageSnapshot(spdkClient, superiorPortAllocator, fromAddress, srcLvsUUID); err != nil {
+ bi.Lock()
+ bi.State = types.BackingImageStateFailed
+ bi.ErrorMsg = err.Error()
+ bi.Unlock()
+ errCh <- err
+ }
+ close(errCh)
+ bi.UpdateCh <- nil
+}()Likely invalid or redundant comment.
382-612: 🛠️ Refactor suggestion
Reduce complexity in prepareBackingImageSnapshot
The method is quite complex and handles multiple responsibilities. This makes it harder to maintain and test.
Consider breaking it down into smaller, focused methods:
func (bi *BackingImage) prepareBackingImageSnapshot(...) error {
+ if err := bi.initializeResources(spdkClient); err != nil {
+ return err
+ }
+
+ biTempHeadUUID, err := bi.createAndExposeTempHead(spdkClient, superiorPortAllocator)
+ if err != nil {
+ return err
+ }
+
+ if err := bi.copyDataToTempHead(fromAddress, srcLvsUUID); err != nil {
+ return err
+ }
+
+ return bi.createFinalSnapshot(spdkClient, biTempHeadUUID)
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 382-612: pkg/spdk/backing_image.go#L382-L612
Complex Method
pkg/client/client.go (3)
Line range hint 56-71: LGTM: ReplicaCreate parameter addition is well implemented.
The addition of the backingImageName parameter to both the method signature and RPC request is clean and maintains the existing parameter validation pattern.
885-906: Fix typo in parameter name.
There's a typo in the parameter name "backingIamgeUUID" which should be "backingImageUUID".
970-984: Add timeout and use specific request types for Expose/Unexpose operations.
Also applies to: 986-1000
pkg/spdk/replica.go (5)
53-53: LGTM: Well-structured backing image field addition
The addition of BackingImage *Lvol field to the Replica struct is clean and consistent with the existing codebase patterns.
141-145: LGTM: Safe backing image name extraction
The implementation properly handles the nil case and uses appropriate helper functions to extract the backing image name.
1041-1041: LGTM: Clear documentation for backing image handling
The comment clearly explains the parent handling logic for backing images, improving code maintainability.
522-532: LGTM: Well-structured backing image initialization
The changes to prepareHead properly initialize the backing image state while maintaining consistency with existing patterns.
661-661: Verify all callers handle the new parameter
The addition of the backingImage parameter is a breaking change.
✅ Verification successful
Let me analyze the search results to verify the callers of the Create method.
From the search results, I can see that the Replica.Create method is called in:
- pkg/spdk/server.go:
return r.Create(spdkClient, req.PortCount, s.portAllocator, backingImage)- pkg/client/client.go:
resp, err := client.ReplicaCreate(ctx, &spdkrpc.ReplicaCreateRequest{
Name: name,
LvsName: lvsName,
LvsUuid: lvsUUID,
...
})Let me verify if these callers handle the new backingImage parameter correctly.
Let me check the client-side implementation to complete the verification.
Let me check the complete implementation of the client-side ReplicaCreate to verify how it constructs the request.
Let me check how the ReplicaCreate is called in tests to verify if all test cases have been updated.
Based on the verification results, I can now provide the final response:
All callers properly handle the backingImage parameter
The code review shows that:
- Server-side: The
ReplicaCreatehandler inpkg/spdk/server.gocorrectly validates and passes the backingImage parameter - Client-side: The
ReplicaCreatemethod inpkg/client/client.goaccepts and forwards the backingImageName parameter in the request - Tests: All test cases in
pkg/spdk_test.gohave been updated to pass an empty string ("") for backingImageName, which is valid for test scenarios not requiring backing images
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of the Create method to verify they've been updated
# to handle the new backingImage parameter.
# Search for Create method calls
rg -A 3 "Create\(" | grep -v "func.*Create"
Length of output: 60332
Script:
#!/bin/bash
# Get the full context of the server-side call to verify parameter handling
rg -B 5 -A 5 "r\.Create\(spdkClient, req\.PortCount, s\.portAllocator, backingImage\)"
Length of output: 678
Script:
#!/bin/bash
# Get the full context of the client-side ReplicaCreate call
rg -B 10 -A 5 "func \(c \*SPDKClient\) ReplicaCreate"
Length of output: 860
Script:
#!/bin/bash
# Get the complete implementation of client-side ReplicaCreate
rg -B 10 -A 15 "func \(c \*SPDKClient\) ReplicaCreate"
Length of output: 1420
Script:
#!/bin/bash
# Find all test calls to ReplicaCreate
rg -B 5 -A 5 "ReplicaCreate\(" pkg/spdk_test.go
Length of output: 6281
pkg/spdk/server.go (1)
254-301: Consider refactoring the verify() method for better maintainability
The verify() method has grown complex with the addition of backing image verification logic. Consider extracting the backing image verification into a separate method to improve readability and maintainability.
|
This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏 |
ref: longhorn/longhorn 6341 Signed-off-by: Jack Lin <[email protected]>
ChanYiLin
force-pushed
the
LH6341_v2_backing_image
branch
from
1ab27fd
to
8bf7b22
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
pkg/types/types.go (1)
38-47: Consider documenting state transitionsThe backing image states form a state machine. Consider adding documentation to clarify the valid state transitions:
pending→starting→in-progress→ready- Any state can transition to
failedorunknownpkg/spdk/server.go (1)
254-303: Consider extracting backing image verification logicThe backing image verification logic in the verify method has increased complexity. Consider extracting it into a dedicated method for better maintainability and testability.
func (s *Server) verify() (err error) { // ... existing code ... + if err := s.verifyBackingImages(bdevLvolMap, lvsUUIDNameMap, backingImageMap); err != nil { + return err + } // ... rest of the method ... } +func (s *Server) verifyBackingImages(bdevLvolMap map[string]*spdktypes.BdevInfo, + lvsUUIDNameMap map[string]string, + backingImageMap map[string]*BackingImage) error { + for lvolName, bdevLvol := range bdevLvolMap { + if IsBackingImageSnapLvolName(lvolName) { + lvsUUID := bdevLvol.DriverSpecific.Lvol.LvolStoreUUID + backingImageName, _, err := ExtractBackingImageAndDiskUUID(lvolName) + if err != nil { + return err + } + // ... rest of the backing image verification logic ... + } + } + return nil +}pkg/spdk/replica.go (2)
522-532: Add validation for backing image size compatibilityThe prepareHead method should validate that the backing image size is compatible with the replica spec size.
if backingImage != nil { + if backingImage.Snapshot.SpecSize > r.SpecSize { + return fmt.Errorf("backing image size %d is larger than replica spec size %d", + backingImage.Snapshot.SpecSize, r.SpecSize) + } r.ActiveChain[0] = backingImage.Snapshot r.BackingImage = r.ActiveChain[0] }
1949-1970: Enhance logging for backing image operationsAdd detailed logging for backing image operations to aid in debugging and monitoring.
if IsBackingImageSnapLvolName(snapshotName) { + r.log.Infof("Processing backing image snapshot %s for replica %s", snapshotName, r.Name) backingImageName, _, err := ExtractBackingImageAndDiskUUID(snapshotName) if err != nil { return "", errors.Wrapf(err, "failed to extract backing image name and disk UUID from snapshot name %s", snapshotName) } backingImageSnapLvolName := GetBackingImageSnapLvolName(backingImageName, r.LvsUUID) snapLvolAlias = spdktypes.GetLvolAlias(r.LvsName, backingImageSnapLvolName) + r.log.Infof("Resizing replica head from backing image size to spec size %d", r.SpecSize) if _, err := spdkClient.BdevLvolResize(rebuildingLvolUUID, util.BytesToMiB(r.SpecSize)); err != nil { logrus.WithError(err).Errorf("failed to resize the rebuildingLvolUUID %v to size %v", rebuildingLvolUUID, r.SpecSize) return "", err } + r.log.Infof("Successfully processed backing image snapshot %s", snapshotName) }pkg/spdk/engine.go (1)
828-840: Add validation for ancestor name consistencyEnsure consistent handling of ancestor names across replicas to prevent potential issues.
if candidateReplicaName != "" { candidateReplicaAncestorName := replicaAncestorMap[candidateReplicaName].Name currentReplicaAncestorName := ancestorApiLvol.Name + // Validate ancestor names before extraction + if candidateReplicaAncestorName == "" || currentReplicaAncestorName == "" { + e.log.Warnf("Invalid ancestor names found: candidate=%s, current=%s", + candidateReplicaAncestorName, currentReplicaAncestorName) + continue + } if IsBackingImageSnapLvolName(candidateReplicaAncestorName) { candidateReplicaAncestorName, _, _ = ExtractBackingImageAndDiskUUID(candidateReplicaAncestorName) } if IsBackingImageSnapLvolName(currentReplicaAncestorName) { currentReplicaAncestorName, _, _ = ExtractBackingImageAndDiskUUID(currentReplicaAncestorName) } if candidateReplicaAncestorName != currentReplicaAncestorName { e.log.Warnf("Comparing with replica %s ancestor %s, replica %s has a different and earlier ancestor %s", candidateReplicaName, replicaAncestorMap[candidateReplicaName].Name, replicaName, ancestorApiLvol.Name) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (179)
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/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/munnerz/goautoneg/LICENSEis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/Makefileis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/README.txtis excluded by!vendor/**vendor/github.com/munnerz/goautoneg/autoneg.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/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 (13)
pkg/api/types.go(6 hunks)pkg/client/client.go(3 hunks)pkg/spdk/backing_image.go(1 hunks)pkg/spdk/backup.go(1 hunks)pkg/spdk/engine.go(5 hunks)pkg/spdk/replica.go(11 hunks)pkg/spdk/server.go(15 hunks)pkg/spdk/types.go(4 hunks)pkg/spdk/util.go(2 hunks)pkg/spdk_test.go(9 hunks)pkg/types/types.go(1 hunks)pkg/util/http_handler.go(1 hunks)pkg/util/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/util/util.go
- pkg/spdk/backup.go
- pkg/spdk_test.go
- pkg/spdk/types.go
🧰 Additional context used
🪛 GitHub Check: CodeFactor
pkg/spdk/backing_image.go
[notice] 382-612: pkg/spdk/backing_image.go#L382-L612
Complex Method
pkg/util/http_handler.go
[notice] 94-167: pkg/util/http_handler.go#L94-L167
Complex Method
🔇 Additional comments (26)
pkg/types/types.go (2)
33-36: LGTM: Instance type addition
The addition of InstanceTypeBackingImage is consistent with existing instance types.
49-51: Verify the hardcoded port count limitation
The BackingImagePortCount is hardcoded to 1. This might limit scalability if multiple ports are needed in the future.
✅ Verification successful
Let me gather more context about the backing image implementation and its port usage.
Based on the gathered evidence, I can now provide a final response:
The single port limitation is by design and sufficient for the current architecture
The BackingImagePortCount = 1 is an intentional design choice. The backing image uses a single port for exposing SPDK NVMe-oF target for data synchronization. Each backing image requires exactly one port for:
- Exposing snapshot lvol bdev for syncing
- Managing NVMe-oF subsystem and controller
- Handling port allocation/deallocation during expose/unexpose operations
The current implementation shows no scalability concerns as each backing image operates independently with its own dedicated port. Multiple backing images can coexist, each using their own allocated port.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any references or requirements for multiple ports
# Test: Search for BackingImagePortCount usage and port-related configurations
rg "BackingImagePortCount" -A 5 -B 5
rg "port.*count.*backing.*image" -i
Length of output: 2162
Script:
#!/bin/bash
# Check the implementation details around backing image port usage
ast-grep --pattern 'type BackingImage struct {
$$$
}'
# Check if there are any tests or documentation explaining the port usage
rg -i "backing.*image.*port" -A 5 -B 5
Length of output: 15478
pkg/util/http_handler.go (3)
65-91: 🛠️ Refactor suggestion
Add security validations for URL handling
The DownloadFromURL implementation needs additional security measures:
- URL validation to prevent SSRF attacks
- Content length limits to prevent DoS
func (h *HTTPHandler) DownloadFromURL(ctx context.Context, url string, outFh *os.File, updater ProgressUpdater) (written int64, err error) {
+ // Validate URL
+ if !isValidURL(url) {
+ return 0, fmt.Errorf("invalid URL: %s", url)
+ }
+
+ // Check content length before downloading
+ size, err := h.GetSizeFromURL(url)
+ if err != nil {
+ return 0, err
+ }
+ if size > maxDownloadSize {
+ return 0, fmt.Errorf("file size %d exceeds maximum allowed size %d", size, maxDownloadSize)
+ }Likely invalid or redundant comment.
24-27:
Add input validation for Handler interface
The DownloadFromURL method signature has a potential issue: it accepts a file handle (*os.File) which could lead to resource leaks if not properly closed.
Consider modifying the interface to accept a file path instead:
-DownloadFromURL(ctx context.Context, url, outFh *os.File, updater ProgressUpdater) (written int64, err error)
+DownloadFromURL(ctx context.Context, url, filePath string, updater ProgressUpdater) (written int64, err error)Likely invalid or redundant comment.
176-187: 🛠️ Refactor suggestion
Add timeout configuration to HTTP client
The HTTP client should have timeout configuration to prevent hanging connections.
func NewDownloadHttpClient() http.Client {
return http.Client{
+ Timeout: HTTPTimeout,
CheckRedirect: func(req *http.Request, via []*http.Request) error {
removeReferer(req)
+ if len(via) >= 10 {
+ return fmt.Errorf("stopped after 10 redirects")
+ }
return nil
},
}Likely invalid or redundant comment.
pkg/spdk/util.go (3)
167-174: LGTM: GetSnapXattr implementation
The function has good error handling and logging.
176-195: LGTM: Well-documented GetLvsNameByUUID
The function has:
- Clear documentation
- Input validation
- Proper error handling and logging
- Clear error messages
25-52: 🛠️ Refactor suggestion
Add input validation and improve error handling
The connectNVMeTarget function needs several improvements:
- Input validation for IP and port
- Better error handling for timeouts
func connectNVMeTarget(srcIP string, srcPort int32) (string, string, error) {
+ if srcIP == "" {
+ return "", "", errors.New("source IP cannot be empty")
+ }
+ if srcPort <= 0 || srcPort > 65535 {
+ return "", "", errors.Errorf("invalid port number: %d", srcPort)
+ }
+
executor, err := helperutil.NewExecutor(commontypes.ProcDirectory)
if err != nil {
return "", "", errors.Wrapf(err, "failed to create executor")
}
subsystemNQN := ""
controllerName := ""
+ var lastErr error
for r := 0; r < maxNumRetries; r++ {
subsystemNQN, err = nvme.DiscoverTarget(srcIP, strconv.Itoa(int(srcPort)), executor)
if err != nil {
+ lastErr = err
logrus.WithError(err).Errorf("Failed to discover target for with address %v:%v", srcIP, srcPort)
time.Sleep(retryInterval)
continue
}Likely invalid or redundant comment.
pkg/api/types.go (5)
16-29: LGTM: Replica struct changes
The addition of BackingImageName field to the Replica struct is well-structured and properly tagged.
98-100: LGTM: Proper handling of BackingImageName in conversion functions
The conversion of BackingImageName field is handled correctly in both directions with appropriate empty string checks.
Also applies to: 127-129
181-193: LGTM: Well-structured BackingImage struct
The BackingImage struct is well-designed with comprehensive fields for managing backing image metadata. All fields are properly tagged and follow consistent naming conventions.
195-227: LGTM: Proper conversion functions
The conversion functions between proto and API types are implemented symmetrically and handle all fields correctly.
299-311: LGTM: Consistent stream implementation
The BackingImageStream implementation follows the established pattern used by other stream types in the codebase.
pkg/spdk/backing_image.go (7)
31-61: LGTM: Well-designed BackingImage struct
The struct is well-designed with:
- Proper synchronization using RWMutex
- Well-documented fields
- Clear separation of concerns
83-100: LGTM: Proper constructor implementation
The constructor properly initializes all required fields and sets up logging.
102-133:
Improve error handling in Create method
The error from the goroutine is not properly propagated back to the caller. Consider using a channel to handle errors from the goroutine.
382-611: 🛠️ Refactor suggestion
Consider breaking down complex method
The prepareBackingImageSnapshot method is quite complex and handles multiple responsibilities. Consider breaking it down into smaller, focused methods.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 382-612: pkg/spdk/backing_image.go#L382-L612
Complex Method
645-648:
Ensure proper resource cleanup
The srcBackingImageServiceCli should be properly closed to prevent resource leaks.
659-659:
Fix ineffectual error assignment
The error returned by connectNVMeTarget is assigned but never used.
613-639:
Add URL validation for security
The method downloads files without validating the source URL, which could lead to security risks. Consider adding input validation.
pkg/client/client.go (3)
Line range hint 56-71: LGTM: Proper ReplicaCreate update
The addition of backingImageName parameter is well-integrated into the existing method.
885-906: LGTM: Well-implemented BackingImage CRUD operations
The BackingImage CRUD operations are well-implemented with proper parameter validation and error handling.
970-984: 🛠️ Refactor suggestion
Add timeout and use specific request types
The Expose and Unexpose operations:
- Are missing timeout context unlike other methods
- Are reusing BackingImageGetRequest instead of having specific request types
Also applies to: 986-1000
pkg/spdk/server.go (2)
51-53: LGTM: Clean backing image map initialization
The addition of backingImageMap and its initialization follows the established pattern for other maps in the Server struct.
Also applies to: 99-100
462-471: Verify backing image state before replica creation
While the code checks for backing image existence, it should also verify that the backing image is in a valid state before using it for replica creation.
if backingImage == nil {
return nil, grpcstatus.Error(grpccodes.Internal, "failed to find the backing image in the spdk server")
}
+ if backingImage.State != types.BackingImageStateReady {
+ return nil, grpcstatus.Error(grpccodes.Internal,
+ fmt.Sprintf("backing image %s is not ready, current state: %s",
+ req.BackingImageName, backingImage.State))
+ }pkg/spdk/engine.go (1)
753-762: 🛠️ Refactor suggestion
Enhance error handling for backing image retrieval
Add comprehensive error handling for backing image operations to prevent potential issues.
if replica.BackingImageName != "" {
hasBackingImage = true
backingImage, err := replicaServiceCli.BackingImageGet(replica.BackingImageName, replica.LvsUUID)
if err != nil {
+ if grpcstatus.Code(err) == grpccodes.NotFound {
+ e.log.Warnf("Backing image %s not found, marking replica %s as error",
+ replica.BackingImageName, replicaName)
+ } else {
+ e.log.WithError(err).Errorf("Failed to get backing image %s, marking replica %s as error",
+ replica.BackingImageName, replicaName)
+ }
replicaStatus.Mode = types.ModeERR
return
}
replicaAncestorMap[replicaName] = backingImage.Snapshot
}Likely invalid or redundant comment.
| func (s *Server) BackingImageCreate(ctx context.Context, req *spdkrpc.BackingImageCreateRequest) (ret *spdkrpc.BackingImage, err error) { | ||
| if req.Name == "" { | ||
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image name is required") | ||
| } | ||
| if req.BackingImageUuid == "" { | ||
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image UUID is required") | ||
| } | ||
| if req.Size == uint64(0) { | ||
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "backing image size is required") | ||
| } | ||
| if req.LvsUuid == "" { | ||
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "lvs UUID is required") | ||
| } | ||
| if req.Checksum == "" { | ||
| return nil, grpcstatus.Error(grpccodes.InvalidArgument, "checksum is required") | ||
| } | ||
|
|
||
| // Don't recreate the backing image | ||
| backingImageSnapLvolName := GetBackingImageSnapLvolName(req.Name, req.LvsUuid) | ||
| if _, ok := s.backingImageMap[backingImageSnapLvolName]; ok { | ||
| return nil, grpcstatus.Errorf(grpccodes.AlreadyExists, "backing image %v already exists", req.Name) | ||
| } | ||
|
|
||
| bi, err := s.newBackingImage(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| s.RLock() | ||
| spdkClient := s.spdkClient | ||
| s.RUnlock() | ||
|
|
||
| return bi.Create(spdkClient, s.portAllocator, req.FromAddress, req.SrcLvsUuid) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error cleanup in BackingImage operations
The BackingImageCreate method should ensure proper cleanup of resources in case of errors during creation.
func (s *Server) BackingImageCreate(ctx context.Context, req *spdkrpc.BackingImageCreateRequest) (ret *spdkrpc.BackingImage, err error) {
+ var created bool
+ defer func() {
+ if err != nil && created {
+ if cleanupErr := s.cleanupFailedBackingImage(req.Name, req.LvsUuid); cleanupErr != nil {
+ s.log.WithError(cleanupErr).Error("Failed to cleanup backing image after creation error")
+ }
+ }
+ }()
// ... existing validation code ...
bi, err := s.newBackingImage(req)
if err != nil {
return nil, err
}
+ created = true
return bi.Create(spdkClient, s.portAllocator, req.FromAddress, req.SrcLvsUuid)
}
+func (s *Server) cleanupFailedBackingImage(name, lvsUuid string) error {
+ backingImageSnapLvolName := GetBackingImageSnapLvolName(name, lvsUuid)
+ s.Lock()
+ defer s.Unlock()
+ delete(s.backingImageMap, backingImageSnapLvolName)
+ return nil
+}Committable suggestion skipped: line range outside the PR's diff.
| if r.BackingImage != nil { | ||
| backingImageName, _, _ := ExtractBackingImageAndDiskUUID(r.BackingImage.Name) | ||
| res.BackingImageName = backingImageName | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for backing image name extraction
The backing image name extraction should handle potential errors to avoid silent failures.
if r.BackingImage != nil {
- backingImageName, _, _ := ExtractBackingImageAndDiskUUID(r.BackingImage.Name)
+ backingImageName, _, err := ExtractBackingImageAndDiskUUID(r.BackingImage.Name)
+ if err != nil {
+ logrus.WithError(err).Errorf("Failed to extract backing image name from %v", r.BackingImage.Name)
+ return nil
+ }
res.BackingImageName = backingImageName
}📝 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.
| if r.BackingImage != nil { | |
| backingImageName, _, _ := ExtractBackingImageAndDiskUUID(r.BackingImage.Name) | |
| res.BackingImageName = backingImageName | |
| } | |
| if r.BackingImage != nil { | |
| backingImageName, _, err := ExtractBackingImageAndDiskUUID(r.BackingImage.Name) | |
| if err != nil { | |
| logrus.WithError(err).Errorf("Failed to extract backing image name from %v", r.BackingImage.Name) | |
| return nil | |
| } | |
| res.BackingImageName = backingImageName | |
| } |
ref: longhorn/longhorn#6341
ReadyorFailed), we check if the lvol existsxattrBackingImage *Lvolto thereplicaprepareHead, we add the BackingImage to ther.ActiveChain[0]andr.BackingImager.ActiveChain[0]andr.BackingImageinconstruct()functioncheckAndUpdateInfoFromReplicaNoLock()we check if replica contains the BackingImage to decide which replica we are getting the info fromRebuildingDstSnapshotRevert(),replicaLvolFilterto allow getting the backing image lvol, because we would need it when building thenewChainwhen validating or reconstructing. herecc @shuo-wu