etan39
/
cloudstack
mirror of https://github.com/apache/cloudstack.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Gabriel Beims Bräscher 2021-04-30 03:29:50 -03:00 committed by GitHub
parent 72f6612971
commit ab790c11d5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 104 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java
View File

@ -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);
}

50
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
View File

@ -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) {

56
engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
View File

@ -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);
}
}

10
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java
View File

@ -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) {