This is an automated email from the ASF dual-hosted git repository.
rohit pushed a commit to branch 4.15
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.15 by this push:
new ab790c1 server: Allow to upgrade service offerings from local <>
shared storage pools (#4915)
ab790c1 is described below
commit ab790c11d5663afb4e2da098aab5701a625fb3b1
Author: Gabriel Beims Bräscher <[email protected]>
AuthorDate: Fri Apr 30 03:29:50 2021 -0300
server: Allow to upgrade service offerings from local <> shared storage
pools (#4915)
This PR addresses the issue raised at #4545 (Fail to change Service
offering from local <> shared storage).
When upgrading a VM service offering it is validated if the new offering
has the same storage scope (local or shared) as the current offering. I think
that the validation makes sense in a way of preventing running Root disks with
an offering that does not match the current storage pool. However, the
validation only compares both offerings and does not consider that it is
possible to migrate Volumes between local <> shared storage pools.
The idea behind this implementation is that CloudStack should check the
scope of the current storage pool which the ROOT volume is allocated; this, it
is possible to migrate the volume between storage pools and list/upgrade
according to the offerings that are supported for such pool.
This PR also fixes an issue where the API command that lists offerings for
a VM should follow the same idea and list based on the storage pool that the
volume is allocated and not the previous offering.
Fixes: #4545
---
.../java/com/cloud/vm/VirtualMachineManager.java | 6 +++
.../com/cloud/vm/VirtualMachineManagerImpl.java | 50 ++++++++++++-------
.../cloud/vm/VirtualMachineManagerImplTest.java | 56 ++++++++++++++++++++++
.../java/com/cloud/api/query/QueryManagerImpl.java | 10 +++-
4 files changed, 104 insertions(+), 18 deletions(-)
diff --git a/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java
b/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java
index 40777b38..9670464 100644
--- a/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java
+++ b/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java
@@ -257,5 +257,11 @@ public interface VirtualMachineManager extends Manager {
UserVm restoreVirtualMachine(long vmId, Long newTemplateId) throws
ResourceUnavailableException, InsufficientCapacityException;
+ /**
+ * Returns true if the VM's Root volume is allocated at a local storage
pool
+ */
+ boolean isRootVolumeOnLocalStorage(long vmId);
+
Pair<Long, Long> findClusterAndHostIdForVm(long vmId);
+
}
diff --git
a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
index 765bb90..6967e74 100755
---
a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
+++
b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
@@ -3632,22 +3632,7 @@ public class VirtualMachineManagerImpl extends
ManagerBase implements VirtualMac
final ServiceOfferingVO currentServiceOffering =
_offeringDao.findByIdIncludingRemoved(vmInstance.getId(),
vmInstance.getServiceOfferingId());
- // Check that the service offering being upgraded to has the same
Guest IP type as the VM's current service offering
- // NOTE: With the new network refactoring in 2.2, we shouldn't need
the check for same guest IP type anymore.
- /*
- * if
(!currentServiceOffering.getGuestIpType().equals(newServiceOffering.getGuestIpType()))
{ String errorMsg =
- * "The service offering being upgraded to has a guest IP type: " +
newServiceOffering.getGuestIpType(); errorMsg +=
- * ". Please select a service offering with the same guest IP type as
the VM's current service offering (" +
- * currentServiceOffering.getGuestIpType() + ")."; throw new
InvalidParameterValueException(errorMsg); }
- */
-
- // Check that the service offering being upgraded to has the same
storage pool preference as the VM's current service
- // offering
- if (currentServiceOffering.isUseLocalStorage() !=
newServiceOffering.isUseLocalStorage()) {
- throw new InvalidParameterValueException("Unable to upgrade
virtual machine " + vmInstance.toString() +
- ", cannot switch between local storage and shared storage
service offerings. Current offering " + "useLocalStorage=" +
- currentServiceOffering.isUseLocalStorage() + ", target
offering useLocalStorage=" + newServiceOffering.isUseLocalStorage());
- }
+ checkIfNewOfferingStorageScopeMatchesStoragePool(vmInstance,
newServiceOffering);
// if vm is a system vm, check if it is a system service offering, if
yes return with error as it cannot be used for user vms
if (currentServiceOffering.isSystemUse() !=
newServiceOffering.isSystemUse()) {
@@ -3669,6 +3654,39 @@ public class VirtualMachineManagerImpl extends
ManagerBase implements VirtualMac
}
}
+ /**
+ * Throws an InvalidParameterValueException in case the new service
offerings does not match the storage scope (e.g. local or shared).
+ */
+ protected void
checkIfNewOfferingStorageScopeMatchesStoragePool(VirtualMachine vmInstance,
ServiceOffering newServiceOffering) {
+ boolean isRootVolumeOnLocalStorage =
isRootVolumeOnLocalStorage(vmInstance.getId());
+
+ if (newServiceOffering.isUseLocalStorage() &&
!isRootVolumeOnLocalStorage) {
+ String message = String .format("Unable to upgrade virtual machine
%s, target offering use local storage but the storage pool where "
+ + "the volume is allocated is a shared storage.",
vmInstance.toString());
+ throw new InvalidParameterValueException(message);
+ }
+
+ if (!newServiceOffering.isUseLocalStorage() &&
isRootVolumeOnLocalStorage) {
+ String message = String.format("Unable to upgrade virtual machine
%s, target offering use shared storage but the storage pool where "
+ + "the volume is allocated is a local storage.",
vmInstance.toString());
+ throw new InvalidParameterValueException(message);
+ }
+ }
+
+ public boolean isRootVolumeOnLocalStorage(long vmId) {
+ ScopeType poolScope = ScopeType.ZONE;
+ List<VolumeVO> volumes = _volsDao.findByInstanceAndType(vmId,
Type.ROOT);
+ if(CollectionUtils.isNotEmpty(volumes)) {
+ VolumeVO rootDisk = volumes.get(0);
+ Long poolId = rootDisk.getPoolId();
+ if (poolId != null) {
+ StoragePoolVO storagePoolVO = _storagePoolDao.findById(poolId);
+ poolScope = storagePoolVO.getScope();
+ }
+ }
+ return ScopeType.HOST == poolScope;
+ }
+
@Override
public boolean upgradeVmDb(final long vmId, final ServiceOffering
newServiceOffering, ServiceOffering currentServiceOffering) {
diff --git
a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
index 1725a41..d8df27d 100644
---
a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
+++
b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
@@ -31,6 +31,7 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import com.cloud.exception.InvalidParameterValueException;
import org.apache.cloudstack.engine.subsystem.api.storage.StoragePoolAllocator;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
@@ -623,4 +624,59 @@ public class VirtualMachineManagerImplTest {
assertTrue(VirtualMachineManagerImpl.matches(tags,three));
assertTrue(VirtualMachineManagerImpl.matches(others,three));
}
+
+ @Test
+ public void isRootVolumeOnLocalStorageTestOnLocal() {
+ prepareAndTestIsRootVolumeOnLocalStorage(ScopeType.HOST, true);
+ }
+
+ @Test
+ public void isRootVolumeOnLocalStorageTestCluster() {
+ prepareAndTestIsRootVolumeOnLocalStorage(ScopeType.CLUSTER, false);
+ }
+
+ @Test
+ public void isRootVolumeOnLocalStorageTestZone() {
+ prepareAndTestIsRootVolumeOnLocalStorage(ScopeType.ZONE, false);
+ }
+
+ private void prepareAndTestIsRootVolumeOnLocalStorage(ScopeType scope,
boolean expected) {
+ StoragePoolVO storagePoolVoMock = Mockito.mock(StoragePoolVO.class);
+
Mockito.doReturn(storagePoolVoMock).when(storagePoolDaoMock).findById(Mockito.anyLong());
+ Mockito.doReturn(scope).when(storagePoolVoMock).getScope();
+ List<VolumeVO> mockedVolumes = new ArrayList<>();
+ mockedVolumes.add(volumeVoMock);
+
Mockito.doReturn(mockedVolumes).when(volumeDaoMock).findByInstanceAndType(Mockito.anyLong(),
Mockito.any());
+
+ boolean result =
virtualMachineManagerImpl.isRootVolumeOnLocalStorage(0l);
+
+ Assert.assertEquals(expected, result);
+ }
+
+ @Test
+ public void
checkIfNewOfferingStorageScopeMatchesStoragePoolTestLocalLocal() {
+ prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(true,
true);
+ }
+
+ @Test
+ public void
checkIfNewOfferingStorageScopeMatchesStoragePoolTestSharedShared() {
+ prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(false,
false);
+ }
+
+ @Test (expected = InvalidParameterValueException.class)
+ public void
checkIfNewOfferingStorageScopeMatchesStoragePoolTestLocalShared() {
+ prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(true,
false);
+ }
+
+ @Test (expected = InvalidParameterValueException.class)
+ public void
checkIfNewOfferingStorageScopeMatchesStoragePoolTestSharedLocal() {
+ prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(false,
true);
+ }
+
+ private void
prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(boolean
isRootOnLocal, boolean isOfferingUsingLocal) {
+
Mockito.doReturn(isRootOnLocal).when(virtualMachineManagerImpl).isRootVolumeOnLocalStorage(Mockito.anyLong());
+
Mockito.doReturn("vmInstanceMockedToString").when(vmInstanceMock).toString();
+
Mockito.doReturn(isOfferingUsingLocal).when(serviceOfferingMock).isUseLocalStorage();
+
virtualMachineManagerImpl.checkIfNewOfferingStorageScopeMatchesStoragePool(vmInstanceMock,
serviceOfferingMock);
+ }
}
diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java
b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java
index cf01b8d..2e6bc8a 100644
--- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java
+++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java
@@ -32,6 +32,7 @@ import java.util.stream.Stream;
import javax.inject.Inject;
import com.cloud.storage.dao.VMTemplateDetailsDao;
+import com.cloud.vm.VirtualMachineManager;
import org.apache.cloudstack.acl.ControlledEntity.ACLType;
import org.apache.cloudstack.affinity.AffinityGroupDomainMapVO;
import org.apache.cloudstack.affinity.AffinityGroupResponse;
@@ -424,6 +425,9 @@ public class QueryManagerImpl extends
MutualExclusiveIdsManagerBase implements Q
@Inject
private UserDao userDao;
+ @Inject
+ private VirtualMachineManager virtualMachineManager;
+
/*
* (non-Javadoc)
*
@@ -2959,8 +2963,10 @@ public class QueryManagerImpl extends
MutualExclusiveIdsManagerBase implements Q
sc.addAnd("id", SearchCriteria.Op.NEQ,
currentVmOffering.getId());
}
- // 1. Only return offerings with the same storage type
- sc.addAnd("useLocalStorage", SearchCriteria.Op.EQ,
currentVmOffering.isUseLocalStorage());
+ boolean isRootVolumeUsingLocalStorage =
virtualMachineManager.isRootVolumeOnLocalStorage(vmId);
+
+ // 1. Only return offerings with the same storage type than the
storage pool where the VM's root volume is allocated
+ sc.addAnd("useLocalStorage", SearchCriteria.Op.EQ,
isRootVolumeUsingLocalStorage);
// 2.In case vm is running return only offerings greater than
equal to current offering compute.
if (vmInstance.getState() == VirtualMachine.State.Running) {
