Hostdb restructuring #7874

SolidWallOfCode · 2021-05-24T22:56:50Z

This is a major restructuring of the HostDB internals, which should (externally) be backwards compatible. The original goal of this work was to provide better control of sending requests to down upstreams. That turned out to require rewriting the internal data structures to support atomic operations on shared state (otherwise it is not possible to have fine grained control of requests to the same upstream). Additional work was done to be in compliance with the long term L7 Routing strategy as defined by the L7 Working group.

This has been tested in production but more should be done.

The documentation needs a lot of work.

include/ts/ts.h

iocore/hostdb/HostDB.cc

brbzull0 · 2021-05-28T14:20:15Z

iocore/hostdb/I_HostDBProcessor.h

+}
+
+inline bool
+ResolveInfo::mark_active_server_alive()


from the comments:

/** Mark the active target as alive. * * @return @c true if the target changed state. */ bool mark_active_server_alive();

how this function could return true? should the mark_up() play a role in here?

bryancall · 2021-06-14T23:11:10Z

@rob05c says he is going to take a look at it

bryancall · 2021-06-25T17:08:27Z

There is an issue with this PR and the Carp plugin.

rob05c · 2021-07-13T20:36:47Z

doc/developer-guide/core-architecture/hostdb.en.rst

+=========
+
+The primary operation for HostDB is to resolve a fully qualified domain name ("FQDN"). As noted each
+FQDN is associated with a single record. Each record has an array of items. When a resolution


Not sure I understand "FQDN is associated with a single record." FQDNs are associated with a record of each type, e.g. an FQDN can have an A and an AAAA record, right? Or does "record" here refer to a code structure, not a DNS Record?

rob05c · 2021-07-13T20:40:02Z

doc/developer-guide/core-architecture/hostdb.en.rst

+``ResolveInfo`` may contain a reference to a HostDB record, which preserves the record even if it is
+replaced due to DNS queries in other transactions. The record is not required as the resolution
+information can be supplied directly without DNS or HostDB, e.g. a plugin sets the upstream address
+explicitly. The ``resolved_p`` flag indicates if the current information is valid and ready to be


I don't fully understand what it means for resolved_p to be false, i.e. what it means for it to be "ready to be used," or why a plugin or core would set that. Docs explaining that might be helpful

rob05c · 2021-07-13T20:46:59Z

doc/developer-guide/core-architecture/hostdb.en.rst

+   provides a noticeable simplification of the code.
+
+*  Single and multiple address results are treated identically - a singleton is simply a multiple
+   of size 1. This yeilds a major simplification of the implementation.


Typo "yields"

ywkaras · 2021-07-13T22:01:40Z

iocore/hostdb/HostDB.cc

+   *
+   * @param v The variable to update.
+   */
+  ConfigDuration(V &v) : _var(&v) {}


ywkaras · 2021-07-13T22:16:53Z

iocore/hostdb/HostDB.cc

+  if (query_name.empty()) {
    if (is_byname()) {
-      Debug("hostdb", "lookup_done() failed for '%.*s'", hash.host_len, hash.host_name);
+      Debug("hostdb", "lookup_done() failed for '%.*s'", int(hash.host_name.size()), hash.host_name.data());


Constructor style-casts are not Leif-legal.

ywkaras · 2021-07-13T22:52:48Z

iocore/hostdb/HostDB.cc

+    } else {
+      break;
+    }
+  };


ywkaras · 2021-07-13T22:53:24Z

iocore/hostdb/HostDB.cc

