[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7061: Rollback of changes with errors during the VM assign

[GitBox <gi...@apache.org>]

weizhouapache commented on code in PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#discussion_r1062529433


##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -7052,525 +7051,779 @@ public VirtualMachine migrateVirtualMachineWithVolume(Long vmId, Host destinatio
     @DB
     @Override
     @ActionEvent(eventType = EventTypes.EVENT_VM_MOVE, eventDescription = "move VM to another user", async = false)
-    public UserVm moveVMToUser(final AssignVMCmd cmd) throws ResourceAllocationException, ConcurrentOperationException, ResourceUnavailableException, InsufficientCapacityException {
-        // VERIFICATIONS and VALIDATIONS
-
-        // VV 1: verify the two users
+    public UserVm moveVmToUser(final AssignVMCmd cmd) throws ResourceAllocationException, InsufficientCapacityException {
         Account caller = CallContext.current().getCallingAccount();
-        if (!_accountMgr.isRootAdmin(caller.getId())
-                && !_accountMgr.isDomainAdmin(caller.getId())) { // only
-            // root
-            // admin
-            // can
-            // assign
-            // VMs
-            throw new InvalidParameterValueException("Only domain admins are allowed to assign VMs and not " + caller.getType());
-        }
-
-        // get and check the valid VM
-        final UserVmVO vm = _vmDao.findById(cmd.getVmId());
-        if (vm == null) {
-            throw new InvalidParameterValueException("There is no vm by that id " + cmd.getVmId());
-        } else if (vm.getState() == State.Running) { // VV 3: check if vm is
-            // running
-            if (s_logger.isDebugEnabled()) {
-                s_logger.debug("VM is Running, unable to move the vm " + vm);
-            }
-            InvalidParameterValueException ex = new InvalidParameterValueException("VM is Running, unable to move the vm with specified vmId");
-            ex.addProxyObject(vm.getUuid(), "vmId");
-            throw ex;
-        }
-
-        final Account oldAccount = _accountService.getActiveAccountById(vm.getAccountId());
-        if (oldAccount == null) {
-            throw new InvalidParameterValueException("Invalid account for VM " + vm.getAccountId() + " in domain.");
-        }
-        final Account newAccount = _accountMgr.finalizeOwner(caller, cmd.getAccountName(), cmd.getDomainId(), cmd.getProjectId());
-        if (newAccount == null) {
-            throw new InvalidParameterValueException("Invalid accountid=" + cmd.getAccountName() + " in domain " + cmd.getDomainId());
+        Long callerId = caller.getId();
+        s_logger.trace(String.format("Verifying if caller [%s] is root or domain admin.", caller));
+        if (!_accountMgr.isRootAdmin(callerId) && !_accountMgr.isDomainAdmin(callerId)) {
+            throw new InvalidParameterValueException(String.format("Only root or domain admins are allowed to assign VMs. Caller [%s] is of type [%s].", caller, caller.getType()));
         }
 
-        if (newAccount.getState() == Account.State.DISABLED) {
-            throw new InvalidParameterValueException("The new account owner " + cmd.getAccountName() + " is disabled.");
-        }
+        Long vmId = cmd.getVmId();
+        final UserVmVO vm = _vmDao.findById(vmId);
+        validateIfVmExistsAndIsNotRunning(vm, vmId);
 
-        if (cmd.getProjectId() != null && cmd.getDomainId() == null) {
+        Long domainId = cmd.getDomainId();
+        Long projectId = cmd.getProjectId();
+        Long oldAccountId = vm.getAccountId();
+        String newAccountName = cmd.getAccountName();
+        final Account oldAccount = _accountService.getActiveAccountById(oldAccountId);
+        final Account newAccount = _accountMgr.finalizeOwner(caller, newAccountName, domainId, projectId);
+        validateAccountsAndCallerAccessToThem(caller, oldAccount, newAccount, oldAccountId, newAccountName, domainId);
+
+        s_logger.trace(String.format("Verifying if the provided domain ID [%s] is valid.", domainId));
+        if (projectId != null && domainId == null) {
             throw new InvalidParameterValueException("Please provide a valid domain ID; cannot assign VM to a project if domain ID is NULL.");
         }
 
-        //check caller has access to both the old and new account
-        _accountMgr.checkAccess(caller, null, true, oldAccount);
-        _accountMgr.checkAccess(caller, null, true, newAccount);
+        validateIfVmHasNoRules(vm, vmId);
 
-        // make sure the accounts are not same
-        if (oldAccount.getAccountId() == newAccount.getAccountId()) {
-            throw new InvalidParameterValueException("The new account is the same as the old account. Account id =" + oldAccount.getAccountId());
+        final List<VolumeVO> volumes = _volsDao.findByInstance(vmId);
+        validateIfVolumesHaveNoSnapshots(volumes);
+
+        final ServiceOfferingVO offering = serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId());
+        verifyResourceLimitsForAccountAndStorage(newAccount, vm, vmId, offering, volumes);
+
+        VirtualMachineTemplate template = _templateDao.findByIdIncludingRemoved(vm.getTemplateId());
+        validateIfNewOwnerHasAccessToTemplate(vm, newAccount, template);
+
+        DomainVO domain = _domainDao.findById(domainId);
+        s_logger.trace(String.format("Verifying if the new account [%s] has access to the specified domain [%s].", newAccount, domain));
+        _accountMgr.checkAccess(newAccount, domain);
+
+        Transaction.execute(new TransactionCallbackNoReturn() {
+            @Override
+            public void doInTransactionWithoutResult(TransactionStatus status) {
+                executeStepsToChangeOwnershipOfVm(cmd, caller, oldAccount, newAccount, vm, offering, volumes, template, domainId);
+            }
+        });
+
+        s_logger.info(String.format("VM [%s] now belongs to account [%s].", vm.getInstanceName(), newAccountName));
+        return vm;
+    }
+
+    protected void validateIfVmExistsAndIsNotRunning(UserVmVO vm, Long vmId) {
+        s_logger.trace(String.format("Validating if VM [%s] exists and is not in state [%s].", vmId, State.Running));
+
+        if (vm == null) {
+            throw new InvalidParameterValueException(String.format("There is no VM by ID [%s].", vmId));
+        } else if (vm.getState() == State.Running) {
+            String errMsg = String.format("Unable to move VM [%s] in [%s] state.", vm, vm.getState());
+            s_logger.warn(errMsg);
+            throw new InvalidParameterValueException(errMsg);
         }
+    }
 
-        // don't allow to move the vm if there are existing PF/LB/Static Nat
-        // rules, or vm is assigned to static Nat ip
-        List<PortForwardingRuleVO> pfrules = _portForwardingDao.listByVm(cmd.getVmId());
-        if (pfrules != null && pfrules.size() > 0) {
-            throw new InvalidParameterValueException("Remove the Port forwarding rules for this VM before assigning to another user.");
+    /**
+     * Verifies if both old and new accounts are valid ({@link #validateOldAndNewAccounts}) and if the caller has access to them ({@link #checkCallerAccessToAccounts}).
+     * @param caller The caller to check for access.
+     * @param oldAccount The old account to be checked for validity.
+     * @param newAccount The new account to be checked for validity.
+     * @param oldAccountId The ID of the old account to be checked for validity.
+     * @param newAccountName The name of the new account to be checked for validity.
+     * @param domainId The ID of the domain where to validate the conditions.
+     */
+    protected void validateAccountsAndCallerAccessToThem(Account caller, Account oldAccount, Account newAccount, Long oldAccountId, String newAccountName, Long domainId) {

Review Comment:
   is it necessary to create this new method ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org