Skip to content

<fix>[storage]: refactor expon bm2 #2685

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
zstack-robot-2 wants to merge 1 commit into
base: 5.4.0
Choose a base branch
from
Open

<fix>[storage]: refactor expon bm2 #2685

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
52 changes: 52 additions & 0 deletions header/src/main/java/org/zstack/header/storage/addon/IscsiRemoteTarget.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
package org.zstack.header.storage.addon;

import org.zstack.utils.Utils;
import org.zstack.utils.logging.CLogger;

import java.net.URI;

public class IscsiRemoteTarget extends BlockRemoteTarget {
private final static CLogger logger = Utils.getLogger(IscsiRemoteTarget.class);
private String transport = "tcp";

private String iqn;
Expand Down Expand Up @@ -70,4 +76,50 @@ public enum DiskIdType {
wwn,
serial
}

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();
String authority = uri.getAuthority();
if (authority == null || authority.isEmpty()) {
logger.info("Invalid URI authority: " + uri.getAuthority());
return null;
}
String[] serverHostNames = authority.split(":")[0].split(",");
target.setIp(serverHostNames[0]);
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;
}
}
}
3 changes: 3 additions & 0 deletions ...er/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,7 @@ public interface PrimaryStorageControllerSvc {
void onFirstAdditionConfigure(Completion completion);

long alignSize(long size);

String getVolumeLunId(String volInstallPath);

}
27 changes: 19 additions & 8 deletions plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,7 @@
import org.zstack.header.storage.primary.ImageCacheInventory;
import org.zstack.header.storage.primary.VolumeSnapshotCapability;
import org.zstack.header.storage.snapshot.VolumeSnapshotStats;
import org.zstack.header.volume.VolumeConstant;
import org.zstack.header.volume.VolumeInventory;
import org.zstack.header.volume.VolumeProtocol;
import org.zstack.header.volume.VolumeStats;
import org.zstack.header.volume.*;
import org.zstack.iscsi.IscsiUtils;
import org.zstack.iscsi.kvm.IscsiHeartbeatVolumeTO;
import org.zstack.iscsi.kvm.IscsiVolumeTO;
Expand All @@ -63,6 +60,7 @@
import static org.zstack.core.Platform.operr;
import static org.zstack.expon.ExponIscsiHelper.*;
import static org.zstack.expon.ExponNameHelper.*;
import static org.zstack.iscsi.IscsiUtils.getGatewayMnIpFromInitiatorName;
import static org.zstack.iscsi.IscsiUtils.getHostMnIpFromInitiatorName;
import static org.zstack.storage.addon.primary.ExternalPrimaryStorageNameHelper.*;

