Adding configuration support

[sebastienros]

Fixes #2038
Fixes #1456
Fixes #825
Fixes #2051

[jtkech]

So, because now tenant names are only resolved through the shell container directories, tenants.json renamed to appsettings.tenants[.EnvironmentName].json doesn't work.

The 1st idea is to also lookup to these config files to resolve tenants. I will work on it.

Update: Hmm, or maybe by jus registering another IShellSettingsManager when using WithTenants(), this in place of the previous FileShellSettingsConfigurationProvider. I will look into it and try it.

[sebastienros]

Please don't change anything you are mixing things up. I can explain on Monday.

[jtkech]

Okay cool, i will not change anything else, just doing tests to better understand things.

I'm playing with children, sections, how to specify a setting to a specific tenant or for all. In our context, for json files we need to specify the full path, even it is at the root.

For infos, with tenants.json, each tenant was using a ShellSettingsWithTenants holding a list of features grabbed from the old settings.Configuration.

[rserj]

I think this is a really useful feature, any updates on this?

[sebastienros]

We are actively working on it. @jtkech has a PR for my PR, where he's testing many ways to do it, and my crazy requirements. We are at a design right now that will also allow for faster cold-start when using many tenants.

[sebastienros]

I did a demo during yesterday's meeting. The video should be available soon.

[jtkech]

Todo: Filter invalid tenant section without the "State" marker field.

[sebastienros]

Can you remove this library altogether, and parse the yaml manually. It should just be about reading line by line, and splitting on the name.

[jtkech]

Okay

[sebastienros]

Same here

[jtkech]

Okay

[sebastienros]

Can you also merge dev, there are conflict since Antoine improved the Tenants ui

[jtkech]

Okay no problem i will do it. Why did he do that just now ;)

I will also comment my last changes and maybe some last choices i'm not sure.

[sebastienros]

Don't worry about the yaml dependency. We can just file a bug for RC with p0 so we don't forget
I assume that's fine as we actually did remove the yaml files themselves

[jtkech]

Okay cool.

Yes, i just kept the reference to use the Yaml Deserializer.

[jtkech]

@sebastienros

• So, in my last commit, just to show, can be reverted, the suggestion is to lazily create each shell on a 1st request, as done for the pipeline, so that its specific config is effectively lazily built.

  Tried in debug mode with 20 tenants, the very 1st request is much more faster, then, on the 1st request of a given tenant, it adds an average time e.g of 80 ms depending on the recipe used.

• Doing so, listing all shells, as in the tenants index page, may need more time. But, in fact the tenants page only use ShellSettings so i updated it to use the recent _shellHost.GetAllSettings(). I also fixed a little issue where the bulk action to enable multiple tenants was not working.

• The openId module also uses ListShellContextsAsync() in some place to create a scope on each one to see if it is a Server, so nothing that we can do here.

---

For infos

• Another compromise would be to create shells with full parallelism. We can do it because to 1st get the ShellDescriptor we now use a regular scope without swapping RequestServices.

  Tried in debug mode with 20 tenants, always a base time of 500 ms, maybe for the 1st shell and / or to resolve app level services. Then an average time e.g of 35 ms per tenant creation in place of 70 ms.

• But in this case, lazily build specific configs would not be so useful and maybe we could get rid of these "local" configs by putting, after a tenant edit / setput, more values in the shared tenants.json.

[kevinchalet]

The openId module also uses ListShellContextsAsync() in some place to create a scope on each one to see if it is a Server, so nothing that we can do here.

If it's too expensive, we may remove that and list all the tenants instead of just the ones that have the server feature enabled. If we don't want to degrade the user experience too much, we may want to make a dynamic check to verify the server feature is enabled once the admin selected a tenant in the available tenants list.

[jtkech]

Okay cool, thanks.

It is in case of many tenants and if we choose to lazily create shells, i will let you know. Just tried 20 tenants with a release build in prod mode, on my machine it adds around 55 ms per tenant (blog recipe).

[Jetski5822]

Have we thought about using IOptionsSnapshot for configuration reloading?

[kevinchalet]

If you decide to support live options update, please use IOptionsMonitor instead of IOptionsSnapshot, that doesn't cache options and would basically run all the options initializers for every request (which may have a huge perf impact).

[jtkech]

@Jetski5822 @PinpointTownes

There are both implicitly implemented because we expose an IShellConfiguration which inherits of IConfiguration. That said the related source needs to be reloadable.

We use the application sources on which we add other sources as e.g a json file under App_Data for all tenants, specific ones under tenant folders ...

Just tried IOptionsMonitor by defining an class, using services.Configure<MyOptions>(_shellConfiguration) and defining some properties in the application appsettings.json, this under the OrchardCore section for all tenants and / or under a subsection for a given tenant.

Through an injected IOptionsMonitor<MyOptions> in a tenant singleton i was able to update
the json file and see the change through options.CurrentValue. It works because i updated the application json file which by default is reloadable.

It would not work by updating one of our additional sources which are not reloadable by default. But you can override source(s) through app level service(s) which will participate to build the config. E.g you can easily define that the App_Data/appsettings.json source will use reloadOnChange = true.

[Jetski5822]

Heads up - I made a change to the GraphQL stuff and introduced some configuration in to there. So we need to make that change in the PR too

[kevinchalet]

the json file and see the change through options.CurrentValue. It works because i updated the application json file which by default is reloadable.

👍

[jtkech]

@Jetski5822 Okay, it's noted

[jtkech]

@PinpointTownes

We opted to keep the lazy creating of shells, and now only the background service uses ListShellContextsAsync() which now doesn't create a released shell (released or not yet created).

I already updated your driver code to use instead _shellHost.GetAllSettings() but right now we still call on all settings _shellHost.GetScopeAsync(shellSettings) so all the shells are still created in one pass.

So, would be good to make, as you said, a dynamic check to verify the server feature.

Can be done in another PR as we will merge this one very soon.