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 04227056885eaa30b34b5fb2720aef60811decae..60ae59b223351a9d3e00b765bff7d5d239bb9e2d 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -1863,7 +1863,7 @@ parse_peer(peer ** head)
 
     *head = p;
 
-    peerClearRR(p);
+    peerClearRRStart();
 }
 
 static void

diff --git a/src/neighbors.cc b/src/neighbors.cc
index 2336375e1848e6e45ec05cd6d72c91619722927c..9418187cd862dacfc0142c25f9618c5e4e24ef2a 100644 (file)
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -384,19 +384,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 *
@@ -857,13 +905,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)
@@ -901,14 +943,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;
 }
@@ -1317,13 +1352,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 b3603b0da283543f7bac2c6e2759aeb0abebcd1b..313358bace63e7b9723f4c55aedd902c66f19e33 100644 (file)
--- a/src/protos.h
+++ b/src/protos.h
@@ -390,7 +390,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 95b2b33de61f4392dc6f6fbf666357c44e9a0c56..852d3a2fd33c2076e34d018f3e9c145a8a383501 100644 (file)
--- a/src/structs.h
+++ b/src/structs.h
@@ -963,7 +963,6 @@ struct peer
     IPAddress addresses[10];
     int n_addresses;
     int rr_count;
-    int rr_lastcount;
     peer *next;
     int test_fd;