Merge pull request #1727 from nvazquez/change-serv-offering

CLOUDSTACK-9539: Support changing Service offering for instance with VM Snapshots## Actual behaviour

CloudStack doesn't support changing service offering for vm instances which have vm snapshots, they should be removed before changing service offering.
## Goal

Extend actual behaviour by supporting changing service offering for vms which have vm snapshots. In that case, previously taken snapshots (if reverted) should use previous service offering, future snapshots should use the newest.
## Proposed solution:
1. Adding `service_offering_id` column on `vm_snapshots` table: This way snapshot can be reverted to original state even though service offering can be changed for vm instance.
   NOTE: Existing vm snapshots are populated on update script by:
   `UPDATE vm_snapshots s JOIN vm_instance v ON v.id = s.vm_id SET s.service_offering_id = v.service_offering_id;`
2. New vm snapshots will use instance vm service offering id as `service_offering_id`
3. Revert to vm snapshots should use vm snapshot's `service_offering_id` value.
## Example use case:
- Deploy vm using service offering A
- Take vm snapshot -> snap1 (service offering A)
- Stop vm
- Change vm service offering to B
- Revert to VM snapshot snap 1
- Start vm

It is expected that vm has service offering A after last step

* pr/1727:
  CLOUDSTACK-9539: Support changing Service offering for instance with VM Snapshots

Signed-off-by: Rajani Karuturi <[email protected]>

Author: karuturi

api/src/com/cloud/vm/snapshot/VMSnapshot.java

@@ -102,4 +102,6 @@ enum Event {
 
     @Override
     public long getAccountId();
+
+    public long getServiceOfferingId();
 }

engine/schema/src/com/cloud/vm/snapshot/VMSnapshotVO.java

@@ -72,6 +72,9 @@ public class VMSnapshotVO implements VMSnapshot {
     @Column(name = "domain_id")
     long domainId;
 
+    @Column(name = "service_offering_id")
+    private long serviceOfferingId;
+
     @Column(name = "vm_snapshot_type")
     @Enumerated(EnumType.STRING)
     VMSnapshot.Type type;
@@ -139,6 +142,7 @@ public VMSnapshotVO(Long accountId, Long domainId, Long vmId, String description
         displayName = vsDisplayName;
         this.type = type;
         this.current = current;
+        this.serviceOfferingId = serviceOfferingId;
     }
 
     @Override
@@ -248,4 +252,9 @@ public void setRemoved(Date removed) {
     public Class<?> getEntityType() {
         return VMSnapshot.class;
     }
+
+    @Override
+    public long getServiceOfferingId() {
+        return serviceOfferingId;
+    }
 }

server/src/com/cloud/vm/UserVmManagerImpl.java

@@ -950,12 +950,6 @@ public UserVm upgradeVirtualMachine(UpgradeVMCmd cmd) throws ResourceAllocationE
                     + "; make sure the virtual machine is stopped");
         }
 
-        // If target VM has associated VM snapshots then don't allow upgrading of VM
-        List<VMSnapshotVO> vmSnapshots = _vmSnapshotDao.findByVm(vmId);
-        if (vmSnapshots.size() > 0) {
-            throw new InvalidParameterValueException("Unable to change service offering for VM, please remove VM snapshots before changing service offering of VM");
-        }
-
         _accountMgr.checkAccess(caller, null, true, vmInstance);
 
         // Check resource limits for CPU and Memory.
@@ -1616,11 +1610,6 @@ public boolean upgradeVirtualMachine(Long vmId, Long newServiceOfferingId, Map<S
         VMInstanceVO vmInstance = _vmInstanceDao.findById(vmId);
 
         if (vmInstance != null) {
-            // If target VM has associated VM snapshots then don't allow upgrading of VM
-            List<VMSnapshotVO> vmSnapshots = _vmSnapshotDao.findByVm(vmId);
-            if (vmSnapshots.size() > 0) {
-                throw new InvalidParameterValueException("Unable to scale VM, please remove VM snapshots before scaling VM");
-            }
             if (vmInstance.getState().equals(State.Stopped)) {
                 upgradeStoppedVirtualMachine(vmId, newServiceOfferingId, customParameters);
                 return true;

server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java

@@ -57,12 +57,16 @@
 import com.cloud.exception.ConcurrentOperationException;
 import com.cloud.exception.InsufficientCapacityException;
 import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.ManagementServerException;
 import com.cloud.exception.ResourceAllocationException;
 import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.exception.VirtualMachineMigrationException;
 import com.cloud.gpu.GPU;
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao;
 import com.cloud.projects.Project.ListProjectResourcesCriteria;
+import com.cloud.service.ServiceOfferingVO;
+import com.cloud.service.dao.ServiceOfferingDao;
 import com.cloud.service.dao.ServiceOfferingDetailsDao;
 import com.cloud.storage.GuestOSVO;
 import com.cloud.storage.Snapshot;
@@ -89,7 +93,13 @@
 import com.cloud.utils.db.Filter;
 import com.cloud.utils.db.SearchBuilder;
 import com.cloud.utils.db.SearchCriteria;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallbackWithException;
+import com.cloud.utils.db.TransactionCallbackWithExceptionNoReturn;
+import com.cloud.utils.db.TransactionStatus;
 import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.UserVmDetailVO;
+import com.cloud.vm.UserVmManager;
 import com.cloud.vm.UserVmVO;
 import com.cloud.vm.VMInstanceVO;
 import com.cloud.vm.VirtualMachine;
@@ -102,8 +112,10 @@
 import com.cloud.vm.VmWorkJobHandlerProxy;
 import com.cloud.vm.VmWorkSerializer;
 import com.cloud.vm.dao.UserVmDao;
+import com.cloud.vm.dao.UserVmDetailsDao;
 import com.cloud.vm.dao.VMInstanceDao;
 import com.cloud.vm.snapshot.dao.VMSnapshotDao;
+import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao;
 
 @Component
 public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase implements VMSnapshotManager, VMSnapshotService, VmWorkJobHandler {
@@ -135,6 +147,14 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme
 
     @Inject
     VmWorkJobDao _workJobDao;
+    @Inject
+    protected UserVmManager _userVmManager;
+    @Inject
+    protected ServiceOfferingDao _serviceOfferingDao;
+    @Inject
+    protected UserVmDetailsDao _userVmDetailsDao;
+    @Inject
+    protected VMSnapshotDetailsDao _vmSnapshotDetailsDao;
 
     VmWorkJobHandlerProxy _jobHandlerProxy = new VmWorkJobHandlerProxy(this);
 
@@ -346,21 +366,65 @@ public VMSnapshot allocVMSnapshot(Long vmId, String vsDisplayName, String vsDesc
             vmSnapshotType = VMSnapshot.Type.DiskAndMemory;
 
         try {
-            VMSnapshotVO vmSnapshotVo =
-                new VMSnapshotVO(userVmVo.getAccountId(), userVmVo.getDomainId(), vmId, vsDescription, vmSnapshotName, vsDisplayName, userVmVo.getServiceOfferingId(),
-                    vmSnapshotType, null);
-            VMSnapshot vmSnapshot = _vmSnapshotDao.persist(vmSnapshotVo);
-            if (vmSnapshot == null) {
-                throw new CloudRuntimeException("Failed to create snapshot for vm: " + vmId);
-            }
-            return vmSnapshot;
+            return createAndPersistVMSnapshot(userVmVo, vsDescription, vmSnapshotName, vsDisplayName, vmSnapshotType);
         } catch (Exception e) {
             String msg = e.getMessage();
             s_logger.error("Create vm snapshot record failed for vm: " + vmId + " due to: " + msg);
         }
         return null;
     }
 
+    /**
+     * Create, persist and return vm snapshot for userVmVo with given parameters.
+     * Persistence and support for custom service offerings are done on the same transaction
+     * @param userVmVo user vm
+     * @param vmId vm id
+     * @param vsDescription vm description
+     * @param vmSnapshotName vm snapshot name
+     * @param vsDisplayName vm snapshot display name
+     * @param vmSnapshotType vm snapshot type
+     * @return vm snapshot
+     * @throws CloudRuntimeException if vm snapshot couldn't be persisted
+     */
+    protected VMSnapshot createAndPersistVMSnapshot(UserVmVO userVmVo, String vsDescription, String vmSnapshotName, String vsDisplayName, VMSnapshot.Type vmSnapshotType) {
+        final Long vmId = userVmVo.getId();
+        final Long serviceOfferingId = userVmVo.getServiceOfferingId();
+        final VMSnapshotVO vmSnapshotVo =
+                new VMSnapshotVO(userVmVo.getAccountId(), userVmVo.getDomainId(), vmId, vsDescription, vmSnapshotName, vsDisplayName, serviceOfferingId,
+                        vmSnapshotType, null);
+        return Transaction.execute(new TransactionCallbackWithException<VMSnapshot, CloudRuntimeException>() {
+            @Override
+            public VMSnapshot doInTransaction(TransactionStatus status) {
+                VMSnapshot vmSnapshot = _vmSnapshotDao.persist(vmSnapshotVo);
+                if (vmSnapshot == null) {
+                    throw new CloudRuntimeException("Failed to create snapshot for vm: " + vmId);
+                }
+                addSupportForCustomServiceOffering(vmId, serviceOfferingId, vmSnapshot.getId());
+                return vmSnapshot;
+            }
+        });
+    }
+
+    /**
+     * Add entries on vm_snapshot_details if service offering is dynamic. This will allow setting details when revert to vm snapshot
+     * @param vmId vm id
+     * @param serviceOfferingId service offering id
+     * @param vmSnapshotId vm snapshot id
+     */
+    protected void addSupportForCustomServiceOffering(long vmId, long serviceOfferingId, long vmSnapshotId) {
+        ServiceOfferingVO serviceOfferingVO = _serviceOfferingDao.findById(serviceOfferingId);
+        if (serviceOfferingVO.isDynamic()) {
+            List<UserVmDetailVO> vmDetails = _userVmDetailsDao.listDetails(vmId);
+            List<VMSnapshotDetailsVO> vmSnapshotDetails = new ArrayList<VMSnapshotDetailsVO>();
+            for (UserVmDetailVO detail : vmDetails) {
+                if(detail.getName().equalsIgnoreCase("cpuNumber") || detail.getName().equalsIgnoreCase("cpuSpeed") || detail.getName().equalsIgnoreCase("memory")) {
+                    vmSnapshotDetails.add(new VMSnapshotDetailsVO(vmSnapshotId, detail.getName(), detail.getValue(), detail.isDisplay()));
+                }
+            }
+            _vmSnapshotDetailsDao.saveDetails(vmSnapshotDetails);
+        }
+    }
+
     @Override
     public String getName() {
         return _name;
@@ -654,16 +718,76 @@ else if (jobResult instanceof Throwable)
         }
     }
 
+    /**
+     * If snapshot was taken with a different service offering than actual used in vm, should change it back to it
+     * @param userVm vm to change service offering (if necessary)
+     * @param vmSnapshotVo vm snapshot
+     */
+    protected void updateUserVmServiceOffering(UserVm userVm, VMSnapshotVO vmSnapshotVo) {
+        if (vmSnapshotVo.getServiceOfferingId() != userVm.getServiceOfferingId()) {
+            changeUserVmServiceOffering(userVm, vmSnapshotVo);
+        }
+    }
+
+    /**
+     * Get user vm details as a map
+     * @param userVm user vm
+     * @return map
+     */
+    protected Map<String, String> getVmMapDetails(UserVm userVm) {
+        List<UserVmDetailVO> userVmDetails = _userVmDetailsDao.listDetails(userVm.getId());
+        Map<String, String> details = new HashMap<String, String>();
+        for (UserVmDetailVO detail : userVmDetails) {
+            details.put(detail.getName(), detail.getValue());
+        }
+        return details;
+    }
+
+    /**
+     * Update service offering on {@link userVm} to the one specified in {@link vmSnapshotVo}
+     * @param userVm user vm to be updated
+     * @param vmSnapshotVo vm snapshot
+     */
+    protected void changeUserVmServiceOffering(UserVm userVm, VMSnapshotVO vmSnapshotVo) {
+        Map<String, String> vmDetails = getVmMapDetails(userVm);
+        boolean result = upgradeUserVmServiceOffering(userVm.getId(), vmSnapshotVo.getServiceOfferingId(), vmDetails);
+        if (! result){
+            throw new CloudRuntimeException("VM Snapshot reverting failed due to vm service offering couldn't be changed to the one used when snapshot was taken");
+        }
+        s_logger.debug("Successfully changed service offering to " + vmSnapshotVo.getServiceOfferingId() + " for vm " + userVm.getId());
+    }
+
+    /**
+     * Upgrade virtual machine {@linkplain vmId} to new service offering {@linkplain serviceOfferingId}
+     * @param vmId vm id
+     * @param serviceOfferingId service offering id
+     * @param details vm details
+     * @return if operation was successful
+     */
+    protected boolean upgradeUserVmServiceOffering(Long vmId, Long serviceOfferingId, Map<String, String> details) {
+        boolean result;
+        try {
+            result = _userVmManager.upgradeVirtualMachine(vmId, serviceOfferingId, details);
+            if (! result){
+                s_logger.error("Couldn't change service offering for vm " + vmId + " to " + serviceOfferingId);
+            }
+            return result;
+        } catch (ConcurrentOperationException | ResourceUnavailableException | ManagementServerException | VirtualMachineMigrationException e) {
+            s_logger.error("Couldn't change service offering for vm " + vmId + " to " + serviceOfferingId + " due to: " + e.getMessage());
+            return false;
+        }
+    }
+
     private UserVm orchestrateRevertToVMSnapshot(Long vmSnapshotId) throws InsufficientCapacityException, ResourceUnavailableException, ConcurrentOperationException {
 
         // check if VM snapshot exists in DB
-        VMSnapshotVO vmSnapshotVo = _vmSnapshotDao.findById(vmSnapshotId);
+        final VMSnapshotVO vmSnapshotVo = _vmSnapshotDao.findById(vmSnapshotId);
         if (vmSnapshotVo == null) {
             throw new InvalidParameterValueException(
                     "unable to find the vm snapshot with id " + vmSnapshotId);
         }
         Long vmId = vmSnapshotVo.getVmId();
-        UserVmVO userVm = _userVMDao.findById(vmId);
+        final UserVmVO userVm = _userVMDao.findById(vmId);
         // check if VM exists
         if (userVm == null) {
             throw new InvalidParameterValueException("Revert vm to snapshot: "
@@ -721,13 +845,37 @@ private UserVm orchestrateRevertToVMSnapshot(Long vmSnapshotId) throws Insuffici
         try {
             VMSnapshotStrategy strategy = findVMSnapshotStrategy(vmSnapshotVo);
             strategy.revertVMSnapshot(vmSnapshotVo);
+            Transaction.execute(new TransactionCallbackWithExceptionNoReturn<CloudRuntimeException>() {
+                @Override
+                public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException {
+                    revertUserVmDetailsFromVmSnapshot(userVm, vmSnapshotVo);
+                    updateUserVmServiceOffering(userVm, vmSnapshotVo);
+                }
+            });
             return userVm;
         } catch (Exception e) {
             s_logger.debug("Failed to revert vmsnapshot: " + vmSnapshotId, e);
             throw new CloudRuntimeException(e.getMessage());
         }
     }
 
+    /**
+     * Update or add user vm details from vm snapshot for vms with custom service offerings
+     * @param userVm user vm
+     * @param vmSnapshotVo vm snapshot
+     */
+    protected void revertUserVmDetailsFromVmSnapshot(UserVmVO userVm, VMSnapshotVO vmSnapshotVo) {
+        ServiceOfferingVO serviceOfferingVO = _serviceOfferingDao.findById(vmSnapshotVo.getServiceOfferingId());
+        if (serviceOfferingVO.isDynamic()) {
+            List<VMSnapshotDetailsVO> vmSnapshotDetails = _vmSnapshotDetailsDao.listDetails(vmSnapshotVo.getId());
+            List<UserVmDetailVO> userVmDetails = new ArrayList<UserVmDetailVO>();
+            for (VMSnapshotDetailsVO detail : vmSnapshotDetails) {
+                userVmDetails.add(new UserVmDetailVO(userVm.getId(), detail.getName(), detail.getValue(), detail.isDisplay()));
+            }
+            _userVmDetailsDao.saveDetails(userVmDetails);
+        }
+    }
+
     @Override
     public RestoreVMSnapshotCommand createRestoreCommand(UserVmVO userVm, List<VMSnapshotVO> vmSnapshotVOs) {
         if (!HypervisorType.KVM.equals(userVm.getHypervisorType()))