-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Network rate must be multiplied by 125 not 128 #10645
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
Conversation
wido
left a comment
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.
Change looks good. We need to multiple by 125 when working with Bits. If it would be bytes we would need to convert with 128.
This change looks good to make the proper calculation.
LGTM
|
@Rubueno , i am assuming you are testing this in production, are you? ;) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10645 +/- ##
============================================
+ Coverage 16.30% 17.36% +1.05%
- Complexity 13448 15235 +1787
============================================
Files 5674 5885 +211
Lines 499203 525624 +26421
Branches 60369 64152 +3783
============================================
+ Hits 81408 91257 +9849
- Misses 408726 424072 +15346
- Partials 9069 10295 +1226
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:
|
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java
Outdated
Show resolved
Hide resolved
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java
Outdated
Show resolved
Hide resolved
In a libvirt domain xml when nw_rate=5000 the kilobytes per second configured in the interface is 640000 which is 5.12 Gbit/s. Which is more than the configured network rate of 5Gbit/s. So instead multiply by 125.
DaanHoogland
left a comment
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
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12982 |
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.
LGTM - All functional, edge, and lifecycle scenarios pass; fix verified.
Test Cases Covered & Status
-
NetworkRate-50Mbps
- Expected: 6250 Kbps (50×125)
- Status: PASS
-
NetworkRate-100Mbps
- Expected: 12500 Kbps (100×125)
- Status: PASS
-
NetworkRate-500Mbps
- Expected: 62500 Kbps (500×125)
- Status: PASS
-
NetworkRate-ZeroRate
- Expected: No bandwidth tags
- Status: PASS
-
CrossHost-Consistency
- Expected: Same values across KVM hosts
- Status: PASS
-
EdgeCase-Handling
- Expected: Zero rate properly handled
- Status: PASS
-
Negative/Invalid-Input
- Expected: Handled gracefully (error/no effect)
- Status: PASS
-
Instance Lifecycle Retest
- Expected: Same values after destroy & redeploy
- Status: PASS
-
Hot-Update Check
- Expected: XML updates correctly on rate change
- Status: PASS
Evidence - libvirt XML Dumps
- 100 Mbps Instance: - inbound/outbound average=12500 peak=12500
<interface type='bridge'>
<mac address='02:01:00:cd:00:04'/>
<source bridge='breth1-1639'/>
<bandwidth>
<inbound average='12500' peak='12500'/>
<outbound average='12500' peak='12500'/>
</bandwidth>
<target dev='vnet9'/>
<model type='virtio'/>
</interface>
- 50 Mbps Instance: - inbound/outbound average=6250 peak=6250
<interface type='bridge'>
<mac address='02:01:00:cd:00:03'/>
<source bridge='breth1-1639'/>
<bandwidth>
<inbound average='6250' peak='6250'/>
<outbound average='6250' peak='6250'/>
</bandwidth>
<target dev='vnet2'/>
<model type='virtio'/>
</interface>
- 500 Mbps Instance: - inbound/outbound average=62500 peak=62500
<interface type='bridge'>
<mac address='02:01:00:cd:00:05'/>
<source bridge='breth1-1639'/>
<bandwidth>
<inbound average='62500' peak='62500'/>
<outbound average='62500' peak='62500'/>
</bandwidth>
<target dev='vnet3'/>
<model type='virtio'/>
</interface>
- Zero Rate Instance: - No bandwidth section found - correctly handled zero rate case
[root@kvm1 ~]# virsh dumpxml i-2-9-VM | grep -A5 -B5 bandwidth
[root@kvm1 ~]#
- Original Instance, confirming New Factor (5000 Mbps): - inbound/outbound average=625000 peak=625000
- 5000 Mbps × 125 = 625,000 Kbps (confirms new factor working)
<interface type='bridge'>
<mac address='02:01:00:cd:00:01'/>
<source bridge='breth1-1639'/>
<bandwidth>
<inbound average='625000' peak='625000'/>
<outbound average='625000' peak='625000'/>
</bandwidth>
<target dev='vnet1'/>
<model type='virtio'/>
</interface>
|
@blueorangutan test |
|
@rosi-shapeblue a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/VifDriverBase.java
Outdated
Show resolved
Hide resolved
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/VifDriverBase.java
Outdated
Show resolved
Hide resolved
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/VifDriverBase.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Suresh Kumar Anaparti <[email protected]>
sureshanaparti
left a comment
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
|
@blueorangutan package |
|
@sureshanaparti 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 14595 |
|
[SF] Trillian test result (tid-14050)
|
* Network rate must be multiplied by 125 not 128 In a libvirt domain xml when nw_rate=5000 the kilobytes per second configured in the interface is 640000 which is 5.12 Gbit/s. Which is more than the configured network rate of 5Gbit/s. So instead multiply by 125. * Apply suggestions from code review Co-authored-by: Suresh Kumar Anaparti <[email protected]>
Description
This PR fixes a minor bug in the libvirt domain xml concering the interface bandwidth. In a libvirt domain xml when nw_rate=5000 the kilobytes per second configured in the interface is 640000 which is 5.12 Gbit/s. Which is more than the configured network rate of 5Gbit/s. So instead multiply by 125 (1 Megabit = 125 Kilobyte). 5000 * 125 = 625000 KB/s = 5Gbit/s
QoS is defined in Kilobytes per second according to libvirt docs
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
How Has This Been Tested?
Untested, but the change is trivial.
How did you try to break this feature and the system with this change?
N/A