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 SystemdServices package that provides Host integration for systemd. #1804

Merged
merged 12 commits into from Jun 10, 2019

Conversation

@tmds
Copy link
Member

@tmds tmds commented Jun 6, 2019

This package provides a IHostBuilder UseSystemdService method that:

  • changes the Console logger to ConsoleLoggerFormat.Systemd
  • provides SystemdServiceLifetime that triggers shutdown on SIGTERM
  • and which uses the systemd notify protocol to inform systemd when the service
    has started, and when it is stopping.

When the extension method is used outside systemd, it noops.

Contributes to #1241.

This package provides a IHostBuilder UseSystemdService method that:
- changes the Console logger to ConsoleLoggerFormat.Systemd
- provides SystemdServiceLifetime that triggers shutdown on SIGTERM
- and which uses the systemd notify protocol to inform systemd when the service
has started, and when it is stopping.

When the extension method is used outside systemd, it noops.

Contributes to dotnet#1241.
@tmds tmds requested review from anurse, Tratcher and as code owners Jun 6, 2019
@tmds
Copy link
Member Author

@tmds tmds commented Jun 6, 2019

The implementation mostly is a copy-paste from Tmds.Systemd adjusted to make it more DI friendly and following style of the WindowsServices package.

I'll add some tests that verify the socket peer receives the expected messages next week.

@tmds
Copy link
Member Author

@tmds tmds commented Jun 6, 2019

src/Hosting/SystemdServices/src/ISystemdNotifier.cs Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/ISystemdNotifier.cs Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/ServiceHelpers.cs Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/ServiceHelpers.cs Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/SystemdNotifier.cs Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/SystemdNotifier.cs Outdated Show resolved Hide resolved
public bool IsEnabled => _socketPath != null;

/// <inheritdoc />
public void Notify(ServiceState state, ServiceState[] states)

Span<ServiceState>

var endPoint = new UnixDomainSocketEndPoint(_socketPath);
socket.Connect(endPoint);

int rv = socket.Send(data);
  1. Async?
  2. Errors?

Copy link

@gfoidl gfoidl Jun 6, 2019

I thought about async too, but it's called in a sync context, so there will be waiting anywhere.
Is socket.SendAsync(...).GetAwaiter().GetResult() a better option here?

It's currently called in a single place but this is a general purpose interface. We may need both sync and async versions.

Copy link

@anurse anurse Jun 6, 2019

Is socket.SendAsync(...).GetAwaiter().GetResult() a better option here?

Definitely not. I'd make this a NotifyAsync method and allow callers to decide what to do. For example, a caller could decide to fire-and-forget instead of blocking.

Copy link
Member Author

@tmds tmds Jun 6, 2019

We can treat this as a sync API. The buffer in the kernel is far larger than what we are trying to send.

Copy link

@gfoidl gfoidl Jun 6, 2019

Good to know. Can you add a comment for that fact please?

Copy link

@anurse anurse Jun 6, 2019

We can treat this as a sync API. The buffer in the kernel is far larger than what we are trying to send.

Hrm, we generally avoid operating under assumptions like that, but I see your point. Thoughts @davidfowl ?

public class UseSystemdServiceTests
{
[Fact]
public void DefaultsToOffOutsideOfService()

What's the strategy for testing this? Kestrel was using Docker to test the systemd activation feature https://github.com/aspnet/AspNetCore/tree/master/src/Servers/Kestrel/test/SystemdActivation, we should probably do the same here.

Copy link

@anurse anurse Jun 6, 2019

Some of it could be tested without systemd at all. For example the notifier could be tested by creating a socket, flowing it down (preferably via internals rather than an env-var) and reading from the other end.

But I agree, docker for a simple integration test would be good as well.

Copy link
Member Author

@tmds tmds Jun 6, 2019

For example the notifier could be tested by creating a socket, flowing it down (preferably via internals rather than an env-var) and reading from the other end.

I plan to add such a test.

Kestrel was using Docker to test the systemd activation feature https://github.com/aspnet/AspNetCore/tree/master/src/Servers/Kestrel/test/SystemdActivation, we should probably do the same here.

Are you running these tests on regular basis?

They don't run at all, in fact, I need to file an issue to make them run again.

<TargetFramework>netstandard2.1</TargetFramework>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>hosting</PackageTags>
<IsShipping>true</IsShipping>
Copy link

@anurse anurse Jun 6, 2019

Should we consider this for the linux shared framework? That would make it possible to enable it by default in the worker template (assuming it does what .UseWindowsServices does and only lights up if actually hosted in systemd).

That would also mean it needs to be in source-build.

Of course, I don't know exactly what (if any?) shared framework the worker template references. Does it reference Microsoft.AspNetCore.App or Microsoft.NETCore.App? If the latter, the point is moot, it's not going in to that shared fx in this release. (cc @glennc)

We don't do this for the windows service so we shouldn't do it here either....

src/Hosting/SystemdServices/Directory.Build.props Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/ServiceHelpers.cs Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/ServiceHelpers.cs Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/SystemdNotifier.cs Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/ServiceState.cs Outdated Show resolved Hide resolved
var endPoint = new UnixDomainSocketEndPoint(_socketPath);
socket.Connect(endPoint);

int rv = socket.Send(data);
Copy link

@anurse anurse Jun 6, 2019

Is socket.SendAsync(...).GetAwaiter().GetResult() a better option here?

Definitely not. I'd make this a NotifyAsync method and allow callers to decide what to do. For example, a caller could decide to fire-and-forget instead of blocking.

src/Hosting/SystemdServices/src/SystemdServiceLifetime.cs Outdated Show resolved Hide resolved
src/Hosting/SystemdServices/src/SystemdServiceLifetime.cs Outdated Show resolved Hide resolved
public class UseSystemdServiceTests
{
[Fact]
public void DefaultsToOffOutsideOfService()
Copy link

@anurse anurse Jun 6, 2019

Some of it could be tested without systemd at all. For example the notifier could be tested by creating a socket, flowing it down (preferably via internals rather than an env-var) and reading from the other end.

But I agree, docker for a simple integration test would be good as well.

Co-Authored-By: campersau <buchholz.bastian@googlemail.com>
/// <returns></returns>
public static IHostBuilder UseSystemd(this IHostBuilder hostBuilder)
{
if (SystemdHelpers.IsSystemdService ())
Copy link
Member

@jkotalik jkotalik Jun 7, 2019

Suggested change
if (SystemdHelpers.IsSystemdService ())
if (SystemdHelpers.IsSystemdService())

src/Hosting/Systemd/src/SystemdLifetime.cs Show resolved Hide resolved
return;
}

using (var socket = new Socket(AddressFamily.Unix, SocketType.Dgram, ProtocolType.Unspecified))

Is it reasonable to open and close these on demand? Is there a universe where we'd want to keep the connection open because of sending other types of messages?

Copy link
Member Author

@tmds tmds Jun 10, 2019

This is only for occasional messaging (now: two messages per application lifetime).
The native sd_notify implementation does the same thing: create and close socket per message.
We can change the scope of the socket to the SystemdNotifier instance if it ever makes sense.

Sounds good.

@anurse anurse added this to the 3.0.0-preview7 milestone Jun 10, 2019
@davidfowl davidfowl merged commit 451fed1 into dotnet:master Jun 10, 2019
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet