Fix rollback while creating a private gateway

[hsato03]

Description

When creating a private gateway, if an ACL verification error occurs, the changes made up to that point are not rolled back, resulting in inconsistent records in the database.

This PR intends to fix this bug and, if an error occurs during the creation of the private gateway, the changes will be rolled back.

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)
• build/CI

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?

I followed the same steps from How did you try to break this feature and the system with this change? and verified in the database that the network created to the private gateway was rolled back.

How did you try to break this feature and the system with this change?

1. Create 2 VPCs (VPC1 and VPC2);
2. Create an ACL for VPC2;
3. Using CloudMonkey, try to create a private gateway in VPC1 using the ACL from VPC2 (an exception will be thrown: Private gateway and network acl are not in the same vpc);
4. Check the networks table in the database and the network created to the private gateway will be there.

[codecov]

Codecov Report

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

Comparison is base (c7100c3) 21.64% compared to head (101511f) 29.16%.
Report is 16 commits behind head on main.

Files	Patch %	Lines
...ain/java/com/cloud/network/vpc/VpcManagerImpl.java	80.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8244      +/-   ##
============================================
+ Coverage     21.64%   29.16%   +7.52%     
- Complexity    21530    31072    +9542     
============================================
  Files          5052     5193     +141     
  Lines        343910   366294   +22384     
  Branches      49538    53554    +4016     
============================================
+ Hits          74431   106827   +32396     
+ Misses       258607   244825   -13782     
- Partials      10872    14642    +3770     

Flag	Coverage Δ
simulator-marvin-tests	25.11% <50.00%> (+1.96%)	⬆️
uitests	4.49% <ø> (-0.01%)	⬇️
unit-tests	14.81% <30.00%> (?)

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.

[DaanHoogland]

definately an improvement. the try-catch this is embedded in is still to large though and the InvalidParameterValueExceptions thrown are cought in an pokemon catch at the end of it. This createVpcPrivateGateway(..) deserves further cleanup.

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland 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 7780

[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 Build Failed (tid-8342)

[blueorangutan]

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

Test	Result	Time (s)	Test File
test_08_migrate_vm	Error	44.79	test_vm_life_cycle.py
test_hostha_enable_ha_when_host_in_maintenance	Error	302.79	test_hostha_kvm.py

[DaanHoogland]

@shwstppr , I added thois to 4.19 as it is a simple fix and comes with unittest...

[shwstppr]

@DaanHoogland cc @hsato03 do we need any manual testing for this?

[DaanHoogland]

@DaanHoogland cc @hsato03 do we need any manual testing for this?

@shwstppr, the failure case as @hsato03 describes could be encoded in an integration test. I don't think we have to make that conditional for this fix. Normal functionality is verified in existing smoke tests.

[hsato03]

@DaanHoogland cc @hsato03 do we need any manual testing for this?

@shwstppr Yes, I think it would be interesting to have a third party testing.

@shwstppr, the failure case as @hsato03 describes could be encoded in an integration test. I don't think we have to make that conditional for this fix. Normal functionality is verified in existing smoke tests.

@DaanHoogland I'll try to add the smoke tests.

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

[weizhouapache]

code lgtm

it makes sense to validate the parameters before db changes

[weizhouapache]

@blueorangutan package

[blueorangutan]

@weizhouapache 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 7837

[weizhouapache]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

[SF] Trillian test result (tid-8394)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46211 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8244-t8394-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]

Code LGTM, also unit test coverage 👍