From 6a756d4f6f1e5c9dd262ff30b308c3554a1fe549 Mon Sep 17 00:00:00 2001 From: Anurag Awasthi Date: Mon, 15 Apr 2019 17:33:30 +0530 Subject: [PATCH 1/9] Add more states for attaching from allocated state for the use case where users want to attach disk in allocated state but not ready, we need to have more states for keeping track of attaching state. 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. --- api/src/com/cloud/storage/Volume.java | 12 ++++++---- .../orchestration/VolumeOrchestrator.java | 8 ++++++- .../element/ConfigDriveNetworkElement.java | 5 ++-- .../cloud/storage/VolumeApiServiceImpl.java | 24 +++++++++++-------- 4 files changed, 32 insertions(+), 17 deletions(-) diff --git a/api/src/com/cloud/storage/Volume.java b/api/src/com/cloud/storage/Volume.java index 133a59df8a6d..a7fece6bb0d5 100644 --- a/api/src/com/cloud/storage/Volume.java +++ b/api/src/com/cloud/storage/Volume.java @@ -52,7 +52,8 @@ 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"); + AttachingFromReady("The volume is attaching to a VM from Ready State"), + AttachingFromAllocated("The volume is attaching to a VM from Allocated State"); String _description; @@ -119,9 +120,12 @@ public String getDescription() { s_fsm.addTransition(new StateMachine2.Transition(UploadInProgress, Event.OperationTimeout, UploadError, null)); 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(Allocated, Event.AttachRequested, AttachingFromAllocated, null)); + s_fsm.addTransition(new StateMachine2.Transition(AttachingFromAllocated, Event.OperationSucceeded, Allocated, null)); + s_fsm.addTransition(new StateMachine2.Transition(AttachingFromAllocated, Event.OperationFailed, Allocated, null)); + s_fsm.addTransition(new StateMachine2.Transition(Ready, Event.AttachRequested, AttachingFromReady, null)); + s_fsm.addTransition(new StateMachine2.Transition(AttachingFromReady, Event.OperationSucceeded, Ready, null)); + s_fsm.addTransition(new StateMachine2.Transition(AttachingFromReady, Event.OperationFailed, Ready, null)); } } diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 7435a3eaf231..a5c75289cf64 100644 --- a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1466,11 +1466,17 @@ private void cleanupVolumeDuringAttachFailure(Long volumeId, Long vmId) { _volsDao.remove(volume.getId()); } - if (volume.getState().equals(Volume.State.Attaching)) { + if (volume.getState().equals(Volume.State.AttachingFromReady)) { s_logger.warn("Vol: " + volume.getName() + " failed to attach to VM: " + _userVmDao.findById(vmId).getHostName() + " on last mgt server stop, changing state back to Ready"); volume.setState(Volume.State.Ready); _volsDao.update(volumeId, volume); } + + if (volume.getState().equals(Volume.State.AttachingFromAllocated)) { + s_logger.warn("Vol: " + volume.getName() + " failed to attach to VM: " + _userVmDao.findById(vmId).getHostName() + " on last mgt server stop, changing state back to Allocated"); + volume.setState(Volume.State.Allocated); + _volsDao.update(volumeId, volume); + } } private void cleanupVolumeDuringMigrationFailure(Long volumeId, Long destPoolId) { diff --git a/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java b/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java index 4d2452d45ce5..43f61590344a 100644 --- a/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java +++ b/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; @@ -377,7 +377,8 @@ private boolean doesVolumeStateCheckout(Volume vol) { case RevertSnapshotting: case Resizing: case Copying: - case Attaching: + case AttachingFromReady: + case AttachingFromAllocated: return true; case Migrating: case Expunging: diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java index 372ab3423af4..72acdb83b400 100644 --- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java +++ b/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 { @@ -2640,14 +2641,14 @@ private synchronized void checkAndSetAttaching(Long volumeId, Long hostId) { 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 (volumeToAttach.getState().equals(Volume.State.Ready) || volumeToAttach.getState().equals(Volume.State.Allocated)) { 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"; + error = "Volume: " + volumeToAttach.getName() + " is in " + volumeToAttach.getState() + ". It should be in Ready or Allocated state"; } s_logger.error(error); throw new CloudRuntimeException(error); @@ -2773,9 +2774,12 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L volumeToAttach = _volsDao.findById(volumeToAttach.getId()); - if (vm.getHypervisorType() == HypervisorType.KVM && volumeToAttachStoragePool.isManaged() && volumeToAttach.getPath() == null) { - volumeToAttach.setPath(volumeToAttach.get_iScsiName()); + 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); } } From d6a1e30c3660e0fafe28e94eef4a9f444f300405 Mon Sep 17 00:00:00 2001 From: Anurag Awasthi Date: Wed, 17 Apr 2019 14:46:46 +0530 Subject: [PATCH 2/9] Add Marvin test for the use case of attaching volume to unstarted VM --- test/integration/smoke/test_volumes.py | 94 ++++++++++++++++++++------ 1 file changed, 74 insertions(+), 20 deletions(-) diff --git a/test/integration/smoke/test_volumes.py b/test/integration/smoke/test_volumes.py index d40c0fd065f6..c71411a25e6d 100644 --- a/test/integration/smoke/test_volumes.py +++ b/test/integration/smoke/test_volumes.py @@ -398,32 +398,32 @@ 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()) @@ -431,7 +431,7 @@ def test_02_attach_volume(self): 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( From bdd10218fb3e8cec464a487e9f1a10368582e49b Mon Sep 17 00:00:00 2001 From: Anurag Awasthi Date: Thu, 18 Apr 2019 14:34:09 +0530 Subject: [PATCH 3/9] Addressed code review comments --- api/src/com/cloud/storage/Volume.java | 13 +++++-------- .../orchestration/VolumeOrchestrator.java | 8 +------- .../element/ConfigDriveNetworkElement.java | 3 +-- .../com/cloud/storage/VolumeApiServiceImpl.java | 17 ++++------------- 4 files changed, 11 insertions(+), 30 deletions(-) diff --git a/api/src/com/cloud/storage/Volume.java b/api/src/com/cloud/storage/Volume.java index a7fece6bb0d5..a4e40671f8bb 100644 --- a/api/src/com/cloud/storage/Volume.java +++ b/api/src/com/cloud/storage/Volume.java @@ -52,8 +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"), - AttachingFromReady("The volume is attaching to a VM from Ready State"), - AttachingFromAllocated("The volume is attaching to a VM from Allocated State"); + Attaching("The volume is attaching to a VM from Ready State"); String _description; @@ -120,12 +119,10 @@ public String getDescription() { s_fsm.addTransition(new StateMachine2.Transition(UploadInProgress, Event.OperationTimeout, UploadError, null)); 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(Allocated, Event.AttachRequested, AttachingFromAllocated, null)); - s_fsm.addTransition(new StateMachine2.Transition(AttachingFromAllocated, Event.OperationSucceeded, Allocated, null)); - s_fsm.addTransition(new StateMachine2.Transition(AttachingFromAllocated, Event.OperationFailed, Allocated, null)); - s_fsm.addTransition(new StateMachine2.Transition(Ready, Event.AttachRequested, AttachingFromReady, null)); - s_fsm.addTransition(new StateMachine2.Transition(AttachingFromReady, Event.OperationSucceeded, Ready, null)); - s_fsm.addTransition(new StateMachine2.Transition(AttachingFromReady, Event.OperationFailed, Ready, null)); + s_fsm.addTransition(new StateMachine2.Transition(Allocated, Event.AttachRequested, Attaching, 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)); } } diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index a5c75289cf64..7435a3eaf231 100644 --- a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1466,17 +1466,11 @@ private void cleanupVolumeDuringAttachFailure(Long volumeId, Long vmId) { _volsDao.remove(volume.getId()); } - if (volume.getState().equals(Volume.State.AttachingFromReady)) { + if (volume.getState().equals(Volume.State.Attaching)) { s_logger.warn("Vol: " + volume.getName() + " failed to attach to VM: " + _userVmDao.findById(vmId).getHostName() + " on last mgt server stop, changing state back to Ready"); volume.setState(Volume.State.Ready); _volsDao.update(volumeId, volume); } - - if (volume.getState().equals(Volume.State.AttachingFromAllocated)) { - s_logger.warn("Vol: " + volume.getName() + " failed to attach to VM: " + _userVmDao.findById(vmId).getHostName() + " on last mgt server stop, changing state back to Allocated"); - volume.setState(Volume.State.Allocated); - _volsDao.update(volumeId, volume); - } } private void cleanupVolumeDuringMigrationFailure(Long volumeId, Long destPoolId) { diff --git a/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java b/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java index 43f61590344a..9a6e19b740a5 100644 --- a/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java +++ b/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java @@ -377,8 +377,7 @@ private boolean doesVolumeStateCheckout(Volume vol) { case RevertSnapshotting: case Resizing: case Copying: - case AttachingFromReady: - case AttachingFromAllocated: + case Attaching: return true; case Migrating: case Expunging: diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java index 72acdb83b400..3077bba667ee 100644 --- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/com/cloud/storage/VolumeApiServiceImpl.java @@ -2635,7 +2635,7 @@ 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()) { @@ -2644,12 +2644,7 @@ private synchronized void checkAndSetAttaching(Long volumeId, Long hostId) { if (volumeToAttach.getState().equals(Volume.State.Ready) || volumeToAttach.getState().equals(Volume.State.Allocated)) { 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 or Allocated state"; - } + 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); } @@ -2685,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 { @@ -2774,11 +2769,7 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L volumeToAttach = _volsDao.findById(volumeToAttach.getId()); - if (volumeToAttach.getState().equals(Volume.State.Ready) && - vm.getHypervisorType() == HypervisorType.KVM && - volumeToAttachStoragePool.isManaged() && - volumeToAttach.getPath() == null) { - + if (vm.getHypervisorType() == HypervisorType.KVM && volumeToAttachStoragePool.isManaged() && volumeToAttach.getPath() == null) { volumeToAttach.setPath(volumeToAttach.get_iScsiName()); _volsDao.update(volumeToAttach.getId(), volumeToAttach); } From c5f75247b09c8c089095c82a0def42cc513b0df1 Mon Sep 17 00:00:00 2001 From: Anurag Awasthi Date: Thu, 18 Apr 2019 15:02:27 +0530 Subject: [PATCH 4/9] Revert to allocated state when cleaning up jobs for volumes not having pool id in attaching state --- .../cloudstack/engine/orchestration/VolumeOrchestrator.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 7435a3eaf231..fb38ba4fd597 100644 --- a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1468,7 +1468,11 @@ private void cleanupVolumeDuringAttachFailure(Long volumeId, Long vmId) { if (volume.getState().equals(Volume.State.Attaching)) { s_logger.warn("Vol: " + volume.getName() + " failed to attach to VM: " + _userVmDao.findById(vmId).getHostName() + " on last mgt server stop, changing state back to Ready"); - volume.setState(Volume.State.Ready); + if (volume.getPoolId() != null) { + volume.setState(Volume.State.Ready); + } else { + volume.setState(Volume.State.Allocated); + } _volsDao.update(volumeId, volume); } } From 47fae855e2dbf7750e302786e67969d48a00e967 Mon Sep 17 00:00:00 2001 From: Anurag Awasthi Date: Mon, 22 Apr 2019 12:33:46 +0530 Subject: [PATCH 5/9] Revert unintended code change during code review addressal --- server/src/com/cloud/storage/VolumeApiServiceImpl.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java index 3077bba667ee..735af5453311 100644 --- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/com/cloud/storage/VolumeApiServiceImpl.java @@ -2769,7 +2769,9 @@ 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); } From c518abaed70b95504a4305702d6469c3fb658bcb Mon Sep 17 00:00:00 2001 From: Anurag Awasthi Date: Mon, 22 Apr 2019 14:35:09 +0530 Subject: [PATCH 6/9] Fix for tracking allocated vs ready state when attaching --- api/src/com/cloud/storage/Volume.java | 16 ++++++++++++---- .../engine/orchestration/VolumeOrchestrator.java | 6 +----- .../com/cloud/storage/VolumeApiServiceImpl.java | 8 +++++--- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/api/src/com/cloud/storage/Volume.java b/api/src/com/cloud/storage/Volume.java index a4e40671f8bb..4d4dd43f081d 100644 --- a/api/src/com/cloud/storage/Volume.java +++ b/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 from Ready State"); + Attaching("The volume is attaching to a VM"); String _description; @@ -119,10 +119,14 @@ public String getDescription() { s_fsm.addTransition(new StateMachine2.Transition(UploadInProgress, Event.OperationTimeout, UploadError, null)); s_fsm.addTransition(new StateMachine2.Transition(UploadError, Event.DestroyRequested, Destroy, null)); s_fsm.addTransition(new StateMachine2.Transition(UploadAbandoned, Event.DestroyRequested, Destroy, null)); + + // Attaching can start from Allocated or Ready state. We need to track attaching and return to the original state. s_fsm.addTransition(new StateMachine2.Transition(Allocated, Event.AttachRequested, Attaching, 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.AttachFromAllocatedSucceeded, Allocated, null)); + s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.AttachFromAllocatedFailed, Allocated, null)); + s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.AttachFromReadySucceeded, Ready, null)); + s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.AttachFromReadyFailed, Ready, null)); } } @@ -145,7 +149,11 @@ enum Event { ExpungingRequested, ResizeRequested, AttachRequested, - OperationTimeout; + OperationTimeout, + AttachFromReadySucceeded, + AttachFromReadyFailed, + AttachFromAllocatedSucceeded, + AttachFromAllocatedFailed; } /** diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index fb38ba4fd597..58322c1bdf8a 100644 --- a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1468,11 +1468,7 @@ private void cleanupVolumeDuringAttachFailure(Long volumeId, Long vmId) { if (volume.getState().equals(Volume.State.Attaching)) { s_logger.warn("Vol: " + volume.getName() + " failed to attach to VM: " + _userVmDao.findById(vmId).getHostName() + " on last mgt server stop, changing state back to Ready"); - if (volume.getPoolId() != null) { - volume.setState(Volume.State.Ready); - } else { - volume.setState(Volume.State.Allocated); - } + volume.setState((volume.getPoolId() == null && volume.getLastPoolId() == null) ? Volume.State.Allocated : Volume.State.Ready); _volsDao.update(volumeId, volume); } } diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java index 735af5453311..310c4d5abcd6 100644 --- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/com/cloud/storage/VolumeApiServiceImpl.java @@ -2798,12 +2798,14 @@ 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()); + final VolumeInfo volInfo = volFactory.getVolume(volumeToAttach.getId()); + final boolean isVolumeInAllocated = volInfo.getPoolId() == null && volInfo.getLastPoolId() == null; + final Volume.Event ev; if (attached) { - ev = Volume.Event.OperationSucceeded; + ev = isVolumeInAllocated ? Volume.Event.AttachFromAllocatedSucceeded : Volume.Event.AttachFromReadySucceeded; s_logger.debug("Volume: " + volInfo.getName() + " successfully attached to VM: " + volInfo.getAttachedVmName()); } else { + ev = isVolumeInAllocated ? Volume.Event.AttachFromAllocatedFailed : Volume.Event.AttachFromReadyFailed; s_logger.debug("Volume: " + volInfo.getName() + " failed to attach to VM: " + volInfo.getAttachedVmName()); } volInfo.stateTransit(ev); From 49526556a5c4cdbdff63643dbd56c4df36523596 Mon Sep 17 00:00:00 2001 From: Anurag Awasthi Date: Mon, 22 Apr 2019 18:25:52 +0530 Subject: [PATCH 7/9] Don't transition at all for Allocated state --- api/src/com/cloud/storage/Volume.java | 17 ++++--------- .../orchestration/VolumeOrchestrator.java | 2 +- .../cloud/storage/VolumeApiServiceImpl.java | 24 ++++++++++--------- 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/api/src/com/cloud/storage/Volume.java b/api/src/com/cloud/storage/Volume.java index 4d4dd43f081d..b84622f5c534 100644 --- a/api/src/com/cloud/storage/Volume.java +++ b/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; @@ -119,14 +119,9 @@ public String getDescription() { s_fsm.addTransition(new StateMachine2.Transition(UploadInProgress, Event.OperationTimeout, UploadError, null)); s_fsm.addTransition(new StateMachine2.Transition(UploadError, Event.DestroyRequested, Destroy, null)); s_fsm.addTransition(new StateMachine2.Transition(UploadAbandoned, Event.DestroyRequested, Destroy, null)); - - // Attaching can start from Allocated or Ready state. We need to track attaching and return to the original state. - s_fsm.addTransition(new StateMachine2.Transition(Allocated, Event.AttachRequested, Attaching, null)); s_fsm.addTransition(new StateMachine2.Transition(Ready, Event.AttachRequested, Attaching, null)); - s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.AttachFromAllocatedSucceeded, Allocated, null)); - s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.AttachFromAllocatedFailed, Allocated, null)); - s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.AttachFromReadySucceeded, Ready, null)); - s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.AttachFromReadyFailed, Ready, null)); + s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.OperationFailed, Ready, null)); + s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.OperationSucceeded, Ready, null)); } } @@ -149,11 +144,7 @@ enum Event { ExpungingRequested, ResizeRequested, AttachRequested, - OperationTimeout, - AttachFromReadySucceeded, - AttachFromReadyFailed, - AttachFromAllocatedSucceeded, - AttachFromAllocatedFailed; + OperationTimeout; } /** diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 58322c1bdf8a..7435a3eaf231 100644 --- a/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1468,7 +1468,7 @@ private void cleanupVolumeDuringAttachFailure(Long volumeId, Long vmId) { if (volume.getState().equals(Volume.State.Attaching)) { s_logger.warn("Vol: " + volume.getName() + " failed to attach to VM: " + _userVmDao.findById(vmId).getHostName() + " on last mgt server stop, changing state back to Ready"); - volume.setState((volume.getPoolId() == null && volume.getLastPoolId() == null) ? Volume.State.Allocated : Volume.State.Ready); + volume.setState(Volume.State.Ready); _volsDao.update(volumeId, volume); } } diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java index 310c4d5abcd6..33a5fa75f72c 100644 --- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/com/cloud/storage/VolumeApiServiceImpl.java @@ -2641,9 +2641,9 @@ private synchronized void checkAndSetAttaching(Long 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.getState().equals(Volume.State.Allocated)) { + if (volumeToAttach.getState().equals(Volume.State.Ready)) { volumeToAttach.stateTransit(Volume.Event.AttachRequested); - } else { + } 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); @@ -2799,16 +2799,18 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L } } finally { final VolumeInfo volInfo = volFactory.getVolume(volumeToAttach.getId()); - final boolean isVolumeInAllocated = volInfo.getPoolId() == null && volInfo.getLastPoolId() == null; - final Volume.Event ev; - if (attached) { - ev = isVolumeInAllocated ? Volume.Event.AttachFromAllocatedSucceeded : Volume.Event.AttachFromReadySucceeded; - s_logger.debug("Volume: " + volInfo.getName() + " successfully attached to VM: " + volInfo.getAttachedVmName()); - } else { - ev = isVolumeInAllocated ? Volume.Event.AttachFromAllocatedFailed : Volume.Event.AttachFromReadyFailed; - s_logger.debug("Volume: " + volInfo.getName() + " failed to attach to VM: " + volInfo.getAttachedVmName()); + // 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()); } From 36646bfe773eb7d832cb824a916122cad713b4c9 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Wed, 24 Apr 2019 01:05:10 +0530 Subject: [PATCH 8/9] Simplify bugfix implementation Signed-off-by: Rohit Yadav --- api/src/com/cloud/storage/Volume.java | 2 + .../storage/volume/VolumeServiceImpl.java | 3 ++ .../cloud/storage/VolumeApiServiceImpl.java | 54 ++++++++++--------- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/api/src/com/cloud/storage/Volume.java b/api/src/com/cloud/storage/Volume.java index b84622f5c534..17d57d681758 100644 --- a/api/src/com/cloud/storage/Volume.java +++ b/api/src/com/cloud/storage/Volume.java @@ -72,6 +72,8 @@ public String getDescription() { 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 d2979f7415dd..4abb190dcf2a 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 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); diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java index 33a5fa75f72c..085c69953489 100644 --- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java +++ b/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) { @@ -1920,14 +1924,14 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) { // Mark the volume as detached _volsDao.detachVolume(volume.getId()); - // 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) { + // volume.getPoolId() should be null if the VM we are detaching the disk from has never been started before + 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 { @@ -2641,13 +2645,19 @@ private synchronized void checkAndSetAttaching(Long 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 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); + 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) { @@ -2769,9 +2779,9 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L volumeToAttach = _volsDao.findById(volumeToAttach.getId()); - if (volumeToAttach.getState().equals(Volume.State.Ready) && - 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); } @@ -2798,19 +2808,15 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L throw new CloudRuntimeException(errorMsg); } } finally { - 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); + 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()); } + volInfo.stateTransit(ev); } return _volsDao.findById(volumeToAttach.getId()); } From fa7e8bfedd5528c7084d886de51d6648843571f3 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Wed, 24 Apr 2019 01:15:09 +0530 Subject: [PATCH 9/9] reduce diff Signed-off-by: Rohit Yadav --- api/src/com/cloud/storage/Volume.java | 4 ++-- .../com/cloud/network/element/ConfigDriveNetworkElement.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/com/cloud/storage/Volume.java b/api/src/com/cloud/storage/Volume.java index 17d57d681758..0e86ac016386 100644 --- a/api/src/com/cloud/storage/Volume.java +++ b/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 from Ready State. Attach requests when in allocated state do not transit to this state."); + Attaching("The volume is attaching to a VM from Ready state."); String _description; @@ -122,8 +122,8 @@ public String getDescription() { 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.OperationFailed, Ready, null)); s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.OperationSucceeded, Ready, null)); + s_fsm.addTransition(new StateMachine2.Transition(Attaching, Event.OperationFailed, Ready, null)); } } diff --git a/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java b/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java index 9a6e19b740a5..4d2452d45ce5 100644 --- a/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java +++ b/server/src/com/cloud/network/element/ConfigDriveNetworkElement.java @@ -23,6 +23,7 @@ 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; @@ -58,7 +59,6 @@ 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;