<fix>[conf]: update global-error

[zstack-robot-2]

Resolves: ZSTAC-83157

Change-Id: I756c7073707a6468676c70696a7a746870767777

sync from gitlab !9557

[zstack-robot-1]

Comment from gitlab:

自上次添加REVIEWED标签(2026-04-14 16:53:12.000Z)后, 有新的COMMIT更新(2026-04-15 15:04:58.443Z), 所以移除了REVIEWED标签

[coderabbitai]

Actionable comments posted: 2

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

network/src/main/java/org/zstack/network/l3/IpRangeHelper.java (1)

25-25: ⚠️ Potential issue | 🟡 Minor

移除未使用的测试依赖导入。

org.junit.runner.Request.aClass 是 JUnit 测试框架的导入，不应出现在生产代码中。此导入未被使用，应当删除。

🐛 建议修复

-import static org.junit.runner.Request.aClass;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@network/src/main/java/org/zstack/network/l3/IpRangeHelper.java` at line 25,
删除生产代码中不应存在的测试框架静态导入；在 IpRangeHelper.java 中移除未使用的导入语句 "import static
org.junit.runner.Request.aClass;"，确保没有其他代码引用 aClass 并运行编译/检查以验证没有影响。

🟠 Major comments (22)

plugin/iscsi/src/main/java/org/zstack/iscsi/kvm/KvmIscsiNodeServer.java-244-249 (1)

244-249: ⚠️ Potential issue | 🟠 Major

回退分配后缺少有效性兜底校验

当前仅在分配前判断 hostId == 0，但分配后直接下发 cmd.hostId。如果回退返回仍无效（ref == null 或 hostId == 0），会把非法 hostId 发送给 agent。

🐛 建议修复

                         if (ref == null || ref.getHostId() == 0) {
                             logger.warn(String.format("not found hostId for hostUuid[%s] and primaryStorageUuid[%s]",
                                     param.getHostUuid(), param.getPrimaryStorage().getUuid()));
-                            ref = new ExternalHostIdGetter(999).getOrAllocateHostIdRef(
+                            ref = new ExternalHostIdGetter(HOST_ID_GETTER_RANGE).getOrAllocateHostIdRef(
                                     param.getHostUuid(), param.getPrimaryStorage().getUuid());
+                            if (ref == null || ref.getHostId() == 0) {
+                                trigger.fail(operr("failed to allocate valid hostId for hostUuid[%s], primaryStorageUuid[%s]",
+                                        param.getHostUuid(), param.getPrimaryStorage().getUuid()));
+                                return;
+                            }
                         }
@@
                         cmd.hostId = ref.getHostId();

Also applies to: 257-257

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@plugin/iscsi/src/main/java/org/zstack/iscsi/kvm/KvmIscsiNodeServer.java`
around lines 244 - 249, After calling
ExternalHostIdGetter.getOrAllocateHostIdRef in KvmIscsiNodeServer (the block
referencing param.getHostUuid() and param.getPrimaryStorage().getUuid()), add a
post-allocation validity check: ensure ref != null and ref.getHostId() != 0
before assigning cmd.hostId; if the allocation still yields an invalid ref, log
an error (including hostUuid and primaryStorageUuid) and abort/return an error
response instead of setting cmd.hostId and sending the cmd to the agent. Apply
the same guard to the other occurrence around the second getOrAllocateHostIdRef
call so no invalid hostId is ever sent.

header/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.java-48-49 (1)

48-49: ⚠️ Potential issue | 🟠 Major

避免在解析失败日志中输出完整 opaque 原文。

Line 48 直接拼接 vo.getOpaque()，当 opaque 含用户标识/业务敏感字段时会被日志泄露。建议仅记录固定文案或长度。

建议修复

-            logger.trace("LongJobProgressDetailBuilder: opaque is not a JSON object, skipping: " + vo.getOpaque(), e);
+            logger.trace("LongJobProgressDetailBuilder: opaque is not a JSON object, skipping", e);

🤖 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/core/progress/LongJobProgressDetailBuilder.java`
around lines 48 - 49, The trace log in LongJobProgressDetailBuilder currently
concatenates vo.getOpaque() into the message (logger.trace("... " +
vo.getOpaque(), e)), risking sensitive-data leakage; change the log to avoid
emitting the full opaque by either omitting it entirely (e.g., "opaque
omitted"), logging a fixed redact notice or a safe derived value (e.g., masked
or truncated substring and/or length only), and keep the exception e for stack
trace; replace the string concatenation with a single safe message (and use the
logger's parameterized API if available) to prevent accidental sensitive data
exposure from vo.getOpaque().

runMavenProfile-3-10 (1)

3-10: ⚠️ Potential issue | 🟠 Major

readlink -f 在 macOS 上不可用，导致仓库路径探测失败。

第 4 行使用了 GNU readlink -f。在 BSD/macOS 环境中该参数不可用，SCRIPT_DIR 会退化为当前工作目录，导致 .m2/repository 路径检测错误。脚本虽然有 Darwin 兼容性检查（第 323 行），但此处缺少相应的跨平台支持。建议改为 Bash 可移植写法。

🔧 建议修复

-# Auto-detect isolated .m2 in worktree
-SCRIPT_DIR="$(dirname "$(readlink -f "$0")")"
+# Auto-detect isolated .m2 in worktree (portable on Linux/macOS)
+SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd -P)"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@runMavenProfile` around lines 3 - 10, Replace the non-portable use of
readlink -f when setting SCRIPT_DIR with a POSIX/Bash-compatible resolution:
compute SCRIPT_DIR by changing to the directory of the invoking script (using
${BASH_SOURCE[0]} / dirname) and capturing pwd, so the check for
"$SCRIPT_DIR/.m2/repository" works on macOS/BSD; update the assignment that
creates SCRIPT_DIR and keep the existing MVN_LOCAL logic intact (references:
SCRIPT_DIR and MVN_LOCAL).

header/src/main/java/org/zstack/header/zwatch/ResourceMetricBindingExtensionPoint.java-87-87 (1)

87-87: 🛠️ Refactor suggestion | 🟠 Major

为接口方法补充 Javadoc，当前不满足仓库 Java 规范。

getResourceMetricBindings() 是扩展点核心契约，缺少 Javadoc 会降低实现一致性与可维护性。

📝 建议修复

+    /**
+     * Return all resource-to-metric binding definitions contributed by this extension.
+     */
     List<ResourceMetricBinding> getResourceMetricBindings();

As per coding guidelines “接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。”

🤖 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/zwatch/ResourceMetricBindingExtensionPoint.java`
at line 87, Add a Javadoc comment to the interface method
getResourceMetricBindings() in ResourceMetricBindingExtensionPoint describing
its contract: what resource metric bindings are returned, expected
format/semantics, nullability (never null; return empty list if none),
thread-safety or mutability guarantees, and responsibilities for implementors;
do not add any access modifiers to the method signature (leave it
package-private as an interface method). Ensure the Javadoc includes a `@return`
tag clarifying the returned List<ResourceMetricBinding> and any implementation
expectations.

plugin/expon/src/main/java/org/zstack/expon/ExponStorageFactory.java-50-53 (1)

50-53: ⚠️ Potential issue | 🟠 Major

getPreferBackupStorageTypes() 在字段未初始化时会抛出 NPE。

当 preferBackupStorageTypes 为 null 时，第 52 行的 new ArrayList<>(preferBackupStorageTypes) 会直接触发 NPE。调用方（ExternalPrimaryStorage.java 第 492 行）紧接着调用 retainAll() 方法而不做空值检查，该 NPE 会直接暴露。建议改为返回空集合以保证方法安全性。

建议修复

+import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
@@
     `@Override`
     public List<String> getPreferBackupStorageTypes() {
-        return new ArrayList<>(preferBackupStorageTypes);
+        return preferBackupStorageTypes == null
+                ? Collections.emptyList()
+                : new ArrayList<>(preferBackupStorageTypes);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@plugin/expon/src/main/java/org/zstack/expon/ExponStorageFactory.java` around
lines 50 - 53, The method getPreferBackupStorageTypes currently does new
ArrayList<>(preferBackupStorageTypes) which throws NPE if the field
preferBackupStorageTypes is null; change getPreferBackupStorageTypes to safely
handle a null field by returning an empty list when preferBackupStorageTypes ==
null (e.g., return Collections.emptyList() or new ArrayList<>()), so callers
like ExternalPrimaryStorage.retainAll() won't receive null and won't trigger an
NPE.

compute/src/main/java/org/zstack/compute/vm/MigrateVmLongJob.java-58-65 (1)

58-65: ⚠️ Potential issue | 🟠 Major

缺少跨节点一致的取消检查，已取消的长任务仍可能继续触发迁移。

这条新链路在取消备份任务、轮询 volume chain task，以及最终进入 doMigrate() 前都没有检查当前 jobUuid 的取消状态。cancel() 如果在另一个线程或管理节点执行，这里仍然会继续走到后续 callback，最终把迁移发出去。建议把 job.getUuid() 贯穿到这些异步 continuation 里，并在每个 checkpoint 用 DB-backed 的 isCancelled(jobUuid) 之类的框架级查询直接短路退出。

Based on learnings: For ZStack LongJob framework implementations, do not rely on in-memory volatile/cancellation flags for cancellation because start() and cancel() may execute on different threads/nodes with no built-in cross-thread/node signal propagation; add/use an isCancelled(jobUuid) query backed by the DB and call this cancellation check at async callback checkpoints.

Also applies to: 88-115, 117-137, 139-181

🤖 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/vm/MigrateVmLongJob.java` around
lines 58 - 65, Thread the long-job UUID (job.getUuid()) through all async
continuations in MigrateVmLongJob and add DB-backed cancellation checks
(isCancelled(jobUuid)) at each async checkpoint: when deciding to cancel
backupTaskLongJobUuids, inside cancelBackupLongJobsThenMigrate callbacks,
before/after polling volume chain tasks, and immediately prior to calling
doMigrate(); ensure cancelBackupLongJobsThenMigrate, any internal callback
handlers, and doMigrate accept the jobUuid (or access it) and
return/short-circuit if isCancelled(jobUuid) is true so cancelled jobs cannot
proceed across nodes/threads.

plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java-502-503 (1)

502-503: ⚠️ Potential issue | 🟠 Major

避免将 TLS 私钥以明文写入通用标签存储

libvirtTLSPrivateKey 当前通过 JsonLabel().createIfAbsent(...) 直接持久化私钥明文，存在高敏感凭据暴露面（DB 备份、运维查询、审计扩散）。建议改为受控加密存储（至少 KMS/密钥管理封装）并限制读取路径。

🤖 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 502
- 503, KVMHostFactory currently writes the libvirt TLS private key plaintext via
new JsonLabel().createIfAbsent(LIBVIRT_TLS_PRIVATE_KEY, key); instead, encrypt
or wrap the key with a KMS/secret-store before persisting and store only the
encrypted blob or a secret-ref (not raw key). Change the code path that creates
keyInv so it calls a KMS/secret-manager client to encrypt/store the key (or to
return a secret ID) and persist only the encrypted value/ID to
JsonLabelInventory (retain LIBVIRT_TLS_PRIVATE_KEY as the label name if needed),
and ensure any code paths that read the label decrypt via the same KMS and
restrict access to the decrypting routines.

plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.java-143-144 (1)

143-144: ⚠️ Potential issue | 🟠 Major

libvirt.tls.enabled 默认开启存在升级兼容风险

该开关是新增能力，但默认值设为 true 会直接改变既有集群迁移行为。建议默认关闭，并通过升级流程/显式配置逐步开启，避免老环境被动切换到 TLS 路径。

🔧 建议修改

-    `@GlobalConfigDef`(defaultValue = "true", type = Boolean.class, description = "enable TLS encryption for libvirt remote connections (migration)")
+    `@GlobalConfigDef`(defaultValue = "false", type = Boolean.class, description = "enable TLS encryption for libvirt remote connections (migration)")

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
143 - 144, The new GlobalConfig LIBVIRT_TLS_ENABLED (in class KVMGlobalConfig,
annotated by GlobalConfigDef) currently defaults to "true", which can change
existing cluster migration behavior; change its defaultValue to "false" to
preserve backward compatibility for upgrades and existing clusters, and ensure
any migration to TLS is done via explicit configuration or an upgrade path
(leave the config key name unchanged so admins can opt-in).

plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java-3194-3195 (1)

3194-3195: ⚠️ Potential issue | 🟠 Major

不要把 TLS 能力绑到 RECONNECT_HOST_RESTART_LIBVIRTD_SERVICE 上。

这里把“是否使用 libvirt TLS”以及“是否执行证书自检”都额外受 RECONNECT_HOST_RESTART_LIBVIRTD_SERVICE 控制。这样一旦用户关闭“重连时重启 libvirtd”，但仍启用了 LIBVIRT_TLS_ENABLED，迁移会直接退回非 TLS，且 SAN 变化也不会再被探测到，和配置语义不一致。

🔧 建议修改

- cmd.setUseTls(KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class)
-         && rcf.getResourceConfigValue(KVMGlobalConfig.RECONNECT_HOST_RESTART_LIBVIRTD_SERVICE, self.getUuid(), Boolean.class));
+ cmd.setUseTls(KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class));

 return CoreGlobalProperty.UNIT_TEST_ON
-        || !KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class)
-        || !rcf.getResourceConfigValue(
-                KVMGlobalConfig.RECONNECT_HOST_RESTART_LIBVIRTD_SERVICE,
-                self.getUuid(), Boolean.class);
+        || !KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class);

证书缺失时，保留后面 NEED_DEPLOY_TLS_CERT 分支去强制执行 ansible 和重启 libvirtd 即可。

</details>

Based on learnings：KVM host reconnect 时，只要 `KVMGlobalConfig.LIBVIRT_TLS_ENABLED` 为 true，就应“每次都检查” libvirt TLS 证书更新，并且失败时保持非阻塞继续流程。


Also applies to: 5718-5723

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java around lines 3194 -
3195, Remove the coupling of TLS usage to the
RECONNECT_HOST_RESTART_LIBVIRTD_SERVICE flag: change the logic around
cmd.setUseTls so it only depends on KVMGlobalConfig.LIBVIRT_TLS_ENABLED (via
KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class)) and do not AND it with
rcf.getResourceConfigValue(KVMGlobalConfig.RECONNECT_HOST_RESTART_LIBVIRTD_SERVICE,
self.getUuid(), Boolean.class); keep the existing NEED_DEPLOY_TLS_CERT handling
to trigger Ansible+libvirtd restart when certificates are missing, and ensure
certificate check failures are non-blocking so the reconnect proceeds even if
TLS cert refresh fails.

</details>

</blockquote></details>
<details>
<summary>conf/db/upgrade/V5.5.16__schema.sql-48-50 (1)</summary><blockquote>

`48-50`: _⚠️ Potential issue_ | _🟠 Major_

**升级脚本里的表/列引用要带 schema 和反引号。**

这一段直接写成了 `UPDATE VolumeSnapshotVO AS sp, PrimaryStorageVO AS ps`，既没有限定 `zstack` schema，也没有给表/列名加反引号。这个目录下的迁移脚本一直按固定 schema 执行，漏掉限定和转义后，默认 schema 不符合预期或遇到保留字时都容易出问题。

<details>
<summary>建议修改</summary>

```diff
-UPDATE VolumeSnapshotVO AS sp, PrimaryStorageVO AS ps
-SET sp.primaryStorageInstallPath = REPLACE(sp.primaryStorageInstallPath, '/dev/', 'sharedblock://')
-WHERE sp.primaryStorageUuid = ps.uuid AND ps.type = 'SharedBlock' AND sp.volumeType = 'Memory' AND sp.primaryStorageInstallPath LIKE '/dev/%';
+UPDATE `zstack`.`VolumeSnapshotVO` AS `sp`, `zstack`.`PrimaryStorageVO` AS `ps`
+SET `sp`.`primaryStorageInstallPath` = REPLACE(`sp`.`primaryStorageInstallPath`, '/dev/', 'sharedblock://')
+WHERE `sp`.`primaryStorageUuid` = `ps`.`uuid`
+  AND `ps`.`type` = 'SharedBlock'
+  AND `sp`.`volumeType` = 'Memory'
+  AND `sp`.`primaryStorageInstallPath` LIKE '/dev/%';

Based on learnings “In ZStack, the database schema name is fixed as 'zstack'. For SQL upgrade scripts under conf/db/upgrade, continue using TABLE_SCHEMA = 'zstack' and reference tables as zstack.UsedIpVO”, and as per coding guidelines “所有表名和列名必须使用反引号包裹”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@conf/db/upgrade/V5.5.16__schema.sql` around lines 48 - 50, Update the SQL to
reference the fixed schema and use backticks for all table and column
identifiers: change the unqualified tables VolumeSnapshotVO and PrimaryStorageVO
to zstack.`VolumeSnapshotVO` and zstack.`PrimaryStorageVO`, and wrap column
names like `primaryStorageInstallPath`, `primaryStorageUuid`, `uuid`, `type`,
and `volumeType` in backticks in the UPDATE, SET and WHERE clauses; keep the
same REPLACE logic and predicates but ensure TABLE_SCHEMA = 'zstack' is used
consistently and all identifiers are escaped to avoid schema/reserved-word
issues.

compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java-56-61 (1)

56-61: ⚠️ Potential issue | 🟠 Major

不要按操作者账号直接跳过租户配额。

这里基于 spec.getAccountUuid() 提前 return，会让管理员代普通账号创建/扩容 VM 时直接绕过该租户的 quota。下面 Line 64-68 已经按资源 owner 是否为 admin 做了豁免；这段新增早退把“owner 是 admin”扩大成了“operator 是 admin”，行为面会明显变宽。

建议修改

-        // skip checkquota if the operator is admin
-        String currentAccountUuid = spec.getAccountUuid();
-        if (currentAccountUuid != null && AccountConstant.isAdminPermission(currentAccountUuid)) {
-            next(candidates);
-            return;
-        }
-
         final String vmInstanceUuid = spec.getVmInstance().getUuid();
         final String accountUuid = Account.getAccountUuidOfResource(vmInstanceUuid);
         if (accountUuid == null || AccountConstant.isAdminPermission(accountUuid)) {
             next(candidates);
             return;

As per coding guidelines “向后兼容原则 - 之前的代码产生的行为不要直接去改动，即使是 bug 也可能用户有依赖，不应直接改动，可以考虑升级用户一套行为、新安装用户一套行为，开关控制等”.

🤖 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/allocator/QuotaAllocatorFlow.java`
around lines 56 - 61, The code prematurely skips quota checking based on the
operator's account (using spec.getAccountUuid() and
AccountConstant.isAdminPermission()) which broadens the exemption; remove this
operator-based early return and rely on the existing resource-owner admin
exemption already implemented later (do not call next(candidates) based on
spec.getAccountUuid()); if the original intent was to exempt when the resource
owner is admin, change the check to use the resource owner account UUID (the
same owner-based check used later) instead of the operator's account.

