<fix>[kvm]: user vm mount model#3599
Open
MatheMatrix wants to merge 1 commit intofeature-5.5.12-dgpufrom
Open
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (10)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough新增 VM 模型挂载持久化表、KVM 路由常量、测试库 API 封装方法及 12 个错误码常量，涉及数据库模式、KVM 常量、测试库与错误码声明的基础设施改动（均为新增内容，无现有逻辑删除）。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 庆祝诗
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.1)utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.javaComment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java (2)
5189-5214: 建议为新增 DTO 字段补充@GrayVersion，让灰度升级行为更可控。当前新增命令/响应字段未标注版本信息。建议按本文件既有模式补齐，避免升级窗口期出现字段下发不一致。
♻️ 参考改法（示例）
public static class KvmAttachVirtiofsCmd extends AgentCommand { + `@GrayVersion`(value = "5.5.12") public String vmInstanceUuid; + `@GrayVersion`(value = "5.5.12") public String tag; + `@GrayVersion`(value = "5.5.12") public String sourcePath; + `@GrayVersion`(value = "5.5.12") public String mountPath; }As per coding guidelines “向后兼容原则：之前的代码产生的行为不要直接去改动…应当做好兼容/开关控制”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java` around lines 5189 - 5214, Add `@GrayVersion` annotations to the newly introduced DTOs/fields so gray/rolling upgrades remain controlled; specifically annotate the classes and/or their fields in KvmAttachVirtiofsCmd (vmInstanceUuid, tag, sourcePath, mountPath), KvmAttachVirtiofsRsp, KvmDetachVirtiofsCmd (vmInstanceUuid, tag), KvmDetachVirtiofsRsp, MountModelCenterCmd (modelCenterUuid, zdfsUrl) and MountModelCenterRsp (mountPoint) following the existing `@GrayVersion` placement pattern used elsewhere in this file (match annotation placement and version values consistent with neighboring DTOs) to preserve backward compatibility during rollout.
5189-5210: 建议对新增命令参数做 trim 归一化，避免因空白字符导致挂载参数失配。这些字段来自上游消息输入，当前直接暴露为
public且无归一化处理，容易把复制粘贴带来的空格/换行带入uuid/url/path/tag。♻️ 参考改法（示例）
public static class MountModelCenterCmd extends AgentCommand { - public String modelCenterUuid; - public String zdfsUrl; + private String modelCenterUuid; + private String zdfsUrl; + + public String getModelCenterUuid() { + return modelCenterUuid; + } + + public void setModelCenterUuid(String modelCenterUuid) { + this.modelCenterUuid = modelCenterUuid == null ? null : modelCenterUuid.trim(); + } + + public String getZdfsUrl() { + return zdfsUrl; + } + + public void setZdfsUrl(String zdfsUrl) { + this.zdfsUrl = zdfsUrl == null ? null : zdfsUrl.trim(); + } }As per coding guidelines “注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java` around lines 5189 - 5210, The new AgentCommand classes expose raw public string fields that may contain leading/trailing whitespace from upstream messages; update KvmAttachVirtiofsCmd (vmInstanceUuid, tag, sourcePath, mountPath), KvmDetachVirtiofsCmd (vmInstanceUuid, tag) and MountModelCenterCmd (modelCenterUuid, zdfsUrl) to normalize inputs by trimming whitespace: make the string fields private and add setters that call .trim() (or add a normalize/validate() method that trims all fields and is invoked after deserialization) so all UUID/URL/path/tag values are canonicalized before use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java`:
- Around line 5189-5214: Add `@GrayVersion` annotations to the newly introduced
DTOs/fields so gray/rolling upgrades remain controlled; specifically annotate
the classes and/or their fields in KvmAttachVirtiofsCmd (vmInstanceUuid, tag,
sourcePath, mountPath), KvmAttachVirtiofsRsp, KvmDetachVirtiofsCmd
(vmInstanceUuid, tag), KvmDetachVirtiofsRsp, MountModelCenterCmd
(modelCenterUuid, zdfsUrl) and MountModelCenterRsp (mountPoint) following the
existing `@GrayVersion` placement pattern used elsewhere in this file (match
annotation placement and version values consistent with neighboring DTOs) to
preserve backward compatibility during rollout.
- Around line 5189-5210: The new AgentCommand classes expose raw public string
fields that may contain leading/trailing whitespace from upstream messages;
update KvmAttachVirtiofsCmd (vmInstanceUuid, tag, sourcePath, mountPath),
KvmDetachVirtiofsCmd (vmInstanceUuid, tag) and MountModelCenterCmd
(modelCenterUuid, zdfsUrl) to normalize inputs by trimming whitespace: make the
string fields private and add setters that call .trim() (or add a
normalize/validate() method that trims all fields and is invoked after
deserialization) so all UUID/URL/path/tag values are canonicalized before use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 1df8207b-0207-42b1-9154-53fa2280c06f
⛔ Files ignored due to path filters (10)
conf/i18n/globalErrorCodeMapping/global-error-zh_CN.jsonis excluded by!**/*.jsonsdk/src/main/java/SourceClassMap.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/MountModelToVmInstanceAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/MountModelToVmInstanceResult.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/QueryVmModelMountAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/QueryVmModelMountResult.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/UnmountModelFromVmInstanceAction.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/UnmountModelFromVmInstanceResult.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/VmModelMountInventory.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/VmModelMountStatus.javais excluded by!sdk/**
📒 Files selected for processing (5)
conf/db/upgrade/V5.5.12__schema.sqlplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMConstant.javatestlib/src/main/java/org/zstack/testlib/ApiHelper.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
Collaborator
|
Comment from ye.zou: Code Review: ZSTAC-83157 — 用户VM挂载模型 (zstack 部分)Verdict: 🚫 BLOCK — 完整 review 见 premium !13327 评论，此处列本 MR 相关问题。 本 MR 问题N1. MR title 写 M1. KVM 命令命名冗余前缀 public static class KvmAttachVirtiofsCmd extends AgentCommand { ... }
public static class KvmDetachVirtiofsCmd extends AgentCommand { ... }
其余部分没问题：
完整 review 含 4 Critical + 5 Major 见 premium !13327 |
Collaborator
|
Comment from ye.zou: 补充 Review：命名规范M2. public static class MountModelCenterCmd extends AgentCommand {
public String modelCenterUuid;
public String zdfsUrl; // ← 绑死 ZDFS
}Agent 协议是跨版本契约，字段改名要做兼容处理，代价随版本递增。第一版就该取对： "ZDFS" 是当前实现选择（ZStack 封装的 JuiceFS），不是能力描述。如果明天换 CephFS/NFS，这个字段名就是负债。 详见 premium !13327 补充 review。 补充 Review by @ye.zou |
Collaborator
|
Comment from zhong.xian: 已修改 |
Collaborator
|
Comment from zhong.xian: 已经修改 |
MatheMatrix
force-pushed
the
sync/zhong.xian/feat/ZSTAC-83157@@2
branch
3 times, most recently
from
a6eae48 to
8f3da0f
Compare
Add virtiofs-based model mounting capability for user VMs: - Add VmModelMountVO database table and schema with hostUuid tracking - Add AttachVirtiofsCmd/DetachVirtiofsCmd for KVM agent - Add MountModelCenterCmd with storageUrl field - Add SDK actions for mount/unmount/query operations - Add error codes 10138-10149 Resolves: ZSTAC-83157 Change-Id: I746679736f7a7176646e646d797969766f697a76
MatheMatrix
force-pushed
the
sync/zhong.xian/feat/ZSTAC-83157@@2
branch
from
8f3da0f to
40df30e
Compare
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.
DBImpact
Resolves: ZSTAC-83157
Change-Id: I746679736f7a7176646e646d797969766f697a76
sync from gitlab !9459