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

Use systemd-notify(1) shell helper as fallback #573

Conversation

binarin
Copy link
Contributor

@binarin binarin commented Jan 22, 2016

As discussed at the end of #52

Currently external erlang library sd_notify is used to make systemd
unit with Type=notify to work correctly. This library contains some C
code and thus cannot be built into architecture-independent package.

But it is not actually needed, as systemd provides
systemd-notify(1) helper for shell scripts.

The only thing is that you need to add NotifyAccess=all to your unit file to
make everything work well.

@michaelklishin michaelklishin self-assigned this Jan 22, 2016
@michaelklishin michaelklishin added this to the 3.6.1 milestone Jan 22, 2016
Currently external erlang library `sd_notify` is used to make systemd
unit with `Type=notify` to work correctly. This library contains some C
code and thus cannot be built into architecture-independent package.

But it is not actually needed, as systemd provides systemd-notify(1)
helper for shell scripts which serves exactly the same purpose.

The only thing is that you need to add `NotifyAccess=all` to your unit
file to make everything work well.
@binarin binarin force-pushed the rabbitmq-server-systemd-notify-zero-deps branch from 941c2de to 466dea8 Compare January 22, 2016 13:47
@@ -287,7 +287,7 @@ broker_start() ->
case code:load_file(sd_notify) of
{module, sd_notify} -> SDNotify = sd_notify,
SDNotify:sd_notify(0, "READY=1");
{error, _} -> ok
{error, _} -> os:cmd("systemd-notify --ready")
Copy link
Member

Choose a reason for hiding this comment

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

Thinking out loud: os:cmd/1 always returns a string, even when execution failed. I couldn't make it throw an exception, so this is hopefully safe.

@michaelklishin
Copy link
Member

@binarin your commit message says that

The only thing is that you need to add `NotifyAccess=all` to your unit
file to make everything work well.

should we do it in our own RPM package?

@binarin
Copy link
Contributor Author

binarin commented Jan 22, 2016

Currently there is no systemd unit files in source tree and so there is no place where this change can be applied.

RedHat maintains their unit file externally, and they will need to make that change there when sd_notify will be completely removed.

And I'll keep an eye on Debian packages and will suggest to switch from rabbitmqctl wait in their unit file. Right after they'll start to package 3.6.1 or some later version.

michaelklishin added a commit that referenced this pull request Jan 22, 2016
…ero-deps

Use systemd-notify(1) shell helper as fallback
@michaelklishin michaelklishin merged commit 6d3636a into rabbitmq:stable Jan 22, 2016
@michaelklishin
Copy link
Member

@binarin someone brought up a few related questions today: should we use --pid to communicate RabbitMQ node PID to systemd? Can it be that os:cmd/1 uses a separate PID? Can we notify systemd when RabbitMQ shuts down?

@michaelklishin
Copy link
Member

I see that every os:cmd/1 call starts a separate port/OS process. Try running

os:cmd("sleep 10 &\necho $!").

multiple times in a row.

@michaelklishin
Copy link
Member

The comment above means that we can't let systemd-notify use the caller OS process' pid.

@binarin
Copy link
Contributor Author

binarin commented Feb 29, 2016

I though that systemd-notify will infer parrent pid correctly because

os:cmd("echo $PPID").

works correctly and systemd-notify by default uses this parent pid. But after some strace-ing I observe
that os:cmd/1 introduces one extra intermediary shell process. And even if it was working as intended, it wouldn't have lasted long: erlang 19 is going to change this scheme.

So yes, --pid should indeed be used.

@michaelklishin
Copy link
Member

@binarin is this shortcoming worth delaying 3.6.1 for in your opinion?

@binarin
Copy link
Contributor Author

binarin commented Feb 29, 2016

No, because old users aren't affected - as 3.6.1 still contains old code which uses sd_notify NIF.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants