microsoft / service-fabric-aspnetcore

This repo contains ASP.NET Core integration for Service Fabric Reliable Services.
Other
152 stars 49 forks source link

Integrate with generic host (IHost interface) in asp.net core 2.1 #48

Open chuanboz opened 6 years ago

chuanboz commented 6 years ago

asp.net core is refactor with generic host in https://github.com/aspnet/Hosting/issues/1163 that has better support on DI and lifecycle management etc, but I do not see an integration for the ServiceFabric-AspnetCore.

the ask is to provide a ServiceFabricHost that implement IHost interface, to wrap the generic logic to register service type and also creating service instance.

here are specific difference for Service Fabric that deserve its own ServiceFabricHost:

  1. different dependency scope of service instance level.
    • Host scope (Singleton) -- Service instance scope (* unique for Service Fabric) ---Request scope ----Transient scope
  2. different execution and lifecycle for IHostedService. for regular console host, the IHostedService shall be started right after host start; but for Service Fabric, it shall be registered with service factory and won't start the IHostedService directly; All IHostedService will be started as part of ICommunicationListener.OpenAsync; and IHostedService could be started multiple times within the same process, each time whenever there is a new service instance created.

here are sample scenario usage:

public static class ServiceFabricHostBuilderExtension
    {
        public static IHostBuilder UseServiceFabricHost(this IHostBuilder hostBuilder, string ServiceTypeName);

        public static IHostBuilder ConfigureWebHost(this IHostBuilder hostBuilder, Action<HostBuilderContext, IWebHostBuilder> configureWebHost);
    }

/// <summary>
        /// This is the entry point of the service host process.
        /// </summary>
        private static void Main()
        {
            var builder = new HostBuilder();

            // required, register service factory with ServiceFabricHost
            builder.UseServiceFabricHost("TestHttpServiceType");

            // config WebHost
            builder.ConfigureWebHost((context, webHostBuilder) => // the same native Startup experience from WebHost
                    webHostBuilder
                    .UseKestrel() // config to use Kestrel or HttpSys
                    .UseStartup<Startup>() // further extension with standard asp.net core Startup extension.
                );

            // optional, standard DI injection, injection here will be used in both host and service instance
            builder.ConfigureServices(sc => { });
            builder.ConfigureAppConfiguration(sc => { }); // configuration that will be applied to service instance.
            builder.ConfigureLogging(loggingBuilder => { }); // optional to config additional logging.

            // required, run the service until cancelled or aborted
            builder.Build().Run();
        }
amanbha commented 6 years ago
chuanboz commented 6 years ago

I will use this feature in Microsoft Intune which builds on top of Service Fabric + Asp.net Core. Within Intune we have many Microservice teams and one common infra team, I want to leverage the flexible extensions support provided by the generic host and hook it up with Service Fabric, so that each Microservice team register their custom logic follow the generic host extension point, while as common infra team we could control certain common function consistently.

I'm still working on get the 2 tied together, I will share some sample code to demonstrating this later this month.

OlegKarasik commented 5 years ago

@chuanboz Can you take a look at this project? Maybe it can be of any use.

chuanboz commented 5 years ago

thanks @OlegKarasik for the link of your implementation on the similar idea to link generic host and service fabric, glad to know that I'm not alone here.

I have implemented above already and is using it in Microsoft Intune, but after discussing with @amanbha and @vturecek from Service Fabric team, they are hesitate to take the contribution for this yet in favor of Service Fabric Mesh as the future. I could understand where they're from for that decision, since Service Fabric Mesh has a complete different app model, in fact with SF Mesh you could just use regular generic host and regular asp.net core directly.

Several of internal Microsoft members from different teams has also reached out to me directly asking about the reference implementation details. I need all of you help to vote for this idea to show enough support, hoping that someday we could publish this as a public reference implementation and with the spirit of open source and community support this could become matual over time.

chuanboz commented 5 years ago

and for reference, here is the core interface that I used to implement this to link generic host with service fabric by translate the host service provider to service replica service provider, rest of interface are just the public one from generic host or from service fabric.

public interface IServiceProviderAdapter
{
        /// <summary>
        /// translate the host service provider to service replica (partition for statefull service or instance for stateless service) service provider
        /// </summary>
        /// <param name="hostServiceProvider">the service provider from process host.</param>
        /// <param name="serviceContext">The service context used to create service replica.</param>
        /// <returns>the service provider used for service replica dependency resolution.</returns>
        IServiceProvider CreateReplicaServiceProvider(IServiceProvider hostServiceProvider, ServiceContext serviceContext);
}
OlegKarasik commented 5 years ago

@chuanboz Thanks for the great comment!

I didn't dive deep into Mesh but as far as I understand (and hope) it isn't a replacement for Service Fabric (because if I am not mistaken there is no way to create stateful services in Mesh). I think when we would have a first release of Mesh then Service Fabric team will have some time to take a look at what community did to integrate generic host and Service Fabric.

johncrim commented 5 years ago

This definitely still needs to be implemented - from https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/generic-host?view=aspnetcore-3.0 :

Starting in ASP.NET Core 3.0, Generic Host is recommended for both HTTP and non-HTTP workloads. An HTTP server implementation, if included, runs as an implementation of IHostedService. IHostedService is an interface that can be used for other workloads as well.

Note that IHostedService is very similar to ICommunicationListener.

