<fix>[network]: ZCF-3703 add ZNS attach skip hook#4007
Open
zstack-robot-2 wants to merge 6 commits into
Open
Conversation
ZCF-3703: allow ZNS to ignore Flat DHCP attach. Change-Id: If8929ee4cc111825b42b688678bca81b3f08eed5
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent 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 (1)
功能概述该 PR 实现了网络服务挂载的扩展点跳过机制，并增强了 DHCP IPv6 分配控制。通过消息标志、拦截器判定与处理器早期返回支持按需跳过网络服务挂载；同时为 provider 与后端引入 DHCPv6 开关，启用按 IP 版本的精细化分配校验。 变更网络服务挂载跳过与 DHCPv6 控制
🎯 4 (Complex) | ⏱️ ~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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
header/src/main/java/org/zstack/header/network/service/NetworkServiceAttachExtensionPoint.java (1)
3-4: ⚡ Quick win请为扩展点方法补充 Javadoc 约定。
Line 4 缺少接口方法说明，调用方和实现方很难统一“返回
true的触发条件/副作用边界”。✏️ 建议修改
public interface NetworkServiceAttachExtensionPoint { + /** + * Decide whether current attach request should be treated as no-op. + * + * `@param` msg attach request message (already normalized by interceptor) + * `@return` true to skip attach workflow and return success event directly + */ boolean skipAttachNetworkService(APIAttachNetworkServiceToL3NetworkMsg msg); }As per coding guidelines "接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。"🤖 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 `@header/src/main/java/org/zstack/header/network/service/NetworkServiceAttachExtensionPoint.java` around lines 3 - 4, 为接口 NetworkServiceAttachExtensionPoint 与其方法 skipAttachNetworkService(APIAttachNetworkServiceToL3NetworkMsg msg) 补充 Javadoc：在接口注释中简短描述该扩展点用途；在方法注释明确说明何时应返回 true（例如：实现方检测到应跳过附加且不会有副作用或已完成所需操作时返回 true）、返回 false 表示继续正常附加流程；声明实现方不得对 msg 做不可预期的可变更更改、应保证幂等性/线程安全（如适用）、任何异常应抛出或记录的约定以及调用方对返回值的期望和后续行为（调用方据此决定是否执行附加逻辑）。
🤖 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
`@network/src/main/java/org/zstack/network/service/NetworkServiceApiInterceptor.java`:
- Around line 42-45: For APIAttachNetworkServiceToL3NetworkMsg, trim
whitespace/newlines from the message's network service strings before performing
provider-type-to-UUID conversion and before calling skipAttachNetworkService:
obtain the values via attachMsg.getNetworkServices(), trim each element (and
ignore empty results), then pass the trimmed list to
convertNetworkProviderTypeToUuid and to the skipAttachNetworkService logic, and
finally use attachMsg.setNetworkServices(...) with the cleaned list so
leading/trailing spaces won't break matching.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/network/service/NetworkServiceAttachExtensionPoint.java`:
- Around line 3-4: 为接口 NetworkServiceAttachExtensionPoint 与其方法
skipAttachNetworkService(APIAttachNetworkServiceToL3NetworkMsg msg) 补充
Javadoc：在接口注释中简短描述该扩展点用途；在方法注释明确说明何时应返回 true（例如：实现方检测到应跳过附加且不会有副作用或已完成所需操作时返回
true）、返回 false 表示继续正常附加流程；声明实现方不得对 msg
做不可预期的可变更更改、应保证幂等性/线程安全（如适用）、任何异常应抛出或记录的约定以及调用方对返回值的期望和后续行为（调用方据此决定是否执行附加逻辑）。
🪄 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: 9d9916c6-6ea5-4014-820c-eacdf4018029
📒 Files selected for processing (4)
header/src/main/java/org/zstack/header/network/service/APIAttachNetworkServiceToL3NetworkMsg.javaheader/src/main/java/org/zstack/header/network/service/NetworkServiceAttachExtensionPoint.javanetwork/src/main/java/org/zstack/network/l3/L3BasicNetwork.javanetwork/src/main/java/org/zstack/network/service/NetworkServiceApiInterceptor.java
MatheMatrix
reviewed
May 18, 2026
| */ | ||
| @APIParam | ||
| private Map<String, List<String>> networkServices; | ||
| private transient boolean skipAttach; |
Owner
There was a problem hiding this comment.
Comment from shixin.ruan:
是不是应该加@APINoSee
Collaborator
There was a problem hiding this comment.
Comment from shixin.ruan:
fixed in 3a61b56.\n\nDefect class: API surface hygiene.\nBefore: skipAttach was transient but still had no explicit API hiding annotation.\nAfter: added @APINoSee to skipAttach so this internal control flag is hidden from REST/API field exposure.\nValidation: mvn -pl header,network -DskipTests install passed.
Normalize attach network service provider keys and service names before provider conversion and skip extension checks. Hide the internal skipAttach flag from API surface. Resolves: ZCF-3703 Change-Id: Id6ff18d0bf5e86937c50a82721249055d3f76ade
MatheMatrix
force-pushed
the
sync/shixin.ruan/shixin-ZCF-3703@@2
branch
from
3a61b56 to
fd6da83
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
`@plugin/flatNetworkProvider/src/main/java/org/zstack/network/service/flat/FlatDhcpBackend.java`:
- Around line 997-1000: The method allocateDhcpIp currently returns null when
isAllocateDhcpServerIp(l3Uuid, ipVersion) is false which silently treats
“capability not supported” as a normal allocation failure; change this to
fail-fast by throwing a clear exception (e.g., an
OperationFailureException/CloudRuntimeException) when DHCP server IP allocation
is not supported for the given l3Uuid/ipVersion so callers like
afterAddIpRange() and FlatDhcpAcquireDhcpServerIpMsg will treat it as an
explicit error; keep isAllocateDhcpServerIp check but replace the null return
with a descriptive exception message stating DHCPv6 allocation is unsupported
for that L3 and IP version.
🪄 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: 9cf2c686-15c4-410f-96d8-968744a04020
📒 Files selected for processing (4)
header/src/main/java/org/zstack/header/network/service/APIAttachNetworkServiceToL3NetworkMsg.javaheader/src/main/java/org/zstack/header/network/service/NetworkServiceProviderType.javanetwork/src/main/java/org/zstack/network/service/NetworkServiceApiInterceptor.javaplugin/flatNetworkProvider/src/main/java/org/zstack/network/service/flat/FlatDhcpBackend.java
🚧 Files skipped from review as they are similar to previous changes (2)
- header/src/main/java/org/zstack/header/network/service/APIAttachNetworkServiceToL3NetworkMsg.java
- network/src/main/java/org/zstack/network/service/NetworkServiceApiInterceptor.java
Comment on lines
997
to
1000
| @Deferred | ||
| private String allocateDhcpIp(String l3Uuid, int ipVersion, boolean allocate_ip, String requiredIp, String excludedIp) { | ||
| if (!isAllocateDhcpServerIp(l3Uuid)) { | ||
| if (!isAllocateDhcpServerIp(l3Uuid, ipVersion)) { | ||
| return null; |
There was a problem hiding this comment.
不要用 null 静默表示“不支持 DHCPv6 分配”。
这里把“不支持当前 IP 版本分配”折叠成 null 返回后，afterAddIpRange()、FlatDhcpAcquireDhcpServerIpMsg 等现有调用方不会把它当成错误处理。这样一来，非 SLAAC 的 IPv6 range 可能成功走完整个流程，但 DHCPv6 server IP/tag 根本没有落下来，L3 会进入部分配置状态。建议把 capability 不支持与普通分配失败区分开，并在 attach / add-range 这类入口上显式报错。
🤖 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/flatNetworkProvider/src/main/java/org/zstack/network/service/flat/FlatDhcpBackend.java`
around lines 997 - 1000, The method allocateDhcpIp currently returns null when
isAllocateDhcpServerIp(l3Uuid, ipVersion) is false which silently treats
“capability not supported” as a normal allocation failure; change this to
fail-fast by throwing a clear exception (e.g., an
OperationFailureException/CloudRuntimeException) when DHCP server IP allocation
is not supported for the given l3Uuid/ipVersion so callers like
afterAddIpRange() and FlatDhcpAcquireDhcpServerIpMsg will treat it as an
explicit error; keep isAllocateDhcpServerIp check but replace the null return
with a descriptive exception message stating DHCPv6 allocation is unsupported
for that L3 and IP version.
added 2 commits
May 18, 2026 16:57
Resolves: ZCF-3703 Change-Id: I8b002e1c0b7e0535df0803a8107e9b4ebbdadf21
Split ZNS DHCP error codes used by premium DHCP paths.\nAdd i18n mappings for the new concrete error templates. Resolves: ZCF-3703 Change-Id: I11bf0fc6df8cf5a5e1ae24af958c5ca3712b4347
There was a problem hiding this comment.
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/network/l3/SdnControllerEnableDHCPMsg.java (1)
30-35: ⚡ Quick win建议对
systemTags做防御性拷贝，避免消息负载被外部可变引用污染。当前 getter/setter 直接暴露并持有同一
List引用，后续外部修改可能影响已构造的消息内容。♻️ 建议修改
public List<String> getSystemTags() { - return systemTags; + return systemTags == null ? null : new java.util.ArrayList<>(systemTags); } public void setSystemTags(List<String> systemTags) { - this.systemTags = systemTags; + this.systemTags = systemTags == null ? null : new java.util.ArrayList<>(systemTags); }🤖 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 `@header/src/main/java/org/zstack/header/network/l3/SdnControllerEnableDHCPMsg.java` around lines 30 - 35, The getter and setter for the field systemTags in SdnControllerEnableDHCPMsg expose the internal List reference; change setSystemTags(List<String>) to defensively copy the incoming list (e.g. this.systemTags = incoming == null ? null : new ArrayList<>(incoming)) and change getSystemTags() to return either an unmodifiable copy or a defensive copy (e.g. return systemTags == null ? null : Collections.unmodifiableList(systemTags) or new ArrayList<>(systemTags)), and ensure null-safety in both methods so external mutations cannot affect the message's internal state.utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java (1)
12127-12132: ⚡ Quick win统一错误码注释的首字母大写。
第 12127 行和第 12132 行的注释以小写 "failed" 开头，而第 12128-12131 行以大写 "ZNS" 开头。这与上方已有注释（第 12124-12126 行均以大写开头）不一致，也造成新增注释内部风格不统一。建议将第 12127 和 12132 行改为大写开头以保持一致性。
♻️ 建议的修复
- // 10043 failed to allocate DHCPv4 server IP in ZNS DHCP helper + // 10043 Failed to allocate DHCPv4 server IP in ZNS DHCP helper // 10044 ZNS DHCP enable has no eligible IpRange // 10045 ZNS DHCP enable has no Cloud-reserved DHCPv4 server IP // 10046 ZNS DHCP 409 fallback GET returned empty // 10047 ZNS DHCP update has no Cloud-reserved DHCPv4 server IP - // 10048 failed to allocate DHCPv4 server IP in ZNS DHCP backend + // 10048 Failed to allocate DHCPv4 server IP in ZNS DHCP backend🤖 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 `@utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java` around lines 12127 - 12132, The two comment lines in CloudOperationsErrorCode.java that start with "failed to allocate DHCPv4 server IP in ZNS DHCP helper" and "failed to allocate DHCPv4 server IP in ZNS DHCP backend" should have their initial word capitalized to match surrounding comments; update those strings to start with "Failed" so all error-code comment entries (including the existing "ZNS ..." lines) use a consistent initial capital letter.
🤖 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
`@header/src/main/java/org/zstack/header/network/l3/SdnControllerEnableDHCPMsg.java`:
- Around line 30-35: The getter and setter for the field systemTags in
SdnControllerEnableDHCPMsg expose the internal List reference; change
setSystemTags(List<String>) to defensively copy the incoming list (e.g.
this.systemTags = incoming == null ? null : new ArrayList<>(incoming)) and
change getSystemTags() to return either an unmodifiable copy or a defensive copy
(e.g. return systemTags == null ? null :
Collections.unmodifiableList(systemTags) or new ArrayList<>(systemTags)), and
ensure null-safety in both methods so external mutations cannot affect the
message's internal state.
In
`@utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java`:
- Around line 12127-12132: The two comment lines in
CloudOperationsErrorCode.java that start with "failed to allocate DHCPv4 server
IP in ZNS DHCP helper" and "failed to allocate DHCPv4 server IP in ZNS DHCP
backend" should have their initial word capitalized to match surrounding
comments; update those strings to start with "Failed" so all error-code comment
entries (including the existing "ZNS ..." lines) use a consistent initial
capital letter.
ℹ️ 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: 599e46e6-3dcd-428b-baf1-0a44a02765b9
⛔ Files ignored due to path filters (2)
conf/i18n/globalErrorCodeMapping/global-error-en_US.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_CN.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
header/src/main/java/org/zstack/header/network/l3/SdnControllerEnableDHCPMsg.javanetwork/src/main/java/org/zstack/network/service/DhcpExtension.javautils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
JIRA: ZCF-3703 Remove the duplicate systemTags field from SdnControllerEnableDHCPMsg. NeedReplyMessage already owns systemTags, so redeclaring it makes Gson fail while CloudBus logs the message before sending it to the ZNS DHCP handler. Change-Id: I4546348c9fd45004e0f9b9c823f9cccec9ad4774
Avoid NPE when MtuGetter is constructed outside Spring. Resolves: ZCF-3703 Change-Id: I21a88fca05ab157a767d569435c2d37a48d4179b
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
5 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.
Summary
ZCF-3703: add a generic attach skip extension so ZNS L3 can accept legacy Flat DHCP attach API as a no-op.
Changes
Testing
Related Jira: ZCF-3703
sync from gitlab !9900