Skip to content

<fix>[ceph]: support thirdparty_ceph to bm root volume #3189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MatheMatrix wants to merge 1 commit into
base: 5.5.6
Choose a base branch
from
Open

<fix>[ceph]: support thirdparty_ceph to bm root volume #3189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 76 additions & 1 deletion conf/db/upgrade/V5.5.6__schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,79 @@ ALTER TABLE `GuestVmScriptExecutedRecordDetailVO`
MODIFY `stdout` MEDIUMTEXT CHARACTER SET `utf8mb4` COLLATE `utf8mb4_unicode_ci`;

ALTER TABLE `GuestVmScriptExecutedRecordDetailVO`
MODIFY `stderr` MEDIUMTEXT CHARACTER SET `utf8mb4` COLLATE `utf8mb4_unicode_ci`;
MODIFY `stderr` MEDIUMTEXT CHARACTER SET `utf8mb4` COLLATE `utf8mb4_unicode_ci`;

DELIMITER $$

CREATE PROCEDURE UpdateBareMetal2InstanceProvisionNicVO()
BEGIN
DECLARE instanceUuid_exists INT;
DECLARE isPrimaryProvisionNic_exists_in_ProvisionNicVO INT;
DECLARE isPrimaryProvisionNic_exists_in_ChassisNicVO INT;

DECLARE EXIT HANDLER FOR SQLEXCEPTION
BEGIN
ROLLBACK;
SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'An error occurred during the update process.';
END;

START TRANSACTION;

SELECT COUNT(*)
INTO instanceUuid_exists
FROM INFORMATION_SCHEMA.COLUMNS
WHERE TABLE_SCHEMA = 'zstack'
AND TABLE_NAME = 'BareMetal2InstanceProvisionNicVO'
AND COLUMN_NAME = 'instanceUuid';

IF instanceUuid_exists = 0 THEN
CALL ADD_COLUMN('BareMetal2InstanceProvisionNicVO', 'instanceUuid', 'VARCHAR(32)', FALSE, '');

UPDATE `zstack`.`BareMetal2InstanceProvisionNicVO`
SET `instanceUuid` = `uuid`;

ALTER TABLE `zstack`.`BareMetal2InstanceProvisionNicVO`
DROP FOREIGN KEY `fkBareMetal2InstanceProvisionNicVOInstanceVO`;
Comment on lines +36 to +37
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

DROP FOREIGN KEY 缺少存在性检查，可能导致执行失败

当外键 fkBareMetal2InstanceProvisionNicVOInstanceVO 不存在时（例如新数据库或脚本重复执行），此语句会抛出异常。

建议在执行前检查外键是否存在：

🔧 建议修复：添加外键存在性检查 
    IF instanceUuid_exists = 0 THEN
        CALL ADD_COLUMN('BareMetal2InstanceProvisionNicVO', 'instanceUuid', 'VARCHAR(32)', FALSE, '');

        UPDATE `zstack`.`BareMetal2InstanceProvisionNicVO`
        SET `instanceUuid` = `uuid`;

+        -- Check if foreign key exists before dropping
+        IF EXISTS (
+            SELECT 1 FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS
+            WHERE TABLE_SCHEMA = 'zstack'
+              AND TABLE_NAME = 'BareMetal2InstanceProvisionNicVO'
+              AND CONSTRAINT_NAME = 'fkBareMetal2InstanceProvisionNicVOInstanceVO'
+              AND CONSTRAINT_TYPE = 'FOREIGN KEY'
+        ) THEN
             ALTER TABLE `zstack`.`BareMetal2InstanceProvisionNicVO`
             DROP FOREIGN KEY `fkBareMetal2InstanceProvisionNicVOInstanceVO`;
+        END IF;

        CALL ADD_CONSTRAINT('BareMetal2InstanceProvisionNicVO', 'fkBareMetal2InstanceProvisionNicVOInstanceVO',
                                    'instanceUuid', 'BareMetal2InstanceVO', 'uuid', 'CASCADE');

        UPDATE `zstack`.`BareMetal2InstanceProvisionNicVO`
        SET `uuid` = REPLACE(UUID(), '-', '');
    END IF;
📝 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
ALTER TABLE `zstack`.`BareMetal2InstanceProvisionNicVO`
DROP FOREIGN KEY `fkBareMetal2InstanceProvisionNicVOInstanceVO`;
-- Check if foreign key exists before dropping
IF EXISTS (
SELECT 1 FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS
WHERE TABLE_SCHEMA = 'zstack'
AND TABLE_NAME = 'BareMetal2InstanceProvisionNicVO'
AND CONSTRAINT_NAME = 'fkBareMetal2InstanceProvisionNicVOInstanceVO'
AND CONSTRAINT_TYPE = 'FOREIGN KEY'
) THEN
ALTER TABLE `zstack`.`BareMetal2InstanceProvisionNicVO`
DROP FOREIGN KEY `fkBareMetal2InstanceProvisionNicVOInstanceVO`;
END IF;
🤖 Prompt for AI Agents 
In @conf/db/upgrade/V5.5.6__schema.sql around lines 30 - 31, ALTER TABLE DROP
FOREIGN KEY is executed unconditionally and will fail if
fkBareMetal2InstanceProvisionNicVOInstanceVO doesn't exist; modify
V5.5.6__schema.sql to first check information_schema for a constraint named
fkBareMetal2InstanceProvisionNicVOInstanceVO on table
BareMetal2InstanceProvisionNicVO in schema zstack and only run ALTER TABLE
`zstack`.`BareMetal2InstanceProvisionNicVO` DROP FOREIGN KEY
`fkBareMetal2InstanceProvisionNicVOInstanceVO` when that check returns true,
ensuring the migration is idempotent and safe to re-run.


CALL ADD_CONSTRAINT('BareMetal2InstanceProvisionNicVO', 'fkBareMetal2InstanceProvisionNicVOInstanceVO',
'instanceUuid', 'BareMetal2InstanceVO', 'uuid', 'CASCADE');
Comment on lines +36 to +40
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

外键删除操作缺少存在性检查，影响脚本幂等性

DROP FOREIGN KEY 操作在执行前没有检查外键是否存在。如果数据库处于部分升级状态（外键已被删除但列迁移未完成），此操作会失败并导致整个事务回滚。

建议在删除外键前先检查其是否存在，类似于列的存在性检查方式：

🔧 建议的修复方案 
+    -- Check if foreign key exists before dropping
+    SELECT COUNT(*)
+        INTO @fk_exists
+        FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS
+        WHERE TABLE_SCHEMA = 'zstack'
+          AND TABLE_NAME = 'BareMetal2InstanceProvisionNicVO'
+          AND CONSTRAINT_NAME = 'fkBareMetal2InstanceProvisionNicVOInstanceVO'
+          AND CONSTRAINT_TYPE = 'FOREIGN KEY';
+
+    IF @fk_exists > 0 THEN
         ALTER TABLE `zstack`.`BareMetal2InstanceProvisionNicVO`
         DROP FOREIGN KEY `fkBareMetal2InstanceProvisionNicVOInstanceVO`;
+    END IF;

Based on learnings，ZStack 升级脚本需要通过 information_schema 检查表和约束的存在性来确保脚本可以安全重复执行。

🤖 Prompt for AI Agents 
In @conf/db/upgrade/V5.5.6__schema.sql around lines 30 - 34, The DROP FOREIGN
KEY for fkBareMetal2InstanceProvisionNicVOInstanceVO on table
BareMetal2InstanceProvisionNicVO lacks an existence check and breaks
idempotency; modify V5.5.6__schema.sql to query information_schema
(constraints/KEY_COLUMN_USAGE) to confirm the constraint exists before executing
ALTER TABLE `BareMetal2InstanceProvisionNicVO` DROP FOREIGN KEY
`fkBareMetal2InstanceProvisionNicVOInstanceVO`, and only run the DROP when the
check returns a positive match so the upgrade script is safe to re-run; keep the
subsequent CALL ADD_CONSTRAINT(...) as-is after the conditional drop.


UPDATE `zstack`.`BareMetal2InstanceProvisionNicVO`
SET `uuid` = REPLACE(UUID(), '-', '');
Comment on lines +42 to +43
Copy link

@coderabbitai coderabbitai bot Jan 15, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# 搜索 Java 代码中是否有其他表引用 BareMetal2InstanceProvisionNicVO.uuid

# 查找 BareMetal2InstanceProvisionNicVO 相关的引用
rg -n "BareMetal2InstanceProvisionNicVO" --type java -C 3

# 搜索 SQL schema 中的外键定义
rg -n "BareMetal2InstanceProvisionNic" --glob "*.sql" -C 3

# 查找 VO 定义文件
fd "BareMetal2InstanceProvisionNicVO.java" --type f

Repository: MatheMatrix/zstack

Length of output: 6310

🏁 Script executed:

# 查看升级脚本的完整上下文
head -50 conf/db/upgrade/V5.5.6__schema.sql
tail -20 conf/db/upgrade/V5.5.6__schema.sql
wc -l conf/db/upgrade/V5.5.6__schema.sql

Repository: MatheMatrix/zstack

Length of output: 2669

🏁 Script executed:

# 检查是否有外键约束引用此表的 uuid
rg -n "FOREIGN KEY" conf/db/upgrade/V5.5.6__schema.sql -C 3

Repository: MatheMatrix/zstack

Length of output: 466

🏁 Script executed:

# 搜索所有引用 BareMetal2InstanceProvisionNicVO.uuid 的外键约束
rg -n "BareMetal2InstanceProvisionNicVO" conf/db/upgrade/ --glob "*.sql" -B 2 -A 2 | rg -i "foreign|constraint|reference"