While addressing this, I would also like to see separation between building the app (configuration, services, http pipeline) and creating listeners. For example, it should be easy to create both HTTP and HTTPS listeners using the same configuration, DI container, and HTTP pipeline. The current design of AspNetCoreCommunicationListener doesn't support this separation very well.

I'm willing to provide this as a contribution, if it would be considered.

amanbha commented 5 years ago

@johncrim Thanks for offering to contribute, sure, we would consider your contribution as long as tis not a breaking change. Please feel free to create a PR.

chuanboz commented 5 years ago

@johncrim , in your design to use IHostedService as ICommunicationListener, can you clarify the following:

  1. does the service fabric host and asp.net core app share the same IServiceProvider or different, and how do you plan to handle the case of multiple service instances hosted on the same process?
  2. does asp.net core app and service fabric host share the same lifetime, in other words if the app stopped does it also stop the host, or support the scenario of just stop current instance of app and create a new instance but still keep the host active?

I'm asking because we implemented this already with a different model as your above and want to check if we could possible converge over time and have one shared public supported experience eventually.

OlegKarasik commented 5 years ago

Hi @johncrim @amanbha, I have worked on the project that does integration with generic host (I was mentioning it about a year ago in the comments above in discussion with @chuanboz).

Does it looks like something you are looking for? (You can check projects Wiki for more detailed information about all the features).

johncrim commented 5 years ago

Here's my analysis + opinion on what can and should be done. The work to provide a good generic host design seems bigger than I initially expected, based on the design of Microsoft.ServiceFabric.AspNetCore.* and the implementation of GenericWebHost.

Current Design + Limitations

  1. A Service Fabric service (as defined in a ServiceManifest.xml) can have multiple service types, and multiple endpoints within a service process/code package. There is no direct association in the ServiceManifest.xml between endpoints and service types - the default is all service types listen on all endpoints, and service types can be addressed by URL prefix on all endpoints.
  2. A Service Fabric service in Microsoft.ServiceFabric.AspNetCore.* effectively requires a separate WebHost per CommunicationsListener. The CommunicationsListener starts and stops the WebHost. Since the CommunicationListener is a child of a (Stateless/Stateful) service (aka a service type), this means a separate WebHost per service type.
  3. ServiceFabricMiddleware in Microsoft.ServiceFabric.AspNetCore.* only handles a single partition or replica (see ServiceFabricMiddleware.cs). It clearly assumes that the WebHost is not shared between more than one service type.

In other words, the current design of Microsoft.ServiceFabric.AspNetCore.* results in this object hierarchy (representing ownership/lifetimes) per process:

Microsoft.ServiceFabric.AspNetCore.Kestrel and Microsoft.ServiceFabric.AspNetCore (github:microsoft/service-fabric-aspnetcore) are integration libraries on top of Microsoft.ServiceFabric.Services, Microsoft.ServiceFabric.Data and Microsoft.ServiceFabric (github:microsoft/service-fabric-services-and-actors-dotnet and github:microsoft/service-fabric) - the good news is that this hierarchy is only required by the Microsoft.ServiceFabric.AspNetCore.* projects, not by the inner service fabric libs.

Also note that a "CommunicationListener" object in ServiceFabric libs doesn't explicitly do anything with communications. It has lifecycle methods (open/close/abort) and an endpoint string (open returns the endpoint string). In my opinion the key flaw with the current design of Microsoft.ServiceFabric.AspNetCore is making the CommunicationListener the required owner of a WebHost. A good generic host implementation should also address this.

At worst, this existing design appears to be at odds with the intended design of service fabric core, given that it doesn't support multiple endpoints providing access to multiple service types. At best, it's wasteful and is likely to cause confusion when some developers can't figure out that there are multiple WebHosts/DI containers/etc being created. I would also characterize the current setup process as clunkier than it needs to be and misleads developers who are new to Service Fabric.

Design Proposal

Putting aside the need to be backwards compatible, I think it's worth discussing an "ideal" design within generic hosts. Then we can come back to the topic of making it backwards compatible.

Principles:

The obvious common use case is 1 service type, 1 communication listener, 1 DI container, and 1 WebHost per service process.

"Advanced" scenarios include:

Here's what I think is the correct object hierarchy within a generic host:

It should be possible to put 1 or more WebHosts within the ServiceFabric IHostedService, but I think a better default is 1 WebHost per Host. With a little additional work, it should be possible to use a separate DI container and web host pipeline, and port per SF service.

To put this in terms of app setup code, I envision something like the following:

var hostBuilder = new HostBuilder()
    .UseContentRoot(Directory.GetCurrentDirectory())
    // Host configuration
    .ConfigureAppConfiguration(config =>
    {
        config.AddEnvironmentVariables()
              .AddCommandLine(args)
              .AddServiceFabricConfiguration();
    })
    // Host level services
    .ConfigureServices(services => /* ... */)
    // Web host pipeline exposes APIs used by all the SF services
    .ConfigureWebHost(webhostBuilder =>
     {
         webhostBuilder.UseStartup<ServerStartup>()
                       .UseKestrel();
     })
    .ConfigureServiceFabricHost((hostContext, sfHostBuilder) =>
    {
        sfHostBuilder.RegisterService<MyStatelessService>("StatelessServiceTypeName", (serviceContext, sfServiceBuilder) =>
        {
      sfServiceBuilder.UseEndpoints(serviceContext.CodePackageActivationContext.GetEndpoints(), ServiceFabricEndpointPrefix.None);
        });
        sfHostBuilder.RegisterService<MyStatefulService>("StatefulServiceTypeName", (serviceContext, sfServiceBuilder) =>
        {
      sfServiceBuilder.UseEndpoints(serviceContext.CodePackageActivationContext.GetEndpoints(), ServiceFabricEndpointPrefix.Partition);
        });
    });

