TS-4747: make marking parent proxies down in hostdb configurable. #1464

jrushford · 2017-02-17T17:10:12Z

For the use case where a parent in parent.config has multiple ip's for failover, TS-4747 added marking the parent down in hostdb so that subsequent requests to the parent would go to another ip/host. For parent proxies listed in parent.config tat are not using multiple ips for failover, it is not desirable to mark the parent proxy down in hostdb as the parent selection system will manage the availability of the parent.

This PR reverts to the original behavior where parent proxies are not marked down in hostdb but adds a configuration option so that need parent proxies marked down in hostdb may enable it.

atsci · 2017-02-17T17:20:18Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/12/ for details.

zwoop · 2017-02-17T17:18:32Z

mgmt/RecordsConfig.cc

@@ -518,6 +518,8 @@ static const RecordElement RecordsConfig[] =
  ,
  {RECT_CONFIG, "proxy.config.http.parent_proxy.connect_attempts_timeout", RECD_INT, "30", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
  ,
+  {RECT_CONFIG, "proxy.config.http.update_hostdb_on_parent_proxy_connect_errors", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_INT, "[0-1]", RECA_NULL}


This is a rather long name of a configuration ... :) And it doesn't feel properly "name spaced" ? How about

CONFIG proxy.config.hostdb.mark_parent_down INT 0

or perhaps

CONFIG proxy.config.http.parent_proxy.mark_down_hostdb INT 0

Fwiw, I like the first of my two alternative best.

yeah it is long, i prefer your second suggestion since it's used in the http transaction to update or not hostdb. All the ones named spaced to proxy.config.hostdb are internal to hostdb and its configuration. But if you prefer #1, I'll use it.

zwoop · 2017-02-17T17:20:19Z

proxy/http/HttpConfig.cc

@@ -1255,6 +1257,7 @@ HttpConfig::reconfigure()
  params->oride.parent_connect_attempts       = m_master.oride.parent_connect_attempts;
  params->per_parent_connect_attempts         = m_master.per_parent_connect_attempts;
  params->parent_connect_timeout              = m_master.parent_connect_timeout;
+  params->parent_failures_update_hostdb       = m_master.parent_failures_update_hostdb;


Can we make this configuration overridable instead? Or are we positive that this is something we always want completely on, or completely off? In general, new configurations ought to be overridable unless it's impossible to do so (i.e. configs outside of the HttpSM are difficult to make overridable).

Thats a good point. In our case we would never mark a parent down in hostdb so completely off works for us. However making it overidable is a good idea. Let me make changes.

atsci · 2017-02-17T17:25:08Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1579/ for details.

atsci · 2017-02-17T17:26:14Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1473/ for details.

atsci · 2017-02-17T17:37:30Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/144/ for details.

atsci · 2017-02-17T18:14:17Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1580/ for details.

atsci · 2017-02-17T18:14:28Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/13/ for details.

atsci · 2017-02-17T18:21:17Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1474/ for details.

atsci · 2017-02-17T18:32:06Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/145/ for details.

atsci · 2017-02-17T20:31:39Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1476/ for details.

atsci · 2017-02-17T20:32:32Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/15/ for details.

atsci · 2017-02-17T20:33:00Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1582/ for details.

atsci · 2017-02-17T20:47:12Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/147/ for details.

jrushford · 2017-02-17T22:06:17Z

@zwoop, @bryancall, @PSUdaemon - changes were made, see last two commits. changed the config variable name and made it overridable. Tested with header_rewrite plugin and traffic_ctl

jrushford · 2017-02-17T22:06:30Z

[approve ci]

atsci · 2017-02-17T22:19:29Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/16/ for details.

atsci · 2017-02-17T22:19:40Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1583/ for details.

atsci · 2017-02-17T22:21:10Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1477/ for details.

atsci · 2017-02-17T22:36:50Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/148/ for details.

zwoop · 2017-02-18T01:29:26Z

proxy/InkAPI.cc

