-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Implement support for multiple sockets for systemd activation #24118
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
Conversation
src/Servers/Kestrel/Core/src/Systemd/KestrelServerOptionsSystemdExtensions.cs
Outdated
Show resolved
Hide resolved
ac675dd to
4933094
Compare
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.
byte. Is it documented anywhere that LISTEN_FDS cannot be larger than 127?
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.
No, this is actually allowed to be in range of [1;INT_MAX-3], but I thought it might be a good idea to limit it to a more reasonable number since I don't really believe that supporting inheritance of INT_MAX-4 sockets is a practical idea because I don't think it would be robust and/or testable.
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.
Still, matching systemd's "native" functionality is not an issue to me, if you believe this is a correct way of handling this.
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.
I also opted out of hard failure when invalid values for %LISTEN_FDS% (like 0 or -84) are presented, not sure if this is a proper course of action, since systemd "native" library is very explicit about invalid values (considers this an error) and missing envvar (treats this as a non-socket-activated case).
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.
Sorry for the slow reply, I think we should change this to an int. If LISTEN_FDS is really large, so be it.
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.
No problems, Stephen, had a lot on my plate recently too. I'll match sd_listen_fds behavior then.
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.
I think I fixed this, also rebased to more recent head.
|
It occured to me that the basic testing framework based on a docker container appears to be dead now, killed when Travis was removed and it wasn't picked up by the new CI pipeline. |
|
@onyxmaster You're correct to point out that we no longer have regression testing for the logic in UseSystemd since we stopped running tests on Travis. We do have a test for socket handles in general, but that's not affected by this change. You don't need to take responsibility for adding regression tests where there currently are none (thought that would certainly be welcome). For now, manual verification should sufficient. |
|
Thanks. I would really like to provide a test, and I think that taking a look at Azure Pipelines would be interesting, but I really don't have time for this right now 😔 |
60dd800 to
435e1e5
Compare
435e1e5 to
bddc440
Compare
bddc440 to
457118e
Compare
|
It does look like CI failures are unrelated to introduced changes. Would you please take a look? |
|
Thanks @onyxmaster! |
Implements support for multiple sockets when using systemd socket activation by taking into account the
%LISTEN_FDS%. Attempts to preserve backward feature compatibility, falling back to default number of sockets to be 1.Addresses #24067