Re: [PATCH] pconn_lifetime

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.
Received on Tue Sep 02 2014 - 16:25:24 MDT