[JoaoJandre]

Description

Sometimes users have the need to move resources between domains, for example, in a big company, a department may be moved from one part of the company to another, changing the company's department hierarchy, the easiest way of reflecting this change on the company's cloud environment would be to move subdomains between domains, but currently ACS offers no option to do that.
This PR adds the moveDomain API, which will move domains between subdomains. Furthermore, if the domain that is being moved has any subdomains, those will also be moved, maintaining the current subdomain tree.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Description of tests

Test	Result
Move an Inactive Domain	Error, as an inactive domain should not be able to be moved
Move a Domain to a new inactive parent domain	Error, as a domain that is inactive should not receive any new children
Move a domain to a new parent domain that has a child with the same name as the domain to be moved	Error, as two subdomains of the same domain should not have the same name
Move Domain ROOT	Error, root domain by definition must always be the father of all other domains
Move a domain that is already on the destination	Error, as it is already is were it should be
Move a domain to itself (moved domain = new parent domain)	Error, as this makes no sense
Move a domain that uses a shared network that is inaccessible to the new parent domain	Error, as the new parent domain must have access to all of its children's resources
Move a domain that has a child subdomain that uses a shared network that is inaccessible to the new parent domain	Error, as the new parent domain must have access to all of its children's resources
Move a domain that uses a affinity group that is inaccessible to the new parent domain	Error, as the new parent domain must have access to all of its children's resources
Move a domain that has a child subdomain that uses a affinity group that is inaccessible to the new parent domain	Error, as the new parent domain must have access to all of its children's resources
Move a domain that owns a shared network	Success
Move a domain that has a subdomain that owns a shared network	Success
Move a domain that uses a shared network that is accessible to the new parent domain	Success
Move a domain that has a subdomain that uses a shared network that is accessible to the new parent domain	Success
Move a domain that owns an affinity group	Success
Move a domain that has a subdomain that owns an affinity group	Success
Move a domain that uses a affinity group that is accessible to the new parent domain	Success
Move a domain that has a subdomain that uses a affinity group that is accessible to the new parent domain	Success
Move a domain that exceeds the resource limits of the new parent domain	Success

All tests performed as expected. In the case of shared networks, all had subdomain access.

For successful cases, it was checked whether the following attributes of the moved domain and its children were correctly changed:

• Path
• Level
• Use of resources
• Limits
• Parent domain

In addition, it was verified that the number of children of the current parent domain and the target domain were correctly accounted for.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

2 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

0.4% Coverage
0.0% Duplication

[yadvr]

Thanks for the PR @JoaoJandre interesting use-case. Do you think there should also be some kind of domain-limits check (for ex. moving sub-domain1 to a new parent could mean different limits on vms, volume, cpu, templates, storage etc. need to be recalculated/checked)? Also side-effects in (already produced) usage data and networks?

[codecov]

Codecov Report

Attention: 223 lines in your changes are missing coverage. Please review.

Comparison is base (32448e1) 14.28% compared to head (c989ba0) 22.20%.
Report is 334 commits behind head on main.

Files	Patch %	Lines
...rc/main/java/com/cloud/user/DomainManagerImpl.java	0.00%	115 Missing ⚠️
...va/com/cloud/network/dao/NetworkDomainDaoImpl.java	5.00%	19 Missing ⚠️
...ck/affinity/dao/AffinityGroupDomainMapDaoImpl.java	5.00%	19 Missing ⚠️
...loud/api/query/dao/NetworkOfferingJoinDaoImpl.java	0.00%	19 Missing ⚠️
...loud/api/query/dao/ServiceOfferingJoinDaoImpl.java	0.00%	19 Missing ⚠️
...ava/com/cloud/dc/dao/DedicatedResourceDaoImpl.java	5.00%	19 Missing ⚠️
...dstack/api/command/admin/domain/MoveDomainCmd.java	0.00%	13 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7446      +/-   ##
============================================
+ Coverage     14.28%   22.20%   +7.92%     
- Complexity     9994    22258   +12264     
============================================
  Files          2747     5129    +2382     
  Lines        258905   347916   +89011     
  Branches      40318    49920    +9602     
============================================
+ Hits          36987    77264   +40277     
- Misses       217125   259515   +42390     
- Partials       4793    11137    +6344     

Flag	Coverage Δ
simulator-marvin-tests	23.78% <2.19%> (?)
uitests	4.42% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JoaoJandre]

Thanks for the PR @JoaoJandre interesting use-case. Do you think there should also be some kind of domain-limits check (for ex. moving sub-domain1 to a new parent could mean different limits on vms, volume, cpu, templates, storage etc. need to be recalculated/checked)? Also side-effects in (already produced) usage data and networks?

Hey @rohityadavcloud, thanks for the suggestion, I actually agree with you, there should be some kind of domain-limits check. I will put this in draft until I have the time to add this check.