# 具体查看 BareMetal2BondingNicRefVO 相关的处理
rg -n "BareMetal2BondingNicRefVO" conf/db/upgrade/ --glob "*.sql" -C 3

# 检查 V5.5.6 脚本中是否处理了 provisionNicUuid
grep -n "provisionNicUuid\|BondingNicRefVO" conf/db/upgrade/V5.5.6__schema.sql

Repository: MatheMatrix/zstack

Length of output: 2286

🏁 Script executed:

# 查看完整的 V4.7.13 脚本中关于 BondingNicRefVO 的处理
cat conf/db/upgrade/V4.7.13__schema.sql | grep -n "BondingNicRefVO" -A 3 -B 3

Repository: MatheMatrix/zstack

Length of output: 611

🏁 Script executed:

# 检查 upgrade 脚本中是否有对 provisionNicUuid 的更新操作
rg -n "provisionNicUuid" conf/db/upgrade/ --glob "*.sql"

Repository: MatheMatrix/zstack

Length of output: 399

🏁 Script executed:

# 查看 ADD_CONSTRAINT 函数定义，了解默认的 ON UPDATE 行为
rg -n "CREATE PROCEDURE.*ADD_CONSTRAINT\|CREATE FUNCTION.*ADD_CONSTRAINT" conf/db/ --glob "*.sql" -A 20

# 查看所有提到 provisionNicUuid 的脚本
grep -r "provisionNicUuid" conf/db/upgrade/ --include="*.sql"

# 检查 BareMetal2BondingNicRefVO 在其他版本中是否有修改
rg -n "BareMetal2BondingNicRefVO" conf/db/upgrade/ --glob "*.sql"

Repository: MatheMatrix/zstack

Length of output: 958

🏁 Script executed:

# 查看 FK 的完整定义，包括 ON UPDATE 和 ON DELETE 选项
rg -n "fkBareMetal2BondingNicRefVOProvisionNicVO\|FOREIGN KEY.*provisionNicUuid" conf/db/upgrade/ --glob "*.sql" -B 3 -A 3

Repository: MatheMatrix/zstack

Length of output: 44

🏁 Script executed:

# 查看 V5.5.6 脚本的完整内容，确保没有遗漏任何 FK 处理
sed -n '1,80p' conf/db/upgrade/V5.5.6__schema.sql

Repository: MatheMatrix/zstack

Length of output: 2996

🏁 Script executed:

# 查找 ADD_CONSTRAINT 函数的实现
rg -n "CREATE PROCEDURE\|CREATE FUNCTION" conf/db/ --glob "*.sql" -A 10 | grep -A 10 "ADD_CONSTRAINT"

# 直接查看初始化脚本或工具脚本
fd "init.*\.sql\|tool.*\.sql\|procedure" conf/db/ --type f -x head -20 {}

Repository: MatheMatrix/zstack

Length of output: 44

🏁 Script executed:

# 检查是否有其他脚本中处理了 BareMetal2BondingNicRefVO 的 provisionNicUuid
for file in conf/db/upgrade/V*.sql; do
  if grep -q "provisionNicUuid\|BareMetal2BondingNicRefVO" "$file"; then
    echo "=== $file ==="
    grep -n "provisionNicUuid\|BareMetal2BondingNicRefVO" "$file"
  fi
done

# 查看是否有删除或修改这个 FK 的操作
rg -n "DROP.*fkBareMetal2BondingNicRefVOProvisionNicVO\|ALTER TABLE.*BareMetal2BondingNicRefVO" conf/db/upgrade/ --glob "*.sql"

Repository: MatheMatrix/zstack

Length of output: 570

🏁 Script executed:

# 查找初始化脚本中 ADD_CONSTRAINT 的定义
find conf/db -name "*.sql" -type f | xargs grep -l "CREATE PROCEDURE.*ADD_CONSTRAINT" | head -1 | xargs cat | head -100

Repository: MatheMatrix/zstack

Length of output: 2994

🏁 Script executed:

# 查找 ADD_CONSTRAINT 函数的完整定义，特别是关于 ON UPDATE 的部分
rg -n "DROP PROCEDURE.*ADD_CONSTRAINT|CREATE PROCEDURE.*ADD_CONSTRAINT" conf/db/ --glob "*.sql" -A 50 | head -100

Repository: MatheMatrix/zstack

Length of output: 3762

🏁 Script executed:

# 也可以从所有 SQL 文件中搜索
cat conf/db/init.sql 2>/dev/null | grep -A 50 "ADD_CONSTRAINT" | head -100

Repository: MatheMatrix/zstack

Length of output: 44

🏁 Script executed:

# 如果 init.sql 不存在，查找所有包含 ADD_CONSTRAINT 定义的文件
find conf/db -name "*.sql" -type f -exec grep -l "CREATE PROCEDURE.*ADD_CONSTRAINT\|DROP PROCEDURE.*ADD_CONSTRAINT" {} \;

Repository: MatheMatrix/zstack