@@ -8153,6 +8153,10 @@ _conf_to_memberp(TSOverridableConfigKey conf, OverridableHttpConfigParams *overr
    typ = OVERRIDABLE_TYPE_STRING;
    ret = &overridableHttpConfig->client_cert_filepath;
    break;
+  case TS_CONFIG_PARENT_FAILURES_UPDATE_HOSTDB:
+    typ = OVERRIDABLE_TYPE_INT;


Now that I see this, this type should probably be 'byte'. See some of the earlier examples how to do that (both in the SM structs as well as here). OVERRiDABLE_TYPE_BYTE is also the default, and we should use that (I probably need to take a look at this, to make sure it's done correctly elsewhere as well).

Also, I think the way this was written, the string types were all collected towards the end. Not sure that's important any more, but might be worth keeping that semantics?

zwoop · 2017-02-18T01:30:44Z

proxy/http/HttpConfig.h

@@ -563,6 +564,7 @@ struct OverridableHttpConfigParams {
  // hostdb/dns variables //
  //////////////////////////
  MgmtByte srv_enabled;
+  MgmtInt parent_failures_update_hostdb;


So, similar to above, but i think this should be MgmtByte.

jrushford · 2017-02-20T17:56:58Z

changed type on parent_failures_update_hostdb from MgmtInt to MgmtByte.

atsci · 2017-02-20T18:05:10Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/22/ for details.

atsci · 2017-02-20T18:09:57Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1589/ for details.

atsci · 2017-02-20T18:11:14Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1483/ for details.

atsci · 2017-02-20T18:22:25Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/154/ for details.

jrushford · 2017-02-22T22:54:24Z

@zwoop All changes are in, see the commits.

zwoop

+1

atsci · 2017-02-23T16:49:19Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1605/ for details.

atsci · 2017-02-23T16:49:27Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/37/ for details.

atsci · 2017-02-23T16:51:28Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1499/ for details.

atsci · 2017-02-23T17:00:11Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/38/ for details.

atsci · 2017-02-23T17:01:56Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1500/ for details.

atsci · 2017-02-23T17:02:05Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1606/ for details.

atsci · 2017-02-23T17:07:26Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/169/ for details.

atsci · 2017-02-23T17:12:23Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/170/ for details.

jrushford added the HostDB label Feb 17, 2017

jrushford added this to the 7.1.1 milestone Feb 17, 2017

jrushford self-assigned this Feb 17, 2017

jrushford requested review from zwoop, bryancall and PSUdaemon February 17, 2017 17:10

zwoop requested changes Feb 17, 2017

View reviewed changes

zwoop requested changes Feb 18, 2017

View reviewed changes

zwoop approved these changes Feb 23, 2017

View reviewed changes

jrushford force-pushed the ts-4747 branch from a44a531 to b453d03 Compare February 23, 2017 16:38

TS-4747: make marking parent proxies down in hostdb configurable.

66e70db

jrushford force-pushed the ts-4747 branch from b453d03 to 66e70db Compare February 23, 2017 16:45

jrushford merged commit 2752c75 into apache:master Feb 23, 2017

jrushford deleted the ts-4747 branch February 23, 2017 17:16

zwoop modified the milestones: 7.1.0, 7.1.1 Feb 28, 2017

This was referenced Mar 1, 2017

TS-4747: make marking parent proxies down in hostdb configurable. #1513

Merged

TS-2888: remove vararg and format parameters from build_error_resoponse #1299

Merged

TS-4747: make marking parent proxies down in hostdb configurable. #1464

TS-4747: make marking parent proxies down in hostdb configurable. #1464

Conversation

jrushford commented Feb 17, 2017

atsci commented Feb 17, 2017

zwoop Feb 17, 2017

Choose a reason for hiding this comment

zwoop Feb 17, 2017

Choose a reason for hiding this comment

jrushford Feb 17, 2017

Choose a reason for hiding this comment

zwoop Feb 17, 2017

Choose a reason for hiding this comment

jrushford Feb 17, 2017

Choose a reason for hiding this comment

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

jrushford commented Feb 17, 2017

jrushford commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

atsci commented Feb 17, 2017

zwoop Feb 18, 2017

Choose a reason for hiding this comment

zwoop Feb 18, 2017

Choose a reason for hiding this comment

jrushford commented Feb 20, 2017

atsci commented Feb 20, 2017

atsci commented Feb 20, 2017

atsci commented Feb 20, 2017

atsci commented Feb 20, 2017

jrushford commented Feb 22, 2017

zwoop left a comment

Choose a reason for hiding this comment

atsci commented Feb 23, 2017

atsci commented Feb 23, 2017

atsci commented Feb 23, 2017

atsci commented Feb 23, 2017

atsci commented Feb 23, 2017

atsci commented Feb 23, 2017

atsci commented Feb 23, 2017

atsci commented Feb 23, 2017