[ZCF-2893] VM NIC lifecycle extension point abstraction#3807
Open
zstack-robot-2 wants to merge 8 commits intofeature-5.5.12-znsfrom
Open
[ZCF-2893] VM NIC lifecycle extension point abstraction#3807zstack-robot-2 wants to merge 8 commits intofeature-5.5.12-znsfrom
zstack-robot-2 wants to merge 8 commits 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新增 VM 网卡生命周期框架：添加扩展点接口与默认实现契约、全局配置、生命周期管理器、KVM 心跳桥接实现，以及大量单元与集成测试以协调网卡的创建、清理、迁移、失败处理与主机端对齐（reconcile）。 Changes
Sequence Diagram(s)sequenceDiagram
participant Spec as VmInstanceSpec
participant Mgr as VmNicLifecycleManager
participant DB as Database
participant Ext as VmNicLifecycleExtension
participant Host as KVM Host
Note over Spec,Mgr: preInstantiate 流程（启动/迁移前）
Spec->>Mgr: preInstantiateVmResource(spec, completion)
activate Mgr
Mgr->>DB: 查询 VM lastHostUuid / NICs
DB-->>Mgr: 返回 VM 与 NIC 列表
alt lastHost != destHost 且为 Start
Mgr->>Ext: cleanupStaleResource(lastHost, matchedNics, NoErrorCompletion)
Ext-->>Mgr: done
end
Mgr->>Ext: setupOnHost(destHost, matchedNics, Completion) (fail-fast)
Ext-->>Mgr: success / fail
Mgr-->>Spec: completion.success()/fail()
deactivate Mgr
sequenceDiagram
participant Kvm as KVM 主机
participant Bridge as VmNicLifecycleKvmBridge
participant DB as Database
participant Ext as VmNicLifecycleExtension
participant Thd as ThreadFacade
Note over Kvm,Bridge: kvmPingAgentNoFailure（reconcile）
Kvm->>Bridge: kvmPingAgentNoFailure(host, NoErrorCompletion)
activate Bridge
Bridge->>DB: 查询该主机上 Running VM 的 NICs
DB-->>Bridge: 返回预期 NIC 列表
loop 对每个注册的扩展
Bridge->>Ext: ext.isApplicable(filter) -> matchedNics
par 调用与超时
Ext->>Ext: ext.reconcileOnHost(host, matchedNics, NoErrorCompletion)
Thd->>Bridge: 超时回调（记录并完成该扩展步）
end
Ext-->>Bridge: done / 异常（记录并继续）
end
Bridge-->>Kvm: NoErrorCompletion.done()
deactivate Bridge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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.1)plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@header/src/main/java/org/zstack/header/vm/VmNicLifecycleExtensionPoint.java`:
- Around line 18-33: Replace the Chinese Javadoc comments on the
VmNicLifecycleExtensionPoint interface with English descriptions: update the
comment for isApplicable(VmNicInventory nic) to explain it's a filter that
returns false for NICs to be skipped by the Manager; update setupOnHost(String
hostUuid, List<VmNicInventory> nics, Completion completion) to state it
establishes resources on the specified host for a batch of NICs and that Manager
calls it serially with fail-fast semantics; and update cleanupFromHost(String
hostUuid, List<VmNicInventory> nics, NoErrorCompletion completion) to state it
cleans up resources on the specified host for a batch of NICs and that Manager
calls it serially with fail-logged semantics (individual failures are logged and
do not block subsequent cleanup).
- Around line 35-80: The Java interface default method Javadoc contains Chinese
text; replace those comments with English equivalents for all affected methods
(preMigrate, postMigrate, failedMigrate, cleanupStaleResource, reconcileOnHost
and any surrounding Javadocs) so that the documentation is fully English —
update each Javadoc block to convey the same semantics in English (e.g., “Before
migration: pre-create resources on destination host; defaults to
setupOnHost(destHost)”, “After successful migration: clean up resources on
source host; failures must be logged and not propagated”, “On migration failure:
clean up resources on destination host; defaults to cleanupFromHost(destHost)”,
“Clean up stale resources remaining on lastHost; defaults to
cleanupFromHost(lastHost)”, and “Host heartbeat reconciliation: compare actual
host state with expectedNics and fix drift; default no-op; manager calls with
concurrency and per-item timeout”) while preserving method signatures and
behavior.
- Around line 8-15: Replace all Chinese documentation comments in the
VmNicLifecycleExtensionPoint interface with clear, idiomatic English;
specifically, reword the class/interface-level Javadoc so it explains that this
is a unified abstraction for VM NIC lifecycle of host-bound resources (e.g., OVS
DPDK ports, security group rules), that implementers should describe how to
create/delete/reconcile NIC resources on a host, and that VmNicLifecycleManager
will route VM start/stop/migration/HA/host-reconnect lifecycle events to this
interface; ensure the Javadoc references the same design doc
(docs/plans/vmnic-lifecycle-resource-abstraction-func-spec.md F-001) and remove
all Chinese text while preserving meaning and tags like {`@code`
VmNicLifecycleManager}.
🪄 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: 2e8a7418-31fd-4404-94cd-2f75a2b4e3db
⛔ Files ignored due to path filters (3)
conf/globalConfig/vmNicLifecycle.xmlis excluded by!**/*.xmlconf/springConfigXml/Kvm.xmlis excluded by!**/*.xmlconf/springConfigXml/VmInstanceManager.xmlis excluded by!**/*.xml
📒 Files selected for processing (9)
compute/src/main/java/org/zstack/compute/vm/VmNicLifecycleGlobalConfig.javacompute/src/main/java/org/zstack/compute/vm/VmNicLifecycleManager.javaheader/src/main/java/org/zstack/header/vm/VmNicLifecycleExtensionPoint.javaplugin/kvm/src/main/java/org/zstack/kvm/VmNicLifecycleKvmBridge.javatest/src/test/java/org/zstack/test/compute/vmnic/MockNicLifecycle.javatest/src/test/java/org/zstack/test/compute/vmnic/VmNicLifecycleExtensionPointDefaultTest.javatest/src/test/java/org/zstack/test/compute/vmnic/VmNicLifecycleKvmBridgeTest.javatest/src/test/java/org/zstack/test/compute/vmnic/VmNicLifecycleManagerDispatchTest.javautils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/src/test/java/org/zstack/test/compute/vmnic/TestVmNicLifecycleExtension.java (1)
46-46: 建议不要直接暴露可变内部状态
getCalls()直接返回内部calls，调用方可修改该列表并影响后续断言。建议返回只读视图或快照，降低测试间耦合风险。可选修改示例
- public List<Call> getCalls() { return calls; } + public List<Call> getCalls() { return List.copyOf(calls); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/java/org/zstack/test/compute/vmnic/TestVmNicLifecycleExtension.java` at line 46, 方法 getCalls 直接返回可变字段 calls，导致外部可以修改内部状态并影响后续断言；请在 getCalls 中返回不可变视图或快照（例如 Collections.unmodifiableList(calls) 或 new ArrayList<>(calls)），以保护内部 lists 的不可变性，修改涉及的方法名 getCalls 和字段 calls 即可完成修复。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@test/src/test/java/org/zstack/test/compute/vmnic/TestVmNicLifecycleExtension.java`:
- Line 46: 方法 getCalls 直接返回可变字段 calls，导致外部可以修改内部状态并影响后续断言；请在 getCalls
中返回不可变视图或快照（例如 Collections.unmodifiableList(calls) 或 new
ArrayList<>(calls)），以保护内部 lists 的不可变性，修改涉及的方法名 getCalls 和字段 calls 即可完成修复。
ℹ️ 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: fbdc9789-078d-4b44-9bc1-f2a4b46d499c
⛔ Files ignored due to path filters (1)
test/src/test/resources/springConfigXml/VmNicLifecycleExtension.xmlis excluded by!**/*.xml
📒 Files selected for processing (4)
test/src/test/groovy/org/zstack/test/integration/kvm/vmnic/VmNicLifecycleCoexistenceCase.groovytest/src/test/groovy/org/zstack/test/integration/kvm/vmnic/VmNicLifecycleMigrateCase.groovytest/src/test/groovy/org/zstack/test/integration/kvm/vmnic/VmNicLifecycleStartStopCase.groovytest/src/test/java/org/zstack/test/compute/vmnic/TestVmNicLifecycleExtension.java
Why - VM/VMNic lifecycle extension points are scattered across 6+ interfaces (Pre/Release/Migrate/NIC attach-detach/Host heartbeat); every new OvsKvmBackend-style NIC-bound feature duplicates ~850 lines of boilerplate across those extension points. What (Phase 1 foundation only) - New VmNicLifecycleExtensionPoint interface (header/vm) with 7 methods unifying setup/cleanup/migrate/reconcile semantics. - New VmNicLifecycleManager skeleton (compute/vm) implementing 5 legacy extension points; methods are pass-through success()/done() in PH-1. - New VmNicLifecycleKvmBridge skeleton (plugin/kvm) implementing KVMPingAgentNoFailureExtensionPoint; pass-through done() in PH-1. - New VmNicLifecycleGlobalConfig and conf/globalConfig/vmNicLifecycle .xml declaring reconcileOnHost.timeout (default 30s). - Register two Spring beans in VmInstanceManager.xml and Kvm.xml. Side effects - Zero: skeleton methods are pass-through, legacy extension points are unchanged; the new interface is an opt-in abstraction layer. - Full routing logic will be filled in by PH-2 (Manager) and PH-3 (Bridge). Resolves: ZCF-2893 Change-Id: I330238e91d18c727db106af7057f89f331e082eb
MatheMatrix
force-pushed
the
sync/shixin.ruan/shixin.ruan-zcf-2893
branch
from
8530dc6 to
90b0714
Compare
Include VmNicLifecycleExtension.xml in KvmTest.springSpec so the
stub bean is registered when TestCaseStabilityTest pre-builds the
Spring context (it uses ZStackTest.springSpec; the sub-case's
spring{include()} block is silently ignored in that mode).
Default applicable=false so the stub is inert across all other KVM
tests; each vmnic case calls ext.reset() which restores applicable=true
before running its scenario.
http://jira.zstack.io/browse/ZCF-2893
Change-Id: I4e54753a0377010beb0d706c167424f7e68ffa39
Allow commit-msg hook to recognize ZCF-XXXX and AIOS-XXXX as valid jira links so that commits referencing these projects no longer trigger the missing-jira-link warning. Resolves: ZCF-2893
Add IPv6 support for ZStack management network (milestone M1). New files: - IPv6Utils: buildUrl/bracketIpv6/normalizeIpv6/isValidManagementIp - NetworkGlobalConfig: management.server.prefer.ipv6 (default false) Modified: - Platform: getManagementServerIp() prefer IPv4/IPv6 via GlobalConfig; getManagementServerCidr() IPv6 CIDR; jgroupsAddr() JGroups bracket fix; volatile fields for JMM correctness - RESTFacadeImpl: callbackUrl uses IPv6Utils.buildUrl(); Pattern static final - HostApiInterceptor: accept + normalize IPv6 management IP - ApplianceVmFacadeImpl: getMnIpForVr() CIDR matching for dual-stack MN Tests (TP-001~031): IPv6UtilsCase, MnIpv6Case, KvmHostIpv6Case, ApplianceVmIpv6Case Resolves: ZSTAC-79206 Change-Id: I3fec17c211d84c82a84e6711b7d09eea
New utility methods: - IPv6Utils.buildAddr(ip, port): format Ceph monAddr with IPv6 brackets - NetworkUtils.isIpInCidr(ip, cidr): dual-stack CIDR matching F-010: KVMHost.getDataNetworkAddress() uses isIpInCidr for IPv6 storage CIDR F-011: CephBackupStorageMonBase/CephPrimaryStorageMonBase use IPv6Utils.buildAddr F-012: KVMHost.checkMigrateNetworkCidrOfHost() uses isIpInCidr for IPv6 migration F-013: VxlanPoolApiInterceptor accepts IPv6 VTEP addresses F-014a: ZSha2Helper.isMaster() grep pattern fixed for IPv6 VIP detection NFS: NfsApiParamChecker.isValidStorageCidr supports IPv6 CIDR format Resolves: ZSTAC-79206 Change-Id: Idc7022fbdfce429ca8795f3522df682b
MnIpv6StorageMigrationCase: TP-032~038 storage/migration CIDR dual-stack VxlanIpv6Case: TP-039~041 VXLAN vtep IPv6 support ZSha2Ipv6Case: TP-042~046 HA zsha2 IPv6 SSH/nginx/URL/grep Resolves: ZSTAC-79206 Change-Id: Ia4e1480db3514b9c9f9e0e24aff2310d
- KvmHostIpmiPowerExecutor: remove IPv4-only guard for IPMI (F-025) - ConsoleManagerImpl: bracketIpv6() for console proxy hostname (F-026) - KVMAgentCommands/KVMHost: vncListenAddress='::' for IPv6 host (F-027) Resolves: ZSTAC-79206 Change-Id: I3f8a901c7d2e4b6f5c1a8d3e9f2b7c0d1e4a5f6
Add MnIpv6M3Case and MnIpv6M4Case covering: - TP-062~069,076,077: IPMI IPv6, Console Proxy bracket, BM DPU callback, COLO qemu URL - TP-083~089: ZWatch InfluxDB/Prometheus/Grafana URL, License HTTPS URL, Keycloak container name, SSO CAS login URL Resolves: ZSTAC-79206 Change-Id: If19fdab6ef4f6a7f8d9a16fb97c1ee29365197b2
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
utils/src/main/java/org/zstack/utils/network/NetworkUtils.java-561-563 (1)
561-563:⚠️ Potential issue | 🟡 Minor注释语言需统一为英文
Line [562] 的注释为中文，和仓库规范不一致，建议改为英文注释并保持术语统一。
As per coding guidelines
**/*.*: 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/src/main/java/org/zstack/utils/network/NetworkUtils.java` around lines 561 - 563, Replace the Chinese Javadoc comment "判断 IP（IPv4 或 IPv6）是否属于指定 CIDR。" in NetworkUtils (around the method that checks IP vs CIDR) with an English comment such as "Determines whether an IP address (IPv4 or IPv6) belongs to the specified CIDR." and ensure terminology matches other comments in NetworkUtils and the repo (use "IP address", "IPv4/IPv6", "CIDR"). Also scan the same file for any other Chinese text (comments, error strings) and convert them to consistent English phrasing.core/src/main/java/org/zstack/core/rest/RESTFacadeImpl.java-984-999 (1)
984-999:⚠️ Potential issue | 🟡 Minor请将新增方法注释改为英文
sanitizeCallbackUrl的新增注释目前为中文，建议改为英文以满足仓库统一规范。As per coding guidelines
**/*.*: 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写。🤖 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/RESTFacadeImpl.java` around lines 984 - 999, Update the Chinese method comment for sanitizeCallbackUrl to English while keeping the same content and references: explain that the method detects and fixes bare IPv6 callback URLs (those missing square brackets), note that normally URLs are produced by IPv6Utils.buildUrl and this is a fallback, and include the example mapping (e.g. http://2001:db8::1:8080/path → http://[2001:db8::1]:8080/path). Keep existing inline comments and references to BARE_IPV6_URL_PATTERN and IPv6NetworkUtils.isIpv6Address unchanged.utils/src/main/java/org/zstack/utils/network/IPv6Utils.java-34-40 (1)
34-40:⚠️ Potential issue | 🟡 Minor
buildHttpsUrl在path无前导/时会拼出错误 URL当前直接
base + path，当调用方传入"api"时会得到https://host:443api。建议在拼接前统一补齐分隔符。🔧 建议修复
public static String buildHttpsUrl(String ip, int port, String path) { String base = "https://" + bracketIpv6(ip) + ":" + port; if (path == null || path.isEmpty()) { return base; } - return base + path; + return path.startsWith("/") ? base + path : base + "/" + path; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/src/main/java/org/zstack/utils/network/IPv6Utils.java` around lines 34 - 40, buildHttpsUrl currently concatenates base + path which produces malformed URLs when path lacks a leading slash (e.g., "api" -> "https://host:443api"); update buildHttpsUrl to normalize the path before concatenation: if path is null/empty return base (unchanged), otherwise ensure path begins with a single '/' (prepend one when missing and avoid duplicating slashes if path already starts with '/'), then return base + normalizedPath; refer to the buildHttpsUrl method and bracketIpv6 usage to locate the code.utils/src/main/java/org/zstack/utils/network/IPv6Utils.java-16-21 (1)
16-21:⚠️ Potential issue | 🟡 Minor请将工具类注释统一改为英文
当前新增 Javadoc/注释含中文，建议统一替换为英文，避免与仓库规范冲突。
As per coding guidelines
**/*.*: 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写。Also applies to: 28-33, 43-49, 66-74, 94-106, 112-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/src/main/java/org/zstack/utils/network/IPv6Utils.java` around lines 16 - 21, Replace all Chinese text in the IPv6Utils class Javadoc and inline comments with clear, idiomatic English; specifically update the class-level and method-level Javadocs (e.g., the description/examples for buildUrl) and any comments inside methods like buildUrl, isIPv6 (or similar utility methods present) so they match repository guidelines and contain no Chinese characters or spelling errors—keep the original meaning and examples but rewrite them in English throughout the file.test/src/test/groovy/org/zstack/test/integration/utils/IPv6UtilsCase.groovy-7-9 (1)
7-9:⚠️ Potential issue | 🟡 Minor请将测试注释统一改为英文
当前新增注释含中文，和仓库通用规范不一致，建议统一替换为准确英文描述。
As per coding guidelines
**/*.*: 代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写。Also applies to: 38-40, 47-48, 55-56, 67-68, 75-76, 83-84, 91-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/groovy/org/zstack/test/integration/utils/IPv6UtilsCase.groovy` around lines 7 - 9, Replace all Chinese comments in the IPv6UtilsCase test class with clear English equivalents; e.g., change the file header comment to "TP-008~014: IPv6Utils pure unit tests" and "No Spring context required; directly test static utility methods." Locate the IPv6UtilsCase class and update the other commented blocks (existing Chinese on lines referenced in the review) to accurate English phrases matching their intent, ensuring no Chinese characters remain in comments, error messages, or annotations.test/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovy-98-114 (1)
98-114:⚠️ Potential issue | 🟡 MinorTP-017 现在没有观察到“规范化”结果。
这里只断言“不返回
INVALID_ARGUMENT_ERROR”，这只能证明输入没有在 API 层被拒绝，不能证明HostApiInterceptor.normalizeIpv6()真把全展开地址改写成了压缩格式。即使以后去掉规范化、只保留放行逻辑，这个用例也仍然会通过。建议成功后查询持久化的managementIp，或直接对拦截器输入/输出做断言。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovy` around lines 98 - 114, The test testFullIpv6NormalizedBeforeConnect currently only checks that INVALID_ARGUMENT_ERROR is not returned, which doesn't prove HostApiInterceptor.normalizeIpv6() actually rewrote the full IPv6; update the test to verify the normalization by (a) after successful AddKVMHostAction.call() query the persisted host (or the action result) and assert the stored managementIp equals the expected compressed/normalized form, or (b) add an assertion on the interceptor input/output path to confirm HostApiInterceptor.normalizeIpv6() converted "2001:0db8:0000:0000:0000:0000:0000:0001" to the compressed representation; reference AddKVMHostAction, testFullIpv6NormalizedBeforeConnect, and HostApiInterceptor.normalizeIpv6() when locating where to add the verification.test/src/test/groovy/org/zstack/test/integration/core/MnIpv6Case.groovy-194-200 (1)
194-200:⚠️ Potential issue | 🟡 MinorTP-022 的断言过弱，抓不住
sanitizeCallbackUrl()的回归。这里仅检查
result != null。即使方法把裸 IPv6 URL 原样返回，甚至返回了别的错误字符串，这个测试也仍然会通过。既然输入是固定的http://2001:db8::1:8080/callback，这里至少应断言 host 已经被方括号包裹，最好直接校验成http://[2001:db8::1]:8080/callback。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/groovy/org/zstack/test/integration/core/MnIpv6Case.groovy` around lines 194 - 200, The test testSanitizeCallbackUrlBareIpv6 currently only asserts result != null which is too weak; update the assertion to verify sanitizeCallbackUrl produces the normalized IPv6 form by asserting the returned string equals "http://[2001:db8::1]:8080/callback" (or at minimum contains "[2001:db8::1]") when invoking RESTFacadeImpl.sanitizeCallbackUrl with bareIpv6Url, so the test will catch regressions in host-bracketing behavior.test/src/test/groovy/org/zstack/test/integration/appliancevm/ApplianceVmIpv6Case.groovy-10-22 (1)
10-22:⚠️ Potential issue | 🟡 Minor请把新增注释改成英文。
这个新测试文件里的新增注释和文档块大量使用了中文，和仓库规范不一致。
As per coding guidelines**/*.*:代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写。Also applies to: 25-28, 32-42, 56-77, 95-97, 107-109, 121-123, 132-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/groovy/org/zstack/test/integration/appliancevm/ApplianceVmIpv6Case.groovy` around lines 10 - 22, Translate all Chinese comments and docblocks in the ApplianceVmIpv6Case test file into clear, idiomatic English while preserving original intent; specifically update the class-level Javadoc that references "TP-025~029", the description mentioning ApplianceVmFacadeImpl.getMnIpForVr and its behavior, and all inline comments around the test cases (including the ranges noted) to English with correct spelling and grammar; ensure you do not change code identifiers like ApplianceVmIpv6Case or the reflected method name getMnIpForVr, and keep any technical terms (CIDR, IPv4, Platform.getManagementServerIp) unchanged.test/src/test/groovy/org/zstack/test/integration/appliancevm/ApplianceVmIpv6Case.groovy-110-118 (1)
110-118:⚠️ Potential issue | 🟡 Minor
10.99.88.0/24会让这个 fallback 用例变得环境相关。这不是保留文档网段，实验室或用户环境里真实存在该网段并不罕见；一旦命中，这个用例就会把正确行为误判成失败。这里最好改成按当前 management IP 的地址族使用文档保留网段，例如 IPv4 用
203.0.113.0/24、IPv6 用2001:db8::/64，或者从当前 CIDR 派生一个确定不匹配的网段。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/groovy/org/zstack/test/integration/appliancevm/ApplianceVmIpv6Case.groovy` around lines 110 - 118, The test testUnmatchedCidrFallback uses a hardcoded CIDR "10.99.88.0/24" which can exist in some environments; change it to pick a guaranteed-documentation-reserved CIDR based on the management IP family (use Platform.getManagementServerIp() to detect IPv4 vs IPv6) and set unmatchedCidr to "203.0.113.0/24" for IPv4 or "2001:db8::/64" for IPv6 (or alternatively derive a non-overlapping subnet from the MN CIDR), then call callGetMnIpForVr(unmatchedCidr) and keep the same assertions and log using the new unmatchedCidr variable.test/src/test/groovy/org/zstack/test/integration/core/MnIpv6Case.groovy-14-34 (1)
14-34:⚠️ Potential issue | 🟡 Minor请把新增注释改成英文。
这个新测试文件里的新增注释和文档块包含中文，和仓库规范冲突。
As per coding guidelines**/*.*:代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写。Also applies to: 39-49, 69-73, 82-84, 94-97, 107-124, 135-179, 191-193, 204-207, 222-239, 251-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/groovy/org/zstack/test/integration/core/MnIpv6Case.groovy` around lines 14 - 34, The file-level and inline comment blocks in the test class MnIpv6Case (the Javadoc header and subsequent comment blocks inside the Groovy test file) contain Chinese text; update all comments, Javadoc, and any human-readable strings in that file to English (clear, grammatically correct, no Chinese characters) while preserving the meaning of each test description (e.g., TP-001..TP-007, TP-021..TP-024, TP-030..TP-031) and any identifiers like MnIpv6Case, sanitizeCallbackUrl, getManagementServerIp, getManagementServerCidr, getManagementServerId, jgroupsAddr so reviewers can still map descriptions to tests; apply the same translation to all other commented regions flagged in the review (the additional comment blocks listed) so the file contains only English comments and documentation.core/src/main/java/org/zstack/core/Platform.java-400-403 (1)
400-403:⚠️ Potential issue | 🟡 Minor请把新增注释改成英文。
当前这些新增注释包含中文，和仓库规范冲突。
As per coding guidelines**/*.*:代码里不应当有有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写。Also applies to: 804-805, 827-829, 837-837, 857-857, 895-900
🤖 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/Platform.java` around lines 400 - 403, The new inline JGroups comment in Platform.java contains Chinese; replace it with clear, idiomatic English so it follows repository guidelines (e.g., change "F-010: JGroups initial_hosts IPv6 括号修复。IPv6 地址在 JGroups 中须使用 [addr][port] 格式，IPv4 使用 addr[port] 格式。" to an English-only description explaining the IPv6/IPv4 bracket format for JGroups initial_hosts). Update the same for the other affected comment blocks referenced (lines ~804-805, 827-829, 837, 857, 895-900) so all comments in Platform.java are written in proper English with no Chinese characters.
🧹 Nitpick comments (2)
test/src/test/groovy/org/zstack/test/integration/core/ZSha2Ipv6Case.groovy (1)
48-82:contains()复刻不了ZSha2Helper的真实命令行为。这里是手工拼了
" ${vip}/"再做字符串匹配，没有调用ZSha2Helper，也不会验证 shell 模板、参数转义或命令拼接位置。后续如果 helper 的命令串回退或拼错，这两个用例仍然可能全绿。建议把匹配逻辑抽成可单测的方法，或者直接对 helper 生成的命令/行为做断言。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/groovy/org/zstack/test/integration/core/ZSha2Ipv6Case.groovy` around lines 48 - 82, The test currently fakes matching by using output.contains(" ${vip}/") instead of exercising the real helper; update the tests to either (A) call ZSha2Helper.isMaster(...) with a mocked command output or (B) extract the VIP-grep construction into a small method (e.g. a new ZSha2Helper.buildVipGrepPattern(vip) or ZSha2Helper.isVipInOutput(output, vip)) and assert that method returns the correct pattern/boolean; specifically replace direct contains checks in testIsMasterGrepPatternMatchesIpv6 and testIsMasterGrepLogicWithVip with calls to ZSha2Helper.isMaster or the new build/isVipInOutput helper so the actual command/grep logic and escaping are validated.test/src/test/groovy/org/zstack/test/integration/core/MnIpv6M4Case.groovy (1)
54-259: 这些断言没有约束真实业务代码是否接入了IPv6Utils。文件里验证的基本都是
IPv6Utils和String.format自身行为；如果 InfluxDB/Grafana/License/Keycloak/CAS 的实际调用点后来又改回裸字符串拼接，这些用例仍然会全部通过。建议至少挑几个关键生产方法直接断言最终 URL/名称，避免“工具方法正确但业务没接入”的假阳性。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/test/groovy/org/zstack/test/integration/core/MnIpv6M4Case.groovy` around lines 54 - 259, The tests currently only assert IPv6Utils and String.format behavior (seen in methods testInfluxDbUrlIpv6Bracket, testPrometheusWriteUrlIpv6, testGrafanaDataSourceUrlIpv6, testLicenseHttpUrlIpv6, testKeycloakContainerNameSanitize, testSsoCasLoginUrlIpv6) which can produce false positives if business code doesn't use IPv6Utils; update each test to also invoke the actual production entrypoints that build or register those endpoints (e.g. the Influx/Grafana registration or client factory, license callback URL builder, Keycloak container-name generator, and CAS login URL builder) and assert their resulting URL/container name equals the expected RFC-compliant string, ensuring the code paths that should consume IPv6Utils are exercised rather than only testing IPv6Utils itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/org/zstack/core/Platform.java`:
- Around line 895-949: The current logic in Platform (the block building
ipv4List/ipv6List and then selecting preferred.get(0)/fallback.get(0)) replaces
the original default-route-based interface selection and can pick the wrong NIC;
restore the original behavior by first determining the default outbound
interface (the interface used for the system default route) and only then select
an address from that interface honoring NetworkGlobalConfig.PREFER_IPV6;
specifically, keep NetworkGlobalConfig.PREFER_IPV6.value(Boolean.class) and the
IPv4/IPv6 classification code, but instead of taking
preferred.get(0)/fallback.get(0) from all interfaces, locate the default
interface (e.g., by creating a socket to a well-known remote address to obtain
the local address or by querying routing info) and choose the IPv4/IPv6 address
on that interface, falling back to the previous enumeration logic only if
default-route detection fails.
- Around line 896-900: getManagementServerIp() is reading
NetworkGlobalConfig.PREFER_IPV6 during static initialization and silently
falling back to false when GlobalConfigFacade isn't ready, then caching the
result into managementServerIp so later GlobalConfig changes (e.g.,
management.server.prefer.ipv6=true) won't take effect; fix by avoiding relying
on GlobalConfig in static init: either read the preference from system
properties/zstack.properties as a bootstrap fallback (use System.getProperty or
the existing properties loader) inside getManagementServerIp/Platform static
init, or stop caching the value at static init and instead compute
managementServerIp lazily or add a GlobalConfig-ready hook/listener (via
GlobalConfigFacade or similar lifecycle) to recompute and update the
managementServerIp when GlobalConfig becomes ready; update code paths that
reference managementServerIp/getManagementServerIp to use the new lazy/refresh
logic and ensure NetworkGlobalConfig.PREFER_IPV6 remains the authoritative
source once GlobalConfig is initialized.
In
`@plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmFacadeImpl.java`:
- Around line 556-558: The code writes BootstrapParams.managementNodeIp using
getMnIpForVr(vrMgmtCidr) but unconditionally writes managementNodeCidr from
Platform.getManagementServerCidr(), which can produce inconsistent IP/CIDR
pairs; change the implementation so that the CIDR is derived from the chosen
mnIp (either by updating getMnIpForVr to return a structure containing both ip
and cidr, or by calling a helper like getManagementCidrForIp(mnIp) immediately
after getMnIpForVr) and then write the matching CIDR to
BootstrapParams.managementNodeCidr; update references to getMnIpForVr and
Platform.getManagementServerCidr accordingly so managementNodeIp and
managementNodeCidr are always computed from the same selected IP.
In `@test/src/test/groovy/org/zstack/test/integration/core/MnIpv6Case.groovy`:
- Around line 107-132: Tests TP-004/TP-005 are using a cached value from
Platform.managementServerIp and never toggle NetworkGlobalConfig.PREFER_IPV6, so
they don’t exercise the intended branches; update the tests to explicitly reset
the cache (clear Platform.managementServerIp) and set
NetworkGlobalConfig.PREFER_IPV6 to the desired value (true for TP-005, false for
TP-004) before calling Platform.getManagementServerIp(), then restore the
original config afterwards (or extract a small helper that sets PREFER_IPV6,
clears Platform.managementServerIp, invokes getManagementServerIp and returns
the result) so each test deterministically exercises the IPv4/IPv6 and fallback
paths.
In `@test/src/test/groovy/org/zstack/test/integration/core/MnIpv6M3Case.groovy`:
- Around line 74-84: The test method testIpmiAddressFullLengthIpv6 only
validates IPv6 string length and format but does not exercise the ipmiAddress
field or persistence; update the test to either inspect the entity
mapping/column definition for ipmiAddress or perform a write-and-read roundtrip
using the entity that contains ipmiAddress (e.g., create an instance, set
ipmiAddress = "2001:0db8:...0001", save via the repository/DAO, flush/clear,
reload and assert the persisted value equals the input) so the test actually
verifies the ipmiAddress column can store the full 39-char IPv6 value. Ensure
you reference the test method testIpmiAddressFullLengthIpv6 and the ipmiAddress
field in your changes.
In
`@test/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovy`:
- Around line 63-70: The test testManagementIpColumnLength should not rely on
JPA default `@Column` length on HostAO.managementIp; remove the
reflection/assertion against Column.length() and instead validate the
authoritative schema by either reading the database metadata for the host
table's managementIp column (via the project's DataSource/JdbcTemplate and
INFORMATION_SCHEMA/COLUMNS) or by parsing the project's SQL migration that
defines the host table to assert it uses a varchar long enough for 39 chars;
update the test to query the DB metadata or migration content and assert the
actual column length >= 39 and log the found length.
In
`@test/src/test/groovy/org/zstack/test/integration/network/ipv6/MnIpv6StorageMigrationCase.groovy`:
- Around line 47-135: Tests only exercise utility methods
(NetworkUtils.isIpInCidr, IPv6Utils.buildAddr) but do not exercise the real
business paths (e.g., KVMHost.checkMigrateNetworkCidrOfHost, NFS/Ceph flows and
the extraIps/fallback branches). Update the test cases to invoke the actual code
paths: call or simulate the plugin logic that triggers
KVMHost.checkMigrateNetworkCidrOfHost (or drive the migration workflow in the
kvm plugin), create test hosts with extraIps set and add NFS and Ceph storage
resources so the code hits the NFS/Ceph call sites, and add assertions that
validate the extraIps handling and fallback branch outcomes rather than only
asserting the utility methods; reference NetworkUtils.isIpInCidr,
IPv6Utils.buildAddr, KVMHost.checkMigrateNetworkCidrOfHost and the extraIps
handling when implementing the test changes.
In
`@test/src/test/groovy/org/zstack/test/integration/network/vxlan/VxlanIpv6Case.groovy`:
- Around line 69-85: The test checkVtepIpColumnLength currently inspects the JPA
`@Column` annotation (field vtepIp on VtepVO and RemoteVtepVO) which returns the
JPA default length rather than the actual DB column width, so update the test to
validate the real schema or behavior instead: either query the live
database/schema migration SQL (or use JDBC DatabaseMetaData) to read the
declared column size for table columns backing vtepIp, or perform a round-trip
persistence test that inserts and reads a full 39-character expanded IPv6 string
to ensure the DB accepts it; also remove the Chinese comments around the length
checks and replace them with brief English comments explaining why we validate
the DB schema/round-trip rather than the annotation.
In `@utils/src/main/java/org/zstack/utils/network/NetworkUtils.java`:
- Around line 564-568: The isIpInCidr method currently only checks the ip family
and calls isIpv4InCidr or IPv6NetworkUtils.isIpv6InCidrRange, which can
mis-handle cross-family inputs; update isIpInCidr to also verify the cidr family
before invoking the family-specific check: when isIpv4Address(ip) is true ensure
cidr represents an IPv4 CIDR (e.g., by using isIpv4Address on the cidr's base
address or a helper like isCidrIpv4) and only then call isIpv4InCidr(ip, cidr),
otherwise return false; similarly, when IPv6NetworkUtils.isIpv6Address(ip) is
true ensure cidr is an IPv6 CIDR before calling
IPv6NetworkUtils.isIpv6InCidrRange(ip, cidr), else return false.
In `@utils/src/main/java/org/zstack/utils/zsha2/ZSha2Helper.java`:
- Line 42: The shell command string that checks for the DB VIP uses grep -q
which treats the pattern as a regex, so in ZSha2Helper where the command is
built ("ip addr show %s | grep -q \" %s/\"") replace the grep invocation with
fixed-string matching (grep -Fq) to avoid regex interpretation of characters in
info.getDbvip(); update the command string to use "grep -Fq" so the check
reliably matches the literal IP substring.
---
Minor comments:
In `@core/src/main/java/org/zstack/core/Platform.java`:
- Around line 400-403: The new inline JGroups comment in Platform.java contains
Chinese; replace it with clear, idiomatic English so it follows repository
guidelines (e.g., change "F-010: JGroups initial_hosts IPv6 括号修复。IPv6 地址在
JGroups 中须使用 [addr][port] 格式，IPv4 使用 addr[port] 格式。" to an English-only
description explaining the IPv6/IPv4 bracket format for JGroups initial_hosts).
Update the same for the other affected comment blocks referenced (lines
~804-805, 827-829, 837, 857, 895-900) so all comments in Platform.java are
written in proper English with no Chinese characters.
In `@core/src/main/java/org/zstack/core/rest/RESTFacadeImpl.java`:
- Around line 984-999: Update the Chinese method comment for sanitizeCallbackUrl
to English while keeping the same content and references: explain that the
method detects and fixes bare IPv6 callback URLs (those missing square
brackets), note that normally URLs are produced by IPv6Utils.buildUrl and this
is a fallback, and include the example mapping (e.g.
http://2001:db8::1:8080/path → http://[2001:db8::1]:8080/path). Keep existing
inline comments and references to BARE_IPV6_URL_PATTERN and
IPv6NetworkUtils.isIpv6Address unchanged.
In
`@test/src/test/groovy/org/zstack/test/integration/appliancevm/ApplianceVmIpv6Case.groovy`:
- Around line 10-22: Translate all Chinese comments and docblocks in the
ApplianceVmIpv6Case test file into clear, idiomatic English while preserving
original intent; specifically update the class-level Javadoc that references
"TP-025~029", the description mentioning ApplianceVmFacadeImpl.getMnIpForVr and
its behavior, and all inline comments around the test cases (including the
ranges noted) to English with correct spelling and grammar; ensure you do not
change code identifiers like ApplianceVmIpv6Case or the reflected method name
getMnIpForVr, and keep any technical terms (CIDR, IPv4,
Platform.getManagementServerIp) unchanged.
- Around line 110-118: The test testUnmatchedCidrFallback uses a hardcoded CIDR
"10.99.88.0/24" which can exist in some environments; change it to pick a
guaranteed-documentation-reserved CIDR based on the management IP family (use
Platform.getManagementServerIp() to detect IPv4 vs IPv6) and set unmatchedCidr
to "203.0.113.0/24" for IPv4 or "2001:db8::/64" for IPv6 (or alternatively
derive a non-overlapping subnet from the MN CIDR), then call
callGetMnIpForVr(unmatchedCidr) and keep the same assertions and log using the
new unmatchedCidr variable.
In `@test/src/test/groovy/org/zstack/test/integration/core/MnIpv6Case.groovy`:
- Around line 194-200: The test testSanitizeCallbackUrlBareIpv6 currently only
asserts result != null which is too weak; update the assertion to verify
sanitizeCallbackUrl produces the normalized IPv6 form by asserting the returned
string equals "http://[2001:db8::1]:8080/callback" (or at minimum contains
"[2001:db8::1]") when invoking RESTFacadeImpl.sanitizeCallbackUrl with
bareIpv6Url, so the test will catch regressions in host-bracketing behavior.
- Around line 14-34: The file-level and inline comment blocks in the test class
MnIpv6Case (the Javadoc header and subsequent comment blocks inside the Groovy
test file) contain Chinese text; update all comments, Javadoc, and any
human-readable strings in that file to English (clear, grammatically correct, no
Chinese characters) while preserving the meaning of each test description (e.g.,
TP-001..TP-007, TP-021..TP-024, TP-030..TP-031) and any identifiers like
MnIpv6Case, sanitizeCallbackUrl, getManagementServerIp, getManagementServerCidr,
getManagementServerId, jgroupsAddr so reviewers can still map descriptions to
tests; apply the same translation to all other commented regions flagged in the
review (the additional comment blocks listed) so the file contains only English
comments and documentation.
In
`@test/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovy`:
- Around line 98-114: The test testFullIpv6NormalizedBeforeConnect currently
only checks that INVALID_ARGUMENT_ERROR is not returned, which doesn't prove
HostApiInterceptor.normalizeIpv6() actually rewrote the full IPv6; update the
test to verify the normalization by (a) after successful AddKVMHostAction.call()
query the persisted host (or the action result) and assert the stored
managementIp equals the expected compressed/normalized form, or (b) add an
assertion on the interceptor input/output path to confirm
HostApiInterceptor.normalizeIpv6() converted
"2001:0db8:0000:0000:0000:0000:0000:0001" to the compressed representation;
reference AddKVMHostAction, testFullIpv6NormalizedBeforeConnect, and
HostApiInterceptor.normalizeIpv6() when locating where to add the verification.
In `@test/src/test/groovy/org/zstack/test/integration/utils/IPv6UtilsCase.groovy`:
- Around line 7-9: Replace all Chinese comments in the IPv6UtilsCase test class
with clear English equivalents; e.g., change the file header comment to
"TP-008~014: IPv6Utils pure unit tests" and "No Spring context required;
directly test static utility methods." Locate the IPv6UtilsCase class and update
the other commented blocks (existing Chinese on lines referenced in the review)
to accurate English phrases matching their intent, ensuring no Chinese
characters remain in comments, error messages, or annotations.
In `@utils/src/main/java/org/zstack/utils/network/IPv6Utils.java`:
- Around line 34-40: buildHttpsUrl currently concatenates base + path which
produces malformed URLs when path lacks a leading slash (e.g., "api" ->
"https://host:443api"); update buildHttpsUrl to normalize the path before
concatenation: if path is null/empty return base (unchanged), otherwise ensure
path begins with a single '/' (prepend one when missing and avoid duplicating
slashes if path already starts with '/'), then return base + normalizedPath;
refer to the buildHttpsUrl method and bracketIpv6 usage to locate the code.
- Around line 16-21: Replace all Chinese text in the IPv6Utils class Javadoc and
inline comments with clear, idiomatic English; specifically update the
class-level and method-level Javadocs (e.g., the description/examples for
buildUrl) and any comments inside methods like buildUrl, isIPv6 (or similar
utility methods present) so they match repository guidelines and contain no
Chinese characters or spelling errors—keep the original meaning and examples but
rewrite them in English throughout the file.
In `@utils/src/main/java/org/zstack/utils/network/NetworkUtils.java`:
- Around line 561-563: Replace the Chinese Javadoc comment "判断 IP（IPv4 或
IPv6）是否属于指定 CIDR。" in NetworkUtils (around the method that checks IP vs CIDR)
with an English comment such as "Determines whether an IP address (IPv4 or IPv6)
belongs to the specified CIDR." and ensure terminology matches other comments in
NetworkUtils and the repo (use "IP address", "IPv4/IPv6", "CIDR"). Also scan the
same file for any other Chinese text (comments, error strings) and convert them
to consistent English phrasing.
---
Nitpick comments:
In `@test/src/test/groovy/org/zstack/test/integration/core/MnIpv6M4Case.groovy`:
- Around line 54-259: The tests currently only assert IPv6Utils and
String.format behavior (seen in methods testInfluxDbUrlIpv6Bracket,
testPrometheusWriteUrlIpv6, testGrafanaDataSourceUrlIpv6,
testLicenseHttpUrlIpv6, testKeycloakContainerNameSanitize,
testSsoCasLoginUrlIpv6) which can produce false positives if business code
doesn't use IPv6Utils; update each test to also invoke the actual production
entrypoints that build or register those endpoints (e.g. the Influx/Grafana
registration or client factory, license callback URL builder, Keycloak
container-name generator, and CAS login URL builder) and assert their resulting
URL/container name equals the expected RFC-compliant string, ensuring the code
paths that should consume IPv6Utils are exercised rather than only testing
IPv6Utils itself.
In `@test/src/test/groovy/org/zstack/test/integration/core/ZSha2Ipv6Case.groovy`:
- Around line 48-82: The test currently fakes matching by using
output.contains(" ${vip}/") instead of exercising the real helper; update the
tests to either (A) call ZSha2Helper.isMaster(...) with a mocked command output
or (B) extract the VIP-grep construction into a small method (e.g. a new
ZSha2Helper.buildVipGrepPattern(vip) or ZSha2Helper.isVipInOutput(output, vip))
and assert that method returns the correct pattern/boolean; specifically replace
direct contains checks in testIsMasterGrepPatternMatchesIpv6 and
testIsMasterGrepLogicWithVip with calls to ZSha2Helper.isMaster or the new
build/isVipInOutput helper so the actual command/grep logic and escaping are
validated.
🪄 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: b9e57033-3648-41c7-a064-85209857fa93
📒 Files selected for processing (28)
.gitconfig/hooks/commit-msgcompute/src/main/java/org/zstack/compute/host/HostApiInterceptor.javaconsole/src/main/java/org/zstack/console/ConsoleManagerImpl.javacore/src/main/java/org/zstack/core/Platform.javacore/src/main/java/org/zstack/core/config/NetworkGlobalConfig.javacore/src/main/java/org/zstack/core/rest/RESTFacadeImpl.javaplugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmFacadeImpl.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageMonBase.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageMonBase.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHost.javaplugin/kvm/src/main/java/org/zstack/kvm/KvmHostIpmiPowerExecutor.javaplugin/nfsPrimaryStorage/src/main/java/org/zstack/storage/primary/nfs/NfsApiParamChecker.javaplugin/vxlan/src/main/java/org/zstack/network/l2/vxlan/vxlanNetworkPool/VxlanPoolApiInterceptor.javatest/src/test/groovy/org/zstack/test/integration/appliancevm/ApplianceVmIpv6Case.groovytest/src/test/groovy/org/zstack/test/integration/core/MnIpv6Case.groovytest/src/test/groovy/org/zstack/test/integration/core/MnIpv6M3Case.groovytest/src/test/groovy/org/zstack/test/integration/core/MnIpv6M4Case.groovytest/src/test/groovy/org/zstack/test/integration/core/ZSha2Ipv6Case.groovytest/src/test/groovy/org/zstack/test/integration/kvm/KvmTest.groovytest/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovytest/src/test/groovy/org/zstack/test/integration/network/ipv6/MnIpv6StorageMigrationCase.groovytest/src/test/groovy/org/zstack/test/integration/network/vxlan/VxlanIpv6Case.groovytest/src/test/groovy/org/zstack/test/integration/utils/IPv6UtilsCase.groovytest/src/test/java/org/zstack/test/compute/vmnic/TestVmNicLifecycleExtension.javautils/src/main/java/org/zstack/utils/network/IPv6Utils.javautils/src/main/java/org/zstack/utils/network/NetworkUtils.javautils/src/main/java/org/zstack/utils/zsha2/ZSha2Helper.java
💤 Files with no reviewable changes (1)
- plugin/kvm/src/main/java/org/zstack/kvm/KvmHostIpmiPowerExecutor.java
✅ Files skipped from review due to trivial changes (3)
- test/src/test/groovy/org/zstack/test/integration/kvm/KvmTest.groovy
- plugin/kvm/src/main/java/org/zstack/kvm/KVMAgentCommands.java
- test/src/test/java/org/zstack/test/compute/vmnic/TestVmNicLifecycleExtension.java
Comment on lines
+895
to
+949
| // F-002: 支持 IPv6 — 枚举所有网卡，按 PREFER_IPV6 配置决定优先级 | ||
| boolean preferIpv6 = false; | ||
| try { | ||
| preferIpv6 = NetworkGlobalConfig.PREFER_IPV6.value(Boolean.class); | ||
| } catch (Exception ignored) { | ||
| // GlobalConfig 可能在静态初始化阶段还未就绪，安全降级为 false | ||
| } | ||
|
|
||
| String err = "cannot get management server ip of this machine. there are three ways to get the ip.\n1) search for 'management.server.ip' java property\n2) search for 'ZSTACK_MANAGEMENT_SERVER_IP' environment variable\n3) search for default route printed out by '/sbin/ip route'\nhowever, all above methods failed"; | ||
| if (defaultLine == null) { | ||
| throw new CloudRuntimeException(err); | ||
| } | ||
| List<InetAddress> ipv4List = new ArrayList<>(); | ||
| List<InetAddress> ipv6List = new ArrayList<>(); | ||
|
|
||
| try { | ||
| Enumeration<NetworkInterface> nets = NetworkInterface.getNetworkInterfaces(); | ||
| for (NetworkInterface iface : Collections.list(nets)) { | ||
| String name = iface.getName(); | ||
| if (defaultLine.contains(name)) { | ||
| for (InetAddress ia : Collections.list(iface.getInetAddresses())) { | ||
| ip = ia.getHostAddress(); | ||
| if (ia instanceof Inet4Address) { | ||
| // we prefer IPv4 address | ||
| ip = ia.getHostAddress(); | ||
| break; | ||
| } | ||
| Enumeration<NetworkInterface> ifaces = NetworkInterface.getNetworkInterfaces(); | ||
| if (ifaces == null) { | ||
| throw new IllegalStateException("no available network interfaces"); | ||
| } | ||
| while (ifaces.hasMoreElements()) { | ||
| NetworkInterface iface = ifaces.nextElement(); | ||
| if (iface.isLoopback() || !iface.isUp()) { | ||
| continue; | ||
| } | ||
| Enumeration<InetAddress> addrs = iface.getInetAddresses(); | ||
| while (addrs.hasMoreElements()) { | ||
| InetAddress addr = addrs.nextElement(); | ||
| if (addr.isLoopbackAddress() || addr.isLinkLocalAddress()) { | ||
| continue; | ||
| } | ||
| if (addr instanceof Inet4Address) { | ||
| ipv4List.add(addr); | ||
| } else if (addr instanceof Inet6Address) { | ||
| ipv6List.add(addr); | ||
| } | ||
| } | ||
| } | ||
| } catch (SocketException e) { | ||
| throw new CloudRuntimeException(e); | ||
| } | ||
|
|
||
| if (ip == null) { | ||
| throw new CloudRuntimeException(err); | ||
| List<InetAddress> preferred = preferIpv6 ? ipv6List : ipv4List; | ||
| List<InetAddress> fallback = preferIpv6 ? ipv4List : ipv6List; | ||
|
|
||
| if (!preferred.isEmpty()) { | ||
| ip = preferred.get(0).getHostAddress(); | ||
| logger.info(String.format("get management IP[%s] from network interface enumeration (prefer IPv%s)", | ||
| ip, preferIpv6 ? "6" : "4")); | ||
| return ip; | ||
| } | ||
| if (!fallback.isEmpty()) { | ||
| ip = fallback.get(0).getHostAddress(); | ||
| logger.info(String.format("get management IP[%s] from network interface enumeration (fallback IPv%s)", | ||
| ip, preferIpv6 ? "4" : "6")); | ||
| return ip; | ||
| } | ||
|
|
||
| logger.info(String.format("get management IP[%s] from default route[/sbin/ip route]", ip)); | ||
| return ip; | ||
| throw new IllegalStateException("no available management IP found on any network interface"); |
There was a problem hiding this comment.
不要用“第一个非 loopback 地址”替代原来的默认路由选址。
这里直接返回 preferred.get(0) / fallback.get(0)，会把 management IP 绑定到网卡枚举顺序上。多网卡主机上这很容易选中 docker/bridge/storage 地址，而不是管理流量实际走的接口；后续 msId、JGroups 地址和回调 URL 都会跟着错。更稳妥的做法是保留原来的默认路由定位，再只在目标接口上按 IPv4/IPv6 偏好挑地址。
As per coding guidelines 向后兼容原则：之前的代码产生的行为不要直接去改动...可以考虑升级用户一套行为、新安装用户一套行为，开关控制等。
🤖 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/Platform.java` around lines 895 - 949, The
current logic in Platform (the block building ipv4List/ipv6List and then
selecting preferred.get(0)/fallback.get(0)) replaces the original
default-route-based interface selection and can pick the wrong NIC; restore the
original behavior by first determining the default outbound interface (the
interface used for the system default route) and only then select an address
from that interface honoring NetworkGlobalConfig.PREFER_IPV6; specifically, keep
NetworkGlobalConfig.PREFER_IPV6.value(Boolean.class) and the IPv4/IPv6
classification code, but instead of taking preferred.get(0)/fallback.get(0) from
all interfaces, locate the default interface (e.g., by creating a socket to a
well-known remote address to obtain the local address or by querying routing
info) and choose the IPv4/IPv6 address on that interface, falling back to the
previous enumeration logic only if default-route detection fails.
Comment on lines
+896
to
+900
| boolean preferIpv6 = false; | ||
| try { | ||
| preferIpv6 = NetworkGlobalConfig.PREFER_IPV6.value(Boolean.class); | ||
| } catch (Exception ignored) { | ||
| // GlobalConfig 可能在静态初始化阶段还未就绪，安全降级为 false |
There was a problem hiding this comment.
这个 PREFER_IPV6 开关在启动阶段实际上不会生效。
getManagementServerIp() 会在本类静态初始化里先被调用来生成 msId，那时 GlobalConfigFacade 还没启动。这里一旦 NetworkGlobalConfig.PREFER_IPV6.value(Boolean.class) 取值失败就静默降级成 false，随后结果又缓存进 managementServerIp，所以数据库里把 management.server.prefer.ipv6 设成 true 也不会影响本次启动的选址。若这是启动期行为，值需要来自 zstack.properties/system property，或者在 GlobalConfig 就绪后刷新缓存再重算。
🤖 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/Platform.java` around lines 896 - 900,
getManagementServerIp() is reading NetworkGlobalConfig.PREFER_IPV6 during static
initialization and silently falling back to false when GlobalConfigFacade isn't
ready, then caching the result into managementServerIp so later GlobalConfig
changes (e.g., management.server.prefer.ipv6=true) won't take effect; fix by
avoiding relying on GlobalConfig in static init: either read the preference from
system properties/zstack.properties as a bootstrap fallback (use
System.getProperty or the existing properties loader) inside
getManagementServerIp/Platform static init, or stop caching the value at static
init and instead compute managementServerIp lazily or add a GlobalConfig-ready
hook/listener (via GlobalConfigFacade or similar lifecycle) to recompute and
update the managementServerIp when GlobalConfig becomes ready; update code paths
that reference managementServerIp/getManagementServerIp to use the new
lazy/refresh logic and ensure NetworkGlobalConfig.PREFER_IPV6 remains the
authoritative source once GlobalConfig is initialized.
Comment on lines
+556
to
558
| String mnIp = getMnIpForVr(vrMgmtCidr); | ||
| ret.put(BootstrapParams.managementNodeIp.toString(), mnIp); | ||
| ret.put(BootstrapParams.managementNodeCidr.toString(), Platform.getManagementServerCidr()); |
There was a problem hiding this comment.
managementNodeIp 和 managementNodeCidr 现在可能变成一组不一致的数据。
新逻辑会按 VR 管理网选出一个本机地址写入 managementNodeIp，但 Line 558 仍然无条件写 Platform.getManagementServerCidr()。当选中的 mnIp 不是平台默认管理 IP 时，这两个字段可能属于不同网段，甚至不同协议栈，bootstrap 侧拿到的是一组自相矛盾的配置。建议基于最终选中的 mnIp 同步计算对应 CIDR，或者让 helper 一次返回 IP + CIDR。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugin/applianceVm/src/main/java/org/zstack/appliancevm/ApplianceVmFacadeImpl.java`
around lines 556 - 558, The code writes BootstrapParams.managementNodeIp using
getMnIpForVr(vrMgmtCidr) but unconditionally writes managementNodeCidr from
Platform.getManagementServerCidr(), which can produce inconsistent IP/CIDR
pairs; change the implementation so that the CIDR is derived from the chosen
mnIp (either by updating getMnIpForVr to return a structure containing both ip
and cidr, or by calling a helper like getManagementCidrForIp(mnIp) immediately
after getMnIpForVr) and then write the matching CIDR to
BootstrapParams.managementNodeCidr; update references to getMnIpForVr and
Platform.getManagementServerCidr accordingly so managementNodeIp and
managementNodeCidr are always computed from the same selected IP.
Comment on lines
+107
to
+132
| /** | ||
| * TP-004: PREFER_IPV6=false（默认值）时，CI IPv4 环境返回 IPv4 格式 | ||
| */ | ||
| void testGetManagementServerIpIpv4() { | ||
| String ip = Platform.getManagementServerIp() | ||
| assert ip != null : "TP-004: getManagementServerIp() should not be null" | ||
| // CI 环境为 IPv4-only，PREFER_IPV6 默认 false，返回 IPv4 地址 | ||
| logger.info("TP-004: management server IP = $ip (preferIpv6=false default)") | ||
| // 验证是合法的 IP 地址格式 | ||
| boolean isValidIp = NetworkUtils.isIpv4Address(ip) || IPv6NetworkUtils.isIpv6Address(ip) | ||
| assert isValidIp : "TP-004: should be valid IP, got: $ip" | ||
| } | ||
|
|
||
| /** | ||
| * TP-005: PREFER_IPV6=true 时（无 IPv6 接口）能回退到 IPv4，不抛异常 | ||
| */ | ||
| void testGetManagementServerIpFallback() { | ||
| // Platform.getManagementServerIp() 内部异常安全降级；此处验证方法不抛出异常 | ||
| String ip = null | ||
| try { | ||
| ip = Platform.getManagementServerIp() | ||
| } catch (Exception e) { | ||
| assert false : "TP-005: getManagementServerIp() should not throw exception even when PREFER_IPV6=true with no IPv6, got: ${e.message}" | ||
| } | ||
| assert ip != null : "TP-005: getManagementServerIp() should return fallback IP, not null" | ||
| logger.info("TP-005: PREFER_IPV6 fallback returns $ip") |
There was a problem hiding this comment.
TP-004 / TP-005 现在没有真正覆盖到声明的分支。
这两个用例都只是再次调用已经缓存过的 Platform.getManagementServerIp()：既没有清空 Platform.managementServerIp，也没有把 NetworkGlobalConfig.PREFER_IPV6 显式切到 true/false 两种状态。结果是 TP-004 即使返回 IPv6 也会通过，TP-005 也完全没有证明“prefer IPv6 后回退到 IPv4”这条路径，回归时很容易出现假阳性。建议把选址逻辑抽成可控 helper，或者至少在测试里重置缓存并显式设置配置值后再断言。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/src/test/groovy/org/zstack/test/integration/core/MnIpv6Case.groovy`
around lines 107 - 132, Tests TP-004/TP-005 are using a cached value from
Platform.managementServerIp and never toggle NetworkGlobalConfig.PREFER_IPV6, so
they don’t exercise the intended branches; update the tests to explicitly reset
the cache (clear Platform.managementServerIp) and set
NetworkGlobalConfig.PREFER_IPV6 to the desired value (true for TP-005, false for
TP-004) before calling Platform.getManagementServerIp(), then restore the
original config afterwards (or extract a small helper that sets PREFER_IPV6,
clears Platform.managementServerIp, invokes getManagementServerIp and returns
the result) so each test deterministically exercises the IPv4/IPv6 and fallback
paths.
Comment on lines
+74
to
+84
| /** | ||
| * TP-064: 完整展开的 IPv6 地址长度为 39 字符，NetworkUtils 应能正确识别。 | ||
| */ | ||
| void testIpmiAddressFullLengthIpv6() { | ||
| String fullIpv6 = "2001:0db8:0000:0000:0000:0000:0000:0001" | ||
|
|
||
| assert fullIpv6.length() == 39 : "TP-064: full IPv6 address should be 39 chars, got: ${fullIpv6.length()}" | ||
|
|
||
| boolean isV6 = IPv6NetworkUtils.isIpv6Address(fullIpv6) | ||
| assert isV6 : "TP-064: 39-char full IPv6 '$fullIpv6' should be recognized as valid IPv6" | ||
| logger.info("TP-064: full 39-char IPv6 '$fullIpv6' → isIpv6=$isV6 (accepted by interceptor)") |
There was a problem hiding this comment.
TP-064 并没有验证 ipmiAddress 字段容量。
这里仅仅证明了“一个 39 字符的字符串是合法 IPv6”，没有触达 ipmiAddress 字段、实体注解或数据库 schema，所以“字段可存储完整 IPv6 地址”的结论当前并不成立。建议改成检查实际列定义，或做一次落库/回读验证。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/src/test/groovy/org/zstack/test/integration/core/MnIpv6M3Case.groovy`
around lines 74 - 84, The test method testIpmiAddressFullLengthIpv6 only
validates IPv6 string length and format but does not exercise the ipmiAddress
field or persistence; update the test to either inspect the entity
mapping/column definition for ipmiAddress or perform a write-and-read roundtrip
using the entity that contains ipmiAddress (e.g., create an instance, set
ipmiAddress = "2001:0db8:...0001", save via the repository/DAO, flush/clear,
reload and assert the persisted value equals the input) so the test actually
verifies the ipmiAddress column can store the full 39-char IPv6 value. Ensure
you reference the test method testIpmiAddressFullLengthIpv6 and the ipmiAddress
field in your changes.
Comment on lines
+63
to
+70
| void testManagementIpColumnLength() { | ||
| Field field = HostAO.class.getDeclaredField("managementIp") | ||
| field.setAccessible(true) | ||
| Column col = field.getAnnotation(Column.class) | ||
| assert col != null : "TP-015: managementIp should have @Column annotation" | ||
| assert col.length() >= 39 : "TP-015: managementIp column length ${col.length()} is too short for 39-char full-expanded IPv6" | ||
| logger.info("TP-015: managementIp @Column length = ${col.length()}, sufficient for IPv6") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== HostAO annotation =="
fd -i 'HostAO.java' | while read -r f; do
echo "--- $f ---"
sed -n '1,120p' "$f"
done
echo "== Schema / migration definitions mentioning managementIp =="
fd -i 'schema\.sql|.*upgrade.*\.sql' | while read -r f; do
rg -n -C2 'managementIp' "$f" || true
doneRepository: MatheMatrix/zstack
Length of output: 6224
测试依赖 JPA 默认值，不符合项目 schema 权威性原则。
HostAO.managementIp 仅有 @Column 注解，未指定 length 属性。根据 ZStack 项目的设计规范，JPA 实体不需要通过 Column(length = ...) 来指定列宽——项目依赖 SQL migration 脚本作为权威 schema 源。因此，通过反射读取 col.length() 验证列宽是不适当的。
虽然实际 SQL 定义 managementIp varchar(255) 对 39 字符 full-expanded IPv6 充足，但测试应直接验证 SQL schema（如检查 migration 脚本或从数据库元数据读取列宽），而非依赖 JPA 注解默认值。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@test/src/test/groovy/org/zstack/test/integration/kvm/host/KvmHostIpv6Case.groovy`
around lines 63 - 70, The test testManagementIpColumnLength should not rely on
JPA default `@Column` length on HostAO.managementIp; remove the
reflection/assertion against Column.length() and instead validate the
authoritative schema by either reading the database metadata for the host
table's managementIp column (via the project's DataSource/JdbcTemplate and
INFORMATION_SCHEMA/COLUMNS) or by parsing the project's SQL migration that
defines the host table to assert it uses a varchar long enough for 39 chars;
update the test to query the DB metadata or migration content and assert the
actual column length >= 39 and log the found length.
Comment on lines
+47
to
+135
| /** | ||
| * TP-032: NFS 存储 CIDR = IPv6 CIDR，验证 IPv6 CIDR 格式可被工具方法正确识别， | ||
| * 不会因 INVALID_ARGUMENT_ERROR 逻辑被拒绝。 | ||
| * 直接验证 CIDR 内的 IP 可通过 isIpInCidr 匹配。 | ||
| */ | ||
| void testNfsCidrIpv6NotRejected() { | ||
| String ipv6Cidr = "2001:db8::/64" | ||
| String ipv6InCidr = "2001:db8::1" | ||
|
|
||
| // TP-032: IPv6 CIDR 应能被正确解析，CIDR 内的 IP 匹配成功（不报错） | ||
| boolean result = NetworkUtils.isIpInCidr(ipv6InCidr, ipv6Cidr) | ||
| assert result : "TP-032: IPv6 address $ipv6InCidr should be in CIDR $ipv6Cidr (NFS IPv6 CIDR should not be rejected)" | ||
| logger.info("TP-032: IPv6 CIDR '$ipv6Cidr' recognized correctly, isIpInCidr='$result'") | ||
| } | ||
|
|
||
| /** | ||
| * TP-033: NetworkUtils.isIpInCidr() 通过 IPv6NetworkUtils 正确匹配 IPv6 地址。 | ||
| */ | ||
| void testIsIpInCidrIpv6Match() { | ||
| // TP-033: IPv6 IP 在 IPv6 CIDR 内 → true | ||
| assert NetworkUtils.isIpInCidr("2001:db8::10", "2001:db8::/64") : | ||
| "TP-033: 2001:db8::10 should be in 2001:db8::/64" | ||
| // IPv4 IP 在 IPv4 CIDR 内 → true | ||
| assert NetworkUtils.isIpInCidr("192.168.1.10", "192.168.1.0/24") : | ||
| "TP-033: 192.168.1.10 should be in 192.168.1.0/24" | ||
| // IPv6 IP 对 IPv4 CIDR → false（不同协议不匹配） | ||
| assert !NetworkUtils.isIpInCidr("2001:db8::10", "10.0.0.0/8") : | ||
| "TP-033: IPv6 address should not match IPv4 CIDR" | ||
| logger.info("TP-033: isIpInCidr IPv6 matching logic verified") | ||
| } | ||
|
|
||
| /** | ||
| * TP-034: isIpInCidr() 无匹配时返回 false（fallback 逻辑）。 | ||
| */ | ||
| void testIsIpInCidrNoMatchFallback() { | ||
| // TP-034: IPv6 IP 对 IPv4 CIDR 不匹配 | ||
| assert !NetworkUtils.isIpInCidr("2001:db8::1", "192.168.0.0/24") : | ||
| "TP-034: IPv6 IP should not match IPv4 CIDR (fallback returns false)" | ||
| // IPv4 IP 对 IPv6 CIDR 不匹配 | ||
| assert !NetworkUtils.isIpInCidr("192.168.1.1", "2001:db8::/64") : | ||
| "TP-034: IPv4 IP should not match IPv6 CIDR (fallback returns false)" | ||
| logger.info("TP-034: isIpInCidr fallback (no match) returns false correctly") | ||
| } | ||
|
|
||
| /** | ||
| * TP-035: Ceph MonUri 解析 [IPv6] 括号输入。 | ||
| * buildAddr：IPv6 → "[ip]:port"，IPv4 → "ip:port" | ||
| */ | ||
| void testBuildAddrIpv6BracketFormat() { | ||
| // TP-035: IPv6 地址应加方括号 | ||
| String ipv6Addr = IPv6Utils.buildAddr("2001:db8::1", 6789) | ||
| assert ipv6Addr == "[2001:db8::1]:6789" : | ||
| "TP-035: buildAddr IPv6 should produce '[ip]:port' format, got: $ipv6Addr" | ||
| // IPv4 地址不加方括号 | ||
| String ipv4Addr = IPv6Utils.buildAddr("192.168.1.1", 6789) | ||
| assert ipv4Addr == "192.168.1.1:6789" : | ||
| "TP-035: buildAddr IPv4 should produce 'ip:port' format (no brackets), got: $ipv4Addr" | ||
| logger.info("TP-035: buildAddr IPv6='$ipv6Addr', IPv4='$ipv4Addr'") | ||
| } | ||
|
|
||
| /** | ||
| * TP-036: Ceph monAddr 输出格式 [ipv6]:port。 | ||
| * 验证 IPv6Utils.buildAddr() 对 IPv6 加括号。 | ||
| */ | ||
| void testCephMonAddrIpv6Format() { | ||
| // TP-036: Ceph mon IPv6 地址格式 [ipv6]:port | ||
| String monAddr = IPv6Utils.buildAddr("2001:db8:20::1", 6789) | ||
| assert monAddr == "[2001:db8:20::1]:6789" : | ||
| "TP-036: Ceph monAddr for IPv6 should be '[ipv6]:port', got: $monAddr" | ||
| // IPv4 不加括号 | ||
| String monAddrV4 = IPv6Utils.buildAddr("10.0.0.1", 6789) | ||
| assert monAddrV4 == "10.0.0.1:6789" : | ||
| "TP-036: Ceph monAddr for IPv4 should be 'ip:port' (no brackets), got: $monAddrV4" | ||
| logger.info("TP-036: Ceph monAddr IPv6='$monAddr', IPv4='$monAddrV4'") | ||
| } | ||
|
|
||
| /** | ||
| * TP-038: checkMigrateNetworkCidrOfHost fallback 逻辑。 | ||
| * IPv6 IP 在 IPv6 CIDR 内返回 true；不在范围内返回 false。 | ||
| */ | ||
| void testCheckMigrateNetworkCidrFallback() { | ||
| // TP-038: IPv6 IP 在 IPv6 CIDR 内 | ||
| assert NetworkUtils.isIpInCidr("2001:db8::100", "2001:db8::/64") : | ||
| "TP-038: 2001:db8::100 should be in 2001:db8::/64" | ||
| // fallback 场景：IP 不在指定 CIDR 内，返回 false | ||
| assert !NetworkUtils.isIpInCidr("2001:db8::1", "fd00::/8") : | ||
| "TP-038: 2001:db8::1 should not be in fd00::/8 (fallback returns false)" | ||
| logger.info("TP-038: checkMigrateNetworkCidrOfHost fallback logic verified") | ||
| } |
There was a problem hiding this comment.
这些用例没有覆盖它们宣称的业务路径。
当前断言只调用了 NetworkUtils.isIpInCidr() / IPv6Utils.buildAddr()。例如 Line 127-134 并没有触达 plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java 的 checkMigrateNetworkCidrOfHost()，extraIps 和 fallback 分支都没被测到；TP-032/TP-036 也没有经过 NFS/Ceph 的真实调用点。这个文件现在更像 utils 单测，覆盖不到实际回归面。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@test/src/test/groovy/org/zstack/test/integration/network/ipv6/MnIpv6StorageMigrationCase.groovy`
around lines 47 - 135, Tests only exercise utility methods
(NetworkUtils.isIpInCidr, IPv6Utils.buildAddr) but do not exercise the real
business paths (e.g., KVMHost.checkMigrateNetworkCidrOfHost, NFS/Ceph flows and
the extraIps/fallback branches). Update the test cases to invoke the actual code
paths: call or simulate the plugin logic that triggers
KVMHost.checkMigrateNetworkCidrOfHost (or drive the migration workflow in the
kvm plugin), create test hosts with extraIps set and add NFS and Ceph storage
resources so the code hits the NFS/Ceph call sites, and add assertions that
validate the extraIps handling and fallback branch outcomes rather than only
asserting the utility methods; reference NetworkUtils.isIpInCidr,
IPv6Utils.buildAddr, KVMHost.checkMigrateNetworkCidrOfHost and the extraIps
handling when implementing the test changes.
Comment on lines
+69
to
+85
| void testVtepVoColumnLength() { | ||
| checkVtepIpColumnLength(VtepVO.class, "VtepVO") | ||
| checkVtepIpColumnLength(RemoteVtepVO.class, "RemoteVtepVO") | ||
| } | ||
|
|
||
| private void checkVtepIpColumnLength(Class<?> voClass, String className) { | ||
| Field field = voClass.getDeclaredField("vtepIp") | ||
| field.setAccessible(true) | ||
| Column col = field.getAnnotation(Column.class) | ||
| assert col != null : "TP-040: $className.vtepIp should have @Column annotation" | ||
|
|
||
| int length = col.length() | ||
| // JPA @Column 默认 length 为 255；若未显式设置则为 255 | ||
| // 全展开 IPv6 最长 39 字符，255 >= 39 即可 | ||
| assert length >= 39 : | ||
| "TP-040: $className.vtepIp @Column length $length must be >= 39 to store full IPv6 address" | ||
| logger.info("TP-040: $className.vtepIp @Column length=$length (>= 39, IPv6-safe)") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Entity annotations =="
fd -i 'VtepVO.java|RemoteVtepVO.java' | while read -r f; do
echo "--- $f ---"
sed -n '1,120p' "$f"
done
echo "== Schema / migration definitions mentioning vtepIp =="
fd -i 'schema\.sql|.*upgrade.*\.sql' | while read -r f; do
rg -n -C2 'vtepIp' "$f" || true
doneRepository: MatheMatrix/zstack
Length of output: 7192
测试检查的是 JPA 注解默认值而非实际数据库列宽，无法验证 IPv6 容量；另外代码中含有中文注释。
根据 SQL 迁移脚本，VtepVO 和 RemoteVtepVO 的 vtepIp 列定义为 varchar(32)，不足以存储 39 字符的全展开 IPv6 地址。但由于 vtepIp 字段没有显式 @Column(length=...) 注解，col.length() 只会返回 JPA 默认值 255，所以测试会错误地通过。这个项目依赖 SQL 迁移脚本作为 schema 真相源，而非 JPA 注解生成 DDL，因此注解层面的验证无法保证数据库实际容量。
另外，第 77-78 行和第 80 行的中文注释（"JPA @Column 默认 length 为 255；若未显式设置则为 255" 以及 "全展开 IPv6 最长 39 字符，255 >= 39 即可"）违反了编码准则。所有注释应使用正确的无拼写错误的英文。
建议：改为直接查询 SQL 迁移脚本中 vtepIp 的列定义来验证长度是否 >= 39，或执行一次 39 字符 IPv6 地址的持久化/读取验证。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@test/src/test/groovy/org/zstack/test/integration/network/vxlan/VxlanIpv6Case.groovy`
around lines 69 - 85, The test checkVtepIpColumnLength currently inspects the
JPA `@Column` annotation (field vtepIp on VtepVO and RemoteVtepVO) which returns
the JPA default length rather than the actual DB column width, so update the
test to validate the real schema or behavior instead: either query the live
database/schema migration SQL (or use JDBC DatabaseMetaData) to read the
declared column size for table columns backing vtepIp, or perform a round-trip
persistence test that inserts and reads a full 39-character expanded IPv6 string
to ensure the DB accepts it; also remove the Chinese comments around the length
checks and replace them with brief English comments explaining why we validate
the DB schema/round-trip rather than the annotation.
Comment on lines
+564
to
+568
| public static boolean isIpInCidr(String ip, String cidr) { | ||
| if (ip == null || cidr == null) return false; | ||
| if (isIpv4Address(ip)) return isIpv4InCidr(ip, cidr); | ||
| if (IPv6NetworkUtils.isIpv6Address(ip)) return IPv6NetworkUtils.isIpv6InCidrRange(ip, cidr); | ||
| return false; |
There was a problem hiding this comment.
跨 IP 族判断缺失会导致错误路径执行
Line [566] 在 ip 为 IPv4 时直接进入 IPv4 CIDR 判断，但未先校验 cidr 也是 IPv4。跨族输入（如 IPv4 + IPv6 CIDR）应返回 false，不要走错分支导致异常路径。
建议修复
public static boolean isIpInCidr(String ip, String cidr) {
- if (ip == null || cidr == null) return false;
- if (isIpv4Address(ip)) return isIpv4InCidr(ip, cidr);
- if (IPv6NetworkUtils.isIpv6Address(ip)) return IPv6NetworkUtils.isIpv6InCidrRange(ip, cidr);
+ if (ip == null || cidr == null) {
+ return false;
+ }
+ if (isIpv4Address(ip)) {
+ return isCidr(cidr, IPv6Constants.IPv4) && isIpv4InCidr(ip, cidr);
+ }
+ if (IPv6NetworkUtils.isIpv6Address(ip)) {
+ return isCidr(cidr, IPv6Constants.IPv6) && IPv6NetworkUtils.isIpv6InCidrRange(ip, cidr);
+ }
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/src/main/java/org/zstack/utils/network/NetworkUtils.java` around lines
564 - 568, The isIpInCidr method currently only checks the ip family and calls
isIpv4InCidr or IPv6NetworkUtils.isIpv6InCidrRange, which can mis-handle
cross-family inputs; update isIpInCidr to also verify the cidr family before
invoking the family-specific check: when isIpv4Address(ip) is true ensure cidr
represents an IPv4 CIDR (e.g., by using isIpv4Address on the cidr's base address
or a helper like isCidrIpv4) and only then call isIpv4InCidr(ip, cidr),
otherwise return false; similarly, when IPv6NetworkUtils.isIpv6Address(ip) is
true ensure cidr is an IPv6 CIDR before calling
IPv6NetworkUtils.isIpv6InCidrRange(ip, cidr), else return false.
|
|
||
| info.setMaster(ShellUtils.runAndReturn(String.format( | ||
| "ip addr show %s | grep -q '[^0-9]%s[^0-9]'", info.getNic(), info.getDbvip())).isReturnCode(0)); | ||
| "ip addr show %s | grep -q \" %s/\"", info.getNic(), info.getDbvip())).isReturnCode(0)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sample='inet 10x0x0x1/24 brd 10.0.0.255 scope global eth0'
target=' 10.0.0.1/'
echo "$sample" | grep -q "$target" && echo "regex_match=true" || echo "regex_match=false"
echo "$sample" | grep -Fq "$target" && echo "fixed_match=true" || echo "fixed_match=false"Repository: MatheMatrix/zstack
Length of output: 97
🏁 Script executed:
find . -name "ZSha2Helper.java" -type fRepository: MatheMatrix/zstack
Length of output: 124
🏁 Script executed:
head -60 ./utils/src/main/java/org/zstack/utils/zsha2/ZSha2Helper.java | cat -nRepository: MatheMatrix/zstack
Length of output: 2590
🏁 Script executed:
rg 'grep\s+-q' utils/src/main/java/org/zstack/utils/zsha2/ -A2 -B2Repository: MatheMatrix/zstack
Length of output: 557
使用 grep -F 进行固定字符串匹配，避免正则表达式误判
Line 42 的 grep -q 使用正则表达式匹配，IPv4 地址中的 . 会被当作任意字符。虽然 info.getDbvip() 应为有效的 IP 地址，但使用正则匹配存在风险：若输入包含未转义的正则字符或验证逻辑改变，可能导致误匹配。建议改用 grep -Fq 进行固定字符串匹配以确保准确性。
建议修复
info.setMaster(ShellUtils.runAndReturn(String.format(
- "ip addr show %s | grep -q \" %s/\"", info.getNic(), info.getDbvip())).isReturnCode(0));
+ "ip addr show %s | grep -Fq \" %s/\"", info.getNic(), info.getDbvip())).isReturnCode(0));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "ip addr show %s | grep -q \" %s/\"", info.getNic(), info.getDbvip())).isReturnCode(0)); | |
| "ip addr show %s | grep -Fq \" %s/\"", info.getNic(), info.getDbvip())).isReturnCode(0)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/src/main/java/org/zstack/utils/zsha2/ZSha2Helper.java` at line 42, The
shell command string that checks for the DB VIP uses grep -q which treats the
pattern as a regex, so in ZSha2Helper where the command is built ("ip addr show
%s | grep -q \" %s/\"") replace the grep invocation with fixed-string matching
(grep -Fq) to avoid regex interpretation of characters in info.getDbvip();
update the command string to use "grep -Fq" so the check reliably matches the
literal IP substring.
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: http://jira.zstack.io/browse/ZCF-2893
Background
Existing host-bound VM NIC features (OVS DPDK port, security-group
rules, etc.) scatter the same add/delete logic across 10+ VM lifecycle
extension points (create/start/stop/destroy/migrate pre & post/
reattach/recover/vmha/kvmPingAgentNoFailure). Every new feature repeats
the full fan-out by hand, which has caused regressions.
This MR introduces a single, narrow abstraction — each feature only
implements
setupOnHost/cleanupFromHostplus optional reconcile,and the framework drives every lifecycle scenario.
Scope
In this MR (PH-1..PH-4):
VmNicLifecycleExtensionPointinterface (5 default methods),GlobalConfig, Spring wiring.
VmNicLifecycleManager— single dispatch entry point withapplicability filter, fail-fast on VM state changes, fail-logged on
reconcile/cleanup, parallel fan-out with configurable cap, full
observability counters.
VmNicLifecycleKvmBridge— wires into all 10 existing VM/VmNiclifecycle points plus
kvmPingAgentNoFailurereconcile.Deferred (PH-5 follow-up MR): Spock integration tests, perf
benchmark with >=100 NICs.
Test Results
Design Docs
docs/plans/(all codex-5.3reviewed).
Risk
Pure addition; no existing lifecycle wiring removed. GlobalConfig
defaults disable the manager until a feature opts in, so 5.5.12 ZNS
behaviour is unchanged until an explicit consumer is registered.
sync from gitlab !9678