<feature>[kvm]: add libvirt TLS config#3458
Closed
zstack-robot-2 wants to merge 1 commit into
Closed
Conversation
|
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一组功能与平台改进：为 KVM 迁移加入 TLS 支持并传递证书 IP；扩展数据库表以支持通知与 BareMetal2 DPU；新增/修改平台核心（实体生命周期可卸载回调、任务进度详细解析）；放宽安全组优先级校验；大量测试与 SDK 辅助方法扩展；若干配置与脚本调整。 Changes
Sequence Diagram(s)sequenceDiagram
participant KVMHost as KVMHost
participant Config as KVMGlobalConfig
participant AgentCmd as KVMAgentCommands.MigrateVmCmd
participant DeployArgs as KVMHostDeployArguments
participant Libvirt as Libvirt (remote)
KVMHost->>Config: 读取 LIBVIRT_TLS_ENABLED
Config-->>KVMHost: 返回值 (true/false)
KVMHost->>AgentCmd: new MigrateVmCmd(...)\nsetUseTls(value)\nsetSrcHostManagementIp(srcHostMnIp)
KVMHost->>DeployArgs: setTlsCertIps(managementIp[, extraIps])
KVMHost->>Libvirt: 发送迁移请求(包含 useTls, srcHostManagementIp, tlsCertIps)
Libvirt-->>KVMHost: 返回迁移响应
预计代码审查工作量🎯 5 (Critical) | ⏱️ ~120 分钟 诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 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/KVMHost.java`:
- Line 3177: The call to
KVMGlobalConfig.LIBVIRT_TLS_ENABLED.value(Boolean.class) in KVMHost (affecting
cmd.setUseTls) bypasses cluster-scoped overrides; change it to fetch the
cluster-level value via the resource config facade (use
rcf.getResourceConfigValue) for the host's cluster (use the host/cluster UUID
available in KVMHost) and pass that Boolean into cmd.setUseTls, matching the
pattern used elsewhere in this class (e.g., where rcf.getResourceConfigValue is
used around lines ~3157, ~3931, ~1782).
ℹ️ 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: 7bb792e6-a9b5-4822-b465-e9ee96620481
📒 Files selected for processing (3)
plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalConfig.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
MatheMatrix
force-pushed
the
sync/yingzhe.hu/ZSTAC-81343@@2
branch
from
69e99d0 to
5e7f24f
Compare
Collaborator
Author
|
Comment from yaohua.wu: Review: MR !9317 — ZSTAC-81343 (libvirt TLS 配置 - Java 侧)关联 MR: zstack-utility !6727 | premium !13134 正面评价
Warning
Suggestion
Verdict: APPROVEDJava 侧改动简洁正确，架构决策合理（全局配置 vs 集群级配置的选择正确）。Warning #1 建议确认但不阻塞合并。 🤖 Robot Reviewer |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostDeployArguments.java (1)
140-146: 建议为tlsCertIps的格式约束补充简短说明。当前使用
String表达“IP 列表”语义，建议在 Line 140-146 的访问器处补一行 Javadoc（例如分隔符、是否允许空值、IPv6 表达方式），可降低调用方误传风险。🤖 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/KVMHostDeployArguments.java` around lines 140 - 146, 为 KVMHostDeployArguments 类的 tlsCertIps 访问器补充一行 Javadoc，说明该字段是用何种格式表示“IP 列表”的（例如分隔符为逗号或分号）、是否允许空值/空字符串表示“无 IP”、是否支持 IPv6（是否需要方括号或纯文本形式）、以及一个短示例；在 getTlsCertIps 和 setTlsCertIps 的注释处添加上述说明，以便调用方明确传入格式与校验预期。plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (1)
5820-5828: 建议规范化tlsCertIps输入，避免空白/重复 IP 进入 SAN。当前拼接方式可工作，但对
EXTRA_IPS的空白与重复值不够稳健，建议在赋值前做trim + 过滤空值 + 去重。♻️ 建议修改
- // Build TLS cert IP list: management IP + extra IPs (migration network etc.) - String managementIp = getSelf().getManagementIp(); - String extraIps = HostSystemTags.EXTRA_IPS.getTokenByResourceUuid( - self.getUuid(), HostSystemTags.EXTRA_IPS_TOKEN); - if (extraIps != null && !extraIps.isEmpty()) { - deployArguments.setTlsCertIps(managementIp + "," + extraIps); - } else { - deployArguments.setTlsCertIps(managementIp); - } + // Build TLS cert IP list: management IP + normalized extra IPs (migration network etc.) + String managementIp = getSelf().getManagementIp(); + String extraIps = HostSystemTags.EXTRA_IPS.getTokenByResourceUuid( + self.getUuid(), HostSystemTags.EXTRA_IPS_TOKEN); + List<String> tlsCertIps = new ArrayList<>(); + tlsCertIps.add(managementIp); + if (StringUtils.isNotBlank(extraIps)) { + Arrays.stream(extraIps.split(",")) + .map(String::trim) + .filter(StringUtils::isNotBlank) + .filter(ip -> !managementIp.equals(ip)) + .distinct() + .forEach(tlsCertIps::add); + } + deployArguments.setTlsCertIps(String.join(",", tlsCertIps));🤖 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` around lines 5820 - 5828, The TLS SAN construction in KVMHost currently concatenates management IP and HostSystemTags.EXTRA_IPS raw string, which can introduce blank or duplicate entries; update the logic around getSelf().getManagementIp() and HostSystemTags.EXTRA_IPS.getTokenByResourceUuid(self.getUuid(), HostSystemTags.EXTRA_IPS_TOKEN) to: split the EXTRA_IPS string by comma, trim each entry, filter out empty strings, remove duplicates while preserving order (ensuring the management IP is present and first), then join with commas and pass the normalized result to deployArguments.setTlsCertIps(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5820-5828: The TLS SAN construction in KVMHost currently
concatenates management IP and HostSystemTags.EXTRA_IPS raw string, which can
introduce blank or duplicate entries; update the logic around
getSelf().getManagementIp() and
HostSystemTags.EXTRA_IPS.getTokenByResourceUuid(self.getUuid(),
HostSystemTags.EXTRA_IPS_TOKEN) to: split the EXTRA_IPS string by comma, trim
each entry, filter out empty strings, remove duplicates while preserving order
(ensuring the management IP is present and first), then join with commas and
pass the normalized result to deployArguments.setTlsCertIps(...).
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostDeployArguments.java`:
- Around line 140-146: 为 KVMHostDeployArguments 类的 tlsCertIps 访问器补充一行
Javadoc，说明该字段是用何种格式表示“IP 列表”的（例如分隔符为逗号或分号）、是否允许空值/空字符串表示“无 IP”、是否支持
IPv6（是否需要方括号或纯文本形式）、以及一个短示例；在 getTlsCertIps 和 setTlsCertIps
的注释处添加上述说明，以便调用方明确传入格式与校验预期。
ℹ️ 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: 8eb5611a-11f0-4037-9c40-662ac0aa5c2c
📒 Files selected for processing (2)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostDeployArguments.java
MatheMatrix
force-pushed
the
sync/yingzhe.hu/ZSTAC-81343@@2
branch
from
1701b41 to
82f907d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
conf/tools/install.sh (1)
98-138:⚠️ Potential issue | 🟡 Minor建议将
/usr/bin/ansible与/usr/bin/ansible-playbook的存在性纳入重建条件。当前仅校验 venv 内
ansible可执行与版本；若系统包装脚本丢失但 venv 正常，会跳过重建，导致后续通过/usr/bin/ansible*调用失败。建议把包装脚本检查也加入条件，保证安装流程幂等。建议修改
- if [ ! -x "$SYS_VIRENV_PATH/bin/ansible" ] || ! "$SYS_VIRENV_PATH/bin/ansible" --version 2>/dev/null | grep -q 'core 2.16.14'; then + if [ ! -x "$SYS_VIRENV_PATH/bin/ansible" ] \ + || ! "$SYS_VIRENV_PATH/bin/ansible" --version 2>/dev/null | grep -q 'core 2.16.14' \ + || [ ! -x "/usr/bin/ansible" ] \ + || [ ! -x "/usr/bin/ansible-playbook" ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conf/tools/install.sh` around lines 98 - 138, The current venv-recreate conditional only checks "$SYS_VIRENV_PATH/bin/ansible" and its version, but not the wrapper scripts /usr/bin/ansible and /usr/bin/ansible-playbook, so if those wrappers are missing the block is skipped and calls break; update the if condition to also test the presence/executability of the wrapper scripts (e.g. add checks like [ ! -x /usr/bin/ansible ] || [ ! -x /usr/bin/ansible-playbook ] into the combined condition) so the venv and wrappers are rebuilt when either the venv binary, its version, or either wrapper is absent or non-executable; keep the existing rebuild steps (rm -rf $SYS_VIRENV_PATH, python3.11 -m venv, pip install, and the cat > /usr/bin/ansible* wrapper creation) unchanged.
🧹 Nitpick comments (8)
header/src/main/java/org/zstack/header/cluster/APICreateClusterMsg.java (1)
68-73: 请同步更新@choices注释，避免与实际校验值不一致。Line 73 已允许
baremetal2、xdragon、baremetal2Dpu，但 Lines 68-72 的注释仍是旧列表，容易误导 API 使用方。建议修改
- * `@choices` - KVM - * - Simulator - * - baremetal + * `@choices` - KVM + * - Simulator + * - baremetal + * - baremetal2 + * - xdragon + * - baremetal2DpuAs per coding guidelines, “对于较长的注释，需要仔细校对并随代码更新，确保内容正确”。
🤖 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/cluster/APICreateClusterMsg.java` around lines 68 - 73, 更新 APICreateClusterMsg 类中与字段 'hypervisorType' 相关的 Javadoc 注释，使 `@choices` 列表与 `@APIParam`(validValues = {"KVM", "Simulator", "baremetal", "baremetal2", "xdragon", "baremetal2Dpu"}) 保持一致；具体操作是在 APICreateClusterMsg 的 hypervisorType 注释（当前第68-72行）补充或替换旧的选项，加入 baremetal2、xdragon、baremetal2Dpu，确保注释文本与 `@APIParam` 的 validValues 完全匹配以避免误导 API 使用者。test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/securitygroup/AddSecurityGroupRuleOptimizedCase.groovy (1)
594-617: 测试更新正确，但缺少对新添加规则的断言验证。移除
expect(AssertionError)包装器的改动正确反映了新的范围校验逻辑。但在 Lines 611-615 添加rule_12和ingressRule后，测试方法直接结束，没有验证这些规则是否成功创建。建议添加类似 Line 599 的断言来验证规则是否以预期的优先级被创建：
💡 建议的修复
sg3 = addSecurityGroupRule { securityGroupUuid = sg3.uuid rules = [rule_12, ingressRule] priority = 12 } + assert sg3.rules.find { it.dstIpRange == rule_12.dstIpRange && it.priority == 12 } != null + assert sg3.rules.find { it.dstPortRange == ingressRule.dstPortRange && it.type == "Ingress" } != null }🤖 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 594 - 617, After adding rule_12 and ingressRule to sg3 via the addSecurityGroupRule call, add assertions that verify the new rules were created with the expected fields and priority: assert that sg3.rules contains an entry with dstIpRange == rule_12.dstIpRange and priority == 12 (for rule_12), and assert that sg3.rules contains an entry with protocol == ingressRule.protocol, dstPortRange == ingressRule.dstPortRange and priority == 12 (for ingressRule); locate the check near the existing assertion that validated rule_13 and mirror its style using sg3, rule_12, ingressRule and the addSecurityGroupRule call.plugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupApiInterceptor.java (1)
1181-1186: 错误消息重复，建议合并逻辑。两个条件检查的错误消息内容完全相同，但使用了不同的错误码（10120 和 10121）。考虑到逻辑可读性和一致性，建议将两个检查合并：
♻️ 建议的重构
- if (msg.getPriority() > SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT.value(Integer.class) && toCreateIngressRuleCount > 0) { - throw new ApiMessageInterceptionException(argerr(ORG_ZSTACK_NETWORK_SECURITYGROUP_10120, "could not add security group rule, because priority[%d] exceeds the maximum allowed priority [%d]", msg.getPriority(), SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT.value(Integer.class))); - } - if (msg.getPriority() > SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT.value(Integer.class) && toCreateEgressRuleCount > 0) { - throw new ApiMessageInterceptionException(argerr(ORG_ZSTACK_NETWORK_SECURITYGROUP_10121, "could not add security group rule, because priority[%d] exceeds the maximum allowed priority [%d]", msg.getPriority(), SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT.value(Integer.class))); - } + if (msg.getPriority() > SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT.value(Integer.class)) { + throw new ApiMessageInterceptionException(argerr(ORG_ZSTACK_NETWORK_SECURITYGROUP_10120, "could not add security group rule, because priority[%d] exceeds the maximum allowed priority [%d]", msg.getPriority(), SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT.value(Integer.class))); + }当前拆分检查的条件
toCreateIngressRuleCount > 0或toCreateEgressRuleCount > 0实际上是冗余的，因为在 Line 963-965 已经检查了rules.isEmpty()，所以至少有一种规则类型的数量 > 0。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupApiInterceptor.java` around lines 1181 - 1186, Combine the two duplicate priority checks in SecurityGroupApiInterceptor into a single condition: check if msg.getPriority() > SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT.value(Integer.class) and (toCreateIngressRuleCount > 0 || toCreateEgressRuleCount > 0), then throw a single ApiMessageInterceptionException with a unified error message; use one consistent error code (e.g., ORG_ZSTACK_NETWORK_SECURITYGROUP_10120) and reference msg.getPriority() and SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT in the thrown message.plugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerApiInterceptor.java (1)
868-875: 验证逻辑正确，与现有模式一致。这段新增代码阻止用户通过 system tag 直接设置
httpCompressAlgos::disable，与 Lines 1136-1156 中通过 API 字段处理的逻辑形成互补，防止绕过。错误信息使用英文，符合编码规范。可选改进：考虑对提取的值进行 trim 处理
根据 Interceptor 相关编码规范，用户从浏览器复制粘贴的数据可能带有空格。虽然文件中现有的类似模式（如 Lines 849-867）也未使用 trim，但如果希望更严格地匹配，可以考虑：
if (LoadBalancerSystemTags.HTTP_COMPRESS_ALGOS.isMatch(tag)) { String compressAlgos = LoadBalancerSystemTags.HTTP_COMPRESS_ALGOS.getTokenByTag(tag, LoadBalancerSystemTags.HTTP_COMPRESS_ALGOS_TOKEN); - if (DisableLbSupportHttpCompressAlgos.equals(compressAlgos)) { + if (compressAlgos != null && DisableLbSupportHttpCompressAlgos.equals(compressAlgos.trim())) { throw new ApiMessageInterceptionException(argerr(ORG_ZSTACK_NETWORK_SERVICE_LB_10172, "could not create the loadbalancer listener with systemTag httpCompressAlgos::disable, please remove this tag")); } },
🤖 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/LoadBalancerApiInterceptor.java` around lines 868 - 875, The new interceptor block correctly blocks the system tag value "httpCompressAlgos::disable", but to be robust against leading/trailing spaces from copy/paste, trim the extracted token before comparison: call getTokenByTag(...) and apply trim() to the result, then compare against DisableLbSupportHttpCompressAlgos; keep throwing ApiMessageInterceptionException with argerr when matched (same behavior around LoadBalancerSystemTags.HTTP_COMPRESS_ALGOS and the surrounding tag-checking block).plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (1)
5821-5829: 建议规范化tlsCertIps，避免脏数据进入 SAN 列表当前直接拼接
extraIps，若标签里有空白/空项/重复值，会把无效项透传到部署参数。建议在组装时做 trim、去空、去重。♻️ 建议修改
- String managementIp = getSelf().getManagementIp(); - String extraIps = HostSystemTags.EXTRA_IPS.getTokenByResourceUuid( - self.getUuid(), HostSystemTags.EXTRA_IPS_TOKEN); - if (extraIps != null && !extraIps.isEmpty()) { - deployArguments.setTlsCertIps(managementIp + "," + extraIps); - } else { - deployArguments.setTlsCertIps(managementIp); - } + String managementIp = getSelf().getManagementIp(); + String extraIps = HostSystemTags.EXTRA_IPS.getTokenByResourceUuid( + self.getUuid(), HostSystemTags.EXTRA_IPS_TOKEN); + List<String> tlsCertIps = new ArrayList<>(); + tlsCertIps.add(managementIp); + if (extraIps != null) { + Arrays.stream(extraIps.split(",")) + .map(String::trim) + .filter(ip -> !ip.isEmpty()) + .filter(ip -> !ip.equals(managementIp)) + .distinct() + .forEach(tlsCertIps::add); + } + deployArguments.setTlsCertIps(String.join(",", tlsCertIps));🤖 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` around lines 5821 - 5829, The tlsCertIps string is built by directly concatenating extraIps which may contain blanks, empty items or duplicates; in KVMHost (use getSelf().getManagementIp(), HostSystemTags.EXTRA_IPS.getTokenByResourceUuid(...) and deployArguments.setTlsCertIps(...)) split extraIps by comma, trim each entry, filter out empty strings, remove duplicates (preserve order), prepend the management IP if not already present, then join with commas and pass the cleaned result to deployArguments.setTlsCertIps; ensure null/empty extraIps is handled by setting just the management IP.conf/db/upgrade/beforeMigrate.sql (1)
70-86: 表名缺少反引号包裹根据编码规范，所有表名和列名必须使用反引号包裹，以避免 MySQL 8.0 / GreatSQL 保留关键字冲突。
cleanupUsedIpVO存储过程中的表名未使用反引号。♻️ 建议修复
- SELECT COUNT(*) INTO vipCount FROM VipVO WHERE usedIpUuid = curUsedIpUuid; + SELECT COUNT(*) INTO vipCount FROM `VipVO` WHERE `usedIpUuid` = curUsedIpUuid; IF (vipCount > 0) THEN ITERATE read_loop; END IF; - SELECT COUNT(*) INTO vmNicCount FROM VmNicVO WHERE usedIpUuid = curUsedIpUuid; + SELECT COUNT(*) INTO vmNicCount FROM `VmNicVO` WHERE `usedIpUuid` = curUsedIpUuid; IF (vmNicCount > 0) THEN ITERATE read_loop; END IF; - SELECT COUNT(*) INTO dhcpCount FROM SystemTagVO WHERE resourceType='L3NetworkVO' + SELECT COUNT(*) INTO dhcpCount FROM `SystemTagVO` WHERE `resourceType`='L3NetworkVO' AND tag LIKE CONCAT('flatNetwork::DhcpServer::%::ipUuid::', curUsedIpUuid, '%'); IF (dhcpCount > 0) THEN ITERATE read_loop; END IF; - DELETE FROM UsedIpVO WHERE uuid = curUsedIpUuid; + DELETE FROM `UsedIpVO` WHERE `uuid` = curUsedIpUuid;As per coding guidelines: "所有表名和列名必须使用反引号包裹（例如：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/beforeMigrate.sql` around lines 70 - 86, The SQL uses unquoted table and column identifiers inside the cleanupUsedIpVO logic (queries against VipVO, VmNicVO, SystemTagVO, UsedIpVO and columns like usedIpUuid, uuid, resourceType, tag), which can break on MySQL 8+/GreatSQL; update each SQL statement in the shown block to wrap all table and column names with backticks (e.g., `VipVO`, `usedIpUuid`, `VmNicVO`, `SystemTagVO`, `resourceType`, `tag`, `UsedIpVO`, `uuid`) while keeping the same control flow (IF checks, ITERATE read_loop, DELETE) and references to the variable curUsedIpUuid and the read_loop label unchanged.conf/db/upgrade/V5.5.12__schema.sql (1)
112-124: 外键操作缺少幂等性检查
ALTER TABLE ... DROP FOREIGN KEY在约束不存在时会失败。如果升级脚本需要重复执行（例如升级失败后重试），这可能导致问题。建议使用存储过程或条件检查来确保幂等性：
-- 可以考虑封装为存储过程，先检查约束是否存在 DROP PROCEDURE IF EXISTS `DROP_FK_IF_EXISTS`; DELIMITER $$ CREATE PROCEDURE `DROP_FK_IF_EXISTS`(IN tb_name VARCHAR(64), IN fk_name VARCHAR(64)) BEGIN IF EXISTS (SELECT 1 FROM information_schema.TABLE_CONSTRAINTS WHERE TABLE_SCHEMA = 'zstack' AND TABLE_NAME = tb_name AND CONSTRAINT_NAME = fk_name AND CONSTRAINT_TYPE = 'FOREIGN KEY') THEN SET `@sql` = CONCAT('ALTER TABLE zstack.', tb_name, ' DROP FOREIGN KEY ', fk_name); PREPARE stmt FROM `@sql`; EXECUTE stmt; DEALLOCATE PREPARE stmt; END IF; END$$ DELIMITER ;Based on learnings: "ZStack 数据库升级脚本中的 DDL 操作应该通过 information_schema 检查存在性来确保脚本可以安全重复执行。"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conf/db/upgrade/V5.5.12__schema.sql` around lines 112 - 124, The ALTER TABLE drops are not idempotent; before calling ALTER TABLE ... DROP FOREIGN KEY for fkBareMetal2InstanceVOGatewayVO and fkBareMetal2InstanceVOGatewayVO1 on BareMetal2InstanceVO, add an existence check against information_schema.TABLE_CONSTRAINTS (TABLE_SCHEMA='zstack', TABLE_NAME='BareMetal2InstanceVO', CONSTRAINT_NAME=<fk>) and only execute the DROP if the row exists (you can implement a small stored procedure like DROP_FK_IF_EXISTS(tb_name,fk_name) or inline prepared statement logic); likewise guard the ADD CONSTRAINT blocks by checking that those foreign keys do not already exist before adding them to ensure the upgrade script is safe to re-run.core/src/main/java/org/zstack/core/db/DatabaseFacade.java (1)
88-88: 给新接口补一段 Javadoc，明确卸载语义。
entityClass == null是否表示“对所有实体类卸载”、以及evt/cb是否允许为null，现在只能从实现里反推。把契约写在接口上，后续调用方才不容易误用。📝 建议补充
+ /** + * Uninstall a previously installed entity lifecycle callback. + * If entityClass is null, the callback is removed from all registered entity classes. + */ void uninstallEntityLifeCycleCallback(Class entityClass, EntityEvent evt, EntityLifeCycleCallback cb);As per coding guidelines, "接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。"🤖 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/db/DatabaseFacade.java` at line 88, 为 uninstallEntityLifeCycleCallback 方法补一段 Javadoc，明确卸载语义：说明参数 entityClass 为 null 时是否表示“对所有实体类卸载”，evt（EntityEvent）和 cb（EntityLifeCycleCallback）是否允许为 null 及其含义（例如 evt 为 null 表示卸载该回调对所有事件，cb 为 null 表示移除所有回调），并注明方法的副作用/并发行为（是否线程安全、是否即时生效或延迟生效）和返回/异常契约；在注释中引用方法名 uninstallEntityLifeCycleCallback 及参数名 entityClass、evt、cb 以便调用方明确用法。
🤖 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/HostManagerImpl.java`:
- Around line 476-477: 更新注释以匹配实际逻辑：当前注释只提到 baremetal2，但方法实际基于
SKIP_ARCH_CHECK_HYPERVISOR_TYPES 对 cluster.getHypervisorType() 做判断（包含
baremetal2Dpu 等项）；请修改该注释为通用描述，例如说明“跳过对集群架构和 hypervisor 类型的校验，对于在
SKIP_ARCH_CHECK_HYPERVISOR_TYPES 列表中的 hypervisor 类型（例如 baremetal2、baremetal2Dpu
等）不进行校验”，以确保注释与 SKIP_ARCH_CHECK_HYPERVISOR_TYPES 和 cluster.getHypervisorType()
的行为一致。
In `@core/src/main/java/org/zstack/core/db/DatabaseFacadeImpl.java`:
- Around line 910-915: The uninstallEntityLifeCycleCallback currently silently
no-ops when entityInfoMap.get(clz) returns null; change it to mirror
installEntityLifeCycleCallback by asserting or throwing when EntityInfo is
missing so wrong class arguments are not swallowed. Locate
uninstallEntityLifeCycleCallback and replace the silent path that ignores a null
EntityInfo with the same guard used in installEntityLifeCycleCallback (check
entityInfoMap for clz and assert/throw if null) before calling
info.uninstallLifeCycleCallback(evt, cb), referencing EntityInfo, entityInfoMap,
EntityEvent, and EntityLifeCycleCallback.
---
Outside diff comments:
In `@conf/tools/install.sh`:
- Around line 98-138: The current venv-recreate conditional only checks
"$SYS_VIRENV_PATH/bin/ansible" and its version, but not the wrapper scripts
/usr/bin/ansible and /usr/bin/ansible-playbook, so if those wrappers are missing
the block is skipped and calls break; update the if condition to also test the
presence/executability of the wrapper scripts (e.g. add checks like [ ! -x
/usr/bin/ansible ] || [ ! -x /usr/bin/ansible-playbook ] into the combined
condition) so the venv and wrappers are rebuilt when either the venv binary, its
version, or either wrapper is absent or non-executable; keep the existing
rebuild steps (rm -rf $SYS_VIRENV_PATH, python3.11 -m venv, pip install, and the
cat > /usr/bin/ansible* wrapper creation) unchanged.
---
Nitpick comments:
In `@conf/db/upgrade/beforeMigrate.sql`:
- Around line 70-86: The SQL uses unquoted table and column identifiers inside
the cleanupUsedIpVO logic (queries against VipVO, VmNicVO, SystemTagVO, UsedIpVO
and columns like usedIpUuid, uuid, resourceType, tag), which can break on MySQL
8+/GreatSQL; update each SQL statement in the shown block to wrap all table and
column names with backticks (e.g., `VipVO`, `usedIpUuid`, `VmNicVO`,
`SystemTagVO`, `resourceType`, `tag`, `UsedIpVO`, `uuid`) while keeping the same
control flow (IF checks, ITERATE read_loop, DELETE) and references to the
variable curUsedIpUuid and the read_loop label unchanged.
In `@conf/db/upgrade/V5.5.12__schema.sql`:
- Around line 112-124: The ALTER TABLE drops are not idempotent; before calling
ALTER TABLE ... DROP FOREIGN KEY for fkBareMetal2InstanceVOGatewayVO and
fkBareMetal2InstanceVOGatewayVO1 on BareMetal2InstanceVO, add an existence check
against information_schema.TABLE_CONSTRAINTS (TABLE_SCHEMA='zstack',
TABLE_NAME='BareMetal2InstanceVO', CONSTRAINT_NAME=<fk>) and only execute the
DROP if the row exists (you can implement a small stored procedure like
DROP_FK_IF_EXISTS(tb_name,fk_name) or inline prepared statement logic); likewise
guard the ADD CONSTRAINT blocks by checking that those foreign keys do not
already exist before adding them to ensure the upgrade script is safe to re-run.
In `@core/src/main/java/org/zstack/core/db/DatabaseFacade.java`:
- Line 88: 为 uninstallEntityLifeCycleCallback 方法补一段 Javadoc，明确卸载语义：说明参数
entityClass 为 null 时是否表示“对所有实体类卸载”，evt（EntityEvent）和
cb（EntityLifeCycleCallback）是否允许为 null 及其含义（例如 evt 为 null 表示卸载该回调对所有事件，cb 为 null
表示移除所有回调），并注明方法的副作用/并发行为（是否线程安全、是否即时生效或延迟生效）和返回/异常契约；在注释中引用方法名
uninstallEntityLifeCycleCallback 及参数名 entityClass、evt、cb 以便调用方明确用法。
In `@header/src/main/java/org/zstack/header/cluster/APICreateClusterMsg.java`:
- Around line 68-73: 更新 APICreateClusterMsg 类中与字段 'hypervisorType' 相关的 Javadoc
注释，使 `@choices` 列表与 `@APIParam`(validValues = {"KVM", "Simulator", "baremetal",
"baremetal2", "xdragon", "baremetal2Dpu"}) 保持一致；具体操作是在 APICreateClusterMsg 的
hypervisorType 注释（当前第68-72行）补充或替换旧的选项，加入
baremetal2、xdragon、baremetal2Dpu，确保注释文本与 `@APIParam` 的 validValues 完全匹配以避免误导 API
使用者。
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 5821-5829: The tlsCertIps string is built by directly
concatenating extraIps which may contain blanks, empty items or duplicates; in
KVMHost (use getSelf().getManagementIp(),
HostSystemTags.EXTRA_IPS.getTokenByResourceUuid(...) and
deployArguments.setTlsCertIps(...)) split extraIps by comma, trim each entry,
filter out empty strings, remove duplicates (preserve order), prepend the
management IP if not already present, then join with commas and pass the cleaned
result to deployArguments.setTlsCertIps; ensure null/empty extraIps is handled
by setting just the management IP.
In
`@plugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerApiInterceptor.java`:
- Around line 868-875: The new interceptor block correctly blocks the system tag
value "httpCompressAlgos::disable", but to be robust against leading/trailing
spaces from copy/paste, trim the extracted token before comparison: call
getTokenByTag(...) and apply trim() to the result, then compare against
DisableLbSupportHttpCompressAlgos; keep throwing ApiMessageInterceptionException
with argerr when matched (same behavior around
LoadBalancerSystemTags.HTTP_COMPRESS_ALGOS and the surrounding tag-checking
block).
In
`@plugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupApiInterceptor.java`:
- Around line 1181-1186: Combine the two duplicate priority checks in
SecurityGroupApiInterceptor into a single condition: check if msg.getPriority()
> SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT.value(Integer.class)
and (toCreateIngressRuleCount > 0 || toCreateEgressRuleCount > 0), then throw a
single ApiMessageInterceptionException with a unified error message; use one
consistent error code (e.g., ORG_ZSTACK_NETWORK_SECURITYGROUP_10120) and
reference msg.getPriority() and
SecurityGroupGlobalConfig.SECURITY_GROUP_RULES_NUM_LIMIT in the thrown message.
In
`@test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/securitygroup/AddSecurityGroupRuleOptimizedCase.groovy`:
- Around line 594-617: After adding rule_12 and ingressRule to sg3 via the
addSecurityGroupRule call, add assertions that verify the new rules were created
with the expected fields and priority: assert that sg3.rules contains an entry
with dstIpRange == rule_12.dstIpRange and priority == 12 (for rule_12), and
assert that sg3.rules contains an entry with protocol == ingressRule.protocol,
dstPortRange == ingressRule.dstPortRange and priority == 12 (for ingressRule);
locate the check near the existing assertion that validated rule_13 and mirror
its style using sg3, rule_12, ingressRule and the addSecurityGroupRule call.
ℹ️ 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: d3e8a1fc-872d-438c-a2be-4ad96376b402
⛔ Files ignored due to path filters (33)
conf/i18n/globalErrorCodeMapping/global-error-de-DE.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-en_US.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-fr-FR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-id-ID.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ja-JP.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ko-KR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ru-RU.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-th-TH.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_CN.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_TW.jsonis excluded by!**/*.jsonsdk/src/main/java/SourceClassMap.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/AddBareMetal2DpuChassisAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/BareMetal2DpuChassisInventory.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/BareMetal2DpuHostInventory.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/CreateBareMetal2InstanceAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/CreateClusterAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/GpuVendor.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/LongJobProgressDetail.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/StartBareMetal2InstanceAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/TaskProgressInventory.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/ZSClient.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/DeleteResNotifySubscriptionAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/DeleteResNotifySubscriptionResult.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/QueryResNotifySubscriptionAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/QueryResNotifySubscriptionResult.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/ResNotifySubscriptionInventory.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/ResNotifySubscriptionState.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/ResNotifyType.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/ResNotifyWebhookRefInventory.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/SubscribeResNotifyAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/SubscribeResNotifyResult.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/UpdateResNotifySubscriptionAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/zwatch/resnotify/UpdateResNotifySubscriptionResult.javais excluded by!sdk/**
📒 Files selected for processing (26)
VERSIONcompute/src/main/java/org/zstack/compute/host/HostManagerImpl.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javaconf/db/upgrade/V5.5.12__schema.sqlconf/db/upgrade/beforeMigrate.sqlconf/tools/install.shcore/src/main/java/org/zstack/core/db/DatabaseFacade.javacore/src/main/java/org/zstack/core/db/DatabaseFacadeImpl.javacore/src/main/java/org/zstack/core/errorcode/LocaleUtils.javacore/src/main/java/org/zstack/core/progress/ProgressReportService.javaheader/src/main/java/org/zstack/header/cluster/APICreateClusterMsg.javaheader/src/main/java/org/zstack/header/cluster/APICreateClusterMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/core/progress/APIGetTaskProgressReply.javaheader/src/main/java/org/zstack/header/core/progress/LongJobProgressDetail.javaheader/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.javaheader/src/main/java/org/zstack/header/core/progress/TaskProgressInventory.javaheader/src/main/java/org/zstack/header/errorcode/ErrorCode.javaheader/src/main/java/org/zstack/header/longjob/LongJobProgressNotificationMessage.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerApiInterceptor.javaplugin/securityGroup/src/main/java/org/zstack/network/securitygroup/SecurityGroupApiInterceptor.javatest/src/test/groovy/org/zstack/test/integration/core/ErrorCodeI18nCase.groovytest/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/securitygroup/AddSecurityGroupRuleOptimizedCase.groovytest/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/securitygroup/ChangeSecurityGroupRuleCase.groovytestlib/src/main/java/org/zstack/testlib/ApiHelper.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
Comment on lines
+910
to
+915
| public void uninstallEntityLifeCycleCallback(Class clz, EntityEvent evt, EntityLifeCycleCallback cb) { | ||
| if (clz != null) { | ||
| EntityInfo info = entityInfoMap.get(clz); | ||
| if (info != null) { | ||
| info.uninstallLifeCycleCallback(evt, cb); | ||
| } |
There was a problem hiding this comment.
按类卸载时不要静默吞掉 EntityInfo 缺失。
这里和 installEntityLifeCycleCallback() 不对称：安装时会 Assert，卸载时却直接 no-op。传错类时，回调实际上不会被卸掉，还会继续响应后续 entity event，问题会被悄悄吞掉。
🧩 建议修改
public void uninstallEntityLifeCycleCallback(Class clz, EntityEvent evt, EntityLifeCycleCallback cb) {
if (clz != null) {
EntityInfo info = entityInfoMap.get(clz);
- if (info != null) {
- info.uninstallLifeCycleCallback(evt, cb);
- }
+ DebugUtils.Assert(info != null, String.format("cannot find EntityInfo for the class[%s]", clz));
+ info.uninstallLifeCycleCallback(evt, cb);
} else {
for (EntityInfo info : entityInfoMap.values()) {
info.uninstallLifeCycleCallback(evt, cb);🤖 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/db/DatabaseFacadeImpl.java` around lines
910 - 915, The uninstallEntityLifeCycleCallback currently silently no-ops when
entityInfoMap.get(clz) returns null; change it to mirror
installEntityLifeCycleCallback by asserting or throwing when EntityInfo is
missing so wrong class arguments are not swallowed. Locate
uninstallEntityLifeCycleCallback and replace the silent path that ignores a null
EntityInfo with the same guard used in installEntityLifeCycleCallback (check
entityInfoMap for clz and assert/throw if null) before calling
info.uninstallLifeCycleCallback(evt, cb), referencing EntityInfo, entityInfoMap,
EntityEvent, and EntityLifeCycleCallback.
6629b8f to
d6f16dc
Compare
Add libvirt.tls.enabled GlobalConfig and useTls field in MigrateVmCmd to support TLS-encrypted libvirt connections for migration and V2V. Resolves: ZSTAC-81343 Change-Id: I391fa36c0dd63c25c5d85d102bc3579c8eb3d685
MatheMatrix
force-pushed
the
sync/yingzhe.hu/ZSTAC-81343@@2
branch
from
d6f16dc to
3feb9e9
Compare
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.
Add libvirt.tls.enabled GlobalConfig and useTls field in MigrateVmCmd to support TLS-encrypted libvirt connections for migration and V2V.
Resolves: ZSTAC-81343
Change-Id: I391fa36c0dd63c25c5d85d102bc3579c8eb3d685
sync from gitlab !9317