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
Contributor

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.

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

@tmds tmds requested review from anurse, Tratcher and aspnet/build as code owners Jun 6, 2019

@tmds

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@Eilon Eilon added the area-hosting label Jun 6, 2019

public bool IsEnabled => _socketPath != null;

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

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jun 6, 2019

Member

Span<ServiceState>

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

int rv = socket.Send(data);

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jun 6, 2019

Member
  1. Async?
  2. Errors?

This comment has been minimized.

Copy link
@gfoidl

gfoidl Jun 6, 2019

Contributor

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?

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jun 6, 2019

Member

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

This comment has been minimized.

Copy link
@anurse

anurse Jun 6, 2019

Member

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.

This comment has been minimized.

Copy link
@tmds

tmds Jun 6, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@gfoidl

gfoidl Jun 6, 2019

Contributor

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

This comment has been minimized.

Copy link
@anurse

anurse Jun 6, 2019

Member

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()

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jun 6, 2019

Member

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.

This comment has been minimized.

Copy link
@anurse

anurse Jun 6, 2019

Member

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.

This comment has been minimized.

Copy link
@tmds

tmds Jun 6, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jun 6, 2019

Member

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>

This comment has been minimized.

Copy link
@anurse

anurse Jun 6, 2019

Member

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)

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jun 6, 2019

Member

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

Show resolved Hide resolved 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
var endPoint = new UnixDomainSocketEndPoint(_socketPath);
socket.Connect(endPoint);

int rv = socket.Send(data);

This comment has been minimized.

Copy link
@anurse

anurse Jun 6, 2019

Member

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.

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

This comment has been minimized.

Copy link
@anurse

anurse Jun 6, 2019

Member

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.

Update src/Hosting/Systemd/src/ServiceState.cs
Co-Authored-By: campersau <buchholz.bastian@googlemail.com>
/// <returns></returns>
public static IHostBuilder UseSystemd(this IHostBuilder hostBuilder)
{
if (SystemdHelpers.IsSystemdService ())

This comment has been minimized.

Copy link
@jkotalik

jkotalik Jun 7, 2019

Member
Suggested change
if (SystemdHelpers.IsSystemdService ())
if (SystemdHelpers.IsSystemdService())
Show resolved Hide resolved src/Hosting/Systemd/src/SystemdLifetime.cs
return;
}

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

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jun 10, 2019

Member

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?

This comment has been minimized.

Copy link
@tmds

tmds Jun 10, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@davidfowl

davidfowl Jun 10, 2019

Member

Sounds good.

@anurse anurse added this to the 3.0.0-preview7 milestone Jun 10, 2019

@davidfowl davidfowl merged commit 451fed1 into aspnet:master Jun 10, 2019

10 checks passed

Extensions-ci Build #20190607.1 succeeded
Details
Extensions-ci (Code check) Code check succeeded
Details
Extensions-ci (OSX Debug) OSX Debug succeeded
Details
Extensions-ci (OSX Release) OSX Release succeeded
Details
Extensions-ci (Test: Linux Source Build) Test: Linux Source Build succeeded
Details
Extensions-ci (Ubuntu 16.04 Debug) Ubuntu 16.04 Debug succeeded
Details
Extensions-ci (Ubuntu 16.04 Release) Ubuntu 16.04 Release succeeded
Details
Extensions-ci (Windows Release) Windows Release succeeded
Details
Extensions-ci (Windows debug) Windows debug succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.