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

Fix: sbd-inquisitor: fail startup if pacemaker integration is disabled while SBD_SYNC_RESOURCE_STARTUP is conflicting #149

Conversation

gao-yan
Copy link
Member

@gao-yan gao-yan commented Nov 24, 2022

And tell user to fix the configuration by either enabling SBD_PACEMAKER
or explicitly disabling SBD_SYNC_RESOURCE_STARTUP. Otherwise startup of
pacemaker would be hanging forever, since pacemaker only knows about
SBD_SYNC_RESOURCE_STARTUP.

AFAICS this is so far the best we can do to prevent cluster services from running into never ending startup, not to mention cluster services could not even be shut down under the situation.

This is to address the topic brought up from:
ClusterLabs/pacemaker#2119 (comment)

I realized this more or less matches the ideas brought up by the other PR:
#141

…SBD_PACEMAKER is set

It doesn't necessarily tell whether pacemaker integration is enabled,
since it can be overridden by -P or -PP option.
…ntegration is even intentionally disabled

It doesn't make sense to recommend startup syncing if one doesn't even
want pacemaker integration.
…d while SBD_SYNC_RESOURCE_STARTUP is conflicting

And tell user to fix the configuration by either enabling SBD_PACEMAKER
or explicitly disabling SBD_SYNC_RESOURCE_STARTUP. Otherwise startup of
pacemaker would be hanging forever, since pacemaker only knows about
SBD_SYNC_RESOURCE_STARTUP.
@wenningerk
Copy link

Looking good to me.
Guess the CI stuff is just a timing-glitch. Thought we were past those since we're running on ram-disks ...
Give me a moment to recall my #141 thought though ...

@wenningerk
Copy link

test this please

@wenningerk
Copy link

Guess what is in here either implements suggestions from #141 or at least doesn't contradict other possible additions mentioned there.

@gao-yan
Copy link
Member Author

gao-yan commented Nov 28, 2022

at least doesn't contradict other possible additions mentioned there.

Indeed so.

@wenningerk
Copy link

test this please

@wenningerk
Copy link

CI seems to break at random places (maybe we can gain some robustness regarding timing but we've seen issues with coverity as well) - guess it is fine though as all in all we've seen all tests passing.

Thanks for the contribution!

@wenningerk wenningerk merged commit 8ec8e01 into ClusterLabs:main Nov 28, 2022
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