using (var host = hostBuilder.Build())
{
    host.Start();
    Console.WriteLine($"SF Node started.");
    Console.WriteLine("Use Ctrl-C to exit.");
    await host.WaitForShutdownAsync();
}

One question that came up when thinking this through: What should control the lifetime of the WebHost(s)? The WebHost could run within the ServiceFabricHostedService (a new IHostedService that would be created), so it is started only when one or more service types are started; or it could run within the GenericWebHostService, which is the setup for normal ASP.NET servers; or both could be supported.

Unfortunately, if one wants to setup a WebHost within a generic host, the current design (as of ASP.NET Core 3.0 preview 7) requires that the WebHost is run within a GenericWebHostService - it doesn't appear to be a supported scenario to setup a WebHost within a generic host, and then start and stop it within a custom IHostedService. (See ConfigureWebHost(this IHostBuilder builder, Action<IWebHostBuilder> configure) - both GenericWebHostBuilder and GenericWebHostService are internal). I've opened https://github.com/aspnet/AspNetCore/issues/11884 to see if we can separate them, though even if these classes aren't made public it may be possible to functionally separate them with some DI hacking.

Also unfortunately, the only way to preserve backward compatibility with the current design of Microsoft.ServiceFabric.AspNetCore.* within a generic host would be run each webhost within the communication listener. Which currently isn't possible, but would be if https://github.com/aspnet/AspNetCore/issues/11884 is fixed.

My preference would be to support running WebHosts both within the standard AspNetCore hosted service, and within the ServiceFabricHostedService; and have the remainder of the integration functionality (middleware, communication listeners, etc) work with both setups.

It's clear to me that the current design of the Microsoft.ServiceFabric.AspNetCore.* libraries aren't compatible with the generic host concept. IMO, to preserve backward compatibility Microsoft.ServiceFabric.AspNetCore.* should remain as is (WebHost only); and a new library should be created (perhaps Microsoft.ServiceFabric.AspNetCore.GenericHost?) with a new integration design and new middleware, supporting routing multiple service types within a web host.

Existing Solutions

The coherent solutions [https://github.com/coherentsolutionsinc/aspnetcore-service-fabric-hosting] project that @OlegKarasik linked looks well done. The docs appear good, and the setup looks more like what I would expect. I'm not yet clear on whether it has support for sharing a WebHost between service types; and it doesn't look like there is any middleware that replaces ServiceFabricMiddleware and handles service addressing. I will certainly try it out.

Next Steps

For my use cases, I'm going to evaluate the coherent solutions lib and/or implementing a generic ServiceFabricHostedService that exposes the endpoints implemented by our controllers and uses the core SF libs more directly (fewer abstractions in the middle).

I don't think I can help with this issue given the current limitations in the GenericWebHost and the need to keep KestrelCommunicationListener and AspNetCoreCommunicationListener backwards compatible. As noted above, I don't think it makes sense to add generic host support to the current library; however I'd be happy to contribute to a new library, particularly if Microsoft expresses interest in creating one.

What do you all think?

johncrim commented 5 years ago

@chuanboz - to answer your earlier questions:

  1. does the service fabric host and asp.net core app share the same IServiceProvider or different, and how do you plan to handle the case of multiple service instances hosted on the same process?

IMO both should be possible. I do think that the generic host design generally favors a single, host-level DI container/IServiceProvider, because it's simplest, and scoping can be added where necessary to control the lifetime of injected objects. But I also think that one should be able to create a separate webhost and DI container per service type if required.

  1. does asp.net core app and service fabric host share the same lifetime, in other words if the app stopped does it also stop the host, or support the scenario of just stop current instance of app and create a new instance but still keep the host active?

ServiceFabric (the system) already controls the lifetime of the executable (aka code package). Your executable won't be started until SF decides it needs the service types declared in your ServiceManifest.xml. I think it's worth keeping that in mind.

But in general I think both scenarios should be supported. You should be able to run a webhost pipeline per service instance (and have the service start/stop the webhost), and you should be able to share a webhost pipeline between any number of services.

chuanboz commented 5 years ago

Principles:

  • Make the common cases easy + intuitive; support advanced scenarios
  • Use AspNetCore concepts + classes wherever possible, don't wrap or hide them
  • Give developers the flexibility they expect with normal AspNetCore development

Agree on those principles, and I'd like to add one more: Introduce fewer new concept for the integration.

The obvious common use case is 1 service type, 1 communication listener, 1 DI container, and 1 WebHost per service process.

There is 1 use case even for 1 service type 1 listener, that the service instance could have failures to instantiate initially but subsequencial instantiate could success, in such case the host process does not need to be restarted, only the service instance needs to be restarted, to avoid the cost of creating new process.

My preference would be to support running WebHosts both within the standard AspNetCore hosted service, and within the ServiceFabricHostedService; and have the remainder of the integration functionality (middleware, communication listeners, etc) work with both setups.

the service fabric hosting model supports to restart the service instance multiple times upon failures to instantiate the service types. If you use HostedService, how do you plan to support to restart the instance. IHostedService is one direction of StartAsnc->StopAsync.

It's clear to me that the current design of the Microsoft.ServiceFabric.AspNetCore.* libraries aren't compatible with the generic host concept. IMO, to preserve backward compatibility Microsoft.ServiceFabric.AspNetCore.* should remain as is (WebHost only); and a new library should be created (perhaps Microsoft.ServiceFabric.AspNetCore.GenericHost?) with a new integration design and new middleware, supporting routing multiple service types within a web host.

Agree on new libraries should be introduced. I used Microsoft.ServiceFabric.AspNetCore.Hosting and Microsoft.ServiceFabric.AspNetCore.Hosting.Abstractions to align with asp.net core extensions naming pattern, but Microsoft.ServiceFabric.AspNetCore.GenericHost would also work and actually a good name.

I don't think I can help with this issue given the current limitations in the GenericWebHost and the need to keep KestrelCommunicationListener and AspNetCoreCommunicationListener backwards compatible. As noted above, I don't think it makes sense to add generic host support to the current library; however I'd be happy to contribute to a new library, particularly if Microsoft expresses interest in creating one.

Agree on new library route, that's also what I used.

What do you all think?

I like your overall thinking and principles, but I have concern to use IHostedService to host WebHost, I see that would work with simple scenario but not work with more complex scenarios.

take a look of https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-hosting-model especially the shared process models and use that to validate the design.

OlegKarasik commented 5 years ago

Hi @johncrim,

Thanks you for such a detailed walk through and warm words about the project :)

