-
Notifications
You must be signed in to change notification settings - Fork 1.3k
VM dynamic scaling option granularity #4643
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
|
@blueorangutan package |
api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java
Outdated
Show resolved
Hide resolved
e49f963 to
efb6850
Compare
|
@blueorangutan package |
|
@blueorangutan test matrix |
|
These failed test cases are commonly seen on all PRs, not related to this PR changes. test_01_scale_vm on XenServer is failing with License error |
utchoang
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.
UI Code LGTM!
...src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java
Show resolved
Hide resolved
|
|
||
| @Column(name = "dynamic_scaling_enabled") | ||
| private boolean dynamicScalingEnabled; | ||
|
|
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.
@harikrishna-patnala what's the difference between "dynamic_scaling_enabled" vs "is_dynamic" field?
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.
is_dynamic is related to custom compute offerings and thats a transient parameter.
| getCurrentTimeStampString(), | ||
| "autoScaleVm-" + asGroup.getId() + "-" + getCurrentTimeStampString(), null, null, null, HypervisorType.XenServer, HTTPMethod.GET, null, null, null, | ||
| null, true, null, null, null, null, null, null, null); | ||
| null, true, null, null, null, null, null, null, null, true); |
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.
@harikrishna-patnala is there a reason that we've set it to true as opposed to find the value from the service offering?
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 parameter value in deploy VM flow is the one that read from deploy VM API, where as in other cases there is no user/admin option to specify whether the VM can be dynamically scalable or not, so it is defaulted to true. All these method calls will reach checkIfDynamicScalingCanBeEnabled() where other dynamic scaling flags from template, offering and global setting are considered.
| owner, "autoScaleVm-" + asGroup.getId() + "-" + getCurrentTimeStampString(), | ||
| "autoScaleVm-" + asGroup.getId() + "-" + getCurrentTimeStampString(), null, null, null, HypervisorType.XenServer, HTTPMethod.GET, null, null, | ||
| null, null, true, null, null, null, null, null, null, null); | ||
| null, null, true, null, null, null, null, null, null, null, true); |
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.
same as above
| vm = _userVmService.createAdvancedVirtualMachine(zone, serviceOffering, template, null, owner, "autoScaleVm-" + asGroup.getId() + "-" + | ||
| getCurrentTimeStampString(), "autoScaleVm-" + asGroup.getId() + "-" + getCurrentTimeStampString(), | ||
| null, null, null, HypervisorType.XenServer, HTTPMethod.GET, null, null, null, addrs, true, null, null, null, null, null, null, null); | ||
| null, null, null, HypervisorType.XenServer, HTTPMethod.GET, null, null, null, addrs, true, null, null, null, null, null, null, null, true); |
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.
same as above
nvazquez
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.
Nice feature, I left some comments
| } | ||
|
|
||
| public Boolean getDynamicScalingEnabled() { | ||
| return isDynamicScalingEnabled == null ? Boolean.TRUE : isDynamicScalingEnabled; |
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.
As the null check is in place, what about returning boolean instead of Boolean on this function? Also, this will mean that all the service offerings registered before 4.16 will now become dynamically scallable as well?
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.
fixed all Boolean to boolean, thanks for that.
Yes all old service offerings prior to upgrade will be marked as dynamically scalable. This is made not to break existing VMs because service offering was not part of dynamic scaling decision before. After upgrade we need to bypass that check for old VMs, which can be achieved by marking them true.
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.
Copy, thanks
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.
@harikrishna-patnala and @nvazquez this method is Boolean was it meant to become boolean?
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java
Outdated
Show resolved
Hide resolved
engine/schema/src/main/java/org/apache/cloudstack/engine/cloud/entity/api/db/VMEntityVO.java
Outdated
Show resolved
Hide resolved
|
@sureshanaparti @Pearl1594 @nvazquez I've addressed your comments and updated the code as per the suggestions. Please review. |
|
@blueorangutan package |
|
@harikrishna-patnala a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
...src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java
Show resolved
Hide resolved
|
@harikrishna-patnala a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 110 |
|
@blueorangutan test matrix |
|
@blueorangutan test matrix |
|
@DaanHoogland a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests |
|
Trillian test result (tid-788)
|
|
LGTM |
|
@blueorangutan test centos7 vmware-67u3 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests |
|
@blueorangutan test centos7 xenserver-71 |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + xenserver-71) has been kicked to run smoke tests |
|
Trillian Build Failed (tid-815) |
|
Trillian Build Failed (tid-814) |
|
Trillian test result (tid-858)
|
|
Trillian test result (tid-859)
|
Description
This PR introduces new granularity levels to configure VM dynamic scalability. Previously VM is configured to be dynamically scalable based on the template and global setting. Now we bringing this option to configure at service offering and VM level also.
VM can dynamically scale only when all flags are ON at VM level, template, service offering and global setting. If any of the flags is set to false then VM cannot be scalable. This result will be persisted in DB for each VM and will be honoured for that VM till it is updated.
We are introducing 'dynamicscalingallowed' parameter with permitted values of true or false for deployVM API and createServiceOffering API.
Following are the API parameter changes:
createServiceOffering API:
dynamicscalingenabled: an optional parameter of type Boolean with default value “true”.
deployVirtualMachine API:
dynamicscalingenabled: an optional parameter of type Boolean with default value “true”.
Following are the UI changes:
Service offering creation has ON/OFF switch for dynamic scaling enabled with default value true.
Deploy VM wizard has ON/OFF switch for dynamic scaling enabled with default value true in Advanced Mode section.
Added a documentation PR to support these changes
apache/cloudstack-documentation#186
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Following are the test cases that I've covered as part of my testing on VMware environment.
VM Creation:
- VM1 (scalable)
- VM2 (not scalable)
- VM3 (not scalable)
- VM4 (not scalable)
- VM5 (not scalable)
VM Update:
- VM is updated to Dynamic Scaling Enabled value false
- VM update operation fails because VM cannot be scaled with any of the template/service offering/global setting dynamic scaling enabled flag with false
VM dynamic Scaling:
While trying to scale VM from UI, it will only list service offering that are suitable to scale and also offering with "Dynamic Scaling Enabled" value same as old service offering.
- SO1
- SO2
- Successful
- Operation failed because, VM1 Service Offering is marked Dynamic Scaling Enabled but not on SO2