Make negative caching configurable with error HTTP status codes #3953

nqyy · 2018-07-11T18:40:48Z

This PR is a resurrection of #2031.
This will resolve the issue: #3211.

A new configurable variable is added to records.config: proxy.config.http.negative_caching_list.
It enables users to self-define status code for negative caching.

SolidWallOfCode · 2018-07-11T20:56:42Z

proxy/http/HttpConfig.h

  MgmtByte server_session_sharing_pool = TS_SERVER_SESSION_SHARING_POOL_THREAD;

+  // Vector to hold the status codes that will BE cached with negative caching enabled
+  std::vector<std::bitset<10>> codeNegCache;


Why is this a std::vector? And why 10 as the base bitset size? Why not 32 or 64? But overall, the maximum possibly status code is 599, which makes std::bitset<600> a reasonable choice. Then you can do c.codeNegCache[HTTP_STATUS_NOT_FOUND] = true;.

P.S. "codeNegCache" is a bad name. "isNegativeStatus" might be better, for something like "if (isNegativeStatus[client.info->status]) { ...}`.

I agree. I had some misunderstanding on the vector stuff.

SolidWallOfCode · 2018-07-11T20:58:59Z

proxy/http/HttpTransact.cc

-  case HTTP_STATUS_GATEWAY_TIMEOUT:
+  int status  = s->hdr_info.server_response.status_get();
+  auto params = HttpConfig::acquire();
+  if (std::binary_search(params->codeNegCache.begin(), params->codeNegCache.end(), status, cmp_func)) {


Right, riffing on the previous comment, this becomes

if (params->isNegativeStatus[status]) {

SolidWallOfCode · 2018-07-11T20:59:44Z

Where is the configuration parsing? This needs to be able to be set from records.config.

nqyy · 2018-07-12T21:51:20Z

Updated. @SolidWallOfCode

SolidWallOfCode · 2018-07-13T01:25:31Z

proxy/http/HttpConfig.cc

+{
+  char *negative_status = nullptr;
+  HttpEstablishStaticConfigStringAlloc(negative_status, config_var.data());
+  std::string buf;


You should use TextView here - it is as clean and avoids any memory allocation. Also, if the configuration is set, don't enable the defaults (because then it would be impossible to turn those off). Error checking and reporting is good too. In terms of hooking this up to the configuration file, take a look at the code in HttpConnectionCount.cc, around line 161.

char *negative_status = nullptr; ts::TextView list(config_var); if (list.empty()) { // The responses with the following status code WILL BE cached by default with negative caching enabled. set[HTTP_STATUS_NO_CONTENT] = true; set[HTTP_STATUS_USE_PROXY] = true; set[HTTP_STATUS_FORBIDDEN] = true; set[HTTP_STATUS_NOT_FOUND] = true; set[HTTP_STATUS_METHOD_NOT_ALLOWED] = true; set[HTTP_STATUS_REQUEST_URI_TOO_LONG] = true; set[HTTP_STATUS_INTERNAL_SERVER_ERROR] = true; set[HTTP_STATUS_NOT_IMPLEMENTED] = true; set[HTTP_STATUS_BAD_GATEWAY] = true; set[HTTP_STATUS_SERVICE_UNAVAILABLE] = true; set[HTTP_STATUS_GATEWAY_TIMEOUT] = true; } auto is_sep { [](char c) { return isspace(c) || ',' == c || ';' == c; } }; while (! list.ltrim_if(is_sep).empty()) { ts::TextView span, token { list.take_prefix_if(is_sep) }; auto n = ts::svtoi(token, &span); if (span.size() != token.size()) { ink_fatal("Invalid status code '%.*s' for negative caching: not a number", static_cast<int>(token.size()), token.data()); } else if (n <= 0 || n >= 600) { ink_fatal("Invalid status code '%.*s' for negative caching: out of range", static_cast<int>(token.size()), token.data()); } else { set[n] = true; } }

SolidWallOfCode · 2018-07-13T22:21:00Z

proxy/http/HttpConfig.cc

+  if (0 == strcasecmp("proxy.config.http.negative_caching_list", name) && RECD_STRING == dtype) {
+    if (data.rec_string) {
+      // parse the list of status code
+      ts::TextView status_list(std::string(data.rec_string));


Why is a std::string used here? Why not status_list(data.rec_string, strlen(data.rec_string));? The use of std::string in this way already requires a null terminate string.

SolidWallOfCode · 2018-07-13T22:22:18Z

proxy/http/HttpConfig.cc

+      ret = true;
+    }
+  }
+  if (update == true) {


Perhaps return update? Or assign to zret instead of ret or update. There's no comment indicating what exactly the return value of this function means so it's hard to be sure.

SolidWallOfCode · 2018-07-13T22:23:48Z

proxy/http/HttpConfig.cc

+  set_negative_caching_list(r->name, r->data_type, r->data, c, false);
+
+  // The responses with the following status code WILL BE cached by default with negative caching enabled.
+  c->negative_caching_list[HTTP_STATUS_NO_CONTENT]            = true;


Suppose I want to configure negative caching to not include HTTP_STATUS_FORBIDDEN. How do I do that?

In previous logic, they are there by default. Maybe, I can put them in the records.config default value but not here.

Yes, but in the previous logic they could not be configured at all. These should be used only if the incoming string is null or empty.

I agree. I'll change this logic.

SolidWallOfCode · 2018-07-16T16:44:16Z

configs/records.config.default.in

 ##############################################################################
 CONFIG proxy.config.http.negative_caching_enabled INT 0
 CONFIG proxy.config.http.negative_caching_lifetime INT 1800
+CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 405 414 500 501 502 503 504


No - it's a policy goal to reduce the amount of things in this file. The defaults should be handled if needed in RecordsConfig.cc, as you've already done.

But users need to put 204 305 403 404 405 414 500 501 502 503 504 in here on their own, and modify it if they want.

Well, if you want, you can allow a special token like "default" to mean "add all the default status codes". Therefore to add 301, it would be "default 301".

SolidWallOfCode · 2018-07-16T16:46:23Z

proxy/http/HttpConfig.cc

+    ts::TextView status_list(data.rec_string, strlen(data.rec_string));
+    while (!status_list.empty()) {
+      auto is_sep{[](char c) { return isspace(c) || ',' == c || ';' == c; }};
+      ts::TextView span, token{status_list.ltrim_if(is_sep).take_prefix_if(is_sep)};


You need to either move is_sep and the ltrim_if up, or handle the possibility of an empty token if there are trailing separators. I'd favor the former - use while (!status_list.ltrim_if(is_sep).empty()) { and move the is_sep definition to just before the while.

SolidWallOfCode · 2018-07-16T16:49:51Z

proxy/http/HttpConfig.cc

+      }
+    }
+  }
+  // set the return value


It would probably be faster to do

if ((set ^ c->negative_caching_list).any()) { c->negative_caching_list = set; ret = ret || update; }

I missed it the first time, but double checking there is an equality operator for bitset. I thought it was a bit odd to not have that.

SolidWallOfCode · 2018-07-16T16:59:10Z

proxy/http/HttpTransact.cc

-  case HTTP_STATUS_SERVICE_UNAVAILABLE:
-  case HTTP_STATUS_GATEWAY_TIMEOUT:
+  int status  = s->hdr_info.server_response.status_get();
+  auto params = HttpConfig::acquire();


I think you want to use s->http_config_params here - that is the HTTP configuration parameters acquired when the HttpSM was created (and which HttpTransact is a member). You can see other examples of this in HttpTransact methods.

OK, I see. I was following the way that the original PR was constructed.

SolidWallOfCode · 2018-07-16T17:00:29Z

proxy/http/HttpConfig.cc

 }

+static bool
+set_negative_caching_list(const char *name, RecDataT dtype, RecData data, HttpConfigParams *c, bool update)


Is this called from anywhere else? Maybe it should be folded in to negative_caching_list_cb.

negative_caching_list_cb is only called when reloading. set_negative_caching_list is called once in the first place.

nqyy · 2018-07-16T21:20:58Z

Good to go. @SolidWallOfCode

SolidWallOfCode · 2018-07-17T13:52:39Z

How much testing have you done with this?

nqyy · 2018-07-17T15:32:45Z

I did some basic "print out" tests of using both default value and configured values in records.config when running and reloading traffic server. @SolidWallOfCode

mlibbey · 2018-07-19T19:44:58Z

Worth trying the test from #3211?

$ traffic_ctl config get proxy.config.http.negative_caching_lifetime
proxy.config.http.negative_caching_lifetime: 10
$ for status in `seq 400 499`; do 
echo "####status code=$status####"; 
for retry in `seq 3`; do 
  result=$(curl -sD- -o/dev/null -x 127.0.0.1:80 "http://httpbin.org/status/$status" | egrep "(Age|X-Cache)"); 
  if [[ $retry < 3 ]]; then 
    sleep 6; 
  else if [[ "$result" == *"Age: 0"* ]]; then 
      echo "$status code works"; 
    else 
      echo "$status is cached too long"; 
    fi;
  fi; 
  done; 
done

(needs a remap like

map http://httpbin.org http://httpbin.org

SolidWallOfCode · 2018-07-19T20:52:45Z

An excellent idea. Make it so, Xavier.

zwoop · 2018-08-13T18:15:34Z

This does not cherry-pick cleanly to 8.0.x, if you would like this feature in 8.0.x, please make a PR against this branch.

bryancall · 2018-08-30T19:03:28Z

I also tried to cherry pick this change and it failed. There needs to be a new PR opened that is a backport to the 8.0.x tree. I am going to remove this PR from the 8.0.x project.

bryancall · 2018-08-31T19:12:17Z

The cherry pick conflict was minor, so I fixed it myself.

SolidWallOfCode assigned nqyy Jul 11, 2018

SolidWallOfCode added the Cache label Jul 11, 2018

SolidWallOfCode added this to the 9.0.0 milestone Jul 11, 2018

SolidWallOfCode reviewed Jul 11, 2018

View reviewed changes

nqyy force-pushed the NegativeCaching branch 2 times, most recently from 9b8d100 to 1f46202 Compare July 11, 2018 21:28

nqyy changed the title ~~Negative caching shouldn't happen on 400 response~~ Make negative caching accept configured error status codes Jul 11, 2018

nqyy force-pushed the NegativeCaching branch 4 times, most recently from d5e6cfb to 0dd7bd9 Compare July 12, 2018 21:50

SolidWallOfCode reviewed Jul 13, 2018

View reviewed changes

SolidWallOfCode mentioned this pull request Jul 13, 2018

TextView: More unit tests about separators and tokens. #3963

Merged

nqyy force-pushed the NegativeCaching branch 2 times, most recently from fd5453a to f1fd8b8 Compare July 13, 2018 22:12

SolidWallOfCode reviewed Jul 13, 2018

View reviewed changes

nqyy force-pushed the NegativeCaching branch from 55ceefd to 541421b Compare July 16, 2018 16:31

nqyy changed the title ~~Make negative caching accept configured error status codes~~ Make negative caching configurable with error HTTP status codes Jul 16, 2018

SolidWallOfCode suggested changes Jul 16, 2018

View reviewed changes

nqyy force-pushed the NegativeCaching branch from f051fcb to b6b911d Compare July 16, 2018 20:04

Make negative caching accept configured error status codes

35f25ce

nqyy force-pushed the NegativeCaching branch from 2cc7915 to 35f25ce Compare July 16, 2018 20:51

SolidWallOfCode approved these changes Jul 17, 2018

View reviewed changes

SolidWallOfCode merged commit 4c34cbc into apache:master Jul 17, 2018

nqyy deleted the NegativeCaching branch July 17, 2018 18:40

bryancall modified the milestones: 9.0.0, 8.0.0 Aug 31, 2018

Make negative caching configurable with error HTTP status codes #3953

Make negative caching configurable with error HTTP status codes #3953

Uh oh!

Conversation

nqyy commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nqyy Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode commented Jul 11, 2018

Uh oh!

nqyy commented Jul 12, 2018

Uh oh!

SolidWallOfCode Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nqyy commented Jul 16, 2018

Uh oh!

SolidWallOfCode commented Jul 17, 2018

Uh oh!

nqyy commented Jul 17, 2018

Uh oh!

mlibbey commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SolidWallOfCode commented Jul 19, 2018

Uh oh!

zwoop commented Aug 13, 2018

Uh oh!

bryancall commented Aug 30, 2018

Uh oh!

bryancall commented Aug 31, 2018

Uh oh!

nqyy commented Jul 11, 2018 •

edited

Loading

nqyy Jul 11, 2018 •

edited

Loading

SolidWallOfCode Jul 13, 2018 •

edited

Loading

mlibbey commented Jul 19, 2018 •

edited

Loading