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

allow 'systemctl enable' and ensure start after local and remote fs #121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aflyhorse
Copy link

Current service file missing [Install] section, causing:

# systemctl enable robinhood.service
The unit files have no installation config (WantedBy, RequiredBy, Also, Alias
settings in the [Install] section, and DefaultInstance for template units).
This means they are not meant to be enabled using systemctl.

Also, add dependency to ensure robinhood start after local and remote fs

@tl-cea tl-cea requested a review from martinetd March 4, 2022 07:53
Copy link

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! We've never really been starting robinhood at boot because our lustre isn't mounted automatically so the lack of install was not really a problem, but I guess it makes sense for sites that do.

@@ -1,5 +1,7 @@
[Unit]
Description=Robinhood server
After=local-fs.target remote-fs.target

Choose a reason for hiding this comment

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

Hmm, this is a tough one.
On one hand it's too broad - we'd to better with a "RequiresMountsFor" with the filesystem paths required, so filesystems we don't care about being slow or fickle don't impact robinhood startup.
On the other hand just after isn't strong enough: if the remote filesystems failed to mount, robinhood will be started anyway, but with a broad "all filesystems" requirement we don't want to stop robinhood from starting if some useless filesystem had a problem.

I don't think we can have a solution for everyone here honestly -- I guess this is "good enough" as a middle ground, with perhaps a comment saying it can be replaced in a drop-in config by RequiresMountsfor=/mnt/lustre ?

Copy link
Author

Choose a reason for hiding this comment

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

I installed via the RPMs so normally I won't modify installed service file. I added these lines to ensure robinhood won't start before the mount and terminate itself.
I'll append a suggestion comment.

Choose a reason for hiding this comment

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

Thanks for the comment!

Let's not suggest something as barbaric as modifying this file in place: suggest systemctl edit robinhood.service (or robinhood@smth.service) with the following content:

[Unit]
After=
After=local-fs.target
RequiresMountsFor=/path/to/scan

The first After resets previous value, the second restores local-fs which is probably needed anyway, and the last adds proper requirements for only the fs we care about.

(If I was true to myself I'd say we should make this into a systemd generator so conditons could be rewritten dynamically based on the EnvironmentFiles, but that would be overdoing it...)

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