I'm not yet clear on whether it has support for sharing a WebHost between service types

Can you elaborate a bit more on this? Looks like I don't really understand the concept of shared WebHost.

and it doesn't look like there is any middleware that replaces ServiceFabricMiddleware and handles service addressing

The ServiceFabricMiddleware can be configured as described here.

Please feel free to ask any question about the project.

chuanboz commented 5 years ago

@OlegKarasik and @johncrim , can you clarify what's issues you're trying to address with the generic integration? In other words, let's say this is implemented, how do you plan to use it, and how that make your experience better?

I want to understand better of the user scenario and use case. For us, it's mainly for integration test support without the service fabric dependency, and also to launch the service directly without service owner to change their code, and faster debuging without deployment, with a mock service fabric clusters.

OlegKarasik commented 5 years ago

@chuanboz In general the ideas I have behind the project are the following:

  1. Simplify configuration of Reliable Services:

    • Remove a need for infrastructure level classes i.e. descendant of StatelessService or StatefulService, ..., etc.
    • Create well defined configuration flow for common operations i.e. configuring listener, code to execute on RunAsync, ..., etc.
    • ... more examples are on Wiki or inside README.md
  2. Simplify dependency injection scenarios:

    • Allow to register a dependency on HostBuilder level and consume it inside listeners, code in RunAsync, ..., etc.
    • ... more examples are on Wiki.
  3. Simplify debugging of Reliable Service (currently this is the work in progress for Stateless service).

    • Idea is to allow simple services to be debugged without a need to deploy them to Service Fabric cluster.
  4. Support for test scenarios - this is something in a very early design phase and currently depends on the 3rd point.

chuanboz commented 5 years ago

thanks @OlegKarasik for the details reponse and link to well structured and maintained wiki. Seems we have many in common, especially your point 2, 3,and 4 that's the driving factor for me.

@johncrim , can you share your thought on the issues to address and the goal to achieve with this?

I briefly meet with Service Fabric team and as @amanbha already called out, they are willing to accept the contribution now as long as it does not introduce breaking change.

I will spend sometime to clean up and prepare my current implementation used in Microsoft Intune, and send a PR for discussion.

@johncrim and @OlegKarasik , we shall work together and bring up the best of all we have to support majority of sceanrios called out above out of box and make it extensible for service owner to hook up to add remaining themselves.

johncrim commented 5 years ago

I agree with @OlegKarasik and @chuanboz RE all the benefits of the generic host approach (global/shared DI), simplified + more intuitive configuration/startup code (eg more along the lines of "normal" ASP.NET Core apps), better support for testing/mocking, and removing what appear to be legacy design requirements (eg subclassing StatelessService or StatefulService). It appears that we're in agreement about these benefits.

I wrote my own SF host integration library, to allow using the generic host model in ASP.NET Core. This replaces the 2+ SF integration libraries (Microsoft.ServiceFabric.Services and Microsoft.ServiceFabric.AspNetCore.Kestrel, which depend on Microsoft.ServiceFabric), and uses a direct dependency on Microsoft.ServiceFabric, which is the primary .NET/C++ interop API to ServiceFabric. It provides these benefits:

Oleg's library is much more feature complete than my library is - but my library meets our needs for now, and the small size is a virtue. I will share it on github as soon as I can.

One point which seems contentious (and which I don't think we're all in agreement on) is backward compatibility, and along with it the need to preserve StatelessService and StatefulService as application base classes. In my view (as stated above) the better option is a new ASP.NET Core integration library that meets all of the above goals. I do think it would be ideal to provide an easy migration path for existing projects (from the existing Microsoft.ServiceFabric.AspNetCore.* libraries to this new library), and probably a migration guide document would be a useful design document for this new library.

IMO backward compatibility of existing Microsoft.ServiceFabric.AspNetCore.* libraries should be achieved by not significantly changing them.

aL3891 commented 5 years ago

i would love to see this as well, thank you @johncrim @OlegKarasik and @chuanboz for pushing for it.

we're also looking for to run multiple services in a single host as you describe, as well as sharing a webhost.

for us the shared webhost scenario is basically multiple services sharing a port, so instead of having service1 at 10.0.0.1:5000 and service2 at 10.0.0.1:5002 we'd like to have service1 at 10.0.0.1:4000/Service1 and 10.0.0.1:4000/Service2. We want to do this to create a way for services to happen to be on the same host to be able to call each other really quickly, via a custom in-process path. (thats not really part of the generic host stuff, i just wanted to give some background)

Mesh is not really usable for us since it lacks support for the app models the SF team has pushed for years.. (as well as the inability to host multiple services in a single host)

if you need any help or scenario validation, i'd love to help out :)

OlegKarasik commented 5 years ago

@aL3891 Sounds interesting. What about web host configuration? Do these service also share all the DI stuff?

aL3891 commented 5 years ago

That would be the case in my solution yeah (i havent gotten it working yet) but there would only be one kestrel instance running per host, and in the default case when a new replica came online, it would just be another route in the already running kestrel server. we'd have some custom middleware that a replica would interact with via DI and basically tell the middleware "i'm running on this host now"

Those last parts havent really materalized yet, but even in a more traditional one-kestrel-per-replica scenario, we'd want to share some DI services globally on the host. We're making a online mobile strategy game, things we'd keep in the host level scope would be things like all the game settings, caches for bases and resource tiles and so on.

chuanboz commented 5 years ago

I'm preparing my changes to send out for review and discussion, I see 2 open issues and want to seek feedbacks:

  1. right now SF repo still support asp.net core 1.1, the LTS release is 2.1, any concern to make new library depends on asp.net core 2.1?
  2. right ow SF repo build both .net framework and .net standard library, any concern to only build .net standard 2.0 for the new library?

I propose to add this constraints for the new library as I see anyone that could benifet from the new library would already use asp.net core anyway, that will reduce the support cost as well as development cost for the new library.

amanbha commented 5 years ago

@chuanboz I concur with having these constraints for the new binary and its nuget package.

chuanboz commented 5 years ago

I've added the initial changes that I have used, I will spend more test to add integration test to cover the changes, but the major part of change including integration test support and visual studio F5 support are there.

End user experience is like this:

private static void Main()
        {
            var hostBuilder = CreateHostBuilder();
            hostBuilder.Build().Run();
        }

        public static IHostBuilder CreateHostBuilder()
        {
            return new HostBuilder()
                .UseServiceFabricHost()
                .UseServiceFabricRuntime()
                .ConfigureWebHost(b => b.UseStartup<Startup>())
                ;
        }

details see https://github.com/microsoft/service-fabric-aspnetcore/compare/develop...chuanboz:GenericHost

@johncrim , @OlegKarasik and @aL3891 , let me know your feedback.

After all the changes are done, I expect the following experience to be true:

  1. The service code could be launched directly in visual studio without deployment and dependency on service fabric, just launch in second (VS F5 experience).
  2. The same service code could also be launched with service fabric without change.
  3. The integration test could use the same code created for IHostBuilder to launch service without service fabric environment, and run the test just fine. (Built-in Integration Test support).
  4. Support multiple services with the service fabric cluster to communicate with each other, and test the logic in integration tests, without service fabric dependency, and without service code change.

thanks, Chuanbo

aL3891 commented 5 years ago

Sorry, been on my phone only all week so reading code is a bit difficult but you solution looks close to what I was cooking up, a global scope, then a scope for each replica, when you put the context instance in an adapter class.

One thing I did, because I also need this to work for reliable services where I need to conform to certain constructors, was to add the context to a generic IValueHolder. Then I have a transient DI registration for the context class that pulls the instance out of the scoped IValueHolder. That way I can have the service constructors be the same. There might be a better way to solve this but I thought it was kind of neat :)

I'll post some code once I'm back behind a proper keyboard

aL3891 commented 5 years ago

I also looked at @OlegKarasik library, it's really comprehensive and also very well documented :). To me it does look like quite a departure from the built in models though, it's almost like it's own programming model. not saying that's a bad thing but I think we'd have to rewrite a non trivial amount of code to adapt it fully, perhaps there is a middle road that is also easier to port exsisting apps to

OlegKarasik commented 5 years ago

@chuanboz I have read the code and look like now I understand the idea much better :) I am not sure (would need to try the code in action) but it looks like there is no way to specify more than one listener?


@aL3891 Thanks for the good words :) I wasn't tried to introduce a separate programming model but rather simplify the process of configuring reliable services in general (ASP.NET Core and Remoting listeners, delegates that execute on service life-cycle and RunAsync events, ..., etc.), but it also can be used to satisfy very simple use cases like configuring a single ASP.NET Core listener:

private static void Main(string[] args)
{
    new HostBuilder()
        .DefineStatefulService(
            serviceBuilder =>
            {
                serviceBuilder
                    .UseServiceType("ServiceType")
                    .DefineAspNetCoreListener(
                        listenerBuilder =>
                        {
                            listenerBuilder
                                .UseEndpoint("ServiceEndpoint")
                                .ConfigureWebHost(webHostBuilder => { 
                                  webHostBuilder.UseStartup<Startup>(); 
                                });
                        });
            })
        .Build()
        .Run();
}
aL3891 commented 5 years ago

