From 877edf59610922a1a41b73dd293fbbfeac857d30 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 12 May 2023 15:06:23 +0530 Subject: [PATCH 1/3] server: fix volume detach operation when no vm host CloudStack may not keep the VM associated to any host when there is offline migration across cluster or pod. In such cases, CloudStack should be able to send the AttachCommand or DetachCommand to any available host in the current VM cluster. Signed-off-by: Abhishek Kumar --- .../cloud/storage/VolumeApiServiceImpl.java | 50 +++++++------------ 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index b02dac051cb1..1b835c41c743 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -34,12 +34,10 @@ import javax.inject.Inject; -import com.cloud.projects.Project; -import com.cloud.projects.ProjectManager; -import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd; +import org.apache.cloudstack.api.ApiConstants.IoDriverPolicy; import org.apache.cloudstack.api.ApiErrorCode; import org.apache.cloudstack.api.ServerApiException; -import org.apache.cloudstack.api.ApiConstants.IoDriverPolicy; +import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd; import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.ChangeOfferingForVolumeCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; @@ -104,8 +102,8 @@ import org.apache.cloudstack.utils.volume.VirtualMachineDiskInfo; import org.apache.commons.collections.CollectionUtils; import org.apache.commons.collections.MapUtils; -import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.BooleanUtils; +import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import org.jetbrains.annotations.NotNull; @@ -148,6 +146,8 @@ import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao; import com.cloud.offering.DiskOffering; import com.cloud.org.Grouping; +import com.cloud.projects.Project; +import com.cloud.projects.ProjectManager; import com.cloud.resource.ResourceState; import com.cloud.serializer.GsonHelper; import com.cloud.server.ManagementService; @@ -2778,26 +2778,15 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) { String errorMsg = "Failed to detach volume " + volume.getName() + " from VM " + vm.getHostName(); boolean sendCommand = vm.getState() == State.Running; - Long hostId = vm.getHostId(); - - if (hostId == null) { - hostId = vm.getLastHostId(); - HostVO host = _hostDao.findById(hostId); - - if (host != null && host.getHypervisorType() == HypervisorType.VMware) { - sendCommand = true; - } - } - - HostVO host = null; StoragePoolVO volumePool = _storagePoolDao.findByIdIncludingRemoved(volume.getPoolId()); - - if (hostId != null) { - host = _hostDao.findById(hostId); - - if (host != null && host.getHypervisorType() == HypervisorType.XenServer && volumePool != null && volumePool.isManaged()) { - sendCommand = true; - } + HostVO host = getHostForVmVolumeAttachDetach(vm, volumePool); + Long hostId = host == null ? null : host.getId(); + if (host != null && HypervisorType.VMware.equals(host.getHypervisorType())) { + sendCommand = true; + } + if (host != null && HypervisorType.XenServer.equals(host.getHypervisorType()) && + volumePool != null && volumePool.isManaged()) { + sendCommand = true; } if (volumePool == null) { @@ -4080,15 +4069,15 @@ private String getNameOfClusteredFileSystem(HostVO hostVO) { return "clustered file systems"; } - private HostVO getHostForVmVolumeAttach(UserVmVO vm, StoragePoolVO volumeToAttachStoragePool) { + private HostVO getHostForVmVolumeAttachDetach(VirtualMachine vm, StoragePoolVO volumeStoragePool) { HostVO host = null; Pair clusterAndHostId = virtualMachineManager.findClusterAndHostIdForVm(vm.getId()); Long hostId = clusterAndHostId.second(); Long clusterId = clusterAndHostId.first(); if (hostId == null && clusterId != null && State.Stopped.equals(vm.getState()) && - volumeToAttachStoragePool != null && - !ScopeType.HOST.equals(volumeToAttachStoragePool.getScope())) { + volumeStoragePool != null && + !ScopeType.HOST.equals(volumeStoragePool.getScope())) { List hosts = _hostDao.findHypervisorHostInCluster(clusterId); if (!hosts.isEmpty()) { host = hosts.get(0); @@ -4108,13 +4097,12 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L if (s_logger.isTraceEnabled() && volumeToAttachStoragePool != null) { s_logger.trace(String.format("storage is gotten from volume to attach: %s/%s",volumeToAttachStoragePool.getName(),volumeToAttachStoragePool.getUuid())); } - HostVO host = getHostForVmVolumeAttach(vm, volumeToAttachStoragePool); + HostVO host = getHostForVmVolumeAttachDetach(vm, volumeToAttachStoragePool); Long hostId = host == null ? null : host.getId(); - if (host != null && host.getHypervisorType() == HypervisorType.VMware) { + if (host != null && HypervisorType.VMware.equals(host.getHypervisorType())) { sendCommand = true; } - - if (host != null && host.getHypervisorType() == HypervisorType.XenServer && + if (host != null && HypervisorType.XenServer.equals(host.getHypervisorType()) && volumeToAttachStoragePool != null && volumeToAttachStoragePool.isManaged()) { sendCommand = true; } From 7b9dd82beeed1209f15fbea001c97b2c386f0d49 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 12 May 2023 16:06:55 +0530 Subject: [PATCH 2/3] wip Signed-off-by: Abhishek Kumar --- .../cloud/storage/VolumeApiServiceImpl.java | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 1b835c41c743..3978d2bcdb09 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -2779,15 +2779,8 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) { boolean sendCommand = vm.getState() == State.Running; StoragePoolVO volumePool = _storagePoolDao.findByIdIncludingRemoved(volume.getPoolId()); - HostVO host = getHostForVmVolumeAttachDetach(vm, volumePool); - Long hostId = host == null ? null : host.getId(); - if (host != null && HypervisorType.VMware.equals(host.getHypervisorType())) { - sendCommand = true; - } - if (host != null && HypervisorType.XenServer.equals(host.getHypervisorType()) && - volumePool != null && volumePool.isManaged()) { - sendCommand = true; - } + Long hostId = null; + updateHostIdAndSendCommandForVmVolumeAttachDetach(vm, volumePool, hostId, sendCommand); if (volumePool == null) { sendCommand = false; @@ -4089,6 +4082,28 @@ private HostVO getHostForVmVolumeAttachDetach(VirtualMachine vm, StoragePoolVO v return host; } + private void updateHostIdAndSendCommandForVmVolumeAttachDetach(VirtualMachine vm, StoragePoolVO volumeStoragePool, + Long hostId, boolean sendCommand) { + HostVO host = getHostForVmVolumeAttachDetach(vm, volumeStoragePool); + if (host == null) { + return; + } + hostId = host.getId(); + if (sendCommand) { + return; + } + if (HypervisorType.VMware.equals(host.getHypervisorType())) { + sendCommand = true; + } + if (HypervisorType.XenServer.equals(host.getHypervisorType()) && + volumeStoragePool != null && volumeStoragePool.isManaged()) { + sendCommand = true; + } + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Volume attach-detach operation for %s can be done using host ID: %d, sendCommand: %s", vm, hostId, sendCommand)); + } + } + private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, Long deviceId) { String errorMsg = "Failed to attach volume " + volumeToAttach.getName() + " to VM " + vm.getHostName(); boolean sendCommand = vm.getState() == State.Running; @@ -4097,15 +4112,8 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L if (s_logger.isTraceEnabled() && volumeToAttachStoragePool != null) { s_logger.trace(String.format("storage is gotten from volume to attach: %s/%s",volumeToAttachStoragePool.getName(),volumeToAttachStoragePool.getUuid())); } - HostVO host = getHostForVmVolumeAttachDetach(vm, volumeToAttachStoragePool); - Long hostId = host == null ? null : host.getId(); - if (host != null && HypervisorType.VMware.equals(host.getHypervisorType())) { - sendCommand = true; - } - if (host != null && HypervisorType.XenServer.equals(host.getHypervisorType()) && - volumeToAttachStoragePool != null && volumeToAttachStoragePool.isManaged()) { - sendCommand = true; - } + Long hostId = null; + updateHostIdAndSendCommandForVmVolumeAttachDetach(vm, volumeToAttachStoragePool, hostId, sendCommand); if (host != null) { _hostDao.loadDetails(host); From d4f7f2a4bf500800c830cc9a02b838ffefc2f7c4 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 12 May 2023 17:11:40 +0530 Subject: [PATCH 3/3] tests Signed-off-by: Abhishek Kumar --- .../cloud/storage/VolumeApiServiceImpl.java | 36 +++++-------- .../storage/VolumeApiServiceImplTest.java | 54 ++++++++++++++++--- 2 files changed, 58 insertions(+), 32 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 3978d2bcdb09..cec789767479 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -2779,12 +2779,9 @@ private Volume orchestrateDetachVolumeFromVM(long vmId, long volumeId) { boolean sendCommand = vm.getState() == State.Running; StoragePoolVO volumePool = _storagePoolDao.findByIdIncludingRemoved(volume.getPoolId()); - Long hostId = null; - updateHostIdAndSendCommandForVmVolumeAttachDetach(vm, volumePool, hostId, sendCommand); - - if (volumePool == null) { - sendCommand = false; - } + HostVO host = getHostForVmVolumeAttachDetach(vm, volumePool); + Long hostId = host != null ? host.getId() : null; + sendCommand = sendCommand || isSendCommandForVmVolumeAttachDetach(host, volumePool); Answer answer = null; @@ -4082,26 +4079,16 @@ private HostVO getHostForVmVolumeAttachDetach(VirtualMachine vm, StoragePoolVO v return host; } - private void updateHostIdAndSendCommandForVmVolumeAttachDetach(VirtualMachine vm, StoragePoolVO volumeStoragePool, - Long hostId, boolean sendCommand) { - HostVO host = getHostForVmVolumeAttachDetach(vm, volumeStoragePool); - if (host == null) { - return; - } - hostId = host.getId(); - if (sendCommand) { - return; - } - if (HypervisorType.VMware.equals(host.getHypervisorType())) { - sendCommand = true; + protected boolean isSendCommandForVmVolumeAttachDetach(HostVO host, StoragePoolVO volumeStoragePool) { + if (host == null || volumeStoragePool == null) { + return false; } + boolean sendCommand = HypervisorType.VMware.equals(host.getHypervisorType()); if (HypervisorType.XenServer.equals(host.getHypervisorType()) && - volumeStoragePool != null && volumeStoragePool.isManaged()) { + volumeStoragePool.isManaged()) { sendCommand = true; } - if (s_logger.isDebugEnabled()) { - s_logger.debug(String.format("Volume attach-detach operation for %s can be done using host ID: %d, sendCommand: %s", vm, hostId, sendCommand)); - } + return sendCommand; } private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, Long deviceId) { @@ -4112,8 +4099,9 @@ private VolumeVO sendAttachVolumeCommand(UserVmVO vm, VolumeVO volumeToAttach, L if (s_logger.isTraceEnabled() && volumeToAttachStoragePool != null) { s_logger.trace(String.format("storage is gotten from volume to attach: %s/%s",volumeToAttachStoragePool.getName(),volumeToAttachStoragePool.getUuid())); } - Long hostId = null; - updateHostIdAndSendCommandForVmVolumeAttachDetach(vm, volumeToAttachStoragePool, hostId, sendCommand); + HostVO host = getHostForVmVolumeAttachDetach(vm, volumeToAttachStoragePool); + Long hostId = host != null ? host.getId() : null; + sendCommand = sendCommand || isSendCommandForVmVolumeAttachDetach(host, volumeToAttachStoragePool); if (host != null) { _hostDao.loadDetails(host); diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index e08e3775e355..16c7887ef915 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -17,10 +17,6 @@ package com.cloud.storage; import static org.junit.Assert.assertEquals; -import com.cloud.exception.PermissionDeniedException; -import com.cloud.projects.Project; -import com.cloud.projects.ProjectManager; -import com.cloud.storage.dao.SnapshotDao; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyLong; import static org.mockito.Matchers.anyObject; @@ -40,10 +36,6 @@ import java.util.UUID; import java.util.concurrent.ExecutionException; -import com.cloud.event.EventTypes; -import com.cloud.event.UsageEventUtils; -import com.cloud.service.ServiceOfferingVO; -import com.cloud.service.dao.ServiceOfferingDao; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; @@ -90,16 +82,25 @@ import com.cloud.configuration.Resource.ResourceType; import com.cloud.dc.DataCenterVO; import com.cloud.dc.dao.DataCenterDao; +import com.cloud.event.EventTypes; +import com.cloud.event.UsageEventUtils; import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceAllocationException; +import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.org.Grouping; +import com.cloud.projects.Project; +import com.cloud.projects.ProjectManager; import com.cloud.serializer.GsonHelper; import com.cloud.server.TaggedResourceService; +import com.cloud.service.ServiceOfferingVO; +import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.storage.Storage.ProvisioningType; import com.cloud.storage.Volume.Type; import com.cloud.storage.dao.DiskOfferingDao; +import com.cloud.storage.dao.SnapshotDao; import com.cloud.storage.dao.StoragePoolTagsDao; import com.cloud.storage.dao.VMTemplateDao; import com.cloud.storage.dao.VolumeDao; @@ -1525,4 +1526,41 @@ public void testGetMinimumHypervisorVersionInDatacenterVersions() { String expected = "6.7"; testBaseListOrderedHostsHypervisorVersionInDc(hwVersions, hypervisorType, expected); } + + @Test + public void testIsSendCommandForVmVolumeAttachDetachNullValues() { + Assert.assertFalse(volumeApiServiceImpl.isSendCommandForVmVolumeAttachDetach(null, null)); + Assert.assertFalse(volumeApiServiceImpl.isSendCommandForVmVolumeAttachDetach(null, Mockito.mock(StoragePoolVO.class))); + Assert.assertFalse(volumeApiServiceImpl.isSendCommandForVmVolumeAttachDetach(Mockito.mock(HostVO.class), null)); + } + + @Test + public void testIsSendCommandForVmVolumeAttachDetachVMwareHost() { + HostVO host = Mockito.mock(HostVO.class); + Mockito.when(host.getHypervisorType()).thenReturn(HypervisorType.VMware); + Assert.assertTrue(volumeApiServiceImpl.isSendCommandForVmVolumeAttachDetach(host, Mockito.mock(StoragePoolVO.class))); + } + + @Test + public void testIsSendCommandForVmVolumeAttachDetachXenserverHostNonMananged() { + HostVO host = Mockito.mock(HostVO.class); + Mockito.when(host.getHypervisorType()).thenReturn(HypervisorType.XenServer); + Assert.assertFalse(volumeApiServiceImpl.isSendCommandForVmVolumeAttachDetach(host, Mockito.mock(StoragePoolVO.class))); + } + + @Test + public void testIsSendCommandForVmVolumeAttachDetachXenserverHostMananged() { + HostVO host = Mockito.mock(HostVO.class); + Mockito.when(host.getHypervisorType()).thenReturn(HypervisorType.XenServer); + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + Mockito.when(pool.isManaged()).thenReturn(true); + Assert.assertTrue(volumeApiServiceImpl.isSendCommandForVmVolumeAttachDetach(host, pool)); + } + + @Test + public void testIsSendCommandForVmVolumeAttachDetachKVMHost() { + HostVO host = Mockito.mock(HostVO.class); + Mockito.when(host.getHypervisorType()).thenReturn(HypervisorType.KVM); + Assert.assertFalse(volumeApiServiceImpl.isSendCommandForVmVolumeAttachDetach(host, Mockito.mock(StoragePoolVO.class))); + } }