<fix>[compute]: ZSV-6589 validate local storage mount point before mounting#4340
<fix>[compute]: ZSV-6589 validate local storage mount point before mounting#4340zstack-robot-2 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 55 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough新增挂载块设备请求的前置校验顺序，补充 Changes挂载块设备校验
Sequence Diagram(s)sequenceDiagram
participant HostApiInterceptor
participant ProxyHardwareFactory
participant KVMHostFactory
participant Database
HostApiInterceptor->>HostApiInterceptor: validate path and mountPoint
HostApiInterceptor->>ProxyHardwareFactory: getClusterUuid(hostname)
ProxyHardwareFactory->>KVMHostFactory: getClusterUuid(hostName)
KVMHostFactory-->>ProxyHardwareFactory: clusterUuid
ProxyHardwareFactory-->>HostApiInterceptor: clusterUuid
HostApiInterceptor->>Database: count LOCAL_STORAGE_TYPE by url and clusterUuid
Database-->>HostApiInterceptor: count
HostApiInterceptor->>HostApiInterceptor: reject when mountPoint is occupied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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 253-265: Local storage mount-point matching is using the raw URL,
so the occupancy check can miss equivalent paths that differ only by a trailing
slash. Update the normalization flow in LocalStorageFactory.createPrimaryStorage
and LocalStorageApiInterceptor so the stored/queryable URL is canonicalized
consistently, and adjust HostApiInterceptor’s occupancy query to compare against
the normalized mount point instead of ps.url = :url on the raw value. Use the
existing createPrimaryStorage and LocalStorageApiInterceptor handling as the
reference points so both write and read sides stay aligned.
In `@header/src/main/java/org/zstack/header/agent/ProxyHardwareFactory.java`:
- Line 6: The new ProxyHardwareFactory.getClusterUuid method needs Javadoc
describing the hostName matching semantics and whether returning null is allowed
when no cluster is found. Add a proper Javadoc comment directly on
getClusterUuid so the interface contract is explicit and compliant, and keep the
method declaration free of extra modifiers.
🪄 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: 13f7ed18-25d7-4566-a06c-7460afc64157
📒 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
| public interface ProxyHardwareFactory { | ||
| ProxyHardware getProxyHardware(String hostName); | ||
|
|
||
| String getClusterUuid(String hostName); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
为新增接口方法补充 Javadoc。
接口新增契约缺少说明，建议明确 hostName 的匹配语义以及找不到集群时是否允许返回 null。
建议修改
+ /**
+ * Returns the cluster UUID for the host identified by {`@code` hostName}, or {`@code` null} when it cannot be resolved.
+ */
String getClusterUuid(String hostName);按路径规则，“接口方法不应有多余的修饰符（例如 public），且必须配有有效的 Javadoc 注释。”
📝 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.
| String getClusterUuid(String hostName); | |
| /** | |
| * Returns the cluster UUID for the host identified by {`@code` hostName}, or {`@code` null} when it cannot be resolved. | |
| */ | |
| String getClusterUuid(String hostName); |
🤖 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 `@header/src/main/java/org/zstack/header/agent/ProxyHardwareFactory.java` at
line 6, The new ProxyHardwareFactory.getClusterUuid method needs Javadoc
describing the hostName matching semantics and whether returning null is allowed
when no cluster is found. Add a proper Javadoc comment directly on
getClusterUuid so the interface contract is explicit and compliant, and keep the
method declaration free of extra modifiers.
Source: Path instructions
MatheMatrix
force-pushed
the
sync/tao.gan/fix/ZSV-6589-mount-block-device-path-check
branch
7 times, most recently
from
63aa7f4 to
6d768a3
Compare
Resolves: ZSV-6589 Test: rtk mvn -pl header,compute,plugin/kvm -am -DskipTests compile Change-Id: Ia3925e6015178899e302f0c9c98dbf3a5432101f
MatheMatrix
force-pushed
the
sync/tao.gan/fix/ZSV-6589-mount-block-device-path-check
branch
from
6d768a3 to
7b35aad
Compare
2 participants
Summary
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.
Changes
Testing
rtk mvn -pl header,compute,plugin/kvm -am -DskipTests compilertk git -C zstack diff --check -- zstack/header/src/main/java/org/zstack/header/agent/ProxyHardwareFactory.java zstack/plugin/kvm/src/main/java/org/zstack/kvm/KVMHostFactory.java zstack/compute/src/main/java/org/zstack/compute/host/HostApiInterceptor.javaResolves: ZSV-6589
sync from gitlab !10301