feat(auto-salvage): v2 support #3170
Conversation
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
82f0307 to
f93085b
Compare
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
3 times, most recently
from
66f7725 to
5aec546
Compare
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
5aec546 to
88859ee
Compare
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
88859ee to
5cb12ad
Compare
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
5cb12ad to
4a76f0f
Compare
WalkthroughThe pull request introduces several modifications across three files, focusing on enhancing volume management and synchronization processes. Key changes include the unconditional setting of the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
|
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
4a76f0f to
b8328b2
Compare
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
5 times, most recently
from
c87cb79 to
3c1af3e
Compare
![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 (1)
controller/volume_controller.go (1)
1391-1391: Improve log message clarityThe log message could be more descriptive to indicate the auto-salvage operation.
-log.Infof("Bringing up %v replicas for auto-salvage", len(failedUsableReplicas)) +log.Infof("Auto-salvaging %v failed but usable replicas", len(failedUsableReplicas))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (6)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/longhorn-instance-manager/pkg/client/instance.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)
controller/volume_controller.go(2 hunks)engineapi/instance_manager.go(1 hunks)manager/volume.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- manager/volume.go
🔇 Additional comments (2)
engineapi/instance_manager.go (1)
516-516: LGTM: SalvageRequested field addition
The addition of the SalvageRequested field to the EngineCreateRequest structure properly propagates the salvage request flag from the engine specification to the instance manager.
Let's verify the salvage flag propagation:
✅ Verification successful
Based on the search results, I can now generate the final response as there's sufficient evidence to verify the proper propagation of the SalvageRequested field.
✓ SalvageRequested field is properly integrated
The SalvageRequested field is consistently implemented across the codebase:
- Properly defined in protobuf specifications (
spdk.pb.goandinstance.pb.go) - Correctly propagated from engine spec to instance manager
- Integrated with the auto-salvage workflow in
volume_controller.go - Properly handled in instance lifecycle management (
instance_handler.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent handling of SalvageRequested across the codebase
rg -A 5 "SalvageRequested"
Length of output: 9142
controller/volume_controller.go (1)
1336-1337: Unconditional auto-salvage request for failed replicas
The change removes the data engine version check and unconditionally sets e.Spec.SalvageRequested = true when all replicas fail. This change enables auto-salvage support for v2 volumes.
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
3c1af3e to
d000dab
Compare
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
2 times, most recently
from
f8ffd7e to
39e294e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
controller/volume_controller.go (1)
Line range hint
1-1924: Consider code structure improvementsWhile the implementation is functionally correct, consider the following improvements:
- Break down large functions like
ReconcileVolumeStateinto smaller, more focused functions- Add package-level documentation explaining the controller's responsibilities
- Extract magic numbers into named constants (e.g., retry counts, timeouts)
- Add more inline documentation for complex logic sections
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sumvendor/github.com/longhorn/longhorn-instance-manager/pkg/client/instance.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (3)
controller/volume_controller.go(2 hunks)engineapi/instance_manager.go(1 hunks)manager/volume.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- engineapi/instance_manager.go
- manager/volume.go
🔇 Additional comments (1)
controller/volume_controller.go (1)
Line range hint 1391-1401: LGTM: Well-structured error handling and logging
The error handling and logging implementation is robust:
- Uses structured logging with appropriate context
- Proper error wrapping and propagation
- Clear log messages for debugging
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
39e294e to
7468162
Compare
longhorn/longhorn-8430 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-8430 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-8430 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-8430 Signed-off-by: Chin-Ya Huang <[email protected]>
Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
7468162 to
3d806c6
Compare
Which issue(s) this PR fixes:
Issue longhorn/longhorn#8430
What this PR does / why we need it:
Support auto-salvage for v2 volumes.
Special notes for your reviewer:
NoneAdditional documentation or context
None