<fix>[snapshot]: fixed the issue where too many snapshots caused the qmp... #2904
Conversation
|
Warning Rate limit exceeded@MatheMatrix has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Walkthrough增量快照数量配置添加上界限制。数据库升级脚本将超过120的现有配置值调整为64，Java验证层添加numberLessThan约束确保最大值不超过120。 Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
conf/globalConfig/snapshot.xmlis excluded by!**/*.xml
📒 Files selected for processing (2)
conf/db/upgrade/V4.10.20__schema.sql(1 hunks)storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotGlobalConfig.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 1. API 设计要求
- API 命名：
- API 名称必须唯一，不能重复。
- API 消息类需要继承
APIMessage；其返回类必须继承APIReply或APIEvent，并在注释中用@RestResponse进行标注。- API 消息上必须添加注解
@RestRequest，并满足如下规范：
path:
- 针对资源使用复数形式。
- 当 path 中引用消息类变量时，使用
{variableName}格式。- HTTP 方法对应：
- 查询操作 →
HttpMethod.GET- 更新操作 →
HttpMethod.PUT- 创建操作 →
HttpMethod.POST- 删除操作 →
HttpMethod.DELETE- API 类需要实现
__example__方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。
2. 命名与格式规范
类名：
- 使用 UpperCamelCase 风格。
- 特殊情况：
- VO/AO/EO 类型类除外。
- 抽象类采用
Abstract或Base前缀/后缀。- 异常类应以
Exception结尾。- 测试类需要以
Test或Case结尾。方法名、参数名、成员变量和局部变量：
- 使用 lowerCamelCase 风格。
常量命名：
- 全部大写，使用下划线分隔单词。
- 要求表达清楚，避免使用含糊或不准确的名称。
包名：
- 统一使用小写，使用点分隔符，每个部分应是一个具有自然语义的英文单词（参考 Spring 框架的结构）。
命名细节：
- 避免在父子类或同一代码块中出现相同名字的成员或局部变量，防止混淆。
- 命名缩写：
- 不允许使用不必要的缩写，如：
AbsSchedulerJob、condi、Fu等。应使用完整单词提升可读性。
3. 编写自解释代码
意图表达：
- 避免使用布尔型参数造成含义不明确。例如：
- 对于
stopAgent(boolean ignoreError)，建议拆分为不同函数（如stopAgentIgnoreError()），或使用枚举表达操作类型。- 命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途（例如在常量与变量名称中，将类型词放在末尾）。
注释：
- 代码应尽量做到自解释，对少于两行的说明可以直接写在代码中。
- 对于较长的注释，需要仔细校对并随代码更新，确保内容正确。
- 接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。
4. 流程控制和结构优化
if...else 的使用：
- 应尽量减少 if...else 结构的使用，建议：
- 限制嵌套层级最多为两层，且内层不应再出现
else分支。- 尽早返回（Early Return），将条件判断中的处理逻辑提前结束或抽成独立方法。
- 使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断。
条件判断：
- if 条件表达不宜过长或过于复杂，必要时可以将条件抽成 boolean 变量描述。
代码块长度：
- 单个 if 代码块不宜超过一屏显示，以提高可读性和后续维护性。
5. 异常处理与日志
- 捕获异常的原则：
- 对于可以通过预检查避免的 RuntimeException（如
NullPointerException、IndexOutOfBoundsException等），不建议使用 try-catch 来进行处理。- 捕获异常应仅用于处理真正的意外情况，不应将异常逻辑当作正常流程控制。
- 在必要时，应继续抛出异常，使上层业务处理者可以转换为用户友好的错误提示。
- 使用 try-with-resources 语法管理资源，确保在 finally 块中正确关闭资源，并避免在 finally 中返回值。
...
Files:
storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotGlobalConfig.java
**/*.sql
⚙️ CodeRabbit configuration file
**/*.sql: Review the SQL code, make sure has no errors and confirm that:
- Upgrading scene has been carefully handled
- Do not use
DEFAULT 0000-00-00 00:00:00, useDEFAULT CURRENT_TIMESTAMPinstead- When
NOT NULLexists, must usestored procedureor other functions to process historical data, this is very very important
Files:
conf/db/upgrade/V4.10.20__schema.sql
| AND v.rootImageUuid IS NOT NULL | ||
| AND t.rootImageUuid IS NULL; | ||
|
|
||
| UPDATE `zstack`.`GlobalConfigVO` SET value="64", defaultValue="64" WHERE category="volumeSnapshot" AND name="incrementalSnapshot.maxNum" AND value > 120; |
There was a problem hiding this comment.
🧩 Analysis chain
SQL 更新逻辑与 Java 验证约束不一致
当前 SQL 迁移脚本存在以下问题：
-
条件不一致：SQL 使用
value > 120（仅处理 >= 121 的值），但 Java 验证层添加了numberLessThan = 120（允许最大值为 119）。这意味着：- 值为 120 的配置不会被 SQL 更新
- 但 120 不满足 Java 的
numberLessThan = 120验证条件 - 建议将 SQL 条件改为
value >= 120以保持一致性
-
目标值与验证范围不匹配：
- PR 描述提到"clamps the maximum value to 64"
- SQL 将超限值重置为 64
- 但 Java 验证允许最大值到 119
- 如果业务需求是限制最大值为 64，Java 验证应改为
numberLessThan = 65；如果允许到 119，需要明确说明为何 SQL 重置为 64 而非 119
-
同时更新 defaultValue 的合理性：SQL 同时更新了
value和defaultValue。请确认是否所有超过 120 的配置，其defaultValue也需要被强制改为 64，还是应该只更新value。 -
数据类型处理：
value字段在数据库中可能存储为字符串类型，使用value > 120进行数值比较可能产生意外结果。建议使用CAST(value AS UNSIGNED) > 120确保正确的数值比较。
建议的修复方案（需根据实际业务需求选择）：
方案 1：如果最大值确实应该限制为 64
UPDATE `zstack`.`GlobalConfigVO` SET value="64", defaultValue="64"
WHERE category="volumeSnapshot" AND name="incrementalSnapshot.maxNum"
AND CAST(value AS UNSIGNED) >= 64;同时修改 Java 验证为：@GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 65)
方案 2：如果最大值应该是 119（根据 Java 验证）
UPDATE `zstack`.`GlobalConfigVO` SET value="119"
WHERE category="volumeSnapshot" AND name="incrementalSnapshot.maxNum"
AND CAST(value AS UNSIGNED) >= 120;另外，根据编码规范，当字段存在 NOT NULL 约束时，应使用存储过程处理历史数据。请确认 value 和 defaultValue 字段是否有 NOT NULL 约束，如有则需要改用存储过程实现。
Based on coding guidelines
生成验证脚本以检查SQL迁移脚本、Java验证约束和数据库schema定义。
---
🏁 Script executed:
```shell
#!/bin/bash
# 查找SQL迁移脚本并显示其内容
find . -name "V4.10.20__schema.sql" -type f 2>/dev/null | head -5
Length of output: 38
🏁 Script executed:
#!/bin/bash
# 搜索GlobalConfigVO类定义
find . -name "GlobalConfigVO.java" -type f 2>/dev/nullLength of output: 63
🏁 Script executed:
#!/bin/bash
# 搜索@GlobalConfigValidation注解的使用
rg -n "GlobalConfigValidation|incrementalSnapshot.maxNum" --type=java -A3 -B3 2>/dev/null | head -50Length of output: 6626
🏁 Script executed:
#!/bin/bash
# 查找GlobalConfig相关的schema定义或表创建语句
rg -n "GlobalConfigVO|CREATE TABLE.*GlobalConfig" --type=sql -A5 -B2 2>/dev/null | head -60Length of output: 5411
🏁 Script executed:
#!/bin/bash
# 查找value字段的定义和约束
rg -n "value\s*(VARCHAR|TEXT|INT|BIGINT|UNSIGNED)" --type=sql -C3 2>/dev/null | grep -i "globalconfig" -A3 -B3 | head -40Length of output: 876
需要继续查找与volumeSnapshot配置相关的Java定义和表结构。
---
🏁 Script executed:
```shell
#!/bin/bash
# 搜索VolumeSnapshot相关的配置类
find . -name "*VolumeSnapshot*Config*.java" -type f 2>/dev/null | head -10
Length of output: 172
🏁 Script executed:
#!/bin/bash
# 搜索incrementalSnapshot.maxNum的具体定义
rg -n "incrementalSnapshot.maxNum|incrementalSnapshot" --type=java -A5 -B2 2>/dev/null | head -60Length of output: 1251
🏁 Script executed:
#!/bin/bash
# 显示完整的V4.10.20__schema.sql文件内容
cat ./conf/db/upgrade/V4.10.20__schema.sqlLength of output: 1264
🏁 Script executed:
#!/bin/bash
# 查找GlobalConfigVO表的定义
rg -n "CREATE TABLE.*GlobalConfigVO|CREATE TABLE.*global_config" --type=sql -A20 2>/dev/null | head -80Length of output: 1359
🏁 Script executed:
#!/bin/bash
# 查看GlobalConfigVO.java的内容
cat ./core/src/main/java/org/zstack/core/config/GlobalConfigVO.javaLength of output: 1860
SQL更新条件与Java验证约束存在严重不一致
核心问题已确认：
-
条件漏洞：Java验证允许的范围是
numberGreaterThan = 0, numberLessThan = 120（即1-119），但SQL条件value > 120仅处理≥121的值。值为120的配置既不会被SQL更新，也无法通过Java验证。建议改为value >= 120。 -
目标值与验证范围矛盾：SQL将超限值重置为64，但Java验证允许的最大值是119。需要明确业务需求：
- 若限制最大值为64，应将Java验证改为
numberLessThan = 65 - 若允许到119，应将SQL目标值改为119
- 若限制最大值为64，应将Java验证改为
-
数据类型隐患：GlobalConfigVO表中
value字段定义为text类型，直接进行value > 120的数值比较依赖隐式转换。建议显式转换：CAST(value AS UNSIGNED) >= 120。 -
defaultValue同步更新：SQL同时修改了
value和defaultValue，需确认所有超过限制的配置是否都需要两个字段都重置为64，还是仅更新value。
注：value和defaultValue字段无NOT NULL约束，无需使用存储过程。
| @GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 120) | ||
| public static GlobalConfig MAX_INCREMENTAL_SNAPSHOT_NUM = new GlobalConfig(CATEGORY, "incrementalSnapshot.maxNum"); |
There was a problem hiding this comment.
🧩 Analysis chain
验证约束的上界值需要与业务需求和 SQL 迁移保持一致
当前添加的 numberLessThan = 120 验证约束存在以下问题：
-
与 PR 描述不符：
- PR 描述："clamps the maximum value for incremental snapshots to 64"
- SQL 迁移：将超过 120 的值重置为 64
- 当前验证：允许最大值为 119（
numberLessThan = 120表示 < 120） - 如果业务需求是限制最大值为 64，此处应改为
numberLessThan = 65
-
缺少技术说明：
- 为什么选择 120 作为上界？是否存在技术限制（如 QMP 命令的限制）？
- 为什么 SQL 重置为 64，但验证层允许到 119？这个范围差异（64-119）的业务含义是什么？
- 建议添加 Javadoc 注释说明此限制的原因和技术背景
-
与 SQL 迁移的一致性问题：参见 SQL 文件的审查意见，两处修改需要协调一致
建议根据实际业务需求选择以下方案之一：
方案 1：严格限制为 64（与 PR 描述一致）
-@GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 120)
+@GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 65)方案 2：保持 120 为上限（需要说明理由）
-@GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 120)
+/**
+ * 增量快照最大数量配置
+ * 有效范围: (0, 120)
+ * 技术限制: [补充具体原因，如 QMP 命令限制等]
+ * 推荐值: 64（超过此值可能导致性能问题）
+ */
+@GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 120)请确认实际的业务需求和技术限制，并与 SQL 迁移脚本保持一致。
Based on coding guidelines
🏁 Script executed:
#!/bin/bash
# Search for SQL migration files related to incremental snapshots or snapshot migrations
echo "=== Searching for SQL files with incremental snapshot references ==="
fd -e sql | xargs grep -l "incrementalSnapshot\|incremental.*snapshot" 2>/dev/null | head -20
echo ""
echo "=== Searching for SQL files with maxNum references ==="
fd -e sql | xargs grep -l "maxNum\|max_num" 2>/dev/null | head -20
echo ""
echo "=== Checking for any PR or migration documentation ==="
fd -type f \( -name "*.md" -o -name "*.txt" \) | xargs grep -l "incrementalSnapshot\|incremental snapshot" 2>/dev/null | head -10Length of output: 425
🏁 Script executed:
#!/bin/bash
# Search for references to MAX_INCREMENTAL_SNAPSHOT_NUM in the codebase
echo "=== Searching for MAX_INCREMENTAL_SNAPSHOT_NUM usage ==="
rg -n "MAX_INCREMENTAL_SNAPSHOT_NUM" --type java -C 3
echo ""
echo "=== Searching for incrementalSnapshot.maxNum references ==="
rg -n "incrementalSnapshot\.maxNum" --type java -C 3
echo ""
echo "=== Searching for numberLessThan.*120 or similar patterns ==="
rg -n "numberLessThan\s*=\s*120" --type java -C 2Length of output: 5576
🏁 Script executed:
#!/bin/bash
# Get more context about the VolumeSnapshotGlobalConfig file
echo "=== Full content of VolumeSnapshotGlobalConfig.java ==="
fd "VolumeSnapshotGlobalConfig.java" --type f -x cat {}
echo ""
echo "=== Searching for snapshot-related SQL migration files ==="
fd -path "*/db/migration/*" -name "*.sql" | xargs grep -l "snapshot" 2>/dev/null | head -10Length of output: 1673
🏁 Script executed:
#!/bin/bash
# Read the SQL migration file to check what value is actually used
echo "=== Content of SQL migration file V4.10.20__schema.sql ==="
cat conf/db/upgrade/V4.10.20__schema.sql | grep -A 5 -B 5 "incrementalSnapshot\|maxNum\|64\|120" | head -100Length of output: 340
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about snapshot limits
echo "=== Searching for documentation about snapshot limits ==="
rg -n "incremental.*snapshot|snapshot.*limit|qmp.*command|snapshot.*performance" --type java -i | grep -i "limit\|max\|64\|120\|qmp\|performance" | head -30
echo ""
echo "=== Checking commit messages or PR files if available ==="
git log --all --oneline --grep="incremental\|snapshot" 2>/dev/null | head -20
echo ""
echo "=== Looking for any test files related to snapshot configuration ==="
fd -name "*SnapshotGlobalConfig*" --type fLength of output: 4201
🏁 Script executed:
#!/bin/bash
# Search for any documentation files mentioning snapshot configuration
echo "=== Searching README or documentation files ==="
find . -name "README*" -o -name "*.md" | xargs grep -l "snapshot\|incremental" 2>/dev/null | head -10
echo ""
echo "=== Checking git diff or recent changes ==="
git diff HEAD~1 HEAD -- "*SnapshotGlobalConfig*" 2>/dev/null || echo "No recent git diff available"
echo ""
echo "=== Looking at the full SQL migration context ==="
cat conf/db/upgrade/V4.10.20__schema.sqlLength of output: 2429
验证约束上界值与 SQL 迁移、业务需求不一致
根据代码审查，发现以下问题：
现状分析：
- 代码验证：
numberLessThan = 120（允许最大值 119） - SQL 迁移：
UPDATE GlobalConfigVO SET value="64" WHERE ... AND value > 120 - 提交记录：修复"过多快照导致 QMP 'query block' 命令失败"的问题
核心问题：
- 逻辑不一致：用户可设置该值为 119 并通过验证，但不会触发 SQL 迁移的重置逻辑，且可能仍会导致 QMP 命令失败
- 缺少文档说明：未解释为什么验证上限是 120，而 SQL 重置值是 64；120 是 QMP 的硬限制，还是其他技术限制？
- 与 PR 描述偏离：PR 描述"将最大值限制为 64"，但当前验证允许至 119
建议修复方案：
方案 1：调整验证以匹配 SQL 逻辑（推荐）
-@GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 120)
+@GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 65)方案 2：添加文档说明技术限制
+/**
+ * 增量快照最大数量，用于防止过多快照导致 QMP 'query block' 命令失败
+ * 有效范围: (0, 120)
+ * 安全推荐值: 64（超过此值可能性能下降；达到 120 可能导致 QMP 命令超时）
+ * @see <a href="...">QMP 命令限制文档</a>
+ */
@GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 120)需确认：为什么选择 120 作为验证硬限制，而非直接用 65 以匹配 SQL 迁移的 64 值？
🤖 Prompt for AI Agents
In
storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotGlobalConfig.java
around lines 14-15, the validation upper bound (numberLessThan = 120) is
inconsistent with the SQL migration and PR intent (cap at 64); change the
validation to numberLessThan = 65 so the max allowed value is 64, and add a
short inline comment documenting why 64 is enforced (reference the SQL migration
and the QMP limitation/bug fix); also run/update any unit tests or migration
checks that assume the old 120 limit and ensure release notes mention the
tightened limit.
…qmp 'query block' command to fail when the maximum value of incremental snapshot exceeds 64, change it to 64. Resolves/Related: ZSTAC-67846 Resolves/Related: ZSV-9177 Change-Id: I717a65746462647666626263717674796f627973
MatheMatrix
force-pushed
the
sync/tao.gan/ZSV-9177@@3
branch
from
696b11b to
1e0eb14
Compare
[snapshot]: fixed the issue where too many snapshots caused the qmp 'query block' command to fail
when the maximum value of incremental snapshot exceeds 64, change it to 64.
Resolves/Related: ZSTAC-67846
Resolves/Related: ZSV-9177
Change-Id: I717a65746462647666626263717674796f627973
sync from gitlab !8702