FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Allow to configure root disk size via Service Offering (diskoffering of type Service). by GabrielBrascher · Pull Request #4341 · apache/cloudstack · GitHub

Repository navigation

Allow to configure root disk size via Service Offering (diskoffering of type Service). - #4341

Merged
DaanHoogland merged 1 commit into
apache:masterfrom
CLDIN:resize-root-service-offering
Oct 30, 2020
Merged

DaanHoogland merged 1 commit into
apache:masterfrom
CLDIN:resize-root-service-offering

Conversation

GabrielBrascher commented Sep 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Service Offering allows ADMINs to set multiple parameters such as IOPS, memory, and CPU; however, we still are not able to configure the Root Disk size of a VM via service Offering.

This PR adds control over the Root disk size and therefore helps providers that want to enforce the root disk size in the same way that it is done with compute offering.

How does it work?

  • If the Service Offering is created without setting root disk size, all operations over root volume size are done as currently.
  • If a Service Offering is created with a root disk size, then there are some (intentionally) limitations under the root disk size resizing, as the following ones:
1. Users cannot deploy VMs with custom root disk size when using such offerings
2. Users cannot resize the VM root disk size when using such offerings
3. the Root Volume of such VMs can only be resized when changing to another Service Offering with a Root disk size equals or larger than the current one.
4. Users can change the VM offering to a service offering with a Root size of 0GB (default), and then customize the volume size.

To easily view all possible service offering resizing, the following table shows if such offering migration is supported:

# Service Offering Root size new Service Offering Root Does support offering resize?
1 0GB (default) Any YES
2 5GB 5GB YES
3 5GB 10GB YES
4 10GB 5GB NO
5 Any 0GB YES

How users can resize the root volume if the VM was deployed with a Root disk size configured via its Service offering?

  • Users/Admins can at any time change to an offering with the default root disk size of 0; on such case, then it would be possible to resize as it is done nowadays.

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)

Screenshots (if appropriate):

Setting root disk size for service offering via UI

How Has This Been Tested?

Tests:
Created the following service offerings (via API and also via UI):

  • 0GB - default behavior with no Root Disk Size configured
  • 5GB - 5GB Root
  • 10GB - 10GB Root
  • 15GB - 15GB Root

Executed following tests

1. Test root volume resize.
Expected result: not allowed to resize
Steps:
a - deploy a VM with any of the root size configured offerings (e.g. 5GB)
b - Change root volume size
c - not allowed

2. Test resize via service offering.
Expected result: allow 5GB -> 10GB, and 10 GB -> 15, fail to "dowsize" from 10GB -> 5GB
Steps:
a - deploy VM with offering 5GB
b - Stop VM
c - Change Service offering to 10GB
d - Change offering back to 5GB, fails (as expected)
e - Change offering to 15GB

3. Test resize via offering to a Service offering with the default root size (0 GB) and then customize the volume
Expected result: allow 5GB -> 0GB, successfully change the root volume to a custom size
Steps:
a - deploy VM with offering 5GB
b - Stop VM
c - Change Service offering to 0GB
d - resize volume to custom size, e.g 50 GB

GabrielBrascher added this to the 4.15.0.0 milestone Sep 23, 2020
GabrielBrascher self-assigned this Sep 23, 2020

Copy link
Copy Markdown
Member Author

@blueorangutan package

Copy link
Copy Markdown

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Copy link
Copy Markdown

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2074

Copy link
Copy Markdown
Contributor

@blueorangutan package

apache deleted a comment from blueorangutan Oct 21, 2020
apache deleted a comment from blueorangutan Oct 21, 2020
apache deleted a comment from blueorangutan Oct 21, 2020
apache deleted a comment from blueorangutan Oct 21, 2020

Copy link
Copy Markdown

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

Copy link
Copy Markdown

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2246

Copy link
Copy Markdown
Member Author

Copy link
Copy Markdown
Contributor

Code LGTM
But noticed a behavioral difference between the Legacy UI and Primate @GabrielBrascher. Seems like Primate calls changeServiceForVirtualMachine when the VM is stopped and an attempt is made to scale - as a result of which most observations expected aren't seen i.e., on scaling up to an offering with a rootdisksize > current root disk size, though the operation is successful, there is no change in the disksize parameter of listVolumesMetrics. Also, scaling it down (to a service offering with lower rootdisksize value than the current size) works via primate for the same reason.
All mentioned cases work fine via legacy UI.

@Parameter(name = ApiConstants.SERVICE_OFFERING_DETAILS, type = CommandType.MAP, description = "details for planner, used to store specific parameters")
private Map details;

@Parameter(name = ApiConstants.ROOT_DISK_SIZE, type = CommandType.LONG, required = false, description = "the Root disk size in GB.")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do we need since parameter here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sounds like a good place to add since ;)
Thanks for the heads up @ravening!

throw new InvalidParameterValueException(String.format("The Root disk size is of %s GB but it must be greater than 0.", rootDiskSizeInGiB));
} else if (rootDiskSizeInGiB != null) {
long rootDiskSizeInBytes = rootDiskSizeInGiB * GiB_TO_BYTES;
offering.setDiskSize(rootDiskSizeInBytes);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

An optimization suggestion here. You can write below one liner since rootDiskSizeInBytes is not used anywhere else

offering.setDiskSize(rootDiskSizeInGiB * GiB_TO_BYTES)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@ravening that does not make any difference. The JVM feeds its bytecodes to the Just-In-Time compiler (JIT) which will already take care of doing such optimization (among many others).

I did that way because of 2 reasons, one tecnical and the other a personal option: (i) in case of remote debugging connected via JVM it helps on changing a variable value in runtime; (ii) I prefer that way for code readability.

}

