Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions conf/springConfigXml/VolumeManager.xml
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
<bean id="VolumeSnapshotEncryptionHelper" class="org.zstack.storage.encrypt.VolumeSnapshotEncryptionHelper"/>

<bean id="ZbsVolumeEncryptionMaterialFactory" class="org.zstack.storage.encrypt.ZbsVolumeEncryptionMaterialFactory"/>
<bean id="ZbsVolumeEncryptionConverter" class="org.zstack.storage.encrypt.ZbsVolumeEncryptionConverter"/>
<bean id="ZbsVolumeEncryptionKvmCaller" class="org.zstack.storage.encrypt.ZbsVolumeEncryptionKvmCaller"/>
<bean id="ZbsVolumeEncryptionCleanup" class="org.zstack.storage.encrypt.ZbsVolumeEncryptionCleanup"/>
<bean id="ZbsEncryptedEmptyVolumeCreator" class="org.zstack.storage.encrypt.ZbsEncryptedEmptyVolumeCreator"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ public class CreateVolumeSpec {
private boolean encrypted;
private String hostUuid;
private String encryptedDek;
private String encryptionKeyResourceType;
private String encryptionKeyResourceUuid;

private String allocatedUrl;
private boolean dryRun;
Expand Down Expand Up @@ -85,4 +87,20 @@ public String getEncryptedDek() {
public void setEncryptedDek(String encryptedDek) {
this.encryptedDek = encryptedDek;
}

public String getEncryptionKeyResourceType() {
return encryptionKeyResourceType;
}

public void setEncryptionKeyResourceType(String encryptionKeyResourceType) {
this.encryptionKeyResourceType = encryptionKeyResourceType;
}

public String getEncryptionKeyResourceUuid() {
return encryptionKeyResourceUuid;
}

public void setEncryptionKeyResourceUuid(String encryptionKeyResourceUuid) {
this.encryptionKeyResourceUuid = encryptionKeyResourceUuid;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import org.zstack.header.storage.addon.*;
import org.zstack.header.storage.primary.EncryptVolumeBitsOnPrimaryStorageMsg;
import org.zstack.header.storage.primary.EncryptVolumeBitsOnPrimaryStorageReply;
import org.zstack.header.storage.primary.ConvertVolumeEncryptionOnPrimaryStorageMsg;
import org.zstack.header.storage.primary.ConvertVolumeEncryptionOnPrimaryStorageReply;
import org.zstack.header.storage.snapshot.VolumeSnapshotStats;
import org.zstack.header.volume.VolumeInventory;
import org.zstack.header.volume.VolumeProtocol;
Expand Down Expand Up @@ -34,6 +36,8 @@ public interface PrimaryStorageControllerSvc {
void cloneVolume(String srcInstallPath, CreateVolumeSpec dst, ReturnValueCompletion<VolumeStats>comp);
void copyVolume(String srcInstallPath, CreateVolumeSpec dst, ReturnValueCompletion<VolumeStats>comp);
void encryptVolumeBits(EncryptVolumeBitsOnPrimaryStorageMsg msg, ReturnValueCompletion<EncryptVolumeBitsOnPrimaryStorageReply> comp);
void convertVolumeEncryption(ConvertVolumeEncryptionOnPrimaryStorageMsg msg,
ReturnValueCompletion<ConvertVolumeEncryptionOnPrimaryStorageReply> completion);
void flattenVolume(String installPath, ReturnValueCompletion<VolumeStats>comp);

// support uri or path
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,12 @@ public interface ZbsVolumeEncryptionBackend {

void checkNoSnapshots(String installPath, Completion completion);

void validateConversionPaths(String sourceInstallPath, String targetInstallPath);

void createConversionTarget(String targetInstallPath, long virtualSize, boolean targetEncrypted,
ReturnValueCompletion<String> completion);

void deleteConversionTarget(String targetInstallPath, Completion completion);

void stats(String installPath, ReturnValueCompletion<VolumeStats> completion);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import org.zstack.header.core.ReturnValueCompletion;
import org.zstack.header.storage.primary.EncryptVolumeBitsOnPrimaryStorageMsg;
import org.zstack.header.storage.primary.EncryptVolumeBitsOnPrimaryStorageReply;
import org.zstack.header.storage.primary.ConvertVolumeEncryptionOnPrimaryStorageMsg;
import org.zstack.header.storage.primary.ConvertVolumeEncryptionOnPrimaryStorageReply;
import org.zstack.header.storage.snapshot.VolumeSnapshotInventory;
import org.zstack.header.volume.VolumeInventory;
import org.zstack.header.volume.VolumeStats;
Expand All @@ -23,6 +25,9 @@ void copyEncryptedVolumeFromSnapshot(ZbsVolumeEncryptionBackend backend, String
void encryptVolumeBits(ZbsVolumeEncryptionBackend backend, EncryptVolumeBitsOnPrimaryStorageMsg msg,
ReturnValueCompletion<EncryptVolumeBitsOnPrimaryStorageReply> completion);

void convertVolumeEncryption(ZbsVolumeEncryptionBackend backend, ConvertVolumeEncryptionOnPrimaryStorageMsg msg,
ReturnValueCompletion<ConvertVolumeEncryptionOnPrimaryStorageReply> completion);

void resizeEncryptedVolume(String primaryStorageUuid, VolumeInventory volume, long size,
ReturnValueCompletion<VolumeStats> completion);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
import org.zstack.header.storage.addon.primary.*;
import org.zstack.header.storage.primary.EncryptVolumeBitsOnPrimaryStorageMsg;
import org.zstack.header.storage.primary.EncryptVolumeBitsOnPrimaryStorageReply;
import org.zstack.header.storage.primary.ConvertVolumeEncryptionOnPrimaryStorageMsg;
import org.zstack.header.storage.primary.ConvertVolumeEncryptionOnPrimaryStorageReply;
import org.zstack.header.storage.primary.ImageCacheInventory;
import org.zstack.header.storage.primary.VolumeSnapshotCapability;
import org.zstack.header.storage.snapshot.VolumeSnapshotStats;
Expand Down Expand Up @@ -1102,6 +1104,13 @@ public void encryptVolumeBits(EncryptVolumeBitsOnPrimaryStorageMsg msg, ReturnVa
comp.fail(new ErrorCode(SysErrors.UNIMPLEMENTED_OPERATION_ERROR.toString(), "unimplemented operation", details));
}

@Override
public void convertVolumeEncryption(ConvertVolumeEncryptionOnPrimaryStorageMsg msg,
ReturnValueCompletion<ConvertVolumeEncryptionOnPrimaryStorageReply> completion) {
String details = String.format("primary storage controller[%s] does not support volume encryption conversion", getIdentity());
completion.fail(new ErrorCode(SysErrors.UNIMPLEMENTED_OPERATION_ERROR.toString(), "unimplemented operation", details));
}

@Override
public void flattenVolume(String installPath, ReturnValueCompletion<VolumeStats> comp) {
// TODO flatten snapshot
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -789,6 +789,16 @@ public void encryptVolumeBits(EncryptVolumeBitsOnPrimaryStorageMsg msg, ReturnVa
}
}

@Override
public void convertVolumeEncryption(ConvertVolumeEncryptionOnPrimaryStorageMsg msg,
ReturnValueCompletion<ConvertVolumeEncryptionOnPrimaryStorageReply> completion) {
try {
getVolumeEncryptionExtension().convertVolumeEncryption(encryptionBackend(), msg, completion);
} catch (OperationFailureException e) {
completion.fail(e.getErrorCode());
}
}

@Override
public void flattenVolume(String installPath, ReturnValueCompletion<VolumeStats> comp) {
FlattenVolumeCmd cmd = new FlattenVolumeCmd();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import org.zstack.header.storage.addon.primary.ZbsVolumeEncryptionBackend;
import org.zstack.header.volume.VolumeConstant;
import org.zstack.header.volume.VolumeStats;
import org.zstack.utils.Utils;
import org.zstack.utils.logging.CLogger;

import java.util.UUID;

Expand All @@ -22,6 +24,7 @@
import static org.zstack.storage.zbs.ZbsHelper.getSizeUnit;

class ZbsVolumeEncryptionBackendImpl implements ZbsVolumeEncryptionBackend {
private static final CLogger logger = Utils.getLogger(ZbsVolumeEncryptionBackendImpl.class);
private static final String QUERY_SNAPSHOT_PATH = "/zbs/primarystorage/snapshot/query";
private static final long LUKS_PAYLOAD_OFFSET = 8L * 1024 * 1024;

Expand Down Expand Up @@ -178,6 +181,97 @@ public void fail(ErrorCode errorCode) {
});
}

@Override
public void validateConversionPaths(String sourceInstallPath, String targetInstallPath) {
String version = addonInfo.getClusterInfo() == null ? null : addonInfo.getClusterInfo().getVersion();
if (StringUtils.isBlank(version) || !ZbsConstants.MEGABYTE_UNIT.equals(getSizeUnit(version))) {
throw new OperationFailureException(operr(
"ZBS volume encryption conversion requires ZBS version[%s] or later, but current version is[%s]",
ZbsConstants.MEGABYTE_SUPPORTED_VERSION, version));
}

CbdPath source = parseConversionCbdPath(sourceInstallPath);
CbdPath target = parseConversionCbdPath(targetInstallPath);
if (!source.physicalPool.equals(target.physicalPool) || !source.logicalPool.equals(target.logicalPool)) {
throw new OperationFailureException(operr(
"ZBS volume encryption conversion requires source[%s] and target[%s] in the same physical and logical pool",
sourceInstallPath, targetInstallPath));
}
if (source.volume.equals(target.volume)) {
throw new OperationFailureException(operr(
"ZBS volume encryption conversion requires different source and target volumes, but both are[%s]",
sourceInstallPath));
}
}

@Override
public void createConversionTarget(String targetInstallPath, long virtualSize, boolean targetEncrypted,
ReturnValueCompletion<String> completion) {
CbdPath target;
try {
target = parseConversionCbdPath(targetInstallPath);
} catch (OperationFailureException e) {
completion.fail(e.getErrorCode());
return;
}

ZbsStorageController.CreateVolumeCmd cmd = new ZbsStorageController.CreateVolumeCmd();
cmd.setLogicalPool(target.logicalPool);
cmd.setVolume(target.volume);
cmd.setUnit(getSizeUnit(addonInfo.getClusterInfo().getVersion()));
long allocatedSize = targetEncrypted ? luksBackingSize(virtualSize) : virtualSize;
cmd.setSize(alignSizeTo(allocatedSize, cmd.getUnit()));
cmd.setSkipIfExisting(false);

controller.httpCall(ZbsStorageController.CREATE_VOLUME_PATH, cmd, ZbsStorageController.CreateVolumeRsp.class,
new ReturnValueCompletion<ZbsStorageController.CreateVolumeRsp>(completion) {
@Override
public void success(ZbsStorageController.CreateVolumeRsp returnValue) {
String createdInstallPath = returnValue.getInstallPath();
if (targetInstallPath.equals(createdInstallPath)) {
completion.success(createdInstallPath);
return;
}

ErrorCode error = operr(
"ZBS volume encryption conversion target[%s] was created at unexpected path[%s]",
targetInstallPath, createdInstallPath);
String cleanupInstallPath = targetInstallPath;
try {
deleteConversionTarget(cleanupInstallPath, new Completion(completion) {
@Override
public void success() {
completion.fail(error);
}

@Override
public void fail(ErrorCode cleanupError) {
logger.warn(String.format(
"failed to cleanup unexpected ZBS conversion target[installPath:%s] after create path mismatch: %s",
cleanupInstallPath, cleanupError));
completion.fail(error);
}
});
} catch (Exception e) {
logger.warn(String.format(
"failed to cleanup unexpected ZBS conversion target[installPath:%s] after create path mismatch: %s",
cleanupInstallPath, e.getMessage()));
completion.fail(error);
}
}

@Override
public void fail(ErrorCode errorCode) {
completion.fail(errorCode);
}
});
}
Comment on lines +207 to +268

@coderabbitai coderabbitai Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

路径不一致时清理了错误的路径,导致泄漏卷无法被回收。

当创建目标卷成功但返回的 createdInstallPath 与请求的 targetInstallPath 不一致时,cleanupInstallPath 被赋值为 targetInstallPath(该路径实际从未被创建),而真正被 ZBS 创建出来的卷位于 createdInstallPath。对 targetInstallPath 执行删除无法清理真正泄漏的卷,会在 ZBS 侧留下一个无主卷;且由于创建时 skipIfExisting=false,后续对同一转换目标的重试还会因路径已存在而持续失败。

🐛 建议修复:清理实际创建出来的路径
                         ErrorCode error = operr(
                                 "ZBS volume encryption conversion target[%s] was created at unexpected path[%s]",
                                 targetInstallPath, createdInstallPath);
-                        String cleanupInstallPath = targetInstallPath;
+                        String cleanupInstallPath = createdInstallPath;
                         try {
                             deleteConversionTarget(cleanupInstallPath, new Completion(completion) {
📝 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
@Override
public void createConversionTarget(String targetInstallPath, long virtualSize, boolean targetEncrypted,
ReturnValueCompletion<String> completion) {
CbdPath target;
try {
target = parseConversionCbdPath(targetInstallPath);
} catch (OperationFailureException e) {
completion.fail(e.getErrorCode());
return;
}
ZbsStorageController.CreateVolumeCmd cmd = new ZbsStorageController.CreateVolumeCmd();
cmd.setLogicalPool(target.logicalPool);
cmd.setVolume(target.volume);
cmd.setUnit(getSizeUnit(addonInfo.getClusterInfo().getVersion()));
long allocatedSize = targetEncrypted ? luksBackingSize(virtualSize) : virtualSize;
cmd.setSize(alignSizeTo(allocatedSize, cmd.getUnit()));
cmd.setSkipIfExisting(false);
controller.httpCall(ZbsStorageController.CREATE_VOLUME_PATH, cmd, ZbsStorageController.CreateVolumeRsp.class,
new ReturnValueCompletion<ZbsStorageController.CreateVolumeRsp>(completion) {
@Override
public void success(ZbsStorageController.CreateVolumeRsp returnValue) {
String createdInstallPath = returnValue.getInstallPath();
if (targetInstallPath.equals(createdInstallPath)) {
completion.success(createdInstallPath);
return;
}
ErrorCode error = operr(
"ZBS volume encryption conversion target[%s] was created at unexpected path[%s]",
targetInstallPath, createdInstallPath);
String cleanupInstallPath = targetInstallPath;
try {
deleteConversionTarget(cleanupInstallPath, new Completion(completion) {
@Override
public void success() {
completion.fail(error);
}
@Override
public void fail(ErrorCode cleanupError) {
logger.warn(String.format(
"failed to cleanup unexpected ZBS conversion target[installPath:%s] after create path mismatch: %s",
cleanupInstallPath, cleanupError));
completion.fail(error);
}
});
} catch (Exception e) {
logger.warn(String.format(
"failed to cleanup unexpected ZBS conversion target[installPath:%s] after create path mismatch: %s",
cleanupInstallPath, e.getMessage()));
completion.fail(error);
}
}
@Override
public void fail(ErrorCode errorCode) {
completion.fail(errorCode);
}
});
}
ErrorCode error = operr(
"ZBS volume encryption conversion target[%s] was created at unexpected path[%s]",
targetInstallPath, createdInstallPath);
String cleanupInstallPath = createdInstallPath;
try {
deleteConversionTarget(cleanupInstallPath, new Completion(completion) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsVolumeEncryptionBackendImpl.java`
around lines 200 - 261, Update the path-mismatch cleanup in
createConversionTarget so cleanupInstallPath uses createdInstallPath, the actual
path returned by ZBS, instead of targetInstallPath. Preserve the existing error
reporting and completion behavior while ensuring deleteConversionTarget removes
the created orphan volume.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment from 周众:

ZBS 创建卷的实际标识由请求中的 logicalPool/volume 决定,CreateVolumeRsp.installPath 只是基于相同请求字段拼装的响应值;删除接口同样按 logicalPool/volume 删除。路径不一致说明响应字段异常,不能信任它执行删除,否则返回源卷或其他卷路径时可能误删数据。当前逻辑及回归测试有意只清理请求的 targetInstallPath。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@ZStack-Robot 感谢周众的解释,这个说明是合理的。既然 ZBS 创建卷时实际使用的标识是请求中的 logicalPool/volume(即 target 解析出的字段),而 CreateVolumeRsp.installPath 只是基于同样字段拼装出的回显值,一旦该字段异常,也不能保证其指向的就是真正创建出来的卷;而删除接口同样是按 logicalPool/volume 定位卷。若信任这个不可靠的回显字段去执行删除,反而有误删源卷或其他卷的风险。因此当前只清理 targetInstallPath(对应实际用于创建的请求参数)是更安全的选择,我撤回此前的建议。


@Override
public void deleteConversionTarget(String targetInstallPath, Completion completion) {
controller.doDeleteVolume(targetInstallPath, true, completion);
}

@Override
public void stats(String installPath, ReturnValueCompletion<VolumeStats> completion) {
controller.stats(installPath, completion);
Expand Down Expand Up @@ -207,6 +301,25 @@ private CbdPath parseActiveCbdPath(String installPath) {
return path;
}

private CbdPath parseConversionCbdPath(String installPath) {
if (StringUtils.isBlank(installPath) || !installPath.startsWith("cbd:") || installPath.contains("@")) {
throw new OperationFailureException(operr(
"ZBS volume encryption conversion requires an active CBD path, but got[%s]", installPath));
}

String[] parts = installPath.substring("cbd:".length()).split("/", -1);
if (parts.length != 3 || StringUtils.isBlank(parts[0]) || StringUtils.isBlank(parts[1]) ||
StringUtils.isBlank(parts[2])) {
throw new OperationFailureException(operr("invalid ZBS CBD path[%s]", installPath));
}

CbdPath path = new CbdPath();
path.physicalPool = parts[0];
path.logicalPool = parts[1];
path.volume = parts[2];
return path;
}

private long luksBackingSize(long virtualSize) {
return virtualSize + LUKS_PAYLOAD_OFFSET;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,24 @@ public void fail(ErrorCode errorCode) {
});
}

@Override
protected void handle(ConvertVolumeEncryptionOnPrimaryStorageMsg msg) {
ConvertVolumeEncryptionOnPrimaryStorageReply reply = new ConvertVolumeEncryptionOnPrimaryStorageReply();
controller.convertVolumeEncryption(msg,
new ReturnValueCompletion<ConvertVolumeEncryptionOnPrimaryStorageReply>(msg) {
@Override
public void success(ConvertVolumeEncryptionOnPrimaryStorageReply returnValue) {
bus.reply(msg, returnValue);
}

@Override
public void fail(ErrorCode errorCode) {
reply.setError(errorCode);
bus.reply(msg, reply);
}
});
}

protected void handle(final SetTrashExpirationTimeMsg msg) {
controller.setTrashExpireTime(msg.getExpirationTime(), new Completion(msg) {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ private void createOnPreparedBacking(String primaryStorageUuid, String installPa
ReturnValueCompletion<VolumeStats> completion) {
ZbsVolumeEncryptionMaterial material;
try {
material = materialFactory.prepareVolumeEncryption(primaryStorageUuid, spec.getUuid(),
material = materialFactory.prepareVolumeEncryption(primaryStorageUuid, spec,
String.format("create encrypted volume[uuid:%s]", spec.getUuid()));
} catch (OperationFailureException e) {
completion.fail(e.getErrorCode());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ static class KVMHostLuksResizeCmd extends KVMAgentCommands.AgentCommand implemen
public Long virtualSize;
}

static class KVMHostLuksConvertCmd extends KVMAgentCommands.AgentCommand implements Serializable {
public String psUuid;
@NoLogging
public String encryptedDek;
public String installPath;
public String targetInstallPath;
public boolean targetEncrypted;
public long virtualSize;
}

static class KVMHostLuksRsp extends KVMAgentCommands.AgentResponse {
public Long actualSize;
public String installPath;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ final class ZbsVolumeEncryptionConstants {
static final String KVM_HOST_LUKS_CLONE_PATH = "/zbs/primarystorage/kvmhost/luksclone";
static final String KVM_HOST_LUKS_CREATE_EMPTY_PATH = "/zbs/primarystorage/kvmhost/lukscreateempty";
static final String KVM_HOST_LUKS_ENCRYPT_IN_PLACE_PATH = "/zbs/primarystorage/kvmhost/encryptinplace";
static final String KVM_HOST_LUKS_CONVERT_PATH = "/zbs/primarystorage/kvmhost/luksconvert";
static final String KVM_HOST_LUKS_RESIZE_PATH = "/zbs/primarystorage/kvmhost/luksresize";

private ZbsVolumeEncryptionConstants() {
Expand Down
Loading