From 28126b526dcd0e9f83f66f9ae23b57edfaa3455b Mon Sep 17 00:00:00 2001 From: overseer Date: Sat, 8 Aug 2026 17:07:07 +0200 Subject: [PATCH] overseer: exec iteration 1 --- internal/grpcapi/snapshot.go | 3 +- internal/grpcapi/snapshot_test.go | 100 ++++++++++++++++++++++++++++++ internal/libvirt/checkpoint.go | 23 +------ internal/libvirt/errors.go | 35 +++++++++++ internal/libvirt/errors_test.go | 78 +++++++++++++++++++++++ 5 files changed, 215 insertions(+), 24 deletions(-) create mode 100644 internal/libvirt/errors.go create mode 100644 internal/libvirt/errors_test.go diff --git a/internal/grpcapi/snapshot.go b/internal/grpcapi/snapshot.go index 394a7539..731d4709 100644 --- a/internal/grpcapi/snapshot.go +++ b/internal/grpcapi/snapshot.go @@ -6,7 +6,6 @@ import ( "log/slog" "os" "path/filepath" - "strings" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -392,7 +391,7 @@ func (s *Server) DeleteSnapshot(ctx context.Context, req *pb.DeleteSnapshotReque // here — that must NOT block cleanup of the corrosion record + vmstate file // (otherwise they leak). Treat a missing libvirt snapshot as already-deleted. if delErr != nil { - if strings.Contains(delErr.Error(), "not found") || strings.Contains(delErr.Error(), "no domain snapshot") { + if lv.IsNotFound(delErr) { slog.Info("delete snapshot: libvirt metadata already gone — cleaning up record", "vm", req.VmName, "snap", req.SnapshotName) } else { return nil, status.Errorf(codes.Internal, "delete snapshot: %v", delErr) diff --git a/internal/grpcapi/snapshot_test.go b/internal/grpcapi/snapshot_test.go index 77c1477d..3ff1b65d 100644 --- a/internal/grpcapi/snapshot_test.go +++ b/internal/grpcapi/snapshot_test.go @@ -1,13 +1,18 @@ package grpcapi import ( + "context" + "errors" + "fmt" "testing" + golibvirt "github.com/digitalocean/go-libvirt" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" pb "github.com/litevirt/litevirt/gen/litevirt/v1" "github.com/litevirt/litevirt/internal/corrosion" + "github.com/litevirt/litevirt/internal/libvirtfake" ) func TestCreateSnapshot_VMNotFound(t *testing.T) { @@ -177,3 +182,98 @@ func TestDeleteSnapshot_WrongHost(t *testing.T) { t.Errorf("code = %v, want Unavailable or FailedPrecondition", c) } } + +// deleteErrVirt is a libvirtfake whose DeleteSnapshot returns a chosen error, so the +// "libvirt metadata already gone" classification can be driven directly. +type deleteErrVirt struct { + *libvirtfake.Fake + delErr error +} + +func (v *deleteErrVirt) DeleteSnapshot(string, string) error { return v.delErr } + +// deleteSnapshotServer stages a stopped local VM with one snapshot record and a +// backend whose DeleteSnapshot fails with delErr. Stopped means flatten is false at +// snapshot.go's `flatten` decision, so only DeleteSnapshot runs — one path into the +// classifier, no flatten fallback to reason about. +func deleteSnapshotServer(t *testing.T, delErr error) (*Server, context.Context) { + t.Helper() + s := testServer(t) + s.dataDir = t.TempDir() + s.virt = &deleteErrVirt{Fake: libvirtfake.New(), delErr: delErr} + ctx := adminCtx() + insertTestVM(t, ctx, s.db, "dvm", s.hostName, "stopped") + if err := corrosion.InsertSnapshot(ctx, s.db, corrosion.SnapshotRecord{ + VMName: "dvm", HostName: s.hostName, Name: "snap1", State: "ok", Type: "disk", + }); err != nil { + t.Fatalf("InsertSnapshot: %v", err) + } + return s, ctx +} + +// A snapshot that vanishes between libvirt's lookup and its delete (a revert racing a +// delete) surfaces a RAW golibvirt.Error — code ErrNoDomainSnapshot, arbitrary message. +// The old substring check called that a hard failure and returned Internal, leaking the +// corrosion record; the typed arm of lv.IsNotFound is what fixes it. "gone" contains +// neither "not found" nor "no domain snapshot", so this test fails without that arm. +func TestDeleteSnapshot_TypedNotFoundTreatedAsAlreadyGone(t *testing.T) { + s, ctx := deleteSnapshotServer(t, golibvirt.Error{ + Code: uint32(golibvirt.ErrNoDomainSnapshot), Message: "gone", + }) + + if _, err := s.DeleteSnapshot(ctx, &pb.DeleteSnapshotRequest{ + VmName: "dvm", SnapshotName: "snap1", + }); err != nil { + t.Fatalf("DeleteSnapshot: want success for a typed not-found libvirt error, got %v", err) + } + snap, _ := corrosion.GetSnapshot(ctx, s.db, "dvm", "snap1") + if snap != nil { + t.Errorf("corrosion record must be tombstoned, still present: %+v", snap) + } +} + +// The two message shapes the hand-rolled check covered before the helper existed: +// libvirt's own wording, and litevirt's `snapshot %q not found: %w` wrapper. These +// passed before the change too — they are the no-regression guard. +func TestDeleteSnapshot_MessageNotFoundTreatedAsAlreadyGone(t *testing.T) { + cases := []struct { + name string + delErr error + }{ + {"libvirt wording", errors.New("no domain snapshot with matching name 'snap1'")}, + {"litevirt wrapper", fmt.Errorf("snapshot %q not found: %w", "snap1", errors.New("boom"))}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s, ctx := deleteSnapshotServer(t, tc.delErr) + + if _, err := s.DeleteSnapshot(ctx, &pb.DeleteSnapshotRequest{ + VmName: "dvm", SnapshotName: "snap1", + }); err != nil { + t.Fatalf("DeleteSnapshot: want success for %v, got %v", tc.delErr, err) + } + snap, _ := corrosion.GetSnapshot(ctx, s.db, "dvm", "snap1") + if snap != nil { + t.Errorf("corrosion record must be tombstoned, still present: %+v", snap) + } + }) + } +} + +// A libvirt failure that is NOT an absence must still fail the RPC and LEAVE the record +// in place. Both assertions matter: the code pins that a real fault is not swallowed, and +// the surviving record pins that the handler did not run the cleanup path anyway. +func TestDeleteSnapshot_RealFailureStillPropagates(t *testing.T) { + s, ctx := deleteSnapshotServer(t, errors.New("internal error: qemu unexpectedly closed the monitor")) + + _, err := s.DeleteSnapshot(ctx, &pb.DeleteSnapshotRequest{ + VmName: "dvm", SnapshotName: "snap1", + }) + if c := status.Code(err); c != codes.Internal { + t.Fatalf("code = %v (err=%v), want Internal", c, err) + } + snap, _ := corrosion.GetSnapshot(ctx, s.db, "dvm", "snap1") + if snap == nil { + t.Error("corrosion record must survive a real libvirt failure, it was tombstoned") + } +} diff --git a/internal/libvirt/checkpoint.go b/internal/libvirt/checkpoint.go index 9995acbb..779d0921 100644 --- a/internal/libvirt/checkpoint.go +++ b/internal/libvirt/checkpoint.go @@ -13,7 +13,6 @@ package libvirt import ( "encoding/xml" "fmt" - "strings" golibvirt "github.com/digitalocean/go-libvirt" ) @@ -88,7 +87,7 @@ func (c *Client) DeleteCheckpoint(domain, checkpointName string, withChildren bo } cp, err := c.virt.DomainCheckpointLookupByName(dom, checkpointName, 0) if err != nil { - if isNotFound(err) { + if IsNotFound(err) { return nil // already gone — idempotent } return fmt.Errorf("lookup checkpoint %s: %w", checkpointName, err) @@ -156,23 +155,3 @@ func buildBackupXML(diskTarget, parentCheckpoint, socket, scratch string) (strin } return string(b), nil } - -// isNotFound classifies a libvirt error as "object does not exist" so -// existence probes don't conflate a missing checkpoint with a real fault. -func isNotFound(err error) bool { - if err == nil { - return false - } - if e, ok := err.(golibvirt.Error); ok { - switch e.Code { - case uint32(golibvirt.ErrNoDomainCheckpoint), - uint32(golibvirt.ErrNoDomain), - uint32(golibvirt.ErrNoDomainSnapshot): - return true - } - } - msg := strings.ToLower(err.Error()) - return strings.Contains(msg, "not found") || - strings.Contains(msg, "no domain checkpoint") || - strings.Contains(msg, "cannot find") -} diff --git a/internal/libvirt/errors.go b/internal/libvirt/errors.go new file mode 100644 index 00000000..a91596ea --- /dev/null +++ b/internal/libvirt/errors.go @@ -0,0 +1,35 @@ +package libvirt + +import ( + "strings" + + golibvirt "github.com/digitalocean/go-libvirt" +) + +// IsNotFound classifies a libvirt error as "the object does not exist", so callers +// can treat a delete/lookup of an already-gone domain, snapshot, or checkpoint as +// success instead of conflating it with a real fault. +// +// Typed go-libvirt error codes are authoritative and checked first. The substring +// fallback exists because litevirt wraps several libvirt calls with its own text +// (e.g. snapshot.go's `snapshot %q not found: %w`), and because some paths surface +// a message-only error with no code attached — a typed-only check would regress +// those callers. +func IsNotFound(err error) bool { + if err == nil { + return false + } + if e, ok := err.(golibvirt.Error); ok { + switch e.Code { + case uint32(golibvirt.ErrNoDomainCheckpoint), + uint32(golibvirt.ErrNoDomain), + uint32(golibvirt.ErrNoDomainSnapshot): + return true + } + } + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "not found") || + strings.Contains(msg, "no domain checkpoint") || + strings.Contains(msg, "no domain snapshot") || + strings.Contains(msg, "cannot find") +} diff --git a/internal/libvirt/errors_test.go b/internal/libvirt/errors_test.go new file mode 100644 index 00000000..88f34a19 --- /dev/null +++ b/internal/libvirt/errors_test.go @@ -0,0 +1,78 @@ +package libvirt + +import ( + "errors" + "fmt" + "testing" + + golibvirt "github.com/digitalocean/go-libvirt" +) + +// Each row exercises exactly ONE arm of IsNotFound: the typed rows carry an +// "opaque" message that matches no substring, and the message rows carry no +// libvirt error code. A row that satisfied both would not tell us which arm +// answered, so it could not catch that arm being deleted. +func TestIsNotFound(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + { + "typed ErrNoDomain", + golibvirt.Error{Code: uint32(golibvirt.ErrNoDomain), Message: "opaque"}, + true, + }, + { + "typed ErrNoDomainSnapshot", + golibvirt.Error{Code: uint32(golibvirt.ErrNoDomainSnapshot), Message: "opaque"}, + true, + }, + { + "typed ErrNoDomainCheckpoint", + golibvirt.Error{Code: uint32(golibvirt.ErrNoDomainCheckpoint), Message: "opaque"}, + true, + }, + { + // The typed switch is a whitelist of absence codes, not "any typed error". + "typed ErrInternalError", + golibvirt.Error{Code: uint32(golibvirt.ErrInternalError), Message: "opaque"}, + false, + }, + { + // libvirt's real wording for a vanished snapshot, and the string + // grpcapi's DeleteSnapshot matched by hand before this helper existed. + "message no domain snapshot", + errors.New("no domain snapshot with matching name 'snap1'"), + true, + }, + { + // litevirt's own wrapper text (snapshot.go's DeleteSnapshot lookup). + "message litevirt snapshot wrapper", + fmt.Errorf("snapshot %q not found: %w", "snap1", errors.New("boom")), + true, + }, + {"message mixed case", errors.New("Domain Not Found"), true}, + { + "message unrelated failure", + errors.New("internal error: qemu unexpectedly closed the monitor"), + false, + }, + { + // Write-lock contention is a RETRY predicate, a different question from + // "does this object exist?" — snapshot.go keeps its own heuristics for it. + // This row exists so a future refactor cannot quietly merge the two. + "message write lock", + errors.New("Failed to get write lock"), + false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsNotFound(tt.err); got != tt.want { + t.Errorf("IsNotFound(%v) = %v, want %v", tt.err, got, tt.want) + } + }) + } +}