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

[Alex Rousskov]

On 05/15/2017 08:31 AM, Eduard Bagdasaryan wrote:
> I followed your plan with few adjustments and reattached the patch.

LGTM. Will commit soon if there are not objections.

Alex.


> 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.
> 
> 
> 
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>