From 52c5607fd6d2a449f37c2801fbdc42d6cdd3742c Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Sun, 5 Mar 2023 18:38:06 +0530 Subject: [PATCH 1/4] storage: fix vmware volume cleanup failure Fixes #7311 Signed-off-by: Abhishek Kumar --- .../endpoint/DefaultEndPointSelector.java | 49 ++++++++++++++----- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java b/engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java index 6a903e4235a4..703d7a9e4bd3 100644 --- a/engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java +++ b/engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java @@ -30,15 +30,12 @@ import javax.inject.Inject; -import com.cloud.dc.DedicatedResourceVO; -import com.cloud.dc.dao.DedicatedResourceDao; -import com.cloud.user.Account; -import com.cloud.utils.Pair; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.EndPoint; import org.apache.cloudstack.engine.subsystem.api.storage.EndPointSelector; +import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore; import org.apache.cloudstack.engine.subsystem.api.storage.Scope; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo; import org.apache.cloudstack.engine.subsystem.api.storage.StorageAction; @@ -50,6 +47,8 @@ import org.springframework.stereotype.Component; import com.cloud.capacity.CapacityManager; +import com.cloud.dc.DedicatedResourceVO; +import com.cloud.dc.dao.DedicatedResourceDao; import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.host.Status; @@ -58,6 +57,8 @@ import com.cloud.storage.DataStoreRole; import com.cloud.storage.ScopeType; import com.cloud.storage.Storage.TemplateType; +import com.cloud.user.Account; +import com.cloud.utils.Pair; import com.cloud.utils.db.DB; import com.cloud.utils.db.QueryBuilder; import com.cloud.utils.db.SearchCriteria.Op; @@ -318,6 +319,35 @@ protected EndPoint findEndpointForImageStorage(DataStore store) { return RemoteHostEndPoint.getHypervisorHostEndPoint(host); } + protected Host getVmwareHostFromVolumeToDelete(VolumeInfo volume) { + VirtualMachine vm = volume.getAttachedVM(); + if (vm == null) { + return null; + } + Long hostId = vm.getHostId() != null ? vm.getHostId() : vm.getLastHostId(); + if (hostId == null) { + return null; + } + HostVO host = hostDao.findById(hostId); + if (host == null) { + return null; + } + if (volume.getDataStore() instanceof PrimaryDataStore) { + PrimaryDataStore dataStore = (PrimaryDataStore)volume.getDataStore(); + if (dataStore.getScope() == null) { + return null; + } + if (ScopeType.HOST.equals(dataStore.getScope().getScopeType()) && !host.getStorageIpAddress().equals(dataStore.getHostAddress())) { + return null; + } + if (ScopeType.CLUSTER.equals(dataStore.getScope().getScopeType()) && !host.getClusterId().equals(dataStore.getClusterId())) { + return null; + } + } + s_logger.debug(String.format("Selected host %s for deleting volume ID: %d", host, volume.getId())); + return host; + } + @Override public List findAllEndpointsForScope(DataStore store) { Long dcId = null; @@ -436,13 +466,10 @@ public EndPoint select(DataObject object, StorageAction action) { } } else if (action == StorageAction.DELETEVOLUME) { VolumeInfo volume = (VolumeInfo)object; - if (volume.getHypervisorType() == Hypervisor.HypervisorType.VMware) { - VirtualMachine vm = volume.getAttachedVM(); - if (vm != null) { - Long hostId = vm.getHostId() != null ? vm.getHostId() : vm.getLastHostId(); - if (hostId != null) { - return getEndPointFromHostId(hostId); - } + if (Hypervisor.HypervisorType.VMware.equals(volume.getHypervisorType())) { + Host host = getVmwareHostFromVolumeToDelete(volume); + if (host != null) { + return RemoteHostEndPoint.getHypervisorHostEndPoint(host); } } } From 915ae0ac6e30cd839e8d9869c14c456cae4300bf Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 6 Mar 2023 12:37:08 +0530 Subject: [PATCH 2/4] add test Signed-off-by: Abhishek Kumar --- .../endpoint/DefaultEndPointSelectorTest.java | 155 ++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java diff --git a/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java b/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java new file mode 100644 index 000000000000..749e2f89feb3 --- /dev/null +++ b/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java @@ -0,0 +1,155 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.cloudstack.storage.endpoint; + +import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore; +import org.apache.cloudstack.engine.subsystem.api.storage.Scope; +import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo; +import org.apache.cloudstack.storage.image.datastore.ImageStoreInfo; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; + +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; +import com.cloud.storage.ScopeType; +import com.cloud.vm.VirtualMachine; + +public class DefaultEndPointSelectorTest { + + @Mock + HostDao hostDao; + + @Before + public void initMocks() { + MockitoAnnotations.initMocks(this); + } + @InjectMocks + private DefaultEndPointSelector defaultEndPointSelector = new DefaultEndPointSelector(); + + @Test + public void testGetVmwareHostFromVolumeToDeleteBasic() { + VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class); + Mockito.when(volumeInfo.getAttachedVM()).thenReturn(null); + Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + + VirtualMachine virtualMachine = Mockito.mock(VirtualMachine.class); + Mockito.when(virtualMachine.getHostId()).thenReturn(null); + Mockito.when(virtualMachine.getLastHostId()).thenReturn(null); + Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); + Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + + Long hostId = 1L; + Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); + Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); + Mockito.when(hostDao.findById(hostId)).thenReturn(null); + Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + } + + @Test + public void testGetVmwareHostFromVolumeToDeleteNotPrimary() { + VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class); + Long hostId = 1L; + VirtualMachine virtualMachine = Mockito.mock(VirtualMachine.class); + Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); + Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); + Mockito.when(hostDao.findById(hostId)).thenReturn(Mockito.mock(HostVO.class)); + Mockito.when(volumeInfo.getDataStore()).thenReturn(Mockito.mock(ImageStoreInfo.class)); + Assert.assertNotNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + } + + @Test + public void testGetVmwareHostFromVolumeToDeletePrimaryNoScope() { + VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class); + Long hostId = 1L; + VirtualMachine virtualMachine = Mockito.mock(VirtualMachine.class); + Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); + Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); + Mockito.when(hostDao.findById(hostId)).thenReturn(Mockito.mock(HostVO.class)); + PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class); + Mockito.when(store.getScope()).thenReturn(null); + Mockito.when(volumeInfo.getDataStore()).thenReturn(store); + Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + } + + @Test + public void testGetVmwareHostFromVolumeToDeletePrimaryNotHostOrClusterScope() { + VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class); + Long hostId = 1L; + VirtualMachine virtualMachine = Mockito.mock(VirtualMachine.class); + Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); + Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); + Mockito.when(hostDao.findById(hostId)).thenReturn(Mockito.mock(HostVO.class)); + PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class); + Scope scope = Mockito.mock(Scope.class); + Mockito.when(scope.getScopeType()).thenReturn(ScopeType.ZONE); + Mockito.when(store.getScope()).thenReturn(scope); + Mockito.when(volumeInfo.getDataStore()).thenReturn(store); + Assert.assertNotNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + } + + @Test + public void testGetVmwareHostFromVolumeToDeletePrimaryClusterScope() { + VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class); + Long hostId = 1L; + Long clusterId = 1L; + VirtualMachine virtualMachine = Mockito.mock(VirtualMachine.class); + Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); + Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); + HostVO hostVO = Mockito.mock(HostVO.class); + Mockito.when(hostVO.getClusterId()).thenReturn(clusterId); + Mockito.when(hostDao.findById(hostId)).thenReturn(hostVO); + PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class); + Scope scope = Mockito.mock(Scope.class); + Mockito.when(scope.getScopeType()).thenReturn(ScopeType.CLUSTER); + Mockito.when(store.getScope()).thenReturn(scope); + Mockito.when(store.getClusterId()).thenReturn(clusterId); + Mockito.when(volumeInfo.getDataStore()).thenReturn(store); + Assert.assertNotNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + + Mockito.when(store.getClusterId()).thenReturn(2L); + Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + } + + @Test + public void testGetVmwareHostFromVolumeToDeletePrimaryHostScope() { + VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class); + Long hostId = 1L; + String storageIp = "something"; + VirtualMachine virtualMachine = Mockito.mock(VirtualMachine.class); + Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); + Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); + HostVO hostVO = Mockito.mock(HostVO.class); + Mockito.when(hostVO.getStorageIpAddress()).thenReturn(storageIp); + Mockito.when(hostDao.findById(hostId)).thenReturn(hostVO); + PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class); + Scope scope = Mockito.mock(Scope.class); + Mockito.when(scope.getScopeType()).thenReturn(ScopeType.HOST); + Mockito.when(store.getScope()).thenReturn(scope); + Mockito.when(store.getHostAddress()).thenReturn(storageIp); + Mockito.when(volumeInfo.getDataStore()).thenReturn(store); + Assert.assertNotNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + + Mockito.when(store.getHostAddress()).thenReturn("something-else"); + Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + } +} \ No newline at end of file From 166e461b8418e5dc97981e8ce7fe540b68f072f6 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 6 Mar 2023 12:42:06 +0530 Subject: [PATCH 3/4] newline Signed-off-by: Abhishek Kumar --- .../storage/endpoint/DefaultEndPointSelectorTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java b/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java index 749e2f89feb3..549dbb569dc9 100644 --- a/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java +++ b/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java @@ -152,4 +152,4 @@ public void testGetVmwareHostFromVolumeToDeletePrimaryHostScope() { Mockito.when(store.getHostAddress()).thenReturn("something-else"); Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); } -} \ No newline at end of file +} From d8ca07b57a6a5cd3ef7e6d967729d5448ab62797 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 6 Mar 2023 15:26:03 +0530 Subject: [PATCH 4/4] consider only enabled and up host Signed-off-by: Abhishek Kumar --- .../endpoint/DefaultEndPointSelector.java | 3 +- .../endpoint/DefaultEndPointSelectorTest.java | 30 +++++++++++++++++-- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java b/engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java index 703d7a9e4bd3..7f5e4996f55b 100644 --- a/engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java +++ b/engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java @@ -54,6 +54,7 @@ import com.cloud.host.Status; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor; +import com.cloud.resource.ResourceState; import com.cloud.storage.DataStoreRole; import com.cloud.storage.ScopeType; import com.cloud.storage.Storage.TemplateType; @@ -329,7 +330,7 @@ protected Host getVmwareHostFromVolumeToDelete(VolumeInfo volume) { return null; } HostVO host = hostDao.findById(hostId); - if (host == null) { + if (host == null || !ResourceState.Enabled.equals(host.getResourceState()) || !Status.Up.equals(host.getState())) { return null; } if (volume.getDataStore() instanceof PrimaryDataStore) { diff --git a/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java b/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java index 549dbb569dc9..ab7cced831bd 100644 --- a/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java +++ b/engine/storage/src/test/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java @@ -30,7 +30,9 @@ import org.mockito.MockitoAnnotations; import com.cloud.host.HostVO; +import com.cloud.host.Status; import com.cloud.host.dao.HostDao; +import com.cloud.resource.ResourceState; import com.cloud.storage.ScopeType; import com.cloud.vm.VirtualMachine; @@ -63,6 +65,15 @@ public void testGetVmwareHostFromVolumeToDeleteBasic() { Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); Mockito.when(hostDao.findById(hostId)).thenReturn(null); Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + + HostVO hostVO = Mockito.mock(HostVO.class); + Mockito.when(hostVO.getResourceState()).thenReturn(ResourceState.Disabled); + Mockito.when(hostDao.findById(hostId)).thenReturn(hostVO); + Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); + + Mockito.when(hostVO.getResourceState()).thenReturn(ResourceState.Enabled); + Mockito.when(hostVO.getState()).thenReturn(Status.Down); + Assert.assertNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); } @Test @@ -72,7 +83,10 @@ public void testGetVmwareHostFromVolumeToDeleteNotPrimary() { VirtualMachine virtualMachine = Mockito.mock(VirtualMachine.class); Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); - Mockito.when(hostDao.findById(hostId)).thenReturn(Mockito.mock(HostVO.class)); + HostVO hostVO = Mockito.mock(HostVO.class); + Mockito.when(hostVO.getResourceState()).thenReturn(ResourceState.Enabled); + Mockito.when(hostVO.getState()).thenReturn(Status.Up); + Mockito.when(hostDao.findById(hostId)).thenReturn(hostVO); Mockito.when(volumeInfo.getDataStore()).thenReturn(Mockito.mock(ImageStoreInfo.class)); Assert.assertNotNull(defaultEndPointSelector.getVmwareHostFromVolumeToDelete(volumeInfo)); } @@ -84,7 +98,10 @@ public void testGetVmwareHostFromVolumeToDeletePrimaryNoScope() { VirtualMachine virtualMachine = Mockito.mock(VirtualMachine.class); Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); - Mockito.when(hostDao.findById(hostId)).thenReturn(Mockito.mock(HostVO.class)); + HostVO hostVO = Mockito.mock(HostVO.class); + Mockito.when(hostVO.getResourceState()).thenReturn(ResourceState.Enabled); + Mockito.when(hostVO.getState()).thenReturn(Status.Up); + Mockito.when(hostDao.findById(hostId)).thenReturn(hostVO); PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class); Mockito.when(store.getScope()).thenReturn(null); Mockito.when(volumeInfo.getDataStore()).thenReturn(store); @@ -98,7 +115,10 @@ public void testGetVmwareHostFromVolumeToDeletePrimaryNotHostOrClusterScope() { VirtualMachine virtualMachine = Mockito.mock(VirtualMachine.class); Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); - Mockito.when(hostDao.findById(hostId)).thenReturn(Mockito.mock(HostVO.class)); + HostVO hostVO = Mockito.mock(HostVO.class); + Mockito.when(hostVO.getResourceState()).thenReturn(ResourceState.Enabled); + Mockito.when(hostVO.getState()).thenReturn(Status.Up); + Mockito.when(hostDao.findById(hostId)).thenReturn(hostVO); PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class); Scope scope = Mockito.mock(Scope.class); Mockito.when(scope.getScopeType()).thenReturn(ScopeType.ZONE); @@ -116,6 +136,8 @@ public void testGetVmwareHostFromVolumeToDeletePrimaryClusterScope() { Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); HostVO hostVO = Mockito.mock(HostVO.class); + Mockito.when(hostVO.getResourceState()).thenReturn(ResourceState.Enabled); + Mockito.when(hostVO.getState()).thenReturn(Status.Up); Mockito.when(hostVO.getClusterId()).thenReturn(clusterId); Mockito.when(hostDao.findById(hostId)).thenReturn(hostVO); PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class); @@ -139,6 +161,8 @@ public void testGetVmwareHostFromVolumeToDeletePrimaryHostScope() { Mockito.when(virtualMachine.getHostId()).thenReturn(hostId); Mockito.when(volumeInfo.getAttachedVM()).thenReturn(virtualMachine); HostVO hostVO = Mockito.mock(HostVO.class); + Mockito.when(hostVO.getResourceState()).thenReturn(ResourceState.Enabled); + Mockito.when(hostVO.getState()).thenReturn(Status.Up); Mockito.when(hostVO.getStorageIpAddress()).thenReturn(storageIp); Mockito.when(hostDao.findById(hostId)).thenReturn(hostVO); PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class);