<fix>[storage]: test #2598
<fix>[storage]: test #2598
Conversation
Walkthrough新增 IscsiRemoteTarget.fromUri(String) 用于解析 iscsi:// URI 并返回对象或 null；在 ExponStorageController 中为 hypervisor 为 baremetal2 的主机在 iSCSI 卷激活/撤销时改为基于卷/VM UUID 推导 IQN，并合并 volume 导入为通配符。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Parser as IscsiRemoteTarget.fromUri
participant URI as URI.parse
Caller->>Parser: fromUri(uriString)
Parser->>URI: 解析 scheme/host/port/path
alt 非 iscsi 或 路径/ID 格式非法 或 异常
Parser-->>Caller: null（记录日志）
else 合法
Parser-->>Caller: IscsiRemoteTarget 实例
end
sequenceDiagram
autonumber
participant Ctrl as ExponStorageController
participant Host as Host
participant Vol as Volume/VolumeVO
participant iSCSI as iSCSI 服务
Ctrl->>Host: 获取 hypervisor 类型
alt hypervisor = baremetal2
Ctrl->>Vol: 通过 volumeUuid 或 installPath 获取 VolumeVO
Ctrl->>Ctrl: 基于 Volume/VM UUID 推导 IQN
else 其他类型
Ctrl->>Host: 读取宿主机 initiator IQN
end
Ctrl->>iSCSI: 激活/撤销 卷（使用选定 IQN）
iSCSI-->>Ctrl: 返回结果
Estimated code review effort🎯 3 (中等) | ⏱️ ~25 分钟 Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
storage/src/main/java/org/zstack/storage/addon/primary/BlockExternalPrimaryStorageFactory.java (1)
14-14: 布尔参数语义不直观，考虑用枚举表达模式（后续演进）
shareable为布尔位，语义扩展性与可读性较弱。建议在后续版本引入枚举（如VolumeShareMode { EXCLUSIVE, SHAREABLE }）替代，并提供兼容的 default 适配层。当前 PR 可不阻塞，仅作为演进建议。
📜 Review details
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
storage/src/main/java/org/zstack/storage/addon/primary/BlockExternalPrimaryStorageFactory.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.*
⚙️ CodeRabbit configuration file
**/*.*: - 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写
Files:
storage/src/main/java/org/zstack/storage/addon/primary/BlockExternalPrimaryStorageFactory.java
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 1. API 设计要求
- API 命名：
- API 名称必须唯一，不能重复。
- API 消息类需要继承
APIMessage；其返回类必须继承APIReply或APIEvent，并在注释中用@RestResponse进行标注。- API 消息上必须添加注解
@RestRequest，并满足如下规范：
path:
- 针对资源使用复数形式。
- 当 path 中引用消息类变量时，使用
{variableName}格式。- HTTP 方法对应：
- 查询操作 →
HttpMethod.GET- 更新操作 →
HttpMethod.PUT- 创建操作 →
HttpMethod.POST- 删除操作 →
HttpMethod.DELETE- API 类需要实现
__example__方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。
2. 命名与格式规范
类名：
- 使用 UpperCamelCase 风格。
- 特殊情况：
- VO/AO/EO 类型类除外。
- 抽象类采用
Abstract或Base前缀/后缀。- 异常类应以
Exception结尾。- 测试类需要以
Test或Case结尾。方法名、参数名、成员变量和局部变量：
- 使用 lowerCamelCase 风格。
常量命名：
- 全部大写，使用下划线分隔单词。
- 要求表达清楚，避免使用含糊或不准确的名称。
包名：
- 统一使用小写，使用点分隔符，每个部分应是一个具有自然语义的英文单词（参考 Spring 框架的结构）。
命名细节：
- 避免在父子类或同一代码块中出现相同名字的成员或局部变量，防止混淆。
- 命名缩写：
- 不允许使用不必要的缩写，如：
AbsSchedulerJob、condi、Fu等。应使用完整单词提升可读性。
3. 编写自解释代码
意图表达：
- 避免使用布尔型参数造成含义不明确。例如：
- 对于
stopAgent(boolean ignoreError)，建议拆分为不同函数（如stopAgentIgnoreError()），或使用枚举表达操作类型。- 命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途（例如在常量与变量名称中，将类型词放在末尾）。
- 避免使用魔法值（Magic Value）：
直接使用未经定义的数值或字符串（如 if (status == 5)）应替换为枚举或常量。
示例：
// 错误示例：魔法值
if (user.getStatus() == 5) { ... }
// 正确示例：常量或枚举
public static final int STATUS_ACTIVE = 5;
if (user.getStatus() == STATUS_ACTIVE) { ... }
// 或使用枚举
enum UserStatus { ACTIVE, INACTIVE }
注释：
- 代码应尽量做到自解释，对少于两行的说明可以直接写在代码中。
- 对于较长的注释，需要仔细校对并随代码更新，确保内容正确。
- 接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。
4. 流程控制和结构优化
if...else 的使用：
- 应尽量减少 if...else 结构的使用，建议：
- 限制嵌套层级最多为两层，且内层不应再出现
else分支。- 尽早返回（Early Return），将条件判断中的处理逻辑提前结束或抽成独立方法。
- 使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断。
条件判断：
- if 条件表达不宜过长或过于复杂，必要时可以将条件抽成 boolean 变量描述。
代码块长度：
...
Files:
storage/src/main/java/org/zstack/storage/addon/primary/BlockExternalPrimaryStorageFactory.java
🔇 Additional comments (1)
storage/src/main/java/org/zstack/storage/addon/primary/BlockExternalPrimaryStorageFactory.java (1)
14-14: BlockExternalPrimaryStorageFactory 接口无实现或调用 仓库中未发现任何类实现该接口或调用 activeIscsiVolume，变更不会影响现有代码。
| BlockExternalPrimaryStorageBackend getBlockExternalPrimaryStorageBackend(PrimaryStorageVO vo); | ||
|
|
||
| void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable); | ||
| void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable, PrimaryStorageVO vo); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
为新增/变更的 API 方法补充英文 Javadoc，明确契约与可空性
根据规范“接口方法必须配有有效的 Javadoc 注释”，请为该方法补充用途、参数语义、异常与可空性约定（是否允许 null）。示例：
- default void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable, PrimaryStorageVO primaryStorage) {
+ /**
+ * Activate an iSCSI volume for the specified client IQN on a primary storage.
+ *
+ * @param clientIqn iSCSI initiator IQN of the client
+ * @param vol volume info to be activated
+ * @param shareable whether the volume should be activated in shareable mode
+ * @param primaryStorage the target primary storage context; when omitted by legacy callers,
+ * implementations SHOULD derive it from the volume or ignore it safely
+ * @throws IllegalArgumentException if inputs are invalid
+ * @throws RuntimeException for unexpected backend errors
+ */
+ default void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable, PrimaryStorageVO primaryStorage) {
// keep compatibility: by default ignore the extra arg and call legacy signature
activeIscsiVolume(clientIqn, vol, shareable);
}若项目已采用可空性注解（如 @Nullable/@NotNull），也请同步标注以避免 NPE 争议。
Committable suggestion skipped: line range outside the PR's diff.
🛠️ Refactor suggestion
新增参数导致接口破坏性变更，建议恢复旧签名并以 default 方法做过渡
此改动会在 5.3.46 稳定分支上直接破坏所有实现类与调用方（接口方法签名变化）。按“向后兼容原则”，建议保留旧方法签名（标记 @deprecated），并将新签名实现为 default，内部委托旧方法，给下游留出迁移窗口。同时将参数名由含糊的 vo 改为更语义化的 primaryStorage。示例修复：
- void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable, PrimaryStorageVO vo);
+ /**
+ * @deprecated Use {@link #activeIscsiVolume(String, BaseVolumeInfo, boolean, PrimaryStorageVO)}.
+ */
+ @Deprecated
+ void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable);
+
+ default void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable, PrimaryStorageVO primaryStorage) {
+ // keep compatibility: by default ignore the extra arg and call legacy signature
+ activeIscsiVolume(clientIqn, vol, shareable);
+ }说明：
- 不改变现有实现类即可通过编译；新调用方可使用新签名；逐步迁移后再移除旧方法。
- 参数命名采用完整语义，符合命名规范。
📝 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.
| void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable, PrimaryStorageVO vo); | |
| /** | |
| * @deprecated Use {@link #activeIscsiVolume(String, BaseVolumeInfo, boolean, PrimaryStorageVO)}. | |
| */ | |
| @Deprecated | |
| void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable); | |
| default void activeIscsiVolume(String clientIqn, BaseVolumeInfo vol, boolean shareable, PrimaryStorageVO primaryStorage) { | |
| // keep compatibility: by default ignore the extra arg and call legacy signature | |
| activeIscsiVolume(clientIqn, vol, shareable); | |
| } |
🤖 Prompt for AI Agents
In
storage/src/main/java/org/zstack/storage/addon/primary/BlockExternalPrimaryStorageFactory.java
around line 14, the interface method signature was changed to add a new
parameter which breaks existing implementations; restore the original method
signature (mark it @Deprecated) and add a new default method with the new
signature that delegates to the old deprecated method, and use a clearer
parameter name (primaryStorage instead of vo) in the new method; this preserves
backward compatibility so existing implementors compile, allows new callers to
use the new signature, and gives downstreams time to migrate before removing the
deprecated method.
Resolves: ZSTAC-77315 Change-Id: I6670626f746f6e617466737965776f756a766774
MatheMatrix
force-pushed
the
sync/jin.shen/test-77315@@2
branch
from
cf59c62 to
6d1d1a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.java (1)
90-103: 建议：为缺省端口的 iSCSI URI 补上默认值
URI#getPort()在 URI 未显式给出端口时会返回 -1。当前直接写入IscsiRemoteTarget，后续getResourceURI()会生成iscsi://host:-1/...这样的非法路径，容易让调用方误用。建议在解析阶段就补上 iSCSI 的默认端口 3260，或至少在缺省时显式失败，避免生成错误数据。- target.setPort(uri.getPort()); + int port = uri.getPort() == -1 ? 3260 : uri.getPort(); + target.setPort(port);
📜 Review details
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.java(2 hunks)plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.*
⚙️ CodeRabbit configuration file
**/*.*: - 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写
Files:
header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.javaplugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 1. API 设计要求
- API 命名：
- API 名称必须唯一，不能重复。
- API 消息类需要继承
APIMessage；其返回类必须继承APIReply或APIEvent，并在注释中用@RestResponse进行标注。- API 消息上必须添加注解
@RestRequest，并满足如下规范：
path:
- 针对资源使用复数形式。
- 当 path 中引用消息类变量时，使用
{variableName}格式。- HTTP 方法对应：
- 查询操作 →
HttpMethod.GET- 更新操作 →
HttpMethod.PUT- 创建操作 →
HttpMethod.POST- 删除操作 →
HttpMethod.DELETE- API 类需要实现
__example__方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。
2. 命名与格式规范
类名：
- 使用 UpperCamelCase 风格。
- 特殊情况：
- VO/AO/EO 类型类除外。
- 抽象类采用
Abstract或Base前缀/后缀。- 异常类应以
Exception结尾。- 测试类需要以
Test或Case结尾。方法名、参数名、成员变量和局部变量：
- 使用 lowerCamelCase 风格。
常量命名：
- 全部大写，使用下划线分隔单词。
- 要求表达清楚，避免使用含糊或不准确的名称。
包名：
- 统一使用小写，使用点分隔符，每个部分应是一个具有自然语义的英文单词（参考 Spring 框架的结构）。
命名细节：
- 避免在父子类或同一代码块中出现相同名字的成员或局部变量，防止混淆。
- 命名缩写：
- 不允许使用不必要的缩写，如：
AbsSchedulerJob、condi、Fu等。应使用完整单词提升可读性。
3. 编写自解释代码
意图表达：
- 避免使用布尔型参数造成含义不明确。例如：
- 对于
stopAgent(boolean ignoreError)，建议拆分为不同函数（如stopAgentIgnoreError()），或使用枚举表达操作类型。- 命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途（例如在常量与变量名称中，将类型词放在末尾）。
- 避免使用魔法值（Magic Value）：
直接使用未经定义的数值或字符串（如 if (status == 5)）应替换为枚举或常量。
示例：
// 错误示例：魔法值
if (user.getStatus() == 5) { ... }
// 正确示例：常量或枚举
public static final int STATUS_ACTIVE = 5;
if (user.getStatus() == STATUS_ACTIVE) { ... }
// 或使用枚举
enum UserStatus { ACTIVE, INACTIVE }
注释：
- 代码应尽量做到自解释，对少于两行的说明可以直接写在代码中。
- 对于较长的注释，需要仔细校对并随代码更新，确保内容正确。
- 接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。
4. 流程控制和结构优化
if...else 的使用：
- 应尽量减少 if...else 结构的使用，建议：
- 限制嵌套层级最多为两层，且内层不应再出现
else分支。- 尽早返回（Early Return），将条件判断中的处理逻辑提前结束或抽成独立方法。
- 使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断。
条件判断：
- if 条件表达不宜过长或过于复杂，必要时可以将条件抽成 boolean 变量描述。
代码块长度：
...
Files:
header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.javaplugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
| private void deactivateIscsi(String installPath, HostInventory h) { | ||
| String iqn = IscsiUtils.getHostInitiatorName(h.getUuid()); | ||
| if (h.getHypervisorType().equals("baremetal2")) { | ||
| String[] parts = installPath.split("/"); | ||
| String volumeUuid = parts[parts.length - 1]; | ||
| VolumeVO volume = dbf.findByUuid(volumeUuid, VolumeVO.class); | ||
| iqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid()); | ||
| } |
There was a problem hiding this comment.
严重：baremetal2 卷卸载时会因路径解析错误触发 NPE
installPath 的最后一段是 diskIdType_diskId（例如 wwn_...），不是卷的 UUID。这里用它去 dbf.findByUuid 必然返回 null，进而在 volume.getVmInstanceUuid() 处抛出 NPE，导致 baremetal2 场景下的卸载流程直接失败。建议复用刚新增的 IscsiRemoteTarget.fromUri 来解析路径，并通过 diskId（WWN）反查 VolumeVO，同时对查不到或缺少 VM UUID 的情况做出显式处理。
- if (h.getHypervisorType().equals("baremetal2")) {
- String[] parts = installPath.split("/");
- String volumeUuid = parts[parts.length - 1];
- VolumeVO volume = dbf.findByUuid(volumeUuid, VolumeVO.class);
- iqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid());
+ if (h.getHypervisorType().equals("baremetal2")) {
+ IscsiRemoteTarget target = IscsiRemoteTarget.fromUri(installPath);
+ if (target == null || target.getDiskId() == null) {
+ throw new RuntimeException(String.format("invalid iscsi install path[%s]", installPath));
+ }
+ VolumeVO volume = Q.New(VolumeVO.class).eq(VolumeVO_.wwn, target.getDiskId()).find();
+ if (volume == null || volume.getVmInstanceUuid() == null) {
+ throw new RuntimeException(String.format("cannot resolve vm uuid from iscsi install path[%s]", installPath));
+ }
+ iqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid());
}📝 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.
| private void deactivateIscsi(String installPath, HostInventory h) { | |
| String iqn = IscsiUtils.getHostInitiatorName(h.getUuid()); | |
| if (h.getHypervisorType().equals("baremetal2")) { | |
| String[] parts = installPath.split("/"); | |
| String volumeUuid = parts[parts.length - 1]; | |
| VolumeVO volume = dbf.findByUuid(volumeUuid, VolumeVO.class); | |
| iqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid()); | |
| } | |
| private void deactivateIscsi(String installPath, HostInventory h) { | |
| String iqn = IscsiUtils.getHostInitiatorName(h.getUuid()); | |
| if (h.getHypervisorType().equals("baremetal2")) { | |
| IscsiRemoteTarget target = IscsiRemoteTarget.fromUri(installPath); | |
| if (target == null || target.getDiskId() == null) { | |
| throw new RuntimeException(String.format("invalid iscsi install path[%s]", installPath)); | |
| } | |
| VolumeVO volume = Q.New(VolumeVO.class) | |
| .eq(VolumeVO_.wwn, target.getDiskId()) | |
| .find(); | |
| if (volume == null || volume.getVmInstanceUuid() == null) { | |
| throw new RuntimeException(String.format("cannot resolve vm uuid from iscsi install path[%s]", installPath)); | |
| } | |
| iqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid()); | |
| } |
🤖 Prompt for AI Agents
In plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
around lines 752 to 759, the code incorrectly treats the last segment of
installPath as a volume UUID causing dbf.findByUuid to return null and trigger
an NPE; replace this logic to parse installPath with IscsiRemoteTarget.fromUri
to extract the diskId (e.g. WWN), then query VolumeVO by that diskId (not by the
path segment) and handle both cases where no VolumeVO is found or the VolumeVO
lacks a vmInstanceUuid by logging an error and aborting the deactivate flow (or
returning early) to avoid NPE; keep existing iqn fallback behavior for
non-baremetal2 hosts.
Resolves: ZSTAC-77315 Change-Id: I71786673736d6473707a6d616c716c7474616c76
Resolves: ZSTAC-1111 Change-Id: I79646c6a747a6471676c6861676f787479716765
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.java(2 hunks)plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.java
🧰 Additional context used
📓 Path-based instructions (2)
**/*.*
⚙️ CodeRabbit configuration file
**/*.*: - 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写
Files:
plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 1. API 设计要求
- API 命名：
- API 名称必须唯一，不能重复。
- API 消息类需要继承
APIMessage；其返回类必须继承APIReply或APIEvent，并在注释中用@RestResponse进行标注。- API 消息上必须添加注解
@RestRequest，并满足如下规范：
path:
- 针对资源使用复数形式。
- 当 path 中引用消息类变量时，使用
{variableName}格式。- HTTP 方法对应：
- 查询操作 →
HttpMethod.GET- 更新操作 →
HttpMethod.PUT- 创建操作 →
HttpMethod.POST- 删除操作 →
HttpMethod.DELETE- API 类需要实现
__example__方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。
2. 命名与格式规范
类名：
- 使用 UpperCamelCase 风格。
- 特殊情况：
- VO/AO/EO 类型类除外。
- 抽象类采用
Abstract或Base前缀/后缀。- 异常类应以
Exception结尾。- 测试类需要以
Test或Case结尾。方法名、参数名、成员变量和局部变量：
- 使用 lowerCamelCase 风格。
常量命名：
- 全部大写，使用下划线分隔单词。
- 要求表达清楚，避免使用含糊或不准确的名称。
包名：
- 统一使用小写，使用点分隔符，每个部分应是一个具有自然语义的英文单词（参考 Spring 框架的结构）。
命名细节：
- 避免在父子类或同一代码块中出现相同名字的成员或局部变量，防止混淆。
- 命名缩写：
- 不允许使用不必要的缩写，如：
AbsSchedulerJob、condi、Fu等。应使用完整单词提升可读性。
3. 编写自解释代码
意图表达：
- 避免使用布尔型参数造成含义不明确。例如：
- 对于
stopAgent(boolean ignoreError)，建议拆分为不同函数（如stopAgentIgnoreError()），或使用枚举表达操作类型。- 命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途（例如在常量与变量名称中，将类型词放在末尾）。
- 避免使用魔法值（Magic Value）：
直接使用未经定义的数值或字符串（如 if (status == 5)）应替换为枚举或常量。
示例：
// 错误示例：魔法值
if (user.getStatus() == 5) { ... }
// 正确示例：常量或枚举
public static final int STATUS_ACTIVE = 5;
if (user.getStatus() == STATUS_ACTIVE) { ... }
// 或使用枚举
enum UserStatus { ACTIVE, INACTIVE }
注释：
- 代码应尽量做到自解释，对少于两行的说明可以直接写在代码中。
- 对于较长的注释，需要仔细校对并随代码更新，确保内容正确。
- 接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。
4. 流程控制和结构优化
if...else 的使用：
- 应尽量减少 if...else 结构的使用，建议：
- 限制嵌套层级最多为两层，且内层不应再出现
else分支。- 尽早返回（Early Return），将条件判断中的处理逻辑提前结束或抽成独立方法。
- 使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断。
条件判断：
- if 条件表达不宜过长或过于复杂，必要时可以将条件抽成 boolean 变量描述。
代码块长度：
...
Files:
plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
🧠 Learnings (1)
📚 Learning: 2025-08-25T03:52:37.301Z
Learnt from: ZStack-Robot
PR: MatheMatrix/zstack#2504
File: storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java:775-819
Timestamp: 2025-08-25T03:52:37.301Z
Learning: 在ZStack项目的VolumeSnapshotTreeBase类中，当通过dbf.findByUuid()方法获取VolumeVO对象时，需要进行null检查，因为该方法可能在找不到对应记录时返回null，直接使用可能导致NullPointerException。
Applied to files:
plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
🧬 Code graph analysis (1)
plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java (1)
plugin/iscsi/src/main/java/org/zstack/iscsi/IscsiUtils.java (1)
IscsiUtils(18-56)
🔇 Additional comments (1)
plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java (1)
756-763: baremetal2 卷卸载仍然错误解析 installPath
installPath的最后一段是diskIdType_diskId，不是卷 UUID。继续用它去dbf.findByUuid会得到null并在volume.getVmInstanceUuid()处抛 NPE，baremetal2 的卸载流程仍然会失败。请改用刚新增的IscsiRemoteTarget.fromUri抽取 WWN，再按 WWN 反查卷并做好缺失校验。示例：if (h.getHypervisorType().equals("baremetal2")) { - String[] parts = installPath.split("/"); - String volumeUuid = parts[parts.length - 1]; - VolumeVO volume = dbf.findByUuid(volumeUuid, VolumeVO.class); - iqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid()); + IscsiRemoteTarget target = IscsiRemoteTarget.fromUri(installPath); + if (target == null || StringUtils.isEmpty(target.getDiskId())) { + throw new OperationFailureException(operr("invalid iscsi install path[%s]", installPath)); + } + VolumeVO volume = Q.New(VolumeVO.class).eq(VolumeVO_.wwn, target.getDiskId()).find(); + if (volume == null || StringUtils.isEmpty(volume.getVmInstanceUuid())) { + throw new OperationFailureException(operr("cannot resolve vm uuid from iscsi install path[%s]", installPath)); + } + iqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid()); } else {
| private synchronized ActiveVolumeTO activeIscsiVolume(HostInventory h, BaseVolumeInfo vol, boolean shareable) { | ||
| String clientIqn = IscsiUtils.getHostInitiatorName(h.getUuid()); | ||
| String clientIqn; | ||
| if (h.getHypervisorType().equals("baremetal2")) { | ||
| VolumeVO volume = dbf.findByUuid(vol.getUuid(), VolumeVO.class); | ||
| clientIqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid()); | ||
| } else { | ||
| clientIqn = IscsiUtils.getHostInitiatorName(h.getUuid()); | ||
| } |
There was a problem hiding this comment.
baremetal2 卷激活缺少 Volume 空指针校验
dbf.findByUuid() 找不到记录会返回 null，目前 baremetal2 分支立刻访问 volume.getVmInstanceUuid()，会触发 NPE 并终止卷激活流程。请先校验 Volume 是否存在且挂载了 VM，再生成 IQN。修正示例：
if (h.getHypervisorType().equals("baremetal2")) {
- VolumeVO volume = dbf.findByUuid(vol.getUuid(), VolumeVO.class);
- clientIqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid());
+ VolumeVO volume = dbf.findByUuid(vol.getUuid(), VolumeVO.class);
+ if (volume == null || StringUtils.isEmpty(volume.getVmInstanceUuid())) {
+ throw new OperationFailureException(operr("cannot resolve vm uuid from volume[%s] for host[%s]", vol.getUuid(), h.getUuid()));
+ }
+ clientIqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid());
} else {Based on learnings
Resolves: ZSTAC-1111 Change-Id: I7763786c70636b6d636f6b776e73787a67687565
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.*
⚙️ CodeRabbit configuration file
**/*.*: - 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写
Files:
header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.java
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 1. API 设计要求
- API 命名：
- API 名称必须唯一，不能重复。
- API 消息类需要继承
APIMessage；其返回类必须继承APIReply或APIEvent，并在注释中用@RestResponse进行标注。- API 消息上必须添加注解
@RestRequest，并满足如下规范：
path:
- 针对资源使用复数形式。
- 当 path 中引用消息类变量时，使用
{variableName}格式。- HTTP 方法对应：
- 查询操作 →
HttpMethod.GET- 更新操作 →
HttpMethod.PUT- 创建操作 →
HttpMethod.POST- 删除操作 →
HttpMethod.DELETE- API 类需要实现
__example__方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。
2. 命名与格式规范
类名：
- 使用 UpperCamelCase 风格。
- 特殊情况：
- VO/AO/EO 类型类除外。
- 抽象类采用
Abstract或Base前缀/后缀。- 异常类应以
Exception结尾。- 测试类需要以
Test或Case结尾。方法名、参数名、成员变量和局部变量：
- 使用 lowerCamelCase 风格。
常量命名：
- 全部大写，使用下划线分隔单词。
- 要求表达清楚，避免使用含糊或不准确的名称。
包名：
- 统一使用小写，使用点分隔符，每个部分应是一个具有自然语义的英文单词（参考 Spring 框架的结构）。
命名细节：
- 避免在父子类或同一代码块中出现相同名字的成员或局部变量，防止混淆。
- 命名缩写：
- 不允许使用不必要的缩写，如：
AbsSchedulerJob、condi、Fu等。应使用完整单词提升可读性。
3. 编写自解释代码
意图表达：
- 避免使用布尔型参数造成含义不明确。例如：
- 对于
stopAgent(boolean ignoreError)，建议拆分为不同函数（如stopAgentIgnoreError()），或使用枚举表达操作类型。- 命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途（例如在常量与变量名称中，将类型词放在末尾）。
- 避免使用魔法值（Magic Value）：
直接使用未经定义的数值或字符串（如 if (status == 5)）应替换为枚举或常量。
示例：
// 错误示例：魔法值
if (user.getStatus() == 5) { ... }
// 正确示例：常量或枚举
public static final int STATUS_ACTIVE = 5;
if (user.getStatus() == STATUS_ACTIVE) { ... }
// 或使用枚举
enum UserStatus { ACTIVE, INACTIVE }
注释：
- 代码应尽量做到自解释，对少于两行的说明可以直接写在代码中。
- 对于较长的注释，需要仔细校对并随代码更新，确保内容正确。
- 接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。
4. 流程控制和结构优化
if...else 的使用：
- 应尽量减少 if...else 结构的使用，建议：
- 限制嵌套层级最多为两层，且内层不应再出现
else分支。- 尽早返回（Early Return），将条件判断中的处理逻辑提前结束或抽成独立方法。
- 使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断。
条件判断：
- if 条件表达不宜过长或过于复杂，必要时可以将条件抽成 boolean 变量描述。
代码块长度：
...
Files:
header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.java
| String[] serverHostNames = uri.getAuthority().split(":")[0].split(","); | ||
| target.setIp(serverHostNames[0]); | ||
| target.setPort(Integer.parseInt(uri.getAuthority().split(":")[1])); | ||
|
|
||
| // parse: /{iqn}/{diskIdType}_{diskId} | ||
| String path = uri.getPath(); | ||
| if (path != null && path.startsWith("/")) { | ||
| String[] pathParts = path.substring(1).split("/"); | ||
| if (pathParts.length >= 2) { | ||
| target.setIqn(pathParts[0]); | ||
| String[] diskParts = pathParts[1].split("_", 2); | ||
| if (diskParts.length == 2) { | ||
| target.setDiskIdType(diskParts[0]); | ||
| target.setDiskId(diskParts[1]); | ||
| } else { |
There was a problem hiding this comment.
修正 IPv6 与缺失端口的 URI 解析漏洞
当前依赖 uri.getAuthority().split(":") 同时截取主机与端口。在合法的 IPv6 形式（例：iscsi://[2001:db8::1]:3260/...）下该表达式会把地址切成 "[2001", "db8::1]", "3260"，导致写入的 IP 变成 "[2001"，端口解析也会抛 NumberFormatException。遇到没有显式端口的 URI 时同样会触发 ArrayIndexOutOfBoundsException，使得原本可接受的输入直接判为失败。建议改用 URI#getHost() 与 URI#getPort() 提供的结构化解析，并在缺失信息时给出清晰的校验逻辑。如下修订可避免该问题：
- String[] serverHostNames = uri.getAuthority().split(":")[0].split(",");
- target.setIp(serverHostNames[0]);
- target.setPort(Integer.parseInt(uri.getAuthority().split(":")[1]));
+ String authority = uri.getAuthority();
+ if (authority == null) {
+ logger.info("Missing authority in URI: " + uriString);
+ return null;
+ }
+
+ String hostSegment = uri.getHost();
+ if (hostSegment == null) {
+ hostSegment = authority;
+ if (authority.contains("@")) {
+ hostSegment = authority.substring(authority.indexOf('@') + 1);
+ }
+ int lastColon = hostSegment.lastIndexOf(':');
+ if (lastColon >= 0) {
+ hostSegment = hostSegment.substring(0, lastColon);
+ }
+ }
+
+ if (hostSegment == null || hostSegment.isEmpty()) {
+ logger.info("Missing host in URI authority: " + authority);
+ return null;
+ }
+
+ String[] serverHostNames = hostSegment.split(",");
+ target.setIp(serverHostNames[0]);
+
+ int parsedPort = uri.getPort();
+ if (parsedPort < 0) {
+ logger.info("Missing port in URI authority: " + authority);
+ return null;
+ }
+ target.setPort(parsedPort);📝 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.
| String[] serverHostNames = uri.getAuthority().split(":")[0].split(","); | |
| target.setIp(serverHostNames[0]); | |
| target.setPort(Integer.parseInt(uri.getAuthority().split(":")[1])); | |
| // parse: /{iqn}/{diskIdType}_{diskId} | |
| String path = uri.getPath(); | |
| if (path != null && path.startsWith("/")) { | |
| String[] pathParts = path.substring(1).split("/"); | |
| if (pathParts.length >= 2) { | |
| target.setIqn(pathParts[0]); | |
| String[] diskParts = pathParts[1].split("_", 2); | |
| if (diskParts.length == 2) { | |
| target.setDiskIdType(diskParts[0]); | |
| target.setDiskId(diskParts[1]); | |
| } else { | |
| String authority = uri.getAuthority(); | |
| if (authority == null) { | |
| logger.info("Missing authority in URI: " + uriString); | |
| return null; | |
| } | |
| String hostSegment = uri.getHost(); | |
| if (hostSegment == null) { | |
| // fallback to manual parsing of authority (e.g. userinfo@host:port) | |
| hostSegment = authority; | |
| if (authority.contains("@")) { | |
| hostSegment = authority.substring(authority.indexOf('@') + 1); | |
| } | |
| int lastColon = hostSegment.lastIndexOf(':'); | |
| if (lastColon >= 0) { | |
| hostSegment = hostSegment.substring(0, lastColon); | |
| } | |
| } | |
| if (hostSegment.isEmpty()) { | |
| logger.info("Missing host in URI authority: " + authority); | |
| return null; | |
| } | |
| String[] serverHostNames = hostSegment.split(","); | |
| target.setIp(serverHostNames[0]); | |
| int parsedPort = uri.getPort(); | |
| if (parsedPort < 0) { | |
| logger.info("Missing port in URI authority: " + authority); | |
| return null; | |
| } | |
| target.setPort(parsedPort); | |
| // parse: /{iqn}/{diskIdType}_{diskId} | |
| String path = uri.getPath(); | |
| if (path != null && path.startsWith("/")) { | |
| String[] pathParts = path.substring(1).split("/"); | |
| if (pathParts.length >= 2) { | |
| target.setIqn(pathParts[0]); | |
| String[] diskParts = pathParts[1].split("_", 2); | |
| if (diskParts.length == 2) { | |
| target.setDiskIdType(diskParts[0]); | |
| target.setDiskId(diskParts[1]); | |
| } else { | |
| // … | |
| } | |
| } | |
| } |
1 participant
Resolves: ZSTAC-77315
Change-Id: I75656f6178726675636f6d63656b666873786d62
sync from gitlab !8384