Cache volumes: RAM cache settings and remap option#12717
Cache volumes: RAM cache settings and remap option#12717zwoop wants to merge 7 commits intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds per-volume RAM cache configuration and a new @volume= remap directive, providing fine-grained control over cache volume selection and RAM cache allocation beyond the existing hostname-based volume selection mechanism.
- Introduces
ram_cache_sizeandram_cache_cutoffparameters in volume.config for per-volume RAM cache control - Adds
@volume=directive in remap.config to override default volume selection - Implements private RAM cache allocation with automatic subtraction from the global pool
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/cache/cache_volume_features.test.py | Test suite entry point for cache volume features |
| tests/gold_tests/cache/cache_volume_features.replay.yaml | Comprehensive test scenarios covering volume selection and RAM cache configurations |
| src/proxy/http/remap/RemapProcessor.cc | Integrates cache_volume override from remap rules into transaction state |
| src/proxy/http/remap/RemapConfig.cc | Parses and validates @Volume= option from remap.config |
| src/proxy/http/HttpCacheSM.cc | Passes volume override to cache operations (open_read/open_write) |
| src/iocore/cache/Stripe.h | Adds ram_cache_size and ram_cache_cutoff fields to CacheVol structure |
| src/iocore/cache/P_CacheHosting.h | Adds ram_cache_size and ram_cache_cutoff to ConfigVol structure |
| src/iocore/cache/P_CacheInternal.h | Updates Cache::open_read/open_write/key_to_stripe signatures with volume_override parameter |
| src/iocore/cache/CacheVC.cc | Implements per-volume RAM cache cutoff logic using volume-specific or global settings |
| src/iocore/cache/CacheProcessor.cc | Implements private RAM allocation calculation and shared pool distribution |
| src/iocore/cache/CacheHosting.cc | Parses ram_cache_size and ram_cache_cutoff from volume.config |
| src/iocore/cache/Cache.cc | Implements volume override selection logic in key_to_stripe function |
| include/proxy/http/remap/UrlMapping.h | Adds cache_volume field to url_mapping class |
| include/proxy/http/remap/RemapConfig.h | Defines REMAP_OPTFLG_VOLUME flag for volume option |
| include/proxy/http/HttpTransact.h | Adds cache_volume_override field to transaction state |
| include/iocore/cache/Cache.h | Updates CacheProcessor API signatures with volume_override parameter |
| doc/admin-guide/files/volume.config.en.rst | Documents ram_cache_size and ram_cache_cutoff configuration options |
| doc/admin-guide/files/remap.config.en.rst | Documents @Volume= directive usage and behavior |
| doc/admin-guide/files/records.yaml.en.rst | Documents interaction between global and per-volume RAM cache settings |
| configs/volume.config.default | Adds examples and documentation for new RAM cache parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3e4aaed to
88d3b83
Compare
0c8b658 to
b56dae1
Compare
5d19907 to
7bd004b
Compare
7bd004b to
5160673
Compare
24ef624 to
8555711
Compare
|
[approve ci autest 1] |
|
I've addressed CoPilot's reviews again. |
bryancall
left a comment
There was a problem hiding this comment.
Overall: Approve with comments
Solid feature PR. The 4-tier priority system in key_to_stripe() is clean, the RAM cache subtraction math is correct, and the deferred initialization pattern for @volume= is well-reasoned for the startup ordering constraint. A few items worth addressing:
Medium Priority
-
Lowercase size suffixes silently produce wrong values.
ink_atoi64only recognizes uppercaseK,M,G,T, but the suffix-skip loops inBuildListFromStringaccept both cases (strchr("kmgtKMGT", *tmp)). Soram_cache_size=2gparses as 2 bytes, not 2GB, with no warning. Same issue foravg_obj_size,fragment_size, andram_cache_cutoff. Either restrict the skip loop to uppercase only (to matchink_atoi64), or document that suffixes are case-sensitive. -
@volume=character validation is too permissive. The validation loop allows degenerate inputs like@volume=,,,@volume=0,@volume=,1,, and@volume=999. For the eager path (cache ready),createCacheHostRecord()catches bad volume numbers. But for the deferred path (initial startup), the string is stored as-is and errors only surface later as a non-fatalError()log — ATS keeps running with no volume override for that rule. Consider validating individual volume numbers (1–255, no empty segments) during remap parsing.
Low Priority / Informational
-
@volume=99in the features test — if volume 99 isn't defined involume.config,createCacheHostRecord()will fail. During initial startup this is deferred and the error is swallowed (logged but non-fatal). The test may only pass because of deferred init. Worth verifying this is the intended behavior, or adding an explicit "invalid volume gracefully degrades" test. -
Brief startup window where
@volume=rules don't take effect — ifwait_for_cacheis disabled, requests arriving between remap load andCB_After_Cache_Initwill seevolume_host_rec == nullptrand fall through to the normal priority chain. This is correct degradation, but worth a note in the docs. -
No test for hosting.config +
@volume=priority interaction — the 4-tier priority is documented but only tiers 1, 3, and 4 are tested. A test showing@volume=overriding a hostname match fromhosting.configwould strengthen confidence.
bryancall
left a comment
There was a problem hiding this comment.
Overall: Approve with comments
Solid feature PR. The 4-tier priority system in key_to_stripe() is clean, the RAM cache subtraction math is correct, and the deferred initialization pattern for @volume= is well-reasoned for the startup ordering constraint. A few items worth addressing:
Medium Priority
-
Lowercase size suffixes silently produce wrong values.
ink_atoi64only recognizes uppercaseK,M,G,T, but the suffix-skip loops inBuildListFromStringaccept both cases (strchr("kmgtKMGT", *tmp)). Soram_cache_size=2gparses as 2 bytes, not 2GB, with no warning. Same issue foravg_obj_size,fragment_size, andram_cache_cutoff. Either restrict the skip loop to uppercase only (to matchink_atoi64), or document that suffixes are case-sensitive. -
@volume=character validation is too permissive. The validation loop allows degenerate inputs like@volume=,,,@volume=0,@volume=,1,, and@volume=999. For the eager path (cache ready),createCacheHostRecord()catches bad volume numbers. But for the deferred path (initial startup), the string is stored as-is and errors only surface later as a non-fatalError()log — ATS keeps running with no volume override for that rule. Consider validating individual volume numbers (1–255, no empty segments) during remap parsing.
Low Priority / Informational
-
@volume=99in the features test — if volume 99 isn't defined involume.config,createCacheHostRecord()will fail. During initial startup this is deferred and the error is swallowed (logged but non-fatal). The test may only pass because of deferred init. Worth verifying this is the intended behavior, or adding an explicit "invalid volume gracefully degrades" test. -
Brief startup window where
@volume=rules don't take effect — ifwait_for_cacheis disabled, requests arriving between remap load andCB_After_Cache_Initwill seevolume_host_rec == nullptrand fall through to the normal priority chain. This is correct degradation, but worth a note in the docs. -
No test for hosting.config +
@volume=priority interaction — the 4-tier priority is documented but only tiers 1, 3, and 4 are tested. A test showing@volume=overriding a hostname match fromhosting.configwould strengthen confidence.
This gets a bit complicated because we allow for ATS to start up with remap before the cache is properly started.
- foreach_mapping: remove const (Trie only has const_iterator, so const_cast is still needed but function is now semantically correct) - Cache.cc: document default_volumes_host_rec as intentional singleton - CacheHosting.cc: restore tmp++ (tmp=line_end broke multi-param lines), remove unreachable null check after new, use char array for "volume" literal instead of const_cast - ReverseProxy.cc: add acquire()/release() around rewrite_table access in init_remap_volume_host_records, document startup-only constraint - Test files: remove trailing commas from ATSReplayTest calls
- CacheHosting.cc: restrict suffix skip loops to uppercase KMGT to match ink_atoi64's uppercase-only handling - RemapConfig.cc: use swoc::TextView to validate @Volume= segments (rejects empty, zero, out-of-range, trailing comma) - ReverseProxy.h/cc, HttpSM.cc: use std::atomic<UrlRewrite*> for rewrite_table to make atomic access explicit - cache_volume_features.replay.yaml: remove misleading @Volume=99 remap entry and its corresponding session test
This adds new configuration options for volume.config, as well as
adding an
@volume=setting to remap.config (avoiding the hosting.configstepping stone).