<fix>[eip]: expose attaching EIP to release#4168
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ 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 (2)
总体概述本PR通过在EIP attach流程中引入早期绑定和失败回滚机制，改进了并发场景下的数据一致性。实现缓存绑定目标后立即写库，attach失败时通过条件SQL回滚清理绑定数据。新增集成测试验证VM停止期间的EIP释放和失败回滚场景。 变更详情EIP Attach早期绑定与失败回滚
🎯 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/eip/AttachEipRaceCase.groovy (1)
152-226: 💤 Low value测试方法名称与实际逻辑不符。
方法名
testStopVmDuringAttachReleasesEip暗示在 attach 进行中停止 VM，但实际流程是：
- Line 211:
attachThread.join(30000)等待 attach 完成- Line 217-225: attach 完成后才停止 VM
当前测试实际验证的是：
- 后端处理期间早期绑定在 DB 可见
- attach 成功完成
- attach 完成后停止 VM 触发 EIP 释放
如果要测试"attach 进行中停止 VM 触发清理"，应在
releaseApply.countDown()之前执行StopVmInstanceAction，然后验证清理行为。建议将方法名改为
testEarlyBindingVisibleAndVmStopReleasesEip或调整测试逻辑以匹配原名称。🤖 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 `@test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/eip/AttachEipRaceCase.groovy` around lines 152 - 226, The test method name testStopVmDuringAttachReleasesEip is misleading because the code waits for attach to finish (attachThread.join) before calling StopVmInstanceAction; either rename the method to reflect current behavior (e.g., testEarlyBindingVisibleAndVmStopReleasesEip) or change the control flow so StopVmInstanceAction is invoked while apply is blocked: move the StopVmInstanceAction call to before releaseApply.countDown() (after applyEntered.await and DB assertions) and remove the attachThread.join wait that forces attach to complete first; update assertions accordingly to verify EIP release during the blocked attach using deleteCalled and deleteCmd.
🤖 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
`@test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/eip/AttachEipRaceCase.groovy`:
- Around line 152-226: The test method name testStopVmDuringAttachReleasesEip is
misleading because the code waits for attach to finish (attachThread.join)
before calling StopVmInstanceAction; either rename the method to reflect current
behavior (e.g., testEarlyBindingVisibleAndVmStopReleasesEip) or change the
control flow so StopVmInstanceAction is invoked while apply is blocked: move the
StopVmInstanceAction call to before releaseApply.countDown() (after
applyEntered.await and DB assertions) and remove the attachThread.join wait that
forces attach to complete first; update assertions accordingly to verify EIP
release during the blocked attach using deleteCalled and deleteCmd.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9918d773-e295-4668-b26f-76421777e2d6
📒 Files selected for processing (2)
plugin/eip/src/main/java/org/zstack/network/service/eip/EipManagerImpl.javatest/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/eip/AttachEipRaceCase.groovy
Persist vmNicUuid/guestIp before backend apply so VM release can see in-flight EIP bindings. Roll back only when the failed attach still matches the same EIP/NIC/IP tuple. Test: real Flat EIP flow and AttachEipRaceCase JIRA: ZSTAC-83309 Change-Id: Ie8795d5bd1c3e90aa10e18cfef19d4968c73cfee
MatheMatrix
force-pushed
the
sync/dejing.liu/codex/ZSTAC-83309-eip-attach-race-5.5.28
branch
from
02ffcce to
9877fbb
Compare
1 participant
Persist vmNicUuid and guestIp before applying an EIP on the backend. This lets VM stop and migration release paths see the in-flight EIP and enqueue cleanup while the backend apply is still running.
Roll back the binding only when the same EIP, NIC, and guest IP are still present if backend apply fails.
Related: ZSTAC-83309
Change-Id: Ia9b75acc82004755b905a02acca1a6a82c207332
sync from gitlab !10074