diff --git a/api/Snapshot/Snapshot.go b/api/Snapshot/Snapshot.go index 4f26ad0..364079d 100644 --- a/api/Snapshot/Snapshot.go +++ b/api/Snapshot/Snapshot.go @@ -76,8 +76,6 @@ func (h *Handler) CreateExternalSnapshot(w http.ResponseWriter, r *http.Request) snapName, err := externalsnapshot.CreateExternalSnapshot(dom, param.Name, &externalsnapshot.ExternalSnapshotOptions{ BaseDir: param.BaseDir, Description: param.Description, - Quiesce: param.Quiesce, - Live: param.Live, }) if err != nil { resp.ResponseWriteErr(w, err, http.StatusInternalServerError) @@ -259,6 +257,40 @@ func (h *Handler) RevertSnapshot(w http.ResponseWriter, r *http.Request) { resp.ResponseWriteOK(w, nil) } +func (h *Handler) DeleteExternalSnapshot(w http.ResponseWriter, r *http.Request) { + param := &ExternalSnapshotRequest{} + resp := httputil.ResponseGen[any]("Delete External Snapshot") + + if err := httputil.HttpDecoder(r, param); err != nil { + resp.ResponseWriteErr(w, err, http.StatusBadRequest) + h.Logger.Error("external snapshot delete decode failed", zap.Error(err)) + return + } + + if param.Name == "" { + resp.ResponseWriteErr(w, virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("snapshot name required")), http.StatusBadRequest) + return + } + + h.Logger.Info("external snapshot delete start", zap.String("domain_uuid", param.UUID), zap.String("snapshot_name", param.Name)) + + dom, err := h.DomainControl.GetDomain(param.UUID) + if err != nil { + resp.ResponseWriteErr(w, err, http.StatusInternalServerError) + h.Logger.Error("external snapshot delete failed - domain not found", zap.String("domain_uuid", param.UUID), zap.Error(err)) + return + } + + if err := externalsnapshot.DeleteExternalSnapshot(dom, param.Name); err != nil { + resp.ResponseWriteErr(w, err, http.StatusInternalServerError) + h.Logger.Error("external snapshot delete failed", zap.String("domain_uuid", param.UUID), zap.String("snapshot_name", param.Name), zap.Error(err)) + return + } + + h.Logger.Info("external snapshot delete success", zap.String("domain_uuid", param.UUID), zap.String("snapshot_name", param.Name)) + resp.ResponseWriteOK(w, nil) +} + func (h *Handler) DeleteSnapshot(w http.ResponseWriter, r *http.Request) { param := &SnapshotRequest{} resp := httputil.ResponseGen[any]("Delete Snapshot") diff --git a/cmd/doddle/doddle b/cmd/doddle/doddle new file mode 100755 index 0000000..c3c3d12 Binary files /dev/null and b/cmd/doddle/doddle differ diff --git a/internal/server/server.go b/internal/server/server.go index 34b0c10..832c193 100755 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -43,6 +43,7 @@ func InitServer(portNum int, h Handlers, logger *zap.Logger) { mux.HandleFunc("POST /RevertSnapshot", h.Snapshot.RevertSnapshot) mux.HandleFunc("POST /RevertExternalSnapshot", h.Snapshot.RevertExternalSnapshot) mux.HandleFunc("POST /MergeExternalSnapshot", h.Snapshot.MergeExternalSnapshot) + mux.HandleFunc("POST /DeleteExternalSnapshot", h.Snapshot.DeleteExternalSnapshot) mux.HandleFunc("POST /DeleteSnapshot", h.Snapshot.DeleteSnapshot) mux.HandleFunc("GET /metrics", h.Metric.DefaultMetric().ServeHTTP) diff --git a/internal/snapshotlibvirt/external_snapshot_ops.go b/internal/snapshotlibvirt/external_snapshot_ops.go index 330fbed..d447958 100644 --- a/internal/snapshotlibvirt/external_snapshot_ops.go +++ b/internal/snapshotlibvirt/external_snapshot_ops.go @@ -2,36 +2,13 @@ package snapshotlibvirt import "libvirt.org/go/libvirt" -type ExternalSnapshotCreateOptions struct { - Live bool - Quiesce bool - Atomic bool -} - -func CreateExternalSnapshot(domain *libvirt.Domain, snapshotXML string, opts ExternalSnapshotCreateOptions) (*libvirt.DomainSnapshot, error) { - flags := libvirt.DOMAIN_SNAPSHOT_CREATE_DISK_ONLY - if opts.Live { - flags |= libvirt.DOMAIN_SNAPSHOT_CREATE_LIVE - } - if opts.Quiesce { - flags |= libvirt.DOMAIN_SNAPSHOT_CREATE_QUIESCE - } - if opts.Atomic { - flags |= libvirt.DOMAIN_SNAPSHOT_CREATE_ATOMIC - } - +// RegisterExternalSnapshot registers an external snapshot using pre-existing overlay files +// created by qemu-img. REUSE_EXT tells libvirt to reuse the files rather than creating new ones. +func RegisterExternalSnapshot(domain *libvirt.Domain, snapshotXML string) (*libvirt.DomainSnapshot, error) { + flags := libvirt.DOMAIN_SNAPSHOT_CREATE_DISK_ONLY | libvirt.DOMAIN_SNAPSHOT_CREATE_REUSE_EXT return domain.CreateSnapshotXML(snapshotXML, flags) } -func StartBlockCommit(domain *libvirt.Domain, disk, base, top string) error { - flags := libvirt.DOMAIN_BLOCK_COMMIT_ACTIVE | libvirt.DOMAIN_BLOCK_COMMIT_DELETE - return domain.BlockCommit(disk, base, top, 0, flags) -} - -func AbortBlockJobPivot(domain *libvirt.Domain, disk string) error { - return domain.BlockJobAbort(disk, libvirt.DOMAIN_BLOCK_JOB_ABORT_PIVOT) -} - func UpdateDeviceConfig(domain *libvirt.Domain, deviceXML string) error { return domain.UpdateDeviceFlags(deviceXML, libvirt.DOMAIN_DEVICE_MODIFY_CONFIG) } diff --git a/internal/snapshotlibvirt/snapshot_handle.go b/internal/snapshotlibvirt/snapshot_handle.go index 769c85a..d3d9be8 100644 --- a/internal/snapshotlibvirt/snapshot_handle.go +++ b/internal/snapshotlibvirt/snapshot_handle.go @@ -6,6 +6,10 @@ import ( "libvirt.org/go/libvirt" ) +// metadataOnlyFlag tells libvirt to remove only the snapshot metadata record, +// not the external overlay files on disk. We always manage overlay files ourselves. +const metadataOnlyFlag = libvirt.DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY + type LibvirtSnapshotHandle struct { snapshot *libvirt.DomainSnapshot } @@ -25,7 +29,7 @@ func (s *LibvirtSnapshotHandle) Delete() error { if s == nil || s.snapshot == nil { return fmt.Errorf("nil snapshot") } - return s.snapshot.Delete(0) + return s.snapshot.Delete(metadataOnlyFlag) } func (s *LibvirtSnapshotHandle) Revert() error { diff --git a/services/snapshot/external_snap/create_ext.go b/services/snapshot/external_snap/create_ext.go index 6ca0b02..e39c559 100644 --- a/services/snapshot/external_snap/create_ext.go +++ b/services/snapshot/external_snap/create_ext.go @@ -15,9 +15,6 @@ func CreateExternalSnapshot(domain *domCon.Domain, name string, opts *ExternalSn return "", virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("nil domain")) } - active, err := domain.Domain.IsActive() - domainActive := err == nil && active - xmlDesc, err := domain.Domain.GetXMLDesc(0) if err != nil { return "", virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to get domain xml: %w", err)) @@ -28,10 +25,10 @@ func CreateExternalSnapshot(domain *domCon.Domain, name string, opts *ExternalSn return "", virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to resolve domain uuid: %w", err)) } - return createExternalSnapshot(newExternalSnapshotDomain(domain.Domain), domainActive, domainUUID, xmlDesc, name, opts) + return createExternalSnapshot(newExternalSnapshotDomain(domain.Domain), newQemuImg(), domainUUID, xmlDesc, name, opts) } -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 { return "", virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("nil domain")) } @@ -61,17 +58,25 @@ func createExternalSnapshot(domain SnapshotDomain, domainActive bool, domainUUID snapDisks := make([]snapshotDisk, 0, len(disks)) for _, d := range disks { + overlayPath := filepath.Join(snapshotDir, fmt.Sprintf("%s.qcow2", d.TargetDev)) + + backingFormat := d.Driver + if backingFormat == "" { + backingFormat = "qcow2" + } + 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)) + } + var driver *snapshotDriver if d.Driver != "" { driver = &snapshotDriver{Type: d.Driver} } - - snapshotFile := filepath.Join(snapshotDir, fmt.Sprintf("%s.qcow2", d.TargetDev)) snapDisks = append(snapDisks, snapshotDisk{ Name: d.TargetDev, Snapshot: "external", Driver: driver, - Source: &snapshotSource{File: snapshotFile}, + Source: &snapshotSource{File: overlayPath}, }) } @@ -91,23 +96,15 @@ func createExternalSnapshot(domain SnapshotDomain, domainActive bool, domainUUID return "", virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to build snapshot xml: %w", err)) } - live := opts != nil && opts.Live && domainActive - - createOpts := externalSnapshotCreateExecOptions{ - Live: live, - Quiesce: opts != nil && opts.Quiesce, - Atomic: len(disks) > 1, - } - - snap, err := domain.CreateExternalSnapshot(string(snapBytes), createOpts) + snap, err := domain.RegisterExternalSnapshot(string(snapBytes)) if err != nil { - return "", virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to create external snapshot: %w", err)) + return "", virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to register external snapshot: %w", err)) } defer snap.Free() snapName, err := snap.Name() if err != nil { - return "", virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("snapshot created but failed to read name: %w", err)) + return "", virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("snapshot registered but failed to read name: %w", err)) } return snapName, nil diff --git a/services/snapshot/external_snap/delete_ext.go b/services/snapshot/external_snap/delete_ext.go index f834ac8..a0b768d 100644 --- a/services/snapshot/external_snap/delete_ext.go +++ b/services/snapshot/external_snap/delete_ext.go @@ -2,6 +2,7 @@ package external import ( "fmt" + "os" domCon "github.com/easy-cloud-Knet/KWS_Core/DomCon" virerr "github.com/easy-cloud-Knet/KWS_Core/internal/error" @@ -33,14 +34,30 @@ func deleteExternalSnapshot(domain SnapshotDomain, snapName string) error { if err != nil { return err } - if target == nil { return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("snapshot %s not found", snapName)) } + // Collect overlay file paths before removing libvirt metadata. + overlayFiles, err := extractExternalSnapshotSources(target) + if err != nil { + return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to extract snapshot sources: %w", err)) + } + if err := target.Delete(); err != nil { return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to delete external snapshot %s: %w", snapName, err)) } + // Remove overlay files from disk. Libvirt does not delete external overlay + // files automatically, so we handle it here. + for _, path := range overlayFiles { + if path == "" { + continue + } + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to remove overlay file %s: %w", path, err)) + } + } + return nil } diff --git a/services/snapshot/external_snap/domain_type.go b/services/snapshot/external_snap/domain_type.go index 2a2b1e2..03640e7 100644 --- a/services/snapshot/external_snap/domain_type.go +++ b/services/snapshot/external_snap/domain_type.go @@ -8,25 +8,11 @@ import ( ) type SnapshotDomain interface { - CreateExternalSnapshot(snapshotXML string, opts externalSnapshotCreateExecOptions) (SnapshotHandle, error) + RegisterExternalSnapshot(snapshotXML string) (SnapshotHandle, error) ListAllSnapshots() ([]SnapshotHandle, error) - StartBlockCommit(disk, base, top string) error - BlockJobInfo(disk string) (externalBlockJobInfo, error) - AbortBlockJobPivot(disk string) error UpdateDeviceConfig(deviceXML string) error } -type externalBlockJobInfo struct { - Cur uint64 - End uint64 -} - -type externalSnapshotCreateExecOptions struct { - Live bool - Quiesce bool - Atomic bool -} - type SnapshotHandle interface { Name() (string, error) XMLDesc() (string, error) @@ -42,16 +28,12 @@ func newExternalSnapshotDomain(domain *libvirt.Domain) SnapshotDomain { return &libvirtExternalSnapshotDomain{domain: domain} } -func (d *libvirtExternalSnapshotDomain) CreateExternalSnapshot(snapshotXML string, opts externalSnapshotCreateExecOptions) (SnapshotHandle, error) { +func (d *libvirtExternalSnapshotDomain) RegisterExternalSnapshot(snapshotXML string) (SnapshotHandle, error) { if d == nil || d.domain == nil { return nil, fmt.Errorf("nil domain") } - snapshot, err := snapshotlibvirt.CreateExternalSnapshot(d.domain, snapshotXML, snapshotlibvirt.ExternalSnapshotCreateOptions{ - Live: opts.Live, - Quiesce: opts.Quiesce, - Atomic: opts.Atomic, - }) + snapshot, err := snapshotlibvirt.RegisterExternalSnapshot(d.domain, snapshotXML) if err != nil { return nil, err } @@ -77,35 +59,6 @@ func (d *libvirtExternalSnapshotDomain) ListAllSnapshots() ([]SnapshotHandle, er return handles, nil } -func (d *libvirtExternalSnapshotDomain) StartBlockCommit(disk, base, top string) error { - if d == nil || d.domain == nil { - return fmt.Errorf("nil domain") - } - - return snapshotlibvirt.StartBlockCommit(d.domain, disk, base, top) -} - -func (d *libvirtExternalSnapshotDomain) BlockJobInfo(disk string) (externalBlockJobInfo, error) { - if d == nil || d.domain == nil { - return externalBlockJobInfo{}, fmt.Errorf("nil domain") - } - - job, err := d.domain.GetBlockJobInfo(disk, 0) - if err != nil { - return externalBlockJobInfo{}, err - } - - return externalBlockJobInfo{Cur: job.Cur, End: job.End}, nil -} - -func (d *libvirtExternalSnapshotDomain) AbortBlockJobPivot(disk string) error { - if d == nil || d.domain == nil { - return fmt.Errorf("nil domain") - } - - return snapshotlibvirt.AbortBlockJobPivot(d.domain, disk) -} - func (d *libvirtExternalSnapshotDomain) UpdateDeviceConfig(deviceXML string) error { if d == nil || d.domain == nil { return fmt.Errorf("nil domain") diff --git a/services/snapshot/external_snap/external_snap_test.go b/services/snapshot/external_snap/external_snap_test.go index 85a9783..ea0a792 100644 --- a/services/snapshot/external_snap/external_snap_test.go +++ b/services/snapshot/external_snap/external_snap_test.go @@ -7,24 +7,22 @@ import ( virerr "github.com/easy-cloud-Knet/KWS_Core/internal/error" ) -type mockExternalSnapshotDomain struct { - createErr error - createHandle SnapshotHandle - listErr error - snapshots []SnapshotHandle - blockJobInfo externalBlockJobInfo - blockJobInfoErr error +// --- mock SnapshotDomain --- - lastCreateOpts externalSnapshotCreateExecOptions +type mockExternalSnapshotDomain struct { + registerErr error + registerHandle SnapshotHandle + listErr error + snapshots []SnapshotHandle + updateErr error } -func (m *mockExternalSnapshotDomain) CreateExternalSnapshot(_ string, opts externalSnapshotCreateExecOptions) (SnapshotHandle, error) { - m.lastCreateOpts = opts - if m.createErr != nil { - return nil, m.createErr +func (m *mockExternalSnapshotDomain) RegisterExternalSnapshot(_ string) (SnapshotHandle, error) { + if m.registerErr != nil { + return nil, m.registerErr } - if m.createHandle != nil { - return m.createHandle, nil + if m.registerHandle != nil { + return m.registerHandle, nil } return &mockExternalSnapshotHandle{name: "created-snap"}, nil } @@ -36,28 +34,12 @@ func (m *mockExternalSnapshotDomain) ListAllSnapshots() ([]SnapshotHandle, error return m.snapshots, nil } -func (m *mockExternalSnapshotDomain) StartBlockCommit(_, _, _ string) error { - return nil -} - -func (m *mockExternalSnapshotDomain) BlockJobInfo(_ string) (externalBlockJobInfo, error) { - if m.blockJobInfoErr != nil { - return externalBlockJobInfo{}, m.blockJobInfoErr - } - if m.blockJobInfo.End == 0 { - return externalBlockJobInfo{Cur: 1, End: 1}, nil - } - return m.blockJobInfo, nil -} - -func (m *mockExternalSnapshotDomain) AbortBlockJobPivot(_ string) error { - return nil -} - func (m *mockExternalSnapshotDomain) UpdateDeviceConfig(_ string) error { - return nil + return m.updateErr } +// --- mock SnapshotHandle --- + type mockExternalSnapshotHandle struct { name string nameErr error @@ -88,9 +70,46 @@ func (m *mockExternalSnapshotHandle) Free() error { return nil } +// --- mock QemuImg --- + +type mockQemuImg struct { + createErr error + infoErr error + infoFunc func(string) (string, string, error) + backing string + backingFmt string + convertErr error + commitErr error +} + +func (q *mockQemuImg) Create(_, _, _ string) error { + return q.createErr +} + +func (q *mockQemuImg) Info(path string) (string, string, error) { + if q.infoFunc != nil { + return q.infoFunc(path) + } + if q.infoErr != nil { + return "", "", q.infoErr + } + return q.backing, q.backingFmt, nil +} + +func (q *mockQemuImg) Convert(_, _ string) error { + return q.convertErr +} + +func (q *mockQemuImg) Commit(_, _ string) error { + return q.commitErr +} + +// --- Create tests --- + func TestCreateExternalSnapshot_InvalidName(t *testing.T) { domain := &mockExternalSnapshotDomain{} - _, err := createExternalSnapshot(domain, false, "test-uuid", ``, "", nil) + qimg := &mockQemuImg{} + _, err := createExternalSnapshot(domain, qimg, "test-uuid", ``, "", nil) if err == nil { t.Fatal("expected error, got nil") } @@ -99,31 +118,72 @@ func TestCreateExternalSnapshot_InvalidName(t *testing.T) { } } -func TestCreateExternalSnapshot_MapsCreateOptions(t *testing.T) { +func TestCreateExternalSnapshot_XMLDescError(t *testing.T) { + domain := &mockExternalSnapshotDomain{} + qimg := &mockQemuImg{} + _, err := createExternalSnapshot(domain, qimg, "test-uuid", " - - - ` + + ` - name, err := createExternalSnapshot(domain, true, "test-uuid", xmlDesc, "snap-a", &ExternalSnapshotOptions{BaseDir: tmp, Live: true, Quiesce: true}) + name, err := createExternalSnapshot(domain, qimg, "test-uuid", xmlDesc, "snap-a", &ExternalSnapshotOptions{BaseDir: tmp}) if err != nil { t.Fatalf("unexpected error: %v", err) } if name != "snap-a" { t.Fatalf("expected snapshot name snap-a, got %s", name) } - if !domain.lastCreateOpts.Live || !domain.lastCreateOpts.Quiesce || !domain.lastCreateOpts.Atomic { - t.Fatalf("expected live/quiesce/atomic all true, got %+v", domain.lastCreateOpts) +} + +func TestCreateExternalSnapshot_QemuImgError(t *testing.T) { + tmp := t.TempDir() + domain := &mockExternalSnapshotDomain{} + qimg := &mockQemuImg{createErr: errors.New("qemu-img failed")} + xmlDesc := ` + + ` + + _, err := createExternalSnapshot(domain, qimg, "test-uuid", xmlDesc, "snap-a", &ExternalSnapshotOptions{BaseDir: tmp}) + if err == nil { + t.Fatal("expected error, got nil") + } + if !errors.Is(err, virerr.SnapshotError) { + t.Fatalf("expected SnapshotError, got %v", err) } } -func TestCreateExternalSnapshot_XMLDescError(t *testing.T) { +// --- Revert tests --- + +func TestRevertExternalSnapshot_RequiresName(t *testing.T) { domain := &mockExternalSnapshotDomain{} - _, err := createExternalSnapshot(domain, false, "test-uuid", "`, "") + if err == nil { + t.Fatal("expected error, got nil") + } + if !errors.Is(err, virerr.InvalidParameter) { + t.Fatalf("expected InvalidParameter, got %v", err) + } +} + +func TestRevertExternalSnapshot_SnapshotNotFound(t *testing.T) { + domain := &mockExternalSnapshotDomain{snapshots: []SnapshotHandle{}} + qimg := &mockQemuImg{} + err := revertExternalSnapshot(domain, qimg, ``, "missing") if err == nil { t.Fatal("expected error, got nil") } @@ -132,6 +192,8 @@ func TestCreateExternalSnapshot_XMLDescError(t *testing.T) { } } +// --- Delete tests --- + func TestDeleteExternalSnapshot_NotFound(t *testing.T) { domain := &mockExternalSnapshotDomain{ snapshots: []SnapshotHandle{}, @@ -146,6 +208,8 @@ func TestDeleteExternalSnapshot_NotFound(t *testing.T) { } } +// --- List tests --- + func TestListExternalSnapshots_FiltersExternalOnly(t *testing.T) { domain := &mockExternalSnapshotDomain{ snapshots: []SnapshotHandle{ @@ -169,10 +233,13 @@ func TestListExternalSnapshots_FiltersExternalOnly(t *testing.T) { } } -func TestMergeExternalSnapshot_InactiveDomain(t *testing.T) { +// --- Merge tests --- + +func TestMergeExternalSnapshot_InvalidXML(t *testing.T) { domain := &mockExternalSnapshotDomain{} + qimg := &mockQemuImg{} - _, err := mergeExternalSnapshot(domain, ``, "") + // Info returns empty backing — disk is already a base, nothing to merge. + qimg := &mockQemuImg{backing: ""} + xmlDesc := ` + + ` + + _, err := mergeExternalSnapshot(domain, qimg, xmlDesc, "") if err == nil { - t.Fatal("expected error, got nil") + t.Fatal("expected error for no mergeable disks, got nil") } - if !errors.Is(err, virerr.InvalidParameter) { - t.Fatalf("expected InvalidParameter, got %v", err) + if !errors.Is(err, virerr.SnapshotError) { + t.Fatalf("expected SnapshotError, got %v", err) + } +} + +func TestMergeExternalSnapshot_NoOrigin(t *testing.T) { + domain := &mockExternalSnapshotDomain{} + // Chain: base ← /snapshots/ex-1/vda.qcow2 (overlay backed directly by base, no origin disk) + infoResults := map[string]struct{ b, f string }{ + "/vm/snapshots/ex-1/vda.qcow2": {"/vm/base.qcow2", "qcow2"}, + "/vm/base.qcow2": {"", ""}, + } + qimg := &mockQemuImg{ + infoFunc: func(path string) (string, string, error) { + r := infoResults[path] + return r.b, r.f, nil + }, + } + xmlDesc := ` + + ` + + _, err := mergeExternalSnapshot(domain, qimg, xmlDesc, "") + if err == nil { + t.Fatal("expected error for chain with no origin, got nil") + } + if !errors.Is(err, virerr.SnapshotError) { + t.Fatalf("expected SnapshotError, got %v", err) + } +} + +func TestMergeExternalSnapshot_CommitsIntoOrigin(t *testing.T) { + domain := &mockExternalSnapshotDomain{} + // Chain: base ← /vm/origin.qcow2 ← /vm/snapshots/ex-1/vda.qcow2 ← /vm/snapshots/ex-2/vda.qcow2 + infoResults := map[string]struct{ b, f string }{ + "/vm/snapshots/ex-2/vda.qcow2": {"/vm/snapshots/ex-1/vda.qcow2", "qcow2"}, + "/vm/snapshots/ex-1/vda.qcow2": {"/vm/origin.qcow2", "qcow2"}, + "/vm/origin.qcow2": {"/vm/base.qcow2", "qcow2"}, + "/vm/base.qcow2": {"", ""}, + } + qimg := &mockQemuImg{ + infoFunc: func(path string) (string, string, error) { + r := infoResults[path] + return r.b, r.f, nil + }, + } + xmlDesc := ` + + ` + + disks, err := mergeExternalSnapshot(domain, qimg, xmlDesc, "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(disks) != 1 || disks[0] != "vda" { + t.Fatalf("expected [vda], got %v", disks) } } diff --git a/services/snapshot/external_snap/helpers.go b/services/snapshot/external_snap/helpers.go index e93246b..fd9a2ea 100644 --- a/services/snapshot/external_snap/helpers.go +++ b/services/snapshot/external_snap/helpers.go @@ -1,15 +1,72 @@ package external import ( + "encoding/json" "encoding/xml" "fmt" + "os/exec" "path/filepath" "strings" - "time" virerr "github.com/easy-cloud-Knet/KWS_Core/internal/error" ) +// realQemuImg is the production implementation of QemuImg using exec.Command. +type realQemuImg struct{} + +func newQemuImg() QemuImg { + return &realQemuImg{} +} + +func (q *realQemuImg) Create(backingFile, backingFormat, overlayPath string) error { + args := []string{"create", "-f", "qcow2", "-b", backingFile} + if backingFormat != "" { + args = append(args, "-F", backingFormat) + } + args = append(args, overlayPath) + + out, err := exec.Command("qemu-img", args...).CombinedOutput() + if err != nil { + return fmt.Errorf("qemu-img create failed: %w: %s", err, strings.TrimSpace(string(out))) + } + return nil +} + +type qemuImgInfoResult struct { + BackingFilename string `json:"backing-filename"` + BackingFileFormat string `json:"backing-filename-format"` +} + +func (q *realQemuImg) Info(diskPath string) (backingFile, backingFormat string, err error) { + out, execErr := exec.Command("qemu-img", "info", "--output=json", diskPath).CombinedOutput() + if execErr != nil { + return "", "", fmt.Errorf("qemu-img info failed: %w: %s", execErr, strings.TrimSpace(string(out))) + } + + var result qemuImgInfoResult + if jsonErr := json.Unmarshal(out, &result); jsonErr != nil { + return "", "", fmt.Errorf("failed to parse qemu-img info output: %w", jsonErr) + } + + return result.BackingFilename, result.BackingFileFormat, nil +} + +func (q *realQemuImg) Convert(src, dst string) error { + out, err := exec.Command("qemu-img", "convert", "-f", "qcow2", "-O", "qcow2", src, dst).CombinedOutput() + if err != nil { + return fmt.Errorf("qemu-img convert failed: %w: %s", err, strings.TrimSpace(string(out))) + } + return nil +} + +func (q *realQemuImg) Commit(overlay, base string) error { + out, err := exec.Command("qemu-img", "commit", "-b", base, overlay).CombinedOutput() + if err != nil { + return fmt.Errorf("qemu-img commit failed: %w: %s", err, strings.TrimSpace(string(out))) + } + return nil +} + func freeSnapshotHandles(snaps []SnapshotHandle) { for _, s := range snaps { s.Free() @@ -37,33 +94,6 @@ func findExternalSnapshotByName(snaps []SnapshotHandle, snapName string) (Snapsh return nil, nil } -func waitBlockJobReady(domain SnapshotDomain, disk string, timeout time.Duration) error { - if domain == nil { - return virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("nil domain")) - } - - deadline := time.Now().Add(timeout) - for { - job, err := domain.BlockJobInfo(disk) - if err != nil { - if time.Now().After(deadline) { - return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("timeout waiting for block job on disk %s: %w", disk, err)) - } - time.Sleep(500 * time.Millisecond) - continue - } - - if job.End > 0 && job.Cur >= job.End { - return nil - } - - if time.Now().After(deadline) { - return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("timeout waiting for block commit to complete on disk %s", disk)) - } - time.Sleep(500 * time.Millisecond) - } -} - func listFileDisksFromXMLDesc(xmlDesc string) ([]diskInfo, error) { var doc domainXML @@ -193,3 +223,51 @@ func extractExternalSnapshotSources(snapshot SnapshotHandle) (map[string]string, return out, nil } + +// workingDiskPath derives the "working" overlay path from a snapshot overlay path. +// Snapshot layout: //snapshots//.qcow2 +// Working layout: //working/.qcow2 +func workingDiskPath(snapOverlay, diskName string) string { + snapNameDir := filepath.Dir(snapOverlay) + snapshotsDir := filepath.Dir(snapNameDir) + uuidDir := filepath.Dir(snapshotsDir) + return filepath.Join(uuidDir, "working", diskName+".qcow2") +} + +// isSnapshotOverlay reports whether path is one of the overlay files managed +// by this snapshot system (under snapshots/ or working/ directories). +func isSnapshotOverlay(path string) bool { + return strings.Contains(path, "/snapshots/") || strings.Contains(path, "/working/") +} + +// findOriginAndOverlays traverses the backing chain of topOverlay and returns +// the origin disk (the VM's own qcow2 that sits directly on top of the base +// image) together with the list of overlay paths above it. +// Returns an error if the chain has no origin (overlays backed directly by base). +func findOriginAndOverlays(qimg QemuImg, topOverlay string) (origin string, overlays []string, err error) { + current := topOverlay + for { + backing, _, infoErr := qimg.Info(current) + if infoErr != nil { + return "", nil, fmt.Errorf("failed to query disk info for %s: %w", current, infoErr) + } + if backing == "" { + return "", nil, fmt.Errorf("no VM origin disk found: chain root reached without an intermediate disk") + } + if !isSnapshotOverlay(backing) { + // backing is a non-overlay file — verify it is origin (has its own backing) + // rather than the base image itself (which would have no backing). + backingOfBacking, _, infoErr := qimg.Info(backing) + if infoErr != nil { + return "", nil, fmt.Errorf("failed to query disk info for %s: %w", backing, infoErr) + } + if backingOfBacking == "" { + return "", nil, fmt.Errorf("no VM origin disk found: overlays are backed directly by the base image") + } + overlays = append(overlays, current) + return backing, overlays, nil + } + overlays = append(overlays, current) + current = backing + } +} diff --git a/services/snapshot/external_snap/merge_ext.go b/services/snapshot/external_snap/merge_ext.go index 2245d5f..1382a92 100644 --- a/services/snapshot/external_snap/merge_ext.go +++ b/services/snapshot/external_snap/merge_ext.go @@ -2,7 +2,7 @@ package external import ( "fmt" - "time" + "os" domCon "github.com/easy-cloud-Knet/KWS_Core/DomCon" virerr "github.com/easy-cloud-Knet/KWS_Core/internal/error" @@ -14,11 +14,8 @@ func MergeExternalSnapshot(domain *domCon.Domain, targetDisk string) ([]string, } active, err := domain.Domain.IsActive() - if err != nil { - return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to check domain state: %w", err)) - } - if !active { - return nil, virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("external snapshot merge requires the domain to be running")) + if err == nil && active { + return nil, virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("offline merge requires the domain to be shut down")) } xmlDesc, err := domain.Domain.GetXMLDesc(0) @@ -26,10 +23,10 @@ func MergeExternalSnapshot(domain *domCon.Domain, targetDisk string) ([]string, return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to get domain xml: %w", err)) } - return mergeExternalSnapshot(newExternalSnapshotDomain(domain.Domain), xmlDesc, targetDisk) + return mergeExternalSnapshot(newExternalSnapshotDomain(domain.Domain), newQemuImg(), xmlDesc, targetDisk) } -func mergeExternalSnapshot(domain SnapshotDomain, domainXMLDesc, targetDisk string) ([]string, error) { +func mergeExternalSnapshot(domain SnapshotDomain, qimg QemuImg, domainXMLDesc, targetDisk string) ([]string, error) { if domain == nil { return nil, virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("nil domain")) } @@ -39,44 +36,103 @@ func mergeExternalSnapshot(domain SnapshotDomain, domainXMLDesc, targetDisk stri return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to list file disks: %w", err)) } - merged := make([]string, 0, len(disks)) + snaps, err := domain.ListAllSnapshots() + if err != nil { + return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to list snapshots: %w", err)) + } + defer freeSnapshotHandles(snaps) + + type mergeTarget struct { + disk diskInfo + originPath string + overlays []string + } + var targets []mergeTarget + + // Phase 1: commit each overlay chain into the VM's origin disk. + // qemu-img commit -b collapses all layers above origin + // into origin. Only origin needs a write lock; the base image is read-only. for _, d := range disks { if targetDisk != "" && d.TargetDev != targetDisk { continue } - backingSource := d.BackingSource - - if backingSource == "" { + backingFile, _, err := qimg.Info(d.Source) + if err != nil { + return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to query backing file for disk %s: %w", d.TargetDev, err)) + } + if backingFile == "" { + // Disk has no backing chain — nothing to merge. continue } - if err := domain.StartBlockCommit(d.TargetDev, backingSource, d.Source); err != nil { - return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to start block commit on disk %s: %w", d.TargetDev, err)) + originPath, overlays, err := findOriginAndOverlays(qimg, d.Source) + if err != nil { + return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to resolve disk chain for %s: %w", d.TargetDev, err)) } - if err := waitBlockJobReady(domain, d.TargetDev, 2*time.Minute); err != nil { - return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("block commit did not complete for disk %s: %w", d.TargetDev, err)) + if err := qimg.Commit(d.Source, originPath); err != nil { + return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to commit disk %s into origin: %w", d.TargetDev, err)) } - if err := domain.AbortBlockJobPivot(d.TargetDev); err != nil { - return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to pivot disk %s after commit: %w", d.TargetDev, err)) - } + targets = append(targets, mergeTarget{d, originPath, overlays}) + } - diskXML := buildDiskDeviceXML(d, backingSource) + if targetDisk != "" && len(targets) == 0 { + return nil, virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("disk %s has no backing store to merge", targetDisk)) + } + if len(targets) == 0 { + return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("no mergeable disks found")) + } + + // Phase 2: delete snapshot metadata while the domain still points to the + // overlay chain so libvirt's reachability validation passes. + deleteSnapshotMetadataLeafFirst(snaps) + + // Phase 3: update domain config to origin and remove the overlay files. + merged := make([]string, 0, len(targets)) + for _, t := range targets { + originDisk := diskInfo{ + TargetDev: t.disk.TargetDev, + TargetBus: t.disk.TargetBus, + Driver: t.disk.Driver, + DriverName: t.disk.DriverName, + } + diskXML := buildDiskDeviceXML(originDisk, t.originPath) if err := domain.UpdateDeviceConfig(diskXML); err != nil { - return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to update disk %s after merge: %w", d.TargetDev, err)) + return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to update disk %s after merge: %w", t.disk.TargetDev, err)) } - merged = append(merged, d.TargetDev) - } + for _, overlayPath := range t.overlays { + if err := os.Remove(overlayPath); err != nil && !os.IsNotExist(err) { + return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to remove overlay %s: %w", overlayPath, err)) + } + } - if targetDisk != "" && len(merged) == 0 { - return nil, virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("disk %s is not mergeable or has no external backing chain", targetDisk)) - } - if len(merged) == 0 { - return nil, virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("no mergeable external snapshot disks found")) + merged = append(merged, t.disk.TargetDev) } return merged, nil } + +// deleteSnapshotMetadataLeafFirst deletes libvirt snapshot records leaf-first so +// that parent records become deletable once all their children are gone. +// Errors are ignored per iteration; the loop stops when no further progress is made. +func deleteSnapshotMetadataLeafFirst(snaps []SnapshotHandle) { + deleted := make([]bool, len(snaps)) + for pass := 0; pass <= len(snaps); pass++ { + progress := false + for i, s := range snaps { + if deleted[i] { + continue + } + if s.Delete() == nil { + deleted[i] = true + progress = true + } + } + if !progress { + break + } + } +} diff --git a/services/snapshot/external_snap/revert_ext.go b/services/snapshot/external_snap/revert_ext.go index 727f447..0ba054a 100644 --- a/services/snapshot/external_snap/revert_ext.go +++ b/services/snapshot/external_snap/revert_ext.go @@ -2,6 +2,8 @@ package external import ( "fmt" + "os" + "path/filepath" domCon "github.com/easy-cloud-Knet/KWS_Core/DomCon" virerr "github.com/easy-cloud-Knet/KWS_Core/internal/error" @@ -22,10 +24,10 @@ func RevertExternalSnapshot(domain *domCon.Domain, snapName string) error { return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to get domain xml: %w", err)) } - return revertExternalSnapshot(newExternalSnapshotDomain(domain.Domain), xmlDesc, snapName) + return revertExternalSnapshot(newExternalSnapshotDomain(domain.Domain), newQemuImg(), xmlDesc, snapName) } -func revertExternalSnapshot(domain SnapshotDomain, domainXMLDesc, snapName string) error { +func revertExternalSnapshot(domain SnapshotDomain, qimg QemuImg, domainXMLDesc, snapName string) error { if domain == nil { return virerr.ErrorGen(virerr.InvalidParameter, fmt.Errorf("nil domain")) } @@ -43,16 +45,16 @@ func revertExternalSnapshot(domain SnapshotDomain, domainXMLDesc, snapName strin if err != nil { return err } - if target == nil { return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("snapshot %s not found", snapName)) } - targetSources, err := extractExternalSnapshotSources(target) + // snapOverlays maps diskName → overlay file path recorded in the snapshot + snapOverlays, err := extractExternalSnapshotSources(target) if err != nil { return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to extract snapshot sources: %w", err)) } - if len(targetSources) == 0 { + if len(snapOverlays) == 0 { return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("snapshot %s has no external disk sources", snapName)) } @@ -63,19 +65,47 @@ func revertExternalSnapshot(domain SnapshotDomain, domainXMLDesc, snapName strin updated := false for _, d := range disks { - targetSource, ok := targetSources[d.TargetDev] - if !ok || targetSource == "" { + snapOverlay, ok := snapOverlays[d.TargetDev] + if !ok || snapOverlay == "" { continue } - diskXML := buildDiskDeviceXML(d, targetSource) + + // Resolve the backing file of the snapshot overlay — this is the state at snapshot time. + backingFile, backingFormat, err := qimg.Info(snapOverlay) + if err != nil { + return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to query backing file for disk %s: %w", d.TargetDev, err)) + } + if backingFile == "" { + return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("snapshot overlay for disk %s has no backing file", d.TargetDev)) + } + if backingFormat == "" { + backingFormat = "qcow2" + } + + // Create a fresh writable overlay backed by the snapshot's backing file. + // Layout: //working/.qcow2 + workingPath := workingDiskPath(snapOverlay, d.TargetDev) + if err := os.MkdirAll(filepath.Dir(workingPath), 0755); err != nil { + return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to create working directory for disk %s: %w", d.TargetDev, err)) + } + + // Remove stale working overlay from a previous revert if present. + _ = os.Remove(workingPath) + + if err := qimg.Create(backingFile, backingFormat, workingPath); err != nil { + return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to create working overlay for disk %s: %w", d.TargetDev, err)) + } + + diskXML := buildDiskDeviceXML(d, workingPath) if err := domain.UpdateDeviceConfig(diskXML); err != nil { return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("failed to update disk %s: %w", d.TargetDev, err)) } + updated = true } if !updated { - return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("no backingStore entries found to restore")) + return virerr.ErrorGen(virerr.SnapshotError, fmt.Errorf("no matching disks found to revert")) } return nil diff --git a/services/snapshot/external_snap/type.go b/services/snapshot/external_snap/type.go index ba2ae30..b69541d 100644 --- a/services/snapshot/external_snap/type.go +++ b/services/snapshot/external_snap/type.go @@ -2,12 +2,22 @@ package external import "encoding/xml" -// ExternalSnapshotOptions defines options for creating external snapshots. type ExternalSnapshotOptions struct { BaseDir string Description string - Quiesce bool - Live bool +} + +// QemuImg abstracts qemu-img command execution for testability. +type QemuImg interface { + Create(backingFile, backingFormat, overlayPath string) error + Info(diskPath string) (backingFile, backingFormat string, err error) + // Convert flattens the full backing chain of src into a new standalone dst file. + // Unlike Commit, it never writes to any backing file so shared base images stay untouched. + Convert(src, dst string) error + // Commit writes all changes recorded in overlay into base, collapsing every + // intermediate layer between them. Only base needs a write lock; files above + // base in the chain are read-only during the operation. + Commit(overlay, base string) error } type domainXML struct {