Rollback of changes with errors during the VM assign #7061

stephankruggg · 2023-01-05T13:34:08Z

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

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 ?

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

codecov · 2023-01-05T14:30:37Z

Codecov Report

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

Project coverage is 16.13%. Comparing base (ab76d3c) to head (7d882a9).
Report is 6 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     16.07%   16.13%   +0.06%     
- Complexity    12883    12965      +82     
============================================
  Files          5639     5639              
  Lines        494193   494241      +48     
  Branches      59925    59896      -29     
============================================
+ Hits          79419    79753     +334     
+ Misses       405944   405667     -277     
+ Partials       8830     8821       -9

Flag	Coverage Δ
uitests	`4.02% <ø> (ø)`
unittests	`16.98% <86.41%> (+0.07%)`	⬆️

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 · 2023-01-05T15:08:26Z

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 · 2023-01-05T15:37:11Z

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 · 2023-01-18T05:14:53Z

@blueorangutan package

blueorangutan · 2023-01-18T05:16:06Z

@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 · 2023-01-18T05:57:11Z

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 ?

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

github-actions · 2023-02-08T14:56:38Z

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

sonarqubecloud · 2023-02-09T22:29:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

75.3% Coverage
0.0% Duplication

github-actions · 2023-03-23T15:38:01Z

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

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

DaanHoogland · 2023-10-31T14:19:54Z

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

DaanHoogland · 2023-11-15T15:18:15Z

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

stephankruggg · 2023-11-15T15:47:41Z

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

@GaOrtiga can you take at this one, please?

GaOrtiga · 2023-11-15T15:57:39Z

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 · 2023-11-21T09:21:53Z

@blueorangutan package

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 ?

github-actions · 2025-01-06T11:47:14Z

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

winterhazel · 2025-01-08T13:23:22Z

@blueorangutan package

blueorangutan · 2025-01-08T13:24:07Z

@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 · 2025-01-08T14:05:08Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 12014

winterhazel · 2025-01-08T14:39:16Z

@blueorangutan package

blueorangutan · 2025-01-08T14:40:03Z

@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 · 2025-01-08T15:49:51Z

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

DaanHoogland · 2025-01-09T08:38:56Z

@blueorangutan test

blueorangutan · 2025-01-09T08:40:03Z

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

blueorangutan · 2025-01-10T00:19:41Z

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

Test	Result	Time (s)	Test File
test_11_isolated_network_with_dynamic_routed_mode	`Error`	2.27	test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode	`Error`	3.42	test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode	`Error`	3.42	test_ipv4_routing.py

bernardodemarco

CLGTM

DaanHoogland · 2025-01-15T14:34:27Z

[SF] Trillian test result (tid-12067) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 53801 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7061-t12067-kvm-ol8.zip Smoke tests completed. 140 look OK, 1 have errors, 0 did not run Only failed and skipped tests results shown below:
Test Result Time (s) Test File
test_11_isolated_network_with_dynamic_routed_mode Error 2.27 test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode Error 3.42 test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode Error 3.42 test_ipv4_routing.py

these are a generic issue on 4.20 and not specific to this PR. Is there any more testing required for this @stephankruggg ?
cc @winterhazel @weizhouapache @bernardodemarco

winterhazel · 2025-01-15T15:04:45Z

these are a generic issue on 4.20 and not specific to this PR. Is there any more testing required for this @stephankruggg ? cc @winterhazel @weizhouapache @bernardodemarco

I don't think we require more testing. I already validated that it fixed the original issue, and took a good look at the code comparing it with the current version to ensure that the behavior is the same.

