<fix>[rest]: fix bug<description>#3611
Open
zstack-robot-2 wants to merge 2 commits intofeature-5.5.12-znsfrom
Open
<fix>[rest]: fix bug<description>#3611zstack-robot-2 wants to merge 2 commits intofeature-5.5.12-znsfrom
zstack-robot-2 wants to merge 2 commits intofeature-5.5.12-znsfrom
Conversation
Walkthrough添加多处功能与集成改动：WebhookCallbackClient 增加覆盖与投递方法；ZNS 集成文档大幅扩展协议/数据模型/流程；引入 AfterAllocateVmNicIp 扩展点并在 NIC IP 分配后调用；SDN 控制器管理钩子与异常处理调整；KVM/NIC 与系统标签相关扩展。 Changes
Sequence Diagram(s)sequenceDiagram
participant Flow as VmAllocateNicIpFlow
participant Registry as PluginRegistry
participant Ext as AfterAllocateVmNicIpExtensionPoint
participant SDN as SdnControllerManagerImpl
Flow->>Registry: 查询 AfterAllocateVmNicIp 扩展列表
alt 无扩展
Registry-->>Flow: 空列表
Flow-->>Flow: 继续后续流程（trigger.next）
else 有扩展
Registry-->>Flow: 返回扩展实例列表
loop 为每个扩展调用
Flow->>Ext: afterAllocateVmNicIp(spec, Completion)
Ext-->>Flow: success() / fail(ErrorCode)
end
alt 任一扩展 fail
Flow-->>Flow: 以首个错误回滚/触发失败
else 全部 success
Flow-->>Flow: 继续后续流程（trigger.next）
Flow->>SDN: 可能触发 SDN 端口创建等后续操作
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.1)plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaComment |
Resolves: ZCF-1365 Change-Id: I696f6a6a6e6d7867746e70736d6d646861686166
MatheMatrix
force-pushed
the
sync/shixin.ruan/shixin-ZCF-1365@@2
branch
from
5d0bfa2 to
fec131c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
core/src/main/java/org/zstack/core/rest/webhook/WebhookCallbackClient.java (1)
134-141: 代码变更合理，建议考虑添加基本的防御性校验。
setCallbackUrl方法允许覆盖回调 URL，这对于使用独立 HTTP 端点处理回调的场景很有用。建议考虑：
- 添加
null或空字符串校验，防止意外设置无效值- 由于
callbackUrl字段可能被多线程访问（设置和读取），可考虑将其声明为volatile♻️ 可选的防御性改进
public void setCallbackUrl(String callbackUrl) { + if (callbackUrl == null || callbackUrl.trim().isEmpty()) { + throw new IllegalArgumentException("callbackUrl cannot be null or empty"); + } this.callbackUrl = callbackUrl; }或者将字段声明为 volatile：
- private String callbackUrl; + private volatile String callbackUrl;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/rest/webhook/WebhookCallbackClient.java` around lines 134 - 141, 在 WebhookCallbackClient 中，setCallbackUrl 方法应增加防御性校验并确保字段线程安全：在 setCallbackUrl(String callbackUrl) 中拒绝 null 或空字符串（抛出 IllegalArgumentException 或忽略并记录），并将类成员 callbackUrl 声明为 volatile 以保证多线程可见性；保留现有语义但在设置前执行非空/非空白验证并记录或抛出以避免无效值被写入。plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java (1)
277-277: 捕获通用Exception过于宽泛当前代码捕获了所有
Exception，包括可能表示编程错误的RuntimeException（如NullPointerException、IllegalArgumentException）。建议明确捕获预期的异常类型。根据编码规范："捕获异常时需严格匹配期望的异常类型或其父类，避免捕获错误的异常类型"。
建议：区分处理预期异常和编程错误
- } catch (Exception e) { + } catch (RuntimeException e) { + // Re-throw programming errors + if (e instanceof NullPointerException || e instanceof IllegalArgumentException) { + throw e; + } logger.warn(String.format("failed to load SdnControllerVO[uuid:%s] after init: %s", vo.getUuid(), e.getMessage()), e);或者明确只捕获预期的异常类型（如数据库相关异常）。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java` at line 277, The catch block in SdnControllerManagerImpl that currently uses a broad "catch (Exception e)" is too wide; replace it by catching only the expected checked exceptions (e.g., specific DB/IO/checked types thrown by the enclosed logic) and let unchecked exceptions (RuntimeException, NullPointerException, IllegalArgumentException) bubble up or rethrow them after logging; if multiple different checked exceptions can occur, catch them separately or catch their common checked superclass, and ensure any logging inside the catch includes the exception variable and context. Locate the catch in SdnControllerManagerImpl (the try/catch around the code at the reported diff) and update the catch clause(s) accordingly, rethrowing or failing fast for programming errors while handling only anticipated checked exceptions.docs/modules/network/pages/networkResource/ZnsIntegration.adoc (2)
337-337: 明确 cms_resource_uuid 的映射关系和命名约定文档第 337 行提到通过
cms.cms_resource_uuid关联 Cloud L2，这里涉及两个层次的命名：
- ZNS 侧的 JSON 字段使用 snake_case：
cms_resource_uuid（符合 Go 语言 JSON 标签约定，见第 22 行）- Cloud 侧的结构体字段使用 camelCase：
CmsResourceUuid（第 22 行）建议在文档中补充说明：
- 当描述 ZNS API 调用时，应使用
cms_resource_uuid（snake_case）- 当描述 Cloud 内部数据结构时，应使用
CmsResourceUuid（camelCase）这样可以帮助读者区分不同系统的命名约定，避免混淆。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/modules/network/pages/networkResource/ZnsIntegration.adoc` at line 337, 在第 337 行补充关于命名约定的说明：说明 ZNS 对外 JSON 字段采用 snake_case（示例字段 cms_resource_uuid，用于描述 ZNS API 请求/响应），而 Cloud 内部结构体采用 camelCase（示例字段 CmsResourceUuid，用于描述 Cloud 端数据结构/代码），并建议在文档中约定在谈及 API payload 时使用 cms_resource_uuid，在谈及 Cloud 内部实现或 Go 结构体时使用 CmsResourceUuid，以便读者区分两端命名并避免混淆。
94-95: 建议统一 HTTP Header 名称的大小写格式文档中 HTTP Header 名称的大小写格式不一致：
x-web-hook和x-job-uuid使用全小写格式（第 94-95 行，711-712 行）X-Api-Version和X-Api-Versions使用首字母大写格式（第 626、677、700、709 行等）虽然 HTTP Header 名称根据 RFC 7230 是大小写不敏感的，但在技术文档中保持一致的书写格式有助于提高可读性。
建议统一使用首字母大写的格式（如
X-Web-Hook、X-Job-Uuid），与X-Api-Version保持一致。Also applies to: 711-712
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/modules/network/pages/networkResource/ZnsIntegration.adoc` around lines 94 - 95, Update the HTTP header name casing in the ZnsIntegration.adoc docs to be consistent with the Title-Case style used elsewhere: replace occurrences of `x-web-hook` with `X-Web-Hook` and `x-job-uuid` with `X-Job-Uuid` (also update any repeated occurrences around the 711-712 region); ensure other headers like `X-Api-Version` / `X-Api-Versions` remain unchanged so all header names follow the same capitalized-word format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`:
- Around line 273-282: The current post-processing in SdnControllerManagerImpl
inside the reply.isSuccess() branch treats failures in tag creation or inventory
loading as API errors, risking state inconsistency because doCreateSdnController
already persisted the SdnController and pingTracker.track() was invoked; change
the handling so that tagMgr.createTagsFromAPICreateMessage(...) and the
dbf.findByUuid(...) -> SdnControllerInventory.valueOf(...) call are wrapped to
log warnings/errors but do not set event.setError(...) or change the API success
response; instead ensure event.setInventory(...) is still set when possible and,
on irrecoverable post-processing failure, either roll back by deleting the
created SdnController (using the same persistence APIs used in
doCreateSdnController) or explicitly document/emit an async cleanup task—modify
the code in SdnControllerManagerImpl (the block referencing
tagMgr.createTagsFromAPICreateMessage, dbf.findByUuid, event.setInventory, and
event.setError) to implement the logging-only behavior or the explicit
rollback/cleanup path.
---
Nitpick comments:
In `@core/src/main/java/org/zstack/core/rest/webhook/WebhookCallbackClient.java`:
- Around line 134-141: 在 WebhookCallbackClient 中，setCallbackUrl
方法应增加防御性校验并确保字段线程安全：在 setCallbackUrl(String callbackUrl) 中拒绝 null 或空字符串（抛出
IllegalArgumentException 或忽略并记录），并将类成员 callbackUrl 声明为 volatile
以保证多线程可见性；保留现有语义但在设置前执行非空/非空白验证并记录或抛出以避免无效值被写入。
In `@docs/modules/network/pages/networkResource/ZnsIntegration.adoc`:
- Line 337: 在第 337 行补充关于命名约定的说明：说明 ZNS 对外 JSON 字段采用 snake_case（示例字段
cms_resource_uuid，用于描述 ZNS API 请求/响应），而 Cloud 内部结构体采用 camelCase（示例字段
CmsResourceUuid，用于描述 Cloud 端数据结构/代码），并建议在文档中约定在谈及 API payload 时使用
cms_resource_uuid，在谈及 Cloud 内部实现或 Go 结构体时使用 CmsResourceUuid，以便读者区分两端命名并避免混淆。
- Around line 94-95: Update the HTTP header name casing in the
ZnsIntegration.adoc docs to be consistent with the Title-Case style used
elsewhere: replace occurrences of `x-web-hook` with `X-Web-Hook` and
`x-job-uuid` with `X-Job-Uuid` (also update any repeated occurrences around the
711-712 region); ensure other headers like `X-Api-Version` / `X-Api-Versions`
remain unchanged so all header names follow the same capitalized-word format.
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`:
- Line 277: The catch block in SdnControllerManagerImpl that currently uses a
broad "catch (Exception e)" is too wide; replace it by catching only the
expected checked exceptions (e.g., specific DB/IO/checked types thrown by the
enclosed logic) and let unchecked exceptions (RuntimeException,
NullPointerException, IllegalArgumentException) bubble up or rethrow them after
logging; if multiple different checked exceptions can occur, catch them
separately or catch their common checked superclass, and ensure any logging
inside the catch includes the exception variable and context. Locate the catch
in SdnControllerManagerImpl (the try/catch around the code at the reported diff)
and update the catch clause(s) accordingly, rethrowing or failing fast for
programming errors while handling only anticipated checked exceptions.
🪄 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: 2a699efe-4261-47d9-893e-a70d0be4bb5b
📒 Files selected for processing (3)
core/src/main/java/org/zstack/core/rest/webhook/WebhookCallbackClient.javadocs/modules/network/pages/networkResource/ZnsIntegration.adocplugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java
Comment on lines
273
to
+282
| if (reply.isSuccess()) { | ||
| tagMgr.createTagsFromAPICreateMessage(msg, vo.getUuid(), SdnControllerVO.class.getSimpleName()); | ||
| event.setInventory(SdnControllerInventory.valueOf(dbf.findByUuid(vo.getUuid(), SdnControllerVO.class))); | ||
| try { | ||
| tagMgr.createTagsFromAPICreateMessage(msg, vo.getUuid(), SdnControllerVO.class.getSimpleName()); | ||
| event.setInventory(SdnControllerInventory.valueOf(dbf.findByUuid(vo.getUuid(), SdnControllerVO.class))); | ||
| } catch (Exception e) { | ||
| logger.warn(String.format("failed to load SdnControllerVO[uuid:%s] after init: %s", | ||
| vo.getUuid(), e.getMessage()), e); | ||
| event.setError(operr("failed to load SdnController[uuid:%s] after successful init: %s", | ||
| vo.getUuid(), e.getMessage())); | ||
| } |
There was a problem hiding this comment.
状态不一致风险：SDN 控制器已创建成功但 API 返回错误
当 reply.isSuccess() 为 true 时，SDN 控制器已经在 doCreateSdnController 中成功创建并持久化到数据库，且 pingTracker.track() 已被调用。此时如果 tag 创建或 inventory 加载失败，API 会返回错误，但资源实际已存在。
这会导致：
- 用户认为操作失败，可能重试导致困惑
- 资源存在但无法通过常规方式管理
- 孤儿资源难以清理
建议：对于非关键的 post-processing 失败（如 tag 创建），考虑记录错误但仍返回成功，或者在失败时回滚已创建的资源。
建议方案：tag 创建失败时仅记录警告，不影响 API 响应
if (reply.isSuccess()) {
try {
tagMgr.createTagsFromAPICreateMessage(msg, vo.getUuid(), SdnControllerVO.class.getSimpleName());
+ } catch (Exception e) {
+ logger.warn(String.format("failed to create tags for SdnController[uuid:%s]: %s",
+ vo.getUuid(), e.getMessage()), e);
+ }
+ try {
event.setInventory(SdnControllerInventory.valueOf(dbf.findByUuid(vo.getUuid(), SdnControllerVO.class)));
} catch (Exception e) {
logger.warn(String.format("failed to load SdnControllerVO[uuid:%s] after init: %s",
vo.getUuid(), e.getMessage()), e);
event.setError(operr("failed to load SdnController[uuid:%s] after successful init: %s",
vo.getUuid(), e.getMessage()));
}
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`
around lines 273 - 282, The current post-processing in SdnControllerManagerImpl
inside the reply.isSuccess() branch treats failures in tag creation or inventory
loading as API errors, risking state inconsistency because doCreateSdnController
already persisted the SdnController and pingTracker.track() was invoked; change
the handling so that tagMgr.createTagsFromAPICreateMessage(...) and the
dbf.findByUuid(...) -> SdnControllerInventory.valueOf(...) call are wrapped to
log warnings/errors but do not set event.setError(...) or change the API success
response; instead ensure event.setInventory(...) is still set when possible and,
on irrecoverable post-processing failure, either roll back by deleting the
created SdnController (using the same persistence APIs used in
doCreateSdnController) or explicitly document/emit an async cleanup task—modify
the code in SdnControllerManagerImpl (the block referencing
tagMgr.createTagsFromAPICreateMessage, dbf.findByUuid, event.setInventory, and
event.setError) to implement the logging-only behavior or the explicit
rollback/cleanup path.
Resolves: ZCF-1365 Change-Id: I726b776f6b73637a7379677566706e64667a7161
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/modules/network/pages/networkResource/ZnsIntegration.adoc (1)
55-911:⚠️ Potential issue | 🟡 Minor新增 ZNS 集成文档需要统一成英文。
这次新增的章节基本都是中文，和仓库对注释/文档/报错统一使用英文的规则不一致；继续这样合入会让外部文档和代码注释长期混用两套语言。
As per coding guidelines: "代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/modules/network/pages/networkResource/ZnsIntegration.adoc` around lines 55 - 911, The new ZNS integration doc contains extensive Chinese text and must be converted to English to comply with repository language rules; update every Chinese heading and paragraph (e.g. sections titled "UUID 格式约定", "ZNS SDN控制器", "创建SDN控制器", "重连SDN控制器", "心跳探活（Ping）", "API 拦截器", the "[[api-version]]" block and all NOTE blocks) into clear, correct English while preserving all technical identifiers, code snippets, headers, tables, inline code (e.g. `zns.controller.scheme`, API paths like /zns/api/v1/segments, systemTag keys, class/VO names such as ZnsControllerVO, ZnsTransportZoneVO, ZnsApiInterceptor), JSON examples, and semantics; keep formatting (Asciidoc syntax, tables, bullets, examples) unchanged and ensure any translated warnings/messages use proper English phrasing and repository-approved terminology.
♻️ Duplicate comments (1)
plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java (1)
273-280:⚠️ Potential issue | 🟠 Major成功落库后不要再把 API 结果翻成失败。
reply.isSuccess()到这里时，doCreateSdnController()已经完成持久化，控制器也已经进入pingTracker.track()的成功路径。这里只是打 tag 或回读 inventory 失败就event.setError(...)，会制造“资源已存在、调用方却收到失败”的状态不一致。后处理异常应该降级为告警，或者在这里显式回滚已创建的控制器。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java` around lines 273 - 280, The catch block after tagMgr.createTagsFromAPICreateMessage(...) and event.setInventory(...) should not convert a successful persistence into an API failure; instead either downgrade the failure to a logged warning/alert or perform an explicit rollback of the created SdnController. Replace the current event.setError(...) behavior in the catch with one of: (A) log the full exception (logger.warn/error) and leave event as success (do not call event.setError), or (B) call a rollback helper (e.g. remove the persisted SdnController via dbf and stop pingTracker.track for the SdnControllerVO uuid) then set event error. Update the catch around tagMgr.createTagsFromAPICreateMessage and event.setInventory to implement one of these approaches and ensure references to doCreateSdnController(), pingTracker.track(), SdnControllerVO, tagMgr.createTagsFromAPICreateMessage, and event.setInventory are handled accordingly.
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java (1)
4183-4184: 建议将 ZNS 网桥参数提取为常量，避免协议字符串漂移Line 4183 和 Line 4184 直接写入
"br-int"、"openvswitch"，后续若 agent 侧或其他模块调整命名，容易出现静默不一致。建议提取为共享常量（例如放在KVMConstant或专用常量类）并统一引用。As per coding guidelines: “避免使用魔法值（Magic Value）：直接使用未经定义的数值或字符串应替换为枚举或常量。”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java` around lines 4183 - 4184, 在 KVMHost 中直接使用字面量 "br-int" 和 "openvswitch"（在 to.setBridgeName(...) 和 to.setBridgePortType(...) 处）属于魔法字符串，建议在统一常量类（例如 新增或扩充 KVMConstant 或专用常量类）中定义如 ZNS_BRIDGE_NAME 和 ZNS_BRIDGE_PORT_TYPE 常量并在此处引用；修改步骤：在 KVMConstant 添加 public static final String ZNS_BRIDGE_NAME = "br-int" 和 ZNS_BRIDGE_PORT_TYPE = "openvswitch"，然后将 KVMHost 中的 to.setBridgeName("br-int") 改为 to.setBridgeName(KVMConstant.ZNS_BRIDGE_NAME) 及 to.setBridgePortType(KVMConstant.ZNS_BRIDGE_PORT_TYPE)，并搜索/替换仓库中相同字面量以统一引用。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmAllocateNicIpFlow.java`:
- Around line 193-194: The flow currently passes the entire spec into
callAfterAllocateVmNicIpExtensions(spec, trigger), which exposes both
IP-assigned and non-assigned NICs to AfterAllocateVmNicIpExtensionPoint
implementations (e.g., SdnControllerManagerImpl.afterAllocateVmNicIp); change
the call to only include the NICs that actually received IPs (the nicsWithIp /
firstL3s result) by constructing a filtered spec (or a minimal wrapper) that
contains only those destNics and their ip/UsedIpVO entries, then invoke
callAfterAllocateVmNicIpExtensions(filteredSpec, trigger) so extensions only see
NICs with prepared IPs.
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`:
- Around line 464-468: 注释描述的 VM NIC 生命周期钩子已过时：当前代码已改为在端口创建时使用
AfterAllocateVmNicIpExtensionPoint（不是 beforeAllocateVmNic /
BeforeAllocateVmNicExtensionPoint），端口删除仍应对应
AfterReleaseVmNic/AfterReleaseVmNicExtensionPoint；请更新该注释段（包含提到的函数/钩子名
PreVmInstantiateResourceExtensionPoint, VmReleaseResourceExtensionPoint,
InstantiateResourceOnAttachingNicExtensionPoint,
ReleaseNetworkServiceOnDetachingNicExtensionPoint, beforeAllocateVmNic,
BeforeAllocateVmNicExtensionPoint）以反映实际使用的 AfterAllocateVmNicIpExtensionPoint 和
AfterReleaseVmNic(ExtensionPoint)；保持表述简洁，避免误导后续排查。
---
Outside diff comments:
In `@docs/modules/network/pages/networkResource/ZnsIntegration.adoc`:
- Around line 55-911: The new ZNS integration doc contains extensive Chinese
text and must be converted to English to comply with repository language rules;
update every Chinese heading and paragraph (e.g. sections titled "UUID 格式约定",
"ZNS SDN控制器", "创建SDN控制器", "重连SDN控制器", "心跳探活（Ping）", "API 拦截器", the
"[[api-version]]" block and all NOTE blocks) into clear, correct English while
preserving all technical identifiers, code snippets, headers, tables, inline
code (e.g. `zns.controller.scheme`, API paths like /zns/api/v1/segments,
systemTag keys, class/VO names such as ZnsControllerVO, ZnsTransportZoneVO,
ZnsApiInterceptor), JSON examples, and semantics; keep formatting (Asciidoc
syntax, tables, bullets, examples) unchanged and ensure any translated
warnings/messages use proper English phrasing and repository-approved
terminology.
---
Duplicate comments:
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`:
- Around line 273-280: The catch block after
tagMgr.createTagsFromAPICreateMessage(...) and event.setInventory(...) should
not convert a successful persistence into an API failure; instead either
downgrade the failure to a logged warning/alert or perform an explicit rollback
of the created SdnController. Replace the current event.setError(...) behavior
in the catch with one of: (A) log the full exception (logger.warn/error) and
leave event as success (do not call event.setError), or (B) call a rollback
helper (e.g. remove the persisted SdnController via dbf and stop
pingTracker.track for the SdnControllerVO uuid) then set event error. Update the
catch around tagMgr.createTagsFromAPICreateMessage and event.setInventory to
implement one of these approaches and ensure references to
doCreateSdnController(), pingTracker.track(), SdnControllerVO,
tagMgr.createTagsFromAPICreateMessage, and event.setInventory are handled
accordingly.
---
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java`:
- Around line 4183-4184: 在 KVMHost 中直接使用字面量 "br-int" 和 "openvswitch"（在
to.setBridgeName(...) 和 to.setBridgePortType(...) 处）属于魔法字符串，建议在统一常量类（例如 新增或扩充
KVMConstant 或专用常量类）中定义如 ZNS_BRIDGE_NAME 和 ZNS_BRIDGE_PORT_TYPE 常量并在此处引用；修改步骤：在
KVMConstant 添加 public static final String ZNS_BRIDGE_NAME = "br-int" 和
ZNS_BRIDGE_PORT_TYPE = "openvswitch"，然后将 KVMHost 中的 to.setBridgeName("br-int")
改为 to.setBridgeName(KVMConstant.ZNS_BRIDGE_NAME) 及
to.setBridgePortType(KVMConstant.ZNS_BRIDGE_PORT_TYPE)，并搜索/替换仓库中相同字面量以统一引用。
🪄 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: 1932a2d1-98e8-47a3-a42d-ce7ba339628d
⛔ Files ignored due to path filters (1)
conf/springConfigXml/sdnController.xmlis excluded by!**/*.xml
📒 Files selected for processing (8)
compute/src/main/java/org/zstack/compute/vm/VmAllocateNicIpFlow.javacompute/src/main/java/org/zstack/compute/vm/VmSystemTags.javacore/src/main/java/org/zstack/core/rest/webhook/WebhookCallbackClient.javadocs/modules/network/pages/networkResource/ZnsIntegration.adocheader/src/main/java/org/zstack/header/vm/AfterAllocateVmNicIpExtensionPoint.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/org/zstack/core/rest/webhook/WebhookCallbackClient.java
Comment on lines
+193
to
+194
| callAfterAllocateVmNicIpExtensions(spec, trigger); | ||
| } |
There was a problem hiding this comment.
不要把全量 spec 直接交给 AfterAllocateVmNicIpExtensionPoint。
当前 flow 只会给进入 firstL3s 的 NIC 回填 ip/UsedIpVO；enableIpAddressAllocation() == false 的 L3 会把未更新的 NIC 留在 spec.getDestNics() 里。这里把整个 spec 传给 after-hook 后，SdnControllerManagerImpl.afterAllocateVmNicIp() 这类实现会同时看到“已分配 IP”和“未分配 IP”的 NIC，混合/no-IPAM 场景很容易把未就绪 NIC 一起拿去建 port。建议改为传递本次真正完成 IP 准备的 NIC 集合，或至少先按 nicsWithIp 过滤后再触发扩展。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/main/java/org/zstack/compute/vm/VmAllocateNicIpFlow.java` around
lines 193 - 194, The flow currently passes the entire spec into
callAfterAllocateVmNicIpExtensions(spec, trigger), which exposes both
IP-assigned and non-assigned NICs to AfterAllocateVmNicIpExtensionPoint
implementations (e.g., SdnControllerManagerImpl.afterAllocateVmNicIp); change
the call to only include the NICs that actually received IPs (the nicsWithIp /
firstL3s result) by constructing a filtered spec (or a minimal wrapper) that
contains only those destNics and their ip/UsedIpVO entries, then invoke
callAfterAllocateVmNicIpExtensions(filteredSpec, trigger) so extensions only see
NICs with prepared IPs.
Comment on lines
+464
to
+468
| // VM lifecycle hooks (PreVmInstantiateResourceExtensionPoint, VmReleaseResourceExtensionPoint, | ||
| // InstantiateResourceOnAttachingNicExtensionPoint, ReleaseNetworkServiceOnDetachingNicExtensionPoint) | ||
| // have been removed. SDN port creation/deletion is now unified through VmNic lifecycle hooks: | ||
| // - beforeAllocateVmNic (BeforeAllocateVmNicExtensionPoint) for port creation | ||
| // - afterReleaseVmNic (AfterReleaseVmNicExtensionPoint) for port deletion |
There was a problem hiding this comment.
这里的生命周期注释还停留在旧钩子。
代码已经切到 AfterAllocateVmNicIpExtensionPoint，但注释仍写 beforeAllocateVmNic 负责 port 创建，后续排查 VM NIC 生命周期时会被误导。
📝 建议同步更新注释
- // - beforeAllocateVmNic (BeforeAllocateVmNicExtensionPoint) for port creation
+ // - afterAllocateVmNicIp (AfterAllocateVmNicIpExtensionPoint) for port creationAs per coding guidelines: "对于较长的注释，需要仔细校对并随代码更新，确保内容正确"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`
around lines 464 - 468, 注释描述的 VM NIC 生命周期钩子已过时：当前代码已改为在端口创建时使用
AfterAllocateVmNicIpExtensionPoint（不是 beforeAllocateVmNic /
BeforeAllocateVmNicExtensionPoint），端口删除仍应对应
AfterReleaseVmNic/AfterReleaseVmNicExtensionPoint；请更新该注释段（包含提到的函数/钩子名
PreVmInstantiateResourceExtensionPoint, VmReleaseResourceExtensionPoint,
InstantiateResourceOnAttachingNicExtensionPoint,
ReleaseNetworkServiceOnDetachingNicExtensionPoint, beforeAllocateVmNic,
BeforeAllocateVmNicExtensionPoint）以反映实际使用的 AfterAllocateVmNicIpExtensionPoint 和
AfterReleaseVmNic(ExtensionPoint)；保持表述简洁，避免误导后续排查。
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
2 participants
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.
Resolves: ZCF-13651
Change-Id: I696f6a6a6e6d7867746e70736d6d646861686166
sync from gitlab !9471