Skip to content

Conversation

abh1sar
Copy link
Collaborator

@abh1sar abh1sar commented Sep 25, 2025

Description

This PR fixes #7849

Also fixes a bug in kvm extraconfig #11718

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

Screenshot 2025-09-25 at 2 44 25 PM Screenshot 2025-09-25 at 2 45 04 PM ### How Has This Been Tested?

How did you try to break this feature and the system with this change?

@abh1sar abh1sar changed the title Econfig UI support for extraconfig in deploy and update instance Sep 25, 2025
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 11.11111% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.17%. Comparing base (36cfd76) to head (3d9d2ba).
⚠️ Report is 4 commits behind head on 4.20.

Files with missing lines Patch % Lines
.../cloudstack/api/response/CapabilitiesResponse.java 0.00% 3 Missing ⚠️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java 0.00% 3 Missing ⚠️
...k/api/command/user/config/ListCapabilitiesCmd.java 0.00% 1 Missing ⚠️
...in/java/com/cloud/server/ManagementServerImpl.java 0.00% 1 Missing ⚠️
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     
Flag Coverage Δ
uitests 4.00% <ø> (-0.01%) ⬇️
unittests 17.02% <11.11%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@weizhouapache
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@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.

Copy link
Member

@weizhouapache weizhouapache left a 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

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 15166

@abh1sar
Copy link
Collaborator Author

abh1sar commented Sep 25, 2025

@blueorangutan package

@blueorangutan
Copy link

@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.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15170

@weizhouapache
Copy link
Member

@blueorangutan test

@blueorangutan
Copy link

@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

Copy link
Contributor

@shwstppr shwstppr left a 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")
Copy link
Contributor

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

opts: []
}
},
extraConfigEnabled: this.$store.getters.features.additionalconfigenabled
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

@blueorangutan
Copy link

[SF] Trillian test result (tid-14442)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 54817 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11719-t14442-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

Copy link
Contributor

@shwstppr shwstppr left a 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.

@abh1sar

@abh1sar
Copy link
Collaborator Author

abh1sar commented Sep 26, 2025

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.

I'll add that @shwstppr

  • Currently, existing extraconfig value is overridden so it would be better if UpdateVM form shows the existing value pre-filled.

This can be done but is a bit tricky as multiple extraconfigs are stored and shown as separate keys. e.g,

    "details": {                                     
      "Message.ReservedCapacityFreed.Flag": "false",
      "cpuOvercommitRatio": "2.0",
      "extraconfig-1": "<memoryBacking>\n    <locked/>\n</memoryBacking>",
      "extraconfig-2": "<memoryBacking>\n    <nosharepages/>\n</memoryBacking>"
    },

I would have to combine these into a single string in UI

<memoryBacking>
    <locked/>
</memoryBacking>

<memoryBacking>
    <nosharepages/>
</memoryBacking>

One more thing is that the separator is different in VMware/XenServer and KVM
Vmware/Xen : Single new line (String[] extraConfigs = decodedUrl.split("\\r?\\n");
KVM : Double new lines ( String[] extraConfigs = decodedUrl.split("\n\n");

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 split.("\\r?\\n+)". That will take care of both cases

This would need testing on all 3 hypervisors

@abh1sar
Copy link
Collaborator Author

abh1sar commented Sep 29, 2025

@blueorangutan package

@abh1sar
Copy link
Collaborator Author

abh1sar commented Sep 29, 2025

  • 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.

@shwstppr the form already contains a warning at the top

Screenshot 2025-09-29 at 5 16 51 PM

@abh1sar abh1sar requested a review from shwstppr September 29, 2025 11:48
@blueorangutan
Copy link

@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.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15218

@weizhouapache
Copy link
Member

@blueorangutan test

Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

Copy link
Contributor

@shwstppr shwstppr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

LGTM. Existing config shown in the Update VM form. Tested with a KVM env

@weizhouapache
Copy link
Member

thanks @shwstppr for the testing

merging

@weizhouapache weizhouapache merged commit 70af55e into apache:4.20 Sep 30, 2025
41 of 43 checks passed
@abh1sar abh1sar deleted the econfig branch September 30, 2025 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple extra config values cannot be set on KVM UI: deploy and update vm instances with extraconfig

4 participants