<fix>[rest]: fix bug<description>#3611
Open
zstack-robot-2 wants to merge 1 commit intofeature-5.5.12-znsfrom
Open
<fix>[rest]: fix bug<description>#3611zstack-robot-2 wants to merge 1 commit intofeature-5.5.12-znsfrom
zstack-robot-2 wants to merge 1 commit intofeature-5.5.12-znsfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough此PR重构了虚拟机NIC生命周期的SDN控制器集成，将多个细粒度的VM扩展点统一为 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 unit tests (beta)
Comment |
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
plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java
Show resolved
Hide resolved
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
compute/src/main/java/org/zstack/compute/vm/VmAllocateNicIpFlow.java
Outdated
Show resolved
Hide resolved
plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 673-678: The error messages in SdnControllerManagerImpl that use
operr(ORG_ZSTACK_SDNCONTROLLER_10006, ...) are grammatically incorrect; locate
the call that checks controllerUuid (the block referencing
L2NetworkSystemTags.L2_NETWORK_SDN_CONTROLLER_UUID and variable controllerUuid)
and the similar second occurrence later in the same class, and change the
message string from "sdn l2 network[uuid:%s] is not attached controller" (and
the other misspelled variant) to a correct English phrase such as "sdn l2
network[uuid:%s] is not attached to a controller" or "sdn l2 network[uuid:%s]
has no attached controller" so both completion.fail(...) calls emit proper,
typo-free messages.
🪄 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: 3bf2b97a-5568-43bb-9723-7cffa1436de6
⛔ Files ignored due to path filters (1)
conf/springConfigXml/sdnController.xmlis excluded by!**/*.xml
📒 Files selected for processing (2)
compute/src/main/java/org/zstack/compute/vm/VmAllocateNicIpFlow.javaplugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java
plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
compute/src/main/java/org/zstack/compute/vm/VmDetachNicFlow.java (1)
120-146:callReleaseSdnNics方法与VmReturnReleaseNicFlow中的实现重复。此方法与
VmReturnReleaseNicFlow.callReleaseSdnNics()逻辑几乎完全一致。建议考虑将其抽取到公共工具类（如VmNicHelper或新建SdnNicHelper）中复用。另外，
VmReturnReleaseNicFlow中有nics.isEmpty()的检查（line 119），而此处缺少该检查。虽然当前调用方传入的是List.of(nic)不会为空，但为保持一致性建议补充。♻️ 建议的一致性修复
private void callReleaseSdnNics(List<VmNicInventory> nics, Completion completion) { List<AfterAllocateSdnNicExtensionPoint> exts = pluginRgty.getExtensionList(AfterAllocateSdnNicExtensionPoint.class); - if (exts.isEmpty()) { + if (exts.isEmpty() || nics.isEmpty()) { completion.success(); return; }🤖 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/VmDetachNicFlow.java` around lines 120 - 146, The callReleaseSdnNics implementation in VmDetachNicFlow duplicates the logic in VmReturnReleaseNicFlow.callReleaseSdnNics; extract the common logic into a shared helper (e.g., add a static method releaseSdnNics(List<VmNicInventory>, PluginRegistry/PluginRgty, Completion) in a new VmNicHelper or SdnNicHelper) and have both VmDetachNicFlow.callReleaseSdnNics and VmReturnReleaseNicFlow.callReleaseSdnNics delegate to that helper; also add the missing early-return empty-list check (nics.isEmpty()) in the helper to match VmReturnReleaseNicFlow behavior so callers don’t need to guard against empty lists.plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java (2)
272-279: 异常处理改进后仍存在用户体验问题。当
tagMgr.createTagsFromAPICreateMessage或dbf.findByUuid抛出异常时，代码会记录警告并继续发布 event，但此时event.setInventory()未被调用。这意味着 API 响应成功但没有返回 inventory 数据，可能导致用户困惑。建议在 catch 块内尝试单独加载 inventory，或在 finally 块中确保 inventory 被设置：
♻️ 建议的改进方案
if (reply.isSuccess()) { try { tagMgr.createTagsFromAPICreateMessage(msg, vo.getUuid(), SdnControllerVO.class.getSimpleName()); + } catch (Exception e) { + logger.warn(String.format("failed to create tags for SdnControllerVO[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); + // Still try to return minimal inventory info } }🤖 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 272 - 279, The catch block currently swallows exceptions from tagMgr.createTagsFromAPICreateMessage or dbf.findByUuid and leaves event.inventory unset; modify the catch to separately attempt to load and set the inventory by calling dbf.findByUuid(vo.getUuid(), SdnControllerVO.class) and event.setInventory(SdnControllerInventory.valueOf(...)) even if tag creation failed, and only if that second attempt also fails log a warning via logger.warn with the error; ensure event.setInventory is always invoked (or explicitly set to null/empty sentinel) before publishing the event so the API response consistently contains inventory state.
655-761: 三个方法中存在大量重复的 NIC 过滤逻辑。
afterAllocateSdnNic、rollbackSdnNic和releaseSdnNics三个方法中的 NIC 过滤逻辑（L3VO 查询 → L2VO 查询 →shouldSkipSdnForNic检查 → controllerUuid 获取）几乎完全相同。建议抽取公共方法来减少重复代码：
♻️ 建议抽取公共方法
/** * Groups NICs by their SDN controller UUID, filtering out NICs that: * - Have no resolvable L3/L2 network * - Belong to non-SDN L2 networks * - (optionally) Have no controller UUID attached * * `@param` nics the NICs to filter and group * `@param` failOnMissingController if true, returns null when a non-skipped L2 has no controller * `@param` missingControllerError output parameter for error when failOnMissingController is true * `@return` map of controllerUuid -> NICs, or null if failOnMissingController and controller missing */ private Map<String, List<VmNicInventory>> groupNicsBySdnController( List<VmNicInventory> nics, boolean failOnMissingController, ErrorCode[] missingControllerError) { Map<String, List<VmNicInventory>> nicMaps = new HashMap<>(); for (VmNicInventory nic : nics) { L3NetworkVO l3Vo = dbf.findByUuid(nic.getL3NetworkUuid(), L3NetworkVO.class); if (l3Vo == null) continue; L2NetworkVO l2VO = dbf.findByUuid(l3Vo.getL2NetworkUuid(), L2NetworkVO.class); if (l2VO == null || shouldSkipSdnForNic(l2VO)) continue; String controllerUuid = L2NetworkSystemTags.L2_NETWORK_SDN_CONTROLLER_UUID .getTokenByResourceUuid(l2VO.getUuid(), L2NetworkSystemTags.L2_NETWORK_SDN_CONTROLLER_UUID_TOKEN); if (controllerUuid == null) { if (failOnMissingController) { missingControllerError[0] = operr(ORG_ZSTACK_SDNCONTROLLER_10006, "sdn l2 network[uuid:%s] has no attached controller", l2VO.getUuid()); return null; } continue; } nicMaps.computeIfAbsent(controllerUuid, k -> new ArrayList<>()).add(nic); } return nicMaps; }🤖 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 655 - 761, The three methods afterAllocateSdnNic, rollbackSdnNic and releaseSdnNics duplicate NIC filtering (dbf.findByUuid L3/L2 lookups → shouldSkipSdnForNic → L2NetworkSystemTags.L2_NETWORK_SDN_CONTROLLER_UUID token fetch); extract a helper like groupNicsBySdnController(List<VmNicInventory> nics, boolean failOnMissingController, ErrorCode[] outError) that returns Map<String,List<VmNicInventory>> (or null when failOnMissingController and a non-skippable L2 has no controller) and uses dbf.findByUuid, shouldSkipSdnForNic and L2NetworkSystemTags.L2_NETWORK_SDN_CONTROLLER_UUID_TOKEN; call it from afterAllocateSdnNic with failOnMissingController=true and if it returns null call completion.fail(outError[0]) (use operr and ORG_ZSTACK_SDNCONTROLLER_10006), and from rollbackSdnNic/releaseSdnNics with failOnMissingController=false and skip null/empty maps, then continue to call sdnAddVmNics or removeLogicalPort as before.
🤖 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 675-678: The error message passed to completion.fail in
SdnControllerManagerImpl (the call using operr with
ORG_ZSTACK_SDNCONTROLLER_10006 and l2VO.getUuid()) has incorrect English ("has
not attached controller"); update the message to correct grammar — e.g. "sdn l2
network[uuid:%s] has no attached controller" or "sdn l2 network[uuid:%s] is not
attached to a controller" — ensuring the operr invocation and parameters
(ORG_ZSTACK_SDNCONTROLLER_10006, l2VO.getUuid()) remain unchanged.
---
Nitpick comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmDetachNicFlow.java`:
- Around line 120-146: The callReleaseSdnNics implementation in VmDetachNicFlow
duplicates the logic in VmReturnReleaseNicFlow.callReleaseSdnNics; extract the
common logic into a shared helper (e.g., add a static method
releaseSdnNics(List<VmNicInventory>, PluginRegistry/PluginRgty, Completion) in a
new VmNicHelper or SdnNicHelper) and have both
VmDetachNicFlow.callReleaseSdnNics and VmReturnReleaseNicFlow.callReleaseSdnNics
delegate to that helper; also add the missing early-return empty-list check
(nics.isEmpty()) in the helper to match VmReturnReleaseNicFlow behavior so
callers don’t need to guard against empty lists.
In
`@plugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java`:
- Around line 272-279: The catch block currently swallows exceptions from
tagMgr.createTagsFromAPICreateMessage or dbf.findByUuid and leaves
event.inventory unset; modify the catch to separately attempt to load and set
the inventory by calling dbf.findByUuid(vo.getUuid(), SdnControllerVO.class) and
event.setInventory(SdnControllerInventory.valueOf(...)) even if tag creation
failed, and only if that second attempt also fails log a warning via logger.warn
with the error; ensure event.setInventory is always invoked (or explicitly set
to null/empty sentinel) before publishing the event so the API response
consistently contains inventory state.
- Around line 655-761: The three methods afterAllocateSdnNic, rollbackSdnNic and
releaseSdnNics duplicate NIC filtering (dbf.findByUuid L3/L2 lookups →
shouldSkipSdnForNic → L2NetworkSystemTags.L2_NETWORK_SDN_CONTROLLER_UUID token
fetch); extract a helper like groupNicsBySdnController(List<VmNicInventory>
nics, boolean failOnMissingController, ErrorCode[] outError) that returns
Map<String,List<VmNicInventory>> (or null when failOnMissingController and a
non-skippable L2 has no controller) and uses dbf.findByUuid, shouldSkipSdnForNic
and L2NetworkSystemTags.L2_NETWORK_SDN_CONTROLLER_UUID_TOKEN; call it from
afterAllocateSdnNic with failOnMissingController=true and if it returns null
call completion.fail(outError[0]) (use operr and
ORG_ZSTACK_SDNCONTROLLER_10006), and from rollbackSdnNic/releaseSdnNics with
failOnMissingController=false and skip null/empty maps, then continue to call
sdnAddVmNics or removeLogicalPort as before.
🪄 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: 13d0cb6f-c2ef-45d1-abab-9384747fd663
⛔ Files ignored due to path filters (2)
conf/springConfigXml/VmInstanceManager.xmlis excluded by!**/*.xmlconf/springConfigXml/sdnController.xmlis excluded by!**/*.xml
📒 Files selected for processing (7)
compute/src/main/java/org/zstack/compute/vm/VmAllocateNicFlow.javacompute/src/main/java/org/zstack/compute/vm/VmAllocateSdnNicFlow.javacompute/src/main/java/org/zstack/compute/vm/VmDetachNicFlow.javacompute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javacompute/src/main/java/org/zstack/compute/vm/VmReturnReleaseNicFlow.javaheader/src/main/java/org/zstack/header/vm/AfterAllocateSdnNicExtensionPoint.javaplugin/sdnController/src/main/java/org/zstack/sdnController/SdnControllerManagerImpl.java
Comment on lines
+675
to
+678
| if (controllerUuid == null) { | ||
| completion.fail(operr(ORG_ZSTACK_SDNCONTROLLER_10006, "sdn l2 network[uuid:%s] has not attached controller", l2VO.getUuid())); | ||
| return; | ||
| } |
There was a problem hiding this comment.
错误信息语法仍有问题。
Line 676 的错误信息 "sdn l2 network[uuid:%s] has not attached controller" 语法不正确。"has not attached" 表示"尚未附加"，缺少宾语。应改为 "has no attached controller" 或 "is not attached to a controller"。
As per coding guidelines: "代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写"
📝 建议修复
- completion.fail(operr(ORG_ZSTACK_SDNCONTROLLER_10006, "sdn l2 network[uuid:%s] has not attached controller", l2VO.getUuid()));
+ completion.fail(operr(ORG_ZSTACK_SDNCONTROLLER_10006, "sdn l2 network[uuid:%s] has no attached controller", l2VO.getUuid()));🤖 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 675 - 678, The error message passed to completion.fail in
SdnControllerManagerImpl (the call using operr with
ORG_ZSTACK_SDNCONTROLLER_10006 and l2VO.getUuid()) has incorrect English ("has
not attached controller"); update the message to correct grammar — e.g. "sdn l2
network[uuid:%s] has no attached controller" or "sdn l2 network[uuid:%s] is not
attached to a controller" — ensuring the operr invocation and parameters
(ORG_ZSTACK_SDNCONTROLLER_10006, l2VO.getUuid()) remain unchanged.
Resolves: ZCF-1365 Change-Id: I696f6a6a6e6d7867746e70736d6d646861686166
MatheMatrix
force-pushed
the
sync/shixin.ruan/shixin-ZCF-1365@@2
branch
from
4e77460 to
4b30043
Compare
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