From de6ce503dcef9c11fa88770445b20361956871d4 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 3 Jul 2023 12:57:14 +0530 Subject: [PATCH 1/3] api: correct error on resize volume resource allocation failure (#7687) This PR resource throws exception with the correct error code and logs the error message when a resource allocation failure is encountered during resize volume operation. Signed-off-by: Abhishek Kumar --- .../cloudstack/api/command/user/volume/ResizeVolumeCmd.java | 5 ++++- .../src/main/java/com/cloud/api/ApiAsyncJobDispatcher.java | 4 +--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ResizeVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ResizeVolumeCmd.java index 6ad512ad29a..0daf141ba4a 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ResizeVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ResizeVolumeCmd.java @@ -184,7 +184,7 @@ public class ResizeVolumeCmd extends BaseAsyncCmd implements UserCmd { } @Override - public void execute() throws ResourceAllocationException { + public void execute() { Volume volume = null; try { if (size != null) { @@ -194,6 +194,9 @@ public class ResizeVolumeCmd extends BaseAsyncCmd implements UserCmd { } volume = _volumeService.resizeVolume(this); + } catch (ResourceAllocationException ex) { + s_logger.error(ex.getMessage()); + throw new ServerApiException(ApiErrorCode.RESOURCE_ALLOCATION_ERROR, ex.getMessage()); } catch (InvalidParameterValueException ex) { s_logger.info(ex.getMessage()); throw new ServerApiException(ApiErrorCode.UNSUPPORTED_ACTION_ERROR, ex.getMessage()); diff --git a/server/src/main/java/com/cloud/api/ApiAsyncJobDispatcher.java b/server/src/main/java/com/cloud/api/ApiAsyncJobDispatcher.java index 196f640252a..b596254994c 100644 --- a/server/src/main/java/com/cloud/api/ApiAsyncJobDispatcher.java +++ b/server/src/main/java/com/cloud/api/ApiAsyncJobDispatcher.java @@ -129,9 +129,7 @@ public class ApiAsyncJobDispatcher extends AdapterBase implements AsyncJobDispat response.setErrorText(errorMsg); response.setResponseName((cmdObj == null) ? "unknowncommandresponse" : cmdObj.getCommandName()); - // FIXME: setting resultCode to ApiErrorCode.INTERNAL_ERROR is not right, usually executors have their exception handling - // and we need to preserve that as much as possible here - _asyncJobMgr.completeAsyncJob(job.getId(), JobInfo.Status.FAILED, ApiErrorCode.INTERNAL_ERROR.getHttpCode(), ApiSerializerHelper.toSerializedString(response)); + _asyncJobMgr.completeAsyncJob(job.getId(), JobInfo.Status.FAILED, errorCode, ApiSerializerHelper.toSerializedString(response)); } } } From 70820137e642e026b20773214684fc2af816b856 Mon Sep 17 00:00:00 2001 From: Harikrishna Date: Mon, 3 Jul 2023 12:58:42 +0530 Subject: [PATCH 2/3] scaleio: Avoid race condition while handling host disconnect and connect scenarios (#282) (#7689) This PR fixes an intermittent issue where SDC id (local_path) is getting deleted and not getting populated when host connects back again. Fix is to remove the code to delete the records from storage_pool_host_ref table. We are anyways updating the entry if the SDC ID is changed during agent restart which is anyways required inorder to get the new connections. I've quickly verified the host delete scenario to check the storage_pool_host_ref entries behavior, entries are getting deleted. --- .../storage/datastore/provider/ScaleIOHostListener.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java b/plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java index 68044baf170..bb269e85a95 100644 --- a/plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java +++ b/plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java @@ -171,11 +171,7 @@ public class ScaleIOHostListener implements HypervisorHostListener { @Override public boolean hostDisconnected(long hostId, long poolId) { - StoragePoolHostVO storagePoolHost = _storagePoolHostDao.findByPoolHost(poolId, hostId); - if (storagePoolHost != null) { - _storagePoolHostDao.deleteStoragePoolHostDetails(hostId, poolId); - } - + // SDC ID is getting updated upon host connect, no need to delete the storage_pool_host_ref entry return true; } From 31dbdd0f5c0a0eb33e71fa7e7bef0b8b74f408b3 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 3 Jul 2023 13:00:32 +0530 Subject: [PATCH 3/3] engine-orchestration: fix volume size resource count mismatch (#7666) Fixes case of account/domain having negative storage count when there is a volume size difference while deploying volumes on certain stroages. In case of Powerflex volume size results in a multiple of 8. If user deploys a volume of 12GB it will result in 16GB in size. But currently CloudStack will not update resource count after deployment. Signed-off-by: Abhishek Kumar --- .../orchestration/VolumeOrchestrator.java | 23 +++- .../orchestration/VolumeOrchestratorTest.java | 103 ++++++++++++++++++ 2 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 49c822d25ad..2aa997e13aa 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -28,6 +28,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.UUID; @@ -83,6 +84,7 @@ import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import org.apache.commons.collections.CollectionUtils; import org.apache.commons.collections.MapUtils; +import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import org.jetbrains.annotations.Nullable; @@ -1656,6 +1658,23 @@ public class VolumeOrchestrator extends ManagerBase implements VolumeOrchestrati return tasks; } + protected void checkAndUpdateVolumeAccountResourceCount(VolumeVO originalEntry, VolumeVO updateEntry) { + if (Objects.equals(originalEntry.getSize(), updateEntry.getSize())) { + return; + } + s_logger.debug(String.format("Size mismatch found for %s after creation, old size: %d, new size: %d. Updating resource count", updateEntry, originalEntry.getSize(), updateEntry.getSize())); + if (ObjectUtils.anyNull(originalEntry.getSize(), updateEntry.getSize())) { + _resourceLimitMgr.recalculateResourceCount(updateEntry.getAccountId(), updateEntry.getDomainId(), + ResourceType.primary_storage.getOrdinal()); + return; + } + if (updateEntry.getSize() > originalEntry.getSize()) { + _resourceLimitMgr.incrementResourceCount(updateEntry.getAccountId(), ResourceType.primary_storage, updateEntry.isDisplayVolume(), updateEntry.getSize() - originalEntry.getSize()); + } else { + _resourceLimitMgr.decrementResourceCount(updateEntry.getAccountId(), ResourceType.primary_storage, updateEntry.isDisplayVolume(), originalEntry.getSize() - updateEntry.getSize()); + } + } + private Pair recreateVolume(VolumeVO vol, VirtualMachineProfile vm, DeployDestination dest) throws StorageUnavailableException, StorageAccessException { String volToString = getReflectOnlySelectedFields(vol); @@ -1793,8 +1812,8 @@ public class VolumeOrchestrator extends ManagerBase implements VolumeOrchestrati throw new StorageUnavailableException(msg, destPool.getId()); } } - - return new Pair(newVol, destPool); + checkAndUpdateVolumeAccountResourceCount(vol, newVol); + return new Pair<>(newVol, destPool); } private VolumeVO setPassphraseForVolumeEncryption(VolumeVO volume) { diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java new file mode 100644 index 00000000000..b2f4c436ca3 --- /dev/null +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java @@ -0,0 +1,103 @@ +// 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.engine.orchestration; + +import java.util.ArrayList; + +import org.apache.commons.lang3.ObjectUtils; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.stubbing.Answer; + +import com.cloud.configuration.Resource; +import com.cloud.storage.VolumeVO; +import com.cloud.user.ResourceLimitService; + +@RunWith(MockitoJUnitRunner.class) +public class VolumeOrchestratorTest { + + @Mock + protected ResourceLimitService resourceLimitMgr; + + @Spy + @InjectMocks + private VolumeOrchestrator volumeOrchestrator = new VolumeOrchestrator(); + + private static final Long DEFAULT_ACCOUNT_PS_RESOURCE_COUNT = 100L; + private Long accountPSResourceCount; + + @Before + public void setUp() throws Exception { + accountPSResourceCount = DEFAULT_ACCOUNT_PS_RESOURCE_COUNT; + Mockito.when(resourceLimitMgr.recalculateResourceCount(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyInt())).thenReturn(new ArrayList<>()); + Mockito.doAnswer((Answer) invocation -> { + Resource.ResourceType type = (Resource.ResourceType)invocation.getArguments()[1]; + Long increment = (Long)invocation.getArguments()[3]; + if (Resource.ResourceType.primary_storage.equals(type)) { + accountPSResourceCount += increment; + } + return null; + }).when(resourceLimitMgr).incrementResourceCount(Mockito.anyLong(), Mockito.any(Resource.ResourceType.class), Mockito.anyBoolean(), Mockito.anyLong()); + Mockito.doAnswer((Answer) invocation -> { + Resource.ResourceType type = (Resource.ResourceType)invocation.getArguments()[1]; + Long decrement = (Long)invocation.getArguments()[3]; + if (Resource.ResourceType.primary_storage.equals(type)) { + accountPSResourceCount -= decrement; + } + return null; + }).when(resourceLimitMgr).decrementResourceCount(Mockito.anyLong(), Mockito.any(Resource.ResourceType.class), Mockito.anyBoolean(), Mockito.anyLong()); + } + + private void runCheckAndUpdateVolumeAccountResourceCountTest(Long originalSize, Long newSize) { + VolumeVO v1 = Mockito.mock(VolumeVO.class); + Mockito.when(v1.getSize()).thenReturn(originalSize); + VolumeVO v2 = Mockito.mock(VolumeVO.class); + Mockito.when(v2.getSize()).thenReturn(newSize); + volumeOrchestrator.checkAndUpdateVolumeAccountResourceCount(v1, v2); + Long expected = ObjectUtils.anyNull(originalSize, newSize) ? + DEFAULT_ACCOUNT_PS_RESOURCE_COUNT : DEFAULT_ACCOUNT_PS_RESOURCE_COUNT + (newSize - originalSize); + Assert.assertEquals(expected, accountPSResourceCount); + } + + @Test + public void testCheckAndUpdateVolumeAccountResourceCountSameSize() { + runCheckAndUpdateVolumeAccountResourceCountTest(10L, 10L); + } + + @Test + public void testCheckAndUpdateVolumeAccountResourceCountEitherSizeNull() { + runCheckAndUpdateVolumeAccountResourceCountTest(null, 10L); + runCheckAndUpdateVolumeAccountResourceCountTest(10L, null); + } + + @Test + public void testCheckAndUpdateVolumeAccountResourceCountMoreSize() { + runCheckAndUpdateVolumeAccountResourceCountTest(10L, 20L); + } + + @Test + public void testCheckAndUpdateVolumeAccountResourceCountLessSize() { + runCheckAndUpdateVolumeAccountResourceCountTest(20L, 10L); + } +}