You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/05/04 21:11:15 UTC
[GitHub] [trafficserver] sudheerv opened a new pull request #7782: Short circuit remap reload when a valid remap file is not specified
sudheerv opened a new pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782
This fixes issue #7767
Ensures resiliency against config errors erasing remaps previously loaded.
Note that failing to load remap.config during startup will no longer fail
the TS startup to allow for optionality in remap.config. The missing remaps
would need to be detected via functional tests
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv removed a comment on pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv removed a comment on pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#issuecomment-832896532
[approve ci autest]
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#discussion_r626139829
##########
File path: proxy/ReverseProxy.cc
##########
@@ -67,7 +67,8 @@ init_reverse_proxy()
Note("%s loading ...", ts::filename::REMAP);
if (!rewrite_table->load()) {
- Fatal("%s failed to load", ts::filename::REMAP);
+ Warning("%s failed to load", ts::filename::REMAP);
+ return 0;
Review comment:
Related, but not the same.
> It seems reasonable that we'd only enable remap reloads, if there was a valid remap during start up.
I strongly disagree with that, as it means we can't do discovery to pull in "remap.config", which is something we're looking at with regard to containerization of ATS.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#issuecomment-832323473
[approve ci autest]
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv removed a comment on pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv removed a comment on pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#issuecomment-832323473
[approve ci autest]
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#discussion_r626118317
##########
File path: proxy/ReverseProxy.cc
##########
@@ -67,7 +67,7 @@ init_reverse_proxy()
Note("%s loading ...", ts::filename::REMAP);
if (!rewrite_table->load()) {
- Fatal("%s failed to load", ts::filename::REMAP);
+ Warning("%s failed to load", ts::filename::REMAP);
Review comment:
+1 makes sense.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#discussion_r626130874
##########
File path: proxy/ReverseProxy.cc
##########
@@ -67,7 +67,8 @@ init_reverse_proxy()
Note("%s loading ...", ts::filename::REMAP);
if (!rewrite_table->load()) {
- Fatal("%s failed to load", ts::filename::REMAP);
+ Warning("%s failed to load", ts::filename::REMAP);
+ return 0;
Review comment:
But, the code and the configs that are being added past this if check all seem to be somehow related to remaps though?
>> Note there should be nothing to prevent adding a valid "remap.config" file after process start. It's just another variant of reloading.
I'd say this is more of what we _want_ to support though. It seems reasonable that we'd only enable remap reloads, if there was a valid remap during start up.
``` REC_RegisterConfigUpdateFunc("proxy.config.url_remap.filename", url_rewrite_CB, (void *)FILE_CHANGED);
REC_RegisterConfigUpdateFunc("proxy.config.proxy_name", url_rewrite_CB, (void *)TSNAME_CHANGED);
REC_RegisterConfigUpdateFunc("proxy.config.reverse_proxy.enabled", url_rewrite_CB, (void *)REVERSE_CHANGED);
REC_RegisterConfigUpdateFunc("proxy.config.http.referer_default_redirect", url_rewrite_CB, (void *)HTTP_DEFAULT_REDIRECT_CHANGED);
// Hold at least one lease, until we reload the configuration
rewrite_table->acquire();```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#issuecomment-832896532
[approve ci autest]
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode edited a comment on pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode edited a comment on pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#issuecomment-832965203
Why add the all the code of `cleanTables` when you could, in `UrlRewrite::BuildTable` add something like
```
int zret = 0;
// blah blah
if (! remap_parse_config()) {
zret = 3;
}
// table cleaning code
return zret;
```
That's only an additional 2 or 3 lines.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#issuecomment-832997651
> Why add the all the code of `cleanTables` when you could, in `UrlRewrite::BuildTable` add something like
>
> ```
> int zret = 0;
> // blah blah
> if (! remap_parse_config()) {
> zret = 3;
> }
> // table cleaning code
> return zret;
> ```
>
> That's only an additional 2 or 3 lines.
Duh, sorry totally overlooked it. Thank you for the suggestion!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#discussion_r626141936
##########
File path: proxy/ReverseProxy.cc
##########
@@ -67,7 +67,8 @@ init_reverse_proxy()
Note("%s loading ...", ts::filename::REMAP);
if (!rewrite_table->load()) {
- Fatal("%s failed to load", ts::filename::REMAP);
+ Warning("%s failed to load", ts::filename::REMAP);
+ return 0;
Review comment:
After further discussion with @SolidWallOfCode and @randall we will allow the on-the-fly updates independent of the startup remap load status.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#discussion_r626130874
##########
File path: proxy/ReverseProxy.cc
##########
@@ -67,7 +67,8 @@ init_reverse_proxy()
Note("%s loading ...", ts::filename::REMAP);
if (!rewrite_table->load()) {
- Fatal("%s failed to load", ts::filename::REMAP);
+ Warning("%s failed to load", ts::filename::REMAP);
+ return 0;
Review comment:
But, the code and the configs that are being added past this if check all seem to be somehow related to remaps though?
``` REC_RegisterConfigUpdateFunc("proxy.config.url_remap.filename", url_rewrite_CB, (void *)FILE_CHANGED);
REC_RegisterConfigUpdateFunc("proxy.config.proxy_name", url_rewrite_CB, (void *)TSNAME_CHANGED);
REC_RegisterConfigUpdateFunc("proxy.config.reverse_proxy.enabled", url_rewrite_CB, (void *)REVERSE_CHANGED);
REC_RegisterConfigUpdateFunc("proxy.config.http.referer_default_redirect", url_rewrite_CB, (void *)HTTP_DEFAULT_REDIRECT_CHANGED);
// Hold at least one lease, until we reload the configuration
rewrite_table->acquire();```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a change in pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
randall commented on a change in pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#discussion_r626116582
##########
File path: proxy/ReverseProxy.cc
##########
@@ -67,7 +67,7 @@ init_reverse_proxy()
Note("%s loading ...", ts::filename::REMAP);
if (!rewrite_table->load()) {
- Fatal("%s failed to load", ts::filename::REMAP);
+ Warning("%s failed to load", ts::filename::REMAP);
Review comment:
should we return early here? So we don't see a "finished loading" in the failure case?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#issuecomment-832965203
Why add the all the code of `cleanTables` when you could, in `UrlRewrite::BuildTable` add something like
```
int zret = 0;
// blah blah
if (! table->load()) {
zret = 3;
}
// table cleaning code
return zret;
```
That's only an additional 2 or 3 lines.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv merged pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv merged pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#discussion_r626125336
##########
File path: proxy/ReverseProxy.cc
##########
@@ -67,7 +67,8 @@ init_reverse_proxy()
Note("%s loading ...", ts::filename::REMAP);
if (!rewrite_table->load()) {
- Fatal("%s failed to load", ts::filename::REMAP);
+ Warning("%s failed to load", ts::filename::REMAP);
+ return 0;
Review comment:
I must disagree with @randall - this return will prevent unrelated configuration update handlers from being installed. See lines 74-77. The GC protection on `rewrite_table` is also lost - that might cause a dangling pointer. You might do better to add an `else`.
```
if (rewrite_table.load()) {
Note("loaded");
} else {
Warning("failed");
}
```
Note there should be nothing to prevent adding a valid "remap.config" file after process start. It's just another variant of reloading.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7782: Short circuit remap reload when a valid remap file is not specified
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7782:
URL: https://github.com/apache/trafficserver/pull/7782#discussion_r626130874
##########
File path: proxy/ReverseProxy.cc
##########
@@ -67,7 +67,8 @@ init_reverse_proxy()
Note("%s loading ...", ts::filename::REMAP);
if (!rewrite_table->load()) {
- Fatal("%s failed to load", ts::filename::REMAP);
+ Warning("%s failed to load", ts::filename::REMAP);
+ return 0;
Review comment:
But, the code and the configs that are being added past this if check all seem to be somehow related to remaps though?
``` REC_RegisterConfigUpdateFunc("proxy.config.url_remap.filename", url_rewrite_CB, (void *)FILE_CHANGED);
REC_RegisterConfigUpdateFunc("proxy.config.proxy_name", url_rewrite_CB, (void *)TSNAME_CHANGED);
REC_RegisterConfigUpdateFunc("proxy.config.reverse_proxy.enabled", url_rewrite_CB, (void *)REVERSE_CHANGED);
REC_RegisterConfigUpdateFunc("proxy.config.http.referer_default_redirect", url_rewrite_CB, (void *)HTTP_DEFAULT_REDIRECT_CHANGED);
// Hold at least one lease, until we reload the configuration
rewrite_table->acquire();```
>> Note there should be nothing to prevent adding a valid "remap.config" file after process start. It's just another variant of reloading.
I'd say this is more of what we _want_ to support though. It seems reasonable that we'd only enable remap reloads, if there was a valid remap during start up.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org