Conversation
Jbchan01
force-pushed
the
snapshot_update
branch
from
7988f72 to
bb08830
Compare
|
우선적으로 각 코드 분리를 해두었습니다. |
kwonkwonn
left a comment
kwonkwonn
left a comment
There was a problem hiding this comment.
전반적으로 구조는 아주 좋은 거 같아요.
이 방식으로 PoC 진행하고, 공통 로직을 분리하는 식으로 진행해도 좋을 것 같습니다.
services/snapshot/external.go
Outdated
|
|
||
| // waitBlockJobReady waits until a block commit job has copied all data and is | ||
| // ready for pivot. | ||
| func waitBlockJobReady(domain *libvirt.Domain, disk string, timeout time.Duration) error { |
There was a problem hiding this comment.
서비스에서 libvirt.Domain 필연적인 경우가 아니라면 interface 로 감춰주세요
services/snapshot/external.go
Outdated
| job, err := domain.GetBlockJobInfo(disk, 0) | ||
| if err != nil { | ||
| if time.Now().After(deadline) { | ||
| return fmt.Errorf("timeout waiting for block job on disk %s: %w", disk, err) |
There was a problem hiding this comment.
에러 반환 컨밴션 확인해주세요
VirError 로 합성해야 될거 같아요
services/snapshot/external.go
Outdated
| // - source: The source file path to use for the disk | ||
| // | ||
| // Returns the XML string for the disk device. | ||
| func buildDiskDeviceXML(info diskInfo, source string) string { |
There was a problem hiding this comment.
devicedefiner 에서 쓰는걸 추천하지만, 아직 완성된 패키지가 아니라서 염두만 해주세요
|
|
||
| func ListSnapshots(domain *domCon.Domain) ([]string, error) { | ||
| if domain == nil || domain.Domain == nil { | ||
| return nil, virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("nil domain")) |
There was a problem hiding this comment.
지금 이 방식대로 다른 에러에서도 적용해주세용
virtError 로 특정하기 어려운 깊이<- return error
virtError 로 특정할 수 있는 시작점 <-- ErrorGen
상위레이어로 갈때 ErrorJoin <-- 으로 생각해주시면 됩니다
| } | ||
|
|
||
| var listFlags libvirt.DomainSnapshotListFlags | ||
| snaps, err := domain.Domain.ListAllSnapshots(listFlags) |
There was a problem hiding this comment.
libvirt 패키지에서 호출하는 함수를 기반으로 interface 정의하고 받아주는게 좋을 거 같아요
e.g,
type Domain interface{
ListAllstanpshots(listFlags) (snaps, error)
}
이렇게 두고 함수 호출시점에 domcon,Domain을 넣는식으로 하면 테스트 넣기 유용해질 거 같아요.
listFlags가 libvirt 종속적이라면 alias 나 타입 변환 고려해서 서비스에서는 libvirt를 호출하지 않으려고 합니다
|
|
||
| snapXML := fmt.Sprintf(`<domainsnapshot><name>%s</name><description>%s</description></domainsnapshot>`, name, description) | ||
|
|
||
| flags := libvirt.DomainSnapshotCreateFlags(0) |
There was a problem hiding this comment.
이 부분도 libvirt 노출 최소화와 같은 맥락입니당
인터페이스로 감추는게 좋아보여요
| description = opts.Description | ||
| } | ||
|
|
||
| snapXML := fmt.Sprintf(`<domainsnapshot><name>%s</name><description>%s</description></domainsnapshot>`, name, description) |
There was a problem hiding this comment.
string 하드코딩 대신 다른 방식으로 넣는게 좋아보입니다.
pkg/virtXML에 넣어놓고 받아오는 것도 좋아보여요
| targetSource, ok := targetSources[d.TargetDev] | ||
| if !ok || targetSource == "" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
전반적으로 함수가 비대해졌는데, 역할별로 분리시키는게 좋아보여요
예를 들어 xml 수정 로직은 virtxml로 이동시켜서 수행하는 식으로
snapshot 도 만약 공통 로직이 있다면 internal/snapshot 같은 파일로 옮기는 것도 좋아보입니다
| return nil, fmt.Errorf("failed to parse domain xml: %w", err) | ||
| } | ||
|
|
||
| out := make([]diskInfo, 0, len(doc.Devices.Disks)) |
There was a problem hiding this comment.
XML 수정 로직은 MAP + libvirt_xml 로 이전시켜서 진행하는게 좋아보입니다.
추후에 xml 관련 수정이 있을때 골치 아파질것 같아요
|
리뷰 인지만 해주세요. |
|
확인했습니다. |
2 participants
Type of Change
Summary
merge 기능이 추가되었습니다. 종속성 문제가 있긴 한데, revert 기능과 종속성 문제가 다소 있어 보여서 해결해야 하긴 합니다.
Related Issue
Closes #
Test Plan
make testpasses