Skip to content

Conversation

@winterhazel
Copy link
Member

Description

In the deploy VM wizard, the settings inherited from a template (iothreads, iothreads, io.policy, keyboard, UEFI and dynamically scalable) are not automatically inferred after selecting the template. This would not matter for most of these settings if the user did not change the fields, since they would still be inherited when the parameters were null; however, the boot type and mode would always default to BIOS and LEGACY, even when set to something else in the template.

This PR aims to address this issue by making the deploy VM wizard automatically infer the settings inherited from a template.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

In the deploy VM wizard:

  1. I selected a template with dynamic scaling enabled and the settings keyboard=jp, io.policy=io_uring, UEFI=LEGACY and iothreads=enabled. Then, I verified that the fields were automatically inferred and, after deploying, that the expected parameters were present in the API call.
  2. I selected the same template as in item 1, changed the value in the fields and verified that the parameters also changed in the API call.
  3. I selected a template that had dynamic scaling disabled and no settings. Then, I verified that the fields were not automatically inferred (except dynamic scaling) and, after deploying, that the corresponding parameters were not in the API call (with the exception of dynamicscalingenabled=false).
  4. I selected the template from item 1 and then selected the template from item 3. Finally, I verified that the fields in the UI and the parameters in the API call had the same values as in item 3.

@weizhouapache weizhouapache changed the title Infer template settings in the deploy VM wizard UI: Infer template settings in the deploy VM wizard Aug 14, 2023
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

code looks good, will test

@DaanHoogland
Copy link
Contributor

@blueorangutan ui

@DaanHoogland DaanHoogland self-assigned this Aug 15, 2023
@blueorangutan
Copy link

@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/7867 (QA-JID-142)

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #7867 (b8fa506) into 4.18 (a47a4f4) will increase coverage by 0.03%.
Report is 25 commits behind head on 4.18.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               4.18    #7867      +/-   ##
============================================
+ Coverage     13.02%   13.05%   +0.03%     
- Complexity     9040     9083      +43     
============================================
  Files          2720     2720              
  Lines        257094   257370     +276     
  Branches      40092    40124      +32     
============================================
+ Hits          33491    33612     +121     
- Misses       219398   219537     +139     
- Partials       4205     4221      +16     

see 21 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Aug 16, 2023

verified, but only the UI part as no API change is done

@yadvr
Copy link
Member

yadvr commented Aug 22, 2023

I always thought, the backend would infer the template specific details/settings ?

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

@DaanHoogland
Copy link
Contributor

@winterhazel , will you apply @Pearl1594 's suggestion?

@winterhazel
Copy link
Member Author

winterhazel commented Aug 23, 2023

I always thought, the backend would infer the template specific details/settings ?

@rohityadavcloud
The backend infers the setings when the parameters in the API call are null.

The problem is that the deploy VM wizard always defaults the boot type and mode to BIOS and LEGACY, even if they are set to something else in the template. For that reason, these two settings are not being inherited when deploying a VM through the UI.

@winterhazel , will you apply @Pearl1594 's suggestion?

@Pearl1594 @DaanHoogland
I have checked how the backend infers this setting and I think that it is better to change this line to this.form.iothreadsenabled = template.details && Object.prototype.hasOwnProperty.call(template.details, 'iothreads') to match its behavior. The backend considers iothreads as enabled when the VM details contain the key "iothreads", independently of its value.

With the current changes, if "iothreads" is set to "example", the UI will set the parameter to false. With @Pearl1594 's suggestion, the UI will not infer it, but the backend will infer it as enabled since the parameter in the API call was null.

@DaanHoogland DaanHoogland merged commit 57d4d0d into apache:4.18 Aug 25, 2023
DaanHoogland added a commit that referenced this pull request Aug 25, 2023
* 4.18:
  UI: Infer template settings in the deploy VM wizard (#7867)
@DaanHoogland DaanHoogland removed their assignment Nov 16, 2023
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.

6 participants