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 Windows Services support #394

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

hez2010
Copy link
Contributor

@hez2010 hez2010 commented May 17, 2024

Add Windows Services support.
Users can use sc create "Microsoft Garnet Server" binPath=path/to/GarnetServer to create a Windows Services so that it can be run in the background.

Guarded under OperatingSystem.IsWindows() so it won't introduce any overhead for other OS.

@PaulusParssinen
Copy link
Contributor

PaulusParssinen commented May 17, 2024

👍 for adding generic host! It'll make a lot of future stuff such as configuration/metrics much cleaner.

catch (Exception ex)
var builder = Host.CreateEmptyApplicationBuilder(null);
builder.Services.AddHostedService(sp => new GarnetService(args));
builder.Services.AddWindowsService(options =>
Copy link
Contributor

Choose a reason for hiding this comment

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

As this says Windows service, just checking how does this work on Linux and other platforms?

Copy link
Contributor Author

@hez2010 hez2010 May 18, 2024

Choose a reason for hiding this comment

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

It does not affect other OS. AddWindowsServices is a no-op on other OS.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also no-op for if the process is not launched as service, see #394 (comment).

There is similar extension for Systemd integration (Microsoft.Extensions.Hosting.Systemd).

@badrishc badrishc requested a review from yangmsft May 17, 2024 18:39
@yangmsft
Copy link
Contributor

yangmsft commented May 17, 2024

Thank you for adding hosting capability for Garnet. To keep architecture clean and Garnet reusable by all parties, Garnet.Server standalone exe and hosting solution should ideally be separated (hosting should be a dedicated library in its own folder). This ensures the purity in terms of functionality of Garnet.Server, and do not unncessarily include redundant code in case other people have a different hosting strategy leveraging this standalone executable.

Let me know what you think - I am happy to have a discussion, or provide a starting place where you can make contributions!

@hez2010
Copy link
Contributor Author

hez2010 commented May 18, 2024

We use OperatingSystem.IsWindows() guard the OS so that other systems won't be affected by it.
Also, OperatingSystem.IsWindows is a compiler intrinsic in .NET and the branch (including all codepath called in that branch) will be removed completely after trimming if the target OS is not Windows, which makes sure no redundant code will be introduced on OS other than Windows.

As you can see when I publish the GarnetServer for Linux with /p:PublishTrimmed=true, the produced artifacts doesn't contain Microsoft.Extensions.Hosting.WindowsServices.dll at all.

image

@yangmsft does it resolve your concerns?
@PaulusParssinen we still need to use OperatingSystem.IsWindows otherwise the code won't be trimmed on other OS.

@PaulusParssinen
Copy link
Contributor

PaulusParssinen commented May 19, 2024

Ah right, if that's a concern for consumers of the standalone server then the guard has use 👍

The main server executable will be expected to get almost all of the dependencies in the screenshot above regarless, given future generic host & observability improvements to better align the Garnet with existing server application .NET practices (See ASP.NET, Orleans, Aspire, YARP..)

@yangmsft
Copy link
Contributor

We use OperatingSystem.IsWindows() guard the OS so that other systems won't be affected by it. Also, OperatingSystem.IsWindows is a compiler intrinsic in .NET and the branch (including all codepath called in that branch) will be removed completely after trimming if the target OS is not Windows, which makes sure no redundant code will be introduced on OS other than Windows.

As you can see when I publish the GarnetServer for Linux with /p:PublishTrimmed=true, the produced artifacts doesn't contain Microsoft.Extensions.Hosting.WindowsServices.dll at all.

image

@yangmsft does it resolve your concerns? @PaulusParssinen we still need to use OperatingSystem.IsWindows otherwise the code won't be trimmed on other OS.

My concern is on library/architectural design - GarnetServer as a project should be kept as strictly a library that produces standalone executable, for demo/exploration/proof-of-concept/other purposes.

Hosting solutions should be placed in a dedicated folder (/hosting, for example), with a dedicated csproj for each hosting model. Even in the same operating system there can be multiple ways of hosting Garnet, and windows service is just one way of hosting and thus should be modeled as a standalone library. Otherwise we will eventually end up with a significant number of if statements in main() for each OS/hosting solution etc. that will be hard to maintain and introduce a lot of noise for anyone trying to use it.

@hez2010
Copy link
Contributor Author

hez2010 commented May 20, 2024

Otherwise we will eventually end up with a significant number of if statements in main() for each OS/hosting solution etc. that will be hard to maintain and introduce a lot of noise for anyone trying to use it.

This is not true. We are using the extensible generic host provided by Microsoft.Extensions.Hosting. It was invented for any form of "hosting application" which doesn't require the developer to maintain the platform-specific code.
For example, the Windows Services integration is just an extension of the generic host and is a no-op on other platforms by default.

@yangmsft
Copy link
Contributor

Otherwise we will eventually end up with a significant number of if statements in main() for each OS/hosting solution etc. that will be hard to maintain and introduce a lot of noise for anyone trying to use it.

This is not true. We are using the extensible generic host provided by Microsoft.Extensions.Hosting. It was invented for any form of "hosting application" which doesn't require the developer to maintain the platform-specific code. For example, the Windows Services integration is just an extension of the generic host and is a no-op on other platforms by default.

While Microsoft.Extensions.Hosting sets up a good generic model for hosting applications, BackgroundService nonetheless introduces assumptions on hosting model and OS. I understand that redundant/unused dependencies will be excluded at compile time, but this does not avoid all the if statements if different hosting strategies are introduced even if they all come from Microsoft.Extensions.Hosting. As an example, Service Fabric, k8s and other containerized solutions all require different implementations and it is not economic to keep adding them to the same project - not to mention that Microsoft.Extensions.Hosting might not be the only hosting strategy.

In a sense GarnetServer project on its own is already a hosting solution - hosting Garnet as a standalone executable without any hosting infra. Other hosting solutions should live in their own dedicated projects.

@PaulusParssinen
Copy link
Contributor

PaulusParssinen commented May 20, 2024

In a sense GarnetServer project on its own is already a hosting solution - hosting Garnet as a standalone executable without any hosting infra. Other hosting solutions should live in their own dedicated projects.

This is fine for me. Windows service based hosting sample can be in its own project.

While Microsoft.Extensions.Hosting sets up a good generic model for hosting applications, BackgroundService nonetheless introduces assumptions on hosting model and OS. I understand that redundant/unused dependencies will be excluded at compile time, but this does not avoid all the if statements if different hosting strategies are introduced even if they all come from Microsoft.Extensions.Hosting.

As an example, Service Fabric, k8s and other containerized solutions all require different implementations and it is not economic to keep adding them to the same project - not to mention that Microsoft.Extensions.Hosting might not be the only hosting strategy.

However I would like to hear more on this.. What in containerized solutions is not handled by generic host? How is this not problem for the ASP.NET/Orleans etc.? Why do they "require different implementation"?

Doesn't the M.E.Hosting provide flexibility to provide customized implementations? (Given Garnet would implement it's hosting model to be built around it, instead of hand rolling its own)

@yangmsft
Copy link
Contributor

However I would like to hear more on this.. What in containerized solutions is not handled by generic host? How is this not problem for the ASP.NET/Orleans etc.? Why do they "require different implementation"?

Doesn't the M.E.Hosting provide flexibility and configuration to allow customized implementations?

I might have misspoken and I apologize if I caused confusion; to my limited knowledge, I am not aware of containerized solution that actively deviates from Microsoft.Extensions.Hosting - the main point is to not make assumptions. Flexibility of Microsoft.Extensions.Hosting does allow diverse implementations but that is kind of the point of not putting every custom implementation into the same project - if there is a complete hosting solution targeting some hosting infrastructure, it belongs to its own project. Choosing a hosting solution should be choosing the right csproj to build, rather than select the correct if-else (or switch case) branch among many implementations in the same project.

@PaulusParssinen
Copy link
Contributor

PaulusParssinen commented May 20, 2024

I might have misspoken and I apologize if I caused confusion; to my limited knowledge, I am not aware of containerized solution that actively deviates from Microsoft.Extensions.Hosting - the main point is to not make assumptions.

I agree wholeheartedly.

Flexibility of Microsoft.Extensions.Hosting does allow diverse implementations but that is kind of the point of not putting every custom implementation into the same project - if there is a complete hosting solution targeting some hosting infrastructure, it belongs to its own project. Choosing a hosting solution should be choosing the right csproj to build, rather than select the correct if-else (or switch case) branch among many implementations in the same project.

I think the generic hosting model avoids the branching quite well but that's not the point here, the if statement in this case is just the most straightforward response at the moment to address your concerns about the redundant code (which is generic host & trimming concern).

In my opinion, fully embracing generic host would get the best of both worlds. You could even hide that if statement behind the very flexible configuration abstraction which the generic host brings. I think using Generic Host in the standalone server would improve its configuration flexibility which I would imagine one wants from the main server binary. Garnet would also reap the benefits of the .NET DevDiv investments that are made to make this hosting solution AOT compatible (See Configuration Binder generator).

The generic host would allow the in-process consumers of the Garnet to configure the server to their liking and hook it into their DI (like Orleans and other libraries, e.g. .AddGarnetInProcOrSomething(o => ..)). Of course not everyone want's to learn the generic host model and we can also provide those consumers straightforward way to continue just new GarnetServer(..) away and register their custom commands and what not manually. All of these abstractions can also be used without requiring library consumers to use Generic Host.

@badrishc
Copy link
Contributor

Instead of modifying main/GarnetServer, can we perhaps add a new main/GarnetHost (open to other namings) so that the existing straightforward server is not affected or have to carry additional library burdens?

@badrishc
Copy link
Contributor

Instead of modifying main/GarnetServer, can we perhaps add a new main/GarnetHost (open to other namings) so that the existing straightforward server is not affected or have to carry additional library burdens?

@hez2010 - what do you think of this? thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants