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

[Andrew Beekhof]

On Sat, Jul 16, 2016 at 9:34 AM, Ken Gaillot  wrote:
> On 07/14/2016 06:21 PM, Andrew Beekhof wrote:
>> On Fri, Jul 15, 2016 at 2:33 AM, Ken Gaillot  wrote:
>>> On 07/13/2016 11:20 PM, Andrew Beekhof wrote:
 On Wed, Jul 6, 2016 at 12:57 AM, Ken Gaillot  wrote:
> On 07/04/2016 02:01 AM, Ulrich Windl wrote:
>> 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").
>
> That is a widely recommended approach for the current "reload"
> implementation, but I don't think it's desirable. It still does not
> distinguish changes in the Pacemaker resource configuration from changes
> in the service configuration.
>
> For example, of an RA has one parameter that is agent-reloadable and
> another that is service-reloadable, and it gets a "reload" action, it
> has no way of knowing which of the two (or both) changed. It would have
> to always reload all agent-reloadable parameters, and trigger a service
> reload. That seems inefficient to me. Also, only Pacemaker should
> trigger agent reloads, and only the user should trigger service reloads,
> so combining them doesn't make sense to me.

 Totally disagree :-)

 The whole reason service reloads exist is that they are more efficient
 than a stop/start cycle.

 So I'm not seeing how calling one, on the rare occasion that the
 parameters change and allow a reload, when it wasn't necessary can be
 classed as inefficient.   On the contrary, trying to avoid it seems
 like over-optimizing when we should be aiming for correctness - ie.
 reloading the whole thing.
>>>
>>> I just don't see any logical connection between modifying a service's
>>> Pacemaker configuration and modifying its service configuration file.
>>
>> There isn't one beyond they are both bypassing a stop/start cycle.
>>
>>>
>>> Is the idea that people will tend to change them together?
>>
>> No, the idea is that the "penalty" of making sure both are up-to-date,
>> in the rare event that either one is changed, does not justify
>> splitting them up.
>
> OK. In that case, we'd keep the "reload" action for doing both types of
> reload together, and the only change we need to consider is unique vs
> reloadable.
>
> Thinking it through some more, I'm leaning to this approach:
>
>
> 1. Let's buckle down and update the OCF spec to reflect the accrued
> real-world practices, as well as this change. This will allow resource
> agents to specify that they comply with the new terminology by setting
>  to 1.1, and both pacemaker and higher-level tools can rely on
> that to determine whether to use the new behavior.

Agreed

>
> The alternative is that pacemaker and higher-level tools could check
> whether a resource agent specifies "reloadable" for any parameter, and
> use the new behavior if so. It's doable, but it's another hacky
> workaround when we're really overdue for this anyway.
>
>
> 2. Since the current usage of "unique" is so broken, I think we should
> abandon it altogether, and use two new attribute names to indicate
> uniqueness and reloadability. We've already converged on "reloadable",
> so we just need something to indicate that two instances of a resource
> cannot share the same value of a given parameter. Maybe "reject_duplicate"?

Agree on concept, not fan of the name.  "exclusive"? "unrepeatable"?

>
> I think it might even be worthwhile for pacemaker (not just high-level
> tools) to enforce the new attribute,

Not necessarily disagreeing, but the implementation would be "fun".
Are you sure pcs isn't the right place in the same way that it removes
related constraints when deleting a resource?

> because it would be used to
> indicate that there's a problem if it's used twice. For example, you can
> start two instances of apache with different config files, but you don't
> want to try to start two instances with the same config file. We can't
> do that currently because unique is often set wrong, but if we create a
> new attribute, we can enforce it from the get-go.

We need to agree on semantics though.
Do all of the "exclusive" parameters need to match before a resource
is rejected?

>
> If we don't come up with a new name, I think "unique" becomes completely
> unusable -- resource agents couldn't rely on pacemaker or high-level
> tools to interpret it consistently, and high-level tools couldn't rely
> on resource agents to specify it properly.
>
>
> 3. If a resource agent specifies OCF 1.1 or greater, Pacemaker can look
> for reloadability and uniqueness; otherwise, it would never reload or
> enforce uniqueness. And, we can add a crm_resource --force-reload option
> to do a reload without needing to change a dummy attribute.

Agreed

>
>
> The above would let resource agents confidently specify 

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

[Andrew Beekhof]

On Fri, Jul 15, 2016 at 2:33 AM, Ken Gaillot  wrote:
> On 07/13/2016 11:20 PM, Andrew Beekhof wrote:
>> On Wed, Jul 6, 2016 at 12:57 AM, Ken Gaillot  wrote:
>>> On 07/04/2016 02:01 AM, Ulrich Windl wrote:
 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").
>>>
>>> That is a widely recommended approach for the current "reload"
>>> implementation, but I don't think it's desirable. It still does not
>>> distinguish changes in the Pacemaker resource configuration from changes
>>> in the service configuration.
>>>
>>> For example, of an RA has one parameter that is agent-reloadable and
>>> another that is service-reloadable, and it gets a "reload" action, it
>>> has no way of knowing which of the two (or both) changed. It would have
>>> to always reload all agent-reloadable parameters, and trigger a service
>>> reload. That seems inefficient to me. Also, only Pacemaker should
>>> trigger agent reloads, and only the user should trigger service reloads,
>>> so combining them doesn't make sense to me.
>>
>> Totally disagree :-)
>>
>> The whole reason service reloads exist is that they are more efficient
>> than a stop/start cycle.
>>
>> So I'm not seeing how calling one, on the rare occasion that the
>> parameters change and allow a reload, when it wasn't necessary can be
>> classed as inefficient.   On the contrary, trying to avoid it seems
>> like over-optimizing when we should be aiming for correctness - ie.
>> reloading the whole thing.
>
> I just don't see any logical connection between modifying a service's
> Pacemaker configuration and modifying its service configuration file.

