fix(ZCF-4049): split attachable L3 domain filters#4125
Closed
zstack-robot-1 wants to merge 3 commits into
Closed
Conversation
Split attachable L3 domain filtering by SDN vendor. Keep non-domain filters as common AND constraints. Resolves: ZCF-4049 Change-Id: I417ca4abbe6dfdd5cabea67edcbcace4f2681acd
|
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)
Walkthrough新增域级扩展点并在 VmInstanceBase 中实现集中过滤：先按 SDN controller vendorType 将候选 L3 分组并应用对应域过滤器，再执行其它扩展点过滤；两个相关 API 改为调用该统一入口。 ChangesL3 网络域级过滤重构
sequenceDiagram
participant API_GetAttachable as APIGetVmAttachableL3Network handler
participant API_GetCandidate as APIGetCandidateL3NetworksForChangeVmNicNetworkMsg handler
participant FilterEntry as filterAttachableL3Network
participant DomainFilter as VmAttachableL3NetworkDomainFilterExtensionPoint
participant VendorLookup as getSdnControllerVendorTypeByL3Uuid
API_GetAttachable->>FilterEntry: 调用 filterAttachableL3Network(vm, l3Invs)
API_GetCandidate->>FilterEntry: 调用 filterAttachableL3Network(vm, l3Invs)
FilterEntry->>VendorLookup: 批量解析每个 L3 的 vendorType
FilterEntry->>DomainFilter: 按 vendorType 将 L3 分组并调用 filterAttachableL3NetworkInDomain
FilterEntry->>FilterEntry: 跳过实现 DomainFilter 的扩展点，继续调用其它 FilterAttachableL3NetworkExtensionPoint
FilterEntry->>API_GetAttachable: 返回最终过滤结果
🎯 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)
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.42.3)compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javaComment |
![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/vm/VmAttachableL3NetworkDomainFilterExtensionPoint.java (1)
8-10: ⚡ Quick win请为接口方法补充 Javadoc 以满足规范要求。
这两个扩展点方法当前没有接口级说明，后续实现方难以统一语义边界（如入参约束、返回值是否允许
null、过滤语义是“包含/排除”）。建议在方法声明处补充简洁且可执行的 Javadoc。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/vm/VmAttachableL3NetworkDomainFilterExtensionPoint.java` around lines 8 - 10, 为接口方法补充明确的 Javadoc：在 VmAttachableL3NetworkDomainFilterExtensionPoint 接口的两个方法 getSdnControllerVendorType() 和 filterAttachableL3NetworkInDomain(VmInstanceInventory vm, List<L3NetworkInventory> l3s) 上分别添加简洁说明（方法用途/语义），对入参 vm 和 l3s 做约束说明（是否允许为 null、是否会被修改），声明返回值语义和 nullability（例如返回不可为 null 的过滤后列表，或者允许返回 null 并说明含义），明确过滤行为是“包含哪些可附加的 L3Network”还是“排除哪些不可附加的 L3Network”，并说明抛出的受检/运行时异常（若有）；注意仍不要添加方法修饰符（如 public）。
🤖 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 `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java`:
- Around line 5161-5194: The loop calls getSdnControllerVendorType(l3.getUuid())
for each L3 and causes N+1 DB queries; fix by precomputing a l3Uuid->vendorType
map before grouping: collect all L3 UUIDs, map each to its controller UUID via
L3NetworkHelper.getSdnControllerUuidFromL3Uuid, collect non-null controller
UUIDs, batch-query SdnControllerVO (select uuid,vendorType with
Q.New(...).in(...).list()) to build controllerUuid->vendorType, then produce
l3Uuid->vendorType by looking up each L3's controller UUID (falling back to
null), and use that map instead of calling getSdnControllerVendorType inside the
loop (or alternatively implement a method-level cache used by
getSdnControllerVendorType to avoid DB hits).
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/vm/VmAttachableL3NetworkDomainFilterExtensionPoint.java`:
- Around line 8-10: 为接口方法补充明确的 Javadoc：在
VmAttachableL3NetworkDomainFilterExtensionPoint 接口的两个方法
getSdnControllerVendorType() 和
filterAttachableL3NetworkInDomain(VmInstanceInventory vm,
List<L3NetworkInventory> l3s) 上分别添加简洁说明（方法用途/语义），对入参 vm 和 l3s 做约束说明（是否允许为
null、是否会被修改），声明返回值语义和 nullability（例如返回不可为 null 的过滤后列表，或者允许返回 null
并说明含义），明确过滤行为是“包含哪些可附加的 L3Network”还是“排除哪些不可附加的
L3Network”，并说明抛出的受检/运行时异常（若有）；注意仍不要添加方法修饰符（如 public）。
🪄 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: 7f82b004-8a6f-4b1f-bf4d-2f1e7d744db5
📒 Files selected for processing (2)
compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javaheader/src/main/java/org/zstack/header/vm/VmAttachableL3NetworkDomainFilterExtensionPoint.java
Batch SDN vendor lookup for attachable L3 filters. Document the domain filter extension contract. Resolves: ZCF-4049 Change-Id: Iaaa3e66a1be47840999965330cc4efbb4c96f3b1
| return Q.New(SdnControllerVO.class) | ||
| .select(SdnControllerVO_.vendorType) | ||
| .eq(SdnControllerVO_.uuid, controllerUuid) | ||
| .findValue(); |
Collaborator
There was a problem hiding this comment.
Comment from shixin.ruan:
已处理。
- 将 per-L3
getSdnControllerVendorType()改为本次过滤内批量构造l3Uuid -> vendorType：先批量查询 L3/L2，再批量读取 L2 SDN controller system tag，最后批量查询 controller vendor type。 - 保持原有 VPC/ZNS/OVN domain OR 语义不变，EIP/common filter 仍在后续 AND 阶段执行。
- 顺手补充了新 extension point 的接口 Javadoc。
验证：mvn -pl compute -am -DskipTests compile 通过；远端 Codex review 未发现新的 correctness issue。
Commit: cc254e16d1
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 `@compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java`:
- Around line 5165-5169: The code groups L3 networks by vendor using
vendorTypeByL3Uuid.get(...) and treats a missing mapping as null, which mixes
SDN L3s with unresolved vendor into the default (non-SDN) bucket; change the
grouping logic in VmInstanceBase (the loop that builds l3sByVendor using
getSdnControllerVendorTypeByL3Uuid) to distinguish three cases: (1) non-SDN L3s
(no SdnControllerUuid tag) => keep using null/defaultFilters, (2) SDN L3s with a
resolved vendorType => use that vendorType as the key, and (3) SDN L3s with an
unresolved vendor record or empty vendorType => use an explicit sentinel key
(e.g., "UNRESOLVED_SDN_VENDOR") so they remain a separate group and are
forwarded to SDN handling; apply the same change to the similar grouping code
around the other occurrence (lines ~5249-5259) so unresolved vendorType is not
mixed into defaultFilters.
🪄 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: cd2345fa-0dde-4099-b134-e24c1f32a578
📒 Files selected for processing (2)
compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javaheader/src/main/java/org/zstack/header/vm/VmAttachableL3NetworkDomainFilterExtensionPoint.java
🚧 Files skipped from review as they are similar to previous changes (1)
- header/src/main/java/org/zstack/header/vm/VmAttachableL3NetworkDomainFilterExtensionPoint.java
Do not route unresolved SDN vendors to default filters. Resolves: ZCF-4049 Change-Id: I257433e821925538f2a1ad0b68788a20a020bd17
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
4 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.
修复说明
修复无网卡 VM 加载网卡时 ZNS VPC L3 不可选的问题。
变更内容
验证
sync from gitlab !10024