<fix>[host]: ZSTAC-85091 keep in-flight migrate dest mem reserve on recalc#4365
<fix>[host]: ZSTAC-85091 keep in-flight migrate dest mem reserve on recalc#4365zstack-robot-2 wants to merge 4 commits into
Conversation
RecalculateHostCapacity reset available memory to total minus landed Running VMs, erasing the destination reservation of VMs still mid live-migration (hostUuid=source). Scheduler then double-booked freed memory and OOM'd big VMs. Guard recalc to never raise a host's available memory while any VM is Migrating. Test: RecalculateHostCapacityKeepInflightReserveCase. Resolves: ZSTAC-85091 Change-Id: I37fde9329dffb3e44e03b6b251c9f73145713bbb
Scope the recalc freeze to hosts that are migration dest via sched history, not all hosts; restore available once VM lands. Test adds recovery and unrelated-host assertions. Resolves: ZSTAC-85091 Change-Id: I37fde9329dffb3e44e03b6b251c9f73145713bbb
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Warning
|
| Layer / File(s) | Summary |
|---|---|
迁移目的主机判断与容量回钳 compute/src/main/java/org/zstack/compute/allocator/HostAllocatorManagerImpl.java |
补充 VmInstanceVO/VmSchedHistoryVO 相关导入；新增 isMigrationDestHost(hostUuid) 方法，查询处于 Migrating 状态的 VM 并通过 VmSchedHistoryVO 判断目的主机；在 HostCapacityUpdaterRunnable.call 中新增分支，当目标主机为迁移目的地且重算可用内存高于重算前值时，将 avail 回钳为 before。 |
集成测试：重算保留 inflight reserve test/src/test/groovy/org/zstack/test/integration/kvm/capacity/RecalculateHostCapacityKeepInflightReserveCase.groovy |
新增 RecalculateHostCapacityKeepInflightReserveCase，构造两台 KVM 主机环境，创建 VM 并将状态置为 Migrating，分别模拟目标主机和无关主机的 inflight reserve；断言迁移进行中时目标主机可用内存不被抬升、无关主机恢复真实可用内存；VM 变为 Running 后再次断言目标主机恢复基准值。 |
估计代码审查工作量
🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
🐇 小兔跳过迁移路，
内存回钳稳如故。
目的主机莫抬升，
重算之时需自律。
inflight reserve 不丢失，
容量守护乐无虞！
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | 标题准确概括了重算时保留迁移目标机预留内存这一主要改动。 |
| Description check | ✅ Passed | 描述与本次修改高度相关，清楚说明了问题、原因、修复和测试内容。 |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
sync/jin.ma/fix/ZSTAC-85091
Comment @coderabbitai help to get the list of available commands.
![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
🧹 Nitpick comments (1)
compute/src/main/java/org/zstack/compute/allocator/HostAllocatorManagerImpl.java (1)
254-260: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win把迁移目的地主机集合提前一次性查出来。
isMigrationDestHost()现在在每个 host 的更新回调里都会重新查询一遍全部MigratingVM 和调度历史。RecalculateHostCapacityMsg支持 zone/cluster 级批量重算，这会把一次重算放大成按 host 数量线性增长的额外 DB 往返。建议在进入循环前先查出需要冻结的destHostUuid集合，再在循环里做contains判断。Also applies to: 287-298
🤖 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 `@compute/src/main/java/org/zstack/compute/allocator/HostAllocatorManagerImpl.java` around lines 254 - 260, In HostAllocatorManagerImpl, the host capacity recalculation loop is calling isMigrationDestHost() for every HostUsedCpuMem entry, causing repeated queries for Migrating VMs and scheduling history during batch RecalculateHostCapacityMsg handling. Precompute the destination host UUID set once before iterating hostUsedCpuMemList, then replace the per-host isMigrationDestHost() call with a simple contains check inside the HostCapacityUpdater callback to avoid linear DB overhead.
🤖 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
`@compute/src/main/java/org/zstack/compute/allocator/HostAllocatorManagerImpl.java`:
- Around line 287-298: 当前 isMigrationDestHost 通过 VmSchedHistoryVO 的 isExists()
只要命中任意历史记录就会把 host 误判为当前迁移目的主机。请在 HostAllocatorManagerImpl 的 isMigrationDestHost
中改为只根据每个 Migrating VM 的最新调度历史，或限定到当前迁移 attempt 对应的 VmSchedHistoryVO 记录，再判断
destHostUuid 是否等于目标 hostUuid，避免旧迁移记录持续命中导致误冻结。
In
`@test/src/test/groovy/org/zstack/test/integration/kvm/capacity/RecalculateHostCapacityKeepInflightReserveCase.groovy`:
- Around line 111-119: The VM migration test in
RecalculateHostCapacityKeepInflightReserveCase is relying on whatever host the
scheduler picks, so the assertions against host capacity can be wrong. Fix the
setup in the VM creation flow and the simulated migration completion so the
source host is explicitly controlled, and update VmInstanceVO.hostUuid to
destHost.uuid when marking the VM back to Running. This keeps the reserve
recalculation logic aligned with the intended host and makes the later
assertions deterministic.
---
Nitpick comments:
In
`@compute/src/main/java/org/zstack/compute/allocator/HostAllocatorManagerImpl.java`:
- Around line 254-260: In HostAllocatorManagerImpl, the host capacity
recalculation loop is calling isMigrationDestHost() for every HostUsedCpuMem
entry, causing repeated queries for Migrating VMs and scheduling history during
batch RecalculateHostCapacityMsg handling. Precompute the destination host UUID
set once before iterating hostUsedCpuMemList, then replace the per-host
isMigrationDestHost() call with a simple contains check inside the
HostCapacityUpdater callback to avoid linear DB overhead.
🪄 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: 2578603b-aa9a-4381-b1d7-e02e2debcce2
📒 Files selected for processing (2)
compute/src/main/java/org/zstack/compute/allocator/HostAllocatorManagerImpl.javatest/src/test/groovy/org/zstack/test/integration/kvm/capacity/RecalculateHostCapacityKeepInflightReserveCase.groovy
| private boolean isMigrationDestHost(String hostUuid) { | ||
| List<String> migratingVmUuids = Q.New(VmInstanceVO.class) | ||
| .eq(VmInstanceVO_.state, VmInstanceState.Migrating) | ||
| .select(VmInstanceVO_.uuid) | ||
| .listValues(); | ||
| if (migratingVmUuids.isEmpty()) { | ||
| return false; | ||
| } | ||
| return Q.New(VmSchedHistoryVO.class) | ||
| .in(VmSchedHistoryVO_.vmInstanceUuid, migratingVmUuids) | ||
| .eq(VmSchedHistoryVO_.destHostUuid, hostUuid) | ||
| .isExists(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
不要用“任意历史记录存在”来判断当前迁移目的主机。
VmSchedHistoryVO 是历史表，这里的 isExists() 只要命中某个 Migrating VM 过去留下的一条 destHostUuid = hostUuid 记录，就会把该 host 当成当前目的地。VM 发生过多次迁移时，旧记录仍会命中，导致当前并非目的地的 host 也被持续冻结，和这次修复“只冻结实际目的主机”的目标相反。这里至少要收敛到该 VM 最新的一条调度历史，或当前迁移 attempt 对应的记录。
🤖 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
`@compute/src/main/java/org/zstack/compute/allocator/HostAllocatorManagerImpl.java`
around lines 287 - 298, 当前 isMigrationDestHost 通过 VmSchedHistoryVO 的 isExists()
只要命中任意历史记录就会把 host 误判为当前迁移目的主机。请在 HostAllocatorManagerImpl 的 isMigrationDestHost
中改为只根据每个 Migrating VM 的最新调度历史，或限定到当前迁移 attempt 对应的 VmSchedHistoryVO 记录，再判断
destHostUuid 是否等于目标 hostUuid，避免旧迁移记录持续命中导致误冻结。
RecalculateHostCapacityMsg has no reply handler; the IT used bus.call which hung 5 min on future timeout. Switch to bus.send + retryInSecs polling assertions. Also fill VmSchedHistoryVO NOT NULL columns (accountUuid, schedType) that crashed the forked VM. Resolves: ZSTAC-85091 Change-Id: I37fde9329dffb3e44e03b6b251c9f73145713bbb
Rewrite to write-groovy-test rules: all preconditions via SDK Action (createVmInstance + migrateVm), no SQL/dbf.persist construction. Intercept the KVM migrate agent path mid-flight (VM Migrating, dest reserved), trigger RecalculateHostCapacityMsg (internal periodic task, no SDK Action), assert dest availableMemory keeps the reservation (56G) instead of rising to total (64G). Verified: destAvailInflight=destAvailAfterRecalc=60129542144. Resolves: ZSTAC-85091 Change-Id: I37fde9329dffb3e44e03b6b251c9f73145713bbb
2 participants
Summary
热迁移期间周期任务 RecalculateHostCapacity 重算可用内存时，把仍在 Migrating（hostUuid=源）VM 的目标机预留内存擦除，调度器叠加双订导致大内存 VM 触发宿主机 OOM。现场：available before 158G → now 364G，多算回 ~206G 预留。
Root Cause
重算 available = total − 已落库 Running VM 内存，未计入热迁移 in-flight VM 在目标机的预留（VM 状态未切到该 host）。
Fix
重算抬高某 host available 前，经 VmSchedHistory.destHostUuid 判定该 host 是否为某 Migrating VM 的迁移目标；是则不抬高（保留预留）。仅冻结迁移目标机，其余 host 正常回收，迁移结束后恢复。
Changes
Testing
Resolves: ZSTAC-85091
sync from gitlab !10325