Skip to content

Commit

Permalink
Simplify the systemd unit file by reading options from the environment
Browse files Browse the repository at this point in the history
  • Loading branch information
@beekhof
beekhof committed Oct 8, 2014
1 parent 756d0e0 commit 6c1c641
Show file tree
Hide file tree
Showing 5 changed files with 166 additions and 15 deletions.
126 changes: 121 additions & 5 deletions src/sbd-inquisitor.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,7 @@ void inquisitor_child(void)

sigemptyset(&procmask);
sigaddset(&procmask, SIGCHLD);
sigaddset(&procmask, SIGTERM);
sigaddset(&procmask, SIG_LIVENESS);
sigaddset(&procmask, SIG_EXITREQ);
sigaddset(&procmask, SIG_TEST);
Expand All @@ -415,7 +416,7 @@ void inquisitor_child(void)

clock_gettime(CLOCK_MONOTONIC, &t_now);

if (sig == SIG_EXITREQ) {
if (sig == SIG_EXITREQ || sig == SIGTERM) {
servants_kill();
watchdog_close(true);
exiting = 1;
Expand Down Expand Up @@ -628,12 +629,70 @@ int inquisitor(void)
return -1;
}


static int
parse_device_line(const char *line)
{
int lpc = 0;
int last = 0;
int max = 0;
int found = 0;

if(line) {
max = strlen(line);
}

if (max <= 0) {
return found;
}

crm_trace("Processing %d bytes: [%s]", max, line);
/* Skip initial whitespace */
for (lpc = 0; lpc <= max && isspace(line[lpc]); lpc++) {
last = lpc + 1;
}

/* Now the actual content */
for (lpc = 0; lpc <= max; lpc++) {
int a_space = isspace(line[lpc]);

if (a_space && lpc < max && isspace(line[lpc + 1])) {
/* fast-forward to the end of the spaces */

} else if (a_space || line[lpc] == ';' || line[lpc] == 0) {
int rc = 1;
char *entry = NULL;

if (lpc != last) {
entry = calloc(1, 1 + lpc - last);
rc = sscanf(line + last, "%[^;]", entry);
}

if (entry == NULL) {
/* Skip */
} else if (rc != 1) {
crm_warn("Could not parse (%d %d): %s", last, lpc, line + last);
} else {
crm_trace("Adding '%s'", entry);
recruit_servant(entry, 0);
found++;
}

free(entry);
last = lpc + 1;
}
}
return found;
}

int main(int argc, char **argv, char **envp)
{
int exit_status = 0;
int c;
int w = 0;
int qb_facility;
const char *value = NULL;
int start_delay = 0;

if ((cmdname = strrchr(argv[0], '/')) == NULL) {
cmdname = argv[0];
Expand All @@ -649,6 +708,49 @@ int main(int argc, char **argv, char **envp)

sbd_get_uname();

value = getenv("SBD_DEVICE");
if(value) {
#if SUPPORT_SHARED_DISK
int devices = parse_device_line(value);
if(devices > 0) {
fprintf(stderr, "Invalid device line: %s\n", value);
exit_status = -2;
goto out;
}
#else
fprintf(stderr, "Shared disk functionality not supported\n");
exit_status = -2;
goto out;
#endif
}

value = getenv("SBD_PACEMAKER");
if(value) {
check_pcmk = crm_is_true(value);
}

value = getenv("SBD_STARTMODE");
if(value) {
start_mode = crm_int_helper(value, NULL);
cl_log(LOG_INFO, "Start mode set to: %d", (int)start_mode);
}

value = getenv("SBD_WATCHDOG_DEV");
if(value) {
watchdogdev = value;
}

value = getenv("SBD_PIDFILE");
if(value) {
pidfile = strdup(value);
cl_log(LOG_INFO, "pidfile set to %s", pidfile);
}

value = getenv("SBD_DELAY_START");
if(value) {
start_delay = crm_is_true(value);
}

while ((c = getopt(argc, argv, "C:DPRTWZhvw:d:n:p:1:2:3:4:5:t:I:F:S:s:")) != -1) {
switch (c) {
case 'D':
Expand Down Expand Up @@ -681,7 +783,7 @@ int main(int argc, char **argv, char **envp)
w++;
break;
case 'w':
watchdogdev = strdup(optarg);
watchdogdev = optarg;
break;
case 'd':
#if SUPPORT_SHARED_DISK
Expand Down Expand Up @@ -751,8 +853,11 @@ int main(int argc, char **argv, char **envp)
}

if (w > 0) {
watchdog_use = w % 2;
}
watchdog_use = w % 2;

} else if(watchdogdev == NULL || strcmp(watchdogdev, "/dev/null") == 0) {
watchdog_use = 0;
}

if (watchdog_use) {
cl_log(LOG_INFO, "Watchdog enabled.");
Expand Down Expand Up @@ -795,7 +900,10 @@ int main(int argc, char **argv, char **envp)
} else if (strcmp(argv[optind], "ping") == 0) {
exit_status = ping_via_slots(argv[optind + 1], servants_leader);
} else if (strcmp(argv[optind], "watch") == 0) {
open_any_device(servants_leader);
if(servant_count > 0) {
/* If no devices are specified, its not an error to be unable to find one */
open_any_device(servants_leader);
}

/* We only want this to have an effect during watch right now;
* pinging and fencing would be too confused */
Expand All @@ -804,13 +912,21 @@ int main(int argc, char **argv, char **envp)
servant_count--;
}

if(start_delay) {
unsigned long delay = get_first_msgwait(servants_leader);

sleep(delay);
}

exit_status = inquisitor();

} else {
exit_status = -2;
}
#else
if (strcmp(argv[optind], "watch") == 0) {
/* sleep $(sbd -d "$SBD_DEVICE" dump | grep -m 1 msgwait | awk '{print $4}') 2>/dev/null */

/* We only want this to have an effect during watch right now;
* pinging and fencing would be too confused */
if (check_pcmk) {
Expand Down
26 changes: 26 additions & 0 deletions src/sbd-md.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,32 @@ int messenger(const char *name, const char *msg, struct servants_list_item *serv
}
}

unsigned long
get_first_msgwait(struct servants_list_item *servants)
{
unsigned long msgwait = 0;
struct servants_list_item *s = servants;

for (s = servants; s; s = s->next) {
struct sbd_context *st;
struct sector_header_s *s_header;
st = open_device(s->devname, LOG_WARNING);
if (!st) {
continue;
}

s_header = header_get(st);
if (s_header != NULL) {
msgwait = (unsigned long)s_header->timeout_msgwait;
close_device(st);
return msgwait;
}

close_device(st);
}
return msgwait;
}

int dump_headers(struct servants_list_item *servants)
{
int rc = 0;
Expand Down
1 change: 1 addition & 0 deletions src/sbd.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ int allocate_slots(const char *name, struct servants_list_item *servants);
int list_slots(struct servants_list_item *servants);
int ping_via_slots(const char *name, struct servants_list_item *servants);
int dump_headers(struct servants_list_item *servants);
unsigned long get_first_msgwait(struct servants_list_item *servants);
int messenger(const char *name, const char *msg, struct servants_list_item *servants);
int servant(const char *diskname, int mode, const void* argp);
#endif
Expand Down
10 changes: 6 additions & 4 deletions src/sbd.service
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ RefuseManualStart=true

[Service]
Type=forking
ExecStart=/usr/share/sbd/sbd.sh start
ExecStop=/usr/share/sbd/sbd.sh stop
PIDFile=/var/run/sbd.pid
EnvironmentFile=-@sysconfdir@/sysconfig/sbd
ExecStart=@sbindir@/sbd $SBD_OPTS watch
ExecStop=kill -TERM $MAINPID
PIDFile=${SBD_PIDFILE}

# Could this benefit from exit codes for restart?
# Does this need to be set to msgwait * 1.2?
# TimeoutSec=
Expand All @@ -19,5 +21,5 @@ PIDFile=/var/run/sbd.pid
Restart=on-abort

[Install]
RequiredBy=pacemaker.service
RequiredBy=corosync.service

18 changes: 12 additions & 6 deletions src/sbd.sysconfig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
# and to monitor. If specifying more than one path, use ";" as
# separator.
#
SBD_DEVICE=""
#SBD_DEVICE=""

## Type: yesno
## Default: yes

This comment has been minimized.

Sign in to view

Copy link
@gao-yan

gao-yan Jun 26, 2017

Member

SBD_PACEMAKER defaults to "no" according to the code though. Previously, invocation of sbd.sh made sure it defaulted to "yes". But it's not the case in the code itself. The question is, we actually want the default to be "yes", right? ;-) If so, we'd probably need to change that in the code and support disabling it by supplying "-P -P"?

This comment has been minimized.

Sign in to view

Copy link
@wenningerk

wenningerk Jun 26, 2017

For consistency with usage via sbd.sh it might make sense to default to 'yes' in the code.
On the other hand a switch from being used via sbd.sh to being started via systemd always requires a config-change while somebody running sbd from systemd now having the SBD_PACEMAKER line commented out would experience a change of behavior through an update while not having changed any config.
I'm tending to see the 2nd argument stronger.

This comment has been minimized.

Sign in to view

Copy link
@gao-yan

gao-yan Jun 27, 2017

Member

A switch from using sbd.sh to systemd doesn't necessarily require a config change actually. The default value for "-P" option has never been mentioned in sbd man page, but "Default: yes" for SBD_PACEMAKER has always been stated in sbd.sysconfig. I wonder what users would expect if they commented out SBD_PACEMAKER ;-)

I admit it's a little bit tricky in here, but I guess it's still more of a question what is intended to be the default...

This comment has been minimized.

Sign in to view

Copy link
@wenningerk

wenningerk Jun 27, 2017

I personally don't really have a use-case where pacemaker-watcher would be disabled.
The only one interesting seems to be pacemaker-remote-nodes although I don't see that working properly at all atm (interimistic results of my research are under #14 - unfortunately stalled for some time now :-( ).
Are there other use-cases I'm not aware of? Of course if you have shared disks (disks is important because without pacemaker-watcher a single disk would be a spof) you have something basically useful without pacemaker-watcher ... e.g. for pacemaker-remote ...
Guess one can call enabling a systemd-service a config-change of some kind - at least something you would look at your config more closely than if you just update a package.
Anyway - correcting the comment in the example-config would be the other option. And I would guess people relying on the wrong default have found out by now ;-)

This comment has been minimized.

Sign in to view

Copy link
@gao-yan

gao-yan Jun 27, 2017

Member

With a sbd setup for example with 3 sbd devices which is no longer a spof, users might want to just rely on the state of the sbd devices without considering any health information from pacemaker/cluster. But I assume they would explicitly set SBD_PACEMAKER=no in sysconfig given the stated default in the example-config.

When sbd daemon didn't yet parse the environment variables, the early version of sbd systemd-service file actually invoked sbd.sh... These users would not expect any change of config/behaviors when updating I think.

Hmm, some users would realize certain changes either way...
#
# Whether to enable the pacemaker integration.
#
SBD_PACEMAKER=
SBD_PACEMAKER=yes

## Type: list(always,clean)
## Default: always
Expand All @@ -21,7 +21,7 @@ SBD_PACEMAKER=
# allow sbd to start if it was not previously fenced. See the -S option
# in the man page.
#
SBD_STARTMODE=
SBD_STARTMODE=clean

## Type: yesno
## Default: no
Expand All @@ -34,19 +34,25 @@ SBD_STARTMODE=
# This option may be ignored at a later point, once pacemaker handles
# this case better.
#
SBD_DELAY_START=
SBD_DELAY_START=no

## Type: yesno
## Default: yes
#
# Whether to use a watchdog.
#
SBD_WATCHDOG=
SBD_WATCHDOG_DEV=/dev/watchdog

## Type: string
## Default: /var/run/sbd.pid
#
# Where to store the PID of the active process
#
SBD_PIDFILE=/var/run/sbd.pid

## Type: string
## Default: ""
#
# Additional options for starting sbd
#
SBD_OPTS=

0 comments on commit 6c1c641

Please sign in to comment.