<fix>[storage]: filter non-preferred backup storage types before sorting#3395
<fix>[storage]: filter non-preferred backup storage types before sorting#3395zstack-robot-2 wants to merge 1 commit into5.5.12from
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调整 ExternalPrimaryStorage 的备份存储选择:先按首选类型过滤再依据首选顺序排序,若无匹配则提前返回错误;新增单元测试覆盖相关场景;添加新的错误码常量。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Comment from yaohua.wu: Review: MR !9243 — ZSTAC-71706
业务背景
Critical无 Warning
Suggestion
亮点
Verdict: APPROVED修复正确地解决了根因问题,方案稳健,测试充分。仅有 CRLF 行尾问题建议修复后合并。 🤖 Robot Reviewer |
There was a problem hiding this comment.
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
⛔ Files ignored due to path filters (10)
conf/i18n/globalErrorCodeMapping/global-error-de-DE.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-en_US.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-fr-FR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-id-ID.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ja-JP.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ko-KR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ru-RU.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-th-TH.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_CN.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_TW.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
| // 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()); | ||
| } |
There was a problem hiding this comment.
🧩 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.
0ecb2e3 to
ef843cc
Compare
There was a problem hiding this comment.
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
⛔ Files ignored due to path filters (10)
conf/i18n/globalErrorCodeMapping/global-error-de-DE.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-en_US.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-fr-FR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-id-ID.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ja-JP.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ko-KR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ru-RU.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-th-TH.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_CN.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_TW.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovyutils/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
.../test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovy
Outdated
Show resolved
Hide resolved
30f6c35 to
cec0838
Compare
There was a problem hiding this comment.
🧹 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
⛔ Files ignored due to path filters (10)
conf/i18n/globalErrorCodeMapping/global-error-de-DE.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-en_US.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-fr-FR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-id-ID.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ja-JP.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ko-KR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ru-RU.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-th-TH.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_CN.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_TW.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
cec0838 to
4e632e9
Compare
There was a problem hiding this comment.
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
⛔ Files ignored due to path filters (10)
conf/i18n/globalErrorCodeMapping/global-error-de-DE.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-en_US.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-fr-FR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-id-ID.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ja-JP.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ko-KR.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-ru-RU.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-th-TH.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_CN.jsonis excluded by!**/*.jsonconf/i18n/globalErrorCodeMapping/global-error-zh_TW.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
storage/src/main/java/org/zstack/storage/addon/primary/ExternalPrimaryStorage.javatest/src/test/groovy/org/zstack/test/integration/storage/primary/addon/ExternalPrimaryStorageSelectBackupStorageCase.groovyutils/src/main/java/org/zstack/utils/clouderrorcode/CloudOperationsErrorCode.java
| 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; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 验证错误码定义
rg -n "ORG_ZSTACK_STORAGE_ADDON_PRIMARY_10015" --type=javaRepository: MatheMatrix/zstack
Length of output: 422
🏁 Script executed:
#!/bin/bash
# 查看 ExternalPrimaryStorage.java 的相关代码上下文
fd "ExternalPrimaryStorage.java" --type fRepository: 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.javaRepository: MatheMatrix/zstack
Length of output: 1664
🏁 Script executed:
#!/bin/bash
# 查找国际化配置或错误消息定义
fd -e properties -e i18n | head -20Repository: 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 -40Repository: MatheMatrix/zstack
Length of output: 144
🏁 Script executed:
#!/bin/bash
# 查看是否有对应 10015 错误码的 i18n 翻译
rg "10015" --type=propertiesRepository: 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 -40Repository: 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 1Repository: MatheMatrix/zstack
Length of output: 3356
🏁 Script executed:
#!/bin/bash
# 查看相似错误码(10010)在代码和i18n中的定义,作为对比
rg "10010" conf/i18n/ -B 1 -A 1Repository: 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
4e632e9 to
89f989a
Compare
|
Comment from yaohua.wu: Review: MR !9243 — ZSTAC-71706
✅ 总体评价修复方案正确且优于前一版 MR !9234(已关闭)。从"排序修正"升级为"先过滤后排序",语义更清晰,同时补充了空列表防御和专用错误码,质量良好。 Warning
Suggestion
Upstream Freshness
Verdict: APPROVED(修复韩文 i18n 占位符顺序后)🤖 Robot Reviewer |
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