Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions benchmarking/locust/common/ateapi_pb2_grpc.py
Original file line number Diff line number Diff line change
Expand Up @@ -357,9 +357,9 @@ def UpdateTag(self, request, context):
raise NotImplementedError('Method not implemented!')

def DeleteTag(self, request, context):
"""Delete a Tag and the external snapshot it owns. Actors created from the
tag that have not yet been suspended still point at that external snapshot
and become unrecoverable, so do not delete a tag while such Actors exist.
"""Delete a Tag and the external snapshot it owns. Rejects
(FailedPrecondition) while an Actor created from the tag is still
borrowing that snapshot, which it does until its own first suspend.
"""
context.set_code(grpc.StatusCode.UNIMPLEMENTED)
context.set_details('Method not implemented!')
Expand Down
20 changes: 15 additions & 5 deletions cmd/ateapi/internal/controlapi/functionaltest/actor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func TestCreateActor_Success(t *testing.T) {
Status: &ateapipb.ActorStatus{
State: ateapipb.ActorState_ACTOR_STATE_SUSPENDED,
CurrentActorTemplateUid: tmpl.GetMetadata().GetUid(),
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t, tc, tmpl), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
},
WorkerSelector: &ateapipb.Selector{MatchLabels: map[string]string{"tier": "free"}},
}
Expand Down Expand Up @@ -694,7 +694,7 @@ func TestUpdateActor_Success(t *testing.T) {
Status: &ateapipb.ActorStatus{
State: ateapipb.ActorState_ACTOR_STATE_SUSPENDED,
CurrentActorTemplateUid: tmpl.GetMetadata().GetUid(),
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t, tc, tmpl), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
},
WorkerSelector: &ateapipb.Selector{
MatchLabels: map[string]string{"tier": "paid"},
Expand Down Expand Up @@ -841,7 +841,7 @@ func TestUpdateActor(t *testing.T) {
Status: &ateapipb.ActorStatus{
State: ateapipb.ActorState_ACTOR_STATE_SUSPENDED,
CurrentActorTemplateUid: tmpl.GetMetadata().GetUid(),
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t, tc, tmpl), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
},
WorkerSelector: &ateapipb.Selector{
MatchLabels: map[string]string{"tier": "paid"},
Expand Down Expand Up @@ -1812,7 +1812,7 @@ func TestResumeActor(t *testing.T) {
Status: &ateapipb.ActorStatus{
State: ateapipb.ActorState_ACTOR_STATE_RUNNING,
CurrentActorTemplateUid: tmpl.GetMetadata().GetUid(),
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t, tc, tmpl), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
WorkerAssignment: &ateapipb.WorkerAssignment{
Worker: &ateapipb.ObjectRef{Name: podUID},
WorkerNamespace: ns,
Expand Down Expand Up @@ -2357,6 +2357,16 @@ func TestSuspendActor(t *testing.T) {
assertSnapshotCollected(t, tc, snapshotURI)
assertSnapshotPresent(t, tc, tagSnapshotURI)

// The cross-atespace clone never suspended, so the tag's snapshot is still
// its starting state. A borrow holds the tag back from whichever atespace it
// was taken out in.
if _, err := tc.client.DeleteTag(context.Background(), &ateapipb.DeleteTagRequest{Tag: tagRef}); status.Code(err) != codes.FailedPrecondition {
t.Fatalf("DeleteTag while other/cross-atespace borrows it = %v, want FailedPrecondition", err)
}
if _, err := tc.client.DeleteActor(context.Background(), &ateapipb.DeleteActorRequest{Actor: &ateapipb.ObjectRef{Atespace: "other", Name: "cross-atespace"}}); err != nil {
t.Fatalf("DeleteActor(other/cross-atespace) failed: %v", err)
}

if deleted, err := tc.client.DeleteTag(context.Background(), &ateapipb.DeleteTagRequest{Tag: tagRef}); err != nil || deleted.GetMetadata().GetName() != tagRef.GetName() {
t.Fatalf("DeleteTag = (%v, %v)", deleted, err)
}
Expand Down Expand Up @@ -2557,7 +2567,7 @@ func TestPauseActor(t *testing.T) {
ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL,
},
CurrentActorTemplateUid: tmpl.GetMetadata().GetUid(),
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
ExternalSnapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t, tc, tmpl), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
},
}

Expand Down
32 changes: 25 additions & 7 deletions cmd/ateapi/internal/controlapi/functionaltest/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -341,15 +341,16 @@ func assertSnapshotCollected(t *testing.T, tc *testContext, snapshotURI string)
}
}

// goldenSnapshotURI is the snapshot owned by the test template's golden tag.
func goldenSnapshotURI(t *testing.T) string {
// goldenSnapshotURI is the snapshot owned by the golden tag of tmpl. An Actor
// created from tmpl with no source tag of its own starts out on it.
func goldenSnapshotURI(t *testing.T, tc *testContext, tmpl *ateapipb.ActorTemplate) string {
t.Helper()
const goldenSnapshotName = "9c2f7b41-6d05-4e83-a1f7-3b8c0d5e2a94"
uri, err := resources.NewTagSnapshotURI(testStorageLocation, resources.GoldenActorAtespace, goldenSnapshotName)
goldenRef := tmpl.GetStatus().GetGoldenSnapshotStatus().GetGoldenTag()
tag, err := tc.client.GetTag(context.Background(), &ateapipb.GetTagRequest{Tag: goldenRef})
if err != nil {
t.Fatalf("NewTagSnapshotURI: %v", err)
t.Fatalf("GetTag(golden tag of %s/%s): %v", tmpl.GetMetadata().GetAtespace(), tmpl.GetMetadata().GetName(), err)
}
return uri.String()
return tag.GetStatus().GetSnapshot().GetSnapshotUri()
}

// snapshotOwnedByActor reports whether snapshotURI sits under the actor's own
Expand Down Expand Up @@ -459,14 +460,31 @@ func createTemplateWithContainersAndVolumes(t *testing.T, tc *testContext, ns st
SourceActor: &ateapipb.ObjectRef{Atespace: resources.GoldenActorAtespace, Name: created.GetMetadata().GetUid()},
Scope: ateapipb.TagScope_TAG_SCOPE_PUBLISHED,
Status: &ateapipb.TagStatus{
Snapshot: &ateapipb.ExternalSnapshot{SnapshotUri: goldenSnapshotURI(t), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL},
ActorTemplateUid: created.GetMetadata().GetUid(),
SourceActorUid: "9c2f7b41-6d05-4e83-a1f7-3b8c0d5e2a94",
StorageLocation: testStorageLocation,
},
})
if err != nil {
t.Fatalf("create golden tag: %v", err)
}
// The golden snapshot sits under the tag's own prefix, keyed on the UID the
// store assigns, so it can only be recorded once the row exists. That is
// what makes an Actor that inherits it a borrower of this tag, the same way
// a clone of an explicitly named tag is.
goldenURI, err := resources.NewTagSnapshotURI(testStorageLocation, resources.GoldenActorAtespace, tag.GetMetadata().GetUid())
if err != nil {
t.Fatalf("NewTagSnapshotURI: %v", err)
}
tc.objectStore.PutSnapshot(t, goldenURI, "manifest.json", "memory.zst")
tag, err = tc.persistence.UpdateTag(context.Background(), resources.TagRefFromTag(tag), store.PreconditionFrom(tag),
func(toUpdate *ateapipb.Tag) error {
toUpdate.Status.Snapshot = &ateapipb.ExternalSnapshot{SnapshotUri: goldenURI.String(), ContentScope: ateapipb.SnapshotContentScope_SNAPSHOT_CONTENT_SCOPE_FULL}
return nil
})
if err != nil {
t.Fatalf("record the golden tag's snapshot: %v", err)
}

// Record the golden snapshot on the template's status directly in the
// store, as the ActorTemplateReconciler's checkpoint would: there is no
Expand Down
154 changes: 154 additions & 0 deletions cmd/ateapi/internal/controlapi/functionaltest/tag_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
"github.com/google/go-cmp/cmp"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/testing/protocmp"
)

Expand Down Expand Up @@ -175,6 +176,14 @@ func suspendActorForTest(t *testing.T, tc *testContext, workerName, name string)
}}); err != nil {
t.Fatalf("CreateActor(%s) failed: %v", name, err)
}
return runAndSuspendActorForTest(t, tc, workerName, name)
}

// runAndSuspendActorForTest resumes an existing actor on workerName and
// suspends it, returning the URI of the external snapshot the suspend wrote.
func runAndSuspendActorForTest(t *testing.T, tc *testContext, workerName, name string) string {
t.Helper()
ctx := context.Background()
// Successive actors share the one worker, and scheduling reads the worker
// cache: the preceding suspend released the worker in the store, but the
// cache only learns of it on its next watch poll.
Expand All @@ -197,6 +206,151 @@ func suspendActorForTest(t *testing.T, tc *testContext, workerName, name string)
return uri
}

// TestDeleteTag_RefusedWhileCloneBorrowsSnapshot walks the whole loop over the
// wire: a tag cannot be deleted while an actor cloned from it is still running
// on the tag's snapshot, and can be once that clone has suspended into one of
// its own and resumed off that one.
func TestDeleteTag_RefusedWhileCloneBorrowsSnapshot(t *testing.T) {
ns := namespaceForTest("ns-delete-tag-borrowed")
tc := setupTest(t, ns)
defer tc.cleanup()

ctx := context.Background()
createTemplate(t, tc, ns)
workerName := createWorkerPod(t, tc, ns, "worker-1", "node1", "pool1")
suspendActorForTest(t, tc, workerName, "actor-a")

const tagName = "v1"
tagRef := &ateapipb.ObjectRef{Atespace: testAtespace, Name: tagName}
tag, err := tc.client.CreateTag(ctx, &ateapipb.CreateTagRequest{
Tag: &ateapipb.Tag{
Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: tagName},
Scope: ateapipb.TagScope_TAG_SCOPE_ATESPACE,
SourceActor: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "actor-a"},
},
})
if err != nil {
t.Fatalf("CreateTag failed: %v", err)
}
tagSnapshotURI := tag.GetStatus().GetSnapshot().GetSnapshotUri()

if _, err := tc.client.CreateActor(ctx, &ateapipb.CreateActorRequest{Actor: &ateapipb.Actor{
Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "clone-1"},
ActorTemplate: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "tmpl1"},
SourceTag: tagRef,
}}); err != nil {
t.Fatalf("CreateActor(clone-1) from tag %s failed: %v", tagName, err)
}

_, err = tc.client.DeleteTag(ctx, &ateapipb.DeleteTagRequest{Tag: tagRef})
if got, want := status.Code(err), codes.FailedPrecondition; got != want {
t.Fatalf("DeleteTag while clone-1 borrows its snapshot = %v (code %v), want %v", err, got, want)
}
if _, err := tc.client.GetTag(ctx, &ateapipb.GetTagRequest{Tag: tagRef}); err != nil {
t.Fatalf("GetTag after the refusal: %v", err)
}
if got := snapshotObjectNames(t, tc, tagSnapshotURI); len(got) == 0 {
t.Error("the refused delete collected the tag's external snapshot anyway")
}

// The clone's first suspend writes a snapshot of its own, which is what
// ends the borrow and frees the tag.
cloneSnapshotURI := runAndSuspendActorForTest(t, tc, workerName, "clone-1")
if cloneSnapshotURI == tagSnapshotURI {
t.Fatalf("clone-1 suspended back into the tag's snapshot %s", cloneSnapshotURI)
}

// Resume actor: check that the tag can be deleted, while the actor is up,
// since the actor no longer borrows the tag's snapshot.
waitForWorkerAvailable(t, tc, workerName)
resumed, err := tc.client.ResumeActor(ctx, &ateapipb.ResumeActorRequest{Actor: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "clone-1"}})
if err != nil {
t.Fatalf("ResumeActor(clone-1) failed: %v", err)
}
if got := resumed.GetActor().GetStatus().GetExternalSnapshot().GetSnapshotUri(); got != cloneSnapshotURI {
t.Fatalf("clone-1 resumed holding snapshot %q, want its own %q", got, cloneSnapshotURI)
}
if got, want := resumed.GetActor().GetStatus().GetState(), ateapipb.ActorState_ACTOR_STATE_RUNNING; got != want {
t.Fatalf("clone-1 state after the resume = %v, want %v", got, want)
}

if _, err := tc.client.DeleteTag(ctx, &ateapipb.DeleteTagRequest{Tag: tagRef}); err != nil {
t.Fatalf("DeleteTag once the borrow ended: %v", err)
}
if _, err := tc.client.GetTag(ctx, &ateapipb.GetTagRequest{Tag: tagRef}); status.Code(err) != codes.NotFound {
t.Errorf("GetTag after the delete = %v, want NotFound", err)
}
if got := snapshotObjectNames(t, tc, cloneSnapshotURI); len(got) == 0 {
t.Error("deleting the tag collected the clone's own external snapshot")
}
}

func TestDeleteTag_RefusedWhileGoldenCloneBorrowsSnapshot(t *testing.T) {
ns := namespaceForTest("ns-delete-golden-tag-borrowed")
tc := setupTest(t, ns)
defer tc.cleanup()

ctx := context.Background()
template := createTemplate(t, tc, ns)
workerName := createWorkerPod(t, tc, ns, "worker-1", "node1", "pool1")
templateRef := resources.ActorTemplateRefFromActorTemplate(template).ToObjectRef()
goldenRef := template.GetStatus().GetGoldenSnapshotStatus().GetGoldenTag()

created, err := tc.client.CreateActor(ctx, &ateapipb.CreateActorRequest{Actor: &ateapipb.Actor{
Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "actor-a"},
ActorTemplate: templateRef,
}})
if err != nil {
t.Fatalf("CreateActor(actor-a) failed: %v", err)
}
goldenURI := goldenSnapshotURI(t, tc, template)
if got := created.GetStatus().GetExternalSnapshot().GetSnapshotUri(); got != goldenURI {
t.Fatalf("actor-a was created holding snapshot %q, want the template's golden %q", got, goldenURI)
}

_, err = tc.client.DeleteTag(ctx, &ateapipb.DeleteTagRequest{Tag: goldenRef})
if got, want := status.Code(err), codes.FailedPrecondition; got != want {
t.Fatalf("DeleteTag(golden) while actor-a borrows its snapshot = %v (code %v), want %v", err, got, want)
}
// Deleting the template is the way a golden tag is normally collected, so
// the borrow has to hold that back too, leaving both resources in place.
_, err = tc.client.DeleteActorTemplate(ctx, &ateapipb.DeleteActorTemplateRequest{ActorTemplate: templateRef})
if got, want := status.Code(err), codes.FailedPrecondition; got != want {
t.Fatalf("DeleteActorTemplate while actor-a borrows the golden snapshot = %v (code %v), want %v", err, got, want)
}
if _, err := tc.client.GetActorTemplate(ctx, &ateapipb.GetActorTemplateRequest{ActorTemplate: templateRef}); err != nil {
t.Fatalf("GetActorTemplate after the refusal: %v", err)
}
if _, err := tc.client.GetTag(ctx, &ateapipb.GetTagRequest{Tag: goldenRef}); err != nil {
t.Fatalf("GetTag(golden) after the refusal: %v", err)
}
assertSnapshotPresent(t, tc, goldenURI)

// The first suspend writes a snapshot under the actor's own prefix and the
// resume restores that one, so the golden tag is free from the suspend on.
ownSnapshotURI := runAndSuspendActorForTest(t, tc, workerName, "actor-a")
assertSnapshotOwnedByActor(t, created, ownSnapshotURI)
waitForWorkerAvailable(t, tc, workerName)
resumed, err := tc.client.ResumeActor(ctx, &ateapipb.ResumeActorRequest{Actor: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "actor-a"}})
if err != nil {
t.Fatalf("ResumeActor(actor-a) failed: %v", err)
}
if got := resumed.GetActor().GetStatus().GetExternalSnapshot().GetSnapshotUri(); got != ownSnapshotURI {
t.Fatalf("actor-a resumed holding snapshot %q, want its own %q", got, ownSnapshotURI)
}

if _, err := tc.client.DeleteActorTemplate(ctx, &ateapipb.DeleteActorTemplateRequest{ActorTemplate: templateRef}); err != nil {
t.Fatalf("DeleteActorTemplate once the borrow ended: %v", err)
}
if _, err := tc.client.GetTag(ctx, &ateapipb.GetTagRequest{Tag: goldenRef}); status.Code(err) != codes.NotFound {
t.Errorf("GetTag(golden) after the delete = %v, want NotFound", err)
}
assertSnapshotCollected(t, tc, goldenURI)
if got := snapshotObjectNames(t, tc, ownSnapshotURI); len(got) == 0 {
t.Error("deleting the template collected the actor's own external snapshot")
}
}

// TestUpdateTag_Preconditions verifies the required version and uid
// guards carried in the tag's metadata.
func TestUpdateTag_Preconditions(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions cmd/ateapi/internal/controlapi/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ type serviceStore interface {
ListTags(ctx context.Context, atespace string, opts store.ListOptions) (store.ListResponse[*ateapipb.Tag], error)
UpdateTag(ctx context.Context, tagRef resources.TagRef, precondition store.Precondition, mutate func(toUpdate *ateapipb.Tag) error) (*ateapipb.Tag, error)
DeleteTag(ctx context.Context, tagRef resources.TagRef) (*ateapipb.Tag, error)
ListTagBorrowers(ctx context.Context, tagUID string, opts store.ListOptions) (store.ListResponse[string], error)
CreateAtespace(ctx context.Context, atespace *ateapipb.Atespace) (*ateapipb.Atespace, error)
GetAtespace(ctx context.Context, name string) (*ateapipb.Atespace, error)
ListAtespaces(ctx context.Context, opts store.ListOptions) (store.ListResponse[*ateapipb.Atespace], error)
Expand Down
29 changes: 26 additions & 3 deletions cmd/ateapi/internal/controlapi/tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,8 @@ func ValidateCustom_UpdateTagRequest_Tag(ctx context.Context, op operation.Opera
// CreateActor racing this delete can seed an Actor from content that is going
// away. That race is accepted for now.
//
// Note that this destroys the external snapshot: an Actor created from the tag
// and never suspended is still borrowing it and becomes unrecoverable. Do not
// delete a tag while clones of it exist.
// DeletaTag is refused with FailedPrecondition if at least one actor is still borrowing
// the tag's snapshot.
func (s *RPCService) DeleteTag(ctx context.Context, req *ateapipb.DeleteTagRequest) (*ateapipb.Tag, error) {
// TODO: mode delete orchestration to a workflow.
if errs := validateDeleteTagRequest(ctx, req); len(errs) > 0 {
Expand All @@ -280,6 +279,9 @@ func (s *RPCService) DeleteTag(ctx context.Context, req *ateapipb.DeleteTagReque
}
return nil, fmt.Errorf("while getting tag: %w", err)
}
if err := s.checkTagBorrowers(ctx, stored); err != nil {
return nil, err
}
if err := s.releaseTagSnapshot(ctx, stored); err != nil {
return nil, err
}
Expand All @@ -294,6 +296,23 @@ func (s *RPCService) DeleteTag(ctx context.Context, req *ateapipb.DeleteTagReque
return tag, nil
}

// checkTagBorrowers refuses the delete while an Actor is still using the tag's
// external snapshot as its own.
func (s *RPCService) checkTagBorrowers(ctx context.Context, tag *ateapipb.Tag) error {
atespace, name := tag.GetMetadata().GetAtespace(), tag.GetMetadata().GetName()

borrowers, err := s.impl.ListTagBorrowers(ctx, tag.GetMetadata().GetUid(), store.ListOptions{PageSize: 1})
if err != nil {
return fmt.Errorf("while listing the borrowers of tag %s/%s: %w", atespace, name, err)
}
if len(borrowers.Items) == 0 {
return nil
}
return status.Errorf(codes.FailedPrecondition,
"Tag %s/%s cannot be deleted because its snapshot is still in use by at least one Actor created from it",
atespace, name)
}

// releaseTagSnapshot deletes the objects the tag's external snapshot is made
// of. It tolerates a partly-collected snapshot, so a retry finishes cleanly.
// It collects the in-progress snapshot too.
Expand All @@ -317,6 +336,10 @@ func (s *ServiceImpl) DeleteTag(ctx context.Context, tagRef resources.TagRef) (*
return s.store.DeleteTag(ctx, tagRef)
}

func (s *ServiceImpl) ListTagBorrowers(ctx context.Context, tagUID string, opts store.ListOptions) (store.ListResponse[string], error) {
return s.store.ListTagBorrowers(ctx, tagUID, opts)
}

func validateDeleteTagRequest(ctx context.Context, req *ateapipb.DeleteTagRequest) field.ErrorList {
op := operation.Operation{Type: operation.Create}
return Validate_DeleteTagRequest(ctx, op, nil, req, nil)
Expand Down
Loading
Loading