Copilot commented on code in PR #13191:
URL: https://github.com/apache/trafficserver/pull/13191#discussion_r3286494122
##########
src/traffic_server/traffic_server.cc:
##########
@@ -784,7 +784,7 @@ register_config_files()
ConfigSource::FileOnly);
// Static (non-reloadable) files only.
- reg.register_static_file("storage", ts::filename::STORAGE, {}, true);
+ reg.register_static_file("storage", ts::filename::STORAGE_YAML, {}, true);
Review Comment:
With legacy fallback enabled, registering only storage.yaml as a required
static file means FileManager/config registry will still treat storage.yaml as
required (and will not inventory storage.config/volume.config when they are
actually used). Consider marking storage.yaml as not-required and registering
the legacy storage.config and volume.config (keyed separately) so
filemanager.get_files_registry reflects the effective configuration files in
both modes.
##########
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 warning emitted when both storage.yaml and the legacy volume config
exist always prints the literal filename "volume.config"
(ts::filename::VOLUME). Since the volume config path is configurable via
proxy.config.cache.volume_filename, this message can be misleading; include the
resolved path (volume_config_path) in the log text so operators can identify
which file is being ignored.
##########
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);
+ }
+
+ auto storage_result = parser.parse(storage_config_path.get());
+ if (!storage_result.ok()) {
+ std::string msg;
+ for (auto const &annotation : storage_result.errata) {
+ if (!msg.empty()) {
+ msg += "; ";
+ }
+ msg += std::string(annotation.text());
+ }
+ return Result::failure("failed to load %s: %s", ts::filename::STORAGE,
msg.c_str());
+ }
+
+ config::StorageConfig volume_config;
+ if (volume_config_exists) {
+ config::VolumeParser volume_parser;
+ config::ConfigResult<config::StorageConfig> volume_result =
volume_parser.parse(volume_config_path.get());
+ if (!volume_result.ok()) {
+ std::string msg;
+ for (auto const &annotation : volume_result.errata) {
+ if (!msg.empty()) {
+ msg += "; ";
+ }
+ msg += std::string(annotation.text());
+ }
+ return Result::failure("failed to load %s: %s", ts::filename::VOLUME,
msg.c_str());
+ }
+ volume_config = std::move(volume_result.value);
+ } else {
+ Note("%s not found, treating as no volumes configured",
ts::filename::VOLUME);
Review Comment:
When volume.config is absent, the log says it is treated as "no volumes
configured", but legacy storage.config can still define volumes via volume=
annotations and merge_legacy_storage_configs will preserve them. Reword this
message to avoid claiming there are no volumes when storage.config may have
implicit volume assignments (and consider logging the resolved volume file path
here too).
--
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]
