fix: use volume state to check all volumes detached when changing some settings #3490
Conversation
WalkthroughThe pull request introduces modifications to the Longhorn system's volume and engine instance state checking mechanisms. The primary change involves shifting from checking engine instances to using volume states for determining whether volumes are detached. This modification addresses potential issues with orphaned engine instances that could incorrectly block settings changes. The changes are implemented across multiple files, including the setting controller, datastore, and system restore validator, ensuring a consistent approach to volume state validation. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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
|
james-munson
force-pushed
the
10233-volumes-detached
branch
from
f45199a to
58a62ec
Compare
|
Will defer to @derekbit to take a look first |
Signed-off-by: James Munson <[email protected]>
derekbit
force-pushed
the
10233-volumes-detached
branch
from
58a62ec to
cbb72cd
Compare
|
@james-munson LGTM. Could you run full regression and e2e test before merging it? Thanks. |
![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
🧹 Nitpick comments (1)
datastore/longhorn.go (1)
Line range hint
397-597: Consider standardizing the volume detachment check approachThe codebase currently has a mix of approaches for checking volume detachment:
- Engine instance-based checks via
AreAllEngineInstancesStopped- Volume state-based checks via
AreAllVolumesDetachedStateConsider:
- Standardizing on the volume state-based approach as per PR objectives
- Updating
ValidateV1DataEngineEnabledandValidateV2DataEngineEnabledto useAreAllVolumesDetachedState- Adding documentation explaining the rationale for the chosen approach
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/setting_controller.go(5 hunks)datastore/longhorn.go(3 hunks)webhook/resources/systemrestore/validator.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webhook/resources/systemrestore/validator.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (6)
controller/setting_controller.go (4)
Line range hint
303-315: LGTM! Clean implementation of volume state check.The change to use
AreAllVolumesDetachedState()for storage network setting updates is well-implemented with proper error handling and clear error messages.
Line range hint
425-431: LGTM! Consistent implementation of volume state check.The change to use
AreAllVolumesDetachedState()for taint toleration updates maintains consistency with other setting updates and includes proper error handling.
Line range hint
596-602: LGTM! Consistent implementation of volume state check.The change to use
AreAllVolumesDetachedState()for priority class updates follows the same pattern, maintaining code consistency.
1261-1266: LGTM! Appropriate use of engine instance check.Keeping
AreAllEngineInstancesStopped()for instance manager CPU request updates is correct since this setting specifically deals with engine instances rather than volume states.Let's verify this decision with the following script:
✅ Verification successful
Verification confirms appropriate use of engine instance check
The usage of
AreAllEngineInstancesStopped()is correctly implemented for both instance manager CPU settings and data engine enablement settings, as these configurations directly impact engine instances and require them to be stopped before applying changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage context of AreAllEngineInstancesStopped # Expected: This method should only be used for engine-specific operations # Search for all usages of AreAllEngineInstancesStopped rg -A 5 "AreAllEngineInstancesStopped" # Search for any related engine instance management code ast-grep --pattern 'func $_($_) { $$$ AreAllEngineInstancesStopped($$$) $$$ }'Length of output: 1867
Script:
#!/bin/bash # Search for setting names and their context rg -B 2 -A 2 "SettingNameV[12]DataEngine" # Search for setting definitions rg "const.*SettingName.*DataEngine.*=" # Search for the specific setting update in setting controller ast-grep --pattern 'func ($_ *SettingController) $_($_) { $$$ AreAllEngineInstancesStopped($$$) $$$ }'Length of output: 27415
datastore/longhorn.go (2)
Line range hint
537-567: LGTM! Well-structured implementation for checking engine instancesThe function properly handles both engine and all-in-one instance managers, with appropriate error handling and clear logic for determining if instances are stopped.
Line range hint
584-597: LGTM! Clean implementation for checking volume statesThe function provides a straightforward way to check if all volumes are detached by directly checking their states, which aligns with the PR objectives.
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10233
What this PR does / why we need it:
Revert to checking volume state rather than engine instances when changing some settings.
Rename the engine instance check to reflect what it actually does.
Special notes for your reviewer:
Regression tests for changing settings passed: https://ci.longhorn.io/job/private/job/longhorn-tests-regression/8332/
Additional documentation or context