Skip to content

<feature>[kvm]: add kvmagent auto-restart window config#3863

Open
zstack-robot-2 wants to merge 1 commit into5.5.16from
sync/jin.ma/feat/kvmagent-autorestart-window
Open

<feature>[kvm]: add kvmagent auto-restart window config#3863
zstack-robot-2 wants to merge 1 commit into5.5.16from
sync/jin.ma/feat/kvmagent-autorestart-window

Conversation

@zstack-robot-2
Copy link
Copy Markdown
Collaborator

背景

ZStack 已有当 kvmagent 物理内存超过 hardlimit 时自动重启的机制(由 processKvmagentPhysicalMemUsageAbnormal 触发)。但目前没有时间约束,可能在业务高峰随时触发重启。本 MR 增加一个时间窗口配置,仅在窗口内允许自动重启。

Resolves: ZSTAC-84618

变更

新增全局配置 kvm.kvmagent.autorestart.window

  • 格式:HH:MM-HH:MM 24 小时制,服务器本地时间,例如 02:00-04:00
  • 支持跨午夜窗口,例如 22:00-02:00
  • 默认空字符串 = 任意时间允许(与升级前行为一致)

触发逻辑

processKvmagentPhysicalMemUsageAbnormal 在原有"超过 hardlimit"判断之后、发送 RestartKvmAgentMsg 之前,新增一个时间窗口关卡:

1. memoryUsage > hardlimit?           (已有)
2. 当前时间在窗口内?                   (新增)
3. 发送 RestartKvmAgentMsg            (已有)

KVMHost.restartKvmAgentOnHost 链中的"无运行任务才重启" (noRunningTaskOnHost) 保持不变,所以最终守卫是:(在窗口内) ∧ (超 hardlimit) ∧ (host 无任务)

跳过时打 INFO 日志(含 hostUuid 和窗口字符串),方便运维排查。kvmagent 端 30 分钟一次告警上报,进入窗口后下一波告警自然触发重启,最坏延迟约 30 分钟。

配置校验

KVMHostFactory#start() 注册 GlobalConfigValidatorExtensionPoint,inline 校验(仿 RESERVED_MEMORY_CAPACITY 风格,不用 try/catch 包装),拒绝以下非法值:

  • 格式不是 HH:MM-HH:MM
  • 小时 > 23 或分钟 > 59
  • 起止时间相同(零长度窗口)

影响范围

  • 仅 Java 改动(plugin/kvm),不涉及 kvmagent Python 端
  • 仅影响告警触发的自动重启路径;显式 RestartKvmAgentMsg(运维主动重启)不受窗口限制
  • 无 DB schema 改动,无 Flyway 迁移
  • 默认值空 → 老环境零感知升级

测试

新增单元测试 TestKVMAutoRestartWindow(5 个用例,全部通过):

  • 空/null/空白值 → 任意时间允许
  • 普通窗口成员判断 + 起点包含 / 终点排除
  • 跨午夜窗口(前半段、后半段、外部、边界)
  • 00:00-23:59 全天减一分钟边界
mvn test -pl plugin/kvm -am -Dtest=TestKVMAutoRestartWindow
Tests run: 5, Failures: 0, Errors: 0, Skipped: 0

文件变更

文件 变更
conf/globalConfig/kvm.xml 新增配置项 + 默认空
test/src/test/resources/globalConfig/kvm.xml 同上(测试镜像)
plugin/kvm/.../KVMGlobalConfig.java 新增常量 KVMAGENT_AUTO_RESTART_WINDOW
plugin/kvm/.../KVMHostFactory.java gate + validator + isNowInAutoRestartWindow
plugin/kvm/pom.xml 加 junit test scope 依赖
plugin/kvm/src/test/.../TestKVMAutoRestartWindow.java 新增单元测试

GlobalConfigImpact: 新增 kvm.kvmagent.autorestart.window

sync from gitlab !9738

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent 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: 76c10253-be44-4b22-b19f-cac028bd8296

📥 Commits

Reviewing files that changed from the base of the PR and between e104db5 and 2d6e1b0.

⛔ Files ignored due to path filters (2)
  • conf/globalConfig/kvm.xml is excluded by !**/*.xml
  • test/src/test/resources/globalConfig/kvm.xml is excluded by !**/*.xml
📒 Files selected for processing (4)
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java
  • test/src/test/groovy/org/zstack/test/unittest/JUnitTestSuite.groovy
  • test/src/test/groovy/org/zstack/test/unittest/utils/KVMAutoRestartWindowCase.java
🚧 Files skipped from review as they are similar to previous changes (4)
  • test/src/test/groovy/org/zstack/test/unittest/JUnitTestSuite.groovy
  • test/src/test/groovy/org/zstack/test/unittest/utils/KVMAutoRestartWindowCase.java
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java

Walkthrough

新增 KVM 代理自动重启时间窗口配置与校验逻辑:增加全局配置项、在内存硬限告警触发的重启路径中加入时间窗检查并阻止不在允许窗口内的重启,同时新增对应单元测试覆盖边界和跨午夜情况。

Changes

