4.22 merge fwd #12261
4.22 merge fwd #12261
Conversation
DaanHoogland
requested review from
Copilot,
shwstppr,
sudo87,
sureshanaparti and
vishesh92
and removed request for
shwstppr
There was a problem hiding this comment.
Pull request overview
This PR represents a forward merge from version 4.22, containing multiple enhancements and refactoring across UI components, server-side logic, and testing infrastructure.
Key Changes:
- Enhanced VPN management UI with IP range specification and improved modal flow
- Refactored NIC management with better form structure and tooltips
- Optimized DRS (Dynamic Resource Scheduling) with affinity constraints and performance improvements
- Improved database upgrade checker with standalone/cluster detection
- Refactored storage pool maintenance handling for better code organization
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/network/VpnDetails.vue | Added IP range specification for VPN and reorganized enable/disable flow |
| ui/src/views/network/NicsTab.vue | Converted to vertical form layout with tooltips and added MAC address field |
| ui/src/views/infra/network/IpRangesTabPublic.vue | Added isolation method selection (VLAN/VXLAN) for public IP ranges |
| ui/src/views/iam/EditAccount.vue | Added validation to prevent role changes for default accounts |
| ui/public/locales/en.json | Added localization strings for VPN IP range and isolation method |
| server/.../ClusterDrsServiceImpl.java | Major refactoring to optimize DRS with caching and affinity constraints |
| server/.../AccountManagerImpl.java | Added check to prevent role modification for default accounts |
| server/.../StoragePoolAutomationImpl.java | Refactored maintenance logic into smaller, more focused methods |
| server/.../ManagementServerImpl.java | Split host migration logic into separate phases (compatibility, affinity, capacity) |
| engine/.../DatabaseUpgradeChecker.java | Added standalone detection to prevent clustered upgrades |
| plugins/drs/cluster/.../Balanced.java | Updated to use optimized imbalance calculation |
| plugins/drs/cluster/.../Condensed.java | Updated to use optimized imbalance calculation |
| api/.../ClusterDrsAlgorithm.java | Added optimized methods with pre-calculated metrics arrays |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ui/src/views/network/VpnDetails.vue
Outdated
| </div> | ||
| <div v-else> |
There was a problem hiding this comment.
There is a duplicate closing div tag followed by a v-else that seems to be orphaned. Line 100 has a closing </div> tag, and line 101 has another closing </div> with v-else attribute. This v-else appears misplaced as it's on a closing tag rather than an opening tag. This could cause rendering issues or break the component's template structure.
| </template> | ||
| <a-input | ||
| :placeholder="addNetworkData.apiParams.ipaddress.description" | ||
| v-model:value="addNetworkData.ipaddress" /> |
There was a problem hiding this comment.
The variable name changed from ip to ipaddress but this inconsistency with the previous usage suggests this could be incomplete. The field is named ip in the data structure on line 364, but the v-model binds to addNetworkData.ipaddress on line 120. This mismatch will cause the IP address value not to be properly stored or cleared.
| v-model:value="addNetworkData.ipaddress" /> | |
| v-model:value="addNetworkData.ip" /> |
| :filterOption="(input, option) => { | ||
| return option.label.toLowerCase().indexOf(input.toLowerCase()) >= 0 | ||
| }" > | ||
| <a-select-option value="">{{ }}</a-select-option> |
There was a problem hiding this comment.
The empty string value for the first select option will display as blank, making it unclear to users what this option represents. Consider adding a meaningful label like "Default" or "None" to improve user experience.
| */ | ||
| private static void checkHierarchyForPreparingForMaintenance(StoragePoolVO pool, List<StoragePoolVO> storagePools) { | ||
| for (StoragePoolVO storagePool : storagePools) { | ||
| if (!(storagePool.getParent().equals(pool.getParent()) || !pool.getParent().equals(storagePool.getId())) && |
There was a problem hiding this comment.
The logic for checking hierarchy in PrepareForMaintenance mode appears inverted. The condition !(storagePool.getParent().equals(pool.getParent()) || !pool.getParent().equals(storagePool.getId())) uses double negation which is confusing. The expression !(A || !B) is equivalent to !A && B by De Morgan's law, but this doesn't clearly express the intended logic. This should be simplified for clarity and correctness.
| if (!(storagePool.getParent().equals(pool.getParent()) || !pool.getParent().equals(storagePool.getId())) && | |
| if (!storagePool.getParent().equals(pool.getParent()) && pool.getParent().equals(storagePool.getId()) && |
| hostVmMap.get(vm.getHostId()).remove(vm); | ||
| hostVmMap.get(destHost.getId()).add(vm); |
There was a problem hiding this comment.
The duplicate lines 414-415 suggest an incomplete merge. Line 414 and 415 both perform the same operation: removing vm from source host and adding to destination host. One of these duplicate blocks should be removed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12261 +/- ##
============================================
+ Coverage 17.55% 17.58% +0.02%
- Complexity 15544 15575 +31
============================================
Files 5910 5910
Lines 529162 529372 +210
Branches 64641 64668 +27
============================================
+ Hits 92918 93093 +175
- Misses 425786 425796 +10
- Partials 10458 10483 +25
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:
|
|
DaanHoogland
force-pushed
the
4.22-merge-fwd
branch
from
644948b to
7aba434
Compare
4 participants
Description
This PR...
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?