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)
• 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

Bug Severity

• BLOCKER
• Critical
• Major
• 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.

[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 ?

[codecov]

Codecov Report

Attention: Patch coverage is 86.41304% with 50 lines in your changes missing coverage. Please review.

Project coverage is 16.02%. Comparing base (c63c7ee) to head (1c79d0d).
Report is 9 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	87.60%	38 Missing and 7 partials ⚠️
...e/cloudstack/api/command/admin/vm/AssignVMCmd.java	0.00%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20    #7061      +/-   ##
============================================
+ Coverage     15.80%   16.02%   +0.22%     
- Complexity    12586    12818     +232     
============================================
  Files          5627     5627              
  Lines        492343   492478     +135     
  Branches      59694    59682      -12     
============================================
+ Hits          77828    78937    +1109     
+ Misses       405992   404877    -1115     
- Partials       8523     8664     +141     

Flag	Coverage Δ
uitests	4.03% <ø> (-0.01%)	⬇️
unittests	16.86% <86.41%> (+0.23%)	⬆️

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.

[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: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5302

[weizhouapache]

overall code lgtm.
left 2 minor comments.

@stephankruggg
can you ask your team to test this and share the testing result as a comment ?

[github-actions]

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

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

75.3% Coverage
0.0% Duplication

[github-actions]

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

[winterhazel]

This looks really good. I compared the original code and the refactored code line by line, and verified that their behavior is overall the same.

Also, I did some basic VM assign tests and verified that, before the patch, if an error occurred when updating the VM's network, the VM would be left in an inconsistent state (to result in an error, I passed the ID of a network that does not exist to the networkids parameter). After the patch, a rollback is performed.

@stephankruggg @BryanMLima @DaanHoogland could someone change this PR to ready?

[winterhazel]

@blueorangutan package

[blueorangutan]

@winterhazel 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]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 11663

[winterhazel]

@blueorangutan package

[blueorangutan]

@winterhazel 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]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11679

[DaanHoogland]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

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

Test	Result	Time (s)	Test File

[DaanHoogland]

clgtm, though a lot of changes there are unit tests to support those. I don't think there is an integration test supporting this scenario, but as it is a bit niche I think we can risk merging. Do you agree @weizhouapache ?