feat(v2 upgrade): support engine live upgrade #241
Conversation
WalkthroughThe pull request introduces a new field, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
pkg/api/types.go (1)
132-132: LGTM! Consider adding field documentation.The new
StandbyTargetPortfield is well-positioned and follows the codebase conventions. Consider adding a comment to document its purpose in the context of live upgrades.+ // StandbyTargetPort is used during live upgrades to maintain service availability StandbyTargetPort int32 `json:"standby_target_port"`pkg/spdk/engine.go (3)
252-252: Possible misuse of variable in log messageAt line 252, the log statement uses
e.ReplicaModeMapto display the replicas being connected. However,e.ReplicaModeMapmight not represent the list of replicas intended for logging.Consider using
replicaBdevList, which contains the list of replica block devices:- e.log.Infof("Connecting all available replicas %+v, then launching raid during engine creation", e.ReplicaModeMap) + e.log.Infof("Connecting all available replicas %+v, then launching raid during engine creation", replicaBdevList)This change will provide more accurate logging information about the replicas being connected.
685-688: Remove commented-out code to improve code clarityThe commented-out code at lines 685-688 is not used and can be removed to enhance readability and maintainability.
Apply this diff to remove the unused code:
- // podIP, err := commonnet.GetIPForPod() - // if err != nil { - // return err - // }If this code is needed for future development, consider adding a comment explaining its purpose or moving it to a separate location.
Line range hint
1881-1881: Typographical error in function namecloseRplicaClientsThere is a typo in the function name
closeRplicaClients. It should becloseReplicaClientsfor clarity and consistency.Apply this diff to correct the function name:
- func (e *Engine) closeRplicaClients(replicaClients map[string]*client.SPDKClient) { + func (e *Engine) closeReplicaClients(replicaClients map[string]*client.SPDKClient) {Make sure to update all references to this function accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!go.modvendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (2)
pkg/api/types.go(2 hunks)pkg/spdk/engine.go(15 hunks)
🧰 Additional context used
🪛 golangci-lint
pkg/spdk/engine.go
315-315: expected declaration, found '<<'
(typecheck)
🔇 Additional comments (5)
pkg/api/types.go (2)
153-153: LGTM! Field mapping is correct.
The StandbyTargetPort field is properly mapped from the protobuf struct to the Engine type.
Line range hint 132-153: Verify protobuf definition exists.
Ensure that the standby_target_port field is properly defined in the SPDK service protobuf files.
pkg/spdk/engine.go (3)
45-45: New field StandbyTargetPort added to Engine struct
The addition of the StandbyTargetPort field to the Engine struct is appropriate. Ensure that this new field is properly initialized and consistently used throughout the codebase to avoid any unexpected behavior.
108-110: Function isNewEngine correctly determines if the engine is new
The isNewEngine function effectively checks if the IP field is empty to determine if the engine instance is new. This is a simple and efficient approach.
618-618: Field StandbyTargetPort included in getWithoutLock
The StandbyTargetPort field is correctly added to the Engine object returned by the getWithoutLock method. This ensures that the field will be included in any serialized representations of the engine's state.
derekbit
force-pushed
the
v2-control-upgrade
branch
from
668e4d2
to
84a18b8
Compare
derekbit
changed the title
derekbit
force-pushed
the
v2-control-upgrade
branch
from
84a18b8
to
c80ba34
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: 1
🧹 Outside diff range and nitpick comments (1)
pkg/spdk/engine.go (1)
Line range hint
2173-2224: Document target port management logicThe target switching logic correctly handles the port assignments, but would benefit from a comment explaining when and why
StandbyTargetPortis reset to 0.Add a comment before the condition:
+ // Reset StandbyTargetPort when switching to the pod's IP as the target, + // since this node is now the primary target if targetIP == podIP { e.TargetPort = targetPort e.StandbyTargetPort = 0 }
🛑 Comments failed to post (1)
pkg/spdk/engine.go (1)
315-315:
⚠️ Potential issueCritical: Resolve merge conflict
There is an unresolved merge conflict marker at line 315 (
<<<<<<< HEAD). This needs to be resolved before the code can be merged.Please resolve the merge conflict by:
- Running
git mergeorgit rebaseto update your branch- Resolving the conflicts by choosing the appropriate code
- Removing all conflict markers (
<<<<<<<,=======,>>>>>>>)🧰 Tools
🪛 golangci-lint
315-315: expected declaration, found '<<'
(typecheck)
derekbit
force-pushed
the
v2-control-upgrade
branch
2 times, most recently
from
b15f126
to
7f0d7e1
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: 1
🧹 Outside diff range and nitpick comments (1)
pkg/spdk/engine.go (1)
Line range hint
4-4: Consider adding tests for live upgrade scenariosWhile the implementation looks solid, consider adding comprehensive test coverage for:
- Standby target creation and cleanup
- Target switchover scenarios
- Error cases during live upgrades
Would you like me to help generate test cases for these scenarios?
🛑 Comments failed to post (1)
pkg/spdk/engine.go (1)
216-218:
⚠️ Potential issueAdd error handling for BdevRaidGet
The empty branch after
BdevRaidGeterror check could lead to silent failures. Consider adding appropriate error handling.Apply this diff to add error handling:
if targetCreationRequired { _, err := spdkClient.BdevRaidGet(e.Name, 0) if err != nil { + if !jsonrpc.IsJSONRPCRespErrorNoSuchDevice(err) { + return nil, errors.Wrapf(err, "failed to get raid bdev %s", e.Name) + } }📝 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 targetCreationRequired { _, err := spdkClient.BdevRaidGet(e.Name, 0) if err != nil { if !jsonrpc.IsJSONRPCRespErrorNoSuchDevice(err) { return nil, errors.Wrapf(err, "failed to get raid bdev %s", e.Name) } }🧰 Tools
🪛 golangci-lint
218-218: SA9003: empty branch
(staticcheck)
derekbit
force-pushed
the
v2-control-upgrade
branch
from
7f0d7e1
to
5b4f847
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
pkg/spdk/engine.go (3)
176-202: Consider simplifying the initialization logicThe current implementation has nested conditions that could be simplified for better readability and maintainability. Consider extracting the logic into separate helper functions.
Example refactor:
- if podIP == initiatorIP && podIP == targetIP { - if e.Port == 0 && e.TargetPort == 0 { - e.log.Info("Creating both initiator and target instances") - initiatorCreationRequired = true - targetCreationRequired = true - } else if e.Port != 0 && e.TargetPort == 0 { - e.log.Info("Creating a target instance") - targetCreationRequired = true - if e.StandbyTargetPort != 0 { - e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) - return e.getWithoutLock(), nil - } - } else { - return nil, fmt.Errorf("invalid initiator and target address for engine %s creation", e.Name) - } + creationMode := determineCreationMode(podIP, initiatorIP, targetIP, e.Port, e.TargetPort) + switch creationMode { + case createBoth: + e.log.Info("Creating both initiator and target instances") + initiatorCreationRequired = true + targetCreationRequired = true + case createTargetOnly: + e.log.Info("Creating a target instance") + targetCreationRequired = true + if e.StandbyTargetPort != 0 { + e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) + return e.getWithoutLock(), nil + } + case createInitiatorOnly: + e.log.Info("Creating an initiator instance") + initiatorCreationRequired = true + default: + return nil, fmt.Errorf("invalid initiator and target address for engine %s creation", e.Name) + }
397-399: Add documentation for standby target creation conditionThe condition for standby target creation could benefit from a comment explaining when and why it's needed.
+ // Create a standby target if we have an active initiator (e.Port != 0) + // but no active target (e.TargetPort == 0) standbyTargetCreationRequired := false if e.Port != 0 && e.TargetPort == 0 { standbyTargetCreationRequired = true }
2169-2173: Enhance error handling for pod IP operationsThe pod IP retrieval and port updates are critical for live upgrades. Consider adding more detailed error messages and logging.
podIP, err := commonnet.GetIPForPod() if err != nil { - return err + return errors.Wrapf(err, "failed to get pod IP for engine %s target switchover", e.Name) } if targetIP == podIP { + e.log.Infof("Target IP matches pod IP, updating ports: target=%d, standby=0", targetPort) e.TargetPort = targetPort e.StandbyTargetPort = 0 }Also applies to: 2216-2220
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/api/types.go(2 hunks)pkg/spdk/engine.go(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/api/types.go
🔇 Additional comments (5)
pkg/spdk/engine.go (5)
45-45: LGTM: Added StandbyTargetPort field for standby target management
The new field is well-integrated with the existing port fields and supports the live upgrade functionality.
108-110: LGTM: Clear and focused helper function
The function provides a clean way to determine if an engine is new based on its IP state.
204-207: LGTM: Clear initialization of engine IPs
The initialization logic correctly sets the IP values for new engines using the helper function.
476-485: LGTM: Clear port assignment logic
The code correctly handles port assignment for both initiator and target instances, including the standby target scenario.
Line range hint 2169-2220: Verify concurrent access during target switchover
The target switchover process involves multiple steps and state changes. Ensure there are no race conditions when:
- Multiple goroutines attempt switchover simultaneously
- A switchover occurs during engine creation/deletion
- Pod IP changes during switchover
✅ Verification successful
Let me generate additional verification scripts to check for proper locking mechanisms and concurrent access patterns.
Based on the verification results, I can now generate the final response:
Proper locking mechanisms are in place during target switchover
The code correctly handles concurrent access during target switchover:
SwitchOverTargetmethod acquires the engine lock before state modifications- Related operations like
Create,Delete, andDeleteTargetalso use proper locking - State changes (TargetPort, StandbyTargetPort) are protected by the mutex
The original concerns about race conditions are addressed by the existing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential concurrent access patterns
ast-grep --pattern $'func \(.*\) SwitchOverTarget\(.*\) {
$$$
e.Lock()
$$$
}'
# Check for other methods that might interact with target state
rg -A 5 "e\.(TargetPort|StandbyTargetPort)"
Length of output: 6650
Script:
#!/bin/bash
# Check for all methods that modify engine state and their locking patterns
ast-grep --pattern $'func \(e \*Engine\) $_($$) {
$$$
e.Lock()
$$$
}'
# Check for methods that might be called during switchover
rg -A 5 "func.*Engine.*\b(Create|Delete|Switch|Connect|Disconnect|Reload)" pkg/spdk/
Length of output: 1794
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
========================================
+ Coverage 8.39% 8.88% +0.49%
========================================
Files 21 21
Lines 7150 7216 +66
========================================
+ Hits 600 641 +41
- Misses 6470 6493 +23
- Partials 80 82 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
derekbit
force-pushed
the
v2-control-upgrade
branch
from
0aae707
to
3c6c4a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
pkg/spdk/engine_test.go (1)
98-115: Improve test execution logic and error handling.The test execution can be improved in several ways:
- Use test case name in log message instead of array index:
-c.Logf("testing TestCheckInitiatorAndTargetCreationRequirements.%v", testName) +c.Logf("testing TestCheckInitiatorAndTargetCreationRequirements: %s", testCase.name)
- Consider extracting engine creation to a helper function for better reusability:
func createTestEngine(port, targetPort, standbyTargetPort int32, name string) *Engine { return &Engine{ Port: port, TargetPort: targetPort, StandbyTargetPort: standbyTargetPort, Name: name, log: logrus.New(), } }
- Add validation of log messages to ensure proper error logging.
pkg/spdk/engine.go (3)
Line range hint
398-498: Consider enhancing error handling in deferred functionThe deferred function at line 429 contains complex logic for initiator assignment. Consider extracting this into a separate helper function for better maintainability and error handling.
Consider refactoring like this:
+ func (e *Engine) assignInitiator(initiator *nvme.Initiator, dmDeviceBusy bool, standbyTargetCreationRequired bool) { + if !standbyTargetCreationRequired { + e.initiator = initiator + e.dmDeviceBusy = dmDeviceBusy + e.Endpoint = initiator.GetEndpoint() + e.log = e.log.WithFields(logrus.Fields{ + "endpoint": e.Endpoint, + "port": e.Port, + "targetPort": e.TargetPort, + }) + } + e.log.Infof("Finished handling frontend for engine: %+v", e) + } defer func() { if err == nil { - if !standbyTargetCreationRequired { - e.initiator = initiator - e.dmDeviceBusy = dmDeviceBusy - ... - } - e.log.Infof("Finished handling frontend for engine: %+v", e) + e.assignInitiator(initiator, dmDeviceBusy, standbyTargetCreationRequired) } }()
208-214: Consider standardizing error handling patternsWhile error handling is generally good, consider standardizing the approach across the codebase. For example, some errors are wrapped with context while others are returned directly.
Consider consistently using error wrapping with context:
- if err != nil { - return nil, err - } + if err != nil { + return nil, errors.Wrapf(err, "failed to get pod IP during engine creation") + }Also applies to: 2181-2184
261-262: Address TODO comment about loggingThe TODO comment indicates that log messages need improvement. Consider enhancing the log message to include more context about the operation being performed.
Consider improving the log message:
- // TODO: improve the log message - e.log.Infof("Connecting all available replicas %+v, then launching raid during engine creation", e.ReplicaStatusMap) + e.log.Infof("Connecting available replicas for engine %s (replicas: %+v) and initializing RAID configuration", e.Name, e.ReplicaStatusMap)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
pkg/api/types.go(2 hunks)pkg/spdk/engine.go(14 hunks)pkg/spdk/engine_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/api/types.go
🔇 Additional comments (7)
pkg/spdk/engine_test.go (2)
1-10: LGTM!
The imports are clean and necessary. The test file is properly structured using the check.v1 testing framework.
1-115: Verify test coverage for live upgrade functionality.
The test file introduces basic validation for the standby target port, which is a step towards supporting live upgrades. However, to ensure robust support for engine live upgrades (as per PR objectives), consider adding more comprehensive test scenarios that validate:
- The complete upgrade workflow
- State transitions during upgrade
- Error recovery scenarios
- Upgrade rollback cases
✅ Verification successful
Based on the code inspection results, I can now generate the final response:
Test coverage appears sufficient for the current implementation scope
The test coverage aligns well with the current implementation, which focuses on the standby port allocation and basic validation aspects of the upgrade functionality. The test cases appropriately verify:
- Standby target port validation
- Port allocation scenarios
- Creation requirements for different network configurations
While the initial review suggested adding more test scenarios, the current implementation scope is primarily focused on the port management aspect of upgrades, which is adequately tested. The suggested additional test scenarios would be premature as the complete upgrade workflow implementation is not yet present in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for upgrade-related code
# Look for other upgrade-related tests
rg -l "upgrade|standby" --type go
# Check for upgrade-related functions in the engine implementation
ast-grep --pattern 'func $_($$) {
$$$
standby
$$$
}'
Length of output: 633
Script:
#!/bin/bash
# Let's examine the engine implementation and related test files more closely
# Check engine.go for standby-related functions and their implementation
rg "standby|Standby" pkg/spdk/engine.go -B 2 -A 2
# Look for upgrade-related functions in engine.go
rg "upgrade|Upgrade" pkg/spdk/engine.go -B 2 -A 2
# Check the API types related to upgrade
rg "upgrade|Upgrade" pkg/api/types.go -B 2 -A 2
Length of output: 2242
pkg/spdk/engine.go (5)
37-49: LGTM: Engine struct changes
The addition of StandbyTargetPort field is well-placed and consistent with other port-related fields in the struct.
108-110: LGTM: Clear and focused helper function
The isNewEngine function provides a clean abstraction for checking engine initialization state.
Line range hint 2181-2232: LGTM: Well-structured target switchover logic
The changes properly handle the standby target port during switchover operations, with appropriate error handling and state management.
112-142:
Fix redundant condition check
The function contains a redundant check for e.StandbyTargetPort != 0 at line 123, which is unnecessary since it's already within a block that checks for e.Port != 0 && e.TargetPort == 0.
Apply this diff to fix the redundant condition:
if e.Port != 0 && e.TargetPort == 0 {
e.log.Info("Creating a target instance")
- if e.StandbyTargetPort != 0 {
e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort)
- } else {
targetCreationRequired = true
- }
}Likely invalid or redundant comment.
112-142: Ensure test coverage for new functionality
The new checkInitiatorAndTargetCreationRequirements function contains complex logic for standby target handling. Please ensure adequate test coverage is added.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
pkg/spdk_test.go (1)
Line range hint
225-322: Document the new upgrade mechanism architectureThe changes suggest a significant shift in how engine upgrades are handled, moving from a parameter-based approach to potentially a more sophisticated mechanism. To ensure maintainability and clarity:
- Consider adding documentation that explains the new upgrade mechanism
- Update or create integration tests that specifically validate the live upgrade scenarios
- Consider adding a migration guide for users who might be affected by this architectural change
Would you like assistance in creating:
- Documentation template for the new upgrade mechanism?
- Integration test scenarios for live upgrades?
- Migration guide for existing users?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/client/client.go(1 hunks)pkg/spdk_test.go(2 hunks)
🔇 Additional comments (3)
pkg/spdk_test.go (2)
225-225: Verify the removal of upgradeRequired parameter
The EngineCreate call has been updated to remove the upgradeRequired parameter. This change should be verified against the engine creation logic to ensure it aligns with the new live upgrade support feature.
#!/bin/bash
# Description: Verify the changes in EngineCreate signature across the codebase
# Check for any remaining references to upgradeRequired in engine creation
rg -l "EngineCreate.*upgradeRequired"
# Check the updated EngineCreate method signature
ast-grep --pattern 'func $_EngineCreate($_) {
$$$
}'250-322:
Clarify the status of the commented test implementation
The TestSPDKEngineCreateWithUpgradeRequired test appears to contain important validations for the engine upgrade functionality. If this test is being replaced or modified as part of the live upgrade feature:
- Please clarify if this test will be replaced with new test cases
- Ensure the new test coverage includes validation of both initiator and target properties
- Consider preserving the valuable assertions in any new test implementation
pkg/client/client.go (1)
Line range hint 474-492: LGTM: Method implementation remains consistent
The implementation maintains proper parameter validation, error handling, and gRPC timeout management while removing the upgrade-related parameter.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/spdk/engine.go (1)
112-142: Improve error messages and add test coverageThe function has clear logic but could benefit from:
- More descriptive error messages that explain why the addresses are invalid
- Unit tests to verify the complex branching logic
Consider improving the error messages:
-err = fmt.Errorf("invalid initiator and target address for engine %s creation", e.Name) +err = fmt.Errorf("invalid initiator address %s and target address %s for engine %s creation: addresses must match pod IP %s", initiatorIP, targetIP, e.Name, podIP)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
pkg/spdk/engine.go(14 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/spdk/engine.go
[warning] 108-109: pkg/spdk/engine.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 208-214: pkg/spdk/engine.go#L208-L214
Added lines #L208 - L214 were not covered by tests
[warning] 216-220: pkg/spdk/engine.go#L216-L220
Added lines #L216 - L220 were not covered by tests
[warning] 223-226: pkg/spdk/engine.go#L223-L226
Added lines #L223 - L226 were not covered by tests
[warning] 228-231: pkg/spdk/engine.go#L228-L231
Added lines #L228 - L231 were not covered by tests
[warning] 263-264: pkg/spdk/engine.go#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 269-269: pkg/spdk/engine.go#L269
Added line #L269 was not covered by tests
[warning] 271-271: pkg/spdk/engine.go#L271
Added line #L271 was not covered by tests
[warning] 282-285: pkg/spdk/engine.go#L282-L285
Added lines #L282 - L285 were not covered by tests
[warning] 292-292: pkg/spdk/engine.go#L292
Added line #L292 was not covered by tests
[warning] 302-303: pkg/spdk/engine.go#L302-L303
Added lines #L302 - L303 were not covered by tests
[warning] 306-315: pkg/spdk/engine.go#L306-L315
Added lines #L306 - L315 were not covered by tests
[warning] 401-401: pkg/spdk/engine.go#L401
Added line #L401 was not covered by tests
[warning] 411-413: pkg/spdk/engine.go#L411-L413
Added lines #L411 - L413 were not covered by tests
[warning] 422-429: pkg/spdk/engine.go#L422-L429
Added lines #L422 - L429 were not covered by tests
[warning] 431-442: pkg/spdk/engine.go#L431-L442
Added lines #L431 - L442 were not covered by tests
[warning] 444-444: pkg/spdk/engine.go#L444
Added line #L444 was not covered by tests
[warning] 448-462: pkg/spdk/engine.go#L448-L462
Added lines #L448 - L462 were not covered by tests
[warning] 464-466: pkg/spdk/engine.go#L464-L466
Added lines #L464 - L466 were not covered by tests
[warning] 468-468: pkg/spdk/engine.go#L468
Added line #L468 was not covered by tests
[warning] 471-473: pkg/spdk/engine.go#L471-L473
Added lines #L471 - L473 were not covered by tests
[warning] 476-476: pkg/spdk/engine.go#L476
Added line #L476 was not covered by tests
[warning] 479-482: pkg/spdk/engine.go#L479-L482
Added lines #L479 - L482 were not covered by tests
[warning] 484-495: pkg/spdk/engine.go#L484-L495
Added lines #L484 - L495 were not covered by tests
[warning] 501-502: pkg/spdk/engine.go#L501-L502
Added lines #L501 - L502 were not covered by tests
[warning] 510-511: pkg/spdk/engine.go#L510-L511
Added lines #L510 - L511 were not covered by tests
[warning] 515-515: pkg/spdk/engine.go#L515
Added line #L515 was not covered by tests
[warning] 517-519: pkg/spdk/engine.go#L517-L519
Added lines #L517 - L519 were not covered by tests
[warning] 628-628: pkg/spdk/engine.go#L628
Added line #L628 was not covered by tests
🔇 Additional comments (3)
pkg/spdk/engine.go (3)
Line range hint 2183-2234: LGTM: Robust target switchover implementation
The target switchover implementation:
- Properly validates pod IP
- Handles standby target port appropriately for live upgrades
- Includes fallback logic for error cases
The error handling and state management look solid.
Line range hint 400-520: Add test coverage for critical paths
The live upgrade functionality includes critical paths that should be tested:
- Engine creation with standby target
- Target switchover scenarios
- Error handling and recovery paths
Consider adding integration tests to verify the end-to-end functionality.
#!/bin/bash
# Check existing test coverage
rg -l "func Test.*Target" 🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 422-429: pkg/spdk/engine.go#L422-L429
Added lines #L422 - L429 were not covered by tests
[warning] 431-442: pkg/spdk/engine.go#L431-L442
Added lines #L431 - L442 were not covered by tests
[warning] 444-444: pkg/spdk/engine.go#L444
Added line #L444 was not covered by tests
[warning] 448-462: pkg/spdk/engine.go#L448-L462
Added lines #L448 - L462 were not covered by tests
[warning] 464-466: pkg/spdk/engine.go#L464-L466
Added lines #L464 - L466 were not covered by tests
[warning] 468-468: pkg/spdk/engine.go#L468
Added line #L468 was not covered by tests
[warning] 471-473: pkg/spdk/engine.go#L471-L473
Added lines #L471 - L473 were not covered by tests
[warning] 476-476: pkg/spdk/engine.go#L476
Added line #L476 was not covered by tests
[warning] 479-482: pkg/spdk/engine.go#L479-L482
Added lines #L479 - L482 were not covered by tests
[warning] 484-495: pkg/spdk/engine.go#L484-L495
Added lines #L484 - L495 were not covered by tests
[warning] 501-502: pkg/spdk/engine.go#L501-L502
Added lines #L501 - L502 were not covered by tests
[warning] 510-511: pkg/spdk/engine.go#L510-L511
Added lines #L510 - L511 were not covered by tests
[warning] 515-515: pkg/spdk/engine.go#L515
Added line #L515 was not covered by tests
[warning] 517-519: pkg/spdk/engine.go#L517-L519
Added lines #L517 - L519 were not covered by tests
108-110: Add documentation and tests for isNewEngine
The function logic is clear but would benefit from documentation explaining what constitutes a "new" engine. Additionally, consider adding unit tests to cover this helper function.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-109: pkg/spdk/engine.go#L108-L109
Added lines #L108 - L109 were not covered by tests
derekbit
force-pushed
the
v2-control-upgrade
branch
from
2adf734
to
f654d45
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
pkg/spdk/engine_test.go (2)
11-23: Add documentation for the test function and struct fields.Consider adding documentation to improve code maintainability:
- Add a function comment explaining the purpose and methodology of the test
- Document the struct fields, especially explaining the relationship between different IP addresses and ports
+// TestCheckInitiatorAndTargetCreationRequirements validates the logic for determining +// when new initiator and target instances need to be created based on various +// network configurations and port settings. func (s *TestSuite) TestCheckInitiatorAndTargetCreationRequirements(c *C) { testCases := []struct { + // name is a descriptive identifier for the test case name string + // podIP is the IP address of the pod running the engine podIP string + // initiatorIP is the IP address for the SPDK initiator initiatorIP string + // targetIP is the IP address for the SPDK target targetIP string
24-96: Add test cases for port validation and error conditions.While the current test cases cover basic scenarios, consider adding these additional cases to improve coverage:
Port validation:
- Test negative port values
- Test port numbers exceeding valid range
- Test zero port with non-zero standby port
Error conditions for standby target:
- Test transition from active to standby when active port is in use
- Test invalid port combinations
}, + { + name: "Negative port values", + podIP: "192.168.1.1", + initiatorIP: "192.168.1.1", + targetIP: "192.168.1.1", + port: -1, + targetPort: -8000, + standbyTargetPort: -8001, + expectedInitiatorCreationRequired: false, + expectedTargetCreationRequired: false, + expectedError: fmt.Errorf("invalid port values"), + }, + { + name: "Invalid port combination", + podIP: "192.168.1.1", + initiatorIP: "192.168.1.1", + targetIP: "192.168.1.1", + port: 0, + targetPort: 0, + standbyTargetPort: 8001, + expectedInitiatorCreationRequired: false, + expectedTargetCreationRequired: false, + expectedError: fmt.Errorf("invalid port combination: standby port without active port"), + }, }pkg/spdk_test.go (1)
Line range hint
1-322: Consider documenting the upgrade architecture changesThe changes suggest a significant architectural shift in how upgrades are handled:
- The
EngineCreatesignature has been simplified- The dedicated upgrade test has been removed
- The basic engine creation test remains unchanged
Consider:
- Adding documentation to explain the new upgrade architecture
- Providing test coverage for the new upgrade mechanism
- Including examples of how to perform live upgrades with the new implementation
Would you like help with:
- Creating documentation for the new upgrade architecture?
- Designing new test cases for the upgrade functionality?
pkg/spdk/engine.go (4)
108-110: Add test coverage for the new helper function.The
isNewEnginefunction lacks test coverage. Consider adding unit tests to verify the behavior for both new and existing engines.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-109: pkg/spdk/engine.go#L108-L109
Added lines #L108 - L109 were not covered by tests
223-226: Consider adding debug level logging.While the logging improvements are good, consider adding debug level logs to help with troubleshooting, especially around the decision points for initiator/target creation.
Also applies to: 306-315
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 223-226: pkg/spdk/engine.go#L223-L226
Added lines #L223 - L226 were not covered by tests
455-468: Document the retry mechanism for NVMe device info loading.The retry mechanism is well implemented, but consider adding a comment explaining why the retry is necessary and what conditions might trigger retries.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 464-466: pkg/spdk/engine.go#L464-L466
Added lines #L464 - L466 were not covered by tests
[warning] 468-468: pkg/spdk/engine.go#L468
Added line #L468 was not covered by tests
Line range hint
208-519: Improve test coverage for new functionality.Several new functions and code paths lack test coverage, including:
isNewEnginecheckInitiatorAndTargetCreationRequirements- Parts of the frontend handling logic
- Target switchover logic
Consider adding comprehensive test cases to verify the behavior of these new components, especially around the upgrade scenarios.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 422-429: pkg/spdk/engine.go#L422-L429
Added lines #L422 - L429 were not covered by tests
[warning] 431-442: pkg/spdk/engine.go#L431-L442
Added lines #L431 - L442 were not covered by tests
[warning] 444-444: pkg/spdk/engine.go#L444
Added line #L444 was not covered by tests
[warning] 448-462: pkg/spdk/engine.go#L448-L462
Added lines #L448 - L462 were not covered by tests
[warning] 464-466: pkg/spdk/engine.go#L464-L466
Added lines #L464 - L466 were not covered by tests
[warning] 468-468: pkg/spdk/engine.go#L468
Added line #L468 was not covered by tests
[warning] 471-473: pkg/spdk/engine.go#L471-L473
Added lines #L471 - L473 were not covered by tests
[warning] 476-476: pkg/spdk/engine.go#L476
Added line #L476 was not covered by tests
[warning] 479-482: pkg/spdk/engine.go#L479-L482
Added lines #L479 - L482 were not covered by tests
[warning] 484-495: pkg/spdk/engine.go#L484-L495
Added lines #L484 - L495 were not covered by tests
[warning] 501-502: pkg/spdk/engine.go#L501-L502
Added lines #L501 - L502 were not covered by tests
[warning] 510-511: pkg/spdk/engine.go#L510-L511
Added lines #L510 - L511 were not covered by tests
[warning] 515-515: pkg/spdk/engine.go#L515
Added line #L515 was not covered by tests
[warning] 517-519: pkg/spdk/engine.go#L517-L519
Added lines #L517 - L519 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
pkg/api/types.go(2 hunks)pkg/client/client.go(1 hunks)pkg/spdk/engine.go(14 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/api/types.go
- pkg/client/client.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/spdk/engine.go
[warning] 108-109: pkg/spdk/engine.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 208-214: pkg/spdk/engine.go#L208-L214
Added lines #L208 - L214 were not covered by tests
[warning] 216-220: pkg/spdk/engine.go#L216-L220
Added lines #L216 - L220 were not covered by tests
[warning] 223-226: pkg/spdk/engine.go#L223-L226
Added lines #L223 - L226 were not covered by tests
[warning] 228-231: pkg/spdk/engine.go#L228-L231
Added lines #L228 - L231 were not covered by tests
[warning] 263-264: pkg/spdk/engine.go#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 269-269: pkg/spdk/engine.go#L269
Added line #L269 was not covered by tests
[warning] 271-271: pkg/spdk/engine.go#L271
Added line #L271 was not covered by tests
[warning] 282-285: pkg/spdk/engine.go#L282-L285
Added lines #L282 - L285 were not covered by tests
[warning] 292-292: pkg/spdk/engine.go#L292
Added line #L292 was not covered by tests
[warning] 302-303: pkg/spdk/engine.go#L302-L303
Added lines #L302 - L303 were not covered by tests
[warning] 306-315: pkg/spdk/engine.go#L306-L315
Added lines #L306 - L315 were not covered by tests
[warning] 401-401: pkg/spdk/engine.go#L401
Added line #L401 was not covered by tests
[warning] 411-413: pkg/spdk/engine.go#L411-L413
Added lines #L411 - L413 were not covered by tests
[warning] 422-429: pkg/spdk/engine.go#L422-L429
Added lines #L422 - L429 were not covered by tests
[warning] 431-442: pkg/spdk/engine.go#L431-L442
Added lines #L431 - L442 were not covered by tests
[warning] 444-444: pkg/spdk/engine.go#L444
Added line #L444 was not covered by tests
[warning] 448-462: pkg/spdk/engine.go#L448-L462
Added lines #L448 - L462 were not covered by tests
[warning] 464-466: pkg/spdk/engine.go#L464-L466
Added lines #L464 - L466 were not covered by tests
[warning] 468-468: pkg/spdk/engine.go#L468
Added line #L468 was not covered by tests
[warning] 471-473: pkg/spdk/engine.go#L471-L473
Added lines #L471 - L473 were not covered by tests
[warning] 476-476: pkg/spdk/engine.go#L476
Added line #L476 was not covered by tests
[warning] 479-482: pkg/spdk/engine.go#L479-L482
Added lines #L479 - L482 were not covered by tests
[warning] 484-495: pkg/spdk/engine.go#L484-L495
Added lines #L484 - L495 were not covered by tests
[warning] 501-502: pkg/spdk/engine.go#L501-L502
Added lines #L501 - L502 were not covered by tests
[warning] 510-511: pkg/spdk/engine.go#L510-L511
Added lines #L510 - L511 were not covered by tests
[warning] 515-515: pkg/spdk/engine.go#L515
Added line #L515 was not covered by tests
[warning] 517-519: pkg/spdk/engine.go#L517-L519
Added lines #L517 - L519 were not covered by tests
[warning] 628-628: pkg/spdk/engine.go#L628
Added line #L628 was not covered by tests
🔇 Additional comments (6)
pkg/spdk_test.go (1)
250-322:
Reconsider removing the upgrade test case
The commented-out test TestSPDKEngineCreateWithUpgradeRequired appears to contain valuable test coverage for upgrade scenarios, which aligns with the PR's objective of supporting live upgrades. Consider:
- Why was this test case removed?
- Is there alternative test coverage for the upgrade functionality?
- Should this test be adapted rather than removed to support the new upgrade implementation?
Let's check for other upgrade-related test coverage:
#!/bin/bash
# Description: Search for other upgrade-related tests that might provide coverage
# Search for upgrade-related test files and functions
rg -l "Test.*Upgrade"
# Search for upgrade-related assertions
rg "c\.Assert.*[Uu]pgrade"pkg/spdk/engine.go (5)
37-49: LGTM! New field addition looks good.
The StandbyTargetPort field is properly integrated with the existing port-related fields and follows the same type pattern.
208-214: LGTM! Good error handling and control flow.
The integration of checkInitiatorAndTargetCreationRequirements is well done with proper error handling and early returns.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 208-214: pkg/spdk/engine.go#L208-L214
Added lines #L208 - L214 were not covered by tests
400-401: LGTM! Good error handling and port management.
The frontend handling changes properly manage both regular and standby target ports with appropriate error handling.
Also applies to: 411-413
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 401-401: pkg/spdk/engine.go#L401
Added line #L401 was not covered by tests
123-128:
Redundant condition check on e.StandbyTargetPort
The nested condition checking e.StandbyTargetPort != 0 is redundant since it's already checked in the outer if statement.
Apply this diff to fix the redundant condition:
if e.StandbyTargetPort != 0 {
- if e.StandbyTargetPort != 0 {
e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort)
return e.getWithoutLock(), nil
- }
}Likely invalid or redundant comment.
2183-2187: LGTM! Good target switchover implementation.
The target switchover logic properly handles port management and state updates. The pod IP check is a good addition for ensuring correct target configuration.
Also applies to: 2230-2234
✅ Verification successful
Pod IP validation is properly implemented across the codebase
The GetIPForPod() usage is consistently implemented across the codebase with proper error handling. The target switchover logic correctly validates pod IP against target and initiator IPs to determine the appropriate instance creation. The implementation in the reviewed code segment aligns with the established pattern seen in other files (replica.go, backup.go) and is followed by proper instance handling logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the pod IP handling logic
ast-grep --pattern $'podIP, err := commonnet.GetIPForPod()
if err != nil {
$$$
}
if podIP == targetIP {
$$$
}'
Length of output: 115
Script:
#!/bin/bash
# Let's search for GetIPForPod usage and its surrounding context
rg -A 5 "GetIPForPod" --type go
# Also search for any IP comparison logic
rg -A 5 "podIP.*==" --type go
Length of output: 2957
derekbit
force-pushed
the
v2-control-upgrade
branch
from
6b4b3c7
to
5d51017
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
pkg/spdk/server.go (1)
878-880: Consider adding documentation for live upgrade feature.Since this change introduces support for live upgrades, it would be helpful to add documentation explaining:
- The live upgrade process
- The role of initiator and target addresses
- Requirements and prerequisites for live upgrades
pkg/spdk/engine.go (1)
45-45: Add documentation for the StandbyTargetPort fieldPlease add a comment explaining that this field is used for managing standby targets during live upgrades.
+ // StandbyTargetPort is the port used by the standby target instance during live upgrades StandbyTargetPort int32
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
pkg/spdk/engine.go(14 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk/server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/spdk/engine_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/spdk/engine.go
[warning] 108-109: pkg/spdk/engine.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 144-144: pkg/spdk/engine.go#L144
Added line #L144 was not covered by tests
[warning] 207-213: pkg/spdk/engine.go#L207-L213
Added lines #L207 - L213 were not covered by tests
[warning] 215-219: pkg/spdk/engine.go#L215-L219
Added lines #L215 - L219 were not covered by tests
[warning] 222-225: pkg/spdk/engine.go#L222-L225
Added lines #L222 - L225 were not covered by tests
[warning] 227-230: pkg/spdk/engine.go#L227-L230
Added lines #L227 - L230 were not covered by tests
[warning] 262-263: pkg/spdk/engine.go#L262-L263
Added lines #L262 - L263 were not covered by tests
[warning] 268-268: pkg/spdk/engine.go#L268
Added line #L268 was not covered by tests
[warning] 270-270: pkg/spdk/engine.go#L270
Added line #L270 was not covered by tests
[warning] 281-284: pkg/spdk/engine.go#L281-L284
Added lines #L281 - L284 were not covered by tests
[warning] 291-291: pkg/spdk/engine.go#L291
Added line #L291 was not covered by tests
[warning] 301-302: pkg/spdk/engine.go#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-313: pkg/spdk/engine.go#L305-L313
Added lines #L305 - L313 were not covered by tests
[warning] 399-399: pkg/spdk/engine.go#L399
Added line #L399 was not covered by tests
[warning] 409-411: pkg/spdk/engine.go#L409-L411
Added lines #L409 - L411 were not covered by tests
[warning] 420-427: pkg/spdk/engine.go#L420-L427
Added lines #L420 - L427 were not covered by tests
[warning] 429-440: pkg/spdk/engine.go#L429-L440
Added lines #L429 - L440 were not covered by tests
[warning] 442-442: pkg/spdk/engine.go#L442
Added line #L442 was not covered by tests
[warning] 446-460: pkg/spdk/engine.go#L446-L460
Added lines #L446 - L460 were not covered by tests
[warning] 462-464: pkg/spdk/engine.go#L462-L464
Added lines #L462 - L464 were not covered by tests
[warning] 466-466: pkg/spdk/engine.go#L466
Added line #L466 was not covered by tests
[warning] 469-471: pkg/spdk/engine.go#L469-L471
Added lines #L469 - L471 were not covered by tests
[warning] 474-474: pkg/spdk/engine.go#L474
Added line #L474 was not covered by tests
[warning] 477-480: pkg/spdk/engine.go#L477-L480
Added lines #L477 - L480 were not covered by tests
[warning] 482-493: pkg/spdk/engine.go#L482-L493
Added lines #L482 - L493 were not covered by tests
[warning] 499-500: pkg/spdk/engine.go#L499-L500
Added lines #L499 - L500 were not covered by tests
[warning] 508-509: pkg/spdk/engine.go#L508-L509
Added lines #L508 - L509 were not covered by tests
[warning] 513-513: pkg/spdk/engine.go#L513
Added line #L513 was not covered by tests
[warning] 515-517: pkg/spdk/engine.go#L515-L517
Added lines #L515 - L517 were not covered by tests
🔇 Additional comments (5)
pkg/spdk/server.go (1)
879-879: LGTM! Verify live upgrade functionality.
The changes to the Create method call properly support engine live upgrades by passing the necessary parameters for initiator and target addresses.
Run the following script to verify the engine creation parameters:
✅ Verification successful
Let me gather more context about the engine creation implementation to verify the changes properly.
Let me search for the Create method implementation specifically since it wasn't found in the previous search.
Let me try one more search with a different pattern to locate the Create method implementation.
Based on the gathered evidence, I can now provide the final response:
The changes to engine creation are correctly implemented for live upgrade support
The code changes are verified to be correct and consistent with the live upgrade functionality:
- The
Createmethod properly handles initiator and target addresses as parameters - The implementation includes proper validation and switching logic for target addresses
- The changes are integrated with the existing port allocation and engine management system
Key validations:
- Proper validation of target address requirement
- Handling of target switching via
isSwitchOverTargetRequired - Safety check for local target with matching addresses
- Proper connection/disconnection handling during target operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the engine creation parameters and usage
# Test: Search for engine creation calls to ensure consistent parameter usage
rg -A 5 "e\.Create\("
Length of output: 371
Script:
#!/bin/bash
# Search for the Engine struct and Create method implementation
ast-grep --pattern 'type Engine struct {
$$$
}'
echo "---"
ast-grep --pattern 'func ($_) Create($$$) {
$$$
}'
Length of output: 7425
Script:
#!/bin/bash
# Search for the Create method implementation and its usage
rg -A 10 "func.*Create\(.*spdkClient.*ReplicaAddressMap"
echo "---"
# Search for any tests related to the Create functionality
rg -A 5 "TestEngine.*Create"
Length of output: 104
Script:
#!/bin/bash
# Search for Create method in engine.go with broader context
rg -A 15 "func \(e \*Engine\) Create" pkg/spdk/engine.go
echo "---"
# Search for the usage of InitiatorAddress and TargetAddress in the codebase
rg "InitiatorAddress|TargetAddress" pkg/spdk/
Length of output: 2137
pkg/spdk/engine.go (4)
Line range hint 2181-2232: Enhance validation and error handling for target switchover
The target switchover logic is critical for live upgrades. Consider adding:
- Validation for the target port range
- Retries for port allocation failures
- Rollback mechanism for failed switchovers
- Metrics/events for monitoring switchover operations
Consider implementing a state machine pattern to manage the switchover lifecycle:
- PREPARING -> SWITCHING -> COMPLETED/FAILED
- Add metrics for each state transition
- Implement automatic rollback on failure
#!/bin/bash
# Check port validation
rg "ValidatePort|PortRange|port.*valid"
# Check retry/rollback mechanisms
rg "Retry|Rollback|recover|revert" Line range hint 1-2232: Improve overall test coverage
The code changes implement critical functionality for live upgrades but have significant gaps in test coverage. Please add:
- Integration tests for the live upgrade flow
- Unit tests for new helper functions
- Error scenario tests
- Performance benchmarks for the switchover process
#!/bin/bash
# Check current test coverage
fd -e go -e test 🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 420-427: pkg/spdk/engine.go#L420-L427
Added lines #L420 - L427 were not covered by tests
[warning] 429-440: pkg/spdk/engine.go#L429-L440
Added lines #L429 - L440 were not covered by tests
[warning] 442-442: pkg/spdk/engine.go#L442
Added line #L442 was not covered by tests
[warning] 446-460: pkg/spdk/engine.go#L446-L460
Added lines #L446 - L460 were not covered by tests
[warning] 462-464: pkg/spdk/engine.go#L462-L464
Added lines #L462 - L464 were not covered by tests
[warning] 466-466: pkg/spdk/engine.go#L466
Added line #L466 was not covered by tests
[warning] 469-471: pkg/spdk/engine.go#L469-L471
Added lines #L469 - L471 were not covered by tests
[warning] 474-474: pkg/spdk/engine.go#L474
Added line #L474 was not covered by tests
[warning] 477-480: pkg/spdk/engine.go#L477-L480
Added lines #L477 - L480 were not covered by tests
[warning] 482-493: pkg/spdk/engine.go#L482-L493
Added lines #L482 - L493 were not covered by tests
[warning] 499-500: pkg/spdk/engine.go#L499-L500
Added lines #L499 - L500 were not covered by tests
[warning] 508-509: pkg/spdk/engine.go#L508-L509
Added lines #L508 - L509 were not covered by tests
[warning] 513-513: pkg/spdk/engine.go#L513
Added line #L513 was not covered by tests
[warning] 515-517: pkg/spdk/engine.go#L515-L517
Added lines #L515 - L517 were not covered by tests
123-127:
Remove redundant condition check
The nested condition checking e.StandbyTargetPort != 0 is redundant as it's already checked in the outer if statement.
- if e.StandbyTargetPort != 0 {
- e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort)
- } else {
- targetCreationRequired = true
- }
+ e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort)Likely invalid or redundant comment.
409-411: Add test coverage for frontend handling
The frontend handling logic, particularly around standby target creation and error handling, lacks test coverage. Please add tests to verify:
- Standby target creation logic
- Error handling scenarios
- Endpoint and logging updates
Also applies to: 420-440
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 409-411: pkg/spdk/engine.go#L409-L411
Added lines #L409 - L411 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
pkg/spdk/engine.go (1)
45-45: Add documentation forStandbyTargetPortfieldTo improve code readability and maintain consistency, consider adding a comment to document the purpose of the new
StandbyTargetPortfield in theEnginestruct.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/spdk/engine.go(14 hunks)pkg/spdk/engine_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/spdk/engine_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/spdk/engine.go
[warning] 108-109: pkg/spdk/engine.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 144-144: pkg/spdk/engine.go#L144
Added line #L144 was not covered by tests
[warning] 207-213: pkg/spdk/engine.go#L207-L213
Added lines #L207 - L213 were not covered by tests
[warning] 215-219: pkg/spdk/engine.go#L215-L219
Added lines #L215 - L219 were not covered by tests
[warning] 222-225: pkg/spdk/engine.go#L222-L225
Added lines #L222 - L225 were not covered by tests
[warning] 227-230: pkg/spdk/engine.go#L227-L230
Added lines #L227 - L230 were not covered by tests
[warning] 262-263: pkg/spdk/engine.go#L262-L263
Added lines #L262 - L263 were not covered by tests
[warning] 268-268: pkg/spdk/engine.go#L268
Added line #L268 was not covered by tests
[warning] 270-270: pkg/spdk/engine.go#L270
Added line #L270 was not covered by tests
[warning] 281-284: pkg/spdk/engine.go#L281-L284
Added lines #L281 - L284 were not covered by tests
[warning] 291-291: pkg/spdk/engine.go#L291
Added line #L291 was not covered by tests
[warning] 301-302: pkg/spdk/engine.go#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-313: pkg/spdk/engine.go#L305-L313
Added lines #L305 - L313 were not covered by tests
[warning] 399-399: pkg/spdk/engine.go#L399
Added line #L399 was not covered by tests
[warning] 409-411: pkg/spdk/engine.go#L409-L411
Added lines #L409 - L411 were not covered by tests
[warning] 420-427: pkg/spdk/engine.go#L420-L427
Added lines #L420 - L427 were not covered by tests
[warning] 429-440: pkg/spdk/engine.go#L429-L440
Added lines #L429 - L440 were not covered by tests
[warning] 442-442: pkg/spdk/engine.go#L442
Added line #L442 was not covered by tests
[warning] 446-460: pkg/spdk/engine.go#L446-L460
Added lines #L446 - L460 were not covered by tests
[warning] 462-464: pkg/spdk/engine.go#L462-L464
Added lines #L462 - L464 were not covered by tests
[warning] 466-466: pkg/spdk/engine.go#L466
Added line #L466 was not covered by tests
[warning] 469-471: pkg/spdk/engine.go#L469-L471
Added lines #L469 - L471 were not covered by tests
[warning] 474-474: pkg/spdk/engine.go#L474
Added line #L474 was not covered by tests
[warning] 477-480: pkg/spdk/engine.go#L477-L480
Added lines #L477 - L480 were not covered by tests
[warning] 482-493: pkg/spdk/engine.go#L482-L493
Added lines #L482 - L493 were not covered by tests
[warning] 499-500: pkg/spdk/engine.go#L499-L500
Added lines #L499 - L500 were not covered by tests
[warning] 508-509: pkg/spdk/engine.go#L508-L509
Added lines #L508 - L509 were not covered by tests
[warning] 513-513: pkg/spdk/engine.go#L513
Added line #L513 was not covered by tests
[warning] 515-517: pkg/spdk/engine.go#L515-L517
Added lines #L515 - L517 were not covered by tests
🔇 Additional comments (4)
pkg/spdk/engine.go (4)
215-220: 🛠️ Refactor suggestion
Verify initialization logic for new engines
In the Create method, when isNewEngine(e) returns true, the code initializes e.IP, e.TargetIP, and potentially e.StandbyTargetPort. Ensure that all necessary fields are correctly initialized to prevent issues with uninitialized or stale data.
Consider initializing e.StandbyTargetPort explicitly if required.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 215-219: pkg/spdk/engine.go#L215-L219
Added lines #L215 - L219 were not covered by tests
409-411:
Review logic for determining standbyTargetCreationRequired
The condition if e.Port != 0 && e.TargetPort == 0 sets standbyTargetCreationRequired to true. Confirm that this condition accurately reflects the scenarios where a standby target creation is needed, especially when e.StandbyTargetPort may have a value.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 409-411: pkg/spdk/engine.go#L409-L411
Added lines #L409 - L411 were not covered by tests
2228-2231:
Potential issue when updating target ports during switchover
In the SwitchOverTarget function, setting e.TargetPort = targetPort and resetting e.StandbyTargetPort = 0 when targetIP == podIP could lead to issues if StandbyTargetPort should be preserved in certain cases. Please verify if this logic aligns with the intended behavior during target switchover.
123-127:
Ensure correct handling of StandbyTargetPort condition
The condition within the if statement checks if e.StandbyTargetPort != 0, and then the else block sets targetCreationRequired = true. This logic might skip target creation when it's actually needed. Please verify that the conditions correctly reflect the intended behavior.
Run the following script to check for redundant or conflicting conditions involving StandbyTargetPort:
✅ Verification successful
Let me gather more context about the surrounding code to understand the complete logic flow.
Based on the context gathered, I can now provide the final response:
The StandbyTargetPort condition is correctly implemented
The logic is correct and serves its intended purpose. Here's why:
- When
StandbyTargetPort != 0, it means a standby target instance already exists, so skipping target creation is the right behavior - When
StandbyTargetPort == 0, settingtargetCreationRequired = trueis correct as no standby target exists yet - This aligns with the engine's lifecycle management where
StandbyTargetPortis used to track standby target instances and is properly reset to 0 during target switchover (as seen in the code)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for conditions involving 'StandbyTargetPort' that might lead to logical errors.
# Find all instances where 'StandbyTargetPort' is compared
rg 'if.*StandbyTargetPort\s*(!=|==)\s*0' -A 3
# Check assignments to 'targetCreationRequired'
rg 'targetCreationRequired\s*=' -A 2
Length of output: 924
Script:
#!/bin/bash
# Get more context around the StandbyTargetPort usage
rg "StandbyTargetPort" -B 5 -A 10
# Get the complete function context where this condition exists
ast-grep --pattern 'func ($_ *Engine) $_($$$) $_ {
$$$
if $_.StandbyTargetPort != 0 {
$$$
}
$$$
}'
Length of output: 11653
derekbit
force-pushed
the
v2-control-upgrade
branch
from
4469b89
to
fe16578
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
pkg/spdk/engine.go (4)
45-45: Add field documentation forStandbyTargetPortPlease add a comment explaining the purpose of this field and its role in live upgrades.
+ // StandbyTargetPort is the port used by the standby target instance during live upgrades StandbyTargetPort int32
128-130: Improve error message clarityThe error message could be more descriptive by including the current state information.
- err = fmt.Errorf("invalid initiator and target addresses for engine %s creation with initiator address %v and target address %v", e.Name, initiatorIP, targetIP) + err = fmt.Errorf("invalid state for engine %s creation: initiator port %v, target port %v with initiator address %v and target address %v", e.Name, e.Port, e.TargetPort, initiatorIP, targetIP)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
262-263: Enhance TODO comment specificityThe TODO comment should be more specific about what aspects of the log message need improvement.
- // TODO: improve the log message + // TODO: improve the log message to include RAID level and configuration details🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 262-263: pkg/spdk/engine.go#L262-L263
Added lines #L262 - L263 were not covered by tests
456-466: Extract retry logic into helper functionConsider extracting this retry logic into a reusable helper function to improve code maintainability.
+func (e *Engine) retryLoadNVMeDeviceInfo(initiator *nvme.Initiator) error { + for r := 0; r < maxNumRetries; r++ { + err := initiator.LoadNVMeDeviceInfo(initiator.TransportAddress, initiator.TransportServiceID, initiator.SubsystemNQN) + if err == nil { + e.log.Infof("Loaded NVMe device info for engine") + return nil + } + if !strings.Contains(err.Error(), "failed to get devices") { + return errors.Wrapf(err, "failed to load NVMe device info for engine %v", e.Name) + } + time.Sleep(retryInterval) + } + return fmt.Errorf("failed to load NVMe device info after %d retries", maxNumRetries) +}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 462-464: pkg/spdk/engine.go#L462-L464
Added lines #L462 - L464 were not covered by tests
[warning] 466-466: pkg/spdk/engine.go#L466
Added line #L466 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
pkg/spdk/engine.go(14 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk/server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/spdk/engine_test.go
- pkg/spdk/server.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/spdk/engine.go
[warning] 108-109: pkg/spdk/engine.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 144-144: pkg/spdk/engine.go#L144
Added line #L144 was not covered by tests
[warning] 207-213: pkg/spdk/engine.go#L207-L213
Added lines #L207 - L213 were not covered by tests
[warning] 215-219: pkg/spdk/engine.go#L215-L219
Added lines #L215 - L219 were not covered by tests
[warning] 222-225: pkg/spdk/engine.go#L222-L225
Added lines #L222 - L225 were not covered by tests
[warning] 227-230: pkg/spdk/engine.go#L227-L230
Added lines #L227 - L230 were not covered by tests
[warning] 262-263: pkg/spdk/engine.go#L262-L263
Added lines #L262 - L263 were not covered by tests
[warning] 268-268: pkg/spdk/engine.go#L268
Added line #L268 was not covered by tests
[warning] 270-270: pkg/spdk/engine.go#L270
Added line #L270 was not covered by tests
[warning] 281-284: pkg/spdk/engine.go#L281-L284
Added lines #L281 - L284 were not covered by tests
[warning] 291-291: pkg/spdk/engine.go#L291
Added line #L291 was not covered by tests
[warning] 301-302: pkg/spdk/engine.go#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-313: pkg/spdk/engine.go#L305-L313
Added lines #L305 - L313 were not covered by tests
[warning] 399-399: pkg/spdk/engine.go#L399
Added line #L399 was not covered by tests
[warning] 409-411: pkg/spdk/engine.go#L409-L411
Added lines #L409 - L411 were not covered by tests
[warning] 420-427: pkg/spdk/engine.go#L420-L427
Added lines #L420 - L427 were not covered by tests
[warning] 429-440: pkg/spdk/engine.go#L429-L440
Added lines #L429 - L440 were not covered by tests
[warning] 442-442: pkg/spdk/engine.go#L442
Added line #L442 was not covered by tests
[warning] 446-460: pkg/spdk/engine.go#L446-L460
Added lines #L446 - L460 were not covered by tests
[warning] 462-464: pkg/spdk/engine.go#L462-L464
Added lines #L462 - L464 were not covered by tests
[warning] 466-466: pkg/spdk/engine.go#L466
Added line #L466 was not covered by tests
[warning] 469-471: pkg/spdk/engine.go#L469-L471
Added lines #L469 - L471 were not covered by tests
[warning] 474-474: pkg/spdk/engine.go#L474
Added line #L474 was not covered by tests
[warning] 477-480: pkg/spdk/engine.go#L477-L480
Added lines #L477 - L480 were not covered by tests
[warning] 482-493: pkg/spdk/engine.go#L482-L493
Added lines #L482 - L493 were not covered by tests
[warning] 499-500: pkg/spdk/engine.go#L499-L500
Added lines #L499 - L500 were not covered by tests
[warning] 508-509: pkg/spdk/engine.go#L508-L509
Added lines #L508 - L509 were not covered by tests
[warning] 513-513: pkg/spdk/engine.go#L513
Added line #L513 was not covered by tests
[warning] 515-517: pkg/spdk/engine.go#L515-L517
Added lines #L515 - L517 were not covered by tests
🔇 Additional comments (1)
pkg/spdk/engine.go (1)
2228-2231: Add test coverage for target switching logic
This critical section handling standby target ports during live upgrades lacks test coverage. Please add test cases to verify:
- Target port assignment when target IP matches pod IP
- Standby target port reset
- Error cases
#!/bin/bash
# Check for existing test coverage
rg -A 5 "TestSwitchOverTarget"
derekbit
force-pushed
the
v2-control-upgrade
branch
from
fe16578
to
be48fa6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
pkg/spdk/engine_test.go (2)
97-99: Enable parallel test execution support.Consider making the tests parallel-safe by using
c.Parallel()at the start of each test case. This would require moving the test case execution into a separate function to ensure proper isolation:- for testName, testCase := range testCases { - c.Logf("testing checkInitiatorAndTargetCreationRequirements.%v", testName) + for _, tc := range testCases { + testCase := tc // capture range variable + c.Run(testCase.name, func(c *C) { + c.Parallel()
11-11: Document test scenarios and their relationship to live upgrade requirements.To better align with the PR's objective of supporting live upgrades, consider adding documentation that:
- Explains how each test case relates to the live upgrade workflow
- Documents the assumptions and preconditions for each scenario
- Maps test cases to specific upgrade requirements
Add a comment block before the test function:
// TestCheckInitiatorAndTargetCreationRequirements validates the engine's ability // to handle live upgrades by testing various scenarios: // 1. Initial state: Only active target // 2. Transition state: Both active and standby targets // 3. Final state: New active target (previously standby) // // Each scenario verifies: // - Correct port allocation // - Proper instance creation flags // - Error handling for invalid configurationspkg/spdk/engine.go (3)
108-110: Add test coverage forisNewEngineWhile the helper function is well-structured, it currently lacks test coverage. Consider adding test cases to verify:
- New engine (all fields empty/zero)
- Existing engine with various field combinations
112-142: Simplify complex branching logicThe function has multiple nested conditions that could be simplified for better maintainability. Consider breaking down the logic into smaller, more focused functions:
+func (e *Engine) shouldCreateInitiator(podIP, initiatorIP string) bool { + return podIP == initiatorIP +} +func (e *Engine) shouldCreateTarget(podIP, targetIP string) bool { + return podIP == targetIP && e.StandbyTargetPort == 0 +} func (e *Engine) checkInitiatorAndTargetCreationRequirements(podIP, initiatorIP, targetIP string) (bool, bool, error) { - // Current complex logic + initiatorCreationRequired := e.shouldCreateInitiator(podIP, initiatorIP) + targetCreationRequired := e.shouldCreateTarget(podIP, targetIP) + + if !initiatorCreationRequired && !targetCreationRequired { + return false, false, fmt.Errorf("invalid initiator and target address for engine %s creation", e.Name) + } + + return initiatorCreationRequired, targetCreationRequired, nil }
2181-2184: Add error handling for IP retrievalConsider adding retry logic for IP retrieval as network operations can be flaky:
+func getPodIPWithRetry(maxRetries int, retryInterval time.Duration) (string, error) { + var lastErr error + for i := 0; i < maxRetries; i++ { + if ip, err := commonnet.GetIPForPod(); err == nil { + return ip, nil + } else { + lastErr = err + time.Sleep(retryInterval) + } + } + return "", fmt.Errorf("failed to get pod IP after %d retries: %v", maxRetries, lastErr) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
pkg/api/types.go(2 hunks)pkg/client/client.go(1 hunks)pkg/spdk/engine.go(14 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk/server.go(1 hunks)pkg/spdk_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/api/types.go
- pkg/client/client.go
- pkg/spdk/server.go
🔇 Additional comments (2)
pkg/spdk_test.go (1)
Line range hint 1-1354: Verify test coverage after removal of multi-threaded tests.
The removal of TestSPDKMultipleThread and TestSPDKMultipleThreadSnapshotOpsAndRebuilding functions could impact test coverage for concurrent operations. Please confirm:
- Are there equivalent tests elsewhere that cover these scenarios?
- Will new tests be added to verify concurrent operations with live upgrades?
Let's check for other test files that might cover these scenarios:
#!/bin/bash
# Description: Look for other test files that might cover concurrent operations
# Search for test files with concurrent operation tests
rg -l "sync.WaitGroup|go func\(\)" --type go --glob "*_test.go"
# Search for snapshot operation tests
rg -l "TestSnapshot|snapshot.*test" --type go --glob "*_test.go"pkg/spdk/engine.go (1)
37-49: LGTM: Engine struct field addition
The addition of StandbyTargetPort field to track standby target instance ports during live upgrades is well-placed and properly typed.
derekbit
force-pushed
the
v2-control-upgrade
branch
from
be48fa6
to
471a59d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
pkg/spdk/engine.go (3)
37-49: Add documentation for the newStandbyTargetPortfieldThe new field
StandbyTargetPortshould be documented to explain its purpose in the live upgrade process and its relationship with other port fields (PortandTargetPort).Add a comment above the field:
TargetPort int32 // Port of the target that is used for letting initiator connect to + // StandbyTargetPort is used during live upgrades to temporarily store the port + // of a standby target instance before switching over StandbyTargetPort int32
116-127: Simplify nested conditionsThe nested conditions checking
StandbyTargetPortcan be simplified to improve readability and reduce complexity.if podIP == initiatorIP && podIP == targetIP { if e.Port == 0 && e.TargetPort == 0 { e.log.Info("Creating both initiator and target instances") initiatorCreationRequired = true targetCreationRequired = true } else if e.Port != 0 && e.TargetPort == 0 { e.log.Info("Creating a target instance") - if e.StandbyTargetPort != 0 { - e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) - } else { - targetCreationRequired = true - } + targetCreationRequired = e.StandbyTargetPort == 0 + if !targetCreationRequired { + e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) + } } else {
693-698: Improve error handling and state validationThe commented-out IP retrieval code and subsequent validation could be improved:
- // podIP, err := commonnet.GetIPForPod() - // if err != nil { - // return err - // } - if e.IP != e.TargetIP { - return nil - } + if e.IP == "" || e.TargetIP == "" { + return fmt.Errorf("invalid engine state: IP or TargetIP is empty") + } + if e.IP != e.TargetIP { + e.log.Debugf("Skipping validation for non-local target (IP: %s, TargetIP: %s)", e.IP, e.TargetIP) + return nil + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
pkg/api/types.go(2 hunks)pkg/client/client.go(1 hunks)pkg/spdk/engine.go(14 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk/server.go(1 hunks)pkg/spdk_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/api/types.go
- pkg/client/client.go
- pkg/spdk/engine_test.go
- pkg/spdk/server.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/spdk/engine.go
[warning] 108-109: pkg/spdk/engine.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 144-144: pkg/spdk/engine.go#L144
Added line #L144 was not covered by tests
[warning] 207-213: pkg/spdk/engine.go#L207-L213
Added lines #L207 - L213 were not covered by tests
[warning] 215-219: pkg/spdk/engine.go#L215-L219
Added lines #L215 - L219 were not covered by tests
[warning] 222-225: pkg/spdk/engine.go#L222-L225
Added lines #L222 - L225 were not covered by tests
[warning] 227-230: pkg/spdk/engine.go#L227-L230
Added lines #L227 - L230 were not covered by tests
[warning] 262-263: pkg/spdk/engine.go#L262-L263
Added lines #L262 - L263 were not covered by tests
[warning] 268-268: pkg/spdk/engine.go#L268
Added line #L268 was not covered by tests
[warning] 270-270: pkg/spdk/engine.go#L270
Added line #L270 was not covered by tests
[warning] 281-284: pkg/spdk/engine.go#L281-L284
Added lines #L281 - L284 were not covered by tests
[warning] 291-291: pkg/spdk/engine.go#L291
Added line #L291 was not covered by tests
[warning] 301-302: pkg/spdk/engine.go#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-313: pkg/spdk/engine.go#L305-L313
Added lines #L305 - L313 were not covered by tests
[warning] 399-399: pkg/spdk/engine.go#L399
Added line #L399 was not covered by tests
[warning] 409-411: pkg/spdk/engine.go#L409-L411
Added lines #L409 - L411 were not covered by tests
[warning] 420-427: pkg/spdk/engine.go#L420-L427
Added lines #L420 - L427 were not covered by tests
[warning] 429-440: pkg/spdk/engine.go#L429-L440
Added lines #L429 - L440 were not covered by tests
[warning] 442-442: pkg/spdk/engine.go#L442
Added line #L442 was not covered by tests
[warning] 446-460: pkg/spdk/engine.go#L446-L460
Added lines #L446 - L460 were not covered by tests
[warning] 462-464: pkg/spdk/engine.go#L462-L464
Added lines #L462 - L464 were not covered by tests
[warning] 466-466: pkg/spdk/engine.go#L466
Added line #L466 was not covered by tests
[warning] 469-471: pkg/spdk/engine.go#L469-L471
Added lines #L469 - L471 were not covered by tests
[warning] 474-474: pkg/spdk/engine.go#L474
Added line #L474 was not covered by tests
[warning] 477-480: pkg/spdk/engine.go#L477-L480
Added lines #L477 - L480 were not covered by tests
[warning] 482-493: pkg/spdk/engine.go#L482-L493
Added lines #L482 - L493 were not covered by tests
[warning] 499-500: pkg/spdk/engine.go#L499-L500
Added lines #L499 - L500 were not covered by tests
[warning] 508-509: pkg/spdk/engine.go#L508-L509
Added lines #L508 - L509 were not covered by tests
[warning] 513-513: pkg/spdk/engine.go#L513
Added line #L513 was not covered by tests
[warning] 515-517: pkg/spdk/engine.go#L515-L517
Added lines #L515 - L517 were not covered by tests
🔇 Additional comments (6)
pkg/spdk_test.go (5)
271-271: LGTM! Parameter removal aligns with engine upgrade support.
The removal of the second false parameter from EngineCreate call is consistent with the PR objectives to support engine live upgrades.
354-354: LGTM! Consistent parameter update in test.
The change maintains consistency with the updated EngineCreate signature.
530-530: LGTM! Consistent parameter update in snapshot test.
The change maintains consistency with the updated EngineCreate signature in the snapshot operations test.
1228-1228: LGTM! Consistent parameter updates in revert helper.
Both EngineCreate calls in the revertSnapshot helper function have been updated consistently.
Also applies to: 1243-1243
1354-1354: LGTM! Consistent parameter update in target-only test.
The change maintains consistency with the updated EngineCreate signature in the target-only test case.
Let's verify that all EngineCreate calls have been updated consistently:
✅ Verification successful
All EngineCreate calls are consistent with the updated signature
The verification shows that all EngineCreate calls in the codebase follow the same pattern with 9 parameters (name, volumeName, frontend, size, replicaAddressMap, portCount, initiatorAddress, targetAddress, salvageRequested). The implementation in pkg/client/client.go confirms this signature, and all usage instances in pkg/spdk_test.go consistently follow this pattern. No instances of the old signature were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all EngineCreate calls have been updated to the new signature
# Expect: No instances of the old signature with two false parameters
# Search for any remaining EngineCreate calls with the old signature pattern
rg 'EngineCreate\([^)]*,[^)]*,[^)]*,[^)]*,[^)]*,[^)]*,[^)]*,[^)]*,[^)]*,[^)]*\)' --type go
Length of output: 92
Script:
#!/bin/bash
# Let's search for all EngineCreate calls to verify the signature pattern
rg "EngineCreate\(" --type go -A 1
Length of output: 3616
pkg/spdk/engine.go (1)
2228-2232: 🛠️ Refactor suggestion
Add validation for target port updates
The port update logic should validate that the ports are within valid ranges:
+ if targetPort <= 0 || targetPort > 65535 {
+ return fmt.Errorf("invalid target port %d", targetPort)
+ }
if targetIP == podIP {
e.TargetPort = targetPort
e.StandbyTargetPort = 0
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
pkg/spdk/engine_test.go (3)
97-117: Improve test execution and error handlingThe test execution can be improved in several ways:
- Use test case name instead of index for better logging
- Add cleanup after each test
- Enhance error messages in assertions
Apply these changes:
- for testName, testCase := range testCases { - c.Logf("testing checkInitiatorAndTargetCreationRequirements.%v", testName) + for _, testCase := range testCases { + c.Logf("testing %s", testCase.name) + + // Clean up after each test + defer func() { + if engine != nil { + engine.log = nil + } + }() engine := &Engine{
119-162: Add edge cases for engine state validationThe test cases should include additional scenarios:
Edge cases:
- Invalid IP formats
- Special IP addresses (localhost, 0.0.0.0)
- Maximum port values
Upgrade scenarios:
- Engine during upgrade state
- Partially configured engine
Add these test cases:
}{ + { + name: "Engine during upgrade with both ports configured", + engine: &Engine{ + IP: "", + TargetIP: "", + TargetPort: 8000, + StandbyTargetPort: 8001, + }, + expected: false, + }, + { + name: "Engine with special IP (localhost)", + engine: &Engine{ + IP: "127.0.0.1", + TargetIP: "", + StandbyTargetPort: 0, + }, + expected: false, + }, }
163-168: Improve test iteration and error reportingSimilar to the previous test, improve the test execution:
- Use test case name instead of index
- Add more descriptive error messages
Apply these changes:
- for testName, testCase := range testCases { - c.Logf("testing isNewEngine.%v", testName) + for _, testCase := range testCases { + c.Logf("testing %s", testCase.name) result := testCase.engine.isNewEngine() - c.Assert(result, Equals, testCase.expected, Commentf("Test case '%s': unexpected result", testCase.name)) + c.Assert(result, Equals, testCase.expected, + Commentf("Test case '%s': got %v, expected %v", + testCase.name, result, testCase.expected)) }pkg/spdk/engine.go (2)
2177-2180: Add context to error return for better debuggingWhen returning an error from
commonnet.GetIPForPod(), it's helpful to wrap it with additional context. This makes debugging easier by providing more information about where and why the error occurred.Apply this diff:
podIP, err := commonnet.GetIPForPod() if err != nil { - return err + return errors.Wrap(err, "failed to get pod IP") }
477-480: Clarify log and error messages by rephrasingThe term "Blindly" in the log and error messages may be unclear or informal. Consider rephrasing to enhance clarity and professionalism.
Apply this diff:
-e.log.Info("Blindly stopping expose bdev for engine") +e.log.Info("Stopping expose bdev for engine without prior checks") if err := spdkClient.StopExposeBdev(e.Nqn); err != nil { - return errors.Wrapf(err, "failed to blindly stop expose bdev for engine %v", e.Name) + return errors.Wrapf(err, "failed to stop expose bdev for engine %v without prior checks", e.Name) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/spdk/engine.go(15 hunks)pkg/spdk/engine_test.go(1 hunks)
🔇 Additional comments (1)
pkg/spdk/engine_test.go (1)
1-10: LGTM!
The package declaration and imports are appropriate for the testing requirements.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
pkg/spdk/engine_test.go (2)
99-100: Use test case name instead of index in logsReplace the numeric index with the test case name for better readability and debugging:
- for testName, testCase := range testCases { - c.Logf("testing checkInitiatorAndTargetCreationRequirements.%v", testName) + for _, testCase := range testCases { + c.Logf("testing checkInitiatorAndTargetCreationRequirements: %s", testCase.name)
165-167: Use test case name instead of index in logsReplace the numeric index with the test case name for better readability and debugging:
- for testName, testCase := range testCases { - c.Logf("testing isNewEngine.%v", testName) + for _, testCase := range testCases { + c.Logf("testing isNewEngine: %s", testCase.name)pkg/spdk/engine.go (3)
37-49: Add documentation for the StandbyTargetPort fieldThe new
StandbyTargetPortfield would benefit from documentation explaining its purpose in the live upgrade process and its relationship withPortandTargetPort.Port int32 // Port that initiator is connecting to TargetIP string TargetPort int32 // Port of the target that is used for letting initiator connect to - StandbyTargetPort int32 + StandbyTargetPort int32 // Port of the standby target used during live upgrades
112-142: Simplify complex branching logicThe
checkInitiatorAndTargetCreationRequirementsmethod has complex nested conditions that could be simplified for better readability and maintainability.Consider restructuring the logic:
func (e *Engine) checkInitiatorAndTargetCreationRequirements(podIP, initiatorIP, targetIP string) (bool, bool, error) { - initiatorCreationRequired, targetCreationRequired := false, false - var err error - - if podIP == initiatorIP && podIP == targetIP { - if e.Port == 0 && e.TargetPort == 0 { - e.log.Info("Creating both initiator and target instances") - initiatorCreationRequired = true - targetCreationRequired = true - } else if e.Port != 0 && e.TargetPort == 0 { - e.log.Info("Creating a target instance") - if e.StandbyTargetPort != 0 { - e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) - } else { - targetCreationRequired = true - } - } else { - e.log.Infof("Initiator instance with port %v and target instance with port %v are already created, will skip the creation", e.Port, e.TargetPort) - } - } else if podIP == initiatorIP { - e.log.Info("Creating an initiator instance") - initiatorCreationRequired = true - } else if podIP == targetIP { - e.log.Info("Creating a target instance") - targetCreationRequired = true - } else { - err = fmt.Errorf("invalid initiator and target addresses for engine %s creation with initiator address %v and target address %v", e.Name, initiatorIP, targetIP) - } + if podIP != initiatorIP && podIP != targetIP { + return false, false, fmt.Errorf("invalid initiator and target addresses for engine %s creation with initiator address %v and target address %v", + e.Name, initiatorIP, targetIP) + } + + initiatorCreationRequired := podIP == initiatorIP && e.Port == 0 + targetCreationRequired := podIP == targetIP && e.TargetPort == 0 && e.StandbyTargetPort == 0 + + if initiatorCreationRequired && targetCreationRequired { + e.log.Info("Creating both initiator and target instances") + } else if initiatorCreationRequired { + e.log.Info("Creating an initiator instance") + } else if targetCreationRequired { + e.log.Info("Creating a target instance") + } else if e.StandbyTargetPort != 0 { + e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) + } else { + e.log.Infof("Initiator instance with port %v and target instance with port %v are already created, will skip the creation", + e.Port, e.TargetPort) + } - return initiatorCreationRequired, targetCreationRequired, err + return initiatorCreationRequired, targetCreationRequired, nil }
2177-2180: Add error handling for GetIPForPodThe error from
GetIPForPodshould include more context about why the IP retrieval failed.podIP, err := commonnet.GetIPForPod() if err != nil { - return err + return errors.Wrapf(err, "failed to get pod IP for engine %s target switchover", e.Name) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/spdk/engine.go(16 hunks)pkg/spdk/engine_test.go(1 hunks)
🔇 Additional comments (2)
pkg/spdk/engine.go (2)
2224-2228: 🛠️ Refactor suggestion
Add port range validation
The port assignment should validate that the port number is within the valid range (1-65535).
if targetIP == podIP {
+ if targetPort <= 0 || targetPort > 65535 {
+ return fmt.Errorf("invalid target port %d: must be between 1 and 65535", targetPort)
+ }
e.TargetPort = targetPort
e.StandbyTargetPort = 0
}Likely invalid or redundant comment.
108-109: Add unit tests for isNewEngine
The isNewEngine helper function lacks test coverage. Consider adding test cases to verify:
- New engine (all fields empty/zero)
- Existing engine with various field combinations
derekbit
force-pushed
the
v2-control-upgrade
branch
from
f7988f5
to
b04800c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
pkg/spdk/engine_test.go (1)
121-170: Improve test execution pattern.The test execution can be improved by:
- Using range over test cases by value instead of index
- Adding cleanup after each test
Apply these changes:
- for testName, testCase := range testCases { - c.Logf("testing isNewEngine.%v", testName) + for _, tc := range testCases { + c.Logf("testing %s", tc.name) + + // Setup and cleanup + defer func() { + if tc.engine != nil { + tc.engine.Close() + } + }() - result := testCase.engine.isNewEngine() - c.Assert(result, Equals, testCase.expected, - Commentf("Test case '%s': unexpected result", testCase.name)) + result := tc.engine.isNewEngine() + c.Assert(result, Equals, tc.expected, + Commentf("Test case '%s': unexpected result", tc.name)) }pkg/spdk/engine.go (5)
37-49: Add documentation for the StandbyTargetPort fieldThe new
StandbyTargetPortfield should be documented to explain its purpose in supporting live upgrades and its relationship with the existingTargetPortfield.Add a comment above the field:
TargetPort int32 // Port of the target that is used for letting initiator connect to + // StandbyTargetPort is used during live upgrades to temporarily store the port + // of a new target instance before switching over StandbyTargetPort int32
108-110: Add unit tests for isNewEngine methodThis new helper method lacks test coverage. Add test cases to verify the logic for both new and existing engines.
Would you like me to help generate unit test cases for this method? The tests should cover:
- New engine (all fields empty/zero)
- Existing engine with IP set
- Existing engine with TargetIP set
- Existing engine with StandbyTargetPort set
- Existing engine with multiple fields set
112-142: Simplify complex creation requirements logicThe method contains nested conditions that make it hard to follow. Consider breaking it down into smaller, more focused functions.
Consider refactoring like this:
+func (e *Engine) shouldCreateInitiator(podIP, initiatorIP string) bool { + return podIP == initiatorIP +} + +func (e *Engine) shouldCreateTarget(podIP, targetIP string) bool { + return podIP == targetIP && e.StandbyTargetPort == 0 +} + func (e *Engine) checkInitiatorAndTargetCreationRequirements(podIP, initiatorIP, targetIP string) (bool, bool, error) { - // Current complex logic + initiatorCreationRequired := e.shouldCreateInitiator(podIP, initiatorIP) + targetCreationRequired := e.shouldCreateTarget(podIP, targetIP) + + if !initiatorCreationRequired && !targetCreationRequired { + return false, false, fmt.Errorf("invalid initiator and target addresses for engine %s creation", e.Name) + } + + return initiatorCreationRequired, targetCreationRequired, nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
429-440: Split deferred function responsibilitiesThe deferred function handles multiple responsibilities including state updates and logging. Consider extracting these into separate helper functions for better maintainability.
Consider refactoring like this:
+func (e *Engine) updateInitiatorState(initiator *nvme.Initiator, dmDeviceBusy bool) { + e.initiator = initiator + e.dmDeviceBusy = dmDeviceBusy + e.Endpoint = initiator.GetEndpoint() + e.log = e.log.WithFields(logrus.Fields{ + "endpoint": e.Endpoint, + "port": e.Port, + "targetPort": e.TargetPort, + }) +} defer func() { if err == nil { if !standbyTargetCreationRequired { - // Current complex logic + e.updateInitiatorState(initiator, dmDeviceBusy) } e.log.Infof("Finished handling frontend for engine: %+v", e) } }()🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 429-440: pkg/spdk/engine.go#L429-L440
Added lines #L429 - L440 were not covered by tests
2401-2421: Enhance error messages for port release failuresWhile the port release logic is correct, the error messages could be more descriptive to aid in debugging.
Consider enhancing error handling:
if releaseTargetPortRequired { if err := superiorPortAllocator.ReleaseRange(e.TargetPort, e.TargetPort); err != nil { - return err + return errors.Wrapf(err, "failed to release target port %d", e.TargetPort) } } if releaseStandbyTargetPortRequired { if err := superiorPortAllocator.ReleaseRange(e.StandbyTargetPort, e.StandbyTargetPort); err != nil { - return err + return errors.Wrapf(err, "failed to release standby target port %d", e.StandbyTargetPort) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/spdk/engine.go(16 hunks)pkg/spdk/engine_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/spdk/engine.go
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 144-144: pkg/spdk/engine.go#L144
Added line #L144 was not covered by tests
[warning] 207-213: pkg/spdk/engine.go#L207-L213
Added lines #L207 - L213 were not covered by tests
[warning] 215-219: pkg/spdk/engine.go#L215-L219
Added lines #L215 - L219 were not covered by tests
[warning] 222-230: pkg/spdk/engine.go#L222-L230
Added lines #L222 - L230 were not covered by tests
[warning] 262-263: pkg/spdk/engine.go#L262-L263
Added lines #L262 - L263 were not covered by tests
[warning] 268-268: pkg/spdk/engine.go#L268
Added line #L268 was not covered by tests
[warning] 270-270: pkg/spdk/engine.go#L270
Added line #L270 was not covered by tests
[warning] 281-284: pkg/spdk/engine.go#L281-L284
Added lines #L281 - L284 were not covered by tests
[warning] 291-291: pkg/spdk/engine.go#L291
Added line #L291 was not covered by tests
[warning] 301-302: pkg/spdk/engine.go#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-313: pkg/spdk/engine.go#L305-L313
Added lines #L305 - L313 were not covered by tests
[warning] 399-399: pkg/spdk/engine.go#L399
Added line #L399 was not covered by tests
[warning] 409-411: pkg/spdk/engine.go#L409-L411
Added lines #L409 - L411 were not covered by tests
[warning] 420-420: pkg/spdk/engine.go#L420
Added line #L420 was not covered by tests
[warning] 422-427: pkg/spdk/engine.go#L422-L427
Added lines #L422 - L427 were not covered by tests
[warning] 429-440: pkg/spdk/engine.go#L429-L440
Added lines #L429 - L440 were not covered by tests
[warning] 442-442: pkg/spdk/engine.go#L442
Added line #L442 was not covered by tests
[warning] 446-460: pkg/spdk/engine.go#L446-L460
Added lines #L446 - L460 were not covered by tests
[warning] 462-464: pkg/spdk/engine.go#L462-L464
Added lines #L462 - L464 were not covered by tests
[warning] 466-466: pkg/spdk/engine.go#L466
Added line #L466 was not covered by tests
[warning] 469-471: pkg/spdk/engine.go#L469-L471
Added lines #L469 - L471 were not covered by tests
[warning] 474-474: pkg/spdk/engine.go#L474
Added line #L474 was not covered by tests
[warning] 477-480: pkg/spdk/engine.go#L477-L480
Added lines #L477 - L480 were not covered by tests
[warning] 482-493: pkg/spdk/engine.go#L482-L493
Added lines #L482 - L493 were not covered by tests
[warning] 499-500: pkg/spdk/engine.go#L499-L500
Added lines #L499 - L500 were not covered by tests
[warning] 508-509: pkg/spdk/engine.go#L508-L509
Added lines #L508 - L509 were not covered by tests
[warning] 513-513: pkg/spdk/engine.go#L513
Added line #L513 was not covered by tests
[warning] 515-517: pkg/spdk/engine.go#L515-L517
Added lines #L515 - L517 were not covered by tests
[warning] 626-626: pkg/spdk/engine.go#L626
Added line #L626 was not covered by tests
🔇 Additional comments (2)
pkg/spdk/engine.go (2)
Line range hint 2364-2393: LGTM! Well-structured target deletion logic
The DeleteTarget method is well-implemented with:
- Proper error handling
- Clear logging
- Cleanup of resources in correct order
2224-2228: 🛠️ Refactor suggestion
Add validation for target port updates
The port update logic should validate that the ports are within valid ranges.
Add port validation:
+ if targetPort <= 0 || targetPort > 65535 {
+ return fmt.Errorf("invalid target port %d", targetPort)
+ }
if targetIP == podIP {
e.TargetPort = targetPort
e.StandbyTargetPort = 0
}Likely invalid or redundant comment.
derekbit
force-pushed
the
v2-control-upgrade
branch
from
b04800c
to
aaada8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
pkg/spdk/engine.go (6)
45-45: Add documentation for the StandbyTargetPort fieldConsider adding a comment explaining that this field is used for managing standby target ports during live upgrades, similar to how other port fields are documented.
+ // StandbyTargetPort is used for managing standby target ports during live upgrades StandbyTargetPort int32
108-110: Add unit tests for isNewEngine functionThis helper function lacks test coverage. Consider adding test cases to verify the logic for both new and existing engines.
Would you like me to help create unit test cases for this function?
112-142: Simplify complex conditional logicThe nested conditions in this function make it harder to follow the logic flow. Consider restructuring to reduce nesting and improve readability:
func (e *Engine) checkInitiatorAndTargetCreationRequirements(podIP, initiatorIP, targetIP string) (bool, bool, error) { initiatorCreationRequired := podIP == initiatorIP targetCreationRequired := podIP == targetIP - if podIP == initiatorIP && podIP == targetIP { - if e.Port == 0 && e.TargetPort == 0 { - e.log.Info("Creating both initiator and target instances") - initiatorCreationRequired = true - targetCreationRequired = true - } else if e.Port != 0 && e.TargetPort == 0 { - e.log.Info("Creating a target instance") - if e.StandbyTargetPort != 0 { - e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) - } else { - targetCreationRequired = true - } - } else { - e.log.Infof("Initiator instance with port %v and target instance with port %v are already created, will skip the creation", e.Port, e.TargetPort) - } + if !initiatorCreationRequired && !targetCreationRequired { + return false, false, fmt.Errorf("invalid initiator and target addresses for engine %s creation with initiator address %v and target address %v", + e.Name, initiatorIP, targetIP) + } + + if podIP == initiatorIP && podIP == targetIP { + switch { + case e.Port == 0 && e.TargetPort == 0: + e.log.Info("Creating both initiator and target instances") + case e.Port != 0 && e.TargetPort == 0: + e.log.Info("Creating a target instance") + if e.StandbyTargetPort != 0 { + e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) + targetCreationRequired = false + } + default: + e.log.Infof("Initiator instance with port %v and target instance with port %v are already created, will skip the creation", + e.Port, e.TargetPort) + initiatorCreationRequired = false + targetCreationRequired = false + } } else if podIP == initiatorIP { e.log.Info("Creating an initiator instance") - initiatorCreationRequired = true } else if podIP == targetIP { e.log.Info("Creating a target instance") - targetCreationRequired = true - } else { - err = fmt.Errorf("invalid initiator and target addresses for engine %s creation with initiator address %v and target address %v", - e.Name, initiatorIP, targetIP) } - return initiatorCreationRequired, targetCreationRequired, err + return initiatorCreationRequired, targetCreationRequired, nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
Line range hint
2153-2180: Improve error handling in target switchoverThe error handling includes good rollback capabilities, but consider adding more context to error messages and logging. Also, the IP retrieval could be moved to a separate function for better testability.
- podIP, err := commonnet.GetIPForPod() - if err != nil { - return err - } + podIP, err := e.getPodIP() + if err != nil { + return errors.Wrap(err, "failed to get pod IP during target switchover") + } +func (e *Engine) getPodIP() (string, error) { + podIP, err := commonnet.GetIPForPod() + if err != nil { + return "", errors.Wrap(err, "failed to get pod IP") + } + return podIP, nil +}
2364-2389: Enhance error handling in DeleteTargetThe error handling could be improved by:
- Adding more context to error messages
- Improving logging of cleanup steps
- Considering partial cleanup on failures
func (e *Engine) DeleteTarget(spdkClient *spdkclient.Client, superiorPortAllocator *commonbitmap.Bitmap) (err error) { + log := e.log.WithField("function", "DeleteTarget") + log.Info("Starting target deletion") + + defer func() { + if err != nil { + log.WithError(err).Error("Failed to delete target") + } else { + log.Info("Successfully deleted target") + } + }() e.log.Infof("Deleting target with target port %d and standby target port %d", e.TargetPort, e.StandbyTargetPort) err = spdkClient.StopExposeBdev(e.Nqn) if err != nil { - return errors.Wrapf(err, "failed to stop expose bdev while deleting target instance for engine %s", e.Name) + return errors.Wrapf(err, "failed to stop expose bdev (NQN: %s) while deleting target instance for engine %s", + e.Nqn, e.Name) }
2401-2421: Add validation in releaseTargetAndStandbyTargetPortsConsider adding validation to ensure port values are valid before attempting release.
func (e *Engine) releaseTargetAndStandbyTargetPorts(superiorPortAllocator *commonbitmap.Bitmap) error { + if superiorPortAllocator == nil { + return fmt.Errorf("port allocator is nil") + } + releaseTargetPortRequired := e.TargetPort != 0 releaseStandbyTargetPortRequired := e.StandbyTargetPort != 0 && e.StandbyTargetPort != e.TargetPort + if e.TargetPort < 0 || e.TargetPort > 65535 { + return fmt.Errorf("invalid target port %d", e.TargetPort) + } + if e.StandbyTargetPort < 0 || e.StandbyTargetPort > 65535 { + return fmt.Errorf("invalid standby target port %d", e.StandbyTargetPort) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
pkg/api/types.go(2 hunks)pkg/client/client.go(1 hunks)pkg/spdk/engine.go(18 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk/server.go(1 hunks)pkg/spdk_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/api/types.go
- pkg/client/client.go
- pkg/spdk/engine_test.go
- pkg/spdk/server.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/spdk/engine.go
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 144-144: pkg/spdk/engine.go#L144
Added line #L144 was not covered by tests
[warning] 207-213: pkg/spdk/engine.go#L207-L213
Added lines #L207 - L213 were not covered by tests
[warning] 215-219: pkg/spdk/engine.go#L215-L219
Added lines #L215 - L219 were not covered by tests
[warning] 222-230: pkg/spdk/engine.go#L222-L230
Added lines #L222 - L230 were not covered by tests
[warning] 262-263: pkg/spdk/engine.go#L262-L263
Added lines #L262 - L263 were not covered by tests
[warning] 268-268: pkg/spdk/engine.go#L268
Added line #L268 was not covered by tests
[warning] 270-270: pkg/spdk/engine.go#L270
Added line #L270 was not covered by tests
[warning] 281-284: pkg/spdk/engine.go#L281-L284
Added lines #L281 - L284 were not covered by tests
[warning] 291-291: pkg/spdk/engine.go#L291
Added line #L291 was not covered by tests
[warning] 301-302: pkg/spdk/engine.go#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-313: pkg/spdk/engine.go#L305-L313
Added lines #L305 - L313 were not covered by tests
[warning] 399-399: pkg/spdk/engine.go#L399
Added line #L399 was not covered by tests
[warning] 409-411: pkg/spdk/engine.go#L409-L411
Added lines #L409 - L411 were not covered by tests
[warning] 420-420: pkg/spdk/engine.go#L420
Added line #L420 was not covered by tests
[warning] 422-427: pkg/spdk/engine.go#L422-L427
Added lines #L422 - L427 were not covered by tests
[warning] 429-440: pkg/spdk/engine.go#L429-L440
Added lines #L429 - L440 were not covered by tests
[warning] 442-442: pkg/spdk/engine.go#L442
Added line #L442 was not covered by tests
[warning] 446-460: pkg/spdk/engine.go#L446-L460
Added lines #L446 - L460 were not covered by tests
[warning] 462-464: pkg/spdk/engine.go#L462-L464
Added lines #L462 - L464 were not covered by tests
[warning] 466-466: pkg/spdk/engine.go#L466
Added line #L466 was not covered by tests
[warning] 469-471: pkg/spdk/engine.go#L469-L471
Added lines #L469 - L471 were not covered by tests
[warning] 474-474: pkg/spdk/engine.go#L474
Added line #L474 was not covered by tests
[warning] 477-480: pkg/spdk/engine.go#L477-L480
Added lines #L477 - L480 were not covered by tests
[warning] 482-493: pkg/spdk/engine.go#L482-L493
Added lines #L482 - L493 were not covered by tests
[warning] 499-500: pkg/spdk/engine.go#L499-L500
Added lines #L499 - L500 were not covered by tests
[warning] 508-509: pkg/spdk/engine.go#L508-L509
Added lines #L508 - L509 were not covered by tests
[warning] 513-513: pkg/spdk/engine.go#L513
Added line #L513 was not covered by tests
[warning] 515-517: pkg/spdk/engine.go#L515-L517
Added lines #L515 - L517 were not covered by tests
[warning] 626-626: pkg/spdk/engine.go#L626
Added line #L626 was not covered by tests
🔇 Additional comments (4)
pkg/spdk_test.go (3)
271-271: LGTM! Parameter removal aligns with engine upgrade support.
The removal of the upgradeRequired parameter from EngineCreate calls is consistent with the PR objectives to support engine live upgrades.
Also applies to: 354-354, 530-530
1228-1228: LGTM! Consistent parameter updates in revertSnapshot function.
The revertSnapshot helper function has been updated to match the new EngineCreate signature, maintaining consistency with the changes in the test methods.
Also applies to: 1243-1243
1354-1354: LGTM! Updated EngineCreate call in TestSPDKEngineOnlyWithTarget.
The test case for engine with target-only configuration has been updated to use the new signature, completing the changes across all test scenarios.
pkg/spdk/engine.go (1)
2224-2228: 🛠️ Refactor suggestion
Add validation for target port updates
The port update logic should validate that the ports are within valid ranges.
+ if targetPort <= 0 || targetPort > 65535 {
+ return fmt.Errorf("invalid target port %d", targetPort)
+ }
if targetIP == podIP {
e.TargetPort = targetPort
e.StandbyTargetPort = 0
}Likely invalid or redundant comment.
derekbit
force-pushed
the
v2-control-upgrade
branch
from
aaada8b
to
7357f50
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
pkg/spdk/engine.go (4)
37-49: Add documentation for the newStandbyTargetPortfield.The new field needs documentation to explain its purpose in the live upgrade process and its relationship with
TargetPort.Add a comment above the field:
+// StandbyTargetPort is the port used by the standby target instance during live upgrades. +// When non-zero, it indicates a standby target is available. StandbyTargetPort int32
112-142: Simplify complex branching logic.The function has nested if conditions that make it hard to follow. Consider restructuring for better readability.
func (e *Engine) checkInitiatorAndTargetCreationRequirements(podIP, initiatorIP, targetIP string) (bool, bool, error) { - initiatorCreationRequired, targetCreationRequired := false, false - var err error + if podIP != initiatorIP && podIP != targetIP { + return false, false, fmt.Errorf("invalid initiator and target addresses for engine %s creation with initiator address %v and target address %v", + e.Name, initiatorIP, targetIP) + } - if podIP == initiatorIP && podIP == targetIP { - if e.Port == 0 && e.TargetPort == 0 { - e.log.Info("Creating both initiator and target instances") - initiatorCreationRequired = true - targetCreationRequired = true - } else if e.Port != 0 && e.TargetPort == 0 { - e.log.Info("Creating a target instance") - if e.StandbyTargetPort != 0 { - e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) - } else { - targetCreationRequired = true - } - } else { - e.log.Infof("Initiator instance with port %v and target instance with port %v are already created, will skip the creation", e.Port, e.TargetPort) - } - } else if podIP == initiatorIP { - e.log.Info("Creating an initiator instance") - initiatorCreationRequired = true - } else if podIP == targetIP { - e.log.Info("Creating a target instance") - targetCreationRequired = true - } else { - err = fmt.Errorf("invalid initiator and target addresses for engine %s creation with initiator address %v and target address %v", e.Name, initiatorIP, targetIP) - } + initiatorCreationRequired := podIP == initiatorIP && e.Port == 0 + targetCreationRequired := podIP == targetIP && e.TargetPort == 0 && e.StandbyTargetPort == 0 - return initiatorCreationRequired, targetCreationRequired, err + if initiatorCreationRequired { + e.log.Info("Creating an initiator instance") + } + if targetCreationRequired { + e.log.Info("Creating a target instance") + } + if !initiatorCreationRequired && !targetCreationRequired { + e.log.Infof("Skipping creation - Port: %v, TargetPort: %v, StandbyTargetPort: %v", + e.Port, e.TargetPort, e.StandbyTargetPort) + } + + return initiatorCreationRequired, targetCreationRequired, nil }
Line range hint
2141-2243: Add validation for target port updates.The function should validate the port values before updating them.
Add validation before updating ports:
+ if newTargetPort <= 0 || newTargetPort > 65535 { + return fmt.Errorf("invalid target port %d", newTargetPort) + } if newTargetIP == podIP { e.TargetPort = newTargetPort e.StandbyTargetPort = 0 } else { e.StandbyTargetPort = e.TargetPort e.TargetPort = 0 }
2161-2176: Enhance error logging for better debugging.The error handling is good but could benefit from more detailed logging.
- e.log.WithError(err).Warnf("Failed to switch over engine to target address %s", newTargetAddress) + e.log.WithError(err).WithFields(logrus.Fields{ + "currentTargetAddress": currentTargetAddress, + "newTargetAddress": newTargetAddress, + "targetPort": e.TargetPort, + "standbyTargetPort": e.StandbyTargetPort, + }).Warnf("Failed to switch over engine to target address")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
pkg/api/types.go(2 hunks)pkg/client/client.go(1 hunks)pkg/spdk/engine.go(16 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk/server.go(1 hunks)pkg/spdk_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/api/types.go
- pkg/client/client.go
- pkg/spdk/engine_test.go
- pkg/spdk/server.go
🔇 Additional comments (8)
pkg/spdk_test.go (5)
271-271: LGTM! Parameter removal aligns with engine upgrade support.
The removal of the second false parameter from EngineCreate call is consistent with the PR objectives to support engine live upgrades.
354-354: Same parameter removal as in previous EngineCreate call.
1228-1228: Same parameter removal as in previous EngineCreate calls.
1243-1243: Same parameter removal as in previous EngineCreate calls.
1354-1354: Same parameter removal as in previous EngineCreate calls.
pkg/spdk/engine.go (3)
108-110: LGTM!
The function correctly checks if the engine is new by verifying all relevant fields are empty/zero.
2371-2403: LGTM!
The function properly handles the cleanup sequence:
- Stops exposing the bdev
- Releases ports
- Deletes the RAID bdev
- Disconnects replicas
2408-2428: LGTM!
The function correctly handles:
- Port release validation
- Error handling for each release operation
- Proper cleanup of both target and standby ports
derekbit
force-pushed
the
v2-control-upgrade
branch
from
7357f50
to
ee27e1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
pkg/spdk_test.go (1)
Based on the search results, I can now provide a final response regarding the upgrade test coverage concern.
Upgrade test coverage needs to be added
The codebase currently lacks dedicated test coverage for upgrade scenarios:
- The only upgrade-related code found is a log message in
pkg/spdk/engine.goabout replica reconstruction during upgrades- The
VersionDetailGetRPC endpoint inserver.gois unimplemented (marked with TODO)- No explicit upgrade or version compatibility tests exist in the test files
- The test suite focuses on basic SPDK operations, snapshots, and rebuilding but not upgrade paths
Recommendations:
- Add test cases for version compatibility checks
- Implement and test the
VersionDetailGetRPC endpoint- Add upgrade scenario tests covering replica reconstruction during upgrades
- Consider adding version compatibility matrix tests
🔗 Analysis chain
Line range hint
271-1354: Verify test coverage for upgrade scenarios.While the parameter removal is consistent, we should ensure that the test coverage for upgrade scenarios is maintained elsewhere.
Consider adding dedicated test cases for the new live upgrade functionality to maintain comprehensive test coverage.
Let me search for upgrade-related code and tests in the SPDK implementation.
Let me search more specifically in the test files and SPDK implementation to understand the upgrade scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for upgrade-related test coverage # Search for upgrade-related test functions rg -t go "Test.*Upgrade" # Search for upgrade-related test scenarios in existing tests rg -t go "upgrade.*test|test.*upgrade" -iLength of output: 70
Script:
#!/bin/bash # Let's try a broader search for upgrade-related tests and functionality # Search for upgrade-related terms in test files echo "=== Searching for upgrade-related tests ===" rg -g "*_test.go" "upgrade|version" -i -A 5 -B 5 # Search for version-related constants and types echo "=== Searching for version-related code ===" rg "Version|SPDK_VERSION" -t go -A 2 -B 2 # Search specifically in the SPDK test file echo "=== Searching in SPDK test file ===" rg "func.*Test" pkg/spdk_test.goLength of output: 372883
Script:
#!/bin/bash # Search for upgrade-related code in SPDK implementation # Search for upgrade-related functions and variables echo "=== Searching for upgrade-related code ===" rg -g "*.go" "upgrade|version" pkg/spdk/ # Search for version-related constants and types echo "=== Searching for version-related constants ===" rg -g "*.go" "const.*version|type.*version" pkg/spdk/ # Search for version checks or compatibility code echo "=== Searching for version checks ===" rg -g "*.go" "check.*version|compatible|compatibility" pkg/spdk/Length of output: 582
Script:
#!/bin/bash # Search for test functions and their descriptions echo "=== Searching test functions in SPDK test file ===" rg -B 5 -A 10 "func.*Test" pkg/spdk_test.go # Search for upgrade/version related test scenarios echo "=== Searching for upgrade scenarios in tests ===" rg -g "*_test.go" "upgrade|version|migration|compatibility" pkg/ # Search for version-related code in SPDK server implementation echo "=== Searching version-related code in server ===" rg -B 5 -A 5 "Version" pkg/spdk/server.goLength of output: 3479
pkg/spdk/engine.go (3)
Line range hint
2141-2238: Add unit tests forSwitchOverTargetmethod and its error handling.The
SwitchOverTargetmethod contains critical logic for switching the engine's target address, including multiple error handling and recovery steps. Currently, there is a lack of unit test coverage for this method. Consider adding comprehensive tests to cover:
- Successful target switchovers
- Error scenarios when disconnecting from the old target
- Error scenarios when connecting to the new target
- Recovery procedures when errors occur during switchover
- Validation of state changes and external effects
Enhancing test coverage will improve reliability and maintainability.
2408-2428: EnsurereleaseTargetAndStandbyTargetPortsis thoroughly tested.The
releaseTargetAndStandbyTargetPortsfunction handles the release of network ports with conditional logic based on the state ofe.TargetPortande.StandbyTargetPort. Currently, this function lacks unit test coverage. Adding tests for all possible scenarios, including:
- Both ports need to be released
- Only one of the ports needs to be released
- Neither port needs to be released
- Error handling when releasing ports fails
will help verify that ports are correctly managed and resources are freed appropriately.
420-427: Add error handling tests for initiator creation inhandleFrontend.In the
handleFrontendfunction, the creation of the NVMe initiator is a critical step:initiator, err := nvme.NewInitiator(e.VolumeName, e.Nqn, nvme.HostProc) if err != nil { return errors.Wrapf(err, "failed to create initiator for engine %v", e.Name) }Currently, this path lacks test coverage. Consider adding unit tests to simulate failures in
nvme.NewInitiatorto ensure that errors are properly handled and that the engine reacts appropriately to initiator creation failures.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 420-427: pkg/spdk/engine.go#L420-L427
Added lines #L420 - L427 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
pkg/api/types.go(2 hunks)pkg/client/client.go(1 hunks)pkg/spdk/engine.go(16 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk/server.go(1 hunks)pkg/spdk_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/api/types.go
- pkg/client/client.go
- pkg/spdk/engine_test.go
- pkg/spdk/server.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/spdk/engine.go
[warning] 128-130: pkg/spdk/engine.go#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 144-144: pkg/spdk/engine.go#L144
Added line #L144 was not covered by tests
[warning] 207-213: pkg/spdk/engine.go#L207-L213
Added lines #L207 - L213 were not covered by tests
[warning] 215-219: pkg/spdk/engine.go#L215-L219
Added lines #L215 - L219 were not covered by tests
[warning] 222-230: pkg/spdk/engine.go#L222-L230
Added lines #L222 - L230 were not covered by tests
[warning] 262-263: pkg/spdk/engine.go#L262-L263
Added lines #L262 - L263 were not covered by tests
[warning] 268-268: pkg/spdk/engine.go#L268
Added line #L268 was not covered by tests
[warning] 270-270: pkg/spdk/engine.go#L270
Added line #L270 was not covered by tests
[warning] 281-284: pkg/spdk/engine.go#L281-L284
Added lines #L281 - L284 were not covered by tests
[warning] 291-291: pkg/spdk/engine.go#L291
Added line #L291 was not covered by tests
[warning] 301-302: pkg/spdk/engine.go#L301-L302
Added lines #L301 - L302 were not covered by tests
[warning] 305-313: pkg/spdk/engine.go#L305-L313
Added lines #L305 - L313 were not covered by tests
[warning] 399-399: pkg/spdk/engine.go#L399
Added line #L399 was not covered by tests
[warning] 409-411: pkg/spdk/engine.go#L409-L411
Added lines #L409 - L411 were not covered by tests
[warning] 420-427: pkg/spdk/engine.go#L420-L427
Added lines #L420 - L427 were not covered by tests
[warning] 429-440: pkg/spdk/engine.go#L429-L440
Added lines #L429 - L440 were not covered by tests
[warning] 442-442: pkg/spdk/engine.go#L442
Added line #L442 was not covered by tests
[warning] 446-460: pkg/spdk/engine.go#L446-L460
Added lines #L446 - L460 were not covered by tests
[warning] 462-464: pkg/spdk/engine.go#L462-L464
Added lines #L462 - L464 were not covered by tests
[warning] 466-466: pkg/spdk/engine.go#L466
Added line #L466 was not covered by tests
[warning] 469-471: pkg/spdk/engine.go#L469-L471
Added lines #L469 - L471 were not covered by tests
[warning] 474-474: pkg/spdk/engine.go#L474
Added line #L474 was not covered by tests
[warning] 477-480: pkg/spdk/engine.go#L477-L480
Added lines #L477 - L480 were not covered by tests
[warning] 482-493: pkg/spdk/engine.go#L482-L493
Added lines #L482 - L493 were not covered by tests
[warning] 499-500: pkg/spdk/engine.go#L499-L500
Added lines #L499 - L500 were not covered by tests
[warning] 508-509: pkg/spdk/engine.go#L508-L509
Added lines #L508 - L509 were not covered by tests
[warning] 513-513: pkg/spdk/engine.go#L513
Added line #L513 was not covered by tests
[warning] 515-517: pkg/spdk/engine.go#L515-L517
Added lines #L515 - L517 were not covered by tests
[warning] 626-626: pkg/spdk/engine.go#L626
Added line #L626 was not covered by tests
[warning] 693-694: pkg/spdk/engine.go#L693-L694
Added lines #L693 - L694 were not covered by tests
🔇 Additional comments (5)
pkg/spdk_test.go (5)
271-271: LGTM! Parameter removal aligns with engine upgrade support.
The removal of the upgradeRequired parameter from EngineCreate call is consistent with the PR objectives to support engine live upgrades.
354-354: Consistent parameter removal in engine re-creation.
The change maintains consistency with the previous EngineCreate call in the same test function.
530-530: LGTM! Consistent parameter update in snapshot test.
The change maintains consistency with other EngineCreate calls while testing snapshot operations.
1228-1228: LGTM! Consistent parameter updates in revert helper.
Both EngineCreate calls in the revertSnapshot helper function have been updated consistently to remove the upgradeRequired parameter.
Also applies to: 1243-1243
1354-1354: LGTM! Consistent parameter update in target-only test.
The change maintains consistency with other EngineCreate calls while testing target-only scenarios.
|
This pull request is now in conflict. Could you fix it @derekbit? 🙏 |
derekbit
force-pushed
the
v2-control-upgrade
branch
from
ee27e1b
to
413647c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
pkg/spdk_test.go (2)
Line range hint
1-1354: Consider enhancing test documentationWhile the test implementation is thorough, consider adding:
- A test overview comment at the file level describing the test coverage
- Brief comments before each test function explaining the test scenario and objectives
- Documentation for helper functions like
checkReplicaSnapshotsandrevertSnapshotThis would improve maintainability and help other developers understand the test scenarios better.
Line range hint
282-290: Address cleanup race conditionsThere are TODO comments indicating race conditions in cleanup, with cleanup code currently commented out. This could lead to:
- Resource leaks if cleanup is not performed
- Flaky tests due to race conditions
- Difficulty in test maintenance
Consider:
- Investigating and fixing the race conditions
- Implementing proper cleanup with appropriate synchronization
- Adding error handling for cleanup failures
Would you like help investigating the race conditions or implementing a more robust cleanup mechanism?
Also applies to: 587-595
pkg/spdk/engine.go (5)
108-110: Add documentation for isNewEngine methodWhile the method logic is clear, adding documentation would improve maintainability. Consider adding a comment explaining the conditions that define a new engine.
+// isNewEngine returns true if the engine instance is newly created, +// determined by checking if IP, TargetIP, and StandbyTargetPort are unset. func (e *Engine) isNewEngine() bool { return e.IP == "" && e.TargetIP == "" && e.StandbyTargetPort == 0 }
Line range hint
2141-2238: Enhance error handling and state management in SwitchOverTargetThe target switchover implementation could be improved in several areas:
- The error handling in the deferred function is complex and could benefit from being extracted into a separate method
- The state transitions during the switchover process should be more explicit
- Consider adding logging for state transitions
Consider refactoring the error handling into a separate method:
+func (e *Engine) handleSwitchOverError(err error, currentTargetAddress string) { + e.log.WithError(err).Warnf("Failed to switch over engine to target address %s", newTargetAddress) + + if disconnected, errCheck := e.isTargetDisconnected(); errCheck != nil { + e.log.WithError(errCheck).Warnf("Failed to check if target %s is disconnected", newTargetAddress) + } else if disconnected { + e.handleTargetReconnection(currentTargetAddress) + } +} + +func (e *Engine) handleTargetReconnection(currentTargetAddress string) { + if currentTargetAddress == "" { + return + } + + if errConnect := e.connectTarget(currentTargetAddress); errConnect != nil { + e.log.WithError(errConnect).Warnf("Failed to connect target back to %s", currentTargetAddress) + return + } + + e.log.Infof("Connected target back to %s", currentTargetAddress) + + if errReload := e.reloadDevice(); errReload != nil { + e.log.WithError(errReload).Warnf("Failed to reload device mapper") + } else { + e.log.Infof("Reloaded device mapper for connecting target back to %s", currentTargetAddress) + } +}
2408-2428: Add port validation in releaseTargetAndStandbyTargetPortsThe port release implementation should include validation of port values before release:
func (e *Engine) releaseTargetAndStandbyTargetPorts(superiorPortAllocator *commonbitmap.Bitmap) error { + // Validate port ranges + if e.TargetPort < 0 || e.TargetPort > 65535 { + return fmt.Errorf("invalid target port %d", e.TargetPort) + } + if e.StandbyTargetPort < 0 || e.StandbyTargetPort > 65535 { + return fmt.Errorf("invalid standby target port %d", e.StandbyTargetPort) + } + releaseTargetPortRequired := e.TargetPort != 0 releaseStandbyTargetPortRequired := e.StandbyTargetPort != 0 && e.StandbyTargetPort != e.TargetPort
Line range hint
398-521: Enhance connection retry mechanism in handleFrontendThe connection handling could be improved with:
- Exponential backoff for retries
- More detailed error categorization
- Circuit breaker pattern for repeated failures
Consider implementing a more sophisticated retry mechanism:
+func (e *Engine) withRetry(operation func() error) error { + backoff := time.Second + maxBackoff := time.Second * 30 + + for r := 0; r < maxNumRetries; r++ { + err := operation() + if err == nil { + return nil + } + + if isUnrecoverable(err) { + return err + } + + time.Sleep(backoff) + backoff = time.Duration(math.Min(float64(backoff*2), float64(maxBackoff))) + } + return fmt.Errorf("operation failed after %d retries", maxNumRetries) +}
Line range hint
2244-2266: Enhance target state validationThe target state validation could be improved with additional checks:
- Validate the initiator state more comprehensively
- Add timeouts for state checks
- Consider adding health checks for the target
func (e *Engine) isTargetDisconnected() (bool, error) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + // Validate initiator state + if err := e.validateInitiatorState(); err != nil { + return false, err + } + initiator, err := nvme.NewInitiator(e.VolumeName, e.Nqn, nvme.HostProc) if err != nil { return false, errors.Wrapf(err, "failed to create initiator for checking engine %s target disconnected", e.Name) } + + // Add health check + if err := e.checkTargetHealth(ctx); err != nil { + return false, err + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (6)
pkg/api/types.go(2 hunks)pkg/client/client.go(1 hunks)pkg/spdk/engine.go(16 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk/server.go(1 hunks)pkg/spdk_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/api/types.go
- pkg/client/client.go
- pkg/spdk/engine_test.go
- pkg/spdk/server.go
🔇 Additional comments (2)
pkg/spdk_test.go (1)
271-271: LGTM! Consistent removal of upgradeRequired parameter
The changes consistently remove the last boolean parameter from all EngineCreate calls, aligning with the PR objectives to support engine live upgrades.
Let's verify that all callers of EngineCreate have been updated:
Also applies to: 354-354, 530-530, 1228-1228, 1243-1243, 1354-1354
pkg/spdk/engine.go (1)
37-49: LGTM: Engine struct changes
The addition of StandbyTargetPort field is well-structured and properly positioned alongside other port-related fields. The field naming is consistent with the existing naming convention.
Longhorn 9104 Signed-off-by: Derek Su <[email protected]>
Longhorn 9104 Signed-off-by: Derek Su <[email protected]>
derekbit
force-pushed
the
v2-control-upgrade
branch
from
413647c
to
0f18b59
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
pkg/spdk/engine.go (7)
108-110: Add unit tests forisNewEngineWhile the implementation is correct, this helper method lacks test coverage. Consider adding test cases for:
- New engine (all fields empty/zero)
- Existing engine with various field combinations
112-142: Simplify complex creation requirements logicConsider refactoring this method to improve readability:
- Extract the conditions into separate helper methods
- Use early returns to reduce nesting
func (e *Engine) checkInitiatorAndTargetCreationRequirements(podIP, initiatorIP, targetIP string) (bool, bool, error) { - initiatorCreationRequired, targetCreationRequired := false, false - var err error + if podIP != initiatorIP && podIP != targetIP { + return false, false, fmt.Errorf("invalid initiator and target addresses for engine %s creation with initiator address %v and target address %v", + e.Name, initiatorIP, targetIP) + } - if podIP == initiatorIP && podIP == targetIP { - if e.Port == 0 && e.TargetPort == 0 { - // ... rest of the conditions + initiatorCreationRequired := podIP == initiatorIP + targetCreationRequired := e.shouldCreateTarget(podIP, targetIP) + return initiatorCreationRequired, targetCreationRequired, nil } + +func (e *Engine) shouldCreateTarget(podIP, targetIP string) bool { + if podIP != targetIP { + return false + } + if e.StandbyTargetPort != 0 { + e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) + return false + } + return e.TargetPort == 0 }
Line range hint
398-475: Enhance method documentationConsider adding detailed documentation for:
- Parameter descriptions and their relationships
- Error scenarios and their handling
- The role of standby target in the frontend handling process
Line range hint
2141-2238: Enhance rollback mechanism for target switchoverConsider implementing a more robust rollback mechanism:
- Track the state of each operation
- Implement compensating actions for each step
- Add logging for rollback operations
+type switchoverState struct { + oldTargetDisconnected bool + newTargetConnected bool + deviceReloaded bool +} +func (e *Engine) rollbackSwitchover(state *switchoverState, oldTargetAddress string) error { + if state.newTargetConnected { + if err := e.disconnectTarget(newTargetAddress); err != nil { + return fmt.Errorf("rollback: failed to disconnect new target: %v", err) + } + } + if state.oldTargetDisconnected { + if err := e.connectTarget(oldTargetAddress); err != nil { + return fmt.Errorf("rollback: failed to reconnect old target: %v", err) + } + } + return nil }
2371-2403: Improve error handling in DeleteTargetConsider enhancing error handling:
- Continue cleanup even if some steps fail
- Aggregate errors from all cleanup steps
- Add metrics/monitoring for cleanup failures
+type cleanupError struct { + step string + err error +} func (e *Engine) DeleteTarget(spdkClient *spdkclient.Client, superiorPortAllocator *commonbitmap.Bitmap) (err error) { + var errors []cleanupError + if err := spdkClient.StopExposeBdev(e.Nqn); err != nil { - return errors.Wrapf(err, "failed to stop expose bdev") + errors = append(errors, cleanupError{"stop_expose", err}) } // Continue with other cleanup steps... + + if len(errors) > 0 { + return fmt.Errorf("cleanup errors: %v", errors) + } return nil }
2408-2428: Add port validation checksConsider adding validation for port values:
- Check for valid port ranges (1-65535)
- Ensure ports are not in use before release
- Add logging for port release operations
func (e *Engine) releaseTargetAndStandbyTargetPorts(superiorPortAllocator *commonbitmap.Bitmap) error { + if err := validatePort(e.TargetPort); err != nil { + return err + } + if err := validatePort(e.StandbyTargetPort); err != nil { + return err + } // Rest of the implementation... } +func validatePort(port int32) error { + if port < 0 || port > 65535 { + return fmt.Errorf("invalid port number: %d", port) + } + return nil }
2404-2406: Add method documentationConsider adding documentation to explain:
- The purpose of the method
- Parameter descriptions
- Return value meaning
+// isSwitchOverTargetRequired determines if a target switchover is needed by comparing addresses +// Parameters: +// oldTargetAddress: current target address in format "host:port" +// newTargetAddress: proposed target address in format "host:port" +// Returns: +// true if addresses differ, indicating switchover is required func isSwitchOverTargetRequired(oldTargetAddress, newTargetAddress string) bool { return oldTargetAddress != newTargetAddress }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (6)
pkg/api/types.go(2 hunks)pkg/client/client.go(1 hunks)pkg/spdk/engine.go(16 hunks)pkg/spdk/engine_test.go(1 hunks)pkg/spdk/server.go(1 hunks)pkg/spdk_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/api/types.go
- pkg/client/client.go
- pkg/spdk/engine_test.go
- pkg/spdk/server.go
🔇 Additional comments (4)
pkg/spdk_test.go (3)
271-271: LGTM! Consistent removal of upgradeRequired parameter
The changes correctly remove the upgradeRequired parameter from all EngineCreate calls in the test functions, aligning with the API changes.
Also applies to: 354-354, 530-530
1228-1228: LGTM! Consistent parameter update in revertSnapshot function
The revertSnapshot helper function has been updated to match the new EngineCreate signature by removing the upgradeRequired parameter.
Also applies to: 1243-1243
1354-1354: LGTM! Updated TestSPDKEngineOnlyWithTarget
The test case for engine with target-only configuration has been updated to use the new EngineCreate signature.
pkg/spdk/engine.go (1)
37-49: LGTM: Engine struct changes for standby target support
The addition of StandbyTargetPort field is well-structured and properly typed for managing standby target ports during live upgrades.
|
This pull request is now in conflict. Could you fix it @derekbit? 🙏 |
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9104
Signed-off-by: Derek Su [email protected]
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context