Skip to content

<fix>[eip]: expose attaching EIP to release#4127

Open
ZStack-Robot wants to merge 1 commit into
5.5.22from
sync/dejing.liu/codex/ZSTAC-83309-eip-attach-race
Open

<fix>[eip]: expose attaching EIP to release#4127
ZStack-Robot wants to merge 1 commit into
5.5.22from
sync/dejing.liu/codex/ZSTAC-83309-eip-attach-race

Conversation

@ZStack-Robot
Copy link
Copy Markdown
Collaborator

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

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
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

Review Change Stack

走查

在 EIP 附加请求处理中,改进了成功和失败两条路径的状态管理逻辑。成功时缓存目标值并使用刷新后的数据生成回调,失败时通过原值精确定位并清空绑定。

变更

EIP 附加状态管理

层 / 文件 摘要
附加流程成功和失败的状态管理
plugin/eip/src/main/java/org/zstack/network/service/eip/EipManagerImpl.java
在成功分支缓存 vmNicUuidguestIp,通过 dbf.updateAndRefresh() 刷新并生成新的 EipInventory 供后续回调使用;在失败分支使用原始绑定值作为 SQL 查询条件,清空对应 EIP 记录的绑定字段。

评估代码审查工作量

🎯 2 (简单) | ⏱️ ~10 分钟

🐰 一只兔子为你欢呼,
附加与清理更加精准!
缓存值,刷新数据,
失败时妥善回滚,
状态管理得以完美~✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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 标题遵循了[scope]: 的格式要求,43字符长度符合72字符限制,且清晰描述了主要改动。
Description check ✅ Passed 描述与代码变更相关联,详细说明了EIP绑定逻辑的优化和失败回滚机制的改进。
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sync/dejing.liu/codex/ZSTAC-83309-eip-attach-race

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.uuidEipVO_.guestIp == oldIp 时,直接 eip.setGuestIp(newIp)dbf.update(eip)
  • 回滚里使用条件更新同时匹配 EipVO_.vmNicUuid == attachedVmNicUuidEipVO_.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

📥 Commits

Reviewing files that changed from the base of the PR and between 113a77a and aa07605.

📒 Files selected for processing (1)
  • plugin/eip/src/main/java/org/zstack/network/service/eip/EipManagerImpl.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant