<fix>[compute]: introduce NVRAM type volume#3347
<fix>[compute]: introduce NVRAM type volume#3347zstack-robot-2 wants to merge 2 commits intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough增加对 NVRAM 的支持：新增常量与卷类型、在 VmInstanceSpec 中加入 nvRamSpec、在 VmTpmExtensions 构建规范时创建 nvram 规格；VmTpmManager 增加注册判断；KVM 安全引导、本地存储与卷服务新增 NVRAM 的创建、实例化、路径与映射逻辑。 变更
时序图sequenceDiagram
participant Builder as VM构建器
participant TpmMgr as VmTpmManager
participant VmTpmExt as VmTpmExtensions
participant SecBootExt as KvmSecureBootExtensions
participant CloudBus as CloudBus
participant VolService as 卷服务
participant KvmAgent as KVM Agent
Builder->>TpmMgr: needRegisterNvram(vmUuid)?
TpmMgr->>TpmMgr: 检查 TPM 表或读取 ENABLE_UEFI_SECURE_BOOT
TpmMgr-->>Builder: 返回 boolean
alt 需要 NVRAM
Builder->>VmTpmExt: afterBuildVmSpec(spec)
VmTpmExt->>VmTpmExt: 创建 DiskAO nvramSpec (NVRAM_DEFAULT_SIZE, 命名)
VmTpmExt-->>Builder: 设置 spec.nvRamSpec
Builder->>SecBootExt: preInstantiateVmResource(spec)
SecBootExt->>CloudBus: CreateVolumeMsg / InstantiateVolumeMsg
CloudBus->>VolService: 转发创建/实例化请求
VolService-->>CloudBus: 返回结果
CloudBus-->>SecBootExt: 回调结果
SecBootExt->>KvmAgent: 将 nvRam 映射到 StartVmCmd.nvRam
else 无需 NVRAM
Builder->>SecBootExt: preReleaseVmResource(spec)
SecBootExt->>CloudBus: DeleteVolumeMsg（如存在则删除）
end
代码审查工作量评估🎯 4 (复杂) | ⏱️ ~45 分钟 诗歌
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 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: 1
🤖 Fix all issues with AI agents
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java`:
- Around line 132-136: The fail(ErrorCode) handler incorrectly calls
deleteNvramVolumeAnyway(spec.getVmInventory().getUuid(), completion) when NVRAM
volume creation fails; since the volume never existed this is wrong and it also
swallows the original error by ultimately calling completion.success(). Replace
the call in the anonymous fail(ErrorCode) implementation so that it forwards the
original error to the completion (e.g., call completion.fail(errorCode) or the
equivalent error callback) instead of invoking deleteNvramVolumeAnyway;
reference the fail(ErrorCode) method, deleteNvramVolumeAnyway, and the
completion object to locate and change the logic.
🧹 Nitpick comments (3)
header/src/main/java/org/zstack/header/volume/VolumeType.java (1)
7-8: 枚举命名不一致：NVRAM应改为Nvram以匹配现有风格。现有枚举值使用 UpperCamelCase 风格（
Root、Data、Memory、Cache），而NVRAM使用全大写。建议保持一致性。♻️ 建议修改
Cache, - NVRAM, + Nvram,plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java (1)
113-113: TODO 注释：应验证 NVRAM 卷状态为 Ready。当 NVRAM 卷已存在时，应检查其状态是否为 Ready，以确保卷可用于 VM 启动。
是否需要我帮助实现卷状态检查的逻辑？
compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java (1)
41-47: 建议添加 rootSpec 的空值检查。
spec.getRootDisk()在某些场景下可能返回 null。虽然注释说明primaryStorageUuid可能为 null 且下游已处理，但如果rootSpec本身为 null，调用rootSpec.getPrimaryStorageUuid()会抛出 NullPointerException。🛡️ 建议添加防御性检查
DiskAO nvramSpec = new DiskAO(); DiskAO rootSpec = spec.getRootDisk(); // nvram always use root disk's primary storage - nvramSpec.setPrimaryStorageUuid(rootSpec.getPrimaryStorageUuid()); // may be null + nvramSpec.setPrimaryStorageUuid(rootSpec != null ? rootSpec.getPrimaryStorageUuid() : null); nvramSpec.setSize(NVRAM_DEFAULT_SIZE);
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java
Outdated
Show resolved
Hide resolved
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap@@2
branch
from
3eaf131 to
9fe3dbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java`:
- Around line 41-47: The code assumes spec.getRootDisk() is non-null; add a
defensive null check for rootSpec before calling
rootSpec.getPrimaryStorageUuid() in the block that builds nvramSpec (DiskAO
nvramSpec, DiskAO rootSpec, nvramSpec.setPrimaryStorageUuid(...),
spec.setNvRamSpec(...)). If rootSpec is null, set
nvramSpec.setPrimaryStorageUuid(null) or otherwise avoid dereferencing rootSpec
(e.g., leave primaryStorageUuid unset or use a safe fallback), then continue to
set size/name and call spec.setNvRamSpec(nvramSpec).
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java`:
- Around line 80-87: prepareNvRamToStartVmCmd currently calls Q.New(...).find()
and immediately passes the result to VolumeInventory.valueOf(vo), which will NPE
if the VolumeVO is null; update prepareNvRamToStartVmCmd to defensively check
the result of Q.New(VolumeVO.class).eq(VolumeVO_.uuid,
nvRamSpec.getSourceUuid()).find() (vo) before calling
VolumeInventory.valueOf(vo) and before cmd.setNvRam(...); if vo is null, either
log a clear warning including nvRamSpec.getSourceUuid() and return without
setting the NVRAM on the cmd or throw a clear exception, ensuring callers handle
the missing volume case rather than allowing a NullPointerException.
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java`:
- Around line 1295-1297: The CreateEmptyVolumeCmd for NVRAM volumes is missing
format/type info and the code currently hardcodes VOLUME_FORMAT_QCOW2; update
the logic in LocalStorageKvmBackend where the CreateEmptyVolumeCmd is built (the
code that sets installUrl via makeNvRamVolumeInstallUrl and the place currently
setting the format to VOLUME_FORMAT_QCOW2) to set the command's format to
VOLUME_FORMAT_RAW when volume.getType() == VolumeType.NVRAM, and include the
volumeType (or format) field on CreateEmptyVolumeCmd so the agent can detect
NVRAM correctly; ensure any existing default remains QCOW2 for non‑NVRAM volumes
and mirror KvmSecureBootExtensions behavior for NVRAM.
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java (1)
118-124: TODO 注释需要处理。Line 122 的 TODO 注释
// TODO make sure status is Ready表明需要验证 NVRAM 卷的状态是否为 Ready。在生产环境中，如果卷状态不是 Ready（例如 Creating 或 Error），可能会导致 VM 启动失败。建议添加状态检查或创建 issue 跟踪此任务。
是否需要我生成状态检查的实现代码，或者创建一个 issue 来跟踪此任务？
| DiskAO nvramSpec = new DiskAO(); | ||
| DiskAO rootSpec = spec.getRootDisk(); | ||
| // nvram always use root disk's primary storage | ||
| nvramSpec.setPrimaryStorageUuid(rootSpec.getPrimaryStorageUuid()); // may be null | ||
| nvramSpec.setSize(NVRAM_DEFAULT_SIZE); | ||
| nvramSpec.setName("nvram-of-VM-" + vmUuid); | ||
| spec.setNvRamSpec(nvramSpec); |
There was a problem hiding this comment.
rootSpec 可能为 null，需要防御性检查。
spec.getRootDisk() 在某些边缘情况下可能返回 null（例如 VM 规格尚未完全构建时）。如果 rootSpec 为 null，line 44 调用 rootSpec.getPrimaryStorageUuid() 会导致 NullPointerException。
🛡️ 建议修复
DiskAO nvramSpec = new DiskAO();
DiskAO rootSpec = spec.getRootDisk();
// nvram always use root disk's primary storage
- nvramSpec.setPrimaryStorageUuid(rootSpec.getPrimaryStorageUuid()); // may be null
+ if (rootSpec != null) {
+ nvramSpec.setPrimaryStorageUuid(rootSpec.getPrimaryStorageUuid());
+ }
nvramSpec.setSize(NVRAM_DEFAULT_SIZE);
nvramSpec.setName("nvram-of-VM-" + vmUuid);
spec.setNvRamSpec(nvramSpec);🤖 Prompt for AI Agents
In `@compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java`
around lines 41 - 47, The code assumes spec.getRootDisk() is non-null; add a
defensive null check for rootSpec before calling
rootSpec.getPrimaryStorageUuid() in the block that builds nvramSpec (DiskAO
nvramSpec, DiskAO rootSpec, nvramSpec.setPrimaryStorageUuid(...),
spec.setNvRamSpec(...)). If rootSpec is null, set
nvramSpec.setPrimaryStorageUuid(null) or otherwise avoid dereferencing rootSpec
(e.g., leave primaryStorageUuid unset or use a safe fallback), then continue to
set size/name and call spec.setNvRamSpec(nvramSpec).
| private void prepareNvRamToStartVmCmd(KVMAgentCommands.StartVmCmd cmd, DiskAO nvRamSpec, KVMHostInventory host) { | ||
| VolumeVO vo = Q.New(VolumeVO.class) | ||
| .eq(VolumeVO_.uuid, nvRamSpec.getSourceUuid()) | ||
| .find(); | ||
| VolumeInventory nvRamVolume = VolumeInventory.valueOf(vo); | ||
| VolumeTO volume = VolumeTO.valueOfWithOutExtension(nvRamVolume, host, null); | ||
| cmd.setNvRam(volume); | ||
| } |
There was a problem hiding this comment.
prepareNvRamToStartVmCmd 缺少空值检查。
当 nvRamSpec.getSourceUuid() 对应的卷不存在时，Q.New(...).find() 返回 null，后续调用 VolumeInventory.valueOf(vo) 会导致 NullPointerException。
建议添加防御性检查：
🛡️ 建议修复
private void prepareNvRamToStartVmCmd(KVMAgentCommands.StartVmCmd cmd, DiskAO nvRamSpec, KVMHostInventory host) {
VolumeVO vo = Q.New(VolumeVO.class)
.eq(VolumeVO_.uuid, nvRamSpec.getSourceUuid())
.find();
+ if (vo == null) {
+ logger.warn("NVRAM volume not found: " + nvRamSpec.getSourceUuid());
+ return;
+ }
VolumeInventory nvRamVolume = VolumeInventory.valueOf(vo);
VolumeTO volume = VolumeTO.valueOfWithOutExtension(nvRamVolume, host, null);
cmd.setNvRam(volume);
}🤖 Prompt for AI Agents
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java`
around lines 80 - 87, prepareNvRamToStartVmCmd currently calls Q.New(...).find()
and immediately passes the result to VolumeInventory.valueOf(vo), which will NPE
if the VolumeVO is null; update prepareNvRamToStartVmCmd to defensively check
the result of Q.New(VolumeVO.class).eq(VolumeVO_.uuid,
nvRamSpec.getSourceUuid()).find() (vo) before calling
VolumeInventory.valueOf(vo) and before cmd.setNvRam(...); if vo is null, either
log a clear warning including nvRamSpec.getSourceUuid() and return without
setting the NVRAM on the cmd or throw a clear exception, ensuring callers handle
the missing volume case rather than allowing a NullPointerException.
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java
Show resolved
Hide resolved
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap@@2
branch
from
b2fd064 to
bbad3f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java (1)
1291-1310:⚠️ Potential issue | 🟠 MajorNVRAM 预设 installPath 时会被强制为 QCOW2，可能导致 RAW 卷创建错误。
当前在 installPath 已设置时直接写死 QCOW2；若 NVRAM 上游已填 installPath，会覆盖应有的 RAW 格式。建议按类型统一计算格式，避免分支遗漏。✅ 建议修复
if (volume.getInstallPath() != null && !volume.getInstallPath().equals("")) { cmd.setInstallUrl(volume.getInstallPath()); - cmd.setFormat(ImageConstant.QCOW2_FORMAT_STRING); + cmd.setFormat(VolumeType.NVRAM.toString().equals(volume.getType()) + ? ImageConstant.RAW_FORMAT_STRING + : ImageConstant.QCOW2_FORMAT_STRING); } else { cmd.setFormat(ImageConstant.QCOW2_FORMAT_STRING); if (VolumeType.Root.toString().equals(volume.getType())) {
🤖 Fix all issues with AI agents
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java`:
- Around line 118-124: The current early-return when needRegisterNvRam ==
(nvRamVolumeUuid != null) calls completion.success() without verifying the
existing NVRAM volume is actually Ready; update the block in
KvmSecureBootExtensions (symbols: needRegisterNvRam, nvRamVolumeUuid, nvRamSpec,
completion.success) to, after setting nvRamSpec.setSourceUuid(nvRamVolumeUuid),
fetch the volume inventory/state for nvRamVolumeUuid and verify its status
equals Ready before calling completion.success(); if the status is not Ready,
either fail the completion with a clear error or enqueue/return a retry/async
check (consistent with surrounding retry/async patterns) so callers don’t
proceed with a non-Ready volume.
In
`@storage/src/main/java/org/zstack/storage/primary/PrimaryStoragePathMaker.java`:
- Around line 46-48: The NVRAM file extension is incorrect: update
PrimaryStoragePathMaker.makeNvRamVolumeInstallPath to use the .fd extension
instead of .xfs so it matches libvirt/UEFI expectations (and the comment in
KvmSecureBootExtensions). Locate the makeNvRamVolumeInstallPath method and
change the returned filename suffix from volUuid + ".xfs" to volUuid + ".fd",
keeping the existing path structure ("nvRam",
"acct-"+getAccountUuidOfResource(volUuid), "vol-"+volUuid).
🧹 Nitpick comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java (2)
182-235:NoRollbackFlow中实现了 rollback 方法，语义不一致。第 182 行使用了
NoRollbackFlow，但在第 221-235 行又重写了rollback()方法。NoRollbackFlow的语义是"无需回滚的流程"，在其中实现回滚逻辑会导致代码意图不清晰。建议改用普通的
Flow类：♻️ 建议修复
- chain.then(new NoRollbackFlow() { + chain.then(new Flow() { String __name__ = "create-nvram-volume"; `@Override` public void run(FlowTrigger trigger, Map data) { // ... existing implementation } `@Override` public void rollback(FlowRollback trigger, Map data) { deleteNvramVolumeIfExists(context.vmUuid, new Completion(trigger) { // ... existing implementation }); } })
165-168:preReleaseVmResource的错误处理策略与preInstantiateVmResource不一致。在
preInstantiateVmResource中（第 157-161 行），删除 NVRAM 失败时会记录警告但仍然调用completion.success()，允许流程继续。但在
preReleaseVmResource中，删除失败会直接导致整个释放操作失败。这可能导致 VM 销毁流程被阻塞。建议统一错误处理策略：
♻️ 建议修复
`@Override` public void preReleaseVmResource(VmInstanceSpec spec, Completion completion) { - deleteNvramVolumeIfExists(spec.getVmInventory().getUuid(), completion); + deleteNvramVolumeIfExists(spec.getVmInventory().getUuid(), new Completion(completion) { + `@Override` + public void success() { + completion.success(); + } + + `@Override` + public void fail(ErrorCode errorCode) { + logger.warn("failed to delete NVRAM during release but still continue: " + errorCode.getReadableDetails()); + completion.success(); + } + }); }
| if (needRegisterNvRam == (nvRamVolumeUuid != null)) { | ||
| if (nvRamVolumeUuid != null) { | ||
| nvRamSpec.setSourceUuid(nvRamVolumeUuid); | ||
| } | ||
| completion.success(); // TODO make sure status is Ready | ||
| return; | ||
| } |
There was a problem hiding this comment.
状态检查逻辑不完整。
第 122 行的 TODO 注释表明需要确保 NVRAM 卷的状态为 Ready，但当前实现在卷已存在时直接返回 success，没有验证卷的状态。如果卷存在但状态不是 Ready（如 Creating 或 Error），可能会导致后续 VM 启动失败。
建议添加状态检查或在后续迭代中解决此 TODO：
🔍 建议补充状态验证
- String nvRamVolumeUuid = Q.New(VolumeVO.class)
+ VolumeVO nvRamVolume = Q.New(VolumeVO.class)
.eq(VolumeVO_.vmInstanceUuid, spec.getVmInventory().getUuid())
.eq(VolumeVO_.type, VolumeType.NVRAM)
- .select(VolumeVO_.uuid)
- .findValue();
- if (needRegisterNvRam == (nvRamVolumeUuid != null)) {
- if (nvRamVolumeUuid != null) {
- nvRamSpec.setSourceUuid(nvRamVolumeUuid);
+ .find();
+ if (needRegisterNvRam == (nvRamVolume != null)) {
+ if (nvRamVolume != null) {
+ if (nvRamVolume.getStatus() != VolumeStatus.Ready) {
+ completion.fail(operr("NVRAM volume exists but status is not Ready: %s", nvRamVolume.getStatus()));
+ return;
+ }
+ nvRamSpec.setSourceUuid(nvRamVolume.getUuid());
}
- completion.success(); // TODO make sure status is Ready
+ completion.success();
return;
}🤖 Prompt for AI Agents
In `@plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java`
around lines 118 - 124, The current early-return when needRegisterNvRam ==
(nvRamVolumeUuid != null) calls completion.success() without verifying the
existing NVRAM volume is actually Ready; update the block in
KvmSecureBootExtensions (symbols: needRegisterNvRam, nvRamVolumeUuid, nvRamSpec,
completion.success) to, after setting nvRamSpec.setSourceUuid(nvRamVolumeUuid),
fetch the volume inventory/state for nvRamVolumeUuid and verify its status
equals Ready before calling completion.success(); if the status is not Ready,
either fail the completion with a clear error or enqueue/return a retry/async
check (consistent with surrounding retry/async patterns) so callers don’t
proceed with a non-Ready volume.
| public static String makeNvRamVolumeInstallPath(String volUuid) { | ||
| return PathUtil.join("nvRam", "acct-" + getAccountUuidOfResource(volUuid), "vol-" + volUuid, volUuid + ".xfs"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
libvirt NVRAM file extension .fd firmware
💡 Result:
In libvirt/QEMU UEFI guests, a .fd file is typically an edk2/OVMF “firmware volume” image used with pflash. It’s a container format used by edk2 builds, and in practice it’s most often just a raw flash image (not an ISO/ELF/etc.). (kraxel.org)
How libvirt uses these .fd files:
- Firmware code (read-only): points at something like
OVMF_CODE*.fdvia<loader ... type='pflash'>...OVMF_CODE.fd</loader>. This is the executable UEFI firmware. (libvirt.org) - NVRAM / UEFI variables (writable): points at a per-VM file (often under
/var/lib/libvirt/.../nvram/) created by copying a template likeOVMF_VARS.fd. This file stores persistent UEFI variables (boot entries/order, Secure Boot keys, etc.). (libvirt.org)
So when you see a libvirt NVRAM path ending in .fd (e.g. .../nvram/myvm.fd or guest_VARS.fd), it’s usually the VM’s UEFI variable-store flash image, not the firmware code itself. (libvirt.org)
Also note: libvirt can track NVRAM file format (eg raw vs qcow2) via format / templateFormat, and libvirt development explicitly notes that the file extension should match the format (historically .fd for the classic raw VARS/CODE split). (libvirt.org)
Citations:
- 1: https://www.kraxel.org/blog/2022/05/edk2-virt-quickstart/?utm_source=openai
- 2: https://www.libvirt.org/formatdomain?utm_source=openai
- 3: https://www.libvirt.org/formatdomain?utm_source=openai
- 4: https://libvirt.org/drvbhyve.html?utm_source=openai
- 5: https://www.libvirt.org/formatdomain?utm_source=openai
🏁 Script executed:
# Search for any NVRAM or related references in the codebase
rg -i "nvram|nvRam" -A 3 -B 3
# Look for any test files related to PrimaryStoragePathMaker
fd "PrimaryStoragePathMaker"
# Check if there are any comments about .xfs or .fd extensions
rg "\.xfs|\.fd" -A 2 -B 2Repository: MatheMatrix/zstack
Length of output: 32554
文件扩展名应为 .fd 而非 .xfs
根据 libvirt 文档，NVRAM 文件应使用 .fd 扩展名。这与您当前代码库中 KvmSecureBootExtensions.java 的注释相符：// NVRAM is raw type (*.fd) in libvirt 8.0.0。XFS 是一种文件系统类型，不适用于 UEFI NVRAM 固件文件。
修改扩展名以确保与 libvirt 的兼容性：
建议的修复
public static String makeNvRamVolumeInstallPath(String volUuid) {
- return PathUtil.join("nvRam", "acct-" + getAccountUuidOfResource(volUuid), "vol-" + volUuid, volUuid + ".xfs");
+ return PathUtil.join("nvRam", "acct-" + getAccountUuidOfResource(volUuid), "vol-" + volUuid, volUuid + ".fd");
}📝 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.
| public static String makeNvRamVolumeInstallPath(String volUuid) { | |
| return PathUtil.join("nvRam", "acct-" + getAccountUuidOfResource(volUuid), "vol-" + volUuid, volUuid + ".xfs"); | |
| } | |
| public static String makeNvRamVolumeInstallPath(String volUuid) { | |
| return PathUtil.join("nvRam", "acct-" + getAccountUuidOfResource(volUuid), "vol-" + volUuid, volUuid + ".fd"); | |
| } |
🤖 Prompt for AI Agents
In
`@storage/src/main/java/org/zstack/storage/primary/PrimaryStoragePathMaker.java`
around lines 46 - 48, The NVRAM file extension is incorrect: update
PrimaryStoragePathMaker.makeNvRamVolumeInstallPath to use the .fd extension
instead of .xfs so it matches libvirt/UEFI expectations (and the comment in
KvmSecureBootExtensions). Locate the makeNvRamVolumeInstallPath method and
change the returned filename suffix from volUuid + ".xfs" to volUuid + ".fd",
keeping the existing path structure ("nvRam",
"acct-"+getAccountUuidOfResource(volUuid), "vol-"+volUuid).
If TPM or secure boot is enabled, MN will prepare nvram volume and instantiate volume to host Resolves: ZSV-11310 Related: ZSPHER-1 Change-Id: I787672786e6873696c6273647778737364767563
Libvirt 8.0.0 only support raw type NVRAW (ext is '.fd'), Resolves: ZSV-11310 Related: ZSPHER-1 Change-Id: I6c65797078616c706d6c716c796b63737a716375
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-ldap@@2
branch
from
bbad3f9 to
e5bf77f
Compare
|
Comment from wenhao.zhang: AI review 说的 .xfs 结尾问题，其实方案是 lv 构建文件系统，然后把 nvram 的 fd 放到文件系统里面，是之前做过的方案 |
|
Comment from ye.zou: Code Review — NVRAM Volume 补丁整体方案合理，NVRAM Volume 的抽象层级（VolumeType 枚举 + 现有 Volume 创建/实例化流程复用）是好的设计选择。以下是具体 findings： 1. [MEDIUM] Volume state 检查 TODO —
|
|
Comment from wenhao.zhang: secure boot 全局设置改为默认关闭，这样不用影响升级后 UEFI VM 的启动 |
|
Comment from ye.zou: Review Tracker: ZSV-11310Rounds: R1=conditional
Progress: ░░░░░░░░░░ 0/6 resolved (0%) Verdict: CONDITIONAL — 架构设计合理，存储后端覆盖完整。但 |
|
Comment from ye.zou: Code Review: NVRAM Volume Type IntroductionTicket: ZSV-11310 — 支持 TPM 和 Secure Boot Findings1. [Major:Bug] VmExpungeNvRamVolumeFlow:60 — 日志打错 UUID logger.warn(String.format("failed to expunge the NVRAM volume[uuid:%s] ...",
spec.getVmInventory().getRootVolumeUuid(), // ← BUG: 应该是 vol.getUuid()日志说 "NVRAM volume" 但打印的是 Root volume UUID。需要改成 2. [Major:Convention] VolumeType.NVRAM 枚举命名风格不一致 所有其他 VolumeType 都是 PascalCase: 3. [Major:Completeness] 缺少 SharedBlock (sblk) 存储后端支持 Diff 中 Local/NFS/SMP 都加了 NVRAM 路径，但没有看到 sblk 的改动。如果是有意推迟，请加 TODO；如果遗漏了，sblk 上开启 TPM/SecureBoot 的 VM 会在 volume instantiate 时失败。 4. [Minor] LocalStorageKvmBackend:1301 — format 来源不一致 createEmptyVolume 路径用 5. [Minor] PrimaryStoragePathMaker:46 — 注释误导
6. [Nit] MR title prefix 应为 这是新功能 (NVRAM volume type)，不是 bugfix。 亮点
🤖 Review by Claude Code |
|
Comment from ye.zou: Review Tracker: ZSV-11310Rounds: R1=conditional, R2=conditional
Progress: ████░░░░░░ 4/10 resolved (40%) — 3 major open 🤖 Review Tracker by Claude Code |
|
Comment from ye.zou: Review Tracker: ZSV-11310 — Round 3Verdict: CONDITIONAL — 3 major findings require attention before merge.
Progress: ██████░░░░ 13/21 resolved (62%) — good improvement from R1/R2 🤖 Automated review by Claude Code |
|
Comment from ye.zou: Review 详细说明（中文）— Finding #1 ~ #3#1 [major] NFS 后端缺少 NvRam 卷的 RAW 格式设置
// LocalStorageKvmBackend.java — 正确做法
cmd.setFormat(ImageConstant.RAW_FORMAT_STRING); // ✅ 显式设置 RAW而 NFS 只做了： // NfsPrimaryStorageKVMBackend.java — 当前代码
cmd.setInstallUrl(NfsPrimaryStorageKvmHelper.makeNvRamVolumeInstallUrl(pinv, volume.getUuid()));
// ❌ 缺少 cmd.setFormat(...)如果 KVM agent 创建空卷时默认使用 qcow2 格式，NFS 上的 NvRam 卷会被创建成错误的格式，导致 libvirt 无法正确加载 NVRAM 文件。 #2 [major] SMP 后端同样缺少 NvRam 卷的 RAW 格式设置
// SMP KvmBackend.java — 当前代码
cmd.installPath = makeNvRamVolumeInstallUrl(volume.getUuid());
// ❌ 缺少 format 设置建议 NFS 和 SMP 都参照 LocalStorage，在 NvRam 分支中显式设置 #3 [major] 缺少 Ceph 等分布式存储后端的 NvRam 支持 当前 MR 为 LocalStorage、NFS、SMP 三种存储后端添加了 NvRam 卷的路径处理，但没有覆盖 Ceph（及其他分布式存储）。如果用户在 Ceph 主存储上的虚拟机启用了 TPM 或 SecureBoot：
建议二选一：
🤖 Automated review by Claude Code |
|
Comment from ye.zou: Review 详细说明（中文）— Finding #4 ~ #5, #7#4 [minor] 删除 NvRam 卷失败时错误被吞掉
public void fail(ErrorCode errorCode) {
logger.warn("failed to delete NvRam but still continue: " + errorCode.getReadableDetails());
completion.success(); // ⚠️ 吞掉了错误
}场景：当用户关闭 TPM 和 SecureBoot 后启动虚拟机， 建议：可以接受当前行为（不阻塞 VM 启动），但建议增加一个 GC 机制或定期清理任务来处理这类孤儿卷。 #5 [minor] VolumeBase 注释引用了不存在的 TpmState 类型
// include: data volume, NvRam, TpmState当前 #7 [nit] persistence.xml 新增 TpmHostRefVO 和 TpmVO <class>org.zstack.header.tpm.entity.TpmHostRefVO</class>
<class>org.zstack.header.tpm.entity.TpmVO</class>这两个实体类之前是否已经存在但未注册到 persistence.xml？如果是的话，这其实是在修复一个潜在的 JPA 注册遗漏 bug，建议在 commit message 中说明。 🤖 Automated review by Claude Code |
4 participants
If TPM or secure boot is enabled, MN will prepare nvram volume
and instantiate volume to host
Resolves: ZSV-11310
Related: ZSPHER-1
Change-Id: I787672786e6873696c6273647778737364767563
sync from gitlab !9176