+  HostDBInfo *live[rr.count()];
+  for (auto &target : rr) {
+    // skip dead upstreams.
+    if (rr[i].is_dead(now, fail_window)) {


Is this supposed to be target.is_dead ?

rob05c

Ok, I have some questions about things I don't understand, but I generally agree with this.

I went over all the code. I don't fully understand it, hostdb is very complex and it would take considerably longer than I have right now to get a complete understanding. But, the structure looks fine to me, and I didn't see any obvious bugs.

I also ran it, as well as added debugs to see what functions are called and when during requests. It all seems to work without issue, on successes and NXDOMAINs.

rob05c · 2021-07-14T00:11:03Z

iocore/hostdb/I_HostDBProcessor.h

+inline HostDBInfo &
+HostDBInfo::operator=(HostDBInfo const &that)
+{
+  if (this != &that) {


Is it normal to assign a HostDBInfo to itself?

rob05c · 2021-07-14T00:13:41Z

iocore/hostdb/I_HostDBProcessor.h

+    return true; // it's alive and so is valid for selection.
  }
+  // Success means this is a zombie and this thread updated the failure time.
+  return (t0 + fail_window < now) && last_failure.compare_exchange_strong(t0, now);


So this CAS will fail if another thread updated underneath us? What should the caller do if it gets a failure?

rob05c · 2021-07-14T00:33:07Z

proxy/http/HttpSM.cc

+      } else if (t_state.hdr_info.client_request.version_get() == HTTPVersion(1, 0)) {
+        t_state.dns_info.http_version = HTTP_1_0;
+      } else {
+        t_state.dns_info.http_version = HTTP_1_1;


Can this ever be H2? I don't really understand this, we're setting the http_version of the DNS record to the version the client used? Shouldn't it be the server?

rob05c · 2021-07-14T00:34:47Z

proxy/http/HttpSM.cc

-      SMDebug("dns", "[HttpTransact::HandleRequest] Skipping DNS lookup, provided by plugin");
+    } else if (t_state.dns_info.looking_up == ResolveInfo::ORIGIN_SERVER && t_state.http_config_param->no_dns_forward_to_parent &&
+               t_state.parent_result.result != PARENT_UNDEFINED) {
+      t_state.dns_info.resolved_p = true; // seems dangerous - where's the IP address?


Can you explain "dangerous?" Is this an actual bug? Or only if anything ever gets dns_info's address without checking existence?

rob05c · 2021-07-14T00:35:48Z

proxy/http/HttpTransact.cc

-  s->host_db_info.app.http_data.http_version = HTTP_1_0;
+  s->server_info.http_version = HTTP_1_0;
+  s->server_info.keep_alive   = HTTP_NO_KEEPALIVE;
+  s->server_info.http_version = HTTP_1_0;


Looks like this can just be removed, it's duplicated 2 lines up

ywkaras · 2021-07-29T02:40:22Z

iocore/hostdb/HostDB.cc

-              "Generic storage for HostDBApplicationInfo is smaller than the union storage.");
+static ts_time hostdb_hostfile_update_timestamp{TS_TIME_ZERO};
+static char hostdb_filename[PATH_NAME_MAX] = DEFAULT_HOST_DB_FILENAME;
+int hostdb_max_count                       = DEFAULT_HOST_DB_SIZE;


Should we swallow this ugliness to make sure write access to global vars like hostdb_max_count is limited to HostDB.cc?

namespace ts { template <typename T, class Tag> struct GlobalAccess; template <typename T, class Tag, T Iv = T()> class Global { public: Global() : _v{Iv} {} using Type = T; T operator()() const { return _v; } friend struct GlobalAccess<T, Tag>; private: T _v; }; } // end namespace ts // in HostDB.h struct HostDBMaxCountTag {}; using HostDBMaxCount = ts::Global<int, HostDBMaxCountTag>; HostDBMaxCount hostdb_max_count; // test int f() { return hostdb_max_count(); } // in HostDB.cc namespace ts { template <> struct GlobalAccess<int, HostDBMaxCountTag> { static int &ref() { return hostdb_max_count._v; } }; } // test void g(int i) { ts::GlobalAccess<int, HostDBMaxCountTag>::ref() = i; }

Why would that be better than

namespace { int hostdb_max_count = DEFAULT_HOST_DB_SIZE; }

Yes that's better as long as hostdb_max_count doesn't have to be used outside of HostDB.cc.

The idea of the above is to give read-only access outside of HostDB.cc.

bryancall · 2021-10-14T22:09:52Z

[approve ci]

ywkaras · 2022-01-21T18:19:01Z

iocore/hostdb/HostDB.cc

  // number of partitions
  REC_ReadConfigInt32(hostdb_partitions, "proxy.config.hostdb.partitions");
-  // how often to sync hostdb to disk
-  REC_EstablishStaticConfigInt32(hostdb_sync_frequency, "proxy.config.cache.hostdb.sync_frequency");


This change results in hostdb_sync_frequency always being 0, so no host.db file is ever written.

bneradt

Looks like a good change to me. I have some suggestions, but they're minor.

bneradt · 2022-01-28T20:28:29Z

iocore/hostdb/I_HostDBProcessor.h

+  SRV,    ///< SRV record.
+  HOST    ///< Hostname (reverse DNS)
+};
+char const *name_of(HostDBType t);


I believe this can be a constexpr function.

bneradt · 2022-01-28T20:32:35Z

iocore/hostdb/I_HostDBProcessor.h

-    /// - 1.1 tweak HostDBApplicationInfo::http_data.
-    return ts::VersionNumber(1, 1);
-  }
+  HostDBInfo &operator=(HostDBInfo const &that);


Use self_type for HostDBInfo?

bneradt · 2022-01-28T20:53:19Z

iocore/hostdb/I_HostDBProcessor.h

+  /** Select this target.
+   *
+   * @param now Current time.
+   * @param fail_window Failure window.
+   * @return Status of the selection.
+   *


Let's flesh out the description of the function, describing the intention of its use, and the semantics of the boolean returned (I think true means it's alive, but false means dead or zombie).

bneradt · 2022-01-28T20:59:07Z

iocore/hostdb/I_HostDBProcessor.h

+  /// Target has failed and is still in the blocked time window.
+  bool is_dead(ts_time now, ts_seconds fail_window);


Probably good to add an is_zombie function as well.

bneradt · 2022-01-28T21:02:50Z

iocore/hostdb/I_HostDBProcessor.h

+
+  bool is_ip_fail_timeout() const;
+
+  void refresh_ip();


Probably good to document what this function does.

bneradt · 2022-01-28T21:20:17Z

iocore/hostdb/P_HostDBProcessor.h

 struct HostDBCache {
  int start(int flags = 0);
  // Map to contain all of the host file overrides, initialize it to empty
-  Ptr<RefCountedHostsFileMap> hosts_file_ptr;
+  std::shared_ptr<HostFileMap> host_file;
+  std::shared_mutex host_file_mutex;
+
  // TODO: make ATS call a close() method or something on shutdown (it does nothing of the sort today)
-  RefCountCache<HostDBInfo> *refcountcache = nullptr;
+  RefCountCache<HostDBRecord> *refcountcache = nullptr;


Since we're changing HostDBCache, should the HOST_DB_CACHE_MAJOR_VERSION and HOST_DB_CACHE_MINOR_VERSION be updated?

https://github.com/apache/trafficserver/pull/7874/files#diff-944e3e1e73047c1013f2944bd7d5f4eb750efdee052d429ddfdbb799f319d1dbR88-R90

I'm guessing not since they are never referenced. :) Can we simply remove those defines?

bneradt · 2022-01-28T21:26:03Z

doc/developer-guide/core-architecture/hostdb.en.rst

+Each info tracks several status values for its corresponding upstream. These are
+
+*  HTTP version
+*  Last failure time
+


The HostDBInfo also has the IP address or SRV name, depending upon its type. That maybe should be mentioned here too.

bneradt · 2022-01-28T21:27:15Z

doc/developer-guide/core-architecture/hostdb.en.rst

+window has passed or the single connection succeeds. A successful connection clears the last file
+time and the info becomes alive.


"last file time" -> "last fail time"

bneradt · 2022-01-28T21:32:58Z

doc/developer-guide/core-architecture/hostdb.en.rst

+State information for the outbound connection has been moved to a refurbished ``DNSInfo`` class
+named ``ResolveInfo``. As much as possible relevant state information has been moved from the


Referencing DNSInfo is helpful to us as reviewers, but probably not after this PR is merged in. Probably reword the first sentence to:

State information for the output connection is stored in an instance of a class named ``ResolveInfo``.

bneradt · 2022-01-28T21:37:01Z

doc/developer-guide/core-architecture/hostdb.en.rst

+Currently if an upstream is marked down connections are still permitted, the only change is the
+number of retries. This has caused operational problems where dead systems are flooded with requests
+which, despite the timeouts, accumulate in ATS until ATS runs out of memory (there were instances of


These sentences use down and dead interchangeably. #7283 records the desire to generally unify and clarify the wording, but for new docs let's make the wording consistent within the doc.

The default duration of time_since_epoch() is std::chrono::high_resolution_clock::duration, which will not generally be seconds. hostDB.refcountcache->put expects the epoch count to be a number of seconds. This explicitly casts to seconds so we get that expected value. This also makes some other std::chrono time updates. Co-authored-by: bneradt <bneradt@yahooinc.com>

SolidWallOfCode added the HostDB label May 24, 2021

SolidWallOfCode added this to the 10.0.0 milestone May 24, 2021

SolidWallOfCode self-assigned this May 24, 2021

SolidWallOfCode force-pushed the hostdb-restructure branch from 427cccb to 22e0bda Compare May 25, 2021 14:58

brbzull0 reviewed May 26, 2021

View reviewed changes

include/ts/ts.h Outdated Show resolved Hide resolved

brbzull0 reviewed May 26, 2021

View reviewed changes

iocore/hostdb/HostDB.cc Outdated Show resolved Hide resolved

brbzull0 reviewed May 28, 2021

View reviewed changes

SolidWallOfCode force-pushed the hostdb-restructure branch 2 times, most recently from 08c3e53 to ee34782 Compare June 3, 2021 17:31

SolidWallOfCode force-pushed the hostdb-restructure branch 2 times, most recently from f7a9428 to 7221fd1 Compare June 10, 2021 21:10

SolidWallOfCode marked this pull request as ready for review June 11, 2021 06:03

SolidWallOfCode requested review from bryancall and zwoop as code owners June 11, 2021 06:03

SolidWallOfCode force-pushed the hostdb-restructure branch from 5e56c8a to 330a203 Compare June 17, 2021 15:13

bryancall added the New Feature label Jun 25, 2021

rob05c reviewed Jul 13, 2021

View reviewed changes

ywkaras reviewed Jul 13, 2021

View reviewed changes

iocore/hostdb/HostDB.cc

*

* @param v The variable to update.

*/

