<fix>[vm]: validate local storage mount point#4345
Conversation
MountBlockDevice now rejects a mount point that is already used by local primary storage attached to the host's cluster, so the API fails before mounting or formatting the block device. Add cluster lookup to ProxyHardwareFactory and KVMHostFactory. Validate APIMountBlockDeviceMsg mountPoint before proxy hardware retrieval. Check existing local primary storage URLs in the resolved cluster and return API error on conflict. Resolves: ZSV-6589 Change-Id: I747a706370786d6b75666b756c6d7469706b636d
Walkthrough在 Changes挂载点与本地主存储冲突校验
估计代码审查工作量🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Comment from yaohua.wu: Review: MR !10306 — ZSV-6589目标分支: 总体评价变更与 Jira ZSV-6589 及甘涛在 issue 中确认的最终方案一致：在 另外发现一处正向改进：原代码 🟡 Warning1. 删除了 mountPoint 末尾 if (mountPoint.endsWith("/") && !mountPoint.equals("/")) {
throw new ApiMessageInterceptionException(operr(
"mountPoint should not end with '/' except root directory"));
}本次目标只需要在比较时把 2. private static String normalizeMountPoint(String path) {
while (path.length() > 1 && path.endsWith("/")) { ... } // path 为 null 时 NPE
}
🟢 Suggestion3. 校验仅对「已注册到集群的 KVM 物理机」生效，存在覆盖盲区 4. import 顺序 5. TOCTOU（信息项，非阻塞） 验证建议
🤖 Robot Reviewer |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.java`:
- Around line 262-270: The mount point comparison in
HostApiInterceptor.validateMountPoint only trims trailing slashes, so paths
containing "." can bypass the local storage conflict check; update
normalizeMountPoint and the compare flow so both msg.getMountPoint() and each
PrimaryStorageVO_.url are fully normalized before equality checks. Use the
existing validateMountPoint and normalizeMountPoint logic to ensure both values
resolve to the same canonical path representation, then compare the normalized
results in the loop over localStorageUrls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 22003620-0e41-4bdf-9e7d-349ced8c6485
📒 Files selected for processing (3)
compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.javaheader/src/main/java/org/zstack/header/agent/ProxyHardwareFactory.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java
| String mountPoint = normalizeMountPoint(msg.getMountPoint()); | ||
| List<String> localStorageUrls = Q.New(PrimaryStorageVO.class) | ||
| .select(PrimaryStorageVO_.url) | ||
| .eq(PrimaryStorageVO_.type, PrimaryStorageConstants.LOCAL_STORAGE_TYPE) | ||
| .in(PrimaryStorageVO_.uuid, primaryStorageUuids) | ||
| .listValues(); | ||
|
|
||
| for (String url : localStorageUrls) { | ||
| if (!mountPoint.equals(normalizeMountPoint(url))) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.java"
echo "== file line count =="
wc -l "$file"
echo "== relevant slices =="
sed -n '220,320p' "$file" | cat -n
echo "== usages of normalizeMountPoint =="
rg -n "normalizeMountPoint|validateMountPoint|MountPoint" "$file"Repository: MatheMatrix/zstack
Length of output: 4014
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search for path normalization helpers =="
rg -n "Paths\.get\(|normalize\(\)|canonicali[sz]e|normalizeMountPoint" compute/src/main/java
echo "== inspect validateMountPoint area =="
sed -n '210,285p' compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.java | cat -nRepository: MatheMatrix/zstack
Length of output: 4326
补全 mount point 规范化，避免 . 路径段绕过本地主存储占用检查
validateMountPoint 允许 .，但 normalizeMountPoint 只去掉尾部 /；像 /local_ps/./data 这类路径会和 /local_ps/data 比较不相等，却可能落到同一目录。这里应把 msg.getMountPoint() 和已存在的 url 统一做路径归一化后再比对。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.java` around
lines 262 - 270, The mount point comparison in
HostApiInterceptor.validateMountPoint only trims trailing slashes, so paths
containing "." can bypass the local storage conflict check; update
normalizeMountPoint and the compare flow so both msg.getMountPoint() and each
PrimaryStorageVO_.url are fully normalized before equality checks. Use the
existing validateMountPoint and normalizeMountPoint logic to ensure both values
resolve to the same canonical path representation, then compare the normalized
results in the loop over localStorageUrls.
3 participants
MountBlockDevice now rejects a mount point that is already used by local primary storage attached to the host's cluster, so the API fails before mounting or formatting the block device.
Add cluster lookup to ProxyHardwareFactory and KVMHostFactory.
Validate APIMountBlockDeviceMsg mountPoint before proxy hardware retrieval.
Check existing local primary storage URLs in the resolved cluster and return API error on conflict.
Resolves: ZSV-6589
Change-Id: I747a706370786d6b75666b756c6d7469706b636d
sync from gitlab !10306