<fix>[vm]: verify dest state in migrate flow#4009
Conversation
|
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 selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough当迁移请求到目标主机失败时,新增逻辑向目标主机发送 CheckVmStateOnHypervisorMsg 查询 VM 状态;仅当目标主机返回 VM 为 Running 时继续迁移流程,否则以原始迁移错误结束。新增集成测试覆盖回滚与成功场景及相关 mocks。 ChangesVM 迁移故障恢复与状态验证
Sequence DiagramsequenceDiagram
participant Flow as VmMigrateOnHypervisorFlow
participant CloudBus as CloudBus
participant DestHost as 目标主机
Flow->>DestHost: MigrateVmOnHypervisorMsg (迁移请求)
DestHost-->>Flow: 错误响应(迁移失败)
Flow->>CloudBus: CheckVmStateOnHypervisorMsg (请求 vmInstanceUuids)
CloudBus->>DestHost: 转发查询请求
DestHost-->>CloudBus: 返回 VM 状态映射
CloudBus-->>Flow: CheckVmStateOnHypervisorReply (states)
alt states[vmUuid] == Running
Flow->>Flow: chain.next() (继续迁移流程)
else
Flow->>Flow: chain.fail(migrateError) (以原错误结束)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/vm/VmMigrateOnHypervisorFlow.java`:
- Around line 87-89: The code reads the VM state without null checks which can
NPE when states is absent; update the block in VmMigrateOnHypervisorFlow to
null-check CheckVmStateOnHypervisorReply#getStates() and the looked-up state
(r.getStates() and r.getStates().get(spec.getVmInventory().getUuid())) before
comparing to VmInstanceState.Running.toString(), and if either is null treat it
as a failure path and invoke chain.fail(migrateError) (keep the existing
migrateError handling and logging).
🪄 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: 2bc0dd2d-1b7e-4ffb-b185-e5bba819b173
📒 Files selected for processing (2)
compute/src/main/java/org/zstack/compute/vm/VmMigrateOnHypervisorFlow.javatest/src/test/groovy/org/zstack/test/integration/kvm/vm/migrate/MigrateVmFailureCheckTargetHostCase.groovy
|
Comment on Comment from shan.wu: Fixed in
Validation:
|
7d78c9f to
08f52b3
Compare
Check the destination host when the hypervisor migration call fails. If the VM is Running there, continue the normal migration flow. That lets DB sync and post hooks run through the standard path. Otherwise fail the flow and keep the rollback behavior. Resolves: ZSTAC-83894 Change-Id: I8b4774a405fc3b1c05d21b6742facd26bc8d03e6
08f52b3 to
6736c47
Compare
Root Cause
When the hypervisor migration call reports failure, the VM may already be running on the destination host. Treating that response as a hard migration failure can roll back management state even though the data plane migration has completed.
Solution
VmMigrateOnHypervisorFlow, check the VM state on the destination host when the hypervisor migration call fails.Running, continue the normal migration flow withchain.next().chain.fail(originalError).Test
git diff --checkroot@172.20.13.237:/root/zstack-workspace/zstack:mvn -q -Dtest=org.zstack.test.integration.kvm.KvmTest -DsubCaseCollectionStrategy=Designated -DcaseFilePath=/tmp/zstac83894-main-cases.txt -DsurefireArgLine="-noverify" testTests run: 1, Failures: 0, Errors: 0, Skipped: 0sync from gitlab !9902