<feature>[kvm]: add kvmagent auto-restart window config#3863
<feature>[kvm]: add kvmagent auto-restart window config#3863zstack-robot-2 wants to merge 1 commit into5.5.16from
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Walkthrough新增 KVM 代理自动重启时间窗口配置与校验逻辑:增加全局配置项、在内存硬限告警触发的重启路径中加入时间窗检查并阻止不在允许窗口内的重启,同时新增对应单元测试覆盖边界和跨午夜情况。 Changes
Sequence Diagram(s)sequenceDiagram
participant Alarm as AlarmHandler
participant Config as GlobalConfigService
participant HostFactory as KVMHostFactory
participant Agent as KvmAgent (via MsgBus)
Alarm->>HostFactory: 内存硬限告警触发
HostFactory->>Config: 读取 kvmagent.autorestart.window
Config-->>HostFactory: 返回窗口字符串 (或空)
HostFactory->>HostFactory: 调用 isNowInAutoRestartWindow(config, now)
alt 在允许窗口内
HostFactory->>Agent: 发送 RestartKvmAgentMsg
else 不在允许窗口内
HostFactory-->>Alarm: 记录并中止重启(延后)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java`:
- Around line 152-153: The GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW currently
uses the bare `@GlobalConfigValidation` which enforces notNull=true/notEmpty=true
and thus prevents using null/empty to mean "no time window"; update the
annotation on KVMAGENT_AUTO_RESTART_WINDOW to explicitly allow null/empty (e.g.,
set notNull=false and notEmpty=false) so users can clear the configured window
to mean "always allowed", and ensure any downstream validation logic that reads
KVMAGENT_AUTO_RESTART_WINDOW honors null/empty as "no restriction".
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java`:
- Around line 479-490: The method isNowInAutoRestartWindow currently calls
LocalTime.parse on configValue parts without validation, which can throw and
break alert flows for dirty config; update isNowInAutoRestartWindow to validate
parts.length == 2 and wrap parsing in a try-catch that catches
DateTimeParseException/RuntimeException, emit a logger.warn including the
offending configValue and exception, and return false (treat as outside window)
on any parse/validation failure so malformed values do not throw.
🪄 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: 688e3a12-d6ba-4cba-a18d-4750fa67950d
⛔ Files ignored due to path filters (3)
conf/globalConfig/kvm.xmlis excluded by!**/*.xmlplugin/kvm/pom.xmlis excluded by!**/*.xmltest/src/test/resources/globalConfig/kvm.xmlis excluded by!**/*.xml
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.javaplugin/kvm/src/test/java/org/zstack/kvm/TestKVMAutoRestartWindow.java
| @GlobalConfigValidation | ||
| public static GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW = new GlobalConfig(CATEGORY, "kvmagent.autorestart.window"); |
There was a problem hiding this comment.
空值语义与当前注解默认校验存在冲突。
这里使用裸 @GlobalConfigValidation 会沿用默认 notNull=true/notEmpty=true,与“空值表示不限时段”的需求冲突;用户也无法把已配置窗口恢复为“始终允许”。
💡 建议修复
- `@GlobalConfigValidation`
- public static GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW = new GlobalConfig(CATEGORY, "kvmagent.autorestart.window");
+ `@GlobalConfigValidation`(notNull = false, notEmpty = false)
+ `@GlobalConfigDef`(
+ defaultValue = "",
+ description = "daily local-time window for kvmagent auto restart, format HH:MM-HH:MM; empty means always allowed"
+ )
+ public static GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW = new GlobalConfig(CATEGORY, "kvmagent.autorestart.window");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java` around lines
152 - 153, The GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW currently uses the bare
`@GlobalConfigValidation` which enforces notNull=true/notEmpty=true and thus
prevents using null/empty to mean "no time window"; update the annotation on
KVMAGENT_AUTO_RESTART_WINDOW to explicitly allow null/empty (e.g., set
notNull=false and notEmpty=false) so users can clear the configured window to
mean "always allowed", and ensure any downstream validation logic that reads
KVMAGENT_AUTO_RESTART_WINDOW honors null/empty as "no restriction".
07274f2 to
74af3db
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java (1)
479-490:⚠️ Potential issue | 🟠 Major窗口解析缺少异常兜底,脏配置会中断告警处理链路。
Line 483-485 直接
split+LocalTime.parse,遇到历史脏值/绕过校验写库时会抛异常,建议按“窗口外”降级而不是抛出到调用链。💡 建议修复
+import java.time.format.DateTimeParseException; @@ static boolean isNowInAutoRestartWindow(String configValue, LocalTime now) { if (configValue == null || configValue.trim().isEmpty()) { return true; } - String[] parts = configValue.trim().split("-"); - LocalTime start = LocalTime.parse(parts[0]); - LocalTime end = LocalTime.parse(parts[1]); + String[] parts = configValue.trim().split("-"); + if (parts.length != 2) { + logger.warn(String.format("invalid auto-restart window config[%s], treat as out-of-window", configValue)); + return false; + } + + final LocalTime start; + final LocalTime end; + try { + start = LocalTime.parse(parts[0].trim()); + end = LocalTime.parse(parts[1].trim()); + } catch (DateTimeParseException | RuntimeException e) { + logger.warn(String.format("invalid auto-restart window config[%s], treat as out-of-window", configValue), e); + return false; + } if (start.isBefore(end)) { return !now.isBefore(start) && now.isBefore(end); } return !now.isBefore(start) || now.isBefore(end); }#!/bin/bash set -euo pipefail echo "== 1) 检查窗口解析实现是否有 split/parse 防御 ==" rg -n -A20 -B5 'static boolean isNowInAutoRestartWindow|split\("-"\)|LocalTime\.parse|try|catch' \ plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java echo echo "== 2) 检查调用点是否直接依赖该方法返回值(无异常保护) ==" rg -n -A10 -B5 'isNowInAutoRestartWindow\(' \ plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java` around lines 479 - 490, The isNowInAutoRestartWindow method currently splits configValue and calls LocalTime.parse directly which can throw on malformed/dirty values; change isNowInAutoRestartWindow to defensively validate and parse the config string (verify non-null, non-empty, parts.length == 2, each part non-empty) and wrap the parsing/logic in a try-catch that catches DateTimeParseException and any other runtime exceptions, and on any parse/format error return false (treat as "outside window") instead of letting exceptions propagate; keep existing start/end logic (including the overnight case) inside the guarded block so callers (e.g., any isNowInAutoRestartWindow(...) callers) never receive exceptions from malformed config.plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java (1)
152-153:⚠️ Potential issue | 🟠 Major
KVMAGENT_AUTO_RESTART_WINDOW的空值语义可能被默认校验拦截。Line 152 使用裸
@GlobalConfigValidation,可能导致无法把值清空为“始终允许”,这会和本次需求里的“空字符串=不限时段”产生冲突。💡 建议修复
- `@GlobalConfigValidation` - public static GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW = new GlobalConfig(CATEGORY, "kvmagent.autorestart.window"); + `@GlobalConfigValidation`(notNull = false, notEmpty = false) + `@GlobalConfigDef`( + defaultValue = "", + description = "daily local-time window for kvmagent auto restart, format HH:MM-HH:MM; empty means always allowed" + ) + public static GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW = new GlobalConfig(CATEGORY, "kvmagent.autorestart.window");#!/bin/bash set -euo pipefail echo "== 1) 检查 GlobalConfigValidation 注解默认值 ==" fd -i "GlobalConfigValidation.java" --exec sed -n '1,220p' {} echo echo "== 2) 检查该配置项定义是否允许空值/是否有默认值 ==" rg -n 'KVMAGENT_AUTO_RESTART_WINDOW|kvmagent\.autorestart\.window|GlobalConfigDef|GlobalConfigValidation' \ plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java -C3As per coding guidelines: “向后兼容原则……不应直接改动已有行为;需要改动应通过开关/兼容方式处理”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java` around lines 152 - 153, KVMAGENT_AUTO_RESTART_WINDOW is annotated with a bare `@GlobalConfigValidation` which will block empty values (but we need empty string to mean “no time limit”); update the validation for GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW so empty string is treated as valid — e.g. replace or modify the annotation on KVMAGENT_AUTO_RESTART_WINDOW (or its GlobalConfigDef) to explicitly allow empty/blank values (use the validation flag like allowEmpty=true or switch to a validator variant that permits blank) so the config can be cleared to mean “always allowed” while keeping other validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java`:
- Around line 152-153: KVMAGENT_AUTO_RESTART_WINDOW is annotated with a bare
`@GlobalConfigValidation` which will block empty values (but we need empty string
to mean “no time limit”); update the validation for GlobalConfig
KVMAGENT_AUTO_RESTART_WINDOW so empty string is treated as valid — e.g. replace
or modify the annotation on KVMAGENT_AUTO_RESTART_WINDOW (or its
GlobalConfigDef) to explicitly allow empty/blank values (use the validation flag
like allowEmpty=true or switch to a validator variant that permits blank) so the
config can be cleared to mean “always allowed” while keeping other validation
behavior.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java`:
- Around line 479-490: The isNowInAutoRestartWindow method currently splits
configValue and calls LocalTime.parse directly which can throw on
malformed/dirty values; change isNowInAutoRestartWindow to defensively validate
and parse the config string (verify non-null, non-empty, parts.length == 2, each
part non-empty) and wrap the parsing/logic in a try-catch that catches
DateTimeParseException and any other runtime exceptions, and on any parse/format
error return false (treat as "outside window") instead of letting exceptions
propagate; keep existing start/end logic (including the overnight case) inside
the guarded block so callers (e.g., any isNowInAutoRestartWindow(...) callers)
never receive exceptions from malformed config.
ℹ️ 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: 0d32d137-1d04-4d79-8317-610f8160c2c4
⛔ Files ignored due to path filters (3)
conf/globalConfig/kvm.xmlis excluded by!**/*.xmlplugin/kvm/pom.xmlis excluded by!**/*.xmltest/src/test/resources/globalConfig/kvm.xmlis excluded by!**/*.xml
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.javaplugin/kvm/src/test/java/org/zstack/kvm/TestKVMAutoRestartWindow.java
✅ Files skipped from review due to trivial changes (1)
- plugin/kvm/src/test/java/org/zstack/kvm/TestKVMAutoRestartWindow.java
74af3db to
e104db5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java (1)
479-490:⚠️ Potential issue | 🟠 Major窗口解析缺少兜底,脏配置可能中断告警处理。
这里直接解析配置字符串,若遇到历史脏值/绕过写入校验的数据会抛运行时异常。建议解析失败时按“窗口外”处理并记录
warn。💡 建议修复
public static boolean isNowInAutoRestartWindow(String configValue, LocalTime now) { if (configValue == null || configValue.trim().isEmpty()) { return true; } - String[] parts = configValue.trim().split("-"); - LocalTime start = LocalTime.parse(parts[0]); - LocalTime end = LocalTime.parse(parts[1]); + String[] parts = configValue.trim().split("-"); + if (parts.length != 2) { + logger.warn(String.format("invalid auto-restart window config[%s], treat as out-of-window", configValue)); + return false; + } + + final LocalTime start; + final LocalTime end; + try { + start = LocalTime.parse(parts[0].trim()); + end = LocalTime.parse(parts[1].trim()); + } catch (Exception e) { + logger.warn(String.format("invalid auto-restart window config[%s], treat as out-of-window", configValue), e); + return false; + } if (start.isBefore(end)) { return !now.isBefore(start) && now.isBefore(end); } return !now.isBefore(start) || now.isBefore(end); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java` around lines 479 - 490, The method KVMHostFactory.isNowInAutoRestartWindow currently parses configValue without validation and can throw runtime exceptions on malformed input; update it to defensively validate/safely parse the window string (check that split yields two parts and catch DateTimeParseException/RuntimeException around LocalTime.parse) and on any parse failure log a warn (using the class logger, e.g. logger.warn) that includes the bad configValue and return "outside window" (false) so alarms/processing are not interrupted; keep existing logic for valid parsed ranges (start/end and wrap-around handling).plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java (1)
152-153:⚠️ Potential issue | 🟠 Major
KVMAGENT_AUTO_RESTART_WINDOW建议显式放开空值校验。当前仅使用裸
@GlobalConfigValidation,存在无法把配置清空回“始终允许”的风险,这与本 PR 的空值语义不一致。💡 建议修复
- `@GlobalConfigValidation` + `@GlobalConfigValidation`(notNull = false, notEmpty = false) public static GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW = new GlobalConfig(CATEGORY, "kvmagent.autorestart.window");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java` around lines 152 - 153, The KVMAGENT_AUTO_RESTART_WINDOW global config is annotated with a bare `@GlobalConfigValidation` which prevents clearing the value back to empty ("always allowed"); update the annotation on KVMAGENT_AUTO_RESTART_WINDOW to explicitly allow empty/null values by adding the validation flag your framework supports (e.g. `@GlobalConfigValidation`(allowEmpty=true) or `@GlobalConfigValidation`(nullable=true) depending on the API) so the config can be cleared to represent the "always allow" semantics; keep the GlobalConfig declaration (KVMAGENT_AUTO_RESTART_WINDOW) unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java`:
- Around line 152-153: The KVMAGENT_AUTO_RESTART_WINDOW global config is
annotated with a bare `@GlobalConfigValidation` which prevents clearing the value
back to empty ("always allowed"); update the annotation on
KVMAGENT_AUTO_RESTART_WINDOW to explicitly allow empty/null values by adding the
validation flag your framework supports (e.g.
`@GlobalConfigValidation`(allowEmpty=true) or
`@GlobalConfigValidation`(nullable=true) depending on the API) so the config can
be cleared to represent the "always allow" semantics; keep the GlobalConfig
declaration (KVMAGENT_AUTO_RESTART_WINDOW) unchanged otherwise.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java`:
- Around line 479-490: The method KVMHostFactory.isNowInAutoRestartWindow
currently parses configValue without validation and can throw runtime exceptions
on malformed input; update it to defensively validate/safely parse the window
string (check that split yields two parts and catch
DateTimeParseException/RuntimeException around LocalTime.parse) and on any parse
failure log a warn (using the class logger, e.g. logger.warn) that includes the
bad configValue and return "outside window" (false) so alarms/processing are not
interrupted; keep existing logic for valid parsed ranges (start/end and
wrap-around handling).
ℹ️ 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: 3d37516e-f286-4707-8148-e15a9d1c65ce
⛔ Files ignored due to path filters (2)
conf/globalConfig/kvm.xmlis excluded by!**/*.xmltest/src/test/resources/globalConfig/kvm.xmlis excluded by!**/*.xml
📒 Files selected for processing (4)
plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.javatest/src/test/groovy/org/zstack/test/unittest/JUnitTestSuite.groovytest/src/test/groovy/org/zstack/test/unittest/utils/KVMAutoRestartWindowCase.java
Add KVMAGENT_AUTO_RESTART_WINDOW (kvm.kvmagent.autorestart.window) so the automatic kvmagent restart triggered by the physical-memory hard-limit alarm only fires within a configured daily time window. Format: HH:MM-HH:MM in 24-hour server local time, e.g. 02:00-04:00. Cross-midnight windows are supported, e.g. 22:00-02:00. Default is 02:00-04:00 so auto-restart only fires during low-traffic hours out of the box; operators who want 24/7 restart can clear the value. The gate is added in processKvmagentPhysicalMemUsageAbnormal() between the existing hard-limit check and the RestartKvmAgentMsg send. The 'no running task on host' check inside restartKvmAgentOnHost is unchanged, so the final guard is: in-window AND over-hardlimit AND no-host-tasks. A GlobalConfigValidatorExtensionPoint is registered in start() to reject malformed values inline, following the pattern used by RESERVED_MEMORY_CAPACITY (no try/catch wrappers). Includes unit tests for the window-membership function covering empty/null, normal window, half-open boundary, cross-midnight, and 00:00-23:59 edge cases. Resolves: ZSTAC-84618 Change-Id: I872bfe96fe30cb83dec21d40157bb315966978ba
e104db5 to
2d6e1b0
Compare
| public static GlobalConfig KVMAGENT_PHYSICAL_MEMORY_USAGE_HARD_LIMIT = new GlobalConfig(CATEGORY, "kvmagent.physicalmemory.usage.hardlimit"); | ||
|
|
||
| @GlobalConfigValidation(notEmpty = false) | ||
| public static GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW = new GlobalConfig(CATEGORY, "kvmagent.autorestart.window"); |
There was a problem hiding this comment.
Comment from jin.ma:
已修复,commit 2d6e1b0。
裸 @GlobalConfigValidation 默认 notEmpty=true,会在 installValidateExtension 之前拒空值,与本字段"空值=任意时间允许"的语义冲突。改为 @GlobalConfigValidation(notEmpty = false),把空值放行给已注册的扩展校验器(KVMHostFactory#start 中对 null/empty 直接 return 通过)。
|
Comment on Comment from jin.ma: 不采纳。
按项目规约:不为内部已校验数据加冗余兜底,避免吞掉真实异常掩盖配置错误。 |
背景
ZStack 已有当 kvmagent 物理内存超过 hardlimit 时自动重启的机制(由
processKvmagentPhysicalMemUsageAbnormal触发)。但目前没有时间约束,可能在业务高峰随时触发重启。本 MR 增加一个时间窗口配置,仅在窗口内允许自动重启。Resolves: ZSTAC-84618
变更
新增全局配置
kvm.kvmagent.autorestart.window:HH:MM-HH:MM24 小时制,服务器本地时间,例如02:00-04:0022:00-02:00触发逻辑
processKvmagentPhysicalMemUsageAbnormal在原有"超过 hardlimit"判断之后、发送RestartKvmAgentMsg之前,新增一个时间窗口关卡:KVMHost.restartKvmAgentOnHost链中的"无运行任务才重启" (noRunningTaskOnHost) 保持不变,所以最终守卫是:(在窗口内) ∧ (超 hardlimit) ∧ (host 无任务)。跳过时打 INFO 日志(含 hostUuid 和窗口字符串),方便运维排查。kvmagent 端 30 分钟一次告警上报,进入窗口后下一波告警自然触发重启,最坏延迟约 30 分钟。
配置校验
KVMHostFactory#start()注册GlobalConfigValidatorExtensionPoint,inline 校验(仿RESERVED_MEMORY_CAPACITY风格,不用 try/catch 包装),拒绝以下非法值:HH:MM-HH:MM影响范围
plugin/kvm),不涉及 kvmagent Python 端RestartKvmAgentMsg(运维主动重启)不受窗口限制测试
新增单元测试
TestKVMAutoRestartWindow(5 个用例,全部通过):00:00-23:59全天减一分钟边界文件变更
conf/globalConfig/kvm.xmltest/src/test/resources/globalConfig/kvm.xmlplugin/kvm/.../KVMGlobalConfig.javaKVMAGENT_AUTO_RESTART_WINDOWplugin/kvm/.../KVMHostFactory.javaisNowInAutoRestartWindowplugin/kvm/pom.xmlplugin/kvm/src/test/.../TestKVMAutoRestartWindow.javaGlobalConfigImpact: 新增
kvm.kvmagent.autorestart.windowsync from gitlab !9738