feat: wire snapshot TakeExternalSnapshot to Core endpoint#70
Conversation
- Fix TakeSnapshotResponse to match Core response shape (uuid, snapKey) - Add CoreClient.TakeExternalSnapshot with 12m timeout for snapshot+upload - Fill in TakeSnapshot TODO: presigned PUT URL -> Core -> RustFS upload
📝 WalkthroughWalkthrough
ChangesExternal Snapshot Core Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@client/vm.go`:
- Around line 189-195: The TakeExternalSnapshot path is sending
model.TakeSnapshotRequest into doRequest without redacting PresignedURL, which
will leak the presigned upload URL in the request-body logs. Update
CoreClient.TakeExternalSnapshot to sanitize or copy-and-clear the PresignedURL
before calling doRequest, or bypass the body logging in doRequest for this
request, so the JSON logged from the TakeExternalSnapshot flow no longer
contains the credential.
In `@service/external_snap.go`:
- Around line 56-63: The Core snapshot request is being started with a detached
background context, so cancellation from the caller cannot stop the work. Update
TakeSnapshot to accept and pass through the caller’s context, then thread that
context from its callers instead of using context.Background(); use the existing
TakeSnapshot and TakeExternalSnapshot call sites to locate the change and keep
the error handling unchanged.
🪄 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: b1100777-10ee-4e85-9f90-f7eab3d365cd
📒 Files selected for processing (3)
client/model/snapshot.goclient/vm.goservice/external_snap.go
| func (c *CoreClient) TakeExternalSnapshot(ctx context.Context, req model.TakeSnapshotRequest) (model.TakeSnapshotResponse, error) { | ||
| snapClient := &CoreClient{ | ||
| baseURL: c.baseURL, | ||
| client: &http.Client{Timeout: 12 * time.Minute}, | ||
| } | ||
| var response model.TakeSnapshotResponse | ||
| if err := snapClient.doRequest(ctx, http.MethodPost, "/TakeExternalSnapshot", req, &response); err != nil { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact the presigned URL before this request goes through doRequest.
model.TakeSnapshotRequest includes PresignedURL, and doRequest logs the full JSON body at client/vm.go Line 41. This new path would write a live upload credential into logs for every snapshot request unless that body logging is skipped or redacted first.
🤖 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 `@client/vm.go` around lines 189 - 195, The TakeExternalSnapshot path is
sending model.TakeSnapshotRequest into doRequest without redacting PresignedURL,
which will leak the presigned upload URL in the request-body logs. Update
CoreClient.TakeExternalSnapshot to sanitize or copy-and-clear the PresignedURL
before calling doRequest, or bypass the body logging in doRequest for this
request, so the JSON logged from the TakeExternalSnapshot flow no longer
contains the credential.
There was a problem hiding this comment.
12분 정도의 긴 Timeout 방식보다 webhook 방식이 연결 자원 점유 문제나 디버깅 상태 확인 그리고 안정성에서는 더 나은 해결책이 될 수 있을 것 같습니다.
There was a problem hiding this comment.
@ukleon123 맞습니다. 장기적으로는 webhook/async 방식이 말씀하신 것처럼 연결 자원 점유, 진행 상태 확인, 재시도/복구 측면에서 더 나은 방향으로 보입니다.
다만 이번 코멘트의 핵심은 timeout 전략과는 별개로, 현재 구현에서는 model.TakeSnapshotRequest.PresignedURL 이 client/vm.go Line 41의 request-body 로그에 그대로 남는 점입니다. 그래서 지금 구조를 유지하더라도 이 값은 마스킹하거나 로그에서 제외하는 처리가 필요합니다.
즉:
- 중장기 개선안: webhook/비동기 전환
- 이번 PR의 최소 안전 조치:
TakeExternalSnapshot경로에서PresignedURL로그 유출 방지
원하시면 webhook 전환은 후속 이슈로 분리하고, 이 PR에서는 먼저 presigned URL 로깅만 막는 방향으로 보는 게 좋겠습니다.
| coreClient := client.NewCoreClient(core) | ||
| if _, err := coreClient.TakeExternalSnapshot(context.Background(), model.TakeSnapshotRequest{ | ||
| UUID: uuid, | ||
| SnapKey: snapName, | ||
| PresignedURL: presignedURL, | ||
| }); err != nil { | ||
| return fmt.Errorf("TakeSnapshot %s: core failed: %w", uuid, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Propagate the caller context into the Core snapshot call.
This new 12-minute request is issued with context.Background(), so cancellation from POST /vm/snapshot will not abort the Core work. The same detachment already exists at service/external_snap.go Line 51, so the fix belongs at TakeSnapshot's signature and callers rather than only at this line.
🤖 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/external_snap.go` around lines 56 - 63, The Core snapshot request is
being started with a detached background context, so cancellation from the
caller cannot stop the work. Update TakeSnapshot to accept and pass through the
caller’s context, then thread that context from its callers instead of using
context.Background(); use the existing TakeSnapshot and TakeExternalSnapshot
call sites to locate the change and keep the error handling unchanged.
There was a problem hiding this comment.
code rabbit이 말해준 영역이긴 하지만 Timeout 설정을 Background context에서 수행하는 방식보다는 timeout context를 사용하는 것이 나을것으로 보이긴 합니다.
There was a problem hiding this comment.
@ukleon123 맞습니다. 핵심은 Background()로 분리하지 않는 것이고, timeout이 필요하다면 말씀하신 것처럼 caller context를 그대로 전파한 뒤 그 위에 context.WithTimeout(...)을 씌우는 방식이 더 자연스럽습니다.
즉, 이상적인 형태는:
TakeSnapshot이 호출자의ctx를 받음ctxWithTimeout, cancel := context.WithTimeout(ctx, 12*time.Minute)처럼 파생- 그
ctxWithTimeout을TakeExternalSnapshot까지 전달
이렇게 하면:
- 상위 요청 취소가 그대로 전파되고
- snapshot 작업의 최대 시간도 명시적으로 제한할 수 있습니다
반대로 context.Background()에서 시작하면 상위 요청과 분리되어 취소 전파가 끊기는 점이 문제라, 그 부분은 그대로 유효한 우려였습니다.
현재 http.Client.Timeout만으로도 상한은 걸리지만, 취소 전파 관점에서는 timeout context 쪽이 더 낫다는 의견에 동의합니다.
(•ᴗ•)◞🐇
✏️ Learnings added
Learnt from: ukleon123
Repo: easy-cloud-Knet/KWS_Control PR: 70
File: service/external_snap.go:56-63
Timestamp: 2026-06-29T07:14:15.915Z
Learning: In the KWS_Control repository, for long-running operations such as external snapshot requests, prefer deriving `context.WithTimeout(...)` from the caller-provided context instead of starting from `context.Background()`, so cancellation from the upstream request is preserved while still enforcing a maximum duration.
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.
2 participants
#WIP
Summary
TakeSnapshotResponse를 Core 응답 스펙(uuid,snapKey)에 맞게 수정CoreClient.TakeExternalSnapshot추가 —POST /TakeExternalSnapshot호출, 스냅샷 생성+업로드 소요 시간 고려해 12분 timeout 전용 클라이언트 사용service/external_snap.go:TakeSnapshotTODO 제거 — presigned PUT URL 발급 후 Core에 실제 전달하는 흐름 완성Paired PR
이 PR은 Core 측 구현과 페어로 동작합니다.
Core PR #152: easy-cloud-Knet/KWS_Core#152
현재 코어에 해당 엔드포인트가 개발이 된 상태입니다.
두 Pr 을 기준으로 Control ↔ Core end-to-end 테스트를 진행하고, 통과 시 본 PR을 머지합니다.
현재 컨트롤의 서비스 로직은 단순히 엔드포인트가 돌아걸 정도의 최소 구현입니다 .
추후에 db 조작등+ 인증, 롤백등 구현을 통해 서비스 로직이 추가될거에요
Flow
WIP / Merge Condition
POST /vm/snapshot→ RustFS 버킷 확인)Summary by CodeRabbit
New Features
Bug Fixes