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

Adding configuration support #2824

Merged
merged 29 commits into from Jan 25, 2019

Conversation

Projects
None yet
5 participants
@sebastienros
Copy link
Member

sebastienros commented Dec 8, 2018

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

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Dec 8, 2018

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

This comment has been minimized.

Copy link
Member Author

sebastienros commented Dec 9, 2018

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

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Dec 9, 2018

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

This comment has been minimized.

Copy link
Contributor

rserj commented Dec 21, 2018

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

@sebastienros

This comment has been minimized.

Copy link
Member Author

sebastienros commented Dec 21, 2018

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.

@jtkech jtkech referenced this pull request Dec 22, 2018

Closed

Jtkech/configuration #2865

@jtkech jtkech referenced this pull request Dec 30, 2018

Merged

Work on Configuration. #2959

@sebastienros sebastienros force-pushed the sebros/configuration branch from e5a6083 to 549b581 Jan 7, 2019

@sebastienros

This comment has been minimized.

Copy link
Member Author

sebastienros commented Jan 9, 2019

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

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Jan 10, 2019

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

jtkech added some commits Jan 10, 2019

@@ -40,6 +40,7 @@
<PackageManagement Include="xunit.analyzers" Version="0.10.0" />
<PackageManagement Include="xunit" Version="2.4.0" />
<PackageManagement Include="xunit.runner.visualstudio" Version="2.4.0" />
<PackageManagement Include="YamlDotNet" Version="5.3.0" />

This comment has been minimized.

@sebastienros

sebastienros Jan 15, 2019

Author Member

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.

This comment has been minimized.

@jtkech
@@ -23,6 +23,7 @@
<PackageReference Include="Microsoft.Extensions.Localization" />
<PackageReference Include="Microsoft.Extensions.Logging" />
<PackageReference Include="NodaTime" />
<PackageReference Include="YamlDotNet" />

This comment has been minimized.

@sebastienros

sebastienros Jan 15, 2019

Author Member

Same here

This comment has been minimized.

@jtkech

jtkech added some commits Jan 16, 2019

@sebastienros

This comment has been minimized.

Copy link
Member Author

sebastienros commented Jan 16, 2019

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

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Jan 16, 2019

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.

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Jan 17, 2019

First comments on my last changes.

  • So, we use the app config on which we 1st add by default App_Data/appsettings.{env}.json. Now through the DI an app level service can override the config sources added at this point.

  • Then we reapply at the end env variables and cmd line arguments to build the base config.

  • Then we use a "local" / tenant specific config source, by default {tenantFolder}/appsettings.json which is lazily built. Here also an app level service can override the tenant specific config sources.

  • When saving a tenant, config values (different from the base config) are saved by default in {Sites}/{tenant}/appsettings.json. Now we 1st read the file so that we don't overwrite other configs which may have been added manually. And because of the above, another source can be updated.

  • When saving a tenant, we do the same for data which are immediately required to start a tenant, url host and prefix and tenant state, but we save them by default in a shared App_Data/tenants.json so that we can use only one source when loading settings. Now we can also override this source.

Todo: Remove the dependency on yalm.

@sebastienros

This comment has been minimized.

Copy link
Member Author

sebastienros commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Member

jtkech commented Jan 17, 2019

Okay cool.

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

@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Jan 20, 2019

@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.

@PinpointTownes

This comment has been minimized.

Copy link
Contributor

PinpointTownes commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Member

jtkech commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Member

Jetski5822 commented Jan 21, 2019

Have we thought about using IOptionsSnapshot for configuration reloading?

@PinpointTownes

This comment has been minimized.

Copy link
Contributor

PinpointTownes commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Member

jtkech commented Jan 22, 2019

@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

This comment has been minimized.

Copy link
Member

Jetski5822 commented Jan 22, 2019

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

@PinpointTownes

This comment has been minimized.

Copy link
Contributor

PinpointTownes commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Member

jtkech commented Jan 23, 2019

@Jetski5822 Okay, it's noted

jtkech added some commits Jan 23, 2019

Fixes bg service, limit the usage of "ListShellContexts" that now don…
…'t create released shells.

Always create the default tenant on statup, other tenants are still lazily created.
@jtkech

This comment has been minimized.

Copy link
Member

jtkech commented Jan 25, 2019

@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.

@jtkech jtkech merged commit d918b13 into dev Jan 25, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@sebastienros sebastienros removed the notready label Jan 25, 2019

@sebastienros sebastienros deleted the sebros/configuration branch Jan 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment