[squid-dev] [PATCH] Do not revive unconditionally dead peers after DNS refresh

[Eduard Bagdasaryan]

I followed your plan with few adjustments and reattached the patch.
Since the new CachePeer::reprobe is a kind of 'helper' flag with
default value, I think no other initialization/reporting/cloning steps
needed (as for similar CachePeer::tcp_up).

Eduard.

On 06.05.2017 00:33, Alex Rousskov wrote:
> On 04/27/2017 02:39 PM, Eduard Bagdasaryan wrote:
>> +    // always start probing in order to effectively detect
>> +    // dead or revived peers
>> +    (void)peerProbeConnect(p);
> I think we can simplify that comment while making it more precise:
>
>      peerProbeConnect(p); // detect any died or revived peers ASAP
>
> The (void) cast was correct, but is no longer needed after v5 r15133.
>
> The above are minor polishing touches that could be done during commit,
> but I am writing this message because I think we may be able to cover
> one more corner case without significant changes...
>
> Imagine a situation where the peers are already being probed during
> peerDNSConfigure(). Patches Squid will do nothing nothing in this case.
> However, the current probes may be using old/wrong IP addresses (or
> other peer configuration parameters) and, hence, may produce wrong
> result. This lack of a fresh probe may delay the discovery of a died or
> revived peer and might even cause an HTTP transaction failure (in the
> "the peer has recently died" case).
>
> On the other hand, we still do not want to create too many concurrent or
> too frequent probes so we want to continue to honor the current
> peerProbeConnect() concurrency and rate limits.
>
> Please see if the following small change allows Squid to handle the
> above case better:
>
> 1. Add a boolean CachePeer::reprobe field, defaulted to false. It can be
> described as "whether to do another TCP probe after the current TCP probes".
>
> 2. peerProbeConnect() should clear the reprobe field after passing the
> two if-statement guards and before entering the loop (even if there are
> no IPs to probe). The right timing here would eliminate any implicit
> reprobing loops while providing the desired functionality.
>
> 3. After closing the connection, peerProbeConnectDone() should call
> peerProbeConnect() if reprobe is true.
>
> 4. Add a boolean reprobeIfBusy parameter to peerProbeConnect() with
> false as the default value. peerDNSConfigure() will call
> peerProbeConnect() with a true parameter value. peerProbeConnect() will
> start by setting reprobe to (reprobe || reprobeIfBusy).
>
> There may be more initialization/reporting/cloning steps needed, just
> like for any other CachePeer data member, but the above reflects the
> core logic. Adjust as needed, of course.
>
>
> Thank you,
>
> Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-287-do-not-revive-dead-peers-after-dns-refresh-t2.patch
Type: text/x-patch
Size: 10887 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170515/2e93a017/attachment.bin>