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(