<fix>[zns]: ZCF-3728 propagate vm system tags#4017
Closed
ZStack-Robot wants to merge 2 commits into
Closed
Conversation
Pass VM API system tags into AllocateHostMsg so host allocator filters can honor znsNicMode during VM placement. Resolves: ZCF-3728 Change-Id: I4fbb522440931aad005ceb82a37a29236bd35b27
Walkthrough新增：在宿主分配请求中转发 APIMessage 的 systemTags；将容量重算中补齐 HostUsedCpuMem 的遍历逻辑改为显式 for 循环并跳过有 VM 的宿主；为 KVM 容量上报消息设置 30 分钟超时。 ChangesHost allocation and capacity changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
compute/src/main/java/org/zstack/compute/vm/VmAllocateHostFlow.java (1)
99-101: ⚡ Quick win建议提取类型转换以提升可读性
当前代码对
spec.getMessage()进行了多次类型转换为APIMessage，建议提取到局部变量以避免重复转换并提升代码可读性。♻️ 建议的重构
- if (spec.getMessage() instanceof APIMessage && ((APIMessage) spec.getMessage()).getSystemTags() != null) { - msg.setSystemTags(new ArrayList<>(((APIMessage) spec.getMessage()).getSystemTags())); + if (spec.getMessage() instanceof APIMessage) { + APIMessage apiMessage = (APIMessage) spec.getMessage(); + if (apiMessage.getSystemTags() != null) { + msg.setSystemTags(new ArrayList<>(apiMessage.getSystemTags())); + } }🤖 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/vm/VmAllocateHostFlow.java` around lines 99 - 101, The code repeatedly casts spec.getMessage() to APIMessage when checking and copying system tags; extract spec.getMessage() as a local APIMessage variable (e.g., APIMessage apiMsg = (APIMessage) spec.getMessage()) after verifying instance-of, then use apiMsg.getSystemTags() when calling msg.setSystemTags(new ArrayList<>(apiMsg.getSystemTags())); update the conditional to use the local variable to improve readability and avoid duplicate casts in VmAllocateHostFlow.
🤖 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.
Nitpick comments:
In `@compute/src/main/java/org/zstack/compute/vm/VmAllocateHostFlow.java`:
- Around line 99-101: The code repeatedly casts spec.getMessage() to APIMessage
when checking and copying system tags; extract spec.getMessage() as a local
APIMessage variable (e.g., APIMessage apiMsg = (APIMessage) spec.getMessage())
after verifying instance-of, then use apiMsg.getSystemTags() when calling
msg.setSystemTags(new ArrayList<>(apiMsg.getSystemTags())); update the
conditional to use the local variable to improve readability and avoid duplicate
casts in VmAllocateHostFlow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 4302ffa1-ded1-43fc-89aa-fdbac3761164
📒 Files selected for processing (1)
compute/src/main/java/org/zstack/compute/vm/VmAllocateHostFlow.java
Avoid bad host allocator lambda bytecode. Set KVM capacity sync timeout explicitly. Test: VmwareTest passed on root@172.20.13.240. Resolves: ZCF-3728 Change-Id: I5db26740b9fef0c0ba1b31096a8fdc3659db412b
Collaborator
Author
|
Comment from gitlab: 自上次添加REVIEWED标签(2026-05-19 08:25:09.000Z)后, 有新的COMMIT更新(2026-05-19 22:31:47.787Z), 所以移除了REVIEWED标签 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin/kvm/src/main/java/org/zstack/kvm/KVMHostCapacityExtension.java (1)
35-35: ⚡ Quick win将超时分钟数提取为命名常量，避免魔法值。
当前实现可用，但
30建议抽成常量，便于统一调整和语义表达。建议修改
public class KVMHostCapacityExtension implements KVMHostConnectExtensionPoint, HostConnectionReestablishExtensionPoint { + private static final long CHECK_HOST_CAPACITY_TIMEOUT_MILLIS = TimeUnit.MINUTES.toMillis(30); @@ - msg.setTimeout(TimeUnit.MINUTES.toMillis(30)); + msg.setTimeout(CHECK_HOST_CAPACITY_TIMEOUT_MILLIS);As per coding guidelines: “避免使用魔法值（Magic Value）：直接使用未经定义的数值或字符串应替换为常量或枚举。”
🤖 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 `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostCapacityExtension.java` at line 35, Replace the magic literal 30 in the timeout call with a named constant: declare a private static final long (e.g. HOST_CAPACITY_TIMEOUT_MINUTES = 30L) in the KVMHostCapacityExtension class and use it in msg.setTimeout(TimeUnit.MINUTES.toMillis(HOST_CAPACITY_TIMEOUT_MINUTES)); this centralizes the value, documents its meaning, and avoids the magic number.
🤖 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.
Nitpick comments:
In `@plugin/kvm/src/main/java/org/zstack/kvm/KVMHostCapacityExtension.java`:
- Line 35: Replace the magic literal 30 in the timeout call with a named
constant: declare a private static final long (e.g.
HOST_CAPACITY_TIMEOUT_MINUTES = 30L) in the KVMHostCapacityExtension class and
use it in
msg.setTimeout(TimeUnit.MINUTES.toMillis(HOST_CAPACITY_TIMEOUT_MINUTES)); this
centralizes the value, documents its meaning, and avoids the magic number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: d9990735-ead3-4930-8388-b05b4a7d2f97
📒 Files selected for processing (2)
compute/src/main/java/org/zstack/compute/allocator/HostAllocatorManagerImpl.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostCapacityExtension.java
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.
Summary\nFix ZCF-3728 ZNS VM placement input propagation. VM API system tags are now copied to AllocateHostMsg so allocator filters can see znsNicMode.\n\n## Changes\n- Propagate API system tags into AllocateHostMsg during VM host allocation.\n\n## Testing\n- [x] mvn -pl compute -am -DskipTests install\n- [x] ./runMavenProfile premium\n- [x] mvn -pl test-premium -Dtest=TestZnsVmNicBasicLifeCycleCase -DskipJacoco=true -DfailIfNoTests=false test\n\nResolves: ZCF-3728
sync from gitlab !9911