-
Notifications
You must be signed in to change notification settings - Fork 297
XAPI throttling proof of concept #6778
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.
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
base: feature/throttling
Are you sure you want to change the base?
Changes from all commits
8f93764
60b637b
9123696
1112ae5
e933851
9fd1f49
69585f8
0047b24
68993df
a540ca7
ebcbe84
d6b68df
2cf148a
786abb6
993e6ca
3cf71dc
18f8a88
19f18b3
3b02b8c
ff6be8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,40 @@ | ||||||
| (* | ||||||
| * Copyright (C) 2023 Cloud Software Group | ||||||
| * | ||||||
| * This program is free software; you can redistribute it and/or modify | ||||||
| * it under the terms of the GNU Lesser General Public License as published | ||||||
| * by the Free Software Foundation; version 2.1 only. with the special | ||||||
| * exception on linking described in file LICENSE. | ||||||
| * | ||||||
| * This program is distributed in the hope that it will be useful, | ||||||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||||||
| * GNU Lesser General Public License for more details. | ||||||
| *) | ||||||
|
|
||||||
| open Datamodel_types | ||||||
| open Datamodel_common | ||||||
| open Datamodel_roles | ||||||
|
|
||||||
| let lifecycle = [] | ||||||
|
|
||||||
| let t = | ||||||
| create_obj ~name:_rate_limit ~descr:"Rate limiting policy for a XAPI client" | ||||||
| ~doccomments:[] ~gen_constructor_destructor:true ~gen_events:true | ||||||
| ~in_db:true ~lifecycle:[] ~persist:PersistEverything ~in_oss_since:None | ||||||
| ~messages_default_allowed_roles:_R_POOL_ADMIN | ||||||
| ~contents: | ||||||
| ([uid _rate_limit ~lifecycle] | ||||||
| @ [ | ||||||
| field ~qualifier:StaticRO ~ty:String ~lifecycle "client_id" | ||||||
| "An identifier for the rate limited client" ~ignore_foreign_key:true | ||||||
| ~default_value:(Some (VString "")) | ||||||
| ; field ~qualifier:StaticRO ~ty:Float ~lifecycle "burst_size" | ||||||
| "Amount of tokens that can be consumed in one burst" | ||||||
| ~ignore_foreign_key:true ~default_value:(Some (VFloat 0.)) | ||||||
| ; field ~qualifier:StaticRO ~ty:Float ~lifecycle "fill_rate" | ||||||
| "Tokens added to token bucket per second" ~ignore_foreign_key:true | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ~default_value:(Some (VFloat 0.)) | ||||||
| ] | ||||||
| ) | ||||||
| ~messages:[] () | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| (* | ||
| * Copyright (C) 2025 Cloud Software Group | ||
| * | ||
| * This program is free software; you can redistribute it and/or modify | ||
| * it under the terms of the GNU Lesser General Public License as published | ||
| * by the Free Software Foundation; version 2.1 only. with the special | ||
| * exception on linking described in file LICENSE. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU Lesser General Public License for more details. | ||
| *) | ||
|
|
||
| type rate_limit_data = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could use a simpler name like |
||
| bucket: Token_bucket.t | ||
| ; process_queue: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to let the caller know how long it should delay before moving ahead? The schedules of different threads can be partially sorted by the delays returned to them. |
||
| (float * (unit -> unit)) Queue.t (* contains token cost and callback *) | ||
| ; process_queue_lock: Mutex.t | ||
| ; worker_thread_cond: Condition.t | ||
| ; should_terminate: bool ref (* signal termination to worker thread *) | ||
| ; worker_thread: Thread.t | ||
| } | ||
| [@@warning "-69"] | ||
|
|
||
| module StringMap = Map.Make (String) | ||
|
|
||
| type t = rate_limit_data StringMap.t Atomic.t | ||
|
|
||
| let with_lock = Xapi_stdext_threads.Threadext.Mutex.execute | ||
|
|
||
| let create () = Atomic.make StringMap.empty | ||
|
|
||
| let mem t ~user_agent = | ||
| let map = Atomic.get t in | ||
| StringMap.mem user_agent map | ||
|
|
||
| (* The worker thread is responsible for calling the callback when the token | ||
| amount becomes available *) | ||
| let rec worker_loop ~bucket ~process_queue ~process_queue_lock | ||
| ~worker_thread_cond ~should_terminate = | ||
| let process_item cost callback = | ||
| Token_bucket.delay_then_consume bucket cost ; | ||
| callback () | ||
| in | ||
| Mutex.lock process_queue_lock ; | ||
| while Queue.is_empty process_queue && not !should_terminate do | ||
| Condition.wait worker_thread_cond process_queue_lock | ||
| done ; | ||
| let item_opt = Queue.take_opt process_queue in | ||
| Mutex.unlock process_queue_lock ; | ||
| match item_opt with | ||
| | None -> | ||
| (* Queue is empty only when termination was signalled *) | ||
| () | ||
| | Some (cost, callback) -> | ||
| process_item cost callback ; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that synchronous API calls, which do not include a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes indeed, that's right. I'm doing this under the assumption that synchronous calls take a relatively small amount of time, so if they have already been rate limited then the rate limiting should be the bottleneck, rather than execution time. If this doesn't hold (as would be the case for a bucket with a high enough refill rate) then we can introduce a thread pool instead of a single thread for handling rate limited requests. |
||
| worker_loop ~bucket ~process_queue ~process_queue_lock ~worker_thread_cond | ||
| ~should_terminate | ||
|
|
||
| (* TODO: Indicate failure reason - did we get invalid config or try to add an | ||
| already present user_agent? *) | ||
| let add_bucket t ~user_agent ~burst_size ~fill_rate = | ||
| let map = Atomic.get t in | ||
| if StringMap.mem user_agent map then | ||
| false | ||
| else | ||
| match Token_bucket.create ~burst_size ~fill_rate with | ||
| | Some bucket -> | ||
| let process_queue = Queue.create () in | ||
| let process_queue_lock = Mutex.create () in | ||
| let worker_thread_cond = Condition.create () in | ||
| let should_terminate = ref false in | ||
| let worker_thread = | ||
| Thread.create | ||
| (fun () -> | ||
| worker_loop ~bucket ~process_queue ~process_queue_lock | ||
| ~worker_thread_cond ~should_terminate | ||
| ) | ||
| () | ||
| in | ||
| let data = | ||
| { | ||
| bucket | ||
| ; process_queue | ||
| ; process_queue_lock | ||
| ; worker_thread_cond | ||
| ; should_terminate | ||
| ; worker_thread | ||
| } | ||
| in | ||
| let updated_map = StringMap.add user_agent data map in | ||
| Atomic.set t updated_map ; true | ||
| | None -> | ||
| false | ||
|
|
||
| let delete_bucket t ~user_agent = | ||
| let map = Atomic.get t in | ||
| match StringMap.find_opt user_agent map with | ||
| | None -> | ||
| () | ||
| | Some data -> | ||
| Mutex.lock data.process_queue_lock ; | ||
| data.should_terminate := true ; | ||
| Condition.signal data.worker_thread_cond ; | ||
| Mutex.unlock data.process_queue_lock ; | ||
| Atomic.set t (StringMap.remove user_agent map) | ||
|
|
||
| let try_consume t ~user_agent amount = | ||
| let map = Atomic.get t in | ||
| match StringMap.find_opt user_agent map with | ||
| | None -> | ||
| false | ||
| | Some data -> | ||
| Token_bucket.consume data.bucket amount | ||
|
|
||
| let peek t ~user_agent = | ||
| let map = Atomic.get t in | ||
| Option.map | ||
| (fun contents -> Token_bucket.peek contents.bucket) | ||
| (StringMap.find_opt user_agent map) | ||
|
|
||
| (* The callback should return quickly - if it is a longer task it is | ||
| responsible for creating a thread to do the task *) | ||
| let submit t ~user_agent ~callback amount = | ||
| let map = Atomic.get t in | ||
| match StringMap.find_opt user_agent map with | ||
| | None -> | ||
| callback () | ||
| | Some {bucket; process_queue; process_queue_lock; worker_thread_cond; _} -> | ||
| with_lock process_queue_lock (fun () -> | ||
| if Queue.is_empty process_queue && Token_bucket.consume bucket amount | ||
| then | ||
| callback () | ||
| else | ||
| let need_signal = Queue.is_empty process_queue in | ||
| Queue.add (amount, callback) process_queue ; | ||
| if need_signal then Condition.signal worker_thread_cond | ||
| ) | ||
|
|
||
| let submit_sync t ~user_agent ~callback amount = | ||
| let result = ref None in | ||
| let mutex = Mutex.create () in | ||
| let condition = Condition.create () in | ||
| let wrapped_callback () = | ||
| let r = callback () in | ||
| Mutex.lock mutex ; | ||
| result := Some r ; | ||
| Condition.signal condition ; | ||
| Mutex.unlock mutex | ||
| in | ||
| submit t ~user_agent ~callback:wrapped_callback amount ; | ||
| Mutex.lock mutex ; | ||
| while Option.is_none !result do | ||
| Condition.wait condition mutex | ||
| done ; | ||
| Mutex.unlock mutex ; | ||
| Option.get !result | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| (* | ||
| * Copyright (C) 2025 Cloud Software Group | ||
| * | ||
| * This program is free software; you can redistribute it and/or modify | ||
| * it under the terms of the GNU Lesser General Public License as published | ||
| * by the Free Software Foundation; version 2.1 only. with the special | ||
| * exception on linking described in file LICENSE. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU Lesser General Public License for more details. | ||
| *) | ||
|
|
||
| (** Hash table mapping client identifiers to their token buckets for rate limiting. *) | ||
| type t | ||
|
|
||
| val create : unit -> t | ||
| (** [create ()] creates a new empty bucket table. *) | ||
|
|
||
| val add_bucket : | ||
| t -> user_agent:string -> burst_size:float -> fill_rate:float -> bool | ||
| (** [add_bucket table ~user_agent ~burst_size ~fill_rate] adds a token bucket | ||
| for the given user agent. Returns [false] if a bucket already exists, or if | ||
| the bucket configuration is invalid, e.g. negative/zero fill rate. *) | ||
|
|
||
| val mem : t -> user_agent:string -> bool | ||
| (** [mem table ~user_agent] returns whether [user_agent] has an associated | ||
| token bucket in the bucket table *) | ||
|
|
||
| val peek : t -> user_agent:string -> float option | ||
| (** [peek table ~user_agent] returns the current token count for the user agent, | ||
| or [None] if no bucket exists. *) | ||
|
|
||
| val delete_bucket : t -> user_agent:string -> unit | ||
| (** [delete_bucket table ~user_agent] removes the bucket for the user agent. *) | ||
|
|
||
| val try_consume : t -> user_agent:string -> float -> bool | ||
| (** [try_consume table ~user_agent amount] attempts to consume tokens. | ||
| Returns [true] on success, [false] if insufficient tokens. *) | ||
|
|
||
| val submit : t -> user_agent:string -> callback:(unit -> unit) -> float -> unit | ||
| (** [submit table ~user_agent ~callback amount] submits a callback to be executed | ||
| under rate limiting. If tokens are immediately available and no callbacks are | ||
| queued, the callback runs synchronously. Otherwise, it is enqueued and will | ||
| be executed by a worker thread when tokens become available. Returns immediately. *) | ||
|
|
||
| val submit_sync : t -> user_agent:string -> callback:(unit -> 'a) -> float -> 'a | ||
| (** [submit_sync table ~user_agent ~callback amount] submits a callback to be | ||
| executed under rate limiting and blocks until it completes, returning the | ||
| callback's result. *) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| (library | ||
| (name rate_limit) | ||
| (public_name rate-limit) | ||
| (libraries threads.posix mtime mtime.clock.os xapi-log xapi-stdext-threads clock) | ||
| ) | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| (tests | ||
| (names test_token_bucket test_bucket_table) | ||
| (package rate-limit) | ||
| (libraries rate_limit alcotest qcheck-core qcheck-alcotest mtime mtime.clock.os fmt xapi-log threads.posix)) |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the idl should mention tokens or buckets at all, instead I would try to communicate the meaning of the parameters in a way that allows users to make a mental model of how rate limiting works:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we shouldn't talk about token buckets and I'll change that. The plan is to assign higher token costs to more expensive calls, e.g. VM create, so we can't simplify to the level of RPC calls, but I'll figure out how to document this for users.