<feature>[storage]: create volume with chosen protocol#4215
Open
zstack-robot-2 wants to merge 3 commits into
Open
<feature>[storage]: create volume with chosen protocol#4215zstack-robot-2 wants to merge 3 commits into
zstack-robot-2 wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 3 hours, 37 minutes, and 33 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (26)
Warning
|
| Layer / File(s) | Summary |
|---|---|
API消息与事件定义 header/src/main/java/org/zstack/header/storage/primary/APIChangeVolumeProtocol*, header/src/main/java/org/zstack/header/volume/CreateDataVolumeMsg.java, header/src/main/java/org/zstack/header/storage/primary/UpdatePrimaryStorageHostStatusMsg.java |
新增APIChangeVolumeProtocolMsg请求和APIChangeVolumeProtocolEvent响应定义卷协议变更REST契约；CreateDataVolumeMsg和APICreateDataVolumeMsg增加可选protocol字段支持创建卷时指定协议；UpdatePrimaryStorageHostStatusMsg新增protocol字段支持per-protocol主机状态更新。 |
按协议主机连通性基础设施 conf/db/upgrade/V5.5.28__schema.sql, header/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageHostProtocolRef*.java, sdk/src/main/java/org/zstack/sdk/ExternalPrimaryStorageHostProtocolRefInventory.java |
新建ExternalPrimaryStorageHostProtocolRefVO数据表及其JPA元模型，以host×primaryStorage×protocol为组合主键追踪连通性；提供对应的库存映射类ExternalPrimaryStorageHostProtocolRefInventory支持序列化和库存展开。 |
协议连通性查询API header/src/main/java/org/zstack/header/storage/addon/primary/APIQuery*, sdk/src/main/java/org/zstack/sdk/QueryExternalPrimaryStorageHostProtocolRef*.java |
新增APIQueryExternalPrimaryStorageHostProtocolRefMsg支持按primaryStorageUuid或全量查询连通性记录；提供SDK QueryExternalPrimaryStorageHostProtocolRefAction和QueryExternalPrimaryStorageHostProtocolRefResult以及中英文文档。 |
SDK操作类扩展 sdk/src/main/java/org/zstack/sdk/ChangeVolumeProtocol*.java, sdk/src/main/java/org/zstack/sdk/CreateDataVolumeAction.java, sdk/src/main/java/SourceClassMap.java |
新增ChangeVolumeProtocolAction和ChangeVolumeProtocolResult支持变更协议的同步/异步调用；扩展CreateDataVolumeAction的protocol参数；更新SourceClassMap映射外接存储协议引用库存类。 |
API请求验证与拦截 storage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.java, storage/src/main/java/org/zstack/storage/volume/VolumeApiInterceptor.java |
在PrimaryStorageApiInterceptor和VolumeApiInterceptor中增加protocol字段导入；对APIChangeVolumeProtocolMsg执行验证：检查协议有效性、primaryStorageUuid非空、主存储暴露协议、卷存在、协议已变更、挂载VM停机约束。 |
存储服务协议变更执行 storage/src/main/java/org/zstack/storage/volume/VolumeBase.java, storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java, storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java |
VolumeBase新增handle(APIChangeVolumeProtocolMsg)通过数据库更新VolumeVO.protocol并发布事件；VolumeManagerImpl在创建卷流程中传递protocol参数；ExternalPrimaryStorage在UpdatePrimaryStorageHostStatusMsg处理中支持per-protocol状态更新。 |
外部存储按协议健康检测 plugin/externalStorage/src/main/java/org/zstack/externalStorage/primary/kvm/ExternalPrimaryStorageKvmFactory.java |
重构checkHostStatus以按协议逐条处理NodeHealthy映射，支持默认协议折叠为host-level状态；updateHostStatus消息新增protocol参数实现协议维度的状态写入。 |
ZBS Vhost目标健康检测 plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsGlobalProperty.java, plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java, header/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.java |
VHOST_TARGET_CORES默认值改为空字符串由代理根据CPU数计算；ZbsStorageController新增VHOST_TARGET_HEALTH_PATH常量和Vhost目标健康检测逻辑，扩展VhostActivateCmd添加coreCount，新增VhostTargetHealthCmd/Rsp；PrimaryStorageControllerSvc新增onProtocolAdded回调。 |
系统标签与文档 storage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.java, docs/zbs-vhost/*.md, testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy |
VolumeSystemTags新增VOLUME_PROTOCOL_TOKEN和VOLUME_PROTOCOL标签用于卷创建时协议选择；前端对接文档详细定义三类API契约、错误处理、UI门控与配色；ApiHelper新增changeVolumeProtocol和queryExternalPrimaryStorageHostProtocolRef包装方法。 |
集成测试 test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovy |
新增testChangeVolumeProtocol验证离线协议切换和拒绝场景；testCreateDataVolumeWithExplicitProtocol验证创建卷显式protocol参数优先级；testAddProtocolPreparesHostsAndRecordsProtocolRefs验证协议增补后主机准备与连通性记录维护及自愈能力。 |
Estimated Code Review Effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
🐰 协议在卷上，健康在主机，
表格追踪每个连接的细节，
离线切换无烦恼，API守好门，
ZBS探测目标活力，test覆盖全链路！
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 2.48% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | PR标题清晰概括了主要功能：在卷创建时支持指定协议。标题简洁、具体，准确反映了本次变更的核心内容。 |
| Description check | ✅ Passed | PR描述详细说明了变更内容、实现细节和测试验证。描述与变更集相关，包括API层改动、校验逻辑、测试场景和验证结果。 |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
sync/jin.ma/feature-zbs-vhost
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@header/src/main/java/org/zstack/header/storage/primary/APIChangeVolumeProtocolMsg.java`:
- Around line 10-15: The REST annotation on APIChangeVolumeProtocolMsg currently
uses HttpMethod.POST but this operation updates an existing volume resource;
change the annotation's method from HttpMethod.POST to HttpMethod.PUT in the
`@RestRequest` on class APIChangeVolumeProtocolMsg to reflect an update semantics
(this will also cause the SDK's ChangeVolumeProtocolAction to be synced when you
regenerate the SDK).
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 2391-2395: The SQL update on VolumeVO
(SQL.New(VolumeVO.class).eq(VolumeVO_.uuid,
msg.getVolumeUuid()).set(VolumeVO_.protocol, msg.getProtocol()).update())
currently always calls completion.success() even when no rows are affected;
change this to capture the update() return (affected rows) and if it's >0 call
completion.success(), otherwise call completion.fail(...) with a clear error
(e.g., "no volume updated for uuid ...") so callers don't treat a no-op as
success; keep existing use of msg.getVolumeUuid(), msg.getProtocol(), and
completion.success()/completion.fail() in the revised flow.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31c42bcb-4da0-43ea-ade2-192be869d64b
⛔ Files ignored due to path filters (1)
conf/serviceConfig/primaryStorage.xmlis excluded by!**/*.xml
📒 Files selected for processing (16)
header/src/main/java/org/zstack/header/storage/primary/APIChangeVolumeProtocolEvent.javaheader/src/main/java/org/zstack/header/storage/primary/APIChangeVolumeProtocolMsg.javaheader/src/main/java/org/zstack/header/volume/APICreateDataVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeMsg.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsGlobalProperty.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javasdk/src/main/java/org/zstack/sdk/ChangeVolumeProtocolAction.javasdk/src/main/java/org/zstack/sdk/ChangeVolumeProtocolResult.javasdk/src/main/java/org/zstack/sdk/CreateDataVolumeAction.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeApiInterceptor.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovytestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
Comment on lines
+2391
to
+2395
| SQL.New(VolumeVO.class) | ||
| .eq(VolumeVO_.uuid, msg.getVolumeUuid()) | ||
| .set(VolumeVO_.protocol, msg.getProtocol()) | ||
| .update(); | ||
| completion.success(); |
There was a problem hiding this comment.
校验更新结果，避免“未修改却返回成功”
Line 2391-2395 当前无论 SQL 是否命中都会 completion.success()。当卷在拦截校验后被并发删除/不存在时，会出现“协议未更新但 API 成功”的错误语义，并可能影响后续基于该成功路径构建返回体的稳定性。建议按 update() 影响行数判定成功与失败，0 行时返回明确错误。
建议修复
`@Override`
protected void doChangeVolumeProtocol(APIChangeVolumeProtocolMsg msg, Completion completion) {
// offline switch: persist the new protocol on the volume; the next activate
// (on vm start) builds the active path for the new protocol.
- SQL.New(VolumeVO.class)
+ int updated = SQL.New(VolumeVO.class)
.eq(VolumeVO_.uuid, msg.getVolumeUuid())
.set(VolumeVO_.protocol, msg.getProtocol())
.update();
- completion.success();
+ if (updated == 0) {
+ completion.fail(operr("volume[uuid:%s] not found when changing protocol", msg.getVolumeUuid()));
+ return;
+ }
+ completion.success();
}🤖 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
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`
around lines 2391 - 2395, The SQL update on VolumeVO
(SQL.New(VolumeVO.class).eq(VolumeVO_.uuid,
msg.getVolumeUuid()).set(VolumeVO_.protocol, msg.getProtocol()).update())
currently always calls completion.success() even when no rows are affected;
change this to capture the update() return (affected rows) and if it's >0 call
completion.success(), otherwise call completion.fail(...) with a clear error
(e.g., "no volume updated for uuid ...") so callers don't treat a no-op as
success; keep existing use of msg.getVolumeUuid(), msg.getProtocol(), and
completion.success()/completion.fail() in the revised flow.
MatheMatrix
force-pushed
the
sync/jin.ma/feature-zbs-vhost
branch
from
79a9e3c to
930f34d
Compare
Auto-deploy the SPDK vhost target on first activate: registry pull (auto insecure-registry) with local-tar/url fallback, lazy, picking agent cores. reportNodeHealthy gains a Vhost branch probing the target container. Volume output protocol is now selectable at create time (APICreateDataVolumeMsg gains a protocol param) and changeable offline afterwards. APIChangeVolumeProtocolMsg is a VolumeMessage handled in VolumeBase as a PUT action that fails on a 0-row update rather than reporting a no-op success. Both create and change validate the protocol is one the target primary storage exposes. SubCase covers all paths. Resolves: ZCF-0 Change-Id: I8c7f4199ddcb65bfe40a765c426aae607d2ac5d7
MatheMatrix
force-pushed
the
sync/jin.ma/feature-zbs-vhost
branch
from
930f34d to
df19328
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovy (1)
262-270: ⚡ Quick win让
testCreateDataVolumeWithExplicitProtocol()自给自足。这个用例现在依赖
testChangeVolumeProtocol()先把 CBD 加到同一个 PS。上游一旦提前失败，或者后面有人调整test()里的调用顺序，这里就会因为前置条件缺失而误报，定位也会被带偏。建议把addStorageProtocol(CBD)挪到本用例自己的准备阶段。🧩 建议修改
void testCreateDataVolumeWithExplicitProtocol() { - // CBD was added to the PS by testChangeVolumeProtocol; default is still Vhost. - // asking for CBD explicitly must win over the Vhost default. + addStorageProtocol { + uuid = ps.uuid + outputProtocol = VolumeProtocol.CBD.toString() + } + + // asking for CBD explicitly must win over the Vhost default. VolumeInventory vol = createDataVolume { name = "explicit-cbd" diskOfferingUuid = diskOffering.uuid🤖 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 `@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovy` around lines 262 - 270, testCreateDataVolumeWithExplicitProtocol currently depends on testChangeVolumeProtocol to add the CBD protocol to the primary storage; make the test self-contained by adding the call that enables CBD for the target primary storage inside this test's setup. Locate testCreateDataVolumeWithExplicitProtocol and, before invoking createDataVolume, call the same helper used to register protocols (e.g. addStorageProtocol(VolumeProtocol.CBD) or the project-specific helper that modifies the PS protocol list) against ps.uuid so CBD is present for this test only; ensure this setup runs even if testChangeVolumeProtocol does not run.
🤖 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 `@conf/db/upgrade/V5.5.28__schema.sql`:
- Around line 7-8: Replace the zero timestamp default on the createDate column
with a non-zero sentinel value so the migration works in strict SQL mode and
coexists with the existing lastOpDate TIMESTAMP that has ON UPDATE
CURRENT_TIMESTAMP; specifically, change the createDate definition (`createDate`
timestamp NOT NULL DEFAULT '0000-00-00 00:00:00') to use a non-zero sentinel
(e.g., '1970-01-01 00:00:01') rather than DEFAULT CURRENT_TIMESTAMP, leaving
`lastOpDate` (`lastOpDate` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00' ON
UPDATE CURRENT_TIMESTAMP) behavior intact.
In `@docs/zbs-vhost/40-frontend-protocol-api.md`:
- Around line 18-20: 在 docs/zbs-vhost/40-frontend-protocol-api.md 中的 5 个 fenced
code block（例如包含文本 "NVMEoF | iSCSI | Vhost | CBD | NBD | RBD"、示例请求行 "GET
/zstack/v1/external-primary-storage/host-protocol-refs?q=primaryStorageUuid=xxx"
/ "GET
/zstack/v1/external-primary-storage/{primaryStorageUuid}/host-protocol-refs"，以及请求体起始行
"POST /zstack/v1/primary-storage/protocol" 和 "{ \"params\": {"）都缺少 language
标识，导致 MD040 警告；为每个代码块在起始 ``` 后加上合适的语言标识（至少使用 text、http 或 json，分别对应纯文本、HTTP 请求示例和
JSON 请求体），确保每个块的语法高亮与示例内容匹配（例如将表格行标为 text，将 GET/POST 请求示例标为 http，将 JSON 体标为
json）。
- Around line 118-123: The documentation contains contradictory statements about
DiskOfferingVO.protocol (mentioned in §5 and §9)—decide a single authoritative
status (either "reserved/planned" or "deprecated/cancelled"), update the
protocol source priority list so it reflects that decision (adjust the numbered
list where DiskOfferingVO.protocol appears), and make the same change to the
other occurrence referenced (lines ~174-175) so both sections consistently state
the final status and behavior for DiskOfferingVO.protocol.
In
`@plugin/externalStorage/src/main/java/org/zstack/externalStorage/primary/kvm/ExternalPrimaryStorageKvmFactory.java`:
- Around line 223-255: The code doesn't handle a null/empty
returnValue.getHealthy(), which makes allMatch(...) return true and may
incorrectly set the host-level status to Connected or cause an NPE; update the
logic in the WhileDoneCompletion after the per-protocol loop to first check if
returnValue.getHealthy() is null or empty, and if so call
updateHostStatus(host.getUuid(), extPs.getUuid(), null,
PrimaryStorageHostStatus.Disconnected, operr(..., "no protocols reported for
host..."), compl) (include a clear error via operr like other branches);
otherwise keep the existing defaultReported check and the folded allMatch(...)
logic that maps StorageHealthy to PrimaryStorageHostStatus and calls
updateHostStatus.
In
`@sdk/src/main/java/org/zstack/sdk/QueryExternalPrimaryStorageHostProtocolRefResult.java`:
- Around line 6-11: 在 QueryExternalPrimaryStorageHostProtocolRefResult 中，字段
inventories 及其访问器 setInventories/getInventories 使用的是原始
java.util.List，导致反序列化丢失元素类型并变成 Map/LinkedTreeMap；将该字段和方法签名改为使用泛型
List<ExternalPrimaryStorageHostProtocolRefInventory>（或全限定名）并添加必要的 import，确保
getter/setter 的参数和返回类型一致，以便 SDK 反序列化出正确的
ExternalPrimaryStorageHostProtocolRefInventory 对象。
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 343-355: The updateHostProtocolStatus method currently does a
read-then-write using Q.New(...).find() followed by dbf.persist/update which
races under concurrent reports; change it to an atomic upsert on the natural key
(primaryStorageUuid, hostUuid, protocol) in
ExternalPrimaryStorageHostProtocolRefVO: attempt an UPDATE by that key first
(set status = newStatus), check affected rows and if zero then INSERT the new
row while catching unique-constraint violations to retry the UPDATE path; ensure
the database defines a unique constraint on (primaryStorageUuid, hostUuid,
protocol) so concurrent callers converge to a single row.
- Around line 2435-2440: The failure handler in onProtocolAdded() must not
proceed to register the protocol; remove the call to
ExternalPrimaryStorage.super.doAddProtocol(msg, completion) from the
fail(ErrorCode) path and instead call completion.fail(errorCode) (or otherwise
propagate the error) so the protocol is not exposed when host preparation
failed. Ensure doAddProtocol(msg, completion) is only invoked from the
successful onProtocolAdded() path (or after verifying at least one host is
Connected and prepared), and keep the existing logger.warn with the error
details.
In `@storage/src/main/java/org/zstack/storage/volume/VolumeApiInterceptor.java`:
- Around line 491-509: The validateVolumeProtocol method currently treats only
null as "unspecified" but should also treat empty/blank strings as unspecified;
update the early-exit check in validateVolumeProtocol so that if protocol is
null or blank (e.g., protocol.trim().isEmpty() or using a StringUtils.isBlank
equivalent) it returns immediately, leaving the enum check and the
PrimaryStorageOutputProtocolRefVO existence check
(Q.New(...).eq(...).isExists()) to run only for truly explicitly-specified
protocol values; keep all existing exception types
(ApiMessageInterceptionException / argerr) unchanged for invalid/unknown
protocols.
In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovy`:
- Around line 468-472: The test changes the global config "host.ping.interval"
to 1 via updateGlobalConfig but never restores it; capture the original value
before calling updateGlobalConfig (in the test method inside class
ZbsVhostVolumeCase or the surrounding setup block), and ensure you restore it in
the finally block (or add a finally if none) by calling updateGlobalConfig with
the saved original value so the global state is not leaked to subsequent tests.
- Around line 434-457: The test can pass spuriously because an existing
ExternalPrimaryStorageHostProtocolRefVO(protocol=Vhost) row may remain from
attachPrimaryStorageToCluster(); before calling addStorageProtocol (and after
deleting PrimaryStorageOutputProtocolRefVO), also delete any
ExternalPrimaryStorageHostProtocolRefVO rows matching
primaryStorageUuid=ps.uuid, hostUuid=host.uuid,
protocol=VolumeProtocol.Vhost.toString() so the subsequent isExists() truly
verifies a new/write update, or alternatively change the assertion to verify
that the existing ExternalPrimaryStorageHostProtocolRefVO's lastOpDate was
advanced by addStorageProtocol (compare previous lastOpDate to a later value)
and still assert ensureCalled reached VHOST_TARGET_ENSURE_PATH; locate these
changes around attachPrimaryStorageToCluster(), the addStorageProtocol block,
ensureCalled and the isExists() retry.
---
Nitpick comments:
In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovy`:
- Around line 262-270: testCreateDataVolumeWithExplicitProtocol currently
depends on testChangeVolumeProtocol to add the CBD protocol to the primary
storage; make the test self-contained by adding the call that enables CBD for
the target primary storage inside this test's setup. Locate
testCreateDataVolumeWithExplicitProtocol and, before invoking createDataVolume,
call the same helper used to register protocols (e.g.
addStorageProtocol(VolumeProtocol.CBD) or the project-specific helper that
modifies the PS protocol list) against ps.uuid so CBD is present for this test
only; ensure this setup runs even if testChangeVolumeProtocol does not run.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: af01e105-5d13-4417-a457-25de0a12559d
⛔ Files ignored due to path filters (3)
conf/persistence.xmlis excluded by!**/*.xmlconf/serviceConfig/externalPrimaryStorage.xmlis excluded by!**/*.xmlconf/serviceConfig/volume.xmlis excluded by!**/*.xml
📒 Files selected for processing (34)
conf/db/upgrade/V5.5.28__schema.sqldocs/zbs-vhost/30-NEXT-SESSION.mddocs/zbs-vhost/40-frontend-protocol-api.mdheader/src/main/java/org/zstack/header/storage/addon/primary/APIQueryExternalPrimaryStorageHostProtocolRefMsg.javaheader/src/main/java/org/zstack/header/storage/addon/primary/APIQueryExternalPrimaryStorageHostProtocolRefMsgDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/addon/primary/APIQueryExternalPrimaryStorageHostProtocolRefReply.javaheader/src/main/java/org/zstack/header/storage/addon/primary/APIQueryExternalPrimaryStorageHostProtocolRefReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageHostProtocolRefInventory.javaheader/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageHostProtocolRefVO.javaheader/src/main/java/org/zstack/header/storage/addon/primary/ExternalPrimaryStorageHostProtocolRefVO_.javaheader/src/main/java/org/zstack/header/storage/addon/primary/PrimaryStorageControllerSvc.javaheader/src/main/java/org/zstack/header/storage/primary/APIChangeVolumeProtocolEvent.javaheader/src/main/java/org/zstack/header/storage/primary/APIChangeVolumeProtocolMsg.javaheader/src/main/java/org/zstack/header/storage/primary/UpdatePrimaryStorageHostStatusMsg.javaheader/src/main/java/org/zstack/header/volume/APICreateDataVolumeMsg.javaheader/src/main/java/org/zstack/header/volume/CreateDataVolumeMsg.javaplugin/externalStorage/src/main/java/org/zstack/externalStorage/primary/kvm/ExternalPrimaryStorageKvmFactory.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsGlobalProperty.javaplugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.javasdk/src/main/java/SourceClassMap.javasdk/src/main/java/org/zstack/sdk/ChangeVolumeProtocolAction.javasdk/src/main/java/org/zstack/sdk/ChangeVolumeProtocolResult.javasdk/src/main/java/org/zstack/sdk/CreateDataVolumeAction.javasdk/src/main/java/org/zstack/sdk/ExternalPrimaryStorageHostProtocolRefInventory.javasdk/src/main/java/org/zstack/sdk/QueryExternalPrimaryStorageHostProtocolRefAction.javasdk/src/main/java/org/zstack/sdk/QueryExternalPrimaryStorageHostProtocolRefResult.javastorage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javastorage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.javastorage/src/main/java/org/zstack/storage/volume/VolumeApiInterceptor.javastorage/src/main/java/org/zstack/storage/volume/VolumeBase.javastorage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.javastorage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovytestlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
✅ Files skipped from review due to trivial changes (8)
- storage/src/main/java/org/zstack/storage/volume/VolumeSystemTags.java
- header/src/main/java/org/zstack/header/storage/addon/primary/APIQueryExternalPrimaryStorageHostProtocolRefMsgDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/storage/addon/primary/APIQueryExternalPrimaryStorageHostProtocolRefReplyDoc_zh_cn.groovy
- storage/src/main/java/org/zstack/storage/primary/PrimaryStorageApiInterceptor.java
- sdk/src/main/java/SourceClassMap.java
- sdk/src/main/java/org/zstack/sdk/ExternalPrimaryStorageHostProtocolRefInventory.java
- docs/zbs-vhost/30-NEXT-SESSION.md
- sdk/src/main/java/org/zstack/sdk/QueryExternalPrimaryStorageHostProtocolRefAction.java
🚧 Files skipped from review as they are similar to previous changes (9)
- header/src/main/java/org/zstack/header/volume/APICreateDataVolumeMsg.java
- sdk/src/main/java/org/zstack/sdk/CreateDataVolumeAction.java
- header/src/main/java/org/zstack/header/volume/CreateDataVolumeMsg.java
- storage/src/main/java/org/zstack/storage/volume/VolumeManagerImpl.java
- sdk/src/main/java/org/zstack/sdk/ChangeVolumeProtocolResult.java
- header/src/main/java/org/zstack/header/storage/primary/APIChangeVolumeProtocolEvent.java
- plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsGlobalProperty.java
- testlib/src/main/java/org/zstack/testlib/ApiHelper.groovy
- plugin/zbs/src/main/java/org/zstack/storage/zbs/ZbsStorageController.java
Comment on lines
+7
to
+8
| `createDate` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00', | ||
| `lastOpDate` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00' ON UPDATE CURRENT_TIMESTAMP, |
There was a problem hiding this comment.
替换零时间默认值，避免升级脚本在严格模式下失败。
这里的 0000-00-00 00:00:00 既违反了升级脚本规范，也会在开启严格 SQL 模式时直接导致建表失败。并且这张表已经有 lastOpDate ... ON UPDATE CURRENT_TIMESTAMP，createDate 也不能简单改成 DEFAULT CURRENT_TIMESTAMP；按仓库现有兼容写法，应使用非零哨兵值。
As per coding guidelines, SQL 升级脚本不能使用 DEFAULT 0000-00-00 00:00:00；based on learnings，当同表存在第二个带 ON UPDATE CURRENT_TIMESTAMP 的 TIMESTAMP 列时，createDate 需要使用非零哨兵默认值而不是 DEFAULT CURRENT_TIMESTAMP。
建议修改
- `createDate` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00',
- `lastOpDate` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00' ON UPDATE CURRENT_TIMESTAMP,
+ `createDate` timestamp NOT NULL DEFAULT '2000-01-01 00:00:00',
+ `lastOpDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,📝 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
| `createDate` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00', | |
| `lastOpDate` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00' ON UPDATE CURRENT_TIMESTAMP, | |
| `createDate` timestamp NOT NULL DEFAULT '2000-01-01 00:00:00', | |
| `lastOpDate` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, |
🤖 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 `@conf/db/upgrade/V5.5.28__schema.sql` around lines 7 - 8, Replace the zero
timestamp default on the createDate column with a non-zero sentinel value so the
migration works in strict SQL mode and coexists with the existing lastOpDate
TIMESTAMP that has ON UPDATE CURRENT_TIMESTAMP; specifically, change the
createDate definition (`createDate` timestamp NOT NULL DEFAULT '0000-00-00
00:00:00') to use a non-zero sentinel (e.g., '1970-01-01 00:00:01') rather than
DEFAULT CURRENT_TIMESTAMP, leaving `lastOpDate` (`lastOpDate` timestamp NOT NULL
DEFAULT '0000-00-00 00:00:00' ON UPDATE CURRENT_TIMESTAMP) behavior intact.
Sources: Coding guidelines, Learnings
Comment on lines
+18
to
+20
| ``` | ||
| NVMEoF | iSCSI | Vhost | CBD | NBD | RBD | ||
| ``` |
There was a problem hiding this comment.
给 fenced code block 补上 language 标识。
这 5 处代码块现在都会触发 markdownlint 的 MD040 warning，也不利于渲染高亮。至少补成 text、http 或 json 之一。
📝 建议修改
-```
+```text
NVMEoF | iSCSI | Vhost | CBD | NBD | RBD- +http
GET /zstack/v1/external-primary-storage/host-protocol-refs?q=primaryStorageUuid=xxx
GET /zstack/v1/external-primary-storage/{primaryStorageUuid}/host-protocol-refs
-```
+```json
POST /zstack/v1/primary-storage/protocol
{
"params": {
Also applies to: 48-51, 76-84, 101-110, 130-137
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/zbs-vhost/40-frontend-protocol-api.md` around lines 18 - 20, 在
docs/zbs-vhost/40-frontend-protocol-api.md 中的 5 个 fenced code block（例如包含文本
"NVMEoF | iSCSI | Vhost | CBD | NBD | RBD"、示例请求行 "GET
/zstack/v1/external-primary-storage/host-protocol-refs?q=primaryStorageUuid=xxx"
/ "GET
/zstack/v1/external-primary-storage/{primaryStorageUuid}/host-protocol-refs"，以及请求体起始行
"POST /zstack/v1/primary-storage/protocol" 和 "{ \"params\": {"）都缺少 language
标识，导致 MD040 警告；为每个代码块在起始 ``` 后加上合适的语言标识（至少使用 text、http 或 json，分别对应纯文本、HTTP 请求示例和
JSON 请求体），确保每个块的语法高亮与示例内容匹配（例如将表格行标为 text，将 GET/POST 请求示例标为 http，将 JSON 体标为
json）。
Source: Linters/SAST tools
Comment on lines
+223
to
+255
| // one status per protocol: the default protocol drives the host-level | ||
| // ref row while other protocols only update their own connectivity rows, | ||
| // so an unhealthy extra data path does not disconnect the whole host | ||
| new While<>(new ArrayList<>(returnValue.getHealthy().entrySet())).each((entry, pcompl) -> { | ||
| PrimaryStorageHostStatus status; | ||
| ErrorCode err = null; | ||
| if (entry.getValue() == StorageHealthy.Ok) { | ||
| status = PrimaryStorageHostStatus.Connected; | ||
| } else { | ||
| status = PrimaryStorageHostStatus.Disconnected; | ||
| err = operr(ORG_ZSTACK_EXTERNALSTORAGE_PRIMARY_KVM_10000, "external primary storage[uuid:%s, name:%s] protocol[%s] returns unhealthy status: %s", | ||
| extPs.getUuid(), extPs.getName(), entry.getKey(), entry.getValue()); | ||
| compl.addError(err); | ||
| } | ||
|
|
||
| if (hostStatus.get(extPs.getUuid()) != status) { | ||
| updateHostStatus(host.getUuid(), extPs.getUuid(), status, err, compl); | ||
| } else { | ||
| compl.done(); | ||
| } | ||
| updateHostStatus(host.getUuid(), extPs.getUuid(), entry.getKey().toString(), status, err, pcompl); | ||
| }).run(new WhileDoneCompletion(compl) { | ||
| @Override | ||
| public void done(ErrorCodeList errorCodeList) { | ||
| // a controller may only report the protocols this host | ||
| // serves (e.g. by hypervisor type); when the default | ||
| // protocol is absent keep the host-level row driven by | ||
| // folding the reported ones, as before | ||
| boolean defaultReported = returnValue.getHealthy().keySet().stream() | ||
| .anyMatch(p -> p.toString().equals(extPs.getDefaultProtocol())); | ||
| if (defaultReported) { | ||
| compl.done(); | ||
| return; | ||
| } | ||
| PrimaryStorageHostStatus folded = returnValue.getHealthy().values().stream() | ||
| .allMatch(h -> h == StorageHealthy.Ok) | ||
| ? PrimaryStorageHostStatus.Connected : PrimaryStorageHostStatus.Disconnected; | ||
| updateHostStatus(host.getUuid(), extPs.getUuid(), null, folded, null, compl); |
There was a problem hiding this comment.
先处理 healthy 为空的情况，否则这里会把“未探测”误写成 Connected。
这段逻辑对空集合做折叠时，allMatch(...) 会返回 true。也就是说控制器一旦返回空 map，这里既可能在 entrySet() 处直接 NPE，也可能在没有任何协议探测结果的情况下把 host-level 状态更新成 Connected。null/empty 应该先按探测失败处理，再决定是否写 Disconnected 和 reason。
建议修改
public void success(NodeHealthy returnValue) {
+ if (returnValue.getHealthy() == null || returnValue.getHealthy().isEmpty()) {
+ ErrorCode err = operr(
+ ORG_ZSTACK_EXTERNALSTORAGE_PRIMARY_KVM_10000,
+ "external primary storage[uuid:%s, name:%s] returned no protocol health for host[uuid:%s, name:%s]",
+ extPs.getUuid(), extPs.getName(), host.getUuid(), host.getName()
+ );
+ compl.addError(err);
+ updateHostStatus(host.getUuid(), extPs.getUuid(), null,
+ PrimaryStorageHostStatus.Disconnected, err, compl);
+ return;
+ }
+
// one status per protocol: the default protocol drives the host-level
// ref row while other protocols only update their own connectivity rows,
// so an unhealthy extra data path does not disconnect the whole host🤖 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/externalStorage/src/main/java/org/zstack/externalStorage/primary/kvm/ExternalPrimaryStorageKvmFactory.java`
around lines 223 - 255, The code doesn't handle a null/empty
returnValue.getHealthy(), which makes allMatch(...) return true and may
incorrectly set the host-level status to Connected or cause an NPE; update the
logic in the WhileDoneCompletion after the per-protocol loop to first check if
returnValue.getHealthy() is null or empty, and if so call
updateHostStatus(host.getUuid(), extPs.getUuid(), null,
PrimaryStorageHostStatus.Disconnected, operr(..., "no protocols reported for
host..."), compl) (include a clear error via operr like other branches);
otherwise keep the existing defaultReported check and the folded allMatch(...)
logic that maps StorageHealthy to PrimaryStorageHostStatus and calls
updateHostStatus.
Comment on lines
+6
to
+11
| public java.util.List inventories; | ||
| public void setInventories(java.util.List inventories) { | ||
| this.inventories = inventories; | ||
| } | ||
| public java.util.List getInventories() { | ||
| return this.inventories; |
There was a problem hiding this comment.
把 inventories 改成带元素类型的列表。
现在是原始 List，SDK 侧反序列化拿不到元素类型信息，结果很容易落成 Map/LinkedTreeMap 而不是 ExternalPrimaryStorageHostProtocolRefInventory。调用方一旦按 inventory 取字段，就会在运行时踩雷。
建议修改
- public java.util.List inventories;
- public void setInventories(java.util.List inventories) {
+ public java.util.List<ExternalPrimaryStorageHostProtocolRefInventory> inventories;
+ public void setInventories(java.util.List<ExternalPrimaryStorageHostProtocolRefInventory> inventories) {
this.inventories = inventories;
}
- public java.util.List getInventories() {
+ public java.util.List<ExternalPrimaryStorageHostProtocolRefInventory> getInventories() {
return this.inventories;
}🤖 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
`@sdk/src/main/java/org/zstack/sdk/QueryExternalPrimaryStorageHostProtocolRefResult.java`
around lines 6 - 11, 在 QueryExternalPrimaryStorageHostProtocolRefResult 中，字段
inventories 及其访问器 setInventories/getInventories 使用的是原始
java.util.List，导致反序列化丢失元素类型并变成 Map/LinkedTreeMap；将该字段和方法签名改为使用泛型
List<ExternalPrimaryStorageHostProtocolRefInventory>（或全限定名）并添加必要的 import，确保
getter/setter 的参数和返回类型一致，以便 SDK 反序列化出正确的
ExternalPrimaryStorageHostProtocolRefInventory 对象。
Comment on lines
+343
to
+355
| private void updateHostProtocolStatus(String psUuid, String hostUuid, String protocol, PrimaryStorageHostStatus newStatus) { | ||
| ExternalPrimaryStorageHostProtocolRefVO ref = Q.New(ExternalPrimaryStorageHostProtocolRefVO.class) | ||
| .eq(ExternalPrimaryStorageHostProtocolRefVO_.primaryStorageUuid, psUuid) | ||
| .eq(ExternalPrimaryStorageHostProtocolRefVO_.hostUuid, hostUuid) | ||
| .eq(ExternalPrimaryStorageHostProtocolRefVO_.protocol, protocol) | ||
| .find(); | ||
| if (ref == null) { | ||
| ref = new ExternalPrimaryStorageHostProtocolRefVO(); | ||
| ref.setPrimaryStorageUuid(psUuid); | ||
| ref.setHostUuid(hostUuid); | ||
| ref.setProtocol(protocol); | ||
| ref.setStatus(newStatus); | ||
| dbf.persist(ref); |
There was a problem hiding this comment.
把协议状态行更新改成原子 upsert。
这里是 find() 再 persist()/update() 的两步式写法。并发收到同一个 (primaryStorageUuid, hostUuid, protocol) 的首条状态上报时，两条消息都可能先读到 null：没有唯一约束会写出重复行；有唯一约束则会把其中一条打成插入异常。这个表后面还要被查询接口和状态汇总消费，数据一旦分叉就不是瞬时噪音了。
建议把唯一性落到库层，并把这里改成原子 upsert（例如先按自然键更新，0 行时再插入并处理唯一键竞争），不要继续依赖“先查再插”的窗口。
🤖 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
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`
around lines 343 - 355, The updateHostProtocolStatus method currently does a
read-then-write using Q.New(...).find() followed by dbf.persist/update which
races under concurrent reports; change it to an atomic upsert on the natural key
(primaryStorageUuid, hostUuid, protocol) in
ExternalPrimaryStorageHostProtocolRefVO: attempt an UPDATE by that key first
(set status = newStatus), check affected rows and if zero then INSERT the new
row while catching unique-constraint violations to retry the UPDATE path; ensure
the database defines a unique constraint on (primaryStorageUuid, hostUuid,
protocol) so concurrent callers converge to a single row.
Comment on lines
+2435
to
+2440
| @Override | ||
| public void fail(ErrorCode errorCode) { | ||
| // the protocol is already registered; host preparation self-heals on the next ping | ||
| logger.warn(String.format("failed to prepare hosts for protocol[%s] on primary storage[uuid:%s]: %s", | ||
| msg.getOutputProtocol(), msg.getUuid(), errorCode.getDetails())); | ||
| ExternalPrimaryStorage.super.doAddProtocol(msg, completion); |
There was a problem hiding this comment.
onProtocolAdded() 失败时不要继续把协议暴露出去。
这里失败后仍然走 super.doAddProtocol()，会把协议注册到主存暴露列表；而本 PR 的创建卷/切协议校验又是按已暴露的 outputProtocol 放行。结果就是 API 会接受这个协议，但当前 Connected 主机并没有完成该协议的准备，直到下一次自愈前都会留下“校验成功、实际不可用”的时间窗。
更稳妥的语义是：只要当前有在线主机且准备失败，就让本次加协议失败；或者至少不要在失败路径里立即注册该协议。
建议修复
controller.onProtocolAdded(msg.getOutputProtocol(), HostInventory.valueOf(hostVOs), new Completion(completion) {
`@Override`
public void success() {
ExternalPrimaryStorage.super.doAddProtocol(msg, completion);
}
`@Override`
public void fail(ErrorCode errorCode) {
- // the protocol is already registered; host preparation self-heals on the next ping
logger.warn(String.format("failed to prepare hosts for protocol[%s] on primary storage[uuid:%s]: %s",
msg.getOutputProtocol(), msg.getUuid(), errorCode.getDetails()));
- ExternalPrimaryStorage.super.doAddProtocol(msg, completion);
+ completion.fail(errorCode);
}
});🤖 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
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`
around lines 2435 - 2440, The failure handler in onProtocolAdded() must not
proceed to register the protocol; remove the call to
ExternalPrimaryStorage.super.doAddProtocol(msg, completion) from the
fail(ErrorCode) path and instead call completion.fail(errorCode) (or otherwise
propagate the error) so the protocol is not exposed when host preparation
failed. Ensure doAddProtocol(msg, completion) is only invoked from the
successful onProtocolAdded() path (or after verifying at least one host is
Connected and prepared), and keep the existing logger.warn with the error
details.
Comment on lines
+491
to
+509
| // a chosen protocol is only meaningful against a specific primary storage that | ||
| // exposes it; mirror APIChangeVolumeProtocolMsg so create and change agree. | ||
| private void validateVolumeProtocol(String protocol, String primaryStorageUuid) { | ||
| if (protocol == null) { | ||
| return; | ||
| } | ||
| boolean known = Arrays.stream(VolumeProtocol.values()).anyMatch(p -> p.name().equals(protocol)); | ||
| if (!known) { | ||
| throw new ApiMessageInterceptionException(argerr("unsupported volume protocol[%s]", protocol)); | ||
| } | ||
| if (primaryStorageUuid == null) { | ||
| throw new ApiMessageInterceptionException(argerr("primaryStorageUuid is required when protocol[%s] is specified", protocol)); | ||
| } | ||
| if (!Q.New(PrimaryStorageOutputProtocolRefVO.class) | ||
| .eq(PrimaryStorageOutputProtocolRefVO_.primaryStorageUuid, primaryStorageUuid) | ||
| .eq(PrimaryStorageOutputProtocolRefVO_.outputProtocol, protocol) | ||
| .isExists()) { | ||
| throw new ApiMessageInterceptionException(argerr("primary storage[uuid:%s] does not expose output protocol[%s]", primaryStorageUuid, protocol)); | ||
| } |
There was a problem hiding this comment.
把空字符串也当成“未指定协议”处理。
这里现在只对 null 直接放行，但本 PR 的目标是“protocol 为空时回落到主存默认协议”。如果上游把未填写的可选字段序列化成 ""，这段逻辑会在 Line 497-500 把它判成非法协议，导致本应继续走默认协议的创建请求被拦截失败。建议这里按 blank 语义短路，后面的枚举/主存暴露校验只针对真正显式指定的协议执行。
💡 建议修改
private void validateVolumeProtocol(String protocol, String primaryStorageUuid) {
- if (protocol == null) {
+ if (protocol == null || protocol.trim().isEmpty()) {
return;
}
boolean known = Arrays.stream(VolumeProtocol.values()).anyMatch(p -> p.name().equals(protocol));🤖 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 `@storage/src/main/java/org/zstack/storage/volume/VolumeApiInterceptor.java`
around lines 491 - 509, The validateVolumeProtocol method currently treats only
null as "unspecified" but should also treat empty/blank strings as unspecified;
update the early-exit check in validateVolumeProtocol so that if protocol is
null or blank (e.g., protocol.trim().isEmpty() or using a StringUtils.isBlank
equivalent) it returns immediately, leaving the enum check and the
PrimaryStorageOutputProtocolRefVO existence check
(Q.New(...).eq(...).isExists()) to run only for truly explicitly-specified
protocol values; keep all existing exception types
(ApiMessageInterceptionException / argerr) unchanged for invalid/unknown
protocols.
Comment on lines
+434
to
+457
| // mimic a storage from before vhost support: drop the Vhost protocol row, | ||
| // then add it back through the API, which must prepare the connected hosts | ||
| SQL.New(PrimaryStorageOutputProtocolRefVO.class) | ||
| .eq(PrimaryStorageOutputProtocolRefVO_.primaryStorageUuid, ps.uuid) | ||
| .eq(PrimaryStorageOutputProtocolRefVO_.outputProtocol, VolumeProtocol.Vhost.toString()) | ||
| .delete() | ||
|
|
||
| addStorageProtocol { | ||
| uuid = ps.uuid | ||
| outputProtocol = VolumeProtocol.Vhost.toString() | ||
| } | ||
|
|
||
| assert ensureCalled.get() : \ | ||
| "addStorageProtocol(Vhost) did not reach VHOST_TARGET_ENSURE_PATH on the connected host" | ||
|
|
||
| // the protocol row write is fire-and-forget behind the PS queue | ||
| retryInSecs { | ||
| assert Q.New(ExternalPrimaryStorageHostProtocolRefVO.class) | ||
| .eq(ExternalPrimaryStorageHostProtocolRefVO_.primaryStorageUuid, ps.uuid) | ||
| .eq(ExternalPrimaryStorageHostProtocolRefVO_.hostUuid, host.uuid) | ||
| .eq(ExternalPrimaryStorageHostProtocolRefVO_.protocol, VolumeProtocol.Vhost.toString()) | ||
| .eq(ExternalPrimaryStorageHostProtocolRefVO_.status, PrimaryStorageHostStatus.Connected) | ||
| .isExists() | ||
| } |
There was a problem hiding this comment.
这里的“写入 Vhost 协议行”断言可能被旧数据误通过。
attachPrimaryStorageToCluster() 之后，host 很可能已经有一条现成的 ExternalPrimaryStorageHostProtocolRefVO(protocol=Vhost)。当前只删了 PrimaryStorageOutputProtocolRefVO，但后面的 isExists() 仍可能命中旧行，即使 addStorageProtocol() 没有新写/刷新协议状态，这段测试也会通过。建议在触发 API 前一并清掉对应的 protocol-ref 行，或者改成校验 lastOpDate 被推进。
🧩 建议修改
SQL.New(PrimaryStorageOutputProtocolRefVO.class)
.eq(PrimaryStorageOutputProtocolRefVO_.primaryStorageUuid, ps.uuid)
.eq(PrimaryStorageOutputProtocolRefVO_.outputProtocol, VolumeProtocol.Vhost.toString())
.delete()
+ SQL.New(ExternalPrimaryStorageHostProtocolRefVO.class)
+ .eq(ExternalPrimaryStorageHostProtocolRefVO_.primaryStorageUuid, ps.uuid)
+ .eq(ExternalPrimaryStorageHostProtocolRefVO_.hostUuid, host.uuid)
+ .eq(ExternalPrimaryStorageHostProtocolRefVO_.protocol, VolumeProtocol.Vhost.toString())
+ .delete()
addStorageProtocol {
uuid = ps.uuid
outputProtocol = VolumeProtocol.Vhost.toString()🤖 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
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovy`
around lines 434 - 457, The test can pass spuriously because an existing
ExternalPrimaryStorageHostProtocolRefVO(protocol=Vhost) row may remain from
attachPrimaryStorageToCluster(); before calling addStorageProtocol (and after
deleting PrimaryStorageOutputProtocolRefVO), also delete any
ExternalPrimaryStorageHostProtocolRefVO rows matching
primaryStorageUuid=ps.uuid, hostUuid=host.uuid,
protocol=VolumeProtocol.Vhost.toString() so the subsequent isExists() truly
verifies a new/write update, or alternatively change the assertion to verify
that the existing ExternalPrimaryStorageHostProtocolRefVO's lastOpDate was
advanced by addStorageProtocol (compare previous lastOpDate to a later value)
and still assert ensureCalled reached VHOST_TARGET_ENSURE_PATH; locate these
changes around attachPrimaryStorageToCluster(), the addStorageProtocol block,
ensureCalled and the isExists() retry.
Comment on lines
+468
to
+472
| updateGlobalConfig { | ||
| category = "host" | ||
| name = "ping.interval" | ||
| value = 1 | ||
| } |
There was a problem hiding this comment.
把 host.ping.interval 在用例结束后恢复。
这里把全局 ping 间隔改成了 1，但没有在 finally 里还原。这个值会泄漏到后面的集成用例，增加额外负载，也会把其他 case 的时序行为改掉，容易引入难定位的波动。
🤖 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
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/zbs/ZbsVhostVolumeCase.groovy`
around lines 468 - 472, The test changes the global config "host.ping.interval"
to 1 via updateGlobalConfig but never restores it; capture the original value
before calling updateGlobalConfig (in the test method inside class
ZbsVhostVolumeCase or the surrounding setup block), and ensure you restore it in
the finally block (or add a finally if none) by calling updateGlobalConfig with
the saved original value so the global state is not leaked to subsequent tests.
track external primary storage host connectivity per protocol in a new ExternalPrimaryStorageHostProtocolRefVO table: node health reports fan out one fire-and-forget status update per reported protocol while the host-level ref row keeps its folded semantics untouched; AddStorageProtocol prepares connected hosts through the new controller hook onProtocolAdded (zbs deploys the vhost target) and records the per-protocol status; expose the rows through QueryExternalPrimaryStorageHostProtocolRef for the frontend; reserve the volumeProtocol system tag definition without consuming it. Resolves: ZCF-0 Change-Id: I2506d6b446f03b15faea1ccc9081a351dd2882c5
MatheMatrix
force-pushed
the
sync/jin.ma/feature-zbs-vhost
branch
from
f8c1d7d to
8ac7d57
Compare
finish the per-protocol host-connectivity framework on the placement path and add protocol selection at VM create. VolumeApiInterceptor.checkHostAccessible now reads the per-protocol ExternalPrimaryStorageHostProtocolRefVO when the volume carries a protocol, so a volume is blocked only when its own data plane is disconnected on the target host; volumes without a protocol keep the folded host-level behavior unchanged. drop the dead protocol column from the JOINED subclass ExternalPrimaryStorageHostRefVO (never read or written; the per- protocol table is authoritative) and its schema column. the volumeProtocol system tag is now an EphemeralPatternSystemTag, consumed into VolumeVO.protocol at create and skipped from persistence by the framework. the VM-create path (no DiskAO) carries it through dataVolumeSystemTags / dataVolumeSystemTagsOnIndex; the interceptor rejects an unknown protocol token at API time. SubCase covers the consume path and the rejection. Resolves: ZCF-0 Change-Id: If240e24984eba74350dcdb39a44f14b0830fbc74
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.
变更
APICreateDataVolumeMsg新增可选protocol参数，经CreateDataVolumeMsg透传到新建卷（vo.setProtocol）VolumeApiInterceptor校验：指定协议必须是目标 PS 暴露的 outputProtocol，且必须指定primaryStorageUuid（与changeVolumeProtocol同款PrimaryStorageOutputProtocolRefVO校验）CreateDataVolumeAction（+protocol）之前只有
changeVolumeProtocol（事后切），这里补齐建卷时直接指定协议。测试
ZbsVhostVolumeCase.testCreateDataVolumeWithExplicitProtocol：显式 CBD 覆盖 Vhost 默认 + NBD（未暴露）被拒；IT 通过（Tests run:1 Failures:0 Errors:0）protocol=NBD被拒does not expose output protocol[NBD]/ 无 PS 被拒primaryStorageUuid is requiredsync from gitlab !10172