FIX4: Allow ready volumes to be attached to VMs that have never been started once (#71)

With this patch apache/cloudstack@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.

Author: anuragaw

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. Attach requests when in allocated state do not transit to this state.");
 
         String _description;
 
@@ -120,8 +120,8 @@ public String getDescription() {
             s_fsm.addTransition(new StateMachine2.Transition<State, Event>(UploadError, Event.DestroyRequested, Destroy, null));
             s_fsm.addTransition(new StateMachine2.Transition<State, Event>(UploadAbandoned, Event.DestroyRequested, Destroy, null));
             s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Ready, Event.AttachRequested, Attaching, null));
-            s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Attaching, Event.OperationSucceeded, Ready, null));
             s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Attaching, Event.OperationFailed, Ready, null));
+            s_fsm.addTransition(new StateMachine2.Transition<State, Event>(Attaching, Event.OperationSucceeded, Ready, null));
         }
     }
 

server/src/com/cloud/network/element/ConfigDriveNetworkElement.java

@@ -23,7 +23,6 @@
 
 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.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;

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

@@ -1920,12 +1920,13 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) {
             // 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,21 +2635,16 @@ 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)) {
             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 @@ 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 +2769,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 (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 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L
                 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());
     }

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(