diff --git a/cmds/db-manager/cleanup/evict.go b/cmds/db-manager/cleanup/evict.go index c03c0b7a9..87b837454 100644 --- a/cmds/db-manager/cleanup/evict.go +++ b/cmds/db-manager/cleanup/evict.go @@ -8,14 +8,8 @@ import ( "github.com/interuss/dss/pkg/logging" dssmodels "github.com/interuss/dss/pkg/models" - ridmodels "github.com/interuss/dss/pkg/rid/models" - ridrepos "github.com/interuss/dss/pkg/rid/repos" rids "github.com/interuss/dss/pkg/rid/store" - scdmodels "github.com/interuss/dss/pkg/scd/models" - scdrepos "github.com/interuss/dss/pkg/scd/repos" scds "github.com/interuss/dss/pkg/scd/store" - dssstore "github.com/interuss/dss/pkg/store" - "github.com/interuss/stacktrace" "github.com/spf13/cobra" "github.com/spf13/pflag" ) @@ -66,101 +60,92 @@ func evict(cmd *cobra.Command, _ []string) error { } var ( - expiredOpIntents []*scdmodels.OperationalIntent - scdExpiredSub []*scdmodels.Subscription - expiredISAs []*ridmodels.IdentificationServiceArea - ridExpiredSub []*ridmodels.Subscription + expiredOpIntents []dssmodels.ID + scdExpiredSub []dssmodels.ID + expiredISAs []dssmodels.ID + ridExpiredSub []dssmodels.ID ) - scdAction := func(ctx context.Context, r scdrepos.Repository) (err error) { - if *checkScdOirs { - expiredOpIntents, err = r.ListExpiredOperationalIntents(ctx, scdThreshold) + scdRepo, err := scdStore.Interact(ctx) + if err != nil { + return fmt.Errorf("failed to interact with SCD store: %w", err) + } + + if *checkScdOirs { + if *deleteExpired { + expiredOpIntents, err = scdRepo.DeleteExpiredOperationalIntents(ctx, scdThreshold) if err != nil { - return fmt.Errorf("listing expired operational intents: %w", err) + return fmt.Errorf("failed to delete expired operational intents: %w", err) } - if *deleteExpired { - for _, opIntent := range expiredOpIntents { - if err = r.DeleteOperationalIntent(ctx, opIntent.ID); err != nil { - return fmt.Errorf("deleting expired operational intents: %w", err) - } - } + } else { + expiredOpIntents, err = scdRepo.ListExpiredOperationalIntents(ctx, scdThreshold) + if err != nil { + return fmt.Errorf("failed to list expired operational intents: %w", err) } } + } - if *checkScdSubs { - scdExpiredSub, err = r.ListExpiredSubscriptions(ctx, scdThreshold) + if *checkScdSubs { + if *deleteExpired { + scdExpiredSub, err = scdRepo.DeleteExpiredSubscriptions(ctx, scdThreshold) if err != nil { - return fmt.Errorf("SCD listing expired subscriptions: %w", err) + return fmt.Errorf("failed to delete expired SCD subscriptions: %w", err) } - if *deleteExpired { - for _, sub := range scdExpiredSub { - if err = r.DeleteSubscription(ctx, sub.ID); err != nil { - return fmt.Errorf("SCD deleting expired subscriptions: %w", err) - } - } + } else { + scdExpiredSub, err = scdRepo.ListExpiredSubscriptions(ctx, scdThreshold) + if err != nil { + return fmt.Errorf("failed to list expired SCD subscriptions: %w", err) } } - return nil - } - if _, err = scdStore.Transact(ctx, dssstore.NewFuncOperation(scdAction)); err != nil { - return fmt.Errorf("failed to execute SCD transaction: %w", err) } - ridAction := func(ctx context.Context, r ridrepos.Repository) (err error) { - if *checkRidISAs { + ridRepo, err := ridStore.Interact(ctx) + if err != nil { + return fmt.Errorf("failed to interact with RID store: %w", err) + } - expiredISAs, err = r.ListExpiredISAs(ctx, *locality, ridThreshold) + if *checkRidISAs { + if *deleteExpired { + expiredISAs, err = ridRepo.DeleteExpiredISAs(ctx, *locality, ridThreshold) if err != nil { - return stacktrace.Propagate(err, "Failed to list expired ISAs") + return fmt.Errorf("failed to delete expired ISAs: %w", err) } - - if *deleteExpired { - for _, isa := range expiredISAs { - _, err := r.DeleteISA(ctx, isa) - if err != nil { - return stacktrace.Propagate(err, "Failed to delete ISAs") - } - } + } else { + expiredISAs, err = ridRepo.ListExpiredISAs(ctx, *locality, ridThreshold) + if err != nil { + return fmt.Errorf("failed to list expired ISAs: %w", err) } - } + } - if *checkRidSubs { - - ridExpiredSub, err = r.ListExpiredSubscriptions(ctx, *locality, ridThreshold) + if *checkRidSubs { + if *deleteExpired { + ridExpiredSub, err = ridRepo.DeleteExpiredSubscriptions(ctx, *locality, ridThreshold) if err != nil { - return stacktrace.Propagate(err, - "Failed to list RID expired Subscriptions") + return fmt.Errorf("failed to delete expired RID subscriptions: %w", err) } - - if *deleteExpired { - for _, sub := range ridExpiredSub { - _, err := r.DeleteSubscription(ctx, sub) - if err != nil { - return stacktrace.Propagate(err, - "Failed to delete RID Subscription") - } - } + } else { + ridExpiredSub, err = ridRepo.ListExpiredSubscriptions(ctx, *locality, ridThreshold) + if err != nil { + return fmt.Errorf("failed to list RID expired subscriptions: %w", err) } - } - - return nil - } - if _, err = ridStore.Transact(ctx, dssstore.NewFuncOperation(ridAction)); err != nil { - return fmt.Errorf("failed to execute RID transaction: %w", err) } - for _, opIntent := range expiredOpIntents { - logExpiredEntity("operational intent", opIntent.ID, scdThreshold, *deleteExpired, opIntent.EndTime != nil) + action := "found" + if *deleteExpired { + action = "deleted" } - for _, sub := range scdExpiredSub { - logExpiredEntity("SCD subscription", sub.ID, scdThreshold, *deleteExpired, sub.EndTime != nil) + for _, id := range expiredOpIntents { + log.Printf("%s expired operational intent %s", action, id) } - for _, isa := range expiredISAs { - logExpiredEntity("ISA", isa.ID, ridThreshold, *deleteExpired, isa.EndTime != nil) + for _, id := range scdExpiredSub { + log.Printf("%s expired SCD subscription %s", action, id) } - for _, sub := range ridExpiredSub { - logExpiredEntity("RID subscription", sub.ID, ridThreshold, *deleteExpired, sub.EndTime != nil) + for _, id := range expiredISAs { + log.Printf("%s expired ISA %s", action, id) + } + for _, id := range ridExpiredSub { + log.Printf("%s expired RID subscription %s", action, id) } if len(expiredOpIntents) == 0 && len(scdExpiredSub) == 0 && len(expiredISAs) == 0 && len(ridExpiredSub) == 0 { log.Printf("no SCD entity older than %s and no RID entity older than %s found", scdThreshold.String(), ridThreshold.String()) @@ -169,16 +154,3 @@ func evict(cmd *cobra.Command, _ []string) error { } return nil } - -func logExpiredEntity(entity string, entityID dssmodels.ID, threshold time.Time, deleted, hasEndTime bool) { - logMsg := "found" - if deleted { - logMsg = "deleted" - } - - expMsg := "last update before %s (missing end time)" - if hasEndTime { - expMsg = "end time before %s" - } - log.Printf("%s %s %s; expired due to %s", logMsg, entity, entityID.String(), fmt.Sprintf(expMsg, threshold.String())) -} diff --git a/docs/operations/cleanup.md b/docs/operations/cleanup.md index 2dbafdb96..c0940c41a 100644 --- a/docs/operations/cleanup.md +++ b/docs/operations/cleanup.md @@ -38,9 +38,9 @@ The defaults shipped with the deployment tooling (`30m` TTL on RID running every ## Performance impact -All expired entities are identified and removed within a single transaction. When the system is under heavy load, lock contention with concurrent transactions may cause the cleanup to fail. There is no risk of data inconsistency in this case - the cleanup may simply be retried. +Each entity type (SCD operational intents, SCD subscriptions, RID ISAs, RID subscriptions) is evicted with a single statement. Entity types are deleted independently, so contention or failure on one does not affect the others. There is no risk of data inconsistency in this case - the cleanup may simply be retried. -To mitigate this: +To mitigate contention: - Run the cleanup during low-intensity periods (e.g. at night). - Clean up iteratively, starting with a lower TTL and progressively increasing it. diff --git a/pkg/rid/operations/subscription_test.go b/pkg/rid/operations/subscription_test.go index 9e053714f..6fea929c2 100644 --- a/pkg/rid/operations/subscription_test.go +++ b/pkg/rid/operations/subscription_test.go @@ -180,7 +180,11 @@ func (r *fakeSubscriptionRepo) MaxSubscriptionCountInCellsByOwner(ctx context.Co return maxValue, nil } -func (r *fakeSubscriptionRepo) ListExpiredSubscriptions(_ context.Context, _ string, _ time.Time) ([]*ridmodels.Subscription, error) { +func (r *fakeSubscriptionRepo) ListExpiredSubscriptions(_ context.Context, _ string, _ time.Time) ([]dssmodels.ID, error) { + return nil, nil +} + +func (r *fakeSubscriptionRepo) DeleteExpiredSubscriptions(_ context.Context, _ string, _ time.Time) ([]dssmodels.ID, error) { return nil, nil } @@ -229,7 +233,11 @@ func (r *fakeSubscriptionRepo) SearchISAs(_ context.Context, cells s2.CellUnion, return isas, nil } -func (r *fakeSubscriptionRepo) ListExpiredISAs(_ context.Context, _ string, _ time.Time) ([]*ridmodels.IdentificationServiceArea, error) { +func (r *fakeSubscriptionRepo) ListExpiredISAs(_ context.Context, _ string, _ time.Time) ([]dssmodels.ID, error) { + panic("not implemented") +} + +func (r *fakeSubscriptionRepo) DeleteExpiredISAs(_ context.Context, _ string, _ time.Time) ([]dssmodels.ID, error) { panic("not implemented") } diff --git a/pkg/rid/repos/isa.go b/pkg/rid/repos/isa.go index 5d6ffaef8..3f2e1d18e 100644 --- a/pkg/rid/repos/isa.go +++ b/pkg/rid/repos/isa.go @@ -29,8 +29,11 @@ type ISA interface { // SearchISAs returns all subscriptions ownded by "owner" in "cells". SearchISAs(ctx context.Context, cells s2.CellUnion, earliest *time.Time, latest *time.Time) ([]*ridmodels.IdentificationServiceArea, error) - // ListExpiredISAs lists all expired ISAs based on writer - ListExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]*ridmodels.IdentificationServiceArea, error) + // ListExpiredISAs lists the IDs of all expired ISAs based on writer + ListExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) + + // DeleteExpiredISAs deletes all expired ISAs based on writer and returns the IDs of the deleted ISAs. + DeleteExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) // Count the number of existing ISA CountISAs(ctx context.Context) (int64, error) diff --git a/pkg/rid/repos/subscription.go b/pkg/rid/repos/subscription.go index 1ec849f72..8a6e241f8 100644 --- a/pkg/rid/repos/subscription.go +++ b/pkg/rid/repos/subscription.go @@ -39,8 +39,11 @@ type Subscription interface { // belonging to the given owner, and returns that number. MaxSubscriptionCountInCellsByOwner(ctx context.Context, cells s2.CellUnion, owner dssmodels.Owner) (int, error) - // ListExpiredSubscriptions lists all expired Subscriptions based on writer. - ListExpiredSubscriptions(ctx context.Context, writer string, threshold time.Time) ([]*ridmodels.Subscription, error) + // ListExpiredSubscriptions lists the IDs of all expired Subscriptions based on writer. + ListExpiredSubscriptions(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) + + // DeleteExpiredSubscriptions deletes all expired Subscriptions based on writer and returns the IDs of the deleted Subscriptions. + DeleteExpiredSubscriptions(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) // Count the number of existing subscriptions CountSubscriptions(ctx context.Context) (int64, error) diff --git a/pkg/rid/store/memstore/identification_service_area.go b/pkg/rid/store/memstore/identification_service_area.go index 3788a2b2a..0f59f9887 100644 --- a/pkg/rid/store/memstore/identification_service_area.go +++ b/pkg/rid/store/memstore/identification_service_area.go @@ -129,8 +129,14 @@ func (r *repo) SearchISAs(_ context.Context, cells s2.CellUnion, earliest *time. return out, nil } -func (r *repo) ListExpiredISAs(_ context.Context, writer string, threshold time.Time) ([]*ridmodels.IdentificationServiceArea, error) { - return listExpired[ridmodels.IdentificationServiceArea](r.state.ISAs, writer, threshold, dssmodels.MaxResultLimit), nil +// TODO: Implement when raftstore evict is implemented (#1718) +func (r *repo) ListExpiredISAs(_ context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { + return listExpired(r.state.ISAs, writer, threshold, dssmodels.MaxResultLimit), nil +} + +// TODO: Implement when raftstore evict is implemented (#1718) +func (r *repo) DeleteExpiredISAs(_ context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { + return nil, stacktrace.NewErrorWithCode(dsserr.NotImplemented, "DeleteExpiredISAs not implemented for memstore") } func (r *repo) CountISAs(_ context.Context) (int64, error) { diff --git a/pkg/rid/store/memstore/store.go b/pkg/rid/store/memstore/store.go index 6709955bc..381588e7e 100644 --- a/pkg/rid/store/memstore/store.go +++ b/pkg/rid/store/memstore/store.go @@ -139,10 +139,9 @@ func findForWrite[R versionedRecord](store map[dssmodels.ID]R, id dssmodels.ID, return rec, true } -type expiringRecord[M any] interface { +type expiringRecord interface { endTime() *time.Time writerName() string - toModel() *M } func (rec *isaRecord) endTime() *time.Time { return rec.EndTime } @@ -153,18 +152,18 @@ func (rec *subscriptionRecord) endTime() *time.Time { return rec.EndTime } func (rec *subscriptionRecord) writerName() string { return rec.Writer } -// listExpired returns the records whose end time is at or before threshold and +// listExpired returns the IDs of the records whose end time is at or before threshold and // whose writer matches. A limit of 0 means unlimited. -func listExpired[M any, R expiringRecord[M]](store map[dssmodels.ID]R, writer string, threshold time.Time, limit int) []*M { - var out []*M - for _, rec := range store { +func listExpired[R expiringRecord](store map[dssmodels.ID]R, writer string, threshold time.Time, limit int) []dssmodels.ID { + var out []dssmodels.ID + for id, rec := range store { if t := rec.endTime(); t == nil || t.After(threshold) { // TODO: Don't allow endtime to be null, see #1492 continue } if rec.writerName() != writer { continue } - out = append(out, rec.toModel()) + out = append(out, id) if limit > 0 && len(out) > limit { // This mimics sqlstore behaviour, but it's not very good. break diff --git a/pkg/rid/store/memstore/subscriptions.go b/pkg/rid/store/memstore/subscriptions.go index b1381354e..8f2c1b497 100644 --- a/pkg/rid/store/memstore/subscriptions.go +++ b/pkg/rid/store/memstore/subscriptions.go @@ -185,9 +185,15 @@ func (r *repo) MaxSubscriptionCountInCellsByOwner(ctx context.Context, cells s2. return slices.Max(slices.Collect(maps.Values(counts))), nil } -func (r *repo) ListExpiredSubscriptions(_ context.Context, writer string, threshold time.Time) ([]*ridmodels.Subscription, error) { +// TODO: Implement when raftstore evict is implemented (#1718) +func (r *repo) ListExpiredSubscriptions(_ context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { // TODO: This mimics sqlstore inconsistency of not limiting results there, compared to ISAs. Should it be normalized? - return listExpired[ridmodels.Subscription](r.state.Subscriptions, writer, threshold, 0), nil + return listExpired(r.state.Subscriptions, writer, threshold, 0), nil +} + +// TODO: Implement when raftstore evict is implemented (#1718) +func (r *repo) DeleteExpiredSubscriptions(_ context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { + return nil, stacktrace.NewErrorWithCode(dsserr.NotImplemented, "DeleteExpiredSubscriptions not implemented for memstore") } func (r *repo) CountSubscriptions(_ context.Context) (int64, error) { diff --git a/pkg/rid/store/raftstore/identification_service_area.go b/pkg/rid/store/raftstore/identification_service_area.go index c633c142c..db96d35fe 100644 --- a/pkg/rid/store/raftstore/identification_service_area.go +++ b/pkg/rid/store/raftstore/identification_service_area.go @@ -6,6 +6,7 @@ import ( "time" "github.com/golang/geo/s2" + dsserr "github.com/interuss/dss/pkg/errors" dssmodels "github.com/interuss/dss/pkg/models" "github.com/interuss/dss/pkg/raftstore/consensus" ridmodels "github.com/interuss/dss/pkg/rid/models" @@ -18,7 +19,7 @@ const ( insertISA consensus.RequestType[*ridmodels.IdentificationServiceArea] = "insertISA" updateISA consensus.RequestType[*ridmodels.IdentificationServiceArea] = "updateISA" searchISAs consensus.RequestType[[]*ridmodels.IdentificationServiceArea] = "searchISAs" - listExpiredISAs consensus.RequestType[[]*ridmodels.IdentificationServiceArea] = "listExpiredISAs" + listExpiredISAs consensus.RequestType[[]dssmodels.ID] = "listExpiredISAs" countISAs consensus.RequestType[int64] = "countISAs" ) @@ -69,7 +70,7 @@ func (r *repo) SearchISAs(ctx context.Context, cells s2.CellUnion, earliest *tim return r.consensus.HandleReadRequest(ctx, searchISAs, buf) } -func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]*ridmodels.IdentificationServiceArea, error) { +func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { buf, err := json.Marshal(expiredPayload{Writer: writer, Threshold: threshold}) if err != nil { return nil, stacktrace.Propagate(err, "failed to marshal payload") @@ -78,6 +79,10 @@ func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold tim return r.consensus.HandleReadRequest(ctx, listExpiredISAs, buf) } +func (r *repo) DeleteExpiredISAs(_ context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { + return nil, stacktrace.NewErrorWithCode(dsserr.NotImplemented, "DeleteExpiredISAs not implemented for raftstore") +} + func (r *repo) CountISAs(ctx context.Context) (int64, error) { return r.consensus.HandleReadRequest(ctx, countISAs, nil) } diff --git a/pkg/rid/store/raftstore/subscriptions.go b/pkg/rid/store/raftstore/subscriptions.go index 6dc450210..5bdaf6669 100644 --- a/pkg/rid/store/raftstore/subscriptions.go +++ b/pkg/rid/store/raftstore/subscriptions.go @@ -6,6 +6,7 @@ import ( "time" "github.com/golang/geo/s2" + dsserr "github.com/interuss/dss/pkg/errors" dssmodels "github.com/interuss/dss/pkg/models" "github.com/interuss/dss/pkg/raftstore/consensus" ridmodels "github.com/interuss/dss/pkg/rid/models" @@ -21,7 +22,7 @@ const ( searchSubscriptionsByOwner consensus.RequestType[[]*ridmodels.Subscription] = "searchSubscriptionsByOwner" updateNotificationIdxsInCells consensus.RequestType[[]*ridmodels.Subscription] = "updateNotificationIdxsInCells" maxSubscriptionCountInCellsByOwner consensus.RequestType[int] = "maxSubscriptionCountInCellsByOwner" - listExpiredSubscriptions consensus.RequestType[[]*ridmodels.Subscription] = "listExpiredSubscriptions" + listExpiredSubscriptions consensus.RequestType[[]dssmodels.ID] = "listExpiredSubscriptions" countSubscriptions consensus.RequestType[int64] = "countSubscriptions" ) @@ -97,7 +98,7 @@ func (r *repo) MaxSubscriptionCountInCellsByOwner(ctx context.Context, cells s2. return r.consensus.HandleReadRequest(ctx, maxSubscriptionCountInCellsByOwner, buf) } -func (r *repo) ListExpiredSubscriptions(ctx context.Context, writer string, threshold time.Time) ([]*ridmodels.Subscription, error) { +func (r *repo) ListExpiredSubscriptions(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { buf, err := json.Marshal(expiredPayload{Writer: writer, Threshold: threshold}) if err != nil { return nil, stacktrace.Propagate(err, "failed to marshal payload") @@ -106,6 +107,10 @@ func (r *repo) ListExpiredSubscriptions(ctx context.Context, writer string, thre return r.consensus.HandleReadRequest(ctx, listExpiredSubscriptions, buf) } +func (r *repo) DeleteExpiredSubscriptions(_ context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { + return nil, stacktrace.NewErrorWithCode(dsserr.NotImplemented, "DeleteExpiredSubscriptions not implemented for raftstore") +} + func (r *repo) CountSubscriptions(ctx context.Context) (int64, error) { return r.consensus.HandleReadRequest(ctx, countSubscriptions, nil) } diff --git a/pkg/rid/store/sqlstore/identification_service_area.go b/pkg/rid/store/sqlstore/identification_service_area.go index 35a97249a..7fda71ad4 100644 --- a/pkg/rid/store/sqlstore/identification_service_area.go +++ b/pkg/rid/store/sqlstore/identification_service_area.go @@ -210,34 +210,52 @@ func (r *repo) SearchISAs(ctx context.Context, cells s2.CellUnion, earliest *tim return r.fetchISAs(ctx, isasInCellsQuery, earliest, latest, dssql.CellUnionToCellIds(cells), dssmodels.MaxResultLimit) } -// ListExpiredISAs lists all expired ISAs based on writer. +// ListExpiredISAs lists the IDs of all expired ISAs based on writer. // The function queries both empty writer and null writer when passing empty string as a writer. -func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]*ridmodels.IdentificationServiceArea, error) { +func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { if len(writer) == 0 { - isasInCellsQuery := fmt.Sprintf(` + isasInCellsQuery := ` SELECT - %s + id FROM identification_service_areas WHERE ends_at <= $1 AND - (writer = '' OR writer IS NULL) - LIMIT $2`, isaFields) - return r.fetchISAs(ctx, isasInCellsQuery, threshold, dssmodels.MaxResultLimit) + (writer = '' OR writer IS NULL)` + return dssql.FetchIDs(ctx, r.Queryable, isasInCellsQuery, threshold) } - isasInCellsQuery := fmt.Sprintf(` + isasInCellsQuery := ` SELECT - %s + id FROM identification_service_areas WHERE ends_at <= $1 AND - writer = $2 - LIMIT $3`, isaFields) - return r.fetchISAs(ctx, isasInCellsQuery, threshold, writer, dssmodels.MaxResultLimit) + writer = $2` + return dssql.FetchIDs(ctx, r.Queryable, isasInCellsQuery, threshold, writer) +} + +// DeleteExpiredISAs deletes all expired ISAs based on writer and returns the IDs of the deleted ISAs. +// The function deletes both empty writer and null writer when passing empty string as a writer. +func (r *repo) DeleteExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { + if len(writer) == 0 { + deleteExpiredQuery := ` + DELETE FROM identification_service_areas + WHERE ends_at <= $1 + AND (writer = '' OR writer IS NULL) + RETURNING id` + return dssql.FetchIDs(ctx, r.Queryable, deleteExpiredQuery, threshold) + } + + deleteExpiredQuery := ` + DELETE FROM identification_service_areas + WHERE ends_at <= $1 + AND writer = $2 + RETURNING id` + return dssql.FetchIDs(ctx, r.Queryable, deleteExpiredQuery, threshold, writer) } func (r *repo) CountISAs(ctx context.Context) (int64, error) { diff --git a/pkg/rid/store/sqlstore/subscriptions.go b/pkg/rid/store/sqlstore/subscriptions.go index 29fde6ca0..8e3ab3dfb 100644 --- a/pkg/rid/store/sqlstore/subscriptions.go +++ b/pkg/rid/store/sqlstore/subscriptions.go @@ -294,32 +294,53 @@ func (r *repo) SearchSubscriptionsByOwner(ctx context.Context, cells s2.CellUnio return r.process(ctx, query, dssql.CellUnionToCellIds(cells), owner, r.clock.Now(), dssmodels.MaxResultLimit) } -// ListExpiredSubscriptions lists all expired Subscriptions based on writer. +// ListExpiredSubscriptions lists the IDs of all expired Subscriptions based on writer. // The function queries both empty writer and null writer when passing empty string as a writer. -func (r *repo) ListExpiredSubscriptions(ctx context.Context, writer string, threshold time.Time) ([]*ridmodels.Subscription, error) { +func (r *repo) ListExpiredSubscriptions(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { if len(writer) == 0 { - query := fmt.Sprintf(` + query := ` SELECT - %s + id FROM subscriptions WHERE ends_at <= $1 AND - (writer = '' OR writer IS NULL)`, subscriptionFields) - return r.process(ctx, query, threshold) + (writer = '' OR writer IS NULL)` + return dssql.FetchIDs(ctx, r.Queryable, query, threshold) } - query := fmt.Sprintf(` + query := ` SELECT - %s + id FROM subscriptions WHERE ends_at <= $1 AND - writer = $2`, subscriptionFields) - return r.process(ctx, query, threshold, writer) + writer = $2` + return dssql.FetchIDs(ctx, r.Queryable, query, threshold, writer) +} + +// DeleteExpiredSubscriptions deletes all expired Subscriptions based on writer and returns the +// IDs of the deleted Subscriptions. +// The function deletes both empty writer and null writer when passing empty string as a writer. +func (r *repo) DeleteExpiredSubscriptions(ctx context.Context, writer string, threshold time.Time) ([]dssmodels.ID, error) { + if len(writer) == 0 { + deleteExpiredQuery := ` + DELETE FROM subscriptions + WHERE ends_at <= $1 + AND (writer = '' OR writer IS NULL) + RETURNING id` + return dssql.FetchIDs(ctx, r.Queryable, deleteExpiredQuery, threshold) + } + + deleteExpiredQuery := ` + DELETE FROM subscriptions + WHERE ends_at <= $1 + AND writer = $2 + RETURNING id` + return dssql.FetchIDs(ctx, r.Queryable, deleteExpiredQuery, threshold, writer) } func (r *repo) CountSubscriptions(ctx context.Context) (int64, error) { diff --git a/pkg/scd/repos/repos.go b/pkg/scd/repos/repos.go index 9d87bae33..06383d133 100644 --- a/pkg/scd/repos/repos.go +++ b/pkg/scd/repos/repos.go @@ -30,9 +30,13 @@ type OperationalIntent interface { // subscription identified by "subscriptionID". GetDependentOperationalIntents(ctx context.Context, subscriptionID dssmodels.ID) ([]dssmodels.ID, error) - // ListExpiredOperationalIntents lists all operational intents older than the threshold. + // ListExpiredOperationalIntents lists the IDs of all operational intents older than the threshold. // Their age is determined by their end time, or by their update time if they do not have an end time. - ListExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]*scdmodels.OperationalIntent, error) + ListExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) + + // DeleteExpiredOperationalIntents deletes all expired operational intents and returns the IDs of the deleted operational intents. + // Age is determined by their end time, or by their update time if they do not have an end time. + DeleteExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) // Count the number of existing operational intent CountOperationalIntents(ctx context.Context) (int64, error) @@ -69,9 +73,13 @@ type Subscription interface { // LockSubscriptionsOnCells locks the subscriptions of interest on specific cells and, optionnaly, specific subscriptions via their IDs LockSubscriptionsOnCells(ctx context.Context, cells s2.CellUnion, subscriptionIds []dssmodels.ID, startTime *time.Time, endTime *time.Time) error - // ListExpiredSubscriptions lists all subscriptions older than the threshold. + // ListExpiredSubscriptions lists the IDs of all subscriptions older than the threshold. // Their age is determined by their end time, or by their update time if they do not have an end time. - ListExpiredSubscriptions(ctx context.Context, threshold time.Time) ([]*scdmodels.Subscription, error) + ListExpiredSubscriptions(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) + + // DeleteExpiredSubscriptions deletes all expired subscriptions and returns the IDs of the deleted subscriptions. + // Age is determined by their end time, or by their update time if they do not have an end time. + DeleteExpiredSubscriptions(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) // Count the number of existing subscriptions CountSubscriptions(ctx context.Context) (int64, error) diff --git a/pkg/scd/store/memstore/operational_intents.go b/pkg/scd/store/memstore/operational_intents.go index 309081a9a..1ef606f4e 100644 --- a/pkg/scd/store/memstore/operational_intents.go +++ b/pkg/scd/store/memstore/operational_intents.go @@ -158,8 +158,18 @@ func (r *repo) GetDependentOperationalIntents(_ context.Context, subscriptionID return dependentOps, nil } -func (r *repo) ListExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]*scdmodels.OperationalIntent, error) { - return r.buildOperationalIntents(ctx, listExpired(r.state.OperationalIntents, threshold, dssmodels.MaxResultLimit)) +// TODO: Implement when raftstore evict is implemented (#1718) +func (r *repo) ListExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) { + var out []dssmodels.ID + for _, rec := range listExpired(r.state.OperationalIntents, threshold, dssmodels.MaxResultLimit) { + out = append(out, rec.ID) + } + return out, nil +} + +// TODO: Implement when raftstore evict is implemented (#1718) +func (r *repo) DeleteExpiredOperationalIntents(_ context.Context, threshold time.Time) ([]dssmodels.ID, error) { + return nil, stacktrace.NewErrorWithCode(dsserr.NotImplemented, "DeleteExpiredOperationalIntents not implemented for memstore") } func (r *repo) CountOperationalIntents(_ context.Context) (int64, error) { diff --git a/pkg/scd/store/memstore/operational_intents_test.go b/pkg/scd/store/memstore/operational_intents_test.go index 7aa0e8750..6a484f1d4 100644 --- a/pkg/scd/store/memstore/operational_intents_test.go +++ b/pkg/scd/store/memstore/operational_intents_test.go @@ -209,12 +209,7 @@ func TestListExpiredOperationalIntents(t *testing.T) { threshold := testCase.timeRef.Add(-testCase.ttl) expired, err := r.ListExpiredOperationalIntents(ctx, threshold) require.NoError(t, err) - - expiredIDs := make([]dssmodels.ID, 0, len(expired)) - for _, expiredOi := range expired { - expiredIDs = append(expiredIDs, expiredOi.ID) - } - require.ElementsMatch(t, expiredIDs, testCase.expired) + require.ElementsMatch(t, expired, testCase.expired) }) } } diff --git a/pkg/scd/store/memstore/subscriptions.go b/pkg/scd/store/memstore/subscriptions.go index a51244f5f..7d20c5348 100644 --- a/pkg/scd/store/memstore/subscriptions.go +++ b/pkg/scd/store/memstore/subscriptions.go @@ -7,6 +7,7 @@ import ( "time" "github.com/golang/geo/s2" + dsserr "github.com/interuss/dss/pkg/errors" "github.com/interuss/dss/pkg/memstore/utils" dssmodels "github.com/interuss/dss/pkg/models" scdmodels "github.com/interuss/dss/pkg/scd/models" @@ -130,14 +131,20 @@ func (r *repo) LockSubscriptionsOnCells(_ context.Context, _ s2.CellUnion, _ []d return nil } -func (r *repo) ListExpiredSubscriptions(_ context.Context, threshold time.Time) ([]*scdmodels.Subscription, error) { - var out []*scdmodels.Subscription +// TODO: Implement when raftstore evict is implemented (#1718) +func (r *repo) ListExpiredSubscriptions(_ context.Context, threshold time.Time) ([]dssmodels.ID, error) { + var out []dssmodels.ID for _, rec := range listExpired(r.state.Subscriptions, threshold, dssmodels.MaxResultLimit) { - out = append(out, rec.toModel()) + out = append(out, rec.ID) } return out, nil } +// TODO: Implement when raftstore evict is implemented (#1718) +func (r *repo) DeleteExpiredSubscriptions(_ context.Context, threshold time.Time) ([]dssmodels.ID, error) { + return nil, stacktrace.NewErrorWithCode(dsserr.NotImplemented, "DeleteExpiredSubscriptions not implemented for memstore") +} + func (r *repo) CountSubscriptions(_ context.Context) (int64, error) { return int64(len(r.state.Subscriptions)), nil } diff --git a/pkg/scd/store/memstore/subscriptions_test.go b/pkg/scd/store/memstore/subscriptions_test.go index 7c530b65c..a7d251502 100644 --- a/pkg/scd/store/memstore/subscriptions_test.go +++ b/pkg/scd/store/memstore/subscriptions_test.go @@ -229,12 +229,7 @@ func TestListExpiredSubscriptions(t *testing.T) { threshold := testCase.timeRef.Add(-testCase.ttl) expired, err := r.ListExpiredSubscriptions(ctx, threshold) require.NoError(t, err) - - expiredIDs := make([]dssmodels.ID, 0, len(expired)) - for _, expiredSub := range expired { - expiredIDs = append(expiredIDs, expiredSub.ID) - } - require.ElementsMatch(t, expiredIDs, testCase.expired) + require.ElementsMatch(t, expired, testCase.expired) }) } } diff --git a/pkg/scd/store/raftstore/operational_intents.go b/pkg/scd/store/raftstore/operational_intents.go index a8c35f771..140d2ca9e 100644 --- a/pkg/scd/store/raftstore/operational_intents.go +++ b/pkg/scd/store/raftstore/operational_intents.go @@ -5,6 +5,7 @@ import ( "encoding/json" "time" + dsserr "github.com/interuss/dss/pkg/errors" dssmodels "github.com/interuss/dss/pkg/models" "github.com/interuss/dss/pkg/raftstore/consensus" scdmodels "github.com/interuss/dss/pkg/scd/models" @@ -17,7 +18,7 @@ const ( upsertOperationalIntent consensus.RequestType[*scdmodels.OperationalIntent] = "upsertOperationalIntent" searchOperationalIntents consensus.RequestType[[]*scdmodels.OperationalIntent] = "searchOperationalIntents" getDependentOperationalIntents consensus.RequestType[[]dssmodels.ID] = "getDependentOperationalIntents" - listExpiredOperationalIntents consensus.RequestType[[]*scdmodels.OperationalIntent] = "listExpiredOperationalIntents" + listExpiredOperationalIntents consensus.RequestType[[]dssmodels.ID] = "listExpiredOperationalIntents" countOperationalIntents consensus.RequestType[int64] = "countOperationalIntents" ) @@ -67,7 +68,7 @@ func (r *repo) GetDependentOperationalIntents(ctx context.Context, subscriptionI return r.consensus.HandleReadRequest(ctx, getDependentOperationalIntents, buf) } -func (r *repo) ListExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]*scdmodels.OperationalIntent, error) { +func (r *repo) ListExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) { buf, err := json.Marshal(threshold) if err != nil { return nil, stacktrace.Propagate(err, "failed to marshal payload") @@ -76,6 +77,10 @@ func (r *repo) ListExpiredOperationalIntents(ctx context.Context, threshold time return r.consensus.HandleReadRequest(ctx, listExpiredOperationalIntents, buf) } +func (r *repo) DeleteExpiredOperationalIntents(_ context.Context, threshold time.Time) ([]dssmodels.ID, error) { + return nil, stacktrace.NewErrorWithCode(dsserr.NotImplemented, "DeleteExpiredOperationalIntents not implemented for raftstore") +} + func (r *repo) CountOperationalIntents(ctx context.Context) (int64, error) { return r.consensus.HandleReadRequest(ctx, countOperationalIntents, nil) } diff --git a/pkg/scd/store/raftstore/subscriptions.go b/pkg/scd/store/raftstore/subscriptions.go index 2b5264dce..c8dcc4f6e 100644 --- a/pkg/scd/store/raftstore/subscriptions.go +++ b/pkg/scd/store/raftstore/subscriptions.go @@ -6,6 +6,7 @@ import ( "time" "github.com/golang/geo/s2" + dsserr "github.com/interuss/dss/pkg/errors" dssmodels "github.com/interuss/dss/pkg/models" "github.com/interuss/dss/pkg/raftstore/consensus" scdmodels "github.com/interuss/dss/pkg/scd/models" @@ -19,7 +20,7 @@ const ( deleteSubscription consensus.RequestType[any] = "deleteSubscription" incrementNotificationIndicesForOperationalIntents consensus.RequestType[[]*scdmodels.Subscription] = "incrementNotificationIndicesForOperationalIntents" incrementNotificationIndicesForConstraints consensus.RequestType[[]*scdmodels.Subscription] = "incrementNotificationIndicesForConstraints" - listExpiredSubscriptions consensus.RequestType[[]*scdmodels.Subscription] = "listExpiredSubscriptions" + listExpiredSubscriptions consensus.RequestType[[]dssmodels.ID] = "listExpiredSubscriptions" countSubscriptions consensus.RequestType[int64] = "countSubscriptions" ) @@ -83,7 +84,7 @@ func (r *repo) LockSubscriptionsOnCells(_ context.Context, _ s2.CellUnion, _ []d return nil } -func (r *repo) ListExpiredSubscriptions(ctx context.Context, threshold time.Time) ([]*scdmodels.Subscription, error) { +func (r *repo) ListExpiredSubscriptions(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) { buf, err := json.Marshal(threshold) if err != nil { return nil, stacktrace.Propagate(err, "failed to marshal payload") @@ -92,6 +93,10 @@ func (r *repo) ListExpiredSubscriptions(ctx context.Context, threshold time.Time return r.consensus.HandleReadRequest(ctx, listExpiredSubscriptions, buf) } +func (r *repo) DeleteExpiredSubscriptions(_ context.Context, threshold time.Time) ([]dssmodels.ID, error) { + return nil, stacktrace.NewErrorWithCode(dsserr.NotImplemented, "DeleteExpiredSubscriptions not implemented for raftstore") +} + func (r *repo) CountSubscriptions(ctx context.Context) (int64, error) { return r.consensus.HandleReadRequest(ctx, countSubscriptions, nil) } diff --git a/pkg/scd/store/sqlstore/operational_intents.go b/pkg/scd/store/sqlstore/operational_intents.go index 3d62ce165..a1f5d73b6 100644 --- a/pkg/scd/store/sqlstore/operational_intents.go +++ b/pkg/scd/store/sqlstore/operational_intents.go @@ -350,32 +350,44 @@ func (s *repo) GetDependentOperationalIntents(ctx context.Context, subscriptionI return dependentOps, nil } -// ListExpiredOperationalIntents lists all operational intents older than the threshold. +// ListExpiredOperationalIntents lists the IDs of all operational intents older than the threshold. // Their age is determined by their end time, or by their last update time if they do not have an end time. -func (s *repo) ListExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]*scdmodels.OperationalIntent, error) { - expiredOpIntentsQuery := fmt.Sprintf(` +func (s *repo) ListExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) { + expiredOpIntentsQuery := ` SELECT - %s, scd_uss_availability.availability + id FROM scd_operations - LEFT JOIN scd_uss_availability - ON scd_operations.owner = scd_uss_availability.id WHERE - scd_operations.ends_at IS NOT NULL AND scd_operations.ends_at <= $1 + ends_at IS NOT NULL AND ends_at <= $1 OR - scd_operations.ends_at IS NULL AND scd_operations.updated_at <= $1 -- use last update time as reference if there is no end time - LIMIT $2`, operationFieldsWithPrefix) + ends_at IS NULL AND updated_at <= $1 -- use last update time as reference if there is no end time` - result, err := s.fetchOperationalIntents( - ctx, s.q, expiredOpIntentsQuery, - threshold, - dssmodels.MaxResultLimit, - ) + ids, err := dsssql.FetchIDs(ctx, s.q, expiredOpIntentsQuery, threshold) if err != nil { return nil, stacktrace.Propagate(err, "Error fetching Operations") } - return result, nil + return ids, nil +} + +// DeleteExpiredOperationalIntents deletes all expired operational intents and returns the IDs of the deleted operational intents. +// Age is determined by their end time, or by their update time if they do not have an end time. +func (s *repo) DeleteExpiredOperationalIntents(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) { + deleteExpiredQuery := ` + DELETE FROM scd_operations + WHERE + (ends_at IS NOT NULL AND ends_at <= $1) + OR + (ends_at IS NULL AND updated_at <= $1) + RETURNING id` + + ids, err := dsssql.FetchIDs(ctx, s.q, deleteExpiredQuery, threshold) + if err != nil { + return nil, stacktrace.Propagate(err, "Error deleting expired Operations") + } + + return ids, nil } func (s *repo) CountOperationalIntents(ctx context.Context) (int64, error) { diff --git a/pkg/scd/store/sqlstore/operational_intents_test.go b/pkg/scd/store/sqlstore/operational_intents_test.go index 123f75750..dd6486cf7 100644 --- a/pkg/scd/store/sqlstore/operational_intents_test.go +++ b/pkg/scd/store/sqlstore/operational_intents_test.go @@ -136,12 +136,7 @@ func TestListExpiredOperationalIntents(t *testing.T) { threshold := testCase.timeRef.Add(-testCase.ttl) expired, err := r.ListExpiredOperationalIntents(ctx, threshold) require.NoError(t, err) - - expiredIDs := make([]models.ID, 0, len(expired)) - for _, expiredOi := range expired { - expiredIDs = append(expiredIDs, expiredOi.ID) - } - require.ElementsMatch(t, expiredIDs, testCase.expired) + require.ElementsMatch(t, expired, testCase.expired) }) } } diff --git a/pkg/scd/store/sqlstore/subscriptions.go b/pkg/scd/store/sqlstore/subscriptions.go index 9d85c988e..9611dc3b9 100644 --- a/pkg/scd/store/sqlstore/subscriptions.go +++ b/pkg/scd/store/sqlstore/subscriptions.go @@ -555,31 +555,44 @@ func (c *repo) LockSubscriptionsOnCells(ctx context.Context, cells s2.CellUnion, return nil } -// ListExpiredSubscriptions lists all subscriptions older than the threshold. +// ListExpiredSubscriptions lists the IDs of all subscriptions older than the threshold. // Their age is determined by their end time, or by their update time if they do not have an end time. -func (c *repo) ListExpiredSubscriptions(ctx context.Context, threshold time.Time) ([]*scdmodels.Subscription, error) { - expiredSubsQuery := fmt.Sprintf(` +func (c *repo) ListExpiredSubscriptions(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) { + expiredSubsQuery := ` SELECT - %s + id FROM scd_subscriptions WHERE - scd_subscriptions.ends_at IS NOT NULL AND scd_subscriptions.ends_at <= $1 + ends_at IS NOT NULL AND ends_at <= $1 OR - scd_subscriptions.ends_at IS NULL AND scd_subscriptions.updated_at <= $1 -- use last update time as reference if there is no end time - LIMIT $2`, subscriptionFieldsWithPrefix) + ends_at IS NULL AND updated_at <= $1 -- use last update time as reference if there is no end time` - subscriptions, err := c.fetchSubscriptions( - ctx, c.q, expiredSubsQuery, - threshold, - dssmodels.MaxResultLimit, - ) + ids, err := dsssql.FetchIDs(ctx, c.q, expiredSubsQuery, threshold) if err != nil { return nil, stacktrace.Propagate(err, "Unable to fetch Subscriptions") } - return subscriptions, nil + return ids, nil +} + +// DeleteExpiredSubscriptions deletes all expired subscriptions and returns the IDs of the deleted subscriptions. +// Age is determined by their end time, or by their update time if they do not have an end time. +func (c *repo) DeleteExpiredSubscriptions(ctx context.Context, threshold time.Time) ([]dssmodels.ID, error) { + deleteExpiredQuery := ` + DELETE FROM scd_subscriptions + WHERE + (ends_at IS NOT NULL AND ends_at <= $1) + OR + (ends_at IS NULL AND updated_at <= $1) + RETURNING id` + + ids, err := dsssql.FetchIDs(ctx, c.q, deleteExpiredQuery, threshold) + if err != nil { + return nil, stacktrace.Propagate(err, "Unable to delete expired Subscriptions") + } + return ids, nil } func (c *repo) CountSubscriptions(ctx context.Context) (int64, error) { diff --git a/pkg/scd/store/sqlstore/subscriptions_test.go b/pkg/scd/store/sqlstore/subscriptions_test.go index 1cc9078bc..f1b0ca524 100644 --- a/pkg/scd/store/sqlstore/subscriptions_test.go +++ b/pkg/scd/store/sqlstore/subscriptions_test.go @@ -114,12 +114,7 @@ func TestListExpiredSubscriptions(t *testing.T) { threshold := testCase.timeRef.Add(-testCase.ttl) expired, err := r.ListExpiredSubscriptions(ctx, threshold) require.NoError(t, err) - - expiredIDs := make([]models.ID, 0, len(expired)) - for _, expiredSub := range expired { - expiredIDs = append(expiredIDs, expiredSub.ID) - } - require.ElementsMatch(t, expiredIDs, testCase.expired) + require.ElementsMatch(t, expired, testCase.expired) }) } } diff --git a/pkg/sql/sql.go b/pkg/sql/sql.go index 6f1f0dce5..aa3e55267 100644 --- a/pkg/sql/sql.go +++ b/pkg/sql/sql.go @@ -2,6 +2,9 @@ package sql import ( "context" + + dssmodels "github.com/interuss/dss/pkg/models" + "github.com/interuss/stacktrace" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" ) @@ -12,3 +15,16 @@ type Queryable interface { QueryRow(ctx context.Context, query string, args ...interface{}) pgx.Row Exec(ctx context.Context, query string, args ...interface{}) (pgconn.CommandTag, error) } + +// FetchIDs runs a query returning a single ID column and returns the IDs of all rows. +func FetchIDs(ctx context.Context, q Queryable, query string, args ...interface{}) ([]dssmodels.ID, error) { + rows, err := q.Query(ctx, query, args...) + if err != nil { + return nil, stacktrace.Propagate(err, "Error in query: %s", query) + } + ids, err := pgx.CollectRows(rows, pgx.RowTo[dssmodels.ID]) + if err != nil { + return nil, stacktrace.Propagate(err, "Error scanning IDs") + } + return ids, nil +}