Skip to content

Conversation

@heywji
Copy link
Contributor

@heywji heywji commented Oct 14, 2025

This patch fixes two places.

  1. Hotfix the extra params for nic1
  2. Change the strict decreasing requirement to allow a plateau at optimal performance. Fixes false failure when packet loss remains 0%.

ID: 4186

Signed-off-by: Wenkang Ji [email protected]

Summary by CodeRabbit

  • New Features

    • Network validation now accepts stable or decreasing packet loss across test runs (plateaus, including 0%, are considered acceptable).
  • Improvements

    • Test messaging updated to state packet loss may remain stable or decrease and to include actual results for troubleshooting.
  • Chores

    • Increased NIC queue sizes in the network test configuration to improve reliability under buffer-shortage scenarios.

@coderabbitai
Copy link

coderabbitai bot commented Oct 14, 2025

Walkthrough

Adds RX/TX queue size parameters to the NIC config string and relaxes the test's ping-loss evaluation to accept non-increasing (stable or decreasing) packet-loss trends; associated comments and error message were updated.

Changes

Cohort / File(s) Summary
Config: NIC queue sizes
qemu/tests/cfg/netkvm_buffer_shortage.cfg
Appends ,rx_queue_size=1024,tx_queue_size=256 to the nic_extra_params_nic1 string.
Test logic: ping result evaluation
qemu/tests/netkvm_buffer_shortage.py
Changes comparison from strictly decreasing (>) to non-increasing (>=) when analyzing ping loss. Updates error message and adds comments noting plateaus (including 0%) are acceptable.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Tester as Test Runner
    participant QEMU as QEMU VM
    participant Guest as Guest OS
    participant Analyzer as analyze_ping_results

    Tester->>QEMU: start VM with updated `nic_extra_params_nic1` (rx_queue_size, tx_queue_size)
    QEMU->>Guest: boot
    Tester->>Guest: run ping workload
    Guest-->>Tester: ping results (packet loss sequence)
    Tester->>Analyzer: analyze ping results
    alt non-increasing or decreasing
        Analyzer-->>Tester: pass (accept stable/decreasing loss)
    else increasing trend
        Analyzer-->>Tester: fail with updated error message (include results)
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my whiskers, configs align,
Queues set tidy—RX and TX shine.
Pings may plateau, steady and bright,
No sudden losses in the night.
A rabbit hops; tests pass light. 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly summarizes the primary change in the pull request by indicating that the netkvm_buffer_shortage test now allows non-increasing packet loss rather than requiring a strict decrease, making it specific and directly related to the main behavior update.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 587dae2 and 753683a.

📒 Files selected for processing (2)
  • qemu/tests/cfg/netkvm_buffer_shortage.cfg (1 hunks)
  • qemu/tests/netkvm_buffer_shortage.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • qemu/tests/netkvm_buffer_shortage.py
  • qemu/tests/cfg/netkvm_buffer_shortage.cfg

Comment @coderabbitai help to get the list of available commands and usage tips.

@heywji
Copy link
Contributor Author

heywji commented Oct 14, 2025

Test Result: PASS

 (1/1) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win2022.x86_64.io-github-autotest-qemu.netkvm_buffer_shortage.q35: STARTED      
 (1/1) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win2022.x86_64.io-github-autotest-qemu.netkvm_buffer_shortage.q35:  PASS (1275.0
5 s)                                                                                                                                               
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0                                                                  
JOB HTML   : /root/avocado/job-results/job-2025-10-14T13.33-af09327/results.html                                                                   
JOB TIME   : 1276.06 s                                                     

@heywji
Copy link
Contributor Author

heywji commented Oct 14, 2025

@leidwang Hi Leidong, could you please help me review this patch?

client_cmd = "start cmd /c py ${client_script} 99999 %s ${port_num}"
param_name = "MinRxBufferPercent"
param_values = "0 25 50 75 100"
nic_extra_params_nic1 += ",rx_queue_size=1024,tx_queue_size=256"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to add these parameters to nic1 now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially pushed the wrong code version to our new test case creation. I found it while debugging the KVMAUTOMA-4186 Jira issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if so, you can add some notes in commit message, to clarify why add this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All done. Please help review the rest. Thanks :)

@heywji heywji force-pushed the fix_netkvm_buffer_shortage branch 2 times, most recently from eb1b048 to 587dae2 Compare October 14, 2025 09:07
@heywji heywji changed the title netkvm_buffer_shortage: allow stable packet loss trend netkvm_buffer_shortage: allow 100% packet reception Oct 14, 2025
This patch fixes two places.
1. Hotfix the extra params for nic1
2. Change the strict decreasing requirement to allow a plateau at
optimal performance. Fixes false failure when packet loss remains 0%.

Signed-off-by: Wenkang Ji <[email protected]>
@heywji heywji force-pushed the fix_netkvm_buffer_shortage branch from 587dae2 to 753683a Compare October 14, 2025 09:13
@heywji heywji changed the title netkvm_buffer_shortage: allow 100% packet reception netkvm_buffer_shortage: allow non-increasing packet lose Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants