Orleans.TestingHost support test clusters without legacy configuration #3878

ReubenBond · 2018-01-17T06:43:38Z

This should allow us to more easily migrate configuration over to non-legacy classes bit-by-bit as well as hopefully allow us to run our tests on .NET Core (i.e, by removing the reliance on creating AppDomains)

cc @jdom, who performed the initial work

jdom · 2018-01-18T05:01:01Z

test/Tester/GrainActivatorTests.cs

            }

-            private class TestSiloBuilderFactory : ISiloBuilderFactory
+            private class TestSiloBuilderFactory : ISiloBuilderConfigurator


nit: you could rename to TestSiloBuilderConfigurator

ah, yeah, I renamed some, but I'm sure I missed many

jdom · 2018-01-18T05:07:53Z

  public virtual TestCluster CreateTestCluster()
  {
      return new TestCluster();
  }

Is this fine or should it use new TestClusterBuilder().Build() instead? Just asking, because I don't remember if the defaults are useable

Refers to: test/TestExtensions/HostedTestClusterBase.cs:62 in 2db2741. [](commit_id = 2db27411cdf8c8770e2b93c389598b3386a1b7a8, deletion_comment = False)

ReubenBond · 2018-01-18T05:10:05Z

The latter, will fix. BTW, your comment isn't anchored to the code.

jdom · 2018-01-18T05:10:33Z

src/Orleans.TestingHost/Grains/ITestClusterManagementGrain.cs

