Repository navigation
conditional error reporting for vm move failure - #13675
DaanHoogland wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.20 #13675 +/- ##
============================================
+ Coverage 16.45% 16.46% +0.01%
- Complexity 13710 13719 +9
============================================
Files 5669 5670 +1
Lines 501734 501741 +7
Branches 60961 60962 +1
============================================
+ Hits 82584 82636 +52
+ Misses 409895 409849 -46
- Partials 9255 9256 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6cfbcc8 to
c03b25e
Compare
There was a problem hiding this comment.
Pull request overview
Adds a configurable way to return more informative API error messages when assignVirtualMachine (VM ownership change) fails, addressing the lack of actionable failure details reported in #13406.
Changes:
- Introduces a new global ConfigKey to control whether detailed assign/move failure reasons are exposed to API callers.
- Updates
AssignVMCmdto optionally returnInvalidParameterValueExceptionmessages to the caller when enabled. - Registers the new ConfigKey in
UserVmManagerImpl’s config key list.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | Registers the new configuration key for VM-assign failure detail exposure. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/vm/AssignVMCmd.java | Conditionally returns a detailed failure reason from InvalidParameterValueException instead of only logging it. |
| api/src/main/java/com/cloud/vm/UserVmService.java | Defines the new global ConfigKey controlling detailed failure message exposure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@blueorangutan package |
|
@DaanHoogland 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 18663 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18765 |
|
[SF] Trillian test result (tid-16694)
|
|
|
@daviftorres this is getting orphaned (slowly) are you still looking for test possibilities? |
Hey @DaanHoogland, I raised this issue just to bring awareness to it. I’m not actively working on or testing this one right now. |
JoaoJandre
left a comment
JoaoJandre
left a comment
There was a problem hiding this comment.
From my point of view, we could always tell the user why the VM could not be moved. I think adding a config for it is a bit much. Also, in the future, will we add new configurations for each situation like this?
I guess we should start thinking about a way to configure which messages get seen by which types of users. And if we should have these behaviors be more configurable and flexible.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
db428c7 to
29f1ccb
Compare
makes sense @JoaoJandre . care to start a discussion or an issue? |
|
Done: #14692 |
Description
This PR...
Fixes: #13406
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?