CLOUDSTACK-9422: Granular 'vmware.create.full.clone' as Primary Storage setting#1602
Merged
asfgit merged 1 commit intoapache:masterapache/cloudstack:masterfrom Sep 21, 2016
Merged
CLOUDSTACK-9422: Granular 'vmware.create.full.clone' as Primary Storage setting#1602asfgit merged 1 commit intoapache:masterapache/cloudstack:masterfrom
asfgit merged 1 commit intoapache:masterapache/cloudstack:masterfrom
Conversation
Contributor
|
@nvazquez is there a use case where you would want some VMWare hypervisors to not do a full clone? |
|
@syed. On Vmware using link cloned was a default behavior. The primary use cases are:
|
Contributor
Author
| vm.addDisk(disk); | ||
|
|
||
| // If hypervisor is vSphere, check for clone type setting. | ||
| if (vm.getHypervisorType().equals(HypervisorType.VMware)) { |
Contributor
There was a problem hiding this comment.
Does this need to be specific to VMWare? I can see a use case where this can be useful for XenServer as well. IIRC for XenServer, the volumes are linked.
Contributor
There was a problem hiding this comment.
So the idea is, instead of creating a clone which is linked to a parent, we would create a full clone of the disk. I can see this being useful when you don't want to have the overhead of nested reads/writes. Am I correct?
So there are reasons to use a linked clone as pointed out by @serg38 and there are reasons to use a full clone as well. The default I believe would be to use the linked clone. Correct?
Contributor
|
@syed. The default behavior won't change. The proposed enhancement will add an ability to control link/full clone deployment of a primary storage level. If it is not defined there a current value of a global setting will be applied. A global configuration setting vmware.create.full.clone is already in ACS starting from version 4.2 if I am not mistaken. |
Contributor
|
Thanks @serg38 my question is more along the lines of can this |
Contributor
Author
Contributor
|
Thanks guys. The code LGTM based on the code review. |
|
Packaging result: ✖centos6 ✖centos7 ✖debian repo: http://packages.shapeblue.com/cloudstack/pr/1602 |
Contributor
Author
|
I rebased master branch and pushed again, could it be ran again? @blueorangutan |
Contributor
|
@rhtyd can you re-kick this PR for @blueorangutan? |
| UserVmCloneType cloneType = UserVmCloneType.linked; | ||
| Boolean value = CapacityManager.VmwareCreateCloneFull.valueIn(vol.getPoolId()); | ||
| if (value != null) { | ||
| if (value.booleanValue() == true) |
Contributor
There was a problem hiding this comment.
Please collapse lines lines 1372 and 1373 into if (value != null && value). The compiler will autobox the Boolean instance -- removing the requirement to call booleanValue.
|
Packaging result: ✔centos6 ✔centos7 ✖debian repo: http://packages.shapeblue.com/cloudstack/pr/1602 |
Member
|
@blueorangutan package |
|
@rhtyd a Trillian-Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian repo: http://packages.shapeblue.com/cloudstack/pr/1602 |
Contributor
Author
|
Thanks @jburwell for your review! I refactored based on your comments. |
| return true; | ||
| } catch (Exception e){ | ||
| s_logger.error("Error while reconfiguring NFS version " + nfsVersion); | ||
| } catch (IllegalStateException e){ |
Contributor
There was a problem hiding this comment.
Per my previous comment, why are we throwing an exception on line 86 and catching it here? Why not log the error and return false in the default case? Using exceptions flow control is both expensive and a bad code smell.
b939dd3 to
9758905
Compare
|
@jburwell Are there any outstanding issues with this PR? It is waiting for second LGTM. |
9758905 to
76484b5
Compare
76484b5 to
c38e0ad
Compare
c38e0ad to
99e3b09
Compare
|
@karuturi @jburwell @rafaelweingartner Can you check if this PR can be merged? |
|
@karuturi @jburwell @rafaelweingartner Please disregard. We are still running tests for this PR |
Member
|
@serg38 ok. |
Contributor
|
Just realized I hadn't re-reviewed. LGTM for code. |
99e3b09 to
bb275a5
Compare
|
LGTM for the testing. Smoke test results (RHEL 6 management server, advanced networking, Vmware ESX 5.5 and 6.0 hypervisors) |
Contributor
Contributor
Author
|
Thanks for your help @jburwell ! |
Member
|
Based on tests, lgtms I'll proceed to merge this. |
asfgit
pushed a commit
that referenced
this pull request
Sep 21, 2016
CLOUDSTACK-9422: Granular 'vmware.create.full.clone' as Primary Storage setting### Introduction For VMware, It is possible to decide creating VMs as full clones on ESX HV, adjusting `vmware.create.full.clone` global setting. We would like to introduce this property as a primary storage detail, and use its value instead of global setting's value. We propose introducing `fullCloneFlag` on `PrimaryDataStoreTO` sent on `CopyCommand`. This way we can reconfigure `VmwareStorageProcessor` and `VmwareStorageSubsystemCommandHandler` similar as it was done for `nfsVersion` but refactoring it to be more general. * pr/1602: CLOUDSTACK-9422: Granular VMware vms creation as full clones on HV Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Contributor
Author
|
Great, thanks @rhtyd! |
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
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduction
For VMware, It is possible to decide creating VMs as full clones on ESX HV, adjusting
vmware.create.full.cloneglobal setting. We would like to introduce this property as a primary storage detail, and use its value instead of global setting's value.Proposed solution
We propose introducing
fullCloneFlagonPrimaryDataStoreTOsent onCopyCommand. This way we can reconfigureVmwareStorageProcessorandVmwareStorageSubsystemCommandHandlersimilar as it was done fornfsVersionbut refactoring it to be more general.