About the side-effects in usage data, there shouldn't be any, as the usage data only uses the domain ID, and that is not changed when the domain is moved, only the domain path is changed. About networks, we already validate shared networks before moving the domain, what other side-effects could be an issue with networks?

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[JoaoJandre]

@blueorangutan package

[blueorangutan]

@JoaoJandre a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6724

[DaanHoogland]

@rohityadavcloud @JoaoJandre was this extra consideration of quota limits addressed here?

Thanks for the PR @JoaoJandre interesting use-case. Do you think there should also be some kind of domain-limits check (for ex. moving sub-domain1 to a new parent could mean different limits on vms, volume, cpu, templates, storage etc. need to be recalculated/checked)? Also side-effects in (already produced) usage data and networks?

Hey @rohityadavcloud, thanks for the suggestion, I actually agree with you, there should be some kind of domain-limits check. I will put this in draft until I have the time to add this check.

About the side-effects in usage data, there shouldn't be any, as the usage data only uses the domain ID, and that is not changed when the domain is moved, only the domain path is changed. About networks, we already validate shared networks before moving the domain, what other side-effects could be an issue with networks?

[JoaoJandre]

@rohityadavcloud @JoaoJandre was this extra consideration of quota limits addressed here?

Thanks for the PR @JoaoJandre interesting use-case. Do you think there should also be some kind of domain-limits check (for ex. moving sub-domain1 to a new parent could mean different limits on vms, volume, cpu, templates, storage etc. need to be recalculated/checked)? Also side-effects in (already produced) usage data and networks?

Hey @rohityadavcloud, thanks for the suggestion, I actually agree with you, there should be some kind of domain-limits check. I will put this in draft until I have the time to add this check.
About the side-effects in usage data, there shouldn't be any, as the usage data only uses the domain ID, and that is not changed when the domain is moved, only the domain path is changed. About networks, we already validate shared networks before moving the domain, what other side-effects could be an issue with networks?

@DaanHoogland yes I addressed it on commit 88d7765, I just forgot to make a comment about it.

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[DaanHoogland]

code looks good

[blueorangutan]

[SF] Trillian test result (tid-7377)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42735 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7446-t7377-kvm-centos7.zip
Smoke tests completed. 112 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_migrate_VM_and_root_volume	Error	83.78	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	Error	54.47	test_vm_life_cycle.py

[blueorangutan]

[SF] Trillian test result (tid-8416)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41024 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7446-t8416-kvm-centos7.zip
Smoke tests completed. 118 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[shwstppr]

@JoaoJandre @DaanHoogland any consensus here?
Are all issues/comments raised by @rajujith already addressed? #7446 (review)

[DaanHoogland]

I recently encoutered a similar issue and I am actually -1 on this without my sugestion because of that. I think you even agreed with me on that ticket, @JoaoJandre. we need to allow users to minimise logging by using a lesser level.
cc @shwstppr

ad. #8278

[shwstppr]

@blueorangutan package

[blueorangutan]

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7921

[shwstppr]

@blueorangutan test

[blueorangutan]

@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[JoaoJandre]

@JoaoJandre @DaanHoogland any consensus here? Are all issues/comments raised by @rajujith already addressed? #7446 (review)

@shwstppr yes, all the comments raised by @rajujith were addressed.

I recently encoutered a similar issue and I am actually -1 on this without my sugestion because of that. I think you even agreed with me on that ticket, @JoaoJandre. we need to allow users to minimise logging by using a lesser level. cc @shwstppr

ad. #8278

@DaanHoogland I thought a little more about it and in this instance I don't see a problem with this approach. But I don't think that this should always be the case (only have stacktraces on debug or lower). I'll make a post on the discussion thread later.

[DaanHoogland]

@harikrishna-patnala @rajujith can you approve on the current state of this PR?

[blueorangutan]

[SF] Trillian test result (tid-8466)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44180 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7446-t8466-kvm-centos7.zip
Smoke tests completed. 121 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[shwstppr]

ping @harikrishna-patnala @rajujith

[rajujith]

LGTM. All reported issues were addressed.
I wish the error returned could be improved to inform the user of the actual reason beyond the current generic error. Such as offerings are not accessible to the new parent domain etc.

Currently, it is :

Error: (HTTP 431, error code 4350) The domain [Domain:4/org4/BU1/] could not be moved to domain [Domain:3/Org2/]

[shwstppr]

Thanks for the testing @rajujith
@JoaoJandre is improving the error message something that can be done in a separate PR?

[JoaoJandre]

@shwstppr @rajujith I've added more context to the error message, could you please review the last commit?

[shwstppr]

@blueorangutan package

[blueorangutan]

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7995

[shwstppr]

@blueorangutan test

[blueorangutan]

@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-8529)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46978 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7446-t8529-kvm-centos7.zip
Smoke tests completed. 120 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_08_upgrade_kubernetes_ha_cluster	Failure	793.35	test_kubernetes_clusters.py