There isn't one beyond they are both bypassing a stop/start cycle.

>
> Is the idea that people will tend to change them together?

No, the idea is that the "penalty" of making sure both are up-to-date,
in the rare event that either one is changed, does not justify
splitting them up.

> I'd expect
> that in most environments, the Pacemaker configuration (e.g. where the
> apache config file is) will remain much more stable than the service
> configuration (e.g. adding/modifying websites).
>
> Service reloads can sometimes be expensive (e.g. a complex/busy postfix
> or apache installation) even if they are less expensive than a full restart.

Right. But you just said that the pacemaker config is much less likely
(out of a thing thats already not very likely) to change. So why are
you optimizing for that scenario?

>
>> The most in-efficient part in all this is the current practice of
>> updating a dummy attribute to trigger a reload after changing the
>> application config file.  That we can address by supporting
>> --force-reload for crm_resource like we do for start/stop/monitor (and
>> exposing it nicely in pcs).

___
Users mailing list: Users@clusterlabs.org
http://clusterlabs.org/mailman/listinfo/users

Project Home: http://www.clusterlabs.org
Getting started: http://www.clusterlabs.org/doc/Cluster_from_Scratch.pdf
Bugs: http://bugs.clusterlabs.org

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

[Ken Gaillot]

On 07/13/2016 11:20 PM, Andrew Beekhof wrote:
> On Wed, Jul 6, 2016 at 12:57 AM, Ken Gaillot  wrote:
>> On 07/04/2016 02:01 AM, Ulrich Windl wrote:
>>> 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").
>>
>> That is a widely recommended approach for the current "reload"
>> implementation, but I don't think it's desirable. It still does not
>> distinguish changes in the Pacemaker resource configuration from changes
>> in the service configuration.
>>
>> For example, of an RA has one parameter that is agent-reloadable and
>> another that is service-reloadable, and it gets a "reload" action, it
>> has no way of knowing which of the two (or both) changed. It would have
>> to always reload all agent-reloadable parameters, and trigger a service
>> reload. That seems inefficient to me. Also, only Pacemaker should
>> trigger agent reloads, and only the user should trigger service reloads,
>> so combining them doesn't make sense to me.
> 
> Totally disagree :-)
> 
> The whole reason service reloads exist is that they are more efficient
> than a stop/start cycle.
> 
> So I'm not seeing how calling one, on the rare occasion that the
> parameters change and allow a reload, when it wasn't necessary can be
> classed as inefficient.   On the contrary, trying to avoid it seems
> like over-optimizing when we should be aiming for correctness - ie.
> reloading the whole thing.

I just don't see any logical connection between modifying a service's
Pacemaker configuration and modifying its service configuration file.

Is the idea that people will tend to change them together? I'd expect
that in most environments, the Pacemaker configuration (e.g. where the
apache config file is) will remain much more stable than the service
configuration (e.g. adding/modifying websites).

Service reloads can sometimes be expensive (e.g. a complex/busy postfix
or apache installation) even if they are less expensive than a full restart.

> The most in-efficient part in all this is the current practice of
> updating a dummy attribute to trigger a reload after changing the
> application config file.  That we can address by supporting
> --force-reload for crm_resource like we do for start/stop/monitor (and
> exposing it nicely in pcs).

___
Users mailing list: Users@clusterlabs.org
http://clusterlabs.org/mailman/listinfo/users

Project Home: http://www.clusterlabs.org
Getting started: http://www.clusterlabs.org/doc/Cluster_from_Scratch.pdf
Bugs: http://bugs.clusterlabs.org

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

[Ken Gaillot]

On 07/04/2016 02:01 AM, Ulrich Windl wrote:
> 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").

That is a widely recommended approach for the current "reload"
implementation, but I don't think it's desirable. It still does not
distinguish changes in the Pacemaker resource configuration from changes
in the service configuration.

For example, of an RA has one parameter that is agent-reloadable and
another that is service-reloadable, and it gets a "reload" action, it
has no way of knowing which of the two (or both) changed. It would have
to always reload all agent-reloadable parameters, and trigger a service
reload. That seems inefficient to me. Also, only Pacemaker should
trigger agent reloads, and only the user should trigger service reloads,
so combining them doesn't make sense to me.

___
Users mailing list: Users@clusterlabs.org
http://clusterlabs.org/mailman/listinfo/users

Project Home: http://www.clusterlabs.org
Getting started: http://www.clusterlabs.org/doc/Cluster_from_Scratch.pdf
Bugs: http://bugs.clusterlabs.org

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

[Ulrich Windl]

>>> Ken Gaillot  schrieb am 01.07.2016 um 17:26 in
Nachricht
<57768bc3.7030...@redhat.com>:
> On 07/01/2016 04:48 AM, Jan Pokorný wrote:
>> On 01/07/16 09:23 +0200, Ulrich Windl wrote:
>> Ken Gaillot  schrieb am 30.06.2016 um 18:58 in
Nachricht
>>> <57754f9f.8070...@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