Length of output: 95

🏁 Script executed:

# 查看 ADD_CONSTRAINT 的完整定义，确认是否包含 ON UPDATE 部分
sed -n '184,213p' conf/db/upgrade/beforeMigrate.sql

# 检查是否在其他脚本中修改或删除了 fkBareMetal2BondingNicRefVOProvisionNicVO
rg -n "fkBareMetal2BondingNicRefVOProvisionNicVO" conf/db/ --glob "*.sql" -B 2 -A 2

# 查看所有涉及 uuid 列修改的升级脚本，看是否有处理 FK 的模式
rg -n "UPDATE.*uuid.*REPLACE\|UPDATE.*uuid.*UUID" conf/db/upgrade/ --glob "*.sql" -B 5 -A 2

Repository: MatheMatrix/zstack

Length of output: 1631

🏁 Script executed:

# 确认 MySQL 的默认 FK ON UPDATE 行为
# 查看脚本中是否有 ON UPDATE CASCADE 的其他示例
rg -n "ON UPDATE CASCADE\|ON UPDATE RESTRICT\|ON UPDATE" conf/db/upgrade/ --glob "*.sql" | head -20

Repository: MatheMatrix/zstack

Length of output: 44

🏁 Script executed:

# 检查是否有其他地方更新了 provisionNicUuid
rg -n "provisionNicUuid" conf/db/upgrade/ --glob "*.sql"

# 查看 MySQL FK 的默认行为，搜索是否有文档说明
rg -n "ON UPDATE" conf/db/upgrade/ --glob "*.sql"

Repository: MatheMatrix/zstack

Length of output: 50374

🏁 Script executed:

# 最重要的：检查 V5.5.6 脚本执行时是否会因为 FK 约束而失败
# 看看脚本中是否在更新 uuid 之前处理了 BareMetal2BondingNicRefVO 的 FK

# 完整查看 V5.5.6 脚本的 IF 块逻辑
cat conf/db/upgrade/V5.5.6__schema.sql | sed -n '30,44p'

Repository: MatheMatrix/zstack

Length of output: 763

处理未考虑到的外键约束导致 uuid 更新失败

此脚本删除了 fkBareMetal2InstanceProvisionNicVOInstanceVO 外键约束，但忽略了来自 BareMetal2BondingNicRefVO.provisionNicUuid 的外键约束（在 V4.7.13 创建）。当执行 UPDATE uuid = REPLACE(UUID(), '-', '') 时，由于 fkBareMetal2BondingNicRefVOProvisionNicVO 的 ON UPDATE RESTRICT 默认行为，该更新操作将被拒绝，导致整个事务回滚。

需要在更新 uuid 之前处理此 FK 约束，可选方案：

  1. 在更新前临时禁用外键约束检查（SET FOREIGN_KEY_CHECKS = 0）
  2. 删除 fkBareMetal2BondingNicRefVOProvisionNicVO 外键，更新后重建
  3. 同时更新 BareMetal2BondingNicRefVO.provisionNicUuid 中的引用值

END IF;

SELECT COUNT(*)
INTO isPrimaryProvisionNic_exists_in_ProvisionNicVO
FROM INFORMATION_SCHEMA.COLUMNS
WHERE TABLE_SCHEMA = 'zstack'
AND TABLE_NAME = 'BareMetal2InstanceProvisionNicVO'
AND COLUMN_NAME = 'isPrimaryProvisionNic';

IF isPrimaryProvisionNic_exists_in_ProvisionNicVO = 0 THEN
CALL ADD_COLUMN('BareMetal2InstanceProvisionNicVO', 'isPrimaryProvisionNic', 'BOOLEAN', FALSE, FALSE);

UPDATE `zstack`.`BareMetal2InstanceProvisionNicVO`
SET `isPrimaryProvisionNic` = TRUE;
END IF;

SELECT COUNT(*)
INTO isPrimaryProvisionNic_exists_in_ChassisNicVO
FROM INFORMATION_SCHEMA.COLUMNS
WHERE TABLE_SCHEMA = 'zstack'
AND TABLE_NAME = 'BareMetal2ChassisNicVO'
AND COLUMN_NAME = 'isPrimaryProvisionNic';

IF isPrimaryProvisionNic_exists_in_ChassisNicVO = 0 THEN
CALL ADD_COLUMN('BareMetal2ChassisNicVO', 'isPrimaryProvisionNic', 'BOOLEAN', FALSE, FALSE);

UPDATE `zstack`.`BareMetal2ChassisNicVO`
SET `isPrimaryProvisionNic` = TRUE
WHERE `isProvisionNic` = TRUE;
END IF;

COMMIT;
END$$

