Rollback of changes with errors during the VM assign

[stephankruggg]

Description

When assigning a VM to another account (assignVirtualMachine API), if a network error happens during the process, no rollback is executed; thus, leaving the DB in an inconsistent state.

This PR fixes this by processing all steps to change the ownership of a VM inside the transaction. It also refactors code related to this procedure.

Types of changes

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

Feature/Enhancement Scale or Bug Severity

Bug Severity

• [ ] BLOCKER
• [ ] Critical
• [ ] Major
• [X] Minor
• [ ] Trivial

How Has This Been Tested?

In a local lab, I created a VM and an account of user type. I then attempted to assign the admin's VM to the user by using a network not accessible to the user.

Before applying the changes, an error was raised, but the VM was listed as belonging to the user.

After applying the changes, an error was raised, and the VM was listed as belonging to the admin - no changes occurred.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 4.26%. Comparing base (21af134) to head (5846f71). Report is 44 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #7061       +/-   ##
============================================
- Coverage     15.28%   4.26%   -11.02%     
============================================
  Files          5425     363     -5062     
  Lines        474138   29565   -444573     
  Branches      58984    5190    -53794     
============================================
- Hits          72486    1262    -71224     
+ Misses       393584   28160   -365424     
+ Partials       8068     143     -7925     

Flag	Coverage Δ
uitests	4.26% <ø> (ø)
unittests	?

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[stephankruggg]

lots of refactoring. I am not sure if the code is good or not. Maybe others can check.

@stephankruggg can you point out the key point of your change ?

@weizhouapache, when assigning a VM to another account (assignVirtualMachine API), if a network error happens during the process, no rollback is executed; thus, leaving the DB in an inconsistent state. This PR fixes this by processing all steps to change the ownership of a VM inside the transaction.

The refactoring is intended to make the code more testable, readable, and easier to maintain.

[weizhouapache]

lots of refactoring. I am not sure if the code is good or not. Maybe others can check. @stephankruggg can you point out the key point of your change ?

@weizhouapache, when assigning a VM to another account (assignVirtualMachine API), if a network error happens during the process, no rollback is executed; thus, leaving the DB in an inconsistent state. This PR fixes this by processing all steps to change the ownership of a VM inside the transaction.

The refactoring is intended to make the code more testable, readable, and easier to maintain.

@stephankruggg thanks.

it seems the key block is

        Transaction.execute(new TransactionCallbackNoReturn() {
            @Override
            public void doInTransactionWithoutResult(TransactionStatus status) {
                executeStepsToChangeOwnershipOfVm(cmd, caller, oldAccount, newAccount, vm, offering, volumes, template, domainId);
            }
        });

[rohityadavcloud]

@blueorangutan package

[blueorangutan]

@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with SystemVM template(s). I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 5302

[github-actions[bot]]

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

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

75.3% Coverage
0.0% Duplication

[github-actions[bot]]

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

[DaanHoogland]

@stephankruggg can you look at the conflicts? (cc @BryanMLima )

[DaanHoogland]

@stephankruggg is anybody looking at this (or should we postpone)?

[stephankruggg]

@stephankruggg is anybody looking at this (or should we postpone)?

@GaOrtiga can you take at this one, please?

[GaOrtiga]

Hi guys, sorry for the delay.

@stephankruggg is anybody looking at this (or should we postpone)?

@GaOrtiga can you take at this one, please?

Yes.

@stephankruggg is anybody looking at this (or should we postpone)?

I have fixed the conflicts and will run some tests in the next few days before pushing the changes.

[kiranchavala]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

Packaging result [SF]: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: el9 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 7815

[DaanHoogland]

@GaOrtiga (cc @stephankruggg ) see https://github.com/apache/cloudstack/actions/runs/6932958288/job/18881597883?pr=7061#step:7:7641

duplicate import

[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]: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: el9 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 7829

[DaanHoogland]

@stephankruggg have a look at https://github.com/apache/cloudstack/actions/runs/6981605172/job/18999868361?pr=7061#step:7:7641

[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]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7885

[DaanHoogland]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

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

Test	Result	Time (s)	Test File
test_01_invalid_upgrade_kubernetes_cluster	Failure	3612.14	test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster	Failure	3622.98	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	Failure	0.04	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	Failure	0.04	test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster	Failure	0.04	test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster	Failure	0.05	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	Failure	0.04	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	Failure	0.05	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	Failure	0.04	test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster	Failure	50.94	test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle	Error	1.26	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	Error	72.05	test_kubernetes_clusters.py

[DaanHoogland]

@GaOrtiga what is the status on this

[GaOrtiga]

@DaanHoogland I ran into some issues with the integration tests, manually doing what the tests do (for example assigning a VM to an account in another domain) works, but the tests fail. I think we should postpone this PR to a later release.

[github-actions[bot]]

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