Skip to content

Commit ab790c1

Browse files
GabrielBrascherGabrielBrascher
authored
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
1 parent 72f6612 commit ab790c1

4 files changed

Lines changed: 104 additions & 18 deletions

File tree

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,5 +257,11 @@ static String getHypervisorHostname(String name) {
257257

258258
UserVm restoreVirtualMachine(long vmId, Long newTemplateId) throws ResourceUnavailableException, InsufficientCapacityException;
259259

260+
/**
261+
* Returns true if the VM's Root volume is allocated at a local storage pool
262+
*/
263+
boolean isRootVolumeOnLocalStorage(long vmId);
264+
260265
Pair<Long, Long> findClusterAndHostIdForVm(long vmId);
266+
261267
}

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

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3632,22 +3632,7 @@ public void checkIfCanUpgrade(final VirtualMachine vmInstance, final ServiceOffe
36323632

36333633
final ServiceOfferingVO currentServiceOffering = _offeringDao.findByIdIncludingRemoved(vmInstance.getId(), vmInstance.getServiceOfferingId());
36343634

3635-
// Check that the service offering being upgraded to has the same Guest IP type as the VM's current service offering
3636-
// NOTE: With the new network refactoring in 2.2, we shouldn't need the check for same guest IP type anymore.
3637-
/*
3638-
* if (!currentServiceOffering.getGuestIpType().equals(newServiceOffering.getGuestIpType())) { String errorMsg =
3639-
* "The service offering being upgraded to has a guest IP type: " + newServiceOffering.getGuestIpType(); errorMsg +=
3640-
* ". Please select a service offering with the same guest IP type as the VM's current service offering (" +
3641-
* currentServiceOffering.getGuestIpType() + ")."; throw new InvalidParameterValueException(errorMsg); }
3642-
*/
3643-
3644-
// Check that the service offering being upgraded to has the same storage pool preference as the VM's current service
3645-
// offering
3646-
if (currentServiceOffering.isUseLocalStorage() != newServiceOffering.isUseLocalStorage()) {
3647-
throw new InvalidParameterValueException("Unable to upgrade virtual machine " + vmInstance.toString() +
3648-
", cannot switch between local storage and shared storage service offerings. Current offering " + "useLocalStorage=" +
3649-
currentServiceOffering.isUseLocalStorage() + ", target offering useLocalStorage=" + newServiceOffering.isUseLocalStorage());
3650-
}
3635+
checkIfNewOfferingStorageScopeMatchesStoragePool(vmInstance, newServiceOffering);
36513636

36523637
// 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
36533638
if (currentServiceOffering.isSystemUse() != newServiceOffering.isSystemUse()) {
@@ -3669,6 +3654,39 @@ public void checkIfCanUpgrade(final VirtualMachine vmInstance, final ServiceOffe
36693654
}
36703655
}
36713656

3657+
/**
3658+
* Throws an InvalidParameterValueException in case the new service offerings does not match the storage scope (e.g. local or shared).
3659+
*/
3660+
protected void checkIfNewOfferingStorageScopeMatchesStoragePool(VirtualMachine vmInstance, ServiceOffering newServiceOffering) {
3661+
boolean isRootVolumeOnLocalStorage = isRootVolumeOnLocalStorage(vmInstance.getId());
3662+
3663+
if (newServiceOffering.isUseLocalStorage() && !isRootVolumeOnLocalStorage) {
3664+
String message = String .format("Unable to upgrade virtual machine %s, target offering use local storage but the storage pool where "
3665+
+ "the volume is allocated is a shared storage.", vmInstance.toString());
3666+
throw new InvalidParameterValueException(message);
3667+
}
3668+
3669+
if (!newServiceOffering.isUseLocalStorage() && isRootVolumeOnLocalStorage) {
3670+
String message = String.format("Unable to upgrade virtual machine %s, target offering use shared storage but the storage pool where "
3671+
+ "the volume is allocated is a local storage.", vmInstance.toString());
3672+
throw new InvalidParameterValueException(message);
3673+
}
3674+
}
3675+
3676+
public boolean isRootVolumeOnLocalStorage(long vmId) {
3677+
ScopeType poolScope = ScopeType.ZONE;
3678+
List<VolumeVO> volumes = _volsDao.findByInstanceAndType(vmId, Type.ROOT);
3679+
if(CollectionUtils.isNotEmpty(volumes)) {
3680+
VolumeVO rootDisk = volumes.get(0);
3681+
Long poolId = rootDisk.getPoolId();
3682+
if (poolId != null) {
3683+
StoragePoolVO storagePoolVO = _storagePoolDao.findById(poolId);
3684+
poolScope = storagePoolVO.getScope();
3685+
}
3686+
}
3687+
return ScopeType.HOST == poolScope;
3688+
}
3689+
36723690
@Override
36733691
public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffering, ServiceOffering currentServiceOffering) {
36743692

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.util.List;
3232
import java.util.Map;
3333

34+
import com.cloud.exception.InvalidParameterValueException;
3435
import org.apache.cloudstack.engine.subsystem.api.storage.StoragePoolAllocator;
3536
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
3637
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
@@ -623,4 +624,59 @@ public void matchesOfSorts() {
623624
assertTrue(VirtualMachineManagerImpl.matches(tags,three));
624625
assertTrue(VirtualMachineManagerImpl.matches(others,three));
625626
}
627+
628+
@Test
629+
public void isRootVolumeOnLocalStorageTestOnLocal() {
630+
prepareAndTestIsRootVolumeOnLocalStorage(ScopeType.HOST, true);
631+
}
632+
633+
@Test
634+
public void isRootVolumeOnLocalStorageTestCluster() {
635+
prepareAndTestIsRootVolumeOnLocalStorage(ScopeType.CLUSTER, false);
636+
}
637+
638+
@Test
639+
public void isRootVolumeOnLocalStorageTestZone() {
640+
prepareAndTestIsRootVolumeOnLocalStorage(ScopeType.ZONE, false);
641+
}
642+
643+
private void prepareAndTestIsRootVolumeOnLocalStorage(ScopeType scope, boolean expected) {
644+
StoragePoolVO storagePoolVoMock = Mockito.mock(StoragePoolVO.class);
645+
Mockito.doReturn(storagePoolVoMock).when(storagePoolDaoMock).findById(Mockito.anyLong());
646+
Mockito.doReturn(scope).when(storagePoolVoMock).getScope();
647+
List<VolumeVO> mockedVolumes = new ArrayList<>();
648+
mockedVolumes.add(volumeVoMock);
649+
Mockito.doReturn(mockedVolumes).when(volumeDaoMock).findByInstanceAndType(Mockito.anyLong(), Mockito.any());
650+
651+
boolean result = virtualMachineManagerImpl.isRootVolumeOnLocalStorage(0l);
652+
653+
Assert.assertEquals(expected, result);
654+
}
655+
656+
@Test
657+
public void checkIfNewOfferingStorageScopeMatchesStoragePoolTestLocalLocal() {
658+
prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(true, true);
659+
}
660+
661+
@Test
662+
public void checkIfNewOfferingStorageScopeMatchesStoragePoolTestSharedShared() {
663+
prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(false, false);
664+
}
665+
666+
@Test (expected = InvalidParameterValueException.class)
667+
public void checkIfNewOfferingStorageScopeMatchesStoragePoolTestLocalShared() {
668+
prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(true, false);
669+
}
670+
671+
@Test (expected = InvalidParameterValueException.class)
672+
public void checkIfNewOfferingStorageScopeMatchesStoragePoolTestSharedLocal() {
673+
prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(false, true);
674+
}
675+
676+
private void prepareAndRunCheckIfNewOfferingStorageScopeMatchesStoragePool(boolean isRootOnLocal, boolean isOfferingUsingLocal) {
677+
Mockito.doReturn(isRootOnLocal).when(virtualMachineManagerImpl).isRootVolumeOnLocalStorage(Mockito.anyLong());
678+
Mockito.doReturn("vmInstanceMockedToString").when(vmInstanceMock).toString();
679+
Mockito.doReturn(isOfferingUsingLocal).when(serviceOfferingMock).isUseLocalStorage();
680+
virtualMachineManagerImpl.checkIfNewOfferingStorageScopeMatchesStoragePool(vmInstanceMock, serviceOfferingMock);
681+
}
626682
}

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import javax.inject.Inject;
3333

3434
import com.cloud.storage.dao.VMTemplateDetailsDao;
35+
import com.cloud.vm.VirtualMachineManager;
3536
import org.apache.cloudstack.acl.ControlledEntity.ACLType;
3637
import org.apache.cloudstack.affinity.AffinityGroupDomainMapVO;
3738
import org.apache.cloudstack.affinity.AffinityGroupResponse;
@@ -424,6 +425,9 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase implements Q
424425
@Inject
425426
private UserDao userDao;
426427

428+
@Inject
429+
private VirtualMachineManager virtualMachineManager;
430+
427431
/*
428432
* (non-Javadoc)
429433
*
@@ -2959,8 +2963,10 @@ private Pair<List<ServiceOfferingJoinVO>, Integer> searchForServiceOfferingsInte
29592963
sc.addAnd("id", SearchCriteria.Op.NEQ, currentVmOffering.getId());
29602964
}
29612965

2962-
// 1. Only return offerings with the same storage type
2963-
sc.addAnd("useLocalStorage", SearchCriteria.Op.EQ, currentVmOffering.isUseLocalStorage());
2966+
boolean isRootVolumeUsingLocalStorage = virtualMachineManager.isRootVolumeOnLocalStorage(vmId);
2967+
2968+
// 1. Only return offerings with the same storage type than the storage pool where the VM's root volume is allocated
2969+
sc.addAnd("useLocalStorage", SearchCriteria.Op.EQ, isRootVolumeUsingLocalStorage);
29642970

29652971
// 2.In case vm is running return only offerings greater than equal to current offering compute.
29662972
if (vmInstance.getState() == VirtualMachine.State.Running) {

0 commit comments

Comments
 (0)