feat(v2 upgrade): support engine live upgrade #241
Conversation
WalkthroughThe changes in this pull request involve the addition of a new field, Changes
Assessment against linked issues
Warning Rate limit exceeded@derekbit has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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.
pkg/spdk/engine.go
Outdated
| 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) |
There was a problem hiding this comment.
Redundant nested condition on e.StandbyTargetPort
The nested if statements both check e.StandbyTargetPort != 0, which is redundant and may indicate a logical error.
Apply this diff to correct 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
- }
}Ensure that the inner condition is either removed or modified to check a different variable if intended.
📝 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 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) | |
| if e.StandbyTargetPort != 0 { | |
| e.log.Warnf("Standby target instance with port %v is already created, will skip the target creation", e.StandbyTargetPort) |
Longhorn 9104 Signed-off-by: Derek Su <derek.su@suse.com>
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)
Longhorn 9104 Signed-off-by: Derek Su <derek.su@suse.com>
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
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9104
Signed-off-by: Derek Su derek.su@suse.com
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context