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

An official way to send signals into a container for ExecReload= #22036

Open
yrro opened this issue Mar 14, 2024 · 14 comments
Open

An official way to send signals into a container for ExecReload= #22036

yrro opened this issue Mar 14, 2024 · 14 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. quadlet stale-issue

Comments

@yrro
Copy link
Contributor

yrro commented Mar 14, 2024

Feature request description

Where foo is a Podman Quadlet .container service, I'd like systemctl reload foo to send a SIGHUP to the container's main process.

I'm currently doing this:

[Service]
ExecReload=/usr/bin/podman kill -s SIGHUP --cidfile=%t/%N.cid

I got the --cidfile=%r/%N.cid construct by looking at the ExecStart= directive in the generated foo.service unit. But it feels a bit non/obvious & like I'm relying on an implementation detail of Quadlet. Not one that's likely to change, but it would be nice if there was a more documented/obvious way to be able to send signals into the container.

This would also work:

[Service]
ExecReload=/usr/bin/podman kill -s SIGHUP systemd-%N

... but it relies on the user not also specifying ContainerName=.

Suggest potential solution

Quadlet could introduce its own specifiers that it expands during .container file processing. Something like:

[Container]
ExecReload=/usr/bin/podman kill -s SIGHUP %N

However it would probably be too confusing for Quadlet and systemd to both be doing their own expansion processing on directives with differently defined specifiers.

Have you considered any alternatives?

Document --cidfile=%t/%N.cid in podman-systemd.unit(5) and add it to the test.container example within podman-systemd.unit(5). But this will make it an interface promise, so if you wanted to put Quadlet's cidfiles somewhere else in the future you'd break people's .container units.

Additional context

No response

@yrro yrro added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 14, 2024
@yrro yrro changed the title A nicer way to be able to configure .container units with An official way to send signals into a container for ExecReload= Mar 14, 2024
@Luap99 Luap99 added the quadlet label Mar 14, 2024
Copy link

A friendly reminder that this issue had no activity for 30 days.

@mikelpr
Copy link

mikelpr commented Jun 10, 2024

+1 need this

@rhatdan
Copy link
Member

rhatdan commented Jun 10, 2024

@ygalblum @mheon @alexlarsson @cgwalters WDYT?

Should we just do this by default?

@mheon
Copy link
Member

mheon commented Jun 10, 2024

I don't think so; SIGHUP isn't really universal.

@rhatdan
Copy link
Member

rhatdan commented Jun 11, 2024

Then Reload: true

@mikelpr
Copy link

mikelpr commented Jun 11, 2024

Then Reload: true

How would that work? Thing is some daemons (like samba and nginx) support being sent SIGHUP for them to reload their configuration data without stopping and restarting, and others do it differently

@rhatdan
Copy link
Member

rhatdan commented Jun 11, 2024

I was thinking it would just add

ExecReload=/usr/bin/podman kill -s SIGHUP --cidfile=%t/%N.cid

@mikelpr
Copy link

mikelpr commented Jun 11, 2024

@rhatdan unfortunately as @mheon mentioned this is not universal, many daemons do it this way but we'd need something more flexible for those that don't. One might, for example, want to execute a command inside the container instead.

@rhatdan
Copy link
Member

rhatdan commented Jun 11, 2024

Sure For the non default users you can just add the
[Service]
ExecReload=...

But for the most common case, we can make it easy to discover the --cidfile... option.

@mikelpr
Copy link

mikelpr commented Jun 11, 2024

@rhatdan maybe so... is that already available?
EDIT: whoops so the OP mentioned he already does that so it must be

@yrro
Copy link
Contributor Author

yrro commented Jun 12, 2024

Sure For the non default users you can just add the [Service] ExecReload=...

Hmm podman exec doesn't have a --cidfile= option - but systemd-%N will work as the container name unless the user uses ContainerName=

But for the most common case, we can make it easy to discover the --cidfile... option.

I think just documenting the correct way to do this in the test.container example in the man page would be fine. Anyone searching for reload will find it.

@rhatdan
Copy link
Member

rhatdan commented Jun 12, 2024

Please open a PR.

@smrqdt
Copy link

smrqdt commented Jul 16, 2024

How about adding two mutually exclusive options:

ReloadSignal=
ReloadCmd=

ReloadSignal= would generate a podman kill based ExecReload:

ExecReload=/usr/bin/podman kill --cidfile=%t/%N.cid --signal ${signal}

while ReloadCmd= would generate an podman exec based ExecReload:

ExecReload=/usr/bin/podman exec --cidfile=%t/%N.cid ${cmd}

An example for an application that could use ReloadCmd would be Caddy.
This would require adding support for --cidfile to podman exec (#21256).

I believe these options would cover most container workloads.

@rhatdan
Copy link
Member

rhatdan commented Jul 16, 2024

Seams reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. quadlet stale-issue
Projects
None yet
Development

No branches or pull requests

6 participants