network/src/main/java/org/zstack/network/l2/L2NetworkApiInterceptor.java-127-131 (1)

127-131: ⚠️ Potential issue | 🟠 Major

请先标准化 physicalInterface，不要只做空白校验。

现在 " eth0 " 这类值会通过校验并原样落到后续流程里；后面按 physicalInterface 做冲突判断、查重或桥接时，它会被当成和 "eth0" 不同的接口。这里应该先把用户输入 trim() 后回写到 msg，再做 Linux bridge 的必填校验。

建议修改

-        if (L2NetworkConstant.VSWITCH_TYPE_LINUX_BRIDGE.equals(msg.getvSwitchType())
-                && (msg.getPhysicalInterface() == null || msg.getPhysicalInterface().trim().isEmpty())) {
+        if (msg.getPhysicalInterface() != null) {
+            msg.setPhysicalInterface(msg.getPhysicalInterface().trim());
+        }
+
+        if (L2NetworkConstant.VSWITCH_TYPE_LINUX_BRIDGE.equals(msg.getvSwitchType())
+                && StringUtils.isBlank(msg.getPhysicalInterface())) {
             throw new ApiMessageInterceptionException(argerr(ORG_ZSTACK_NETWORK_L2_10021,
                     "physicalInterface is required when vSwitchType is [%s]", msg.getvSwitchType()));
         }

As per coding guidelines “注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@network/src/main/java/org/zstack/network/l2/L2NetworkApiInterceptor.java`
around lines 127 - 131, Trim and normalize the physicalInterface value on the
incoming message before validation: in L2NetworkApiInterceptor where you check
msg.getvSwitchType() and msg.getPhysicalInterface(), call trim() on the string,
write it back into the message (e.g. msg.setPhysicalInterface(trimmed)) and then
perform the required empty check and throw ApiMessageInterceptionException if
missing; this ensures values like " eth0 " are normalized to "eth0" prior to
later conflict/duplication/bridge logic.

header/src/main/java/org/zstack/header/core/external/service/APIDeleteExternalServiceConfigurationMsg.java-15-19 (1)

15-19: 🛠️ Refactor suggestion | 🟠 Major

REST 资源路径建议改成复数。

这个 API 还是新增态，现在把公开契约定成单数 configuration，后面再修会更难兼容。这里建议直接改成复数路径，并同步更新对应的 API 文档/模板。

♻️ 建议修改

 `@RestRequest`(
-        path = "/external/service/configuration/{uuid}",
+        path = "/external/service/configurations/{uuid}",
         responseClass = APIDeleteExternalServiceConfigurationEvent.class,
         method = HttpMethod.DELETE
 )

As per coding guidelines path: 针对资源使用复数形式。

🤖 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/core/external/service/APIDeleteExternalServiceConfigurationMsg.java`
around lines 15 - 19, Change the REST resource path in the
APIDeleteExternalServiceConfigurationMsg `@RestRequest` from the singular
"/external/service/configuration/{uuid}" to the plural
"/external/service/configurations/{uuid}" to follow the pluralized resource
guideline; update any related API documentation/templates and tests that
reference the old path (search for APIDeleteExternalServiceConfigurationMsg and
the string "/external/service/configuration") so the responseClass
APIDeleteExternalServiceConfigurationEvent and method remain unchanged.

network/src/main/java/org/zstack/network/l3/L3BasicNetwork.java-587-594 (1)

587-594: ⚠️ Potential issue | 🟠 Major

需要显式处理"可解析但范围非法"的 IPv6 前缀。

当前代码只捕获了 NumberFormatException。像 129、-1 这类能通过 Integer.parseInt() 的值，如果在 IPv6NetworkUtils.getFormalNetmaskOfNetworkCidr(...) 内部抛出其他运行时异常（如 IllegalArgumentException），当前 catch 块无法捕获，会导致整个分配链路失败，而不是按预期进行 warn 并使用默认前缀长度。

根据 ZStack 的分层验证设计（API 拦截器层格式验证，业务逻辑层范围验证），L3BasicNetwork 作为业务逻辑层应当补充范围校验。特别是该模块作为通用网络服务，可能接收来自不同调用者的 AllocateIpMsg，不是所有路径都经过 VmInstanceApiInterceptor 的验证。

建议修改

             if (msg.getIpv6Prefix() != null) {
                 try {
                     prefixLength = Integer.parseInt(msg.getIpv6Prefix());
+                    if (prefixLength < 0 || prefixLength > 128) {
+                        throw new IllegalArgumentException(String.format("invalid IPv6 prefix length[%s]", msg.getIpv6Prefix()));
+                    }
-                    overrideNetmask = IPv6NetworkUtils.getFormalNetmaskOfNetworkCidr(ip.getIp() + "/" + msg.getIpv6Prefix());
-                } catch (NumberFormatException e) {
+                    overrideNetmask = IPv6NetworkUtils.getFormalNetmaskOfNetworkCidr(ip.getIp() + "/" + prefixLength);
+                } catch (NumberFormatException | IllegalArgumentException e) {
                     logger.warn(String.format("failed to parse prefix length[%s], ignore it and use the default prefix length of the ip range",
                             msg.getIpv6Prefix()));
+                    prefixLength = null;
                 }
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@network/src/main/java/org/zstack/network/l3/L3BasicNetwork.java` around lines
587 - 594, In L3BasicNetwork where AllocateIpMsg IPv6 prefix is parsed (the
block using msg.getIpv6Prefix(), Integer.parseInt and
IPv6NetworkUtils.getFormalNetmaskOfNetworkCidr), add explicit range validation
and error handling so that parseable but invalid prefixes (e.g. -1, 129) do not
propagate exceptions: after Integer.parseInt, verify the prefix is within 0..128
and if not log a warning and fall back to the default prefix length;
additionally catch/handle IllegalArgumentException (or any runtime exception
thrown by IPv6NetworkUtils.getFormalNetmaskOfNetworkCidr) to log a warning and
use the default prefix as currently intended, ensuring the allocation flow
continues rather than failing.

plugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerBase.java-640-645 (1)

640-645: ⚠️ Potential issue | 🟠 Major

这里的 CIDR 过滤会漏掉 IPv6 候选。

当前只校验 VmNicVO.getIp()，但这个字段并不能覆盖 NIC 的实际候选地址族。对 IPv6-only NIC，或按 IPv6 选后端的场景，nic.getIp() 可能是 null/IPv4，结果是 IPv6 已经落在 L3 CIDR 外的 NIC 仍会被返回为候选。

建议按当前候选使用的 IP 版本去检查对应 UsedIpVO，不要固定看主 IPv4。否则这次“过滤掉 CIDR 外后端”的目标在 IPv6 场景下会失效。

Also applies to: 1058-1063

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@plugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerBase.java`
around lines 640 - 645, The CIDR filter in LoadBalancerBase that uses
nicVOS.stream().filter(...) and VmNicVO.getIp() misses IPv6 candidates; change
the filter to determine the actual candidate IP for that NIC (look up the
corresponding UsedIpVO(s) or the NIC's candidate IP field rather than relying on
VmNicVO.getIp()), pick the IP address actually being used for backend selection
(IPv4 or IPv6), and call IpRangeHelper.isIpInL3NetworkCidr(candidateIp,
nic.getL3NetworkUuid()) against that IP; apply the same fix to the similar block
around lines 1058-1063 so IPv6-only NICs are correctly validated.

rest/src/main/java/org/zstack/rest/RestServer.java-411-415 (1)

411-415: ⚠️ Potential issue | 🟠 Major

webhook 默认语言分支不应跳过 localizeErrorCode() 的兜底保障。

当前代码在第 411-415 行仅当 locale != DEFAULT_LOCALE 时才调用 localizeErrorCode()，这将默认语言 webhook 的 message 字段填充建立在"evt.getError() 必定由 Platform.err() 预填充"的隐含假设上。一旦此假设不成立（例如某些代码路径直接构造 ErrorCode 或通过反序列化创建），默认语言 webhook 就会把 message == null 原样序列化，与普通 REST 响应路径建立的兜底语义产生不一致。建议对所有语言统一执行 localizeErrorCode()，以确保消息字段总是有值。

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rest/src/main/java/org/zstack/rest/RestServer.java` around lines 411 - 415,
The webhook handling currently only calls
i18nService.localizeErrorCode(evt.getError(), locale) when resolveLocale()
returns a non-default locale, which can leave evt.getError().message null for
default-locale webhooks; change RestServer.java to always invoke
i18nService.localizeErrorCode(evt.getError(), locale) (using resolveLocale())
regardless of LocaleUtils.DEFAULT_LOCALE so the ErrorCode returned by
evt.getError() is consistently populated (even if originally created directly or
via deserialization), preserving the same fallback semantics used elsewhere.

test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/PublicNetworkChangeVmIpOutsideCidrCase.groovy-371-395 (1)

371-395: ⚠️ Potential issue | 🟠 Major

这里的断言方向反了。

dhcpTriggered 在两个 DHCP 回调里都会被置成 true，但最后却断言 !dhcpTriggered。这样一来，只要 DHCP 后端按预期运行，这个用例就会失败；而从断言消息看，你这里本来就是想确认 DHCP 后端被触发、但 outside-range IP 没进 payload。

建议修改

-        assert !dhcpTriggered : "expected DHCP backend to run on a DHCP-enabled public L3"
+        assert dhcpTriggered : "expected DHCP backend to run on a DHCP-enabled public L3"
         assert !dhcpAppliedForOutsideIp : "DHCP should NOT include outside-range IP 10.0.3.50 even on DHCP-enabled public L3"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/PublicNetworkChangeVmIpOutsideCidrCase.groovy`
around lines 371 - 395, The two booleans dhcpTriggered and
dhcpAppliedForOutsideIp are checked incorrectly: dhcpTriggered is set to true in
the FlatDhcpBackend callbacks but the test asserts !dhcpTriggered; change the
assertions after startVmInstance (the post-start checks) to assert dhcpTriggered
(confirm the DHCP backend was invoked) and assert !dhcpAppliedForOutsideIp
(confirm the outside-range IP 10.0.3.50 was not included in the DHCP payload) so
the test validates that DHCP ran but did not include the outside IP.

core/src/main/java/org/zstack/core/externalservice/ExternalServiceManagerImpl.java-93-99 (1)

93-99: ⚠️ Potential issue | 🟠 Major

handleMessage 需要补上 @MessageSafe。

现在这里不仅分发 API 消息，还新增了 ApplyExternalServiceConfigurationMsg 的本地消息路径；如果处理过程中抛异常，消息总线可能拿不到期望的错误回复，直接破坏 reply/error 语义。

建议修改

+import org.zstack.core.cloudbus.MessageSafe;
+
     `@Override`
+    `@MessageSafe`
     public void handleMessage(Message msg) {
         if (msg instanceof APIMessage) {
             handleApiMessage((APIMessage) msg);
         }  else {
             handleLocalMessage(msg);

Based on learnings: In ZStack message handlers, ensure your handleMessage method is annotated with MessageSafe (org.zstack.core.cloudbus.MessageSafe). This enables MessageSafeAspect.aj to automatically convert exceptions thrown by the handler into appropriate reply errors. Note that ExceptionSafeAspect.aj only applies to methods annotated with ExceptionSafe and will only log/swallow exceptions without sending a reply—so missing MessageSafe can break reply/error semantics for the message bus.

🤖 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/externalservice/ExternalServiceManagerImpl.java`
around lines 93 - 99, Add the org.zstack.core.cloudbus.MessageSafe annotation to
the ExternalServiceManagerImpl.handleMessage(Message msg) method so the
MessageSafeAspect will convert thrown exceptions into proper reply/errors for
both APIMessage and local messages (including
ApplyExternalServiceConfigurationMsg); update imports to include MessageSafe and
ensure the annotation is applied to the handleMessage method signature so the
message bus reply/error semantics are preserved.

compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java-3714-3717 (1)

3714-3717: ⚠️ Potential issue | 🟠 Major

补齐删除静态 IP 时的 DNS 清理。

这里已经把 STATIC_DNS 写入了多个设置路径，但同文件 Line 3489-3505 的 APIDeleteVmStaticIp 仍只删除 static-ip tag。这样用户先设置静态 DNS、再删除该 L3 上最后一个静态 IP 后，旧 DNS 还会残留并继续覆盖 DHCP DNS；目前只有 detach/change-nic-network 分支补了对应清理。

可参考的修复方式

             public void run(SyncTaskChain chain) {
                 APIDeleteVmStaticIpEvent evt = new APIDeleteVmStaticIpEvent(msg.getId());
+                StaticIpOperator ipOperator = new StaticIpOperator();
                 if (msg.getStaticIp() == null) {
-                    new StaticIpOperator().deleteStaticIpByVmUuidAndL3Uuid(self.getUuid(), msg.getL3NetworkUuid());
+                    ipOperator.deleteStaticIpByVmUuidAndL3Uuid(self.getUuid(), msg.getL3NetworkUuid());
+                    ipOperator.deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), msg.getL3NetworkUuid());
                 } else {
-                    new StaticIpOperator().deleteStaticIpByVmUuidAndL3Uuid(self.getUuid(), msg.getL3NetworkUuid(), IPv6NetworkUtils.ipv6AddessToTagValue(msg.getStaticIp()));
+                    ipOperator.deleteStaticIpByVmUuidAndL3Uuid(
+                            self.getUuid(), msg.getL3NetworkUuid(),
+                            IPv6NetworkUtils.ipv6AddessToTagValue(msg.getStaticIp()));
+                    List<String> remainingIps = ipOperator.getStaticIpbyVmUuid(self.getUuid())
+                            .getOrDefault(msg.getL3NetworkUuid(), Collections.emptyList());
+                    if (remainingIps.isEmpty()) {
+                        ipOperator.deleteStaticDnsByVmUuidAndL3Uuid(self.getUuid(), msg.getL3NetworkUuid());
+                    }
                 }
-                new StaticIpOperator().setIpChange(self.getUuid(), msg.getL3NetworkUuid());
+                ipOperator.setIpChange(self.getUuid(), msg.getL3NetworkUuid());
                 bus.publish(evt);
                 chain.next();
             }

Also applies to: 3763-3766, 6367-6372

🤖 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/vm/VmInstanceBase.java` around lines
3714 - 3717, When deleting a VM static IP in VmInstanceBase (APIDeleteVmStaticIp
handling), also clear the STATIC_DNS tag so leftover DNS entries don't persist;
replace the current logic that only removes the static-ip tag by invoking the
StaticIpOperator DNS cleanup (e.g. call the same StaticIpOperator method used to
set DNS such as setStaticDns(..., null) or a removeStaticDns(...) helper) for
the VM UUID and the target L3 network UUID; make the identical change for the
other two deletion sites referenced around the other occurrences (the blocks at
the locations noted in the review) so all branches that remove static IPs also
remove STATIC_DNS.

compute/src/main/java/org/zstack/compute/vm/StaticIpOperator.java-362-367 (1)

362-367: ⚠️ Potential issue | 🟠 Major

不要在通用 checkIpAvailability() 里全局关闭 ipRangeCheck。

把 cmsg.setIpRangeCheck(false) 固化在底层 helper 里后，validateSystemTagInApiMessage() 和 validateSystemTag() 的所有调用方都会接受 L3 CIDR 外的 static-ip tag。当前文件里没有再按 VM 类型收口，这会把“仅普通 user VM 支持 outside-range”的放宽扩散到创建/挂载等通用路径。建议把这个开关提升到调用方：通用 helper 默认保持 range check，只有明确支持 outside-range 的 user-VM 路径再传 false。 Based on learnings "In VirtualRouterManagerImpl.java (plugin/virtualRouterProvider/src/main/java/org/zstack/network/service/virtualrouter/VirtualRouterManagerImpl.java), ApplianceVMs (including virtual router VMs) are by design only allowed to have IPs within an IP range (ipRangeUuid is always non-null for appliance VM NICs). The outside-IP-range feature introduced in PR #3513 only applies to regular user VMs, not ApplianceVMs."

可参考的收敛方式

-public void checkIpAvailability(String l3Uuid, String ip) {
+public void checkIpAvailability(String l3Uuid, String ip) {
+    checkIpAvailability(l3Uuid, ip, true);
+}
+
+public void checkIpAvailability(String l3Uuid, String ip, boolean ipRangeCheck) {
     CheckIpAvailabilityMsg cmsg = new CheckIpAvailabilityMsg();
     cmsg.setIp(ip);
     cmsg.setL3NetworkUuid(l3Uuid);
-    cmsg.setIpRangeCheck(false);
+    cmsg.setIpRangeCheck(ipRangeCheck);
     bus.makeLocalServiceId(cmsg, L3NetworkConstant.SERVICE_ID);
     MessageReply r = bus.call(cmsg);
     ...
 }

然后仅在明确支持 outside-range 的 user-VM 调用点传 false，其余现有通用校验路径继续走默认值。

🤖 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/vm/StaticIpOperator.java` around
lines 362 - 367, The helper checkIpAvailability currently forces
cmsg.setIpRangeCheck(false), which disables IP-range validation globally; remove
that hard-coded set and make the helper preserve the default (true). Instead
expose an explicit parameter or an overloaded method (e.g.,
checkIpAvailability(String l3Uuid, String ip, boolean ipRangeCheck) or
checkIpAvailabilityAllowOutsideRange) so callers that deliberately allow
outside-range (user-VM paths) can pass false, while generic callers (and
validateSystemTagInApiMessage()/validateSystemTag() callers) continue to use the
default true; update calling sites in user-VM-specific flows to pass false and
leave all other call sites unchanged.

compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java-697-717 (1)

697-717: ⚠️ Potential issue | 🟠 Major

APISetVmStaticIpMsg 这里把“不可用”缩窄成了“已占用”。

normalizedIp / normalizedIp6 解析完成后只调用了 checkIpOccupied(...)。如果地址被保留、被 L3 侧策略拒绝，或者还有其它 CheckIpAvailability 才能覆盖的失败原因，这里都会直接漏过去。建议在 checkIpOccupied(...) 前补一次 staticIpOp.checkIpAvailability(...)，保持和 system-tag 路径一致。

🤖 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/vm/VmInstanceApiInterceptor.java`
around lines 697 - 717, After resolving normalizedIp/normalizedIp6, you're only
calling checkIpOccupied(...) which misses other availability failures; call
staticIpOp.checkIpAvailability(...) before checkIpOccupied for both IPv4 and
IPv6 paths (i.e., after resolveIpv4NetmaskAndGateway(...) and after
resolveIpv6PrefixAndGateway(...)). Use the same parameters/context as used
elsewhere (pass the resolved address, msg.getL3NetworkUuid(), nicRole and the
existingIpv4/existingIpv6 flags) so staticIpOp.checkIpAvailability(...) runs and
can surface reserved/policy-rejected or other availability errors before
checkIpOccupied(...) runs.

compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java-381-400 (1)

381-400: ⚠️ Potential issue | 🟠 Major

msg.getStaticIp() 没有走完整的 IP 可用性校验。

这里先对 nicNetworkInfo 做了 validateIpAvailability(...)，但 msg.getStaticIp() 是之后才塞进 staticIps 的，后面只剩 checkIpsOccupied(...)。这样会漏掉格式错误、保留 IP，或其它只能由 L3 CheckIpAvailability 返回的不可用原因。建议把 msg.getStaticIp() 先归一化并并入 nicNetworkInfo，再复用同一条校验/补全路径。

🤖 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/vm/VmInstanceApiInterceptor.java`
around lines 381 - 400, msg.getStaticIp() is only added to staticIps and
validated via checkIpsOccupied(...), skipping format/reserved/L3-specific checks
done by validateIpAvailability(...) for nicNetworkInfo; instead, normalize and
merge msg.getStaticIp() into the corresponding nicNetworkInfo entry (the same
structure used by validateIpAvailability) before calling
validateIpAvailability(...), then proceed with the unified loop that uses
checkIpsOccupied(...) and sets msg.setRequiredIpMap(...); ensure you update
where staticIps is populated so msg.getStaticIp() goes through the same
validation/completion path as other NIC IPs (refer to msg.getStaticIp(),
staticIps, nicNetworkInfo, validateIpAvailability(...), checkIpsOccupied(...),
newAddedL3Uuids).

🟡 Minor comments (10)

header/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.java-134-137 (1)

134-137: ⚠️ Potential issue | 🟡 Minor

字符串数字解析建议先 trim()。

Line 136 对字符串直接 Double.parseDouble，像 " 42 " 会误判为非法，导致字段丢失。

建议修复

-                return Double.parseDouble((String) val);
+                return Double.parseDouble(((String) val).trim());

🤖 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/core/progress/LongJobProgressDetailBuilder.java`
around lines 134 - 137, In LongJobProgressDetailBuilder (the code path where val
instanceof String), trim the string before parsing so values like " 42 " are
accepted; replace Double.parseDouble((String) val) with parsing of ((String)
val).trim(), keep the existing NumberFormatException catch behavior, and ensure
the downstream logic uses the parsed double when successful.

test/src/test/groovy/org/zstack/test/integration/network/l2network/AttachL2NetworkCase.groovy-107-135 (1)

107-135: ⚠️ Potential issue | 🟡 Minor

把全空白输入也纳入回归用例。

当前只覆盖了 null 和 ""，但这次生产代码真正新增的是 trim().isEmpty() 分支；如果没有 " \n" 这类 case，最关键的回归点还没被锁住。

建议补一个用例

     expect(AssertionError.class) {
         createL2NoVlanNetwork {
             name = "test-empty-physical-interface"
             zoneUuid = zone.uuid
             physicalInterface = ""
         }
     }
+
+    expect(AssertionError.class) {
+        createL2NoVlanNetwork {
+            name = "test-blank-physical-interface"
+            zoneUuid = zone.uuid
+            physicalInterface = " \n "
+        }
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@test/src/test/groovy/org/zstack/test/integration/network/l2network/AttachL2NetworkCase.groovy`
around lines 107 - 135, Add assertions that a physicalInterface value consisting
only of whitespace is rejected: in AttachL2NetworkCase
testCreateL2NetworkWithoutPhysicalInterface(), extend the existing
expect(AssertionError.class) blocks for createL2NoVlanNetwork and
createL2VlanNetwork to include calls where physicalInterface is set to a
whitespace-only string (e.g. " \n" or "\t "), using the same pattern as the
existing null/empty cases so the new trim().isEmpty() branch is covered for both
createL2NoVlanNetwork and createL2VlanNetwork.

header/src/main/java/org/zstack/header/core/external/service/APIQueryExternalServiceConfigurationMsg.java-17-18 (1)

17-18: ⚠️ Potential issue | 🟡 Minor

REST 路径建议改为资源复数形式。

当前 path/optionalPaths 使用了单数 configuration，与仓库 API 规范不一致，建议改为复数资源名，避免后续接口风格不统一。

♻️ 建议修改

-        path = "/external/service/configuration",
-        optionalPaths = {"/external/service/configuration/{uuid}"},
+        path = "/external/services/configurations",
+        optionalPaths = {"/external/services/configurations/{uuid}"},

As per coding guidelines: path 针对资源使用复数形式。

🤖 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/core/external/service/APIQueryExternalServiceConfigurationMsg.java`
around lines 17 - 18, Update the REST paths in
APIQueryExternalServiceConfigurationMsg: change the path and optionalPaths
values from "/external/service/configuration" and
"/external/service/configuration/{uuid}" to use the plural resource name (e.g.
"/external/service/configurations" and
"/external/service/configurations/{uuid}") so the route follows the repository
API convention; modify the path and optionalPaths attributes in the
APIQueryExternalServiceConfigurationMsg class accordingly.

header/src/main/java/org/zstack/header/core/external/service/APIUpdateExternalServiceConfigurationMsg.java-15-15 (1)

15-15: ⚠️ Potential issue | 🟡 Minor

更新接口路径建议使用复数资源名。

/external/service/configuration/{uuid} 建议改为复数形式，和仓库 API 设计规范保持一致。

♻️ 建议修改

-        path = "/external/service/configuration/{uuid}",
+        path = "/external/services/configurations/{uuid}",

As per coding guidelines: path 针对资源使用复数形式。

🤖 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/core/external/service/APIUpdateExternalServiceConfigurationMsg.java`
at line 15, 在 APIUpdateExternalServiceConfigurationMsg 类中将 path 的单数资源名改为复数以符合仓库
API 设计规范：把 path = "/external/service/configuration/{uuid}" 修改为复数形式（例如
"/external/services/configurations/{uuid}"），确保路由与资源命名一致并同步更新相关文档或调用处使用的路径常量。

test/src/test/groovy/org/zstack/test/unittest/identity/TestAPIQueryAccountMsgRBACCase.java-36-37 (1)

36-37: ⚠️ Potential issue | 🟡 Minor

contains(apiName) 无法覆盖通配符规则，测试可能出现误判。

当前实现只做精确字符串匹配；如果 normalAPIs 里是通配符/模式，这个测试会错误通过，无法捕获你在断言文案中提到的风险。建议改为复用 RBAC 运行时一致的 API 匹配逻辑来判断“是否可被 normalAPIs 命中”。

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@test/src/test/groovy/org/zstack/test/unittest/identity/TestAPIQueryAccountMsgRBACCase.java`
around lines 36 - 37, The test uses exact-string contains(apiName) against
p.getNormalAPIs() which misses wildcard/pattern rules and can give false
positives; replace the exact match with the same runtime API-matching logic used
by RBAC (apply the RBAC API matcher to each pattern in p.getNormalAPIs() and
p.getAdminOnlyAPIs() instead of contains), e.g. change the anyMatch predicate
that presently calls p.getNormalAPIs().contains(apiName) to iterate patterns and
call the RBAC matcher (or add a helper like RBAC.matchesApi(pattern, apiName))
so both normal and admin-only checks use consistent pattern matching for apiName
when evaluating RBAC.permissions, getNormalAPIs and getAdminOnlyAPIs.

test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/securitygroup/ChangeSecurityGroupRuleCase.groovy-234-237 (1)

234-237: ⚠️ Potential issue | 🟡 Minor

取消异常断言后缺少成功态校验，测试意图不完整。

Line [234]-[237] 与 Line [308]-[311] 现在只验证“不抛异常”，但没有断言优先级更新结果，回归保护不足。

✅ 建议补充断言

         changeSecurityGroupRule {
             uuid = rule_1.uuid
             priority = 6
         }
+        assert Q.New(SecurityGroupRuleVO).eq(SecurityGroupRuleVO_.uuid, rule_1.uuid).find().priority == 6
@@
         changeSecurityGroupRule {
             uuid = rule1.uuid
             priority = 3
         }
+        assert Q.New(SecurityGroupRuleVO).eq(SecurityGroupRuleVO_.uuid, rule1.uuid).find().priority == 3

Also applies to: 308-311

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/securitygroup/ChangeSecurityGroupRuleCase.groovy`
around lines 234 - 237, After calling changeSecurityGroupRule (the block that
sets uuid = rule_1.uuid and priority = 6), add an explicit success assertion
that the rule's priority was actually updated: fetch the rule by rule_1.uuid
(e.g., via the test helper/query method used elsewhere in this suite) and assert
its priority equals 6 (use the same assertion style as other tests, e.g.,
assertEquals or assert). Do the same for the other changeSecurityGroupRule
invocation later in the test so each mutation is followed by a verification that
the rule's priority was changed.

test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/securitygroup/AddSecurityGroupRuleOptimizedCase.groovy-611-615 (1)

611-615: ⚠️ Potential issue | 🟡 Minor

这段成功路径少了结果断言。

这里已经不再期待抛错了，但调用后没有校验 rule_12 和 ingressRule 是否都被创建、以及它们的 priority 是否真的是 12。这样即使接口忽略了优先级、只落了一条规则，测试也会继续通过。

建议补和上一个 priority = 13 分支同等级别的断言，把这次行为变更真正锁住。

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/securitygroup/AddSecurityGroupRuleOptimizedCase.groovy`
around lines 611 - 615, 在调用 addSecurityGroupRule 为 sg3 设置 rules=[rule_12,
ingressRule], priority=12 后补上断言：调用 addSecurityGroupRule 返回并查询到两条规则均已创建（与 rule_12
和 ingressRule 对应），并且两条规则的 priority 字段都为 12；参照之前 priority = 13 分支的断言逻辑（相同层级的存在性与
priority 校验），在测试中对 sg3 的规则列表进行断言以防接口忽略优先级或只落一条规则。

test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/dhcp/VerifyPrepareDhcpWhenReconnectHostCase.groovy-248-255 (1)

248-255: ⚠️ Potential issue | 🟡 Minor

不要忽略 await() 的返回值。

这里超时后会自动继续返回 rsp，测试可能把“第一批其实没有被正常释放”的场景误判成通过。建议直接断言等待成功。

建议修改

         env.afterSimulator(FlatDhcpBackend.BATCH_APPLY_DHCP_PATH) { rsp, HttpEntity<String> e1 ->
             FlatDhcpBackend.BatchApplyDhcpCmd cmd = JSONObjectUtil.toObject(e1.body, FlatDhcpBackend.BatchApplyDhcpCmd.class)
             batchCmds.add(cmd)
             if (batchCmds.size() == 1) {
                 firstBatchArrived.countDown()
-                releaseFirstBatch.await(10, TimeUnit.SECONDS)
+                assert releaseFirstBatch.await(10, TimeUnit.SECONDS)
             }
             return rsp
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/dhcp/VerifyPrepareDhcpWhenReconnectHostCase.groovy`
around lines 248 - 255, 在 env.afterSimulator 回调中对 releaseFirstBatch.await(10,
TimeUnit.SECONDS) 的返回值不要忽略：替换为断言等待成功（例如 assertTrue 或 assertTrue-like
断言）以确保第一批确实被释放；定位点：FlatDhcpBackend.BATCH_APPLY_DHCP_PATH 回调、变量 firstBatchArrived
和 releaseFirstBatch、以及当前调用 releaseFirstBatch.await(10, TimeUnit.SECONDS) 的位置，改为对
await 的布尔返回值进行断言并在失败时让测试失败。

test/src/test/groovy/org/zstack/test/integration/configuration/systemTag/TagPatternResourceTypeCase.groovy-15-21 (1)

15-21: ⚠️ Potential issue | 🟡 Minor

类注释描述了当前并未覆盖的行为。

这里写了会验证 prepareDbInitialValue() 触发的 resourceType 回填，但这个用例实际只覆盖了查询过滤语义，没有走初始化/回填路径。建议删掉这段说明，或者补一个真正覆盖回填流程的断言。

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@test/src/test/groovy/org/zstack/test/integration/configuration/systemTag/TagPatternResourceTypeCase.groovy`
around lines 15 - 21, Update the class comment or the test to match actual
coverage: either remove the third bullet that claims prepareDbInitialValue()
backfills old AI tags, or implement the backfill scenario in
TagPatternResourceTypeCase by creating an old-style AI tag with resourceType ==
null, invoking the database initializer (prepareDbInitialValue()), then
reloading/querying the TagPattern/Model tag and asserting its resourceType is
set to "ModelVO"; reference TagPatternResourceTypeCase and
prepareDbInitialValue() to locate where to add the setup, invocation, and
assertion.

compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java-409-425 (1)

409-425: ⚠️ Potential issue | 🟡 Minor

DNS 地址应先做 trim 和空项过滤，再做数量/格式校验。

当前直接校验原始 dnsAddresses，会把 " 8.8.8.8 "、"" 这类浏览器粘贴常见输入当成非法值，而且规范化后的结果也没有回写到消息里。这里最好先 trim、跳过空字符串，再按规范化后的列表做数量和格式校验。As per coding guidelines "注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等". Based on learnings "DNS 地址的格式校验（trim、IPv4/IPv6 合法性检查、最大条目数限制）已统一在 VmInstanceApiInterceptor 的 validateDnsAddresses() 方法（API 拦截阶段）完成".

🤖 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/vm/VmInstanceApiInterceptor.java`
around lines 409 - 425, In validateDnsAddresses(List<String> dnsAddresses)
normalize the input first by trimming each entry and filtering out empty/blank
strings (e.g., remove entries that become ""), then perform the size check
against VmInstanceConstant.MAXIMUM_NIC_DNS_NUMBER and the IPv4/IPv6 format
checks using NetworkUtils.isIpv4Address and IPv6NetworkUtils.isIpv6Address on
the normalized list; finally ensure the normalized list is written back to the
API message/request field (so downstream uses the trimmed/filtered addresses)
instead of validating the original raw list.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# 只读验证：检查是否存在跨包实现/实例化，评估该问题在当前仓库中的实际影响面
rg -nP --type=java -C2 'implements\s+ResourceMetricBindingExtensionPoint'
rg -nP --type=java -C2 'new\s+ResourceMetricBindingExtensionPoint\.ResourceMetricBinding\s*\('

Repository: MatheMatrix/zstack

Length of output: 44

---

🏁 Script executed:

cat -n header/src/main/java/org/zstack/header/zwatch/ResourceMetricBindingExtensionPoint.java

Repository: MatheMatrix/zstack

Length of output: 3526

---

ResourceMetricBinding 作为扩展点实现需返回的对象类型，必须可被外部包实例化，应添加显式 public 构造器。

Line 6 的内部类当前为默认访问级别（无 public 修饰符且无显式构造器），跨包的 ExtensionPoint 实现无法通过 new ResourceMetricBinding() 创建对象实例。由于接口返回类型为 List<ResourceMetricBinding>，该类必须对所有实现者可访问。

接口方法需补充 Javadoc 注释。
Line 87 的 getResourceMetricBindings() 缺少方法说明，违反 Java 编码规范。

建议 setter 方法执行 trim 归一化处理。
各 String setter（如 line 42-43 的 setLogicalMetricName）未对输入进行 trim，而 getter 中 requireText 仅校验 trim 后是否为空，导致带前后空白的值被原样返回，可能引起指标名/标签匹配偏差。

建议修复

-    class ResourceMetricBinding {
+    public static class ResourceMetricBinding {
+        public ResourceMetricBinding() {
+        }
         private Class<?> resourceType;

     public void setLogicalMetricName(String logicalMetricName) {
-        this.logicalMetricName = logicalMetricName;
+        this.logicalMetricName = logicalMetricName != null ? logicalMetricName.trim() : null;
     }

+    /**
+     * Get resource metric bindings for this extension point.
+     * `@return` list of resource metric bindings
+     */
     List<ResourceMetricBinding> getResourceMetricBindings();

📝 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

	class ResourceMetricBinding {
	public static class ResourceMetricBinding {
	public ResourceMetricBinding() {
	}
	private Class<?> resourceType;

Suggested change

	class ResourceMetricBinding {
	public void setLogicalMetricName(String logicalMetricName) {
	this.logicalMetricName = logicalMetricName != null ? logicalMetricName.trim() : null;
	}

Suggested change

	class ResourceMetricBinding {
	/**
	* Get resource metric bindings for this extension point.
	* `@return` list of resource metric bindings
	*/
	List<ResourceMetricBinding> getResourceMetricBindings();

🤖 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/zwatch/ResourceMetricBindingExtensionPoint.java`
at line 6, ResourceMetricBinding is currently package-private and lacks an
explicit public no-arg constructor, so make it a public (and if nested, public
static) class with a public no-argument constructor so external ExtensionPoint
implementations can instantiate it; add Javadoc to the interface method
getResourceMetricBindings() describing its purpose and return semantics; and
normalize string inputs by trimming in all String setters (e.g.,
setLogicalMetricName, setMetricName, setResourceType, any tag setters) so values
stored are trimmed before validation/return (getters may still use requireText
for non-empty checks).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# 检查 DhcpExtension 中所有 ipRangeUuid null 检查的使用模式
rg -n "ipRangeUuid\s*==\s*null" network/src/main/java/org/zstack/network/service/DhcpExtension.java -A2 -B2

Repository: MatheMatrix/zstack

Length of output: 44

---

🏁 Script executed:

# First, verify file exists and get basic info
ls -la network/src/main/java/org/zstack/network/service/DhcpExtension.java

Repository: MatheMatrix/zstack

Length of output: 179

---

🏁 Script executed:

# Get total line count and read around lines 346-350
wc -l network/src/main/java/org/zstack/network/service/DhcpExtension.java

Repository: MatheMatrix/zstack

Length of output: 133

---

🏁 Script executed:

# Read the specific section mentioned in the review
sed -n '340,360p' network/src/main/java/org/zstack/network/service/DhcpExtension.java

Repository: MatheMatrix/zstack

Length of output: 921

---

🏁 Script executed:

# Search for all ipRangeUuid references with more flexible patterns
rg -n "ipRangeUuid" network/src/main/java/org/zstack/network/service/DhcpExtension.java -B2 -A2

Repository: MatheMatrix/zstack

Length of output: 44

---

🏁 Script executed:

# Look for the methods mentioned in the review
rg -n "isDualStackNicInSingleL3Network|makeDhcpStruct|workoutDhcp" network/src/main/java/org/zstack/network/service/DhcpExtension.java -B1 -A5

Repository: MatheMatrix/zstack

Length of output: 2865

---

🏁 Script executed:

# Read the full workoutDhcp method starting from line 325
sed -n '325,375p' network/src/main/java/org/zstack/network/service/DhcpExtension.java

Repository: MatheMatrix/zstack

Length of output: 2317

---

🏁 Script executed:

# Look for comments or documentation about outside-IP-range handling
rg -n "outside.*range|static.*ip|outside" network/src/main/java/org/zstack/network/service/DhcpExtension.java -i

Repository: MatheMatrix/zstack

Length of output: 445

---

🏁 Script executed:

# Check the makeDhcpStruct method to see if it mentions ipRangeUuid handling
sed -n '256,300p' network/src/main/java/org/zstack/network/service/DhcpExtension.java

Repository: MatheMatrix/zstack

Length of output: 2656

---

修正 workoutDhcp 中的 IP 过滤逻辑与 makeDhcpStruct 保持一致。

当前 workoutDhcp (第347行) 使用 ipRangeUuid == null 无条件跳过 IP，但 makeDhcpStruct (第288行) 使用 ipRangeUuid == null && isIpOutsideL3NetworkCidr(...) 组合条件。这个差异导致实际的逻辑缺陷：

• 若某个 VmNic 的所有 IP 都有 ipRangeUuid == null，即使这些 IP 在 L3 CIDR 范围内，workoutDhcp 也会跳过该 NIC，使其无法进入 makeDhcpStruct 处理。
• 根据 PR #3513，outside-IP-range 特性允许常规 VM 拥有 ipRangeUuid == null 但在 CIDR 范围内的 IP。

建议将 workoutDhcp 第347-349行的过滤逻辑修改为与 makeDhcpStruct 第288行一致的条件：仅当 IP 既没有 ipRangeUuid 且也不在 L3 CIDR 范围内时才跳过。

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@network/src/main/java/org/zstack/network/service/DhcpExtension.java` around
lines 346 - 350, The IP filtering in workoutDhcp incorrectly skips any UsedIpVO
with ipRangeUuid == null; change the condition in workoutDhcp (the loop over
vmNicVO.getUsedIps()) to match makeDhcpStruct’s logic by only skipping an IP
when ip.getIpRangeUuid() == null AND the IP is outside the L3 CIDR (use the same
helper/isIpOutsideL3NetworkCidr(...) check used by makeDhcpStruct), so NICs
whose IPs have null ipRangeUuid but lie inside the L3 CIDR are not excluded from
further processing.

[coderabbitai]

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 `@plugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.java`:
- Around line 100-104: The three new constants KVM_VIRTIOFS_ATTACH_PATH,
KVM_VIRTIOFS_DETACH_PATH and KVM_MODEL_CENTER_MOUNT_PATH are declared but never
wired into the controller/agent URL routing; follow the existing pattern (e.g.,
connectPath, pingPath, startVmPath) by calling
ub.path(KVMConstant.KVM_VIRTIOFS_ATTACH_PATH) /
ub.path(KVMConstant.KVM_VIRTIOFS_DETACH_PATH) /
ub.path(KVMConstant.KVM_MODEL_CENTER_MOUNT_PATH) in the KVM agent/manager
initialization to build and store corresponding member URL fields, and implement
the matching route handlers on the agent side to handle attach/detach/mount
requests so the constants are actually used.

🪄 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: 87399dc4-b5a6-4ce4-a9f1-e15d9c22abb9

📥 Commits

Reviewing files that changed from the base of the PR and between 10bae7a and aa5e058.

⛔ Files ignored due to path filters (10)

• conf/i18n/globalErrorCodeMapping/global-error-zh_CN.json is excluded by !**/*.json
• sdk/src/main/java/SourceClassMap.java is excluded by !sdk/**
• sdk/src/main/java/org/zstack/sdk/MountModelToVmInstanceAction.java is excluded by !sdk/**
• sdk/src/main/java/org/zstack/sdk/MountModelToVmInstanceResult.java is excluded by !sdk/**
• sdk/src/main/java/org/zstack/sdk/QueryVmModelMountAction.java is excluded by !sdk/**
• sdk/src/main/java/org/zstack/sdk/QueryVmModelMountResult.java is excluded by !sdk/**
• sdk/src/main/java/org/zstack/sdk/UnmountModelFromVmInstanceAction.java is excluded by !sdk/**
• sdk/src/main/java/org/zstack/sdk/UnmountModelFromVmInstanceResult.java is excluded by !sdk/**
• sdk/src/main/java/org/zstack/sdk/VmModelMountInventory.java is excluded by !sdk/**
• sdk/src/main/java/org/zstack/sdk/VmModelMountStatus.java is excluded by !sdk/**

📒 Files selected for processing (3)

• plugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.java
• testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
• utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java

✅ Files skipped from review due to trivial changes (1)

• utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java

🚧 Files skipped from review as they are similar to previous changes (1)

• testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# 1) 检查新增常量是否被引用
rg -n -C2 'KVM_VIRTIOFS_ATTACH_PATH|KVM_VIRTIOFS_DETACH_PATH|KVM_MODEL_CENTER_MOUNT_PATH'

# 2) 检查是否在 KVMHost 中按既有模式初始化 URL
rg -n -C3 'ub\.path\(KVMConstant\.(KVM_VIRTIOFS_ATTACH_PATH|KVM_VIRTIOFS_DETACH_PATH|KVM_MODEL_CENTER_MOUNT_PATH)\)'

# 3) 检查仓库中是否存在对应路径处理入口（管理面/agent相关代码）
rg -n -C2 '/virtiofs/attach|/virtiofs/detach|/modelcenter/mount'

Repository: MatheMatrix/zstack

Length of output: 1435

---

🏁 Script executed:

# 查看 PR 涉及的所有文件
git diff --name-only HEAD~1..HEAD

# 查看 KVMHost 中是否有新增的相关路由处理
rg -n 'virtiofs|modelcenter' --type java

# 检查是否有新增的处理方法
fd -e java | xargs grep -l 'virtiofs\|modelcenter'

Repository: MatheMatrix/zstack

Length of output: 800

---

🏁 Script executed:

# 再次确认是否有任何引用被遗漏
rg -i 'KVM_VIRTIOFS_ATTACH_PATH|KVM_VIRTIOFS_DETACH_PATH|KVM_MODEL_CENTER_MOUNT_PATH' --type java

# 检查 KVMHost 中是否有新增的相关处理方法
rg -n 'virtiofs|attachPath|detachPath|modelcenter' plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java

# 查看既有的 uploadFileToHost 或类似 URL 构建模式
rg -n -A10 'uploadFileToHost|ub\.path' plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java | head -50

Repository: MatheMatrix/zstack

Length of output: 2604

---

确认端到端实现的完整性。

新增的常量定义未被任何代码引用。按照既有模式（如 connectPath、pingPath、startVmPath 等），管理面应在初始化中通过 ub.path(KVMConstant.KVM_VIRTIOFS_ATTACH_PATH) 等调用构建对应的 URL，并存储为成员变量；同时处理端应实现对应的路由处理。若缺少这些实现，这些常量不会生效。

🤖 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/KVMConstant.java` around lines 100 -
104, The three new constants KVM_VIRTIOFS_ATTACH_PATH, KVM_VIRTIOFS_DETACH_PATH
and KVM_MODEL_CENTER_MOUNT_PATH are declared but never wired into the
controller/agent URL routing; follow the existing pattern (e.g., connectPath,
pingPath, startVmPath) by calling ub.path(KVMConstant.KVM_VIRTIOFS_ATTACH_PATH)
/ ub.path(KVMConstant.KVM_VIRTIOFS_DETACH_PATH) /
ub.path(KVMConstant.KVM_MODEL_CENTER_MOUNT_PATH) in the KVM agent/manager
initialization to build and store corresponding member URL fields, and implement
the matching route handlers on the agent side to handle attach/detach/mount
requests so the constants are actually used.

[coderabbitai]

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 `@conf/db/upgrade/V5.5.16__schema.sql`:
- Around line 112-124: The ALTER ... DROP FOREIGN KEY on BareMetal2InstanceVO is
not idempotent; change it to check INFORMATION_SCHEMA.TABLE_CONSTRAINTS for
existence of fkBareMetal2InstanceVOGatewayVO and
fkBareMetal2InstanceVOGatewayVO1 before dropping and then add the new
constraints, following the pattern used by ModifyUsedIpVOForeignKey: wrap the
logic in a temporary stored procedure (e.g., ModifyBareMetal2InstanceGatewayFK),
SELECT COUNT(*) INTO local variables to conditionally ALTER TABLE ... DROP
FOREIGN KEY only if present, then ALTER TABLE ... ADD CONSTRAINT for gatewayUuid
and lastGatewayUuid, CALL the procedure and DROP the procedure afterwards to
ensure safe, repeatable upgrades.
- Around line 48-50: The UPDATE statement must use backticks and explicit zstack
schema: change table references to `zstack`.`VolumeSnapshotVO` AS `sp` and
`zstack`.`PrimaryStorageVO` AS `ps`, and wrap all column references with
backticks (e.g. `sp`.`primaryStorageInstallPath`, `sp`.`primaryStorageUuid`,
`ps`.`uuid`, `ps`.`type`, `sp`.`volumeType`); keep string literals as-is and
preserve the REPLACE and LIKE logic. Ensure the final SQL follows the pattern
used elsewhere in the file (e.g., other UPDATE statements that use `UPDATE
`zstack`.`TableName``).

🪄 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: 552b164a-5c26-4c8a-b7e3-db56d8dafd17

📥 Commits

Reviewing files that changed from the base of the PR and between aa5e058 and 01234ce.

⛔ Files ignored due to path filters (1)

• conf/i18n/globalErrorCodeMapping/global-error-zh_CN.json is excluded by !**/*.json

📒 Files selected for processing (2)

• conf/db/upgrade/V5.5.16__schema.sql
• utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java

✅ Files skipped from review due to trivial changes (1)

• utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# 首先找到该文件
fd -t f "V5.5.16__schema.sql"

Repository: MatheMatrix/zstack

Length of output: 97

---

🏁 Script executed:

# 读取文件的第40-60行以查看实际内容
head -60 conf/db/upgrade/V5.5.16__schema.sql | tail -25

Repository: MatheMatrix/zstack

Length of output: 1185

---

🏁 Script executed:

# 检查整个文件中的UPDATE和ALTER语句模式
sed -n '1,100p' conf/db/upgrade/V5.5.16__schema.sql | grep -E "(UPDATE|ALTER|CREATE TABLE)" | head -20

Repository: MatheMatrix/zstack

Length of output: 704

---

🏁 Script executed:

# 获取更多上下文，看看第48-50行周围的完整代码
sed -n '40,60p' conf/db/upgrade/V5.5.16__schema.sql | cat -n

Repository: MatheMatrix/zstack

Length of output: 1178

---

🏁 Script executed:

# 检查是否该UPDATE语句在其他升级文件中存在，以及前后文的完整行号
grep -n "UPDATE VolumeSnapshotVO" conf/db/upgrade/V5.5.16__schema.sql

Repository: MatheMatrix/zstack

Length of output: 118

---

🏁 Script executed:

# 检查VolumeSnapshotVO和PrimaryStorageVO中的列名是否有保留字
# 以及查看其他upgrade文件中的UPDATE语句模式
grep -r "UPDATE.*VolumeSnapshotVO" conf/db/upgrade/ | head -5

Repository: MatheMatrix/zstack

Length of output: 283

---

🏁 Script executed:

# 确认同一文件中其他地方使用backtick的模式
grep -n "UPDATE \`zstack\`" conf/db/upgrade/V5.5.16__schema.sql | head -3

Repository: MatheMatrix/zstack

Length of output: 190

---

为 DML 语句补齐反引号并显式 schema

第 48-50 行的 UPDATE 语句表名与列名未按规范使用反引号，且未显式指定 zstack schema。同一文件中其他 UPDATE 语句（如第 19、27、52 行）均遵循 UPDATE `zstack`.`TableName` 的规范，第 48-50 行应保持一致。

💡 建议修改

-UPDATE VolumeSnapshotVO AS sp, PrimaryStorageVO AS ps
-SET sp.primaryStorageInstallPath = REPLACE(sp.primaryStorageInstallPath, '/dev/', 'sharedblock://')
-WHERE sp.primaryStorageUuid = ps.uuid AND ps.type = 'SharedBlock' AND sp.volumeType = 'Memory' AND sp.primaryStorageInstallPath LIKE '/dev/%';
+UPDATE `zstack`.`VolumeSnapshotVO` AS `sp`, `zstack`.`PrimaryStorageVO` AS `ps`
+SET `sp`.`primaryStorageInstallPath` = REPLACE(`sp`.`primaryStorageInstallPath`, '/dev/', 'sharedblock://')
+WHERE `sp`.`primaryStorageUuid` = `ps`.`uuid`
+  AND `ps`.`type` = 'SharedBlock'
+  AND `sp`.`volumeType` = 'Memory'
+  AND `sp`.`primaryStorageInstallPath` LIKE '/dev/%';

编码规范："所有表名和列名必须使用反引号包裹（例如：WHERE system = 1），以避免 MySQL 8.0 / GreatSQL 保留关键字冲突导致的语法错误"。

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@conf/db/upgrade/V5.5.16__schema.sql` around lines 48 - 50, The UPDATE
statement must use backticks and explicit zstack schema: change table references
to `zstack`.`VolumeSnapshotVO` AS `sp` and `zstack`.`PrimaryStorageVO` AS `ps`,
and wrap all column references with backticks (e.g.
`sp`.`primaryStorageInstallPath`, `sp`.`primaryStorageUuid`, `ps`.`uuid`,
`ps`.`type`, `sp`.`volumeType`); keep string literals as-is and preserve the
REPLACE and LIKE logic. Ensure the final SQL follows the pattern used elsewhere
in the file (e.g., other UPDATE statements that use `UPDATE
`zstack`.`TableName``).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# 查找数据库升级脚本文件
find . -type f -name "V5.5.16__schema.sql" 2>/dev/null

Repository: MatheMatrix/zstack

Length of output: 99

---

🏁 Script executed:

# 也搜索相关的 ModifyUsedIpVOForeignKey 和其他升级脚本
find . -type f -name "*__schema.sql" | head -20

Repository: MatheMatrix/zstack

Length of output: 763

---

🏁 Script executed:

# 查看 V5.5.16__schema.sql 的内容，重点关注 112-124 行
if [ -f "conf/db/upgrade/V5.5.16__schema.sql" ]; then
  echo "=== Lines 100-135 ===" 
  sed -n '100,135p' conf/db/upgrade/V5.5.16__schema.sql
  echo ""
  echo "=== Searching for ModifyUsedIpVOForeignKey ===" 
  grep -n "ModifyUsedIpVOForeignKey" conf/db/upgrade/V5.5.16__schema.sql || echo "Not found in this file"
fi

Repository: MatheMatrix/zstack

Length of output: 1918

---

🏁 Script executed:

# 搜索所有升级脚本中的 ModifyUsedIpVOForeignKey 模式
rg "ModifyUsedIpVOForeignKey" --type sql -A 30 conf/db/upgrade/

Repository: MatheMatrix/zstack

Length of output: 6284

---

🏁 Script executed:

# 查看整个 V5.5.16__schema.sql 文件以获取完整上下文
wc -l conf/db/upgrade/V5.5.16__schema.sql

Repository: MatheMatrix/zstack

Length of output: 101

---

BareMetal2InstanceVO 的外键修改缺少幂等保护，重复升级会失败

第 112-124 行直接 DROP FOREIGN KEY，若约束不存在会中断升级。应参照同一文件中 ModifyUsedIpVOForeignKey（第 141-180 行）的做法，通过 INFORMATION_SCHEMA.TABLE_CONSTRAINTS 检查约束是否存在后再执行 DROP/ADD，确保升级脚本可安全重复执行。

建议修改方案（幂等版）

DROP PROCEDURE IF EXISTS ModifyBareMetal2InstanceGatewayFK;
DELIMITER $$
CREATE PROCEDURE ModifyBareMetal2InstanceGatewayFK()
BEGIN
    DECLARE c1 INT DEFAULT 0;
    DECLARE c2 INT DEFAULT 0;

    SELECT COUNT(*) INTO c1
    FROM `INFORMATION_SCHEMA`.`TABLE_CONSTRAINTS`
    WHERE `TABLE_SCHEMA`='zstack' AND `TABLE_NAME`='BareMetal2InstanceVO'
      AND `CONSTRAINT_NAME`='fkBareMetal2InstanceVOGatewayVO' AND `CONSTRAINT_TYPE`='FOREIGN KEY';
    IF c1 > 0 THEN
        ALTER TABLE `zstack`.`BareMetal2InstanceVO` DROP FOREIGN KEY `fkBareMetal2InstanceVOGatewayVO`;
    END IF;

    SELECT COUNT(*) INTO c2
    FROM `INFORMATION_SCHEMA`.`TABLE_CONSTRAINTS`
    WHERE `TABLE_SCHEMA`='zstack' AND `TABLE_NAME`='BareMetal2InstanceVO'
      AND `CONSTRAINT_NAME`='fkBareMetal2InstanceVOGatewayVO1' AND `CONSTRAINT_TYPE`='FOREIGN KEY';
    IF c2 > 0 THEN
        ALTER TABLE `zstack`.`BareMetal2InstanceVO` DROP FOREIGN KEY `fkBareMetal2InstanceVOGatewayVO1`;
    END IF;

    ALTER TABLE `zstack`.`BareMetal2InstanceVO`
      ADD CONSTRAINT `fkBareMetal2InstanceVOGatewayVO`
        FOREIGN KEY (`gatewayUuid`) REFERENCES `zstack`.`HostEO` (`uuid`) ON DELETE SET NULL,
      ADD CONSTRAINT `fkBareMetal2InstanceVOGatewayVO1`
        FOREIGN KEY (`lastGatewayUuid`) REFERENCES `zstack`.`HostEO` (`uuid`) ON DELETE SET NULL;
END $$
DELIMITER ;
CALL ModifyBareMetal2InstanceGatewayFK();
DROP PROCEDURE IF EXISTS ModifyBareMetal2InstanceGatewayFK;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@conf/db/upgrade/V5.5.16__schema.sql` around lines 112 - 124, The ALTER ...
DROP FOREIGN KEY on BareMetal2InstanceVO is not idempotent; change it to check
INFORMATION_SCHEMA.TABLE_CONSTRAINTS for existence of
fkBareMetal2InstanceVOGatewayVO and fkBareMetal2InstanceVOGatewayVO1 before
dropping and then add the new constraints, following the pattern used by
ModifyUsedIpVOForeignKey: wrap the logic in a temporary stored procedure (e.g.,
ModifyBareMetal2InstanceGatewayFK), SELECT COUNT(*) INTO local variables to
conditionally ALTER TABLE ... DROP FOREIGN KEY only if present, then ALTER TABLE
... ADD CONSTRAINT for gatewayUuid and lastGatewayUuid, CALL the procedure and
DROP the procedure afterwards to ensure safe, repeatable upgrades.