Skip to content

<fix>[compute]: fix VM clone quota check fail#3590

Open
zstack-robot-1 wants to merge 1 commit into5.5.12from
sync/tian.huang/ZSTAC-83646@@2
Open

<fix>[compute]: fix VM clone quota check fail#3590
zstack-robot-1 wants to merge 1 commit into5.5.12from
sync/tian.huang/ZSTAC-83646@@2

Conversation

@zstack-robot-1
Copy link
Collaborator

1.Clone a tenant VM using admin account, quota check
didn't base on current role.
2.quota error is overwritten during exception handling.

Resolves: ZSTAC-83646

Change-Id: I706f6d6b726b6864746867727269716a796e7677

sync from gitlab !9451

1.Clone a tenant VM using admin account, quota check
didn't base on current role.
2.quota error is overwritten during exception handling.

Resolves: ZSTAC-83646

Change-Id: I706f6d6b726b6864746867727269716a796e7677
@coderabbitai
Copy link

coderabbitai bot commented Mar 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent 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: e075e489-9c6e-4c26-9b24-95f8596d1e37

📥 Commits

Reviewing files that changed from the base of the PR and between 8f58fad and e790d11.

📒 Files selected for processing (4)
  • compute/src/main/java/org/zstack/compute/allocator/HostAllocatorChain.java
  • compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java
  • header/src/main/java/org/zstack/header/allocator/AllocateHostMsg.java
  • header/src/main/java/org/zstack/header/allocator/HostAllocatorSpec.java

概述

本次变更在主机分配链路中新增了会话账户UUID字段支持,并在配额检查前进行管理员权限提前检查。当会话账户为管理员时,绕过后续的VM/账户配额验证逻辑。同时优化了API消息拦截异常的处理路径。

变更详情

分组 / 文件 变更摘要
消息与规范扩展
header/src/main/java/org/zstack/header/allocator/AllocateHostMsg.java, header/src/main/java/org/zstack/header/allocator/HostAllocatorSpec.java
新增 sessionAccountUuid 字段及其公开的 getter/setter 方法。HostAllocatorSpec.fromAllocationMsg() 工厂方法已更新以从 AllocateHostMsg 复制该字段。
主机分配链处理
compute/src/main/java/org/zstack/compute/allocator/HostAllocatorChain.java
调整异常处理逻辑,对 ApiMessageInterceptionException 进行显式捕获并路由至链失败路径,而非使用通用 Throwable 处理器。
配额分配流
compute/src/main/java/org/zstack/compute/allocator/QuotaAllocatorFlow.java
新增提前的管理员权限检查。当会话账户UUID非空且为管理员时,直接调用 next(candidates) 返回,绕过后续的VM/账户配额验证逻辑。

代码审查工作量评估

🎯 2 (Simple) | ⏱️ ~12 分钟

诗歌

会话追踪添新字,
管理员检查早一步,
配额验证更高效,🐰
分配链路更清晰。

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed PR title follows the required [scope]: format with 46 characters, meeting the 72-character limit and accurately describing the VM clone quota check fix.
Description check ✅ Passed PR description is directly related to the changeset, clearly explaining the quota check issues being fixed and providing context for the changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sync/tian.huang/ZSTAC-83646@@2

Comment @coderabbitai help to get the list of available commands and usage tips.

@ZStack-Robot
Copy link
Collaborator

Comment from yaohua.wu:

Review: MR !9451 + !13318 — ZSTAC-83646

Jira 背景:Admin clone 租户 VM 时,配额检查使用了 VM owner(租户)的 quota 导致报错;同时 ApiMessageInterceptionExceptioncatch(Throwable) 吞掉,报错信息丢失变成 "unhandled exception"。

变更概述(跨 2 个 MR):

  1. AllocateHostMsg / HostAllocatorSpec 新增 sessionAccountUuid 字段,传递操作者身份
  2. QuotaAllocatorFlow 新增前置检查:操作者为 admin 时跳过 quota 检查
  3. HostAllocatorChain 新增 ApiMessageInterceptionException 的 specific catch,保留原始配额错误
  4. MevocoVmInstanceBase(premium)在 KVM clone 流程设置 sessionAccountUuid

Warning

  1. [QuotaAllocatorFlow.java] 修复覆盖面:仅 KVM clone 路径设置了 sessionAccountUuid

    全局搜索 new DesignatedAllocateHostMsg() 发现以下 caller 拥有 session context 但未设置 sessionAccountUuid

    • VmInstanceManagerImpl.handle(APIGetCandidateZonesClustersHostsForCreatingVmMsg) — API handler,有 msg.getSession()

    当前场景下影响有限("获取候选主机" 的查询场景中 VM 尚未创建,owner 就是当前用户),但建议评估是否需要在其他有 session 的 API handler 中统一设置 sessionAccountUuid,避免遗忘路径。

    当前修复仅覆盖 KVM clone 一个入口点。如果未来有其他 "admin 代操作租户资源" 且经过 QuotaAllocatorFlow 的场景,需要记得设置该字段 —— 这是一种隐式约定,存在遗忘路径风险。

Suggestion

  1. [MevocoVmInstanceBase.java:1124] 预存在问题:L3NetworkUuids 条件可能反了(非本 MR 引入)

    if (msg.getVmNicParms().isEmpty()) {
        amsg.setL3NetworkUuids(msg.getVmNicParms().stream()...);  // 空 list stream → 空结果
    } else {
        amsg.setL3NetworkUuids(VmNicHelper.getL3Uuids(VmNicInventory.valueOf(self.getVmNics())));
    }

    getVmNicParms() 为空时,stream 产生空 list;不为空时反而用了 self.getVmNics()。逻辑似乎应该是 !isEmpty()。但这是预存在代码,不阻塞本 MR。

  2. 缺少测试用例 — 建议补充一个 "admin clone 超配额租户 VM" 的集成测试,覆盖 quota 跳过逻辑和错误信息保留两个修复点。

✅ 正确性确认

  • QuotaAllocatorFlow 新增的 admin 检查位置正确:在既有的 "VM owner is admin" 检查之前,新增 "session operator is admin" 检查。两个检查互不冲突,覆盖不同场景:
    • 既有检查:admin 自己的 VM → owner 是 admin → 跳过 ✓
    • 新增检查:admin 操作租户 VM → owner 是租户但 operator 是 admin → 跳过 ✓
  • sessionAccountUuid 为 null 时安全sessionAccountUuid != null && isAdminPermission(...) 的 null 检查确保所有未设置该字段的既有 caller 行为不变。
  • HostAllocatorChain 异常捕获顺序正确ApiMessageInterceptionException(extends RuntimeException)的 catch 在 Throwable 之前,保证 quota 报错被正确传播而非被覆盖。
  • HostAllocatorSpec.fromAllocationMsg() 已正确映射新字段。

Verdict: APPROVED

修复逻辑正确、向后兼容、解决了 Jira 描述的两个子问题。Warning 项为防御性建议,不阻塞合并。


🤖 Robot Reviewer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants