Hello,
Attached is a patch which makes the changes recommended by Amos - each peer
now gets its own event for retrying resolution, dependent on the DNS TTL.
This should also fix up the concerns up by Alex. A few caveats though:
- the cache manager shows generic "peerRefreshDNS" names for each event. I
can't find any examples that give it a dynamic name, e.g. I'd like
something like "peerRefreshDNS(example.com)", but I can't think of how I'd
do that without leaking memory or making some significant changes to the
event handler system.
- I can't figure out how to reproduce the second failure case, where a
result comes back but it has no IP addresses. I _think_ using the TTL would
be valid instead of negative_dns_ttl would be valid in that situation, but
I can't be sure. I figured this was the safest option.
- eventDelete does not appear to be clearing out events as I expect it to,
so if you reconfigure Squid you end up with some dead events, like so:
[root@xxx ~]# squidmgr events | grep peerRefresh
Last event to run: peerRefreshDNS
peerRefreshDNS 0.331 sec 1 yes
peerRefreshDNS 0.679 sec 1 yes
peerRefreshDNS 47.649 sec 1 yes
peerRefreshDNS 61.619 sec 1 yes
peerRefreshDNS 207.682 sec 1 yes
peerRefreshDNS 207.682 sec 1 yes
peerRefreshDNS 207.682 sec 1 yes
peerRefreshDNS 207.682 sec 1 yes
peerRefreshDNS 207.682 sec 1 yes
[root@xxx ~]# squid -k
reconfigure
[root@xxx ~]# squidmgr events | grep peerRefresh
Last event to run: peerRefreshDNS
peerRefreshDNS 0.763 sec 1 yes
peerRefreshDNS 0.763 sec 1 yes
peerRefreshDNS 41.755 sec 1 yes
peerRefreshDNS 55.755 sec 1 yes
peerRefreshDNS 56.187 sec 1 no
peerRefreshDNS 202.250 sec 1 no
peerRefreshDNS 202.250 sec 1 no
peerRefreshDNS 3599.758 sec 1 yes
peerRefreshDNS 3599.758 sec 1 yes
peerRefreshDNS 3599.758 sec 1 yes
peerRefreshDNS 3599.758 sec 1 yes
peerRefreshDNS 3599.758 sec 1 yes
If I run squid -k reconfigure again, then the events with invalid callback
data are cleared out, so it doesn't grow indefinitely at least. I'm not
sure how or if I should fix this.
Thank you,
Nathan.
On 10 May 2016 at 18:13, Alex Rousskov <rouss...@measurement-factory.com>
wrote:
> On 05/10/2016 01:50 AM, Amos Jeffries wrote:
>
> > Then each peer gets its own re-lookup event scheduled
>
> If applied correctly, this approach would also solve the misapplication
> problem I described in my concurrent review. Unfortunately, it requires
> serious work. Fortunately, you have already converted CachePeer from
> being a POD into a proper class. That will help!
>
>
> Thank you,
>
> Alex.
>
>
Use DNS TTL for deciding when to reschedule resolution of cache peers, and retry failed cache peers much more often.
This improves the situation for configurations where Squid forwards all
requests to a cache peer, and a temporary DNS failure left that cache peer
unresolved. Previously this would would have left users without web access for
an hour, without manual intervention.
A summary:
* Use DNS TTL when scheduling the next resolution attempt, resolving each
cache peer independently rather than fixed at every hour.
* In the failure case, attempt another resolution according to negative_dns_ttl
* Add the TTL to the Dns::LookupDetails object.
* Fix a small but consistent typo
This work is submitted on behalf of Bloomberg L.P.
=== modified file 'src/FwdState.cc'
--- src/FwdState.cc 2016-03-12 20:27:35 +0000
+++ src/FwdState.cc 2016-05-11 00:34:29 +0000
@@ -726,41 +726,41 @@ FwdState::connectedToPeer(Security::Encr
peerConnectFailed(p);
serverConnection()->close();
return;
}
if (answer.tunneled) {
// TODO: When ConnStateData establishes tunnels, its state changes
// [in ways that may affect logging?]. Consider informing
// ConnStateData about our tunnel or otherwise unifying tunnel
// establishment [side effects].
unregister(serverConn); // async call owns it now
complete(); // destroys us
return;
}
// should reach ConnStateData before the dispatched Client job starts
CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData,
ConnStateData::notePeerConnection, serverConnection());
if (serverConnection()->getPeer())
- peerConnectSucceded(serverConnection()->getPeer());
+ peerConnectSucceeded(serverConnection()->getPeer());
flags.connected_okay = true;
dispatch();
}
void
FwdState::connectTimeout(int fd)
{
debugs(17, 2, "fwdConnectTimeout: FD " << fd << ": '" << entry->url() << "'" );
assert(serverDestinations[0] != NULL);
assert(fd == serverDestinations[0]->fd);
if (entry->isEmpty()) {
ErrorState *anErr = new ErrorState(ERR_CONNECT_FAIL, Http::scGatewayTimeout, request);
anErr->xerrno = ETIMEDOUT;
fail(anErr);
/* This marks the peer DOWN ... */
if (serverDestinations[0]->getPeer())
peerConnectFailed(serverDestinations[0]->getPeer());
=== modified file 'src/dns/LookupDetails.h'
--- src/dns/LookupDetails.h 2016-01-01 00:12:18 +0000
+++ src/dns/LookupDetails.h 2016-05-17 04:49:08 +0000
@@ -3,40 +3,41 @@
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
/* DEBUG: section 78 DNS lookups */
#ifndef SQUID_DNS_LOOKUPDETAILS_H
#define SQUID_DNS_LOOKUPDETAILS_H
#include "SquidString.h"
namespace Dns
{
/// encapsulates DNS lookup results
class LookupDetails
{
public:
- LookupDetails() : wait(-1) {} ///< no error, no lookup delay (i.e., no lookup)
- LookupDetails(const String &anError, int aWait) : error(anError), wait(aWait) {}
+ LookupDetails() : wait(-1), ttl(-1) {} ///< no error, no lookup delay (i.e., no lookup)
+ LookupDetails(const String &anError, int aWait, time_t aTtl) : error(anError), wait(aWait), ttl(aTtl) {}
std::ostream &print(std::ostream &os) const;
public:
String error; ///< error message for unsuccessful lookups; empty otherwise
int wait; ///< msecs spent waiting for the lookup (if any) or -1 (if none)
+ time_t ttl; ///< the time to live for the lookup (if any) or -1 (if none)
};
} // namespace Dns
inline std::ostream &
operator <<(std::ostream &os, const Dns::LookupDetails &dns)
{
return dns.print(os);
}
#endif /* SQUID_DNS_LOOKUPDETAILS_H */
=== modified file 'src/fqdncache.cc'
--- src/fqdncache.cc 2016-04-22 11:39:23 +0000
+++ src/fqdncache.cc 2016-05-11 02:09:21 +0000
@@ -289,41 +289,41 @@ fqdncacheAddEntry(fqdncache_entry * f)
*
* Walks down the pending list, calling handlers
*/
static void
fqdncacheCallback(fqdncache_entry * f, int wait)
{
FQDNH *callback;
void *cbdata;
f->lastref = squid_curtime;
if (!f->handler)
return;
fqdncacheLockEntry(f);
callback = f->handler;
f->handler = NULL;
if (cbdataReferenceValidDone(f->handlerData, &cbdata)) {
- const Dns::LookupDetails details(f->error_message, wait);
+ const Dns::LookupDetails details(f->error_message, wait, f->expires - squid_curtime);
callback(f->name_count ? f->names[0] : NULL, details, cbdata);
}
fqdncacheUnlockEntry(f);
}
/// \ingroup FQDNCacheInternal
static int
fqdncacheParse(fqdncache_entry *f, const rfc1035_rr * answers, int nr, const char *error_message)
{
int k;
int ttl = 0;
const char *name = (const char *)f->hash.key;
f->expires = squid_curtime + Config.negativeDnsTtl;
f->flags.negcached = true;
if (nr < 0) {
debugs(35, 3, "fqdncacheParse: Lookup of '" << name << "' failed (" << error_message << ")");
f->error_message = xstrdup(error_message);
return -1;
@@ -406,41 +406,41 @@ fqdncacheHandleReply(void *data, const r
*
\param addr IP address of domain to resolve.
\param handler A pointer to the function to be called when
* the reply from the FQDN cache
* (or the DNS if the FQDN cache misses)
\param handlerData Information that is passed to the handler
* and does not affect the FQDN cache.
*/
void
fqdncache_nbgethostbyaddr(const Ip::Address &addr, FQDNH * handler, void *handlerData)
{
fqdncache_entry *f = NULL;
char name[MAX_IPSTRLEN];
generic_cbdata *c;
addr.toStr(name,MAX_IPSTRLEN);
debugs(35, 4, "fqdncache_nbgethostbyaddr: Name '" << name << "'.");
++FqdncacheStats.requests;
if (name[0] == '\0') {
debugs(35, 4, "fqdncache_nbgethostbyaddr: Invalid name!");
- const Dns::LookupDetails details("Invalid hostname", -1); // error, no lookup
+ const Dns::LookupDetails details("Invalid hostname", -1, -1); // error, no lookup
if (handler)
handler(NULL, details, handlerData);
return;
}
f = fqdncache_get(name);
if (NULL == f) {
/* miss */
(void) 0;
} else if (fqdncacheExpiredEntry(f)) {
/* hit, but expired -- bummer */
fqdncacheRelease(f);
f = NULL;
} else {
/* hit */
debugs(35, 4, "fqdncache_nbgethostbyaddr: HIT for '" << name << "'");
if (f->flags.negcached)
++ FqdncacheStats.negative_hits;
=== modified file 'src/ipcache.cc'
--- src/ipcache.cc 2016-04-22 11:39:23 +0000
+++ src/ipcache.cc 2016-05-17 05:04:10 +0000
@@ -309,41 +309,41 @@ ipcacheAddEntry(ipcache_entry * i)
*
* walks down the pending list, calling handlers
*/
static void
ipcacheCallback(ipcache_entry *i, int wait)
{
IPH *callback = i->handler;
void *cbdata = NULL;
i->lastref = squid_curtime;
if (!i->handler)
return;
ipcacheLockEntry(i);
callback = i->handler;
i->handler = NULL;
if (cbdataReferenceValidDone(i->handlerData, &cbdata)) {
- const Dns::LookupDetails details(i->error_message, wait);
+ const Dns::LookupDetails details(i->error_message, wait, i->expires - squid_curtime);
callback((i->addrs.count ? &i->addrs : NULL), details, cbdata);
}
ipcacheUnlockEntry(i);
}
/// \ingroup IPCacheAPI
static int
ipcacheParse(ipcache_entry *i, const rfc1035_rr * answers, int nr, const char *error_message)
{
int k;
int j = 0;
int na = 0;
int ttl = 0;
const char *name = (const char *)i->hash.key;
int cname_found = 0;
i->expires = squid_curtime + Config.negativeDnsTtl;
i->flags.negcached = true;
safe_free(i->addrs.in_addrs);
@@ -492,41 +492,41 @@ ipcacheHandleReply(void *data, const rfc
* XXX: on hits and some errors, the handler is called immediately instead
* of scheduling an async call. This reentrant behavior means that the
* user job must be extra careful after calling ipcache_nbgethostbyname,
* especially if the handler destroys the job. Moreover, the job has
* no way of knowing whether the reentrant call happened.
* Comm::Connection setup usually protects the job by scheduling an async call,
* but some user code calls ipcache_nbgethostbyname directly.
*/
void
ipcache_nbgethostbyname(const char *name, IPH * handler, void *handlerData)
{
ipcache_entry *i = NULL;
const ipcache_addrs *addrs = NULL;
generic_cbdata *c;
debugs(14, 4, "ipcache_nbgethostbyname: Name '" << name << "'.");
++IpcacheStats.requests;
if (name == NULL || name[0] == '\0') {
debugs(14, 4, "ipcache_nbgethostbyname: Invalid name!");
++IpcacheStats.invalid;
- const Dns::LookupDetails details("Invalid hostname", -1); // error, no lookup
+ const Dns::LookupDetails details("Invalid hostname", -1, -1); // error, no lookup
if (handler)
handler(NULL, details, handlerData);
return;
}
if ((addrs = ipcacheCheckNumeric(name))) {
debugs(14, 4, "ipcache_nbgethostbyname: BYPASS for '" << name << "' (already numeric)");
++IpcacheStats.numeric_hits;
const Dns::LookupDetails details; // no error, no lookup
if (handler)
handler(addrs, details, handlerData);
return;
}
i = ipcache_get(name);
if (NULL == i) {
/* miss */
(void) 0;
} else if (ipcacheExpiredEntry(i)) {
=== modified file 'src/neighbors.cc'
--- src/neighbors.cc 2016-01-01 00:12:18 +0000
+++ src/neighbors.cc 2016-05-17 04:37:21 +0000
@@ -1,38 +1,39 @@
/*
* Copyright (C) 1996-2016 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
/* DEBUG: section 15 Neighbor Routines */
#include "squid.h"
#include "acl/FilledChecklist.h"
#include "anyp/PortCfg.h"
#include "base/EnumIterator.h"
#include "CacheDigest.h"
#include "CachePeer.h"
#include "comm/Connection.h"
#include "comm/ConnOpener.h"
+#include "dns/LookupDetails.h"
#include "event.h"
#include "FwdState.h"
#include "globals.h"
#include "htcp.h"
#include "HttpRequest.h"
#include "icmp/net_db.h"
#include "ICP.h"
#include "int.h"
#include "ip/Address.h"
#include "ip/tools.h"
#include "ipcache.h"
#include "MemObject.h"
#include "mgr/Registration.h"
#include "multicast.h"
#include "neighbors.h"
#include "NeighborTypeDomainList.h"
#include "pconn.h"
#include "PeerDigest.h"
#include "PeerPoolMgr.h"
#include "PeerSelectState.h"
@@ -544,41 +545,44 @@ neighbors_init(void)
if (0 != strcmp(thisPeer->host, me))
continue;
for (AnyP::PortCfgPointer s = HttpPortList; s != NULL; s = s->next) {
if (thisPeer->http_port != s->s.port())
continue;
debugs(15, DBG_IMPORTANT, "WARNING: Peer looks like this host");
debugs(15, DBG_IMPORTANT, " Ignoring " <<
neighborTypeStr(thisPeer) << " " << thisPeer->host <<
"/" << thisPeer->http_port << "/" <<
thisPeer->icp.port);
neighborRemove(thisPeer);
}
}
}
- peerRefreshDNS((void *) 1);
+ eventDelete(peerRefreshDNS, NULL);
+
+ for (CachePeer *p = Config.peers; p; p = p->next)
+ ipcache_nbgethostbyname(p->host, peerDNSConfigure, p);
sep = getservbyname("echo", "udp");
echo_port = sep ? ntohs((unsigned short) sep->s_port) : 7;
first_ping = Config.peers;
}
int
neighborsUdpPing(HttpRequest * request,
StoreEntry * entry,
IRCB * callback,
void *callback_data,
int *exprep,
int *timeout)
{
const char *url = entry->url();
MemObject *mem = entry->mem_obj;
CachePeer *p = NULL;
int i;
int reqnum = 0;
@@ -1145,145 +1149,143 @@ neighborUp(const CachePeer * p)
if (p->stats.probe_start != 0 &&
squid_curtime - p->stats.probe_start > Config.Timeout.deadPeer) {
debugs(15, 8, "neighborUp: DOWN (dead): " << p->host << " (" << p->in_addr << ")");
return 0;
}
debugs(15, 8, "neighborUp: UP: " << p->host << " (" << p->in_addr << ")");
return 1;
}
void
peerNoteDigestGone(CachePeer * p)
{
#if USE_CACHE_DIGESTS
cbdataReferenceDone(p->digest);
#endif
}
static void
-peerDNSConfigure(const ipcache_addrs *ia, const Dns::LookupDetails &, void *data)
+peerDNSConfigure(const ipcache_addrs *ia, const Dns::LookupDetails &details, void *data)
{
// TODO: connections to no-longer valid IP addresses should be
// closed when we can detect such IP addresses.
CachePeer *p = (CachePeer *)data;
int j;
if (p->n_addresses == 0) {
debugs(15, DBG_IMPORTANT, "Configuring " << neighborTypeStr(p) << " " << p->host << "/" << p->http_port << "/" << p->icp.port);
if (p->type == PEER_MULTICAST)
debugs(15, DBG_IMPORTANT, " Multicast TTL = " << p->mcast.ttl);
}
p->n_addresses = 0;
if (ia == NULL) {
debugs(0, DBG_CRITICAL, "WARNING: DNS lookup for '" << p->host << "' failed!");
+ eventAdd("peerRefreshDNS", peerRefreshDNS, cbdataReference(p), Config.negativeDnsTtl, 1);
return;
}
if ((int) ia->count < 1) {
debugs(0, DBG_CRITICAL, "WARNING: No IP address found for '" << p->host << "'!");
+ eventAdd("peerRefreshDNS", peerRefreshDNS, cbdataReference(p), Config.negativeDnsTtl, 1);
return;
}
p->tcp_up = p->connect_fail_limit;
for (j = 0; j < (int) ia->count && j < PEER_MAX_ADDRESSES; ++j) {
p->addresses[j] = ia->in_addrs[j];
debugs(15, 2, "--> IP address #" << j << ": " << p->addresses[j]);
++ p->n_addresses;
}
p->in_addr.setEmpty();
p->in_addr = p->addresses[0];
p->in_addr.port(p->icp.port);
if (p->type == PEER_MULTICAST)
peerCountMcastPeersSchedule(p, 10);
#if USE_ICMP
if (p->type != PEER_MULTICAST && IamWorkerProcess())
if (!p->options.no_netdb_exchange)
eventAddIsh("netdbExchangeStart", netdbExchangeStart, p, 30.0, 1);
#endif
if (p->standby.mgr.valid())
PeerPoolMgr::Checkpoint(p->standby.mgr, "resolved peer");
+
+ time_t ttl = details.ttl;
+
+ if (ttl <= 0)
+ ttl = Config.positiveDnsTtl;
+
+ eventAdd("peerRefreshDNS", peerRefreshDNS, cbdataReference(p), ttl, 1);
}
static void
peerRefreshDNS(void *data)
{
- CachePeer *p = NULL;
-
- if (eventFind(peerRefreshDNS, NULL))
- eventDelete(peerRefreshDNS, NULL);
-
- if (!data && 0 == stat5minClientRequests()) {
- /* no recent client traffic, wait a bit */
- eventAddIsh("peerRefreshDNS", peerRefreshDNS, NULL, 180.0, 1);
- return;
- }
-
- for (p = Config.peers; p; p = p->next)
+ if (cbdataReferenceValid(data)) {
+ CachePeer *p = (CachePeer *)data;
ipcache_nbgethostbyname(p->host, peerDNSConfigure, p);
-
- /* Reconfigure the peers every hour */
- eventAddIsh("peerRefreshDNS", peerRefreshDNS, NULL, 3600.0, 1);
+ }
+ cbdataReferenceDone(data);
}
static void
peerConnectFailedSilent(CachePeer * p)
{
p->stats.last_connect_failure = squid_curtime;
if (!p->tcp_up) {
debugs(15, 2, "TCP connection to " << p->host << "/" << p->http_port <<
" dead");
return;
}
-- p->tcp_up;
if (!p->tcp_up) {
debugs(15, DBG_IMPORTANT, "Detected DEAD " << neighborTypeStr(p) << ": " << p->name);
p->stats.logged_state = PEER_DEAD;
}
}
void
peerConnectFailed(CachePeer *p)
{
debugs(15, DBG_IMPORTANT, "TCP connection to " << p->host << "/" << p->http_port << " failed");
peerConnectFailedSilent(p);
}
void
-peerConnectSucceded(CachePeer * p)
+peerConnectSucceeded(CachePeer * p)
{
if (!p->tcp_up) {
- debugs(15, 2, "TCP connection to " << p->host << "/" << p->http_port << " succeded");
+ debugs(15, 2, "TCP connection to " << p->host << "/" << p->http_port << " succeeded");
p->tcp_up = p->connect_fail_limit; // NP: so peerAlive(p) works properly.
peerAlive(p);
if (!p->n_addresses)
ipcache_nbgethostbyname(p->host, peerDNSConfigure, p);
} else
p->tcp_up = p->connect_fail_limit;
}
/*
* peerProbeConnect will be called on dead peers by neighborUp
*/
static bool
peerProbeConnect(CachePeer * p)
{
time_t ctimeout = p->connect_timeout > 0 ? p->connect_timeout : Config.Timeout.peer_connect;
bool ret = (squid_curtime - p->stats.last_connect_failure) > (ctimeout * 10);
if (p->testing_now > 0)
return ret;/* probe already running */
@@ -1300,41 +1302,41 @@ peerProbeConnect(CachePeer * p)
++ p->testing_now;
AsyncCall::Pointer call = commCbCall(15,3, "peerProbeConnectDone", CommConnectCbPtrFun(peerProbeConnectDone, p));
Comm::ConnOpener *cs = new Comm::ConnOpener(conn, call, ctimeout);
cs->setHost(p->host);
AsyncJob::Start(cs);
}
p->stats.last_connect_probe = squid_curtime;
return ret;
}
static void
peerProbeConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int, void *data)
{
CachePeer *p = (CachePeer*)data;
if (status == Comm::OK) {
- peerConnectSucceded(p);
+ peerConnectSucceeded(p);
} else {
peerConnectFailedSilent(p);
}
-- p->testing_now;
conn->close();
// TODO: log this traffic.
}
static void
peerCountMcastPeersSchedule(CachePeer * p, time_t when)
{
if (p->mcast.flags.count_event_pending)
return;
eventAdd("peerCountMcastPeersStart",
peerCountMcastPeersStart,
p,
(double) when, 1);
=== modified file 'src/neighbors.h'
--- src/neighbors.h 2016-01-01 00:12:18 +0000
+++ src/neighbors.h 2016-05-11 00:35:28 +0000
@@ -39,35 +39,35 @@ void neighborsUdpAck(const cache_key *,
void neighborAdd(const char *, const char *, int, int, int, int, int);
void neighbors_init(void);
#if USE_HTCP
void neighborsHtcpClear(StoreEntry *, const char *, HttpRequest *, const HttpRequestMethod &, htcp_clr_reason);
#endif
CachePeer *peerFindByName(const char *);
CachePeer *peerFindByNameAndPort(const char *, unsigned short);
CachePeer *getDefaultParent(HttpRequest * request);
CachePeer *getRoundRobinParent(HttpRequest * request);
CachePeer *getWeightedRoundRobinParent(HttpRequest * request);
void peerClearRRStart(void);
void peerClearRR(void);
lookup_t peerDigestLookup(CachePeer * p, HttpRequest * request);
CachePeer *neighborsDigestSelect(HttpRequest * request);
void peerNoteDigestLookup(HttpRequest * request, CachePeer * p, lookup_t lookup);
void peerNoteDigestGone(CachePeer * p);
int neighborUp(const CachePeer * e);
const char *neighborTypeStr(const CachePeer * e);
peer_t neighborType(const CachePeer *, const URL &);
void peerConnectFailed(CachePeer *);
-void peerConnectSucceded(CachePeer *);
+void peerConnectSucceeded(CachePeer *);
void dump_peer_options(StoreEntry *, CachePeer *);
int peerHTTPOkay(const CachePeer *, HttpRequest *);
/// Whether we can open new connections to the peer (e.g., despite max-conn)
bool peerCanOpenMore(const CachePeer *p);
/// Whether the peer has idle or standby connections that can be used now
bool peerHasConnAvailable(const CachePeer *p);
/// Notifies peer of an associated connection closure.
void peerConnClosed(CachePeer *p);
CachePeer *whichPeer(const Ip::Address &from);
#endif /* SQUID_NEIGHBORS_H_ */
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
