Skip to content

Conversation

@cliping
Copy link
Contributor

@cliping cliping commented Jan 4, 2026

Adjust tcp_retries2 to avoid premature TCP timeout before collecting the expected countToDeath information.

Before:
FAIL: The string 'countToDeath=4 idle=10' is not included in /var/log/avocado/job-results/job-2026-01-04T04.49-bdaf5ae/test-results/1-type_specific.io-github-autotest-libvirt.migration.p2p_keepalive.src_keepalive_disabled.after_keepalive_timeout.desturi_ssh.with_precopy/libvirtd.log

After:
(1/2) type_specific.io-github-autotest-libvirt.migration.p2p_keepalive.src_keepalive_disabled.after_keepalive_timeout.desturi_ssh.with_precopy: STARTED
(1/2) type_specific.io-github-autotest-libvirt.migration.p2p_keepalive.src_keepalive_disabled.after_keepalive_timeout.desturi_ssh.with_precopy: PASS (256.41 s)
(2/2) type_specific.io-github-autotest-libvirt.migration.p2p_keepalive.src_keepalive_disabled.after_keepalive_timeout.desturi_tcp.with_precopy: STARTED
(2/2) type_specific.io-github-autotest-libvirt.migration.p2p_keepalive.src_keepalive_disabled.after_keepalive_timeout.desturi_tcp.with_precopy: PASS (287.43 s)
RESULTS : PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML : /var/log/avocado/job-results/job-2026-01-04T08.36-090b665/results.html
JOB TIME : 546.56 s

(1/2) type_specific.io-github-autotest-libvirt.migration.p2p_keepalive.src_keepalive_disabled.before_keepalive_timeout.desturi_ssh.with_precopy: STARTED
(1/2) type_specific.io-github-autotest-libvirt.migration.p2p_keepalive.src_keepalive_disabled.before_keepalive_timeout.desturi_ssh.with_precopy: PASS (249.35 s)
(2/2) type_specific.io-github-autotest-libvirt.migration.p2p_keepalive.src_keepalive_disabled.before_keepalive_timeout.desturi_tcp.with_precopy: STARTED
(2/2) type_specific.io-github-autotest-libvirt.migration.p2p_keepalive.src_keepalive_disabled.before_keepalive_timeout.desturi_tcp.with_precopy: PASS (277.82 s)
RESULTS : PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML : /var/log/avocado/job-results/job-2026-01-04T09.06-3371443/results.html
JOB TIME : 529.89 s

Summary by CodeRabbit

  • Tests
    • Simplified TCP keepalive configuration in migration test scenarios by reducing parameters to essential settings.

✏️ Tip: You can customize this high-level summary in your review settings.

Adjust tcp_retries2 to avoid premature TCP timeout before collecting
the expected countToDeath information.

Signed-off-by: lcheng <lcheng@redhat.com>
@coderabbitai
Copy link

coderabbitai bot commented Jan 4, 2026

Walkthrough

A test configuration file for libvirt migration keepalive testing was modified to simplify TCP keepalive parameters. The extended TCP keepalive configuration containing multiple probes, intervals, retries, and timeouts was replaced with simplified forms that set only tcp_retries2 for both tcp_config_list and recover_tcp_config_list. This narrows the scope of keepalive parameters tested while maintaining the same control flow and error handling logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title mentions fixing an issue with expected string not being found, but the actual change is a specific adjustment to tcp_retries2 in a test configuration file to fix keepalive timeout behavior. Consider revising the title to be more specific, such as 'migration: Adjust tcp_retries2 in keepalive test config to prevent premature TCP timeout' to better reflect the actual change made.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
libvirt/tests/cfg/migration/p2p_keepalive/src_keepalive_disabled.cfg (1)

34-35: Simplified TCP keepalive configuration successfully fixes the test.

The change from extended TCP keepalive parameters to setting only tcp_retries2 is effective. By adjusting the retransmission retry count (6 for normal operation, 15 for recovery), the TCP connection stays alive longer, allowing the test to properly collect the "countToDeath" diagnostic strings before timeout. This aligns with the PR objective and the tests now pass.

Consider adding a brief inline comment explaining why these specific tcp_retries2 values (6 and 15) were chosen, as it will help future maintainers understand the rationale without needing to reference the PR history.

🔎 Optional: Add clarifying comment
     virsh_migrate_extra = "--bandwidth 10"
     virsh_migrate_dest_state = "running"
     virsh_migrate_src_state = "shut off"
+    # Adjust tcp_retries2 to extend TCP timeout window, allowing keepalive diagnostics
+    # (countToDeath values) to be fully logged before connection drop
     tcp_config_list = '{"tcp_retries2": "6"}'
     recover_tcp_config_list = '{"tcp_retries2": "15"}'
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5944e17 and f0f7667.

📒 Files selected for processing (1)
  • libvirt/tests/cfg/migration/p2p_keepalive/src_keepalive_disabled.cfg
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.

Applied to files:

  • libvirt/tests/cfg/migration/p2p_keepalive/src_keepalive_disabled.cfg
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.

Applied to files:

  • libvirt/tests/cfg/migration/p2p_keepalive/src_keepalive_disabled.cfg
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Python 3.11
  • GitHub Check: Python 3.8
  • GitHub Check: Python 3.12
  • GitHub Check: Python 3.9

@smitterl smitterl merged commit 6734d83 into autotest:master Jan 8, 2026
6 checks passed
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.

3 participants