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.
📝 WalkthroughWalkthroughThis PR adds a ChangesvCPU status and resource manager rework
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Service as CreateVM/DeleteVM
participant ResourceManager
participant Core
Service->>ResourceManager: reserveCoreOrFail (ReserveCore)
ResourceManager->>Core: score cores, decrement Free*
ResourceManager-->>Service: reserved core
alt failure during setup
Service->>ResourceManager: cleanup.run() triggers deallocation
else success
Service->>ResourceManager: AttachVMInfo(core, uuid, vm)
end
Service->>ResourceManager: ReleaseVM(core, uuid) on delete
ResourceManager->>Core: restore Free*, clear VM metadata
ResourceManager-->>Service: released
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
| // 예약 직후 롤백 등록. 이후 모든 실패 경로는 cleanup.run()으로 예약을 되돌려야 함 | ||
| cleanup := &cleanupChain{} | ||
| cleanup.push(func() { | ||
| contextStruct.Resources.DeallocateResources(selectedCore, uuid, hwReq) | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don’t use resource-only rollback after the core VM may already exist.
This cleanup later runs on failures after coreClient.CreateVM succeeds. DeallocateResources restores scheduler capacity, but it does not delete the VM from the core or remove Redis state, so a DB failure can leave a running VM while its resources are made allocatable again. Add a post-core compensating delete and only release Free* after that delete succeeds, or split pre-core and post-core rollback paths.
🤖 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 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.
There was a problem hiding this comment.
일리는 있는데, 지금 모델은 컨트롤 코어가 상태를 지속적으로 비교하는게 아니라 롤백 과정이 과하게 복잡해질 것 같네요.
지금은 코어 자체 로직이나 가비지 컬랙션을 믿거나 삭제 요청하는 로직을 롤백에 넣어야 될 거 같아요
There was a problem hiding this comment.
@kwonkwonn 말씀하신 방향에 동의합니다. 매번 실제 코어 상태를 재조회해서 비교하는 방식은 지금 구조에 비해 과합니다.
절충안으로, 완전한 상태 동기화 대신 최소한의 보정 조치만 추가하는 방법도 고려해볼 수 있습니다: coreClient.CreateVM 성공 이후 실패 경로(Redis 저장 실패는 이미 무시하니 해당 없음, DB persist 실패 등)에서만 cleanup 체인에 coreClient.DeleteVM 호출을 추가하고, 그 뒤에 DeallocateResources를 실행하는 정도입니다. 이렇게 하면 코어에 남은 VM을 정리 요청은 보내되, 삭제 성공/실패 여부와 무관하게 자원 반환은 지금처럼 단순하게 유지할 수 있습니다. 삭제 요청이 실패하더라도 코어 측 GC나 운영자의 후속 정리에 맡기는 것은 그대로 유지하시면 될 것 같습니다.
이 정도로도 과하다고 판단되시면 지금 구조를 그대로 유지하시고, 원하시면 후속 개선 항목으로 이슈를 만들어드릴 수도 있습니다. 필요하시면 말씀해주세요.
✏️ Learnings added
Learnt from: kwonkwonn
Repo: easy-cloud-Knet/KWS_Control PR: 72
File: service/vm.go:35-39
Timestamp: 2026-07-09T12:44:59.233Z
Learning: In `service/vm.go`'s `CreateVM` function (repository easy-cloud-Knet/KWS_Control), the maintainer (kwonkwonn) intentionally keeps rollback logic simple: the control core does not continuously reconcile VM state against cores, so post-core-creation failure rollback should rely on core-side logic/garbage collection or a simple delete-request call, rather than a full compensating delete + Redis state cleanup + resource release ordering. Avoid suggesting complex multi-step reconciliation-based rollback for this codebase's resource manager / cleanup chain pattern.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| 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.
눈에 띄는 에러가 안보이기도 했고, 기존 방식보다는 훨씬 나아 보입니다.
테스트 후 큰 문제가 없으면 머지해도 될 거 같아요
| // 예약 직후 롤백 등록. 이후 모든 실패 경로는 cleanup.run()으로 예약을 되돌려야 함 | ||
| cleanup := &cleanupChain{} | ||
| cleanup.push(func() { | ||
| contextStruct.Resources.DeallocateResources(selectedCore, uuid, hwReq) | ||
| }) |
There was a problem hiding this comment.
일리는 있는데, 지금 모델은 컨트롤 코어가 상태를 지속적으로 비교하는게 아니라 롤백 과정이 과하게 복잡해질 것 같네요.
지금은 코어 자체 로직이나 가비지 컬랙션을 믿거나 삭제 요청하는 로직을 롤백에 넣어야 될 거 같아요
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