[kamailio/kamailio] support sd_notify() to improve systemd integration (#2139)

List overview All Threads
Download

newer

older

[kamailio/kamailio]...

git:master:febdb877: rtpengine:...

Victor Seva

19 Nov 2019 19 Nov '19

1:20 p.m.

https://www.freedesktop.org/software/systemd/man/sd_notify.html

...

sd_notify() may be called by a service to notify the service manager about state changes. It can be used to send arbitrary information, encoded in an environment-block-like string. Most importantly, it can be used for start-up completion notification.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139

Attachments:

attachment.html (text/html — 1.9 KB)

Show replies by date

Daniel-Constantin Mierla

19 Nov 19 Nov

2:03 p.m.

I don't think someone will (re-)compile with systemd support in the core.

We should add a core api that executes callbacks on the events you need and then have a module setting them.

In this case I see the patches are minimal, so I can take care converting this PR to a use of a module with new core api callbacks. Then you can add more to it if needed in the future.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-555521274

Victor Seva

2:21 p.m.

Great, thanks!

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-555528686

Daniel-Constantin Mierla

21 Nov 21 Nov

12:56 p.m.

See the newly added systemdops module -- beware it was not tested! Feel free to improve/change it as you need, I did more or less the framework to avoid the need to re-compile the core with libsystemd dependency.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557072836

Daniel-Constantin Mierla

12:56 p.m.

Closed #2139.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#event-2819536796

Victor Seva

1:22 p.m.

...

Feel free to improve/change it as you need

I will, thank you!

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557082280

Anthony Messina

7:50 p.m.

Trying to test @miconda's commits on top of the 5.3 branch--can't get Kamailio to notify systemd on startup. strace doesn't show anything attempting to send notification to systemd. I'll keep trying...

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557245042

Anthony Messina

8:24 p.m.

...

Trying to test @miconda's commits on top of the 5.3 branch--can't get Kamailio to notify systemd on startup. strace doesn't show anything attempting to send notification to systemd. I'll keep trying...

Looking through `main.c`, I think this is because `sr_corecb_void_exec(app_ready);` is not included in `if (dont_fork)`. The assumption with `Type=notify` is that we'd be using `kamailio -DD` where the main process isn't forked, correct?

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557257170

Anthony Messina

22 Nov 22 Nov

2:26 a.m.

I tested a bit more in Fedora 31, but couldn't get this to work as a module. Similar kamailio/systemd issues like #799, which I'll update there, subjectively.

Thanks again for working toward systemd integration.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557359091

Anthony Messina

5:28 a.m.

...

I tested a bit more in Fedora 31, but couldn't get this to work as a module. Similar kamailio/systemd issues like #799, which I'll update there, subjectively.

Thanks again for working toward systemd integration.

Confirming that @linuxmaniac's initial patch does work with `kamailio -DD`.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557395198

sergey-safarov

7:37 a.m.

I can confirm kamailio process reach `sr_corecb_void_exec(app_ready);` https://github.com/kamailio/kamailio/blob/master/src/main.c#L1793

But `ksr_sd_app_ready` not called. https://github.com/kamailio/kamailio/blob/master/src/modules/systemdops/syst...

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557425458

sergey-safarov

8:03 a.m.

May be that because `mod_register` for `systemdops_mod` is empty function and not make call of `r_event_register_cb`? https://github.com/kamailio/kamailio/blob/master/src/modules/systemdops/syst...

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557432041

Victor Seva

8:06 a.m.

@amessina @sergey-safarov can you try with my 63884a7713af5a96114bf90c36cf63b3e553bcc5

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557433052

Daniel-Constantin Mierla

8:17 a.m.

Indeed I missed setting the callback pointers. I just pushed the commit: d4fc8b0168ba3de78e29deb7c7d7ed9b3fd29a36

Try again with the latest master.

@linuxmaniac - your patch tries to set the address of a static structure defined in core. That doesn't need to be allocated as a pointer, it is already defined.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557436487

sergey-safarov

8:35 a.m.

I combined Victor commit and Daniel commit module registered now. Think need apply ```diff diff --git a/src/modules/systemdops/systemdops_mod.c b/src/modules/systemdops/systemdops_mod.c index 0ac15426d1..4672f47d1a 100644 --- a/src/modules/systemdops/systemdops_mod.c +++ b/src/modules/systemdops/systemdops_mod.c @@ -69,7 +69,8 @@ void ksr_sd_app_shutdown(void) int mod_register(char *path, int *dlflags, void *p1, void *p2) { sr_corecb_t *cbp = sr_corecb_get(); - if(cbp) { + if(!cbp) { + SYS_MEM_ERROR; return -1; } cbp->app_ready = ksr_sd_app_ready; ```

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557442084

sergey-safarov

8:38 a.m.

also systemd unit with `Type=notify` started properly.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557443144

Daniel-Constantin Mierla

8:45 a.m.

Apparently no fully woken up ... indeed the condition on cbp was supposed to test if it is null. The SYS_MEM_ERR is not needed because there is no memory allocated. I pushed a commit to fix the condition.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557445116

Daniel-Constantin Mierla

11:49 a.m.

@sergey-safarov - I pushed a commit that enable loading modules via command parameter, like `--loadmodule=systemdops.so`. Maybe this is an option if people want to have systemd notifications, because it can be added in the systemd unit file starting kamailio.

I had it in mind to add it for very long time, aiming to offer the option to modules to do callbacks before loading the configuration file for various needs (e.g., apply a template system to config file before parsing), but then we had support for defines. Now I thought this can be useful also in this particular case.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557502901

Anthony Messina

9:06 p.m.

@miconda, @sergey-safarov, @linuxmaniac thank you. I confirm this now works atop branch 5.3 with the following in `/lib/systemd/system/kamailio.service` ``` [Unit] Description=Kamailio SIP Server Documentation=man:kamailio(8) https://www.kamailio.org/ After=network.target

[Service] Type=notify User=kamailio Environment=SHM_SIZE=128 PKG_SIZE=8 ExecStartPre=/usr/sbin/kamailio -c ExecStart=/usr/sbin/kamailio -DD -m $SHM_SIZE -M $PKG_SIZE ProtectHome=true Restart=on-failure RestartSec=30

[Install] WantedBy=multi-user.target ```

Now, onto #799 ...

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/issues/2139#issuecomment-557694254

1917

Age (days ago)

1920

Last active (days ago)

sr-dev@lists.kamailio.org

18 comments

4 participants

tags (0)

participants (4)

Anthony Messina
Daniel-Constantin Mierla
sergey-safarov
Victor Seva