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.

Sign up for GitHub

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

Jump to bottom

Fix coturn to also come properly up if dhcpcd is used. #29415

Merged
merged 1 commit into from Sep 18, 2017
Merged

Fix coturn to also come properly up if dhcpcd is used. #29415

merged 1 commit into from Sep 18, 2017

Conversation

eskimor
Copy link
Contributor

@eskimor eskimor commented Sep 15, 2017 •
edited

#Without dhpcpd.service in after, coturn will fail to listen on addresses that
come up with dhcpcd.

Motivation for this change

coturn won't come up correctly if ip addresses are set dynamically.

Things done
  • Configured coturn in configuration.nix and ran nixos-rebuild test
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md. Not exactly, I forgot about the commit headline, I can fix that if needed.

@mention-bot
Copy link

@eskimor, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Ralith and @jazmit to be potential reviewers.

@joachifm
Copy link
Contributor

cc @Ralith

@joachifm joachifm added this to the 17.09 milestone Sep 15, 2017
fpletz
fpletz requested changes Sep 15, 2017
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use the network-online.target instead. dhcpcd.service, if enabled, will be wanted by network-online.target.

@Ralith
Copy link
Contributor

Ralith commented Sep 16, 2017 •
edited

I think network-online.target would need to be in Wants as well as After. It's also interesting to note that the systemd docs seem to advise against this type of dependency.

Is there a reason not to just listen on the wildcard addresses? With a truly dynamic IP, I'm not sure how usefully you could do anything else.

@eskimor
Copy link
Contributor Author

eskimor commented Sep 16, 2017

Is there a reason not to just listen on the wildcard addresses? With a truly dynamic IP, I'm not sure how usefully you could do anything else.

I don't know, why coturn does not simply use a wild card address - by default it seems to gather all addresses and listens on them, if an ip is missing, it just won't listen there.

With a truly dynamic ip this won't work anyway, that's correct. But in my case it is only semi-dynamic, just automated configuration really. In any case, I will look into network-online.target and will fix this PR accordingly.

@eskimor
Copy link
Contributor Author

eskimor commented Sep 16, 2017

The docs only say, not to use it to liberally in order for not delaying the boot. If coturn just won't work correctly otherwise, I would not say that's to liberal.

@eskimor
nixos/services/networking/coturn: Fix coturn to properly come up
3ecf9b1 
properly also in case dhcpcd being used.

Without network-online.target, coturn will fail to listen on addresses that
come up with dhcpcd.
@eskimor
Copy link
Contributor Author

eskimor commented Sep 18, 2017

@Ralith thanks for the doc pointer, that was really helpful! It seems coturn is not a well written service and we have to deal with it, thus the need of network-online.target. I fixed the PR accordingly!

fpletz
fpletz approved these changes Sep 18, 2017
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both for the investigation! I agree that network-online.target is not the ideal solution but in this case we should use it.

@fpletz
Copy link
Member

fpletz commented Sep 18, 2017

Also fixed another annoyance/bug with network-online.target: b179908

@fpletz fpletz merged commit a9f6022 into NixOS:master Sep 18, 2017
fpletz pushed a commit that referenced this pull request Sep 19, 2017
@eskimor @fpletz
coturn service: Fix coturn to properly come up (#29415)
00b7744 
properly also in case dhcpcd being used.

Without network-online.target, coturn will fail to listen on addresses that
come up with dhcpcd.

(cherry picked from commit a9f6022)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpletz fpletz fpletz approved these changes

Assignees
No one assigned
Labels
6.topic: nixos
Projects
None yet
Milestone
17.09
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@eskimor @mention-bot @joachifm @Ralith @fpletz