Copilot commented on code in PR #13191:
URL: https://github.com/apache/trafficserver/pull/13191#discussion_r3285792373
##########
src/iocore/cache/Store.cc:
##########
@@ -218,25 +220,90 @@ Span::~Span()
Result
Store::read_config()
{
- ats_scoped_str storage_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE));
+ config::StorageConfig parsed;
+
+ // Check storage.yaml, storage.config and volume.config
+ ats_scoped_str storage_yaml_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE_YAML));
+ const bool yaml_exists = storage_yaml_path &&
swoc::file::exists(swoc::file::path(storage_yaml_path.get()));
+
+ ats_scoped_str storage_config_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE));
+ const bool storage_config_exists = storage_config_path &&
swoc::file::exists(swoc::file::path(storage_config_path.get()));
- Note("%s loading ...", ts::filename::STORAGE);
+ ats_scoped_str volume_config_path(RecConfigReadConfigPath(nullptr,
ts::filename::VOLUME));
+ const bool volume_config_exists = volume_config_path &&
swoc::file::exists(swoc::file::path(volume_config_path.get()));
config::StorageParser parser;
- auto parse_result = parser.parse(storage_path.get());
- if (!parse_result.ok()) {
- std::string msg;
- for (auto const &annotation : parse_result.errata) {
- if (!msg.empty()) {
- msg += "; ";
+
+ if (yaml_exists) {
+ Note("%s loading ...", ts::filename::STORAGE_YAML);
+
+ auto parse_result = parser.parse(storage_yaml_path.get());
+ if (!parse_result.ok()) {
+ std::string msg;
+ for (auto const &annotation : parse_result.errata) {
+ if (!msg.empty()) {
+ msg += "; ";
+ }
+ msg += std::string(annotation.text());
+ }
+ return Result::failure("failed to load %s: %s",
ts::filename::STORAGE_YAML, msg.c_str());
+ }
+ parsed = std::move(parse_result.value);
+
+ // If a legacy storage.config or volume.config also exists, warn that it
is being ignored.
+ if (storage_config_exists) {
+ Warning("%s exists alongside %s; the legacy file is ignored. "
+ "Use 'traffic_ctl config convert storage' to migrate or remove
storage.yaml",
+ ts::filename::STORAGE, ts::filename::STORAGE_YAML);
+ }
+
+ if (volume_config_exists) {
+ Warning("%s exists alongside %s; the legacy file is ignored. "
+ "Use 'traffic_ctl config convert storage' to migrate or remove
storage.yaml",
+ ts::filename::VOLUME, ts::filename::STORAGE_YAML);
Review Comment:
The warning text says to "migrate or remove storage.yaml" when storage.yaml
is present and storage.config is being ignored. In this scenario operators
should remove the legacy file (storage.config), not storage.yaml; the current
message is misleading.
##########
src/iocore/cache/Store.cc:
##########
@@ -218,25 +220,90 @@ Span::~Span()
Result
Store::read_config()
{
- ats_scoped_str storage_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE));
+ config::StorageConfig parsed;
+
+ // Check storage.yaml, storage.config and volume.config
+ ats_scoped_str storage_yaml_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE_YAML));
+ const bool yaml_exists = storage_yaml_path &&
swoc::file::exists(swoc::file::path(storage_yaml_path.get()));
+
+ ats_scoped_str storage_config_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE));
+ const bool storage_config_exists = storage_config_path &&
swoc::file::exists(swoc::file::path(storage_config_path.get()));
- Note("%s loading ...", ts::filename::STORAGE);
+ ats_scoped_str volume_config_path(RecConfigReadConfigPath(nullptr,
ts::filename::VOLUME));
+ const bool volume_config_exists = volume_config_path &&
swoc::file::exists(swoc::file::path(volume_config_path.get()));
config::StorageParser parser;
- auto parse_result = parser.parse(storage_path.get());
- if (!parse_result.ok()) {
- std::string msg;
- for (auto const &annotation : parse_result.errata) {
- if (!msg.empty()) {
- msg += "; ";
+
+ if (yaml_exists) {
+ Note("%s loading ...", ts::filename::STORAGE_YAML);
+
+ auto parse_result = parser.parse(storage_yaml_path.get());
+ if (!parse_result.ok()) {
+ std::string msg;
+ for (auto const &annotation : parse_result.errata) {
+ if (!msg.empty()) {
+ msg += "; ";
+ }
+ msg += std::string(annotation.text());
+ }
+ return Result::failure("failed to load %s: %s",
ts::filename::STORAGE_YAML, msg.c_str());
+ }
+ parsed = std::move(parse_result.value);
+
+ // If a legacy storage.config or volume.config also exists, warn that it
is being ignored.
+ if (storage_config_exists) {
+ Warning("%s exists alongside %s; the legacy file is ignored. "
+ "Use 'traffic_ctl config convert storage' to migrate or remove
storage.yaml",
+ ts::filename::STORAGE, ts::filename::STORAGE_YAML);
+ }
+
+ if (volume_config_exists) {
+ Warning("%s exists alongside %s; the legacy file is ignored. "
+ "Use 'traffic_ctl config convert storage' to migrate or remove
storage.yaml",
Review Comment:
Same issue as above: when storage.yaml exists and volume.config is ignored,
the warning advises removing storage.yaml, but the operator action should be to
remove the legacy volume.config (or keep it knowing it is ignored). Update the
message accordingly.
##########
src/iocore/cache/Store.cc:
##########
@@ -218,25 +220,90 @@ Span::~Span()
Result
Store::read_config()
{
- ats_scoped_str storage_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE));
+ config::StorageConfig parsed;
+
+ // Check storage.yaml, storage.config and volume.config
+ ats_scoped_str storage_yaml_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE_YAML));
+ const bool yaml_exists = storage_yaml_path &&
swoc::file::exists(swoc::file::path(storage_yaml_path.get()));
+
+ ats_scoped_str storage_config_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE));
+ const bool storage_config_exists = storage_config_path &&
swoc::file::exists(swoc::file::path(storage_config_path.get()));
- Note("%s loading ...", ts::filename::STORAGE);
+ ats_scoped_str volume_config_path(RecConfigReadConfigPath(nullptr,
ts::filename::VOLUME));
+ const bool volume_config_exists = volume_config_path &&
swoc::file::exists(swoc::file::path(volume_config_path.get()));
config::StorageParser parser;
- auto parse_result = parser.parse(storage_path.get());
- if (!parse_result.ok()) {
- std::string msg;
- for (auto const &annotation : parse_result.errata) {
- if (!msg.empty()) {
- msg += "; ";
+
+ if (yaml_exists) {
+ Note("%s loading ...", ts::filename::STORAGE_YAML);
+
+ auto parse_result = parser.parse(storage_yaml_path.get());
+ if (!parse_result.ok()) {
+ std::string msg;
+ for (auto const &annotation : parse_result.errata) {
+ if (!msg.empty()) {
+ msg += "; ";
+ }
+ msg += std::string(annotation.text());
+ }
+ return Result::failure("failed to load %s: %s",
ts::filename::STORAGE_YAML, msg.c_str());
+ }
+ parsed = std::move(parse_result.value);
+
+ // If a legacy storage.config or volume.config also exists, warn that it
is being ignored.
+ if (storage_config_exists) {
+ Warning("%s exists alongside %s; the legacy file is ignored. "
+ "Use 'traffic_ctl config convert storage' to migrate or remove
storage.yaml",
+ ts::filename::STORAGE, ts::filename::STORAGE_YAML);
+ }
+
+ if (volume_config_exists) {
+ Warning("%s exists alongside %s; the legacy file is ignored. "
+ "Use 'traffic_ctl config convert storage' to migrate or remove
storage.yaml",
+ ts::filename::VOLUME, ts::filename::STORAGE_YAML);
+ }
+ } else {
+ // Fall back to legacy storage.config + volume.config.
+ Note("%s not found, falling back to %s + %s", ts::filename::STORAGE_YAML,
ts::filename::STORAGE, ts::filename::VOLUME);
+
+ if (!storage_config_exists) {
+ return Result::failure("neither %s nor %s found",
ts::filename::STORAGE_YAML, ts::filename::STORAGE);
+ }
+
+ if (!volume_config_exists) {
+ return Result::failure("neither %s nor %s found",
ts::filename::STORAGE_YAML, ts::filename::VOLUME);
+ }
Review Comment:
Fallback path currently hard-requires volume.config to exist (returns
failure if missing). That conflicts with the documented behavior in
storage.yaml.en.rst which says volume.config is optional and missing should be
treated as "no volumes configured". Either adjust the implementation to allow a
missing/empty volume.config (e.g., merge with an empty volumes config), or
update the docs to state it is required.
##########
doc/admin-guide/files/storage.yaml.en.rst:
##########
@@ -559,3 +559,27 @@ shares the remaining 1.5GB (4GB - 2GB - 512MB) and caches
objects up to 1MB.
- id: 3
size: 40%
ram_cache_cutoff: 1M
+
+
+Backward compatibility
+======================
+
+For backward compatibility |TS| also accepts the legacy
+:file:`storage.config` and :file:`volume.config` files. The selection rule at
+startup is:
+
+* If :file:`storage.yaml` exists in the configuration directory, it is loaded
+ and any :file:`storage.config` / :file:`volume.config` present alongside it
+ is ignored. A warning is logged in this case so that operators notice the
+ legacy file is being skipped.
+* If :file:`storage.yaml` does not exist, |TS| falls back to
+ :file:`storage.config` (required) and :file:`volume.config` (optional).
+ A missing :file:`volume.config` is treated as "no volumes configured".
Review Comment:
The backward-compatibility section states volume.config is optional when
storage.yaml is absent, but the current implementation in
src/iocore/cache/Store.cc fails startup if volume.config is missing. Please
align the documentation with the actual behavior (or update the implementation
to match the documented optional behavior).
##########
src/iocore/cache/Store.cc:
##########
@@ -218,25 +220,90 @@ Span::~Span()
Result
Store::read_config()
{
- ats_scoped_str storage_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE));
+ config::StorageConfig parsed;
+
+ // Check storage.yaml, storage.config and volume.config
+ ats_scoped_str storage_yaml_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE_YAML));
+ const bool yaml_exists = storage_yaml_path &&
swoc::file::exists(swoc::file::path(storage_yaml_path.get()));
+
+ ats_scoped_str storage_config_path(RecConfigReadConfigPath(nullptr,
ts::filename::STORAGE));
+ const bool storage_config_exists = storage_config_path &&
swoc::file::exists(swoc::file::path(storage_config_path.get()));
- Note("%s loading ...", ts::filename::STORAGE);
+ ats_scoped_str volume_config_path(RecConfigReadConfigPath(nullptr,
ts::filename::VOLUME));
+ const bool volume_config_exists = volume_config_path &&
swoc::file::exists(swoc::file::path(volume_config_path.get()));
Review Comment:
Store::read_config() resolves volume.config using
RecConfigReadConfigPath(nullptr, ts::filename::VOLUME), which ignores the
configurable record proxy.config.cache.volume_filename. This makes custom
volume.config locations ineffective even though the record exists. Use
RecConfigReadConfigPath("proxy.config.cache.volume_filename",
ts::filename::VOLUME) (and base existence checks on that) so runtime behavior
matches the configuration knob.
##########
src/iocore/cache/Cache.cc:
##########
@@ -971,6 +971,6 @@ ink_cache_init(ts::ModuleVersion v)
Result result = theCacheStore.read_config();
if (result.failed()) {
- Fatal("Failed to read cache configuration %s: %s", ts::filename::STORAGE,
result.message());
+ Fatal("Failed to read cache configuration %s: %s",
ts::filename::STORAGE_YAML, result.message());
Review Comment:
This fatal log always names storage.yaml as the cache configuration file,
but Store::read_config() can successfully fall back to
storage.config/volume.config and failures may mention those files. Consider
making this message filename-agnostic (or reflect which source was attempted)
to avoid misleading operators during legacy fallback.
--
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]
