[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[cloudsadhu]

Github user cloudsadhu commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r108862410
  
--- Diff: test/integration/smoke/test_deploy_vm_root_resize.py ---
@@ -114,36 +134,46 @@ def test_00_deploy_vm_root_resize(self):
 # 2. root disk has new size per listVolumes
 # 3. Rejects non-supported hypervisor types
 """
-if(self.hypervisor.lower() == 'kvm'):
-newrootsize = (self.template.size >> 30) + 2
-self.virtual_machine = VirtualMachine.create(
-self.apiclient,
-self.testdata["virtual_machine"],
-accountid=self.account.name,
-zoneid=self.zone.id,
-domainid=self.account.domainid,
-serviceofferingid=self.service_offering.id,
-templateid=self.template.id,
-rootdisksize=newrootsize
+
+
+newrootsize = (self.template.size >> 30) + 2
+if(self.hypervisor.lower() == 'kvm' or self.hypervisor.lower() ==
+'xenserver'or self.hypervisor.lower() == 'vmware'  ):
+
+if self.hypervisor=="vmware":
+self.virtual_machine = VirtualMachine.create(
+self.apiclient, self.services["virtual_machine"],
+zoneid=self.zone.id,
+accountid=self.account.name,
+domainid=self.domain.id,
+serviceofferingid=self.services_offering_vmware.id,
+templateid=self.template.id
+)
+
--- End diff --

sorry thats my  bad  will add the missing parameter and will post results.
if condition is added to address vm deployment in right  storage pool.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[serg38]

Github user serg38 commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r108580807
  
--- Diff: test/integration/smoke/test_deploy_vm_root_resize.py ---
@@ -114,36 +134,46 @@ def test_00_deploy_vm_root_resize(self):
 # 2. root disk has new size per listVolumes
 # 3. Rejects non-supported hypervisor types
 """
-if(self.hypervisor.lower() == 'kvm'):
-newrootsize = (self.template.size >> 30) + 2
-self.virtual_machine = VirtualMachine.create(
-self.apiclient,
-self.testdata["virtual_machine"],
-accountid=self.account.name,
-zoneid=self.zone.id,
-domainid=self.account.domainid,
-serviceofferingid=self.service_offering.id,
-templateid=self.template.id,
-rootdisksize=newrootsize
+
+
+newrootsize = (self.template.size >> 30) + 2
+if(self.hypervisor.lower() == 'kvm' or self.hypervisor.lower() ==
+'xenserver'or self.hypervisor.lower() == 'vmware'  ):
+
+if self.hypervisor=="vmware":
+self.virtual_machine = VirtualMachine.create(
+self.apiclient, self.services["virtual_machine"],
+zoneid=self.zone.id,
+accountid=self.account.name,
+domainid=self.domain.id,
+serviceofferingid=self.services_offering_vmware.id,
+templateid=self.template.id
+)
+
--- End diff --

B.O. tests are failing because for vmware you don't specify 
rootdisksize=newrootsize. You probably better to remove if-else entirely.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[cloudsadhu]

Github user cloudsadhu commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r108092328
  
--- Diff: test/integration/smoke/test_deploy_vm_root_resize.py ---
@@ -114,7 +114,13 @@ def test_00_deploy_vm_root_resize(self):
 # 2. root disk has new size per listVolumes
 # 3. Rejects non-supported hypervisor types
 """
-if(self.hypervisor.lower() == 'kvm'):
+full_clone_config = Configurations.list(self.apiclient,
+name="vmware.create.full.clone")[0].value
+if full_clone_config == 'false' and self.hypervisor.lower() == 
'vmware':
+self.skipTest("root disk resize is not supported "+
+"when vmware.create.full.clone is %s" % full_clone_config)
+
--- End diff --

I have incorporated the review comments (added full clone logic as well  
fine tuned the code) in test_deploy_vm_resize.py script.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[cloudsadhu]

Github user cloudsadhu commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r106085688
  
--- Diff: test/integration/smoke/test_deploy_vm_root_resize.py ---
@@ -114,7 +114,13 @@ def test_00_deploy_vm_root_resize(self):
 # 2. root disk has new size per listVolumes
 # 3. Rejects non-supported hypervisor types
 """
-if(self.hypervisor.lower() == 'kvm'):
+full_clone_config = Configurations.list(self.apiclient,
+name="vmware.create.full.clone")[0].value
+if full_clone_config == 'false' and self.hypervisor.lower() == 
'vmware':
+self.skipTest("root disk resize is not supported "+
+"when vmware.create.full.clone is %s" % full_clone_config)
+
--- End diff --

thanks for  your comment.Sure will add  the logic  to 
test_deploy_vm_resize.py as well . 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[serg38]

Github user serg38 commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r105932339
  
--- Diff: test/integration/smoke/test_deploy_vm_root_resize.py ---
@@ -114,7 +114,13 @@ def test_00_deploy_vm_root_resize(self):
 # 2. root disk has new size per listVolumes
 # 3. Rejects non-supported hypervisor types
 """
-if(self.hypervisor.lower() == 'kvm'):
+full_clone_config = Configurations.list(self.apiclient,
+name="vmware.create.full.clone")[0].value
+if full_clone_config == 'false' and self.hypervisor.lower() == 
'vmware':
+self.skipTest("root disk resize is not supported "+
+"when vmware.create.full.clone is %s" % full_clone_config)
+
--- End diff --

@priyankparihar @cloudsadhu test_rootvolume_resize.py test looks good. Can 
we add exactly the same logic of adding full clone config for smoke test 
test_deploy_vm_root_resize.py so that it is not skipped if a global setting is 
to use link clones?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[priyankparihar]

Github user priyankparihar commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r105532534
  
--- Diff: test/integration/component/test_rootvolume_resize.py ---
@@ -0,0 +1,1140 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+""" P1 tests for testing resize of  root volume functionality
+
+Test Plan: https://cwiki.apache.org/confluence/display/CLOUDSTACK/
+Root+Resize+Support
+
+Issue Link: https://issues.apache.org/jira/browse/CLOUDSTACK-9829
+"""
+# Import Local Modules
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase, unittest
+from marvin.lib.base import (Account,
+ ServiceOffering,
+ VirtualMachine,
+ Resources,
+ Domain,
+ Volume,
+ Snapshot,
+ Template,
+ VmSnapshot,
+ Host,
+ Configurations,
+ StoragePool)
+from marvin.lib.common import (get_domain,
+   get_zone,
+   get_template,
+   matchResourceCount,
+   list_snapshots,
+   list_hosts,
+   list_configurations,
+   list_storage_pools)
+from marvin.lib.utils import (cleanup_resources,
+  validateList)
+from marvin.codes import (PASS,
+  FAIL,
+  FAILED,
+  RESOURCE_PRIMARY_STORAGE,
+  INVALID_INPUT)
+from marvin.lib.utils import checkVolumeSize
+import time
+from marvin.sshClient import SshClient
+
+
+class TestResizeVolume(cloudstackTestCase):
+@classmethod
+def setUpClass(cls):
+cls.testClient = super(TestResizeVolume, cls).getClsTestClient()
+cls.api_client = cls.testClient.getApiClient()
+cls.hypervisor = (cls.testClient.getHypervisorInfo()).lower()
+cls.storageID = None
+# Fill services from the external config file
+cls.services = cls.testClient.getParsedTestDataConfig()
+# Get Zone, Domain and templates
+cls.domain = get_domain(cls.api_client)
+cls.zone = get_zone(
+cls.api_client,
+cls.testClient.getZoneForTests())
+cls.services["mode"] = cls.zone.networktype
+cls._cleanup = []
+cls.unsupportedStorageType = False
+cls.unsupportedHypervisorType = False
+cls.updateclone = False
+if cls.hypervisor not in ['xenserver',"kvm","vmware"]:
+cls.unsupportedHypervisorType=True
+return
+cls.template = get_template(
+cls.api_client,
+cls.zone.id
+)
+cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+cls.services["virtual_machine"]["template"] = cls.template.id
+cls.services["volume"]["zoneid"] = cls.zone.id
+try:
+cls.parent_domain = Domain.create(cls.api_client,
+  services=cls.services[
+  "domain"],
+  parentdomainid=cls.domain.id)
+cls.parentd_admin = Account.create(cls.api_client,
+   cls.services["account"],
+   admin=True,
+   
domainid=cls.parent_domain.id)
+cls._cleanup.append(cls.parentd_admin)
+

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[serg38]

Github user serg38 commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r105409367
  
--- Diff: test/integration/component/test_rootvolume_resize.py ---
@@ -0,0 +1,1140 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+""" P1 tests for testing resize of  root volume functionality
+
+Test Plan: https://cwiki.apache.org/confluence/display/CLOUDSTACK/
+Root+Resize+Support
+
+Issue Link: https://issues.apache.org/jira/browse/CLOUDSTACK-9829
+"""
+# Import Local Modules
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase, unittest
+from marvin.lib.base import (Account,
+ ServiceOffering,
+ VirtualMachine,
+ Resources,
+ Domain,
+ Volume,
+ Snapshot,
+ Template,
+ VmSnapshot,
+ Host,
+ Configurations,
+ StoragePool)
+from marvin.lib.common import (get_domain,
+   get_zone,
+   get_template,
+   matchResourceCount,
+   list_snapshots,
+   list_hosts,
+   list_configurations,
+   list_storage_pools)
+from marvin.lib.utils import (cleanup_resources,
+  validateList)
+from marvin.codes import (PASS,
+  FAIL,
+  FAILED,
+  RESOURCE_PRIMARY_STORAGE,
+  INVALID_INPUT)
+from marvin.lib.utils import checkVolumeSize
+import time
+from marvin.sshClient import SshClient
+
+
+class TestResizeVolume(cloudstackTestCase):
+@classmethod
+def setUpClass(cls):
+cls.testClient = super(TestResizeVolume, cls).getClsTestClient()
+cls.api_client = cls.testClient.getApiClient()
+cls.hypervisor = (cls.testClient.getHypervisorInfo()).lower()
+cls.storageID = None
+# Fill services from the external config file
+cls.services = cls.testClient.getParsedTestDataConfig()
+# Get Zone, Domain and templates
+cls.domain = get_domain(cls.api_client)
+cls.zone = get_zone(
+cls.api_client,
+cls.testClient.getZoneForTests())
+cls.services["mode"] = cls.zone.networktype
+cls._cleanup = []
+cls.unsupportedStorageType = False
+cls.unsupportedHypervisorType = False
+cls.updateclone = False
+if cls.hypervisor not in ['xenserver',"kvm","vmware"]:
+cls.unsupportedHypervisorType=True
+return
+cls.template = get_template(
+cls.api_client,
+cls.zone.id
+)
+cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+cls.services["virtual_machine"]["template"] = cls.template.id
+cls.services["volume"]["zoneid"] = cls.zone.id
+try:
+cls.parent_domain = Domain.create(cls.api_client,
+  services=cls.services[
+  "domain"],
+  parentdomainid=cls.domain.id)
+cls.parentd_admin = Account.create(cls.api_client,
+   cls.services["account"],
+   admin=True,
+   
domainid=cls.parent_domain.id)
+cls._cleanup.append(cls.parentd_admin)
+

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[priyankparihar]

Github user priyankparihar commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r103931610
  
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -3614,6 +3604,26 @@ public UserVmVO doInTransaction(TransactionStatus 
status) throws InsufficientCap
 });
 }
 
+public void validateRootDiskResize(final HypervisorType 
hypervisorType, Long rootDiskSize, VMTemplateVO templateVO, UserVmVO vm, final 
Map customParameters) throws InvalidParameterValueException
+{
+// rootdisksize must be larger than template.
+if ((rootDiskSize << 30) < templateVO.getSize()) {
+Long templateVOSizeGB = templateVO.getSize() / 1024 / 1024 / 
1024;
+s_logger.error("unsupported: rootdisksize override is smaller 
than template size " + templateVO.getSize() + "B (" + templateVOSizeGB + "GB)");
+throw new InvalidParameterValueException("unsupported: 
rootdisksize override is smaller than template size " + templateVO.getSize() + 
"B (" + templateVOSizeGB + "GB)");
+} else if ((rootDiskSize << 30) > templateVO.getSize()){
+if (hypervisorType == HypervisorType.VMware && 
!vm.getDetails().get("rootDiskController").toLowerCase().contains("scsi")) {
+s_logger.error("Found unsupported root disk controller : " 
+ vm.getDetails().get("rootDiskController"));
--- End diff --

 @borisstoyanov @serg38 Thanks for giving your valuable time. Code is 
modified. Now if everything looks ok to you then please provide your LGTM 
comment.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[serg38]

Github user serg38 commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r103719599
  
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -3614,6 +3604,26 @@ public UserVmVO doInTransaction(TransactionStatus 
status) throws InsufficientCap
 });
 }
 
+public void validateRootDiskResize(final HypervisorType 
hypervisorType, Long rootDiskSize, VMTemplateVO templateVO, UserVmVO vm, final 
Map customParameters) throws InvalidParameterValueException
+{
+// rootdisksize must be larger than template.
+if ((rootDiskSize << 30) < templateVO.getSize()) {
+Long templateVOSizeGB = templateVO.getSize() / 1024 / 1024 / 
1024;
+s_logger.error("unsupported: rootdisksize override is smaller 
than template size " + templateVO.getSize() + "B (" + templateVOSizeGB + "GB)");
+throw new InvalidParameterValueException("unsupported: 
rootdisksize override is smaller than template size " + templateVO.getSize() + 
"B (" + templateVOSizeGB + "GB)");
+} else if ((rootDiskSize << 30) > templateVO.getSize()){
+if (hypervisorType == HypervisorType.VMware && 
!vm.getDetails().get("rootDiskController").toLowerCase().contains("scsi")) {
+s_logger.error("Found unsupported root disk controller : " 
+ vm.getDetails().get("rootDiskController"));
--- End diff --

I think NPE on line 3615 is due to rootdiskcontroller might be not set for 
VM. If template is imported without explicitly specifying root disk controller 
than a global setting will be in effect.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[sureshanaparti]

Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r103409558
  
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -3520,27 +3520,17 @@ public UserVmVO doInTransaction(TransactionStatus 
status) throws InsufficientCap
 }
 rootDiskSize = 
Long.parseLong(customParameters.get("rootdisksize"));
 
-// only KVM supports rootdisksize override
-if (hypervisorType != HypervisorType.KVM) {
-throw new 
InvalidParameterValueException("Hypervisor " + hypervisorType + " does not 
support rootdisksize override");
+// only KVM, XenServer and VMware  supports 
rootdisksize override
+if (!(hypervisorType == HypervisorType.KVM || 
hypervisorType == HypervisorType.XenServer || hypervisorType == 
HypervisorType.VMware)) {
+throw new 
InvalidParameterValueException("Hypervisor " + hypervisorType + " does not 
support  rootdisksize override");
--- End diff --

@priyankparihar Ok. thanks for the confirmation


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[priyankparihar]

Github user priyankparihar commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r103154553
  
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -3520,27 +3520,17 @@ public UserVmVO doInTransaction(TransactionStatus 
status) throws InsufficientCap
 }
 rootDiskSize = 
Long.parseLong(customParameters.get("rootdisksize"));
 
-// only KVM supports rootdisksize override
-if (hypervisorType != HypervisorType.KVM) {
-throw new 
InvalidParameterValueException("Hypervisor " + hypervisorType + " does not 
support rootdisksize override");
+// only KVM, XenServer and VMware  supports 
rootdisksize override
+if (!(hypervisorType == HypervisorType.KVM || 
hypervisorType == HypervisorType.XenServer || hypervisorType == 
HypervisorType.VMware)) {
+throw new 
InvalidParameterValueException("Hypervisor " + hypervisorType + " does not 
support  rootdisksize override");
--- End diff --

@sureshanaparti both are serving different purpose. Yes, It is intentional. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[sureshanaparti]

Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r102651816
  
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -3520,27 +3520,17 @@ public UserVmVO doInTransaction(TransactionStatus 
status) throws InsufficientCap
 }
 rootDiskSize = 
Long.parseLong(customParameters.get("rootdisksize"));
 
-// only KVM supports rootdisksize override
-if (hypervisorType != HypervisorType.KVM) {
-throw new 
InvalidParameterValueException("Hypervisor " + hypervisorType + " does not 
support rootdisksize override");
+// only KVM, XenServer and VMware  supports 
rootdisksize override
+if (!(hypervisorType == HypervisorType.KVM || 
hypervisorType == HypervisorType.XenServer || hypervisorType == 
HypervisorType.VMware)) {
+throw new 
InvalidParameterValueException("Hypervisor " + hypervisorType + " does not 
support  rootdisksize override");
--- End diff --

The condition in VolumeApiServiceImpl.java at line# 1029 above and this 
doesn't match. You skipped None and Any. Is that intentional?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[sureshanaparti]

Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r102650751
  
--- Diff: 
plugins/hypervisors/xenserver/test/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixRequestWrapperTest.java
 ---
@@ -436,7 +436,7 @@ public void testResizeVolumeCommand() {
 final Answer answer = wrapper.execute(resizeCommand, 
citrixResourceBase);
 verify(citrixResourceBase, times(1)).getConnection();
 
-assertFalse(answer.getResult());
+//assertFalse(answer.getResult());
--- End diff --

@priyankparihar @anshul1886 Is this commented statement required for future 
reference? If not, better to remove this line.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[sureshanaparti]

Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r102650370
  
--- Diff: 
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
 ---
@@ -2073,6 +2089,43 @@ protected StartAnswer execute(StartCommand cmd) {
 }
 }
 
+private void resizeRootDisk(VirtualMachineMO vmMo, DiskTO rootDiskTO, 
VmwareHypervisorHost hyperHost, VmwareContext context) throws Exception
+{
+Pair vdisk = getVirtualDiskInfo(vmMo, 
rootDiskTO.getPath() + ".vmdk");
+assert(vdisk != null);
+
+Long reqSize=((VolumeObjectTO)rootDiskTO.getData()).getSize()/1024;
+VirtualDisk disk = vdisk.first();
+if(reqSize > disk.getCapacityInKB())
+{
+VirtualMachineDiskInfo diskInfo = 
getMatchingExistingDisk(vmMo.getDiskInfoBuilder(), rootDiskTO, hyperHost, 
context);
+assert (diskInfo != null);
+String[] diskChain = diskInfo.getDiskChain();
+
+if(diskChain!=null && diskChain.length>1)
+{
+s_logger.error("Unsupported Disk chain length "+ 
diskChain.length);
+throw new Exception("Unsupported Disk chain length "+ 
diskChain.length);
+}
+if(diskInfo.getDiskDeviceBusName()==null || 
!diskInfo.getDiskDeviceBusName().toLowerCase().contains("scsi"))
--- End diff --

startsWith("scsi") ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[sureshanaparti]

Github user sureshanaparti commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r102649977
  
--- Diff: 
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
 ---
@@ -703,6 +703,16 @@ private Answer execute(ResizeVolumeCommand cmd) {
 }
 
 VirtualDisk disk = vdisk.first();
+if(vdisk.second()!=null && 
!vdisk.second().toLowerCase().contains("scsi"))
--- End diff --

@priyankparihar _vdisk.second()_ here holds the disk bus name, that would 
ide0:0,... (or) scsi0:0,...
I think startsWith("scsi") check would be more precise.

Move "_VirtualDisk disk = vdisk.first();_" stmt below this if check. For 
all non-scsi disks, the _disk_ var is unnecessarily stacked.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[anshul1886]

Github user anshul1886 commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r101232184
  
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -3520,27 +3520,17 @@ public UserVmVO doInTransaction(TransactionStatus 
status) throws InsufficientCap
 }
 rootDiskSize = 
Long.parseLong(customParameters.get("rootdisksize"));
 
-// only KVM supports rootdisksize override
-if (hypervisorType != HypervisorType.KVM) {
--- End diff --

@pdube There was no resource layer code available for XS for this and root 
volume resize was only implemented for KVM.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[anshul1886]

Github user anshul1886 commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r101231917
  
--- Diff: 
plugins/hypervisors/xenserver/test/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixRequestWrapperTest.java
 ---
@@ -436,7 +436,7 @@ public void testResizeVolumeCommand() {
 final Answer answer = wrapper.execute(resizeCommand, 
citrixResourceBase);
 verify(citrixResourceBase, times(1)).getConnection();
 
-assertFalse(answer.getResult());
+//assertFalse(answer.getResult());
--- End diff --

@pdube That test is flawed as it always expects that command to fail. Till 
now it was working fine as it was always resizing which results in xapi command 
failure. But now since there is check for resize only in case of increase of 
volume size then the test is failing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[pdube]

Github user pdube commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r98259395
  
--- Diff: 
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixResizeVolumeCommandWrapper.java
 ---
@@ -48,6 +48,11 @@ public Answer execute(final ResizeVolumeCommand command, 
final CitrixResourceBas
 long newSize = command.getNewSize();
 
 try {
+
+if(command.getCurrentSize() <= newSize) {
+s_logger.info("No need to resize volume: " + volId +", 
current size " + command.getCurrentSize() + " is same as  new size " + newSize);
+return new ResizeVolumeAnswer(command, true, "success", 
newSize);
+}
--- End diff --

So you can't increase the size of a volume? This seems flawed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[pdube]

Github user pdube commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r98259959
  
--- Diff: server/src/com/cloud/vm/UserVmManagerImpl.java ---
@@ -3520,27 +3520,17 @@ public UserVmVO doInTransaction(TransactionStatus 
status) throws InsufficientCap
 }
 rootDiskSize = 
Long.parseLong(customParameters.get("rootdisksize"));
 
-// only KVM supports rootdisksize override
-if (hypervisorType != HypervisorType.KVM) {
--- End diff --

Why was XS blocked before?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[pdube]

Github user pdube commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1813#discussion_r98259432
  
--- Diff: 
plugins/hypervisors/xenserver/test/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixRequestWrapperTest.java
 ---
@@ -436,7 +436,7 @@ public void testResizeVolumeCommand() {
 final Answer answer = wrapper.execute(resizeCommand, 
citrixResourceBase);
 verify(citrixResourceBase, times(1)).getConnection();
 
-assertFalse(answer.getResult());
+//assertFalse(answer.getResult());
--- End diff --

Why comment this out?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1813: CLOUDSTACK-9604: Root disk resize support for...

[priyankparihar]

GitHub user priyankparihar opened a pull request:

https://github.com/apache/cloudstack/pull/1813

CLOUDSTACK-9604: Root disk resize support for VMware and XenServer.

For complete description please refer -> 
[CLOUDSTACK-9604](https://issues.apache.org/jira/browse/CLOUDSTACK-9604)

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/priyankparihar/cloudstack CLOUDSTACK-9604

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1813.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1813


commit 53382f5171aeb5c671007a89f7264438e3dca3e6
Author: Anshul And Priyank 
Date:   2016-12-02T16:16:49Z

CLOUDSTACK-9604: Root disk resize support for VMware and XenServer.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---