trafficserver-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sudheer Vinukonda <sudheervinuko...@yahoo.com>
Subject Re: Remap plugin DSO reloading - enabling and disabling
Date Sat, 09 May 2020 23:55:20 GMT
The only concern I’ve with an API solution (options 3, 4) is exactly that. What if a user
wants to also not reload (some of the) built-in plugins? Without that, it feels incomplete
and inconsistent to me. A config driven approach can address that.

In any case, I think it comes down to the degree of how optional, we want to make the behavior.
I’d be okay with the consensus, but IMHO, we should allow *any* plugin to be non-reloadable
if a user chooses to. It should not be limited to only plugins that are either used in a mixed
mode or only the plugins that a user has code/control over. 

Apologize, if I’m missing something, but I’m yet to hear any convincing arguments as to
*why* this feature should not be optional. Again, I’d be totally okay with a consensus on
this.

In fact, if we all agree that this should NOT be optional at all, then I’d much rather prefer
to make the behavior consistent for all plugins (built-in and custom) - ie all or none - if
it’s enabled, reloadable applies to all plugins or disabled to all plugins.

Sorry Alan, Leif - Don’t mean to drag the debate on, but couldn’t help raise the concern.
Totally trust your judgement and will be fine with what you suggest.


> On May 9, 2020, at 4:19 PM, Leif Hedstrom <zwoop@apache.org> wrote:
> 
> 
> 
> 
>>> On May 9, 2020, at 14:53, Alan Carroll <solidwallofcode@verizonmedia.com>
wrote:
>>> 
>> 
>> Having been on the other side now, I have to disagree it's a "marginal code change".
I will need to do some significant restructuring to make it work for TxnBox. The biggest issue
is that it's a surprise that static variables don't work as expected anymore. Certainly it
can be dealt with in ATS 9, but it's a bit trickier for code that has to run in ATS 9 and
a previous version. Not being able to use non-cost class statics has a big effect on my coding
and design.
> 
> Ah, it’s the static variable stuff that I ran into as well ? That makes more sense,
and I have no good answer for that, than either not using them, or what you suggested here.
I opted for the former in cache_promote, but it was an easy change with little disadvantage.
> 
> I thought (wrongly) that you were referring to the same problems originally reported
by LI. Sorry.
> 
>> 
>> I think the basic issue comes down to - why do I have to restructure *my* code to
make *your* feature work? It imposes a significant burden on me for no benefit to me, something
you normally oppose. Option (1) also imposes a burden on anyone using it, because they must
remember to put "@reloadable=false" every time the plugin is used.  We can turn it off globally,
but as you note that's not a long term solution as it's effectively certain someone will want
to run plugins that need the feature, and others that don't.
>> 
>> To me, the straight forward approach is to enable the plugin writer to make the choice
for his specific plugin. He knows whether it's reloadable or not, he's the one who should
decide whether to structure his code to support the feature or not. This avoids putting a
burden on anyone except the plugin author. In addition, option (3) puts the burden only on
those plugins that don't support reload, and it's a light burden - one extra API call. No
changes to plugins that can reload are needed. No changes to "remap.config" are needed. It
also means if the plugin is updated at some point, it can be shipped and installed with no
> 
> Yeh I’m fine with option 3 as well. Make it so #1. Question : do you know if any core
plugins that must change ?
> 
> Cheers,
> 
> 
> — Leif 
> 
> 
>> configuration changes. I think (3) gives the closest approach to "it just works",
while being flexible enough to handle all the use cases brought up, except differential reloading
between rules, which I think is not something anyone would ever do. Even then, it could be
implemented on top of (3) if necessary.
>> 
>> I understand why this feature is useful, I just want to be able to have my plugin
opt out if I think that's best for my plugin, without forcing all other plugins to also opt
out.
>> 
>>> On Fri, May 8, 2020 at 4:13 PM Sudheer Vinukonda <sudheervinukonda@yahoo.com>
wrote:
>>> It’s not so much of a problem, but I just feel that this falls into a kind
of feature that should not be forced in a one-way only mode to everyone. 
>>> 
>>> I’m not really convinced that this feature is useful to every user and even
if so, like most features in ATS, don’t see *why* it should not be configurable (unless,
making it so largely complicates things and is not worth the ROI). I think as long as there
are ATS users that do not want to enable this behavior in prod, (at least not right away),
it seems reasonable that we should try and allow that.
>>> 
>>> Just my 2c :)
>>> 
>>> 
>>>>> On May 8, 2020, at 1:42 PM, Leif Hedstrom <zwoop@apache.org> wrote:
>>>>> 
>>>> 
>>>> 
>>>>> On May 8, 2020, at 2:29 PM, Alan Carroll <solidwallofcode@verizonmedia.com>
wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>>> On Fri, May 8, 2020 at 3:17 PM Leif Hedstrom <zwoop@apache.org>
wrote:
>>>>>> 
>>>>>> It should get reloaded for the second, not for the first. As far
as I understand, this is fine, the Gancho made the code such that as long as something uses
some version of a plugin, it will be kept forever.
>>>>>> 
>>>>> 
>>>>> I am dubious of that claim - I don't remember anything like that. We
should ask Gancho, but IIRC the table of plugins for the remap doesn't track per rule, so
for a given configuration and plugin, there is only one version of the plugin DSO loaded.

>>>> 
>>>> It’ll require some changes to the core, I’m just saying there’s nothing
technical that would prevent two remaps to point to differently loaded .so’s. Multiple versions
of the plugin can live simultaneously already, simply by the fact that remap structure is
ref-counted and some request can linger for hours or days.
>>>> 
>>>> Now, I guess I really don’t care much, as long as all the core plugins
that supports both remap and global don’t limit how we use them (which it seemed at least
option #2 would do, possibly #3 and #4 too?). Or, if you can tell me which plugin is having
a problem here, I can look into fixing that problem (likely it’s my fault anyways).
>>>> 
>>>> I still don’t fully understand the problem that anyone has here; As Gancho
explained when the setting was added, it’s a marginal code change to fix the behavior in
plugins that do have problems. We also added the “global” user-data slot table, to make
it easier to share data between different plugins (such that you don’t lose the connection
between the once loaded global instance, and the reloadable remap instances).  The setting
was added as “transitional” aid, making it easier to upgrade to 9.0.x without having to
fix internal plugins immediately.
>>>> 
>>>> — Leif
>>>> 

Mime
View raw message