Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backward compatibility and another fix #25

Merged
merged 4 commits into from
Nov 2, 2017

Conversation

gao-yan
Copy link
Member

@gao-yan gao-yan commented Jul 4, 2017

No description provided.

…kward compatibility

SBD_WATCHDOG option has been dropped from sbd.sysconfig example.
Re-enabling the support only in the code for backward compatibility.
…tised in sysconfig.sbd

"Default: yes" for SBD_PACEMAKER has always been advertised in
the example sbd.sysconfig. This commit is to ensure that's true again.
It can be disabled by either explicitly setting SBD_PACEMAKER=no in
sysconfig or supplying "-P" option twice like "-P -P".
@wenningerk
Copy link

-c has never been in the man-page - bad enough - but since the default changes with this patch-set it probably should go to the man-page together with the change.

@gao-yan
Copy link
Member Author

gao-yan commented Jul 6, 2017

Probably. It was introduced only in 1.3.0, but has never been exposed either from man page or --help. I guess Andrew added it for his own purpose of experiment :-)

But I'm actually quite hesitating about how to expose such a new option that basically defaults to "enabled"... SBD_PACEMAKER is used for switching both "-P" and "-c", which kind of provides a way to disable it already.

"-W" has been through the change of the default value:
b96ac28
, and now it's "-P". Specifying the options twice for disabling them seems not very ideal but probably is the best option for backward compatibility... Not sure if it's a good idea to change -c to be an option that requires an argument. But of course that way would not be consistent with "-W" and "-P". Hmm...

@wenningerk
Copy link

With pacemaker-checker now defaulting to 'on' cluster-checking implicitly changed the default as well.
Meaning what was really implemented and not what was stated in the config-file ;-)

I was not thinking of the case where one would like to disable it but for the case where it should be enabled without the pacemaker-checker. This might me useful to a certain extent on remote-nodes to have it check for liveness of pacemaker-remote-daemon.
Till now it was '-c' and now it would be '-P -P' instead.

Of course you are right that the help is missing '-c' as well ...
I would keep the implementation as suggested by your commit so that '-W', '-P' & '-c' are consistent.

I'm not sure if I understand you correctly here but the alternative to documenting '-c' would be to have it lurking around undocumented till eternity. And this can't really be the goal I guess.

@gao-yan
Copy link
Member Author

gao-yan commented Jul 18, 2017

I mean I'm not sure if it's really necessary to expose this option at least for now... We'd probably like to avoid introducing unnecessary complexity to users unless we really have use cases that require cluster check to be disabled/enabled separately from pacemaker check.

I'm probably missing anything, but AFAICT #14 requires pacemaker check, and cluster check doesn't have to be disabled for this case, right?

But of course if we had any reasonable use cases, we'd probably even need to introduce a SBD_CLUSTER option for it in sbd.sysconfig.

@wenningerk
Copy link

You are right #14 would require pacemaker-watcher. But unfortunately Andrew spotted a possible race - which is why it is not merged. So all we have so far is enabling cluster-watcher so that it at least checks for presence of pacemaker-remote-daemon.

So that it's in conjunction with other descriptions about "Pacemaker
integration" in the man page.
@wenningerk wenningerk merged commit 681ce1a into ClusterLabs:master Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants