-
Notifications
You must be signed in to change notification settings - Fork 1.2k
UI support for extraconfig in deploy and update instance #11719
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11719 +/- ##
============================================
- Coverage 16.17% 16.17% -0.01%
- Complexity 13296 13297 +1
============================================
Files 5656 5656
Lines 498219 498240 +21
Branches 60451 60457 +6
============================================
+ Hits 80579 80580 +1
- Misses 408672 408691 +19
- Partials 8968 8969 +1
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 |
@weizhouapache 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. |
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.
code lgtm
added to 4.20.2 milestone
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 15166 |
@blueorangutan package |
@abh1sar 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 15170 |
@blueorangutan test |
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
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.
LGTM - haven't tested
private Boolean dynamicScalingEnabled; | ||
|
||
@SerializedName(ApiConstants.ADDITONAL_CONFIG_ENABLED) | ||
@Param(description = "true if additional configurations or extraconfig can be passed to Instances", since = "4.20.2") |
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.
Not related to this PR but as some point we should start using a generic Map response or some other means. Adding a new response param for a new global setting value is an unnecessary hassle
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.
+1
ui/src/views/compute/EditVM.vue
Outdated
opts: [] | ||
} | ||
}, | ||
extraConfigEnabled: this.$store.getters.features.additionalconfigenabled |
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.
curious - why a computed variable in DeployVM and a data variable here?
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.
no specific reason. It could have been a data variable in DeployVM as well. Just followed the existing pattern in the file.
[SF] Trillian test result (tid-14442)
|
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.
Tested 4.20.2 env with changes. I could see extraconfig being processed by the host,
2025-09-26 09:15:58,523 DEBUG [kvm.resource.LibvirtComputingResource] (AgentRequest-Handler-1:[]) (logid:) Appending extra configuration data [{extraconfig-1=abc=444, extraconfig-2=xyz=777}] to guest VM [VM {id: "23", name: "i-2-23-VM", uuid: "f6c36574-f999-4ea2-b9b0-3474067b0c0f", type: "User"}] domain XML.
For update VM,
- Maybe we should highlight that VM needs to be stop-started or allow extraconfig only for stopped VM.
a-alert
can be used to show an appropriate message. - Currently, existing extraconfig value is overridden so it would be better if UpdateVM form shows the existing value pre-filled.
I'll add that @shwstppr
This can be done but is a bit tricky as multiple extraconfigs are stored and shown as separate keys. e.g,
I would have to combine these into a single string in UI
One more thing is that the separator is different in VMware/XenServer and KVM I am thinking that for displaying, I can insert two double lines between extra config by default. And modify the VMware/Xen separator to handle multiple newlines This would need testing on all 3 hypervisors |
@blueorangutan package |
@shwstppr the form already contains a warning at the top ![]() |
@abh1sar 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 15218 |
@blueorangutan test |
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.
code lgtm
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.
thanks @shwstppr for the testing merging |
Description
This PR fixes #7849
Also fixes a bug in kvm extraconfig #11718
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How did you try to break this feature and the system with this change?