Allow upgrading offering with shared storage or local storage if the volume is already allocated on the storage that matches with new offering type.

This is done by changing the validation in a way that it checks if new offering matches the current volume's storage pool as a shared or local.

Return Service Offerings with the same storage type (local vs shared) than the storage pool where the VM's root volume is allocated

Author: GabrielBrascher

engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java

@@ -255,4 +255,9 @@ static String getHypervisorHostname(String name) {
     boolean unmanage(String vmUuid);
 
     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);
 }

engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java

@@ -3669,22 +3669,7 @@ public void checkIfCanUpgrade(final VirtualMachine vmInstance, final ServiceOffe
 
         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()) {
@@ -3706,6 +3691,34 @@ public void checkIfCanUpgrade(final VirtualMachine vmInstance, final ServiceOffe
         }
     }
 
+    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);
+        } else 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) {
 

engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java

@@ -31,6 +31,7 @@
 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 void matchesOfSorts() {
         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);
+    }
 }

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

@@ -32,6 +32,7 @@
 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 @@ private Pair<List<ServiceOfferingJoinVO>, Integer> searchForServiceOfferingsInte
                 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) {