Fix skip DRS for a VM#12994
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes DRS skip behavior when the SKIP_DRS VM detail is set but not present in the in-memory VM details map by centralizing skip logic and consulting the DB-backed VM details DAO.
Changes:
- Added DB-backed lookup of
VmDetailConstants.SKIP_DRSviaUserVmDetailsDao. - Centralized DRS skip logic into
shouldSkipVMForDRSand reused it in both the main loop andskipDrs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12994 +/- ##
=========================================
Coverage 16.26% 16.26%
+ Complexity 13433 13430 -3
=========================================
Files 5665 5665
Lines 500572 500580 +8
Branches 60792 60785 -7
=========================================
+ Hits 81417 81423 +6
- Misses 410047 410051 +4
+ Partials 9108 9106 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17444 |
|
[SF] Trillian Build Failed (tid-15852) |
|
@blueorangutan test |
|
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15855)
|
kiranchavala
left a comment
There was a problem hiding this comment.
As discussed, if we could add a log message saying we are skipping a vm from drs plan when "generateClusterDrsPlan" api is called it will be beneficial to the admin user when troubleshooting.
Also, please update the doc about the "skipFromDRS" in vmsettings
https://docs.cloudstack.apache.org/en/4.22.0.0/adminguide/clusters.html#cloudstack-drs
…erviceImpl.java Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>
Created a doc PR here: apache/cloudstack-documentation#653 |
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17932 |
|
@blueorangutan test |
|
@vishesh92 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-16120) |
Description
When a VM has
skipFromDRSdetail set, it is still not getting skipped. This PR fixes it.Doc PR - apache/cloudstack-documentation#653
Details
This pull request refactors how the system determines whether a VM should be skipped for DRS (Distributed Resource Scheduler) operations. The main improvement is centralizing and simplifying the skip logic, now using the `UserVmDetailsDao` to check VM details in the database rather than relying solely on in-memory details. This ensures more accurate and consistent behavior.
Key changes:
Refactoring skip logic for DRS:
shouldSkipVMForDRSmethod to encapsulate all logic for determining if a VM should be skipped, making the code cleaner and easier to maintain.shouldSkipVMForDRSmethod, ensuring consistent skip checks throughout the codebase.Improved VM detail retrieval:
SKIP_DRSflag usingUserVmDetailsDaofrom the database, instead of only checking the VM's in-memory details map. This makes the skip logic more robust and reliable.Dependency injection:
UserVmDetailsDaoto enable database access for VM details.UserVmDetailVOandUserVmDetailsDaofor use in the new logic.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?