<fix>[storage]: refactor expon bm2 #2685
Open
Conversation
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
Walkthrough在 IscsiRemoteTarget 中新增静态工厂方法 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as 调用方
participant T as IscsiRemoteTarget.fromUri
participant L as Logger
C->>T: fromUri(uriString)
T->>T: 构建 URI 并检查 scheme
alt scheme == "iscsi"
T->>T: 解析 host/port 与 path
alt path matches /{iqn}/{diskIdType}_{diskId}
T-->>C: 返回 IscsiRemoteTarget
else 格式不合
T->>L: log.warn(...)
T-->>C: 返回 null
end
else scheme 不匹配
T->>L: log.warn(...)
T-->>C: 返回 null
end
opt 异常
T->>L: log.error(...)
T-->>C: 返回 null
end
sequenceDiagram
autonumber
participant S as ExponStorageController
participant H as Host
participant V as VolumeVO DAO
participant U as IscsiUtils
participant L as Logger
S->>H: 读取 hypervisorType
alt hypervisorType == baremetal2
S->>V: 通过 volumeUuid 加载 VolumeVO
S->>S: 构造 IQN = "iqn.2015-01.io.zstack:initiator.instance.<vmInstanceUuid>"
else 其他类型
S->>U: getHostInitiatorName(h.uuid)
U-->>S: initiatorName (或 null)
end
alt 有可用 IQN
S->>S: 继续激活/撤销 iSCSI 流程
else 无 IQN
S->>L: log.warn(...) / 处理空值
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration 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)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 3
🧹 Nitpick comments (1)
plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java (1)
40-40: 避免使用通配符导入为保持可读性与减少潜在命名冲突，建议改回显式导入所需的类。
📜 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
🔇 Additional comments (1)
header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.java (1)
3-7: 引入日志器与 URI 解析依赖 — LGTM新增 CLogger 与 URI 解析导入合理，便于 fromUri 的实现与故障定位。
Also applies to: 9-9
Comment on lines
+80
to
+118
| public static IscsiRemoteTarget fromUri(String uriString) { | ||
| try { | ||
| URI uri = URI.create(uriString); | ||
|
|
||
| if (!"iscsi".equals(uri.getScheme())) { | ||
| logger.info("Invalid URI scheme. Expected 'iscsi', got: " + uri.getScheme()); | ||
| return null; | ||
| } | ||
|
|
||
| IscsiRemoteTarget target = new IscsiRemoteTarget(); | ||
| target.setIp(uri.getHost()); | ||
| target.setPort(uri.getPort()); | ||
|
|
||
| // 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 { | ||
| logger.info("Invalid diskId format in URI path: " + pathParts[1]); | ||
| return null; | ||
| } | ||
| } else { | ||
| logger.info("Invalid URI path format: " + path); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| return target; | ||
| } catch (Exception e) { | ||
| logger.error("Failed to parse URI: " + uriString, e); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
fromUri 存在默认端口与路径校验缺失，建议增强健壮性
问题与建议：
- 未提供默认端口：
uri.getPort()为 -1 时应回退到 3260，否则getResourceURI()会生成非法端口。 - 路径缺失时未返回失败：若
path为 null 或不以/开头，当前实现直接返回半成品对象，可能导致后续使用出错。 diskIdType未校验：建议校验是否属于支持的枚举值。- 方案名大小写：建议对 scheme 使用不区分大小写匹配。
可按以下补丁修复：
- if (!"iscsi".equals(uri.getScheme())) {
+ if (!"iscsi".equalsIgnoreCase(uri.getScheme())) {
logger.info("Invalid URI scheme. Expected 'iscsi', got: " + uri.getScheme());
return null;
}
IscsiRemoteTarget target = new IscsiRemoteTarget();
target.setIp(uri.getHost());
- target.setPort(uri.getPort());
+ int port = uri.getPort() == -1 ? 3260 : uri.getPort();
+ target.setPort(port);
// parse: /{iqn}/{diskIdType}_{diskId}
String path = uri.getPath();
- if (path != null && path.startsWith("/")) {
+ 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]);
+ // validate diskIdType
+ try {
+ DiskIdType.valueOf(target.getDiskIdType());
+ } catch (IllegalArgumentException e) {
+ logger.info("Unsupported diskIdType in URI path: " + target.getDiskIdType());
+ return null;
+ }
} else {
logger.info("Invalid diskId format in URI path: " + pathParts[1]);
return null;
}
} else {
logger.info("Invalid URI path format: " + path);
return null;
}
- }
+ } else {
+ logger.info("Invalid URI path format: " + path);
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static IscsiRemoteTarget fromUri(String uriString) { | |
| try { | |
| URI uri = URI.create(uriString); | |
| if (!"iscsi".equals(uri.getScheme())) { | |
| logger.info("Invalid URI scheme. Expected 'iscsi', got: " + uri.getScheme()); | |
| return null; | |
| } | |
| IscsiRemoteTarget target = new IscsiRemoteTarget(); | |
| target.setIp(uri.getHost()); | |
| target.setPort(uri.getPort()); | |
| // 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 { | |
| logger.info("Invalid diskId format in URI path: " + pathParts[1]); | |
| return null; | |
| } | |
| } else { | |
| logger.info("Invalid URI path format: " + path); | |
| return null; | |
| } | |
| } | |
| return target; | |
| } catch (Exception e) { | |
| logger.error("Failed to parse URI: " + uriString, e); | |
| return null; | |
| } | |
| } | |
| public static IscsiRemoteTarget fromUri(String uriString) { | |
| try { | |
| URI uri = URI.create(uriString); | |
| if (!"iscsi".equalsIgnoreCase(uri.getScheme())) { | |
| logger.info("Invalid URI scheme. Expected 'iscsi', got: " + uri.getScheme()); | |
| return null; | |
| } | |
| IscsiRemoteTarget target = new IscsiRemoteTarget(); | |
| target.setIp(uri.getHost()); | |
| int port = uri.getPort() == -1 ? 3260 : uri.getPort(); | |
| target.setPort(port); | |
| // 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]); | |
| // validate diskIdType | |
| try { | |
| DiskIdType.valueOf(target.getDiskIdType()); | |
| } catch (IllegalArgumentException e) { | |
| logger.info("Unsupported diskIdType in URI path: " + target.getDiskIdType()); | |
| return null; | |
| } | |
| } else { | |
| logger.info("Invalid diskId format in URI path: " + pathParts[1]); | |
| return null; | |
| } | |
| } else { | |
| logger.info("Invalid URI path format: " + path); | |
| return null; | |
| } | |
| } else { | |
| logger.info("Invalid URI path format: " + path); | |
| return null; | |
| } | |
| return target; | |
| } catch (Exception e) { | |
| logger.error("Failed to parse URI: " + uriString, e); | |
| return null; | |
| } | |
| } |
Comment on lines
239
to
+242
| private synchronized ActiveVolumeTO activeIscsiVolume(HostInventory h, BaseVolumeInfo vol, boolean shareable) { | ||
| String clientIqn = IscsiUtils.getHostInitiatorName(h.getUuid()); | ||
| 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()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
baremetal2 分支可能产生 NPE/无效 IQN，需增加校验与回退
dbf.findByUuid(...) 可能返回 null，volume.getVmInstanceUuid() 也可能为 null（未挂载或数据盘场景）。当前代码直接格式化 IQN 存在 NPE 及生成无效 IQN 的风险。建议当无法获取 vmInstanceUuid 时，回退到主机 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());
- }
+ if ("baremetal2".equals(h.getHypervisorType())) {
+ VolumeVO volume = dbf.findByUuid(vol.getUuid(), VolumeVO.class);
+ if (volume == null || volume.getVmInstanceUuid() == null) {
+ logger.warn(String.format("cannot determine baremetal2 initiator IQN, volume[uuid:%s] vmInstanceUuid is null; fallback to host initiator", vol.getUuid()));
+ } else {
+ clientIqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid());
+ }
+ }此外，建议将 IQN 模板抽成常量以避免重复拼接（可放在类顶部）：
private static final String BM2_INITIATOR_IQN_TEMPLATE = "iqn.2015-01.io.zstack:initiator.instance.%s";
并用该常量替换两处硬编码。
🏁 Script executed:
#!/bin/bash
# 查看 ExponStorageController 类中是否定义了 logger 和方法上下文
rg -n "class ExponStorageController" -n plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
rg -n "private synchronized ActiveVolumeTO activeIscsiVolume" -C3 plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
rg -n "logger" -n plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.javaLength of output: 1019
增加 baremetal2 分支 Null 校验与回退
- 对
dbf.findByUuid(...)和volume.getVmInstanceUuid()增加 null 检查，失败时回退至主机 IQN，避免 NPE 或生成无效 IQN - 推荐将
"iqn.2015-01.io.zstack:initiator.instance.%s"抽成常量BM2_INITIATOR_IQN_TEMPLATE
🤖 Prompt for AI Agents
In plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
around lines 236 to 241, add null checks after dbf.findByUuid(...) and before
using volume.getVmInstanceUuid(); if dbf returns null or vmInstanceUuid is
null/empty, fall back to using the original host IQN (clientIqn) to avoid NPE or
generating an invalid IQN, and extract the literal
"iqn.2015-01.io.zstack:initiator.instance.%s" into a named constant
BM2_INITIATOR_IQN_TEMPLATE; ensure the code remains synchronized and preserves
existing behavior when values are present.
Comment on lines
751
to
+760
| 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.
🧩 Analysis chain
错误的 volumeUuid 解析导致查库失败（功能性缺陷）
对 installPath 使用 split("/") 取最后一段当作 volumeUuid 是不正确的。Expon 安装路径尾段为 Expon 的 volId，不是 ZStack 的 volumeUuid。这将导致 dbf.findByUuid 返回 null，进而生成无效 IQN 或 NPE。
建议改为先取 volId，再通过 Expon 查询回卷名，最后从卷名恢复出 ZStack 卷 UUID，再查库获取 vmInstanceUuid：
- 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 ("baremetal2".equals(h.getHypervisorType())) {
+ String volId = getVolIdFromPath(installPath);
+ VolumeModule vol = apiHelper.getVolumeOrElseNull(volId);
+ if (vol == null) {
+ logger.warn(String.format("cannot find Expon volume by installPath[%s], fallback to host initiator", installPath));
+ } else {
+ String volUuid = getVolumeInfo(vol.getName()).getUuid();
+ VolumeVO volume = dbf.findByUuid(volUuid, VolumeVO.class);
+ if (volume == null || volume.getVmInstanceUuid() == null) {
+ logger.warn(String.format("cannot determine baremetal2 initiator IQN, volume[uuid:%s] vmInstanceUuid is null; fallback to host initiator", volUuid));
+ } else {
+ iqn = String.format("iqn.2015-01.io.zstack:initiator.instance.%s", volume.getVmInstanceUuid());
+ }
+ }
+ }同时与激活路径保持一致的防御式编程，避免异常时中断业务。
🏁 Script executed:
#!/bin/bash
grep -R "activateIscsi" -n plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
grep -R "deactivateIscsi" -n plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
grep -R "getVolIdFromPath" -n plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
grep -R "installPath" -n plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java -n -C3Length of output: 9892
🏁 Script executed:
#!/bin/bash
sed -n '230,300p' plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.javaLength of output: 3868
修正 deactivateIscsi 方法中错误的 volumeUuid 提取逻辑（plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java:752-759）
installPath.split("/") 获取的是 Expon volId 而非 ZStack volumeUuid，会导致 dbf.findByUuid 返回 null 或 NPE。建议：
- 用 getVolIdFromPath(installPath) 获取 volId，再通过 apiHelper.getVolumeOrElseNull(volId) 拿到 Expon VolumeModule
- 从 VolumeModule.getName() 或调用 Expon 接口获取 ZStack 卷 UUID，再 dbf.findByUuid(volUuid, VolumeVO.class)
- 对 volume 或 volume.getVmInstanceUuid() 为 null 的情况做降级处理或日志警告，避免抛出异常
🤖 Prompt for AI Agents
plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java around
lines 752-759: installPath.split("/") extracts an Expon volId, not the ZStack
volume UUID, causing dbf.findByUuid to return null/NPE; replace the naive split
logic with getVolIdFromPath(installPath) to obtain the Expon volId, call
apiHelper.getVolumeOrElseNull(volId) to retrieve the Expon VolumeModule, extract
the ZStack volume UUID from VolumeModule.getName() or by invoking the Expon API
that returns the ZStack UUID, then call dbf.findByUuid(volUuid, VolumeVO.class);
add null checks and defensive handling (log warnings and return/skip) if the
VolumeModule or the resolved VolumeVO or its getVmInstanceUuid() is null to
avoid throwing exceptions.
Resolves: ZSTAC-77315 Change-Id: I6670626f746f6e617466737965776f756a766774
MatheMatrix
force-pushed
the
sync/jin.shen/refactor-77315@@2
branch
from
c8cb6da to
826b472
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves: ZSTAC-77315
Change-Id: I6670626f746f6e617466737965776f756a766774
sync from gitlab !8473