refactor external snapshot using qemu-img#150
Conversation
|
공유 스냅샷 기능도 qemu-img convert를 이용하면 되지 않을까 생각하고 있습니다. |
kwonkwonn
left a comment
kwonkwonn
left a comment
There was a problem hiding this comment.
수고하셨습니당.
리뷰한번 확인해주세용, 아무래도 시스템에 영향이 큰 작업이라 (골든 이미지도 건들 수 있는) 좀 더 꼼꼼하게 리뷰했습니다.
|
|
||
| // realQemuImg is the production implementation of QemuImg using exec.Command. | ||
| type realQemuImg struct{} | ||
|
|
There was a problem hiding this comment.
현재 External_snap 이 qimg 를 가져와서 사용하는 형태이고, qimg 는 시스템 os 를 직접적으로 건드는 부분이 있어,
internal 로 빼서 사용하는 것도 괜찮아보여용.
하나의 거대한 서비스에 모든 관련 파일이 구현되어있으면 수정이나 파악때 어려울것 같습니다
| snapNameDir := filepath.Dir(snapOverlay) | ||
| snapshotsDir := filepath.Dir(snapNameDir) | ||
| uuidDir := filepath.Dir(snapshotsDir) | ||
| return filepath.Join(uuidDir, "working", diskName+".qcow2") |
|
|
||
| func createExternalSnapshot(domain SnapshotDomain, domainActive bool, domainUUID, xmlDesc, name string, opts *ExternalSnapshotOptions) (string, error) { | ||
| func createExternalSnapshot(domain SnapshotDomain, qimg QemuImg, domainUUID, xmlDesc, name string, opts *ExternalSnapshotOptions) (string, error) { | ||
| if domain == nil { |
There was a problem hiding this comment.
CreateExternalSnapshot 에서 데이터를 검증하는 부분에 중복이 있는 것 같습니다.
이름,도메인 검증등
또한 지금은 Create(외부) 와 create(내부)가 강하게 엮여있어서 함수를 분리하는 것이 괜찮을까 생각이듭니다 :)
검증(외부) -> 생성(내부) 처럼 역할을 분리시키면 유지하기 쉬워질거 같아요
| if err := qimg.Create(d.Source, backingFormat, overlayPath); err != nil { | ||
| return "", virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to create overlay for disk %s: %w", d.TargetDev, err)) | ||
| } | ||
|
|
There was a problem hiding this comment.
이 서비스 함수 내부에서 데이터 검증, 파싱, 디렉토리 생성, qemu 이미지 생성, xml 생성 및 도메인 업데이트의 모든 로직이 많은 경우 직접호출로 진행되고 있는 것 같습니다.
실패시 롤백이나, 함수 수정, 테스트 작성이 쉽지 않을 것 같아요.
흐름에는 문제가 없는 것 같은데 역할별로 분리 시킬 수 있으면 더 좋을것 같습니다
| } | ||
|
|
||
| // Remove stale working overlay from a previous revert if present. | ||
| _ = os.Remove(workingPath) |
There was a problem hiding this comment.
로직이 잘 이해가 가지 않는데, workingPath 생성 -> 디렉토리 셍성 -> 삭제 하는 이유가 무엇일까요
There was a problem hiding this comment.
또한 서비스에서 dir 삭제 및 에러 버리는 것은 위험할 것 같습니다
2 participants
Type of Change
Summary
외부 스냅샷 기능에 대한 전반적인 변경사항이 적용되었습니다.
기존: XML 수정방식 -> 변경: qemu-img로 관리.
EX)
before) base<- origin(vm) <- (snap-1) <- (snap-2)
Merge 호출시
after) base<- origin(vm-snap-1,2 변경사항이 적용됨).
으로 동작확인하였습니다.
Related Issue
Closes #
Test Plan
make testpasses