You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2023/01/05 13:34:10 UTC
[GitHub] [cloudstack] stephankruggg opened a new pull request, #7061: Rollback of changes with errors during the VM assign
stephankruggg opened a new pull request, #7061:
URL: https://github.com/apache/cloudstack/pull/7061
### Description
When assigning a VM to another account (`assignVirtualMachine API`), if a network error happens during the process, no rollback is executed; thus, leaving the DB in an inconsistent state.
This PR fixes this by processing all steps to change the ownership of a VM inside the transaction. It also refactors code related to this procedure.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [X] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [X] Minor
- [ ] Trivial
### How Has This Been Tested?
In a local lab, I created a VM and an account of user type. I then attempted to assign the admin's VM to the user by using a network not accessible to the user.
Before applying the changes, an error was raised, but the VM was listed as belonging to the user.
After applying the changes, an error was raised, and the VM was listed as belonging to the admin - no changes occurred.
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1812802095
Hi guys, sorry for the delay.
> > @stephankruggg is anybody looking at this (or should we postpone)?
>
> @GaOrtiga can you take at this one, please?
Yes.
> @stephankruggg is anybody looking at this (or should we postpone)?
I have fixed the conflicts and will run some tests in the next few days before pushing the changes.
--
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
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7061: Rollback of changes with errors during the VM assign
Posted by 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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "stephankruggg (via GitHub)" <gi...@apache.org>.
stephankruggg commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1812783901
> @stephankruggg is anybody looking at this (or should we postpone)?
@GaOrtiga can you take at this one, please?
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1826295717
Packaging result [SF]: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: el9 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 7829
--
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
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1424922787
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=7061)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_coverage&view=list) [75.3% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [cloudstack] github-actions[bot] commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1422725848
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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
[GitHub] [cloudstack] github-actions[bot] commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1481418335
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1835888943
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1836196232
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "kiranchavala (via GitHub)" <gi...@apache.org>.
kiranchavala commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1820530554
@blueorangutan package
--
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
[GitHub] [cloudstack] codecov[bot] commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1372292184
# [Codecov](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7061](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e9d1a84) into [main](https://codecov.io/gh/apache/cloudstack/commit/d0b34b75765d3f36b595f7f5659cdb282e67e3ce?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d0b34b7) will **increase** coverage by `0.12%`.
> The diff coverage is `94.44%`.
```diff
@@ Coverage Diff @@
## main #7061 +/- ##
============================================
+ Coverage 11.58% 11.71% +0.12%
- Complexity 7548 7634 +86
============================================
Files 2494 2494
Lines 247065 247088 +23
Branches 38613 38586 -27
============================================
+ Hits 28629 28948 +319
+ Misses 214692 214403 -289
+ Partials 3744 3737 -7
```
| [Impacted Files](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../src/main/java/com/cloud/vm/UserVmManagerImpl.java](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL3ZtL1VzZXJWbU1hbmFnZXJJbXBsLmphdmE=) | `13.79% <94.44%> (+7.04%)` | :arrow_up: |
| [...main/java/com/cloud/api/query/vo/UserVmJoinVO.java](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL2FwaS9xdWVyeS92by9Vc2VyVm1Kb2luVk8uamF2YQ==) | `8.57% <0.00%> (-0.13%)` | :arrow_down: |
| [...ava/com/cloud/api/query/dao/UserVmJoinDaoImpl.java](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL2FwaS9xdWVyeS9kYW8vVXNlclZtSm9pbkRhb0ltcGwuamF2YQ==) | `3.18% <0.00%> (-0.02%)` | :arrow_down: |
| [...m/cloud/api/query/dao/DomainRouterJoinDaoImpl.java](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL2FwaS9xdWVyeS9kYW8vRG9tYWluUm91dGVySm9pbkRhb0ltcGwuamF2YQ==) | `0.47% <0.00%> (-0.01%)` | :arrow_down: |
| [...src/main/java/com/cloud/api/ApiResponseHelper.java](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL2FwaS9BcGlSZXNwb25zZUhlbHBlci5qYXZh) | `3.87% <0.00%> (-0.01%)` | :arrow_down: |
| [...ava/com/cloud/api/query/vo/DomainRouterJoinVO.java](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL2FwaS9xdWVyeS92by9Eb21haW5Sb3V0ZXJKb2luVk8uamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...n/java/com/cloud/vm/VirtualMachineProfileImpl.java](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZW5naW5lL2NvbXBvbmVudHMtYXBpL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL3ZtL1ZpcnR1YWxNYWNoaW5lUHJvZmlsZUltcGwuamF2YQ==) | `37.38% <0.00%> (+0.93%)` | :arrow_up: |
| [...dstack/network/contrail/model/ModelObjectBase.java](https://codecov.io/gh/apache/cloudstack/pull/7061?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGx1Z2lucy9uZXR3b3JrLWVsZW1lbnRzL2p1bmlwZXItY29udHJhaWwvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Nsb3Vkc3RhY2svbmV0d29yay9jb250cmFpbC9tb2RlbC9Nb2RlbE9iamVjdEJhc2UuamF2YQ==) | `28.84% <0.00%> (+7.69%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [cloudstack] weizhouapache commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1372376990
> > lots of refactoring. I am not sure if the code is good or not. Maybe others can check.
> > @stephankruggg can you point out the key point of your change ?
>
> @weizhouapache, when assigning a VM to another account (`assignVirtualMachine API`), if a network error happens during the process, no rollback is executed; thus, leaving the DB in an inconsistent state. This PR fixes this by processing all steps to change the ownership of a VM inside the transaction.
>
> The refactoring is intended to make the code more testable, readable, and easier to maintain.
@stephankruggg
thanks.
it seems the key block is
```
Transaction.execute(new TransactionCallbackNoReturn() {
@Override
public void doInTransactionWithoutResult(TransactionStatus status) {
executeStepsToChangeOwnershipOfVm(cmd, caller, oldAccount, newAccount, vm, offering, volumes, template, domainId);
}
});
```
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1820639791
@GaOrtiga (cc @stephankruggg ) see https://github.com/apache/cloudstack/actions/runs/6932958288/job/18881597883?pr=7061#step:7:7641
duplicate import
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1812730086
@stephankruggg is anybody looking at this (or should we postpone)?
--
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
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1419881511
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=7061)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_coverage&view=list) [75.3% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1820534313
@kiranchavala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1836193914
@blueorangutan test
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1844820017
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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
[GitHub] [cloudstack] stephankruggg commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by GitBox <gi...@apache.org>.
stephankruggg commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1372338282
> lots of refactoring. I am not sure if the code is good or not. Maybe others can check.
>
> @stephankruggg can you point out the key point of your change ?
@weizhouapache, when assigning a VM to another account (`assignVirtualMachine API`), if a network error happens during the process, no rollback is executed; thus, leaving the DB in an inconsistent state. This PR fixes this by processing all steps to change the ownership of a VM inside the transaction.
The refactoring is intended to make the code more testable, readable, and easier to maintain.
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#discussion_r1064771560
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -566,8 +567,6 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
protected NetworkHelper nwHelper;
@Inject
ReservationDao reservationDao;
- @Inject
Review Comment:
can we remove `_resourceLimitMgr` instead?
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1838013038
@GaOrtiga what is the status on this
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1837051469
<b>[SF] Trillian test result (tid-8447)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 54089 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7061-t8447-kvm-centos7.zip
Smoke tests completed. 118 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_invalid_upgrade_kubernetes_cluster | `Failure` | 3612.14 | test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster | `Failure` | 3622.98 | test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster | `Failure` | 50.94 | test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle | `Error` | 1.26 | test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 72.05 | test_kubernetes_clusters.py
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1836008393
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7885
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on code in PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#discussion_r1399458320
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -566,8 +567,6 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
protected NetworkHelper nwHelper;
@Inject
ReservationDao reservationDao;
- @Inject
Review Comment:
Done
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1820625223
Packaging result [SF]: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: el9 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 7815
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1826285819
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1826285620
@blueorangutan package
--
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
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1372303536
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=7061)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL) [19 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_coverage&view=list) [75.4% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1386530089
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 5302
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1787314838
@stephankruggg can you look at the conflicts? (cc @BryanMLima )
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1826387585
@stephankruggg have a look at https://github.com/apache/cloudstack/actions/runs/6981605172/job/18999868361?pr=7061#step:7:7641
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1838511980
@DaanHoogland I ran into some issues with the integration tests, manually doing what the tests do (for example assigning a VM to an account in another domain) works, but the tests fail.
I think we should postpone this PR to a later release.
--
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
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1386501801
@blueorangutan package
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1386502525
@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with SystemVM template(s). I'll keep you posted as I make progress.
--
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
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7061: Rollback of changes with errors during the VM assign
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on code in PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#discussion_r1073196190
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -7052,525 +7051,766 @@ 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;
+ 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()));
}
- 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 vmId = cmd.getVmId();
+ final UserVmVO vm = _vmDao.findById(vmId);
+ validateIfVmExistsAndIsNotRunning(vm, vmId);
- if (newAccount.getState() == Account.State.DISABLED) {
- throw new InvalidParameterValueException("The new account owner " + cmd.getAccountName() + " is disabled.");
- }
+ 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);
+ validateOldAndNewAccounts(oldAccount, newAccount, oldAccountId, newAccountName, domainId);
+
+ checkCallerAccessToAccounts(caller, oldAccount, newAccount);
- if (cmd.getProjectId() != null && cmd.getDomainId() == null) {
+ 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.");
+ /**
+ * Validates if the provided VM does not have any existing Port Forwarding, Load Balancer, Static Nat, and One to One Nat rules.
+ * If any rules exist, throws a {@link InvalidParameterValueException}.
+ * @param vm the VM to be checked for the rules.
+ * @param vmId the ID of the VM to be checked.
+ * @throws InvalidParameterValueException
+ */
+ protected void validateIfVmHasNoRules(UserVmVO vm, Long vmId) throws InvalidParameterValueException {
+ s_logger.trace(String.format("Validating if VM [%s] has no Port Forwarding, Static Nat, Load Balancing or One to One Nat rules.", vm));
+
+ List<PortForwardingRuleVO> portForwardingRules = _portForwardingDao.listByVm(vmId);
+ if (CollectionUtils.isNotEmpty(portForwardingRules)) {
+ throw new InvalidParameterValueException(String.format("Remove any Port Forwarding rules for VM [%s] before assigning it to another user.", vm));
}
- List<FirewallRuleVO> snrules = _rulesDao.listStaticNatByVmId(vm.getId());
- if (snrules != null && snrules.size() > 0) {
- throw new InvalidParameterValueException("Remove the StaticNat rules for this VM before assigning to another user.");
+
+ List<FirewallRuleVO> staticNatRules = _rulesDao.listStaticNatByVmId(vmId);
+ if (CollectionUtils.isNotEmpty(staticNatRules)) {
+ throw new InvalidParameterValueException(String.format("Remove the StaticNat rules for VM [%s] before assigning it to another user.", vm));
}
- List<LoadBalancerVMMapVO> maps = _loadBalancerVMMapDao.listByInstanceId(vm.getId());
- if (maps != null && maps.size() > 0) {
- throw new InvalidParameterValueException("Remove the load balancing rules for this VM before assigning to another user.");
+
+ List<LoadBalancerVMMapVO> loadBalancerVmMaps = _loadBalancerVMMapDao.listByInstanceId(vmId);
+ if (CollectionUtils.isNotEmpty(loadBalancerVmMaps)) {
+ throw new InvalidParameterValueException(String.format("Remove the Load Balancing rules for VM [%s] before assigning it to another user.", vm));
}
- // check for one on one nat
- List<IPAddressVO> ips = _ipAddressDao.findAllByAssociatedVmId(cmd.getVmId());
+
+ List<IPAddressVO> ips = _ipAddressDao.findAllByAssociatedVmId(vmId);
for (IPAddressVO ip : ips) {
if (ip.isOneToOneNat()) {
- throw new InvalidParameterValueException("Remove the one to one nat rule for this VM for ip " + ip.toString());
+ throw new InvalidParameterValueException(String.format("Remove the One to One Nat rule for VM [%s] for IP [%s].", vm, ip));
}
}
+ }
- final List<VolumeVO> volumes = _volsDao.findByInstance(cmd.getVmId());
-
+ protected void validateIfVolumesHaveNoSnapshots(List<VolumeVO> volumes) throws InvalidParameterValueException {
+ s_logger.trace("Verifying if there are any snapshots for any of the VM volumes.");
for (VolumeVO volume : volumes) {
- List<SnapshotVO> snapshots = _snapshotDao.listByStatusNotIn(volume.getId(), Snapshot.State.Destroyed,Snapshot.State.Error);
- if (snapshots != null && snapshots.size() > 0) {
- throw new InvalidParameterValueException(
- "Snapshots exists for volume: "+ volume.getName()+ ", Detach volume or remove snapshots for volume before assigning VM to another user.");
+ s_logger.trace(String.format("Verifying snapshots for volume [%s].", volume));
+ List<SnapshotVO> snapshots = _snapshotDao.listByStatusNotIn(volume.getId(), Snapshot.State.Destroyed, Snapshot.State.Error);
+ if (CollectionUtils.isNotEmpty(snapshots)) {
+ throw new InvalidParameterValueException(String.format("Snapshots exist for volume [%s]. Detach volume or remove snapshots for the volume before assigning VM to "
+ + "another user.", volume.getName()));
}
}
+ }
- DataCenterVO zone = _dcDao.findById(vm.getDataCenterId());
-
- // Get serviceOffering and Volumes for Virtual Machine
- final ServiceOfferingVO offering = serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId());
-
- //Remove vm from instance group
- removeInstanceFromInstanceGroup(cmd.getVmId());
+ /**
+ * Verifies if the CPU, RAM and volume size do not exceed the account and the primary storage limit.
+ * If any limit is exceeded, throws a {@link ResourceAllocationException}.
+ * @param account The account to check if CPU and RAM limit has been exceeded.
+ * @param vm The VM which can exceed resource limits.
+ * @param vmId The ID for the VM which can exceed resource limits.
+ * @param offering The service offering which can exceed resource limits.
+ * @param volumes The volumes whose total size can exceed resource limits.
+ * @throws ResourceAllocationException
+ */
+ protected void verifyResourceLimitsForAccountAndStorage(Account account, UserVmVO vm, Long vmId, ServiceOfferingVO offering, List<VolumeVO> volumes)
+ throws ResourceAllocationException {
- // VV 2: check if account/domain is with in resource limits to create a new vm
- if (! VirtualMachineManager.ResourceCountRunningVMsonly.value()) {
- resourceLimitCheck(newAccount, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
+ s_logger.trace(String.format("Verifying if CPU and RAM for VM [%s] do not exceed account [%s] limit.", vm, account));
+ if (!isResourceCountRunningVmsOnlyEnabled()) {
+ resourceLimitCheck(account, vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));
}
- // VV 3: check if volumes and primary storage space are with in resource limits
- _resourceLimitMgr.checkResourceLimit(newAccount, ResourceType.volume, _volsDao.findByInstance(cmd.getVmId()).size());
+ s_logger.trace(String.format("Verifying if volume size for VM [%s] does not exceed account [%s] limit.", vm, account));
+ _resourceLimitMgr.checkResourceLimit(account, ResourceType.volume, _volsDao.findByInstance(vmId).size());
Long totalVolumesSize = (long)0;
for (VolumeVO volume : volumes) {
totalVolumesSize += volume.getSize();
}
- _resourceLimitMgr.checkResourceLimit(newAccount, ResourceType.primary_storage, totalVolumesSize);
+ s_logger.trace(String.format("Verifying if primary storage for VM [%s] does not exceed account [%s] limit.", vm, account));
+ _resourceLimitMgr.checkResourceLimit(account, ResourceType.primary_storage, totalVolumesSize);
+ }
+
+ protected void validateIfNewOwnerHasAccessToTemplate(UserVmVO vm, Account newAccount, VirtualMachineTemplate template) {
+ s_logger.trace(String.format("Validating if new owner [%s] has access to the template specified for VM [%s].", newAccount, vm));
- // VV 4: Check if new owner can use the vm template
- VirtualMachineTemplate template = _templateDao.findByIdIncludingRemoved(vm.getTemplateId());
if (template == null) {
- throw new InvalidParameterValueException(String.format("Template for VM: %s cannot be found", vm.getUuid()));
+ throw new InvalidParameterValueException(String.format("Template for VM [%s] cannot be found.", vm.getUuid()));
}
+
if (!template.isPublicTemplate()) {
Account templateOwner = _accountMgr.getAccount(template.getAccountId());
_accountMgr.checkAccess(newAccount, null, true, templateOwner);
}
+ }
- // VV 5: check the new account can create vm in the domain
- DomainVO domain = _domainDao.findById(cmd.getDomainId());
- _accountMgr.checkAccess(newAccount, domain);
+ /**
+ * Executes all ownership steps necessary to assign a VM to another user:
+ * generating a destroy VM event ({@link EventTypes}),
+ * decrementing the old user resource count ({@link #resourceCountDecrement(long, Boolean, Long, Long)}),
+ * removing the VM from its instance group ({@link #removeInstanceFromInstanceGroup(long)}),
+ * updating the VM owner to the new account ({@link #updateVmOwner(Account, UserVmVO, Long, Long)}),
+ * updating the volumes to the new account ({@link #updateVolumesOwner(List, Account, Account, Long)}),
+ * updating the network for the VM ({@link #updateVmNetwork(AssignVMCmd, Account, UserVmVO, Account, VirtualMachineTemplate)}),
+ * incrementing the new user resource count ({@link #resourceCountIncrement(long, Boolean, Long, Long)}),
+ * and generating a create VM event ({@link EventTypes}).
+ * @param cmd The assignVMCmd.
+ * @param caller The account calling the assignVMCmd.
+ * @param oldAccount The old account from whom the VM will be moved.
+ * @param newAccount The new account to whom the VM will move.
+ * @param vm The VM to be moved between accounts.
+ * @param offering The service offering which will be used to decrement and increment resource counts.
+ * @param volumes The volumes of the VM which will be assigned to another user.
+ * @param template The template of the VM which will be assigned to another user.
+ * @param domainId The ID of the domain where the VM which will be assigned to another user is.
+ */
+ protected void executeStepsToChangeOwnershipOfVm(AssignVMCmd cmd, Account caller, Account oldAccount, Account newAccount, UserVmVO vm, ServiceOfferingVO offering,
+ List<VolumeVO> volumes, VirtualMachineTemplate template, Long domainId) {
- Transaction.execute(new TransactionCallbackNoReturn() {
- @Override
- public void doInTransactionWithoutResult(TransactionStatus status) {
- //generate destroy vm event for usage
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_DESTROY, vm.getAccountId(), vm.getDataCenterId(),
- vm.getId(), vm.getHostName(), vm.getServiceOfferingId(), vm.getTemplateId(),
- vm.getHypervisorType().toString(), VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
- // update resource counts for old account
- resourceCountDecrement(oldAccount.getAccountId(), vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
-
- // OWNERSHIP STEP 1: update the vm owner
- vm.setAccountId(newAccount.getAccountId());
- vm.setDomainId(cmd.getDomainId());
- _vmDao.persist(vm);
+ s_logger.trace(String.format("Generating destroy event for VM [%s].", vm));
+ UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_DESTROY, vm.getAccountId(), vm.getDataCenterId(), vm.getId(), vm.getHostName(), vm.getServiceOfferingId(),
+ vm.getTemplateId(), vm.getHypervisorType().toString(), VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
- // OS 2: update volume
- for (VolumeVO volume : volumes) {
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_DELETE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(),
- Volume.class.getName(), volume.getUuid(), volume.isDisplayVolume());
- _resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), ResourceType.volume);
- _resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), ResourceType.primary_storage, new Long(volume.getSize()));
- volume.setAccountId(newAccount.getAccountId());
- volume.setDomainId(newAccount.getDomainId());
- _volsDao.persist(volume);
- _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.volume);
- _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.primary_storage, new Long(volume.getSize()));
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_CREATE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(),
- volume.getDiskOfferingId(), volume.getTemplateId(), volume.getSize(), Volume.class.getName(),
- volume.getUuid(), volume.isDisplayVolume());
- }
-
- //update resource count of new account
- if (! VirtualMachineManager.ResourceCountRunningVMsonly.value()) {
- resourceCountIncrement(newAccount.getAccountId(), vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
- }
+ s_logger.trace(String.format("Decrementing old account [%s] resource count.", oldAccount));
+ resourceCountDecrement(oldAccount.getAccountId(), vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));
- //generate usage events to account for this change
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_CREATE, vm.getAccountId(), vm.getDataCenterId(), vm.getId(),
- vm.getHostName(), vm.getServiceOfferingId(), vm.getTemplateId(), vm.getHypervisorType().toString(),
- VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
- }
- });
+ s_logger.trace(String.format("Removing VM [%s] from its instance group.", vm));
+ removeInstanceFromInstanceGroup(vm.getId());
+
+ Long newAccountId = newAccount.getAccountId();
+ updateVmOwner(newAccount, vm, domainId, newAccountId);
+
+ updateVolumesOwner(volumes, oldAccount, newAccount, newAccountId);
+
+ try {
+ updateVmNetwork(cmd, caller, vm, newAccount, template);
+ } catch (InsufficientCapacityException | ResourceAllocationException e) {
Review Comment:
publish an error event in case of exceptions ?
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -7052,525 +7051,766 @@ 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;
+ 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()));
}
- 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 vmId = cmd.getVmId();
+ final UserVmVO vm = _vmDao.findById(vmId);
+ validateIfVmExistsAndIsNotRunning(vm, vmId);
- if (newAccount.getState() == Account.State.DISABLED) {
- throw new InvalidParameterValueException("The new account owner " + cmd.getAccountName() + " is disabled.");
- }
+ 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);
+ validateOldAndNewAccounts(oldAccount, newAccount, oldAccountId, newAccountName, domainId);
+
+ checkCallerAccessToAccounts(caller, oldAccount, newAccount);
- if (cmd.getProjectId() != null && cmd.getDomainId() == null) {
+ 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.");
+ /**
+ * Validates if the provided VM does not have any existing Port Forwarding, Load Balancer, Static Nat, and One to One Nat rules.
+ * If any rules exist, throws a {@link InvalidParameterValueException}.
+ * @param vm the VM to be checked for the rules.
+ * @param vmId the ID of the VM to be checked.
+ * @throws InvalidParameterValueException
+ */
+ protected void validateIfVmHasNoRules(UserVmVO vm, Long vmId) throws InvalidParameterValueException {
+ s_logger.trace(String.format("Validating if VM [%s] has no Port Forwarding, Static Nat, Load Balancing or One to One Nat rules.", vm));
+
+ List<PortForwardingRuleVO> portForwardingRules = _portForwardingDao.listByVm(vmId);
+ if (CollectionUtils.isNotEmpty(portForwardingRules)) {
+ throw new InvalidParameterValueException(String.format("Remove any Port Forwarding rules for VM [%s] before assigning it to another user.", vm));
}
- List<FirewallRuleVO> snrules = _rulesDao.listStaticNatByVmId(vm.getId());
- if (snrules != null && snrules.size() > 0) {
- throw new InvalidParameterValueException("Remove the StaticNat rules for this VM before assigning to another user.");
+
+ List<FirewallRuleVO> staticNatRules = _rulesDao.listStaticNatByVmId(vmId);
+ if (CollectionUtils.isNotEmpty(staticNatRules)) {
+ throw new InvalidParameterValueException(String.format("Remove the StaticNat rules for VM [%s] before assigning it to another user.", vm));
}
- List<LoadBalancerVMMapVO> maps = _loadBalancerVMMapDao.listByInstanceId(vm.getId());
- if (maps != null && maps.size() > 0) {
- throw new InvalidParameterValueException("Remove the load balancing rules for this VM before assigning to another user.");
+
+ List<LoadBalancerVMMapVO> loadBalancerVmMaps = _loadBalancerVMMapDao.listByInstanceId(vmId);
+ if (CollectionUtils.isNotEmpty(loadBalancerVmMaps)) {
+ throw new InvalidParameterValueException(String.format("Remove the Load Balancing rules for VM [%s] before assigning it to another user.", vm));
}
- // check for one on one nat
- List<IPAddressVO> ips = _ipAddressDao.findAllByAssociatedVmId(cmd.getVmId());
+
+ List<IPAddressVO> ips = _ipAddressDao.findAllByAssociatedVmId(vmId);
for (IPAddressVO ip : ips) {
if (ip.isOneToOneNat()) {
- throw new InvalidParameterValueException("Remove the one to one nat rule for this VM for ip " + ip.toString());
+ throw new InvalidParameterValueException(String.format("Remove the One to One Nat rule for VM [%s] for IP [%s].", vm, ip));
}
}
+ }
- final List<VolumeVO> volumes = _volsDao.findByInstance(cmd.getVmId());
-
+ protected void validateIfVolumesHaveNoSnapshots(List<VolumeVO> volumes) throws InvalidParameterValueException {
+ s_logger.trace("Verifying if there are any snapshots for any of the VM volumes.");
for (VolumeVO volume : volumes) {
- List<SnapshotVO> snapshots = _snapshotDao.listByStatusNotIn(volume.getId(), Snapshot.State.Destroyed,Snapshot.State.Error);
- if (snapshots != null && snapshots.size() > 0) {
- throw new InvalidParameterValueException(
- "Snapshots exists for volume: "+ volume.getName()+ ", Detach volume or remove snapshots for volume before assigning VM to another user.");
+ s_logger.trace(String.format("Verifying snapshots for volume [%s].", volume));
+ List<SnapshotVO> snapshots = _snapshotDao.listByStatusNotIn(volume.getId(), Snapshot.State.Destroyed, Snapshot.State.Error);
+ if (CollectionUtils.isNotEmpty(snapshots)) {
+ throw new InvalidParameterValueException(String.format("Snapshots exist for volume [%s]. Detach volume or remove snapshots for the volume before assigning VM to "
+ + "another user.", volume.getName()));
}
}
+ }
- DataCenterVO zone = _dcDao.findById(vm.getDataCenterId());
-
- // Get serviceOffering and Volumes for Virtual Machine
- final ServiceOfferingVO offering = serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId());
-
- //Remove vm from instance group
- removeInstanceFromInstanceGroup(cmd.getVmId());
+ /**
+ * Verifies if the CPU, RAM and volume size do not exceed the account and the primary storage limit.
+ * If any limit is exceeded, throws a {@link ResourceAllocationException}.
+ * @param account The account to check if CPU and RAM limit has been exceeded.
+ * @param vm The VM which can exceed resource limits.
+ * @param vmId The ID for the VM which can exceed resource limits.
+ * @param offering The service offering which can exceed resource limits.
+ * @param volumes The volumes whose total size can exceed resource limits.
+ * @throws ResourceAllocationException
+ */
+ protected void verifyResourceLimitsForAccountAndStorage(Account account, UserVmVO vm, Long vmId, ServiceOfferingVO offering, List<VolumeVO> volumes)
+ throws ResourceAllocationException {
- // VV 2: check if account/domain is with in resource limits to create a new vm
- if (! VirtualMachineManager.ResourceCountRunningVMsonly.value()) {
- resourceLimitCheck(newAccount, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
+ s_logger.trace(String.format("Verifying if CPU and RAM for VM [%s] do not exceed account [%s] limit.", vm, account));
+ if (!isResourceCountRunningVmsOnlyEnabled()) {
+ resourceLimitCheck(account, vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));
}
- // VV 3: check if volumes and primary storage space are with in resource limits
- _resourceLimitMgr.checkResourceLimit(newAccount, ResourceType.volume, _volsDao.findByInstance(cmd.getVmId()).size());
+ s_logger.trace(String.format("Verifying if volume size for VM [%s] does not exceed account [%s] limit.", vm, account));
+ _resourceLimitMgr.checkResourceLimit(account, ResourceType.volume, _volsDao.findByInstance(vmId).size());
Long totalVolumesSize = (long)0;
for (VolumeVO volume : volumes) {
totalVolumesSize += volume.getSize();
}
- _resourceLimitMgr.checkResourceLimit(newAccount, ResourceType.primary_storage, totalVolumesSize);
+ s_logger.trace(String.format("Verifying if primary storage for VM [%s] does not exceed account [%s] limit.", vm, account));
+ _resourceLimitMgr.checkResourceLimit(account, ResourceType.primary_storage, totalVolumesSize);
+ }
+
+ protected void validateIfNewOwnerHasAccessToTemplate(UserVmVO vm, Account newAccount, VirtualMachineTemplate template) {
+ s_logger.trace(String.format("Validating if new owner [%s] has access to the template specified for VM [%s].", newAccount, vm));
- // VV 4: Check if new owner can use the vm template
- VirtualMachineTemplate template = _templateDao.findByIdIncludingRemoved(vm.getTemplateId());
if (template == null) {
- throw new InvalidParameterValueException(String.format("Template for VM: %s cannot be found", vm.getUuid()));
+ throw new InvalidParameterValueException(String.format("Template for VM [%s] cannot be found.", vm.getUuid()));
}
+
if (!template.isPublicTemplate()) {
Account templateOwner = _accountMgr.getAccount(template.getAccountId());
_accountMgr.checkAccess(newAccount, null, true, templateOwner);
}
+ }
- // VV 5: check the new account can create vm in the domain
- DomainVO domain = _domainDao.findById(cmd.getDomainId());
- _accountMgr.checkAccess(newAccount, domain);
+ /**
+ * Executes all ownership steps necessary to assign a VM to another user:
+ * generating a destroy VM event ({@link EventTypes}),
+ * decrementing the old user resource count ({@link #resourceCountDecrement(long, Boolean, Long, Long)}),
+ * removing the VM from its instance group ({@link #removeInstanceFromInstanceGroup(long)}),
+ * updating the VM owner to the new account ({@link #updateVmOwner(Account, UserVmVO, Long, Long)}),
+ * updating the volumes to the new account ({@link #updateVolumesOwner(List, Account, Account, Long)}),
+ * updating the network for the VM ({@link #updateVmNetwork(AssignVMCmd, Account, UserVmVO, Account, VirtualMachineTemplate)}),
+ * incrementing the new user resource count ({@link #resourceCountIncrement(long, Boolean, Long, Long)}),
+ * and generating a create VM event ({@link EventTypes}).
+ * @param cmd The assignVMCmd.
+ * @param caller The account calling the assignVMCmd.
+ * @param oldAccount The old account from whom the VM will be moved.
+ * @param newAccount The new account to whom the VM will move.
+ * @param vm The VM to be moved between accounts.
+ * @param offering The service offering which will be used to decrement and increment resource counts.
+ * @param volumes The volumes of the VM which will be assigned to another user.
+ * @param template The template of the VM which will be assigned to another user.
+ * @param domainId The ID of the domain where the VM which will be assigned to another user is.
+ */
+ protected void executeStepsToChangeOwnershipOfVm(AssignVMCmd cmd, Account caller, Account oldAccount, Account newAccount, UserVmVO vm, ServiceOfferingVO offering,
+ List<VolumeVO> volumes, VirtualMachineTemplate template, Long domainId) {
- Transaction.execute(new TransactionCallbackNoReturn() {
- @Override
- public void doInTransactionWithoutResult(TransactionStatus status) {
- //generate destroy vm event for usage
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_DESTROY, vm.getAccountId(), vm.getDataCenterId(),
- vm.getId(), vm.getHostName(), vm.getServiceOfferingId(), vm.getTemplateId(),
- vm.getHypervisorType().toString(), VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
- // update resource counts for old account
- resourceCountDecrement(oldAccount.getAccountId(), vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
-
- // OWNERSHIP STEP 1: update the vm owner
- vm.setAccountId(newAccount.getAccountId());
- vm.setDomainId(cmd.getDomainId());
- _vmDao.persist(vm);
+ s_logger.trace(String.format("Generating destroy event for VM [%s].", vm));
+ UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_DESTROY, vm.getAccountId(), vm.getDataCenterId(), vm.getId(), vm.getHostName(), vm.getServiceOfferingId(),
+ vm.getTemplateId(), vm.getHypervisorType().toString(), VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
- // OS 2: update volume
- for (VolumeVO volume : volumes) {
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_DELETE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(),
- Volume.class.getName(), volume.getUuid(), volume.isDisplayVolume());
- _resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), ResourceType.volume);
- _resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), ResourceType.primary_storage, new Long(volume.getSize()));
- volume.setAccountId(newAccount.getAccountId());
- volume.setDomainId(newAccount.getDomainId());
- _volsDao.persist(volume);
- _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.volume);
- _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.primary_storage, new Long(volume.getSize()));
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_CREATE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(),
- volume.getDiskOfferingId(), volume.getTemplateId(), volume.getSize(), Volume.class.getName(),
- volume.getUuid(), volume.isDisplayVolume());
- }
-
- //update resource count of new account
- if (! VirtualMachineManager.ResourceCountRunningVMsonly.value()) {
- resourceCountIncrement(newAccount.getAccountId(), vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
- }
+ s_logger.trace(String.format("Decrementing old account [%s] resource count.", oldAccount));
+ resourceCountDecrement(oldAccount.getAccountId(), vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));
- //generate usage events to account for this change
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_CREATE, vm.getAccountId(), vm.getDataCenterId(), vm.getId(),
- vm.getHostName(), vm.getServiceOfferingId(), vm.getTemplateId(), vm.getHypervisorType().toString(),
- VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
- }
- });
+ s_logger.trace(String.format("Removing VM [%s] from its instance group.", vm));
+ removeInstanceFromInstanceGroup(vm.getId());
+
+ Long newAccountId = newAccount.getAccountId();
+ updateVmOwner(newAccount, vm, domainId, newAccountId);
+
+ updateVolumesOwner(volumes, oldAccount, newAccount, newAccountId);
+
+ try {
+ updateVmNetwork(cmd, caller, vm, newAccount, template);
+ } catch (InsufficientCapacityException | ResourceAllocationException e) {
+ throw new RuntimeException(String.format("Unable to update networks when assigning VM [%s] due to [%s].", vm, e.getMessage()), e);
Review Comment:
use CloudRuntimeException ?
--
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
[GitHub] [cloudstack] stephankruggg commented on a diff in pull request #7061: Rollback of changes with errors during the VM assign
Posted by "stephankruggg (via GitHub)" <gi...@apache.org>.
stephankruggg commented on code in PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#discussion_r1097906811
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -7052,525 +7051,766 @@ 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;
+ 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()));
}
- 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 vmId = cmd.getVmId();
+ final UserVmVO vm = _vmDao.findById(vmId);
+ validateIfVmExistsAndIsNotRunning(vm, vmId);
- if (newAccount.getState() == Account.State.DISABLED) {
- throw new InvalidParameterValueException("The new account owner " + cmd.getAccountName() + " is disabled.");
- }
+ 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);
+ validateOldAndNewAccounts(oldAccount, newAccount, oldAccountId, newAccountName, domainId);
+
+ checkCallerAccessToAccounts(caller, oldAccount, newAccount);
- if (cmd.getProjectId() != null && cmd.getDomainId() == null) {
+ 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.");
+ /**
+ * Validates if the provided VM does not have any existing Port Forwarding, Load Balancer, Static Nat, and One to One Nat rules.
+ * If any rules exist, throws a {@link InvalidParameterValueException}.
+ * @param vm the VM to be checked for the rules.
+ * @param vmId the ID of the VM to be checked.
+ * @throws InvalidParameterValueException
+ */
+ protected void validateIfVmHasNoRules(UserVmVO vm, Long vmId) throws InvalidParameterValueException {
+ s_logger.trace(String.format("Validating if VM [%s] has no Port Forwarding, Static Nat, Load Balancing or One to One Nat rules.", vm));
+
+ List<PortForwardingRuleVO> portForwardingRules = _portForwardingDao.listByVm(vmId);
+ if (CollectionUtils.isNotEmpty(portForwardingRules)) {
+ throw new InvalidParameterValueException(String.format("Remove any Port Forwarding rules for VM [%s] before assigning it to another user.", vm));
}
- List<FirewallRuleVO> snrules = _rulesDao.listStaticNatByVmId(vm.getId());
- if (snrules != null && snrules.size() > 0) {
- throw new InvalidParameterValueException("Remove the StaticNat rules for this VM before assigning to another user.");
+
+ List<FirewallRuleVO> staticNatRules = _rulesDao.listStaticNatByVmId(vmId);
+ if (CollectionUtils.isNotEmpty(staticNatRules)) {
+ throw new InvalidParameterValueException(String.format("Remove the StaticNat rules for VM [%s] before assigning it to another user.", vm));
}
- List<LoadBalancerVMMapVO> maps = _loadBalancerVMMapDao.listByInstanceId(vm.getId());
- if (maps != null && maps.size() > 0) {
- throw new InvalidParameterValueException("Remove the load balancing rules for this VM before assigning to another user.");
+
+ List<LoadBalancerVMMapVO> loadBalancerVmMaps = _loadBalancerVMMapDao.listByInstanceId(vmId);
+ if (CollectionUtils.isNotEmpty(loadBalancerVmMaps)) {
+ throw new InvalidParameterValueException(String.format("Remove the Load Balancing rules for VM [%s] before assigning it to another user.", vm));
}
- // check for one on one nat
- List<IPAddressVO> ips = _ipAddressDao.findAllByAssociatedVmId(cmd.getVmId());
+
+ List<IPAddressVO> ips = _ipAddressDao.findAllByAssociatedVmId(vmId);
for (IPAddressVO ip : ips) {
if (ip.isOneToOneNat()) {
- throw new InvalidParameterValueException("Remove the one to one nat rule for this VM for ip " + ip.toString());
+ throw new InvalidParameterValueException(String.format("Remove the One to One Nat rule for VM [%s] for IP [%s].", vm, ip));
}
}
+ }
- final List<VolumeVO> volumes = _volsDao.findByInstance(cmd.getVmId());
-
+ protected void validateIfVolumesHaveNoSnapshots(List<VolumeVO> volumes) throws InvalidParameterValueException {
+ s_logger.trace("Verifying if there are any snapshots for any of the VM volumes.");
for (VolumeVO volume : volumes) {
- List<SnapshotVO> snapshots = _snapshotDao.listByStatusNotIn(volume.getId(), Snapshot.State.Destroyed,Snapshot.State.Error);
- if (snapshots != null && snapshots.size() > 0) {
- throw new InvalidParameterValueException(
- "Snapshots exists for volume: "+ volume.getName()+ ", Detach volume or remove snapshots for volume before assigning VM to another user.");
+ s_logger.trace(String.format("Verifying snapshots for volume [%s].", volume));
+ List<SnapshotVO> snapshots = _snapshotDao.listByStatusNotIn(volume.getId(), Snapshot.State.Destroyed, Snapshot.State.Error);
+ if (CollectionUtils.isNotEmpty(snapshots)) {
+ throw new InvalidParameterValueException(String.format("Snapshots exist for volume [%s]. Detach volume or remove snapshots for the volume before assigning VM to "
+ + "another user.", volume.getName()));
}
}
+ }
- DataCenterVO zone = _dcDao.findById(vm.getDataCenterId());
-
- // Get serviceOffering and Volumes for Virtual Machine
- final ServiceOfferingVO offering = serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId());
-
- //Remove vm from instance group
- removeInstanceFromInstanceGroup(cmd.getVmId());
+ /**
+ * Verifies if the CPU, RAM and volume size do not exceed the account and the primary storage limit.
+ * If any limit is exceeded, throws a {@link ResourceAllocationException}.
+ * @param account The account to check if CPU and RAM limit has been exceeded.
+ * @param vm The VM which can exceed resource limits.
+ * @param vmId The ID for the VM which can exceed resource limits.
+ * @param offering The service offering which can exceed resource limits.
+ * @param volumes The volumes whose total size can exceed resource limits.
+ * @throws ResourceAllocationException
+ */
+ protected void verifyResourceLimitsForAccountAndStorage(Account account, UserVmVO vm, Long vmId, ServiceOfferingVO offering, List<VolumeVO> volumes)
+ throws ResourceAllocationException {
- // VV 2: check if account/domain is with in resource limits to create a new vm
- if (! VirtualMachineManager.ResourceCountRunningVMsonly.value()) {
- resourceLimitCheck(newAccount, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
+ s_logger.trace(String.format("Verifying if CPU and RAM for VM [%s] do not exceed account [%s] limit.", vm, account));
+ if (!isResourceCountRunningVmsOnlyEnabled()) {
+ resourceLimitCheck(account, vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));
}
- // VV 3: check if volumes and primary storage space are with in resource limits
- _resourceLimitMgr.checkResourceLimit(newAccount, ResourceType.volume, _volsDao.findByInstance(cmd.getVmId()).size());
+ s_logger.trace(String.format("Verifying if volume size for VM [%s] does not exceed account [%s] limit.", vm, account));
+ _resourceLimitMgr.checkResourceLimit(account, ResourceType.volume, _volsDao.findByInstance(vmId).size());
Long totalVolumesSize = (long)0;
for (VolumeVO volume : volumes) {
totalVolumesSize += volume.getSize();
}
- _resourceLimitMgr.checkResourceLimit(newAccount, ResourceType.primary_storage, totalVolumesSize);
+ s_logger.trace(String.format("Verifying if primary storage for VM [%s] does not exceed account [%s] limit.", vm, account));
+ _resourceLimitMgr.checkResourceLimit(account, ResourceType.primary_storage, totalVolumesSize);
+ }
+
+ protected void validateIfNewOwnerHasAccessToTemplate(UserVmVO vm, Account newAccount, VirtualMachineTemplate template) {
+ s_logger.trace(String.format("Validating if new owner [%s] has access to the template specified for VM [%s].", newAccount, vm));
- // VV 4: Check if new owner can use the vm template
- VirtualMachineTemplate template = _templateDao.findByIdIncludingRemoved(vm.getTemplateId());
if (template == null) {
- throw new InvalidParameterValueException(String.format("Template for VM: %s cannot be found", vm.getUuid()));
+ throw new InvalidParameterValueException(String.format("Template for VM [%s] cannot be found.", vm.getUuid()));
}
+
if (!template.isPublicTemplate()) {
Account templateOwner = _accountMgr.getAccount(template.getAccountId());
_accountMgr.checkAccess(newAccount, null, true, templateOwner);
}
+ }
- // VV 5: check the new account can create vm in the domain
- DomainVO domain = _domainDao.findById(cmd.getDomainId());
- _accountMgr.checkAccess(newAccount, domain);
+ /**
+ * Executes all ownership steps necessary to assign a VM to another user:
+ * generating a destroy VM event ({@link EventTypes}),
+ * decrementing the old user resource count ({@link #resourceCountDecrement(long, Boolean, Long, Long)}),
+ * removing the VM from its instance group ({@link #removeInstanceFromInstanceGroup(long)}),
+ * updating the VM owner to the new account ({@link #updateVmOwner(Account, UserVmVO, Long, Long)}),
+ * updating the volumes to the new account ({@link #updateVolumesOwner(List, Account, Account, Long)}),
+ * updating the network for the VM ({@link #updateVmNetwork(AssignVMCmd, Account, UserVmVO, Account, VirtualMachineTemplate)}),
+ * incrementing the new user resource count ({@link #resourceCountIncrement(long, Boolean, Long, Long)}),
+ * and generating a create VM event ({@link EventTypes}).
+ * @param cmd The assignVMCmd.
+ * @param caller The account calling the assignVMCmd.
+ * @param oldAccount The old account from whom the VM will be moved.
+ * @param newAccount The new account to whom the VM will move.
+ * @param vm The VM to be moved between accounts.
+ * @param offering The service offering which will be used to decrement and increment resource counts.
+ * @param volumes The volumes of the VM which will be assigned to another user.
+ * @param template The template of the VM which will be assigned to another user.
+ * @param domainId The ID of the domain where the VM which will be assigned to another user is.
+ */
+ protected void executeStepsToChangeOwnershipOfVm(AssignVMCmd cmd, Account caller, Account oldAccount, Account newAccount, UserVmVO vm, ServiceOfferingVO offering,
+ List<VolumeVO> volumes, VirtualMachineTemplate template, Long domainId) {
- Transaction.execute(new TransactionCallbackNoReturn() {
- @Override
- public void doInTransactionWithoutResult(TransactionStatus status) {
- //generate destroy vm event for usage
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_DESTROY, vm.getAccountId(), vm.getDataCenterId(),
- vm.getId(), vm.getHostName(), vm.getServiceOfferingId(), vm.getTemplateId(),
- vm.getHypervisorType().toString(), VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
- // update resource counts for old account
- resourceCountDecrement(oldAccount.getAccountId(), vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
-
- // OWNERSHIP STEP 1: update the vm owner
- vm.setAccountId(newAccount.getAccountId());
- vm.setDomainId(cmd.getDomainId());
- _vmDao.persist(vm);
+ s_logger.trace(String.format("Generating destroy event for VM [%s].", vm));
+ UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_DESTROY, vm.getAccountId(), vm.getDataCenterId(), vm.getId(), vm.getHostName(), vm.getServiceOfferingId(),
+ vm.getTemplateId(), vm.getHypervisorType().toString(), VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
- // OS 2: update volume
- for (VolumeVO volume : volumes) {
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_DELETE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(),
- Volume.class.getName(), volume.getUuid(), volume.isDisplayVolume());
- _resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), ResourceType.volume);
- _resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), ResourceType.primary_storage, new Long(volume.getSize()));
- volume.setAccountId(newAccount.getAccountId());
- volume.setDomainId(newAccount.getDomainId());
- _volsDao.persist(volume);
- _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.volume);
- _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.primary_storage, new Long(volume.getSize()));
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_CREATE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(),
- volume.getDiskOfferingId(), volume.getTemplateId(), volume.getSize(), Volume.class.getName(),
- volume.getUuid(), volume.isDisplayVolume());
- }
-
- //update resource count of new account
- if (! VirtualMachineManager.ResourceCountRunningVMsonly.value()) {
- resourceCountIncrement(newAccount.getAccountId(), vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
- }
+ s_logger.trace(String.format("Decrementing old account [%s] resource count.", oldAccount));
+ resourceCountDecrement(oldAccount.getAccountId(), vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));
- //generate usage events to account for this change
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VM_CREATE, vm.getAccountId(), vm.getDataCenterId(), vm.getId(),
- vm.getHostName(), vm.getServiceOfferingId(), vm.getTemplateId(), vm.getHypervisorType().toString(),
- VirtualMachine.class.getName(), vm.getUuid(), vm.isDisplayVm());
- }
- });
+ s_logger.trace(String.format("Removing VM [%s] from its instance group.", vm));
+ removeInstanceFromInstanceGroup(vm.getId());
+
+ Long newAccountId = newAccount.getAccountId();
+ updateVmOwner(newAccount, vm, domainId, newAccountId);
+
+ updateVolumesOwner(volumes, oldAccount, newAccount, newAccountId);
+
+ try {
+ updateVmNetwork(cmd, caller, vm, newAccount, template);
+ } catch (InsufficientCapacityException | ResourceAllocationException e) {
Review Comment:
`moveVmToUser` already publishes an event in case an error occurs.
--
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
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #7061: Rollback of changes with errors during the VM assign
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1375993515
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=7061)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7061&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7061&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_coverage&view=list) [75.3% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7061&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Rollback of changes with errors during the VM assign [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #7061:
URL: https://github.com/apache/cloudstack/pull/7061#issuecomment-1835888562
@blueorangutan package
--
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