[squid-dev] [PATCH] pconn_lifetime

[Tsantilas Christos]

Ping....

So which is the decision?
Can the patch go to trunk?
Should the patch replaced with on other solution?

On 09/02/2014 07:25 PM, Alex Rousskov wrote:
> On 09/02/2014 06:28 AM, Amos Jeffries wrote:
>> On 2/09/2014 7:38 p.m., Tsantilas Christos wrote:
>>> On 09/02/2014 03:51 AM, Amos Jeffries wrote:
>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>>>
>>>> On 2/09/2014 4:49 a.m., Tsantilas Christos wrote:
>>>>> Hi all,
>>>>>
>>>>> This patch add a new configuration option the
>>>>> 'pconn_lifetime' to allow users set the desired maximum
>>>>> lifetime of a persistent connection.
>>>>>
>>>>> When set, Squid will close a now-idle persistent connection
>>>>> that exceeded configured lifetime instead of moving the
>>>>> connection into the idle connection pool (or equivalent). No
>>>>> effect on ongoing/active transactions. Connection lifetime
>>>>> is the time period from the connection acceptance or opening
>>>>> time until "now".
>>>>>
>>>>> This limit is useful in environments with long-lived
>>>>> connections where Squid configuration or environmental
>>>>> factors change during a single connection lifetime. If
>>>>> unrestricted, some connections may last for hours and even
>>>>> days, ignoring those changes that should have affected their
>>>>> behavior or their existence.
>>>>>
>>>>> This is a Measurement Factory project
>>>>
>>>> Two problems with this.
>>>>
>>>> * the directive name does not indicate whether it applies to
>>>> client, server, or ICAP conections.
>>
>>> It applies to any connection, server, client or ICAP
>>> connections.
>>
>> Those are very distinct groups of connections. With very different
>> lifetime properties.
>
> Those properties are about the same as far as this option is
> concerned. It would be possible to split this option into 4 or more
> different ones, to allow for more precise control, but I think that
> should not be done until there is a specific use case for such a split.
>
>
>>>> * the rationale for its need is a bit screwey. Any directives
>>>> which affect Squid current running state need to ensure in
>>>> their post-reconfigure logics that state is consistent.
>>>> Preferrably create a runner, or use configDoConfigure if that
>>>> is to much.
>
> The alternative you suggest is bit screwy on two counts:
>
> 1) It does not address environmental factors outside of Squid
> knowledge. A trivial example would be typical IP table rules that
> allow already accepted connections. If the acceptance criteria change,
> the admin may want the already accepted connections to be nicely
> closed to make sure they do not violate the new rules.
>
> 2) Given the number of squid.conf directives with side-effects (and
> side-effects of side-effects), it is unlikely that a reasonable effort
> would be sufficient to identify all of them. Moreover, in some cases,
> it would probably be very difficult to judge whether there is a side
> effect that warrants connection closure. While I agree that we should
> improve reconfigure handling in this area (and have posted specific
> proposals for that), I do not think what you are suggesting is a
> practical alternative for the proposed new directive in the
> foreseeable future.
>
>
>>> Do you mean to run over the persistent connections list and
>>> change the timeout?
>>
>> I mean:
>>
>> 1) for the client connections list, set the flag forcing keep-alive
>> to terminate after current request. - only if the ports config
>> changed.
>
> Port configuration adjustments is only one of the many changes the new
> option is meant to address. Moreover, correctly determining what port
> configuration changes should lead to automatic pconn closures is very
> difficult!
>
>
>> 2) For the server pconn idle pools call close() on all existing
>> comnnections. - but why? what condition requires this?
>
> Unknown. The option is for admins that know what those conditions are.
>
>
>> 3) for the cache_peer standby pools, calling close() - only if the
>> peer config changed.
>
> Same as (1) above.
>
>
>> 4) for the ICAP idle pools calling close(). - but why? what
>> condition needs this other than ICAP config changes, and thus
>> parent object holding them open being destructed?
>
> Same as (2) above.
>
>
>> 5) for the ICAP currently active connections set a flag closing
>> after current request. - but again why? what condition has
>> changed?
>
> Same as (2) above.
>
>
>
>>>> To support that we need some API updates to globally access
>>>> the currently active connection lists for servers/client/icap.
>>>> But there are other things like stats and reporting also
>>>> needing that API, so we should look at adding it instead of yet
>>>> another "temporary" workaround.
>>
>>> We do not need to make any change to support ICAP and server
>>> side connections. This is because we are storing this connections
>>> to IdleConnList or PconnPool structures.
>>
>>> Currently we can not change timeout for client-side keep-alived
>>> connections. We do not have a list with these connections
>>> available.
>>
>>
>> That is what I refer to as needed API. Stats reporting and delay
>> pool reassignnment also need this std::list of active
>> ConnStateData connections.
>
> I agree that having an API to iterate idle user pconns would be
> useful. Implementing that correctly is not easy because those pconns
> are owned by other objects. Do you insist that the API is developed as
> a precondition for this patch acceptance, despite the facts that it it
> is not needed for the new feature to work well, and that the proposed
> changes to the user connection code are very small/simple/unintrusive?
>
>
> Thank you,
>
> Alex.
>