feat(v2 upgrade): support engine live upgrade #716
Conversation
Longhorn 9104 Signed-off-by: Derek Su <[email protected]>
Longhorn 9104 Signed-off-by: Derek Su <[email protected]>
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
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: 0
🧹 Outside diff range and nitpick comments (4)
pkg/api/instance.go (2)
80-88: Consider adding port range validation.While the mapping is correct, consider adding validation to ensure
StandbyTargetPortStartis less thanStandbyTargetPortEndto prevent potential runtime issues.Example validation:
func RPCToInstanceStatus(obj *rpc.InstanceStatus) InstanceStatus { + if obj.StandbyTargetPortStart > obj.StandbyTargetPortEnd { + // Either swap the values or log a warning + obj.StandbyTargetPortStart, obj.StandbyTargetPortEnd = obj.StandbyTargetPortEnd, obj.StandbyTargetPortStart + } return InstanceStatus{ State: obj.State, ErrorMsg: obj.ErrorMsg,
67-88: Consider documenting the live upgrade port management strategy.The addition of standby target ports is a crucial part of the live upgrade infrastructure. Consider adding documentation that explains:
- The lifecycle of standby ports during upgrades
- Port allocation strategy and potential range conflicts
- Coordination between active and standby ports
Add a comment block above the
InstanceStatusstruct explaining the role of standby ports in live upgrades:+// InstanceStatus represents the current state of an instance including its port configurations. +// During live upgrades, the instance can maintain both active ports (TargetPort*) and +// standby ports (StandbyTargetPort*) to facilitate seamless transitions. type InstanceStatus struct {pkg/instance/instance.go (2)
723-731: Consistent port assignments for standby target ports.Setting both
StandbyTargetPortStartandStandbyTargetPortEndtoe.StandbyTargetPortis acceptable ifStandbyTargetPortrepresents a single port value. If a range of ports is intended, consider adjusting these fields to reflect the correct port range.
723-731: Initialize the 'Conditions' map appropriately.The
Conditionsfield is initialized with an empty map usingmake(map[string]bool). Ensure that this map is populated with relevant condition statuses where necessary, or consider removing it if it remains unused to improve clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (9)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/longhorn-spdk-engine/pkg/api/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/client/client.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/server.gois excluded by!vendor/**vendor/github.com/longhorn/types/pkg/generated/imrpc/instance.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (2)
pkg/api/instance.go(1 hunks)pkg/instance/instance.go(3 hunks)
🔇 Additional comments (3)
pkg/api/instance.go (1)
67-75: LGTM! The new standby target port fields are well-structured.
The addition of StandbyTargetPortStart and StandbyTargetPortEnd fields follows consistent naming and typing patterns with existing port fields, supporting the live upgrade functionality.
Let's verify the port range usage across the codebase:
✅ Verification successful
Let me search for port validation logic to ensure proper range checks:
Let me search for port assignment and validation in the instance creation flow:
Based on the search results, I can now provide a final response:
Port range validation is properly handled in the codebase
The standby target port fields are well integrated with existing port management:
- Port range validation exists in the SPDK replica code ensuring
PortStart < PortEnd - Port allocation is handled through a dedicated allocator that manages ranges
- Port cleanup and release is properly implemented when instances are deleted
- The new standby target ports follow the same pattern as existing port fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper port range validation and usage
# Expected: Find validation logic ensuring StandbyTargetPortStart < StandbyTargetPortEnd
# Search for port range validation logic
rg -t go "StandbyTargetPort(Start|End)" -A 5 -B 5
Length of output: 5868
Script:
#!/bin/bash
# Search for validation logic in instance-related files
rg -t go "func.*Instance.*Create" -A 20 -B 5
# Also check for any port validation functions
rg -t go "func.*(ValidatePort|CheckPort|PortRange)" -A 10
Length of output: 26667
Script:
#!/bin/bash
# Search for port-related logic in instance creation
rg -t go "Port.*Start.*End" -B 5 -A 5
# Look for port validation in the instance package
rg -t go "func.*Instance.*Port" -A 10
Length of output: 11909
pkg/instance/instance.go (2)
12-13: Addition of 'emptypb' import is appropriate.
The import of emptypb is required for methods returning *emptypb.Empty, such as InstanceSuspend and InstanceResume. This inclusion is necessary for handling empty protobuf messages.
155-155: Parameters passed to EngineCreate are correctly updated.
The inclusion of additional parameters in the EngineCreate function call aligns with the enhanced functionality for engine live upgrades. Ensure that req.Spec.PortCount, req.Spec.InitiatorAddress, req.Spec.TargetAddress, and req.Spec.SpdkInstanceSpec.SalvageRequested are correctly handled within the EngineCreate method.
derekbit
force-pushed
the
v2-control-upgrade
branch
from
fc70fdf
to
413cf47
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
pkg/instance/instance.go (8)
723-731: Consistency in Port Assignments inengineResponseToInstanceResponseIn the
engineResponseToInstanceResponsefunction, bothPortStartandPortEndare assigned the same valuee.Port. Similarly,TargetPortStartandTargetPortEndare assignede.TargetPort, andStandbyTargetPortStartandStandbyTargetPortEndare assignede.StandbyTargetPort. If these fields are intended to represent a single port, this is acceptable. However, if port ranges are expected in the future, consider updating the implementation to support port ranges.
731-731: Initialization ofConditionsFieldThe
Conditionsfield is initialized as an empty map withmake(map[string]bool). If this field is expected to hold condition statuses, consider populating it with meaningful data or remove it if it's not needed.
Line range hint
846-851: Correct Error Code for Unsupported Operation inInstanceSuspendIn the
InstanceSuspendmethod forV2DataEngineInstanceOps, when suspending a replica instance (which is not supported), the error codegrpccodes.InvalidArgumentis used. Since the operation is not implemented for replicas, it's more appropriate to usegrpccodes.Unimplementedto indicate that the method is not supported for this instance type.Apply this diff to correct the error code:
- return nil, grpcstatus.Errorf(grpccodes.InvalidArgument, "suspend is not supported for instance type %v", req.Type) + return nil, grpcstatus.Errorf(grpccodes.Unimplemented, "suspend is not supported for instance type %v", req.Type)
Line range hint
871-876: Correct Error Code for Unsupported Operation inInstanceResumeSimilarly, in the
InstanceResumemethod forV2DataEngineInstanceOps, returngrpccodes.Unimplementedwhen the resume operation is not supported for replicas, instead ofgrpccodes.InvalidArgument.Apply this diff:
- return nil, grpcstatus.Errorf(grpccodes.InvalidArgument, "resume is not supported for instance type %v", req.Type) + return nil, grpcstatus.Errorf(grpccodes.Unimplemented, "resume is not supported for instance type %v", req.Type)
Line range hint
896-901: Correct Error Code for Unsupported Operation inInstanceSwitchOverTargetIn the
InstanceSwitchOverTargetmethod, usegrpccodes.Unimplementedto indicate that target switch over is not supported for replicas.Apply this diff:
- return nil, grpcstatus.Errorf(grpccodes.InvalidArgument, "target switch over is not supported for instance type %v", req.Type) + return nil, grpcstatus.Errorf(grpccodes.Unimplemented, "target switch over is not supported for instance type %v", req.Type)
Line range hint
921-926: Correct Error Code for Unsupported Operation inInstanceDeleteTargetIn the
InstanceDeleteTargetmethod, adjust the error code togrpccodes.Unimplementedwhen the operation is not supported for replicas.Apply this diff:
- return nil, grpcstatus.Errorf(grpccodes.InvalidArgument, "target deletion is not supported for instance type %v", req.Type) + return nil, grpcstatus.Errorf(grpccodes.Unimplemented, "target deletion is not supported for instance type %v", req.Type)
Line range hint
636-656: Consider ImplementingInstanceReplacefor V2 Data EngineThe
InstanceReplacemethod forV2DataEngineInstanceOpsreturns an unimplemented error. If replacing instances is a required functionality for the V2 data engine, consider implementing this method to provide consistency across data engine versions.
Line range hint
676-684: Consider ImplementingInstanceLogfor V2 Data EngineThe
InstanceLogmethod forV2DataEngineInstanceOpsis currently unimplemented. Providing logging functionality is crucial for debugging and monitoring. Consider implementing this method to allow users to retrieve logs from instances managed by the V2 data engine.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (9)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/longhorn-spdk-engine/pkg/api/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/client/client.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/server.gois excluded by!vendor/**vendor/github.com/longhorn/types/pkg/generated/imrpc/instance.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/github.com/longhorn/types/pkg/generated/spdkrpc/spdk.pb.gois excluded by!**/*.pb.go,!**/generated/**,!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (2)
pkg/api/instance.go(1 hunks)pkg/instance/instance.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/api/instance.go
🔇 Additional comments (2)
pkg/instance/instance.go (2)
729-730: Verify Proper Handling of Standby Target Ports
Ensure that the new fields StandbyTargetPortStart and StandbyTargetPortEnd correctly represent the standby target ports. Verify that these fields are properly utilized throughout the system and that any consumers of this API are updated to handle these new fields appropriately.
Run the following script to find usages of the new standby target port fields:
✅ Verification successful
Let me gather more context about the usage and implementation of these fields.
Based on the search results, I can now provide a conclusive response about the standby target port fields.
Standby Target Port Fields Are Correctly Implemented
The implementation is correct and consistent with the codebase's behavior:
- The original
StandbyTargetPortfield from the engine is properly mapped to bothStandbyTargetPortStartandStandbyTargetPortEnd - This mapping maintains backward compatibility while adapting to the new port range structure
- The implementation is consistent across the API layer (
pkg/api/instance.go) and instance management (pkg/instance/instance.go) - The engine's implementation shows that standby target ports are properly managed, allocated, and released as single ports, which aligns with setting both start and end to the same value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to 'StandbyTargetPortStart' and 'StandbyTargetPortEnd'.
# Test: Find all occurrences of the new standby target port fields in the codebase.
rg 'StandbyTargetPort(Start|End)'
Length of output: 1541
Script:
#!/bin/bash
# Search for StandbyTargetPort to understand the original field and its usage
rg "StandbyTargetPort[^S][^t]" -A 2
# Also check the protobuf definition
fd "instance.proto" --exec cat {}
Length of output: 6492
155-155: Removal of UpgradeRequired Parameter in EngineCreate
In the InstanceCreate method for V2DataEngineInstanceOps, the UpgradeRequired parameter has been removed from the EngineCreate call. Ensure that this change is intentional and that the EngineCreate function no longer requires this parameter. Verify that this does not affect the upgrade functionality or introduce any unintended behavior.
Run the following script to check if the UpgradeRequired parameter is used elsewhere:
derekbit
requested review from
innobead,
PhanLe1010,
c3y1huang,
shuo-wu and
DamiaSan
derekbit
force-pushed
the
v2-control-upgrade
branch
from
413cf47
to
fc70fdf
Compare
|
This pull request is now in conflict. Could you fix it @derekbit? 🙏 |
Signed-off-by: Derek Su [email protected]#### Which issue(s) this PR fixes:
Issue longhorn/longhorn#9104
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context