server: Allow to upgrade service offerings from local <> shared storage pools (apache#4915)

This PR addresses the issue raised at apache#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: apache#4545

Author: GabrielBrascher

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

@@ -262,5 +262,11 @@ static String getHypervisorHostname(String name) {
 
     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);
+
 }

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

@@ -3828,22 +3828,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()) {
@@ -3865,6 +3850,39 @@ public void checkIfCanUpgrade(final VirtualMachine vmInstance, final ServiceOffe
         }
     }
 
+    /**
+     * 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) {
 

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;
@@ -624,4 +625,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)
      *
@@ -2962,8 +2966,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) {