Skip to content

<fix>[compute]: introduce NVRAM type volume#3347

Open
zstack-robot-2 wants to merge 2 commits intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
sync/wenhao.zhang/zsv-ldap@@2
Open

<fix>[compute]: introduce NVRAM type volume#3347
zstack-robot-2 wants to merge 2 commits intofeature-zsv-5.0.0-vm-support-vtpm-and-secucebootfrom
sync/wenhao.zhang/zsv-ldap@@2

Conversation

@zstack-robot-2
Copy link
Collaborator

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

@coderabbitai
Copy link

coderabbitai bot commented Feb 13, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

增加对 NVRAM 的支持:新增常量与卷类型、在 VmInstanceSpec 中加入 nvRamSpec、在 VmTpmExtensions 构建规范时创建 nvram 规格;VmTpmManager 增加注册判断;KVM 安全引导、本地存储与卷服务新增 NVRAM 的创建、实例化、路径与映射逻辑。

变更

Cohort / File(s) Summary
常量与卷类型
header/src/main/java/org/zstack/header/vm/VmInstanceConstant.java, header/src/main/java/org/zstack/header/volume/VolumeType.java
新增常量 NVRAM_DEFAULT_SIZE(1 MB)并新增 VolumeType.NVRAM 枚举常量。
VM 规范
header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java
VmInstanceSpec 中新增私有字段 DiskAO nvRamSpec 及其 getNvRamSpec() / setNvRamSpec() 方法。
TPM 扩展与构建钩子
compute/src/main/java/org/zstack/compute/vm/devices/VmTpmExtensions.java, compute/src/main/java/org/zstack/compute/vm/devices/VmTpmManager.java
VmTpmExtensions 实现 BuildVmSpecExtensionPoint 并新增 afterBuildVmSpec(VmInstanceSpec) 来按需创建并附加 nvram DiskAO(使用 NVRAM_DEFAULT_SIZE、命名为 nvram-of-VM-<uuid>)。VmTpmManager 新增 needRegisterNvram(String vmUuid) 与静态 isUefiBootMode,基于 TPM 表、VM 引导模式和 ENABLE_UEFI_SECURE_BOOT 配置判断。
KVM agent 与 Secure Boot 扩展
plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java, plugin/kvm/src/main/java/org/zstack/kvm/efi/KvmSecureBootExtensions.java
StartVmCmd 增加 nvRamVolumeTO)字段及访问器。KvmSecureBootExtensions 新增 PreVmInstantiateResourceExtensionPoint 实现与 preBeforeInstantiateVmResourcepreInstantiateVmResourcepreReleaseVmResource 等方法,负责 NVRAM 的查找/创建/实例化/删除,并在启动命令中映射 nvRam。
本地存储与路径生成
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java, storage/src/main/java/org/zstack/storage/primary/PrimaryStoragePathMaker.java
新增 makeNvRamVolumeInstallUrl(String)makeNvRamVolumeInstallPath(String);在创建空卷流程中为 NVRAM 设置合适的 install URL 与格式(RAW),并在响应中传播格式信息。
卷实例化逻辑
storage/src/main/java/org/zstack/storage/volume/VolumeBase.java
将私有方法 instantiateDataVolume 重命名为更通用的 instantiateOtherVolume,以同时处理 Data、NVRAM 与 TpmState 等类型。

时序图

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
Loading

代码审查工作量评估

🎯 4 (复杂) | ⏱️ ~45 分钟

诗歌

🐰 我在代码土堆里挖洞,藏下一片小小的 NVRAM,
启动时轻轻推开,安全与启动手牵手,
卷声细碎,路径成行,UUID 如胡萝卜排队在岗,
小小磁室,暖暖静好,云端醒来有新光。

🚥 Pre-merge checks | ✅ 2 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 1.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Merge Conflict Detection ⚠️ Warning ⚠️ Unable to check for merge conflicts: Invalid branch name format
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed PR标题清晰简洁,准确概括了本次变更的核心内容:引入NVRAM类型的存储卷。
Description check ✅ Passed PR描述与变更相关联,说明了NVRAM卷的目的(当启用TPM或安全启动时)及其作用。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sync/wenhao.zhang/zsv-ldap@@2
⚔️ Resolve merge conflicts (beta)
  • Auto-commit resolved conflicts to branch sync/wenhao.zhang/zsv-ldap@@2
  • Create stacked PR with resolved conflicts
  • Post resolved changes as copyable diffs in a comment

No actionable comments were generated in the recent review. 🎉


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 风格(RootDataMemoryCache),而 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);

@MatheMatrix MatheMatrix force-pushed the sync/wenhao.zhang/zsv-ldap@@2 branch from 3eaf131 to 9fe3dbc Compare February 13, 2026 10:26
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 来跟踪此任务?

Comment on lines 41 to 47
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);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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).

Comment on lines +80 to +87
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);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

@MatheMatrix MatheMatrix force-pushed the sync/wenhao.zhang/zsv-ldap@@2 branch from b2fd064 to bbad3f9 Compare February 14, 2026 02:08
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 | 🟠 Major

NVRAM 预设 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();
+            }
+        });
     }

Comment on lines +118 to +124
if (needRegisterNvRam == (nvRamVolumeUuid != null)) {
if (nvRamVolumeUuid != null) {
nvRamSpec.setSourceUuid(nvRamVolumeUuid);
}
completion.success(); // TODO make sure status is Ready
return;
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

状态检查逻辑不完整。

第 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.

Comment on lines +46 to +48
public static String makeNvRamVolumeInstallPath(String volUuid) {
return PathUtil.join("nvRam", "acct-" + getAccountUuidOfResource(volUuid), "vol-" + volUuid, volUuid + ".xfs");
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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*.fd via <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 like OVMF_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:


🏁 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 2

Repository: 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.

Suggested change
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).

Zhang Wenhao added 2 commits February 14, 2026 10:40
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 MatheMatrix force-pushed the sync/wenhao.zhang/zsv-ldap@@2 branch from bbad3f9 to e5bf77f Compare February 14, 2026 02:42
@MatheMatrix
Copy link
Owner

Comment from wenhao.zhang:

AI review 说的 .xfs 结尾问题,其实方案是 lv 构建文件系统,然后把 nvram 的 fd 放到文件系统里面,是之前做过的方案

@zstack-robot-1
Copy link
Collaborator

Comment from ye.zou:

Code Review — NVRAM Volume 补丁

整体方案合理,NVRAM Volume 的抽象层级(VolumeType 枚举 + 现有 Volume 创建/实例化流程复用)是好的设计选择。以下是具体 findings:


1. [MEDIUM] Volume state 检查 TODO — KvmSecureBootExtensions.java

当 NVRAM 卷已存在时,直接 completion.success() 没有验证卷状态。如果卷处于 Creating / Error 状态,后续 prepareNvRamToStartVmCmd 拿到 installPath 为 null 的 VolumeVO 会导致 agent 端异常。

建议:查询时加 status 过滤:

String nvRamVolumeUuid = Q.New(VolumeVO.class)
    .eq(VolumeVO_.vmInstanceUuid, vmUuid)
    .eq(VolumeVO_.type, VolumeType.NVRAM)
    .eq(VolumeVO_.status, VolumeStatus.Ready)
    .select(VolumeVO_.uuid)
    .findValue();

这样非 Ready 的卷视为不存在,走重建逻辑。


2. [MEDIUM] preReleaseVmResource 生命周期语义确认

@Override
public void preReleaseVmResource(VmInstanceSpec spec, Completion completion) {
    deleteNvramVolumeIfExists(spec.getVmInventory().getUuid(), completion);
}

请确认preReleaseVmResource 是每次 VM stop 都调用,还是仅 destroy 时调用?

  • 如果每次 stop 调用 → NVRAM 内容(UEFI 变量、SecureBoot key enrollment)每次重启都丢失
  • 如果仅 destroy 调用 → OK

如果 stop 也调用,建议判断操作类型,stop 时保留 NVRAM 卷。


3. [MEDIUM] LocalStorageKvmBackend — 已有 installPath 的 NVRAM 卷格式回退

if (volume.getInstallPath() != null && !volume.getInstallPath().equals("")) {
    cmd.setInstallUrl(volume.getInstallPath());
    cmd.setFormat(ImageConstant.QCOW2_FORMAT_STRING);  // ← 硬编码 qcow2
}

NVRAM 卷 format 应为 RAW。如果 NVRAM 卷重新实例化时已有 installPath,会走此分支,format 被错设为 QCOW2。

建议

cmd.setFormat(VolumeType.NVRAM.toString().equals(volume.getType())
    ? ImageConstant.RAW_FORMAT_STRING
    : ImageConstant.QCOW2_FORMAT_STRING);

4. [LOW] prepareNvRamToStartVmCmd 缺少防御性空检查

Q.New(VolumeVO.class).find() 返回 null 时 VolumeInventory.valueOf(vo) 会 NPE。建议加 DebugUtils.Assert


5. [LOW] 条件可读性

needRegisterNvRam == (nvRamVolumeUuid != null) 逻辑正确但不直观,建议加注释说明 "both aligned" 语义。


6. [INFO] 仅 LocalStorage 覆盖了 NVRAM 路径

NFS / Ceph / SharedBlock 后端未添加 NVRAM 特定路径,假设后续 MR 补充。


总结:核心 blocking 项是 Finding #1(volume state 检查),其余可 follow-up。建议补一个 VM reboot 场景测试覆盖 Finding #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants