-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Define a templated systemd instance service unit in distribution packaging #26246
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tylerjl for this pull request and the associated explanation. I've never seen any request for instantiated service units but that's not a reason to not propose it. I left some comments, and I'd be more comfortable if we add documentation about this, maybe like how to set up a 3 nodes cluster? I think that writing down the doc will help us to "get opiniated" about how the templated service file looks like and what needs to be instantiated. If we go that route, then we'll have to add packaging tests for this too but I think it can be done after we agree on the doc and process.
Environment=ES_HOME=/usr/share/elasticsearch | ||
Environment=ES_PATH_CONF=${path.conf}/%i | ||
Environment=PID_DIR=/var/run/elasticsearch | ||
EnvironmentFile=-${path.env}-%i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should add AssertPathExists
assertions on EnvironmentFile
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't technically need the EnvironmentFile
to be present, as the most important thing it's doing is setting ES_PATH_CONF
which the unit already does. It may be a good idea to add an AssertPathExists
for the ES_PATH_CONF
directory, though?
@@ -0,0 +1,60 @@ | |||
[Unit] | |||
Description=Elasticsearch Instance %i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use %I
here
StandardError=inherit | ||
|
||
# Specifies the maximum file descriptor number that can be opened by this process | ||
LimitNOFILE=65536 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like that we have to duplicate (and maintain) all the options like LimitNOFILE
... Would it be possible to inherit/import from the main service file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed; I tried to come up with a better way with systemd natively but I couldn't come up with a good solution. For reference, here's some feedback from #systemd on freenode:
leothrix | Am I mistaken, or am I remembering correctly that there's some sort of
| `.import` option in unit files?
zdzichu | leothrix: .include ?
zdzichu | it's deprecated
leothrix | zdzichu: Ah, that was the one I'd seen before. So it's being phased out?
zdzichu | yep, .d/ snippets are better
zdzichu | more clean and visible
leothrix | Is there any way to define a new unit that includes the same options as
| another unit instead of just overriding an existing unit?
boucman_work | leothrix: kinda, but that would be a bit ugly
zdzichu | ha, that's one use case where .include would be helpful ;)
zdzichu | right now, use 'cp'
boucman_work | your second unit would be an empty .service, then you create a .service.d/
| directory and in there you put a symlink (renamed .conf) to your first unit
boucman_work | as I said : ugly
boucman_work | or put all the common parts in a .conf file, and symlink in both .d files (a
| bit cleaner, but not exaclty what you asked)
So there's a deprecated way of doing so, and a hack to get the same effect, but nothing that looks ideal.
Thanks @tlrx, I added some changes and documentation as well. The docs are a little heavy on the bash side, but it's the simplest way to implement a small cluster with the templated units. |
The core/infra team discussed this issue today. This issue was previously discussed in May 2019, where the decision was made that we should do this templating (#24867 (comment)). One item raised during today's discussion is that we need to ensure this does not negatively effect existing rpm/deb users who are upgrading given that a recent change was made to respect ES_PATH_CONF, which we will be testing (#50273). |
Pinging @elastic/es-delivery (Team:Delivery) |
This is an attempt to illustrate the proposed changes in #24867. I'm gradle-dumb so there may very well be a better way to drop this new file, but the changeset gets the point across (it's also unfortunate that the service file is a copypasta from the default one, but I'm not sure if there's a good way to resolve that in gradle either).
The intent here is not to expose an entirely fully-fledged alternative method of running Elasticsearch, but rather to provide the templated service unit for power users/administrators who know what they're doing to avoid manually writing service files in downstream processes (which is the current approach by ansible/puppet/chef/etc.). Keeping the service file contents in sync saves everyone from a ton of problems.
The instance-specific parts of the templated unit are 1) a custom
ES_PATH_CONF
,EnvironmentFile
, and PID file. This should be enough to create a separate environment for an instance of Elasticsearch if someone wants to run it that way, and should hopefully avoid be trappy by failing fast unless an administrator has taken specific actions to createES_PATH_CONF
(which doesn't exist by default) and optionally the instance-specific defaults file. This is going to be hard to bump into and start by accident since it requires a specialsystemctl start [email protected]
invocation and will simply fail since no instance-specific directory in/etc/elasticsearch/master
has been created.I've built and tested this, and was able to get the templated service to work by copying
/etc/elasticsearch/*.*
into/etc/elasticsearch/01
and then startingsystemctl start [email protected]
. It's not perfect, and does require manual steps to setup the instance-specific configuration files, but that's the intent of providing the unit: make the functionality available if needed.(I'm not sure whether this should be PRd against a different branch, but master seems safe). I can provide a small snippet somewhere in the docs if this ends up getting the greenlight, depending on the amount of visibility it should have.