Skip to content

<fix>[snapshot]: fixed the issue where too many snapshots caused the qmp... #2904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: zsv_4.10.20
Choose a base branch
from
Open

<fix>[snapshot]: fixed the issue where too many snapshots caused the qmp... #2904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions conf/db/upgrade/V4.10.20__schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,12 @@ WHERE t.current = true
AND v.rootImageUuid IS NOT NULL
AND t.rootImageUuid IS NULL;


DELETE ref FROM `zstack`.`VolumeSnapshotReferenceVO` ref
INNER JOIN `zstack`.`VolumeEO` vol ON vol.uuid = ref.referenceVolumeUuid
WHERE ref.referenceType = 'VolumeVO'
AND ref.referenceVolumeUuid = ref.referenceUuid
AND ref.referenceInstallUrl NOT LIKE CONCAT('%', SUBSTRING_INDEX(vol.installPath, '/', -1), '%');

UPDATE `zstack`.`GlobalConfigVO` SET value="64", defaultValue="64" WHERE category="volumeSnapshot" AND name="incrementalSnapshot.maxNum" AND value > 120;
Copy link

@coderabbitai coderabbitai bot Nov 13, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

SQL 更新逻辑与 Java 验证约束不一致

当前 SQL 迁移脚本存在以下问题：

  1. 条件不一致：SQL 使用 value > 120（仅处理 >= 121 的值），但 Java 验证层添加了 numberLessThan = 120（允许最大值为 119）。这意味着：

    • 值为 120 的配置不会被 SQL 更新
    • 但 120 不满足 Java 的 numberLessThan = 120 验证条件
    • 建议将 SQL 条件改为 value >= 120 以保持一致性

  2. 目标值与验证范围不匹配：

    • PR 描述提到"clamps the maximum value to 64"
    • SQL 将超限值重置为 64
    • 但 Java 验证允许最大值到 119
    • 如果业务需求是限制最大值为 64，Java 验证应改为 numberLessThan = 65；如果允许到 119，需要明确说明为何 SQL 重置为 64 而非 119

  3. 同时更新 defaultValue 的合理性：SQL 同时更新了 value 和 defaultValue。请确认是否所有超过 120 的配置，其 defaultValue 也需要被强制改为 64，还是应该只更新 value。

  4. 数据类型处理：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/null

Length of output: 63

🏁 Script executed:

#!/bin/bash
# 搜索@GlobalConfigValidation注解的使用
rg -n "GlobalConfigValidation|incrementalSnapshot.maxNum" --type=java -A3 -B3 2>/dev/null | head -50

Length of output: 6626

🏁 Script executed:

#!/bin/bash
# 查找GlobalConfig相关的schema定义或表创建语句
rg -n "GlobalConfigVO|CREATE TABLE.*GlobalConfig" --type=sql -A5 -B2 2>/dev/null | head -60

Length 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 -40

Length 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 -60

Length of output: 1251

🏁 Script executed:

#!/bin/bash
# 显示完整的V4.10.20__schema.sql文件内容
cat ./conf/db/upgrade/V4.10.20__schema.sql

Length of output: 1264

🏁 Script executed:

#!/bin/bash
# 查找GlobalConfigVO表的定义
rg -n "CREATE TABLE.*GlobalConfigVO|CREATE TABLE.*global_config" --type=sql -A20 2>/dev/null | head -80

Length of output: 1359

🏁 Script executed:

#!/bin/bash
# 查看GlobalConfigVO.java的内容
cat ./core/src/main/java/org/zstack/core/config/GlobalConfigVO.java

Length of output: 1860

SQL更新条件与Java验证约束存在严重不一致

核心问题已确认：

  1. 条件漏洞：Java验证允许的范围是 numberGreaterThan = 0, numberLessThan = 120（即1-119），但SQL条件 value > 120 仅处理≥121的值。值为120的配置既不会被SQL更新，也无法通过Java验证。建议改为 value >= 120。

  2. 目标值与验证范围矛盾：SQL将超限值重置为64，但Java验证允许的最大值是119。需要明确业务需求：

    • 若限制最大值为64，应将Java验证改为 numberLessThan = 65
    • 若允许到119，应将SQL目标值改为119

  3. 数据类型隐患：GlobalConfigVO表中 value 字段定义为 text 类型，直接进行 value > 120 的数值比较依赖隐式转换。建议显式转换：CAST(value AS UNSIGNED) >= 120。

  4. defaultValue同步更新：SQL同时修改了 value 和 defaultValue，需确认所有超过限制的配置是否都需要两个字段都重置为64，还是仅更新 value。

注：value和defaultValue字段无NOT NULL约束，无需使用存储过程。


2 changes: 1 addition & 1 deletion conf/globalConfig/snapshot.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<category>volumeSnapshot</category>
<name>incrementalSnapshot.maxNum</name>
<description>The length of a volume snapshot chain. When the lenght of a volume snapshot chain reaches this value, the next volume snapshot will be a full snapshot</description>
<defaultValue>128</defaultValue>
<defaultValue>64</defaultValue>
<type>java.lang.Integer</type>
</config>

Expand Down
2 changes: 1 addition & 1 deletion storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotGlobalConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
public class VolumeSnapshotGlobalConfig {
public static final String CATEGORY = "volumeSnapshot";

@GlobalConfigValidation(numberGreaterThan = 0)
@GlobalConfigValidation(numberGreaterThan = 0, numberLessThan = 120)
public static GlobalConfig MAX_INCREMENTAL_SNAPSHOT_NUM = new GlobalConfig(CATEGORY, "incrementalSnapshot.maxNum");
Comment on lines +14 to 15
Copy link

@coderabbitai coderabbitai bot Nov 13, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

验证约束的上界值需要与业务需求和 SQL 迁移保持一致

当前添加的 numberLessThan = 120 验证约束存在以下问题：

  1. 与 PR 描述不符：

    • PR 描述："clamps the maximum value for incremental snapshots to 64"
    • SQL 迁移：将超过 120 的值重置为 64
    • 当前验证：允许最大值为 119（numberLessThan = 120 表示 < 120）
    • 如果业务需求是限制最大值为 64，此处应改为 numberLessThan = 65

  2. 缺少技术说明：

    • 为什么选择 120 作为上界？是否存在技术限制（如 QMP 命令的限制）？
    • 为什么 SQL 重置为 64，但验证层允许到 119？这个范围差异（64-119）的业务含义是什么？
    • 建议添加 Javadoc 注释说明此限制的原因和技术背景

  3. 与 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 -10

Length 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 2

Length 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 -10

Length 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 -100

Length 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 f

Length 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.sql

Length of output: 2429

验证约束上界值与 SQL 迁移、业务需求不一致

根据代码审查，发现以下问题：

现状分析：

  • 代码验证：numberLessThan = 120（允许最大值 119）
  • SQL 迁移：UPDATE GlobalConfigVO SET value="64" WHERE ... AND value > 120
  • 提交记录：修复"过多快照导致 QMP 'query block' 命令失败"的问题

核心问题：

  1. 逻辑不一致：用户可设置该值为 119 并通过验证，但不会触发 SQL 迁移的重置逻辑，且可能仍会导致 QMP 命令失败
  2. 缺少文档说明：未解释为什么验证上限是 120，而 SQL 重置值是 64；120 是 QMP 的硬限制，还是其他技术限制？
  3. 与 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.

@GlobalConfigValidation(numberGreaterThan = 0)
public static GlobalConfig SNAPSHOT_DELETE_PARALLELISM_DEGREE = new GlobalConfig(CATEGORY, "delete.parallelismDegree");
Expand Down