Skip to content

Fix: pacemaker_remote: set timeout for remote connection to sbd_timeo… - #1130

Merged
kgaillot merged 2 commits into
ClusterLabs:masterfrom
wenningerk:sbd_remote_stuff
Aug 29, 2016
Merged

Fix: pacemaker_remote: set timeout for remote connection to sbd_timeo…#1130
kgaillot merged 2 commits into
ClusterLabs:masterfrom
wenningerk:sbd_remote_stuff

Conversation

@wenningerk

Copy link
Copy Markdown
Contributor

…ut/2 - needed to switch sbd_remote from one control-node to another without rebooting

sbd_remote - when pacemaker-watcher is enabled - connects to the cib via proxy.
having a timeout on the tcp-connection gives clients in general the possibility to
easily fail and possibly recover - in case of sbd_remote without running into a
watchdog-timeout.
Unfortunately TCP_USER_TIMEOUT is one of the more recent socket-options.
So code is guarded by a macro accordingly.
As pacemaker already knows the timeout sbd is setting on the watchdog, for now
half of that is used for the socket-option to still have the other half for recovery.
If this turns out to be of generic interest for other clients on remote-nodes a cluster-property
might be added to set the timeout directly - overruling, if set, what comes from the
sbd-timeout-value.
This fix alone already fixes the watchdog-reboot occurring when the control-resource
for a remote-node switches from one cluster-node to another.
Combined with some tweaking in sbd other unnecessary watchdog-reboots can
be prevented as well (Link to corresponding pull request is coming...).

@jnpkrn

jnpkrn commented Aug 24, 2016

Copy link
Copy Markdown
Contributor

Please, pretty please stick with concise commit summary headers as discussed at #1119

Comment thread lrmd/tls_backend.c Outdated
}
}
#endif

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's functionize this block in lib/common, then we can make get_sbd_timeout() static inline.

Any exposed symbols, even if only used internally, should start with crm_. (Exempting existing symbols of course, for backward API compatibility.)

The functions (including check_sbd_timeout()) should really be in lib/common/watchdog.c. I know the other check_*() functions are in utils.c, but it seems the watchdog aspect is more relevant here.

@wenningerk wenningerk Aug 26, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check_sbd_timeout definitely belongs to lib/common/watchdog.c.

for functionalizing the code-block above I'd rather prefer lib/common/remote.c as it might
turn out to be a good thing to set the timeout for remote in general to make clients on
remote-nodes more responsive. (driven by a cluster-property then so that the actual
tcp-user-timeout derives from sbd_timeout and e.g. remote-proxy-timeout)
of course we need crm_get_sbd_timeout then again ... unless we put it into crm_internal.h.

Probably an overkill but ...: split off a generic crm_tcpconn_user_timeout_set in
lib/common/utils.c that could cover catching the possible unavailability of
the socket-option including a quiet-parameter (disables a log-warning
so that it can be used for more frequent occasions than excepting the
connection of a cluster-node at a remote-node).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind splitting out more functions. They should be in crm_internal.h, but they still need to be prefixed, because anything not static is exposed by the library, even when not part of our self-defined public API. The goal is to avoid linking conflicts with similarly-named user-defined functions.

Maybe crm_set_tcp_timeout() in utils.c, crm_get_sbd_timeout() in watchdog.c and crm_remote_tcp_init() in remote.c.

@wenningerk

Copy link
Copy Markdown
Contributor Author

Think we can spawn out more functions as needed. My concern was just that
further changes already on the radar would again trigger code being shuffled around again.
Rather spawned out crm_remote_accept to remote.c as it better matches the style
of the previous code and its' spawn-out-strategy to remote.c. And it leads to
roughly screen-sized functions on both sides.

@kgaillot
kgaillot merged commit 17cf109 into ClusterLabs:master Aug 29, 2016
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.

3 participants