You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by nvazquez <gi...@git.apache.org> on 2017/02/06 15:10:13 UTC
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
GitHub user nvazquez opened a pull request:
https://github.com/apache/cloudstack/pull/1935
CLOUDSTACK-9764: Delete domain failure due to Account Cleanup task
It was noticed in production environments that `deleteDomain` task failed for domains with multiple accounts and resources. Examining logs it was found out that if Account Cleanup Task got executed after domain (and all of its subchilds) got marked as Inactive; and before delete domain task finishes, it produces a failure.
`AccountCleanupTask` gets executed every `account.cleanup.interval` seconds looking for:
* Removed accounts
* Disabled accounts
* Inactive domains
As `deleteDomain` marks domain to delete (and its subchilds) as Inactive before deleting them, when `AccountCleanupTask` is executed, it removes marked domains. When there are resources to cleanup on domain accounts, domain is not found throwing exception: `com.cloud.exception.InvalidParameterValueException: Please specify a valid domain ID`
### Example
`account.cleanup.interval` = 100
````
2017-01-26 06:07:03,621 DEBUG [cloud.api.ApiServlet] (catalina-exec-8:ctx-50cfa3b6 ctx-92ad5b38) ===END=== 10.39.251.17 -- GET command=deleteDomain&id=1910a3dc-6fa6-457b-ab3a-602b0cfb6686&cleanup=true&response=json&_=1485439623475
...
// Domain and its subchilds marked as Inactive
2017-01-26 06:07:03,640 DEBUG [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Marking domain id=27 as Inactive before actually deleting it
2017-01-26 06:07:03,646 DEBUG [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Cleaning up domain id=27
2017-01-26 06:07:03,670 DEBUG [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Cleaning up domain id=28
2017-01-26 06:07:03,685 DEBUG [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Cleaning up domain id=29
...
// AccountCleanupTask removes Inactive domain id=29, no rollback for it
2017-01-26 06:07:44,285 INFO [cloud.user.AccountManagerImpl] (AccountChecker-1:ctx-b8a01824) Found 0 removed accounts to cleanup
2017-01-26 06:07:44,287 INFO [cloud.user.AccountManagerImpl] (AccountChecker-1:ctx-b8a01824) Found 0 disabled accounts to cleanup
2017-01-26 06:07:44,289 INFO [cloud.user.AccountManagerImpl] (AccountChecker-1:ctx-b8a01824) Found 3 inactive domains to cleanup
2017-01-26 06:07:44,292 DEBUG [cloud.user.AccountManagerImpl] (AccountChecker-1:ctx-b8a01824) Removing inactive domain id=27
2017-01-26 06:07:44,297 DEBUG [db.Transaction.Transaction] (AccountChecker-1:ctx-b8a01824) Rolling back the transaction: Time = 2 Name = AccountChecker-1; called by -TransactionLegacy.rollback:889-TransactionLegacy.removeUpTo:832-TransactionLegacy.close:656-TransactionContextInterceptor.invoke:36-ReflectiveMethodInvocation.proceed:161-ExposeInvocationInterceptor.invoke:91-ReflectiveMethodInvocation.proceed:172-JdkDynamicAopProxy.invoke:204-$Proxy63.remove:-1-DomainManagerImpl.removeDomain:248-NativeMethodAccessorImpl.invoke0:-2-NativeMethodAccessorImpl.invoke:62
2017-01-26 06:07:44,301 DEBUG [cloud.user.AccountManagerImpl] (AccountChecker-1:ctx-b8a01824) Removing inactive domain id=28
2017-01-26 06:07:44,304 DEBUG [db.Transaction.Transaction] (AccountChecker-1:ctx-b8a01824) Rolling back the transaction: Time = 2 Name = AccountChecker-1; called by -TransactionLegacy.rollback:889-TransactionLegacy.removeUpTo:832-TransactionLegacy.close:656-TransactionContextInterceptor.invoke:36-ReflectiveMethodInvocation.proceed:161-ExposeInvocationInterceptor.invoke:91-ReflectiveMethodInvocation.proceed:172-JdkDynamicAopProxy.invoke:204-$Proxy63.remove:-1-DomainManagerImpl.removeDomain:248-NativeMethodAccessorImpl.invoke0:-2-NativeMethodAccessorImpl.invoke:62
2017-01-26 06:07:44,307 DEBUG [cloud.user.AccountManagerImpl] (AccountChecker-1:ctx-b8a01824) Removing inactive domain id=29
2017-01-26 06:07:44,319 INFO [cloud.user.AccountManagerImpl] (AccountChecker-1:ctx-b8a01824) Found 0 disabled projects to cleanup
...
// Failure due to domain is already removed
2017-01-26 06:07:46,369 WARN [cloud.user.AccountManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Failed to cleanup account Acct[6a6e63ad-d89b-4a53-b3ae-1c06ea3d1899-ac2] due to
com.cloud.exception.InvalidParameterValueException: Please specify a valid domain ID.
at com.cloud.resourcelimit.ResourceLimitManagerImpl.recalculateResourceCount(ResourceLimitManagerImpl.java:752)
at com.cloud.vm.UserVmManagerImpl.expunge(UserVmManagerImpl.java:2053)
...
2017-01-26 06:07:46,381 INFO [cloud.user.AccountManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Cleanup for account 2580 is needed.
2017-01-26 06:07:46,390 DEBUG [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Deleting networks for domain id=29
2017-01-26 06:07:46,392 DEBUG [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Can't delete the domain yet because it has 1accounts that need a cleanup
2017-01-26 06:07:46,392 WARN [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Failed to cleanup domain id=29
2017-01-26 06:07:46,394 DEBUG [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Deleting networks for domain id=28
2017-01-26 06:07:46,416 WARN [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Failed to cleanup domain id=28
2017-01-26 06:07:46,418 DEBUG [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Deleting networks for domain id=27
2017-01-26 06:07:46,440 ERROR [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Exception deleting domain with id 27
com.cloud.utils.exception.CloudRuntimeException: Failed to clean up domain resources and sub domains, delete failed on domain A (id: 27).
...
2017-01-26 06:07:46,441 DEBUG [cloud.user.DomainManagerImpl] (API-Job-Executor-29:ctx-23415942 job-7165 ctx-fe3d13d6) Changing domain id=27 state back to Active because it can't be removed due to resources referencing to it
2017-01-26 06:07:46,459 ERROR [cloud.api.ApiAsyncJobDispatcher] (API-Job-Executor-29:ctx-23415942 job-7165) Unexpected exception while executing org.apache.cloudstack.api.command.admin.domain.DeleteDomainCmd
com.cloud.utils.exception.CloudRuntimeException: Failed to clean up domain resources and sub domains, delete failed on domain A (id: 27).
at com.cloud.user.DomainManagerImpl.deleteDomain(DomainManagerImpl.java:290)
at com.cloud.user.DomainManagerImpl.deleteDomain(DomainManagerImpl.java:271)
...
``
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nvazquez/cloudstack deleteDomainFix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1935.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1935
----
commit 8c3ee71ab8bf09defe494e137c5c6ac866424b80
Author: nvazquez <ni...@gmail.com>
Date: 2017-02-06T15:04:34Z
CLOUDSTACK-9764: Delete domain failure due to Account Cleanup task
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102322603
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +289,133 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
- }
+ checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ }
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException)
+ throw (CloudRuntimeException)ex;
+ else
+ return false;
+ } finally {
+ //when success is false
+ if (rollBackState) {
+ s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
+ " because it can't be removed due to resources referencing to it");
+ domain.setState(Domain.State.Active);
+ _domainDao.update(domain.getId(), domain);
}
}
+ }
+ finally {
+ lock.unlock();
+ rollBackState = false;
+ }
+ }
- cleanupDomainOfferings(domain.getId());
- CallContext.current().putContextParameter(Domain.class, domain.getUuid());
- return true;
- } catch (Exception ex) {
- s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
- if (ex instanceof CloudRuntimeException)
- throw (CloudRuntimeException)ex;
- else
- return false;
- } finally {
- //when success is false
- if (rollBackState) {
- s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
- " because it can't be removed due to resources referencing to it");
- domain.setState(Domain.State.Active);
- _domainDao.update(domain.getId(), domain);
- }
+ /**
+ * Check domain resources before removing domain. There are 2 cases:
+ * <ol>
+ * <li>Domain doesn't have accounts for cleanup, non-removed networks, or dedicated resources</li>
+ * <ul><li>Delete domain</li></ul>
+ * <li>Domain has one of the following: accounts set for cleanup, non-removed networks, dedicated resources</li>
+ * <ul><li>Dont' delete domain</li><li>Fail operation</li></ul>
+ * </ol>
+ * @param domain domain to remove
+ * @throws CloudRuntimeException when case 2 or when domain cannot be deleted on case 1
+ */
+ protected void checkDomainAccountsNetworksAndResourcesBeforeRemoving(DomainVO domain) {
+ boolean hasDedicatedResources = false;
+ List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
+ List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
+ List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
+ if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
--- End diff --
what about using "org.apache.commons.collections.CollectionUtils.isNotEmpty(Collection)" here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
<b>Trillian test result (tid-896)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32396 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1935-t896-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Test completed. 46 look ok, 3 have error(s)
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 363.77 | test_vpc_redundant.py
test_04_rvpc_privategw_static_routes | `Failure` | 345.94 | test_privategw_acl.py
test_02_list_snapshots_with_removed_data_store | `Error` | 0.04 | test_snapshots.py
test_01_vpc_site2site_vpn | Success | 141.33 | test_vpc_vpn.py
test_01_vpc_remote_access_vpn | Success | 56.08 | test_vpc_vpn.py
test_01_redundant_vpc_site2site_vpn | Success | 231.05 | test_vpc_vpn.py
test_02_VPC_default_routes | Success | 258.91 | test_vpc_router_nics.py
test_01_VPC_nics_after_destroy | Success | 531.86 | test_vpc_router_nics.py
test_05_rvpc_multi_tiers | Success | 521.44 | test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics | Success | 1407.93 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Success | 537.92 | test_vpc_redundant.py
test_02_redundant_VPC_default_routes | Success | 755.43 | test_vpc_redundant.py
test_09_delete_detached_volume | Success | 151.68 | test_volumes.py
test_08_resize_volume | Success | 156.42 | test_volumes.py
test_07_resize_fail | Success | 161.56 | test_volumes.py
test_06_download_detached_volume | Success | 156.35 | test_volumes.py
test_05_detach_volume | Success | 145.71 | test_volumes.py
test_04_delete_attached_volume | Success | 146.22 | test_volumes.py
test_03_download_attached_volume | Success | 156.37 | test_volumes.py
test_02_attach_volume | Success | 84.13 | test_volumes.py
test_01_create_volume | Success | 621.25 | test_volumes.py
test_03_delete_vm_snapshots | Success | 275.20 | test_vm_snapshots.py
test_02_revert_vm_snapshots | Success | 95.69 | test_vm_snapshots.py
test_01_create_vm_snapshots | Success | 163.72 | test_vm_snapshots.py
test_deploy_vm_multiple | Success | 232.62 | test_vm_life_cycle.py
test_deploy_vm | Success | 0.04 | test_vm_life_cycle.py
test_advZoneVirtualRouter | Success | 0.03 | test_vm_life_cycle.py
test_10_attachAndDetach_iso | Success | 26.52 | test_vm_life_cycle.py
test_09_expunge_vm | Success | 125.25 | test_vm_life_cycle.py
test_08_migrate_vm | Success | 35.90 | test_vm_life_cycle.py
test_07_restore_vm | Success | 0.13 | test_vm_life_cycle.py
test_06_destroy_vm | Success | 125.94 | test_vm_life_cycle.py
test_03_reboot_vm | Success | 125.90 | test_vm_life_cycle.py
test_02_start_vm | Success | 10.17 | test_vm_life_cycle.py
test_01_stop_vm | Success | 35.30 | test_vm_life_cycle.py
test_CreateTemplateWithDuplicateName | Success | 50.55 | test_templates.py
test_08_list_system_templates | Success | 0.03 | test_templates.py
test_07_list_public_templates | Success | 0.06 | test_templates.py
test_05_template_permissions | Success | 0.06 | test_templates.py
test_04_extract_template | Success | 5.14 | test_templates.py
test_03_delete_template | Success | 5.13 | test_templates.py
test_02_edit_template | Success | 90.13 | test_templates.py
test_01_create_template | Success | 30.36 | test_templates.py
test_10_destroy_cpvm | Success | 161.39 | test_ssvm.py
test_09_destroy_ssvm | Success | 138.56 | test_ssvm.py
test_08_reboot_cpvm | Success | 101.35 | test_ssvm.py
test_07_reboot_ssvm | Success | 133.54 | test_ssvm.py
test_06_stop_cpvm | Success | 131.57 | test_ssvm.py
test_05_stop_ssvm | Success | 163.76 | test_ssvm.py
test_04_cpvm_internals | Success | 0.97 | test_ssvm.py
test_03_ssvm_internals | Success | 3.30 | test_ssvm.py
test_02_list_cpvm_vm | Success | 0.13 | test_ssvm.py
test_01_list_sec_storage_vm | Success | 0.15 | test_ssvm.py
test_01_snapshot_root_disk | Success | 11.27 | test_snapshots.py
test_04_change_offering_small | Success | 234.58 | test_service_offerings.py
test_03_delete_service_offering | Success | 0.04 | test_service_offerings.py
test_02_edit_service_offering | Success | 0.06 | test_service_offerings.py
test_01_create_service_offering | Success | 0.11 | test_service_offerings.py
test_02_sys_template_ready | Success | 0.15 | test_secondary_storage.py
test_01_sys_vm_start | Success | 0.18 | test_secondary_storage.py
test_09_reboot_router | Success | 35.33 | test_routers.py
test_08_start_router | Success | 25.25 | test_routers.py
test_07_stop_router | Success | 10.18 | test_routers.py
test_06_router_advanced | Success | 0.06 | test_routers.py
test_05_router_basic | Success | 0.04 | test_routers.py
test_04_restart_network_wo_cleanup | Success | 5.61 | test_routers.py
test_03_restart_network_cleanup | Success | 50.47 | test_routers.py
test_02_router_internal_adv | Success | 0.85 | test_routers.py
test_01_router_internal_basic | Success | 0.46 | test_routers.py
test_router_dns_guestipquery | Success | 76.81 | test_router_dns.py
test_router_dns_externalipquery | Success | 0.06 | test_router_dns.py
test_router_dhcphosts | Success | 252.19 | test_router_dhcphosts.py
test_router_dhcp_opts | Success | 21.69 | test_router_dhcphosts.py
test_01_updatevolumedetail | Success | 0.08 | test_resource_detail.py
test_01_reset_vm_on_reboot | Success | 151.03 | test_reset_vm_on_reboot.py
test_createRegion | Success | 0.04 | test_regions.py
test_create_pvlan_network | Success | 5.21 | test_pvlan.py
test_dedicatePublicIpRange | Success | 0.60 | test_public_ip_range.py
test_03_vpc_privategw_restart_vpc_cleanup | Success | 465.28 | test_privategw_acl.py
test_02_vpc_privategw_static_routes | Success | 365.87 | test_privategw_acl.py
test_01_vpc_privategw_acl | Success | 82.63 | test_privategw_acl.py
test_01_primary_storage_nfs | Success | 35.78 | test_primary_storage.py
test_createPortablePublicIPRange | Success | 15.21 | test_portable_publicip.py
test_createPortablePublicIPAcquire | Success | 15.45 | test_portable_publicip.py
test_isolate_network_password_server | Success | 59.08 | test_password_server.py
test_UpdateStorageOverProvisioningFactor | Success | 0.13 | test_over_provisioning.py
test_oobm_zchange_password | Success | 30.74 | test_outofbandmanagement.py
test_oobm_multiple_mgmt_server_ownership | Success | 16.34 | test_outofbandmanagement.py
test_oobm_issue_power_status | Success | 10.25 | test_outofbandmanagement.py
test_oobm_issue_power_soft | Success | 15.50 | test_outofbandmanagement.py
test_oobm_issue_power_reset | Success | 15.34 | test_outofbandmanagement.py
test_oobm_issue_power_on | Success | 15.36 | test_outofbandmanagement.py
test_oobm_issue_power_off | Success | 15.33 | test_outofbandmanagement.py
test_oobm_issue_power_cycle | Success | 15.36 | test_outofbandmanagement.py
test_oobm_enabledisable_across_clusterzones | Success | 87.70 | test_outofbandmanagement.py
test_oobm_enable_feature_valid | Success | 5.34 | test_outofbandmanagement.py
test_oobm_enable_feature_invalid | Success | 0.09 | test_outofbandmanagement.py
test_oobm_disable_feature_valid | Success | 5.19 | test_outofbandmanagement.py
test_oobm_disable_feature_invalid | Success | 0.12 | test_outofbandmanagement.py
test_oobm_configure_invalid_driver | Success | 0.09 | test_outofbandmanagement.py
test_oobm_configure_default_driver | Success | 0.08 | test_outofbandmanagement.py
test_oobm_background_powerstate_sync | Success | 23.41 | test_outofbandmanagement.py
test_extendPhysicalNetworkVlan | Success | 15.32 | test_non_contigiousvlan.py
test_01_nic | Success | 424.37 | test_nic.py
test_releaseIP | Success | 142.26 | test_network.py
test_reboot_router | Success | 403.52 | test_network.py
test_public_ip_user_account | Success | 10.26 | test_network.py
test_public_ip_admin_account | Success | 40.27 | test_network.py
test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Success | 66.77 | test_network.py
test_network_rules_acquired_public_ip_2_nat_rule | Success | 61.55 | test_network.py
test_network_rules_acquired_public_ip_1_static_nat_rule | Success | 123.69 | test_network.py
test_delete_account | Success | 263.32 | test_network.py
test_02_port_fwd_on_non_src_nat | Success | 55.68 | test_network.py
test_01_port_fwd_on_src_nat | Success | 111.72 | test_network.py
test_nic_secondaryip_add_remove | Success | 202.72 | test_multipleips_per_nic.py
login_test_saml_user | Success | 19.27 | test_login.py
test_assign_and_removal_lb | Success | 133.24 | test_loadbalance.py
test_02_create_lb_rule_non_nat | Success | 187.55 | test_loadbalance.py
test_01_create_lb_rule_src_nat | Success | 217.79 | test_loadbalance.py
test_03_list_snapshots | Success | 0.09 | test_list_ids_parameter.py
test_02_list_templates | Success | 0.04 | test_list_ids_parameter.py
test_01_list_volumes | Success | 0.03 | test_list_ids_parameter.py
test_07_list_default_iso | Success | 0.06 | test_iso.py
test_05_iso_permissions | Success | 0.07 | test_iso.py
test_04_extract_Iso | Success | 5.17 | test_iso.py
test_03_delete_iso | Success | 95.19 | test_iso.py
test_02_edit_iso | Success | 0.06 | test_iso.py
test_01_create_iso | Success | 21.01 | test_iso.py
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces | Success | 193.08 | test_internal_lb.py
test_03_vpc_internallb_haproxy_stats_on_all_interfaces | Success | 127.71 | test_internal_lb.py
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80 | Success | 480.11 | test_internal_lb.py
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Success | 430.12 | test_internal_lb.py
test_dedicateGuestVlanRange | Success | 10.27 | test_guest_vlan_range.py
test_UpdateConfigParamWithScope | Success | 0.18 | test_global_settings.py
test_rolepermission_lifecycle_update | Success | 6.40 | test_dynamicroles.py
test_rolepermission_lifecycle_list | Success | 5.99 | test_dynamicroles.py
test_rolepermission_lifecycle_delete | Success | 5.86 | test_dynamicroles.py
test_rolepermission_lifecycle_create | Success | 5.89 | test_dynamicroles.py
test_rolepermission_lifecycle_concurrent_updates | Success | 6.00 | test_dynamicroles.py
test_role_lifecycle_update_role_inuse | Success | 5.91 | test_dynamicroles.py
test_role_lifecycle_update | Success | 11.02 | test_dynamicroles.py
test_role_lifecycle_list | Success | 5.90 | test_dynamicroles.py
test_role_lifecycle_delete | Success | 10.95 | test_dynamicroles.py
test_role_lifecycle_create | Success | 5.89 | test_dynamicroles.py
test_role_inuse_deletion | Success | 5.88 | test_dynamicroles.py
test_role_account_acls_multiple_mgmt_servers | Success | 8.07 | test_dynamicroles.py
test_role_account_acls | Success | 8.40 | test_dynamicroles.py
test_default_role_deletion | Success | 5.98 | test_dynamicroles.py
test_04_create_fat_type_disk_offering | Success | 0.07 | test_disk_offerings.py
test_03_delete_disk_offering | Success | 0.04 | test_disk_offerings.py
test_02_edit_disk_offering | Success | 0.05 | test_disk_offerings.py
test_02_create_sparse_type_disk_offering | Success | 0.07 | test_disk_offerings.py
test_01_create_disk_offering | Success | 0.10 | test_disk_offerings.py
test_deployvm_userdispersing | Success | 20.60 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userconcentrated | Success | 50.90 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_firstfit | Success | 55.69 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userdata_post | Success | 10.48 | test_deploy_vm_with_userdata.py
test_deployvm_userdata | Success | 45.79 | test_deploy_vm_with_userdata.py
test_02_deploy_vm_root_resize | Success | 5.98 | test_deploy_vm_root_resize.py
test_01_deploy_vm_root_resize | Success | 6.00 | test_deploy_vm_root_resize.py
test_00_deploy_vm_root_resize | Success | 222.52 | test_deploy_vm_root_resize.py
test_deploy_vm_from_iso | Success | 202.50 | test_deploy_vm_iso.py
test_DeployVmAntiAffinityGroup | Success | 61.12 | test_affinity_groups.py
test_change_service_offering_for_vm_with_snapshots | Skipped | 0.00 | test_vm_snapshots.py
test_01_test_vm_volume_snapshot | Skipped | 0.00 | test_vm_snapshots.py
test_06_copy_template | Skipped | 0.00 | test_templates.py
test_static_role_account_acls | Skipped | 0.02 | test_staticroles.py
test_11_ss_nfs_version_on_ssvm | Skipped | 0.02 | test_ssvm.py
test_01_scale_vm | Skipped | 0.00 | test_scale_vm.py
test_01_primary_storage_iscsi | Skipped | 0.04 | test_primary_storage.py
test_nested_virtualization_vmware | Skipped | 0.00 | test_nested_virtualization.py
test_06_copy_iso | Skipped | 0.00 | test_iso.py
test_deploy_vgpu_enabled_vm | Skipped | 0.03 | test_deploy_vgpu_enabled_vm.py
test_3d_gpu_support | Skipped | 0.04 | test_deploy_vgpu_enabled_vm.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102529801
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +289,133 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
--- End diff --
Done, I liked it how it simplified the code inside those blocks. However, I still find it difficult to name methods, do you agree with name `tryCleanupDomain`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@rafaelweingartner Can you review latest updates from @nvazquez . Since tests are passing this PR will be ready for merging to 4.10
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@rafaelweingartner thanks for reviewing again! Minor refactor pushed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102531047
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,67 @@ public void testFindDomainByIdOrPathValidId() {
Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, "/validDomain/"));
}
+ @Test(expected=InvalidParameterValueException.class)
+ public void testDeleteDomainNullDomain() {
+ Mockito.when(_domainDao.findById(DOMAIN_ID)).thenReturn(null);
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ }
+
+ @Test(expected=PermissionDeniedException.class)
+ public void testDeleteDomainRootDomain() {
+ Mockito.when(_domainDao.findById(Domain.ROOT_DOMAIN)).thenReturn(domain);
+ domainManager.deleteDomain(Domain.ROOT_DOMAIN, testDomainCleanup);
+ }
+
+ //TODO testDeleteDomainCleanup
+
+ @Test
+ public void testDeleteDomainNoCleanup() {
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ Mockito.verify(domainManager).deleteDomain(domain, testDomainCleanup);
+ Mockito.verify(domainManager).checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+ Mockito.verify(lock).unlock();
+ Assert.assertEquals(false, domainManager.getRollBackState());
+ }
+
+ @Test
+ public void testCheckDomainAccountsNetworksAndResourcesBeforeRemovingRemoveDomain() {
+ domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testCheckDomainAccountsNetworksAndResourcesBeforeRemovingDontRemoveDomain() {
+ domainNetworkIds.add(2l);
+ domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, false);
+ }
+
+ @Test
+ public void testPublishRemoveEventsAndRemoveDomainSuccessfulDelete() {
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testPublishRemoveEventsAndRemoveDomainExceptionDelete() {
+ Mockito.when(_domainDao.remove(DOMAIN_ID)).thenReturn(false);
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus, Mockito.never()).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testFailRemoveOperation() {
--- End diff --
Great, I've added verification, I have missed that out, thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-614
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by borisstoyanov <gi...@git.apache.org>.
Github user borisstoyanov commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@blueorangutan package
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@rafaelweingartner no problem, I should have mentioned about changing the variable to static. I'll work on your last comments :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r100574299
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+ boolean rollBackState = false;
+ boolean hasDedicatedResources = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if ((cleanup != null) && cleanup.booleanValue()) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
--- End diff --
What about extracting this "else block" to a method?
Lines 308-340
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
<b>Trillian test result (tid-810)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32603 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1935-t810-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Test completed. 47 look ok, 2 have error(s)
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_02_redundant_VPC_default_routes | `Failure` | 854.15 | test_vpc_redundant.py
test_04_rvpc_privategw_static_routes | `Failure` | 310.34 | test_privategw_acl.py
test_01_vpc_site2site_vpn | Success | 159.79 | test_vpc_vpn.py
test_01_vpc_remote_access_vpn | Success | 66.17 | test_vpc_vpn.py
test_01_redundant_vpc_site2site_vpn | Success | 236.31 | test_vpc_vpn.py
test_02_VPC_default_routes | Success | 253.89 | test_vpc_router_nics.py
test_01_VPC_nics_after_destroy | Success | 533.13 | test_vpc_router_nics.py
test_05_rvpc_multi_tiers | Success | 505.05 | test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics | Success | 1413.32 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Success | 543.16 | test_vpc_redundant.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Success | 1269.24 | test_vpc_redundant.py
test_09_delete_detached_volume | Success | 156.44 | test_volumes.py
test_08_resize_volume | Success | 151.68 | test_volumes.py
test_07_resize_fail | Success | 161.54 | test_volumes.py
test_06_download_detached_volume | Success | 156.35 | test_volumes.py
test_05_detach_volume | Success | 150.77 | test_volumes.py
test_04_delete_attached_volume | Success | 151.29 | test_volumes.py
test_03_download_attached_volume | Success | 156.30 | test_volumes.py
test_02_attach_volume | Success | 84.34 | test_volumes.py
test_01_create_volume | Success | 621.02 | test_volumes.py
test_03_delete_vm_snapshots | Success | 275.20 | test_vm_snapshots.py
test_02_revert_vm_snapshots | Success | 95.77 | test_vm_snapshots.py
test_01_create_vm_snapshots | Success | 158.77 | test_vm_snapshots.py
test_deploy_vm_multiple | Success | 262.74 | test_vm_life_cycle.py
test_deploy_vm | Success | 0.03 | test_vm_life_cycle.py
test_advZoneVirtualRouter | Success | 0.02 | test_vm_life_cycle.py
test_10_attachAndDetach_iso | Success | 26.62 | test_vm_life_cycle.py
test_09_expunge_vm | Success | 125.25 | test_vm_life_cycle.py
test_08_migrate_vm | Success | 36.05 | test_vm_life_cycle.py
test_07_restore_vm | Success | 0.14 | test_vm_life_cycle.py
test_06_destroy_vm | Success | 125.82 | test_vm_life_cycle.py
test_03_reboot_vm | Success | 125.86 | test_vm_life_cycle.py
test_02_start_vm | Success | 10.18 | test_vm_life_cycle.py
test_01_stop_vm | Success | 35.30 | test_vm_life_cycle.py
test_CreateTemplateWithDuplicateName | Success | 40.47 | test_templates.py
test_08_list_system_templates | Success | 0.03 | test_templates.py
test_07_list_public_templates | Success | 0.04 | test_templates.py
test_05_template_permissions | Success | 0.06 | test_templates.py
test_04_extract_template | Success | 5.13 | test_templates.py
test_03_delete_template | Success | 5.10 | test_templates.py
test_02_edit_template | Success | 90.13 | test_templates.py
test_01_create_template | Success | 40.52 | test_templates.py
test_10_destroy_cpvm | Success | 161.42 | test_ssvm.py
test_09_destroy_ssvm | Success | 163.61 | test_ssvm.py
test_08_reboot_cpvm | Success | 101.56 | test_ssvm.py
test_07_reboot_ssvm | Success | 103.46 | test_ssvm.py
test_06_stop_cpvm | Success | 101.64 | test_ssvm.py
test_05_stop_ssvm | Success | 138.80 | test_ssvm.py
test_04_cpvm_internals | Success | 1.18 | test_ssvm.py
test_03_ssvm_internals | Success | 3.29 | test_ssvm.py
test_02_list_cpvm_vm | Success | 0.13 | test_ssvm.py
test_01_list_sec_storage_vm | Success | 0.13 | test_ssvm.py
test_01_snapshot_root_disk | Success | 11.27 | test_snapshots.py
test_04_change_offering_small | Success | 210.02 | test_service_offerings.py
test_03_delete_service_offering | Success | 0.04 | test_service_offerings.py
test_02_edit_service_offering | Success | 0.05 | test_service_offerings.py
test_01_create_service_offering | Success | 0.10 | test_service_offerings.py
test_02_sys_template_ready | Success | 0.12 | test_secondary_storage.py
test_01_sys_vm_start | Success | 0.18 | test_secondary_storage.py
test_09_reboot_router | Success | 35.30 | test_routers.py
test_08_start_router | Success | 25.25 | test_routers.py
test_07_stop_router | Success | 10.18 | test_routers.py
test_06_router_advanced | Success | 0.06 | test_routers.py
test_05_router_basic | Success | 0.04 | test_routers.py
test_04_restart_network_wo_cleanup | Success | 5.61 | test_routers.py
test_03_restart_network_cleanup | Success | 55.54 | test_routers.py
test_02_router_internal_adv | Success | 0.85 | test_routers.py
test_01_router_internal_basic | Success | 0.46 | test_routers.py
test_router_dns_guestipquery | Success | 76.73 | test_router_dns.py
test_router_dns_externalipquery | Success | 0.08 | test_router_dns.py
test_router_dhcphosts | Success | 241.90 | test_router_dhcphosts.py
test_router_dhcp_opts | Success | 21.77 | test_router_dhcphosts.py
test_01_updatevolumedetail | Success | 0.13 | test_resource_detail.py
test_01_reset_vm_on_reboot | Success | 130.92 | test_reset_vm_on_reboot.py
test_createRegion | Success | 0.04 | test_regions.py
test_create_pvlan_network | Success | 5.22 | test_pvlan.py
test_dedicatePublicIpRange | Success | 0.43 | test_public_ip_range.py
test_03_vpc_privategw_restart_vpc_cleanup | Success | 505.51 | test_privategw_acl.py
test_02_vpc_privategw_static_routes | Success | 335.16 | test_privategw_acl.py
test_01_vpc_privategw_acl | Success | 82.18 | test_privategw_acl.py
test_01_primary_storage_nfs | Success | 35.76 | test_primary_storage.py
test_createPortablePublicIPRange | Success | 15.20 | test_portable_publicip.py
test_createPortablePublicIPAcquire | Success | 15.47 | test_portable_publicip.py
test_isolate_network_password_server | Success | 59.97 | test_password_server.py
test_UpdateStorageOverProvisioningFactor | Success | 0.14 | test_over_provisioning.py
test_oobm_zchange_password | Success | 30.67 | test_outofbandmanagement.py
test_oobm_multiple_mgmt_server_ownership | Success | 16.40 | test_outofbandmanagement.py
test_oobm_issue_power_status | Success | 10.28 | test_outofbandmanagement.py
test_oobm_issue_power_soft | Success | 15.37 | test_outofbandmanagement.py
test_oobm_issue_power_reset | Success | 15.36 | test_outofbandmanagement.py
test_oobm_issue_power_on | Success | 15.33 | test_outofbandmanagement.py
test_oobm_issue_power_off | Success | 15.35 | test_outofbandmanagement.py
test_oobm_issue_power_cycle | Success | 15.35 | test_outofbandmanagement.py
test_oobm_enabledisable_across_clusterzones | Success | 92.78 | test_outofbandmanagement.py
test_oobm_enable_feature_valid | Success | 0.14 | test_outofbandmanagement.py
test_oobm_enable_feature_invalid | Success | 0.09 | test_outofbandmanagement.py
test_oobm_disable_feature_valid | Success | 5.18 | test_outofbandmanagement.py
test_oobm_disable_feature_invalid | Success | 0.11 | test_outofbandmanagement.py
test_oobm_configure_invalid_driver | Success | 0.09 | test_outofbandmanagement.py
test_oobm_configure_default_driver | Success | 0.08 | test_outofbandmanagement.py
test_oobm_background_powerstate_sync | Success | 23.43 | test_outofbandmanagement.py
test_extendPhysicalNetworkVlan | Success | 15.34 | test_non_contigiousvlan.py
test_01_nic | Success | 414.46 | test_nic.py
test_releaseIP | Success | 177.50 | test_network.py
test_reboot_router | Success | 403.61 | test_network.py
test_public_ip_user_account | Success | 10.28 | test_network.py
test_public_ip_admin_account | Success | 40.33 | test_network.py
test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Success | 66.73 | test_network.py
test_network_rules_acquired_public_ip_2_nat_rule | Success | 61.54 | test_network.py
test_network_rules_acquired_public_ip_1_static_nat_rule | Success | 120.72 | test_network.py
test_delete_account | Success | 293.02 | test_network.py
test_02_port_fwd_on_non_src_nat | Success | 55.77 | test_network.py
test_01_port_fwd_on_src_nat | Success | 111.80 | test_network.py
test_nic_secondaryip_add_remove | Success | 197.69 | test_multipleips_per_nic.py
login_test_saml_user | Success | 19.21 | test_login.py
test_assign_and_removal_lb | Success | 133.01 | test_loadbalance.py
test_02_create_lb_rule_non_nat | Success | 187.00 | test_loadbalance.py
test_01_create_lb_rule_src_nat | Success | 207.45 | test_loadbalance.py
test_03_list_snapshots | Success | 0.09 | test_list_ids_parameter.py
test_02_list_templates | Success | 0.04 | test_list_ids_parameter.py
test_01_list_volumes | Success | 0.03 | test_list_ids_parameter.py
test_07_list_default_iso | Success | 0.06 | test_iso.py
test_05_iso_permissions | Success | 0.07 | test_iso.py
test_04_extract_Iso | Success | 5.17 | test_iso.py
test_03_delete_iso | Success | 95.15 | test_iso.py
test_02_edit_iso | Success | 0.06 | test_iso.py
test_01_create_iso | Success | 21.01 | test_iso.py
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces | Success | 182.98 | test_internal_lb.py
test_03_vpc_internallb_haproxy_stats_on_all_interfaces | Success | 147.78 | test_internal_lb.py
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80 | Success | 495.22 | test_internal_lb.py
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Success | 439.65 | test_internal_lb.py
test_dedicateGuestVlanRange | Success | 10.31 | test_guest_vlan_range.py
test_UpdateConfigParamWithScope | Success | 0.13 | test_global_settings.py
test_rolepermission_lifecycle_update | Success | 6.31 | test_dynamicroles.py
test_rolepermission_lifecycle_list | Success | 5.99 | test_dynamicroles.py
test_rolepermission_lifecycle_delete | Success | 5.87 | test_dynamicroles.py
test_rolepermission_lifecycle_create | Success | 5.90 | test_dynamicroles.py
test_rolepermission_lifecycle_concurrent_updates | Success | 6.04 | test_dynamicroles.py
test_role_lifecycle_update_role_inuse | Success | 5.93 | test_dynamicroles.py
test_role_lifecycle_update | Success | 6.15 | test_dynamicroles.py
test_role_lifecycle_list | Success | 5.92 | test_dynamicroles.py
test_role_lifecycle_delete | Success | 10.96 | test_dynamicroles.py
test_role_lifecycle_create | Success | 5.92 | test_dynamicroles.py
test_role_inuse_deletion | Success | 5.88 | test_dynamicroles.py
test_role_account_acls_multiple_mgmt_servers | Success | 8.21 | test_dynamicroles.py
test_role_account_acls | Success | 8.43 | test_dynamicroles.py
test_default_role_deletion | Success | 5.99 | test_dynamicroles.py
test_04_create_fat_type_disk_offering | Success | 0.07 | test_disk_offerings.py
test_03_delete_disk_offering | Success | 0.04 | test_disk_offerings.py
test_02_edit_disk_offering | Success | 0.05 | test_disk_offerings.py
test_02_create_sparse_type_disk_offering | Success | 0.07 | test_disk_offerings.py
test_01_create_disk_offering | Success | 0.11 | test_disk_offerings.py
test_deployvm_userdispersing | Success | 20.73 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userconcentrated | Success | 45.78 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_firstfit | Success | 50.64 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userdata_post | Success | 10.48 | test_deploy_vm_with_userdata.py
test_deployvm_userdata | Success | 45.76 | test_deploy_vm_with_userdata.py
test_02_deploy_vm_root_resize | Success | 6.03 | test_deploy_vm_root_resize.py
test_01_deploy_vm_root_resize | Success | 5.99 | test_deploy_vm_root_resize.py
test_00_deploy_vm_root_resize | Success | 222.56 | test_deploy_vm_root_resize.py
test_deploy_vm_from_iso | Success | 207.50 | test_deploy_vm_iso.py
test_DeployVmAntiAffinityGroup | Success | 66.05 | test_affinity_groups.py
test_01_test_vm_volume_snapshot | Skipped | 0.00 | test_vm_snapshots.py
test_06_copy_template | Skipped | 0.00 | test_templates.py
test_static_role_account_acls | Skipped | 0.02 | test_staticroles.py
test_11_ss_nfs_version_on_ssvm | Skipped | 0.02 | test_ssvm.py
test_01_scale_vm | Skipped | 0.00 | test_scale_vm.py
test_01_primary_storage_iscsi | Skipped | 0.04 | test_primary_storage.py
test_nested_virtualization_vmware | Skipped | 0.00 | test_nested_virtualization.py
test_06_copy_iso | Skipped | 0.00 | test_iso.py
test_deploy_vgpu_enabled_vm | Skipped | 0.03 | test_deploy_vgpu_enabled_vm.py
test_3d_gpu_support | Skipped | 0.04 | test_deploy_vgpu_enabled_vm.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@nvazquez was the static declaration there before?! I am so sorry I missed it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r103529869
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,69 @@ public void testFindDomainByIdOrPathValidId() {
Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, "/validDomain/"));
}
+ @Test(expected=InvalidParameterValueException.class)
+ public void testDeleteDomainNullDomain() {
+ Mockito.when(_domainDao.findById(DOMAIN_ID)).thenReturn(null);
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ }
+
+ @Test(expected=PermissionDeniedException.class)
+ public void testDeleteDomainRootDomain() {
+ Mockito.when(_domainDao.findById(Domain.ROOT_DOMAIN)).thenReturn(domain);
+ domainManager.deleteDomain(Domain.ROOT_DOMAIN, testDomainCleanup);
+ }
+
+ @Test
+ public void testDeleteDomainNoCleanup() {
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ Mockito.verify(domainManager).deleteDomain(domain, testDomainCleanup);
+ Mockito.verify(domainManager).removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+ Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+ Mockito.verify(lock).unlock();
+ }
+
+ @Test
+ public void testRemoveDomainWithNoAccountsForCleanupNetworksOrDedicatedResourcesRemoveDomain() {
+ domainManager.removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+ Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testRemoveDomainWithNoAccountsForCleanupNetworksOrDedicatedResourcesDontRemoveDomain() {
+ domainNetworkIds.add(2l);
+ domainManager.removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+ Mockito.verify(domainManager).failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, false);
+ }
+
+ @Test
+ public void testPublishRemoveEventsAndRemoveDomainSuccessfulDelete() {
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testPublishRemoveEventsAndRemoveDomainExceptionDelete() {
+ Mockito.when(_domainDao.remove(DOMAIN_ID)).thenReturn(false);
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus, Mockito.never()).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test
+ public void testFailRemoveOperation() {
+ try {
+ domainManager.failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, true);
--- End diff --
Now that you removed the use of `rollBackState`, made the method last problematic to test. Therefore, you can use the `@Test(expected=...)`, instead of this very unusual construction here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-478
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@nvazquez great work.
However, there is a catch there that I think you might have overlooked. This problem is caused by the method extraction I suggested.
If you take a look at the code before the extraction, every time that an exception is thrown, the code was setting the variable `rollBackState = true`. This happens at lines 287, 305, and 313. Now that the code was extracted, setting those variables to `true` does not work anymore, because of the context those variables are declared change.
In my opinion, this code was kind of weird before. It was throwing an exception that is caught right away and setting a control variable to be executed on `finally` block. The only reason I see for this is that if other exceptions that are not the ones generated at lines 292, 310, and 325 happen, and we do not want to execute the rollback for them. However, this seems error prone, leading to database inconsistencies.
I would change the "rollback" code (lines 342-345) to the catch block.
I do not know if I have been clear, we can discuss this further. I may have overlooked some bits of it as well (it is a quite complicated bit of code).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by borisstoyanov <gi...@git.apache.org>.
Github user borisstoyanov commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@blueorangutan test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102264841
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+ boolean rollBackState = false;
+ boolean hasDedicatedResources = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if ((cleanup != null) && cleanup.booleanValue()) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
+ //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
+ List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
+ List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
+ List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
+ if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
+ s_logger.error("There are dedicated resources for the domain " + domain.getId());
+ hasDedicatedResources = true;
+ }
+ if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
+ _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ if (!_domainDao.remove(domain.getId())) {
+ rollBackState = true;
+ CloudRuntimeException e =
+ new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
+ "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ e.addProxyObject(domain.getUuid(), "domainId");
+ throw e;
+ }
+ _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ } else {
+ rollBackState = true;
+ String msg = null;
+ if (!accountsForCleanup.isEmpty()) {
+ msg = accountsForCleanup.size() + " accounts to cleanup";
+ } else if (!networkIds.isEmpty()) {
+ msg = networkIds.size() + " non-removed networks";
+ } else if (hasDedicatedResources) {
+ msg = "dedicated resources.";
+ }
+
+ CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
+ e.addProxyObject(domain.getUuid(), "domainId");
+ throw e;
}
-
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
}
- }
- cleanupDomainOfferings(domain.getId());
- CallContext.current().putContextParameter(Domain.class, domain.getUuid());
- return true;
- } catch (Exception ex) {
- s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
- if (ex instanceof CloudRuntimeException)
- throw (CloudRuntimeException)ex;
- else
- return false;
- } finally {
- //when success is false
- if (rollBackState) {
- s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
- " because it can't be removed due to resources referencing to it");
- domain.setState(Domain.State.Active);
- _domainDao.update(domain.getId(), domain);
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException)
+ throw (CloudRuntimeException)ex;
+ else
+ return false;
+ } finally {
+ lock.unlock();
--- End diff --
Done, thanks @koushik-das!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102530895
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +289,133 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
- }
+ checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ }
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException)
+ throw (CloudRuntimeException)ex;
+ else
+ return false;
+ } finally {
+ //when success is false
+ if (rollBackState) {
+ s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
+ " because it can't be removed due to resources referencing to it");
+ domain.setState(Domain.State.Active);
+ _domainDao.update(domain.getId(), domain);
}
}
+ }
+ finally {
+ lock.unlock();
+ rollBackState = false;
+ }
+ }
- cleanupDomainOfferings(domain.getId());
- CallContext.current().putContextParameter(Domain.class, domain.getUuid());
- return true;
- } catch (Exception ex) {
- s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
- if (ex instanceof CloudRuntimeException)
- throw (CloudRuntimeException)ex;
- else
- return false;
- } finally {
- //when success is false
- if (rollBackState) {
- s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
- " because it can't be removed due to resources referencing to it");
- domain.setState(Domain.State.Active);
- _domainDao.update(domain.getId(), domain);
- }
+ /**
+ * Check domain resources before removing domain. There are 2 cases:
+ * <ol>
+ * <li>Domain doesn't have accounts for cleanup, non-removed networks, or dedicated resources</li>
+ * <ul><li>Delete domain</li></ul>
+ * <li>Domain has one of the following: accounts set for cleanup, non-removed networks, dedicated resources</li>
+ * <ul><li>Dont' delete domain</li><li>Fail operation</li></ul>
+ * </ol>
+ * @param domain domain to remove
+ * @throws CloudRuntimeException when case 2 or when domain cannot be deleted on case 1
+ */
+ protected void checkDomainAccountsNetworksAndResourcesBeforeRemoving(DomainVO domain) {
--- End diff --
I agree with you. I changed it to `removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources`. I think is quite more descriptive but still long
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by borisstoyanov <gi...@git.apache.org>.
Github user borisstoyanov commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@blueorangutan package
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r110421451
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,69 @@ public void testFindDomainByIdOrPathValidId() {
Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, "/validDomain/"));
}
+ @Test(expected=InvalidParameterValueException.class)
+ public void testDeleteDomainNullDomain() {
+ Mockito.when(_domainDao.findById(DOMAIN_ID)).thenReturn(null);
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ }
+
+ @Test(expected=PermissionDeniedException.class)
+ public void testDeleteDomainRootDomain() {
+ Mockito.when(_domainDao.findById(Domain.ROOT_DOMAIN)).thenReturn(domain);
+ domainManager.deleteDomain(Domain.ROOT_DOMAIN, testDomainCleanup);
+ }
+
+ @Test
+ public void testDeleteDomainNoCleanup() {
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ Mockito.verify(domainManager).deleteDomain(domain, testDomainCleanup);
+ Mockito.verify(domainManager).removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+ Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+ Mockito.verify(lock).unlock();
+ }
+
+ @Test
+ public void testRemoveDomainWithNoAccountsForCleanupNetworksOrDedicatedResourcesRemoveDomain() {
+ domainManager.removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+ Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testRemoveDomainWithNoAccountsForCleanupNetworksOrDedicatedResourcesDontRemoveDomain() {
+ domainNetworkIds.add(2l);
+ domainManager.removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+ Mockito.verify(domainManager).failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, false);
+ }
+
+ @Test
+ public void testPublishRemoveEventsAndRemoveDomainSuccessfulDelete() {
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testPublishRemoveEventsAndRemoveDomainExceptionDelete() {
+ Mockito.when(_domainDao.remove(DOMAIN_ID)).thenReturn(false);
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus, Mockito.never()).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test
+ public void testFailRemoveOperation() {
+ try {
+ domainManager.failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, true);
--- End diff --
Great, thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r100478202
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+ boolean rollBackState = false;
+ boolean hasDedicatedResources = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if ((cleanup != null) && cleanup.booleanValue()) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
+ //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
+ List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
+ List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
+ List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
+ if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
+ s_logger.error("There are dedicated resources for the domain " + domain.getId());
+ hasDedicatedResources = true;
+ }
+ if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
+ _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ if (!_domainDao.remove(domain.getId())) {
+ rollBackState = true;
+ CloudRuntimeException e =
+ new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
+ "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ e.addProxyObject(domain.getUuid(), "domainId");
+ throw e;
+ }
+ _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ } else {
+ rollBackState = true;
+ String msg = null;
+ if (!accountsForCleanup.isEmpty()) {
+ msg = accountsForCleanup.size() + " accounts to cleanup";
+ } else if (!networkIds.isEmpty()) {
+ msg = networkIds.size() + " non-removed networks";
+ } else if (hasDedicatedResources) {
+ msg = "dedicated resources.";
+ }
+
+ CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
+ e.addProxyObject(domain.getUuid(), "domainId");
+ throw e;
}
-
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
}
- }
- cleanupDomainOfferings(domain.getId());
- CallContext.current().putContextParameter(Domain.class, domain.getUuid());
- return true;
- } catch (Exception ex) {
- s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
- if (ex instanceof CloudRuntimeException)
- throw (CloudRuntimeException)ex;
- else
- return false;
- } finally {
- //when success is false
- if (rollBackState) {
- s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
- " because it can't be removed due to resources referencing to it");
- domain.setState(Domain.State.Active);
- _domainDao.update(domain.getId(), domain);
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException)
+ throw (CloudRuntimeException)ex;
+ else
+ return false;
+ } finally {
+ lock.unlock();
+ //when success is false
+ if (rollBackState) {
+ s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
+ " because it can't be removed due to resources referencing to it");
+ domain.setState(Domain.State.Active);
+ _domainDao.update(domain.getId(), domain);
+ }
}
}
+ catch (Exception e) {
--- End diff --
Is this catch all needed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102538324
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -109,6 +112,20 @@
@Inject
MessageBus _messageBus;
+ static boolean rollBackState = false;
--- End diff --
@nvazquez I have been thinking about this variable you introduced here. I think it can cause problems (concurrency problems). The `DomainManagerImpl` is a singleton. Therefore, it should not have state variables. The `rollBackState ` is acting as a state variable for requests that use `com.cloud.user.DomainManagerImpl.deleteDomain(DomainVO, Boolean)`. The problem is that every call should have its own context/state for `rollBackState`. However, this will not happen with the current implementation.
I think we should re-work the use of that variable. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
<b>Trillian test result (tid-876)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35807 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1935-t876-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_network.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Test completed. 46 look ok, 3 have error(s)
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_02_redundant_VPC_default_routes | `Failure` | 864.13 | test_vpc_redundant.py
test_04_rvpc_privategw_static_routes | `Failure` | 320.45 | test_privategw_acl.py
test_02_list_snapshots_with_removed_data_store | `Error` | 0.04 | test_snapshots.py
test_01_vpc_site2site_vpn | Success | 160.52 | test_vpc_vpn.py
test_01_vpc_remote_access_vpn | Success | 61.11 | test_vpc_vpn.py
test_01_redundant_vpc_site2site_vpn | Success | 250.72 | test_vpc_vpn.py
test_02_VPC_default_routes | Success | 287.25 | test_vpc_router_nics.py
test_01_VPC_nics_after_destroy | Success | 545.04 | test_vpc_router_nics.py
test_05_rvpc_multi_tiers | Success | 512.25 | test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics | Success | 1414.74 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Success | 548.99 | test_vpc_redundant.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Success | 1297.58 | test_vpc_redundant.py
test_09_delete_detached_volume | Success | 151.41 | test_volumes.py
test_08_resize_volume | Success | 156.44 | test_volumes.py
test_07_resize_fail | Success | 156.52 | test_volumes.py
test_06_download_detached_volume | Success | 156.34 | test_volumes.py
test_05_detach_volume | Success | 155.91 | test_volumes.py
test_04_delete_attached_volume | Success | 151.44 | test_volumes.py
test_03_download_attached_volume | Success | 151.32 | test_volumes.py
test_02_attach_volume | Success | 95.17 | test_volumes.py
test_01_create_volume | Success | 711.28 | test_volumes.py
test_03_delete_vm_snapshots | Success | 275.17 | test_vm_snapshots.py
test_02_revert_vm_snapshots | Success | 95.78 | test_vm_snapshots.py
test_01_create_vm_snapshots | Success | 163.76 | test_vm_snapshots.py
test_deploy_vm_multiple | Success | 247.75 | test_vm_life_cycle.py
test_deploy_vm | Success | 0.04 | test_vm_life_cycle.py
test_advZoneVirtualRouter | Success | 0.03 | test_vm_life_cycle.py
test_10_attachAndDetach_iso | Success | 26.64 | test_vm_life_cycle.py
test_09_expunge_vm | Success | 125.25 | test_vm_life_cycle.py
test_08_migrate_vm | Success | 40.94 | test_vm_life_cycle.py
test_07_restore_vm | Success | 0.13 | test_vm_life_cycle.py
test_06_destroy_vm | Success | 125.84 | test_vm_life_cycle.py
test_03_reboot_vm | Success | 125.87 | test_vm_life_cycle.py
test_02_start_vm | Success | 10.17 | test_vm_life_cycle.py
test_01_stop_vm | Success | 40.33 | test_vm_life_cycle.py
test_CreateTemplateWithDuplicateName | Success | 40.46 | test_templates.py
test_08_list_system_templates | Success | 0.03 | test_templates.py
test_07_list_public_templates | Success | 0.04 | test_templates.py
test_05_template_permissions | Success | 0.06 | test_templates.py
test_04_extract_template | Success | 5.16 | test_templates.py
test_03_delete_template | Success | 5.11 | test_templates.py
test_02_edit_template | Success | 90.18 | test_templates.py
test_01_create_template | Success | 40.43 | test_templates.py
test_10_destroy_cpvm | Success | 166.69 | test_ssvm.py
test_09_destroy_ssvm | Success | 163.56 | test_ssvm.py
test_08_reboot_cpvm | Success | 101.57 | test_ssvm.py
test_07_reboot_ssvm | Success | 163.59 | test_ssvm.py
test_06_stop_cpvm | Success | 132.19 | test_ssvm.py
test_05_stop_ssvm | Success | 164.02 | test_ssvm.py
test_04_cpvm_internals | Success | 1.22 | test_ssvm.py
test_03_ssvm_internals | Success | 3.42 | test_ssvm.py
test_02_list_cpvm_vm | Success | 0.12 | test_ssvm.py
test_01_list_sec_storage_vm | Success | 0.13 | test_ssvm.py
test_01_snapshot_root_disk | Success | 11.11 | test_snapshots.py
test_04_change_offering_small | Success | 210.27 | test_service_offerings.py
test_03_delete_service_offering | Success | 0.04 | test_service_offerings.py
test_02_edit_service_offering | Success | 0.05 | test_service_offerings.py
test_01_create_service_offering | Success | 0.11 | test_service_offerings.py
test_02_sys_template_ready | Success | 0.13 | test_secondary_storage.py
test_01_sys_vm_start | Success | 0.18 | test_secondary_storage.py
test_09_reboot_router | Success | 35.32 | test_routers.py
test_08_start_router | Success | 30.28 | test_routers.py
test_07_stop_router | Success | 10.17 | test_routers.py
test_06_router_advanced | Success | 0.06 | test_routers.py
test_05_router_basic | Success | 0.04 | test_routers.py
test_04_restart_network_wo_cleanup | Success | 5.67 | test_routers.py
test_03_restart_network_cleanup | Success | 60.68 | test_routers.py
test_02_router_internal_adv | Success | 1.04 | test_routers.py
test_01_router_internal_basic | Success | 0.58 | test_routers.py
test_router_dns_guestipquery | Success | 76.72 | test_router_dns.py
test_router_dns_externalipquery | Success | 0.08 | test_router_dns.py
test_router_dhcphosts | Success | 278.15 | test_router_dhcphosts.py
test_router_dhcp_opts | Success | 22.12 | test_router_dhcphosts.py
test_01_updatevolumedetail | Success | 5.12 | test_resource_detail.py
test_01_reset_vm_on_reboot | Success | 146.28 | test_reset_vm_on_reboot.py
test_createRegion | Success | 0.04 | test_regions.py
test_create_pvlan_network | Success | 5.21 | test_pvlan.py
test_dedicatePublicIpRange | Success | 0.43 | test_public_ip_range.py
test_03_vpc_privategw_restart_vpc_cleanup | Success | 469.94 | test_privategw_acl.py
test_02_vpc_privategw_static_routes | Success | 375.28 | test_privategw_acl.py
test_01_vpc_privategw_acl | Success | 92.20 | test_privategw_acl.py
test_01_primary_storage_nfs | Success | 35.76 | test_primary_storage.py
test_createPortablePublicIPRange | Success | 15.20 | test_portable_publicip.py
test_createPortablePublicIPAcquire | Success | 15.50 | test_portable_publicip.py
test_isolate_network_password_server | Success | 89.44 | test_password_server.py
test_UpdateStorageOverProvisioningFactor | Success | 0.13 | test_over_provisioning.py
test_oobm_zchange_password | Success | 30.71 | test_outofbandmanagement.py
test_oobm_multiple_mgmt_server_ownership | Success | 16.43 | test_outofbandmanagement.py
test_oobm_issue_power_status | Success | 10.23 | test_outofbandmanagement.py
test_oobm_issue_power_soft | Success | 15.36 | test_outofbandmanagement.py
test_oobm_issue_power_reset | Success | 15.34 | test_outofbandmanagement.py
test_oobm_issue_power_on | Success | 15.32 | test_outofbandmanagement.py
test_oobm_issue_power_off | Success | 15.39 | test_outofbandmanagement.py
test_oobm_issue_power_cycle | Success | 15.34 | test_outofbandmanagement.py
test_oobm_enabledisable_across_clusterzones | Success | 92.69 | test_outofbandmanagement.py
test_oobm_enable_feature_valid | Success | 5.16 | test_outofbandmanagement.py
test_oobm_enable_feature_invalid | Success | 0.09 | test_outofbandmanagement.py
test_oobm_disable_feature_valid | Success | 5.18 | test_outofbandmanagement.py
test_oobm_disable_feature_invalid | Success | 0.10 | test_outofbandmanagement.py
test_oobm_configure_invalid_driver | Success | 0.08 | test_outofbandmanagement.py
test_oobm_configure_default_driver | Success | 0.07 | test_outofbandmanagement.py
test_oobm_background_powerstate_sync | Success | 23.41 | test_outofbandmanagement.py
test_extendPhysicalNetworkVlan | Success | 15.32 | test_non_contigiousvlan.py
test_01_nic | Success | 394.05 | test_nic.py
test_releaseIP | Success | 242.77 | test_network.py
test_reboot_router | Success | 404.01 | test_network.py
test_public_ip_user_account | Success | 10.27 | test_network.py
test_public_ip_admin_account | Success | 40.35 | test_network.py
test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Success | 66.82 | test_network.py
test_network_rules_acquired_public_ip_2_nat_rule | Success | 61.71 | test_network.py
test_network_rules_acquired_public_ip_1_static_nat_rule | Success | 124.12 | test_network.py
test_delete_account | Success | 272.80 | test_network.py
test_02_port_fwd_on_non_src_nat | Success | 55.72 | test_network.py
test_01_port_fwd_on_src_nat | Success | 111.67 | test_network.py
test_nic_secondaryip_add_remove | Success | 207.69 | test_multipleips_per_nic.py
login_test_saml_user | Success | 19.23 | test_login.py
test_assign_and_removal_lb | Success | 133.50 | test_loadbalance.py
test_02_create_lb_rule_non_nat | Success | 187.13 | test_loadbalance.py
test_01_create_lb_rule_src_nat | Success | 217.69 | test_loadbalance.py
test_03_list_snapshots | Success | 0.06 | test_list_ids_parameter.py
test_02_list_templates | Success | 0.04 | test_list_ids_parameter.py
test_01_list_volumes | Success | 0.03 | test_list_ids_parameter.py
test_07_list_default_iso | Success | 0.06 | test_iso.py
test_05_iso_permissions | Success | 0.06 | test_iso.py
test_04_extract_Iso | Success | 5.21 | test_iso.py
test_03_delete_iso | Success | 95.17 | test_iso.py
test_02_edit_iso | Success | 0.06 | test_iso.py
test_01_create_iso | Success | 21.00 | test_iso.py
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces | Success | 193.08 | test_internal_lb.py
test_03_vpc_internallb_haproxy_stats_on_all_interfaces | Success | 143.24 | test_internal_lb.py
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80 | Success | 516.18 | test_internal_lb.py
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Success | 425.67 | test_internal_lb.py
test_dedicateGuestVlanRange | Success | 10.27 | test_guest_vlan_range.py
test_UpdateConfigParamWithScope | Success | 0.14 | test_global_settings.py
test_rolepermission_lifecycle_update | Success | 6.13 | test_dynamicroles.py
test_rolepermission_lifecycle_list | Success | 5.97 | test_dynamicroles.py
test_rolepermission_lifecycle_delete | Success | 5.84 | test_dynamicroles.py
test_rolepermission_lifecycle_create | Success | 5.87 | test_dynamicroles.py
test_rolepermission_lifecycle_concurrent_updates | Success | 5.99 | test_dynamicroles.py
test_role_lifecycle_update_role_inuse | Success | 5.91 | test_dynamicroles.py
test_role_lifecycle_update | Success | 5.95 | test_dynamicroles.py
test_role_lifecycle_list | Success | 5.88 | test_dynamicroles.py
test_role_lifecycle_delete | Success | 10.93 | test_dynamicroles.py
test_role_lifecycle_create | Success | 5.91 | test_dynamicroles.py
test_role_inuse_deletion | Success | 5.86 | test_dynamicroles.py
test_role_account_acls_multiple_mgmt_servers | Success | 8.23 | test_dynamicroles.py
test_role_account_acls | Success | 8.31 | test_dynamicroles.py
test_default_role_deletion | Success | 6.06 | test_dynamicroles.py
test_04_create_fat_type_disk_offering | Success | 0.07 | test_disk_offerings.py
test_03_delete_disk_offering | Success | 0.06 | test_disk_offerings.py
test_02_edit_disk_offering | Success | 0.05 | test_disk_offerings.py
test_02_create_sparse_type_disk_offering | Success | 0.07 | test_disk_offerings.py
test_01_create_disk_offering | Success | 0.10 | test_disk_offerings.py
test_deployvm_userdispersing | Success | 20.59 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userconcentrated | Success | 20.70 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_firstfit | Success | 85.85 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userdata_post | Success | 10.40 | test_deploy_vm_with_userdata.py
test_deployvm_userdata | Success | 70.86 | test_deploy_vm_with_userdata.py
test_02_deploy_vm_root_resize | Success | 5.97 | test_deploy_vm_root_resize.py
test_01_deploy_vm_root_resize | Success | 5.97 | test_deploy_vm_root_resize.py
test_00_deploy_vm_root_resize | Success | 202.39 | test_deploy_vm_root_resize.py
test_deploy_vm_from_iso | Success | 207.42 | test_deploy_vm_iso.py
test_DeployVmAntiAffinityGroup | Success | 61.00 | test_affinity_groups.py
test_change_service_offering_for_vm_with_snapshots | Skipped | 0.00 | test_vm_snapshots.py
test_01_test_vm_volume_snapshot | Skipped | 0.00 | test_vm_snapshots.py
test_06_copy_template | Skipped | 0.00 | test_templates.py
test_static_role_account_acls | Skipped | 0.02 | test_staticroles.py
test_11_ss_nfs_version_on_ssvm | Skipped | 0.02 | test_ssvm.py
test_01_scale_vm | Skipped | 0.00 | test_scale_vm.py
test_01_primary_storage_iscsi | Skipped | 0.04 | test_primary_storage.py
test_nested_virtualization_vmware | Skipped | 0.00 | test_nested_virtualization.py
test_06_copy_iso | Skipped | 0.00 | test_iso.py
test_deploy_vgpu_enabled_vm | Skipped | 0.03 | test_deploy_vgpu_enabled_vm.py
test_3d_gpu_support | Skipped | 0.03 | test_deploy_vgpu_enabled_vm.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102265150
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+ boolean rollBackState = false;
+ boolean hasDedicatedResources = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if ((cleanup != null) && cleanup.booleanValue()) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
+ //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
+ List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
+ List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
+ List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
+ if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
+ s_logger.error("There are dedicated resources for the domain " + domain.getId());
+ hasDedicatedResources = true;
+ }
+ if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
+ _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ if (!_domainDao.remove(domain.getId())) {
+ rollBackState = true;
+ CloudRuntimeException e =
+ new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
+ "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ e.addProxyObject(domain.getUuid(), "domainId");
+ throw e;
+ }
+ _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ } else {
+ rollBackState = true;
+ String msg = null;
+ if (!accountsForCleanup.isEmpty()) {
+ msg = accountsForCleanup.size() + " accounts to cleanup";
+ } else if (!networkIds.isEmpty()) {
+ msg = networkIds.size() + " non-removed networks";
+ } else if (hasDedicatedResources) {
+ msg = "dedicated resources.";
+ }
+
+ CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
+ e.addProxyObject(domain.getUuid(), "domainId");
+ throw e;
}
-
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
}
- }
- cleanupDomainOfferings(domain.getId());
- CallContext.current().putContextParameter(Domain.class, domain.getUuid());
- return true;
- } catch (Exception ex) {
- s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
- if (ex instanceof CloudRuntimeException)
- throw (CloudRuntimeException)ex;
- else
- return false;
- } finally {
- //when success is false
- if (rollBackState) {
- s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
- " because it can't be removed due to resources referencing to it");
- domain.setState(Domain.State.Active);
- _domainDao.update(domain.getId(), domain);
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException)
+ throw (CloudRuntimeException)ex;
+ else
+ return false;
+ } finally {
+ lock.unlock();
+ //when success is false
+ if (rollBackState) {
+ s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
+ " because it can't be removed due to resources referencing to it");
+ domain.setState(Domain.State.Active);
+ _domainDao.update(domain.getId(), domain);
+ }
}
}
+ catch (Exception e) {
--- End diff --
Actually it wasn't, I removed catch block, thanks @koushik-das
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102265306
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+ boolean rollBackState = false;
+ boolean hasDedicatedResources = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if ((cleanup != null) && cleanup.booleanValue()) {
--- End diff --
Done, thanks @rafaelweingartner
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@nvazquez now everything seems to be ok
LGTM.
Great job!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r100574038
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+ boolean rollBackState = false;
+ boolean hasDedicatedResources = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if ((cleanup != null) && cleanup.booleanValue()) {
--- End diff --
What about using "org.apache.commons.lang.BooleanUtils.toBoolean(Boolean)" here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1935
Hi @rafaelweingartner, I've refactored the code instead of using `rollBackState` as static. I think that using static variable could lead to a problem if new methods are invoked from another method different than `deleteDomain` method. Instead of declaring it as static, we reduced the scope again and only set it true when `CloudRuntimeException` is thrown. What do you think about this refactor? I tryied not to introduce major changes in original code
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r100575787
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+ boolean rollBackState = false;
+ boolean hasDedicatedResources = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if ((cleanup != null) && cleanup.booleanValue()) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
+ //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
+ List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
+ List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
+ List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
+ if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
+ s_logger.error("There are dedicated resources for the domain " + domain.getId());
+ hasDedicatedResources = true;
+ }
+ if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
+ _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
--- End diff --
What about extracting the content of this "�f" for a method?
lines 317-326
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@borisstoyanov I've rebased master branch, can we re-run tests on this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102487341
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +289,133 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
--- End diff --
@nvazquez you have already improve greatly this method.
How do you feel about another code extraction? lines 312-319 can be easily extracted to a method, enabling the reduction of the lines here in these try/try/finally/finally blocks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r100477949
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+ boolean rollBackState = false;
+ boolean hasDedicatedResources = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if ((cleanup != null) && cleanup.booleanValue()) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
+ //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
+ List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
+ List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
+ List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
+ if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
+ s_logger.error("There are dedicated resources for the domain " + domain.getId());
+ hasDedicatedResources = true;
+ }
+ if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
+ _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ if (!_domainDao.remove(domain.getId())) {
+ rollBackState = true;
+ CloudRuntimeException e =
+ new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
+ "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ e.addProxyObject(domain.getUuid(), "domainId");
+ throw e;
+ }
+ _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ } else {
+ rollBackState = true;
+ String msg = null;
+ if (!accountsForCleanup.isEmpty()) {
+ msg = accountsForCleanup.size() + " accounts to cleanup";
+ } else if (!networkIds.isEmpty()) {
+ msg = networkIds.size() + " non-removed networks";
+ } else if (hasDedicatedResources) {
+ msg = "dedicated resources.";
+ }
+
+ CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
+ e.addProxyObject(domain.getUuid(), "domainId");
+ throw e;
}
-
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
}
- }
- cleanupDomainOfferings(domain.getId());
- CallContext.current().putContextParameter(Domain.class, domain.getUuid());
- return true;
- } catch (Exception ex) {
- s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
- if (ex instanceof CloudRuntimeException)
- throw (CloudRuntimeException)ex;
- else
- return false;
- } finally {
- //when success is false
- if (rollBackState) {
- s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
- " because it can't be removed due to resources referencing to it");
- domain.setState(Domain.State.Active);
- _domainDao.update(domain.getId(), domain);
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException)
+ throw (CloudRuntimeException)ex;
+ else
+ return false;
+ } finally {
+ lock.unlock();
--- End diff --
unlock() should be in the outer try-finally block otherwise there may be scenarios where the lock is never released.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
<b>Trillian test result (tid-973)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29259 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1935-t973-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Test completed. 48 look ok, 1 have error(s)
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_04_rvpc_privategw_static_routes | `Failure` | 335.65 | test_privategw_acl.py
test_01_vpc_site2site_vpn | Success | 150.05 | test_vpc_vpn.py
test_01_vpc_remote_access_vpn | Success | 61.16 | test_vpc_vpn.py
test_01_redundant_vpc_site2site_vpn | Success | 245.99 | test_vpc_vpn.py
test_02_VPC_default_routes | Success | 264.01 | test_vpc_router_nics.py
test_01_VPC_nics_after_destroy | Success | 553.63 | test_vpc_router_nics.py
test_05_rvpc_multi_tiers | Success | 512.64 | test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics | Success | 1410.93 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Success | 548.06 | test_vpc_redundant.py
test_02_redundant_VPC_default_routes | Success | 745.11 | test_vpc_redundant.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Success | 1306.39 | test_vpc_redundant.py
test_09_delete_detached_volume | Success | 156.51 | test_volumes.py
test_08_resize_volume | Success | 156.45 | test_volumes.py
test_07_resize_fail | Success | 161.44 | test_volumes.py
test_06_download_detached_volume | Success | 156.43 | test_volumes.py
test_05_detach_volume | Success | 150.80 | test_volumes.py
test_04_delete_attached_volume | Success | 151.22 | test_volumes.py
test_03_download_attached_volume | Success | 156.38 | test_volumes.py
test_02_attach_volume | Success | 89.17 | test_volumes.py
test_01_create_volume | Success | 622.16 | test_volumes.py
test_03_delete_vm_snapshots | Success | 275.25 | test_vm_snapshots.py
test_02_revert_vm_snapshots | Success | 100.71 | test_vm_snapshots.py
test_01_create_vm_snapshots | Success | 168.82 | test_vm_snapshots.py
test_deploy_vm_multiple | Success | 272.66 | test_vm_life_cycle.py
test_deploy_vm | Success | 0.03 | test_vm_life_cycle.py
test_advZoneVirtualRouter | Success | 0.02 | test_vm_life_cycle.py
test_10_attachAndDetach_iso | Success | 26.55 | test_vm_life_cycle.py
test_09_expunge_vm | Success | 125.24 | test_vm_life_cycle.py
test_08_migrate_vm | Success | 30.88 | test_vm_life_cycle.py
test_07_restore_vm | Success | 0.13 | test_vm_life_cycle.py
test_06_destroy_vm | Success | 125.81 | test_vm_life_cycle.py
test_03_reboot_vm | Success | 125.84 | test_vm_life_cycle.py
test_02_start_vm | Success | 10.17 | test_vm_life_cycle.py
test_01_stop_vm | Success | 35.29 | test_vm_life_cycle.py
test_CreateTemplateWithDuplicateName | Success | 161.14 | test_templates.py
test_08_list_system_templates | Success | 0.04 | test_templates.py
test_07_list_public_templates | Success | 0.04 | test_templates.py
test_05_template_permissions | Success | 0.05 | test_templates.py
test_04_extract_template | Success | 5.17 | test_templates.py
test_03_delete_template | Success | 5.11 | test_templates.py
test_02_edit_template | Success | 90.13 | test_templates.py
test_01_create_template | Success | 35.41 | test_templates.py
test_10_destroy_cpvm | Success | 161.64 | test_ssvm.py
test_09_destroy_ssvm | Success | 133.24 | test_ssvm.py
test_08_reboot_cpvm | Success | 131.61 | test_ssvm.py
test_07_reboot_ssvm | Success | 133.65 | test_ssvm.py
test_06_stop_cpvm | Success | 131.74 | test_ssvm.py
test_05_stop_ssvm | Success | 163.77 | test_ssvm.py
test_04_cpvm_internals | Success | 1.24 | test_ssvm.py
test_03_ssvm_internals | Success | 3.37 | test_ssvm.py
test_02_list_cpvm_vm | Success | 0.12 | test_ssvm.py
test_01_list_sec_storage_vm | Success | 0.13 | test_ssvm.py
test_02_list_snapshots_with_removed_data_store | Success | 86.81 | test_snapshots.py
test_01_snapshot_root_disk | Success | 16.33 | test_snapshots.py
test_04_change_offering_small | Success | 209.54 | test_service_offerings.py
test_03_delete_service_offering | Success | 0.04 | test_service_offerings.py
test_02_edit_service_offering | Success | 0.06 | test_service_offerings.py
test_01_create_service_offering | Success | 0.11 | test_service_offerings.py
test_02_sys_template_ready | Success | 0.14 | test_secondary_storage.py
test_01_sys_vm_start | Success | 0.18 | test_secondary_storage.py
test_09_reboot_router | Success | 35.35 | test_routers.py
test_08_start_router | Success | 30.32 | test_routers.py
test_07_stop_router | Success | 10.18 | test_routers.py
test_06_router_advanced | Success | 0.05 | test_routers.py
test_05_router_basic | Success | 0.04 | test_routers.py
test_04_restart_network_wo_cleanup | Success | 5.62 | test_routers.py
test_03_restart_network_cleanup | Success | 50.47 | test_routers.py
test_02_router_internal_adv | Success | 1.10 | test_routers.py
test_01_router_internal_basic | Success | 0.58 | test_routers.py
test_router_dns_guestipquery | Success | 73.75 | test_router_dns.py
test_router_dns_externalipquery | Success | 0.06 | test_router_dns.py
test_router_dhcphosts | Success | 271.94 | test_router_dhcphosts.py
test_router_dhcp_opts | Success | 21.49 | test_router_dhcphosts.py
test_01_updatevolumedetail | Success | 0.10 | test_resource_detail.py
test_01_reset_vm_on_reboot | Success | 130.93 | test_reset_vm_on_reboot.py
test_createRegion | Success | 0.06 | test_regions.py
test_create_pvlan_network | Success | 5.21 | test_pvlan.py
test_dedicatePublicIpRange | Success | 0.41 | test_public_ip_range.py
test_03_vpc_privategw_restart_vpc_cleanup | Success | 515.08 | test_privategw_acl.py
test_02_vpc_privategw_static_routes | Success | 390.25 | test_privategw_acl.py
test_01_vpc_privategw_acl | Success | 82.14 | test_privategw_acl.py
test_03_migration_options_storage_tags | Success | 66.30 | test_primary_storage.py
test_02_edit_primary_storage_tags | Success | 0.10 | test_primary_storage.py
test_01_primary_storage_nfs | Success | 35.76 | test_primary_storage.py
test_01_deploy_vms_storage_tags | Success | 31.02 | test_primary_storage.py
test_createPortablePublicIPRange | Success | 15.19 | test_portable_publicip.py
test_createPortablePublicIPAcquire | Success | 15.44 | test_portable_publicip.py
test_isolate_network_password_server | Success | 89.20 | test_password_server.py
test_UpdateStorageOverProvisioningFactor | Success | 0.14 | test_over_provisioning.py
test_oobm_zchange_password | Success | 35.74 | test_outofbandmanagement.py
test_oobm_multiple_mgmt_server_ownership | Success | 16.34 | test_outofbandmanagement.py
test_oobm_issue_power_status | Success | 10.23 | test_outofbandmanagement.py
test_oobm_issue_power_soft | Success | 15.47 | test_outofbandmanagement.py
test_oobm_issue_power_reset | Success | 15.39 | test_outofbandmanagement.py
test_oobm_issue_power_on | Success | 15.34 | test_outofbandmanagement.py
test_oobm_issue_power_off | Success | 15.32 | test_outofbandmanagement.py
test_oobm_issue_power_cycle | Success | 15.35 | test_outofbandmanagement.py
test_oobm_enabledisable_across_clusterzones | Success | 92.56 | test_outofbandmanagement.py
test_oobm_enable_feature_valid | Success | 5.16 | test_outofbandmanagement.py
test_oobm_enable_feature_invalid | Success | 0.10 | test_outofbandmanagement.py
test_oobm_disable_feature_valid | Success | 0.17 | test_outofbandmanagement.py
test_oobm_disable_feature_invalid | Success | 0.10 | test_outofbandmanagement.py
test_oobm_configure_invalid_driver | Success | 0.07 | test_outofbandmanagement.py
test_oobm_configure_default_driver | Success | 0.08 | test_outofbandmanagement.py
test_oobm_background_powerstate_sync | Success | 23.41 | test_outofbandmanagement.py
test_extendPhysicalNetworkVlan | Success | 15.38 | test_non_contigiousvlan.py
test_01_nic | Success | 409.33 | test_nic.py
test_releaseIP | Success | 237.74 | test_network.py
test_reboot_router | Success | 403.56 | test_network.py
test_public_ip_user_account | Success | 10.25 | test_network.py
test_public_ip_admin_account | Success | 40.30 | test_network.py
test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Success | 67.03 | test_network.py
test_network_rules_acquired_public_ip_2_nat_rule | Success | 61.82 | test_network.py
test_network_rules_acquired_public_ip_1_static_nat_rule | Success | 123.95 | test_network.py
test_delete_account | Success | 328.17 | test_network.py
test_02_port_fwd_on_non_src_nat | Success | 55.66 | test_network.py
test_01_port_fwd_on_src_nat | Success | 111.74 | test_network.py
test_nic_secondaryip_add_remove | Success | 207.71 | test_multipleips_per_nic.py
login_test_saml_user | Success | 19.13 | test_login.py
test_assign_and_removal_lb | Success | 133.47 | test_loadbalance.py
test_02_create_lb_rule_non_nat | Success | 187.04 | test_loadbalance.py
test_01_create_lb_rule_src_nat | Success | 217.70 | test_loadbalance.py
test_03_list_snapshots | Success | 0.09 | test_list_ids_parameter.py
test_02_list_templates | Success | 0.04 | test_list_ids_parameter.py
test_01_list_volumes | Success | 0.03 | test_list_ids_parameter.py
test_07_list_default_iso | Success | 0.06 | test_iso.py
test_05_iso_permissions | Success | 0.06 | test_iso.py
test_04_extract_Iso | Success | 5.17 | test_iso.py
test_03_delete_iso | Success | 95.24 | test_iso.py
test_02_edit_iso | Success | 0.06 | test_iso.py
test_01_create_iso | Success | 20.99 | test_iso.py
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces | Success | 198.19 | test_internal_lb.py
test_03_vpc_internallb_haproxy_stats_on_all_interfaces | Success | 148.41 | test_internal_lb.py
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80 | Success | 505.22 | test_internal_lb.py
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Success | 435.47 | test_internal_lb.py
test_dedicateGuestVlanRange | Success | 10.43 | test_guest_vlan_range.py
test_UpdateConfigParamWithScope | Success | 0.14 | test_global_settings.py
test_rolepermission_lifecycle_update | Success | 6.16 | test_dynamicroles.py
test_rolepermission_lifecycle_list | Success | 5.97 | test_dynamicroles.py
test_rolepermission_lifecycle_delete | Success | 5.85 | test_dynamicroles.py
test_rolepermission_lifecycle_create | Success | 5.89 | test_dynamicroles.py
test_rolepermission_lifecycle_concurrent_updates | Success | 6.02 | test_dynamicroles.py
test_role_lifecycle_update_role_inuse | Success | 5.91 | test_dynamicroles.py
test_role_lifecycle_update | Success | 10.97 | test_dynamicroles.py
test_role_lifecycle_list | Success | 5.88 | test_dynamicroles.py
test_role_lifecycle_delete | Success | 11.00 | test_dynamicroles.py
test_role_lifecycle_create | Success | 5.89 | test_dynamicroles.py
test_role_inuse_deletion | Success | 5.85 | test_dynamicroles.py
test_role_account_acls_multiple_mgmt_servers | Success | 7.97 | test_dynamicroles.py
test_role_account_acls | Success | 8.32 | test_dynamicroles.py
test_default_role_deletion | Success | 6.02 | test_dynamicroles.py
test_04_create_fat_type_disk_offering | Success | 0.07 | test_disk_offerings.py
test_03_delete_disk_offering | Success | 0.04 | test_disk_offerings.py
test_02_edit_disk_offering | Success | 0.06 | test_disk_offerings.py
test_02_create_sparse_type_disk_offering | Success | 0.07 | test_disk_offerings.py
test_01_create_disk_offering | Success | 0.32 | test_disk_offerings.py
test_deployvm_userdispersing | Success | 20.59 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userconcentrated | Success | 20.58 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_firstfit | Success | 60.79 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userdata_post | Success | 10.52 | test_deploy_vm_with_userdata.py
test_deployvm_userdata | Success | 55.84 | test_deploy_vm_with_userdata.py
test_02_deploy_vm_root_resize | Success | 5.99 | test_deploy_vm_root_resize.py
test_01_deploy_vm_root_resize | Success | 5.97 | test_deploy_vm_root_resize.py
test_00_deploy_vm_root_resize | Success | 262.74 | test_deploy_vm_root_resize.py
test_deploy_vm_from_iso | Success | 212.41 | test_deploy_vm_iso.py
test_DeployVmAntiAffinityGroup | Success | 66.34 | test_affinity_groups.py
test_change_service_offering_for_vm_with_snapshots | Skipped | 0.00 | test_vm_snapshots.py
test_01_test_vm_volume_snapshot | Skipped | 0.00 | test_vm_snapshots.py
test_06_copy_template | Skipped | 0.00 | test_templates.py
test_static_role_account_acls | Skipped | 0.02 | test_staticroles.py
test_11_ss_nfs_version_on_ssvm | Skipped | 0.02 | test_ssvm.py
test_01_scale_vm | Skipped | 0.00 | test_scale_vm.py
test_01_primary_storage_iscsi | Skipped | 0.04 | test_primary_storage.py
test_nested_virtualization_vmware | Skipped | 0.00 | test_nested_virtualization.py
test_06_copy_iso | Skipped | 0.00 | test_iso.py
test_deploy_vgpu_enabled_vm | Skipped | 0.03 | test_deploy_vgpu_enabled_vm.py
test_3d_gpu_support | Skipped | 0.03 | test_deploy_vgpu_enabled_vm.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@nvazquez can you check if the failures in above test results are related to your changes, thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@rafaelweingartner I think I got your point, I tried to keep code as similar as it was before, by declaring `rollBackState` as static class variable on line 114. This way inner `finally` block would work the same as before when one of new methods set `rollBackState = true.` On outter `finally` block, `rollBackState` is set to false (line 345), this way each time `deleteDomain` is invoked it would start on false (maybe it would be easier to move it at the beggining of `deleteDomain`). Do you agree with this approach?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102534985
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,67 @@ public void testFindDomainByIdOrPathValidId() {
Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, "/validDomain/"));
}
+ @Test(expected=InvalidParameterValueException.class)
+ public void testDeleteDomainNullDomain() {
+ Mockito.when(_domainDao.findById(DOMAIN_ID)).thenReturn(null);
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ }
+
+ @Test(expected=PermissionDeniedException.class)
+ public void testDeleteDomainRootDomain() {
+ Mockito.when(_domainDao.findById(Domain.ROOT_DOMAIN)).thenReturn(domain);
+ domainManager.deleteDomain(Domain.ROOT_DOMAIN, testDomainCleanup);
+ }
+
+ //TODO testDeleteDomainCleanup
+
+ @Test
+ public void testDeleteDomainNoCleanup() {
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ Mockito.verify(domainManager).deleteDomain(domain, testDomainCleanup);
+ Mockito.verify(domainManager).checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+ Mockito.verify(lock).unlock();
+ Assert.assertEquals(false, domainManager.getRollBackState());
+ }
+
+ @Test
+ public void testCheckDomainAccountsNetworksAndResourcesBeforeRemovingRemoveDomain() {
+ domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testCheckDomainAccountsNetworksAndResourcesBeforeRemovingDontRemoveDomain() {
+ domainNetworkIds.add(2l);
+ domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, false);
+ }
+
+ @Test
+ public void testPublishRemoveEventsAndRemoveDomainSuccessfulDelete() {
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testPublishRemoveEventsAndRemoveDomainExceptionDelete() {
+ Mockito.when(_domainDao.remove(DOMAIN_ID)).thenReturn(false);
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus, Mockito.never()).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testFailRemoveOperation() {
--- End diff --
Here we still have a problem.
the exception is thrown at line 227 and the code at line 228 is never executed.
That is why I called nasty to write a test here. It is very tricky
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by borisstoyanov <gi...@git.apache.org>.
Github user borisstoyanov commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@blueorangutan test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@rafaelweingartner thanks for reviewing! I extracted code to new methods and also added unit tests for them
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by borisstoyanov <gi...@git.apache.org>.
Github user borisstoyanov commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@blueorangutan package
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by borisstoyanov <gi...@git.apache.org>.
Github user borisstoyanov commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@blueorangutan test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-520
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102535697
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +289,133 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
--- End diff --
Ah, the naming of methods, I also find very hard sometimes; I think most programmers have this problem. I think the name you proposed is ok. I am not creative today.
I also liked the way the code became cleaner and easier to read. It also facilitates future changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r103529571
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +284,145 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ boolean rollBackState = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ tryCleanupDomain(domain, ownerId);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
- }
+ removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+ }
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException) {
+ rollBackState = true;
+ throw (CloudRuntimeException)ex;
+ }
+ else
+ return false;
+ } finally {
+ //when success is false
+ if (rollBackState) {
+ s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
--- End diff --
@nvazquez great job.
what about changing thing a little bit?
Extracting lines 328-331 to a method, and then calling this method at line 320. This way you do not need the `rollBackState` control variable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@serg38 I did, the code is great as always. However, I have a concern about that `rollBackState` variable being static there. Because the `DomainManagerImpl ` is a singleton, using that variable may create problems. Instead of using the variable to indicate when the rollback has to be executed, we could do the same using an exception (the exception is already thrown when the variable is set to `true`).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by borisstoyanov <gi...@git.apache.org>.
Github user borisstoyanov commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@blueorangutan test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r100575967
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+ boolean rollBackState = false;
+ boolean hasDedicatedResources = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if ((cleanup != null) && cleanup.booleanValue()) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
+ //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
+ List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
+ List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
+ List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
+ if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
+ s_logger.error("There are dedicated resources for the domain " + domain.getId());
+ hasDedicatedResources = true;
+ }
+ if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
+ _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ if (!_domainDao.remove(domain.getId())) {
+ rollBackState = true;
+ CloudRuntimeException e =
+ new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
+ "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ e.addProxyObject(domain.getUuid(), "domainId");
+ throw e;
+ }
+ _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ } else {
+ rollBackState = true;
--- End diff --
What about another method extraction here?
Lines 328-340
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102536773
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +289,133 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
- }
+ checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ }
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException)
+ throw (CloudRuntimeException)ex;
+ else
+ return false;
+ } finally {
+ //when success is false
+ if (rollBackState) {
+ s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
+ " because it can't be removed due to resources referencing to it");
+ domain.setState(Domain.State.Active);
+ _domainDao.update(domain.getId(), domain);
}
}
+ }
+ finally {
+ lock.unlock();
+ rollBackState = false;
+ }
+ }
- cleanupDomainOfferings(domain.getId());
- CallContext.current().putContextParameter(Domain.class, domain.getUuid());
- return true;
- } catch (Exception ex) {
- s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
- if (ex instanceof CloudRuntimeException)
- throw (CloudRuntimeException)ex;
- else
- return false;
- } finally {
- //when success is false
- if (rollBackState) {
- s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
- " because it can't be removed due to resources referencing to it");
- domain.setState(Domain.State.Active);
- _domainDao.update(domain.getId(), domain);
- }
+ /**
+ * Check domain resources before removing domain. There are 2 cases:
+ * <ol>
+ * <li>Domain doesn't have accounts for cleanup, non-removed networks, or dedicated resources</li>
+ * <ul><li>Delete domain</li></ul>
+ * <li>Domain has one of the following: accounts set for cleanup, non-removed networks, dedicated resources</li>
+ * <ul><li>Dont' delete domain</li><li>Fail operation</li></ul>
+ * </ol>
+ * @param domain domain to remove
+ * @throws CloudRuntimeException when case 2 or when domain cannot be deleted on case 1
+ */
+ protected void checkDomainAccountsNetworksAndResourcesBeforeRemoving(DomainVO domain) {
--- End diff --
The name is great now (very descriptive). Sure it is long, but the size of the method name will change nothing for the JVM (will have not negative effects on the system). And when considering the benefits, I really do not mind long method names. However, we should be open to hear others feedback and opinions on this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102486318
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,67 @@ public void testFindDomainByIdOrPathValidId() {
Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, "/validDomain/"));
}
+ @Test(expected=InvalidParameterValueException.class)
+ public void testDeleteDomainNullDomain() {
+ Mockito.when(_domainDao.findById(DOMAIN_ID)).thenReturn(null);
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ }
+
+ @Test(expected=PermissionDeniedException.class)
+ public void testDeleteDomainRootDomain() {
+ Mockito.when(_domainDao.findById(Domain.ROOT_DOMAIN)).thenReturn(domain);
+ domainManager.deleteDomain(Domain.ROOT_DOMAIN, testDomainCleanup);
+ }
+
+ //TODO testDeleteDomainCleanup
+
+ @Test
+ public void testDeleteDomainNoCleanup() {
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ Mockito.verify(domainManager).deleteDomain(domain, testDomainCleanup);
+ Mockito.verify(domainManager).checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+ Mockito.verify(lock).unlock();
+ Assert.assertEquals(false, domainManager.getRollBackState());
+ }
+
+ @Test
+ public void testCheckDomainAccountsNetworksAndResourcesBeforeRemovingRemoveDomain() {
+ domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testCheckDomainAccountsNetworksAndResourcesBeforeRemovingDontRemoveDomain() {
+ domainNetworkIds.add(2l);
+ domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, false);
+ }
+
+ @Test
+ public void testPublishRemoveEventsAndRemoveDomainSuccessfulDelete() {
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testPublishRemoveEventsAndRemoveDomainExceptionDelete() {
+ Mockito.when(_domainDao.remove(DOMAIN_ID)).thenReturn(false);
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus, Mockito.never()).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testFailRemoveOperation() {
--- End diff --
Here is a nasty method to test.
when an exception happens, the variable `rollBackState ` is changed to `true`. should not we verify that?
What do you think about this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r110421376
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +284,145 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ boolean rollBackState = false;
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ tryCleanupDomain(domain, ownerId);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
- }
+ removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
+ }
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException) {
+ rollBackState = true;
+ throw (CloudRuntimeException)ex;
+ }
+ else
+ return false;
+ } finally {
+ //when success is false
+ if (rollBackState) {
+ s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
--- End diff --
Nice, I like how method is after this last refactor. It is much clearer without `rollBackState` variable, thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102531132
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +289,133 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
- }
+ checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ }
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException)
+ throw (CloudRuntimeException)ex;
+ else
+ return false;
+ } finally {
+ //when success is false
+ if (rollBackState) {
+ s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
+ " because it can't be removed due to resources referencing to it");
+ domain.setState(Domain.State.Active);
+ _domainDao.update(domain.getId(), domain);
}
}
+ }
+ finally {
+ lock.unlock();
+ rollBackState = false;
+ }
+ }
- cleanupDomainOfferings(domain.getId());
- CallContext.current().putContextParameter(Domain.class, domain.getUuid());
- return true;
- } catch (Exception ex) {
- s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
- if (ex instanceof CloudRuntimeException)
- throw (CloudRuntimeException)ex;
- else
- return false;
- } finally {
- //when success is false
- if (rollBackState) {
- s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
- " because it can't be removed due to resources referencing to it");
- domain.setState(Domain.State.Active);
- _domainDao.update(domain.getId(), domain);
- }
+ /**
+ * Check domain resources before removing domain. There are 2 cases:
+ * <ol>
+ * <li>Domain doesn't have accounts for cleanup, non-removed networks, or dedicated resources</li>
+ * <ul><li>Delete domain</li></ul>
+ * <li>Domain has one of the following: accounts set for cleanup, non-removed networks, dedicated resources</li>
+ * <ul><li>Dont' delete domain</li><li>Fail operation</li></ul>
+ * </ol>
+ * @param domain domain to remove
+ * @throws CloudRuntimeException when case 2 or when domain cannot be deleted on case 1
+ */
+ protected void checkDomainAccountsNetworksAndResourcesBeforeRemoving(DomainVO domain) {
+ boolean hasDedicatedResources = false;
+ List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
+ List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
+ List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
+ if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
--- End diff --
Done, thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102323551
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,82 +289,133 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
@Override
public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
- // mark domain as inactive
- s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
- domain.setState(Domain.State.Inactive);
- _domainDao.update(domain.getId(), domain);
- boolean rollBackState = false;
- boolean hasDedicatedResources = false;
+ GlobalLock lock = getGlobalLock("AccountCleanup");
+ if (lock == null) {
+ s_logger.debug("Couldn't get the global lock");
+ return false;
+ }
+
+ if (!lock.lock(30)) {
+ s_logger.debug("Couldn't lock the db");
+ return false;
+ }
try {
- long ownerId = domain.getAccountId();
- if ((cleanup != null) && cleanup.booleanValue()) {
- if (!cleanupDomain(domain.getId(), ownerId)) {
- rollBackState = true;
- CloudRuntimeException e =
- new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
- domain.getId() + ").");
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
- }
- } else {
- //don't delete the domain if there are accounts set for cleanup, or non-removed networks exist, or domain has dedicated resources
- List<Long> networkIds = _networkDomainDao.listNetworkIdsByDomain(domain.getId());
- List<AccountVO> accountsForCleanup = _accountDao.findCleanupsForRemovedAccounts(domain.getId());
- List<DedicatedResourceVO> dedicatedResources = _dedicatedDao.listByDomainId(domain.getId());
- if (dedicatedResources != null && !dedicatedResources.isEmpty()) {
- s_logger.error("There are dedicated resources for the domain " + domain.getId());
- hasDedicatedResources = true;
- }
- if (accountsForCleanup.isEmpty() && networkIds.isEmpty() && !hasDedicatedResources) {
- _messageBus.publish(_name, MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
- if (!_domainDao.remove(domain.getId())) {
+ // mark domain as inactive
+ s_logger.debug("Marking domain id=" + domain.getId() + " as " + Domain.State.Inactive + " before actually deleting it");
+ domain.setState(Domain.State.Inactive);
+ _domainDao.update(domain.getId(), domain);
+
+ try {
+ long ownerId = domain.getAccountId();
+ if (BooleanUtils.toBoolean(cleanup)) {
+ if (!cleanupDomain(domain.getId(), ownerId)) {
rollBackState = true;
CloudRuntimeException e =
- new CloudRuntimeException("Delete failed on domain " + domain.getName() + " (id: " + domain.getId() +
- "); Please make sure all users and sub domains have been removed from the domain before deleting");
+ new CloudRuntimeException("Failed to clean up domain resources and sub domains, delete failed on domain " + domain.getName() + " (id: " +
+ domain.getId() + ").");
e.addProxyObject(domain.getUuid(), "domainId");
throw e;
}
- _messageBus.publish(_name, MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
} else {
- rollBackState = true;
- String msg = null;
- if (!accountsForCleanup.isEmpty()) {
- msg = accountsForCleanup.size() + " accounts to cleanup";
- } else if (!networkIds.isEmpty()) {
- msg = networkIds.size() + " non-removed networks";
- } else if (hasDedicatedResources) {
- msg = "dedicated resources.";
- }
+ checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ }
- CloudRuntimeException e = new CloudRuntimeException("Can't delete the domain yet because it has " + msg);
- e.addProxyObject(domain.getUuid(), "domainId");
- throw e;
+ cleanupDomainOfferings(domain.getId());
+ CallContext.current().putContextParameter(Domain.class, domain.getUuid());
+ return true;
+ } catch (Exception ex) {
+ s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
+ if (ex instanceof CloudRuntimeException)
+ throw (CloudRuntimeException)ex;
+ else
+ return false;
+ } finally {
+ //when success is false
+ if (rollBackState) {
+ s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
+ " because it can't be removed due to resources referencing to it");
+ domain.setState(Domain.State.Active);
+ _domainDao.update(domain.getId(), domain);
}
}
+ }
+ finally {
+ lock.unlock();
+ rollBackState = false;
+ }
+ }
- cleanupDomainOfferings(domain.getId());
- CallContext.current().putContextParameter(Domain.class, domain.getUuid());
- return true;
- } catch (Exception ex) {
- s_logger.error("Exception deleting domain with id " + domain.getId(), ex);
- if (ex instanceof CloudRuntimeException)
- throw (CloudRuntimeException)ex;
- else
- return false;
- } finally {
- //when success is false
- if (rollBackState) {
- s_logger.debug("Changing domain id=" + domain.getId() + " state back to " + Domain.State.Active +
- " because it can't be removed due to resources referencing to it");
- domain.setState(Domain.State.Active);
- _domainDao.update(domain.getId(), domain);
- }
+ /**
+ * Check domain resources before removing domain. There are 2 cases:
+ * <ol>
+ * <li>Domain doesn't have accounts for cleanup, non-removed networks, or dedicated resources</li>
+ * <ul><li>Delete domain</li></ul>
+ * <li>Domain has one of the following: accounts set for cleanup, non-removed networks, dedicated resources</li>
+ * <ul><li>Dont' delete domain</li><li>Fail operation</li></ul>
+ * </ol>
+ * @param domain domain to remove
+ * @throws CloudRuntimeException when case 2 or when domain cannot be deleted on case 1
+ */
+ protected void checkDomainAccountsNetworksAndResourcesBeforeRemoving(DomainVO domain) {
--- End diff --
It is not just a "check/validate" method, right?
If the conditions described in the Java doc are met, the domain will be removed. Reading the method name I get the feeling that it just executes the checks before removing. However, at line 370, it seems that the domain removal happens there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by borisstoyanov <gi...@git.apache.org>.
Github user borisstoyanov commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@blueorangutan package
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-537
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@rhtyd @karuturi To fix errors in B.O in test_02_list_snapshots_with_removed_data_store we need to merge PR1961 and then adjust test_data on B.O side to have correct mapping for nfs2 label
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1935: CLOUDSTACK-9764: Delete domain failure due to Accoun...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1935
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1935#discussion_r102556528
--- Diff: server/test/com/cloud/user/DomainManagerImplTest.java ---
@@ -134,4 +164,67 @@ public void testFindDomainByIdOrPathValidId() {
Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, "/validDomain/"));
}
+ @Test(expected=InvalidParameterValueException.class)
+ public void testDeleteDomainNullDomain() {
+ Mockito.when(_domainDao.findById(DOMAIN_ID)).thenReturn(null);
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ }
+
+ @Test(expected=PermissionDeniedException.class)
+ public void testDeleteDomainRootDomain() {
+ Mockito.when(_domainDao.findById(Domain.ROOT_DOMAIN)).thenReturn(domain);
+ domainManager.deleteDomain(Domain.ROOT_DOMAIN, testDomainCleanup);
+ }
+
+ //TODO testDeleteDomainCleanup
+
+ @Test
+ public void testDeleteDomainNoCleanup() {
+ domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
+ Mockito.verify(domainManager).deleteDomain(domain, testDomainCleanup);
+ Mockito.verify(domainManager).checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).cleanupDomainOfferings(DOMAIN_ID);
+ Mockito.verify(lock).unlock();
+ Assert.assertEquals(false, domainManager.getRollBackState());
+ }
+
+ @Test
+ public void testCheckDomainAccountsNetworksAndResourcesBeforeRemovingRemoveDomain() {
+ domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).publishRemoveEventsAndRemoveDomain(domain);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testCheckDomainAccountsNetworksAndResourcesBeforeRemovingDontRemoveDomain() {
+ domainNetworkIds.add(2l);
+ domainManager.checkDomainAccountsNetworksAndResourcesBeforeRemoving(domain);
+ Mockito.verify(domainManager).failRemoveOperation(domain, domainAccountsForCleanup, domainNetworkIds, false);
+ }
+
+ @Test
+ public void testPublishRemoveEventsAndRemoveDomainSuccessfulDelete() {
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testPublishRemoveEventsAndRemoveDomainExceptionDelete() {
+ Mockito.when(_domainDao.remove(DOMAIN_ID)).thenReturn(false);
+ domainManager.publishRemoveEventsAndRemoveDomain(domain);
+ Mockito.verify(_messageBus).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_messageBus, Mockito.never()).publish(Mockito.anyString(), Matchers.eq(DomainManager.MESSAGE_REMOVE_DOMAIN_EVENT),
+ Matchers.eq(PublishScope.LOCAL), Matchers.eq(domain));
+ Mockito.verify(_domainDao).remove(DOMAIN_ID);
+ }
+
+ @Test(expected=CloudRuntimeException.class)
+ public void testFailRemoveOperation() {
--- End diff --
Thanks! I removed `expected` from `@Test` annotation and added a catch block to assert exception class. It is a nasty method to test indeed :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---