-
Notifications
You must be signed in to change notification settings - Fork 1.3k
api,server: allow cleaning up vm extraconfig #11974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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. |
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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?