<feature>[kvm]: periodic check and sync for VM host files#3693
Closed
zstack-robot-2 wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Closed
<feature>[kvm]: periodic check and sync for VM host files#3693zstack-robot-2 wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
zstack-robot-2 wants to merge 1 commit intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Conversation
Walkthrough为 VmHostFile 添加两时间戳字段（changeDate、lastSyncDate），新增主机文件变更事件与数据契约，调整周期性按组检查/强制同步逻辑并携带 syncReason，修改 KVM 默认检查间隔及相关持久化与选择行为。 Changes
Sequence DiagramsequenceDiagram
participant HostAgent as Host Agent
participant EventBus as Event Bus
participant Tracker as VmHostFileTracker
participant DB as Database
Note over HostAgent,DB: 文件变更报告流程
HostAgent->>EventBus: 发布 VM_HOST_FILE_CHANGED (hostUuid, vmUuid, types)
EventBus->>Tracker: 触发变更处理
Tracker->>DB: UPDATE VmHostFileVO SET changeDate=NOW WHERE hostUuid & vmUuid & type
DB-->>Tracker: 返回更新结果
Note over HostAgent,DB: 周期检查与同步流程
Tracker->>DB: 查询分组记录（含 changeDate、lastSyncDate）
DB-->>Tracker: 返回组内记录
alt 发现 changeDate 非空（脏检查）
Tracker->>HostAgent: 发送 SyncVmHostFilesFromHostMsg (syncReason=PeriodicDirtyCheck)
HostAgent-->>Tracker: 同步结果
Tracker->>DB: 清除组内 changeDate 并设置 lastSyncDate=NOW
DB-->>Tracker: 更新完成
else 未发现脏数据但超过阈值（强制）
Tracker->>HostAgent: 发送 SyncVmHostFilesFromHostMsg (syncReason=PeriodicForceSync)
HostAgent-->>Tracker: 同步结果
Tracker->>DB: 设置组内 lastSyncDate=NOW
DB-->>Tracker: 更新完成
else 无需同步
Tracker->>Tracker: 跳过该组
end
Estimated code review effort🎯 4 (复杂) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java (1)
294-314:⚠️ Potential issue | 🟠 Major
syncToHostFiles中对现有文件的changeDate设置可能有误。对于现有文件（第 298 行），同步时设置了
changeDate = now，但未设置lastSyncDate。对于新文件（第 311 行），设置了lastSyncDate = now，但未设置changeDate。根据 PR 描述的语义：
changeDate：文件被报告变更的时间（应由VM_HOST_FILE_CHANGED事件设置，同步后清除）lastSyncDate：最后一次成功同步的时间当前逻辑对于现有文件：同步完成后应该是
changeDate = null（表示不再脏），lastSyncDate = now（同步刚完成）。建议修正：
🐛 建议修复
if (fileExists) { SQL.New(VmHostFileVO.class) .eq(VmHostFileVO_.uuid, file.getUuid()) - .set(VmHostFileVO_.changeDate, now) + .set(VmHostFileVO_.changeDate, null) + .set(VmHostFileVO_.lastSyncDate, now) .set(VmHostFileVO_.lastOpDate, now) .set(VmHostFileVO_.lastSyncReason, msg.getSyncReason()) .update(); }🤖 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/efi/KvmSecureBootManager.java` around lines 294 - 314, The current syncToHostFiles logic sets changeDate on existing files and omits lastSyncDate, and when creating new VmHostFileVO it sets lastSyncDate but not changeDate; update the existing-file update (SQL.New(...).set(...)) to set VmHostFileVO_.changeDate to null and set VmHostFileVO_.lastSyncDate to now (in addition to lastOpDate/changeDate handling), and when constructing the new VmHostFileVO ensure file.setChangeDate(null) and file.setLastSyncDate(now) so new entries and updates reflect that changeDate is cleared after successful sync and lastSyncDate records the sync time.
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.java (1)
127-138:__example__()方法未包含新字段的示例值。
changeDate和lastSyncDate字段未在__example__()方法中设置示例值，可能影响 API 文档的完整性。建议添加示例值以便 API 文档展示完整的字段结构。📝 建议补充示例值
public static VmHostFileInventory __example__() { VmHostFileInventory ref = new VmHostFileInventory(); ref.setUuid(DocUtils.createFixedUuid(VmHostFileVO.class)); ref.setVmInstanceUuid(DocUtils.createFixedUuid(VmInstanceVO.class)); ref.setHostUuid(DocUtils.createFixedUuid(HostVO.class)); ref.setType(VmHostFileType.TpmState.toString()); ref.setPath("/var/lib/libvirt/swtpm/" + ref.getHostUuid() + "/"); ref.setLastSyncReason("on libvirt shutdown event"); + ref.setChangeDate(null); // null when file is not dirty + ref.setLastSyncDate(DocUtils.timestamp()); ref.setCreateDate(DocUtils.timestamp()); ref.setLastOpDate(DocUtils.timestamp()); return ref; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.java` around lines 127 - 138, 在 VmHostFileInventory.__example__() 中补充缺失的示例字段：为 changeDate 和 lastSyncDate 赋示例时间值（例如使用 DocUtils.timestamp()）以确保示例完整；在方法体内在设置 createDate/lastOpDate 的相同位置调用 ref.setChangeDate(...) 和 ref.setLastSyncDate(...)，保持格式与现有字段一致并返回 ref。plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java (1)
178-178: 高频周期日志建议降级为debugLine 178 每 15 秒打印一次
info，在多管理节点下会产生明显日志噪音。建议改为debug，仅在异常或统计汇总时保留info。🤖 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/vmfiles/VmHostFileTracker.java` at line 178, The periodic start-up log "VmHostFileTracker: starting periodic check of VM host files" in class VmHostFileTracker is emitted every 15s and should be downgraded from logger.info to logger.debug to avoid high-frequency noise; locate the logger.info call with that exact message in VmHostFileTracker and change it to logger.debug, leaving any remaining info-level logs for errors or aggregated/statistical summaries only.
🤖 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/vmfiles/VmHostFileTracker.java`:
- Around line 136-142: The loop over types calls VmHostFileType.valueOf(type)
which will throw on null/unknown values and abort processing; update the for
(String type : types) loop in VmHostFileTracker to validate and/or safely parse
each type (e.g., skip null/blank and use a try/catch or EnumUtils to map to
VmHostFileType) before calling
SQL.New(...).eq(...).eq(...).eq(VmHostFileVO_.type,
VmHostFileType.valueOf(type)).set(...).update(); ensure invalid entries are
logged and skipped so subsequent types continue to be processed.
- Around line 294-300: The current code unconditionally clears
VmHostFileVO.changeDate after a successful sync which can wipe out concurrent
dirty markers; modify the update to only clear changeDate if it still equals the
value that was present when the sync started: capture the file.getChangeDate()
(e.g., preSyncChangeDate) for each VmHostFileVO in the group and change the
SQL.New(...) update to include a conditional equality check on
VmHostFileVO_.changeDate == preSyncChangeDate (while still setting lastSyncDate
to syncTime), so only pre-existing dirty flags are cleared and concurrent
changes are preserved.
---
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java`:
- Around line 294-314: The current syncToHostFiles logic sets changeDate on
existing files and omits lastSyncDate, and when creating new VmHostFileVO it
sets lastSyncDate but not changeDate; update the existing-file update
(SQL.New(...).set(...)) to set VmHostFileVO_.changeDate to null and set
VmHostFileVO_.lastSyncDate to now (in addition to lastOpDate/changeDate
handling), and when constructing the new VmHostFileVO ensure
file.setChangeDate(null) and file.setLastSyncDate(now) so new entries and
updates reflect that changeDate is cleared after successful sync and
lastSyncDate records the sync time.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.java`:
- Around line 127-138: 在 VmHostFileInventory.__example__() 中补充缺失的示例字段：为
changeDate 和 lastSyncDate 赋示例时间值（例如使用 DocUtils.timestamp()）以确保示例完整；在方法体内在设置
createDate/lastOpDate 的相同位置调用 ref.setChangeDate(...) 和
ref.setLastSyncDate(...)，保持格式与现有字段一致并返回 ref。
In `@plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java`:
- Line 178: The periodic start-up log "VmHostFileTracker: starting periodic
check of VM host files" in class VmHostFileTracker is emitted every 15s and
should be downgraded from logger.info to logger.debug to avoid high-frequency
noise; locate the logger.info call with that exact message in VmHostFileTracker
and change it to logger.debug, leaving any remaining info-level logs for errors
or aggregated/statistical summaries only.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 30bf3749-2669-4183-9393-b1799f7dec29
📒 Files selected for processing (9)
conf/db/zsv/V5.0.0__schema.sqlheader/src/main/java/org/zstack/header/vm/VmCanonicalEvents.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileSyncReason.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileVO.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileVO_.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaplugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java
Comment on lines
+294
to
+300
| Timestamp syncTime = new Timestamp(timeHelper.getCurrentTimeMillis()); | ||
| for (VmHostFileVO file : group) { | ||
| SQL.New(VmHostFileVO.class) | ||
| .eq(VmHostFileVO_.uuid, file.getUuid()) | ||
| .set(VmHostFileVO_.changeDate, null) | ||
| .set(VmHostFileVO_.lastSyncDate, syncTime) | ||
| .update(); |
There was a problem hiding this comment.
同步成功后无条件清空 changeDate，会丢失并发变更标记
Line 298 在异步回调里直接把 changeDate 置空；如果同步进行期间又收到一次 VM_HOST_FILE_CHANGED 事件，新脏标记会被覆盖，导致后续周期检查漏同步。建议仅清除“同步开始前”的脏标记。
可选修复示例
SyncVmHostFilesFromHostMsg syncMsg = new SyncVmHostFilesFromHostMsg();
@@
bus.makeLocalServiceId(syncMsg, VmInstanceConstant.SECURE_BOOT_SERVICE_ID);
+ Timestamp syncStartTime = new Timestamp(timeHelper.getCurrentTimeMillis());
bus.send(syncMsg, new CloudBusCallBack(whileCompletion) {
`@Override`
public void run(MessageReply reply) {
@@
} else {
Timestamp syncTime = new Timestamp(timeHelper.getCurrentTimeMillis());
for (VmHostFileVO file : group) {
SQL.New(VmHostFileVO.class)
.eq(VmHostFileVO_.uuid, file.getUuid())
- .set(VmHostFileVO_.changeDate, null)
.set(VmHostFileVO_.lastSyncDate, syncTime)
.update();
+
+ SQL.New(VmHostFileVO.class)
+ .eq(VmHostFileVO_.uuid, file.getUuid())
+ .notNull(VmHostFileVO_.changeDate)
+ .lte(VmHostFileVO_.changeDate, syncStartTime)
+ .set(VmHostFileVO_.changeDate, null)
+ .update();
}
}🤖 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/vmfiles/VmHostFileTracker.java`
around lines 294 - 300, The current code unconditionally clears
VmHostFileVO.changeDate after a successful sync which can wipe out concurrent
dirty markers; modify the update to only clear changeDate if it still equals the
value that was present when the sync started: capture the file.getChangeDate()
(e.g., preSyncChangeDate) for each VmHostFileVO in the group and change the
SQL.New(...) update to include a conditional equality check on
VmHostFileVO_.changeDate == preSyncChangeDate (while still setting lastSyncDate
to syncTime), so only pre-existing dirty flags are cleared and concurrent
changes are preserved.
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap@@2
branch
2 times, most recently
from
d27b9d6 to
207d0b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@header/src/main/java/org/zstack/header/vm/additions/VmHostFileVO.java`:
- Around line 45-48: The changeDate dirty-flag is being unconditionally cleared
in VmHostFileTracker (the sync callback that writes back sync results), which
can race with concurrent event-thread updates and lose new dirty marks; modify
the write-back logic in VmHostFileTracker to perform a CAS-style conditional
update: first read the current VmHostFileVO.changeDate value (capturing null as
a distinct value), then issue an update that sets changeDate = NULL only if the
row's changeDate still equals the previously read value (use the DB/update API
that can compare-to-value or a WHERE clause comparing changeDate, and when
comparing to NULL use an isNull() check), so changeDate is cleared only when
unchanged since read.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a7edbf8e-be95-4bcf-8523-45eb409d0d52
📒 Files selected for processing (10)
conf/db/zsv/V5.0.0__schema.sqlheader/src/main/java/org/zstack/header/vm/VmCanonicalEvents.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileSyncReason.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileVO.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileVO_.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaplugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.javasdk/src/main/java/org/zstack/sdk/vm/entity/VmHostFileInventory.java
✅ Files skipped from review due to trivial changes (4)
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileSyncReason.java
- plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java
- plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileVO_.java
🚧 Files skipped from review as they are similar to previous changes (3)
- conf/db/zsv/V5.0.0__schema.sql
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.java
- plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java
Comment on lines
+45
to
+48
| private Timestamp changeDate; | ||
| @Column | ||
| private Timestamp lastSyncDate; | ||
| @Column |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 目的：确认是否存在“无条件清空 changeDate”的更新语句
# 期望：如果看到 set(changeDate, null) 且附近没有基于旧值/IS NULL 的条件，则可复现该风险
rg -n -C4 'set\(VmHostFileVO_\.changeDate,\s*null\)|eq\(VmHostFileVO_\.changeDate|isNull\(VmHostFileVO_\.changeDate' \
plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.javaRepository: MatheMatrix/zstack
Length of output: 629
🏁 Script executed:
#!/bin/bash
# 1. 查看 VmHostFileVO 中 changeDate 的定义和注释
echo "=== VmHostFileVO changeDate 定义 ==="
rg -n -B2 -A2 'changeDate' header/src/main/java/org/zstack/header/vm/additions/VmHostFileVO.java
# 2. 查看 VmHostFileTracker 的 135-152 行（置脏逻辑）
echo -e "\n=== VmHostFileTracker 135-152 行（置脏逻辑）==="
sed -n '130,160p' plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java
# 3. 查看 changeDate 设置的其他地方
echo -e "\n=== changeDate 在 VmHostFileTracker 中的所有出现 ==="
rg -n 'changeDate' plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.javaRepository: MatheMatrix/zstack
Length of output: 2434
🏁 Script executed:
#!/bin/bash
# 1. 查看同步检查逻辑：如何使用 changeDate 判断是否需要同步
echo "=== changeDate 在检查或过滤中的使用 ==="
rg -n 'changeDate|isNotNull|getChangeDate' plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java
# 2. 查看同步任务的主方法和事务结构
echo -e "\n=== 同步任务主方法（查找 PeriodicTask 或主查询逻辑）==="
sed -n '160,300p' plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java
# 3. 查看是否有数据库事务或锁
echo -e "\n=== 检查同步操作是否在事务中 ==="
rg -n -B3 'SQL.New\(VmHostFileVO' plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java | head -40Repository: MatheMatrix/zstack
Length of output: 6540
🏁 Script executed:
#!/bin/bash
# 1. 查看 forceSyncThresholdMs 的计算和默认值
echo "=== VM_HOST_FILE_SYNC_INTERVAL 配置 ==="
rg -n 'VM_HOST_FILE_SYNC_INTERVAL' plugin/kvm/src/main/java/org/zstack/kvm/ -A2 -B2 | head -50
# 2. 确认 changeDate = null 时是否需要特殊的 IS NULL 处理
echo -e "\n=== 数据库中 changeDate 字段定义 ==="
rg -n 'changeDate' plugin/kvm/src/main/resources/db -r -A1 -B1
# 3. 查看是否有其他地方使用了类似的 CAS 模式进行字段更新
echo -e "\n=== 查看库中 CAS 更新的例子 ==="
rg -n '\.eq.*\(.*\).*\.eq' plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/ | head -10Repository: MatheMatrix/zstack
Length of output: 2749
🏁 Script executed:
#!/bin/bash
# 1. 查看 changeDate 是否在其他地方被使用
echo "=== changeDate 在整个 KVM 模块中的使用情况 ==="
rg -n 'changeDate' plugin/kvm/src/main/java/org/zstack/kvm/ -g '*.java' | grep -v 'VmHostFileTracker\|VmHostFileVO'
# 2. 查看是否有类似的 CAS 更新模式
echo -e "\n=== 查看 ZStack 中条件更新（WHERE子句带多个条件）的例子 ==="
rg -n '\.eq.*\.eq.*\.set' plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/ | head -5
# 3. 确认脏标记是否有其他初始化或检查逻辑
echo -e "\n=== 新增文件时 changeDate 的初始化 ==="
rg -n 'new VmHostFileVO\|setChangeDate' plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/ -B2 -A2Repository: MatheMatrix/zstack
Length of output: 170
🏁 Script executed:
#!/bin/bash
# 1. 确认 SQL 框架如何处理 null 比较
echo "=== 查看 SQL 框架对 null 值的处理 ==="
rg -n 'isNull|IS NULL' plugin/kvm/src/main/java/org/zstack/kvm/ -A1 | head -20
# 2. 查看是否有 isNull 或类似的方法
echo -e "\n=== 在 VmHostFileTracker 或相邻文件中查找 isNull 用法 ==="
fd -t f "*.java" plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/ | xargs rg -l 'isNull' 2>/dev/null
# 3. 快速验证数据库迁移脚本中的字段类型
echo -e "\n=== 查找数据库架构定义或迁移脚本 ==="
find . -name "*schema*" -o -name "*upgrade*" | grep -i sql | head -10Repository: MatheMatrix/zstack
Length of output: 2982
🏁 Script executed:
#!/bin/bash
# 查看 isNull 的具体用法
echo "=== isNull 的具体用法 ==="
rg -n 'isNull' core/src/main/java/org/zstack/core/db/UpdateQueryImpl.java -B2 -A2 | head -30
# 查看是否有 IS NULL 的示例
echo -e "\n=== IS NULL 在代码中的出现 ==="
rg -n 'IS NULL' plugin/kvm/src/main/java/org/zstack/kvm/ -B1 -A1 | head -20
# 查看 VmHostFileVO 的数据库迁移脚本
echo -e "\n=== 查找 VmHostFile 表的创建脚本 ==="
rg -n 'CREATE TABLE.*VmHostFile|vm_host_file' conf/db/ -A5 | head -30Repository: MatheMatrix/zstack
Length of output: 1412
🏁 Script executed:
#!/bin/bash
# 查看完整的 VmHostFileVO 表定义，包括 changeDate 字段
echo "=== VmHostFileVO 完整表定义 ==="
sed -n '12,40p' conf/db/zsv/V5.0.0__schema.sql
# 查看是否有新增 changeDate 字段的迁移脚本
echo -e "\n=== 查找新增 changeDate 的迁移脚本 ==="
rg -n 'changeDate|lastSyncDate' conf/db/ -A2 -B2 | head -50Repository: MatheMatrix/zstack
Length of output: 3032
避免并发覆盖 changeDate 脏标记，防止安全文件同步延迟。
changeDate 被用于脏检测机制（null = 无变更，非 null = 有变更），但当前同步器在成功回写时存在无条件清空（plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java:298），与事件线程置脏并发时会覆盖新变更。这导致脏标记丢失，虽然系统有 25 分钟强制同步补救，但对 TPM/Secure Boot 等安全关键文件延迟不可接受。
建议改为条件更新（CAS）：仅当 changeDate 仍是读取时的值时才清空。由于字段支持 NULL，需使用 isNull() 方法处理：
🔧 建议修复（VmHostFileTracker 同步回写处）
for (VmHostFileVO file : group) {
- SQL.New(VmHostFileVO.class)
- .eq(VmHostFileVO_.uuid, file.getUuid())
- .set(VmHostFileVO_.changeDate, null)
- .set(VmHostFileVO_.lastSyncDate, syncTime)
- .update();
+ // 条件更新：仅当 changeDate 仍未被并发置脏时才清空
+ UpdateQuery query = SQL.New(VmHostFileVO.class)
+ .eq(VmHostFileVO_.uuid, file.getUuid());
+
+ if (file.getChangeDate() == null) {
+ query.isNull(VmHostFileVO_.changeDate);
+ } else {
+ query.eq(VmHostFileVO_.changeDate, file.getChangeDate());
+ }
+
+ query.set(VmHostFileVO_.changeDate, null)
+ .set(VmHostFileVO_.lastSyncDate, syncTime)
+ .update();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@header/src/main/java/org/zstack/header/vm/additions/VmHostFileVO.java` around
lines 45 - 48, The changeDate dirty-flag is being unconditionally cleared in
VmHostFileTracker (the sync callback that writes back sync results), which can
race with concurrent event-thread updates and lose new dirty marks; modify the
write-back logic in VmHostFileTracker to perform a CAS-style conditional update:
first read the current VmHostFileVO.changeDate value (capturing null as a
distinct value), then issue an update that sets changeDate = NULL only if the
row's changeDate still equals the previously read value (use the DB/update API
that can compare-to-value or a WHERE clause comparing changeDate, and when
comparing to NULL use an isNull() check), so changeDate is cleared only when
unchanged since read.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java (1)
249-279:⚠️ Potential issue | 🟠 Major把 dirty/force-sync 过滤下推到查询层。
现在是先把所有运行态 VM 的
VmHostFileVO全量查出并分组，再在内存里判断changeDate/lastSyncDate。默认 15 秒周期下，这会把 DB 扫描和对象分配放大很多倍，即使没有任何脏文件也一样。建议在初始查询里直接过滤changeDate != null、lastSyncDate is null或lastSyncDate < forceSyncBefore，只拉取需要处理的 group。🤖 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/vmfiles/VmHostFileTracker.java` around lines 249 - 279, The current logic pulls all VmHostFileVO for running VMs into memory and then filters by changeDate/lastSyncDate, causing excessive DB scans and allocations; change the initial query that builds groups to only return rows matching "changeDate IS NOT NULL OR lastSyncDate IS NULL OR lastSyncDate < :forceSyncBefore" (where forceSyncBefore = new Timestamp(now - forceSyncThresholdMs)), so grouping (the variable groups used by the While<>), VmHostFileVO objects, and subsequent checks in the While step only iterate over candidates that actually need sync; keep the existing checks around destinationMaker.isManagedByUs(vmUuid) and the VmHostFileSyncReason selection but remove the in-memory filter that tests changeDate/lastSyncDate since it will be handled by the query.
♻️ Duplicate comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java (1)
282-296:⚠️ Potential issue | 🔴 Critical同步请求缺少 pre-sync 标记，仍然会丢并发变更。
这条链路现在依赖
changeDate做 dirty-check，但这里发送消息时没有把“同步开始前”的标记带下去。下游plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java:294-302成功后仍会直接把changeDate清空；如果 host 文件在本次读取之后、清理之前再次变更，新脏标记会被覆盖，下一轮周期检查就可能漏同步。建议把每个文件的 pre-syncchangeDate或统一的syncStartTime传给下游，并在清理时做条件更新。🤖 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/vmfiles/VmHostFileTracker.java` around lines 282 - 296, The SyncVmHostFilesFromHostMsg sent from VmHostFileTracker currently omits any pre-sync marker, so capture and include a pre-sync timestamp (e.g., syncStartTime) or each file's preSync changeDate when building SyncVmHostFilesFromHostMsg (set a new field like syncStartTime on SyncVmHostFilesFromHostMsg and set it alongside setNvRamPath/setTpmStateFolder for the files in the group) and pass it downstream; then update KvmSecureBootManager (the code that clears changeDate) to only clear changeDate conditionally using that provided pre-sync marker (i.e., only clear if DB changeDate equals the pre-sync value or is <= syncStartTime) to avoid losing concurrent updates.
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java (1)
186-188: 把这条周期日志降到debug。默认周期已经变成 15 秒，这里会在每个管理节点持续打
info，即使本轮没有任何候选文件也是如此。建议只在实际进入同步时保留info，空轮询改成debug。🤖 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/vmfiles/VmHostFileTracker.java` around lines 186 - 188, The periodic startup log in VmHostFileTracker.run() currently logs at info for every 15s tick; change logger.info("VmHostFileTracker: starting periodic check of VM host files") to logger.debug(...) so empty polls don't flood info logs, and move or add an info-level log inside syncVmHostFiles() (or where actual work begins) to record when a real sync starts; update references in VmHostFileTracker.run() and syncVmHostFiles() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java`:
- Around line 249-279: The current logic pulls all VmHostFileVO for running VMs
into memory and then filters by changeDate/lastSyncDate, causing excessive DB
scans and allocations; change the initial query that builds groups to only
return rows matching "changeDate IS NOT NULL OR lastSyncDate IS NULL OR
lastSyncDate < :forceSyncBefore" (where forceSyncBefore = new Timestamp(now -
forceSyncThresholdMs)), so grouping (the variable groups used by the While<>),
VmHostFileVO objects, and subsequent checks in the While step only iterate over
candidates that actually need sync; keep the existing checks around
destinationMaker.isManagedByUs(vmUuid) and the VmHostFileSyncReason selection
but remove the in-memory filter that tests changeDate/lastSyncDate since it will
be handled by the query.
---
Duplicate comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java`:
- Around line 282-296: The SyncVmHostFilesFromHostMsg sent from
VmHostFileTracker currently omits any pre-sync marker, so capture and include a
pre-sync timestamp (e.g., syncStartTime) or each file's preSync changeDate when
building SyncVmHostFilesFromHostMsg (set a new field like syncStartTime on
SyncVmHostFilesFromHostMsg and set it alongside setNvRamPath/setTpmStateFolder
for the files in the group) and pass it downstream; then update
KvmSecureBootManager (the code that clears changeDate) to only clear changeDate
conditionally using that provided pre-sync marker (i.e., only clear if DB
changeDate equals the pre-sync value or is <= syncStartTime) to avoid losing
concurrent updates.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java`:
- Around line 186-188: The periodic startup log in VmHostFileTracker.run()
currently logs at info for every 15s tick; change
logger.info("VmHostFileTracker: starting periodic check of VM host files") to
logger.debug(...) so empty polls don't flood info logs, and move or add an
info-level log inside syncVmHostFiles() (or where actual work begins) to record
when a real sync starts; update references in VmHostFileTracker.run() and
syncVmHostFiles() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0ca75f24-2ef7-4b4e-aba2-092540745e64
📒 Files selected for processing (10)
conf/db/zsv/V5.0.0__schema.sqlheader/src/main/java/org/zstack/header/vm/VmCanonicalEvents.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileSyncReason.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileVO.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileVO_.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaplugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.javasdk/src/main/java/org/zstack/sdk/vm/entity/VmHostFileInventory.java
✅ Files skipped from review due to trivial changes (4)
- conf/db/zsv/V5.0.0__schema.sql
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileVO_.java
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileSyncReason.java
- sdk/src/main/java/org/zstack/sdk/vm/entity/VmHostFileInventory.java
🚧 Files skipped from review as they are similar to previous changes (4)
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.java
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileVO.java
- header/src/main/java/org/zstack/header/vm/VmCanonicalEvents.java
- plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java
Implement periodic VM host file checking with dirty detection and force sync mechanism. This improves the sync interval from 30 minutes to 15 seconds, allowing faster detection and sync of file changes. Changes: - Add changeDate and lastSyncDate fields to VmHostFileVO for tracking - Add PeriodicDirtyCheck and PeriodicForceSync sync reasons - Add VM_HOST_FILE_CHANGED canonical event for file change reporting - Update VmHostFileTracker to check files periodically and sync when changed or force sync after long period without changes - Update vm.host.file.sync.interval default from 1800 to 15 seconds - Update config description from 'syncing' to 'checking' - Implement event listener to mark files as changed when reported Resolves: ZSV-11371 Related: ZSV-11310 Change-Id: I6f62727061686b646f617463737672686d667578
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap@@2
branch
from
207d0b7 to
195a8e0
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java (1)
264-292:⚠️ Potential issue | 🟠 Major不要让历史
VmHostFileVO参与本轮 current-file 判定。这里直接对整个
group计算hasChanged/oldestLastSync，但syncMsg最终每种类型只会带一个 path。只要组里残留一条同(vmUuid, hostUuid, type)的历史记录且它的changeDate != null或lastSyncDate == null/过旧，这个 VM 就会反复命中PeriodicDirtyCheck/PeriodicForceSync，而这一轮同步又未必会更新到那条旧记录。建议先按type收敛到 current/latest 记录，再决定syncReason并组装消息。💡 一个可行的收敛方式
+ Map<VmHostFileType, VmHostFileVO> latestByType = new HashMap<>(); + for (VmHostFileVO file : group) { + VmHostFileVO current = latestByType.get(file.getType()); + if (current == null || current.getLastOpDate().before(file.getLastOpDate())) { + latestByType.put(file.getType(), file); + } + } + Collection<VmHostFileVO> currentFiles = latestByType.values(); - - boolean hasChanged = group.stream().anyMatch(f -> f.getChangeDate() != null); + boolean hasChanged = currentFiles.stream().anyMatch(f -> f.getChangeDate() != null); ... - Timestamp oldestLastSync = group.stream() + Timestamp oldestLastSync = currentFiles.stream() ... - for (VmHostFileVO file : group) { + for (VmHostFileVO file : currentFiles) {前面的事件置脏逻辑也要对齐到同一条 current/latest 记录，否则旧行还是会把这组持续判成 dirty。
🤖 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/vmfiles/VmHostFileTracker.java` around lines 264 - 292, 当前代码在 VmHostFileTracker 中对整个 group 直接计算 hasChanged/oldestLastSync 导致历史记录（旧的 VmHostFileVO）把当前文件组误判为需要同步；请先基于 VmHostFileType 收敛到每种 type 的“current/latest” VmHostFileVO（例如按最后更新时间/lastSyncDate 或 id 选最大者）生成一个 deduped 列表，然后在该 deduped 列表上重新计算 hasChanged、oldestLastSync 和决定 syncReason，再用同一 deduped 列表为 SyncVmHostFilesFromHostMsg 填充 nvRam/tpm 路径（涉及符号：VmHostFileVO、VmHostFileType、VmHostFileTracker、SyncVmHostFilesFromHostMsg、hasChanged、oldestLastSync、syncReason）。同时确保前端将某 VM 标记为 dirty 的逻辑也只对这同一条 current/latest 记录生效。
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java (1)
186-188: 把这条周期日志降到debug或做限频。默认间隔已经改成 15 秒，这里的
info会稳定刷屏，线上很容易淹没真正异常。更合适的是debug，或者只在本轮实际触发 dirty/force-sync 时再输出info。♻️ 一个最小改动
- logger.info("VmHostFileTracker: starting periodic check of VM host files"); + logger.debug("VmHostFileTracker: starting periodic check of VM host files");🤖 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/vmfiles/VmHostFileTracker.java` around lines 186 - 188, 将 VmHostFileTracker 中匿名 Runnable 的开头日志从持续性的 logger.info 降级为 debug 或限频输出：在 run() 方法里把 logger.info("VmHostFileTracker: starting periodic check of VM host files") 改为 logger.debug(...)，或者仅在 syncVmHostFiles() 实际触发 dirty/force-sync 时才调用 logger.info。定位符：匿名 Runnable 的 run()、类 VmHostFileTracker 和方法 syncVmHostFiles()；可实现方式：1) 直接改为 logger.debug；或 2) 修改 syncVmHostFiles() 返回布尔/状态表示是否有变更，然后在 run() 中仅当返回 true（或状态包含 dirty/force-sync）时才记录 info 级别日志。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java`:
- Around line 264-292: 当前代码在 VmHostFileTracker 中对整个 group 直接计算
hasChanged/oldestLastSync 导致历史记录（旧的 VmHostFileVO）把当前文件组误判为需要同步；请先基于
VmHostFileType 收敛到每种 type 的“current/latest” VmHostFileVO（例如按最后更新时间/lastSyncDate
或 id 选最大者）生成一个 deduped 列表，然后在该 deduped 列表上重新计算 hasChanged、oldestLastSync 和决定
syncReason，再用同一 deduped 列表为 SyncVmHostFilesFromHostMsg 填充 nvRam/tpm
路径（涉及符号：VmHostFileVO、VmHostFileType、VmHostFileTracker、SyncVmHostFilesFromHostMsg、hasChanged、oldestLastSync、syncReason）。同时确保前端将某
VM 标记为 dirty 的逻辑也只对这同一条 current/latest 记录生效。
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.java`:
- Around line 186-188: 将 VmHostFileTracker 中匿名 Runnable 的开头日志从持续性的 logger.info
降级为 debug 或限频输出：在 run() 方法里把 logger.info("VmHostFileTracker: starting periodic
check of VM host files") 改为 logger.debug(...)，或者仅在 syncVmHostFiles() 实际触发
dirty/force-sync 时才调用 logger.info。定位符：匿名 Runnable 的 run()、类 VmHostFileTracker
和方法 syncVmHostFiles()；可实现方式：1) 直接改为 logger.debug；或 2) 修改 syncVmHostFiles()
返回布尔/状态表示是否有变更，然后在 run() 中仅当返回 true（或状态包含 dirty/force-sync）时才记录 info 级别日志。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa93547d-edae-4a2b-8338-9729ed468986
📒 Files selected for processing (10)
conf/db/zsv/V5.0.0__schema.sqlheader/src/main/java/org/zstack/header/vm/VmCanonicalEvents.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileSyncReason.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileVO.javaheader/src/main/java/org/zstack/header/vm/additions/VmHostFileVO_.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.javaplugin/kvm/src/main/java/org/zstack/kvm/vmfiles/VmHostFileTracker.javasdk/src/main/java/org/zstack/sdk/vm/entity/VmHostFileInventory.java
✅ Files skipped from review due to trivial changes (4)
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileVO_.java
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileSyncReason.java
- sdk/src/main/java/org/zstack/sdk/vm/entity/VmHostFileInventory.java
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileInventory.java
🚧 Files skipped from review as they are similar to previous changes (3)
- conf/db/zsv/V5.0.0__schema.sql
- header/src/main/java/org/zstack/header/vm/additions/VmHostFileVO.java
- plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootManager.java
Collaborator
Author
|
Comment from yaohua.wu: 🔍 Code Review —
|
Owner
|
Comment from wenhao.zhang: (vmUuid, hostUuid, type) 数据库中有唯一键 UNIQUE KEY |
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.
Implement periodic VM host file checking with dirty detection and
force sync mechanism. This improves the sync interval from 30 minutes
to 15 seconds, allowing faster detection and sync of file changes.
Changes:
changed or force sync after long period without changes
Resolves: ZSV-11371
Related: ZSV-11310
Change-Id: I6f62727061686b646f617463737672686d667578
sync from gitlab !9555