[ClusterLabs] Antw: Re: Antw: Doing reload right

[Ulrich Windl]

>>> Ken Gaillot <kgaillot at redhat.com> schrieb am 01.07.2016 um 17:26 in
Nachricht
<57768BC3.7030006 at redhat.com>:
> On 07/01/2016 04:48 AM, Jan Pokorný wrote:
>> On 01/07/16 09:23 +0200, Ulrich Windl wrote:
>>>>>> Ken Gaillot <kgaillot at redhat.com> schrieb am 30.06.2016 um 18:58 in
Nachricht
>>> <57754F9F.8070908 at redhat.com>:
>>>> I've been meaning to address the implementation of "reload" in Pacemaker

[...]

>> IIUIC, reload-options should first reflect the parameters as when
>> "start" is invoked, then delegate the responsibility to something
>> that triggers as native reload as possible (which was mentioned
>> is commonly [and problematically] implemented directly in current
>> "reload" actions of common RAs).
> 
> See how much confusion there is? ;-)
> 
> Let's say we have a custom resource agent that manages a daemon. The
> daemon doesn't provide its own pid file, so the resource agent creates
> one to track it.
> 
> The pacemaker configuration might be:
> 
>   resource frobnicator pidfile=/var/run/frobnicator.pid
> 
> And separately, the daemon has its own configuration file,
> /etc/frobinicator.conf, containing "debug: false".
> 
> We need two separate reload capabilities:
> 
> 1. If the user reconfigures pidfile in the pacemaker configuration, the
> agent needs to handle that.
> 
> 2. If the user reconfigures debug in the daemon configuration, the
> daemon needs to handle that.
> 
> Currently, pacemaker uses "reload" for #1, but all known published
> resource agents use "reload" for #2. These two uses need to be
> separated. I'm proposing we keep "reload" for #2, since most RAs are
> already written that way, and add a new "reload-params" for #1.

I agree.

> 
>>>> * Review all ocf:pacemaker and ocf:heartbeat agents to make sure they
>>>> use unique, reloadable, reload, and reload-options properly.
>>>>
>>>> The downside is that this breaks backward compatibility. Any RA that
>>>> actually implements unique and reload so that reload works will lose
>>>> reload capability until it is updated to the new style.
>>>
>>> Maybe there's a solution that is even simpler: Keep the action name,
>>> but don't use "reload" unless the RA indicated at least one
>>> "reloadable" parameter. Naturally old RAs don't do it. And once an
>>> author touches the RA to fix things, he/she should do so (not just
>>> adding "reloadable" to the metadata).
>> 
>> IMHO, idea worth pursuing
> 
> I don't like the idea of varying pacemaker interpretation of one thing
> based on the presence of another. That could make troubleshooting very
> difficult for someone who's not intimately familiar with the
> particulars. (There may be cases where we already do this, but that
> doesn't mean I like it.)

But we are talking on makeing incompatible changes to the metadata and the RA
interface anyway. The old interpretation will not apply any more, and RAs
should be reviewed.
I have a less refusing opinion on that.

> 
>>>> While we usually go to great lengths to preserve backward compatibility,
>>>> I think it is OK to break it in this case, because most RAs that
>>>> implement reload do so wrongly: some implement it as a service reload, a
>>>> few advertise reload but don't actually implement it, and others map
>>>> reload to start, which might theoretically work in some cases (I'm not
>>>> familiar enough with iSCSILogicalUnit and iSCSITarget to be sure), but
>>>> typically won't, as the previous service options are not reverted (for
>>>> example, I think Route would incorrectly leave the old route in the old
>>>> table).

Yes, sometimes you need to cleanup the mess, even if it breaks old habits.

>>>>
>>>> So, I think breaking backward compatibility is actually a good thing
>>>> here, since the most reload can do with existing RAs is trigger bad
>>>> behavior.
>>>
>>> See my comment above.
>>>
>>>>
>>>> The opposing view would be that we shouldn't punish any RA writer who
>>>> implemented this correctly. However, there's no solution that preserves
>>>
>>> The software could give a warning if the RA provides "reload", but
>>> does not define any reloadble parameter. So to notify users and
>>> developers that some change is needed.
> 
> I wouldn't say that's an error condition. Someone may legitimately
> choose to implement reload from the beginning as a no-op, to make it
> easier to add reloadable parameters in the future. Pacemaker only

Well 8-/ for a start there should be at least one reloadable parameter.

> reloads if both the reload action is supported and a parameter is
> reloadable; there's no problem with there being one without the other.

I think having a working reload action without actually having a reloadable
parameter it an artifical requirement.

For the case of changing the contents of an external configuration file, the
RA would have to provide some reloadable dummy parameter then (maybe like
"config_generation=2").

> 
> Of course, with the proposed change, we're talking about reload-params
here.
> 
> If we separate "reloadable" as its own metadata attribute (vs "unique"),
> it might make sense to warn if a parameter is marked "reloadable"
> without a reload-params action being supported (the inverse of your
> suggestion), because that would more indicate an oversight.

Sounds OK.

[...]


Regards,
Ulrich