From 0c8e483e7bcdee6ac4a5cc79a855d5a4d5c738eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20M=C3=A9ndez=20Hern=C3=A1ndez?= Date: Wed, 1 Apr 2026 21:42:54 +0200 Subject: [PATCH] Fixes #39208 - Cache global registration script in smart-proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /register returns the same shell script for all hosts sharing the same registration parameters (org, location, hostgroup, activation keys). Under bulk registration — 100+ hosts hitting the same capsule simultaneously — this endpoint is called once per host, each time proxying to Foreman and waiting for the ERB template to render (~103ms in profiling). Cache the rendered script in memory (5-minute TTL) keyed on a canonical form of the request query string. The cache key is computed by parsing the query string, sorting parameters alphabetically, and rebuilding — so requests that differ only in parameter order share the same cache entry and the same Foreman response, regardless of how the client ordered them. The implementation uses three clearly separated layers: get '/' — handles errors only; delegates to registration_script registration_script — owns the cache key and the business logic (what to cache and what to do on a miss); raises ScriptFetchError on non-200 so errors never reach the cache write cache(key, &block) — owns the mechanism: per-key double-checked locking via a block abstraction that keeps the caller free of locking concerns Per-key locking allows concurrent requests for genuinely different keys (e.g. different activation keys) to fetch from Foreman in parallel, while serialising only threads competing for the same key. The per-key Mutex is evicted from KEY_MUTEXES immediately after caching — once a key is hot, all future requests take the lock-free fast path and KEY_MUTEXES is empty under steady state. Only HTTP 200 responses are cached. Non-200 responses raise ScriptFetchError out of the cache block, which is rescued in get '/' and rendered via handle_response without poisoning the cache. Both KEY_MUTEXES and SCRIPT_CACHE use Concurrent::Map (already a smart-proxy dependency) for lock-free, thread-safe access on all Ruby VMs without relying on MRI's GIL. Tests added: - Cache hit: Foreman called once for repeated identical requests - Per-key isolation: different parameter sets cached independently - Parameter order independence: requests differing only in param order share the same cache entry - TTL expiry: expired entries are not served; Foreman is re-called - Mutex eviction: KEY_MUTEXES is empty after a successful cache write - Error non-caching: Foreman is called on every request when it errors - setup clears both SCRIPT_CACHE and KEY_MUTEXES between tests --- modules/registration/registration_api.rb | 80 +++++++++++++++++++-- test/registration/registration_api_test.rb | 83 ++++++++++++++++++++++ 2 files changed, 159 insertions(+), 4 deletions(-) diff --git a/modules/registration/registration_api.rb b/modules/registration/registration_api.rb index c4ad22273..b26837ae1 100644 --- a/modules/registration/registration_api.rb +++ b/modules/registration/registration_api.rb @@ -1,9 +1,51 @@ require 'registration/proxy_request' class Proxy::Registration::Api < ::Sinatra::Base + # Cache for the global registration script (GET /register). + # + # The script is identical for all hosts sharing the same registration + # parameters (org, location, hostgroup, activation keys), making it safe + # to serve from an in-memory cache during concurrent bulk registration. + # + # Per-key double-checked locking prevents thundering herd while allowing + # genuinely independent cache keys (e.g. different activation keys) to + # fetch from Foreman in parallel. + # + # Only HTTP 200 responses are cached — errors are raised out of the cache + # block so they never poison the cache. The per-key mutex is evicted + # immediately after caching so KEY_MUTEXES stays bounded to keys currently + # being fetched for the first time (zero under steady state). + REGISTRATION_SCRIPT_CACHE_TTL = 5 * 60 # seconds + KEY_MUTEXES = Concurrent::Map.new + SCRIPT_CACHE = Concurrent::Map.new + + class ScriptFetchError < StandardError + attr_reader :response + + def initialize(response) + super() + @response = response + end + end + + class << self + def registration_script_cache + SCRIPT_CACHE + end + + def key_mutex(cache_key) + KEY_MUTEXES.compute_if_absent(cache_key) { Mutex.new } + end + + def evict_key_mutex(cache_key) + KEY_MUTEXES.delete(cache_key) + end + end + get '/' do - response = Proxy::Registration::ProxyRequest.new.global_register(request) - handle_response(response) + registration_script + rescue ScriptFetchError => e + handle_response(e.response) rescue StandardError => e logger.exception "Error when rendering Global Registration Template", e render_error(default_error_msg) @@ -19,11 +61,41 @@ class Proxy::Registration::Api < ::Sinatra::Base private + def registration_script + cache_key = Rack::Utils.build_query( + Rack::Utils.parse_nested_query(request.query_string).sort_by { |k, _| k } + ) + cache(cache_key) do + response = Proxy::Registration::ProxyRequest.new.global_register(request) + raise ScriptFetchError, response unless response.code == '200' + response.body + end + end + + def cache(key, &block) + value = read_registration_cache(key) + return value if value + + self.class.key_mutex(key).synchronize do + value = read_registration_cache(key) + return value if value + + result = yield + self.class.registration_script_cache[key] = { body: result, at: Time.now } + self.class.evict_key_mutex(key) + result + end + end + + def read_registration_cache(cache_key) + entry = self.class.registration_script_cache[cache_key] + entry[:body] if entry && (Time.now - entry[:at]) < REGISTRATION_SCRIPT_CACHE_TTL + end + def handle_response(response) - if response.code.start_with? '2' + if response.code.start_with?('2') response.body else - # Return error message only if it is not HTML. message = response["content-type"].include?('text/plain') ? response.body : default_error_msg render_error(message, code: response.code) end diff --git a/test/registration/registration_api_test.rb b/test/registration/registration_api_test.rb index bd9ff7b1b..71ff6f828 100644 --- a/test/registration/registration_api_test.rb +++ b/test/registration/registration_api_test.rb @@ -11,6 +11,9 @@ def app def setup @foreman_url = 'http://foreman.example.com' Proxy::SETTINGS.stubs(:foreman_url).returns(@foreman_url) + # Clear class-level state between tests to prevent cross-test contamination + Proxy::Registration::Api.registration_script_cache.clear + Proxy::Registration::Api::KEY_MUTEXES.clear end def test_global_register_template @@ -102,6 +105,86 @@ def test_global_500 assert_match("echo \"Internal Server Error\"\nexit 1\n", last_response.body) end + def test_global_register_caches_response + stub = stub_request(:get, "#{@foreman_url}/register").to_return(body: 'template') + + 2.times do + get '/' + assert last_response.ok? + assert_match('template', last_response.body) + end + + assert_requested stub, times: 1 + end + + def test_global_register_cache_key_is_parameter_order_independent + # Cache key is normalised (params sorted alphabetically), so both orderings + # produce activation_keys=rhel9&owner=Default_Organization and share one entry. + stub_request(:get, "#{@foreman_url}/register?activation_keys=rhel9&owner=Default_Organization") + .to_return(body: 'template') + + get '/', { owner: 'Default_Organization', activation_keys: 'rhel9' } + assert last_response.ok? + + # Different parameter order — must hit cache, not Foreman again + get '/', { activation_keys: 'rhel9', owner: 'Default_Organization' } + assert last_response.ok? + + assert_requested :get, "#{@foreman_url}/register?activation_keys=rhel9&owner=Default_Organization", times: 1 + end + + def test_global_register_caches_per_key + stub_a = stub_request(:get, "#{@foreman_url}/register?key=a").to_return(body: 'template_a') + stub_b = stub_request(:get, "#{@foreman_url}/register?key=b").to_return(body: 'template_b') + + get '/', { key: 'a' } + assert_match('template_a', last_response.body) + get '/', { key: 'b' } + assert_match('template_b', last_response.body) + # second requests — must be served from cache + get '/', { key: 'a' } + assert_match('template_a', last_response.body) + get '/', { key: 'b' } + assert_match('template_b', last_response.body) + + assert_requested stub_a, times: 1 + assert_requested stub_b, times: 1 + end + + def test_global_register_evicts_mutex_after_caching + stub_request(:get, "#{@foreman_url}/register").to_return(body: 'template') + + get '/' + assert last_response.ok? + assert_empty Proxy::Registration::Api::KEY_MUTEXES + end + + def test_global_register_cache_entry_expires_after_ttl + Proxy::Registration::Api.registration_script_cache[''] = { + body: 'stale-template', + at: Time.now - Proxy::Registration::Api::REGISTRATION_SCRIPT_CACHE_TTL - 1, + } + stub = stub_request(:get, "#{@foreman_url}/register").to_return(body: 'fresh-template') + + get '/' + assert last_response.ok? + assert_match('fresh-template', last_response.body) + assert_requested stub, times: 1 + end + + def test_global_register_does_not_cache_errors + stub = stub_request(:get, "#{@foreman_url}/register").to_return( + body: 'error', status: 500, headers: { "Content-Type" => 'text/plain' } + ) + + 2.times do + get '/' + assert last_response.server_error? + end + + assert_requested stub, times: 2 + end + def test_host_500 Rack::NullLogger.any_instance.stubs(:exception) stub_request(:post, "#{@foreman_url}/register").to_timeout