<fix>[kvm]: extend reconnect echo timeout after libvirtd restart#3990
<fix>[kvm]: extend reconnect echo timeout after libvirtd restart#3990zstack-robot-2 wants to merge 1 commit into
Conversation
Walkthrough在 KVMHost 连接/部署流程中加入判断部署是否触发 libvirtd 重启的标记，并在 echo-host 步骤使用基于主机 VM 数量计算的动态 echo 超时（或默认超时），同时新增工具常量与单元测试验证计算逻辑。 变更Libvirtd 重启 Echo 超时自适应
Sequence Diagram(s)sequenceDiagram
participant KVMHost
participant AnsibleDeploy
participant EchoStep
KVMHost->>KVMHost: 初始化 restartLibvirtdDuringDeploy=false
KVMHost->>AnsibleDeploy: 组装并执行部署（deployArguments）
AnsibleDeploy-->>KVMHost: 返回是否触发 restartLibvirtdDuringDeploy
KVMHost->>EchoStep: 调用 getKvmAgentEchoTimeoutAfterDeploy(deployed, restartLibvirtdDuringDeploy)
EchoStep-->>KVMHost: 返回 echoTimeout（默认或延长）
KVMHost->>EchoStep: 调用 restf.echo(..., timeout=echoTimeout)
预计审查工作量🎯 3 (Moderate) | ⏱️ ~20 分钟 诗
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.42.2)plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaComment |
|
Comment from yaohua.wu: Review: MR !9883 — ZSTAC-84691变更概述：在 KVM 物理机重连的 结论：APPROVED修复方向正确、范围收敛、与 Jira 设计方案完全一致，未发现阻塞合并的问题。以下为改进建议。 🟡 Warning1. 新增的两个纯函数缺少单元测试 —
本 issue 标记为 5.5.22「必须解」，建议补齐测试再合入。 🟢 Suggestion2. 超时调参常量硬编码 —
3. 确认扩展点驱动的 libvirtd 重启路径 —
已核对项（无问题）
🤖 Robot Reviewer |
Resolves: ZSTAC-84691 Change-Id: I6668736975707575756d7665647867686e6b6b76
MatheMatrix
force-pushed
the
sync/haoyu.ding/fix-84691
branch
from
24ef2e4 to
719a2aa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.java (1)
37-39: 🏗️ Heavy lift建议将超时调参常量改为可配置项
这三个值目前是编译期常量，现场调优需要改代码发版。建议改成 GlobalProperty（保留当前默认值），便于不同规模主机按需调整而不改变默认行为。
🤖 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/KVMHostUtils.java` around lines 37 - 39, Replace the three compile-time constants in KVMHostUtils (LIBVIRT_RESTART_ECHO_TIMEOUT_VM_THRESHOLD, LIBVIRT_RESTART_ECHO_TIMEOUT_PER_VM_SECONDS, LIBVIRT_RESTART_ECHO_TIMEOUT_MAX_SECONDS) with GlobalProperty-backed configuration entries (keeping the current numeric values as defaults) and update all usages in KVMHostUtils to read the values via the GlobalProperty accessor rather than the static constants; name the properties clearly (e.g., libvirt.restart.echo.timeout.vmThreshold, .perVmSeconds, .maxSeconds), provide the existing defaults, add any necessary `@GlobalProperty` or registration so they are injectable/readable at runtime, and ensure unit tests or callers that referenced the static fields now obtain the values from the new GlobalProperty API.
🤖 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.
Inline comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.java`:
- Around line 154-156: The boolean-string check in
shouldRestartLibvirtdDuringDeploy can mis-evaluate values with surrounding
whitespace; update the method to trim both parameters (init and restartLibvirtd)
before comparing (e.g., call trim() safely after null-checking each param) and
then use equalsIgnoreCase on the trimmed strings so values like " true " or
"\ntrue\t" are treated as true.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.java`:
- Around line 37-39: Replace the three compile-time constants in KVMHostUtils
(LIBVIRT_RESTART_ECHO_TIMEOUT_VM_THRESHOLD,
LIBVIRT_RESTART_ECHO_TIMEOUT_PER_VM_SECONDS,
LIBVIRT_RESTART_ECHO_TIMEOUT_MAX_SECONDS) with GlobalProperty-backed
configuration entries (keeping the current numeric values as defaults) and
update all usages in KVMHostUtils to read the values via the GlobalProperty
accessor rather than the static constants; name the properties clearly (e.g.,
libvirt.restart.echo.timeout.vmThreshold, .perVmSeconds, .maxSeconds), provide
the existing defaults, add any necessary `@GlobalProperty` or registration so they
are injectable/readable at runtime, and ensure unit tests or callers that
referenced the static fields now obtain the values from the new GlobalProperty
API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 9a9b6257-b724-40cc-ba7a-66ed88f82483
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostUtils.javatest/src/test/java/org/zstack/test/kvm/KVMHostUtilsTest.java
| public static boolean shouldRestartLibvirtdDuringDeploy(String init, String restartLibvirtd) { | ||
| return "true".equalsIgnoreCase(init) || "true".equalsIgnoreCase(restartLibvirtd); | ||
| } |
There was a problem hiding this comment.
布尔字符串判断建议先 trim，避免误判
Line 155 当前仅做 equalsIgnoreCase，若参数值包含前后空格（如 " true "）会返回 false，可能导致本应延长的 echo 超时未生效。
建议修改
public static boolean shouldRestartLibvirtdDuringDeploy(String init, String restartLibvirtd) {
- return "true".equalsIgnoreCase(init) || "true".equalsIgnoreCase(restartLibvirtd);
+ String normalizedInit = init == null ? null : init.trim();
+ String normalizedRestartLibvirtd = restartLibvirtd == null ? null : restartLibvirtd.trim();
+ return "true".equalsIgnoreCase(normalizedInit) || "true".equalsIgnoreCase(normalizedRestartLibvirtd);
}As per coding guidelines: 注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等。
🤖 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/KVMHostUtils.java` around lines 154 -
156, The boolean-string check in shouldRestartLibvirtdDuringDeploy can
mis-evaluate values with surrounding whitespace; update the method to trim both
parameters (init and restartLibvirtd) before comparing (e.g., call trim() safely
after null-checking each param) and then use equalsIgnoreCase on the trimmed
strings so values like " true " or "\ntrue\t" are treated as true.
|
|
||
| long vmCount = Q.New(VmInstanceVO.class) | ||
| .eq(VmInstanceVO_.hostUuid, self.getUuid()) | ||
| .count(); |
There was a problem hiding this comment.
Comment from yaohua.wu:
!=Stopped?
| Assert.assertFalse(KVMHostUtils.shouldForceTlsRedeploy(true, false, false)); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Comment from yaohua.wu:
应该要有个集成测试，这个UT在PR系统里不跑的
|
Comment on Comment from yaohua.wu: 在这里加是否发生了deploy以及libvirt重启, libvirtRestarted = deployArguments.getRestartLibvirtd() |
|
|
||
| @Override | ||
| public void run(final FlowTrigger trigger, Map data) { | ||
| final long echoTimeout = getKvmAgentEchoTimeoutAfterDeploy(deployed, restartLibvirtdDuringDeploy); |
There was a problem hiding this comment.
Comment from yaohua.wu:
final long echoTimeout = libvirtRestarted ? getLibvirtRestartedEchoTimeout() : TimeUnit.SECONDS.toMillis(CoreGlobalProperty.REST_FACADE_ECHO_TIMEOUT)
5 participants
Resolves: ZSTAC-84691
Change-Id: I6668736975707575756d7665647867686e6b6b76
sync from gitlab !9883