feat: support v2 volume live migration #3323
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (4)k8s/crds.yaml (1)
The new controller/volume_controller.go (3)
The function now properly handles both v1 and v2 data engines. For v2, it reuses existing replicas instead of creating new clones, which is the expected behavior for v2 engine live migration.
For v2 data engine, the function clears the migrationEngineName instead of deleting the replica, which is the correct behavior for v2 engine live migration.
The function properly updates the migrationEngineName and engineName for v2 data engine replicas, while maintaining the existing active state switching behavior for v1 engine. 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 (2)
k8s/pkg/apis/longhorn/v1beta2/replica.go (1)
28-31: Fix grammatical errors in the comment forMigratingEngineNameThe comment should be corrected for clarity and grammar.
Apply this diff to fix the grammatical errors:
// +optional -// MigratingEngineName is indicating the migrating engine which current connected to this replica. This is only -// used for live migration of v2 data engine +// MigratingEngineName indicates the migrating engine which is currently connected to this replica. This is only +// used for live migration of v2 data engines. MigratingEngineName string `json:"migratingEngineName"`k8s/crds.yaml (1)
2993-2996: Fix grammatical errors in the description ofmigratingEngineNameThe description should be corrected for clarity and grammar.
Apply this diff to fix the grammatical errors:
description: |- - MigratingEngineName is indicating the migrating engine which current connected to this replica. This is only - used for live migration of v2 data engine + MigratingEngineName indicates the migrating engine which is currently connected to this replica. This is only + used for live migration of v2 data engines. type: string
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
controller/volume_controller.go(7 hunks)k8s/crds.yaml(70 hunks)k8s/pkg/apis/longhorn/v1beta2/replica.go(1 hunks)
🔇 Additional comments (6)
controller/volume_controller.go (6)
4025-4035: LGTM!
The code correctly handles Data Engine V2 by reusing existing replicas without cloning. This ensures efficiency by avoiding unnecessary duplication when matching replicas during migration.
4065-4070: LGTM!
Properly differentiates between Data Engine V1 and V2 during migration replica deletion. For V1, it deletes the invalid migration replicas, and for V2, it resets the MigratingEngineName to disconnect the replica from the migrating engine.
4154-4163: LGTM!
Correctly handles the switch of active replicas for Data Engine V1 and V2 during migration completion. It switches active replicas based on the data engine type, ensuring that V1 replicas are appropriately managed and V2 replicas have their MigratingEngineName reset.
4200-4212: LGTM!
Properly reverts migration for Data Engine V1 and V2. In the case of V1, it deletes migration replicas not associated with the current engine. For V2, it clears the MigratingEngineName to clean up migration state from the replicas.
4339-4339: LGTM!
The call to deleteInvalidMigrationReplicas with the volume parameter ensures that invalid replicas are correctly handled based on the data engine type during migration preparation.
4344-4349: LGTM!
Appropriately sets EngineName or MigratingEngineName based on DataEngineType during replica creation. This ensures that replicas are correctly associated with the appropriate engine during migration for both V1 and V2 data engines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
controller/volume_controller.go (1)
4025-4035: Data Engine V2: Replica handling optimization during migrationThe implementation correctly handles replica management differently for data engine v2 by reusing existing replicas instead of cloning them. This is more efficient for v2 volumes.
Consider adding a comment explaining why v2 data engine can reuse replicas directly without cloning, to help future maintainers understand the architectural difference.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/volume_controller.go(7 hunks)k8s/crds.yaml(70 hunks)k8s/pkg/apis/longhorn/v1beta2/replica.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- k8s/pkg/apis/longhorn/v1beta2/replica.go
🔇 Additional comments (4)
k8s/crds.yaml (2)
2992-2996: LGTM: Well-documented migration tracking field
The addition of migratingEngineName field with clear documentation is a good implementation for tracking engine connections during v2 data engine live migration.
2993-2995: LGTM: Consistent description formatting across CRDs
The description formatting is consistent across all CRDs, following the same pattern for:
- Field descriptions
- Transition messages
- Error conditions
This consistency improves maintainability and readability of the CRDs.
Also applies to: 3028-3031, 3644-3647, 3764-3767, 3891-3894
controller/volume_controller.go (2)
4065-4070: Data Engine V2: Safe cleanup of migration replicas
The code appropriately handles v2 engine cleanup by clearing migratingEngineName instead of deleting replicas. This is safer than the v1 approach.
✅ Verification successful
Let me try a different search pattern to verify the handling of v2 engine replicas during migration.
Data Engine V2: Replica handling during migration is correctly implemented
The code search confirms that the implementation correctly handles replicas differently based on the data engine type:
- For V1: Replicas are deleted using
deleteReplica - For V2: Only the
MigratingEngineNamefield is cleared, preserving the replicas
The implementation aligns with the expected behavior mentioned in the review comment and follows a safer approach for V2 engines.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that we don't accidentally delete v2 replicas during migration
ast-grep --pattern 'if v.Spec.DataEngine == longhorn.DataEngineTypeV2 {
$$$
r.Spec.MigratingEngineName = ""
$$$
}'
Length of output: 122
Script:
#!/bin/bash
# Search for data engine type checks and replica handling patterns
rg -U "DataEngine.*Type" -A 5 controller/volume_controller.go
# Also check for any replica deletion patterns
rg -U "deleteReplica.*DataEngine" -A 5 controller/volume_controller.go
Length of output: 1252
4200-4213: Data Engine V2: Migration revert handling
The implementation correctly handles migration revert for v2 by clearing migratingEngineName on replicas instead of deleting them.
✅ Verification successful
Let me verify if there are any other places in the codebase where migration revert is handled differently for v2 engines.
Data Engine V2: Migration revert handling is consistent
The implementation correctly handles migration revert for both v1 and v2 data engines. For v1, it deletes replicas, while for v2, it clears the migratingEngineName field. This behavior is consistent across the codebase, with similar patterns found in other migration-related operations where v2 replicas are handled by managing the migratingEngineName field instead of deletion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent handling of v2 replicas during migration revert
ast-grep --pattern 'if v.Spec.DataEngine == longhorn.DataEngineTypeV1 {
$$$
} else {
for _, r := range rs {
r.Spec.MigratingEngineName = ""
}
}'
Length of output: 1138
Script:
#!/bin/bash
# Search for other instances of migration revert handling
rg -A 5 -B 5 "DataEngine.*DataEngineTypeV1" --type go
# Also check for any other places where MigratingEngineName is modified
rg "MigratingEngineName" --type go
Length of output: 10929
|
Hello @PhanLe1010 |
|
Hello @PhanLe1010 |
Thank you. I am fixing it |
PhanLe1010
dismissed stale reviews from derekbit and james-munson
via
5afd4b6
PhanLe1010
force-pushed
the
6361
branch
3 times, most recently
from
57de909 to
6f2569c
Compare
|
All fixed. Thanks @derekbit |
PhanLe1010
changed the title
longhorn-6361 Signed-off-by: Phan Le <[email protected]>
longhorn/longhorn#6361