diff --git a/api/src/com/cloud/storage/Volume.java b/api/src/com/cloud/storage/Volume.java index 133a59df8a6..b84622f5c53 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. Attach requests when in allocated state do not transit to this state."); String _description; @@ -120,8 +120,8 @@ public interface Volume extends ControlledEntity, Identity, InternalIdentity, Ba s_fsm.addTransition(new StateMachine2.Transition(UploadError, Event.DestroyRequested, Destroy, null)); s_fsm.addTransition(new StateMachine2.Transition(UploadAbandoned, Event.DestroyRequested, Destroy, null)); s_fsm.addTransition(new StateMachine2.Transition(Ready, Event.AttachRequested, Attaching, null)); - s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.OperationSucceeded, Ready, null)); s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.OperationFailed, Ready, null)); + s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.OperationSucceeded, Ready, null)); } } diff --git a/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java b/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java index 4d2452d45ce..9a6e19b740a 100644 --- a/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java +++ b/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java @@ -23,7 +23,6 @@ import java.util.Set; import javax.inject.Inject; -import com.cloud.storage.StoragePool; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.engine.subsystem.api.storage.EndPoint; @@ -59,6 +58,7 @@ import com.cloud.offering.NetworkOffering; import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.storage.DataStoreRole; import com.cloud.storage.Storage; +import com.cloud.storage.StoragePool; import com.cloud.storage.Volume; import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.GuestOSCategoryDao; diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java index 372ab3423af..33a5fa75f72 100644 --- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/com/cloud/storage/VolumeApiServiceImpl.java @@ -1920,12 +1920,13 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic // Mark the volume as detached _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()); + // volumePool() should be null if the VM we are detaching the disk from has never been started before + // only revoke access on volumes that are actually on a datastore + if (volumePool != null) { + DataStore dataStore = dataStoreMgr.getDataStore(volume.getPoolId(), DataStoreRole.Primary); + volService.revokeAccess(volFactory.getVolume(volume.getId()), host, dataStore); + handleTargetsForVMware(hostId, volumePool.getHostAddress(), volumePool.getPort(), volume.get_iScsiName()); + } return _volsDao.findById(volumeId); } else { @@ -2634,7 +2635,7 @@ 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()) { @@ -2642,13 +2643,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic } 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"; - } + } else if (!volumeToAttach.getState().equals(Volume.State.Allocated)) { + 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); } @@ -2684,7 +2680,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 +2769,10 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic volumeToAttach = _volsDao.findById(volumeToAttach.getId()); - if (vm.getHypervisorType() == HypervisorType.KVM && volumeToAttachStoragePool.isManaged() && volumeToAttach.getPath() == null) { + if (volumeToAttach.getState().equals(Volume.State.Ready) && + vm.getHypervisorType() == HypervisorType.KVM && + volumeToAttachStoragePool.isManaged() && volumeToAttach.getPath() == null) { volumeToAttach.setPath(volumeToAttach.get_iScsiName()); - _volsDao.update(volumeToAttach.getId(), volumeToAttach); } } @@ -2801,15 +2798,19 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic throw new CloudRuntimeException(errorMsg); } } finally { - Volume.Event ev = Volume.Event.OperationFailed; - VolumeInfo volInfo = volFactory.getVolume(volumeToAttach.getId()); - if (attached) { - ev = Volume.Event.OperationSucceeded; - s_logger.debug("Volume: " + volInfo.getName() + " successfully attached to VM: " + volInfo.getAttachedVmName()); - } else { - s_logger.debug("Volume: " + volInfo.getName() + " failed to attach to VM: " + volInfo.getAttachedVmName()); + final VolumeInfo volInfo = volFactory.getVolume(volumeToAttach.getId()); + // Transit events are only fired when volume was allocated at some point of time. + if (volInfo.getPoolId() != null || volInfo.getLastPoolId() != null) { + final Volume.Event ev; + if (attached) { + ev = Volume.Event.OperationSucceeded; + s_logger.debug("Volume: " + volInfo.getName() + " successfully attached to VM: " + volInfo.getAttachedVmName()); + } else { + ev = Volume.Event.OperationFailed; + s_logger.debug("Volume: " + volInfo.getName() + " failed to attach to VM: " + volInfo.getAttachedVmName()); + } + volInfo.stateTransit(ev); } - volInfo.stateTransit(ev); } return _volsDao.findById(volumeToAttach.getId()); } 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(