fix[vm]: reject Legacy-BIOS vm on ZBS-vhost root volume (ZSTAC-86547)#4476
Closed
ZStack-Robot wants to merge 1 commit into
Closed
fix[vm]: reject Legacy-BIOS vm on ZBS-vhost root volume (ZSTAC-86547)#4476ZStack-Robot wants to merge 1 commit into
ZStack-Robot wants to merge 1 commit into
Conversation
|
Warning
|
| Layer / File(s) | Summary |
|---|---|
分配特性与容量契约定义 header/.../PrimaryStorageFeature.java, header/.../StorageCapabilities.java, header/.../PrimaryStorageAllocationSpec.java |
新增LEGACY_BOOT、默认值为512的minLogicalSectorSize及requiredProtocol字段和访问器。 |
VM根卷分配时检测Legacy镜像 compute/.../VmAllocatePrimaryStorageFlow.java |
读取镜像bootMode系统标签，Legacy镜像根卷分配时设置LEGACY_BOOT。 |
分配规格构建与协议解析 storage/.../PrimaryStorageManagerImpl.java |
从卷协议系统标签解析并写入requiredProtocol，更新主存分配方法签名。 |
主存分配接口与外部主存过滤实现 storage/.../PrimaryStorageFeatureAllocatorExtensionPoint.java, storage/.../PrimaryStorageFeatureAllocatorFlow.java, storage/.../ExternalPrimaryStorageFactory.java |
沿分配调用链传递协议，并按Vhost协议及minLogicalSectorSize > 512过滤Legacy启动不兼容候选。 |
ZBS控制器扇区大小配置 plugin/zbs/.../ZbsStorageController.java |
将控制器的minLogicalSectorSize设置为4096。 |
Legacy启动分配集成测试 test/.../ZbsVhostLegacyBootAllocateCase.groovy, test/.../ZbsVhostVolumeCase.groovy |
验证Legacy镜像分配失败、UEFI镜像不被过滤，并补充UEFI系统标签。 |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Sequence Diagram(s)
sequenceDiagram
participant VmAllocatePrimaryStorageFlow
participant PrimaryStorageManagerImpl
participant PrimaryStorageFeatureAllocatorFlow
participant ExternalPrimaryStorageFactory
participant ZbsStorageController
VmAllocatePrimaryStorageFlow->>VmAllocatePrimaryStorageFlow: 读取镜像bootMode
VmAllocatePrimaryStorageFlow->>PrimaryStorageManagerImpl: 发送LEGACY_BOOT分配请求
PrimaryStorageManagerImpl->>PrimaryStorageFeatureAllocatorFlow: 传递requiredProtocol
PrimaryStorageFeatureAllocatorFlow->>ExternalPrimaryStorageFactory: 过滤主存候选
ExternalPrimaryStorageFactory->>ZbsStorageController: 读取minLogicalSectorSize
ExternalPrimaryStorageFactory-->>PrimaryStorageFeatureAllocatorFlow: 移除不兼容Vhost候选
Poem
小兔子敲代码到深夜，🐇
Legacy启动要防坏，
扇区太大就绕开，
Vhost协议记心怀，
测试通过蹦蹦跳！🥕
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | 标题准确概括了本次修复的核心问题：阻止 Legacy-BIOS 的 ZBS-vhost 根盘虚机启动。 |
| Description check | ✅ Passed | 描述与变更内容高度一致，清楚说明了问题、修复思路和测试影响。 |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
sync/jin.ma/fix/ZSTAC-86547
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java (1)
619-642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value两处拦截逻辑高度相似，可提取公共校验方法
validateZbsVhostLegacyBootOnStart与validateZbsVhostLegacyBootOnCreate均遵循「取 psUuid → 判断 ZBS-vhost → 判断 bootMode → 抛出拦截异常」的相同流程，仅错误码与错误信息文案略有差异。可以抽取一个私有辅助方法（接收 psUuid、bootMode、errorCode 三个参数）以消除重复逻辑。Also applies to: 1283-1294
🤖 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 `@compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java` around lines 619 - 642, The start-time and create-time ZBS-vhost legacy boot checks in validateZbsVhostLegacyBootOnStart and validateZbsVhostLegacyBootOnCreate duplicate the same psUuid lookup, ZBS-vhost detection, bootMode check, and exception flow. Extract that shared logic into a private helper that takes the primary storage UUID, boot mode, and error code/message parameters, and have both methods delegate to it while keeping their specific validation source and error text intact.test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostLegacyBootInterceptCase.groovy (1)
143-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win断言偏弱，无法确认 UEFI 创建真正成功
当前断言
result.error == null || !contains(ORG_ZSTACK_COMPUTE_VM_10335)只要失败原因不是该拦截错误码即视为通过，无法验证 UEFI + ZBS-vhost 根卷创建确实成功。若创建流程因其他原因失败（例如 stub/环境配置问题），该测试仍会通过，掩盖了真实回归。建议直接断言result.error == null，必要时补充资源清理（destroyVmInstance/expungeVmInstance）以与用例末尾环境保持一致。🤖 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 `@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostLegacyBootInterceptCase.groovy` around lines 143 - 156, The test assertion in testUefiRootOnZbsVhostNotRejectedByInterceptor is too weak because it passes on any failure that is not ORG_ZSTACK_COMPUTE_VM_10335. Tighten the check by asserting that CreateVmInstanceAction.call() returns no error at all, so the UEFI + ZBS-vhost root-volume creation is verified as truly successful. If the test leaves a VM behind, add cleanup using the existing VM lifecycle helpers such as destroyVmInstance or expungeVmInstance to keep the test environment consistent.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.java`:
- Around line 619-642: The start-time and create-time ZBS-vhost legacy boot
checks in validateZbsVhostLegacyBootOnStart and
validateZbsVhostLegacyBootOnCreate duplicate the same psUuid lookup, ZBS-vhost
detection, bootMode check, and exception flow. Extract that shared logic into a
private helper that takes the primary storage UUID, boot mode, and error
code/message parameters, and have both methods delegate to it while keeping
their specific validation source and error text intact.
In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostLegacyBootInterceptCase.groovy`:
- Around line 143-156: The test assertion in
testUefiRootOnZbsVhostNotRejectedByInterceptor is too weak because it passes on
any failure that is not ORG_ZSTACK_COMPUTE_VM_10335. Tighten the check by
asserting that CreateVmInstanceAction.call() returns no error at all, so the
UEFI + ZBS-vhost root-volume creation is verified as truly successful. If the
test leaves a VM behind, add cleanup using the existing VM lifecycle helpers
such as destroyVmInstance or expungeVmInstance to keep the test environment
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b7e7259f-6e0e-4e78-8662-fd025a3252e5
📒 Files selected for processing (4)
compute/src/main/java/org/zstack/compute/vm/VmInstanceApiInterceptor.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostLegacyBootInterceptCase.groovytest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
MatheMatrix
force-pushed
the
sync/jin.ma/fix/ZSTAC-86547
branch
from
c9567cd to
eb1af7c
Compare
Owner
|
Comment from jin.ma: 已按评审意见重做：废弃 |
MatheMatrix
force-pushed
the
sync/jin.ma/fix/ZSTAC-86547
branch
3 times, most recently
from
8972503 to
e2d78b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/storage/primary/PrimaryStorageAllocationSpec.java (1)
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win建议使用
VolumeProtocol枚举类型而非String。
requiredProtocol使用String类型存储协议信息，而同一代码库中类似字段（如StorageCapabilities.defaultIsoActiveProtocol、defaultImageExportProtocol）均使用VolumeProtocol枚举类型。使用字符串会丧失类型安全，调用方需要自行进行字符串比较（如ExternalPrimaryStorageFactory中按协议名过滤 Vhost 候选时），容易因大小写或拼写差异引入隐患。♻️ 建议改用枚举类型
- private String requiredProtocol; + private VolumeProtocol requiredProtocol; - public String getRequiredProtocol() { + public VolumeProtocol getRequiredProtocol() { return requiredProtocol; } - public void setRequiredProtocol(String requiredProtocol) { + public void setRequiredProtocol(VolumeProtocol requiredProtocol) { this.requiredProtocol = requiredProtocol; }若该值来源于系统标签字符串解析（如摘要所述
resolveRequiredProtocol解析VOLUME_PROTOCOL系统标签），可以在解析处转换为枚举后再赋值。🤖 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 `@header/src/main/java/org/zstack/header/storage/primary/PrimaryStorageAllocationSpec.java` around lines 28 - 36, The requiredProtocol field in PrimaryStorageAllocationSpec is using String instead of the strongly typed VolumeProtocol enum. Update the field, getter/setter, and any related usage to use VolumeProtocol so callers do not rely on raw string comparisons; if the value is parsed from a system tag via resolveRequiredProtocol or similar logic, convert the parsed string to VolumeProtocol at that boundary before assigning it.storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageFactory.java (1)
1107-1107: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win过滤逻辑正确，但排除原因未随异常向上传递
LEGACY_BOOT过滤逻辑本身正确：优先使用显式requiredProtocol,否则按需批量查询各候选主存的defaultProtocol,并仅排除Vhost协议且minLogicalSectorSize > 512的候选。不过，具体排除原因目前只写入了
logger.info（第 1143-1144 行），当候选全部被排除时，上层PrimaryStorageFeatureAllocatorFlow抛出的异常只是通用的 "returns zero primary storage candidate"（ORG_ZSTACK_STORAGE_PRIMARY_10035），不包含“ZBS-vhost 协议扇区过大与 Legacy 启动模式不兼容”这类具体诊断信息。这与本 PR 最初通过专用错误码（10335/10336）向用户明确说明失败原因的诉求相比，用户可见的报错信息可读性有所下降。建议将排除原因携带在返回结果或抛出的异常上下文中（例如通过
ErrorCode的详情字段），以便定位问题时无需查看日志。Also applies to: 1119-1149
🤖 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/addon/primary/ExternalPrimaryStorageFactory.java` at line 1107, The LEGACY_BOOT filtering in allocatePrimaryStorage is correct, but the exclusion reason is only logged and not propagated to the caller. Update the allocation path in ExternalPrimaryStorageFactory.allocatePrimaryStorage so the specific “ZBS-vhost protocol with minLogicalSectorSize > 512 is incompatible with Legacy boot” reason is carried in the returned failure context or ErrorCode details, and ensure PrimaryStorageFeatureAllocatorFlow can surface it when no candidates remain instead of only the generic “returns zero primary storage candidate” message.
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@storage/src/main/java/org/zstack/storage/primary/PrimaryStorageFeatureAllocatorExtensionPoint.java`:
- Around line 13-15:
`PrimaryStorageFeatureAllocatorExtensionPoint.allocatePrimaryStorage` 新增了
`requiredProtocol` 但缺少清晰的契约说明，请为该接口方法补充 Javadoc，明确 `requiredProtocol` 与
`requiredFeatures`、`candidates` 的交互语义（例如非空时是否覆盖候选主存的
`defaultProtocol`）以及返回值/入参约束；同时按接口规范检查该方法签名是否保留了多余修饰符，并确保注释能帮助实现者正确处理协议选择逻辑。
In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostLegacyBootAllocateCase.groovy`:
- Around line 152-167: The assertion in
testUefiImageNotRejectedByLegacyBootFilter is too weak because it passes on any
failure except the specific primary-storage message. Tighten the check by
asserting the CreateVmInstanceAction.Result has no error at all, and keep the
test focused on the legacy-boot allocator behavior; if there is any known
unrelated failure path in this scenario, document it in a nearby comment rather
than broadening the assertion.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/storage/primary/PrimaryStorageAllocationSpec.java`:
- Around line 28-36: The requiredProtocol field in PrimaryStorageAllocationSpec
is using String instead of the strongly typed VolumeProtocol enum. Update the
field, getter/setter, and any related usage to use VolumeProtocol so callers do
not rely on raw string comparisons; if the value is parsed from a system tag via
resolveRequiredProtocol or similar logic, convert the parsed string to
VolumeProtocol at that boundary before assigning it.
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageFactory.java`:
- Line 1107: The LEGACY_BOOT filtering in allocatePrimaryStorage is correct, but
the exclusion reason is only logged and not propagated to the caller. Update the
allocation path in ExternalPrimaryStorageFactory.allocatePrimaryStorage so the
specific “ZBS-vhost protocol with minLogicalSectorSize > 512 is incompatible
with Legacy boot” reason is carried in the returned failure context or ErrorCode
details, and ensure PrimaryStorageFeatureAllocatorFlow can surface it when no
candidates remain instead of only the generic “returns zero primary storage
candidate” message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4e65e5ae-0f26-47b5-bc08-4f81fc1b66a2
📒 Files selected for processing (11)
compute/src/main/java/org/zstack/compute/vm/VmAllocatePrimaryStorageFlow.javaheader/src/main/java/org/zstack/header/storage/addon/primary/StorageCapabilities.javaheader/src/main/java/org/zstack/header/storage/primary/PrimaryStorageAllocationSpec.javaheader/src/main/java/org/zstack/header/storage/primary/PrimaryStorageFeature.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorageFactory.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageFeatureAllocatorExtensionPoint.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageFeatureAllocatorFlow.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageManagerImpl.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostLegacyBootAllocateCase.groovytest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovy
🚧 Files skipped from review as they are similar to previous changes (1)
- test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovy
Comment on lines
13
to
15
| public interface PrimaryStorageFeatureAllocatorExtensionPoint { | ||
| List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, List<PrimaryStorageVO> candidates); | ||
| List<PrimaryStorageVO> allocatePrimaryStorage(Set<PrimaryStorageFeature> requiredFeatures, String requiredProtocol, List<PrimaryStorageVO> candidates); | ||
| } |
There was a problem hiding this comment.
📐 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
Comment on lines
+152
to
+167
| void testUefiImageNotRejectedByLegacyBootFilter() { | ||
| setImageBootMode(image.uuid, "UEFI") | ||
|
|
||
| CreateVmInstanceAction action = new CreateVmInstanceAction() | ||
| action.name = "uefi-vm" | ||
| action.instanceOfferingUuid = instanceOffering.uuid | ||
| action.imageUuid = image.uuid | ||
| action.l3NetworkUuids = [l3.uuid] | ||
| action.primaryStorageUuidForRootVolume = ps.uuid | ||
| action.sessionId = adminSession() | ||
|
|
||
| CreateVmInstanceAction.Result result = action.call() | ||
| assert result.error == null || | ||
| !JSONObjectUtil.toJsonString(result.error).contains("returns zero primary storage candidate") : | ||
| "UEFI image must not be rejected by the legacy boot allocator filter" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
断言过弱，可能掩盖非预期失败
result.error == null || !...contains("returns zero primary storage candidate") 这一断言只要报错信息不是"zero primary storage candidate"就会通过，即便 VM 创建因其他无关原因失败也会被判定为测试通过，削弱了该用例验证"UEFI 镜像不应被 LEGACY_BOOT 过滤器拒绝"这一核心诉求的能力。
考虑到测试环境已经为 Vhost 存活检测、创建卷等操作注册了模拟桩，建议直接断言 result.error == null，若确实存在其他已知的无关失败点，请在注释中说明原因。
✅ 建议的断言修改
CreateVmInstanceAction.Result result = action.call()
- assert result.error == null ||
- !JSONObjectUtil.toJsonString(result.error).contains("returns zero primary storage candidate") :
- "UEFI image must not be rejected by the legacy boot allocator filter"
+ assert result.error == null :
+ "UEFI image should be able to create VM successfully, actual: ${JSONObjectUtil.toJsonString(result.error)}"📝 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
| void testUefiImageNotRejectedByLegacyBootFilter() { | |
| setImageBootMode(image.uuid, "UEFI") | |
| CreateVmInstanceAction action = new CreateVmInstanceAction() | |
| action.name = "uefi-vm" | |
| action.instanceOfferingUuid = instanceOffering.uuid | |
| action.imageUuid = image.uuid | |
| action.l3NetworkUuids = [l3.uuid] | |
| action.primaryStorageUuidForRootVolume = ps.uuid | |
| action.sessionId = adminSession() | |
| CreateVmInstanceAction.Result result = action.call() | |
| assert result.error == null || | |
| !JSONObjectUtil.toJsonString(result.error).contains("returns zero primary storage candidate") : | |
| "UEFI image must not be rejected by the legacy boot allocator filter" | |
| } | |
| void testUefiImageNotRejectedByLegacyBootFilter() { | |
| setImageBootMode(image.uuid, "UEFI") | |
| CreateVmInstanceAction action = new CreateVmInstanceAction() | |
| action.name = "uefi-vm" | |
| action.instanceOfferingUuid = instanceOffering.uuid | |
| action.imageUuid = image.uuid | |
| action.l3NetworkUuids = [l3.uuid] | |
| action.primaryStorageUuidForRootVolume = ps.uuid | |
| action.sessionId = adminSession() | |
| CreateVmInstanceAction.Result result = action.call() | |
| assert result.error == null : | |
| "UEFI image should be able to create VM successfully, actual: ${JSONObjectUtil.toJsonString(result.error)}" | |
| } |
🤖 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
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostLegacyBootAllocateCase.groovy`
around lines 152 - 167, The assertion in
testUefiImageNotRejectedByLegacyBootFilter is too weak because it passes on any
failure except the specific primary-storage message. Tighten the check by
asserting the CreateVmInstanceAction.Result has no error at all, and keep the
test focused on the legacy-boot allocator behavior; if there is any known
unrelated failure path in this scenario, document it in a nearby comment rather
than broadening the assertion.
A vm created with a ZBS-vhost root volume reached Running but SeaBIOS reported "No bootable device", because the ZBS-vhost bdev exposes a 4096-byte logical block size on which traditional 512-sector BIOS/MBR (Legacy, non-UEFI) images cannot boot. Replace the hardcoded VmInstanceApiInterceptor guard with a layered allocator filter reusing the existing requiredFeatures pipeline: - StorageCapabilities gains minLogicalSectorSize (default 512); the ZBS controller reports 4096 when its default protocol is Vhost. - Add PrimaryStorageFeature.LEGACY_BOOT. VmAllocatePrimaryStorageFlow detects a Legacy-boot image (via image bootMode system tag) and sets the feature on the allocate msg. - ExternalPrimaryStorageFactory filters out candidates whose reported minLogicalSectorSize exceeds 512 when LEGACY_BOOT is required, so a Legacy image can no longer land on a ZBS-vhost primary storage. Resolves: ZSTAC-86547 Change-Id: If64d639597498884a084108ad76f664b923e158f
MatheMatrix
force-pushed
the
sync/jin.ma/fix/ZSTAC-86547
branch
from
e2d78b8 to
5b9dc6f
Compare
Collaborator
|
Comment from gitlab: 自上次添加REVIEWED标签(2026-07-10 02:38:09.000Z)后, 有新的COMMIT更新(2026-07-10 09:34:57.361Z), 所以移除了REVIEWED标签 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
问题（ZSTAC-86547）
用 ZBS-vhost 根盘创建的 VM 状态 Running，但控制台 SeaBIOS 报
No bootable device，无法进入系统。根因：ZBS-vhost 的 bdev 暴露 4096 字节 logical block size，传统 512-sector BIOS/MBR（Legacy，非 UEFI）镜像在 logical=4096 的启动路径下无法引导。这是 Jira 分析里给出的两条修复方向中的兜底拦截方案（另一条根治方向是 agent 侧 bdev_zbs 实现 512e，工作量大，本 MR 先止血）。
修复
在
VmInstanceApiInterceptor两处拦截 Legacy + ZBS-vhost 根盘的 VM：APICreateVmInstanceMsg）：当显式指定根盘主存储为 ZBS-vhost（identity=zbs 且输出协议 Vhost）且 bootMode 非 UEFI 时，抛ApiMessageInterceptionException（error codeORG_ZSTACK_COMPUTE_VM_10335）提前拦截。APIStartVmInstanceMsg）：VM 已实例化、根盘主存储已确定时，通过 vmUuid → rootVolume → primaryStorage 判定，同样拦截（error code10336）。bootMode 解析口径：VM 的
bootMode::systemTag，无 tag 即视为 Legacy（与KVMHost默认口径一致）。局限（重要）
创建拦截仅覆盖用户显式指定根盘主存储的场景（
primaryStorageUuidForRootVolume非空）。未指定时根盘 PS 由 allocator 调度期决定，创建拦截阶段无法可靠判断——这类场景由启动拦截兜底（此时 PS 已确定）。测试
ZbsVhostLegacyBootInterceptCase：Legacy + ZBS-vhost 根盘创建被拒（断言 error code10335）；UEFI tag 放行不被拦。本地实跑Tests run: 1, Failures: 0。ZbsVhostVolumeCase.testVhostVmStartActivationChain建的是 Legacy vhost 根盘 VM 并期望 Running，会被本拦截误伤——已将其 bootMode 改为 UEFI（该用例本意是验证 vhost 启动链，与 bootMode 无关）。本地实跑Tests run: 1, Failures: 0。Resolves: ZSTAC-86547
sync from gitlab !10437