From f9b61bc737fbd51d8a55b3e94f404703bd3528e4 Mon Sep 17 00:00:00 2001 From: Anurag Awasthi <43956255+anuragaw@users.noreply.github.com> Date: Fri, 10 May 2019 23:40:38 +0530 Subject: [PATCH] orchestration: Allow VM that has never started to have volumes attached (#3276) With this patch b766bf7 we started tracking disks in attaching state so that other attach request can fail gracefully. However this missed the case where disks were in allocated state but attach was requested. For the use case where users want to attach disk in allocated state but not ready, we need to have allocated-attaching transition as well. We must take care of returning to the original state - allocated or ready - when attach request has completed. For the use case of unstarted vm's the disk must proceed as follows - "Allocated" -> Attaching -> Allocated. When VM is started, the disk is "created" and pool is assigned. For the use case of started VMs it's more trivial and disk proceeds as follows - Ready -> Attaching -> Ready. Test this by creating a VM with "startvm=false", create a disk and try attaching it in allocated state. It would give an exception on latest 4.11 but will be fixed on this patch. Signed-off-by: Rohit Yadav --- api/src/com/cloud/storage/Volume.java | 4 +- .../storage/volume/VolumeServiceImpl.java | 3 + .../cloud/storage/VolumeApiServiceImpl.java | 49 +++++----- test/integration/smoke/test_volumes.py | 94 +++++++++++++++---- 4 files changed, 108 insertions(+), 42 deletions(-) diff --git a/api/src/com/cloud/storage/Volume.java b/api/src/com/cloud/storage/Volume.java index 133a59df8a6..0e86ac01638 100644 --- a/api/src/com/cloud/storage/Volume.java +++ b/api/src/com/cloud/storage/Volume.java @@ -52,7 +52,7 @@ public interface Volume extends ControlledEntity, Identity, InternalIdentity, Ba UploadInProgress("Volume upload is in progress"), UploadError("Volume upload encountered some error"), UploadAbandoned("Volume upload is abandoned since the upload was never initiated within a specificed time"), - Attaching("The volume is attaching to a VM"); + Attaching("The volume is attaching to a VM from Ready state."); String _description; @@ -72,6 +72,8 @@ public interface Volume extends ControlledEntity, Identity, InternalIdentity, Ba static { s_fsm.addTransition(new StateMachine2.Transition(Allocated, Event.CreateRequested, Creating, null)); s_fsm.addTransition(new StateMachine2.Transition(Allocated, Event.DestroyRequested, Destroy, null)); + s_fsm.addTransition(new StateMachine2.Transition(Allocated, Event.OperationFailed, Allocated, null)); + s_fsm.addTransition(new StateMachine2.Transition(Allocated, Event.OperationSucceeded, Allocated, null)); s_fsm.addTransition(new StateMachine2.Transition(Creating, Event.OperationRetry, Creating, null)); s_fsm.addTransition(new StateMachine2.Transition(Creating, Event.OperationFailed, Allocated, null)); s_fsm.addTransition(new StateMachine2.Transition(Creating, Event.OperationSucceeded, Ready, null)); diff --git a/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java b/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java index d2979f7415d..4abb190dcf2 100644 --- a/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java +++ b/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java @@ -217,6 +217,9 @@ public class VolumeServiceImpl implements VolumeService { @Override public void revokeAccess(DataObject dataObject, Host host, DataStore dataStore) { DataStoreDriver dataStoreDriver = dataStore != null ? dataStore.getDriver() : null; + if (dataStoreDriver == null) { + return; + } if (dataStoreDriver instanceof PrimaryDataStoreDriver) { ((PrimaryDataStoreDriver)dataStoreDriver).revokeAccess(dataObject, host, dataStore); diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java index 372ab3423af..085c6995348 100644 --- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/com/cloud/storage/VolumeApiServiceImpl.java @@ -1888,6 +1888,10 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic } } + if (volumePool == null) { + sendCommand = false; + } + Answer answer = null; if (sendCommand) { @@ -1921,12 +1925,13 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic _volsDao.detachVolume(volume.getId()); // volume.getPoolId() should be null if the VM we are detaching the disk from has never been started before - DataStore dataStore = volume.getPoolId() != null ? dataStoreMgr.getDataStore(volume.getPoolId(), DataStoreRole.Primary) : null; - - volService.revokeAccess(volFactory.getVolume(volume.getId()), host, dataStore); - - handleTargetsForVMware(hostId, volumePool.getHostAddress(), volumePool.getPort(), volume.get_iScsiName()); - + if (volume.getPoolId() != null) { + DataStore dataStore = dataStoreMgr.getDataStore(volume.getPoolId(), DataStoreRole.Primary); + volService.revokeAccess(volFactory.getVolume(volume.getId()), host, dataStore); + } + if (volumePool != null && hostId != null) { + handleTargetsForVMware(hostId, volumePool.getHostAddress(), volumePool.getPort(), volume.get_iScsiName()); + } return _volsDao.findById(volumeId); } else { @@ -2634,24 +2639,25 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic return !storeForExistingStoreScope.isSameScope(storeForNewStoreScope); } - private synchronized void checkAndSetAttaching(Long volumeId, Long hostId) { + private synchronized void checkAndSetAttaching(Long volumeId) { VolumeInfo volumeToAttach = volFactory.getVolume(volumeId); if (volumeToAttach.isAttachedVM()) { throw new CloudRuntimeException("volume: " + volumeToAttach.getName() + " is already attached to a VM: " + volumeToAttach.getAttachedVmName()); } - if (volumeToAttach.getState().equals(Volume.State.Ready)) { - volumeToAttach.stateTransit(Volume.Event.AttachRequested); - } else { - String error = null; - if (hostId == null) { - error = "Please try attach operation after starting VM once"; - } else { - error = "Volume: " + volumeToAttach.getName() + " is in " + volumeToAttach.getState() + ". It should be in Ready state"; - } - s_logger.error(error); - throw new CloudRuntimeException(error); + + if (Volume.State.Allocated.equals(volumeToAttach.getState())) { + return; } + + if (Volume.State.Ready.equals(volumeToAttach.getState())) { + volumeToAttach.stateTransit(Volume.Event.AttachRequested); + return; + } + + final String error = "Volume: " + volumeToAttach.getName() + " is in " + volumeToAttach.getState() + ". It should be in Ready or Allocated state"; + s_logger.error(error); + throw new CloudRuntimeException(error); } private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, Long deviceId) { @@ -2684,7 +2690,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic // volumeToAttachStoragePool should be null if the VM we are attaching the disk to has never been started before DataStore dataStore = volumeToAttachStoragePool != null ? dataStoreMgr.getDataStore(volumeToAttachStoragePool.getId(), DataStoreRole.Primary) : null; - checkAndSetAttaching(volumeToAttach.getId(), hostId); + checkAndSetAttaching(volumeToAttach.getId()); boolean attached = false; try { @@ -2773,9 +2779,10 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic volumeToAttach = _volsDao.findById(volumeToAttach.getId()); - if (vm.getHypervisorType() == HypervisorType.KVM && volumeToAttachStoragePool.isManaged() && volumeToAttach.getPath() == null) { + if (vm.getHypervisorType() == HypervisorType.KVM && + volumeToAttachStoragePool != null && volumeToAttachStoragePool.isManaged() && + volumeToAttach.getPath() == null && volumeToAttach.get_iScsiName() != null) { volumeToAttach.setPath(volumeToAttach.get_iScsiName()); - _volsDao.update(volumeToAttach.getId(), volumeToAttach); } } diff --git a/test/integration/smoke/test_volumes.py b/test/integration/smoke/test_volumes.py index d40c0fd065f..c71411a25e6 100644 --- a/test/integration/smoke/test_volumes.py +++ b/test/integration/smoke/test_volumes.py @@ -398,32 +398,32 @@ class TestVolumes(cloudstackTestCase): # 3. disk should be attached to instance successfully self.debug( - "Attaching volume (ID: %s) to VM (ID: %s)" % ( - self.volume.id, - self.virtual_machine.id - )) + "Attaching volume (ID: %s) to VM (ID: %s)" % ( + self.volume.id, + self.virtual_machine.id + )) self.virtual_machine.attach_volume(self.apiClient, self.volume) self.attached = True list_volume_response = Volume.list( - self.apiClient, - id=self.volume.id - ) + self.apiClient, + id=self.volume.id + ) self.assertEqual( - isinstance(list_volume_response, list), - True, - "Check list response returns a valid list" - ) + isinstance(list_volume_response, list), + True, + "Check list response returns a valid list" + ) self.assertNotEqual( - list_volume_response, - None, - "Check if volume exists in ListVolumes" - ) + list_volume_response, + None, + "Check if volume exists in ListVolumes" + ) volume = list_volume_response[0] self.assertNotEqual( - volume.virtualmachineid, - None, - "Check if volume state (attached) is reflected" - ) + volume.virtualmachineid, + None, + "Check if volume state (attached) is reflected" + ) try: #Format the attached volume to a known fs format_volume_to_ext3(self.virtual_machine.get_ssh_client()) @@ -431,7 +431,7 @@ class TestVolumes(cloudstackTestCase): except Exception as e: self.fail("SSH failed for VM: %s - %s" % - (self.virtual_machine.ipaddress, e)) + (self.virtual_machine.ipaddress, e)) return @attr(tags = ["advanced", "advancedns", "smoke", "basic"], required_hardware="false") @@ -857,6 +857,60 @@ class TestVolumes(cloudstackTestCase): self.assertTrue(hasattr(root_volume, "podname")) self.assertEqual(root_volume.podname, list_pods.name) + @attr(tags = ["advanced", "advancedns", "smoke", "basic"], required_hardware="true") + def test_11_attach_volume_with_unstarted_vm(self): + """Attach a created Volume to a unstarted VM + """ + # Validate the following + # 1. Attach to a vm in startvm=false state works and vm can be started afterwards. + # 2. shows list of volumes + # 3. "Attach Disk" pop-up box will display with list of instances + # 4. disk should be attached to instance successfully + + test_vm = VirtualMachine.create( + self.apiclient, + self.services, + accountid=self.account.name, + domainid=self.account.domainid, + serviceofferingid=self.service_offering.id, + mode=self.services["mode"], + startvm=False + ) + + self.debug( + "Attaching volume (ID: %s) to VM (ID: %s)" % ( + self.volume.id, + test_vm.id + )) + test_vm.attach_volume(self.apiClient, self.volume) + test_vm.start(self.apiClient) + + list_volume_response = Volume.list( + self.apiClient, + id=self.volume.id + ) + self.assertEqual( + isinstance(list_volume_response, list), + True, + "Check list response returns a valid list" + ) + self.assertNotEqual( + list_volume_response, + None, + "Check if volume exists in ListVolumes" + ) + volume = list_volume_response[0] + self.assertNotEqual( + volume.virtualmachineid, + None, + "Check if volume state (attached) is reflected" + ) + + test_vm.detach_volume(self.apiClient, self.volume) + self.cleanup.append(test_vm) + + return + def wait_for_attributes_and_return_root_vol(self): def checkVolumeResponse(): list_volume_response = Volume.list(