+{
+    public interface ITestClusterManagementGrain : IGrainWithGuidKey
+    {
+        Task<T> GetService<T>();


I wouldn't add this grain here (in the package that we ship) since almost no service would be serializable. The only one I can think of is the legacy config objects, so wouldn't pollute the surface with this.

Alright, should I make a more specific legacy-themed grain to get config objects or try to retrieve them some other way?

If it was just to get the gateway endpoint, you probably don't even need that, you can derive it from the options, or even ask the client

jdom · 2018-01-18T05:13:29Z

test/Tester/ClientConnectionTests/InvalidPreambleConnectionTests.cs

        {
-            var gwEndpoint = this.HostedCluster.Primary.NodeConfiguration.ProxyGatewayEndpoint;
+            var clusterConfig = await this.Client.GetGrain<ITestClusterManagementGrain>(Guid.Empty).GetService<ClusterConfiguration>();
+            var gwEndpoint = clusterConfig.Overrides[Silo.PrimarySiloName].ProxyGatewayEndpoint;


There are other ways to get the proxy endpoint without the need to resort to serializing the config back from the silo (and in fact that would break if you stop using the legacy config).
If you hold on to the TestClusterOptions, you can peek in there. Otherwise there's also the opportunity to get the gateway list provider options from the client itself without asking the silo for the config

jdom · 2018-01-18T05:17:54Z

src/Orleans.TestingHost/SiloHandle.cs

-        /// <summary>Get the proxy address of the silo</summary>
-        public SiloAddress ProxyAddress => SiloAddress.New(this.NodeConfiguration.ProxyGatewayEndpoint, 0);
+        ///// <summary>Get the proxy address of the silo</summary>
+        public SiloAddress GatewayAddress { get; set; }


the default implementation I think could just look for this information in the TestClusterOptions if it uses test cluster membership.

This way other subclasses do not have to rely on extended RPC for it to work (such as MarshalByRefObject as we have in the AppDomainSiloHandle implementation)

I wanted this so that I could query it for some tests. It seems right that if SiloAddress is there, then GatewayAddress should also be there.

On the other hand, I'm not confident about the overall class structure for this change (i.e, could things be separated more cleanly?)

Sure, it's fine, I don't really mind if we keep this way for now. The reality is that I initially was retrieving it from the container in that way before I did all the work to move to strongly typed config. Today you can just as well get it directly from the configuration.
But as I said, it's fine... Both AppDomain and in-domain strategies can RPC from the TestCluster... If we have other approaches (like out of process or containers) we can refactor it so that the silo handle infers it from the config instead of asking the remote host.

jdom · 2018-01-18T05:23:15Z

src/Orleans.TestingHost/TestClusterHostFactory.cs

+                    var clientConfiguration = GetOrCreateClientConfiguration(services, configuration);
+
+                    // Test is running inside debugger - Make timeout ~= infinite
+                    clientConfiguration.ResponseTimeout = TimeSpan.FromMilliseconds(1000000);


Note: Once we move this to strongly typed options, we can get rid of the dependency to the legacy config objects in the TestClusterHostFactory

Moved this to LegacyTestClusterConfiguration

The reality is that this is useful in the non-legacy case too going forward. I would keep this by default, it's just that when you eventually move this setting out to a strongly typed option, then you can stop relying on the legacy configuration entirely

ReubenBond · 2018-01-18T09:34:30Z

I'm thinking of modifying TestClusterPerTest.CreateTestCluster (and similar base class methods) so that instead of returning TestCluster, it consumes a TestClusterBuilder. That way, the base class has a chance to modify the configuration and, eg, insert the defaults loaded from OrleansTestSecrets.json / env vars - the ones loaded in TestDefaultConfiguration.

Something like ~void ConfigureTestCluster(TestClusterBuilder builder). We could also modify LegacyTestClusterBuilder so that it's no longer a subclass of TestClusterBuilder, but instead offers an extension method to let the consumer configure the legacy configuration, eg:

void ConfigureTestCluster(TestClusterBuilder builder)
{
  // probably this would be done in the base class.
  builder.AddTestEnvironmentConfiguration();

  // Set some legacy configuration options.
  builder.ConfigureLegacyConfiguration(cfg =>
  {
    cfg.ClusterConfiguration.SomeProperty = 3;

    cfg.ClientConfiguration.SomeProperty = 3
  });
}

What do you think?

EDIT: the latest commit implements that change to ConfigureTestCluster, but not "AddTestEnvironmentConfiguration"

jdom · 2018-01-18T18:04:26Z

I do like the change to use extension methods on the 1 and and only test cluster builder instead of inheritance 👍 👍 in fact now that I remember, I thought of avoiding inheritance too, but never got to it.

xiazen · 2018-01-18T21:24:03Z

src/Orleans.TestingHost/AppDomainSiloHost.cs

@@ -56,13 +50,6 @@ public void Shutdown()
        {
            this.host.StopAsync().GetAwaiter().GetResult();
        }
-
-        private void InitializeTestHooksSystemTarget()


where is this part of logic moved to ?

Moved to TestClusterHostFactory

jdom · 2018-01-19T00:28:40Z

src/Orleans.TestingHost/LegacyTestClusterConfiguration.cs

+                {
+                    var siloName = options.GetSiloSpecificOptions(i).SiloName;
+
+                    this.ClusterConfiguration.GetOrCreateNodeConfigurationForSilo(siloName);


Why go back to defining these? can't you avoid it and only do so specifically if a particular test was incorrectly relying on it?
I'd like to prevent re-adding this at the infrastructure level if it was just for an edge case test

I was hoping to avoid it, too, but it's used in non-test code in a non-trivial way which was making tests fail. I had a fix for the one test usage I sent to you, but I reverted it and went with this approach.

orleans/src/Orleans.Runtime/Streams/QueueBalancer/StaticClusterDeploymentConfiguration.cs

Line 26 in 9bd486b

return _clusterConfiguration.Overrides.Keys.ToList();

It will [have to] be addressed when we move away from ClusterConfiguration internally

jdom · 2018-01-19T07:17:06Z

test/TestInternalGrains/PersistenceTestGrains.cs

+    {
+        public Task<Guid> GetServiceId()
+        {
+            return Task.FromResult(this.ServiceProvider.GetRequiredService<GlobalConfiguration>().ServiceId);


Why not get this from the strongly typed option (instead of the legacy config) which is the authoritative source of truth?

ReubenBond · 2018-01-19T07:54:07Z

@dotnet-bot test bvt

xiazen · 2018-01-19T18:21:10Z

can we have some write up about new interfaces and major changes, so it is easier to review?

ReubenBond · 2018-01-19T20:18:42Z

@xiazen the gist of it is this:

TestCluster creation now has a builder pattern via TestClusterBuilder
Legacy configuration was removed from TestCluster and into separate classes which can be added to the TestClusterBuilder by using the ConfigureLegacyConfiguration extension method
Instead of serializing configuration objects over an AppDomain boundary, everything is configured via the IConfiguration system
The hope is to be able to swap the AppDomainSiloHandle implementation for different implementations (eg, in-proc, new proc, container, whatever) later, since it's easier to serialize the IConfigurationSource objects which are used to create configuration.

jdom · 2018-01-19T21:32:36Z

src/Orleans.TestingHost/TestClusterHostFactory.cs

-            if (useTestClusterMemebership)
+            if (useTestClusterMemebership
+                && services.All(svc => svc.ServiceType != typeof(IMembershipTable))
+                && services.All(svc => svc.ServiceType != typeof(IMembershipOracle)))


nit: could avoid double enumeration

I'll remove this and make sure UseTestClusterMembership is correctly set instead

ReubenBond · 2018-01-22T22:37:19Z

https://ci.dot.net/job/dotnet_orleans/job/master/job/functional_prtest/1013/

14:12:09 No executable found matching command "dotnet-xunit"
14:12:09     + CategoryInfo          : NotSpecified: (No executable f... "dotnet-xunit" 
14:12:09    :String) [], RemoteException
14:12:09     + FullyQualifiedErrorId : NativeCommandError
14:12:09     + PSComputerName        : localhost

jdom · 2018-01-23T00:14:38Z

@dotnet-bot test this please

ReubenBond · 2018-01-23T10:37:41Z

Please review but don't merge (I'm sure there will be changes requested anyway) - I need to ensure we have a green run in VSO first.

jdom · 2018-01-23T18:34:37Z

yup, sure, pay special attention to Azure tests, since they don't run here, as using different membership providers are directly impacted with how this new test infrastructure works. Should be trivial to resolve if there are issues though

jdom · 2018-01-23T18:46:46Z

src/Orleans.TestingHost/TestCluster.cs

@@ -284,42 +237,39 @@ public static TimeSpan GetLivenessStabilizationTime(GlobalConfiguration global,
        /// <summary>
        /// Start an additional silo, so that it joins the existing cluster.
        /// </summary>
-        /// <returns>SiloHandle for the newly started silo.</returns>
+        /// <returns>SiloHandle2 for the newly started silo.</returns>


Search for old usages of SiloHandle2 and replace with SiloHandle

jdom · 2018-01-23T18:47:07Z

src/Orleans.TestingHost/TestCluster.cs

+        ///// <summary>
+        ///// Restart a previously stopped.
+        ///// </summary>
+        ///// <param name="siloName">Silo to be restarted.</param>


uncomment XML comments

jdom · 2018-01-23T18:54:52Z

LGTM. Ship it! :)

xiazen

LGTM except two minor comments

xiazen · 2018-01-18T22:31:08Z

src/Orleans.TestingHost/ClientBuilderConfiguratorBase.cs

+        /// <summary>
+        /// Configures the client builder.
+        /// </summary>
+        public abstract void Configure(IConfiguration configuration, IClientBuilder clientBuilder);


Why do we have an extra ClientBuilderConfiguratorBase.Configure method here on top of IClientBuilderConfigurator.Configure? if they are supposed to do different things, maybe name them different

The explicit IClientBuilderConfigurator implementation does some work before calling the abstract method. I could collapse the class hierarchy instead

I might be dumb here, but why cannot we merge two Configure method?

we can, it just means moving this code into any consumer. The idea was that this class would be reusable, but ... YAGNI

xiazen · 2018-01-23T19:01:23Z

src/Orleans.Core/Configuration/Legacy/LegacyConfigurationExtensions.cs

+                }
+                catch (FileNotFoundException)
+                {
+                    configuration = new ClientConfiguration();


I'm not sure swallowing FileNotFoundException is a good idea, didn't this change original behavior? And for users who wanted to do StandardLoad from a config file, it hided exception from them which make it harder for them to notice that they made a mistake somewhere.

Not really, this is for the path we're users didn't explicitly pass a config object. If they do want, then they'll get the exception in their code when they try to load the config.
Btw, this code path will be removed as soon as the runtime itself stops requiring the legacy config in the first place

jdom · 2018-01-24T03:24:55Z

test/TesterAzureUtils/Persistence/PersistenceGrainTests_AzureTableStore.cs

@@ -41,7 +41,7 @@ protected override void ConfigureTestCluster(TestClusterBuilder builder)
            {
                Guid serviceId = Guid.NewGuid();
                builder.Options.InitialSilosCount = 4;
-
+                builder.Options.UseTestClusterMembership = false;


Aren't other external membership tests (non azure) affected by the same issue. They wouldn't show up as we don't run them in CI

jdom · 2018-01-24T06:49:16Z

src/Orleans.TestingHost/Orleans.TestingHost.csproj

+  <ItemGroup>
+    <Compile Remove="Grains\**" />
+    <EmbeddedResource Remove="Grains\**" />
+    <None Remove="Grains\**" />


What's all this?

Oh, I thought I fixed that - my bad.

jdom · 2018-01-24T16:46:20Z

There are conflicts now. If you resolve them and tests are passing, I'll merge it asap

ReubenBond · 2018-01-25T03:41:04Z

@dotnet-bot test functional

ReubenBond · 2018-01-29T10:28:38Z

@dotnet-bot test bvt

ReubenBond · 2018-01-29T10:29:05Z

I'm happy for us to merge this now

jdom · 2018-01-29T16:32:47Z

I was about to on Friday, but some multi cluster tests in vso are failing because of this. Should I merge anyway or wait until they are fixed?

sergeybykov · 2018-01-29T16:34:46Z

There were other issues with the multi-cluster tests. I think there's no need to block this waiting for them to be investigated.

jdom · 2018-01-29T16:43:11Z

Oh, OK then. I'll merge as soon as the conflicts are resokved

ReubenBond · 2018-01-29T20:59:37Z

resolved

jdom · 2018-01-29T21:21:31Z

oh, too slow :(
Was waiting for tests to validate the rebase

xiazen · 2018-01-29T21:24:28Z

:) yeah. you are late for the party ;) . I verified that it passed in vso with Reuben. So I merged it

ReubenBond · 2018-01-29T21:52:30Z

The tests passed in .NET CI, so it's all good. I'm just glad this is merged.

jdom · 2018-01-29T21:56:39Z

Yeah, sorry for the delay, I was thinking that you were planning to fix the multi-cluster tests... I was running tests in VSO for the entire week last week, waiting for the moment they passed so I could merge :)

ReubenBond force-pushed the test-cluster branch from 9ac99b4 to 2db2741 Compare January 18, 2018 04:58

jdom reviewed Jan 18, 2018

View reviewed changes

xiazen reviewed Jan 18, 2018

View reviewed changes

sergeybykov mentioned this pull request Jan 19, 2018

Microsoft.Orleans.TestingHost 2.0.0 support for .NetCore #3032

Closed

jdom reviewed Jan 19, 2018

View reviewed changes

ReubenBond changed the title ~~[WIP] Orleans.TestingHost support test clusters without legacy configuration~~ Orleans.TestingHost support test clusters without legacy configuration Jan 22, 2018

jdom reviewed Jan 23, 2018

View reviewed changes

jdom approved these changes Jan 23, 2018

View reviewed changes

xiazen reviewed Jan 23, 2018

View reviewed changes

jdom reviewed Jan 24, 2018

View reviewed changes

ReubenBond force-pushed the test-cluster branch from 2473d77 to 6a3c469 Compare January 25, 2018 01:49

ReubenBond force-pushed the test-cluster branch from 6a3c469 to 5419560 Compare January 29, 2018 05:05

Orleans.TestingHost support test clusters without legacy configuration

61eb1c4

ReubenBond force-pushed the test-cluster branch from 5419560 to 61eb1c4 Compare January 29, 2018 20:59

xiazen merged commit f7c3a73 into dotnet:master Jan 29, 2018

ReubenBond deleted the test-cluster branch January 29, 2018 21:51

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Orleans.TestingHost support test clusters without legacy configuration #3878

Orleans.TestingHost support test clusters without legacy configuration #3878

Conversation

ReubenBond commented Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdom commented Jan 18, 2018 • edited Loading

ReubenBond commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond commented Jan 18, 2018 • edited Loading

jdom commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond commented Jan 19, 2018

xiazen commented Jan 19, 2018

ReubenBond commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond commented Jan 22, 2018

jdom commented Jan 23, 2018

ReubenBond commented Jan 23, 2018

jdom commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdom commented Jan 23, 2018

xiazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdom commented Jan 24, 2018

ReubenBond commented Jan 25, 2018

ReubenBond commented Jan 29, 2018

ReubenBond commented Jan 29, 2018

jdom commented Jan 29, 2018

sergeybykov commented Jan 29, 2018

jdom commented Jan 29, 2018

ReubenBond commented Jan 29, 2018

jdom commented Jan 29, 2018

xiazen commented Jan 29, 2018

ReubenBond commented Jan 29, 2018 • edited Loading

jdom commented Jan 29, 2018

ReubenBond commented Jan 17, 2018 •

edited

Loading

jdom commented Jan 18, 2018 •

edited

Loading

ReubenBond commented Jan 18, 2018 •

edited

Loading

ReubenBond commented Jan 29, 2018 •

edited

Loading