Skip to content

fix[vm]: reject Legacy-BIOS vm on ZBS-vhost root volume (ZSTAC-86547) #4476

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

Closed
ZStack-Robot wants to merge 1 commit into from
Closed

fix[vm]: reject Legacy-BIOS vm on ZBS-vhost root volume (ZSTAC-86547) #4476

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
15 changes: 15 additions & 0 deletions compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,16 @@
import org.zstack.header.errorcode.ErrorCode;
import org.zstack.header.host.HostInventory;
import org.zstack.header.image.ImageInventory;
import org.zstack.header.image.ImageBootMode;
import org.zstack.header.image.ImageVO;
import org.zstack.header.message.MessageReply;
import org.zstack.header.storage.backup.BackupStorageVO;
import org.zstack.header.storage.backup.BackupStorageVO_;
import org.zstack.header.storage.primary.AllocatePrimaryStorageSpaceMsg;
import org.zstack.header.storage.primary.AllocatePrimaryStorageSpaceReply;
import org.zstack.header.storage.primary.PrimaryStorageAllocationPurpose;
import org.zstack.header.storage.primary.PrimaryStorageConstant;
import org.zstack.header.storage.primary.PrimaryStorageFeature;
import org.zstack.header.storage.primary.ReleasePrimaryStorageSpaceMsg;
import org.zstack.header.vm.VmInstanceConstant;
import org.zstack.header.vm.VmInstanceSpec;
Expand All @@ -37,6 +40,7 @@

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -80,6 +84,9 @@ public void run(final FlowTrigger trigger, final Map data) {
rmsg.setPurpose(PrimaryStorageAllocationPurpose.CreateNewVm.toString());
rmsg.setPossiblePrimaryStorageTypes(selectPsTypesFromSpec(spec));
rmsg.setSystemTags(spec.getRootVolumeSystemTags());
if (isLegacyBootImage(rmsg.getImageUuid())) {
rmsg.setRequiredFeatures(Collections.singleton(PrimaryStorageFeature.LEGACY_BOOT));
}
bus.makeLocalServiceId(rmsg, PrimaryStorageConstant.SERVICE_ID);
msgs.add(rmsg);

Expand Down Expand Up @@ -170,6 +177,14 @@ public void rollback(FlowRollback chain, Map data) {
chain.rollback();
}

private boolean isLegacyBootImage(String imageUuid) {
if (imageUuid == null) {
return false;
}
String bootMode = VmSystemTags.BOOT_MODE.getTokenByResourceUuid(imageUuid, ImageVO.class, VmSystemTags.BOOT_MODE_TOKEN);
return bootMode == null || bootMode.equalsIgnoreCase(ImageBootMode.Legacy.name());
}

private List<String> selectPsTypesFromSpec(final VmInstanceSpec spec) {
// get ps types from image bs and cdroms bs
List<String> psTypes = null;
Expand Down
9 changes: 9 additions & 0 deletions header/src/main/java/org/zstack/header/storage/addon/primary/StorageCapabilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@ public class StorageCapabilities {
public List<String> supportedImageFormats;
private VolumeProtocol defaultIsoActiveProtocol;
private VolumeProtocol defaultImageExportProtocol;
private int minLogicalSectorSize = 512;

public int getMinLogicalSectorSize() {
return minLogicalSectorSize;
}

public void setMinLogicalSectorSize(int minLogicalSectorSize) {
this.minLogicalSectorSize = minLogicalSectorSize;
}

public boolean isSupportShareableVolume() {
return supportShareableVolume;
Expand Down
9 changes: 9 additions & 0 deletions header/src/main/java/org/zstack/header/storage/primary/PrimaryStorageAllocationSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@ public class PrimaryStorageAllocationSpec {
private List<String> excludePrimaryStorageTypes;
private String backupStorageUuid;
private Set<PrimaryStorageFeature> requiredFeatures;
private String requiredProtocol;

public String getRequiredProtocol() {
return requiredProtocol;
}

public void setRequiredProtocol(String requiredProtocol) {
this.requiredProtocol = requiredProtocol;
}

public Set<PrimaryStorageFeature> getRequiredFeatures() {
return requiredFeatures;
Expand Down
1 change: 1 addition & 0 deletions header/src/main/java/org/zstack/header/storage/primary/PrimaryStorageFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@
*/
public enum PrimaryStorageFeature {
SHARED_VOLUME,
LEGACY_BOOT,
}
1 change: 1 addition & 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 @@ -142,6 +142,7 @@ public class ZbsStorageController implements PrimaryStorageControllerSvc, Primar
capabilities.setSupportedImageFormats(Collections.singletonList(ImageConstant.RAW_FORMAT_STRING));
capabilities.setDefaultIsoActiveProtocol(VolumeProtocol.CBD);
capabilities.setDefaultImageExportProtocol(VolumeProtocol.NBD);
capabilities.setMinLogicalSectorSize(4096);
}

@Override
Expand Down
34 changes: 33 additions & 1 deletion 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 @@ -41,6 +41,7 @@
import org.zstack.header.vm.cdrom.VmCdRomVO_;
import org.zstack.header.volume.VolumeDeletionPolicyManager;
import org.zstack.header.volume.VolumeInventory;
import org.zstack.header.volume.VolumeProtocol;
import org.zstack.header.volume.VolumeVO;
import org.zstack.header.volume.VolumeVO_;
import org.zstack.header.volume.block.BlockVolumeVO;
Expand Down Expand Up @@ -1103,7 +1104,7 @@ public void iJoin(ManagementNodeInventory inv) {
}

@Override
public List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, List<PrimaryStorageVO> candidates) {
public List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, String requiredProtocol, List<PrimaryStorageVO> candidates) {
if (requiredFeatures.contains(PrimaryStorageFeature.SHARED_VOLUME)) {
List<PrimaryStorageVO> excludeCandidates = candidates.stream()
.filter(v -> PrimaryStorageConstant.EXTERNAL_PRIMARY_STORAGE_TYPE.equals(v.getType()))
Expand All @@ -1115,6 +1116,37 @@ public List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature>
candidates.removeAll(excludeCandidates);
}

if (requiredFeatures.contains(PrimaryStorageFeature.LEGACY_BOOT)) {
List<PrimaryStorageVO> externalCandidates = candidates.stream()
.filter(v -> PrimaryStorageConstant.EXTERNAL_PRIMARY_STORAGE_TYPE.equals(v.getType()))
.filter(v -> controllers.containsKey(v.getUuid()))
.collect(Collectors.toList());

if (!externalCandidates.isEmpty()) {
Map<String, String> protocolByUuid = requiredProtocol != null ? Collections.emptyMap() :
Q.New(ExternalPrimaryStorageVO.class)
.select(ExternalPrimaryStorageVO_.uuid, ExternalPrimaryStorageVO_.defaultProtocol)
.in(ExternalPrimaryStorageVO_.uuid, externalCandidates.stream().map(PrimaryStorageVO::getUuid).collect(Collectors.toList()))
.listTuple().stream()
.collect(Collectors.toMap(t -> t.get(0, String.class), t -> t.get(1, String.class)));

List<PrimaryStorageVO> excludeCandidates = externalCandidates.stream()
.filter(v -> {
String protocol = requiredProtocol != null ? requiredProtocol : protocolByUuid.get(v.getUuid());
// only vhost-user-blk exposes the raw logical sector to the guest; other protocols
// go through the qemu block layer which does 512e emulation
return VolumeProtocol.Vhost.toString().equals(protocol)
&& controllers.get(v.getUuid()).reportCapabilities().getMinLogicalSectorSize() > 512;
})
.collect(Collectors.toList());

logger.info(String.format("exclude external primary storage candidates: %s for legacy boot feature, " +
"they expose a logical sector size larger than 512 bytes on which a Legacy-BIOS image cannot boot", excludeCandidates));

candidates.removeAll(excludeCandidates);
}
}

return candidates;
}

Expand Down
2 changes: 1 addition & 1 deletion ...rc/main/java/org/zstack/storage/primary/PrimaryStorageFeatureAllocatorExtensionPoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@
* @ Date : Created in 10:49 2025/7/15
*/
public interface PrimaryStorageFeatureAllocatorExtensionPoint {
List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, List<PrimaryStorageVO> candidates);
List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, String requiredProtocol, List<PrimaryStorageVO> candidates);
}
Comment on lines 13 to 15

@coderabbitai coderabbitai Bot Jul 9, 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.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

为接口方法补充 Javadoc

allocatePrimaryStorage 新增了 requiredProtocol 参数，其与 candidates/requiredFeatures 的交互语义（例如 requiredProtocol 非空时会覆盖各候选主存自身的 defaultProtocol）并不直观，建议补充 Javadoc 说明该参数的语义与调用契约,便于后续实现者正确处理。

As per path instructions: "API 消息上必须添加注解 @RestRequest...接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。"

📝 建议补充的 Javadoc 
 public interface PrimaryStorageFeatureAllocatorExtensionPoint {
+    /**
+     * Filters/reorders primary storage candidates based on required features.
+     *
+     * `@param` requiredFeatures features the allocation must satisfy (e.g. LEGACY_BOOT)
+     * `@param` requiredProtocol explicit volume protocol requested for this allocation;
+     *                         when non-null it takes precedence over a candidate's own
+     *                         default protocol when evaluating feature compatibility
+     * `@param` candidates the current candidate list to filter, may be mutated in place
+     * `@return` the filtered candidate list, or {`@code` null} to skip this extension point
+     */
     List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, String requiredProtocol, List<PrimaryStorageVO> candidates);
 }
📝 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 interface PrimaryStorageFeatureAllocatorExtensionPoint {
List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, List<PrimaryStorageVO> candidates);
List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, String requiredProtocol, List<PrimaryStorageVO> candidates);
}
public interface PrimaryStorageFeatureAllocatorExtensionPoint {
/**
* Filters/reorders primary storage candidates based on required features.
*
* `@param` requiredFeatures features the allocation must satisfy (e.g. LEGACY_BOOT)
* `@param` requiredProtocol explicit volume protocol requested for this allocation;
* when non-null it takes precedence over a candidate's own
* default protocol when evaluating feature compatibility
* `@param` candidates the current candidate list to filter, may be mutated in place
* `@return` the filtered candidate list, or {`@code` null} to skip this extension point
*/
List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, String requiredProtocol, List<PrimaryStorageVO> candidates);
}
🤖 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
`@storage/src/main/java/org/zstack/storage/primary/PrimaryStorageFeatureAllocatorExtensionPoint.java`
around lines 13 - 15,
`PrimaryStorageFeatureAllocatorExtensionPoint.allocatePrimaryStorage` 新增了
`requiredProtocol` 但缺少清晰的契约说明，请为该接口方法补充 Javadoc，明确 `requiredProtocol` 与
`requiredFeatures`、`candidates` 的交互语义（例如非空时是否覆盖候选主存的
`defaultProtocol`）以及返回值/入参约束；同时按接口规范检查该方法签名是否保留了多余修饰符，并确保注释能帮助实现者正确处理协议选择逻辑。

