<fix>[eip]: expose attaching EIP to release#4127
Open
ZStack-Robot wants to merge 2 commits into
Open
Conversation
走查调整 EIP 附加处理：成功时缓存并刷新绑定字段以生成回调 inventory；失败时使用原始绑定值通过 SQL 清空记录中的绑定字段并进入失败通知。新增集成测试覆盖并发释放和失败回滚场景。 变更EIP 附加状态管理 与 测试
序列图sequenceDiagram
participant Client
participant EipManagerImpl
participant Database
participant FlatEipBackend
participant Callback
Client->>EipManagerImpl: APIAttachEipMsg
EipManagerImpl->>EipManagerImpl: 缓存 attachedVmNicUuid/attachedGuestIp
EipManagerImpl->>Database: dbf.updateAndRefresh(vo)
Database-->>EipManagerImpl: updated EipVO
EipManagerImpl->>FlatEipBackend: APPLY_EIP 请求
alt 附加成功
FlatEipBackend-->>EipManagerImpl: 200 OK
EipManagerImpl->>Callback: 发布成功事件（使用刷新后的 EipInventory）
else 附加失败
FlatEipBackend-->>EipManagerImpl: 500 错误
EipManagerImpl->>Database: SQL 按 eipUuid + 原 vmNicUuid + 原 guestIp 清空绑定字段
Database-->>EipManagerImpl: 清空完成
EipManagerImpl->>Callback: 进入失败通知流程
end
评估代码审查工作量🎯 4 (复杂) | ⏱️ ~45 分钟 诗
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin/eip/src/main/java/org/zstack/network/service/eip/EipManagerImpl.java (1)
610-621: ⚡ Quick win在回滚条件更新后检查受影响行数并告警，防止与
vmIpChanged竞态造成状态不一致。
vmIpChanged(...)会在找到EipVO_.vmNicUuid == nic.uuid且EipVO_.guestIp == oldIp时，直接eip.setGuestIp(newIp)并dbf.update(eip)。- 回滚里使用条件更新同时匹配
EipVO_.vmNicUuid == attachedVmNicUuid且EipVO_.guestIp == attachedGuestIp；若回滚前已发生vmIpChanged改写了guestIp，则该回滚可能更新 0 行，绑定不会被清空。UpdateQuery.update()返回int，建议在回滚处接收返回值：affectedRows == 0时记录 warning 日志（带eipUuid/attachedVmNicUuid/attachedGuestIp），便于排查该边界竞态。🤖 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/eip/src/main/java/org/zstack/network/service/eip/EipManagerImpl.java` around lines 610 - 621, The rollback in the anonymous fail(...) inside EipManagerImpl uses a conditional UpdateQuery but ignores the returned affected row count; modify the fail handler to capture the int returned by SQL.New(...).set(...).update() and when affectedRows == 0 emit a warning log (include msg.getEipUuid(), attachedVmNicUuid and attachedGuestIp) to surface the vmIpChanged race where guestIp was changed and the conditional update cleared 0 rows; keep existing evt.setError(...) and bus.publish(evt) behavior after logging.
🤖 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 `@plugin/eip/src/main/java/org/zstack/network/service/eip/EipManagerImpl.java`:
- Around line 610-621: The rollback in the anonymous fail(...) inside
EipManagerImpl uses a conditional UpdateQuery but ignores the returned affected
row count; modify the fail handler to capture the int returned by
SQL.New(...).set(...).update() and when affectedRows == 0 emit a warning log
(include msg.getEipUuid(), attachedVmNicUuid and attachedGuestIp) to surface the
vmIpChanged race where guestIp was changed and the conditional update cleared 0
rows; keep existing evt.setError(...) and bus.publish(evt) behavior after
logging.
ℹ️ 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: 7442b00d-6e9c-4be2-9da2-4d7be0a506d2
📒 Files selected for processing (1)
plugin/eip/src/main/java/org/zstack/network/service/eip/EipManagerImpl.java
added 2 commits
June 2, 2026 10:07
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
Add an integration subcase that blocks backend EIP apply. Verify the EIP DB binding before the backend call returns. Cover backend apply failure too. Ensure the early binding is rolled back after attach fails. Related: ZSTAC-83309 Change-Id: Ifadfc85f14412579abb6c7b5f1706bc614db5edd
MatheMatrix
force-pushed
the
sync/dejing.liu/codex/ZSTAC-83309-eip-attach-race
branch
from
aa07605 to
8ac6855
Compare
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
`@test/src/test/groovy/org/zstack/test/integration/networkservice/provider/flat/eip/AttachEipRaceCase.groovy`:
- Around line 174-180: The test currently asserts deleteCmd != null immediately
after StopVmInstanceAction.call(), but deleteCmd is set asynchronously in the
env.afterSimulator callback for FlatEipBackend.DELETE_EIP_PATH; update the test
to wait for the simulator callback to run before asserting (e.g. poll/retry with
timeout or use a CountDownLatch/AtomicReference) by waiting until deleteCmd is
non-null (and optionally verifying cmd.eip.eipUuid == eip.uuid) after calling
StopVmInstanceAction.call(); reference the
env.afterSimulator(FlatEipBackend.DELETE_EIP_PATH) callback, the deleteCmd
variable, and the StopVmInstanceAction.call() invocation when making the change.
🪄 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: daff05d3-149d-48e5-8831-b2c376e7c800
📒 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
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/eip/src/main/java/org/zstack/network/service/eip/EipManagerImpl.java
Comment on lines
+174
to
+180
| env.afterSimulator(FlatEipBackend.DELETE_EIP_PATH) { rsp, HttpEntity<String> entity -> | ||
| FlatEipBackend.DeleteEipCmd cmd = json(entity.getBody(), FlatEipBackend.DeleteEipCmd.class) | ||
| if (cmd.eip.eipUuid == eip.uuid) { | ||
| deleteCmd = cmd | ||
| } | ||
| return rsp | ||
| } |
There was a problem hiding this comment.
等待 DELETE_EIP_PATH 回调后再断言。
Line 221 直接断言 deleteCmd != null，但这个值是在模拟器回调里异步赋值的。StopVmInstanceAction.call() 返回时，删除请求不一定已经到达这里，测试会变成偶发失败。
建议修改
CountDownLatch applyEntered = new CountDownLatch(1)
CountDownLatch releaseApply = new CountDownLatch(1)
+ CountDownLatch deleteEntered = new CountDownLatch(1)
FlatEipBackend.ApplyEipCmd applyCmd = null
FlatEipBackend.DeleteEipCmd deleteCmd = null
@@
env.afterSimulator(FlatEipBackend.DELETE_EIP_PATH) { rsp, HttpEntity<String> entity ->
FlatEipBackend.DeleteEipCmd cmd = json(entity.getBody(), FlatEipBackend.DeleteEipCmd.class)
if (cmd.eip.eipUuid == eip.uuid) {
deleteCmd = cmd
+ deleteEntered.countDown()
}
return rsp
}
@@
def result = action.call()
assert result.error == null
+ assert deleteEntered.await(30, TimeUnit.SECONDS)
assert deleteCmd != null
assert deleteCmd.eip.eipUuid == eip.uuidAlso applies to: 215-223
🤖 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 174 - 180, The test currently asserts deleteCmd != null immediately
after StopVmInstanceAction.call(), but deleteCmd is set asynchronously in the
env.afterSimulator callback for FlatEipBackend.DELETE_EIP_PATH; update the test
to wait for the simulator callback to run before asserting (e.g. poll/retry with
timeout or use a CountDownLatch/AtomicReference) by waiting until deleteCmd is
non-null (and optionally verifying cmd.eip.eipUuid == eip.uuid) after calling
StopVmInstanceAction.call(); reference the
env.afterSimulator(FlatEipBackend.DELETE_EIP_PATH) callback, the deleteCmd
variable, and the StopVmInstanceAction.call() invocation when making the change.
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
1 participant
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.
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 !10029