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

Add option to bind to systemd activated sockets #2362

Merged
merged 1 commit into from Nov 10, 2020

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Sep 10, 2020

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

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@dentarg
Copy link
Member

dentarg commented Sep 10, 2020

👍 on 699e495, I've run into the same thing (perhaps it can be a separate PR?)

@MSP-Greg
Copy link
Member

@ekohl

Re the patch for minissl.rb, was that because it was compiled without SSL support? See #2305, which I need to test on JRuby.

@ekohl
Copy link
Contributor Author

ekohl commented Sep 10, 2020

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.

@MSP-Greg
Copy link
Member

@ekohl

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...

@ekohl
Copy link
Contributor Author

ekohl commented Sep 11, 2020

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.

lib/puma/minissl.rb Outdated Show resolved Hide resolved
@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Sep 17, 2020
@ekohl
Copy link
Contributor Author

ekohl commented Sep 17, 2020

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.

Copy link

@Adarsh2204 Adarsh2204 left a comment

Choose a reason for hiding this comment

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

All good👍

@ekohl
Copy link
Contributor Author

ekohl commented Oct 1, 2020

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.

@ekohl
Copy link
Contributor Author

ekohl commented Oct 15, 2020

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 (bundle exec bin/rails) and via a systemd socket. The systemd socket was listening on 0.0.0.0:5000 while configuring the bind on 0.0.0.0:3000. When using the only option it showed as:

Puma starting in cluster mode...
* Version 5.0.2 (ruby 2.7.1-p83), codename: Spoony Bard
* Min threads: 0, max threads: 16
* Environment: production
* Process workers: 2
* Preloading application
* Activated tcp://0.0.0.0:5000
Use Ctrl-C to stop
- Worker 0 (pid: 587260) booted, phase: 0
- Worker 1 (pid: 587265) booted, phase: 0

@nateberkopec nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Oct 15, 2020
@nateberkopec nateberkopec added this to the 5.1.0 milestone Oct 20, 2020
@nateberkopec
Copy link
Member

It is still lacking tests and docs, but I'd like to know if this is a good approach before writing those.

Approach looks good to me

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Oct 20, 2020
@ekohl
Copy link
Contributor Author

ekohl commented Oct 26, 2020

Rebased and added a bit of text to the docs. Actual code is unchanged.

@ekohl
Copy link
Contributor Author

ekohl commented Oct 27, 2020

The failed test timed out but I don't think I can restart it.

@nateberkopec
Copy link
Member

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.
@ekohl
Copy link
Contributor Author

ekohl commented Nov 4, 2020

Rebased to resolve the conflict in History.md. Is there anything else I can do to get this merged?

@nateberkopec
Copy link
Member

Nope! Will review.

@nateberkopec nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Nov 4, 2020
@nateberkopec nateberkopec merged commit 5af91ff into puma:master Nov 10, 2020
@nateberkopec
Copy link
Member

Great stuff @ekohl 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants