Copilot commented on code in PR #13031:
URL: https://github.com/apache/trafficserver/pull/13031#discussion_r3007435195
##########
contrib/set_trafficserver.sh:
##########
@@ -29,7 +29,7 @@
# v1.0.3 - Check to force /mnt based cache.db
REMAP_FILE=/usr/local/etc/trafficserver/remap.config
-STORAGE_FILE=/usr/local/etc/trafficserver/storage.config
+STORAGE_FILE=/usr/local/etc/trafficserver/storage.yaml
EC2_CACHE_LOC=/mnt/trafficserver_cache
Review Comment:
This script now points STORAGE_FILE at storage.yaml, but later logic (e.g.,
the sed replacement) still assumes the old storage.config line-based format.
As-is, ec2Cache() won't update the YAML config correctly. Please update the
modification logic to write valid YAML (or gate on file format) so the script
remains functional.
##########
src/iocore/cache/CacheProcessor.cc:
##########
@@ -873,13 +882,31 @@ cplist_reconfigure()
// in such a way forced volumes will not impact volume percentage
calculations.
if (-1 == gdisks[i]->forced_volume_num) {
tot_space_in_blks += (gdisks[i]->num_usable_blocks / blocks_per_vol) *
blocks_per_vol;
+ } else {
+ // exclusive span
+ for (ConfigVol *config_vol = config_volumes.cp_queue.head; config_vol;
config_vol = config_vol->link.next) {
+ for (auto &vol_span_config : config_vol->spans) {
+ // Convert relative exclusive span size into absolute size
+ if (strncmp(gdisks[i]->span_name, vol_span_config.use.c_str(),
vol_span_config.use.size()) == 0 &&
+ vol_span_config.size.in_percent) {
+ int64_t space_in_blks =
+ (gdisks[i]->num_usable_blocks / blocks_per_vol) *
blocks_per_vol * vol_span_config.size.percent / 100;
+
+ space_in_blks = space_in_blks >> (20 - STORE_BLOCK_SHIFT);
+ // round down to 128 megabyte multiple
+ space_in_blks = (space_in_blks >> 7) << 7;
+ vol_span_config.size.absolute_value = space_in_blks;
+ }
+ }
Review Comment:
Per-span volume assignments appear incomplete for non-exclusive spans:
volumes with config_vol->spans rely on span.size.absolute_value later (size is
computed as a sum), but absolute_value is only set in the forced/exclusive-disk
branch. For shared spans (forced_volume_num == -1), span absolute sizes stay 0,
so the volume size resolves to 0 and the volume will be skipped (<128). The
span-size conversion/allocation logic should be applied for all spans (not just
forced ones), and span name matching should be exact (use full-string compare
rather than the current prefix-based strncmp).
##########
src/iocore/cache/unit_tests/test_ConfigVolumes.cc:
##########
@@ -0,0 +1,296 @@
+/** @file
+
+ Unit tests for ConfigVolumes
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#define CATCH_CONFIG_MAIN
+
Review Comment:
This test defines CATCH_CONFIG_MAIN but the CMake target links against
Catch2::Catch2WithMain. That will result in multiple definitions of main at
link time. Either remove CATCH_CONFIG_MAIN here, or link against Catch2::Catch2
(without main) and keep the macro.
```suggestion
```
##########
src/iocore/cache/CacheHosting.cc:
##########
@@ -900,3 +619,91 @@ createCacheHostRecord(const char *volume_str, char
*errbuf, size_t errbufsize)
return host_rec;
}
+
+bool
+ConfigVol::Size::is_empty() const
+{
+ if (absolute_value == 0 && in_percent == false && percent == 0) {
+ return true;
+ }
+
+ return false;
+}
+
+/**
+ Complement missing volumes[].size and volumes[].spans[].size if there
+ */
+void
+ConfigVolumes::complement()
+{
+ struct SizeTracker {
+ int empty_node = 0;
+ int remaining_size = 100; ///< in percentage
+ };
+
+ SizeTracker volume_tracker;
+ std::unordered_map<std::string_view, SizeTracker> span_size_map;
+
+ // Find missing size and remaining size in percentage
+ for (ConfigVol *conf = cp_queue.head; conf != nullptr; conf =
cp_queue.next(conf)) {
+ if (!conf->spans.empty()) {
+ for (const auto &span : conf->spans) {
+ SizeTracker span_size;
+
+ if (auto it = span_size_map.find(span.use); it != span_size_map.end())
{
+ span_size = it->second;
+ }
+
+ if (span.size.is_empty()) {
+ span_size.empty_node++;
+ } else if (span.size.in_percent) {
+ if (span_size.remaining_size < span.size.percent) {
+ Error("Total volume size (in percent) exceeded 100%% -
span.use=%s", span.use.c_str());
+ continue;
+ }
+
+ span_size.remaining_size -= span.size.percent;
+ }
+
+ span_size_map.insert_or_assign(span.use, span_size);
+ }
+ } else {
+ if (conf->size.is_empty()) {
+ ++volume_tracker.empty_node;
+ } else if (conf->size.in_percent) {
+ if (volume_tracker.remaining_size < conf->size.percent) {
+ Error("Total volume size (in percent) exceeded 100%%");
+ continue;
+ }
+ volume_tracker.remaining_size -= conf->size.percent;
+ }
+ }
+ }
+
+ // If there're no missing size, do nothing
+ if (volume_tracker.empty_node == 0 &&
+ std::all_of(span_size_map.cbegin(), span_size_map.cend(), [](auto &it) {
return it.second.empty_node == 0; })) {
+ return;
Review Comment:
ConfigVolumes::complement() uses std::unordered_map and std::all_of, but
CacheHosting.cc doesn't include the corresponding standard headers in its
include list. Please add explicit includes (e.g., <unordered_map> and
<algorithm>) to avoid fragile reliance on transitive includes.
##########
src/iocore/cache/YamlStorageConfig.cc:
##########
@@ -0,0 +1,360 @@
+/** @file
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "P_CacheHosting.h"
+
+#include "iocore/cache/YamlStorageConfig.h"
+
+#include "tscore/Diags.h"
+
+#include <string_view>
+#include <yaml-cpp/yaml.h>
+
+#include <set>
+#include <string>
+
+namespace
+{
+std::set<std::string> valid_cache_config_keys = {"spans", "volumes"};
+std::set<std::string> valid_spans_config_keys = {"name", "path", "size",
"hash_seed"};
+std::set<std::string> valid_volumes_config_keys = {
+ "id", "size", "scheme", "ram_cache", "ram_cache_size", "ram_cache_cutoff",
"avg_obj_size", "fragment_size", "spans"};
+std::set<std::string> valid_volumes_spans_config_keys = {"use", "size"};
+
+constexpr static int MAX_VOLUME_IDX = 255;
+
+bool
+validate_map(const YAML::Node &node, const std::set<std::string> &keys)
+{
+ if (!node.IsMap()) {
+ throw YAML::ParserException(node.Mark(), "malformed entry");
+ }
+
+ for (const auto &item : node) {
+ if (std::none_of(keys.begin(), keys.end(), [&item](const std::string &s) {
return s == item.first.as<std::string>(); })) {
+ throw YAML::ParserException(item.first.Mark(), "format: unsupported key
'" + item.first.as<std::string>() + "'");
+ }
+ }
+
+ return true;
+}
+
+bool
+parse_volume_scheme(ConfigVol &volume, const std::string &scheme)
+{
+ if (scheme.compare("http") == 0) {
+ volume.scheme = CacheType::HTTP;
+ return true;
+ } else {
+ volume.scheme = CacheType::NONE;
+ return false;
+ }
+}
+
+/**
+ Convert given @s into ConfigVol::Size.
+ @s can be parcent (%), human readable unit (K/M/G/T) or absolute number.
+ */
+bool
+parse_size(ConfigVol::Size &size, const std::string &s)
+{
+ // must start with digit
+ if (!std::isdigit(s[0])) {
+ return false;
+ }
+
+ // s.ends_with('%')
+ if (s[s.length() - 1] == '%') {
+ // parcent
+ int value = std::stoi(s.substr(0, s.length() - 1));
+ if (value > 100) {
+ return false;
+ }
+ size.in_percent = true;
+ size.percent = value;
+ } else {
+ // Human-readable size
+ size.in_percent = false;
+ size.absolute_value = ink_atoi64(s.c_str());
+ }
+
+ return true;
+}
+
+} // namespace
+
+namespace YAML
+{
+////
+// spans
+//
+template <> struct convert<SpanConfig> {
+ static bool
+ decode(const Node &node, SpanConfig &spans)
+ {
+ if (!node.IsSequence()) {
+ Error("malformed data; expected sequence in cache.spans");
+ return false;
+ }
+
+ for (const auto &it : node) {
+ spans.push_back(it.as<SpanConfigParams>());
+ }
+
+ return true;
+ }
+};
+
+template <> struct convert<SpanConfigParams> {
+ static bool
+ decode(const Node &node, SpanConfigParams &span)
+ {
+ validate_map(node, valid_spans_config_keys);
+
+ if (!node["name"]) {
+ throw ParserException(node.Mark(), "missing 'name' argument in
cache.spans[]");
+ }
+ span.name = node["name"].as<std::string>();
+
+ if (!node["path"]) {
+ throw ParserException(node.Mark(), "missing 'path' argument in
cache.spans[]");
+ }
+ span.path = node["path"].as<std::string>();
+
+ // optional configs
+ if (node["size"]) {
+ // Human-readable size
+ std::string size = node["size"].as<std::string>();
+ if (!std::isdigit(size[0])) {
+ throw ParserException(node.Mark(), "Invalid size value (must start
with a number, e.g., 100G)");
+ }
+ span.size = ink_atoi64(size.c_str());
+ }
+
+ if (node["hash_seed"]) {
+ span.hash_seed = node["hash_seed"].as<std::string>();
+ };
+
+ return true;
+ }
+};
+
+////
+// volumes
+//
+template <> struct convert<ConfigVolumes> {
+ static bool
+ decode(const Node &node, ConfigVolumes &volumes)
+ {
+ if (!node.IsSequence()) {
+ Error("malformed data; expected sequence in cache.volumes");
+ return false;
+ }
+
+ int total_percent = 0;
+ for (const auto &it : node) {
+ ConfigVol volume = it.as<ConfigVol>();
+
+ if (volume.size.in_percent) {
+ total_percent += volume.size.percent;
+ }
+
+ if (total_percent > 100) {
+ Error("Total volume size added up to more than 100 percent");
+ return false;
+ }
+
+ volumes.cp_queue.enqueue(new ConfigVol(volume));
+ volumes.num_volumes++;
+ }
Review Comment:
ConfigVolumes YAML decode no longer checks for duplicate volume ids (the old
parser rejected duplicates). As-is, two entries with the same id will both be
enqueued, which will later confuse volume selection and reconfigure logic.
Track seen ids during decode and throw a parse error on duplicates.
##########
src/iocore/cache/CacheProcessor.cc:
##########
@@ -199,8 +199,6 @@ CacheProcessor::start_internal(int flags)
gndisks = 0;
ink_aio_set_err_callback(new AIO_failure_handler());
Review Comment:
volume.config is removed/deprecated in this PR (record
proxy.config.cache.volume_filename is also removed), but CacheProcessor still
references that record later in create_volume() to print an "edit the ... file"
message. This will either fail record lookup or point users at the wrong file;
update the message to reference storage.yaml (and/or remove the record lookup
entirely).
##########
tests/gold_tests/cache/storage-metrics.test.py:
##########
@@ -27,43 +27,75 @@
"case": 0,
"description": "default config",
"storage": '''
-storage 256M
-''',
- "volume": '''
-# empty
+cache:
+ spans:
+ - name: disk.0
+ path: storage
+ size: 256M
'''
- }, {
+ },
+ {
"case": 1,
"description": "four equally devided volumes",
- "storage": '''
-storage 1G
-''',
- "volume":
+ "storage":
'''
-volume=1 scheme=http size=25%
-volume=2 scheme=http size=25%
-volume=3 scheme=http size=25%
-volume=4 scheme=http size=25%
+cache:
+ spans:
+ - name: disk.0
+ path: storage
+ size: 1G
+ volumes:
+ - id: 1
+ size: 25%
+ - id: 2
+ size: 25%
+ - id: 3
+ size: 25%
+ - id: 4
+ size: 25%
'''
- }, {
+ },
+ {
"case": 2,
"description": "exclusive span",
- "storage": '''
-storage 256M volume=1
-''',
- "volume": '''
-volume=1 scheme=http size=262144
+ "storage":
+ '''
+cache:
+ spans:
+ - name: disk.0
+ path: storage
+ size: 256M
+ volumes:
+ - id: 1
+ spans:
+ - use: disk.0
+ size: 100%
''',
"hosting": '''
hostname=* volume=1
'''
- }
+ },
+ {
+ "case": 3,
+ "description": "two equally devided volumes without size option",
+ "storage":
+ '''
Review Comment:
Spelling: "devided" should be "divided" in the test case description.
##########
src/iocore/cache/YamlStorageConfig.cc:
##########
@@ -0,0 +1,360 @@
+/** @file
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "P_CacheHosting.h"
+
+#include "iocore/cache/YamlStorageConfig.h"
+
+#include "tscore/Diags.h"
+
+#include <string_view>
+#include <yaml-cpp/yaml.h>
+
+#include <set>
+#include <string>
Review Comment:
YamlStorageConfig.cc uses std::none_of/std::all_of and std::isdigit but
doesn't include the corresponding standard headers. This can fail to compile
depending on transitive includes; please add the required includes (e.g.,
<algorithm> and <cctype>).
```suggestion
#include <string>
#include <algorithm>
#include <cctype>
```
##########
src/iocore/cache/P_CacheHosting.h:
##########
@@ -281,38 +281,83 @@ class CacheHostTable
const char *matcher_name = "unknown"; // Used for
Debug/Warning/Error messages
};
-/* list of volumes in the volume.config file */
+/**
+ List of volumes in the storage.yaml
+ */
struct ConfigVol {
- int number;
- CacheType scheme;
- off_t size;
- bool in_percent;
- bool ramcache_enabled;
- int percent;
- int avg_obj_size;
- int fragment_size;
- int64_t ram_cache_size; // Per-volume RAM cache size (-1 = use shared
allocation)
- int64_t ram_cache_cutoff; // Per-volume RAM cache cutoff (-1 = use global
cutoff)
- CacheVol *cachep;
+ struct Size {
+ int64_t absolute_value = 0;
+ bool in_percent = false;
+ int percent = 0;
+
+ bool is_empty() const;
+ };
+
+ struct Span {
+ std::string use{};
+ Size size{};
+ };
+ using Spans = std::vector<Span>;
+
Review Comment:
P_CacheHosting.h now uses std::string and std::vector in
ConfigVol::Span/Spans but the header doesn't include <string> or <vector>.
Please add the needed standard includes so the header is self-contained and
doesn't rely on transitive includes.
##########
tests/gold_tests/autest-site/ats_replay.test.ext:
##########
@@ -86,14 +86,10 @@ def configure_ats(obj: 'TestRun', server: 'Process',
ats_config: dict, dns: Opti
if logging_yaml != None:
ts.Disk.logging_yaml.AddLines(yaml.dump(logging_yaml).split('\n'))
- # Configure volume.config if specified.
- volume_config = ats_config.get('volume_config', [])
- for vol in volume_config:
- parts = [f"volume={vol['volume']}", f"scheme={vol['scheme']}",
f"size={vol['size']}"]
- for opt_key in ('ramcache', 'ram_cache_size', 'ram_cache_cutoff',
'avg_obj_size', 'fragment_size'):
- if opt_key in vol:
- parts.append(f"{opt_key}={vol[opt_key]}")
- ts.Disk.volume_config.AddLine(' '.join(parts))
+ # Configure storage.yaml if specified.
+ storage_yaml = ats_config.get('storage_yaml')
+ if storage_yaml is not None:
+ ts.Disk.storage_yaml.AddLines(yaml.dump(storage_yaml).split('\n'))
Review Comment:
ats_replay no longer handles the legacy "volume_config" key, but at least
one existing replay (tests/gold_tests/cache/cache_volume_defaults.replay.yaml)
still uses it. Either add backward-compatible support for volume_config here
(e.g., translate it into storage_yaml.volumes) or update all remaining replays
to the new storage_yaml format to avoid silent misconfiguration/test failures.
##########
src/iocore/cache/YamlStorageConfig.cc:
##########
@@ -0,0 +1,360 @@
+/** @file
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "P_CacheHosting.h"
+
+#include "iocore/cache/YamlStorageConfig.h"
+
+#include "tscore/Diags.h"
+
+#include <string_view>
+#include <yaml-cpp/yaml.h>
+
+#include <set>
+#include <string>
+
+namespace
+{
+std::set<std::string> valid_cache_config_keys = {"spans", "volumes"};
+std::set<std::string> valid_spans_config_keys = {"name", "path", "size",
"hash_seed"};
+std::set<std::string> valid_volumes_config_keys = {
+ "id", "size", "scheme", "ram_cache", "ram_cache_size", "ram_cache_cutoff",
"avg_obj_size", "fragment_size", "spans"};
+std::set<std::string> valid_volumes_spans_config_keys = {"use", "size"};
+
+constexpr static int MAX_VOLUME_IDX = 255;
+
+bool
+validate_map(const YAML::Node &node, const std::set<std::string> &keys)
+{
+ if (!node.IsMap()) {
+ throw YAML::ParserException(node.Mark(), "malformed entry");
+ }
+
+ for (const auto &item : node) {
+ if (std::none_of(keys.begin(), keys.end(), [&item](const std::string &s) {
return s == item.first.as<std::string>(); })) {
+ throw YAML::ParserException(item.first.Mark(), "format: unsupported key
'" + item.first.as<std::string>() + "'");
+ }
+ }
+
+ return true;
+}
+
+bool
+parse_volume_scheme(ConfigVol &volume, const std::string &scheme)
+{
+ if (scheme.compare("http") == 0) {
+ volume.scheme = CacheType::HTTP;
+ return true;
+ } else {
+ volume.scheme = CacheType::NONE;
+ return false;
+ }
+}
+
+/**
+ Convert given @s into ConfigVol::Size.
+ @s can be parcent (%), human readable unit (K/M/G/T) or absolute number.
+ */
+bool
+parse_size(ConfigVol::Size &size, const std::string &s)
+{
+ // must start with digit
+ if (!std::isdigit(s[0])) {
+ return false;
+ }
+
+ // s.ends_with('%')
+ if (s[s.length() - 1] == '%') {
+ // parcent
+ int value = std::stoi(s.substr(0, s.length() - 1));
+ if (value > 100) {
+ return false;
+ }
+ size.in_percent = true;
+ size.percent = value;
+ } else {
+ // Human-readable size
+ size.in_percent = false;
+ size.absolute_value = ink_atoi64(s.c_str());
+ }
+
+ return true;
+}
+
+} // namespace
+
+namespace YAML
+{
+////
+// spans
+//
+template <> struct convert<SpanConfig> {
+ static bool
+ decode(const Node &node, SpanConfig &spans)
+ {
+ if (!node.IsSequence()) {
+ Error("malformed data; expected sequence in cache.spans");
+ return false;
+ }
+
+ for (const auto &it : node) {
+ spans.push_back(it.as<SpanConfigParams>());
+ }
+
+ return true;
+ }
+};
+
+template <> struct convert<SpanConfigParams> {
+ static bool
+ decode(const Node &node, SpanConfigParams &span)
+ {
+ validate_map(node, valid_spans_config_keys);
+
+ if (!node["name"]) {
+ throw ParserException(node.Mark(), "missing 'name' argument in
cache.spans[]");
+ }
+ span.name = node["name"].as<std::string>();
+
+ if (!node["path"]) {
+ throw ParserException(node.Mark(), "missing 'path' argument in
cache.spans[]");
+ }
+ span.path = node["path"].as<std::string>();
+
+ // optional configs
+ if (node["size"]) {
+ // Human-readable size
+ std::string size = node["size"].as<std::string>();
+ if (!std::isdigit(size[0])) {
+ throw ParserException(node.Mark(), "Invalid size value (must start
with a number, e.g., 100G)");
+ }
+ span.size = ink_atoi64(size.c_str());
+ }
+
+ if (node["hash_seed"]) {
+ span.hash_seed = node["hash_seed"].as<std::string>();
+ };
+
+ return true;
+ }
+};
+
+////
+// volumes
+//
+template <> struct convert<ConfigVolumes> {
+ static bool
+ decode(const Node &node, ConfigVolumes &volumes)
+ {
+ if (!node.IsSequence()) {
+ Error("malformed data; expected sequence in cache.volumes");
+ return false;
+ }
+
+ int total_percent = 0;
+ for (const auto &it : node) {
+ ConfigVol volume = it.as<ConfigVol>();
+
+ if (volume.size.in_percent) {
+ total_percent += volume.size.percent;
+ }
+
+ if (total_percent > 100) {
+ Error("Total volume size added up to more than 100 percent");
+ return false;
+ }
+
+ volumes.cp_queue.enqueue(new ConfigVol(volume));
+ volumes.num_volumes++;
+ }
+
+ return true;
+ }
+};
+
+template <> struct convert<ConfigVol> {
+ static bool
+ decode(const Node &node, ConfigVol &volume)
+ {
+ validate_map(node, valid_volumes_config_keys);
+
+ if (!node["id"]) {
+ throw ParserException(node.Mark(), "missing 'id' argument in
cache.volumes[]");
+ }
+ volume.number = node["id"].as<int>();
+ if (volume.number > MAX_VOLUME_IDX) {
+ throw ParserException(node.Mark(), "Bad Volume Number");
+ }
+
+ // optional configs
+ if (node["scheme"]) {
+ std::string scheme = node["scheme"].as<std::string>();
+ if (!parse_volume_scheme(volume, scheme)) {
+ throw ParserException(node.Mark(), "error on parsing 'scheme: " +
scheme + "' in cache.volumes[]");
+ }
+ } else {
+ // default scheme is http
+ volume.scheme = CacheType::HTTP;
+ }
+
+ if (node["size"]) {
+ std::string size = node["size"].as<std::string>();
+ if (!parse_size(volume.size, size)) {
+ throw ParserException(node.Mark(), "error on parsing 'size: " + size +
"' in cache.volumes[]");
+ }
+ }
+
+ if (node["ram_cache"]) {
+ volume.ramcache_enabled = node["ram_cache"].as<bool>();
+ }
+
+ if (node["ram_cache_size"]) {
+ // Human-readable size
+ std::string size = node["ram_cache_size"].as<std::string>();
+ if (!std::isdigit(size[0])) {
+ throw ParserException(node.Mark(), "Invalid ram_cache_size value (must
start with a number, e.g., 10G)");
+ }
+ volume.ram_cache_size = ink_atoi64(size.c_str());
+ }
+
+ if (node["ram_cache_cutoff"]) {
+ // Human-readable size
+ std::string size = node["ram_cache_cutoff"].as<std::string>();
+ if (!std::isdigit(size[0])) {
+ throw ParserException(node.Mark(), "Invalid ram_cache_cutoff value
(must start with a number, e.g., 5M)");
+ }
+ volume.ram_cache_cutoff = ink_atoi64(size.c_str());
+ }
+
+ if (node["avg_obj_size"]) {
+ // Human-readable size
+ std::string size = node["avg_obj_size"].as<std::string>();
+ if (!std::isdigit(size[0])) {
+ throw ParserException(node.Mark(), "Invalid avg_obj_size value (must
start with a number, e.g., 64K)");
+ }
+ volume.avg_obj_size = static_cast<int>(ink_atoi64(size.c_str()));
+ }
+
+ if (node["fragment_size"]) {
+ // Human-readable size
+ std::string size = node["fragment_size"].as<std::string>();
+ if (!std::isdigit(size[0])) {
+ throw ParserException(node.Mark(), "Invalid fragment_size value (must
start with a number, e.g., 1M)");
+ }
+ volume.fragment_size = static_cast<int>(ink_atoi64(size.c_str()));
+ }
+
+ if (node["spans"]) {
+ volume.spans = node["spans"].as<ConfigVol::Spans>();
+ }
+
+ return true;
+ }
+};
+
+template <> struct convert<ConfigVol::Spans> {
+ static bool
+ decode(const Node &node, ConfigVol::Spans &spans)
+ {
+ if (!node.IsSequence()) {
+ Error("malformed data; expected sequence in cache.volumes[].spans");
+ return false;
+ }
+
+ for (const auto &it : node) {
+ spans.push_back(it.as<ConfigVol::Span>());
+ }
+
+ return true;
+ }
+};
+
+template <> struct convert<ConfigVol::Span> {
+ static bool
+ decode(const Node &node, ConfigVol::Span &span)
+ {
+ validate_map(node, valid_volumes_spans_config_keys);
+
+ if (!node["use"]) {
+ throw ParserException(node.Mark(), "missing 'use' argument in
cache.volumes[].spans[]");
+ }
+ span.use = node["use"].as<std::string>();
+
+ if (node["size"]) {
+ std::string size = node["size"].as<std::string>();
+ if (!parse_size(span.size, size)) {
+ throw ParserException(node.Mark(), "error on parsing 'size: " + size +
"' in cache.volumes[].spans[]");
+ }
+ }
+
+ return true;
+ }
+};
+} // namespace YAML
+
+/**
+ ConfigVolume unit test helper
+ */
+bool
+YamlStorageConfig::load_volumes(ConfigVolumes &v_config, const std::string
&yaml)
+{
+ try {
+ YAML::Node node = YAML::Load(yaml);
+ v_config = node["volumes"].as<ConfigVolumes>();
+ v_config.complement();
+ } catch (std::exception &ex) {
+ Error("%s", ex.what());
+ return false;
+ }
+
+ return true;
+}
+
+/**
+ Load storage.yaml as SpanConfig and ConfigVolumes
+ */
+bool
+YamlStorageConfig::load(SpanConfig &s_config, ConfigVolumes &v_config,
std::string_view filename)
+{
+ try {
+ YAML::Node config = YAML::LoadFile(filename.data());
+
+ if (config.IsNull()) {
+ return false;
+ }
+
+ YAML::Node cache_config = config["cache"];
+
+ validate_map(cache_config, valid_cache_config_keys);
+
+ s_config = cache_config["spans"].as<SpanConfig>();
+
+ if (cache_config["volumes"]) {
+ v_config = cache_config["volumes"].as<ConfigVolumes>();
+ v_config.complement();
+ }
Review Comment:
YamlStorageConfig::load() only overwrites v_config when cache.volumes is
present; if storage.yaml omits volumes (or removes it during reconfigure), the
previous ConfigVolumes contents will be retained. Clear/reset v_config when
volumes are absent so reloads can't leave stale volume definitions active.
##########
src/iocore/cache/CMakeLists.txt:
##########
@@ -91,6 +93,11 @@ if(BUILD_TESTING)
add_cache_test(CacheStripe unit_tests/test_Stripe.cc)
add_cache_test(CacheAggregateWriteBuffer
unit_tests/test_AggregateWriteBuffer.cc)
+ # Unit Tests without unit_tests/main.cc
+ add_executable(test_ConfigVolumes unit_tests/test_ConfigVolumes.cc)
+ target_include_directories(test_ConfigVolumes PRIVATE
"${CMAKE_CURRENT_SOURCE_DIR}")
+ target_link_libraries(test_ConfigVolumes ts::inkcache Catch2::Catch2WithMain)
Review Comment:
test_ConfigVolumes links against Catch2::Catch2WithMain while
unit_tests/test_ConfigVolumes.cc also defines CATCH_CONFIG_MAIN. Adjust one
side (link Catch2::Catch2, or remove CATCH_CONFIG_MAIN) to avoid duplicate main
symbol errors.
```suggestion
target_link_libraries(test_ConfigVolumes ts::inkcache Catch2::Catch2)
```
##########
tests/gold_tests/cache/storage-metrics.test.py:
##########
@@ -27,43 +27,75 @@
"case": 0,
"description": "default config",
"storage": '''
-storage 256M
-''',
- "volume": '''
-# empty
+cache:
+ spans:
+ - name: disk.0
+ path: storage
+ size: 256M
'''
- }, {
+ },
+ {
"case": 1,
"description": "four equally devided volumes",
- "storage": '''
-storage 1G
-''',
- "volume":
+ "storage":
'''
Review Comment:
Spelling: "devided" should be "divided" in the test case description.
##########
src/iocore/cache/YamlStorageConfig.cc:
##########
@@ -0,0 +1,360 @@
+/** @file
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "P_CacheHosting.h"
+
+#include "iocore/cache/YamlStorageConfig.h"
+
+#include "tscore/Diags.h"
+
+#include <string_view>
+#include <yaml-cpp/yaml.h>
+
+#include <set>
+#include <string>
+
+namespace
+{
+std::set<std::string> valid_cache_config_keys = {"spans", "volumes"};
+std::set<std::string> valid_spans_config_keys = {"name", "path", "size",
"hash_seed"};
+std::set<std::string> valid_volumes_config_keys = {
+ "id", "size", "scheme", "ram_cache", "ram_cache_size", "ram_cache_cutoff",
"avg_obj_size", "fragment_size", "spans"};
+std::set<std::string> valid_volumes_spans_config_keys = {"use", "size"};
+
+constexpr static int MAX_VOLUME_IDX = 255;
+
+bool
+validate_map(const YAML::Node &node, const std::set<std::string> &keys)
+{
+ if (!node.IsMap()) {
+ throw YAML::ParserException(node.Mark(), "malformed entry");
+ }
+
+ for (const auto &item : node) {
+ if (std::none_of(keys.begin(), keys.end(), [&item](const std::string &s) {
return s == item.first.as<std::string>(); })) {
+ throw YAML::ParserException(item.first.Mark(), "format: unsupported key
'" + item.first.as<std::string>() + "'");
+ }
+ }
+
+ return true;
+}
+
+bool
+parse_volume_scheme(ConfigVol &volume, const std::string &scheme)
+{
+ if (scheme.compare("http") == 0) {
+ volume.scheme = CacheType::HTTP;
+ return true;
+ } else {
+ volume.scheme = CacheType::NONE;
+ return false;
+ }
+}
+
+/**
+ Convert given @s into ConfigVol::Size.
+ @s can be parcent (%), human readable unit (K/M/G/T) or absolute number.
+ */
+bool
+parse_size(ConfigVol::Size &size, const std::string &s)
+{
+ // must start with digit
+ if (!std::isdigit(s[0])) {
+ return false;
+ }
+
+ // s.ends_with('%')
+ if (s[s.length() - 1] == '%') {
+ // parcent
+ int value = std::stoi(s.substr(0, s.length() - 1));
Review Comment:
There are spelling errors in comments/docstrings (e.g., "parcent"). Please
correct these to avoid propagating typos into user-facing error messages or
future documentation.
##########
src/iocore/cache/YamlStorageConfig.cc:
##########
@@ -0,0 +1,360 @@
+/** @file
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "P_CacheHosting.h"
+
+#include "iocore/cache/YamlStorageConfig.h"
+
+#include "tscore/Diags.h"
+
+#include <string_view>
+#include <yaml-cpp/yaml.h>
+
+#include <set>
+#include <string>
+
+namespace
+{
+std::set<std::string> valid_cache_config_keys = {"spans", "volumes"};
+std::set<std::string> valid_spans_config_keys = {"name", "path", "size",
"hash_seed"};
+std::set<std::string> valid_volumes_config_keys = {
+ "id", "size", "scheme", "ram_cache", "ram_cache_size", "ram_cache_cutoff",
"avg_obj_size", "fragment_size", "spans"};
+std::set<std::string> valid_volumes_spans_config_keys = {"use", "size"};
+
+constexpr static int MAX_VOLUME_IDX = 255;
+
+bool
+validate_map(const YAML::Node &node, const std::set<std::string> &keys)
+{
+ if (!node.IsMap()) {
+ throw YAML::ParserException(node.Mark(), "malformed entry");
+ }
+
+ for (const auto &item : node) {
+ if (std::none_of(keys.begin(), keys.end(), [&item](const std::string &s) {
return s == item.first.as<std::string>(); })) {
+ throw YAML::ParserException(item.first.Mark(), "format: unsupported key
'" + item.first.as<std::string>() + "'");
+ }
+ }
+
+ return true;
+}
+
+bool
+parse_volume_scheme(ConfigVol &volume, const std::string &scheme)
+{
+ if (scheme.compare("http") == 0) {
+ volume.scheme = CacheType::HTTP;
+ return true;
+ } else {
+ volume.scheme = CacheType::NONE;
+ return false;
+ }
+}
+
+/**
+ Convert given @s into ConfigVol::Size.
+ @s can be parcent (%), human readable unit (K/M/G/T) or absolute number.
+ */
+bool
+parse_size(ConfigVol::Size &size, const std::string &s)
+{
+ // must start with digit
+ if (!std::isdigit(s[0])) {
+ return false;
+ }
+
+ // s.ends_with('%')
+ if (s[s.length() - 1] == '%') {
+ // parcent
+ int value = std::stoi(s.substr(0, s.length() - 1));
+ if (value > 100) {
+ return false;
+ }
+ size.in_percent = true;
+ size.percent = value;
+ } else {
+ // Human-readable size
+ size.in_percent = false;
+ size.absolute_value = ink_atoi64(s.c_str());
+ }
+
+ return true;
+}
+
+} // namespace
+
+namespace YAML
+{
+////
+// spans
+//
+template <> struct convert<SpanConfig> {
+ static bool
+ decode(const Node &node, SpanConfig &spans)
+ {
+ if (!node.IsSequence()) {
+ Error("malformed data; expected sequence in cache.spans");
+ return false;
+ }
+
+ for (const auto &it : node) {
+ spans.push_back(it.as<SpanConfigParams>());
+ }
+
+ return true;
+ }
+};
+
+template <> struct convert<SpanConfigParams> {
+ static bool
+ decode(const Node &node, SpanConfigParams &span)
+ {
+ validate_map(node, valid_spans_config_keys);
+
+ if (!node["name"]) {
+ throw ParserException(node.Mark(), "missing 'name' argument in
cache.spans[]");
+ }
+ span.name = node["name"].as<std::string>();
+
+ if (!node["path"]) {
+ throw ParserException(node.Mark(), "missing 'path' argument in
cache.spans[]");
+ }
+ span.path = node["path"].as<std::string>();
+
+ // optional configs
+ if (node["size"]) {
+ // Human-readable size
+ std::string size = node["size"].as<std::string>();
+ if (!std::isdigit(size[0])) {
+ throw ParserException(node.Mark(), "Invalid size value (must start
with a number, e.g., 100G)");
+ }
+ span.size = ink_atoi64(size.c_str());
+ }
+
+ if (node["hash_seed"]) {
+ span.hash_seed = node["hash_seed"].as<std::string>();
+ };
+
+ return true;
+ }
+};
+
+////
+// volumes
+//
+template <> struct convert<ConfigVolumes> {
+ static bool
+ decode(const Node &node, ConfigVolumes &volumes)
+ {
+ if (!node.IsSequence()) {
+ Error("malformed data; expected sequence in cache.volumes");
+ return false;
+ }
+
+ int total_percent = 0;
+ for (const auto &it : node) {
+ ConfigVol volume = it.as<ConfigVol>();
+
+ if (volume.size.in_percent) {
+ total_percent += volume.size.percent;
+ }
+
+ if (total_percent > 100) {
+ Error("Total volume size added up to more than 100 percent");
+ return false;
+ }
+
+ volumes.cp_queue.enqueue(new ConfigVol(volume));
+ volumes.num_volumes++;
+ }
+
+ return true;
+ }
+};
+
+template <> struct convert<ConfigVol> {
+ static bool
+ decode(const Node &node, ConfigVol &volume)
+ {
+ validate_map(node, valid_volumes_config_keys);
+
+ if (!node["id"]) {
+ throw ParserException(node.Mark(), "missing 'id' argument in
cache.volumes[]");
+ }
+ volume.number = node["id"].as<int>();
+ if (volume.number > MAX_VOLUME_IDX) {
+ throw ParserException(node.Mark(), "Bad Volume Number");
Review Comment:
Volume id validation only checks the upper bound; ids of 0 (reserved) or
negative values are currently accepted even though the docs say [1-255]. Add a
lower-bound check (>= 1) and reject invalid ids with a clear parse error.
```suggestion
if (volume.number < 1 || volume.number > MAX_VOLUME_IDX) {
throw ParserException(node.Mark(), "Bad Volume Number (must be in
[1-255])");
```
##########
src/iocore/cache/YamlStorageConfig.cc:
##########
@@ -0,0 +1,360 @@
+/** @file
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "P_CacheHosting.h"
+
+#include "iocore/cache/YamlStorageConfig.h"
+
+#include "tscore/Diags.h"
+
+#include <string_view>
+#include <yaml-cpp/yaml.h>
+
+#include <set>
+#include <string>
+
+namespace
+{
+std::set<std::string> valid_cache_config_keys = {"spans", "volumes"};
+std::set<std::string> valid_spans_config_keys = {"name", "path", "size",
"hash_seed"};
+std::set<std::string> valid_volumes_config_keys = {
+ "id", "size", "scheme", "ram_cache", "ram_cache_size", "ram_cache_cutoff",
"avg_obj_size", "fragment_size", "spans"};
+std::set<std::string> valid_volumes_spans_config_keys = {"use", "size"};
+
+constexpr static int MAX_VOLUME_IDX = 255;
+
+bool
+validate_map(const YAML::Node &node, const std::set<std::string> &keys)
+{
+ if (!node.IsMap()) {
+ throw YAML::ParserException(node.Mark(), "malformed entry");
+ }
+
+ for (const auto &item : node) {
+ if (std::none_of(keys.begin(), keys.end(), [&item](const std::string &s) {
return s == item.first.as<std::string>(); })) {
+ throw YAML::ParserException(item.first.Mark(), "format: unsupported key
'" + item.first.as<std::string>() + "'");
+ }
+ }
+
+ return true;
+}
+
+bool
+parse_volume_scheme(ConfigVol &volume, const std::string &scheme)
+{
+ if (scheme.compare("http") == 0) {
+ volume.scheme = CacheType::HTTP;
+ return true;
+ } else {
+ volume.scheme = CacheType::NONE;
+ return false;
+ }
+}
+
+/**
+ Convert given @s into ConfigVol::Size.
+ @s can be parcent (%), human readable unit (K/M/G/T) or absolute number.
+ */
+bool
+parse_size(ConfigVol::Size &size, const std::string &s)
+{
+ // must start with digit
+ if (!std::isdigit(s[0])) {
+ return false;
+ }
+
+ // s.ends_with('%')
+ if (s[s.length() - 1] == '%') {
+ // parcent
+ int value = std::stoi(s.substr(0, s.length() - 1));
+ if (value > 100) {
+ return false;
+ }
Review Comment:
parse_size() assumes the size string is non-empty (uses s[0] and
s[s.length()-1]); an empty YAML value like "size: ''" would trigger
out-of-bounds access/UB. Add an explicit empty-string check before indexing and
consider using std::isdigit(static_cast<unsigned char>(...)) to avoid UB for
negative char values.
##########
doc/admin-guide/files/storage.yaml.en.rst:
##########
@@ -0,0 +1,514 @@
+.. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+.. include:: ../../common.defs
+
+==============
+storage.yaml
+==============
+
+.. configfile:: storage.yaml
+
+The :file:`storage.yaml` file (by default, located in
+``/usr/local/etc/trafficserver/``) lists all the files, directories, and/or
+hard disk partitions that make up the Traffic Server cache. After you
+modify the :file:`storage.yaml` file the new settings will not be effective
until Traffic Server is restarted.
+
+Format
+======
+
+The format of the :file:`storage.yaml` file is a series of lines of the form
+
+.. code-block:: yaml
+
+ cache: # file level key
+ spans: #
+ - name: # name of the span
+ path: # path to storage
+ size: # size in bytes, required for file system storage,
optional for raw device
+ hash_seed: # optional, used to isolate lookup from path changes
+ volumes: # optional
+ - id: # identifier [1-255]
+ size: # optional, size in bytes or percentage
+ scheme: # optional, default to "http"
+ ram_cache: # optional, default to "true"
+ avg_obj_size: # optional, overrides
proxy.config.cache.min_average_object_size
+ fragment_size: # optional, overrides
proxy.config.cache.target_fragment_size
+ spans: # optional
+ - use: # Span identifier
+ size: # size allocated to this volume
+
+:code:`spans` lists the raw storage used for the cache. :code:`volumes`
organizes the storage into locations for
+storing cached objects. This is very similar to operating system partitions
and file systems.
+
+For :code:`spans` the keys are
+
++---------------+-------------+-------------------------------------------------------------+
+| Key | Type | Meaning
|
++===============+=============+=============================================================+
+| name | string | Name of the span.
|
++---------------+-------------+-------------------------------------------------------------+
+| path | string | File system of the storage. This must be a
block device or |
+| | | directory.
|
++---------------+-------------+-------------------------------------------------------------+
+| size | bytes | Size in bytes. This is optional for devices
but required |
+| | | for directories.
|
++---------------+-------------+-------------------------------------------------------------+
+| hash_seed | string | Hashing for object location uses a seed to
randomize the |
+| | | hash. By default this is the path for the
span. |
++---------------+-------------+-------------------------------------------------------------+
+
+For :code:`volumes` the keys are
+
++---------------+-------------+---------------------------------------------------------------------------------------------------------+
+| Key | Type | Meaning
|
++===============+=============+=========================================================================================================+
+| id | integer | Id of the volume. Range is [1-255]. This id
can be referred |
+| | | from :file:`hosting.config`
|
++---------------+-------------+---------------------------------------------------------------------------------------------------------+
+| size | bytes | Target size of the entire volume. This can be
an absolute |
+| | _or_ | number of bytes or a percentage.
|
+| | percentage |
|
++---------------+-------------+---------------------------------------------------------------------------------------------------------+
+| scheme | enumeration | Protocol scheme, defaults to "http". Preserved
for future |
+| | string | use.
|
++---------------+-------------+---------------------------------------------------------------------------------------------------------+
+| ram_cache | boolean | Control of ram caching for this volume.
Default is ``true``. This may be desirable if you are using |
+| | | something like ramdisks, to avoid wasting RAM
and CPU time on double caching objects. |
++---------------+-------------+---------------------------------------------------------------------------------------------------------+
+| avg_obj_size | integer | Overrides the global
:ts:cv:`proxy.config.cache.min_average_object_size` configuration for this
volume. |
+| | | This is useful if you have a volume that is
dedicated for say very small objects, and you need a lot of |
+| | | directory entries to store them.
|
++---------------+-------------+---------------------------------------------------------------------------------------------------------+
+| fragment_size | integer | Overrides the global
:ts:cv:`proxy.config.cache.target_fragment_size` configuration for this volume.
|
+| | | This allows for a smaller, or larger, fragment
size for a particular volume. This may be useful |
+| | | together with ``avg_obj_size`` as well, since
a larger fragment size could reduce the number of |
+| | | directory entries needed for a large object.
Note that this setting has a maximmum value of 4MB. |
Review Comment:
Spelling: "maximmum" should be "maximum" in the table description.
```suggestion
| | | directory entries needed for a large object.
Note that this setting has a maximum value of 4MB. |
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
