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

Provide a way to propagate reload requests from targets to services #710

Closed
mbiebl opened this issue Jul 24, 2015 · 10 comments
Closed

Provide a way to propagate reload requests from targets to services #710

mbiebl opened this issue Jul 24, 2015 · 10 comments
Labels
pid1 RFE 🎁 Request for Enhancement, i.e. a feature request

Comments

@mbiebl
Copy link
Contributor

mbiebl commented Jul 24, 2015

This is a followup from the mailing list discussion http://lists.freedesktop.org/archives/systemd-devel/2015-July/033633.html

2015-07-22 20:41 GMT+02:00 Lennart Poettering lennart@poettering.net:

On Wed, 22.07.15 20:28, Michael Biebl (mbiebl@gmail.com) wrote:

2015-07-22 19:15 GMT+02:00 Lennart Poettering lennart@poettering.net:

On Tue, 21.07.15 13:43, Marc Haber (mh+systemd-devel@zugschlus.de) wrote:

Can I write my nifty.target as a service? I have seen in this case
nifty.service files with Exec=/bin/true to basically create a no-op
service, but that's ugly.

That's a hack. A target is for grouping things, they basically are
services without processes attached to them. You really should use
targets for this, and not misuse services.

Afaik, there is no way to have a .target reloaded and that reload
request propagated to all services in that target.

Say you have foo.target consisting of bar.service and baz.service,
how can you achieve that
systemctl reload foo.target propagates that reload request to {bar,baz}.service?

You cannot do that right now, and I am not sure you should be able to,
after all the operation does not really apply to target units.

I do see your usecase though...

Humm...

Maybe we can change the manager core to propagate Reload() calls
for unit type that do not support it natively to other units listed in
PropagateReloadsTo= and then become a NOP.

Or in other words: invoking reload on a target that knows no
PropagateReloadsTo= should continue to return an error. But if such
deps are defined, it should become a silent NOP and propagate the
event.

Could you file an RFP issue on github asking for this? [or even better,
send a PR with a patch... ;-) ]

@poettering poettering added RFE 🎁 Request for Enhancement, i.e. a feature request pid1 labels Jul 24, 2015
@df7cb
Copy link

df7cb commented Sep 5, 2015

We have the same problem for PostgreSQL in Debian. There can be an arbitrary number of PostgreSQL "clusters" (PG speak for database instance) running, managed by postgresql@.service. In the past, /etc/init.d/postgresql start/stop/restart/reload would act on all clusters. To get the same behavior with systemd, I started with making a postgresql.target, but quickly realized that targets do not support reload. Now we have a postgresql.service with Exec=/bin/true which works, but which keeps confusing users because using true there is just weird.

PropagateReloadsTo= won't work because the set of clusters is dynamic. Currently we have this in postgresql@.service:

PartOf=postgresql.service
ReloadPropagatedFrom=postgresql.service

IMHO the existing PartOf and ReloadPropagatedFrom settings would just work if targets would accept them. (I can see that you wouldn't want "systemctl reload default.target" to reload the whole system, but if something explicitely says "ReloadPropagatedFrom some.target", that could Just Work and wouldn't violate the POLA.)

Thanks for considering :)

@mbiebl
Copy link
Contributor Author

mbiebl commented Sep 5, 2015

Let me copy my reply from #999:

I think I'd like to see a boolean PropagateReload=yes instead or at least in addition to PropagateReloadTo=foo.service
This way services can declare themselves PartOf=foo.target, which can be useful for instanced services, where you don't know the name of the service beforehand.
would basically use the same definition here that we use for restart.

@poettering
Copy link
Member

BTW, this will be fixed by #6428.

@poettering
Copy link
Member

Closing as #6428 got merged.

@martinpitt
Copy link
Contributor

Reopening, as #6428 caused a regression and was reverted in #6836 .

@martinpitt martinpitt reopened this Sep 15, 2017
@johnlinp
Copy link
Contributor

johnlinp commented Oct 8, 2017

Any plan or milestone to fix this issue?

@boucman
Copy link
Contributor

boucman commented Oct 8, 2017

well, the half of the original PR that fixed this has been comitted and released in v235

what's left of the PR is the part that causes a .device reload on event. So I think this FR can actually be closed...

@johnlinp
Copy link
Contributor

@martinpitt What do you say? Thanks.

@poettering
Copy link
Member

Implemented in f54bcca. Closing

@johnlinp
Copy link
Contributor

@poettering Thank you for confirming this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pid1 RFE 🎁 Request for Enhancement, i.e. a feature request
Development

No branches or pull requests

6 participants