-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix NPE on updating security groups for an instance #10493
base: 4.20
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #10493 +/- ##
============================================
- Coverage 15.98% 15.98% -0.01%
+ Complexity 13086 13085 -1
============================================
Files 5650 5650
Lines 495756 495760 +4
Branches 60018 60017 -1
============================================
- Hits 79261 79257 -4
- Misses 407641 407646 +5
- Partials 8854 8857 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
1a1eec2
to
351b012
Compare
@blueorangutan package |
@vishesh92 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. |
} | ||
} | ||
|
||
if (_networkModel.checkSecurityGroupSupportForNetwork( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any issue if check all networks instead of the default network here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible for a VM to have an isolated network as the default network and a shared network with SG as a second one. In this case if we don't check for all networks, security groups won't get updated.
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12641 |
@blueorangutan test keepEnv |
@vishesh92 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
[SF] Trillian test result (tid-12545)
|
Description
This PR fixes #10347
Details
This pull request includes changes to the
UserVmManagerImpl
class and UI components to improve the handling of security groups for virtual machines. The most important changes include refactoring the security group update logic and adding UI support for security groups.Backend changes:
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
: Refactored the security group update logic by removing redundant code and encapsulating the update logic in a newupdateSecurityGroup
method. This improves code readability and maintainability. [1] [2] [3]UI changes:
ui/src/views/compute/EditVM.vue
: Modified the security group enablement check to fall back on a Vuex store getter if the zone response does not provide the information. This ensures that the UI correctly reflects the security group support status.ui/src/views/compute/InstanceTab.vue
: Added a new state variableshowUpdateSecurityGroupsModal
to manage the visibility of the security group update modal in the instance tab. This prepares the UI for future enhancements to support security group updates directly from the instance tab.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?