You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by jrushford <gi...@git.apache.org> on 2016/11/11 20:46:41 UTC
[GitHub] trafficserver pull request #1218: TS-5050: The background_fetch plugin fails...
GitHub user jrushford opened a pull request:
https://github.com/apache/trafficserver/pull/1218
TS-5050: The background_fetch plugin fails to check for config files \u2026
This may be from a regression with TS-2682. Anyway, with the following remap config that we've been using with ATS 5.3.2, the background_fetch config file could not be found and read. The config file is located at /opt/trafficserver/etc/trafficserver/bg_fetch.config
@plugin=background_fetch.so @pparam=bg_fetch.config
This works:
@plugin=background_fetch.so @pparam=etc/trafficserver/bg_fetch.config
This PR restores the original behavior so that both configurations shown above work.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jrushford/trafficserver TS-5050
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1218.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1218
----
commit 42ffc818416a5486d42bb51190f064db05e7e4e5
Author: John J. Rushford <jr...@apache.org>
Date: 2016-11-11T20:31:33Z
TS-5050: The background_fetch plugin fails to check for config files relative to the config dir etc/trafficserver.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1218
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/1094/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1218: TS-5050: The background_fetch plugin fails...
Posted by jrushford <gi...@git.apache.org>.
Github user jrushford closed the pull request at:
https://github.com/apache/trafficserver/pull/1218
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1218
Please add Milestones, Labels etc...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1218: TS-5050: The background_fetch plugin fails...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1218#discussion_r87948595
--- Diff: plugins/background_fetch/configs.cc ---
@@ -44,8 +44,13 @@ BgFetchConfig::readConfig(const char *config_file)
snprintf(file_path, sizeof(file_path), "%s/%s", TSInstallDirGet(), config_file);
file = TSfopen(file_path, "r");
if (nullptr == file) {
- TSError("[%s] invalid config file", PLUGIN_NAME);
- return false;
+ TSDebug(PLUGIN_NAME, "Failed to open config file %s, trying config path", config_file);
+ snprintf(file_path, sizeof(file_path), "%s/%s", TSConfigDirGet(), config_file);
+ file = TSfopen(file_path, "r");
+ if (NULL == file) {
+ TSError("[%s] invalid config file", PLUGIN_NAME);
+ return false;
+ }
--- End diff --
This should be:
```C
if (*config_file == '/') {
file = TSfopen(config_file, "r");
} else {
file = path.join(TSConfigDirGet(), config_file);
}
```
By trying the given path without qualification you allow operators to accidentally depend on the current working directory. The pseudocode above is what all the other plugins do, and I think that it is worth being consistent with this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1218
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/1096/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1218
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1201/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/1218
@jrushford Since you are fixing this, please make sure that the new behavior is consistent with other plugins that load files. IIRC absolute paths are consumes as is, relative paths are relative to the config directory.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1218: TS-5050: The background_fetch plugin fails...
Posted by jrushford <gi...@git.apache.org>.
Github user jrushford commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1218#discussion_r88068883
--- Diff: plugins/background_fetch/configs.cc ---
@@ -44,8 +44,13 @@ BgFetchConfig::readConfig(const char *config_file)
snprintf(file_path, sizeof(file_path), "%s/%s", TSInstallDirGet(), config_file);
file = TSfopen(file_path, "r");
if (nullptr == file) {
- TSError("[%s] invalid config file", PLUGIN_NAME);
- return false;
+ TSDebug(PLUGIN_NAME, "Failed to open config file %s, trying config path", config_file);
+ snprintf(file_path, sizeof(file_path), "%s/%s", TSConfigDirGet(), config_file);
+ file = TSfopen(file_path, "r");
+ if (NULL == file) {
+ TSError("[%s] invalid config file", PLUGIN_NAME);
+ return false;
+ }
--- End diff --
ok, see changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1218
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1203/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by jrushford <gi...@git.apache.org>.
Github user jrushford commented on the issue:
https://github.com/apache/trafficserver/pull/1218
see the changes @jpeach.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by jrushford <gi...@git.apache.org>.
Github user jrushford commented on the issue:
https://github.com/apache/trafficserver/pull/1218
@jpeach - tested and the fix consumes absolute paths, relative paths to install directory, and it now consumes config files relative to the config directory
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by jrushford <gi...@git.apache.org>.
Github user jrushford commented on the issue:
https://github.com/apache/trafficserver/pull/1218
@zwoop I just put back the code used previous to TS-2682 in background_fetch.cc. Didn't spend too much time on it, just wanted our configs to be read wen we deploy 6.2. I can go back and clean it up.
@jpeach I haven't looked at every remap plugin to see how they open and read they're config files. Just put back the code from before the regression.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/1218
Is this change consistent with other plugins are doing?
> On Nov 11, 2016, at 1:07 PM, Leif Hedstrom <no...@github.com> wrote:
>
> @zwoop approved this pull request.
>
> I'm ok with this, but it feels a little clunky that we duplicate the codes / error checking nested like this. Since it's C++, couldn't we iterate over a vector of candidate directories?
>
> \u2014
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1218: TS-5050: The background_fetch plugin fails to che...
Posted by jrushford <gi...@git.apache.org>.
Github user jrushford commented on the issue:
https://github.com/apache/trafficserver/pull/1218
whew.... wipes sweat from furrowed brow :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---