fix(ai): add clusterId column to ModelServiceInstanceVO#3358
Open
MatheMatrix wants to merge 11 commits into5.5.6from
Open
fix(ai): add clusterId column to ModelServiceInstanceVO#3358MatheMatrix wants to merge 11 commits into5.5.6from
MatheMatrix wants to merge 11 commits into5.5.6from
Conversation
…isk offering Resolves: ZSTAC-74683 Change-Id: Id0339ed0221e92e506f60745cde972cc3ee6d9ae
When anti-split-brain check selects a disconnected MDS node, the HTTP call now times out after 30s instead of 5+ minutes, and automatically retries the next available MDS via tryNext mechanism. Resolves: ZSTAC-80595 Change-Id: I1be80f1b70cad1606eb38d1f0078c8f2781e6941
When MN restarts during a destroy operation, the hypervisor may report the VM as Stopped. Without this transition, the state machine throws an exception and the VM stays stuck in Destroying state forever. Resolves: ZSTAC-80620 Change-Id: I037edba70d145a44a88ce0d3573089182fedb162
Resolves: ZSTAC-82153 Change-Id: Ib51c2e21553277416d1a9444be55aca2aa4b2fc4
Walkthrough新增一个 Git 预推送钩子，并在多处虚拟机生命周期与存储控制器代码中增加同步/容错逻辑与状态/容量计算调整。共涉及钩子脚本和四处 Java 源文件修改，包含消息同步调用、根盘大小计算、状态机转移与 HTTP 重试选项。 Changes
Sequence Diagram(s)（无 — 所选更改未满足生成序列图的所有条件） 代码审查工作量🎯 3 (Moderate) | ⏱️ ~25 minutes 诗歌
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.5)compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.javaComment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@conf/db/upgrade/V5.5.6__schema.sql`:
- Around line 183-186: The SQL UPDATE uses unquoted identifiers which can break
on MySQL/GreatSQL reserved words; update the statement to wrap all table and
column names in backticks—specifically reference the tables
ModelServiceInstanceVO and PodVO and the columns vmInstanceUuid, uuid, and
clusterId—so change the UPDATE/INNER JOIN/SET/WHERE identifiers to use backticks
around `ModelServiceInstanceVO`, `msi`, `PodVO`, `p`, `vmInstanceUuid`, `uuid`,
and `clusterId`.
Comment on lines
183
to
186
| UPDATE ModelServiceInstanceVO msi | ||
| INNER JOIN PodVO p ON msi.vmInstanceUuid = p.uuid | ||
| SET msi.clusterId = p.clusterId | ||
| WHERE msi.clusterId IS NULL AND p.clusterId IS NOT NULL; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
表名和列名需要使用反引号包裹。
根据编码规范，所有表名和列名必须使用反引号包裹，以避免 MySQL 8.0 / GreatSQL 保留关键字冲突导致的语法错误。例如 uuid 在某些数据库版本中可能是保留字。
♻️ 建议修改
-UPDATE ModelServiceInstanceVO msi
-INNER JOIN PodVO p ON msi.vmInstanceUuid = p.uuid
-SET msi.clusterId = p.clusterId
-WHERE msi.clusterId IS NULL AND p.clusterId IS NOT NULL;
+UPDATE `ModelServiceInstanceVO` `msi`
+INNER JOIN `PodVO` `p` ON `msi`.`vmInstanceUuid` = `p`.`uuid`
+SET `msi`.`clusterId` = `p`.`clusterId`
+WHERE `msi`.`clusterId` IS NULL AND `p`.`clusterId` IS NOT NULL;As per coding guidelines: "所有表名和列名必须使用反引号包裹（例如：WHERE `system` = 1），以避免 MySQL 8.0 / GreatSQL 保留关键字冲突导致的语法错误"
🤖 Prompt for AI Agents
In `@conf/db/upgrade/V5.5.6__schema.sql` around lines 183 - 186, The SQL UPDATE
uses unquoted identifiers which can break on MySQL/GreatSQL reserved words;
update the statement to wrap all table and column names in
backticks—specifically reference the tables ModelServiceInstanceVO and PodVO and
the columns vmInstanceUuid, uuid, and clusterId—so change the UPDATE/INNER
JOIN/SET/WHERE identifiers to use backticks around `ModelServiceInstanceVO`,
`msi`, `PodVO`, `p`, `vmInstanceUuid`, `uuid`, and `clusterId`.
add syncVmDeviceInfo when vm is running Resolves: ZSTAC-67275 Change-Id: I746e63786a677676676b6d6265657063666b7677
MatheMatrix
force-pushed
the
sync/ye.zou/fix/ZSTAC-81566
branch
from
2137d18 to
abac4af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.gitconfig/hooks/pre-push:
- Around line 65-71: The branch-name check using [[ "$BRANCH" =~ premium ]] is
too loose and will match "premium" anywhere; update the conditional that
references the BRANCH variable to use a stricter pattern (e.g., a regex that
matches whole segment names like (^|/)(premium)(/|$|-) or similar) so only
branches whose final/segment name is exactly "premium" (or clearly intended
premium branches) trigger the error; modify the if that uses BRANCH and
REPO_NAME to apply this tighter match logic.
🧹 Nitpick comments (3)
.gitconfig/hooks/pre-push (3)
13-14:URL变量已定义但未使用第 14 行定义的
URL变量从未被引用。脚本在第 35 行通过git remote get-url "$REMOTE"重新获取远程 URL。建议直接使用传入的$2参数，或者移除未使用的变量。♻️ 建议的修复方案
方案 1：移除未使用的变量
REMOTE="$1" -URL="$2" +# URL is available as $2 but we fetch it via git remote get-url for validation方案 2：使用传入的 URL 参数
REMOTE="$1" URL="$2" BRANCH=$(git branch --show-current) ... # --- Check 2: Remote URL sanity --- -REMOTE_URL=$(git remote get-url "$REMOTE" 2>/dev/null) -if [ -z "$REMOTE_URL" ]; then +if [ -z "$URL" ]; then echo -e "${RED}[pre-push] Cannot resolve remote '$REMOTE'${NC}" ERRORS=$((ERRORS + 1)) fi # Warn if pushing to upstream (zstackio) directly — usually want to push to fork (origin) -if echo "$REMOTE_URL" | grep -q "zstackio/" && [ "$REMOTE" = "upstream" ]; then +if echo "$URL" | grep -q "zstackio/" && [ "$REMOTE" = "upstream" ]; then echo -e "${YELLOW}[pre-push] WARNING: Pushing directly to upstream (zstackio). Are you sure?${NC}" - echo -e "${YELLOW} Remote URL: $REMOTE_URL${NC}" + echo -e "${YELLOW} Remote URL: $URL${NC}"
57-63: 大小写校验不一致第 27 行对分支名称强制要求大写
ZSTAC，但第 59 行对提交信息使用了-i（忽略大小写）。这可能导致分支名为fix/ZSTAC-12345但提交信息中写成zstac-12345的情况。如果这是有意为之（对提交信息更宽松），可以忽略；否则建议保持一致：
♻️ 建议统一为大小写敏感
if [[ "$BRANCH" =~ ^fix/ ]]; then - if ! echo "$LAST_COMMIT_MSG" | grep -qi "ZSTAC-[0-9]\+"; then + if ! echo "$LAST_COMMIT_MSG" | grep -q "ZSTAC-[0-9]\+"; then echo -e "${RED}[pre-push] Latest commit does not reference a ZSTAC ticket${NC}" ERRORS=$((ERRORS + 1)) fi fi
1-7: 建议添加set -u提高脚本健壮性考虑在脚本开头添加
set -u以捕获未定义变量的使用，这在 shell 脚本中是一个良好的防御性编程实践。♻️ 建议的修改
#!/bin/bash +set -u # Exit on undefined variable # Pre-push hook: validate remote, branch name, and commit before pushing
Comment on lines
+65
to
+71
| # --- Check 5: Confirm repo identity (main vs premium) --- | ||
| TOPLEVEL=$(git rev-parse --show-toplevel 2>/dev/null) | ||
| REPO_NAME=$(basename "$TOPLEVEL") | ||
| if [[ "$BRANCH" =~ premium ]] && [[ "$REPO_NAME" != "premium" ]]; then | ||
| echo -e "${RED}[pre-push] Branch name suggests premium but you're in repo '$REPO_NAME'${NC}" | ||
| ERRORS=$((ERRORS + 1)) | ||
| fi |
There was a problem hiding this comment.
premium 匹配规则过于宽松，可能导致误报
当前检查 [[ "$BRANCH" =~ premium ]] 会匹配分支名中任意位置出现的 "premium"。例如，在主仓库中创建的 fix/ZSTAC-12345-premium-bugfix 分支会被误判为 premium 仓库的分支而报错。
建议使用更精确的匹配模式：
🐛 建议的修复方案
# --- Check 5: Confirm repo identity (main vs premium) ---
TOPLEVEL=$(git rev-parse --show-toplevel 2>/dev/null)
REPO_NAME=$(basename "$TOPLEVEL")
-if [[ "$BRANCH" =~ premium ]] && [[ "$REPO_NAME" != "premium" ]]; then
+# Only check branches explicitly targeting premium (e.g., premium/*, */premium/*)
+if [[ "$BRANCH" =~ ^premium/ || "$BRANCH" =~ /premium/ ]] && [[ "$REPO_NAME" != "premium" ]]; then
echo -e "${RED}[pre-push] Branch name suggests premium but you're in repo '$REPO_NAME'${NC}"
ERRORS=$((ERRORS + 1))
fi📝 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
| # --- Check 5: Confirm repo identity (main vs premium) --- | |
| TOPLEVEL=$(git rev-parse --show-toplevel 2>/dev/null) | |
| REPO_NAME=$(basename "$TOPLEVEL") | |
| if [[ "$BRANCH" =~ premium ]] && [[ "$REPO_NAME" != "premium" ]]; then | |
| echo -e "${RED}[pre-push] Branch name suggests premium but you're in repo '$REPO_NAME'${NC}" | |
| ERRORS=$((ERRORS + 1)) | |
| fi | |
| # --- Check 5: Confirm repo identity (main vs premium) --- | |
| TOPLEVEL=$(git rev-parse --show-toplevel 2>/dev/null) | |
| REPO_NAME=$(basename "$TOPLEVEL") | |
| # Only check branches explicitly targeting premium (e.g., premium/*, */premium/*) | |
| if [[ "$BRANCH" =~ ^premium/ || "$BRANCH" =~ /premium/ ]] && [[ "$REPO_NAME" != "premium" ]]; then | |
| echo -e "${RED}[pre-push] Branch name suggests premium but you're in repo '$REPO_NAME'${NC}" | |
| ERRORS=$((ERRORS + 1)) | |
| fi |
🤖 Prompt for AI Agents
In @.gitconfig/hooks/pre-push around lines 65 - 71, The branch-name check using
[[ "$BRANCH" =~ premium ]] is too loose and will match "premium" anywhere;
update the conditional that references the BRANCH variable to use a stricter
pattern (e.g., a regex that matches whole segment names like
(^|/)(premium)(/|$|-) or similar) so only branches whose final/segment name is
exactly "premium" (or clearly intended premium branches) trigger the error;
modify the if that uses BRANCH and REPO_NAME to apply this tighter match logic.
<fix>[compute]: add syncVmDeviceInfo when vm is running See merge request zstackio/zstack!9197
<fix>[vm]: add Destroying->Stopped state transition See merge request zstackio/zstack!9156
<fix>[i18n]: improve snapshot error message for unattached volume See merge request zstackio/zstack!9192
<fix>[zbs]: reduce mds connect timeout and enable tryNext for volume clients See merge request zstackio/zstack!9153
<fix>[vm]: use max of virtual and actual size for root disk allocation See merge request zstackio/zstack!9155
Resolves: ZSTAC-81903 Change-Id: Ia7cc6d756cf62b60b6eda2f4d4daeeee982c02d7
MatheMatrix
force-pushed
the
sync/ye.zou/fix/ZSTAC-81566
branch
from
abac4af to
343bceb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java`:
- Around line 850-852: The code in VmInstanceSpec that computes
virtualSize/actualSize using getImageSpec().getInventory().getSize() and
getActualSize() can NPE when getInventory() is null or when getActualSize()
returns a nullable Long (auto-unboxing); update the logic in the method (where
virtualSize/actualSize are computed) to null-check getImageSpec() and
getInventory() before accessing size, treat a null inventory as size 0, read
actualSize as a Long and fall back to 0L if it is null (avoid auto-unboxing),
then compute Math.max on the safe long values so no NPE occurs (references:
VmInstanceSpec, getImageSpec(), getInventory(), ImageInventory.getSize(),
ImageInventory.getActualSize()).
🧹 Nitpick comments (2)
compute/src/main/java/org/zstack/compute/vm/VmInstanceBase.java (1)
8476-8496: 建议缓存 hostUuid，避免日志与发送目标不一致
self.getHostUuid()可能在异步回调期间变化，建议本地缓存以保证一致性与可读性。♻️ 建议修改
private void syncVmDevicesAddressInfo(String vmUuid) { - if (self.getHostUuid() == null) { + String hostUuid = self.getHostUuid(); + if (hostUuid == null) { return; } SyncVmDeviceInfoMsg msg = new SyncVmDeviceInfoMsg(); msg.setVmInstanceUuid(vmUuid); - msg.setHostUuid(self.getHostUuid()); - bus.makeTargetServiceIdByResourceUuid(msg, HostConstant.SERVICE_ID, msg.getHostUuid()); + msg.setHostUuid(hostUuid); + bus.makeTargetServiceIdByResourceUuid(msg, HostConstant.SERVICE_ID, hostUuid); bus.send(msg, new CloudBusCallBack(msg) { `@Override` public void run(MessageReply reply) { if (!reply.isSuccess()) { logger.warn(String.format("Failed to sync vm device info for vm[uuid:%s], %s", vmUuid, reply.getError())); } else { logger.debug(String.format("Sent SyncVmDeviceInfoMsg for vm[uuid:%s] on host[uuid:%s]", - vmUuid, self.getHostUuid())); + vmUuid, hostUuid)); } } }); }.gitconfig/hooks/pre-push (1)
13-14:$URL变量已声明但未使用第 14 行从参数接收的
$URL变量从未被使用，第 35 行又通过git remote get-url重新获取 URL。建议移除未使用的变量或直接使用传入的参数。♻️ 建议的修复方案
REMOTE="$1" -URL="$2" +# Note: $2 contains URL but we re-resolve it below for reliability BRANCH=$(git branch --show-current)或者直接使用传入的 URL：
REMOTE="$1" URL="$2" ... # --- Check 2: Remote URL sanity --- -REMOTE_URL=$(git remote get-url "$REMOTE" 2>/dev/null) -if [ -z "$REMOTE_URL" ]; then +if [ -z "$URL" ]; then echo -e "${RED}[pre-push] Cannot resolve remote '$REMOTE'${NC}" ERRORS=$((ERRORS + 1)) fi # Warn if pushing to upstream (zstackio) directly -if echo "$REMOTE_URL" | grep -q "zstackio/" && [ "$REMOTE" = "upstream" ]; then +if echo "$URL" | grep -q "zstackio/" && [ "$REMOTE" = "upstream" ]; thenAlso applies to: 35-35
Comment on lines
+850
to
+852
| long virtualSize = this.getImageSpec().getInventory().getSize(); | ||
| long actualSize = this.getImageSpec().getInventory().getActualSize(); | ||
| return Math.max(virtualSize, actualSize); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the VmInstanceSpec.java file, focusing on the getRootDiskAllocateSize method around line 850
head -860 header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java | tail -100Repository: MatheMatrix/zstack
Length of output: 3379
🏁 Script executed:
# Find the ImageSpec class definition
fd -n "ImageSpec.java" --type fRepository: MatheMatrix/zstack
Length of output: 292
🏁 Script executed:
# Search for getInventory() definition and usage
rg -n "getInventory|setInventory" header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java -A 2 -B 2Repository: MatheMatrix/zstack
Length of output: 1141
🏁 Script executed:
# Check ImageInventory class
fd -n "ImageInventory.java" --type fRepository: MatheMatrix/zstack
Length of output: 292
🏁 Script executed:
# Find ImageInventory class
rg -l "class ImageInventory" --type javaRepository: MatheMatrix/zstack
Length of output: 179
🏁 Script executed:
# Check the ImageInventory class for getActualSize method
rg -A 5 "getActualSize" --type java | head -50Repository: MatheMatrix/zstack
Length of output: 5300
🏁 Script executed:
# Look for where getRootDiskAllocateSize is called
rg -B 3 -A 3 "getRootDiskAllocateSize\(\)" --type java | head -80Repository: MatheMatrix/zstack
Length of output: 4161
🏁 Script executed:
# Check if ImageInventory can be null in ImageSpec
rg -B 5 -A 5 "private ImageInventory inventory" header/src/main/java/org/zstack/header/vm/VmInstanceSpec.javaRepository: MatheMatrix/zstack
Length of output: 457
🏁 Script executed:
# Check the ImageInventory class definition to see getActualSize method
head -150 header/src/main/java/org/zstack/header/image/ImageInventory.java | tail -100Repository: MatheMatrix/zstack
Length of output: 3377
🏁 Script executed:
# Look for where ImageSpec is instantiated and how inventory is set
rg "new ImageSpec\(\)" --type java -A 10 | head -100Repository: MatheMatrix/zstack
Length of output: 13363
🏁 Script executed:
# Check if there's any null check pattern used before calling getInventory in the codebase
rg "getInventory\(\)" --type java -B 2 -A 2 | head -150Repository: MatheMatrix/zstack
Length of output: 12197
🏁 Script executed:
# Look for test cases that use getRootDiskAllocateSize to see expected behavior
rg "getRootDiskAllocateSize" --type java -B 5 -A 5Repository: MatheMatrix/zstack
Length of output: 6207
🏁 Script executed:
# Check if there are any unit tests for getRootDiskAllocateSize
find . -name "*Test.java" -type f -exec rg -l "getRootDiskAllocateSize" {} \;Repository: MatheMatrix/zstack
Length of output: 44
🏁 Script executed:
# Look at the full initialization context of VmInstanceSpec.imageSpec
sed -n '320,330p' header/src/main/java/org/zstack/header/vm/VmInstanceSpec.javaRepository: MatheMatrix/zstack
Length of output: 534
🏁 Script executed:
# Check if ImageInventory fields can be null - look at the type of actualSize field
sed -n '1,200p' header/src/main/java/org/zstack/header/image/ImageInventory.java | grep -A 2 "private.*actualSize"Repository: MatheMatrix/zstack
Length of output: 141
🏁 Script executed:
# Check the return type of getSize() in ImageInventory
rg "public.*getSize\(\)" header/src/main/java/org/zstack/header/image/ImageInventory.java -A 1Repository: MatheMatrix/zstack
Length of output: 110
🏁 Script executed:
# Check if there are any defensive null checks before calling getActualSize on actualSize fields
rg "actualSize.*null|null.*actualSize" --type java | head -30Repository: MatheMatrix/zstack
Length of output: 3319
🏁 Script executed:
# Verify Math.max behavior with auto-unboxing of null Long
cat > /tmp/test_autobox.java << 'EOF'
public class Test {
public static void main(String[] args) {
Long actualSize = null;
long virtualSize = 100L;
// This will throw NPE on auto-unboxing
long result = Math.max(virtualSize, actualSize);
}
}
EOF
javac /tmp/test_autobox.java 2>&1 | head -20Repository: MatheMatrix/zstack
Length of output: 106
存在多处空指针异常风险
第 850-852 行代码存在以下空指针异常隐患：
- 当
getInventory()返回null时，调用getSize()和getActualSize()会抛出NullPointerException ImageInventory.getActualSize()返回Long类型（可为 null），在自动拆箱到基本类型long时若为 null 同样会触发NullPointerException
建议添加完整的空值检查，同时考虑 actualSize 为 null 的情况：
🛡️ 建议的修复方案
public long getRootDiskAllocateSize() {
if (rootDiskOffering == null) {
+ ImageInventory inventory = this.getImageSpec().getInventory();
+ if (inventory == null) {
+ return 0;
+ }
- long virtualSize = this.getImageSpec().getInventory().getSize();
- long actualSize = this.getImageSpec().getInventory().getActualSize();
+ long virtualSize = inventory.getSize();
+ Long actualSizeWrapper = inventory.getActualSize();
+ long actualSize = actualSizeWrapper != null ? actualSizeWrapper : virtualSize;
return Math.max(virtualSize, actualSize);
}
return rootDiskOffering.getDiskSize();
}🤖 Prompt for AI Agents
In `@header/src/main/java/org/zstack/header/vm/VmInstanceSpec.java` around lines
850 - 852, The code in VmInstanceSpec that computes virtualSize/actualSize using
getImageSpec().getInventory().getSize() and getActualSize() can NPE when
getInventory() is null or when getActualSize() returns a nullable Long
(auto-unboxing); update the logic in the method (where virtualSize/actualSize
are computed) to null-check getImageSpec() and getInventory() before accessing
size, treat a null inventory as size 0, read actualSize as a Long and fall back
to 0L if it is null (avoid auto-unboxing), then compute Math.max on the safe
long values so no NPE occurs (references: VmInstanceSpec, getImageSpec(),
getInventory(), ImageInventory.getSize(), ImageInventory.getActualSize()).
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.
Resolves: ZSTAC-81566
Target: 5.5.6
sync from gitlab !9188