Skip to content

introduce soft-deletion of commitments #829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wagnerd3 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

introduce soft-deletion of commitments #829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 8 additions & 6 deletions internal/api/commitment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
`))

Expand All @@ -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}}
`))

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Copy link
Member

@SuperSandro2000 SuperSandro2000 Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
http.Error(w, "expired, superseded and deleted commitments cannot be transferred", http.StatusBadRequest)
http.Error(w, "expired, superseded or deleted commitments cannot be transferred", http.StatusBadRequest)

return
}

Expand Down Expand Up @@ -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
Expand Down
11 changes: 9 additions & 2 deletions internal/api/commitment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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{
Expand Down
4 changes: 2 additions & 2 deletions internal/collector/capacity_scrape.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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").
Copy link
Member

@SuperSandro2000 SuperSandro2000 Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// reached one of the final statuses ("superseded", "expired" and "deleted").
// reached one of the final statuses ("superseded", "expired" or "deleted").

//
// The result of this computation is used in all bulk queries on
// project_commitments to replace lengthy and time-dependent conditions with
Expand All @@ -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
Expand Down
18 changes: 14 additions & 4 deletions internal/collector/commitment_cleanup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
32 changes: 32 additions & 0 deletions internal/collector/commitment_cleanup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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;
`)
}
4 changes: 2 additions & 2 deletions internal/collector/keystone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}})
`))
)

Expand Down
2 changes: 1 addition & 1 deletion internal/collector/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion internal/datamodel/consume_transferable_commitments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
`))

Expand Down
3 changes: 3 additions & 0 deletions internal/db/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
Expand Down
6 changes: 6 additions & 0 deletions internal/db/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
`,
}
1 change: 1 addition & 0 deletions internal/db/models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions internal/util/datatypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"github.com/sapcc/go-api-declarations/limes"
"github.com/sapcc/go-api-declarations/liquid"
)

////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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"