Allow VM that has never started to have volumes attached

api/src/com/cloud/storage/Volume.java

@@ -52,7 +52,7 @@ enum State {
         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 String getDescription() {
         static {
             s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Allocated, Event.CreateRequested, Creating, null));
             s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Allocated, Event.DestroyRequested, Destroy, null));
+            s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Allocated, Event.OperationFailed, Allocated, null));
+            s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Allocated, Event.OperationSucceeded, Allocated, null));
             s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Creating, Event.OperationRetry, Creating, null));
             s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Creating, Event.OperationFailed, Allocated, null));
             s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Creating, Event.OperationSucceeded, Ready, null));

engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java

@@ -217,6 +217,9 @@ public boolean grantAccess(DataObject dataObject, Host host, DataStore dataStore
     @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);

server/src/com/cloud/storage/VolumeApiServiceImpl.java

@@ -1888,6 +1888,10 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
             }
         }
 
+        if (volumePool == null) {
+            sendCommand = false;
+        }
+
         Answer answer = null;
 
         if (sendCommand) {
@@ -1921,12 +1925,13 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
             _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 @@ private boolean needMoveVolume(VolumeVO existingVolume, VolumeInfo newVolume) {
         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)) {
+
+        if (Volume.State.Allocated.equals(volumeToAttach.getState())) {
+            return;
+        }
+
+        if (Volume.State.Ready.equals(volumeToAttach.getState())) {
             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);
+            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 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
         // 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 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
 
                     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);
                     }
                 }

test/integration/smoke/test_volumes.py

@@ -398,40 +398,40 @@ def test_02_attach_volume(self):
         # 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())
 
         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 @@ def test_10_list_volumes(self):
         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(