private void checkIfVolumeIsRootAndVmIsRunning(Long newSize, VolumeVO volume, VMInstanceVO vmInstanceVO) {
if (!volume.getSize().equals(newSize) && volume.getVolumeType().equals(Volume.Type.ROOT) && !State.Stopped.equals(vmInstanceVO.getState())) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Any specific reason why we are not allowing resizing when vm is running?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@ravening good question.

This was done to avoid users thinking that the service offering was updated correctly. Unfortunatelly, it is necessary to Stop/Start the VM, otherwise IOps updates would not take effect, for example. Considering that this is a service offering update command, then we need to take such situations into account.

Additionally, not all operation systems support dynamic memory scaling.

Copy link
Copy Markdown
Member Author

@Pearl1594 thanks for bringing that up. I have noticed a few issues when updating disk offering details via changeServiceForVirtualMachine. I created issue #4125 regarding read/write rates.

Currently CloudStack UI uses scaleVirtualMachine and therefore we are not noticing these issues.

Copy link
Copy Markdown
Contributor

So @GabrielBrascher would it make sense to deprecate : changeServiceForVirtualMachine - or probably always call scaleVirtualMachine API in Primate - just as we currently do in the Legacy CloudStack UI? as scaleVirtualMachine handles it both for stopped and running VMs

Copy link
Copy Markdown
Contributor

@blueorangutan test

apache deleted a comment from blueorangutan Oct 22, 2020

Copy link
Copy Markdown

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

Copy link
Copy Markdown
Member Author

@Pearl1594 that might make sense indeed. There is also this issue: #3110. On both cases scaling works but the change command does not.

Copy link
Copy Markdown

Trillian test result (tid-3047)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38163 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4341-t3047-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 84 look OK, 1 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure 308.89 test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Failure 304.84 test_vpc_redundant.py

Copy link
Copy Markdown
Member Author

A couple of conflicts emerged, I will sort them out later today.

Copy link
Copy Markdown
Member Author

@blueorangutan package

Copy link
Copy Markdown

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2287

Copy link
Copy Markdown
Contributor

@pearl @ravening @GabrielBrascher is this good to go (after smoke tests) or should we move it to 4.16?

Copy link
Copy Markdown
Contributor

@DaanHoogland functionality wise LGTM. However, it wouldn't work entirely as expected with Primate because of the invocation of the changeServiceForVirtualMachine API for a stopped VM as opposed to scaleVirtualMachine.
A PR has been raised to address that, if that goes in - this is good to go.

Copy link
Copy Markdown
Contributor

@blueorangutan test

Service Offering can then have a root disk size configured. On such a case the Root Volume can only be resized when changing to another Service Offering with a Root disk size equals or larger than the current one.
GabrielBrascher force-pushed the resize-root-service-offering branch from 3324b0c to 4f6f7f0 Compare October 29, 2020 13:06

Copy link
Copy Markdown
Member Author

@blueorangutan package

Copy link
Copy Markdown

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Copy link
Copy Markdown
Member Author

Synced against master (again), travis is passing now.

Copy link
Copy Markdown

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2309

Copy link
Copy Markdown
Contributor

@blueorangutan test

Copy link
Copy Markdown

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

Copy link
Copy Markdown

Trillian test result (tid-3106)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30973 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4341-t3106-kvm-centos7.zip
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

_itMgr.checkIfCanUpgrade(vmInstance, newServiceOffering);

DiskOfferingVO newROOTDiskOffering = _diskOfferingDao.findById(newServiceOffering.getId());
// Check if the new service offering can be applied to vm instance

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this comment could be a method name instead canServiceOfferingBeAppliedToInstance(...)

Copy link
Copy Markdown
Contributor

@PaulAngus this feature has been long waiting and meets merge requirements. leave it with the RM.

wido commented Oct 30, 2020

Copy link
Copy Markdown
Contributor

@PaulAngus this feature has been long waiting and meets merge requirements. leave it with the RM.

Yes! Especially in a public cloud (Like PCextreme's) this is very useful. Why? It's not always a good thing that end-users (API users) can specify the root disk size. This way they can create a VM with 1GB of memory and a 10TB disk. This can cause problems like small DoS attacks on hypervisors or resources going out of balance.

The provider should be able to dictate the exact ServiceOfferings and DiskOfferings users can deploy so that billing can also be done properly and clearly.

Copy link
Copy Markdown
Contributor

@DaanHoogland as mentioned earlier that Primate needed some changes to make it work as the legacy UI - apache/cloudstack-primate#818 addresses that concern. With that having merged, this is good to go. Thanks

Copy link
Copy Markdown
Contributor

tnx @Pearl1594 . @PaulAngus only 300 lines so I'll merge

DaanHoogland merged commit b3a1cb4 into apache:master Oct 30, 2020
Pearl1594 pushed a commit to shapeblue/cloudstack that referenced this pull request Nov 4, 2020
Pearl1594 pushed a commit to shapeblue/cloudstack that referenced this pull request Nov 4, 2020
wido mentioned this pull request Apr 7, 2021
2 of 7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

7 participants


Back | FazBrowse Home | New Git URL