Show configuration file in reports #1790
Conversation
Ensure the ConfigFile macro is always set, even if it only points at the default squid.conf location. Update 'squid -v' and 'mgr:config' output to name the squid.conf which is being used. This can significantly help admin when trying to solve reconfiguration issues.
|
Inspired by the current squid-users thread where admin appears to be running one configuration and reporting "not-working" issues when editing different files. This is a re-curing problem that comes up every few years. |
|
Current output ( With this PR.. Default output ( Custom config file ( |
yadij
added
M-cleared-for-merge
| ConfigFile = xstrdup(DEFAULT_CONFIG_FILE); | ||
|
|
There was a problem hiding this comment.
PR description: Ensure the ConfigFile macro is always set, even if it only points at the default squid.conf location.
I see no good reason for this code change and it feels like a step backwards: Why set a global variable to an incorrect (in some cases) value? Until command line arguments are parsed, we do not know the configuration file name. It may or may not be the default name.
| ConfigFile = xstrdup(DEFAULT_CONFIG_FILE); |
The suggestion above restores official code but may require other PR adjustments.
There was a problem hiding this comment.
This change is what produces squid -v output:
Squid Cache: Version 7.0.0-VCS
Service Name: squid
Running Configuration in: /usr/local/squid/etc/squid.conf
Build configure options: ....
instead of:
Squid Cache: Version 7.0.0-VCS
Service Name: squid
Running Configuration in: (null)
Build configure options: ....
There was a problem hiding this comment.
This change is what produces
squid -voutput: ...
Yes, I am aware. This fact does not address the concern detailed in this change request. Clearly, the same output1 can be produced without this problematic code change (e.g., by adjusting PR-added code that produces that output).
Footnotes
| @@ -668,7 +668,8 @@ gen_dump(const EntryList &head, std::ostream &fout) | |||
| "static void" << std::endl << | |||
| "dump_config(StoreEntry *entry)" << std::endl << | |||
| "{" << std::endl << | |||
| " debugs(5, 4, MYNAME);" << std::endl; | |||
| " debugs(5, 4, MYNAME);" << std::endl << | |||
| " storeAppendPrintf(entry, \"# File: %s\\n\", ConfigFile);" << std::endl; | |||
There was a problem hiding this comment.
I am not violently against adding comments to the processed configuration file dump even though I am worried that this may open a Pandora box of arguing what comments are appropriate. There are a lot of things we could report here!
Please adjust this comment text to clarify that this is the top-level configuration filename rather than the only configuration file.
Reporting all configuration files used is better than reporting just the top-level one. I do not insist on this enhancement in this PR, but please keep it (i.e. future wording/context changes) in mind when adjusting this text in this PR.
There was a problem hiding this comment.
Suggestions welcome, I am not set on specifics here beyond that it does have to be a #comment. IMO the fact of being top-level should be obvious to the admin reading it - usually by the fact it is /some/path/to/squid.conf.
The other files loaded would be nice, but are not yet recorded anywhere to be included in this output. I am leaving them until the ConfigParser refactoring is done.
There was a problem hiding this comment.
IMO the fact of being top-level should be obvious to the admin reading it
I agree, but that is not the concern detailed in this change request. That concern is that the admin may think that the named file is "the only configuration file".
Suggestions welcome ... [Reporting] The other files loaded would be nice, but ... I am leaving them until the ConfigParser refactoring is done.
If this PR does not improve code to (at least) detect use/presence of other configuration files, then
I suggest
- "# Top-level configuration file: ..."
- "# Based on top-level configuration file: ..."
- "# Derived from top-level configuration file: ..."
... or something like that. Future PRs may add a list of various secondary configuration files used.
| @@ -650,7 +650,9 @@ mainHandleCommandLineOption(const int optId, const char *optValue) | |||
| printf("For legal restrictions on distribution see https://www.openssl.org/source/license.html\n\n"); | |||
| #endif | |||
| #endif | |||
| printf( "configure options: %s\n", SQUID_CONFIGURE_OPTIONS); | |||
|
|
|||
| printf("Running Configuration in: %s\n", ConfigFile); | |||
There was a problem hiding this comment.
AFAICT from sample output reports (thank you for providing those!), this line will appear in "squid -v" output. I do not think it should appear there (because it will often not match the reality of a running Squid instance), especially when "squid -v" command line does not include "-f".
There was a problem hiding this comment.
This line appearing in -v output is the point. Such that admin can verify that their expected config file and/or command-line options used alongside -k parse are correct.
Consider an admin running a locally built Squid installed over a system package. Running Squid would default to /usr/local/squid/etc/squid.conf and squid -k parse report no errors. But admin editing the system /etc/squid/squid.conf. In such a case starting Squid manually would be fine loading the /usr/local config and not doing what the admin actually wanted. But service squid start could crash loading a broken /etc/squid/squid.conf.
There was a problem hiding this comment.
Alex: AFAICT ... this line will appear in "squid -v" output. I do not think it should appear there (because it will often not match the reality of a running Squid instance), especially when "squid -v" command line does not include "-f".
Amos: This line appearing in
-voutput is the point. Such that admin can verify that their expected config file and/or command-line options used alongside-k parseare correct.
The above response does not seem to be related to (and, hence, cannot address) my concern.
Consider an admin running a locally built Squid installed over a system package. Running Squid would default to
/usr/local/squid/etc/squid.confandsquid -k parsereport no errors. But admin editing the system/etc/squid/squid.conf. In such a case starting Squid manually would be fine loading the/usr/localconfig and not doing what the admin actually wanted. Butservice squid startcould crash loading a broken/etc/squid/squid.conf.
$ squid -k parse
2024/04/30 18:14:27| Processing Configuration File: <filename> (depth 0)
$ sudo squid
$ head -n 1 cache.log
2024/04/30 18:14:50| Processing Configuration File: <filename> (depth 0)
I assume the "Consider an admin..." example above is implying that this specific admin will run squid -v and realize that squid -k parse and squid are using a different configuration file than the file the admin is editing; this particular admin does not notice a different configuration filename in squid -k parse output and in cache.log produced by squid (see the samples above for both), but does notice it in squid -v output.
To address my concern while still supporting the above use case, we can adjust squid -v (without -f) output to report "Default configuration filename:" instead of the proposed "Running Configuration in:".
rousskov
added
S-waiting-for-author
yadij
added
S-waiting-for-reviewer
squid-anubis
added
M-failed-other
rousskov
added
S-waiting-for-author
squid-anubis
added
M-failed-other
Ensure the ConfigFile macro is always set, even if it only
points at the default squid.conf location.
Update 'squid -v' and 'mgr:config' output to name the squid.conf
which is being used. This can significantly help admin when
trying to solve reconfiguration issues. By comparing the two
filenames they can tell whether the right file(s) are being
updated and tested.