Skip to content
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

Ensure to revert the tenant to Uninitialized if the setup fails. #12361

Merged
merged 4 commits into from
Sep 19, 2022

Conversation

jtkech
Copy link
Member

@jtkech jtkech commented Sep 9, 2022

It works even if we use AddDatabaseShellsConfiguration(). The tenant state is reverted to Uninitialized not kept to Initializing, so that when we refresh the page we render again the setup form.

For now we still need to manually delete the only Document table that was created with some well knowed documents as the ShellDescriptor which is the one that blocks a new setup, but without having to re-start the application to retry, unless we have to fix some code.

Next step, maybe in a separate PR, the ability to delete this Document table or only the ShellDescriptor, for example if only the Document table exists meaning that a setup failed (not 100% sure), maybe just before a new setup or when we will be able to remove an Uninitialized tenant (unless the Default one).

@MikeAlhayek
Copy link
Member

MikeAlhayek commented Sep 9, 2022

@jtkech how about we also add error with info so that the user knows what needs to happen?

Maybe something like this?

context.Errors.Add(String.Empty, S["The site setup failed! In order to try the setup again, please delete any created table along with tenant related files."]);

@jtkech
Copy link
Member Author

jtkech commented Sep 9, 2022

We can't use context.Errors because here as before after using the initialState we throw so the exception will be logged but we don't return back to the view (maybe we should), will investigate more tomorrow.

@jtkech
Copy link
Member Author

jtkech commented Sep 11, 2022

I did some tests when a tenant setup fails, while thinking about the Tenant Removal.

  1. If it fails before the recipes, no table was created => Okay

  2. If it fails on a migration, the _Document and _identifiers tables was created.

  3. If it fails after on setup event handlers, e.g. to create the Admin user, most of tables was created.


We could think about cleaning these tables before retrying a setup, but for now I opted to be able to do this cleanup when we will be able to remove a tenant, so not applied to the Default tenant.

For testing I was able to do this cleanup, in the Removal branch we already have a throwOnError that I set to false to not break if it can't remove a table, in fact it was only throwing on removing foreign keys.

For some reasons it was not working if in the Uninitialized state that we ensure here, I needed to enforce it to Disabled to be still allowed to remove the tenant. So maybe an additional state, a kind of Incomplete Setup if it failed on 2. or 3.. But I think I can overcome this to not complicate things in this PR.

Hmm will see, maybe this additional state will be still useful, when removing a tenant we could assume that all info to connect to the database are there, and that we could try to re-play the migrations.


Updated: Okay, no need for an additional state, will be implemented in Tenant Removal by checking if a tenant has a valid db connection and at least the Document table, in that case when removing the tenant, even if Uninitialized we will cleanup the tables that may have been created on a setup that failed.

@jtkech jtkech merged commit 92daf3e into main Sep 19, 2022
@jtkech jtkech deleted the jtkech/tenant-setup branch September 19, 2022 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants