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

Fix daemonize #2783

Merged
merged 5 commits into from
Oct 1, 2021
Merged

Fix daemonize #2783

merged 5 commits into from
Oct 1, 2021

Conversation

jj7112
Copy link
Contributor

@jj7112 jj7112 commented Sep 29, 2021

To test

@januscla
Copy link

Thanks for your contribution, @jj7112! Please make sure you sign our CLA, as it's a required step before we can merge this.

@lminiero
Copy link
Member

To be honest, I don't know enough about services to understand whether your change makes sense or not: it was initially contributed in #306, meaning it's been in the documentation for 6 years already, which suggests it's been working for most people just fine so far. As such, I'm hesitant to accept a change that may very well be a fix just for you and break it for others.

Can you elaborate on why you're suggesting this change, and what exactly it does?

@jj7112
Copy link
Contributor Author

jj7112 commented Sep 30, 2021

Hi,

It might be perhaps just a fix for me but there is no any harm replacing "simple" with "idle" according to the system.service manual :

"Behavior of idle is very similar to simple; however, actual execution of the service program is delayed until all active jobs are dispatched. This may be used to avoid interleaving of output of shell services with the status output on the console. Note that this type is useful only to improve console output, it is not useful as a general unit ordering tool, and the effect of this service type is subject to a 5s timeout, after which the service program is invoked anyway"

I'm surprised that during 6 years there were no notifications on, perhaps those few, like me, that encountered it have just fixed it without giving feedback.

Nevertheless, I admit that this fix is just a workaround for the real problem: why "After=network.target" directive is not respected during boot (in my case at least) ?

Investigating further I'm realising that "network.target" is not correct.
It should be "network-online.target".

This is what I'm finding on 4 more recent different linux versions that I checked now (Ubuntu 20.x, 16.x, Debian 10.x and 9.x). In this case there is no need for the "idle" workaround, the system.service manual recommends anyway "simple" for general usage: "It is generally recommended to use Type=simple for long-running services whenever possible..."

According to the above the correct service script is (which is working perfectly now) :

[Unit]
Description=Janus WebRTC Server
After=network-online.target
Wants=network-online.target

[Service]
Type=simple
ExecStart=/opt/janus/bin/janus -o
Restart=on-abnormal
LimitNOFILE=65536

[Install]
WantedBy=multi-user.target

@saghul
Copy link
Contributor

saghul commented Sep 30, 2021

What is the difference between network and network-online ? I forgot, did Janus guess its own IP with STUN at boot? In that case it would make sense yeah.

Whit that fixed, would the change from simple to idle be necessary? In the affirmative, I'd appreciate a throrough commit message explaining why, since it's quite non-obvious.

@jj7112
Copy link
Contributor Author

jj7112 commented Sep 30, 2021

The "network.target" is non existent in /etc/systemd/system/, probably name change since the original commit.
In linux distros like Ubuntu 20.x, Debian 10.x, 9.x this is "network-online.target" therefore the following is needed/working:

After=network-online.target
Wants=network-online.target

No, there is no need for "idle" in this case, that was just a workaround to force delaying the start of janus at boot time, which failed with "simple" using "network.target", since it was not waiting for the network being up. I did not realized that "network.target" was non existent.

@vincentfretin
Copy link
Contributor

I just did some search and found https://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/ that explains the difference between network.target and network-online.target:

  • network.target is a passive unit, that's probably why there is no file, "It only indicates that the network management stack is up after it has been reached. Whether any network interfaces are already configured when it is reached is undefined."
  • network-online.target: "is a target that actively waits until the nework is "up", where the definition of "up" is defined by the network management software. Usually it indicates a configured, routable IP address of some kind"

@lminiero
Copy link
Member

So the right fix for the docs is changing the network.target line? No problem for me to add a couple of sentences that say what you found out, i.e., about different versions of some OS that may require a different naming. As soon as the patch is updated with the suggestions in the comments, this is good to merge for me 👍

@jj7112
Copy link
Contributor Author

jj7112 commented Sep 30, 2021

@vincent, many thanks for the info, that makes it very clear.
@lorenzo, I'll then make another patch (a bit later).
Thanks, best.

@jj7112
Copy link
Contributor Author

jj7112 commented Sep 30, 2021

The commit for amending the mainpage.dox is now updated according to this conversation.

In the systemd service script example of the documentation for controlling Janus daemon, the "network.target" is replaced by "network-online.target" for fixing a startup issue at boot time.

This last is ensuring that the system service actively waits until the network is "up" while the former was not guaranteeing it, the Janus server failing to start at boot time in particular cases.

According to the recommendations within the conversation group and in https://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/ the service script now includes:

After=network-online.target
Wants=network-online.target

ensuring that at boot time the Janus daemon starts after the network is fully up.

@lminiero
Copy link
Member

lminiero commented Oct 1, 2021

Thanks, merging then! 👍

@lminiero lminiero merged commit 4ddb8ac into meetecho:master Oct 1, 2021
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.

None yet

5 participants