<refactor>[pciDevice]: Flyway migrations for GPU scope, BM2 phantom host, MdevDeviceVO subtable#3394
Open
MatheMatrix wants to merge 8 commits into5.5.12from
Open
<refactor>[pciDevice]: Flyway migrations for GPU scope, BM2 phantom host, MdevDeviceVO subtable#3394MatheMatrix wants to merge 8 commits into5.5.12from
MatheMatrix wants to merge 8 commits into5.5.12from
Conversation
…om host, MdevDeviceVO subtable Resolves: ZSTAC-68709 Change-Id: Ib0c9bbcad669d8e0c311f9f3d2d1c83953a689b1
- M1: Wrap V6.0.0.2 BM2 GPU migration in conditional procedure that
checks for BareMetal2ChassisGpuDeviceVO table existence (safe for
non-BM2 deployments)
- M5: Add cleanup of old MdevDeviceVO AccountResourceRefVO entries in
V6.0.0.3 to prevent duplicate resource references
Resolves: ZSTAC-68709
Change-Id: I6d8c934cdc580eef26e01e40122e4f989da84136
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough新增三条数据库升级脚本以扩展和迁移 GPU 与 Mdev 相关表；引入主机跟踪启动抖动与 AsyncTimer 的延迟启动接口；新增 PingLatencyEma 与自适应超时并在 KVMHost 中使用；添加相应单元测试与大量 ApiHelper API 方法封装。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@conf/db/upgrade/V6.0.0.1__gpu_add_scope.sql`:
- Around line 3-13: Wrap all table and column identifiers in backticks to follow
the naming convention and avoid reserved-word conflicts: update ALTER TABLE
statements for GpuDeviceVO to use `GpuDeviceVO`, `scope`, `chassisUuid`; update
the UPDATE ... JOIN block to use `GpuDeviceVO`, `PciDeviceVO`, `g`.`uuid`,
`p`.`uuid`, and `virtStatus` (e.g., `p`.`virtStatus`) and the WHERE string
comparison; and update the CREATE INDEX to use `idxGpuDeviceVOScope` and
`GpuDeviceVO`/`scope` in backticks. Keep string defaults and literals (e.g.,
'VM', 'HAMI_VIRTUALIZED') unchanged. Ensure every bare identifier in the ALTER,
UPDATE, JOIN, WHERE and CREATE INDEX statements is wrapped with backticks.
- Line 13: 在 V6.0.0.1 的迁移脚本中直接执行 CREATE INDEX idxGpuDeviceVOScope ON GpuDeviceVO
(scope) 缺少幂等性检查；请改为先检测索引是否已存在（例如查询 pg_class/pg_indexes 或
information_schema）再创建，从而避免重复运行失败；在该脚本里以 idxGpuDeviceVOScope 和 表名 GpuDeviceVO
为判断条件，若不存在则执行创建索引的语句，否则跳过创建。
- Around line 3-4: The ALTER TABLE statements are not idempotent; wrap them in a
conditional that checks information_schema.COLUMNS for table_name='GpuDeviceVO'
and column_name='scope' / 'chassisUuid' and only run ALTER TABLE GpuDeviceVO ADD
COLUMN ... when the column does not already exist (you can implement this as a
stored procedure or anonymous block that queries information_schema and performs
ALTER TABLE for 'scope' and 'chassisUuid' accordingly); ensure the new columns
use the same definitions (VARCHAR(32) DEFAULT 'VM' NOT NULL for scope,
VARCHAR(32) DEFAULT NULL for chassisUuid) and that each addition is guarded by
its own existence-check to make the migration safely re-runnable.
In `@conf/db/upgrade/V6.0.0.2__bm2_gpu_migrate.sql`:
- Around line 12-82: 给所有 SQL 标识符加反引号以避免保留字冲突：在脚本中把表名
HostEO、BareMetal2ChassisVO、BareMetal2ChassisPciDeviceVO、BareMetal2ChassisGpuDeviceVO、PciDeviceVO、GpuDeviceVO
以及所有列名（例如
uuid、name、description、zoneUuid、clusterUuid、managementIp、hypervisorType、state、status、createDate、lastOpDate、type、virtStatus、vendorId、deviceId、subvendorId、subdeviceId、pciDeviceAddress、iommuGroup、vendor、device、serialNumber、memory、power、isDriverLoaded、scope、chassisUuid、_migrated
等）都用反引号包裹；在所有 SELECT/INSERT/WHERE/JOIN/ALTER/UPDATE 子句内统一替换未加引号的标识符（例如在 INSERT
INTO HostEO、INSERT INTO PciDeviceVO、INSERT INTO GpuDeviceVO、ALTER TABLE
BareMetal2ChassisPciDeviceVO、以及子查询中引用的所有列）为带反引号的形式以保证兼容 MySQL 8.0/GreatSQL。
In `@conf/db/upgrade/V6.0.0.3__mdev_subtable.sql`:
- Around line 72-75: Step 6 currently deletes AccountResourceRefVO rows for
resourceType 'MdevDeviceVO' unconditionally; wrap the Step 5 INSERT and this
DELETE in a single transaction (or implement a stored procedure) so the DELETE
only commits if the INSERT into AccountResourceRefVO (from Step 5) succeeds;
specifically, enclose the Step 5 INSERT and the DELETE FROM AccountResourceRefVO
... WHERE resourceType = 'MdevDeviceVO' AND resourceUuid IN (SELECT uuid FROM
MdevDeviceVO) in BEGIN/COMMIT with ROLLBACK on error (or move delete into the
same stored procedure and check the INSERT result before performing the DELETE)
to ensure atomicity.
- Around line 4-76: The SQL uses unquoted identifiers which can conflict with
MySQL reserved words; update every table and column reference to use backticks
(e.g., `PciDeviceVO`, `MdevDeviceVO`, `AccountResourceRefVO`, and column names
like `type`, `state`, `status`, `name`, `description`, `uuid`,
`mdevDeviceAddress`, `mdevSpecUuid`, `mttyUuid`, etc.) across all statements
(INSERT ... SELECT, JOIN, WHERE, RENAME TABLE, CREATE TABLE, DELETE) so all
identifiers are wrapped in backticks and foreign key clauses and literals remain
unchanged.
- Around line 31-52: 脚本缺乏幂等性——在对 MdevDeviceVO 做 RENAME、CREATE、INSERT
操作时会在重复执行时失败；请将逻辑改为存储过程并在执行前通过 information_schema 检查表/约束是否存在（检查
MdevDeviceVO、MdevDeviceVO_old、PciDeviceVO、MdevDeviceSpecVO、MttyDeviceVO），仅在旧表存在且新表不存在时执行
RENAME TABLE MdevDeviceVO TO MdevDeviceVO_old，并仅在新表不存在时执行 CREATE TABLE
MdevDeviceVO，最后在插入数据时使用条件插入（例如基于 NOT EXISTS 或 INSERT IGNORE 的等效逻辑）以避免主键冲突；参考
V6.0.0.2 的存储过程实现模式来组织这些检查与 DDL/DML 操作。
ℹ️ Review info
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
conf/db/upgrade/V6.0.0.1__gpu_add_scope.sqlconf/db/upgrade/V6.0.0.2__bm2_gpu_migrate.sqlconf/db/upgrade/V6.0.0.3__mdev_subtable.sql
Comment on lines
+31
to
+52
| -- Step 2: Rename old MdevDeviceVO table | ||
| RENAME TABLE MdevDeviceVO TO MdevDeviceVO_old; | ||
|
|
||
| -- Step 3: Create new MdevDeviceVO as subtable of PciDeviceVO | ||
| CREATE TABLE MdevDeviceVO ( | ||
| uuid VARCHAR(32) NOT NULL UNIQUE, | ||
| mdevSpecUuid VARCHAR(32) DEFAULT NULL, | ||
| mttyUuid VARCHAR(32) DEFAULT NULL, | ||
| mdevDeviceAddress VARCHAR(128) DEFAULT NULL, | ||
| PRIMARY KEY (uuid), | ||
| CONSTRAINT fkMdevDeviceVOPciDeviceVO | ||
| FOREIGN KEY (uuid) REFERENCES PciDeviceVO(uuid) ON DELETE CASCADE, | ||
| CONSTRAINT fkMdevDeviceVOMdevSpecVO | ||
| FOREIGN KEY (mdevSpecUuid) REFERENCES MdevDeviceSpecVO(uuid) ON DELETE SET NULL, | ||
| CONSTRAINT fkMdevDeviceVOMttyDeviceVO | ||
| FOREIGN KEY (mttyUuid) REFERENCES MttyDeviceVO(uuid) ON DELETE CASCADE | ||
| ) ENGINE=InnoDB; | ||
|
|
||
| -- Step 4: Migrate mdev-specific data to new subtable | ||
| INSERT INTO MdevDeviceVO (uuid, mdevSpecUuid, mttyUuid, mdevDeviceAddress) | ||
| SELECT uuid, mdevSpecUuid, mttyUuid, mdevDeviceAddress | ||
| FROM MdevDeviceVO_old; |
There was a problem hiding this comment.
缺少幂等性保护，重复执行会失败。
这个迁移脚本存在严重的幂等性问题：
- Line 32:
RENAME TABLE MdevDeviceVO TO MdevDeviceVO_old- 如果脚本重复执行，MdevDeviceVO表已不存在会导致失败 - Line 35:
CREATE TABLE MdevDeviceVO- 如果表已存在会失败 - Lines 50-52: INSERT 语句没有
NOT EXISTS或INSERT IGNORE保护，重复执行会导致主键冲突
建议将整个迁移过程包装在存储过程中，使用 information_schema 检查表是否存在，参考 V6.0.0.2 的实现方式。
🛠️ 建议使用存储过程包装
+DELIMITER $$
+DROP PROCEDURE IF EXISTS migrate_mdev_subtable$$
+CREATE PROCEDURE migrate_mdev_subtable()
+BEGIN
+ -- Only proceed if old table exists and new subtable doesn't
+ IF EXISTS (SELECT 1 FROM information_schema.TABLES
+ WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'MdevDeviceVO')
+ AND NOT EXISTS (SELECT 1 FROM information_schema.TABLE_CONSTRAINTS
+ WHERE TABLE_SCHEMA = DATABASE()
+ AND TABLE_NAME = 'MdevDeviceVO'
+ AND CONSTRAINT_NAME = 'fkMdevDeviceVOPciDeviceVO') THEN
+
+ -- Step 1: Migrate shared fields to PciDeviceVO
+ INSERT INTO `PciDeviceVO` (...)
+ SELECT ... FROM `MdevDeviceVO` m
+ WHERE NOT EXISTS (...);
+
+ -- Step 2: Rename old table
+ RENAME TABLE `MdevDeviceVO` TO `MdevDeviceVO_old`;
+
+ -- Step 3: Create new subtable
+ CREATE TABLE `MdevDeviceVO` (...);
+
+ -- Step 4: Migrate mdev-specific data
+ INSERT INTO `MdevDeviceVO` (...)
+ SELECT ... FROM `MdevDeviceVO_old`;
+
+ END IF;
+
+ -- Steps 5 & 6 can run safely with INSERT IGNORE and conditional DELETE
+END$$
+DELIMITER ;
+
+CALL migrate_mdev_subtable();
+DROP PROCEDURE IF EXISTS migrate_mdev_subtable;Based on learnings: ZStack 数据库升级脚本需要通过 information_schema 检查表和约束的存在性来确保脚本可以安全重复执行。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@conf/db/upgrade/V6.0.0.3__mdev_subtable.sql` around lines 31 - 52,
脚本缺乏幂等性——在对 MdevDeviceVO 做 RENAME、CREATE、INSERT 操作时会在重复执行时失败；请将逻辑改为存储过程并在执行前通过
information_schema 检查表/约束是否存在（检查
MdevDeviceVO、MdevDeviceVO_old、PciDeviceVO、MdevDeviceSpecVO、MttyDeviceVO），仅在旧表存在且新表不存在时执行
RENAME TABLE MdevDeviceVO TO MdevDeviceVO_old，并仅在新表不存在时执行 CREATE TABLE
MdevDeviceVO，最后在插入数据时使用条件插入（例如基于 NOT EXISTS 或 INSERT IGNORE 的等效逻辑）以避免主键冲突；参考
V6.0.0.2 的存储过程实现模式来组织这些检查与 DDL/DML 操作。
- All 3 SQL files: wrap table/column identifiers in backticks for MySQL 8.0/GreatSQL reserved word safety - V6.0.0.1: wrap in stored procedure with information_schema checks for column and index existence (idempotent) - V6.0.0.3: wrap DDL (RENAME/CREATE TABLE) in stored procedure with constraint existence check; use INSERT IGNORE for Step 4 (idempotent) Resolves: ZSTAC-68709 Change-Id: Ic7114c1ac5aeffd571ebb1e4f7b736adcc1c9c03 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@conf/db/upgrade/V6.0.0.3__mdev_subtable.sql`:
- Around line 68-91: Steps 5-6 perform INSERT ... SELECT and DELETE against
AccountResourceRefVO with subqueries referencing the MdevDeviceVO table but lack
a guard that the MdevDeviceVO table exists; wrap these two statements in the
same information_schema existence check used elsewhere so they only run when
MdevDeviceVO exists (e.g., check information_schema.tables for
table_name='MdevDeviceVO' and current DATABASE()), and apply that guard around
the INSERT IGNORE ... SELECT (the statement selecting from AccountResourceRefVO
ar where ar.resourceType='MdevDeviceVO') and the subsequent DELETE FROM
`AccountResourceRefVO` WHERE `resourceType`='MdevDeviceVO' AND `resourceUuid` IN
(SELECT `uuid` FROM `MdevDeviceVO`) so the migration can be safely re-run in
non-mdev or partial-failure scenarios.
ℹ️ Review info
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
conf/db/upgrade/V6.0.0.1__gpu_add_scope.sqlconf/db/upgrade/V6.0.0.2__bm2_gpu_migrate.sqlconf/db/upgrade/V6.0.0.3__mdev_subtable.sql
…check Wrap AccountResourceRefVO migration (Steps 5-6) in IF EXISTS check for MdevDeviceVO table, preventing failure in non-mdev deployments. Resolves: ZSTAC-68709 Change-Id: I7affad970216837e5a3a8656d28bcaf446616476 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
conf/db/upgrade/V6.0.0.3__mdev_subtable.sql (3)
8-13:⚠️ Potential issue | 🟡 Minor
information_schema查询里的标识符仍有未加反引号。Line 8-13、69-70 中
information_schema.TABLES/information_schema.TABLE_CONSTRAINTS及TABLE_SCHEMA、TABLE_NAME、CONSTRAINT_NAME仍为裸标识符。建议按仓库 SQL 规范统一加反引号。🛠️ 建议修复
- IF EXISTS (SELECT 1 FROM information_schema.TABLES - WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'MdevDeviceVO') - AND NOT EXISTS (SELECT 1 FROM information_schema.TABLE_CONSTRAINTS - WHERE TABLE_SCHEMA = DATABASE() - AND TABLE_NAME = 'MdevDeviceVO' - AND CONSTRAINT_NAME = 'fkMdevDeviceVOPciDeviceVO') THEN + IF EXISTS (SELECT 1 FROM `information_schema`.`TABLES` + WHERE `TABLE_SCHEMA` = DATABASE() AND `TABLE_NAME` = 'MdevDeviceVO') + AND NOT EXISTS (SELECT 1 FROM `information_schema`.`TABLE_CONSTRAINTS` + WHERE `TABLE_SCHEMA` = DATABASE() + AND `TABLE_NAME` = 'MdevDeviceVO' + AND `CONSTRAINT_NAME` = 'fkMdevDeviceVOPciDeviceVO') THEN @@ - IF EXISTS (SELECT 1 FROM information_schema.TABLES - WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'MdevDeviceVO') THEN + IF EXISTS (SELECT 1 FROM `information_schema`.`TABLES` + WHERE `TABLE_SCHEMA` = DATABASE() AND `TABLE_NAME` = 'MdevDeviceVO') THENAs per coding guidelines: 所有表名和列名必须使用反引号包裹（例如：WHERE
system= 1），以避免 MySQL 8.0 / GreatSQL 保留关键字冲突导致的语法错误。Also applies to: 69-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conf/db/upgrade/V6.0.0.3__mdev_subtable.sql` around lines 8 - 13, The SQL uses unquoted identifiers in the information_schema queries; update the IF condition and other occurrences (e.g., references to information_schema.TABLES, information_schema.TABLE_CONSTRAINTS, TABLE_SCHEMA, TABLE_NAME, CONSTRAINT_NAME, and values like MdevDeviceVO and fkMdevDeviceVOPciDeviceVO) to wrap all schema/table/column/constraint names and literal identifiers in backticks per repo SQL style (e.g., `information_schema`.`TABLES`, `TABLE_SCHEMA`, `TABLE_NAME`, `CONSTRAINT_NAME`, `MdevDeviceVO`, `fkMdevDeviceVOPciDeviceVO`) and apply the same change to the other occurrences noted at lines 69-70.
8-13:⚠️ Potential issue | 🟠 Major补齐断点恢复分支，否则重跑可能永久跳过迁移。
若一次执行在 Line 44
RENAME成功后、Line 47CREATE TABLE前失败，库里会只剩MdevDeviceVO_old。当前入口条件仅检查MdevDeviceVO，重跑会跳过 Step 1-4，迁移无法自愈。建议增加MdevDeviceVO_old存在且MdevDeviceVO缺失时的恢复分支（继续建新表并从 old 回填）。🛠️ 建议修复（关键条件示例）
- IF EXISTS (SELECT 1 FROM information_schema.TABLES - WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'MdevDeviceVO') - AND NOT EXISTS (SELECT 1 FROM information_schema.TABLE_CONSTRAINTS - WHERE TABLE_SCHEMA = DATABASE() - AND TABLE_NAME = 'MdevDeviceVO' - AND CONSTRAINT_NAME = 'fkMdevDeviceVOPciDeviceVO') THEN + IF ( + EXISTS (SELECT 1 FROM `information_schema`.`TABLES` + WHERE `TABLE_SCHEMA` = DATABASE() AND `TABLE_NAME` = 'MdevDeviceVO') + AND NOT EXISTS (SELECT 1 FROM `information_schema`.`TABLE_CONSTRAINTS` + WHERE `TABLE_SCHEMA` = DATABASE() + AND `TABLE_NAME` = 'MdevDeviceVO' + AND `CONSTRAINT_NAME` = 'fkMdevDeviceVOPciDeviceVO') + ) + OR ( + NOT EXISTS (SELECT 1 FROM `information_schema`.`TABLES` + WHERE `TABLE_SCHEMA` = DATABASE() AND `TABLE_NAME` = 'MdevDeviceVO') + AND EXISTS (SELECT 1 FROM `information_schema`.`TABLES` + WHERE `TABLE_SCHEMA` = DATABASE() AND `TABLE_NAME` = 'MdevDeviceVO_old') + ) THEN#!/bin/bash # 核验是否存在 MdevDeviceVO_old 的恢复路径 rg -nP --type=sql -C3 "TABLE_NAME\s*=\s*'MdevDeviceVO_old'|RENAME TABLE\s+`MdevDeviceVO`\s+TO\s+`MdevDeviceVO_old`|IF EXISTS\s*\(SELECT 1 FROM information_schema"Based on learnings: 在 ZStack 数据库升级脚本中，直接使用 RENAME TABLE 不能保证幂等性。应该通过 information_schema.tables 检查表的存在性，只在源表存在且目标表不存在时才执行重命名操作，以确保升级脚本可以安全地重复执行。
Also applies to: 43-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conf/db/upgrade/V6.0.0.3__mdev_subtable.sql` around lines 8 - 13, The migration currently only checks for MdevDeviceVO and a missing constraint (fkMdevDeviceVOPciDeviceVO), so if the script failed after the RENAME that produced MdevDeviceVO_old, re-running will skip steps and never recover; update the conditional logic around the RENAME/CREATE sequence to be idempotent: add an explicit recovery branch that detects when MdevDeviceVO_old exists but MdevDeviceVO does not and then proceeds to CREATE the new MdevDeviceVO, reapply constraints (fkMdevDeviceVOPciDeviceVO) and copy data back from MdevDeviceVO_old, and change the RENAME logic to only execute RENAME TABLE `MdevDeviceVO` TO `MdevDeviceVO_old` when MdevDeviceVO exists and MdevDeviceVO_old does not so the migration can be safely re-run.
53-55:⚠️ Potential issue | 🟠 Major修复幂等性问题：处理 RENAME 与 CREATE 之间的故障恢复。
第 8-10 行的条件仅检查表是否存在（
EXISTS MdevDeviceVO），无法处理 RENAME 成功但 CREATE 失败的场景。此时MdevDeviceVO已不存在，条件为假，后续迁移步骤被跳过，导致脚本无法恢复。建议修改为检查
MdevDeviceVO_old的存在，确保无论处于哪个中间状态都能继续执行：IF EXISTS (SELECT 1 FROM information_schema.TABLES WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'MdevDeviceVO') OR EXISTS (SELECT 1 FROM information_schema.TABLES WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'MdevDeviceVO_old') AND NOT EXISTS (SELECT 1 FROM information_schema.TABLE_CONSTRAINTS同时，第 8-10、69-70 行的
information_schema标识符建议加上反引号（information_schema.`TABLES`等）以符合 SQL 编码规范。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conf/db/upgrade/V6.0.0.3__mdev_subtable.sql` around lines 53 - 55, The migration's idempotency check should be expanded to handle the case where RENAME succeeded but CREATE failed by checking for the existence of MdevDeviceVO_old as well as MdevDeviceVO; update the conditional that currently only tests EXISTS MdevDeviceVO to also test for EXISTS MdevDeviceVO_old (so the script will proceed when the old table remains) and ensure the later renaming/creation steps run correctly, and also wrap the information_schema identifiers in backticks (e.g., `information_schema`.`TABLES`) in both the existence checks and any queries referencing `information_schema` to follow SQL identifier quoting conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@conf/db/upgrade/V6.0.0.3__mdev_subtable.sql`:
- Around line 8-13: The SQL uses unquoted identifiers in the information_schema
queries; update the IF condition and other occurrences (e.g., references to
information_schema.TABLES, information_schema.TABLE_CONSTRAINTS, TABLE_SCHEMA,
TABLE_NAME, CONSTRAINT_NAME, and values like MdevDeviceVO and
fkMdevDeviceVOPciDeviceVO) to wrap all schema/table/column/constraint names and
literal identifiers in backticks per repo SQL style (e.g.,
`information_schema`.`TABLES`, `TABLE_SCHEMA`, `TABLE_NAME`, `CONSTRAINT_NAME`,
`MdevDeviceVO`, `fkMdevDeviceVOPciDeviceVO`) and apply the same change to the
other occurrences noted at lines 69-70.
- Around line 8-13: The migration currently only checks for MdevDeviceVO and a
missing constraint (fkMdevDeviceVOPciDeviceVO), so if the script failed after
the RENAME that produced MdevDeviceVO_old, re-running will skip steps and never
recover; update the conditional logic around the RENAME/CREATE sequence to be
idempotent: add an explicit recovery branch that detects when MdevDeviceVO_old
exists but MdevDeviceVO does not and then proceeds to CREATE the new
MdevDeviceVO, reapply constraints (fkMdevDeviceVOPciDeviceVO) and copy data back
from MdevDeviceVO_old, and change the RENAME logic to only execute RENAME TABLE
`MdevDeviceVO` TO `MdevDeviceVO_old` when MdevDeviceVO exists and
MdevDeviceVO_old does not so the migration can be safely re-run.
- Around line 53-55: The migration's idempotency check should be expanded to
handle the case where RENAME succeeded but CREATE failed by checking for the
existence of MdevDeviceVO_old as well as MdevDeviceVO; update the conditional
that currently only tests EXISTS MdevDeviceVO to also test for EXISTS
MdevDeviceVO_old (so the script will proceed when the old table remains) and
ensure the later renaming/creation steps run correctly, and also wrap the
information_schema identifiers in backticks (e.g.,
`information_schema`.`TABLES`) in both the existence checks and any queries
referencing `information_schema` to follow SQL identifier quoting conventions.
ℹ️ Review info
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
conf/db/upgrade/V6.0.0.3__mdev_subtable.sql
…61971) to prevent thundering herd Resolves: ZSTAC-67534 Change-Id: Ic9f28ffed4dcc066da2c22491d117acf38662dfe
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (1)
5020-5020: 建议用单调时钟统计 RTT，避免系统时间跳变影响 EMA。
System.currentTimeMillis()会受时钟回拨/NTP 校时影响，可能出现负延迟或异常尖峰，进而污染自适应超时。建议改为System.nanoTime()计算耗时。可选修正示例
- final long pingStartMs = System.currentTimeMillis(); + final long pingStartNs = System.nanoTime(); ... - pingLatencyEma.update(System.currentTimeMillis() - pingStartMs); + long rttMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - pingStartNs); + pingLatencyEma.update(rttMs);Also applies to: 5030-5030
🤖 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/KVMHost.java` at line 5020, The RTT timing uses System.currentTimeMillis() (pingStartMs) in class KVMHost which is vulnerable to system clock shifts; change to a monotonic clock by using System.nanoTime() for pingStartNs and compute elapsed RTT as (System.nanoTime() - pingStartNs)/1_000_000 to get milliseconds, and update any corresponding uses (including the other occurrence around the pingStart variable near the second location) to consume the computed millisecond RTT rather than currentTimeMillis differences.compute/src/test/java/org/zstack/compute/host/PingLatencyEmaTest.java (1)
13-87: 建议提取重复参数为测试常量，减少魔法值。
0.3、3、30在多个用例重复，后续调参容易漏改；提取为常量可提升维护性。建议修改
public class PingLatencyEmaTest { + private static final double DEFAULT_ALPHA = 0.3; + private static final int DEFAULT_TIMEOUT_MULTIPLIER = 3; + private static final long GLOBAL_TIMEOUT_SECONDS = 30; `@Test` public void testEmaWithSyntheticLatencySequence() { - PingLatencyEma ema = new PingLatencyEma(0.3, 3); + PingLatencyEma ema = new PingLatencyEma(DEFAULT_ALPHA, DEFAULT_TIMEOUT_MULTIPLIER); ... public void testAdaptiveTimeoutUsesGlobalWhenNoSamples() { - PingLatencyEma ema = new PingLatencyEma(0.3, 3); + PingLatencyEma ema = new PingLatencyEma(DEFAULT_ALPHA, DEFAULT_TIMEOUT_MULTIPLIER); - Assert.assertEquals(30, ema.computeAdaptiveTimeout(30)); + Assert.assertEquals(GLOBAL_TIMEOUT_SECONDS, ema.computeAdaptiveTimeout(GLOBAL_TIMEOUT_SECONDS)); }As per coding guidelines “避免使用魔法值（Magic Value）…应替换为常量或枚举”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compute/src/test/java/org/zstack/compute/host/PingLatencyEmaTest.java` around lines 13 - 87, Tests for PingLatencyEma use repeated magic numbers (0.3, 3, 30); refactor by introducing class-level test constants (e.g., ALPHA = 0.3, MULTIPLIER = 3, GLOBAL_TIMEOUT = 30) and replace all literal occurrences in tests like testAdaptiveTimeoutUsesGlobalWhenNoSamples, testAdaptiveTimeoutNeverBelowGlobal, testAdaptiveTimeoutIncreasesForHighLatency, testAdaptiveTimeoutCappedAt3xGlobal, testEmaConvergesAfterLatencyDrop and the EMA construction calls (new PingLatencyEma(...)) to reference these constants so future tuning affects all cases consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compute/src/main/java/org/zstack/compute/host/HostTrackImpl.java`:
- Around line 271-274: The code uses
ThreadLocalRandom.current().nextLong(HostGlobalConfig.PING_HOST_INTERVAL.value(Long.class)
* 1000) which can throw IllegalArgumentException if the computed bound is ≤ 0;
add a defensive check around
HostGlobalConfig.PING_HOST_INTERVAL.value(Long.class) (or the computed millis)
before calling nextLong and fall back to a safe positive bound (e.g., clamp to
1000ms or Math.max(1, interval) * 1000) so jitterMs is always computed from a
positive bound, then call t.startWithDelay(jitterMs, TimeUnit.MILLISECONDS);
reference symbols: HostGlobalConfig.PING_HOST_INTERVAL,
ThreadLocalRandom.current().nextLong, jitterMs, t.startWithDelay.
In `@compute/src/main/java/org/zstack/compute/host/PingLatencyEma.java`:
- Around line 12-30: The constructor and methods lack input validation causing
incorrect adaptive timeout results; in PingLatencyEma's constructor validate
that alpha is >0 and <=1 and timeoutMultiplier >0 (throw
IllegalArgumentException), in update(long latencyMs) assert latencyMs >= 0
(reject or ignore negatives by throwing IllegalArgumentException) before using
it to set emaMs, and in computeAdaptiveTimeout(long globalTimeoutSeconds)
validate globalTimeoutSeconds > 0 (throw IllegalArgumentException) and guard
against emaMs overflow/negative values when computing emaBasedTimeout; update
references: PingLatencyEma(double alpha, int timeoutMultiplier), update(long
latencyMs), computeAdaptiveTimeout(long globalTimeoutSeconds), and field emaMs.
In `@compute/src/test/java/org/zstack/compute/host/HostTrackJitterTest.java`:
- Around line 14-15: The test file HostTrackJitterTest.java contains Chinese
comments (e.g., the class/file header and the comment around the jitter
simulation), which violates the repo's English-only rule; update the comments at
the locations that reference the AC and the jitter simulation (previously
Chinese at lines near the class header and the simulation description) to clear,
correct English equivalents—keep the same intent (e.g., "AC: Jitter test —
reconnection timestamps should be uniformly distributed (not all t=0)" and
"Simulates the jitter logic from HostTrackImpl.trackHost() for 3000 hosts") so
static checks and style reviews pass.
In `@compute/src/test/java/org/zstack/compute/host/PingLatencyEmaTest.java`:
- Around line 8-10: Replace the Chinese Javadoc in the PingLatencyEmaTest class
with an accurate English description: update the comment that currently reads
"AC: 合成延迟序列 [100, 200, 150, 300, 250]ms → EMA 产出正确自适应超时" to an English Javadoc
(for example: "AC: Synthetic latency sequence [100, 200, 150, 300, 250] ms → EMA
produces correct adaptive timeout") so the test class PingLatencyEmaTest and its
Javadoc conform to the repository's English-only documentation guideline.
In `@core/src/main/java/org/zstack/core/thread/AsyncTimer.java`:
- Around line 72-81: In startWithDelay(long delay, TimeUnit delayUnit) validate
inputs before using thdf: check that delayUnit is not null and delay is
non-negative and if invalid throw an IllegalArgumentException (or
CloudRuntimeException consistent with this class) with a clear message; keep the
existing cancelled check and existing behavior of assigning cancel =
thdf.submitTimeoutTask(this, delayUnit, delay) and the logger.trace call
unchanged.
---
Nitpick comments:
In `@compute/src/test/java/org/zstack/compute/host/PingLatencyEmaTest.java`:
- Around line 13-87: Tests for PingLatencyEma use repeated magic numbers (0.3,
3, 30); refactor by introducing class-level test constants (e.g., ALPHA = 0.3,
MULTIPLIER = 3, GLOBAL_TIMEOUT = 30) and replace all literal occurrences in
tests like testAdaptiveTimeoutUsesGlobalWhenNoSamples,
testAdaptiveTimeoutNeverBelowGlobal, testAdaptiveTimeoutIncreasesForHighLatency,
testAdaptiveTimeoutCappedAt3xGlobal, testEmaConvergesAfterLatencyDrop and the
EMA construction calls (new PingLatencyEma(...)) to reference these constants so
future tuning affects all cases consistently.
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Line 5020: The RTT timing uses System.currentTimeMillis() (pingStartMs) in
class KVMHost which is vulnerable to system clock shifts; change to a monotonic
clock by using System.nanoTime() for pingStartNs and compute elapsed RTT as
(System.nanoTime() - pingStartNs)/1_000_000 to get milliseconds, and update any
corresponding uses (including the other occurrence around the pingStart variable
near the second location) to consume the computed millisecond RTT rather than
currentTimeMillis differences.
ℹ️ Review info
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
compute/pom.xmlis excluded by!**/*.xmlsdk/src/main/java/org/zstack/sdk/GetGpuMetricsBatchAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/GetGpuMetricsBatchResult.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/GetGpuResourceTreeAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/GetGpuResourceTreeResult.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/GpuMetricsEntry.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/GpuResourceTreeNode.javais excluded by!sdk/**
📒 Files selected for processing (6)
compute/src/main/java/org/zstack/compute/host/HostTrackImpl.javacompute/src/main/java/org/zstack/compute/host/PingLatencyEma.javacompute/src/test/java/org/zstack/compute/host/HostTrackJitterTest.javacompute/src/test/java/org/zstack/compute/host/PingLatencyEmaTest.javacore/src/main/java/org/zstack/core/thread/AsyncTimer.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
Comment on lines
+271
to
+274
| // ZSTAC-61971: jitter to avoid thundering herd when 3000+ hosts reconnect simultaneously | ||
| long jitterMs = ThreadLocalRandom.current().nextLong( | ||
| HostGlobalConfig.PING_HOST_INTERVAL.value(Long.class) * 1000); | ||
| t.startWithDelay(jitterMs, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) 查找 PING_HOST_INTERVAL 的定义与校验逻辑
rg -nP --type=java -C5 'PING_HOST_INTERVAL' | head -100Repository: MatheMatrix/zstack
Length of output: 11166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 2) 查找 HostGlobalConfig 定义
fd -e java | xargs grep -l "class HostGlobalConfig" | head -5Repository: MatheMatrix/zstack
Length of output: 131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 3) 查看完整的 HostTrackImpl.java 文件的相关上下文（271-280行）
fd -e java -path "*/compute/src/main/java/org/zstack/compute/host/HostTrackImpl.java" | xargs sed -n '260,280p'Repository: MatheMatrix/zstack
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read HostGlobalConfig.java
cat -n "compute/src/main/java/org/zstack/compute/host/HostGlobalConfig.java"Repository: MatheMatrix/zstack
Length of output: 4452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read HostTrackImpl.java around lines 271-274
wc -l "compute/src/main/java/org/zstack/compute/host/HostTrackImpl.java"Repository: MatheMatrix/zstack
Length of output: 130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for validation patterns related to HostGlobalConfig
rg -nP --type=java 'installUpdateExtension|validator.*PING_HOST|HostGlobalConfig.*validate' -A 5Repository: MatheMatrix/zstack
Length of output: 39079
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read HostTrackImpl.java lines 260-280 to see full context
sed -n '260,280p' "compute/src/main/java/org/zstack/compute/host/HostTrackImpl.java"Repository: MatheMatrix/zstack
Length of output: 776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for GlobalConfigValidation enforcement/handling in the framework
rg -nP --type=java 'GlobalConfigValidation|numberGreaterThan.*validate|enforc.*GlobalConfig' -A 2 -B 1 | head -80Repository: MatheMatrix/zstack
Length of output: 10645
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are other usages of nextLong() in the codebase to see defensive patterns
rg -nP --type=java 'nextLong\(' -B 2 -A 2 | head -100Repository: MatheMatrix/zstack
Length of output: 2596
框架已通过 @GlobalConfigValidation(numberGreaterThan = 1) 约束 PING_HOST_INTERVAL > 1，但在此代码路径中仍建议增加防御性检查。
PING_HOST_INTERVAL 的定义已包含 @GlobalConfigValidation(numberGreaterThan = 1) 注解（HostGlobalConfig.java 第 26-27 行），框架层面会防止配置为 ≤ 1 的值。不过，若防御层失效或存在绕过机制，当前代码仍会因 nextLong(bound <= 0) 而抛 IllegalArgumentException。建议保留一层防御性检查提升代码鲁棒性：
- long jitterMs = ThreadLocalRandom.current().nextLong(
- HostGlobalConfig.PING_HOST_INTERVAL.value(Long.class) * 1000);
+ long intervalMs = TimeUnit.SECONDS.toMillis(HostGlobalConfig.PING_HOST_INTERVAL.value(Long.class));
+ long jitterMs = intervalMs > 0 ? ThreadLocalRandom.current().nextLong(intervalMs) : 0;
+ t.startWithDelay(jitterMs, TimeUnit.MILLISECONDS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/main/java/org/zstack/compute/host/HostTrackImpl.java` around
lines 271 - 274, The code uses
ThreadLocalRandom.current().nextLong(HostGlobalConfig.PING_HOST_INTERVAL.value(Long.class)
* 1000) which can throw IllegalArgumentException if the computed bound is ≤ 0;
add a defensive check around
HostGlobalConfig.PING_HOST_INTERVAL.value(Long.class) (or the computed millis)
before calling nextLong and fall back to a safe positive bound (e.g., clamp to
1000ms or Math.max(1, interval) * 1000) so jitterMs is always computed from a
positive bound, then call t.startWithDelay(jitterMs, TimeUnit.MILLISECONDS);
reference symbols: HostGlobalConfig.PING_HOST_INTERVAL,
ThreadLocalRandom.current().nextLong, jitterMs, t.startWithDelay.
Comment on lines
+12
to
+30
| public PingLatencyEma(double alpha, int timeoutMultiplier) { | ||
| this.alpha = alpha; | ||
| this.timeoutMultiplier = timeoutMultiplier; | ||
| } | ||
|
|
||
| public void update(long latencyMs) { | ||
| if (emaMs < 0) { | ||
| emaMs = latencyMs; | ||
| } else { | ||
| emaMs = alpha * latencyMs + (1 - alpha) * emaMs; | ||
| } | ||
| } | ||
|
|
||
| public long computeAdaptiveTimeout(long globalTimeoutSeconds) { | ||
| if (emaMs <= 0) { | ||
| return globalTimeoutSeconds; | ||
| } | ||
| long emaBasedTimeout = (long) (emaMs * timeoutMultiplier / 1000) + 1; | ||
| return Math.min(Math.max(globalTimeoutSeconds, emaBasedTimeout), globalTimeoutSeconds * 3); |
There was a problem hiding this comment.
缺少输入边界校验，可能导致自适应超时计算失真。
Line 12-30 建议至少约束：alpha ∈ (0,1]、timeoutMultiplier > 0、latencyMs >= 0、globalTimeoutSeconds > 0。当前无校验时会产生不可预期的超时值。
建议修复
public PingLatencyEma(double alpha, int timeoutMultiplier) {
+ if (alpha <= 0 || alpha > 1) {
+ throw new IllegalArgumentException("alpha must be in (0, 1]");
+ }
+ if (timeoutMultiplier <= 0) {
+ throw new IllegalArgumentException("timeoutMultiplier must be greater than 0");
+ }
this.alpha = alpha;
this.timeoutMultiplier = timeoutMultiplier;
}
public void update(long latencyMs) {
+ if (latencyMs < 0) {
+ throw new IllegalArgumentException("latencyMs cannot be negative");
+ }
if (emaMs < 0) {
emaMs = latencyMs;
} else {
emaMs = alpha * latencyMs + (1 - alpha) * emaMs;
}
}
public long computeAdaptiveTimeout(long globalTimeoutSeconds) {
+ if (globalTimeoutSeconds <= 0) {
+ throw new IllegalArgumentException("globalTimeoutSeconds must be greater than 0");
+ }
if (emaMs <= 0) {
return globalTimeoutSeconds;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/main/java/org/zstack/compute/host/PingLatencyEma.java` around
lines 12 - 30, The constructor and methods lack input validation causing
incorrect adaptive timeout results; in PingLatencyEma's constructor validate
that alpha is >0 and <=1 and timeoutMultiplier >0 (throw
IllegalArgumentException), in update(long latencyMs) assert latencyMs >= 0
(reject or ignore negatives by throwing IllegalArgumentException) before using
it to set emaMs, and in computeAdaptiveTimeout(long globalTimeoutSeconds)
validate globalTimeoutSeconds > 0 (throw IllegalArgumentException) and guard
against emaMs overflow/negative values when computing emaBasedTimeout; update
references: PingLatencyEma(double alpha, int timeoutMultiplier), update(long
latencyMs), computeAdaptiveTimeout(long globalTimeoutSeconds), and field emaMs.
Comment on lines
+14
to
+15
| * AC: Jitter 测试 — 重连时间戳呈均匀分布 (非全部 t=0) | ||
| * Simulates the jitter logic from HostTrackImpl.trackHost() for 3000 hosts. |
There was a problem hiding this comment.
测试注释包含中文，违反仓库统一英文规范。
Line 14 和 Line 56 的注释建议改为英文，避免后续静态检查或规范审查失败。
建议修复
- * AC: Jitter 测试 — 重连时间戳呈均匀分布 (非全部 t=0)
+ * AC: Jitter test — reconnection timestamps should be uniformly distributed (not all at t=0).
@@
- * AC: Not all hosts start at t=0.
+ * AC: Not all hosts start at t=0.Also applies to: 56-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/test/java/org/zstack/compute/host/HostTrackJitterTest.java`
around lines 14 - 15, The test file HostTrackJitterTest.java contains Chinese
comments (e.g., the class/file header and the comment around the jitter
simulation), which violates the repo's English-only rule; update the comments at
the locations that reference the AC and the jitter simulation (previously
Chinese at lines near the class header and the simulation description) to clear,
correct English equivalents—keep the same intent (e.g., "AC: Jitter test —
reconnection timestamps should be uniformly distributed (not all t=0)" and
"Simulates the jitter logic from HostTrackImpl.trackHost() for 3000 hosts") so
static checks and style reviews pass.
Comment on lines
+8
to
+10
| /** | ||
| * AC: 合成延迟序列 [100, 200, 150, 300, 250]ms → EMA 产出正确自适应超时 | ||
| */ |
There was a problem hiding this comment.
请将中文注释改为英文注释。
Line 9 的 Javadoc 含中文，违反仓库统一规范，建议改为准确英文描述。
建议修改
- /**
- * AC: 合成延迟序列 [100, 200, 150, 300, 250]ms → EMA 产出正确自适应超时
- */
+ /**
+ * AC: Given synthetic latency sequence [100, 200, 150, 300, 250] ms,
+ * verify EMA values and adaptive timeout calculations are correct.
+ */As per coding guidelines “代码里不应当有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/test/java/org/zstack/compute/host/PingLatencyEmaTest.java` around
lines 8 - 10, Replace the Chinese Javadoc in the PingLatencyEmaTest class with
an accurate English description: update the comment that currently reads "AC:
合成延迟序列 [100, 200, 150, 300, 250]ms → EMA 产出正确自适应超时" to an English Javadoc (for
example: "AC: Synthetic latency sequence [100, 200, 150, 300, 250] ms → EMA
produces correct adaptive timeout") so the test class PingLatencyEmaTest and its
Javadoc conform to the repository's English-only documentation guideline.
Comment on lines
+72
to
+81
| public void startWithDelay(long delay, TimeUnit delayUnit) { | ||
| if (cancelled.get()) { | ||
| throw new CloudRuntimeException("cannot start a cancelled timer"); | ||
| } | ||
|
|
||
| cancel = thdf.submitTimeoutTask(this, delayUnit, delay); | ||
| if (logger.isTraceEnabled()) { | ||
| logger.trace(String.format("%s starts with delay %d %s", getName(), delay, delayUnit)); | ||
| } | ||
| } |
There was a problem hiding this comment.
为 startWithDelay 增加参数校验，避免异常在调度层才暴露。
Line 72-77 目前允许 delay < 0 或 delayUnit == null，会把错误延迟到 submitTimeoutTask 内部才触发，定位成本较高。
建议修复
public void startWithDelay(long delay, TimeUnit delayUnit) {
if (cancelled.get()) {
throw new CloudRuntimeException("cannot start a cancelled timer");
}
+ if (delayUnit == null) {
+ throw new CloudRuntimeException("delayUnit cannot be null");
+ }
+ if (delay < 0) {
+ throw new CloudRuntimeException("delay cannot be negative");
+ }
cancel = thdf.submitTimeoutTask(this, delayUnit, delay);
if (logger.isTraceEnabled()) {
logger.trace(String.format("%s starts with delay %d %s", getName(), delay, delayUnit));
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void startWithDelay(long delay, TimeUnit delayUnit) { | |
| if (cancelled.get()) { | |
| throw new CloudRuntimeException("cannot start a cancelled timer"); | |
| } | |
| cancel = thdf.submitTimeoutTask(this, delayUnit, delay); | |
| if (logger.isTraceEnabled()) { | |
| logger.trace(String.format("%s starts with delay %d %s", getName(), delay, delayUnit)); | |
| } | |
| } | |
| public void startWithDelay(long delay, TimeUnit delayUnit) { | |
| if (cancelled.get()) { | |
| throw new CloudRuntimeException("cannot start a cancelled timer"); | |
| } | |
| if (delayUnit == null) { | |
| throw new CloudRuntimeException("delayUnit cannot be null"); | |
| } | |
| if (delay < 0) { | |
| throw new CloudRuntimeException("delay cannot be negative"); | |
| } | |
| cancel = thdf.submitTimeoutTask(this, delayUnit, delay); | |
| if (logger.isTraceEnabled()) { | |
| logger.trace(String.format("%s starts with delay %d %s", getName(), delay, delayUnit)); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/zstack/core/thread/AsyncTimer.java` around lines 72 -
81, In startWithDelay(long delay, TimeUnit delayUnit) validate inputs before
using thdf: check that delayUnit is not null and delay is non-negative and if
invalid throw an IllegalArgumentException (or CloudRuntimeException consistent
with this class) with a clear message; keep the existing cancelled check and
existing behavior of assigning cancel = thdf.submitTimeoutTask(this, delayUnit,
delay) and the logger.trace call unchanged.
Resolves: ZSTAC-68709 Change-Id: Id1fe6b527d582473cee6a3bdbfdddc3ca03b4fae Signed-off-by: AlanJager <ye.zou@zstack.io>
MatheMatrix
force-pushed
the
sync/zstackio/fix/ZSTAC-68709@@2
branch
from
db299ea to
4ee663c
Compare
CI UTBuild uses stale core.jar without startWithDelay. Use thdf.submitTimeoutTask in HostTrackImpl directly. Resolves: ZSTAC-68709 Change-Id: Ibdff6b8ba7a7584d1a17f8b9958ea9c936880fc8
ZSTAC-69623: Data migration to update AccountResourceRefVO, SharedResourceVO, and IAM2ProjectResourceRefVO records for GPU specs from "PciDeviceSpecVO" to "GpuDeviceSpecVO". Resolves: ZSTAC-69623 Change-Id: I013a7120c7f5848b534bda311eb91ad008ecf4ef Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
scopeandchassisUuidcolumns to GpuDeviceVO for unified device scope trackingRelated MR
Test plan
Resolves: ZSTAC-68709
🤖 Generated with Claude Code
sync from gitlab !9242