* 4.20: Rollback of changes with errors during the VM assign (#7061) [VMware] Consider CD/DVD drive when calculating next free unit number for volume attachment over IDE controller (#9644) consider a valid ipv4 address as a validish ipv4 /32 cidr (#10174)

Co-authored-by: Stephan Krug <[email protected]> Co-authored-by: Gabriel <[email protected]> Co-authored-by: Fabricio Duarte <[email protected]>

This reverts commit d9a7b6e.

boring-cyborg bot added the component:api label Jan 5, 2023

weizhouapache reviewed Jan 5, 2023

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

weizhouapache reviewed Jan 18, 2023

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Show resolved Hide resolved

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

github-actions bot added the status:has-conflicts label Feb 8, 2023

stephankruggg force-pushed the rollback-on-assign-vm-error branch from 8ecc838 to 5a23d33 Compare February 9, 2023 21:22

github-actions bot removed the status:has-conflicts label Feb 9, 2023

github-actions bot added the status:has-conflicts label Mar 23, 2023

DaanHoogland added this to the 4.19.0.0 milestone Jun 22, 2023

DaanHoogland reviewed Oct 31, 2023

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

DaanHoogland assigned stephankruggg Nov 15, 2023

stephankruggg removed their assignment Nov 15, 2023

DaanHoogland assigned GaOrtiga Nov 16, 2023

GaOrtiga force-pushed the rollback-on-assign-vm-error branch from 5a23d33 to 125353b Compare November 20, 2023 16:27

github-actions bot removed the status:has-conflicts label Nov 20, 2023

DaanHoogland added component:orchestration and removed component:api labels Nov 21, 2023

DaanHoogland requested review from DaanHoogland and GaOrtiga December 4, 2024 07:13

DaanHoogland approved these changes Dec 4, 2024

View reviewed changes

github-actions bot added the status:has-conflicts label Jan 6, 2025

Merge branch '4.20' into rollback-on-assign-vm-error

453896a

github-actions bot removed the status:has-conflicts label Jan 8, 2025

Fix build

7d882a9

bernardodemarco approved these changes Jan 15, 2025

View reviewed changes

DaanHoogland merged commit d9a7b6e into apache:4.20 Jan 15, 2025
26 checks passed

Pearl1594 added this to ACS 4.20.1 Mar 17, 2025

Pearl1594 moved this to Done in ACS 4.20.1 Mar 17, 2025

Pearl1594 mentioned this pull request May 8, 2025

Fix issue with Assign VM operation #10834

Closed

14 tasks

shwstppr mentioned this pull request May 13, 2025

Failure to assign VM to another account #10825

Closed

shwstppr added a commit that referenced this pull request May 13, 2025

Revert "Rollback of changes with errors during the VM assign (#7061)"

734ce3c

This reverts commit d9a7b6e.

shwstppr mentioned this pull request May 13, 2025

[DO NOT MERGE - TEST] Revert "Rollback of changes with errors during the VM assign" #10851

Closed

Pearl1594 mentioned this pull request May 13, 2025

Fix issue with Assign VM operation #10845

Closed

12 tasks

Rollback of changes with errors during the VM assign #7061

Rollback of changes with errors during the VM assign #7061

Uh oh!

Conversation

stephankruggg commented Jan 5, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

How Has This Been Tested?

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stephankruggg commented Jan 5, 2023

Uh oh!

weizhouapache commented Jan 5, 2023

Uh oh!

rohityadavcloud commented Jan 18, 2023

Uh oh!

blueorangutan commented Jan 18, 2023

Uh oh!

blueorangutan commented Jan 18, 2023

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 8, 2023

Uh oh!

sonarqubecloud bot commented Feb 9, 2023

Uh oh!

github-actions bot commented Mar 23, 2023

Uh oh!

Uh oh!

DaanHoogland commented Oct 31, 2023

Uh oh!

DaanHoogland commented Nov 15, 2023

Uh oh!

stephankruggg commented Nov 15, 2023

Uh oh!

GaOrtiga commented Nov 15, 2023

Uh oh!

kiranchavala commented Nov 21, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 6, 2025

Uh oh!

winterhazel commented Jan 8, 2025

Uh oh!

blueorangutan commented Jan 8, 2025

Uh oh!

blueorangutan commented Jan 8, 2025

Uh oh!

winterhazel commented Jan 8, 2025

Uh oh!

blueorangutan commented Jan 8, 2025

Uh oh!

blueorangutan commented Jan 8, 2025

Uh oh!

DaanHoogland commented Jan 9, 2025

Uh oh!

blueorangutan commented Jan 9, 2025

Uh oh!

blueorangutan commented Jan 10, 2025

Uh oh!

bernardodemarco left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jan 15, 2025

Uh oh!

winterhazel commented Jan 15, 2025

codecov bot commented Jan 5, 2023 •

edited

Loading