-
Notifications
You must be signed in to change notification settings - Fork 1.2k
kvm, ui: fix interface when using vlan subnet for storage traffic type #11245
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
base: 4.19
Are you sure you want to change the base?
Conversation
Fixes apache#7816 Signed-off-by: Abhishek Kumar <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #11245 +/- ##
==========================================
Coverage 15.17% 15.17%
- Complexity 11349 11363 +14
==========================================
Files 5416 5415 -1
Lines 475621 476037 +416
Branches 58054 58117 +63
==========================================
+ Hits 72168 72246 +78
- Misses 395380 395708 +328
- Partials 8073 8083 +10
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where VLAN subnet configurations for storage traffic type were not properly handled in both the KVM hypervisor and the UI. The fix ensures that VLAN-tagged storage networks are correctly created and displayed.
- Updates the UI to display the correct VLAN field for storage IP ranges
- Adds VLAN support for storage traffic type in the KVM bridge VIF driver
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
ui/src/views/infra/network/IpRangesTabStorage.vue | Changes dataIndex from 'vlanid' to 'vlan' to fix VLAN display in storage IP ranges table |
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java | Adds VLAN handling logic for storage traffic type to create proper VLAN bridges |
protocol = Networks.BroadcastDomainType.Vlan.scheme(); | ||
} | ||
if (isValidProtocolAndVnetId(vNetId, protocol)) { | ||
s_logger.debug("creating a vNet dev and bridge for public traffic per traffic label {}" + trafficLabel); |
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.
The debug message incorrectly refers to 'public traffic' when this code is handling storage traffic. It should say 'storage traffic' instead.
s_logger.debug("creating a vNet dev and bridge for public traffic per traffic label {}" + trafficLabel); | |
s_logger.debug("creating a vNet dev and bridge for storage traffic per traffic label {}" + trafficLabel); |
Copilot uses AI. Check for mistakes.
protocol = Networks.BroadcastDomainType.Vlan.scheme(); | ||
} | ||
if (isValidProtocolAndVnetId(vNetId, protocol)) { | ||
s_logger.debug("creating a vNet dev and bridge for public traffic per traffic label {}" + trafficLabel); |
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.
String concatenation is used incorrectly with a placeholder '{}'. Either use String.format() or replace '{}' with '%s' for proper formatting, or use simple concatenation without the placeholder.
s_logger.debug("creating a vNet dev and bridge for public traffic per traffic label {}" + trafficLabel); | |
s_logger.debug("creating a vNet dev and bridge for public traffic per traffic label {}", trafficLabel); |
Copilot uses AI. Check for mistakes.
Signed-off-by: Abhishek Kumar <[email protected]>
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.
clgtm
@shwstppr is this ready for review/test? |
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.
code lgtm
@sureshanaparti this would need testing with the specific setup. I need to check with @rajujith about that. Will update you next week |
Description
Fixes #7816
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
After adding the changes I was able to see new bridge with vlan tag getting created,
UI shows VLAN correctly,
How did you try to break this feature and the system with this change?