Author: Mark Nottingham <mnot@pobox.com>

Bug #2376: Round-Robin becomes unbalanced when a peer dies and comes back

When a peer goes down and then comes back, its round-robin counters aren't
reset, causing it to get a disproportionate amount of traffic until it "catches
up" with the rest of the peers in the round-robin pool.

If it was down for load-related issues, this has the effect of making it more
likely that it will go down again, because it's temporarily handling the load
of the entire pool.

Normally, this isn't a concern, because the number of requests that it can get
out-of-step is relatively small (bounded to how many requests it can be given
before it is considered down -- is this 10 in all cases, or are there corner
cases?), but in an accelerator case where the origin has a process-based
request-handling model, or back-end processes are CPU-intensive, it is.

This patch resets the counters each time a peer changes state.

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index 6b3e6b33ddf0a9e279d1e27d34d91878f7a4ba8e..39aabc98c3a62d0bc85a318c844af434dd8a05cb 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -1843,8 +1843,7 @@ parse_peer(peer ** head)
     *head = p;
 
     Config.npeers++;
-
-    peerClearRR(p);
+    peerClearRRStart();
 }
 
 static void

diff --git a/src/neighbors.cc b/src/neighbors.cc
index b14ba8c06522390cb186069fe28a1e45500e3da5..9083b3ce6c6a06d06877f55404652236064a591a 100644 (file)
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -387,19 +387,67 @@ getWeightedRoundRobinParent(HttpRequest * request)
     return q;
 }
 
-/* This gets called every 5 minutes to clear the round-robin counter. */
+/**
+ * This gets called every 5 minutes to clear the round-robin counter.
+ * The exact timing is an arbitrary default, set on estimate timing of a
+ * large number of requests in a high-performance environment during the
+ * period. The larger the number of requests between cycled resets the
+ * more balanced the operations.
+ *
+ \param data   unused.
+ \todo Make the reset timing a selectable parameter in squid.conf
+ */
+static void 
+peerClearRRLoop(void *data)
+{
+    peerClearRR();
+    eventAdd("peerClearRR", peerClearRRLoop, data, 5 * 60.0, 0);
+}
+
+/**
+ * This gets called on startup and restart to kick off the peer round-robin
+ * maintenance event. It ensures that no matter how many times its called
+ * no more than one event is scheduled.
+ */
 void
-peerClearRR(void *data)
+peerClearRRStart(void)
 {
-    peer *p = (peer *)data;
-    p->rr_count -= p->rr_lastcount;
+    static int event_added = 0;
+    if (!event_added) {
+        peerClearRRLoop(NULL);
+    }
+}
 
-    if (p->rr_count < 0)
+/**
+ * Called whenever the round-robin counters need to be reset to a sane state.
+ * So far those times are:
+ \item On startup and reconfigure - to set the counters to sane initial settings.
+ \item When a peer has revived from dead, to prevent the revived peer being
+ *     flooded with requests which it has 'missed' during the down period.
+ */
+void
+peerClearRR()
+{
+    peer *p = NULL;
+    for (p = Config.peers; p; p = p->next) {
         p->rr_count = 0;
+    }
+}
 
-    p->rr_lastcount = p->rr_count;
+/**
+ * Perform all actions when a peer is detected revived.
+ */
+void
+peerAlive(peer *p)
+{
+    if (p->stats.logged_state == PEER_DEAD && p->tcp_up) {
+        debugs(15, 1, "Detected REVIVED " << neighborTypeStr(p) << ": " << p->name);
+        p->stats.logged_state = PEER_ALIVE;
+        peerClearRR();
+    }
 
-    eventAdd("peerClearRR", peerClearRR, p, 5 * 60.0, 0);
+    p->stats.last_reply = squid_curtime;
+    p->stats.probe_start = 0;
 }
 
 peer *
@@ -899,13 +947,7 @@ peerNoteDigestLookup(HttpRequest * request, peer * p, lookup_t lookup)
 static void
 neighborAlive(peer * p, const MemObject * mem, const icp_common_t * header)
 {
-    if (p->stats.logged_state == PEER_DEAD && p->tcp_up) {
-        debugs(15, 1, "Detected REVIVED " << neighborTypeStr(p) << ": " << p->name);
-        p->stats.logged_state = PEER_ALIVE;
-    }
-
-    p->stats.last_reply = squid_curtime;
-    p->stats.probe_start = 0;
+    peerAlive(p);
     p->stats.pings_acked++;
 
     if ((icp_opcode) header->opcode <= ICP_END)
@@ -943,14 +985,7 @@ neighborUpdateRtt(peer * p, MemObject * mem)
 static void
 neighborAliveHtcp(peer * p, const MemObject * mem, const htcpReplyData * htcp)
 {
-    if (p->stats.logged_state == PEER_DEAD && p->tcp_up) {
-        debugs(15, 1, "Detected REVIVED " << neighborTypeStr(p) << ": " << p->name);
-        p->stats.logged_state = PEER_ALIVE;
-    }
-
-    p->stats.last_reply = squid_curtime;
-    p->stats.probe_start = 0;
-    p->stats.pings_acked++;
+    peerAlive(p);
     p->htcp.counts[htcp->hit ? 1 : 0]++;
     p->htcp.version = htcp->version;
 }
@@ -1361,13 +1396,13 @@ peerConnectSucceded(peer * p)
 {
     if (!p->tcp_up) {
         debugs(15, 2, "TCP connection to " << p->host << "/" << p->http_port << " succeded");
-        debugs(15, 1, "Detected REVIVED " << neighborTypeStr(p) << ": " << p->name);
-        p->stats.logged_state = PEER_ALIVE;
+        p->tcp_up = PEER_TCP_MAGIC_COUNT; // NP: so peerAlive(p) works properly.
+        peerAlive(p);
        if (!p->n_addresses)
            ipcache_nbgethostbyname(p->host, peerDNSConfigure, p);
     }
-
-    p->tcp_up = PEER_TCP_MAGIC_COUNT;
+    else
+        p->tcp_up = PEER_TCP_MAGIC_COUNT;
 }
 
 static void

diff --git a/src/protos.h b/src/protos.h
index 8d438ff46609374633ebeb8090994f1c7db57a36..6fc88782d48a921a8f8e81597c80012e70de4d36 100644 (file)
--- a/src/protos.h
+++ b/src/protos.h
@@ -373,7 +373,8 @@ SQUIDCEXTERN peer *peerFindByNameAndPort(const char *, unsigned short);
 SQUIDCEXTERN peer *getDefaultParent(HttpRequest * request);
 SQUIDCEXTERN peer *getRoundRobinParent(HttpRequest * request);
 SQUIDCEXTERN peer *getWeightedRoundRobinParent(HttpRequest * request);
-SQUIDCEXTERN void peerClearRR(void *);
+SQUIDCEXTERN void peerClearRRStart(void);
+SQUIDCEXTERN void peerClearRR(void);
 SQUIDCEXTERN peer *getAnyParent(HttpRequest * request);
 SQUIDCEXTERN lookup_t peerDigestLookup(peer * p, HttpRequest * request);
 SQUIDCEXTERN peer *neighborsDigestSelect(HttpRequest * request);

diff --git a/src/structs.h b/src/structs.h
index c767db43c859dba64ef63623885675e13ac0439c..e10eceeab2d5a1bf689955776844fd2fec8bf409 100755 (executable)
--- a/src/structs.h
+++ b/src/structs.h
@@ -1109,7 +1109,6 @@ unsigned int counting:
     struct IN_ADDR addresses[10];
     int n_addresses;
     int rr_count;
-    int rr_lastcount;
     peer *next;
     int test_fd;
 #if USE_CARP