ConfigDuration(V &v) : _var(&v) {}

Copy link

Contributor

ywkaras Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit?

ywkaras reviewed Jul 13, 2021

View reviewed changes

iocore/hostdb/HostDB.cc

} else {

break;

}

};

Copy link

Contributor

ywkaras Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray ;

ywkaras reviewed Jul 13, 2021

View reviewed changes

rob05c reviewed Jul 20, 2021

View reviewed changes

ywkaras reviewed Jul 29, 2021

View reviewed changes

jrushford self-requested a review August 13, 2021 13:50

ywkaras reviewed Jan 21, 2022

View reviewed changes

ywkaras mentioned this pull request Jan 25, 2022

Replace exclusive locks with rwlocks in hostdb. #8511

Closed

bneradt requested changes Jan 28, 2022

View reviewed changes

SolidWallOfCode added 3 commits April 11, 2022 11:40

Rebase checkpoint for HostDB restructure.

b93a2bc

Post rebase fixup.

eb3060a

Fix PreWarm

630a2e8

SolidWallOfCode force-pushed the hostdb-restructure branch from 0830c0c to 630a2e8 Compare April 11, 2022 18:47

SolidWallOfCode and others added 2 commits April 11, 2022 18:47

Fix nexthop test.

86c776e

SolidWallOfCode closed this Jun 23, 2022

SolidWallOfCode deleted the hostdb-restructure branch June 23, 2022 20:42

SolidWallOfCode mentioned this pull request Jul 12, 2022

Hostdb Restructure #8953

Merged

zwoop removed this from the 10.0.0 milestone Jan 17, 2023

		/// Target has failed and is still in the blocked time window.
		bool is_dead(ts_time now, ts_seconds fail_window);

		window has passed or the single connection succeeds. A successful connection clears the last file
		time and the info becomes alive.

		State information for the outbound connection has been moved to a refurbished ``DNSInfo`` class
		named ``ResolveInfo``. As much as possible relevant state information has been moved from the

Hostdb restructuring #7874

Hostdb restructuring #7874

Uh oh!

Conversation

SolidWallOfCode commented May 24, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryancall commented Jun 14, 2021

Uh oh!

bryancall commented Jun 25, 2021

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!

rob05c left a comment

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!

ywkaras Nov 11, 2021 • 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!

bryancall commented Oct 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bneradt left a comment

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!

Reviewers

Assignees

ywkaras Nov 11, 2021 •

edited

Loading