-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add option to bind to systemd activated sockets #2362
Conversation
05aa425
to
36102bc
Compare
👍 on 699e495, I've run into the same thing (perhaps it can be a separate PR?) |
Yes, I think it was. This allowed me to continue testing, but then I saw #2305. I suspect that on my Fedora installation I didn't have openssl-devel installed so it probably was compiled without SSL. |
Thanks. I apologize for the lack of 'no ssl' support. I've helped with updates as newer versions of OpenSSL were released, and didn't consider installations without it. It's common for Puma to be used with UNIXSockets, so people may want to compile it without OpenSSL. Re #2305, I don't work that much with JRuby, and I'd like to check it on that. I've been busy with other things. Re your patch to minissl.rb, I can always revert it when I finish #2305. A lot of apps may load Ruby's OpenSSL for database connections, etc anyway, but I'd like #2305 to not load OpenSSL if one chooses to compile Puma without it... |
Don't worry about it. It's a development version, not a released version. This PR is still a draft because I only verified it didn't break non-systemd but I have yet to verify actual systemd behavior. It was a trivial workaround that barely held me back. |
36102bc
to
3b5e4ba
Compare
Since #2303 was merged, I've rebased it on master. That commit was not the focus of this PR. I'm still in the process of testing it in a systemd env, but some other work came up that requires my attention. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good👍
I played around with this a bit, but I started to wonder. Ideally you could express something like "clear all binds if you see systemd activated sockets". Right now you can't. Then some other work got in between but I do plan to get back to this. |
3b5e4ba
to
5f39192
Compare
5f39192
to
3090f94
Compare
I have updated this PR and I think that the code itself is good enough to review. It is still lacking tests and docs, but I'd like to know if this is a good approach before writing those. Locally I tested it by running it directly (
|
Approach looks good to me |
e81a942
to
e571be5
Compare
Rebased and added a bit of text to the docs. Actual code is unchanged. |
The failed test timed out but I don't think I can restart it. |
Don't worry about single non-MRI failures. I can only re-run all, and odds are one of them will timeout randomly again. Greg is working on it and it's better than it used to be, but it's still not perfect. |
Bind to (systemd) activated sockets, regardless of configured binds. Systemd can present sockets as file descriptors that are already opened. By default Puma will use these but only if it was explicitly told to bind to the socket. If not, it will close the activated sockets. This means all configuration is duplicated. Binds can contain additional configuration, but only SSL config is really relevant since the unix and TCP socket options are ignored. This means there is a lot of duplicated configuration for no additional value in most setups. This option tells the launcher to bind to all activated sockets, regardless of existing binds. The special value 'only' can be passed. If systemd activated sockets are detected, all other binds are cleared. When they aren't detected, the regular binds will be used.
e571be5
to
e9f8345
Compare
Rebased to resolve the conflict in History.md. Is there anything else I can do to get this merged? |
Nope! Will review. |
Great stuff @ekohl 👍 |
Currently still a draft, based on #1360 (comment). Having it as a PR allows me to refer to this work. That's why the checklist is not filled in yet.
Description
Systemd can present sockets as file descriptors that are already opened. By default Puma will use these but only if it was explicitly told to bind to the socket. If not, it will close the activated sockets. This means all configuration is duplicated.
Binds can contain additional configuration, but only SSL config is really relevant since the unix and TCP socket options are ignored.
This means there is a lot of duplicated configuration for no additional value in most setups. This option tells the launcher to bind to all activated sockets, regardless of existing binds.
Note it does not clear configured binds. That means it's still possible to bind to an additional socket not configured as an activated socket.
Your checklist for this pull request
[changelog skip]
or[ci skip]
to the pull request title.[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.