Copilot commented on code in PR #13191:
URL: https://github.com/apache/trafficserver/pull/13191#discussion_r3300336563
##########
src/iocore/cache/Store.cc:
##########
@@ -218,25 +220,96 @@ 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("proxy.config.cache.volume_filename",
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());
}
- msg += std::string(annotation.text());
+ return Result::failure("failed to load %s: %s",
ts::filename::STORAGE_YAML, msg.c_str());
}
- return Result::failure("failed to load %s: %s", ts::filename::STORAGE,
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) {
+ Note("%s exists alongside %s; the legacy file is ignored. "
+ "To resolve, either: (a) migrate %s to %s (e.g. 'traffic_ctl config
convert storage') and remove %s, "
+ "or (b) remove %s to fall back to %s.",
+ ts::filename::STORAGE, ts::filename::STORAGE_YAML,
ts::filename::STORAGE, ts::filename::STORAGE_YAML,
+ ts::filename::STORAGE, ts::filename::STORAGE_YAML,
ts::filename::STORAGE);
+ }
+
+ if (volume_config_exists) {
+ Note("%s exists alongside %s; the legacy file is ignored. "
+ "To resolve, either: (a) migrate %s to %s (e.g. 'traffic_ctl config
convert storage') and remove %s, "
+ "or (b) remove %s to fall back to %s.",
+ ts::filename::VOLUME, ts::filename::STORAGE_YAML,
ts::filename::VOLUME, ts::filename::STORAGE_YAML, ts::filename::VOLUME,
+ ts::filename::STORAGE_YAML, ts::filename::VOLUME);
+ }
+ } 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);
+ }
+
Review Comment:
This "falling back" Note is emitted before verifying that storage.config
actually exists; if neither file exists the log will claim a fallback and then
immediately fail. Consider moving/rewording the message to after the
storage.config existence check (or make it conditional) to avoid misleading
startup logs.
##########
src/iocore/cache/Store.cc:
##########
@@ -218,25 +220,96 @@ 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("proxy.config.cache.volume_filename",
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());
}
- msg += std::string(annotation.text());
+ return Result::failure("failed to load %s: %s",
ts::filename::STORAGE_YAML, msg.c_str());
}
- return Result::failure("failed to load %s: %s", ts::filename::STORAGE,
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) {
+ Note("%s exists alongside %s; the legacy file is ignored. "
+ "To resolve, either: (a) migrate %s to %s (e.g. 'traffic_ctl config
convert storage') and remove %s, "
+ "or (b) remove %s to fall back to %s.",
+ ts::filename::STORAGE, ts::filename::STORAGE_YAML,
ts::filename::STORAGE, ts::filename::STORAGE_YAML,
+ ts::filename::STORAGE, ts::filename::STORAGE_YAML,
ts::filename::STORAGE);
+ }
+
+ if (volume_config_exists) {
+ Note("%s exists alongside %s; the legacy file is ignored. "
+ "To resolve, either: (a) migrate %s to %s (e.g. 'traffic_ctl config
convert storage') and remove %s, "
+ "or (b) remove %s to fall back to %s.",
+ ts::filename::VOLUME, ts::filename::STORAGE_YAML,
ts::filename::VOLUME, ts::filename::STORAGE_YAML, ts::filename::VOLUME,
+ ts::filename::STORAGE_YAML, ts::filename::VOLUME);
Review Comment:
The PR description/docs say to *warn* when both storage.yaml and legacy
storage.config/volume.config are present, but this path logs with Note().
Consider using Warning() here (or adjust the wording/docs) so operators get the
intended severity when legacy files are being ignored.
--
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]
