<fix>[header]: add VM migrate host-constraint extension point#4062
Open
zstack-robot-1 wants to merge 1 commit into
Open
<fix>[header]: add VM migrate host-constraint extension point#4062zstack-robot-1 wants to merge 1 commit into
zstack-robot-1 wants to merge 1 commit into
Conversation
Introduce VmMigrateHostConstraintExtensionPoint so that components can report VMs pinned to their current host by passthrough devices. 1. Why is this change necessary? DRS automatic load balancing kept generating migration tasks for VMs that hold passthrough devices (physical PCI / dGPU / mdev). Such VMs cannot be live-migrated, so every advised migration was bound to fail. DRS had no way to learn which VMs are pinned to their host. 2. How does it address the problem? A new extension point, VmMigrateHostConstraintExtensionPoint, lets device-owning components report the subset of candidate VMs that cannot be migrated off their current host. DRS queries it before building migration advice. VF / vDPA NIC devices are intentionally excluded since they support migration. 3. Are there any side effects? None. The interface only adds a new query extension point and does not change existing behavior on its own. # Summary of changes (by module): - header: add VmMigrateHostConstraintExtensionPoint interface Related: ZSTAC-85208 Change-Id: Ia303bf54f47fdd54a2814d7e6b298c541f4cf4bf
总体说明此 PR 新增了一个公共扩展点接口 变更清单VM 迁移主机约束接口
代码审查工作量估算🎯 1 (Trivial) | ⏱️ ~3 分钟 庆祝诗
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/vm/VmMigrateHostConstraintExtensionPoint.java (1)
17-22: ⚡ Quick win可选建议:增强方法契约文档的完整性
当前的 Javadoc 已经清晰描述了方法的主要用途,但可以考虑补充以下边界情况的说明以提高实现者的清晰度:
candidateVmUuids参数为null或包含null元素时的预期行为- 返回空列表
[]的明确语义(即没有任何候选 VM 被宿主机约束)- 实现是否允许返回不在输入列表中的 UUID
📝 建议的文档增强示例
/** * `@param` candidateVmUuids VM uuids being considered for migration + * (must not be null; null elements are ignored) * `@return` the subset of candidateVmUuids that cannot be migrated off * their current host because of attached passthrough devices + * (returns empty list if none are host-bound; must not return + * UUIDs that are not present in the input list) */🤖 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 `@header/src/main/java/org/zstack/header/vm/VmMigrateHostConstraintExtensionPoint.java` around lines 17 - 22, Update the Javadoc for VmMigrateHostConstraintExtensionPoint.getHostBoundVmUuids(List<String> candidateVmUuids) to explicitly document null/empty handling and return semantics: state what happens if candidateVmUuids is null or contains null elements (e.g., throw NPE or treat as empty), clarify that returning an empty list means no VMs are host-bound, and explicitly require that implementations only return UUIDs from the provided candidateVmUuids (or document if returning other UUIDs is allowed); ensure the contract mentions whether the returned list may be mutable or must be immutable.
🤖 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
`@header/src/main/java/org/zstack/header/vm/VmMigrateHostConstraintExtensionPoint.java`:
- Around line 17-22: Update the Javadoc for
VmMigrateHostConstraintExtensionPoint.getHostBoundVmUuids(List<String>
candidateVmUuids) to explicitly document null/empty handling and return
semantics: state what happens if candidateVmUuids is null or contains null
elements (e.g., throw NPE or treat as empty), clarify that returning an empty
list means no VMs are host-bound, and explicitly require that implementations
only return UUIDs from the provided candidateVmUuids (or document if returning
other UUIDs is allowed); ensure the contract mentions whether the returned list
may be mutable or must be immutable.
ℹ️ 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: a5cf15cf-a49d-439c-89f1-2490bcd85b36
📒 Files selected for processing (1)
header/src/main/java/org/zstack/header/vm/VmMigrateHostConstraintExtensionPoint.java
Collaborator
|
Comment from yaohua.wu: Review: MR !9960 + !14039 — ZSTAC-85208
Background (preserved across rounds)
总体评价实现思路与 Jira 技术分析的「方案 A」一致，质量良好：
P1 — High
P2 — Moderate
Coverage
Verdict: APPROVED当前 MR 在其声明范围（DRS 预防层 / 方案 A）内实现正确、逻辑清晰，无阻塞性（P0）问题，可以合并。但强烈建议：
🤖 Robot Reviewer |
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
3 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.
ZSTAC-85208
Root Cause
DRS automatic load balancing kept generating migration tasks for VMs that
hold passthrough devices (physical PCI / dGPU / mdev). Such VMs cannot be
live-migrated, so every advised migration was bound to fail. DRS had no
way to learn which VMs are pinned to their current host.
Changes
headerVmMigrateHostConstraintExtensionPointinterface — lets components report the subset of candidate VMs pinned to their host by passthrough devicesNote
This MR adds the extension point interface only. The DRS query logic and
the device-side implementations live in the related premium MR (same
branch
bugfix/ZSTAC-85208@@2).Related: ZSTAC-85208
sync from gitlab !9960