From 61091a2cc2b47ecf67b81866ed41305e2408acfa Mon Sep 17 00:00:00 2001 From: Daniel Wagner Date: Mon, 2 Feb 2026 10:47:47 +0100 Subject: [PATCH] introduce soft-deletion of commitments --- internal/api/commitment.go | 14 ++++---- internal/api/commitment_test.go | 11 +++++-- internal/collector/capacity_scrape.go | 4 +-- internal/collector/commitment_cleanup.go | 18 ++++++++--- internal/collector/commitment_cleanup_test.go | 32 +++++++++++++++++++ internal/collector/keystone.go | 4 +-- internal/collector/metrics.go | 2 +- .../consume_transferable_commitments.go | 2 +- internal/db/builder.go | 3 ++ internal/db/migrations.go | 6 ++++ internal/db/models.go | 1 + internal/util/datatypes.go | 7 ++++ 12 files changed, 86 insertions(+), 18 deletions(-) diff --git a/internal/api/commitment.go b/internal/api/commitment.go index ac1d5366c..11ea76af6 100644 --- a/internal/api/commitment.go +++ b/internal/api/commitment.go @@ -45,7 +45,7 @@ var ( JOIN az_resources azr ON pc.az_resource_id = azr.id JOIN resources r ON azr.resource_id = r.id {{AND r.name = $resource_name}} JOIN services s ON r.service_id = s.id {{AND s.type = $service_type}} - WHERE %s AND pc.status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}) + WHERE %s AND pc.status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}, {{util.CommitmentStatusDeleted}}) ORDER BY pc.id `)) @@ -64,7 +64,7 @@ var ( JOIN az_resources azr ON pc.az_resource_id = azr.id JOIN resources r ON azr.resource_id = r.id WHERE r.path = $1 - AND pc.status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}) + AND pc.status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}, {{util.CommitmentStatusDeleted}}) AND pc.transfer_status = {{limesresources.CommitmentTransferStatusPublic}} `)) @@ -1161,7 +1161,9 @@ func (p *v1Provider) DeleteProjectCommitment(w http.ResponseWriter, r *http.Requ } // perform deletion - _, err = p.DB.Delete(&dbCommitment) + dbCommitment.Status = util.CommitmentStatusDeleted + dbCommitment.DeletedAt = Some(p.timeNow()) + _, err = p.DB.Update(&dbCommitment) if respondwith.ObfuscatedErrorText(w, err) { return } @@ -1242,8 +1244,8 @@ func (p *v1Provider) StartCommitmentTransfer(w http.ResponseWriter, r *http.Requ if req.TransferStatus != limesresources.CommitmentTransferStatusNone { // In order to prevent confusion, only commitments in a certain status can be marked as transferable. - if slices.Contains([]liquid.CommitmentStatus{liquid.CommitmentStatusSuperseded, liquid.CommitmentStatusExpired}, dbCommitment.Status) { - http.Error(w, "expired or superseded commitments cannot be transferred", http.StatusBadRequest) + if slices.Contains([]liquid.CommitmentStatus{liquid.CommitmentStatusSuperseded, liquid.CommitmentStatusExpired, util.CommitmentStatusDeleted}, dbCommitment.Status) { + http.Error(w, "expired, superseded and deleted commitments cannot be transferred", http.StatusBadRequest) return } @@ -2143,7 +2145,7 @@ func (p *v1Provider) UpdateCommitmentDuration(w http.ResponseWriter, r *http.Req return } - if dbCommitment.Status == liquid.CommitmentStatusSuperseded { + if slices.Contains([]liquid.CommitmentStatus{liquid.CommitmentStatusSuperseded, util.CommitmentStatusDeleted}, dbCommitment.Status) { msg := fmt.Sprintf("unable to operate on commitment with a status of %s", dbCommitment.Status) http.Error(w, msg, http.StatusForbidden) return diff --git a/internal/api/commitment_test.go b/internal/api/commitment_test.go index 998e55cb3..52ed53241 100644 --- a/internal/api/commitment_test.go +++ b/internal/api/commitment_test.go @@ -22,6 +22,7 @@ import ( "github.com/sapcc/limes/internal/datamodel" "github.com/sapcc/limes/internal/db" "github.com/sapcc/limes/internal/test" + "github.com/sapcc/limes/internal/util" . "github.com/majewsky/gg/option" ) @@ -487,7 +488,7 @@ func TestCommitmentLifecycleWithDelayedConfirmation(t *testing.T) { ExpectBody: assert.JSONObject{"commitments": []assert.JSONObject{}}, }.Check(t, s.Handler) - // commitments can be deleted with sufficient privilege + // commitments can be soft-deleted with sufficient privilege s.TokenValidator.Enforcer.AllowUncommit = true assert.HTTPRequest{ Method: http.MethodDelete, @@ -516,6 +517,11 @@ func TestCommitmentLifecycleWithDelayedConfirmation(t *testing.T) { }, }, }) + var deletedCommitment db.ProjectCommitment + must.SucceedT(t, s.DB.SelectOne(&deletedCommitment, `SELECT * FROM project_commitments where ID = 2`)) + assert.Equal(t, deletedCommitment.Status, util.CommitmentStatusDeleted) + assert.Equal(t, deletedCommitment.DeletedAt.IsSome(), true) + must.ReturnT(s.DB.Delete(&deletedCommitment)) s.TokenValidator.Enforcer.AllowUncommit = false assert.HTTPRequest{ @@ -606,6 +612,7 @@ func TestCommitmentLifecycleWithDelayedConfirmation(t *testing.T) { }, }, }) + s.MustDBExec(`DELETE FROM project_commitments WHERE ID = $1`, 3) // confirm the remaining commitment s.Clock.StepBy(1 * time.Hour) @@ -2679,7 +2686,7 @@ func Test_MergeCommitments(t *testing.T) { s.MustDBExec("UPDATE project_commitments SET transfer_status = $1 WHERE id = 2", limesresources.CommitmentTransferStatusNone) // Do not merge commitments with statuses other than "active" - unmergeableStatuses := []liquid.CommitmentStatus{liquid.CommitmentStatusPlanned, liquid.CommitmentStatusPending, liquid.CommitmentStatusSuperseded, liquid.CommitmentStatusExpired} + unmergeableStatuses := []liquid.CommitmentStatus{liquid.CommitmentStatusPlanned, liquid.CommitmentStatusPending, liquid.CommitmentStatusSuperseded, liquid.CommitmentStatusExpired, util.CommitmentStatusDeleted} for _, status := range unmergeableStatuses { s.MustDBExec("UPDATE project_commitments SET status = $1 WHERE id = 2", status) assert.HTTPRequest{ diff --git a/internal/collector/capacity_scrape.go b/internal/collector/capacity_scrape.go index db7c6ee72..f8d45ff44 100644 --- a/internal/collector/capacity_scrape.go +++ b/internal/collector/capacity_scrape.go @@ -70,7 +70,7 @@ var ( `) // This query updates `project_commitments.status` on all rows that have not - // reached one of the final statuses ("superseded" and "expired"). + // reached one of the final statuses ("superseded", "expired" and "deleted"). // // The result of this computation is used in all bulk queries on // project_commitments to replace lengthy and time-dependent conditions with @@ -90,7 +90,7 @@ var ( ELSE transfer_token END, transfer_started_at = CASE WHEN superseded_at IS NOT NULL OR expires_at <= $3 THEN NULL ELSE transfer_started_at END - WHERE status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}) AND az_resource_id IN ( + WHERE status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}, {{util.CommitmentStatusDeleted}}) AND az_resource_id IN ( SELECT azr.id FROM services s JOIN resources r ON r.service_id = s.id diff --git a/internal/collector/commitment_cleanup.go b/internal/collector/commitment_cleanup.go index 3c10d5a1c..bca80a7d4 100644 --- a/internal/collector/commitment_cleanup.go +++ b/internal/collector/commitment_cleanup.go @@ -41,12 +41,16 @@ var ( transfer_status = {{limesresources.CommitmentTransferStatusNone}}, transfer_token = NULL, transfer_started_at = NULL - WHERE status != {{liquid.CommitmentStatusSuperseded}} AND expires_at <= $1 + WHERE status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{util.CommitmentStatusDeleted}}) AND expires_at <= $1 `)) - expiredCommitmentsCleanupQuery = sqlext.SimplifyWhitespace(` + hardDeleteCommitmentsQuery = sqlext.SimplifyWhitespace(db.ExpandEnumPlaceholders(` + DELETE FROM project_commitments + WHERE status = {{util.CommitmentStatusDeleted}} AND deleted_at + interval '6 months' <= $1 + `)) + expiredCommitmentsCleanupQuery = sqlext.SimplifyWhitespace(db.ExpandEnumPlaceholders(` DELETE FROM project_commitments pc - WHERE expires_at + interval '1 month' <= $1 - `) + WHERE status != {{util.CommitmentStatusDeleted}} AND expires_at + interval '1 month' <= $1 + `)) ) func (c *Collector) cleanupOldCommitments(_ context.Context, _ prometheus.Labels) error { @@ -71,5 +75,11 @@ func (c *Collector) cleanupOldCommitments(_ context.Context, _ prometheus.Labels return fmt.Errorf("while deleting expired commitments without undeleted successors: %w", err) } + // step 3: hard-delete soft-deleted commitments after a grace period + _, err = c.DB.Exec(hardDeleteCommitmentsQuery, now) + if err != nil { + return fmt.Errorf("while deleting soft-deleted commitments: %w", err) + } + return nil } diff --git a/internal/collector/commitment_cleanup_test.go b/internal/collector/commitment_cleanup_test.go index a11135675..d8eed3091 100644 --- a/internal/collector/commitment_cleanup_test.go +++ b/internal/collector/commitment_cleanup_test.go @@ -18,6 +18,7 @@ import ( "github.com/sapcc/limes/internal/collector" "github.com/sapcc/limes/internal/db" "github.com/sapcc/limes/internal/test" + "github.com/sapcc/limes/internal/util" ) const ( @@ -103,6 +104,8 @@ func TestCleanupOldCommitmentsJob(t *testing.T) { oneDay := 24 * time.Hour commitmentForOneDay, err := limesresources.ParseCommitmentDuration("1 day") must.SucceedT(t, err) + commitmentForTenDays, err := limesresources.ParseCommitmentDuration("10 day") + must.SucceedT(t, err) commitmentForThreeYears, err := limesresources.ParseCommitmentDuration("3 years") must.SucceedT(t, err) @@ -296,4 +299,33 @@ func TestCleanupOldCommitmentsJob(t *testing.T) { DELETE FROM project_commitments WHERE id = 6 AND uuid = '00000000-0000-0000-0000-000000000006' AND transfer_token = NULL; DELETE FROM project_commitments WHERE id = 7 AND uuid = '00000000-0000-0000-0000-000000000007' AND transfer_token = NULL; `) + + // lastly, we test the hard-delete of soft deleted commitments after 6 months + s.MustDBInsert(&db.ProjectCommitment{ + ID: 8, + UUID: "00000000-0000-0000-0000-000000000008", + ProjectID: 1, + AZResourceID: 1, + Amount: 15, + Duration: commitmentForTenDays, + CreatedAt: s.Clock.Now().Add(-2 * oneDay), + ConfirmedAt: Some(s.Clock.Now().Add(-2 * oneDay)), + ExpiresAt: commitmentForTenDays.AddTo(s.Clock.Now().Add(-2 * oneDay)), + Status: util.CommitmentStatusDeleted, + CreationContextJSON: json.RawMessage(buf), + DeletedAt: Some(s.Clock.Now().Add(-oneDay)), + }) + tr.DBChanges().Ignore() + + // some time passes, nothing happens - also this is not picked up by expiration sql statements! + s.Clock.StepBy(150 * oneDay) + must.SucceedT(t, job.ProcessOne(s.Ctx)) + tr.DBChanges().AssertEmpty() + + // now we pass the 6 months mark, hard-delete happens + s.Clock.StepBy(33 * oneDay) + must.SucceedT(t, job.ProcessOne(s.Ctx)) + tr.DBChanges().AssertEqualf(` + DELETE FROM project_commitments WHERE id = 8 AND uuid = '00000000-0000-0000-0000-000000000008' AND transfer_token = NULL; + `) } diff --git a/internal/collector/keystone.go b/internal/collector/keystone.go index 2d05a89a9..76b1503b1 100644 --- a/internal/collector/keystone.go +++ b/internal/collector/keystone.go @@ -257,11 +257,11 @@ func (c *Collector) initProject(domain *db.Domain, project core.KeystoneProject) var ( deleteProjectCheckBlockingCommitmentsQuery = sqlext.SimplifyWhitespace(db.ExpandEnumPlaceholders(` SELECT COUNT(*) FROM project_commitments - WHERE project_id = $1 AND status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}) + WHERE project_id = $1 AND status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}, {{util.CommitmentStatusDeleted}}) `)) deleteProjectRemoveNonblockingCommitmentsQuery = sqlext.SimplifyWhitespace(db.ExpandEnumPlaceholders(` DELETE FROM project_commitments - WHERE project_id = $1 AND status IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}) + WHERE project_id = $1 AND status IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}, {{util.CommitmentStatusDeleted}}) `)) ) diff --git a/internal/collector/metrics.go b/internal/collector/metrics.go index 967bbe54f..f03ad3720 100644 --- a/internal/collector/metrics.go +++ b/internal/collector/metrics.go @@ -452,7 +452,7 @@ var projectAZMetricsQuery = sqlext.SimplifyWhitespace(db.ExpandEnumPlaceholders( WITH project_commitment_sums_by_status AS ( SELECT az_resource_id, project_id, status, SUM(amount) AS amount FROM project_commitments - WHERE status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}) + WHERE status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}, {{util.CommitmentStatusDeleted}}) GROUP BY az_resource_id, project_id, status ), project_commitment_sums AS ( SELECT az_resource_id, project_id, JSON_OBJECT_AGG(status, amount) AS amount_by_status diff --git a/internal/datamodel/consume_transferable_commitments.go b/internal/datamodel/consume_transferable_commitments.go index 5ce1dbfe7..14497ac34 100644 --- a/internal/datamodel/consume_transferable_commitments.go +++ b/internal/datamodel/consume_transferable_commitments.go @@ -38,7 +38,7 @@ var getTransferableCommitmentsQuery = sqlext.SimplifyWhitespace(db.ExpandEnumPla JOIN project_commitments pc ON pc.az_resource_id = azr.id WHERE s.type = $1 AND r.name = $2 AND azr.az = $3 AND pc.transfer_status = {{limesresources.CommitmentTransferStatusPublic}} - AND pc.status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}) + AND pc.status NOT IN ({{liquid.CommitmentStatusSuperseded}}, {{liquid.CommitmentStatusExpired}}, {{util.CommitmentStatusDeleted}}) ORDER BY pc.transfer_started_at ASC, pc.created_at ASC, pc.id ASC `)) diff --git a/internal/db/builder.go b/internal/db/builder.go index ad60319f0..cf1a79007 100644 --- a/internal/db/builder.go +++ b/internal/db/builder.go @@ -10,6 +10,8 @@ import ( limesresources "github.com/sapcc/go-api-declarations/limes/resources" "github.com/sapcc/go-api-declarations/liquid" + + "github.com/sapcc/limes/internal/util" ) // BuildSimpleWhereClause constructs a WHERE clause of the form "field1 = val1 AND @@ -75,6 +77,7 @@ var ( "{{liquid.CommitmentStatusConfirmed}}": enumValueToSQLLiteral(liquid.CommitmentStatusConfirmed), "{{liquid.CommitmentStatusSuperseded}}": enumValueToSQLLiteral(liquid.CommitmentStatusSuperseded), "{{liquid.CommitmentStatusExpired}}": enumValueToSQLLiteral(liquid.CommitmentStatusExpired), + "{{util.CommitmentStatusDeleted}}": enumValueToSQLLiteral(util.CommitmentStatusDeleted), // liquid.Topology "{{liquid.FlatTopology}}": enumValueToSQLLiteral(liquid.FlatTopology), "{{liquid.AZAwareTopology}}": enumValueToSQLLiteral(liquid.AZAwareTopology), diff --git a/internal/db/migrations.go b/internal/db/migrations.go index 82bc6cb32..19002197a 100644 --- a/internal/db/migrations.go +++ b/internal/db/migrations.go @@ -270,4 +270,10 @@ var sqlMigrations = map[string]string{ DROP COLUMN quota, DROP COLUMN backend_quota; `), + "071_add_commitment_deleted_at.down.sql": ` + ALTER TABLE project_commitments DROP COLUMN deleted_at; + `, + "071_add_commitment_deleted_at.up.sql": ` + ALTER TABLE project_commitments ADD COLUMN deleted_at TIMESTAMPTZ DEFAULT NULL; + `, } diff --git a/internal/db/models.go b/internal/db/models.go index 8653fb22d..35d11f0dd 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -172,6 +172,7 @@ type ProjectCommitment struct { ConfirmBy Option[time.Time] `db:"confirm_by"` ConfirmedAt Option[time.Time] `db:"confirmed_at"` ExpiresAt time.Time `db:"expires_at"` + DeletedAt Option[time.Time] `db:"deleted_at"` // Commitments can be superseded due to splits, conversions or merges. // The context columns contain information about the reason and related commitments diff --git a/internal/util/datatypes.go b/internal/util/datatypes.go index 1ea1b4d8a..a568a920f 100644 --- a/internal/util/datatypes.go +++ b/internal/util/datatypes.go @@ -8,6 +8,7 @@ import ( "time" "github.com/sapcc/go-api-declarations/limes" + "github.com/sapcc/go-api-declarations/liquid" ) //////////////////////////////////////////////////////////////////////////////// @@ -44,3 +45,9 @@ func IntoUnixEncodedTime(t time.Time) limes.UnixEncodedTime { func FromUnixEncodedTime(t limes.UnixEncodedTime) time.Time { return t.Time } + +//////////////////////////////////////////////////////////////////////////////// + +// CommitmentStatusDeleted is used for soft-deleting commitments before they get hard-deleted after a grace period. +// It is defined here instead of in the liquid package, because in liquid we model deletions as status=None. +const CommitmentStatusDeleted liquid.CommitmentStatus = "deleted"