Tenant removal (Lombiq Technologies: OCORE-96)

[jtkech]

Fixes #1212, related to #8471
Fixes #11989.

• Remove button in Tenants admin.
• Removing event handlers (tenant IModularTenantEvents and new host handlers).
• Database handler for sqlite => delete the .db file (we clear the pool to unlock the file).
• Database => Drop tables by prefix (from a central place by replaying tenant migrations).
• Uninstall => Drop index table in modules (indirectly done by the above).
• ShellsSettingsSources and ShellConfigurationSources.
• DatabaseShellsSettingsSources and DatabaseShellConfigurationSources.
• BlobShellsSettingsSources and BlobShellConfigurationSources.
• Sitemaps-> Delete cache files under wwwroot (now in separated folder per tenant).
• Media -> delete tenant folder and cache files under wwwroot (idem as above).
• Azure Media storage => Remove Container config option, remove cache files.
• Amazone Media storage => Remove Bucket config option, remove cache files.
• Lucene => delete folder (the folder is unlocked when the tenant is released).
• Keep in sync removed tenants in a distributed environment.
• Allow to delete an Uninitialized tenant.
• Add Disable, Enable and Remove Web API.
• Allow to delete a tenant that failed on setup/migration.

[Skrypt]

How does it work with multiple Azure instances?

[jtkech]

With the OrchardCore.Tenants.Distributed feature, need to be updated too, then it will detect that the shell settings collection has changed, as we already do for newly created tenants. Only one instance will do the removal job.

[Skrypt]

We may need to have something like a "DROP Database" and "DROP Table" dialect in YesSql. Which would result in a different operation for SQLite of course.

[jtkech]

Yes maybe, will see, I may need your help in this area ;)

[Skrypt]

As for removing the proper tables, maybe the migrations themselves should have their own Remove() method per module. This method could evolve over time if needed. Or, keep a dictionary of tables used by each module in the MigrationData.

[hishamco]

I was on waiting state after my comment here ;)

It's great to see the PR started again

[jtkech]

Just for info.

So for now only first testing codes where I tried to apply the ideas of my first comment.

• Use the ShellDescriptor to retrieve all installed features (even if some are no more enabled).

• Use an isolated shell where all the above installed features are enabled, then call all migrations Create[Async]() methods, but by using a custom ISchemaBuilder that only enlists the tables names without any transaction, connection, and sql commands.

[jtkech]

[jtkech]

So, for a given Tenant we can replay the data migrations (of all installed features) but by using a custom SchemaBuilder that enlists tables definitions without using any connection / transaction. Then we can use it under an isolated connection / transaction to remove all tables of this tenant.

For now only tried with Sql Server.

[jtkech]

For info, it also works with Sqlite where at the end we delete the whole database file, but because we use connection pooling, we need to clear the connection pool to unlock the file before removing it.

if (shellSettings["DatabaseProvider"] == "Sqlite" &&
    connection is SqliteConnection sqliteConnection)
{
    // Clear the pool to unlock the file and remove it.
    SqliteConnection.ClearPool(sqliteConnection);
    File.Delete(sqliteConnection.DataSource);
}

[jtkech]

For info about removing Lucene index files.

There is a file lock per index which is created when the index is updated (e.g. on publishing a related indexed item). These file locks are released when the tenant is released, which is the case when we want to reload it, or when we disable it.

For now I opted to disable a tenant before removing it, so that the tenant is no more served and we can still create internally an isolated shell context on it, e.g. with a minimum set of features or all installed features, this depending on the stuff to do.

So here if the tenant is disabled (so released), the Lucene index files are no more locked, so we can just delete them e.g. when we delete the tenant folder.

[jtkech]

@MikeAlhayek

If you try to remove a tenant where the database connection failed for that tenant, you'll get an exception. I don't know that right path here. Should we remove everything else and should a warning that the database was not removed? Should the entire process fail which I don't think is possible. Maybe we should remove everything accessible .

I will check asap this part.

About your review, I will check again each suggestion, already did some of them.

[jtkech]

If you try to remove a tenant where the database connection failed for that tenant, you'll get an exception.

Yes, as it would do if we try to Enable this Disabled tenant. But in this context I will see if we can capture this exception and have an error message instead, as we do for other exceptions while removing.

Should the entire process fail which I don't think is possible.

Currently an exception is thrown but nothing is removed.

Maybe we should remove everything accessible .

Hmm, unless it is an indinvidual file for sqlite, if the database is the harder part to remove maybe better to let the tenant as is, allowing to fix the connection and retry it. Hmm, if not using sqlite, most of the time tenants are sharing the same database (but yes not necessarily), if so normally the connection is valid.

[jtkech]

@MikeAlhayek

Okay, so in that case we capture the exception and display an error message instead (still log the error).

[jtkech]

@MikeAlhayek

I did some last changes related to your review, maybe some remaining things but not related to this PR, being just some copy paste from how it is done everywhere else.

Let me know if I missed something, otherwise if you can approve.

[MikeAlhayek]

@jtkech please make sure the minor agreed on changes are done. But I tested it and approved it

[jtkech]

please make sure the minor agreed on changes are done

Yes the requested changes by @Piedone (only typos) are still red and I lost his approval, but I committed his suggestions, will see.

But I tested it and approved it

Thanks

[jtkech]

@Piedone Okay, I updated the log message as you requested, the PR needs to be approved again ;)

[MikeAlhayek]

@jtkech I upgrade an instance that was using 1.5 to 1.6 preview. Media are stored on Azure Blob Storage. After deployment, none one the images are loading correctly. The images are in the Blob storage by the website show as the images do not exists. Could the change in this PR broke the Media Azure services?

[jtkech]

From memory in this module we only add RemovingAsync() in MediaBlobContainerTenantEvents

Then to not have nested blocks I used at the beginning of RemovingAsync() an opposite test like this

        // Only remove container if options are valid.
        if (!_options.RemoveContainer ||
            String.IsNullOrEmpty(_options.ConnectionString) ||
            String.IsNullOrEmpty(_options.ContainerName))
        {
            return;
        }

Then did the same in ActivatingAsync()

        // Only create container if options are valid.
        if (_shellSettings.State == Environment.Shell.Models.TenantState.Uninitialized ||
            String.IsNullOrEmpty(_options.ConnectionString) ||
            String.IsNullOrEmpty(_options.ContainerName) ||
            !_options.CreateContainer
            )
        {
            return;
        }

But the logic seems to be still okay, hmm but seems to not be related if you say that the blob contains the images. Otherwise nothing else was changed in this module as I can remember.

[MikeAlhayek]

@jtkech not sure what could it be. I don't see anything in the logs. Any idea why wouldn't this connect to the blob storage?

[jtkech]

Will see, maybe only a little missing thing.

First we also change the path where files are cached, in place of all under is-cache, each tenant has its own folder e.g. Default/is-cache

[jtkech]

Can you connect to your blob outside of OC with the same connection string?