aha i see, but the idea is to specify all those things when setting up DI as opposed to inside a "Service class" right? The actual "service" is just the service method implementations as i understand, and then you inject the service context to use reliable collections? again, not saying that's bad or anything, but we use actors quite a bit and they're more constrained in how much you can depart from the traditional model, i didnt see any actor stuff in wiki but i may have missed it

chuanboz commented 5 years ago

@chuanboz I have read the code and look like now I understand the idea much better :) I am not sure (would need to try the code in action) but it looks like there is no way to specify more than one listener?

By listener, do you mean the communication listener or http listener? with a slight change in StatelessCommunicationService, it could support multiple communication listener as below:

public StatelessCommunicationService(StatelessServiceContext serviceContext, IEnumerable<ICommunicationListener> listeners, IApplicationLifetime applicationLifetime, ILogger<StatelessCommunicationService> logger, IOptions<ServiceFabricHostOptions> options)
            : base(serviceContext)
        {
            this.listeners = listeners ?? throw new ArgumentNullException(nameof(listener));
            this.options = options?.Value ?? throw new ArgumentNullException(nameof(options));
            this.applicationLifetime = (applicationLifetime as ApplicationLifetime) ?? throw new ArgumentNullException(nameof(applicationLifetime));
            this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
        }

        internal IStatelessServicePartition ServicePartition => this.Partition;

        /// <summary>
        /// Optional override to create listeners (like tcp, http) for this service instance.
        /// </summary>
        /// <returns>The collection of listeners.</returns>
        protected override IEnumerable<ServiceInstanceListener> CreateServiceInstanceListeners()
        {
            return this.listener.Select(l => new ServiceInstanceListener[]
            {
                new ServiceInstanceListener(serviceContext => l),
            };
        }
OlegKarasik commented 5 years ago

@chuanboz Yeah, that is what I was about. One more thing, correct me if I am wrong but as far as I understand you are focusing only on ASP.NET Core listeners not remoting?


@aL3891 The project is all about Reliable Services, so there is no Reliable Actors support :(

chuanboz commented 5 years ago

Yeah, that is what I was about. One more thing, correct me if I am wrong but as far as I understand you are focusing only on ASP.NET Core listeners not remoting?

@OlegKarasik , That's right I only focus on ASP.NET Core listeners. By remoting, do you mean .net remoting? From what I understand that's not supported in .net core moving forward and is recommended to move to ASP.Net core gRPC support, which still work with ASP.NET Core and my proposed change above.

aL3891 commented 5 years ago

@OlegKarasik No worries, i'm working on somehing on my own :)

@chuanboz i think he means service fabric reliable services remoting, we use that extensivly as well, so im also working on supporting that with generic host

chuanboz commented 5 years ago

@aL3891 , thanks for clarify, I have not used that remoting, but that does not seems provided in the default asp.net support, and so I'd think that shall be a separate packages to add that support. but what we do here with generic host integration shall be extensible to easily add that integration.

from what I quickly read with your link, the StatelessCommunicationService class support to accept ICommunicationListener from DI, then you could just register one more ICommunicationListener to add support for SF remoting.

aL3891 commented 5 years ago

No you're correct, the SF remoting is separate from asp.net, however i think the generic host support should be applied for all service fabric services, with specializations for asp.net if that's needed. It does seem to me that all types of services could inject ICommunicationListeners so i wonder how much special sauce is actually needed for asp.net specifically, and how much can be done with "vanilla" dependnecy injection

I've published my take on generic host intefration with service fabric over at https://github.com/aL3891/ServiceFabricGenericHost

Right now, i've focused on the actual service lifetime more than the communication listeners, but it currently supports Actors, Stateless/Stateful reliable services and stateless/stateful asp.net services. i havent really tested it yet but its also ment to support hosting multiple services in a host, something that we'd really like to do.

Another difference is that i'm not actually using a custom IHost implementation but instead is doing the registration inside a hosted service. I might be missing something but i dont really see a downside to doing this and also it interferes less with the runtime and also allows there to be multiple such services, right now a single library handles services and actors, but those should probably be separated down the line.

This is what the vanilla registration looks like

private static void Main()
{
    Host.CreateDefaultBuilder()
        .UseServicefabricHost()
        .RegisterStatefulService<Stateful1>("Stateful1Type")
        .Build()
        .Run();
}

and for actors

private static void Main()
{
    Host.CreateDefaultBuilder()
        .UseServicefabricHost()
        .RegisterActorService<Actor1, ActorService>()
        .Build()
        .Run();
}

Both services and actors are instanciated from DI and each has its own DI scope. I still have more validation to do, and i'm going to think some more about sharing web configuration (and listener configuratio in general) but i think the fundamentals are there

chuanboz commented 4 years ago

thanks @aL3891 , what I see we have in common:

  1. define some interface for fabric runtime to register the services factory.
  2. define one extensions UseServicefabricHost to wrap the extensions.

The differentate I see is:

  1. how we handle the child DI scope, we use IHostedService to start child scope and I customize IHost with IServiceProviderAdapter to create the child scope;
  2. Another difference is, I tried to auto discover service type based on existing service manifest and register them; both you and @OlegKarasik asks end user to explicit register it again.
amanbha commented 4 years ago

@chuanboz Thanks for following up on this, I have few questions reg. this:

  1. How would this integrate with Service Fabric middleware to handle the scenario explained here https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-reliable-services-communication-aspnetcore#a-case-of-mistaken-identity
  2. How many endpoints are added to service manifest for multiple listeners with thsi approach?
  3. Does it support Multiple replica per process or the Shared process model of SF https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-hosting-model#shared-process-model?
chuanboz commented 4 years ago

@amanbha , inline answer to your questions with my implementation. I will let @aL3891 and @OlegKarasik talk about their implementation regarding below questions.

  1. How would this integrate with Service Fabric middleware to handle the scenario explained here https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-reliable-services-communication-aspnetcore#a-case-of-mistaken-identity

there might be some minor change in the middleware, but from end user perspective they could continue to use UseServiceFabricIntegration extensions against IWebHostBuilder.

  1. How many endpoints are added to service manifest for multiple listeners with thsi approach?

It depends on user intention, for our production code we don't use the SF endpoint configuration at all. Service owner decide and manage the endpoints in their serviceManifest.xml based on their needs.

  1. Does it support Multiple replica per process or the Shared process model of SF https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-hosting-model#shared-process-model?

It does support it with my implementation since it register an factory method which creates additional WebHost per replica with fabric service creation callback.

aL3891 commented 4 years ago

@chuanboz

regariding the regarding the differences, I used IHostedService primarily to be able to have multiple service fabric registration services down the line (one for actors and one for regular services) witch i dont think is possible when using a custom IHost. Also I host does other things that you'd have to reimplement, in a custom host, specifically, launching other hosted services and dealing with lifetimes. Also, we'd really like to have multple services in a host and from what i understood you didnt support that? maybe the auto discovery from code packages can still work though,

@amanbha

How would this integrate with Service Fabric middleware to handle mistaken identity

Currently that would not be handled any differently than the standard integration, i've just focused on the host part, registering and instanciating services/actors from DI, the Service is still responsible for the listeners and services would have to implement checks so they're talking to the correct replica.

How many endpoints are added to service manifest for multiple listeners with thsi approach?

This would also be up to user code, the library just injects the service context and the service class is free to use to that as much or as little as it wants

Does it support Multiple replica per process or the Shared process model of SF

Yes, this is a core scenario for us. during the host registration the user would just keep adding Register*Service calls. The user would be responsible for making sure the service/app manifests are correct

chuanboz commented 4 years ago

@aL3891 , inline answer to your questions.

Also, we'd really like to have multple services in a host and from what i understood you didnt support that? maybe the auto discovery from code packages can still work though,

My implementation does support that and actually that's one main reason I choose it, in the host it auto discover all the service types and register the factory for it, then whenever SF decides to instantiate that service type, the callback registered in host will create new isolated DI instance to run the services, so if you have multiple service types, each one get their own DI container in isolation.

In your IHostedService case, how do you handle the case where the current service instance crashed and service fabric decide to create another service instance while keep using the same process?

Yes, this is a core scenario for us. during the host registration the user would just keep adding Register*Service calls. The user would be responsible for making sure the service/app manifests are correct

The part that user now have 2 places to define the same information and could go wong if that's out of sync worry me, and that would leads to future support volume. For the information that's already defined in ServiceManifest.xml, we shall try to leverage that than re-invent another new way of doing it.

aL3891 commented 4 years ago

My implementation does support that and actually that's one main reason I choose it, in the host it auto discover all the service types and register the factory for it

oh, then perhaps i've misunderstood something :) i was looking at UseServiceFabricRuntime where it checks and thows if multiple service types are registered, and i took that to mean there could only be one service. Also in your IHost implementation, you resolve a StatelessService instance from DI, doesnt that mean that there can be only one service implementation?

In the hosted service i have, i just register the callback, same as you do in your IHost, i'm not sure about crashes (because i think the process just dies in that case), but in that callback from the runtime, a new scope is created for each replica, so the runtime can make more than one call to that if it wants.

One problem with my current way of doign it is that the SF runtime doesnt seem to have any external way to detect that a replica is closing, so it doesnt know when to dispose the DI scope. Ideally there would be an event for this on the service baseclasses but for now i'll probably just make an interface or baseclass of my own to dispose the scope when the replica closes

The part that user now have 2 places to define the same information and could go wong if that's out of sync worry me

I agree, but that's the case with the SF libraries today as well though, if the service type name and class can be detected via the code package that's great but i dont think that's the case. The servicemanifest only has the type name and not the service class and to my knowledge the only connection between the service fabric type name and the actual service class is in the code. i could be wrong about this though. There could be a convention of course but that might be an obsticle for porting if that's the only option

OlegKarasik commented 4 years ago

@amanbha, Here are my answers inline.

How would this integrate with Service Fabric middleware to handle the scenario explained here https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-reliable-services-communication-aspnetcore#a-case-of-mistaken-identity

This is supported (more information can be found in this wiki page). Here is an example:

public static void Main(string[] args)
{
  new HostBuilder()
    /*
      Define stateful service
    */
    .DefineStatefulService(
      serviceBuilder =>
      {
        serviceBuilder
          /*
            Set name of service type defined in PackageRoot/ServiceManifest.xml
          */
          .UseServiceType("ServiceType")
          /*
            Define aspnetcore listener
          */
          .DefineAspNetCoreListener(
            listenerBuilder =>
            {
              listenerBuilder
                /*
                  Set name of the endpoint defined in PackageRoot/ServiceManifest.xml
                */
                .UseEndpoint("ServiceEndpoint")
                /*
                  Use unique service URL integration.
                  This method configures service fabric 
                  middleware with ServiceFabricIntegrationOptions.UseUniqueServiceUrl.

                  There is also a UseReverseProxyIntegration method which in turns
                  configures service fabric middleware with
                  ServiceFabricIntegrationOptions.UseReverseProxyIntegration
                */
                .UseUniqueServiceUrlIntegration()
                .ConfigureWebHost(
                  webHostBuilder => 
                  { 
                    webHostBuilder.UseStartup<Startup>(); 
                  });
              });
            })
            .Build()
            .Run();
        }

How many endpoints are added to service manifest for multiple listeners with thsi approach?

In my approach all listeners should be linked to corresponding endpoints in ServiceManifest.xml. This is done by UseEndpoint("ServiceEndpoint") method (like in the example above).

Does it support Multiple replica per process or the Shared process model of SF https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-hosting-model#shared-process-model?

Yes.

But it would be great if you can provide some testing scenarios so I could make sure everything is working as expected.

chuanboz commented 4 years ago

it's a great discussion so far, and we all got better understanding of the scenarios, requirement. I'd propose let's first work together to agree on the user experience without tie to specific implementations, and then discuss on major technical design, and eventually agree on a standard implementation that's extensible to meet everyone of us' use cases.

aL3891 commented 4 years ago

Imo the high level requirements are:

Regarding the actual asp.net integration, i'm not actually sure this neccearily needs to be tied to the GenericHost implementation as a whole. Personally i feel like that is something you'd build on top of the requirements above. Things like sharing a webhost are great and a core scenario for us, but i feel like doing that is something that the user will have to build into their solution, rather than something that can be done "under" the user services. Things like the asp.net service fabric middleware can pull the partition info out of DI (and the "low" level generic host library should make sure to register them). This also means that i dont think all the generic host stuff should neccarily be in this repo

Specifically, i think the packages/repos should look like this (actual name pending):

ServiceFabric.GenericHost.Native

Lives in service-fabric. Contains the base implementation for generic host integration as well as support for "native" SF services and generic host (IStatefulServiceReplica/IStatelessServiceInstance) and registration of multiple services in a single host. References only Microsoft.ServiceFabric.

ServiceFabric.GenericHost.ReliableServices

Lives in service-fabric-services-and-actors-dotnet. Adds support for StatelessService and StatefulService. References Microsoft.ServiceFabric.Services. Potentially also contains ways to specify multiple ICommunicationListeners via GenericHost.

ServiceFabric.GenericHost.Actors

Lives in service-fabric-services-and-actors-dotnet. Adds support for registering actors.

ServiceFabric.AspnetIntegration

Lives in service-fabric-aspnetcore. Contains the asp.net middleware as well as infrastructue to host asp.net in service fabric (in a very similar way as is done today). Contains additional things that you can use when defining listeners via the host using ServiceFabric.GenericHost.ReliableServices.

One thing i'm think about right now is what the experience should be when registering an actor service, should you register each layer separetly or should the higher level bring in the lower level. ie:

Host.CreateDefaultBuilder()
    .UseServicefabric()
    .UseServicefabricActors()
    .RegisterServicefabricActor<Actor1, ActorService>()
    .Build()
    .Run();

or

Host.CreateDefaultBuilder()
    .UseServicefabricActors() // registers everything below
    .RegisterServicefabricActor<Actor1, ActorService>()
    .Build()
    .Run();

i'm leaning towards the second option.

aL3891 commented 4 years ago

As a side note, i'd love to compare notes on bypassing the Reliable services and targeting SF directly @johncrim. I think i got it working in my project by digging though the reliable services but i've yet to find any docs on how to actually do it properly

amanbha commented 4 years ago

closed by mistake, re-opening it + @masnider as well.

masnider commented 4 years ago

Ping in this issue. Work still going on?

johncrim commented 4 years ago

Yes, for my part I think it would be premature to close this. I have been working with my own SF integration library (which runs in an IHostedService and takes a direct dependency on Microsoft.ServiceFabric (6.5.641)), but I both wanted to test it out a while and make sure it holds up. I would like to merge efforts with the other contributors on this thread, but am not quite ready and/or have not had the time to separate it in a presentable form.

OlegKarasik commented 4 years ago

@masnider

Yes, the work is still in progress. I am planning to release a new version of my library before end of the year with support for stateless service local debugging (ability to debug ASP.NET Core stateless service without running SF cluster).

aL3891 commented 4 years ago

i'm very close to deploying my version of this, https://github.com/aL3891/ServiceFabricGenericHost, to production as well, (like next week), i havent tested the actor parts extensivley yet though, but stateless and stateful services are there at least and actors should work too :) The api might might go a though a little churn as well, but its pretty close to locked

OlegKarasik commented 4 years ago

Hi All,

As I mentioned previously I have release a new version of CoherentSolutions.Extensions.Hosting.ServiceFabric with so-called local runtime which enables local debugging for simplest stateless service scenarios.

Feel free to try and share your feedback (or report issues).

Here are sample application and wiki page with implementation details.

While current version supports I would say essential things I am planning to improve local runtime to allow more scenarios in future to be debugged and verified locally.