refactor: VM 할당 개선#72
Conversation
- Introduced loadScore function to calculate core load based on resource utilization. - Added utilRatio and saturationPenalty functions for improved resource management. - Updated SelectCore to select the core with the lowest load score that meets resource requirements.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds vCPU status to CPU responses, replaces first-fit allocation with scored reservation and unified release APIs, and updates VM lifecycle handling and core initialization to use reserved resources and idle-based CPU tracking. ChangesvCPU status and resource manager rework
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CreateVM
participant ResourceManager
participant Core
CreateVM->>ResourceManager: reserveCoreOrFail(req)
ResourceManager->>Core: score cores and reserve Free*
ResourceManager-->>CreateVM: reserved core
CreateVM->>ResourceManager: AttachVMInfo(core, uuid, vm)
alt setup failure
CreateVM->>ResourceManager: cleanup.run()
ResourceManager->>Core: ReleaseVM and restore Free*
else core VM creation succeeds
CreateVM->>Core: CreateVM
end
CreateVM->>ResourceManager: ReleaseVM(core, uuid) on deletion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🧹 Nitpick comments (2)
structure/resource_manager.go (1)
36-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid scoring model — consider adding unit tests for the new helpers.
The weighting, saturation curve, and variance-balance terms are well thought out and internally consistent with their doc comments. Since this directly drives placement decisions, table-driven tests for
utilRatio/saturationPenalty/loadScore(boundary cases:total==0,free==total,free==req) would guard against regressions when the weights are tuned later.🤖 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 `@structure/resource_manager.go` around lines 36 - 95, Add table-driven unit tests for the new scoring helpers in resource_manager.go, covering utilRatio, saturationPenalty, and loadScore. Focus on boundary cases like total==0, free==total, and free==req, plus a few representative weighting/variance scenarios so future tuning of loadWeightMemory, loadWeightDisk, loadWeightCPU, and loadBalanceGain won’t silently change placement behavior. Use the function names utilRatio, saturationPenalty, and loadScore to locate the logic.startup/init.go (1)
207-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated fetch/error-handling pattern across Memory, Disk, and CPU info.
The three info-fetch blocks (memory, disk, now CPU) repeat the same
if err != nil { currentCore.IsAlive = false; return fmt.Errorf(...) }shape. Extracting a small helper (e.g., a generic wrapper taking a fetch func and a resource-name string) would reduce duplication and keep future resource additions consistent.♻️ Example helper sketch
func fetchOrFail[T any](currentCore *structure.Core, resourceName, ip string, port uint16, fetch func() (T, error)) (T, error) { val, err := fetch() if err != nil { currentCore.IsAlive = false } return val, err }🤖 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 `@startup/init.go` around lines 207 - 225, The Memory, Disk, and CPU fetch blocks in initCoreInfo repeat the same error-handling pattern, so factor that logic into a small helper around the coreClient calls. Add a reusable function (for example, a fetchOrFail-style helper near startup/init.go) that takes currentCore, a resource name, and a fetch callback, sets currentCore.IsAlive to false on failure, and returns the wrapped error message; then use it for GetCoreMachineMemoryInfo, GetCoreMachineDiskInfo, and GetCoreMachineCpuInfo to keep the behavior identical while removing duplication.
🤖 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 `@service/vm.go`:
- Around line 35-39: The rollback registered in the cleanup chain is only
releasing scheduler resources, but after core VM creation it can leave an
existing VM and Redis state behind if a later DB step fails. Update the failure
handling around core VM creation and the cleanup path in vm.go so that post-core
rollback first deletes the VM from the core and clears Redis state before
calling Resources.DeallocateResources. If needed, split the pre-core reservation
rollback from the post-core compensating rollback to ensure Free* capacity is
only restored after the core-side delete succeeds.
- Around line 247-252: The cleanup flow in ReleaseVM is removing
VMLocation/AliveVM too early, before VMRepo.DeleteInstance can still fail
afterward. Update the VM deletion path in ReleaseVM and its caller so the
in-memory VM mapping is only cleared after the fatal DB cleanup succeeds, or
otherwise make the later DeleteInstance failure non-fatal by preserving enough
state for a retry/reconciliation path. Use the ReleaseVM and
VMRepo.DeleteInstance flow to keep FindCoreByVmUUID retryable and avoid leaving
stale DB state with no recovery path.
- Line 174: The debug log in the core selection path is using the wrong units
for the request values. Update the log message in the core selection flow that
uses `log.DebugInfo` so it reports `HardwareRequirement.Memory` and `Disk` in
MiB, not GiB, while keeping `CPU` unchanged. Make sure the message in the
selection logic reflects the actual units used by `req.Memory`, `req.CPU`, and
`req.Disk`.
In `@startup/init.go`:
- Around line 217-225: Handle GetCoreMachineCpuInfo failures locally inside the
per-core goroutine in InitializeCoreData instead of returning the error to
errgroup and aborting g.Wait(). In the block that sets currentCore.IsAlive and
calls coreClient.GetCoreMachineCpuInfo, keep marking the specific core as not
alive and then continue processing the other cores; only return from the
goroutine for truly fatal setup errors. Use the existing InitializeCoreData,
GetCoreMachineCpuInfo, and currentCore.IsAlive flow to keep the failure scoped
to that core.
---
Nitpick comments:
In `@startup/init.go`:
- Around line 207-225: The Memory, Disk, and CPU fetch blocks in initCoreInfo
repeat the same error-handling pattern, so factor that logic into a small helper
around the coreClient calls. Add a reusable function (for example, a
fetchOrFail-style helper near startup/init.go) that takes currentCore, a
resource name, and a fetch callback, sets currentCore.IsAlive to false on
failure, and returns the wrapped error message; then use it for
GetCoreMachineMemoryInfo, GetCoreMachineDiskInfo, and GetCoreMachineCpuInfo to
keep the behavior identical while removing duplication.
In `@structure/resource_manager.go`:
- Around line 36-95: Add table-driven unit tests for the new scoring helpers in
resource_manager.go, covering utilRatio, saturationPenalty, and loadScore. Focus
on boundary cases like total==0, free==total, and free==req, plus a few
representative weighting/variance scenarios so future tuning of
loadWeightMemory, loadWeightDisk, loadWeightCPU, and loadBalanceGain won’t
silently change placement behavior. Use the function names utilRatio,
saturationPenalty, and loadScore to locate the logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c3f3fba-8be2-4cac-b200-7eca6f6ecee5
📒 Files selected for processing (5)
client/model/vm.goclient/vm.goservice/vm.gostartup/init.gostructure/resource_manager.go
| func reserveCoreOrFail(contextStruct *vms.ControlContext, req vms.HardwareRequirement) (*vms.Core, int, error) { | ||
| log := util.GetLogger() | ||
|
|
||
| log.DebugInfo("core selection process. req: memory=%d GiB, cpu=%d, disk=%d", req.Memory, req.CPU, req.Disk) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the diagnostic units.
HardwareRequirement.Memory and Disk are MiB, but this log prints GiB, which makes capacity failures look 1024x different from the actual request.
Suggested fix
- log.DebugInfo("core selection process. req: memory=%d GiB, cpu=%d, disk=%d", req.Memory, req.CPU, req.Disk)
+ log.DebugInfo("core selection process. req: memory=%d MiB, cpu=%d, disk=%d MiB", req.Memory, req.CPU, req.Disk)📝 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.
| log.DebugInfo("core selection process. req: memory=%d GiB, cpu=%d, disk=%d", req.Memory, req.CPU, req.Disk) | |
| log.DebugInfo("core selection process. req: memory=%d MiB, cpu=%d, disk=%d MiB", req.Memory, req.CPU, req.Disk) |
🤖 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 `@service/vm.go` at line 174, The debug log in the core selection path is using
the wrong units for the request values. Update the log message in the core
selection flow that uses `log.DebugInfo` so it reports
`HardwareRequirement.Memory` and `Disk` in MiB, not GiB, while keeping `CPU`
unchanged. Make sure the message in the selection logic reflects the actual
units used by `req.Memory`, `req.CPU`, and `req.Disk`.
| // Core 삭제 성공 -> 인메모리 자원 회수 (Free* 복구 + VMInfoIdx/VMLocation/AliveVM 정리) | ||
| if contextStruct.Resources.ReleaseVM(core, uuid) { | ||
| log.DebugInfo("released in-memory resources for VM %s on core %s: FreeMemory=%d, FreeCPU=%d, FreeDisk=%d", | ||
| uuid, core.IP, core.FreeMemory, core.FreeCPU, core.FreeDisk) | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don’t remove the in-memory VM location before later fatal cleanup can fail.
ReleaseVM deletes VMLocation/AliveVM, but VMRepo.DeleteInstance can still return an error afterward. A retry then may fail at FindCoreByVmUUID, leaving stale DB state without a normal retry path. Keep enough in-memory deletion state until fatal cleanup succeeds, or make the later DB failure non-fatal with a reconciliation/retry path.
🤖 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 `@service/vm.go` around lines 247 - 252, The cleanup flow in ReleaseVM is
removing VMLocation/AliveVM too early, before VMRepo.DeleteInstance can still
fail afterward. Update the VM deletion path in ReleaseVM and its caller so the
in-memory VM mapping is only cleared after the fatal DB cleanup succeeds, or
otherwise make the later DeleteInstance failure non-fatal by preserving enough
state for a retry/reconciliation path. Use the ReleaseVM and
VMRepo.DeleteInstance flow to keep FindCoreByVmUUID retryable and avoid leaving
stale DB state with no recovery path.
| cpuResp, err := coreClient.GetCoreMachineCpuInfo(ctx) | ||
| if err != nil { | ||
| currentCore.IsAlive = false | ||
| return fmt.Errorf("failed to get CPU info for core %s:%d: %w", currentCore.IP, currentCore.Port, err) | ||
| } | ||
| if cpuResp == nil || cpuResp.VcpuStatus == nil { | ||
| currentCore.IsAlive = false | ||
| return fmt.Errorf("core %s:%d returned no vcpu_status in CPU info", currentCore.IP, currentCore.Port) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== startup/init.go around InitializeCoreData ==\n'
nl -ba startup/init.go | sed -n '140,280p'
printf '\n== client/model/vm.go around VcpuStatus ==\n'
nl -ba client/model/vm.go | sed -n '70,120p'
printf '\n== client/vm.go around GetCoreMachineCpuInfo ==\n'
nl -ba client/vm.go | sed -n '80,140p'
printf '\n== search for errgroup usage in startup/init.go ==\n'
rg -n "errgroup|Wait\(\)|Go\(" startup/init.goRepository: easy-cloud-Knet/KWS_Control
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== startup/init.go around InitializeCoreData ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path("startup/init.go")
for i, line in enumerate(p.read_text().splitlines(), 1):
if 140 <= i <= 280:
print(f"{i:4d}\t{line}")
PY
printf '\n== client/model/vm.go around VcpuStatus ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path("client/model/vm.go")
for i, line in enumerate(p.read_text().splitlines(), 1):
if 70 <= i <= 120:
print(f"{i:4d}\t{line}")
PY
printf '\n== client/vm.go around GetCoreMachineCpuInfo ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path("client/vm.go")
for i, line in enumerate(p.read_text().splitlines(), 1):
if 80 <= i <= 140:
print(f"{i:4d}\t{line}")
PY
printf '\n== search for errgroup usage in startup/init.go ==\n'
python3 - <<'PY'
from pathlib import Path
for i, line in enumerate(Path("startup/init.go").read_text().splitlines(), 1):
if "errgroup" in line or "Wait()" in line or ".Go(" in line:
print(f"{i:4d}\t{line}")
PYRepository: easy-cloud-Knet/KWS_Control
Length of output: 9872
Single core CPU lookup still aborts startup for all cores. GetCoreMachineCpuInfo errors are returned from the errgroup goroutine, so g.Wait() fails InitializeCoreData instead of just marking that core IsAlive = false. If this is meant to be a per-core health update, keep the failure local and continue.
🤖 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 `@startup/init.go` around lines 217 - 225, Handle GetCoreMachineCpuInfo
failures locally inside the per-core goroutine in InitializeCoreData instead of
returning the error to errgroup and aborting g.Wait(). In the block that sets
currentCore.IsAlive and calls coreClient.GetCoreMachineCpuInfo, keep marking the
specific core as not alive and then continue processing the other cores; only
return from the goroutine for truly fatal setup errors. Use the existing
InitializeCoreData, GetCoreMachineCpuInfo, and currentCore.IsAlive flow to keep
the failure scoped to that core.
kwonkwonn
left a comment
kwonkwonn
left a comment
There was a problem hiding this comment.
눈에 띄는 에러가 안보이기도 했고, 기존 방식보다는 훨씬 나아 보입니다.
테스트 후 큰 문제가 없으면 머지해도 될 거 같아요
엥 commit하나 붙이니 리뷰가 사라져버렸네요. |
2 participants
Summary
Motivation
기존
SelectCore는 그저 앞에서부터 조건을 만족하는 첫 코어를 고르는 문제가 있었음.동시 생성 시 over subscription이 발생 가능했으며, 삭제 시 인메모리쪽 되돌리지 않았음.
Approach
부하분산 스케줄링
loadScore가 가장 작은 코어 선택.0.6> disk0.3> cpu0.1.init CPU 정보
GetCoreMachineCpuInfo(vcpu_status)로CoreInfoIdx.Cpu(total)·FreeCPU(idle)를 실제 값으로 설정.IsAlive = false처리.VM 삭제 시 인메모리 회수
ReleaseVM(core, uuid)추가, Reserve+Attach+Register atomic하게 역연산.Free*복구 +VMInfoIdx/VMLocation/AliveVM정리를 한 lock 안에서 수행.Type of Change
Related Issue
Testing
Checklist
추후 개선 필요.
DeleteInstance실패 시 DB에 레코드가 남아 서버 재부팅 시 다시 생겨버릴 수도 있음. 코어 삭제를 idempotent하게 만들거나 할 필요가 있음.Summary by CodeRabbit
New Features
Bug Fixes