<feature>[kvm]: ZSTAC-82672 add iothread vq mapping data path#4071
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新增 HostFactResponse.qemuKvmPackageVersion(灰度 5.5.28),在 KVMSystemTags 中新增对应系统标签常量并在保存主机事实时写入或删除该标签;在 KVMConstant 中新增 iothread/vq 映射最低版本常量;在 VolumeTO 中增加 ioThreads 字段及其复制构造与访问器;新增集成测试验证标签清除逻辑。 ChangesQEMU/KVM 包版本跟踪
VolumeTO IO 线程配置与版本常量
测试
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 unit tests (beta)
Comment |
7241a03 to
5da32cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.java (1)
84-84: ⚡ Quick win澄清
VolumeTO.ioThreads默认值 0 的语义:当前代码无外部依赖,建议补充字段说明
plugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.java中ioThreads仅存在字段声明、复制构造this.ioThreads = other.ioThreads;、以及getIoThreads/setIoThreads,全仓库未发现任何getIoThreads()的调用点或基于ioThreads(含与 0 的比较)的业务判断逻辑。- 因此目前不存在因默认
0产生行为差异的依赖;但建议在字段/注释或相关参数说明中明确0表示的语义(未设置/等价默认限制/无限制等),避免后续接入误用。🤖 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 `@plugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.java` at line 84, The VolumeTO.ioThreads field's default value of 0 has no external use now but its semantics should be explicit to avoid future misuse: update VolumeTO by adding a JavaDoc comment on the ioThreads field (and matching javadoc on getIoThreads/setIoThreads) that states what 0 means (e.g., "0 = unset / use host default / no limit" — choose the correct semantics for your design), and ensure the copy constructor assignment (this.ioThreads = other.ioThreads;) preserves that meaning; reference the VolumeTO class, the ioThreads field, and the getIoThreads/setIoThreads methods when making the clarification.
🤖 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/VolumeTO.java`:
- Line 54: The PR text references a field named ioThreadMax but the VolumeTO
class actually declares private int ioThreads; decide whether to align code or
docs: either rename the field in VolumeTO from ioThreads to ioThreadMax (update
any getters/setters, constructors, serialization uses and references to
VolumeTO) or update the PR description and any related docs/tests to mention
ioThreads; locate VolumeTO and its accessor methods to apply the rename or
update references elsewhere in the diff so names remain consistent across the
codebase and PR description.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.java`:
- Line 84: The VolumeTO.ioThreads field's default value of 0 has no external use
now but its semantics should be explicit to avoid future misuse: update VolumeTO
by adding a JavaDoc comment on the ioThreads field (and matching javadoc on
getIoThreads/setIoThreads) that states what 0 means (e.g., "0 = unset / use host
default / no limit" — choose the correct semantics for your design), and ensure
the copy constructor assignment (this.ioThreads = other.ioThreads;) preserves
that meaning; reference the VolumeTO class, the ioThreads field, and the
getIoThreads/setIoThreads methods when making the clarification.
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 06c45ba1-3c8d-4f24-8e90-66017521253e
📒 Files selected for processing (4)
plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMSystemTags.javaplugin/kvm/src/main/java/org/zstack/kvm/VolumeTO.java
|
Comment from yaohua.wu: Review: MR !9970 — ZSTAC-82672Background
关联 MR
P3 — Low / Suggestion
Coverage
Verdict: APPROVED主线侧改动极小、纯 DTO/Tag 拓展,无副作用,可合入。真正的复杂度集中在 premium 与 zstack-utility 两个关联 MR,本 MR 仅作为数据通路存在。 🤖 Robot Reviewer |
5da32cb to
fc63a59
Compare
|
Comment from haidong.pang: 感谢 review,已按意见处理:
主线侧仍保持只提供 fact/tag/DTO 数据通路,策略和行为边界放在 premium / utility。 |
35e3eae to
3d99feb
Compare
Resolves: ZSTAC-82672 Change-Id: Ib43a7a02a0528cc40cd8aa370b93b7fba7dfc051
3d99feb to
cff7e71
Compare
Summary
Implement the mainline KVM data path needed by the unified IOThread VQ mapping feature.
Changes
VolumeTO.ioThreadMaxso premium policy can pass target IOThread limits to KVM Agent.Resolves: ZSTAC-82672
sync from gitlab !9970