From 8849e0f464cf2d7781846bc3259ee98199aadb96 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 25 May 2023 19:18:27 +0530 Subject: [PATCH] server: fix volume detach operation when no vm host (#7526) Signed-off-by: Abhishek Kumar --- .../cloud/storage/VolumeApiServiceImpl.java | 68 +++++++------------ .../storage/VolumeApiServiceImplTest.java | 54 ++++++++++++--- 2 files changed, 72 insertions(+), 50 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index b02dac051cb..cec78976747 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 java.util.concurrent.ExecutionException; 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.reflectiontostringbuilderutils.ReflectionToSt 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.HypervisorCapabilitiesVO; 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,31 +2778,10 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic 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; - } - } - - 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; @@ -4080,15 +4059,15 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic 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); @@ -4100,6 +4079,18 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic return host; } + 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.isManaged()) { + sendCommand = true; + } + return 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; @@ -4108,16 +4099,9 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic 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); - Long hostId = host == null ? null : host.getId(); - if (host != null && host.getHypervisorType() == HypervisorType.VMware) { - sendCommand = true; - } - - if (host != null && host.getHypervisorType() == HypervisorType.XenServer && - volumeToAttachStoragePool != null && volumeToAttachStoragePool.isManaged()) { - sendCommand = true; - } + 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 e08e3775e35..16c7887ef91 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.List; 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; 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 class VolumeApiServiceImplTest { 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))); + } }