Source: Path instructions

2 changes: 1 addition & 1 deletion storage/src/main/java/org/zstack/storage/primary/PrimaryStorageFeatureAllocatorFlow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public void run(FlowTrigger trigger, Map data) {
List<PrimaryStorageVO> candidates = (List<PrimaryStorageVO>) data.get(PrimaryStorageConstant.AllocatorParams.CANDIDATES);
List<PrimaryStorageVO> ret;
for (PrimaryStorageFeatureAllocatorExtensionPoint extp : pluginRgty.getExtensionList(PrimaryStorageFeatureAllocatorExtensionPoint.class)) {
ret = extp.allocatePrimaryStorage(spec.getRequiredFeatures(), candidates);
ret = extp.allocatePrimaryStorage(spec.getRequiredFeatures(), spec.getRequiredProtocol(), candidates);
if (ret == null) {
continue;
}
Expand Down
12 changes: 11 additions & 1 deletion storage/src/main/java/org/zstack/storage/primary/PrimaryStorageManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import org.zstack.header.vm.VmInstanceStartExtensionPoint;
import org.zstack.header.vm.*;
import org.zstack.resourceconfig.*;
import org.zstack.storage.volume.VolumeSystemTags;
import org.zstack.tag.TagManager;
import org.zstack.utils.*;
import org.zstack.utils.function.Function;
Expand Down Expand Up @@ -830,6 +831,7 @@ private PrimaryStorageAllocationSpec buildAllocateSpecFromMsg(AllocatePrimarySto
PrimaryStorageAllocationSpec spec = new PrimaryStorageAllocationSpec();
spec.setPossiblePrimaryStorageTypes(msg.getPossiblePrimaryStorageTypes());
spec.setRequiredFeatures(msg.getRequiredFeatures());
spec.setRequiredProtocol(resolveRequiredProtocol(msg));
spec.setExcludePrimaryStorageTypes(msg.getExcludePrimaryStorageTypes());
spec.setImageUuid(msg.getImageUuid());
spec.setDiskOfferingUuid(msg.getDiskOfferingUuid());
Expand Down Expand Up @@ -858,6 +860,14 @@ private PrimaryStorageAllocationSpec buildAllocateSpecFromMsg(AllocatePrimarySto
return spec;
}

private String resolveRequiredProtocol(AllocatePrimaryStorageMsg msg) {
String protocolTag = msg.getSystemTag(VolumeSystemTags.VOLUME_PROTOCOL::isMatch);
if (protocolTag == null) {
return null;
}
return VolumeSystemTags.VOLUME_PROTOCOL.getTokenByTag(protocolTag, VolumeSystemTags.VOLUME_PROTOCOL_TOKEN);
}

@Override
public String getId() {
return bus.makeLocalServiceId(PrimaryStorageConstant.SERVICE_ID);
Expand Down Expand Up @@ -1027,7 +1037,7 @@ public void sort(List<PrimaryStorageVO> primaryStorageVOS, VmInstanceSpec.ImageS
}

@Override
public List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, List<PrimaryStorageVO> candidates) {
public List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, String requiredProtocol, List<PrimaryStorageVO> candidates) {
if (requiredFeatures.contains(PrimaryStorageFeature.SHARED_VOLUME)) {
candidates = candidates.stream()
.filter(v -> PrimaryStorageType.getSupportFeaturesTypes(PrimaryStorageType::isSupportSharedVolume)
Expand Down
Loading