Skip to content

<fix>[storage]: filter non-preferred backup storage types before sorting#3395

Open
zstack-robot-2 wants to merge 1 commit into5.5.12from
sync/yaohua.wu/bugfix/ZSTAC-71706
Open

<fix>[storage]: filter non-preferred backup storage types before sorting#3395
zstack-robot-2 wants to merge 1 commit into5.5.12from
sync/yaohua.wu/bugfix/ZSTAC-71706

Conversation

@zstack-robot-2
Copy link
Collaborator

ZSTAC-71706

indexOf() returns -1 for non-preferred backup storage types, causing them to sort before preferred ones. Fix: filter by preferBsTypes first, then sort. Added dedicated error code ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015 with i18n entries in all 10 locales.

Related: ZSTAC-71706

sync from gitlab !9243

@coderabbitai
Copy link

coderabbitai bot commented Feb 26, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

调整 ExternalPrimaryStorage 的备份存储选择:先按首选类型过滤再依据首选顺序排序,若无匹配则提前返回错误;新增单元测试覆盖相关场景;添加新的错误码常量。

Changes

Cohort / File(s) Summary
主逻辑变更
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
将原先就地按类型排序替换为“先过滤(仅保留首选类型)再按首选类型顺序排序”,在过滤后无匹配时提前返回错误并终止流程;保留从结果中选第一个 inventory 的行为。
单元测试新增
test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy
新增测试类,覆盖非首选类型排除、混合类型排序、首选与非首选共存时选择首选、以及无首选类型时的错误处理等场景。
错误码扩展
utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
新增常量 ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015,用于表示“无可用首选类型的备份存储”相关错误。

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

我是小兔守仓库,
先筛再排步稳妥,
无匹配时早报错,
测试来护又细活,
小兔点点胜欢喜 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed Pull request title follows the required [scope]: format and is exactly 72 characters, clearly summarizing the main change of filtering non-preferred backup storage types before sorting.
Description check ✅ Passed Pull request description is directly related to the changeset, referencing the specific JIRA issue ZSTAC-71706 and explaining the bug fix and error code additions.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sync/yaohua.wu/bugfix/ZSTAC-71706

Comment @coderabbitai help to get the list of available commands and usage tips.

@MatheMatrix
Copy link
Owner

Comment from yaohua.wu:

Review: MR !9243 — ZSTAC-71706

含VCenter的环境中ZBS存储上创建的虚机全量克隆失败

业务背景

ExternalPrimaryStorage.handle(SelectBackupStorageMsg) 中使用 List.indexOf() 排序备份存储,对不在 preferBsTypes 中的类型返回 -1,导致非首选类型(如 VCenterBackupStorage)被错误排到最前面。后续操作向 VCenter BS 发送不支持的消息,克隆失败。

Critical

Warning

  1. [ExternalPrimaryStorageSelectBackupStorageCase.groovy: 全文] CRLF 行尾 — 新增的测试文件使用了 \r\n(Windows 风格)行尾,与项目其余代码(LF)不一致。建议在提交前转换为 LF:sed -i 's/\r$//' <file>。虽然不影响功能,但可能导致 diff 噪音和跨平台编辑问题。

Suggestion

  1. [ExternalPrimaryStorage.java:516] 错误消息中 List.toString() 格式preferBsTypes.toString() 会输出 [ImageStoreBackupStorage](带方括号)。建议用 String.join(", ", preferBsTypes) 使消息更清晰,输出为 ImageStoreBackupStorage 而非 [ImageStoreBackupStorage]

  2. [ExternalPrimaryStorage.java:524] 方法可见性filterAndSortByPreferredTypes 为 package-visible static 方法,注释说明了 // package-visible for testing,这是合理的设计。但如果后续有其他模块需要复用此逻辑,可考虑提升到 utility 类。当前无需修改。

  3. [ExternalPrimaryStorageSelectBackupStorageCase.groovy] 集成测试覆盖 — 当前测试直接调用 static 方法验证 filter+sort 逻辑,覆盖充分(6 个场景)。如果条件允许,可补充一个通过 SelectBackupStorageMsg 消息驱动的集成测试,验证完整消息处理链路(包括 SQL 查询返回空列表时的行为)。优先级低,当前单元测试已足够验证核心逻辑。

亮点

  • 修复策略选择合理:采用 filter+sort(方案 B)而非仅修正 comparator(方案 A),更加严格——非首选类型被彻底排除,避免了 fallback 到不兼容的 BS 类型。
  • 附带修复了原有的空列表风险:原代码 availableBs.get(0) 在 SQL 返回空列表时会抛 IndexOutOfBoundsException,新代码通过 filter 后的空检查统一处理了此边界条件。
  • i18n 覆盖完整:新增错误码 ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015 的 10 个语言翻译齐全。
  • 测试覆盖充分:6 个测试场景覆盖了 bug 复现、修复验证、多类型排序、空结果、全首选、单首选等关键路径。

Verdict: APPROVED

修复正确地解决了根因问题,方案稳健,测试充分。仅有 CRLF 行尾问题建议修复后合并。


🤖 Robot Reviewer

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 527-534: The method filterAndSortByPreferredTypes is
package-private but is used from a test in a different package; make it public
(change its signature to public static List<BackupStorageVO>
filterAndSortByPreferredTypes(...)) and add a short javadoc noting it’s exposed
for tests/tools, or alternatively move the
ExternalPrimaryStorageSelectBackupStorageCase test into the same package as
ExternalPrimaryStorage (org.zstack.storage.addon.primary) so package-private
visibility is valid.

ℹ️ Review info

Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2442f28 and 0ecb2e3.

⛔ Files ignored due to path filters (10)
  • conf/i18n/globalErrorCodeMapping/global-error-de-DE.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-en_US.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-fr-FR.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-id-ID.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ja-JP.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ko-KR.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ru-RU.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-th-TH.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-zh_CN.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-zh_TW.json is excluded by !**/*.json
📒 Files selected for processing (3)
  • storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
  • test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy
  • utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java

Comment on lines 527 to 534
// package-visible for testing
static List<BackupStorageVO> filterAndSortByPreferredTypes(
List<BackupStorageVO> candidates, List<String> preferBsTypes) {
return candidates.stream()
.filter(bs -> preferBsTypes.contains(bs.getType()))
.sorted(Comparator.comparingInt(o -> preferBsTypes.indexOf(o.getType())))
.collect(Collectors.toList());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== ExternalPrimaryStorage.java package + method signature =="
sed -n '1,5p;525,535p' storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java

echo
echo "== Test package + call site =="
sed -n '1,8p;60,66p' test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy

echo
echo "== All call sites =="
rg -n --type=java --type=groovy 'filterAndSortByPreferredTypes\s*\('

Repository: MatheMatrix/zstack

Length of output: 3335


filterAndSortByPreferredTypes 改为 public,或将测试类移到同包。

方法 filterAndSortByPreferredTypes 目前是包级可见(无访问修饰符),但测试类在不同包 org.zstack.test.integration.storage.primary.addon 中调用它。根据 Java 可见性规则,包级可见方法不应被不同包中的类访问。

虽然 Groovy 的运行时反射机制可能允许此类访问,但这违反了 Java 封装原则,且易导致维护问题。建议:

  • 改为 public static(添加注释说明仅供测试和工具方法使用),或
  • 将 ExternalPrimaryStorageSelectBackupStorageCase 测试类移至 org.zstack.storage.addon.primary.test 或同包位置。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`
around lines 527 - 534, The method filterAndSortByPreferredTypes is
package-private but is used from a test in a different package; make it public
(change its signature to public static List<BackupStorageVO>
filterAndSortByPreferredTypes(...)) and add a short javadoc noting it’s exposed
for tests/tools, or alternatively move the
ExternalPrimaryStorageSelectBackupStorageCase test into the same package as
ExternalPrimaryStorage (org.zstack.storage.addon.primary) so package-private
visibility is valid.

@MatheMatrix MatheMatrix force-pushed the sync/yaohua.wu/bugfix/ZSTAC-71706 branch from 0ecb2e3 to ef843cc Compare February 27, 2026 02:56
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy`:
- Around line 1-160: Convert the file
ExternalPrimaryStorageSelectBackupStorageCase.groovy from CRLF to LF line
endings to avoid cross-platform diffs: ensure the class
ExternalPrimaryStorageSelectBackupStorageCase and its methods
(testBugReproduction, testFilterThenSortSelectsPreferredType, etc.) are saved
with Unix-style LF endings (for example via your editor's EOL setting or git's
core.autocrlf/ .gitattributes) and commit the normalized file so the tests and
diffs are clean.

ℹ️ Review info

Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ecb2e3 and ef843cc.

⛔ Files ignored due to path filters (10)
  • conf/i18n/globalErrorCodeMapping/global-error-de-DE.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-en_US.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-fr-FR.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-id-ID.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ja-JP.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ko-KR.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ru-RU.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-th-TH.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-zh_CN.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-zh_TW.json is excluded by !**/*.json
📒 Files selected for processing (3)
  • storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
  • test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy
  • utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java

@MatheMatrix MatheMatrix force-pushed the sync/yaohua.wu/bugfix/ZSTAC-71706 branch 2 times, most recently from 30f6c35 to cec0838 Compare February 27, 2026 04:42
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java (1)

513-525: 修复逻辑正确,建议优化错误消息格式

过滤和排序逻辑正确修复了原始 bug:先过滤掉非首选类型,再按首选顺序排序,确保 indexOf() 不会返回 -1。空列表检查也避免了后续 get(0)IndexOutOfBoundsException

建议:错误消息中 preferBsTypes.toString() 会输出带方括号的格式(如 [ImageStoreBackupStorage]),可使用 String.join(", ", preferBsTypes) 使输出更简洁。

♻️ 建议的错误消息格式优化
 if (availableBs.isEmpty()) {
     reply.setError(operr(ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015,
             "no available backup storage with preferred types %s for primary storage[uuid:%s]",
-            preferBsTypes.toString(), self.getUuid()));
+            String.join(", ", preferBsTypes), self.getUuid()));
     bus.reply(msg, reply);
     return;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`
around lines 513 - 525, The error message uses preferBsTypes.toString(), which
prints brackets; change the message to join the list into a cleaner
comma-separated string (e.g., String.join(", ", preferBsTypes)) when building
the operr for the empty availableBs case in the method that filters/sorts
availableBs; update the operr call that currently uses preferBsTypes.toString()
(and references reply, self.getUuid(), bus.reply, msg) to pass the joined string
instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 513-525: The error message uses preferBsTypes.toString(), which
prints brackets; change the message to join the list into a cleaner
comma-separated string (e.g., String.join(", ", preferBsTypes)) when building
the operr for the empty availableBs case in the method that filters/sorts
availableBs; update the operr call that currently uses preferBsTypes.toString()
(and references reply, self.getUuid(), bus.reply, msg) to pass the joined string
instead.

ℹ️ Review info

Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 30f6c35 and cec0838.

⛔ Files ignored due to path filters (10)
  • conf/i18n/globalErrorCodeMapping/global-error-de-DE.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-en_US.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-fr-FR.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-id-ID.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ja-JP.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ko-KR.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ru-RU.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-th-TH.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-zh_CN.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-zh_TW.json is excluded by !**/*.json
📒 Files selected for processing (3)
  • storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
  • test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy
  • utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java

@MatheMatrix MatheMatrix force-pushed the sync/yaohua.wu/bugfix/ZSTAC-71706 branch from cec0838 to 4e632e9 Compare February 27, 2026 07:28
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy (1)

116-127: 测试方法间存在顺序依赖。

testSelectPreferredOverNonPreferred 依赖于 testErrorWhenNoPreferredTypeAvailable 中创建的 VCenterBackupStorage(如第 149 行注释所述)。当前在 test() 方法中顺序是受控的,但如果后续重构或单独运行某个测试方法可能会出现问题。

可选改进:在 testSelectPreferredOverNonPreferred 中显式创建所需的 VCenterBackupStorage,使每个测试方法独立。

🤖 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/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy`
around lines 116 - 127, The two test helpers testSelectPreferredOverNonPreferred
and testErrorWhenNoPreferredTypeAvailable have an implicit ordering dependency
because testSelectPreferredOverNonPreferred relies on the VCenterBackupStorage
created in testErrorWhenNoPreferredTypeAvailable; make
testSelectPreferredOverNonPreferred self-sufficient by explicitly creating or
ensuring the required VCenterBackupStorage (the same backup storage setup
currently done in testErrorWhenNoPreferredTypeAvailable) inside
testSelectPreferredOverNonPreferred (or factor the backup-storage creation into
a shared helper invoked by both) so each test can run independently under
env.create without relying on execution order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`:
- Around line 519-525: The review warns not to change the parameter passed to
operr for error code ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015: keep passing the
List preferBsTypes directly (as in the current
ExternalPrimaryStorage.reply.setError(...) call using operr) and do not replace
it with String.join(", ", preferBsTypes) or any other formatted string;
revert/remove any edits that convert preferBsTypes to a joined String so the
i18n translation receives the original List as expected.

---

Nitpick comments:
In
`@test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy`:
- Around line 116-127: The two test helpers testSelectPreferredOverNonPreferred
and testErrorWhenNoPreferredTypeAvailable have an implicit ordering dependency
because testSelectPreferredOverNonPreferred relies on the VCenterBackupStorage
created in testErrorWhenNoPreferredTypeAvailable; make
testSelectPreferredOverNonPreferred self-sufficient by explicitly creating or
ensuring the required VCenterBackupStorage (the same backup storage setup
currently done in testErrorWhenNoPreferredTypeAvailable) inside
testSelectPreferredOverNonPreferred (or factor the backup-storage creation into
a shared helper invoked by both) so each test can run independently under
env.create without relying on execution order.

ℹ️ Review info

Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cec0838 and 4e632e9.

⛔ Files ignored due to path filters (10)
  • conf/i18n/globalErrorCodeMapping/global-error-de-DE.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-en_US.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-fr-FR.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-id-ID.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ja-JP.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ko-KR.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-ru-RU.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-th-TH.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-zh_CN.json is excluded by !**/*.json
  • conf/i18n/globalErrorCodeMapping/global-error-zh_TW.json is excluded by !**/*.json
📒 Files selected for processing (3)
  • storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java
  • test/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy
  • utils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java

Comment on lines +519 to +525
if (availableBs.isEmpty()) {
reply.setError(operr(ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015,
"no available backup storage with preferred types %s for primary storage[uuid:%s]",
preferBsTypes, self.getUuid()));
bus.reply(msg, reply);
return;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# 验证错误码定义
rg -n "ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015" --type=java

Repository: MatheMatrix/zstack

Length of output: 422


🏁 Script executed:

#!/bin/bash
# 查看 ExternalPrimaryStorage.java 的相关代码上下文
fd "ExternalPrimaryStorage.java" --type f

Repository: MatheMatrix/zstack

Length of output: 144


🏁 Script executed:

#!/bin/bash
# 搜索 preferBsTypes 的定义和类型
rg -B 5 -A 5 "preferBsTypes" storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java

Repository: MatheMatrix/zstack

Length of output: 1664


🏁 Script executed:

#!/bin/bash
# 查找国际化配置或错误消息定义
fd -e properties -e i18n | head -20

Repository: MatheMatrix/zstack

Length of output: 1185


🏁 Script executed:

#!/bin/bash
# 搜索 operr 方法的定义,了解错误信息处理
rg -n "def operr|operr(" --type=java utils/src/main/java/org/zstack/utils/ -A 3 | head -40

Repository: MatheMatrix/zstack

Length of output: 144


🏁 Script executed:

#!/bin/bash
# 查看是否有对应 10015 错误码的 i18n 翻译
rg "10015" --type=properties

Repository: MatheMatrix/zstack

Length of output: 96


🏁 Script executed:

#!/bin/bash
# 搜索 operr 方法的定义
rg -n "operr\(" --type=java utils/src/main/java/org/zstack/utils/ -A 3 | head -40

Repository: MatheMatrix/zstack

Length of output: 44


🏁 Script executed:

#!/bin/bash
# 查找包含 10015 的 i18n 文件
rg "10015" conf/i18n/

Repository: MatheMatrix/zstack

Length of output: 50375


🏁 Script executed:

#!/bin/bash
# 查看错误码对应的国际化文本定义
rg "ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015" conf/i18n/ -A 1

Repository: MatheMatrix/zstack

Length of output: 3356


🏁 Script executed:

#!/bin/bash
# 查看相似错误码(10010)在代码和i18n中的定义,作为对比
rg "10010" conf/i18n/ -B 1 -A 1

Repository: MatheMatrix/zstack

Length of output: 50375


移除可选的格式优化建议,因其会破坏国际化一致性。

错误码 ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015 已正确定义。然而,使用 String.join(", ", preferBsTypes) 的建议应被撤销。

虽然 String.join() 在视觉上更整洁(避免方括号),但它改变了传递给国际化系统的参数格式。i18n 文件中已定义的翻译(如中文:"匹配首选类型 %s")期望接收的是 List 的原始格式。改变参数格式将导致翻译的一致性问题,违反 ZStack 维持 i18n 兼容性的原则。保留当前实现,直接使用 preferBsTypes 作为参数。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.java`
around lines 519 - 525, The review warns not to change the parameter passed to
operr for error code ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015: keep passing the
List preferBsTypes directly (as in the current
ExternalPrimaryStorage.reply.setError(...) call using operr) and do not replace
it with String.join(", ", preferBsTypes) or any other formatted string;
revert/remove any edits that convert preferBsTypes to a joined String so the
i18n translation receives the original List as expected.