Expand Down Expand Up @@ -239,6 +237,10 @@ public List<IscsiSeverNode> getIscsiServers(String tianshuId) {

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());
}
if (clientIqn == null) {
throw new RuntimeException(String.format("cannot get host[uuid:%s] initiator name", h.getUuid()));
}
Expand Down Expand Up @@ -529,7 +531,7 @@ public List<ActiveVolumeClient> getActiveClients(String installPath, String prot
ActiveVolumeClient c = new ActiveVolumeClient();
if (it.contains("iqn")) {
c.setQualifiedName(it);
c.setManagerIp(getHostMnIpFromInitiatorName(it));
c.setManagerIp(getHostMnIpFromInitiatorName(it) != null ? getHostMnIpFromInitiatorName(it) : getGatewayMnIpFromInitiatorName(it));
} else {
c.setManagerIp(it);
}
Expand Down Expand Up @@ -579,12 +581,16 @@ public void deactivate(String installPath, String protocol, HostInventory h, Com
@Override
public void deactivate(String installPath, String protocol, ActiveVolumeClient client, Completion comp) {
HostVO host = Q.New(HostVO.class).eq(HostVO_.managementIp, client.getManagerIp()).find();
if (host != null) {
deactivate(installPath, protocol, HostInventory.valueOf(host), comp);
} else {
if (host == null) {
comp.fail(operr("deactivate fail, cannot find host[ip:%s]", client.getManagerIp()));
return;
}
if (host.getHypervisorType().equals("baremetal2")) {
// bm instance InitiatorName
deactivateIscsi(installPath, client.getQualifiedName());
comp.success();
} else {
deactivate(installPath, protocol, HostInventory.valueOf(host), comp);
}
Comment on lines 583 to 594
Copy link

@coderabbitai coderabbitai bot Oct 11, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

建议提取魔法值为常量

代码逻辑正确，但仍在多处使用魔法字符串 "baremetal2"。根据编码规范，应避免使用魔法值。

建议在类顶部定义常量：

private static final String HYPERVISOR_TYPE_BAREMETAL2 = "baremetal2";

然后在使用处替换：

-        if (host.getHypervisorType().equals("baremetal2")) {
+        if (HYPERVISOR_TYPE_BAREMETAL2.equals(host.getHypervisorType())) {

这样可以提高代码的可维护性，并在多处使用时保持一致性。As per coding guidelines.

🤖 Prompt for AI Agents 
In plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
around lines 582 to 593, replace the hard-coded magic string "baremetal2" with a
class-level constant; add a private static final String
HYPERVISOR_TYPE_BAREMETAL2 = "baremetal2" near the top of the class and then
change host.getHypervisorType().equals("baremetal2") to compare against that
constant (prefer constant.equals(host.getHypervisorType()) or Objects.equals for
null-safety).

}

Expand Down Expand Up @@ -1327,6 +1333,11 @@ public long alignSize(long size) {
return size;
}

@Override
public String getVolumeLunId(String volInstallPath) {
return apiHelper.getVolumeLunDetail(getVolIdFromPath(volInstallPath)).getLunId();
}
Comment on lines +1336 to +1339
Copy link

@coderabbitai coderabbitai bot Oct 20, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

缺少空值检查可能导致 NPE

该方法调用链 apiHelper.getVolumeLunDetail(...).getLunId() 可能在中间步骤返回 null，导致空指针异常。建议添加防御性检查。Based on learnings.

     @Override
     public String getVolumeLunId(String volInstallPath) {
-        return apiHelper.getVolumeLunDetail(getVolIdFromPath(volInstallPath)).getLunId();
+        String volId = getVolIdFromPath(volInstallPath);
+        if (volId == null) {
+            return null;
+        }
+        VolumeDetail detail = apiHelper.getVolumeLunDetail(volId);
+        return detail != null ? detail.getLunId() : null;
     }
🤖 Prompt for AI Agents 
In plugin/expon/src/main/java/org/zstack/expon/ExponStorageController.java
around lines 1336 to 1339, the method directly calls
apiHelper.getVolumeLunDetail(getVolIdFromPath(volInstallPath)).getLunId() which
can produce a NullPointerException if getVolIdFromPath(...) returns null or
apiHelper.getVolumeLunDetail(...) returns null or a detail with null lunId; add
defensive null checks: obtain volId first and return null or throw a clear
exception if it’s missing, fetch the lunDetail and verify it’s non-null before
accessing getLunId(), and handle a null lunId case (return null or appropriate
empty/optional value) so the method never dereferences a null reference.


private void retry(Runnable r) {
retry(r, 3);
}
Expand Down
23 changes: 23 additions & 0 deletions plugin/iscsi/src/main/java/org/zstack/iscsi/IscsiUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
import org.zstack.header.storage.backup.*;
import org.zstack.header.tag.SystemTagVO;
import org.zstack.header.tag.SystemTagVO_;
import org.zstack.header.vm.VmInstanceVO;
import org.zstack.header.vm.VmInstanceVO_;
import org.zstack.storage.backup.BackupStorageSystemTags;

import java.util.Collections;
Expand Down Expand Up @@ -38,6 +40,27 @@ public static String getHostMnIpFromInitiatorName(String initiatorName) {
return null;
}

public static String getGatewayMnIpFromInitiatorName(String initiatorName) {
String requiredPrefix = "iqn.2015-01.io.zstack:initiator.instance.";

if (initiatorName == null || initiatorName.isEmpty()) {
return null;
}

if (!initiatorName.startsWith(requiredPrefix)) {
return null;
}

String uuid = initiatorName.substring(requiredPrefix.length());
String hostUuid = Q.New(VmInstanceVO.class).eq(VmInstanceVO_.uuid, uuid)
.select(VmInstanceVO_.hostUuid)
.findValue();
return Q.New(HostVO.class).eq(HostVO_.uuid, hostUuid)
.eq(HostVO_.hypervisorType, "baremetal2")
.select(HostVO_.managementIp)
.findValue();
}
Comment on lines +43 to +62
Copy link

@coderabbitai coderabbitai bot Oct 20, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

存在空指针风险和魔法值使用问题

该方法存在以下问题：

  1. 缺少 null 检查：Line 55-57 的 findValue() 可能返回 null，直接传递给 Line 58 的 eq(HostVO_.uuid, hostUuid) 可能导致问题。Based on learnings.
  2. 魔法值：Line 44 的 IQN 前缀字符串和 Line 59 的 "baremetal2" 应提取为常量。As per coding guidelines.

建议应用以下修复：

+    private static final String BM2_INITIATOR_IQN_PREFIX = "iqn.2015-01.io.zstack:initiator.instance.";
+    private static final String HYPERVISOR_TYPE_BAREMETAL2 = "baremetal2";
+
     public static String getGatewayMnIpFromInitiatorName(String initiatorName) {
-        String requiredPrefix = "iqn.2015-01.io.zstack:initiator.instance.";
-
         if (initiatorName == null || initiatorName.isEmpty()) {
             return null;
         }
 
-        if (!initiatorName.startsWith(requiredPrefix)) {
+        if (!initiatorName.startsWith(BM2_INITIATOR_IQN_PREFIX)) {
             return null;
         }
 
-        String uuid = initiatorName.substring(requiredPrefix.length());
+        String uuid = initiatorName.substring(BM2_INITIATOR_IQN_PREFIX.length());
         String hostUuid = Q.New(VmInstanceVO.class).eq(VmInstanceVO_.uuid, uuid)
                 .select(VmInstanceVO_.hostUuid)
                 .findValue();
+        if (hostUuid == null) {
+            return null;
+        }
         return Q.New(HostVO.class).eq(HostVO_.uuid, hostUuid)
-                .eq(HostVO_.hypervisorType, "baremetal2")
+                .eq(HostVO_.hypervisorType, HYPERVISOR_TYPE_BAREMETAL2)
                 .select(HostVO_.managementIp)
                 .findValue();
     }
🤖 Prompt for AI Agents 
In plugin/iscsi/src/main/java/org/zstack/iscsi/IscsiUtils.java around lines 43
to 62, the method can NPE when the VmInstance lookup returns null and also uses
magic strings for the IQN prefix and hypervisor type; add private static final
constants for the IQN prefix and the baremetal hypervisor value, replace the
literals with those constants, and after computing hostUuid from the VM query
check for null/empty and return null (or handle appropriately) before passing it
into the Host query to avoid the NPE.


private static String getBsMnIp(String bsUuid) {
CloudBus bus = Platform.getComponentLoader().getComponent(CloudBus.class);
GetBackupStorageManagerHostnameMsg msg = new GetBackupStorageManagerHostnameMsg();
Expand Down
5 changes: 5 additions & 0 deletions plugin/xinfini/src/main/java/org/zstack/xinfini/XInfiniStorageController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,6 +1030,11 @@ public long alignSize(long size) {
return SizeUnit.MEGABYTE.toByte(convertBytesToMegaBytes(size));
}

@Override
public String getVolumeLunId(String volInstallPath) {
return null;
}

public void cleanActiveRecord(VolumeInventory vol) {

}
Expand Down
5 changes: 5 additions & 0 deletions plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,6 +1024,11 @@ public long alignSize(long size) {
return convertSizeToByte(alignSizeTo(size, unit), unit);
}

@Override
public String getVolumeLunId(String volInstallPath) {
return null;
}

public void doDeleteVolume(String installPath, Boolean force, Completion comp) {
DeleteVolumeCmd cmd = new DeleteVolumeCmd();
cmd.setPath(installPath);
Expand Down
4 changes: 4 additions & 0 deletions storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,10 @@ private void activeVolumeIfNeed(VmInstanceInventory vm, VolumeInventory volume,
}

HostInventory host = HostInventory.valueOf(dbf.findByUuid(vm.getHostUuid(), HostVO.class));
if (host.getHypervisorType().equals("baremetal2")) {
completion.success();
return;
}
svc.activate(BaseVolumeInfo.valueOf(volume), host, volume.isShareable(), new ReturnValueCompletion<ActiveVolumeTO>(completion) {
@Override
public void success(ActiveVolumeTO returnValue) {
Expand Down