DELIMITER ;
CALL UpdateBareMetal2InstanceProvisionNicVO();
DROP PROCEDURE IF EXISTS UpdateBareMetal2InstanceProvisionNicVO;
64 changes: 64 additions & 0 deletions plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,12 @@
import org.zstack.header.storage.primary.*;
import org.zstack.header.storage.primary.VolumeSnapshotCapability.VolumeSnapshotArrangementType;
import org.zstack.header.storage.snapshot.*;
import org.zstack.header.tag.SystemTagVO;
import org.zstack.header.tag.SystemTagVO_;
import org.zstack.header.vm.VmInstanceSpec;
import org.zstack.header.vm.VmInstanceSpec.ImageSpec;
import org.zstack.header.vm.VmInstanceVO;
import org.zstack.header.vm.VmInstanceVO_;
import org.zstack.header.vo.ResourceVO;
import org.zstack.header.volume.*;
import org.zstack.identity.AccountManager;
Expand Down Expand Up @@ -485,6 +489,8 @@ public static class CloneRsp extends AgentResponse {
public Long size;
public Long actualSize;
public String installPath;
public String volumeId;
public String volumeStatus;

public String getInstallPath() {
return installPath;
Expand All @@ -493,6 +499,22 @@ public String getInstallPath() {
public void setInstallPath(String installPath) {
this.installPath = installPath;
}

public String getVolumeId() {
return volumeId;
}

public void setVolumeId(String volumeId) {
this.volumeId = volumeId;
}

public String getVolumeStatus() {
return volumeStatus;
}

public void setVolumeStatus(String volumeStatus) {
this.volumeStatus = volumeStatus;
}
}

public static class FlattenCmd extends AgentCommand implements HasThreadContext {
Expand Down Expand Up @@ -2636,6 +2658,40 @@ public void run(final FlowTrigger trigger, Map data) {
CloneCmd cmd = new CloneCmd();
cmd.srcPath = cloneInstallPath;
cmd.dstPath = volumePath;
List<CephPrimaryStorageCheckInstanceTypeExtensionPoint> exts = pluginRgty.getExtensionList(CephPrimaryStorageCheckInstanceTypeExtensionPoint.class);
for (CephPrimaryStorageCheckInstanceTypeExtensionPoint ext : exts) {
Boolean result = ext.isSupportCloneByThirdParty(msg.getVolume().getVmInstanceUuid());
if (!result) {
break;
}
boolean isRootVolume = Q.New(VolumeVO.class)
.eq(VolumeVO_.uuid, msg.getVolume().getUuid())
.eq(VolumeVO_.type, VolumeType.Root)
.isExists();
boolean isBareMetal2Instance = Q.New(VmInstanceVO.class)
.eq(VmInstanceVO_.uuid, msg.getVolume().getVmInstanceUuid())
.eq(VmInstanceVO_.type, "baremetal2")
.isExists();
boolean hasToken = CephSystemTags.THIRDPARTY_PLATFORM.hasTag(msg.getPrimaryStorageUuid());
if (isRootVolume && isBareMetal2Instance && hasToken) {
CephPrimaryStorageVO cephPrimaryStorageVO = dbf.findByUuid(msg.getPrimaryStorageUuid(), CephPrimaryStorageVO.class);
String monIp = cephPrimaryStorageVO.getMons()
.stream()
.filter(v -> v.getStatus() == MonStatus.Connected)
.map(CephPrimaryStorageMonVO::getHostname)
.findAny()
.orElseThrow(() -> new OperationFailureException(
operr("all ceph mons of primary storage[uuid:%s] are not in Connected state", cephPrimaryStorageVO.getUuid())
));
cmd.token = CephSystemTags.THIRDPARTY_PLATFORM.getTokenByResourceUuid(msg.getPrimaryStorageUuid(),
CephSystemTags.THIRDPARTY_PLATFORM_TOKEN);
cmd.monIp = monIp;
cmd.tpTimeout = CephGlobalConfig.THIRD_PARTY_SDK_TIMEOUT.value(String.class);
VolumeVO vo = Q.New(VolumeVO.class)
.eq(VolumeVO_.uuid, msg.getVolume().getUuid()).find();
ext.convertToBlockVolume(vo);
Comment on lines +2676 to +2692
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

cmd.monIp 在这里赋值很可能会被 HttpCaller.doCall() 覆盖，导致第三方 clone 依赖的 monIp 不可控。
HttpCaller.doCall() 每次发送前都会 cmd.monIp = base.getSelf().getHostname();，因此你在 clone flow 里提前写入的 cmd.monIp 可能根本不会到 agent（或变成随机挑选的 mon）。如果第三方 SDK 需要“指定的 monIp”，这会直接造成功能不稳定。

修复方向（示例）：HttpCaller 仅在 monIp 为空时填充 
-            cmd.monUuid = base.getSelf().getUuid();
-            cmd.monIp = base.getSelf().getHostname();
+            cmd.monUuid = base.getSelf().getUuid();
+            if (cmd.monIp == null) {
+                cmd.monIp = base.getSelf().getHostname();
+            }

如果 monIp 语义需要区分“ceph mon 用于 httpCall 的地址”和“第三方平台 SDK 需要的 monIp”，建议新增独立字段（例如 thirdPartyMonIp）而不是复用 monIp。

Also applies to: 3624-3636

🤖 Prompt for AI Agents 
In
@plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java
around lines 2675 - 2691, The code sets cmd.monIp in CephPrimaryStorageBase but
HttpCaller.doCall() unconditionally overwrites cmd.monIp, so third-party SDKs
cannot rely on the specified mon; fix by either making HttpCaller.doCall()
populate cmd.monIp only when it is null (check HttpCaller.doCall() and change
assignment to if (cmd.monIp == null) cmd.monIp = base.getSelf().getHostname();)
or introduce a distinct field (e.g., cmd.thirdPartyMonIp) set in
CephPrimaryStorageBase (where cmd.monIp is currently assigned) and update all
call sites and the agent/HttpCaller to use thirdPartyMonIp for SDK usage while
preserving monIp for HTTP routing; update references in CephPrimaryStorageBase
(cmd.monIp assignment near ext.convertToBlockVolume(...) and similar blocks at
the other noted location) and in HttpCaller.doCall()/agent handling to use the
new conditional or new field consistently.

}
Comment on lines +2676 to +2693
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

convertToBlockVolume() 放在 clone 之前且无回滚，失败路径可能把 Volume 状态改脏。
这段在 NoRollbackFlow 内执行：一旦 httpCall(CLONE_PATH...) 失败，当前 flow 会 fail，但 DB 侧转换可能已发生；后续重试/清理可能遇到不一致状态。至少需要：

  • 要么把转换放到 clone 成功后；
  • 要么把该 flow 改成可 rollback，并在 rollback 里恢复转换（或让扩展点提供 revert）。

}
Comment on lines +2661 to +2694
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

第三方 clone 扩展点循环的控制流有明显 bug：break 会阻断后续扩展点；且 Boolean 可能 NPE。

  • if (!result) { break; }：一旦第一个扩展返回 false，就不会再检查其他扩展（通常应 continue）。
  • Boolean result 直接 !result：返回 null 会 NPE。
  • 另外，isRootVolume/isBareMetal2Instance/hasToken 每次循环都会重复 DB 查询，建议提到循环外计算一次。
可行修复（continue + null-safe + 提前计算条件） 
-                        List<CephPrimaryStorageCheckInstanceTypeExtensionPoint> exts = pluginRgty.getExtensionList(CephPrimaryStorageCheckInstanceTypeExtensionPoint.class);
-                        for (CephPrimaryStorageCheckInstanceTypeExtensionPoint ext : exts) {
-                            Boolean result = ext.isSupportCloneByThirdParty(msg.getVolume().getVmInstanceUuid());
-                            if (!result) {
-                                break;
-                            }
-                            boolean isRootVolume = Q.New(VolumeVO.class)
-                                    .eq(VolumeVO_.uuid, msg.getVolume().getUuid())
-                                    .eq(VolumeVO_.type, VolumeType.Root)
-                                    .isExists();
-                            boolean isBareMetal2Instance = Q.New(VmInstanceVO.class)
-                                    .eq(VmInstanceVO_.uuid, msg.getVolume().getVmInstanceUuid())
-                                    .eq(VmInstanceVO_.type, "baremetal2")
-                                    .isExists();
-                            boolean hasToken = CephSystemTags.THIRDPARTY_PLATFORM.hasTag(msg.getPrimaryStorageUuid());
-                            if (isRootVolume && isBareMetal2Instance && hasToken) {
+                        List<CephPrimaryStorageCheckInstanceTypeExtensionPoint> exts =
+                                pluginRgty.getExtensionList(CephPrimaryStorageCheckInstanceTypeExtensionPoint.class);
+
+                        boolean isRootVolume = Q.New(VolumeVO.class)
+                                .eq(VolumeVO_.uuid, msg.getVolume().getUuid())
+                                .eq(VolumeVO_.type, VolumeType.Root)
+                                .isExists();
+                        boolean isBareMetal2Instance = Q.New(VmInstanceVO.class)
+                                .eq(VmInstanceVO_.uuid, msg.getVolume().getVmInstanceUuid())
+                                .eq(VmInstanceVO_.type, "baremetal2")
+                                .isExists();
+                        boolean hasToken = CephSystemTags.THIRDPARTY_PLATFORM.hasTag(msg.getPrimaryStorageUuid());
+
+                        for (CephPrimaryStorageCheckInstanceTypeExtensionPoint ext : exts) {
+                            if (!Boolean.TRUE.equals(ext.isSupportCloneByThirdParty(msg.getVolume().getVmInstanceUuid()))) {
+                                continue;
+                            }
+                            if (isRootVolume && isBareMetal2Instance && hasToken) {
                                 CephPrimaryStorageVO cephPrimaryStorageVO = dbf.findByUuid(msg.getPrimaryStorageUuid(), CephPrimaryStorageVO.class);
                                 String monIp = cephPrimaryStorageVO.getMons()
                                         .stream()
                                         .filter(v -> v.getStatus() == MonStatus.Connected)
                                         .map(CephPrimaryStorageMonVO::getHostname)
                                         .findAny()
                                         .orElseThrow(() -> new OperationFailureException(
                                                 operr("all ceph mons of primary storage[uuid:%s] are not in Connected state", cephPrimaryStorageVO.getUuid())
                                         ));
                                 cmd.token = CephSystemTags.THIRDPARTY_PLATFORM.getTokenByResourceUuid(msg.getPrimaryStorageUuid(),
                                         CephSystemTags.THIRDPARTY_PLATFORM_TOKEN);
                                 cmd.monIp = monIp;
                                 cmd.tpTimeout = CephGlobalConfig.THIRD_PARTY_SDK_TIMEOUT.value(String.class);
                                 VolumeVO vo = Q.New(VolumeVO.class)
                                         .eq(VolumeVO_.uuid, msg.getVolume().getUuid()).find();
                                 ext.convertToBlockVolume(vo);
                             }
                         }

另外强烈建议把 "baremetal2" 抽成常量/枚举，避免魔法值扩散。


httpCall(CLONE_PATH, cmd, CloneRsp.class, new ReturnValueCompletion<CloneRsp>(trigger) {
@Override
Expand All @@ -2646,6 +2702,14 @@ public void fail(ErrorCode err) {
@Override
public void success(CloneRsp ret) {
actualSize = ret.actualSize;
List<CephPrimaryStorageCheckInstanceTypeExtensionPoint> exts = pluginRgty.getExtensionList(CephPrimaryStorageCheckInstanceTypeExtensionPoint.class);
for (CephPrimaryStorageCheckInstanceTypeExtensionPoint ext : exts) {
if (!ext.isBlockVolume(msg.getVolume().getUuid())) {
break;
}
volumePath = ret.installPath == null ? volumePath : makeVolumeInstallPathByTargetPool(ret.installPath, targetCephPoolName);
ext.populateBlockVolumeDetails(msg.getVolume().getUuid(), ret.volumeId, ret.volumeStatus);
}
Comment on lines +2705 to +2712
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

成功回调里的扩展点循环同样不应 break；且 installPath 重写逻辑可能拼错路径。

  • if (!ext.isBlockVolume(...)) { break; }：同样会阻断后续扩展；更合理是 continue（除非你能保证扩展点只有一个）。
  • volumePath = ... makeVolumeInstallPathByTargetPool(ret.installPath, targetCephPoolName)：如果 ret.installPath 已经是完整的 ceph://... 或 pool/uuid，这里会生成错误路径。至少应先判断前缀并保底。
可行修复（continue + installPath 前缀判断） 
                                 List<CephPrimaryStorageCheckInstanceTypeExtensionPoint> exts = pluginRgty.getExtensionList(CephPrimaryStorageCheckInstanceTypeExtensionPoint.class);
                                 for (CephPrimaryStorageCheckInstanceTypeExtensionPoint ext : exts) {
                                     if (!ext.isBlockVolume(msg.getVolume().getUuid())) {
-                                        break;
+                                        continue;
                                     }
-                                    volumePath = ret.installPath == null ? volumePath : makeVolumeInstallPathByTargetPool(ret.installPath, targetCephPoolName);
+                                    if (ret.installPath != null) {
+                                        volumePath = ret.installPath.startsWith("ceph://")
+                                                ? ret.installPath
+                                                : makeVolumeInstallPathByTargetPool(ret.installPath, targetCephPoolName);
+                                    }
                                     ext.populateBlockVolumeDetails(msg.getVolume().getUuid(), ret.volumeId, ret.volumeStatus);
                                 }
🤖 Prompt for AI Agents 
In
@plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java
around lines 2704 - 2711, In the success-callback loop in CephPrimaryStorageBase
where you iterate CephPrimaryStorageCheckInstanceTypeExtensionPoint, replace the
current `if (!ext.isBlockVolume(...)) { break; }` with `continue` so one
non-matching extension doesn't stop others, and change the installPath rewrite
to only call makeVolumeInstallPathByTargetPool when ret.installPath is non-null
and not already a Ceph install path for the target pool (e.g., check
ret.installPath.startsWith("ceph://") or startsWith(targetCephPoolName + "/") or
a similar prefix/pattern); otherwise leave volumePath alone—use these checks
around the assignment that currently does `volumePath = ret.installPath == null
? volumePath : makeVolumeInstallPathByTargetPool(ret.installPath,
targetCephPoolName)`.

trigger.next();
}
});
Expand Down
13 changes: 13 additions & 0 deletions ...va/org/zstack/storage/ceph/primary/CephPrimaryStorageCheckInstanceTypeExtensionPoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package org.zstack.storage.ceph.primary;

import org.zstack.header.volume.VolumeVO;

public interface CephPrimaryStorageCheckInstanceTypeExtensionPoint {
Boolean isSupportCloneByThirdParty(String uuid);

void convertToBlockVolume(VolumeVO vo);

Boolean isBlockVolume(String uuid);

void populateBlockVolumeDetails(String uuid, String volumeId, String volumeStatus);
}
Comment on lines +5 to +13
Copy link

@coderabbitai coderabbitai bot Jan 15, 2026

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

建议把 Boolean 改为 boolean 并明确入参语义（避免 NPE/歧义）

isSupportCloneByThirdParty() / isBlockVolume() 返回 Boolean 容易在调用方出现 !xxx 的 NPE；另外多个入参命名为 uuid 不利于实现方理解（vmUuid vs volumeUuid）。建议在接口层直接收敛为 primitive + 明确命名，并补齐 Javadoc 约束（幂等/是否落库/失败回滚策略）。

Proposed diff 
 public interface CephPrimaryStorageCheckInstanceTypeExtensionPoint {
-    Boolean isSupportCloneByThirdParty(String uuid);
+    boolean isSupportCloneByThirdParty(String vmInstanceUuid);

-    void convertToBlockVolume(VolumeVO vo);
+    void convertToBlockVolume(VolumeVO volume);

-    Boolean isBlockVolume(String uuid);
+    boolean isBlockVolume(String volumeUuid);

-    void populateBlockVolumeDetails(String uuid, String volumeId, String volumeStatus);
+    void populateBlockVolumeDetails(String volumeUuid, String volumeId, String volumeStatus);
 }
🤖 Prompt for AI Agents 
In
`@plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageCheckInstanceTypeExtensionPoint.java`
around lines 5 - 13, Change the nullable Boolean return types to primitive
boolean for isSupportCloneByThirdParty and isBlockVolume to avoid NPEs (update
CephPrimaryStorageCheckInstanceTypeExtensionPoint interface accordingly), rename
ambiguous parameter names to explicit ones (e.g., use vmUuid for VM-related IDs
and volumeUuid or volumeId for volume-related IDs) in method signatures
convertToBlockVolume(VolumeVO vo) and populateBlockVolumeDetails(...) to make
intent clear, and add concise Javadoc on each method describing semantics
(idempotency, whether changes are persisted, and rollback/error behavior) so
implementers know expected behavior and null handling.

8 changes: 8 additions & 0 deletions sdk/src/main/java/org/zstack/sdk/BareMetal2ChassisNicInventory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ public java.lang.Boolean getIsProvisionNic() {
return this.isProvisionNic;
}

public java.lang.Boolean isPrimaryProvisionNic;
public void setIsPrimaryProvisionNic(java.lang.Boolean isPrimaryProvisionNic) {
this.isPrimaryProvisionNic = isPrimaryProvisionNic;
}
public java.lang.Boolean getIsPrimaryProvisionNic() {
return this.isPrimaryProvisionNic;
}

public java.sql.Timestamp createDate;
public void setCreateDate(java.sql.Timestamp createDate) {
this.createDate = createDate;
Expand Down
12 changes: 6 additions & 6 deletions sdk/src/main/java/org/zstack/sdk/BareMetal2InstanceInventory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.zstack.sdk;

import org.zstack.sdk.BareMetal2InstanceProvisionNicInventory;


public class BareMetal2InstanceInventory extends org.zstack.sdk.VmInstanceInventory {

Expand Down Expand Up @@ -84,12 +84,12 @@ public boolean getIsLatestAgent() {
return this.isLatestAgent;
}

public BareMetal2InstanceProvisionNicInventory provisionNic;
public void setProvisionNic(BareMetal2InstanceProvisionNicInventory provisionNic) {
this.provisionNic = provisionNic;
public java.util.List provisionNics;
public void setProvisionNics(java.util.List provisionNics) {
this.provisionNics = provisionNics;
}
public BareMetal2InstanceProvisionNicInventory getProvisionNic() {
return this.provisionNic;
public java.util.List getProvisionNics() {
return this.provisionNics;
}

}
16 changes: 16 additions & 0 deletions sdk/src/main/java/org/zstack/sdk/BareMetal2InstanceProvisionNicInventory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,22 @@ public java.lang.String getGateway() {
return this.gateway;
}

public java.lang.String instanceUuid;
public void setInstanceUuid(java.lang.String instanceUuid) {
this.instanceUuid = instanceUuid;
}
public java.lang.String getInstanceUuid() {
return this.instanceUuid;
}

public java.lang.Boolean isPrimaryProvisionNic;
public void setIsPrimaryProvisionNic(java.lang.Boolean isPrimaryProvisionNic) {
this.isPrimaryProvisionNic = isPrimaryProvisionNic;
}
public java.lang.Boolean getIsPrimaryProvisionNic() {
return this.isPrimaryProvisionNic;
}

public java.lang.String metadata;
public void setMetadata(java.lang.String metadata) {
this.metadata = metadata;
Expand Down