When SelectBackupStorageMsg carries preferBsTypes, the sorting
logic uses indexOf() which returns -1 for non-preferred types
(e.g. VCenter BS), causing them to sort before preferred ones.
This fix filters backup storages by preferBsTypes first, then
sorts within the filtered set. Also adds error code
ADDON_PRIMARY_10015 when no matching backup storage is available.

1. Why is this change necessary?
SelectBackupStorageMsg.preferBsTypes is used to sort backup
storages via indexOf() on the preference list. However,
indexOf() returns -1 for types not in the list, which sorts
non-preferred types (like VCenter BS) before preferred ones.
In a mixed VCenter + Expon environment, this causes VCenter
BS to be incorrectly selected over the intended Expon BS.

2. How does it address the problem?
Instead of only sorting, the code now first filters the
candidate backup storages to only include those whose bsType
matches the preferBsTypes list. Then it sorts within the
filtered set. A new error code ADDON_PRIMARY_10015 is added
for the case where no matching backup storage is available
after filtering. All 10 locale i18n JSON files are updated.

3. Are there any side effects?
None. The filtering is only applied when preferBsTypes is
non-empty, preserving existing behavior for other callers.

# Summary of changes (by module):
- storage: filter BS candidates by preferBsTypes before sort
- utils: add ADDON_PRIMARY_10015 error code constant
- conf/i18n: add i18n entries for new error code (10 locales)
- test: add ExternalPrimaryStorageSelectBackupStorageCase

