<fix>[kvm]: extend reconnect echo timeout after libvirtd restart#4479
Open
zstack-robot-2 wants to merge 1 commit into
Open
<fix>[kvm]: extend reconnect echo timeout after libvirtd restart#4479zstack-robot-2 wants to merge 1 commit into
zstack-robot-2 wants to merge 1 commit into
Conversation
Resolves: ZSTAC-84691 Change-Id: I6668736975707575756d7665647867686e6b6b76 (cherry picked from commit e7851c6)
|
Warning
|
| Layer / File(s) | Summary |
|---|---|
超时计算工具方法与常量 plugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.java |
新增3个超时阈值常量及3个静态方法：判断部署期是否需重启libvirtd、统计host上非Stopped状态VM数、根据VM数计算受上限约束的动态毫秒级echo超时。 |
KVMHost连接流程集成动态超时 plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java |
新增getLibvirtRestartedEchoTimeout()方法，连接流程状态中加入libvirtRestarted标记；部署完成后据此判定，echo-host及假死重启二次echo分支均改用动态超时替换固定的REST_FACADE_ECHO_TIMEOUT。 |
单元测试与集成测试 test/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java, test/src/test/groovy/org/zstack/test/integration/kvm/host/LibvirtRestartEchoTimeoutCase.groovy |
新增单元测试覆盖shouldRestartLibvirtdDuringDeploy及calculateLibvirtRestartEchoTimeoutMillis在阈值、超阈值、上限约束等场景下的行为；新增集成测试验证超时计算及VM计数排除Stopped虚拟机的逻辑。 |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Sequence Diagram(s)
sequenceDiagram
participant KVMHost
participant KVMHostUtils
participant Database
KVMHost->>KVMHostUtils: shouldRestartLibvirtdDuringDeploy(init, restartLibvirtd)
KVMHostUtils-->>KVMHost: libvirtRestarted
alt libvirtRestarted为true
KVMHost->>KVMHostUtils: countVmsForLibvirtRestartEchoTimeout(hostUuid)
KVMHostUtils->>Database: 查询非Stopped状态的VM数量
Database-->>KVMHostUtils: vmCount
KVMHostUtils->>KVMHostUtils: calculateLibvirtRestartEchoTimeoutMillis(vmCount)
KVMHostUtils-->>KVMHost: echoTimeout(扩展)
else libvirtRestarted为false
KVMHost->>KVMHost: echoTimeout=REST_FACADE_ECHO_TIMEOUT
end
KVMHost->>KVMHost: restf.echo(echoPath, echoTimeout)
Poem
兔子敲键盘，超时算得妙 🐰
libvirtd重启莫慌张，
数一数虚拟机几只忙，
加点耐心加点秒，
echo稳稳到彼岸～
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | 标题准确概括了本次改动：在 libvirtd 重启后延长 KVM 重连 echo 超时。 |
| Description check | ✅ Passed | 描述虽简短，但与该补丁的来源、工单和同步信息一致，未偏离本次改动。 |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
sync/haoyu.ding/fix-cherrypick5@@3
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 ast-grep (0.44.1)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
ast-grep timed out on this file
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/src/test/groovy/org/zstack/test/integration/kvm/host/LibvirtRestartEchoTimeoutCase.groovy (1)
45-59: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
testCalculateLibvirtRestartEchoTimeout与单元测试内容重复该方法的断言与
KVMHostUtilsTest.java中4个测试方法完全重复，且不依赖env环境（host/vm），放在集成测试中会不必要地增加该用例套件的执行开销。可考虑仅保留testCountVmsForLibvirtRestartEchoTimeoutExcludesStoppedVm（真正需要DB/环境支持的用例），将纯逻辑断言留给Java单元测试即可。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/src/test/groovy/org/zstack/test/integration/kvm/host/LibvirtRestartEchoTimeoutCase.groovy` around lines 45 - 59, Remove the pure logic coverage from LibvirtRestartEchoTimeoutCase by deleting testCalculateLibvirtRestartEchoTimeout, since its assertions are already covered in KVMHostUtilsTest and it does not need env-dependent integration setup. Keep only the env/DB-backed test that validates VM counting behavior, and leave the timeout calculation checks in the existing unit tests around KVMHostUtils.calculateLibvirtRestartEchoTimeoutMillis.plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (1)
5903-5949: 🧹 Nitpick | 🔵 Trivial大规模主机上echo超时延长可能推迟"假死重启"检测
当
libvirtRestarted为真且主机VM数量较多时，首次restf.echo的等待时间最长可达180s（或更高，取决于REST_FACADE_ECHO_TIMEOUT基础值）。若此时kvmagent确实处于假死状态（而非仅仅因libvirt初始化慢导致响应慢），"假死重启"逻辑的触发会因此被推迟，从而延长整体主机重连耗时。这是该特性的合理权衡，建议在验收/监控层面关注大规模VM主机重连耗时的变化。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 5903 - 5949, The `restf.echo` retry path in `KVMHost` can delay fake-dead detection too long when `libvirtRestarted` is true and the host is slow, so adjust the timeout strategy in this block to keep the initial echo check short enough to trigger `AnsibleRunner.restartAgent` promptly. Use `libvirtRestarted`, `getLibvirtRestartedEchoTimeout()`, and the `restf.echo(..., Completion)` fail branch to separate “restart verification” from the longer post-restart stabilization wait, so the fake-dead restart decision is not postponed by the extended echo timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5903-5949: The `restf.echo` retry path in `KVMHost` can delay
fake-dead detection too long when `libvirtRestarted` is true and the host is
slow, so adjust the timeout strategy in this block to keep the initial echo
check short enough to trigger `AnsibleRunner.restartAgent` promptly. Use
`libvirtRestarted`, `getLibvirtRestartedEchoTimeout()`, and the `restf.echo(...,
Completion)` fail branch to separate “restart verification” from the longer
post-restart stabilization wait, so the fake-dead restart decision is not
postponed by the extended echo timeout.
In
`@test/src/test/groovy/org/zstack/test/integration/kvm/host/LibvirtRestartEchoTimeoutCase.groovy`:
- Around line 45-59: Remove the pure logic coverage from
LibvirtRestartEchoTimeoutCase by deleting
testCalculateLibvirtRestartEchoTimeout, since its assertions are already covered
in KVMHostUtilsTest and it does not need env-dependent integration setup. Keep
only the env/DB-backed test that validates VM counting behavior, and leave the
timeout calculation checks in the existing unit tests around
KVMHostUtils.calculateLibvirtRestartEchoTimeoutMillis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e391a83-fca8-410c-81a1-5a9c00c76c50
📒 Files selected for processing (4)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.javatest/src/test/groovy/org/zstack/test/integration/kvm/host/LibvirtRestartEchoTimeoutCase.groovytest/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: ZSTAC-84691
Change-Id: I6668736975707575756d7665647867686e6b6b76
(cherry picked from commit e7851c6)
sync from gitlab !10441