feat(v2 upgrade): support engine live upgrade #716
Conversation
WalkthroughThe changes introduce two new fields, Changes
Assessment against linked issues
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package qcow: could not load export data: no export data for "github.com/longhorn/longhorn-engine/pkg/qcow"" 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (19)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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? 🙏 |
derekbit
force-pushed
the
v2-control-upgrade
branch
from
fc70fdf
to
293e308
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
pkg/api/instance.go (3)
66-72: Add field documentation for NvmeDevicePath structConsider adding documentation comments for each field to clarify their purpose and expected values, especially for fields like
Trtype(valid values: rdma, tcp) andState(possible states).type NvmeDevicePath struct { + // Trtype specifies the NVMe-oF transport type (e.g., "rdma", "tcp") Trtype string `json:"trtype"` + // Traddr specifies the transport address of the NVMe target Traddr string `json:"traddr"` + // Trsvcid specifies the transport service ID or port number Trsvcid string `json:"trsvcid"` + // SrcAddr specifies the source/initiator address for the connection SrcAddr string `json:"src_addr"` + // State represents the current state of the NVMe device path State string `json:"state"` }
92-92: Fix typo in variable nameThe variable name 'nvmeSubstem' should be 'nvmeSubsystem'.
- nvmeSubstem := NvmeSubsystem{ + nvmeSubsystem := NvmeSubsystem{
66-115: Architecture supports live upgrade requirementsThe implementation provides a solid foundation for live upgrades through:
- NVMe path management for handling device connections
- Separate port ranges for active and standby instances
- State tracking for individual paths
Consider documenting the upgrade workflow and potential failure scenarios in the package documentation.
pkg/instance/instance.go (4)
726-726: Adjust log level for debugging informationThe log statement
logrus.Infof("Debug =====> pathName: %v, path: %v", pathName, path)is more suitable at the debug level rather than info level. This prevents verbose output in production environments.Apply this diff to adjust the log level:
-logrus.Infof("Debug =====> pathName: %v, path: %v", pathName, path) +logrus.Debugf("pathName: %v, path: %v", pathName, path)
Line range hint
1029-1032: Consistent error handling for unsupported operationsIn
InstanceReplaceforV2DataEngineInstanceOps, the error returned isgrpcstatus.Error(grpccodes.Unimplemented, "v2 data engine instance replace is not supported"). Ensure that all methods returning unimplemented errors use consistent messages and status codes for clarity.Consider updating the error messages for consistency:
- return grpcstatus.Error(grpccodes.Unimplemented, "v2 data engine instance replace is not supported") + return grpcstatus.Errorf(grpccodes.Unimplemented, "%s is not supported for v2 data engine", "InstanceReplace")Apply similar changes to other methods that return unimplemented errors.
Line range hint
1128-1129: Improve error messages for unimplemented methodsIn
InstanceLogforV2DataEngineInstanceOps, the error message is"v2 data engine instance log is not supported". For better clarity and consistency, consider specifying the method name in the error message.Update the error message as follows:
- return grpcstatus.Error(grpccodes.Unimplemented, "v2 data engine instance log is not supported") + return grpcstatus.Errorf(grpccodes.Unimplemented, "%s is not supported for v2 data engine", "InstanceLog")
Line range hint
1620-1636: Consider implementingInstanceReplacefor V2 data engineCurrently,
InstanceReplaceforV2DataEngineInstanceOpsreturns an unimplemented error. IfInstanceReplacefunctionality is required for V2 data engines, consider implementing this method to support seamless instance replacement.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (7)
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/spdk/engine.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(4 hunks)
🔇 Additional comments (5)
pkg/api/instance.go (1)
86-88: Verify port range validation
The addition of standby target ports suggests support for live migration. Consider adding validation to ensure:
- Standby port ranges don't overlap with active target ports
- Port ranges are valid (start ≤ end)
pkg/instance/instance.go (4)
738-745: Verify the assignment of Port and TargetPort fields
The PortStart and PortEnd fields are both assigned e.Port, and similarly, TargetPortStart and TargetPortEnd are assigned e.TargetPort. Please verify that the engine uses single ports and that this assignment is intentional. If the engine can operate over a range of ports, consider adjusting the assignments accordingly.
744-745: Ensure correct assignment of StandbyTargetPort fields
The StandbyTargetPortStart and StandbyTargetPortEnd are both assigned e.StandbyTargetPort. Please confirm that this is intentional and that e.StandbyTargetPort represents a single port. If a range is intended, ensure that both start and end ports are correctly assigned.
Line range hint 1356-1357: Handle context cancellation appropriately in watchers
In the watchProcess, watchSPDKEngine, and watchSPDKReplica functions, ensure that the context cancellation is properly handled to prevent resource leaks. Specifically, verify that any goroutines or streams are terminated when the context is done.
155-155: Update function call to match new parameter requirements
The call to EngineCreate includes req.Spec.SpdkInstanceSpec.SalvageRequested as a new parameter. Confirm that this change aligns with the updated function signature of EngineCreate and that all necessary parameters are correctly passed.
Run the following script to verify the function signature:
✅ Verification successful
Based on the search results, I can now generate the final response:
The function call parameters match the implementation
The EngineCreate function call in pkg/instance/instance.go correctly matches the implementation in vendor/github.com/longhorn/longhorn-spdk-engine/pkg/client/client.go. The function signature is:
func (c *SPDKClient) EngineCreate(name, volumeName, frontend string, specSize uint64, replicaAddressMap map[string]string, portCount int32, initiatorAddress, targetAddress string, salvageRequested bool) (*api.Engine, error)And the call is made with the corresponding parameters in the correct order:
engine, err := c.EngineCreate(
req.Spec.Name, // name
req.Spec.VolumeName, // volumeName
req.Spec.SpdkInstanceSpec.Frontend, // frontend
req.Spec.SpdkInstanceSpec.Size, // specSize
req.Spec.SpdkInstanceSpec.ReplicaAddressMap,// replicaAddressMap
req.Spec.PortCount, // portCount
req.Spec.InitiatorAddress, // initiatorAddress
req.Spec.TargetAddress, // targetAddress
req.Spec.SpdkInstanceSpec.SalvageRequested // salvageRequested
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the definition of EngineCreate to ensure it matches the parameters used.
# Search for the EngineCreate function definition
rg -A 10 'func\s+EngineCreate' | grep -A 10 'func EngineCreate'
Length of output: 65
Script:
#!/bin/bash
# Let's try a broader search for EngineCreate without restricting to function definition
rg -A 5 "EngineCreate"
# Also search for any interface or type that might define EngineCreate
ast-grep --pattern 'interface { $$$ EngineCreate $$$ }'
# Additionally search for any struct method implementations
ast-grep --pattern 'func ($_ $_) EngineCreate'
Length of output: 24836
pkg/instance/instance.go
Outdated
| for pathName, path := range e.NvmeSubsystem.Paths { | ||
| nvmeSubsystem.Paths[pathName] = &rpc.NvmeDevicePath{ | ||
| Trtype: path.Trtype, | ||
| Traddr: path.Traddr, | ||
| Trsvcid: path.Trsvcid, | ||
| SrcAddr: path.SrcAddr, | ||
| State: path.State, | ||
| } | ||
|
|
||
| logrus.Infof("Debug =====> pathName: %v, path: %v", pathName, path) | ||
| } |
There was a problem hiding this comment.
Add nil check to prevent potential nil pointer dereference
In the engineResponseToInstanceResponse function, there is a potential for a nil pointer dereference when accessing e.NvmeSubsystem.Paths. If e.NvmeSubsystem is nil, this will cause a runtime panic. It's advisable to check if e.NvmeSubsystem is not nil before iterating over e.NvmeSubsystem.Paths.
Apply this diff to add the nil check:
nvmeSubsystem := &rpc.NvmeSubsystem{
Paths: make(map[string]*rpc.NvmeDevicePath),
}
+if e.NvmeSubsystem != nil {
for pathName, path := range e.NvmeSubsystem.Paths {
nvmeSubsystem.Paths[pathName] = &rpc.NvmeDevicePath{
Trtype: path.Trtype,
Traddr: path.Traddr,
Trsvcid: path.Trsvcid,
SrcAddr: path.SrcAddr,
State: path.State,
}
- logrus.Infof("Debug =====> pathName: %v, path: %v", pathName, path)
+ logrus.Debugf("pathName: %v, path: %v", pathName, path)
}
+}📝 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.
| for pathName, path := range e.NvmeSubsystem.Paths { | |
| nvmeSubsystem.Paths[pathName] = &rpc.NvmeDevicePath{ | |
| Trtype: path.Trtype, | |
| Traddr: path.Traddr, | |
| Trsvcid: path.Trsvcid, | |
| SrcAddr: path.SrcAddr, | |
| State: path.State, | |
| } | |
| logrus.Infof("Debug =====> pathName: %v, path: %v", pathName, path) | |
| } | |
| nvmeSubsystem := &rpc.NvmeSubsystem{ | |
| Paths: make(map[string]*rpc.NvmeDevicePath), | |
| } | |
| if e.NvmeSubsystem != nil { | |
| for pathName, path := range e.NvmeSubsystem.Paths { | |
| nvmeSubsystem.Paths[pathName] = &rpc.NvmeDevicePath{ | |
| Trtype: path.Trtype, | |
| Traddr: path.Traddr, | |
| Trsvcid: path.Trsvcid, | |
| SrcAddr: path.SrcAddr, | |
| State: path.State, | |
| } | |
| logrus.Debugf("pathName: %v, path: %v", pathName, path) | |
| } | |
| } |
derekbit
force-pushed
the
v2-control-upgrade
branch
from
9db0036
to
a8e9e60
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
pkg/api/instance.go (1)
66-76: Consider adding field validation and documentationWhile the NVMe-related structs are well-structured, consider:
- Adding field validation for required fields (e.g., Trtype, Traddr)
- Including documentation comments to describe valid values and constraints
- Adding constants for known transport types and states
+// Known transport types for NVMe devices +const ( + TrtypeTCP = "tcp" + TrtypeRDMA = "rdma" +) + +// Known states for NVMe device paths +const ( + PathStateActive = "active" + PathStateInactive = "inactive" +) + +// NvmeDevicePath represents the addressing and state information for an NVMe device type NvmeDevicePath struct { + // Trtype specifies the transport type (e.g., tcp, rdma) Trtype string `json:"trtype"` + // Traddr specifies the transport address Traddr string `json:"traddr"` + // Trsvcid specifies the transport service ID (typically the port) Trsvcid string `json:"trsvcid"` + // SrcAddr specifies the source address for the connection SrcAddr string `json:"src_addr"` + // State indicates the current state of the path State string `json:"state"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (8)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/go-spdk-helper/pkg/nvme/initiator.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/api/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.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(4 hunks)
🔇 Additional comments (4)
pkg/api/instance.go (1)
79-115: Verify the usage of new fields for live upgrades
Let's verify how these new fields are used in the upgrade process.
✅ Verification successful
Implementation of standby ports and NVMe subsystem is consistent
The code changes are properly implemented across the codebase:
- The standby target ports are consistently used in both the API layer (
pkg/api/instance.go) and instance implementation (pkg/instance/instance.go) - NVMe subsystem implementation is complete with proper mapping between RPC and API types
- The fields are properly propagated from the engine response to instance response
The implementation appears to be part of the infrastructure needed to support live upgrades, with standby ports allowing for seamless transition during the upgrade process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of standby target ports and NVMe subsystem
echo "Searching for standby target port usage..."
rg "StandbyTargetPort(Start|End)" -A 5
echo "Searching for NVMe subsystem usage in upgrade logic..."
rg "NvmeSubsystem.*upgrade" -A 5
Length of output: 3794
Script:
#!/bin/bash
# Let's search for more context about the usage of these fields
# Search for files containing upgrade-related code
echo "Searching for upgrade-related files..."
fd -e go | xargs rg -l "upgrade"
# Search for instance status usage in upgrade context
echo "Searching for instance status in upgrade context..."
rg "InstanceStatus.*upgrade" -B 5 -A 5
# Search for standby port usage in non-generated files
echo "Searching for standby port usage in implementation..."
rg "StandbyTargetPort" --type go --glob '!vendor/**' --glob '!**/generated/**' -B 5 -A 5
# Search for NVMe subsystem implementation
echo "Searching for NVMe subsystem implementation..."
rg "NvmeSubsystem" --type go --glob '!vendor/**' --glob '!**/generated/**' -B 5 -A 5
Length of output: 10523
pkg/instance/instance.go (3)
714-724: LGTM: Clean implementation of NVMe subsystem handling
The implementation correctly maps the NVMe subsystem paths while maintaining all necessary fields.
738-747: LGTM: Proper implementation of standby target ports for live upgrade support
The addition of StandbyTargetPortStart and StandbyTargetPortEnd fields, populated from the Engine's StandbyTargetPort, provides the necessary support for live upgrades. The implementation maintains consistency with the existing port handling pattern.
✅ Verification successful
LGTM: Standby target port implementation is properly integrated and validated
The verification confirms that the standby target port implementation is well-integrated across the codebase with proper:
- Port allocation and release handling in
releaseTargetAndStandbyTargetPort - Validation checks to prevent duplicate port assignments (
StandbyTargetPort != e.TargetPort) - State management during target switchover
- Consistent usage in RPC/API interfaces
The implementation maintains proper port lifecycle management and includes necessary validation checks, confirming it's production-ready for live upgrade support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the usage of standby target ports in the codebase
rg -A 5 "StandbyTargetPort"
# Check for any validation of port ranges
ast-grep --pattern 'if $_.StandbyTargetPort$_ {
$$$
}'
Length of output: 13693
155-155: Verify the removal of UpgradeRequired parameter
The removal of UpgradeRequired from EngineCreate call suggests a change in upgrade handling strategy. Please confirm if this aligns with the new live upgrade implementation.
✅ Verification successful
Based on the search results, I can now generate the final response:
The removal of UpgradeRequired parameter is intentional
The UpgradeRequired field still exists in the protobuf definitions and request specs (pkg/client/instance.go and protobuf files), but its usage has been changed. The field is now only used for logging purposes before engine creation ("upgradeRequired": req.Spec.UpgradeRequired), indicating that the upgrade handling has been refactored to use a different mechanism, likely through the new NVMe subsystem handling and standby target port fields that were added as part of the live upgrade support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to UpgradeRequired in engine creation
rg -A 5 "UpgradeRequired.*EngineCreate"
# Check for any migration code handling the parameter removal
rg "UpgradeRequired.*deprecated"
Length of output: 76
Script:
#!/bin/bash
# Let's search for any references to UpgradeRequired in the codebase
rg "UpgradeRequired" -A 3
# Also check for the EngineCreate function definition to understand its parameters
ast-grep --pattern 'func $_ EngineCreate($_) {
$$$
}'
Length of output: 3318
Script:
#!/bin/bash
# Let's look at the git history of the specific line to understand the change
git log -L155,155:pkg/instance/instance.go
# Also check for any new upgrade-related functionality that might have replaced this
rg "upgrade" -g "*.go" --no-filename -A 3 | grep -v "vendor"
Length of output: 20064
pkg/api/instance.go
Outdated
| State: obj.State, | ||
| ErrorMsg: obj.ErrorMsg, | ||
| Conditions: obj.Conditions, | ||
| PortStart: obj.PortStart, | ||
| PortEnd: obj.PortEnd, | ||
| TargetPortStart: obj.TargetPortStart, | ||
| TargetPortEnd: obj.TargetPortEnd, | ||
| StandbyTargetPortStart: obj.StandbyTargetPortStart, | ||
| StandbyTargetPortEnd: obj.StandbyTargetPortEnd, | ||
| NvmeSubsystem: nvmeSubstem, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate instance status before return
Add validation of the constructed InstanceStatus before returning it to ensure data consistency.
- return InstanceStatus{
+ status := InstanceStatus{
State: obj.State,
ErrorMsg: obj.ErrorMsg,
Conditions: obj.Conditions,
PortStart: obj.PortStart,
PortEnd: obj.PortEnd,
TargetPortStart: obj.TargetPortStart,
TargetPortEnd: obj.TargetPortEnd,
StandbyTargetPortStart: obj.StandbyTargetPortStart,
StandbyTargetPortEnd: obj.StandbyTargetPortEnd,
NvmeSubsystem: nvmeSubstem,
}
+ if err := status.Validate(); err != nil {
+ return InstanceStatus{
+ State: "error",
+ ErrorMsg: fmt.Sprintf("invalid instance status: %v", err),
+ }
+ }
+ return statusCommittable suggestion skipped: line range outside the PR's diff.
pkg/api/instance.go
Outdated
| nvmeSubstem := NvmeSubsystem{ | ||
| Paths: map[string]NvmeDevicePath{}, | ||
| } | ||
| for pathName, path := range obj.NvmeSubsystem.Paths { | ||
| nvmeSubstem.Paths[pathName] = NvmeDevicePath{ | ||
| Trtype: path.Trtype, | ||
| Traddr: path.Traddr, | ||
| Trsvcid: path.Trsvcid, | ||
| SrcAddr: path.SrcAddr, | ||
| State: path.State, | ||
| } | ||
| } |
There was a problem hiding this comment.
Add nil checks and validation for NVMe subsystem conversion
The current implementation could panic if obj.NvmeSubsystem is nil. The previous review comment about this issue is still valid.
Apply this fix:
func RPCToInstanceStatus(obj *rpc.InstanceStatus) InstanceStatus {
nvmeSubstem := NvmeSubsystem{
Paths: map[string]NvmeDevicePath{},
}
+ if obj == nil {
+ return InstanceStatus{
+ State: "error",
+ ErrorMsg: "invalid instance status: nil object",
+ }
+ }
+ if obj.NvmeSubsystem == nil {
+ return InstanceStatus{
+ State: obj.State,
+ ErrorMsg: obj.ErrorMsg,
+ }
+ }
for pathName, path := range obj.NvmeSubsystem.Paths {
+ if path == nil {
+ continue
+ }
nvmeSubstem.Paths[pathName] = NvmeDevicePath{
Trtype: path.Trtype,
Traddr: path.Traddr,
Trsvcid: path.Trsvcid,
SrcAddr: path.SrcAddr,
State: path.State,
}
}📝 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.
| nvmeSubstem := NvmeSubsystem{ | |
| Paths: map[string]NvmeDevicePath{}, | |
| } | |
| for pathName, path := range obj.NvmeSubsystem.Paths { | |
| nvmeSubstem.Paths[pathName] = NvmeDevicePath{ | |
| Trtype: path.Trtype, | |
| Traddr: path.Traddr, | |
| Trsvcid: path.Trsvcid, | |
| SrcAddr: path.SrcAddr, | |
| State: path.State, | |
| } | |
| } | |
| nvmeSubstem := NvmeSubsystem{ | |
| Paths: map[string]NvmeDevicePath{}, | |
| } | |
| if obj == nil { | |
| return InstanceStatus{ | |
| State: "error", | |
| ErrorMsg: "invalid instance status: nil object", | |
| } | |
| } | |
| if obj.NvmeSubsystem == nil { | |
| return InstanceStatus{ | |
| State: obj.State, | |
| ErrorMsg: obj.ErrorMsg, | |
| } | |
| } | |
| for pathName, path := range obj.NvmeSubsystem.Paths { | |
| if path == nil { | |
| continue | |
| } | |
| nvmeSubstem.Paths[pathName] = NvmeDevicePath{ | |
| Trtype: path.Trtype, | |
| Traddr: path.Traddr, | |
| Trsvcid: path.Trsvcid, | |
| SrcAddr: path.SrcAddr, | |
| State: path.State, | |
| } | |
| } |
pkg/instance/instance.go
Outdated
| State: path.State, | ||
| } | ||
|
|
||
| logrus.Infof("Debug =====> pathName: %v, path: %v", pathName, path) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use appropriate logging level for debug messages
The debug message is using logrus.Infof which is inappropriate for debug information.
-logrus.Infof("Debug =====> pathName: %v, path: %v", pathName, path)
+logrus.Debugf("pathName: %v, path: %v", pathName, path)📝 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.
| logrus.Infof("Debug =====> pathName: %v, path: %v", pathName, path) | |
| logrus.Debugf("pathName: %v, path: %v", pathName, path) |
derekbit
force-pushed
the
v2-control-upgrade
branch
from
a8e9e60
to
338bbab
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
pkg/api/instance.go (3)
74-76: Add documentation and helper methods for NvmeSubsystemThe struct needs documentation and helper methods for path management.
Add documentation and methods:
+// NvmeSubsystem represents a collection of NVMe device paths type NvmeSubsystem struct { Paths map[string]NvmeDevicePath `json:"paths"` } +// AddPath adds a new device path to the subsystem +func (s *NvmeSubsystem) AddPath(name string, path NvmeDevicePath) error { + if err := path.Validate(); err != nil { + return fmt.Errorf("invalid path: %v", err) + } + s.Paths[name] = path + return nil +}
79-88: Add documentation for new fieldsThe new fields need documentation explaining their purpose and relationship:
- How standby ports relate to active ports
- The role of NvmeSubsystem in the instance status
Add field documentation:
type InstanceStatus struct { State string `json:"state"` ErrorMsg string `json:"errorMsg"` Conditions map[string]bool `json:"conditions"` PortStart int32 `json:"portStart"` PortEnd int32 `json:"portEnd"` TargetPortStart int32 `json:"targetPortStart"` TargetPortEnd int32 `json:"targetPortEnd"` + // StandbyTargetPortStart is the starting port number for standby targets during live upgrade StandbyTargetPortStart int32 `json:"standbyTargetPortStart"` + // StandbyTargetPortEnd is the ending port number for standby targets during live upgrade StandbyTargetPortEnd int32 `json:"standbyTargetPortEnd"` + // NvmeSubsystem contains the NVMe device paths configuration and state NvmeSubsystem NvmeSubsystem `json:"nvmeSubsystem"` }
79-117: Document live upgrade strategyThe code introduces support for live upgrades through standby ports and NVMe subsystems, but the strategy needs documentation:
- How the standby ports are used during upgrades
- The role of NVMe subsystem in maintaining availability
- The upgrade process and failure handling
Consider adding a package-level documentation file (doc.go) explaining:
- The live upgrade architecture
- Port allocation strategy
- State transition flows
- Failure recovery mechanisms
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (8)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/go-spdk-helper/pkg/nvme/initiator.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/api/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.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(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/instance/instance.go
pkg/api/instance.go
Outdated
| type NvmeDevicePath struct { | ||
| Trtype string `json:"trtype"` | ||
| Traddr string `json:"traddr"` | ||
| Trsvcid string `json:"trsvcid"` | ||
| SrcAddr string `json:"src_addr"` | ||
| State string `json:"state"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and validation for NvmeDevicePath
The struct needs documentation and field validation:
- Add godoc comments explaining the purpose and usage
- Consider adding validation for required fields
- Define constants for possible state values
Add documentation and constants:
+// NvmeDevicePath represents the configuration and state of an NVMe device path
+const (
+ NvmeDeviceStateActive = "active"
+ NvmeDeviceStateStandby = "standby"
+ // Add other valid states
+)
type NvmeDevicePath struct {
Trtype string `json:"trtype"` // Transport type (e.g., tcp, rdma)
Traddr string `json:"traddr"` // Transport address
Trsvcid string `json:"trsvcid"` // Transport service ID
SrcAddr string `json:"src_addr"`// Source address
State string `json:"state"` // Current state of the device path
}
+// Validate ensures all required fields are set and values are valid
+func (p *NvmeDevicePath) Validate() error {
+ if p.Trtype == "" {
+ return fmt.Errorf("transport type is required")
+ }
+ // Add other validation as needed
+ return nil
+}Committable suggestion skipped: line range outside the PR's diff.
pkg/api/instance.go
Outdated
| nvmeSubstem := NvmeSubsystem{ | ||
| Paths: map[string]NvmeDevicePath{}, | ||
| } | ||
| if obj.NvmeSubsystem != nil { | ||
| for pathName, path := range obj.NvmeSubsystem.Paths { | ||
| nvmeSubstem.Paths[pathName] = NvmeDevicePath{ | ||
| Trtype: path.Trtype, | ||
| Traddr: path.Traddr, | ||
| Trsvcid: path.Trsvcid, | ||
| SrcAddr: path.SrcAddr, | ||
| State: path.State, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation during NVMe path conversion
The path conversion needs validation to ensure data integrity:
- Validate individual paths during conversion
- Handle invalid states gracefully
Add validation during conversion:
nvmeSubstem := NvmeSubsystem{
Paths: map[string]NvmeDevicePath{},
}
if obj.NvmeSubsystem != nil {
for pathName, path := range obj.NvmeSubsystem.Paths {
+ if path == nil {
+ continue
+ }
+ devicePath := NvmeDevicePath{
+ Trtype: path.Trtype,
+ Traddr: path.Traddr,
+ Trsvcid: path.Trsvcid,
+ SrcAddr: path.SrcAddr,
+ State: path.State,
+ }
+ if err := devicePath.Validate(); err != nil {
+ // Log the error but continue processing other paths
+ continue
+ }
- nvmeSubstem.Paths[pathName] = NvmeDevicePath{
- Trtype: path.Trtype,
- Traddr: path.Traddr,
- Trsvcid: path.Trsvcid,
- SrcAddr: path.SrcAddr,
- State: path.State,
- }
+ nvmeSubstem.Paths[pathName] = devicePath
}
}Committable suggestion skipped: line range outside the PR's diff.
derekbit
force-pushed
the
v2-control-upgrade
branch
from
338bbab
to
f706647
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (8)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/go-spdk-helper/pkg/nvme/initiator.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/api/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.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(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/instance/instance.go
🔇 Additional comments (1)
pkg/api/instance.go (1)
79-117: Verify live upgrade functionality
Please verify the following aspects of live upgrade:
- Port allocation strategy during upgrade
- NVMe path state transitions
- Handling of in-flight I/O during upgrade
✅ Verification successful
Let me gather more specific information about the port allocation and state management in the context of upgrades.
Based on the gathered information, I can now generate the final response:
Live upgrade functionality appears properly implemented
The codebase shows robust handling of live upgrades through:
- Proper port allocation and release mechanisms for both target and standby target ports
- Clean port switchover during upgrades (
StandbyTargetPortbecomesTargetPort) - NVMe subsystem state tracking for paths and controllers
- Device mapper reloading after target switchover
Key implementation details:
- Port allocation is managed through a bitmap allocator ensuring no conflicts
- Target switchover includes proper cleanup and state management
- NVMe paths maintain state information for monitoring health
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify live upgrade implementation details
# Check for upgrade-related logic
echo "Searching for upgrade handlers..."
rg -l 'upgrade|migration' --type go
# Check for port allocation logic
echo "Searching for port allocation..."
rg -l 'port.*allocation|allocate.*port' --type go
# Check for NVMe state transition logic
echo "Searching for NVMe state management..."
rg -l 'nvme.*state|state.*transition' --type go
# Look for upgrade tests
echo "Searching for upgrade tests..."
rg -l 'test.*upgrade|upgrade.*test' --type go
Length of output: 6631
Script:
#!/bin/bash
# Check process manager's port allocation implementation
echo "Checking process manager's port allocation..."
rg -A 10 'func.*[Aa]llocate.*[Pp]ort' pkg/process/process_manager.go
# Check instance state management
echo "Checking instance state management..."
rg -A 10 'State.*=' pkg/instance/instance.go
# Check SPDK engine's port and state handling
echo "Checking SPDK engine implementation..."
rg -A 10 'StandbyTargetPort|NvmeSubsystem' vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.go
# Look for upgrade-related state transitions
echo "Checking state transitions..."
rg 'transition|upgrade' pkg/instance/instance.go
Length of output: 6531
pkg/api/instance.go
Outdated
| nvmeSubsystem := NvmeSubsystem{ | ||
| Paths: map[string]NvmeDevicePath{}, | ||
| } | ||
| if obj.NvmeSubsystem != nil { | ||
| for pathName, path := range obj.NvmeSubsystem.Paths { | ||
| nvmeSubsystem.Paths[pathName] = NvmeDevicePath{ | ||
| Trtype: path.Trtype, | ||
| Traddr: path.Traddr, | ||
| Trsvcid: path.Trsvcid, | ||
| SrcAddr: path.SrcAddr, | ||
| State: path.State, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return InstanceStatus{ | ||
| State: obj.State, | ||
| ErrorMsg: obj.ErrorMsg, | ||
| Conditions: obj.Conditions, | ||
| PortStart: obj.PortStart, | ||
| PortEnd: obj.PortEnd, | ||
| TargetPortStart: obj.TargetPortStart, | ||
| TargetPortEnd: obj.TargetPortEnd, | ||
| State: obj.State, | ||
| ErrorMsg: obj.ErrorMsg, | ||
| Conditions: obj.Conditions, | ||
| PortStart: obj.PortStart, | ||
| PortEnd: obj.PortEnd, | ||
| TargetPortStart: obj.TargetPortStart, | ||
| TargetPortEnd: obj.TargetPortEnd, | ||
| StandbyTargetPortStart: obj.StandbyTargetPortStart, | ||
| StandbyTargetPortEnd: obj.StandbyTargetPortEnd, | ||
| NvmeSubsystem: nvmeSubsystem, |
There was a problem hiding this comment.
Add proper error handling and validation in conversion function
The RPCToInstanceStatus function needs better error handling and validation:
- Add nil checks for input parameters
- Validate NVMe paths during conversion
- Validate the final InstanceStatus before returning
-func RPCToInstanceStatus(obj *rpc.InstanceStatus) InstanceStatus {
+func RPCToInstanceStatus(obj *rpc.InstanceStatus) (InstanceStatus, error) {
+ if obj == nil {
+ return InstanceStatus{}, fmt.Errorf("nil instance status")
+ }
+
nvmeSubsystem := NvmeSubsystem{
Paths: map[string]NvmeDevicePath{},
}
if obj.NvmeSubsystem != nil {
for pathName, path := range obj.NvmeSubsystem.Paths {
+ if path == nil {
+ continue
+ }
+ if !isValidTransportType(path.Trtype) {
+ return InstanceStatus{}, fmt.Errorf(
+ "invalid transport type: %s", path.Trtype)
+ }
nvmeSubsystem.Paths[pathName] = NvmeDevicePath{
Trtype: path.Trtype,
Traddr: path.Traddr,
Trsvcid: path.Trsvcid,
SrcAddr: path.SrcAddr,
State: path.State,
}
}
}
- return InstanceStatus{
+ status := InstanceStatus{
State: obj.State,
ErrorMsg: obj.ErrorMsg,
Conditions: obj.Conditions,
PortStart: obj.PortStart,
PortEnd: obj.PortEnd,
TargetPortStart: obj.TargetPortStart,
TargetPortEnd: obj.TargetPortEnd,
StandbyTargetPortStart: obj.StandbyTargetPortStart,
StandbyTargetPortEnd: obj.StandbyTargetPortEnd,
NvmeSubsystem: nvmeSubsystem,
}
+
+ if err := status.Validate(); err != nil {
+ return InstanceStatus{}, fmt.Errorf(
+ "invalid instance status: %v", err)
+ }
+
+ return status, nil
}This change will require updating all callers to handle the error return value.
Committable suggestion skipped: line range outside the PR's diff.
pkg/api/instance.go
Outdated
| type NvmeDevicePath struct { | ||
| Trtype string `json:"trtype"` | ||
| Traddr string `json:"traddr"` | ||
| Trsvcid string `json:"trsvcid"` | ||
| SrcAddr string `json:"src_addr"` | ||
| State string `json:"state"` | ||
| } | ||
|
|
||
| type NvmeSubsystem struct { | ||
| Paths map[string]NvmeDevicePath `json:"paths"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and type safety for NVMe structures
The new NVMe-related structures need documentation and type safety improvements:
- Add package documentation explaining the NVMe subsystem concept
- Define string constants for valid states and transport types
- Add validation methods
+// NvmeTransportType represents the supported NVMe transport protocols
+type NvmeTransportType string
+
+const (
+ NvmeTransportTCP NvmeTransportType = "tcp"
+ NvmeTransportRDMA NvmeTransportType = "rdma"
+)
+
+// NvmeDeviceState represents the possible states of an NVMe device path
+type NvmeDeviceState string
+
+const (
+ NvmeDeviceStateActive NvmeDeviceState = "active"
+ NvmeDeviceStateStandby NvmeDeviceState = "standby"
+)
+
+// NvmeDevicePath represents a path configuration for NVMe-oF (NVMe over Fabrics)
type NvmeDevicePath struct {
- Trtype string `json:"trtype"`
+ Trtype NvmeTransportType `json:"trtype"`
Traddr string `json:"traddr"`
Trsvcid string `json:"trsvcid"`
SrcAddr string `json:"src_addr"`
- State string `json:"state"`
+ State NvmeDeviceState `json:"state"`
}
+// NvmeSubsystem represents an NVMe subsystem with multiple device paths
+// used for high availability and live migration
type NvmeSubsystem struct {
Paths map[string]NvmeDevicePath `json:"paths"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type NvmeDevicePath struct { | |
| Trtype string `json:"trtype"` | |
| Traddr string `json:"traddr"` | |
| Trsvcid string `json:"trsvcid"` | |
| SrcAddr string `json:"src_addr"` | |
| State string `json:"state"` | |
| } | |
| type NvmeSubsystem struct { | |
| Paths map[string]NvmeDevicePath `json:"paths"` | |
| } | |
| // NvmeTransportType represents the supported NVMe transport protocols | |
| type NvmeTransportType string | |
| const ( | |
| NvmeTransportTCP NvmeTransportType = "tcp" | |
| NvmeTransportRDMA NvmeTransportType = "rdma" | |
| ) | |
| // NvmeDeviceState represents the possible states of an NVMe device path | |
| type NvmeDeviceState string | |
| const ( | |
| NvmeDeviceStateActive NvmeDeviceState = "active" | |
| NvmeDeviceStateStandby NvmeDeviceState = "standby" | |
| ) | |
| // NvmeDevicePath represents a path configuration for NVMe-oF (NVMe over Fabrics) | |
| type NvmeDevicePath struct { | |
| Trtype NvmeTransportType `json:"trtype"` | |
| Traddr string `json:"traddr"` | |
| Trsvcid string `json:"trsvcid"` | |
| SrcAddr string `json:"src_addr"` | |
| State NvmeDeviceState `json:"state"` | |
| } | |
| // NvmeSubsystem represents an NVMe subsystem with multiple device paths | |
| // used for high availability and live migration | |
| type NvmeSubsystem struct { | |
| Paths map[string]NvmeDevicePath `json:"paths"` | |
| } |
pkg/api/instance.go
Outdated
| State string `json:"state"` | ||
| ErrorMsg string `json:"errorMsg"` | ||
| Conditions map[string]bool `json:"conditions"` | ||
| PortStart int32 `json:"portStart"` | ||
| PortEnd int32 `json:"portEnd"` | ||
| TargetPortStart int32 `json:"targetPortStart"` | ||
| TargetPortEnd int32 `json:"targetPortEnd"` | ||
| StandbyTargetPortStart int32 `json:"standbyTargetPortStart"` | ||
| StandbyTargetPortEnd int32 `json:"standbyTargetPortEnd"` | ||
| NvmeSubsystem NvmeSubsystem `json:"nvmeSubsystem"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for port ranges and document fields
The InstanceStatus struct needs validation and documentation for the new fields:
- Document the relationship between active and standby ports
- Add validation to prevent port range overlaps
- Consider using a dedicated type for port ranges
+// PortRange represents a range of network ports
+type PortRange struct {
+ Start int32 `json:"start"`
+ End int32 `json:"end"`
+}
+
+// InstanceStatus represents the current state and configuration of an instance
type InstanceStatus struct {
State string `json:"state"`
ErrorMsg string `json:"errorMsg"`
Conditions map[string]bool `json:"conditions"`
- PortStart int32 `json:"portStart"`
- PortEnd int32 `json:"portEnd"`
- TargetPortStart int32 `json:"targetPortStart"`
- TargetPortEnd int32 `json:"targetPortEnd"`
- StandbyTargetPortStart int32 `json:"standbyTargetPortStart"`
- StandbyTargetPortEnd int32 `json:"standbyTargetPortEnd"`
+ // Port range for general instance communication
+ Ports PortRange `json:"ports"`
+ // Port range for active NVMe target
+ TargetPorts PortRange `json:"targetPorts"`
+ // Port range for standby NVMe target used during live migration
+ StandbyTargetPorts PortRange `json:"standbyTargetPorts"`
NvmeSubsystem NvmeSubsystem `json:"nvmeSubsystem"`
}
+
+func (pr *PortRange) Validate() error {
+ if pr.Start > pr.End {
+ return fmt.Errorf("invalid port range: start(%d) > end(%d)",
+ pr.Start, pr.End)
+ }
+ return nil
+}
+
+func (is *InstanceStatus) Validate() error {
+ if err := is.Ports.Validate(); err != nil {
+ return fmt.Errorf("invalid ports: %v", err)
+ }
+ if err := is.TargetPorts.Validate(); err != nil {
+ return fmt.Errorf("invalid target ports: %v", err)
+ }
+ if err := is.StandbyTargetPorts.Validate(); err != nil {
+ return fmt.Errorf("invalid standby target ports: %v", err)
+ }
+ // Check for overlaps between active and standby ports
+ if is.TargetPorts.Start <= is.StandbyTargetPorts.End &&
+ is.StandbyTargetPorts.Start <= is.TargetPorts.End {
+ return fmt.Errorf("target port ranges overlap")
+ }
+ return nil
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| State string `json:"state"` | |
| ErrorMsg string `json:"errorMsg"` | |
| Conditions map[string]bool `json:"conditions"` | |
| PortStart int32 `json:"portStart"` | |
| PortEnd int32 `json:"portEnd"` | |
| TargetPortStart int32 `json:"targetPortStart"` | |
| TargetPortEnd int32 `json:"targetPortEnd"` | |
| StandbyTargetPortStart int32 `json:"standbyTargetPortStart"` | |
| StandbyTargetPortEnd int32 `json:"standbyTargetPortEnd"` | |
| NvmeSubsystem NvmeSubsystem `json:"nvmeSubsystem"` | |
| // PortRange represents a range of network ports | |
| type PortRange struct { | |
| Start int32 `json:"start"` | |
| End int32 `json:"end"` | |
| } | |
| // InstanceStatus represents the current state and configuration of an instance | |
| type InstanceStatus struct { | |
| State string `json:"state"` | |
| ErrorMsg string `json:"errorMsg"` | |
| Conditions map[string]bool `json:"conditions"` | |
| // Port range for general instance communication | |
| Ports PortRange `json:"ports"` | |
| // Port range for active NVMe target | |
| TargetPorts PortRange `json:"targetPorts"` | |
| // Port range for standby NVMe target used during live migration | |
| StandbyTargetPorts PortRange `json:"standbyTargetPorts"` | |
| NvmeSubsystem NvmeSubsystem `json:"nvmeSubsystem"` | |
| } | |
| func (pr *PortRange) Validate() error { | |
| if pr.Start > pr.End { | |
| return fmt.Errorf("invalid port range: start(%d) > end(%d)", | |
| pr.Start, pr.End) | |
| } | |
| return nil | |
| } | |
| func (is *InstanceStatus) Validate() error { | |
| if err := is.Ports.Validate(); err != nil { | |
| return fmt.Errorf("invalid ports: %v", err) | |
| } | |
| if err := is.TargetPorts.Validate(); err != nil { | |
| return fmt.Errorf("invalid target ports: %v", err) | |
| } | |
| if err := is.StandbyTargetPorts.Validate(); err != nil { | |
| return fmt.Errorf("invalid standby target ports: %v", err) | |
| } | |
| // Check for overlaps between active and standby ports | |
| if is.TargetPorts.Start <= is.StandbyTargetPorts.End && | |
| is.StandbyTargetPorts.Start <= is.TargetPorts.End { | |
| return fmt.Errorf("target port ranges overlap") | |
| } | |
| return nil | |
| } |
derekbit
force-pushed
the
v2-control-upgrade
branch
from
f706647
to
5977643
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/util/util.go (1)
Line range hint
129-137: Consider enhancing error logging in IsSPDKTgtReady.While the function works correctly, it could benefit from debug-level logging to help diagnose connectivity issues in production.
Consider adding debug logging:
func IsSPDKTgtReady(timeout time.Duration) bool { for i := 0; i < int(timeout.Seconds()); i++ { conn, err := net.DialTimeout(helpertypes.DefaultJSONServerNetwork, helpertypes.DefaultUnixDomainSocketPath, 1*time.Second) if err == nil { conn.Close() return true } + logrus.Debugf("SPDK target not ready yet, attempt %d: %v", i+1, err) time.Sleep(time.Second) } return false }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (11)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/go-spdk-helper/pkg/nvme/initiator.gois excluded by!vendor/**vendor/github.com/longhorn/go-spdk-helper/pkg/nvme/nvme.gois excluded by!vendor/**vendor/github.com/longhorn/go-spdk-helper/pkg/nvme/nvmecli.gois excluded by!vendor/**vendor/github.com/longhorn/go-spdk-helper/pkg/types/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/api/types.gois excluded by!vendor/**vendor/github.com/longhorn/longhorn-spdk-engine/pkg/spdk/engine.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 (3)
pkg/api/instance.go(1 hunks)pkg/instance/instance.go(4 hunks)pkg/util/util.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/instance/instance.go
🔇 Additional comments (5)
pkg/util/util.go (1)
20-20: LGTM! Clean import alias change.
The change from spdkhelpertypes to helpertypes is applied consistently across the codebase. The usage of the imported constants remains correct.
Also applies to: 131-131
pkg/api/instance.go (4)
92-117: Verify and update all callers of RPCToInstanceStatus
The signature change of RPCToInstanceStatus requires updates to all calling functions.
92-117:
Add proper error handling and validation in conversion function
The RPCToInstanceStatus function needs better error handling to prevent panics and ensure data consistency:
- Add nil checks for input parameters
- Validate NVMe paths during conversion
- Validate the final status before returning
-func RPCToInstanceStatus(obj *rpc.InstanceStatus) InstanceStatus {
+func RPCToInstanceStatus(obj *rpc.InstanceStatus) (InstanceStatus, error) {
+ if obj == nil {
+ return InstanceStatus{}, fmt.Errorf("nil instance status")
+ }
+
nvmeSubsystem := NvmeSubsystem{
Paths: map[string]NvmeDevicePath{},
}
if obj.NvmeSubsystem != nil {
for pathName, path := range obj.NvmeSubsystem.Paths {
+ if path == nil {
+ continue
+ }
nvmeSubsystem.Paths[pathName] = NvmeDevicePath{
Trtype: path.Trtype,
Traddr: path.Traddr,
Trsvcid: path.Trsvcid,
SrcAddr: path.SrcAddr,
State: path.State,
}
+ if err := nvmeSubsystem.Paths[pathName].Validate(); err != nil {
+ return InstanceStatus{}, fmt.Errorf("invalid path %q: %v", pathName, err)
+ }
}
}
- return InstanceStatus{
+ status := InstanceStatus{
State: obj.State,
ErrorMsg: obj.ErrorMsg,
Conditions: obj.Conditions,
PortStart: obj.PortStart,
PortEnd: obj.PortEnd,
TargetPortStart: obj.TargetStart,
TargetPortEnd: obj.TargetEnd,
StandbyTargetPortStart: obj.StandbyTargetPortStart,
StandbyTargetPortEnd: obj.StandbyTargetPortEnd,
NvmeSubsystem: nvmeSubsystem,
}
+
+ if err := status.Validate(); err != nil {
+ return InstanceStatus{}, fmt.Errorf("invalid status: %v", err)
+ }
+
+ return status, nil
}Note: This change will require updating all callers to handle the error return value.
Likely invalid or redundant comment.
66-76: 🛠️ Refactor suggestion
Enhance type safety and add documentation for NVMe structures
The NVMe structures need improvements for better maintainability and reliability:
- Add type safety for transport types and states
- Add documentation for types and fields
- Add validation methods
+// TransportType represents the NVMe over Fabrics transport protocol
+type TransportType string
+const (
+ TransportTCP TransportType = "tcp"
+ TransportRDMA TransportType = "rdma"
+)
+// DeviceState represents the operational state of an NVMe device path
+type DeviceState string
+const (
+ DeviceStateActive DeviceState = "active"
+ DeviceStateStandby DeviceState = "standby"
+)
+// NvmeDevicePath represents a connection path to an NVMe over Fabrics target
type NvmeDevicePath struct {
- Trtype string `json:"trtype"`
+ Trtype TransportType `json:"trtype"` // Transport protocol (tcp/rdma)
Traddr string `json:"traddr"` // Target address
Trsvcid string `json:"trsvcid"` // Target port
SrcAddr string `json:"src_addr"`// Source address
- State string `json:"state"`
+ State DeviceState `json:"state"` // Current device state
}
+// Validate checks if the NvmeDevicePath fields are valid
+func (p *NvmeDevicePath) Validate() error {
+ if p.Trtype == "" {
+ return fmt.Errorf("transport type is required")
+ }
+ if p.Traddr == "" {
+ return fmt.Errorf("target address is required")
+ }
+ return nil
+}
+// NvmeSubsystem represents an NVMe subsystem containing multiple device paths
+// used for high availability and live migration support
type NvmeSubsystem struct {
Paths map[string]NvmeDevicePath `json:"paths"`
}
+// Validate checks if the NvmeSubsystem configuration is valid
+func (s *NvmeSubsystem) Validate() error {
+ for name, path := range s.Paths {
+ if err := path.Validate(); err != nil {
+ return fmt.Errorf("invalid path %q: %v", name, err)
+ }
+ }
+ return nil
+}Likely invalid or redundant comment.
79-88: 🛠️ Refactor suggestion
Add documentation and validation for InstanceStatus
The InstanceStatus struct needs documentation and validation for the new fields to prevent configuration errors:
- Document the purpose of standby target ports
- Add validation to prevent port range conflicts
- Consider using a dedicated type for port ranges
+// InstanceStatus represents the current state and configuration of an instance
+// including its NVMe subsystem and port allocations for live upgrades
type InstanceStatus struct {
State string `json:"state"` // Current operational state
ErrorMsg string `json:"errorMsg"` // Error message if any
Conditions map[string]bool `json:"conditions"`// Instance conditions
PortStart int32 `json:"portStart"` // Start of general port range
PortEnd int32 `json:"portEnd"` // End of general port range
TargetPortStart int32 `json:"targetPortStart"` // Start of active target ports
TargetPortEnd int32 `json:"targetPortEnd"` // End of active target ports
StandbyTargetPortStart int32 `json:"standbyTargetPortStart"` // Start of standby target ports for live upgrade
StandbyTargetPortEnd int32 `json:"standbyTargetPortEnd"` // End of standby target ports for live upgrade
NvmeSubsystem NvmeSubsystem `json:"nvmeSubsystem"` // NVMe subsystem configuration
}
+// Validate checks if the InstanceStatus configuration is valid
+func (s *InstanceStatus) Validate() error {
+ if s.PortEnd < s.PortStart {
+ return fmt.Errorf("invalid port range: %d-%d", s.PortStart, s.PortEnd)
+ }
+ if s.TargetPortEnd < s.TargetPortStart {
+ return fmt.Errorf("invalid target port range: %d-%d", s.TargetPortStart, s.TargetPortEnd)
+ }
+ if s.StandbyTargetPortEnd < s.StandbyTargetPortStart {
+ return fmt.Errorf("invalid standby target port range: %d-%d", s.StandbyTargetPortStart, s.StandbyTargetPortEnd)
+ }
+ // Check for overlapping port ranges
+ if s.TargetPortStart <= s.StandbyTargetPortEnd && s.StandbyTargetPortStart <= s.TargetPortEnd {
+ return fmt.Errorf("target port ranges overlap: active(%d-%d) standby(%d-%d)",
+ s.TargetPortStart, s.TargetPortEnd, s.StandbyTargetPortStart, s.StandbyTargetPortEnd)
+ }
+ return s.NvmeSubsystem.Validate()
+}Likely invalid or redundant comment.
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
5977643
to
36f5024
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