Cohort / File(s) Summary
全局配置定义
plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java
新增导出全局配置 KVMAGENT_AUTO_RESTART_WINDOW,键名 kvmagent.autorestart.window,允许为空并带验证注解。
时间窗口检查与重启控制
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java
新增 isNowInAutoRestartWindow(String, LocalTime) 方法解析 HH:MM-HH:MM 格式(支持跨午夜、包含开始不含结束);在物理内存硬限告警触发的重启路径读取该全局配置并在不在允许窗口时中止重启;增加对配置格式与边界的校验逻辑。
测试套件与用例
test/src/test/groovy/org/zstack/test/unittest/JUnitTestSuite.groovy, test/src/test/groovy/org/zstack/test/unittest/utils/KVMAutoRestartWindowCase.java
在 JUnit 测试套件中注册 KVMAutoRestartWindowCase 并新增针对 isNowInAutoRestartWindow() 的单元测试,覆盖空值、同日窗口、跨午夜窗口与边界排除等场景。

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

兔子看时间,窗开关着重启心欢,
午夜环绕去,规则把关不慌忙,
测试稳如春,配置生效梦正甜。 🐰⏰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed PR标题遵循[scope]: 格式,内容准确反映了主要变更(新增kvmagent自动重启窗口配置),长度55字符在72字符以内。
Description check ✅ Passed PR描述详细说明了功能背景、变更内容、触发逻辑、配置校验、影响范围和测试情况,与变更内容高度相关且信息充分。
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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/jin.ma/feat/kvmagent-autorestart-window

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

Copy link
Copy Markdown

@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: 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

📥 Commits

Reviewing files that changed from the base of the PR and between e6cdf84 and c7f332b.

⛔ Files ignored due to path filters (3)
  • conf/globalConfig/kvm.xml is excluded by !**/*.xml
  • plugin/kvm/pom.xml is excluded by !**/*.xml
  • test/src/test/resources/globalConfig/kvm.xml is excluded by !**/*.xml
📒 Files selected for processing (3)
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java
  • plugin/kvm/src/test/java/org/zstack/kvm/TestKVMAutoRestartWindow.java

Comment on lines +152 to +153
@GlobalConfigValidation
public static GlobalConfig KVMAGENT_AUTO_RESTART_WINDOW = new GlobalConfig(CATEGORY, "kvmagent.autorestart.window");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

空值语义与当前注解默认校验存在冲突。

这里使用裸 @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".

Comment thread plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java Outdated
@MatheMatrix MatheMatrix force-pushed the sync/jin.ma/feat/kvmagent-autorestart-window branch 2 times, most recently from 07274f2 to 74af3db Compare April 28, 2026 02:48
Copy link
Copy Markdown

@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.

♻️ 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 -C3

As 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

📥 Commits

Reviewing files that changed from the base of the PR and between c7f332b and 74af3db.

⛔ Files ignored due to path filters (3)
  • conf/globalConfig/kvm.xml is excluded by !**/*.xml
  • plugin/kvm/pom.xml is excluded by !**/*.xml
  • test/src/test/resources/globalConfig/kvm.xml is excluded by !**/*.xml
📒 Files selected for processing (3)
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java
  • plugin/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

@MatheMatrix MatheMatrix force-pushed the sync/jin.ma/feat/kvmagent-autorestart-window branch from 74af3db to e104db5 Compare April 28, 2026 03:01
Copy link
Copy Markdown

@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.

♻️ 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

📥 Commits

Reviewing files that changed from the base of the PR and between 74af3db and e104db5.

⛔ Files ignored due to path filters (2)
  • conf/globalConfig/kvm.xml is excluded by !**/*.xml
  • test/src/test/resources/globalConfig/kvm.xml is excluded by !**/*.xml
📒 Files selected for processing (4)
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java
  • plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java
  • test/src/test/groovy/org/zstack/test/unittest/JUnitTestSuite.groovy
  • test/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
@MatheMatrix MatheMatrix force-pushed the sync/jin.ma/feat/kvmagent-autorestart-window branch from e104db5 to 2d6e1b0 Compare April 28, 2026 03:06
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");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment from jin.ma:

已修复,commit 2d6e1b0

@GlobalConfigValidation 默认 notEmpty=true,会在 installValidateExtension 之前拒空值,与本字段"空值=任意时间允许"的语义冲突。改为 @GlobalConfigValidation(notEmpty = false),把空值放行给已注册的扩展校验器(KVMHostFactory#start 中对 null/empty 直接 return 通过)。

@zstack-robot-1
Copy link
Copy Markdown
Collaborator

Comment on plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java:

Comment from jin.ma:

不采纳。

isNowInAutoRestartWindow 是读路径,输入来自 GlobalConfig,写入时已经过 installValidateExtension 校验(格式 + 时分范围 + 非零长度)。读路径再加 try/catch 属于"对不会发生的场景做防御"——本字段是 5.5.16 新增、有非空默认值、无 flyway 迁移路径,不存在历史脏数据;绕过 API 直接改库本就是非支持操作。

按项目规约:不为内部已校验数据加冗余兜底,避免吞掉真实异常掩盖配置错误。

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.

4 participants