Add new capacity resource by SuperSandro2000 · Pull Request #710 · sapcc/keppel

SuperSandro2000 · 2026-05-05T14:59:51Z

No description provided.

Copilot

Pull request overview

This PR introduces an optional “bytes/capacity” quota alongside the existing manifest-count quota, wiring it through the quota APIs and enforcing it during blob upload initiation when enabled via configuration.

Changes:

Add a new bytes quota field to the quota model and database schema (migration 054).
Extend the Quotas APIs (Keppel-native + LIQUID) to expose/accept the optional bytes quota and report bytes usage when enabled.
Enforce bytes quota in the registry upload start endpoint when TrackBytesQuota is enabled; add test coverage for the “bytes quota provided but feature disabled” error case.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
internal/test/setup.go	Adds a test setup option to enable bytes quota tracking in config.
internal/processor/quotas.go	Adds bytes quota fields to request/response and introduces bytes-usage/quota logic.
internal/models/quotas.go	Extends `Quotas` DB/JSON model with a `Bytes` field.
internal/keppel/database.go	Adds DB migration to introduce `quotas.bytes` column.
internal/keppel/config.go	Adds `TrackBytesQuota` config flag sourced from `KEPPEL_TRACK_BYTES_QUOTA`.
internal/api/registry/uploads.go	Enforces bytes quota (when enabled) before starting a blob upload.
internal/api/keppel/quotas.go	Threads request context into quota processor calls.
internal/api/keppel/quotas_test.go	Adds tests for rejecting bytes quota updates when the feature is disabled.
internal/api/keppel/liquid.go	Adds LIQUID “capacity” resource and maps it to the bytes quota field.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wagnerd3

The point about the naming applies in many cases, I stopped commenting now. Though I understand that my POV is very billing-focused, I can see that a keppel-developer maybe understands the bytes naming better? Probably 2nd opinion from @majewsky would help.

majewsky · 2026-05-07T12:06:20Z

 type Quotas struct {
 	AuthTenantID  string `db:"auth_tenant_id" json:"-"`
+	Bytes         uint64 `db:"bytes" json:"bytes,omitempty"`
 	ManifestCount uint64 `db:"manifests" json:"manifests"`
 }


Can we get rid of these json:"..." tags? As in, does it break any tests if we do? I'm assuming that this used to be relevant because those records were directly rendered on the quota GET endpoint, but now there is the indirection of processor.QuotaResponse.

The AuditQuotas type is still using this type and it breaks test if removed.

--- FAIL: TestQuotasAPIWithBytes (0.18s) quotas_test.go:358: value mismatch at /events/0/target/attachments/0/content: expected "{\"manifests\":0}", but got "{\"AuthTenantID\":\"tenant1\",\"Bytes\":0,\"ManifestCount\":0}" quotas_test.go:358: value mismatch at /events/0/target/attachments/1/content: expected "{\"bytes\":5000000,\"manifests\":50}", but got "{\"AuthTenantID\":\"tenant1\",\"Bytes\":5000000,\"ManifestCount\":50}" quotas_test.go:400: value mismatch at /events/0/target/attachments/0/content: expected "{\"bytes\":5000000,\"manifests\":50}", but got "{\"AuthTenantID\":\"tenant1\",\"Bytes\":5000000,\"ManifestCount\":50}" quotas_test.go:400: value mismatch at /events/0/target/attachments/1/content: expected "{\"bytes\":50000000,\"manifests\":100}", but got "{\"AuthTenantID\":\"tenant1\",\"Bytes\":50000000,\"ManifestCount\":100}" 2026/05/08 17:25:38 REQUEST: - - "GET https://trivy.example.org/trivy?format=spdx-json&image=registry.example.org%2Ftest1%2Ffoo%40sha256%3Ae3c1e46560a7ce30e3d107791e1f60a588eda9554564a5d17aa365e53dd6ae58 HTTP/1.1" 200 24970 "-" "-" 0.002s --- FAIL: TestQuotasAPI (0.11s) quotas_test.go:138: value mismatch at /events/0/target/attachments/0/content: expected "{\"manifests\":0}", but got "{\"AuthTenantID\":\"tenant1\",\"Bytes\":0,\"ManifestCount\":0}" quotas_test.go:138: value mismatch at /events/0/target/attachments/1/content: expected "{\"manifests\":50}", but got "{\"AuthTenantID\":\"tenant1\",\"Bytes\":0,\"ManifestCount\":50}" quotas_test.go:179: value mismatch at /events/0/target/attachments/0/content: expected "{\"manifests\":50}", but got "{\"AuthTenantID\":\"tenant1\",\"Bytes\":0,\"ManifestCount\":50}" quotas_test.go:179: value mismatch at /events/0/target/attachments/1/content: expected "{\"manifests\":100}", but got "{\"AuthTenantID\":\"tenant1\",\"Bytes\":0,\"ManifestCount\":100}" FAIL

SuperSandro2000 · 2026-05-08T15:19:06Z

The point about the naming applies in many cases, I stopped commenting now. Though I understand that my POV is very billing-focused, I can see that a keppel-developer maybe understands the bytes naming better? Probably 2nd opinion from @majewsky would help.

See https://github.wdf.sap.corp/sap-cloud-infrastructure/storage-resource-services/issues/657 for the naming. I probably mixed some cases up though 😅

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

SuperSandro2000 · 2026-05-08T16:18:15Z

+var liquidInfoVersion int64 = time.Now().Unix()



@wagnerd3 @majewsky I do not know if that is a concern if we spawn multiple API pods.

Oh yeah, that is indeed a problem. We will have to think about this. The obvious solution would be to deploy the LIQUID API as a standalone process that does not allow for horizontal scaling, but maybe we can come up with something that does not increase deployment complexity.

SuperSandro2000 · 2026-05-08T16:12:17Z

+	if a.cfg.TrackBytesQuota {
+		bytesUsage, err := a.sd.UsedBytes(r.Context(), account.AuthTenantID)
+		if respondWithError(w, r, err) {
+			return
+		}
+		if bytesUsage >= quotas.Bytes {
+			msg := fmt.Sprintf("bytes quota exceeded (quota = %d, usage = %d)", quotas.Bytes, bytesUsage)
+			keppel.ErrDenied.With(msg).WithStatus(http.StatusConflict).WriteAsRegistryV2ResponseTo(w, r)
+			return
+		}


@majewsky how do we want to do the activation? If we enable the feature, limes will not instantly be able to set it everywhere. Do we do the migration via a SQL query?

OpenStack often uses a default of -1 to mean unlimited, and then in practice Limes overrides that as soon as it assigns quota. This would allow for a non-disruptive rollout.

Since I don't want to go into the negative-numbers territory, we could default to math.MaxUint64 (or maybe math.MaxInt64 to not overflow in case of int64 conversions).

github-actions · 2026-05-08T16:20:38Z

Merging this branch changes the coverage (2 decrease, 2 increase)

Impacted Packages	Coverage Δ	🤖
github.com/sapcc/keppel/internal/api/keppel	83.23% (+0.80%)	👍
github.com/sapcc/keppel/internal/api/registry	86.12% (-0.33%)	👎
github.com/sapcc/keppel/internal/keppel	84.90% (ø)
github.com/sapcc/keppel/internal/models	93.07% (ø)
github.com/sapcc/keppel/internal/processor	83.79% (+0.36%)	👍
github.com/sapcc/keppel/internal/test	86.41% (-0.03%)	👎

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/sapcc/keppel/internal/api/keppel/liquid.go	78.00% (+16.46%)	550 (+121)	429 (+165)	121 (-44)	🎉
github.com/sapcc/keppel/internal/api/keppel/quotas.go	88.00% (-4.00%)	275	242 (-11)	33 (+11)	👎
github.com/sapcc/keppel/internal/api/registry/uploads.go	86.23% (-0.77%)	4235 (+88)	3652 (+44)	583 (+44)	👎
github.com/sapcc/keppel/internal/keppel/config.go	32.35% (ø)	748	242	506
github.com/sapcc/keppel/internal/keppel/database.go	100.00% (ø)	231	231	0
github.com/sapcc/keppel/internal/models/quotas.go	100.00% (ø)	11	11	0
github.com/sapcc/keppel/internal/processor/quotas.go	85.92% (+5.43%)	426 (+180)	366 (+168)	60 (+12)	👍
github.com/sapcc/keppel/internal/test/setup.go	95.24% (-0.60%)	756 (+36)	720 (+30)	36 (+6)	👎

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

github.com/sapcc/keppel/internal/api/keppel/quotas_test.go

SuperSandro2000 requested a review from Copilot May 5, 2026 15:00

Copilot started reviewing on behalf of SuperSandro2000 May 5, 2026 15:00 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread internal/processor/quotas.go

Comment thread internal/api/registry/uploads.go

Comment thread internal/api/keppel/liquid.go

Comment thread internal/api/keppel/quotas_test.go

Comment thread internal/api/registry/uploads.go

SuperSandro2000 force-pushed the bytes-quota branch 3 times, most recently from 6914805 to 5ba472c Compare May 5, 2026 17:41

SuperSandro2000 marked this pull request as ready for review May 5, 2026 17:41

SuperSandro2000 requested a review from a team as a code owner May 5, 2026 17:41

SuperSandro2000 force-pushed the bytes-quota branch from 5ba472c to 10245f0 Compare May 5, 2026 17:42

wagnerd3 requested changes May 6, 2026

View reviewed changes

Comment thread internal/api/keppel/liquid.go

Comment thread internal/api/keppel/quotas_test.go

Comment thread internal/api/keppel/quotas_test.go

Comment thread internal/test/setup.go

Comment thread internal/api/keppel/liquid.go

majewsky requested changes May 7, 2026

View reviewed changes

SuperSandro2000 force-pushed the bytes-quota branch from 10245f0 to 45de8a3 Compare May 8, 2026 15:28

SuperSandro2000 added 4 commits May 8, 2026 17:28

Add new capacity resource

3bbbf41

Resolve easy todo to check for audit events

11244ed

Bump liquid info version and test /liquid/v1/info endpoint

f68be3e

Switch to standard helpers for uploading

75c0b21

SuperSandro2000 requested review from majewsky and wagnerd3 May 8, 2026 15:30

SuperSandro2000 force-pushed the bytes-quota branch from 45de8a3 to 2c1dbe3 Compare May 8, 2026 15:41

SuperSandro2000 requested a review from Copilot May 8, 2026 15:49

Copilot started reviewing on behalf of SuperSandro2000 May 8, 2026 15:50 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

SuperSandro2000 added 3 commits May 8, 2026 18:10

Handle the absence of images when doing a resource request as an error

2dea540

Add quotas test with bytes enabled

6260eb1

Expand quota test coverage

5ddfd6c

SuperSandro2000 force-pushed the bytes-quota branch from 2c1dbe3 to 5ddfd6c Compare May 8, 2026 16:17

Conversation

SuperSandro2000 commented May 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wagnerd3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

majewsky May 7, 2026

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SuperSandro2000 commented May 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

SuperSandro2000 May 8, 2026

Choose a reason for hiding this comment

Uh oh!

majewsky May 8, 2026

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 May 8, 2026

Choose a reason for hiding this comment

Uh oh!

majewsky May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 8, 2026

Merging this branch changes the coverage (2 decrease, 2 increase)

Changed files (no unit tests)

Changed unit test files

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants