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.

Sign up for GitHub

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

Jump to bottom

[WIP] Modernize start method #135

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+47 −21
Open

[WIP] Modernize start method #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ba9640
Feature: sbd-inquisitor: don't fork if no pid-file given
wenningerk Sep 23, 2021
940ced8
Fix: systemd: switch from legacy forking to simple service
wenningerk Sep 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions src/sbd-common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,11 +1160,28 @@ sbd_cdtocoredir(void)
return rc;
}

void
sbd_detach(void)
{
const char *devnull = "/dev/null";

maximize_priority();
sysrq_init();

umask(022);
close(0);
(void)open(devnull, O_RDONLY);
close(1);
(void)open(devnull, O_WRONLY);
close(2);
(void)open(devnull, O_WRONLY);
sbd_cdtocoredir();
}

pid_t
make_daemon(void)
{
pid_t pid;
const char * devnull = "/dev/null";

pid = fork();
if (pid < 0) {
Expand All @@ -1179,17 +1196,8 @@ make_daemon(void)
qb_log_ctl(QB_LOG_STDERR, QB_LOG_CONF_ENABLED, QB_FALSE);

/* This is the child; ensure privileges have not been lost. */
maximize_priority();
sysrq_init();

umask(022);
close(0);
(void)open(devnull, O_RDONLY);
close(1);
(void)open(devnull, O_WRONLY);
close(2);
(void)open(devnull, O_WRONLY);
sbd_cdtocoredir();
sbd_detach();

return 0;
}

Expand Down
9 changes: 8 additions & 1 deletion src/sbd-inquisitor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1387,7 +1387,14 @@ int main(int argc, char **argv, char **envp)

cl_log(LOG_NOTICE, "%s flush + write \'%c\' to sysrq in case of timeout",
do_flush?"Do":"Skip", timeout_sysrq_char);
exit_status = inquisitor();
if (pidfile) {
exit_status = inquisitor();
} else {
sbd_detach();
inquisitor_child();
Copy link

Choose a reason for hiding this comment

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

If I follow correctly, with "forking", the parent doesn't exit until it gets SIG_LIVENESS. Do we need to go all the way to "sd_notify" to keep the behavior similar?

Copy link
Author

Choose a reason for hiding this comment

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

As to my knowledge I'm afraid yes.
Unless of course we totally rely on the daemons talking to each other.

Copy link
Author

Choose a reason for hiding this comment

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

The comment in inquisitor_decouple btw. is a lie - I'll remove/correct it in the course of this PR - the inquisitor decouples once cluster-servant is online - meaning it has received it's own membership.

/* not reached */
exit(0);
}
} else {
exit_status = -2;
}
Expand Down
1 change: 1 addition & 0 deletions src/sbd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ void do_reset(void);
void do_off(void);
void do_timeout_action(void);
pid_t make_daemon(void);
void sbd_detach(void);
void maximize_priority(void);
void sbd_get_uname(void);
void sbd_set_format_string(int method, const char *daemon);
Expand Down
13 changes: 9 additions & 4 deletions src/sbd.service.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,16 @@ RefuseManualStop=true
RefuseManualStart=true

[Service]
Type=forking
PIDFile=@runstatedir@/sbd.pid
Type=simple
Copy link

Choose a reason for hiding this comment

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

If we make the unit file changes a configure option defaulting to forking (for now at least), then we have backward compatibility with older pacemaker

"exec" seems more useful than "simple" if we don't want to go all the way to "sd_notify"

Copy link
Author

Choose a reason for hiding this comment

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

Well, slightly as it needs the initial fork & exec to succeed but I guess not to an extent that it is really useful for us here.

# sbd-inquisitor will usually have opened a watchdog-device
# thus we should give it a chance to do what
# it needs to (anyway kills the subprocesses the
# hard way) and close that gracefully.
# If that isn't successful within time let mixed
# do a cleanup although that will most likely lead to suicide.
KillMode=mixed
EnvironmentFile=-@CONFIGDIR@/sbd
ExecStart=@sbindir@/sbd $SBD_OPTS -p @runstatedir@/sbd.pid watch
ExecStop=@bindir@/kill -TERM $MAINPID
ExecStart=@sbindir@/sbd $SBD_OPTS watch

# Could this benefit from exit codes for restart?
# Does this need to be set to msgwait * 1.2?
Expand Down
13 changes: 9 additions & 4 deletions src/sbd_remote.service.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,16 @@ RefuseManualStop=true
RefuseManualStart=true

[Service]
Type=forking
PIDFile=@runstatedir@/sbd.pid
Type=simple
# sbd-inquisitor will usually have opened a watchdog-device
# thus we should give it a chance to do what
# it needs to (anyway kills the subprocesses the
# hard way) and close that gracefully.
# If that isn't successful within time let mixed
# do a cleanup although that will most likely lead to suicide.
KillMode=mixed
EnvironmentFile=-@CONFIGDIR@/sbd
ExecStart=@sbindir@/sbd $SBD_OPTS -p @runstatedir@/sbd.pid watch
ExecStop=@bindir@/kill -TERM $MAINPID
ExecStart=@sbindir@/sbd $SBD_OPTS watch

# Could this benefit from exit codes for restart?
# Does this need to be set to msgwait * 1.2?
Expand Down