Related: ZSTAC-71706
Change-Id: Ia3af38cc50e69132a1c769180792363495c1080f
@MatheMatrix MatheMatrix force-pushed the sync/yaohua.wu/bugfix/ZSTAC-71706 branch from 4e632e9 to 89f989a Compare February 27, 2026 12:07
@ZStack-Robot
Copy link
Collaborator

Comment from yaohua.wu:

Review: MR !9243 — ZSTAC-71706

含VCenter的环境里面在ZBS存储上创建的虚机进行全量克隆失败

✅ 总体评价

修复方案正确且优于前一版 MR !9234(已关闭)。从"排序修正"升级为"先过滤后排序",语义更清晰,同时补充了空列表防御和专用错误码,质量良好。

Warning

  1. [conf/i18n/globalErrorCodeMapping/global-error-ko-KR.json] %s 占位符顺序与英文模板不一致

    Java 代码中参数传入顺序为 (preferBsTypes, self.getUuid()),即第 1 个 %s = 类型列表,第 2 个 %s = UUID。

    • 英文(正确):"no available backup storage with preferred types %s for primary storage[uuid:%s]"
    • 韩文(错误):"기본 저장소[uuid:%s]에 대해 선호하는 유형 %s의 사용 가능한 백업 저장소가 없습니다"

    韩文模板把 uuid:%s 放在第一个占位符位置,实际会渲染为 uuid:[ImageStoreBackupStorage],而类型位置会显示 UUID。建议调整为:

    "ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015": "선호하는 유형 %s에 대해 사용 가능한 백업 저장소가 기본 저장소[uuid:%s]에 없습니다"

    其余 9 个语言的占位符顺序均正确(已逐一验证)。

Suggestion

  1. [ExternalPrimaryStorage.java:~514] 行为变更值得在 commit message 中明确标注

    原逻辑:当没有首选类型的 BS 可用时,会退化选择非首选类型的 BS(然后下游失败)。
    新逻辑:严格过滤,没有首选类型直接返回错误。

    这是正确的行为改进(选错 BS 必然下游失败,不如提前报错),但属于行为变更,建议在 MR description 或 commit message 中明确说明此语义变化,方便后续 cherry-pick 时评审者理解。

  2. [ExternalPrimaryStorageSelectBackupStorageCase.groovy] 测试覆盖良好

    两个场景覆盖了核心路径:

    • testErrorWhenNoPreferredTypeAvailable — 验证过滤后空列表返回错误
    • testSelectPreferredOverNonPreferred — 验证混合环境下选中首选类型

    如果想更完善,可以考虑追加一个场景:多个首选类型 BS 都可用时,验证按 preferBsTypes 顺序选择第一个(当前排序逻辑已支持,但没有显式测试)。这是非阻塞建议。

Upstream Freshness

  • upstream/5.5.12ExternalPrimaryStorage.java 无新提交,分支基线是最新的
  • merge_status: can_be_merged,无冲突

Verdict: APPROVED(修复韩文 i18n 占位符顺序后)


🤖 Robot Reviewer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants