Token rate limit reservations#190
Conversation
📝 WalkthroughWalkthroughThis RFC proposes token capacity reservations on request arrival, followed by commitment of actual usage on response. It defines configuration modes, per-counter reservation state, new custom-service RPCs, storage behaviour, rollout wiring, and future mode constraints. ChangesToken Rate-Limit Reservations
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Gateway
participant Limitador
participant Upstream
Gateway->>Limitador: Reserve estimated tokens
Limitador-->>Gateway: Return admission decision
Gateway->>Upstream: Forward admitted request
Upstream-->>Gateway: Return actual token usage
Gateway->>Limitador: Commit actual usage
Limitador-->>Gateway: Remove reservation
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
025c67d to
15b0770
Compare
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
15b0770 to
7da814f
Compare
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
98b81b8 to
6f3a8b9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rfcs/0000-token-rate-limit-reservations.md`:
- Line 7: Replace the placeholder Issue tracking entry in the RFC with the
actual issue URL, or remove the entry entirely if no issue exists. Ensure the
document no longer references issue `#0000`.
- Around line 86-90: Update the fenced code block containing the max_tokens CEL
expression to include a language identifier, preferably cel or text if CEL is
unsupported, while leaving the example content unchanged.
- Around line 73-77: Update the reservation examples and fallback behavior in
the reservation configuration and Reserve/Commit flow so reservation.amount is a
safe upper bound for actual usage, preventing Commit from exceeding the enforced
limit; otherwise explicitly document that enforcement is best-effort. Ensure the
example expression and fallback value do not under-reserve requests, and apply
the same contract to the additional referenced sections.
- Around line 155-169: Align CounterReservationRegistry with per-reservation
expiry by defining a keying or purge strategy that removes individual expired
ReservationEntry records rather than relying on container-level Moka or Redis
expiry. Ensure outstanding(counter) ignores and cleanup removes expired
reservations even while the counter remains active, and update the related TTL
description at the referenced section consistently.
- Around line 222-234: Update the Commit reservation flow and CommitResponse
semantics so retries with the same reservation_id are idempotent: retain durable
terminal state or otherwise distinguish already-committed IDs from
expired/unknown IDs, and do not apply actual_amount again for an
already-committed reservation. Preserve Report-style fallback only for empty,
expired, or genuinely unknown reservation IDs.
- Around line 257-285: Resolve the contradictory reservation flags in the
“Limitador-level opt-in” and “limitador” work items by specifying one flag name
and its default, then align all references to it. Define the
capability-detection or upgrade-sequencing behavior used by the reconciler so
the default CR mode does not emit Reserve/Commit actions until the target
Limitador supports and enables those RPCs, preserving Check/Report during
upgrades or when reservations are disabled.
- Line 12: Clarify the default reservation guarantee in the RFC: because
Reservation mode’s local registry does not synchronize outstanding reservations
for the distributed gossip backend or RocksDbStorage, state that the default
protects concurrency only within a single replica, or change the default so
those backends use CheckReport mode. Update the surrounding description of the
default behavior without altering rejection, headers, or multi-limit semantics.
- Around line 168-170: Define cross-counter transaction semantics for Reserve
and Commit: require a shared lock or equivalent atomic transaction across every
affected counter, perform all capacity checks before creating any reservation
entries, and roll back all changes if any check or write fails. Specify that
Commit atomically updates actual_amount and removes all live reservation
entries, or leaves every counter and reservation unchanged on failure, including
the multi-limit behavior described near lines 241-245.
- Line 96: Update the RFC’s default ttl definition to specify a deterministic
finite fallback when HTTPRoute.spec.rules[].timeouts.backendRequest is absent or
unsuitable, while retaining the route timeout when it is available. Ensure the
default behavior remains well-defined for Gateway TargetRefs spanning routes
with differing or missing timeouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8a2d1e1-d947-4630-8979-6bac8f8b9ee9
📒 Files selected for processing (1)
rfcs/0000-token-rate-limit-reservations.md
| - Status: Draft | ||
| - Start Date: 2026-07-20 | ||
| - RFC PR: [Kuadrant/architecture#190](https://github.com/Kuadrant/architecture/pull/190) | ||
| - Issue tracking: ~[Kuadrant/architecture#0000](https://github.com/Kuadrant/architecture/issues/0000)~ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the placeholder issue-tracking link.
Line 7 points to issue #0000, which is not a usable tracking reference. Link the real issue or remove this entry before merging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rfcs/0000-token-rate-limit-reservations.md` at line 7, Replace the
placeholder Issue tracking entry in the RFC with the actual issue URL, or remove
the entry entirely if no issue exists. Ensure the document no longer references
issue `#0000`.
|
|
||
| # Summary | ||
|
|
||
| `TokenRateLimitPolicy` ([RFC 0013](./0013-ai-policies.md)) checks capacity when a request arrives but only reports actual token usage after the model responds, which lets concurrent in-flight requests race past a limit before any of them report usage. This RFC introduces a **reservation layer** in Limitador: on arrival, an estimated token volume is reserved and held (with a TTL) against a limit's capacity; when the response comes back, the caller commits the reservation with actual usage in a single call, releasing any over-reservation immediately. Rejection semantics, rate-limit headers, and multi-limit all-or-nothing behavior are unchanged. The feature is additive at every layer (new gRPC RPCs, new CRD field, new Limitador flag) and is enabled by default, with an explicit `CheckReport` mode to revert to today's behavior. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files around rfcs:"
git ls-files | rg '^rfcs/0000-token-rate-limit-reservations\.md$|^rfcs/|limitador|reservation|reserv' || true
echo
echo "File slice:"
if [ -f rfcs/0000-token-rate-limit-reservations.md ]; then
wc -l rfcs/0000-token-rate-limit-reservations.md
sed -n '1,320p' rfcs/0000-token-rate-limit-reservations.md | cat -n
fiRepository: Kuadrant/architecture
Length of output: 29535
Clarify the default reservation guarantee.
The default mode is now advertised as automatically closing the concurrency race, but Reservation mode uses a local registry for the distributed gossip backend and for RocksDbStorage. Replicas can therefore still admit against incomplete outstanding state. Either disable default Reservation mode for those backends or reword the default guarantee to reflect single-replica protection only.
🧰 Tools
🪛 LanguageTool
[grammar] ~12-~12: Did you mean “responses”?
Context: ...orts actual token usage after the model responds, which lets concurrent in-flight reques...
(PREPOSITION_VERB)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rfcs/0000-token-rate-limit-reservations.md` at line 12, Clarify the default
reservation guarantee in the RFC: because Reservation mode’s local registry does
not synchronize outstanding reservations for the distributed gossip backend or
RocksDbStorage, state that the default protects concurrency only within a single
replica, or change the default so those backends use CheckReport mode. Update
the surrounding description of the default behavior without altering rejection,
headers, or multi-limit semantics.
| reservation: | ||
| # estimated tokens to hold before the model responds | ||
| amount: "uint(size(requestBodyJSON('/messages')) / 4)" | ||
| # how long to hold it before automatically releasing it | ||
| ttl: "duration('30s')" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Require reservation amounts to be safe upper bounds.
Reserve admits on the estimated amount, while Commit applies actual_amount unconditionally. The example expression and the fallback value of 100 can under-reserve real usage, so concurrent requests can still push a counter beyond its limit. Require reservation.amount to be an upper bound, or explicitly document that enforcement is best-effort.
Also applies to: 80-94, 241-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rfcs/0000-token-rate-limit-reservations.md` around lines 73 - 77, Update the
reservation examples and fallback behavior in the reservation configuration and
Reserve/Commit flow so reservation.amount is a safe upper bound for actual
usage, preventing Commit from exceeding the enforced limit; otherwise explicitly
document that enforcement is best-effort. Ensure the example expression and
fallback value do not under-reserve requests, and apply the same contract to the
additional referenced sections.
| ``` | ||
| has(requestBodyJSON('/max_tokens')) | ||
| ? uint(requestBodyJSON('/max_tokens')) | ||
| : 100 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced CEL example.
The unlabelled fence triggers Markdownlint MD040. Use an appropriate identifier such as cel, or text if the renderer does not support CEL.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 86-86: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rfcs/0000-token-rate-limit-reservations.md` around lines 86 - 90, Update the
fenced code block containing the max_tokens CEL expression to include a language
identifier, preferably cel or text if CEL is unsupported, while leaving the
example content unchanged.
Source: Linters/SAST tools
|
|
||
| Note that the `amount` default assumes `requestBodyJSON('/max_tokens')` is reliably readable at this point, which is itself unresolved (see [Unresolved questions](#unresolved-questions)). If it isn't, this default degrades to the flat fallback constant for every request. | ||
|
|
||
| - **Default `ttl`**: the route's own configured backend-request timeout (Gateway API `HTTPRoute.spec.rules[].timeouts.backendRequest`). A reservation only needs to outlive a request for as long as that request could legitimately still be in flight, and the gateway already has an opinion about that. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate RFC and related lines =="
fd -a '0000-token-rate-limit-reservations\.md$' . || true
file=$(fd '0000-token-rate-limit-reservations\.md$' . | head -n1 || true)
if [ -n "${file:-}" ]; then
echo "FILE=$file"
wc -l "$file"
sed -n '1,160p' "$file" | nl -ba
fi
echo
echo "== search for ttl/default/fallback/backendRequest in RFC =="
if [ -n "${file:-}" ]; then
rg -n -i 'ttl|fallback|backendrequest|backend-request|timeout|duration|reserve|reservation' "$file" || true
fi
echo
echo "== search backendRequest mentions in repo =="
rg -n -i 'backendRequest|backend-request|HTTPRoute.*timeout|timeouts' . || trueRepository: Kuadrant/architecture
Length of output: 396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./rfcs/0000-token-rate-limit-reservations.md"
echo "== RFC excerpt =="
python3 - <<'PY' "$file"
import sys
for line in open(sys.argv[1], encoding="utf-8", newline="\n"):
print(line.rstrip())
PY
echo
echo "== TTL/fallback/timeout/search terms =="
grep -n -i 'ttl\|fallback\|backendrequest\|backend-request\|timeout\|duration\|reserve\|reservation' "$file" || trueRepository: Kuadrant/architecture
Length of output: 42461
🌐 Web query:
Gateway API HTTPRoute spec rules timeouts backendRequest optional timeout documentation
💡 Result:
In the Kubernetes Gateway API, the backendRequest timeout is an optional field within the HTTPRoute rule specification [1][2]. Key details regarding the backendRequest timeout include: Purpose: It defines a timeout for an individual request sent from the Gateway to a backend service. It covers the duration from when the request starts being sent from the gateway until the full response is received from the backend [1][3]. Optionality: The timeouts field (including backendRequest) is optional. If left unspecified, the timeout behavior is implementation-specific [1][4]. Relationship to Request Timeout: Because the overall request timeout encompasses the backendRequest timeout, the value of backendRequest must not exceed the value of the request timeout [1][2]. Value Format: Timeouts are defined as Gateway API Duration strings [2]. A zero-valued timeout (e.g., "0s") must be interpreted as disabling the timeout [1][4]. Any valid non-zero timeout must be at least 1 millisecond [1][3]. Support Level: The configuration of backendRequest (HTTPRouteBackendTimeout) is considered an Extended support feature [2]. When configuring an HTTPRoute, you include the timeout under the rules section as follows: apiVersion: gateway.networking.k8s.io/v1 kind: HTTPRoute metadata: name: timeout-example spec: rules: - timeouts: request: 10s backendRequest: 2s backendRefs: - name: timeout-svc port: 8080
Citations:
- 1: https://gateway-api.sigs.k8s.io/reference/api-types/httproute/
- 2: https://gateway-api.sigs.k8s.io/geps/gep-1742/
- 3: https://gateway.envoyproxy.io/latest/api/gateway_api/httproute/
- 4: https://gateway-api.sigs.k8s.io/api-types/httproute/?h=httprout
Define a deterministic TTL fallback.
HTTPRoute.spec.rules[].timeouts.backendRequest is optional and implementation-specific when omitted, and a TargetRef on Gateway can span routes with different or absent route timeouts. This RFC still says reservations are enabled by default, so it should define a finite fallback value rather than relying on an optional Gateway API field that may be missing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rfcs/0000-token-rate-limit-reservations.md` at line 96, Update the RFC’s
default ttl definition to specify a deterministic finite fallback when
HTTPRoute.spec.rules[].timeouts.backendRequest is absent or unsuitable, while
retaining the route timeout when it is available. Ensure the default behavior
remains well-defined for Gateway TargetRefs spanning routes with differing or
missing timeouts.
| CounterReservationRegistry: CounterKey -> [ReservationEntry] | ||
|
|
||
| // A single outstanding hold against one counter. | ||
| ReservationEntry { | ||
| reservation_id: ReservationId // opaque; the SAME id is stored under every | ||
| // counter this reservation touched, so one | ||
| // token released by the caller settles all | ||
| // of them in a single Commit call | ||
| amount: u64 // estimated tokens reserved, uniform across | ||
| // every counter this reservation touched | ||
| expires_at: Timestamp // see "TTL and expiry" below | ||
| } | ||
|
|
||
| // Capacity available for a counter must now account for outstanding holds: | ||
| outstanding(counter) = sum(e.amount for e in registry[counter] if e.expires_at > now) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Make per-reservation expiry compatible with the registry shape.
The registry is described as CounterKey -> [ReservationEntry], but Moka entry expiry and Redis key-level EXPIRE expire the containing cache entry/hash, not individual reservations. Define per-entry keying or an explicit purge strategy; otherwise expired fields can accumulate whenever a counter remains active.
Also applies to: 191-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rfcs/0000-token-rate-limit-reservations.md` around lines 155 - 169, Align
CounterReservationRegistry with per-reservation expiry by defining a keying or
purge strategy that removes individual expired ReservationEntry records rather
than relying on container-level Moka or Redis expiry. Ensure
outstanding(counter) ignores and cleanup removes expired reservations even while
the counter remains active, and update the related TTL description at the
referenced section consistently.
| // Capacity available for a counter must now account for outstanding holds: | ||
| outstanding(counter) = sum(e.amount for e in registry[counter] if e.expires_at > now) | ||
| admit if counter.value + outstanding(counter) + requested_amount <= limit |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '^rfcs/0000-token-rate-limit-reservations\.md$|0000-token-rate-limit-reservations' || true
if [ -f rfcs/0000-token-rate-limit-reservations.md ]; then
echo "--- Relevant sections ---"
sed -n '140,260p' rfcs/0000-token-rate-limit-reservations.md
fi
echo "--- Atomicity keywords ---"
rg -n "atomic|transaction|rollback|lock|partial|all-or-nothing|all or nothing|Reserve|Commit|multi-limit" rfcs/0000-token-rate-limit-reservations.md || trueRepository: Kuadrant/architecture
Length of output: 16979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Outline/keywords for Reserve/Commit implementation mentions ---"
rg -n "Reserve|Commit|all-or-nothing|all or nothing|atomic|transaction|rollback|lock|multi-limit|outstanding|reservation_id|update_counter|CounterReservationRegistry" . --glob '!target/**' --glob '!build/**' --glob '!dist/**' | head -n 240 || trueRepository: Kuadrant/architecture
Length of output: 30921
Define cross-counter atomicity for reservations.
Reserve stores CounterReservationRegistry[counter] per counter and the RFC says multi-limit behaviour is unchanged, but it does not specify the transaction/locking/rollback semantics needed when a request touches several counters. Define how Reserve keeps the all-or-nothing check-all-then-create-all invariant, and how Commit either updates actual_amount and removes the live reservation entries atomically or fails/rolls back without leaving partial holds or charges; this applies to lines 241-245 as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rfcs/0000-token-rate-limit-reservations.md` around lines 168 - 170, Define
cross-counter transaction semantics for Reserve and Commit: require a shared
lock or equivalent atomic transaction across every affected counter, perform all
capacity checks before creating any reservation entries, and roll back all
changes if any check or write fails. Specify that Commit atomically updates
actual_amount and removes all live reservation entries, or leaves every counter
and reservation unchanged on failure, including the multi-limit behavior
described near lines 241-245.
| message CommitRequest { | ||
| string domain = 1; | ||
| repeated RateLimitDescriptor descriptors = 2; // used to re-resolve counters -- see below | ||
| string reservation_id = 3; // optional; empty behaves like plain Report | ||
| uint64 actual_amount = 4; | ||
| } | ||
|
|
||
| message CommitResponse { | ||
| bool reservation_released = 1; // diagnostic only: false if reservation_id | ||
| // was empty, unknown, or already expired, | ||
| // and actual_amount was applied directly | ||
| // instead (Report-style) -- see below | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make Commit idempotent for a reservation ID.
After a successful commit, a retry with the same ID finds no live reservation and falls back to applying actual_amount again. A lost response can therefore double-count usage. Add durable deduplication/terminal state, or distinguish an already-committed ID from an expired or unknown ID.
Also applies to: 241-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rfcs/0000-token-rate-limit-reservations.md` around lines 222 - 234, Update
the Commit reservation flow and CommitResponse semantics so retries with the
same reservation_id are idempotent: retain durable terminal state or otherwise
distinguish already-committed IDs from expired/unknown IDs, and do not apply
actual_amount again for an already-committed reservation. Preserve Report-style
fallback only for empty, expired, or genuinely unknown reservation IDs.
| ### Limitador-level opt-in | ||
|
|
||
| A server-level flag, `--enable-reservations` (default: `false`, i.e. reservations disabled), independent of anything the Kuadrant CR requests. When not set, `Reserve`/`Commit` are not registered (or return `UNIMPLEMENTED`). This exists as defense-in-depth for operators who want a hard kill-switch regardless of what any CR asks for. | ||
|
|
||
| ## Expected work by component | ||
|
|
||
| **limitador** | ||
|
|
||
| - `ReservationEntry` type and per-counter registry: persisted (dedicated Moka cache for `InMemoryStorage`, hash-field-backed for `RedisStorage`/`AsyncRedisStorage`/`CachedRedisStorage`) or local-memory-only (`RocksDbStorage`, distributed backend), per [Storage backend scope for this RFC](#storage-backend-scope-for-this-rfc). | ||
| - `RateLimiter::reserve` / `RateLimiter::commit_reservation` (and async twins), composed from existing `counters_that_apply` / `update_counter` / admission-check logic. | ||
| - `Reserve` / `Commit` proto messages and RPCs on the existing Kuadrant service, implemented alongside (not replacing) `CheckRateLimit`/`Report`. | ||
| - `--disable-reservations` server flag. | ||
| - Concurrency/race tests exercising the scenario in [Motivation](#motivation) directly. | ||
|
|
||
| **wasm-shim** | ||
|
|
||
| - TBD. How the wasm shim actually issues the request-phase `Reserve` call and the response/stream-end-phase `Commit` call. | ||
| - `ServiceType` gains `RateLimitReserve` / `RateLimitCommit`, additive to the existing open enum (`RateLimit | RateLimitCheck | RateLimitReport | Auth | Tracing | Dynamic`). Existing variants and their handling in `DynamicService`/`DynamicTask` are untouched. | ||
| - `reservation.amount` / `reservation.ttl` are evaluated exactly like `hits_addend` is today: via `message_builder` CEL on the `Reserve` action. | ||
| - The `reservation_id` returned by `Reserve` is captured via an `on_reply` → `Store` action into `ReqRespCtx.stored_values`, the same mechanism already used to carry state between phases of one HTTP transaction . It needs no host export, since it's read back within the same `KuadrantFilter` instance later in the same request. | ||
|
|
||
| **kuadrant-operator** | ||
|
|
||
| - `Kuadrant` CR field (`spec.tokenRateLimiting.mode`, enum `Reservation`/`CheckReport`) and `TokenRateLimitPolicy` `reservation` field (`amount`, `ttl` CEL expressions), including defaults/overrides merge behavior. | ||
| - Default `amount`/`ttl` CEL generation for limits that omit `reservation` | ||
| - Reconciler branch generating `Reserve`/`Commit` vs `Check`/`Report` wasm actions based on effective state. | ||
| - **Effectively active**: generate a `Reserve` action (new `ServiceType::RateLimitReserve`) at request phase, with a `message_builder` evaluating `reservation.amount`/`reservation.ttl`, and a `Commit` action (new `ServiceType::RateLimitCommit`) at response/stream-end phase. | ||
| - **Effectively inactive**: generate exactly what is generated today, i.e. `ratelimit-check-service` (`hits_addend=0`) + `ratelimit-report-service` (`hits_addend=<actual>`). | ||
| - Docs and e2e tests covering both modes and the upgrade-window transition. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve the feature-flag and upgrade-window contract.
Line 259 defines --enable-reservations with a default of false, while Line 268 requires --disable-reservations; meanwhile the default CR mode emits Reserve/Commit. Specify one flag and default, and define capability detection or upgrade sequencing so the operator never emits new RPCs to an older or disabled Limitador.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~259-~259: Possible missing comma found.
Context: ...epth for operators who want a hard kill-switch regardless of what any CR asks for. ##...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~266-~266: The usual collocation for “composed” is “of”, not “from”. Did you mean “composed of”?
Context: ...:commit_reservation(and async twins), composed from existingcounters_that_apply/updat...
(COMPOSE_FROM_OF)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rfcs/0000-token-rate-limit-reservations.md` around lines 257 - 285, Resolve
the contradictory reservation flags in the “Limitador-level opt-in” and
“limitador” work items by specifying one flag name and its default, then align
all references to it. Define the capability-detection or upgrade-sequencing
behavior used by the reconciler so the default CR mode does not emit
Reserve/Commit actions until the target Limitador supports and enables those
RPCs, preserving Check/Report during upgrades or when reservations are disabled.
|
|
||
| ### Limitador-level opt-in | ||
|
|
||
| A server-level flag, `--enable-reservations` (default: `false`, i.e. reservations disabled), independent of anything the Kuadrant CR requests. When not set, `Reserve`/`Commit` are not registered (or return `UNIMPLEMENTED`). This exists as defense-in-depth for operators who want a hard kill-switch regardless of what any CR asks for. |
There was a problem hiding this comment.
The Kuadrant default is to enable, and the Limitador default is to disable?
--enable-reservations here vs --disable-reservations a few lines below vs disable: false in kuadrant CR .
Even if the CR will have an enum of: strategy: reservation/default (or something like this, as discussed at the community call) we might want the same "default" state across all components
| expires_at(counter) = min( | ||
| now + reservation.ttl, // caller-supplied TTL (CEL-computed, see above) | ||
| current_window_expiry(counter), // this counter's own window boundary | ||
| server_max_reservation_ttl // operator-configured safety clamp |
There was a problem hiding this comment.
How do we set the server_max_reservation_ttl?
| } | ||
|
|
||
| // Capacity available for a counter must now account for outstanding holds: | ||
| outstanding(counter) = sum(e.amount for e in registry[counter] if e.expires_at > now) |
There was a problem hiding this comment.
This might be nothing, but this will loop over every reservation/request in process (O(n) for n reservations).
Is keeping a single counter that each reservation will update a better idea (O(1))?
|
|
||
| `TokenRateLimitPolicy` ([RFC 0013](./0013-ai-policies.md)) checks capacity when a request arrives but only reports actual token usage after the model responds, which lets concurrent in-flight requests race past a limit before any of them report usage. This RFC introduces a **reservation layer** in Limitador: on arrival, an estimated token volume is reserved and held (with a TTL) against a limit's capacity; when the response comes back, the caller commits the reservation with actual usage in a single call, releasing any over-reservation immediately. Rejection semantics, rate-limit headers, and multi-limit all-or-nothing behavior are unchanged. The feature is additive at every layer (new gRPC RPCs, new CRD field, new Limitador flag) and is enabled by default with an explicit opt-out. | ||
|
|
||
| # Motivation |
There was a problem hiding this comment.
I was looking at it from the Egress perspective: adding the Egress gateway as motivation will make the case for this much stronger (not that it needs that, but hey, why not?)
Also, for the egress - we will be making a heavy guess on the reservation size. Maybe the default of 100 is too generous? I'd rather over-reserve with ~5k or something like that. What do you think (make it configurable in kuadrant CR?)?
I'm talking here about the default that is hardcoded here
| kind: Kuadrant | ||
| spec: | ||
| tokenRateLimiting: | ||
| mode: Reservation # default; set to CheckReport to revert every |
There was a problem hiding this comment.
Yes ok this feels better than a kill switch. I don't know what reason someone would chose the old behaviour though, but if it may be the case we add new "modes" then a choice is useful.
|
|
||
| ### Limitador-level opt-in | ||
|
|
||
| A server-level flag, `--enable-reservations` (default: `false`, i.e. reservations disabled), independent of anything the Kuadrant CR requests. When not set, `Reserve`/`Commit` are not registered (or return `UNIMPLEMENTED`). This exists as defense-in-depth for operators who want a hard kill-switch regardless of what any CR asks for. |
There was a problem hiding this comment.
interesting so I wonder is this enough and so can avoid the additional API in the kuadrant CR? This boils down to the same type of operation. Patch a resource with a new value?
There was a problem hiding this comment.
This looks good from my perspective. I only question the need for the Kuadrant level api to set the mode. If we have a "bail out option" on limitador do we really need another one on the kuadarant CR? Its not completely clear to me when a user would actually want the old behaviour to come back beyond some catostrophic bug?
That said I don't think this is a blocker it just sets a precendent where more and more policy level behaviour could be added to the Kuadrant CR.
Generally approve of the changes outlined though
|
This is a cool idea! So one question that I have that is probably out of scope here but I want to bring up the topic. The idea of rate limiting in a "batch" setting has been brought up many times. It would be great if there was a way to deny request BEFORE they were put on the queue but that becomes very difficult... So question here is do we think could eventually expand this "reservation" system to help take care of that problem? |
5 participants
Summary by CodeRabbit