<feature>[header]: Add TPM key backup for snapshot revert and rollback#3916
Closed
zstack-robot-1 wants to merge 1 commit intozsv_5.1.0from
Closed
<feature>[header]: Add TPM key backup for snapshot revert and rollback#3916zstack-robot-1 wants to merge 1 commit intozsv_5.1.0from
zstack-robot-1 wants to merge 1 commit intozsv_5.1.0from
Conversation
Walkthrough此 PR 为 TPM 加密密钥恢复添加备份实体与消息，扩展后端接口与 KVM 管理器以支持备份-恢复-清理流程，并在快照组恢复工作流中记录与回滚/删除备份 UUID。 变更说明TPM密钥备份与恢复流
Sequence Diagram(s)sequenceDiagram
participant Caller
participant KvmTpmManager
participant DatabaseFacade
participant TpmBackend
Caller->>KvmTpmManager: RestoreTpmEncryptionKeyMsg(backupCurrentKey=true)
KvmTpmManager->>DatabaseFacade: persist TpmKeyBackupVO (uuid)
KvmTpmManager->>TpmBackend: backup current key -> store encrypted material under uuid
TpmBackend-->>KvmTpmManager: backup OK
KvmTpmManager->>TpmBackend: restore encrypted resource key from src
TpmBackend-->>KvmTpmManager: restore OK
KvmTpmManager-->>Caller: RestoreTpmEncryptionKeyReply(tpmKeyBackupUuid)
Note over KvmTpmManager,DatabaseFacade: on Exception -> clean encrypted backup material, delete TpmKeyBackupVO
🎯 3 (Moderate) | ⏱️ ~25 分钟
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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 `@plugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.java`:
- Around line 698-706: In KvmTpmManager's catch(Throwable t) block, avoid
passing a possibly null t.getMessage() into reply.setError(operr(...)); instead
build a non-null error string (e.g. use t.getMessage() != null ? t.getMessage()
: t.toString() or include t.getClass().getName()) and pass that to operr,
keeping the existing cleanup calls to
tpmKeyBackend.cleanTpmKeyBackupEncryptedResourceKey(tpmKeyBackupUuid) and
databaseFacade.removeByPrimaryKey(tpmKeyBackupUuid, TpmKeyBackupVO.class) and
then reply.setTpmKeyBackupUuid(null); reply.setError(operr(...)); bus.reply(msg,
reply).
In
`@storage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupBase.java`:
- Around line 595-614: The delete-tpm-key-backup flow in VolumeSnapshotGroupBase
currently treats a DeleteTpmKeyBackupMsg failure as fatal (calls trigger.fail),
but TPM key backup cleanup is best-effort and should not abort the overall
revert; change the CloudBusCallBack in the "delete-tpm-key-backup" Flow (the
bus.send callback handling DeleteTpmKeyBackupMsg) so that on r.isSuccess() it
clears context.tpmKeyBackupUuid and calls trigger.next(), and on failure it logs
a warning (including r.getError()) instead of calling trigger.fail, then still
calls trigger.next() so the main revert continues.
🪄 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: 69c493af-229e-40ed-98ec-fd7f2e015ac9
⛔ Files ignored due to path filters (1)
conf/persistence.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
compute/src/main/java/org/zstack/compute/vm/devices/DummyTpmEncryptedResourceKeyBackend.javacompute/src/main/java/org/zstack/compute/vm/devices/TpmEncryptedResourceKeyBackend.javaheader/src/main/java/org/zstack/header/tpm/entity/TpmKeyBackupVO.javaheader/src/main/java/org/zstack/header/tpm/entity/TpmKeyBackupVO_.javaheader/src/main/java/org/zstack/header/tpm/message/DeleteTpmKeyBackupMsg.javaheader/src/main/java/org/zstack/header/tpm/message/DeleteTpmKeyBackupReply.javaheader/src/main/java/org/zstack/header/tpm/message/RestoreTpmEncryptionKeyMsg.javaheader/src/main/java/org/zstack/header/tpm/message/RestoreTpmEncryptionKeyReply.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.javastorage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupBase.java
Comment on lines
+595
to
+614
| .then(Flow.of("delete-tpm-key-backup") | ||
| .runIf(data -> context.tpmKeyBackupUuid != null) | ||
| .handle(trigger -> { | ||
| DeleteTpmKeyBackupMsg delMsg = new DeleteTpmKeyBackupMsg(); | ||
| delMsg.setTpmUuid(context.tpmUuid); | ||
| delMsg.setTpmKeyBackupUuid(context.tpmKeyBackupUuid); | ||
| bus.makeTargetServiceIdByResourceUuid(delMsg, SERVICE_ID, context.tpmUuid); | ||
| bus.send(delMsg, new CloudBusCallBack(trigger) { | ||
| @Override | ||
| public void run(MessageReply r) { | ||
| if (r.isSuccess()) { | ||
| context.tpmKeyBackupUuid = null; | ||
| trigger.next(); | ||
| } else { | ||
| trigger.fail(r.getError()); | ||
| } | ||
| } | ||
| }); | ||
| }) | ||
| .build()) |
There was a problem hiding this comment.
备份清理失败不应导致整个 revert 操作失败。
此时主要的 revert 操作（卷恢复、TPM 密钥恢复）已经成功完成。TpmKeyBackupVO 是临时性的辅助记录，其清理失败不应该使整个流程失败。建议改为记录警告日志并继续执行。
🔧 建议的修复方案
.then(Flow.of("delete-tpm-key-backup")
.runIf(data -> context.tpmKeyBackupUuid != null)
.handle(trigger -> {
DeleteTpmKeyBackupMsg delMsg = new DeleteTpmKeyBackupMsg();
delMsg.setTpmUuid(context.tpmUuid);
delMsg.setTpmKeyBackupUuid(context.tpmKeyBackupUuid);
bus.makeTargetServiceIdByResourceUuid(delMsg, SERVICE_ID, context.tpmUuid);
bus.send(delMsg, new CloudBusCallBack(trigger) {
`@Override`
public void run(MessageReply r) {
- if (r.isSuccess()) {
- context.tpmKeyBackupUuid = null;
- trigger.next();
- } else {
- trigger.fail(r.getError());
+ if (!r.isSuccess()) {
+ logger.warn(String.format(
+ "failed to delete TpmKeyBackupVO[uuid:%s] after successful revert, but continuing: %s",
+ context.tpmKeyBackupUuid, r.getError().getDetails()));
}
+ context.tpmKeyBackupUuid = null;
+ trigger.next();
}
});
})
.build())🤖 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/snapshot/group/VolumeSnapshotGroupBase.java`
around lines 595 - 614, The delete-tpm-key-backup flow in
VolumeSnapshotGroupBase currently treats a DeleteTpmKeyBackupMsg failure as
fatal (calls trigger.fail), but TPM key backup cleanup is best-effort and should
not abort the overall revert; change the CloudBusCallBack in the
"delete-tpm-key-backup" Flow (the bus.send callback handling
DeleteTpmKeyBackupMsg) so that on r.isSuccess() it clears
context.tpmKeyBackupUuid and calls trigger.next(), and on failure it logs a
warning (including r.getError()) instead of calling trigger.fail, then still
calls trigger.next() so the main revert continues.
Introduce internal TpmKeyBackupVO (no owner) to hold the VM TPM encryption key during snapshot-group revert Add DeleteTpmKeyBackupMsg / DeleteTpmKeyBackupReply to remove the backup VO and its key ref. Resolves: ZSV-12208 Related: ZSV-11310 DBImpact Change-Id: I7a7967637667726a727666676a6162787277616e
MatheMatrix
force-pushed
the
sync/wenhao.zhang/zsv-8@@2
branch
from
77c5bb0 to
121c535
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
conf/db/zsv/V5.1.0__schema.sql (2)
2-2: 💤 Low value
UNIQUE约束冗余。
uuid列已定义为PRIMARY KEY，主键本身就保证了唯一性，无需额外的UNIQUE约束。♻️ 建议的修改
- `uuid` char(32) NOT NULL UNIQUE, + `uuid` char(32) NOT NULL,🤖 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 `@conf/db/zsv/V5.1.0__schema.sql` at line 2, 在表定义中移除多余的 UNIQUE 约束：在包含列定义 `uuid char(32) NOT NULL UNIQUE` 的 SQL 片段中删除 `UNIQUE`，只保留 `uuid char(32) NOT NULL`，因为 `uuid` 已在表的 `PRIMARY KEY` 上被声明，主键已保证唯一性；请更新相关建表语句以移除重复约束并保持主键声明不变。
4-4: 💤 Low value
createDate的默认值不符合最佳实践。
createDate使用硬编码的默认值'1999-12-31 23:59:59'，虽然 Java 代码会显式设置该字段，但建议使用DEFAULT CURRENT_TIMESTAMP以保持一致性并作为更安全的回退值。♻️ 建议的修改
- `createDate` timestamp NOT NULL DEFAULT '1999-12-31 23:59:59', + `createDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP,🤖 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 `@conf/db/zsv/V5.1.0__schema.sql` at line 4, 当前 schema 中列 createDate 使用硬编码默认值 '1999-12-31 23:59:59'，请将其改为使用数据库时间戳回退值：把 createDate 的列定义从 DEFAULT '1999-12-31 23:59:59' 修改为 DEFAULT CURRENT_TIMESTAMP，以确保在未由 Java 明确设置时使用当前时间作为默认值并保持一致性（查找并修改 V5.1.0 migration 中定义 createDate 的列）。
🤖 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 `@conf/db/zsv/V5.1.0__schema.sql`:
- Line 2: 在表定义中移除多余的 UNIQUE 约束：在包含列定义 `uuid char(32) NOT NULL UNIQUE` 的 SQL
片段中删除 `UNIQUE`，只保留 `uuid char(32) NOT NULL`，因为 `uuid` 已在表的 `PRIMARY KEY`
上被声明，主键已保证唯一性；请更新相关建表语句以移除重复约束并保持主键声明不变。
- Line 4: 当前 schema 中列 createDate 使用硬编码默认值 '1999-12-31
23:59:59'，请将其改为使用数据库时间戳回退值：把 createDate 的列定义从 DEFAULT '1999-12-31 23:59:59' 修改为
DEFAULT CURRENT_TIMESTAMP，以确保在未由 Java 明确设置时使用当前时间作为默认值并保持一致性（查找并修改 V5.1.0
migration 中定义 createDate 的列）。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f4a5c4e-e178-433e-af8a-a43aae2deb8e
⛔ Files ignored due to path filters (1)
conf/persistence.xmlis excluded by!**/*.xml
📒 Files selected for processing (11)
compute/src/main/java/org/zstack/compute/vm/devices/DummyTpmEncryptedResourceKeyBackend.javacompute/src/main/java/org/zstack/compute/vm/devices/TpmEncryptedResourceKeyBackend.javaconf/db/zsv/V5.1.0__schema.sqlheader/src/main/java/org/zstack/header/tpm/entity/TpmKeyBackupVO.javaheader/src/main/java/org/zstack/header/tpm/entity/TpmKeyBackupVO_.javaheader/src/main/java/org/zstack/header/tpm/message/DeleteTpmKeyBackupMsg.javaheader/src/main/java/org/zstack/header/tpm/message/DeleteTpmKeyBackupReply.javaheader/src/main/java/org/zstack/header/tpm/message/RestoreTpmEncryptionKeyMsg.javaheader/src/main/java/org/zstack/header/tpm/message/RestoreTpmEncryptionKeyReply.javaplugin/kvm/src/main/java/org/zstack/kvm/tpm/KvmTpmManager.javastorage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupBase.java
✅ Files skipped from review due to trivial changes (4)
- header/src/main/java/org/zstack/header/tpm/message/RestoreTpmEncryptionKeyMsg.java
- header/src/main/java/org/zstack/header/tpm/entity/TpmKeyBackupVO_.java
- header/src/main/java/org/zstack/header/tpm/message/DeleteTpmKeyBackupReply.java
- header/src/main/java/org/zstack/header/tpm/entity/TpmKeyBackupVO.java
🚧 Files skipped from review as they are similar to previous changes (5)
- compute/src/main/java/org/zstack/compute/vm/devices/TpmEncryptedResourceKeyBackend.java
- header/src/main/java/org/zstack/header/tpm/message/DeleteTpmKeyBackupMsg.java
- compute/src/main/java/org/zstack/compute/vm/devices/DummyTpmEncryptedResourceKeyBackend.java
- header/src/main/java/org/zstack/header/tpm/message/RestoreTpmEncryptionKeyReply.java
- storage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupBase.java
Owner
|
Comment from ye.zou: Code Review: ZSV-12208 — 【5.1.0】物理机详情页 CPU 信息增加显示 Socket 数量变更概览
核心变更1.
|
| 检查项 | 状态 | 说明 |
|---|---|---|
| 功能完整性 | ✅ | 前后端字段对齐，数据流完整 |
| 兼容性 | ✅ | 仅新增字段，不影响现有逻辑 |
| 国际化 | ✅ | 中英文文案已补充 |
| 代码风格 | ✅ | 符合项目规范 |
| 目标分支 | 确认合并到 zsv_5.1.0，非 master，请确认是否符合预期 |
建议
- 建议确认
socketNum在旧版本 Host 数据中的默认值处理（是否为 NULL 或 0） - 若目标分支确认是
zsv_5.1.0，可正常合并
结论: 代码逻辑清晰，变更范围合理，Approve（目标分支确认后）
Collaborator
Author
|
Comment from ye.zou: Code Review: ZSV-12208 — 【TPM】恢复内存快照时，恢复 TPM 的步骤没有 rollback变更概览
核心变更1.
|
| 检查项 | 状态 | 说明 |
|---|---|---|
| 功能完整性 | ✅ | 完整实现了 TPM key 的备份、恢复、rollback 和清理流程 |
| 数据一致性 | ✅ | 异常时清理已创建的 TpmKeyBackupVO，避免脏数据 |
| 回滚逻辑 | ✅ | rollback 中先恢复 key 再删除 backup，顺序正确 |
| 清理逻辑 | ✅ | 成功后在 delete-tpm-key-backup flow 中清理 backup |
| 兼容性 | RestoreTpmEncryptionKeyMsg 默认 backupCurrentKey = true，可能影响其他调用方，需确认 |
|
| 目标分支 | 目标分支为 zsv_5.1.0，请确认是否符合预期 |
建议
- 建议确认
RestoreTpmEncryptionKeyMsg.backupCurrentKey默认值设为true是否会影响其他现有调用场景（如非快照恢复的 TPM key 恢复） VolumeSnapshotGroupBase中的 TODO 注释// TODO: It should has rollback已被实现，建议移除
结论
代码逻辑清晰，rollback 机制完整，数据清理到位。确认上述建议后可 Approve。
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
2 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.
Introduce internal TpmKeyBackupVO (no owner) to hold
the VM TPM encryption key during snapshot-group revert
Add DeleteTpmKeyBackupMsg / DeleteTpmKeyBackupReply
to remove the backup VO and its key ref.
Resolves: ZSV-12208
Related: ZSV-11310
Change-Id: I7a7967637667726a727666676a6162787277616e
sync from gitlab !9802