api,server: allow cleaning up vm extraconfig#11974
Conversation
Fixes apache#9878 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11974 +/- ##
============================================
+ Coverage 17.84% 17.86% +0.01%
- Complexity 15983 16008 +25
============================================
Files 5929 5930 +1
Lines 531084 531457 +373
Branches 64914 64973 +59
============================================
+ Hits 94795 94923 +128
- Misses 425675 425908 +233
- Partials 10614 10626 +12
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:
|
|
@blueorangutan package |
|
@shwstppr 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 15738 |
|
@blueorangutan package |
|
@shwstppr 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. |
| sb.and("vmId", sb.entity().getResourceId(), SearchCriteria.Op.EQ); | ||
| sb.and("prefix", sb.entity().getName(), SearchCriteria.Op.LIKE); | ||
| sb.done(); | ||
| SearchCriteria<VMInstanceDetailVO> sc = sb.create(); | ||
| sc.setParameters("vmId", vmId); | ||
| sc.setParameters("prefix", prefix + "%"); |
There was a problem hiding this comment.
This bit gives me to think that we might want to include these in a DbConstants, similar to the ApiConstants. No critisism on this PR as I know this is the general practice atm.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15874 |
abh1sar
left a comment
There was a problem hiding this comment.
A minor comment. Otherwise the code LGTM.
|
@blueorangutan package |
|
@shwstppr 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 15963 |
|
@shwstppr Since this is for the 4.22.1 release, could you retarget the PR to the 4.22 branch? |
|
@rajujith this adds a new API request param, so I think it is better to go in 4.23. I feel this was wrongly added to 4.22.1 |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
Pull request overview
Enables updateVirtualMachine to remove (“clean up”) stored VM extraconfig entries, with UI support and unit tests.
Changes:
- Added
cleanupextraconfigAPI parameter toupdateVirtualMachine. - Implemented server-side extraconfig cleanup via a new DAO helper that removes VM instance details by prefix.
- Updated the UI to send
cleanupextraconfig=truewhen extraconfig is cleared, and added unit tests for the new behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/compute/EditVM.vue | Sends cleanupextraconfig when the user clears extraconfig in the Edit VM form. |
| api/src/main/java/org/apache/cloudstack/api/ApiConstants.java | Adds the CLEAN_UP_EXTRA_CONFIG API constant. |
| api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java | Adds the cleanupextraconfig request parameter and accessor. |
| server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | Adds updateVmExtraConfig logic and wires cleanupextraconfig into updateVirtualMachine. |
| engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDetailsDao.java | Extends DAO contract with removeDetailsWithPrefix. |
| engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDetailsDaoImpl.java | Implements prefix-based detail deletion. |
| server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java | Adds unit tests for updateVmExtraConfig and config toggling helper. |
| engine/schema/src/test/java/com/cloud/vm/dao/VMInstanceDetailsDaoImplTest.java | Adds unit tests for removeDetailsWithPrefix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (values.extraconfig && values.extraconfig.length > 0) { | ||
| params.extraconfig = encodeURIComponent(values.extraconfig) |
There was a problem hiding this comment.
In the UI, whitespace-only values for extraconfig are treated as “present” (length > 0), so the request sends extraconfig instead of cleanupextraconfig. On the server side StringUtils.isNotBlank treats whitespace as blank, so this results in no update and the existing extraconfig is not cleaned. Consider trimming before the length check (or using a blank check) so clearing via whitespace triggers cleanup as expected.
| if (values.extraconfig && values.extraconfig.length > 0) { | |
| params.extraconfig = encodeURIComponent(values.extraconfig) | |
| const extraConfig = typeof values.extraconfig === 'string' ? values.extraconfig.trim() : values.extraconfig | |
| if (extraConfig && extraConfig.length > 0) { | |
| params.extraconfig = encodeURIComponent(extraConfig) |
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDetailsDaoImpl.java
Show resolved
Hide resolved
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16580 |
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16594 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15324)
|
sudo87
left a comment
There was a problem hiding this comment.
lgtm, tested it as well
Description
Fixes #9878
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
clear-vm-extraconfig.mp4
How did you try to break this feature and the system with this change?