Fix issue with Assign VM operation #10834
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #10834 +/- ##
=========================================
Coverage 16.13% 16.13%
Complexity 13220 13220
=========================================
Files 5651 5651
Lines 496740 496797 +57
Branches 60183 60190 +7
=========================================
+ Hits 80148 80161 +13
- Misses 407674 407712 +38
- Partials 8918 8924 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There was a problem hiding this comment.
Somehow changes doesn't look right to me. @Pearl1594 could there be a better way to handle errors?
| if (networkVOS.size() == 1) { | ||
| _networkDao.remove(networkVOS.get(0).getId()); | ||
| } | ||
| _accountMgr.getActiveAccountByName(newAccount.getAccountName() + "-network", newAccount.getDomainId()); |
There was a problem hiding this comment.
Why this? name won't be null with current code. Do we expect domainId to return null and then throw InvalidParameterValueException?
There was a problem hiding this comment.
that's by mistake
| List<NetworkVO> networkVOS = _networkDao.listByAccountIdNetworkName(newAccountId, newAccount.getAccountName() + "-network"); | ||
| if (networkVOS.size() == 1) { | ||
| _networkDao.remove(networkVOS.get(0).getId()); | ||
| } |
There was a problem hiding this comment.
Can network be in implemented state or something? Do we really want to mark it as removed directly?
There was a problem hiding this comment.
It's not ready for review - stil working on a way to see if the network was created during the assignVM operation and only then remove it
| @@ -7961,7 +7967,10 @@ protected void selectApplicableNetworkToCreateVm(Account caller, Account newAcco | |||
| NetworkVO defaultNetwork; | |||
| List<? extends Network> virtualNetworks = _networkModel.listNetworksForAccount(newAccount.getId(), zone.getId(), Network.GuestType.Isolated); | |||
| if (virtualNetworks.isEmpty()) { | |||
| defaultNetwork = createApplicableNetworkToCreateVm(caller, newAccount, zone, firstRequiredOffering); | |||
| try (TransactionLegacy txn = TransactionLegacy.open("CreateNetworkTxn")) { | |||
There was a problem hiding this comment.
Transaction.execute() won't work here?
Description
This PR fixes: #10825
Caused by: #7061
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
I also simulated a scenario where an exception occurs after the network is created in the new account, and confirmed that the network is properly cleaned up — thereby preserving atomicity.
How did you try to break this feature and the system with this change?