search

umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.45k stars 2.68k forks source link

[v8] Presence of a connection string can cause boot failure #3935

Closed aaronpowell closed 5 years ago

aaronpowell commented 5 years ago

In v8 if you have a connection string set but haven't configured the DB yet/haven't run the installer, a BootFailure will be reported and result in a crash.

The problem comes from here: https://github.com/umbraco/Umbraco-CMS/blob/temp8/src/Umbraco.Core/Runtime/CoreRuntime.cs#L345-L353 (https://github.com/umbraco/Umbraco-CMS/blob/84e0d0571fdbb9b8ddf354bd8e9f088806859345/src/Umbraco.Core/Runtime/CoreRuntime.cs#L345-L353 is the current commit).

Basically, when it's trying to set the runtime state level if there is a connection string but it fails to connect an exception is thrown which results in the BootLoader never running.

I don't think this should be treated as an exception, but it should be treated as a need to run the installer. The scenarios where this could come up are using tools like Chauffeur or uSync to setup an instance from a fresh clone from git.

zpqrtbnk commented 5 years ago

(sorry for slow response time, vacations here)

So you have a connection string configured... do you also have a version configured? E.g. "8.0.0" in appSettings? Because then... if you have both a version and a connection string, this means that Umbraco should run, and not being able to connect to DB is... an error. Meaning that for some reason, the DB is not available (server down, or whatever). Not that we need to install. Otherwise - how can we differentiate between a failing DB and a "need to install"?

I think I need a better understanding of what you are trying to do, to figure out a solution.

aaronpowell commented 5 years ago

No need to apologise about response time, I know it's holidays, that's why I have time to look at this too 😁.

I see your point about handling the status appropriately.

The scenario is I'm working on how to make the install deliverable work. With v7 you could have the following workflow:

The difference between v7 and v8 is that the API can "boot" in v7 without being able to connect to the DB, but v8 it can't. Mostly the problem is that the components aren't run, meaning the dependencies are not added to the container.

Maybe it'd be better to have a ServerUnavilable states for the runtime to set so that it can boot, components can decide how they handle it (since the state flows into the Compose method). You could then have the web runtime throw up a YSOD, or set something that results in a friendly error page instead.

aaronpowell commented 5 years ago

Thinking a bit more about it, there's a few different problems/scenarios you might want to try and cover.

I'm initially testing with SQL CE, so the error I'm finding is when the DB file doesn't exist on disk (the install delierable will create it).

Now if you're using a SQL Server (or MySQL) then it'd be a little different, not connecting to the server is a bit different a problem.

Maybe a few new statuses could be in order? Or just some different logic (pseudo-code attached):

if (not_able_to_connect) {
    if (db_is_sql_ce) {
      assume file is missing
      set status as `Install`
    } else {
      assume db server is unavailable
      throw BootLoaderException
   }
}
zpqrtbnk commented 5 years ago

So... the use case is,

Interesting problem... and we may have to deal with a similar situation when Umbraco Cloud restores a site on a developer's machine. And... when we encounter the fatal error, we abort everything, and therefore we do not register anything, nor initialize components... nothing. So no code can really run.

Problem is, I kinda really want to keep treating that situation as a fatal error, by default, even when the database is SqlCe. At least I think I do. I'd like to have a way for tools such as Chauffeur to tell Umbraco that, in this very situation, it is OK to not being able to connect to the database, and then please move to the install state. Could also happen if we can connect to the database, but it is empty.

Thinking...

aaronpowell commented 5 years ago

Another scenario I thought of is for integration testing. Part of the Chauffeur tools is some integration testing tools and you can use those to bootstrap an Umbraco instance to then interact with the API as though Umbraco is completely running.

In fact, that's how I do all the tests for Chauffeur itself: https://github.com/aaronpowell/Chauffeur/blob/master/Chauffeur.Tests.Integration/UpgradeDeliverable.fs#L12-L19 and it's how Plumber does some of its tests: https://github.com/nathanwoulfe/Plumber/blob/master/Workflow.Tests/Api/GroupControllerTests.cs#L21-L35

aaronpowell commented 5 years ago

Hmm good point on Umbraco Cloud restores, I'll admit that I don't have a lot of experience with Cloud.

Why not rather than throwing an exception it just sets the status appropriately and then leave the upstream pipeline to work out if Umbraco is "dead" or just limping along.

The main problem is that the method throws an exception which results in the BootLoader being skipped. It'd be better if it just set a status appropriately (might require more states though) and leave everything to run.

It's then the more upstream pieces that can kill the process, after all, it's not until https://github.com/umbraco/Umbraco-CMS/blob/temp8/src/Umbraco.Web/UmbracoModule.cs#L515-L544 that the state is checked.

zpqrtbnk commented 5 years ago

Found how Deploy / Cloud does it - There is a PreApplicationStartMethod method which scans the version and connection string, and creates and initializes the database, if required, before Umbraco tries to boot. But I am not sure I like it a lot. I'd much rather implement a way for you to tell Umbraco, before it boots, to not die on a failing connection string but assume it must install.

Thinking...

aaronpowell commented 5 years ago

That's why I'd think about having a few more statuses and not throw exceptions, but leave the top-level Runtime to decide what to do.

aaronpowell commented 5 years ago

This is how I'd be changing it: https://github.com/umbraco/Umbraco-CMS/compare/temp8...aaronpowell:temp8-3935?expand=1

Using the bitwise OR operator you can join the enum values together so it can represent a BootFailure and more specific type of problem at the same time.

For the record - I haven't actually tested this 😝

zpqrtbnk commented 5 years ago

Currently testing a solution where ... instead of failing the boot, we would treat a missing (or empty) database as an install. So we set the runtime level to Install and run the full boot. Then, you can have a component run only on the Install state and take control of the install.

What I need to figure out is how to tell Umbraco to treat a missing database as an install (as I would like to still fail the boot, by default) - considering that the detection happens very soon in the process. Would an appSetting be acceptable? would love to avoid a PreApplicationStartMethod.

zpqrtbnk commented 5 years ago

Want to look at branch https://github.com/umbraco/Umbraco-CMS/tree/temp8-auto-install ?

Works with app settings for the moment:

<add key="Umbraco.Core.RuntimeState.InstallMissingDatabase" value="true" />
<add key="Umbraco.Core.RuntimeState.InstallEmptyDatabase" value="false" />

And then, using a composer/component similar to the code below, I have been able to create and populate the database. We need to restart when done, because the current runtime level is Install and cannot be switched to Run without restarting.

Making sense?

namespace Our.Umbraco
{
    [RuntimeLevel(MaxLevel = RuntimeLevel.Install)]
    [ComposeBefore(typeof(ICoreComposer))] // before any other non-runtime component
    public class InstallComposer : ComponentComposer<InstallComponent>
    { }

    public class InstallComponent : IComponent
    {
        private readonly IRuntimeState _runtimeState;
        private readonly IUmbracoDatabaseFactory _databaseFactory;
        private readonly IUserService _userService;
        private readonly ILogger _logger;

        public InstallWipComponent(IRuntimeState runtimeState, IUmbracoDatabaseFactory databaseFactory, IUserService userService, ILogger logger)
        {
            _runtimeState = runtimeState;
            _databaseFactory = databaseFactory;
            _userService = userService;
            _logger = logger;
        }

        public void Initialize()
        {
            // better be safe - should never happen
            if (_runtimeState.Level != RuntimeLevel.Install)
                return;

            // is it something we can handle?
            if (_runtimeState.Reason != RuntimeLevelReason.InstallEmptyDatabase 
               && _runtimeState.Reason != RuntimeLevelReason.InstallMissingDatabase)
                return;

            // then, handle it
            _logger.Info<InstalComponent>("Install!");

            if (_runtimeState.Reason == RuntimeLevelReason.InstallMissingDatabase)
            {
                // create database
                _logger.Info<InstalComponent>("Create Database!");
                using (var engine = new SqlCeEngine(_databaseFactory.ConnectionString))
                {
                    engine.CreateDatabase();
                }
                _logger.Info<InstalComponent>("Created Database!");
            }

            _logger.Info<InstalComponent>("Populate Database!");
            using (var database = _databaseFactory.CreateDatabase())
            {
                database.BeginTransaction();
                var creator = new DatabaseSchemaCreator(database, _logger);
                creator.InitializeDatabaseSchema();
                database.CompleteTransaction();
            }
            _logger.Info<InstalComponent>("Populated Database!");

            // create user
            var user = _userService.GetUserById(Constants.Security.SuperUserId);
            if (user != null)
            {
                user.Username = "stephan";
                user.IsApproved = true;
                user.IsLockedOut = false;

                // save changes
                _userService.Save(user);

                // change password
                applicationContext.Services.UserService.SavePassword(user, "abcdefghijkl");
            }

            // and restart - in order to get out of "install" mode
            _logger.Info<InstallComponent>("Restarting!");
            Current.RestartAppPool();
        }

        public void Terminate()
        { }
    }
}
aaronpowell commented 5 years ago

That's kind of the approach I was going for with the commit I linked above, but using more enum flags to depict it, rather than a separate property to explain why you're back at install.

zpqrtbnk commented 5 years ago

I get it. Thought about it. Point is, we compare levels in some places, ie Run > Upgrade. So if we do it with only one value (which indeed feels nicer) it would be something along

enum Level
{
   Install = 1000,
  InstallEmptyDatabase = 1001,
  InstallMissingDatabase = 1002,

  Upgrade = 2000,
  UpgradeVersion = 2001,

  Run = 3000
}

or something equivalent. and all tests for level == Level.Install need to be changed for level.HasFlag(Level.Install) - which also can be done. Guess I was lazy. Guess... your approach is probably cleaner, will look into it

aaronpowell commented 5 years ago

I see your point, Just wondering if using a > test is really the best way to achieve what you're after.

If it was F# I'd be suggesting a discriminated union, that'd work sweetly 😛.

Ultimately either approach works, so it comes down to what sees the most long-term understandable.

aaronpowell commented 5 years ago

@zpqrtbnk Just wanted to revisit this as I'd like to pick up working on Chauffeur for v8

zpqrtbnk commented 5 years ago

Some updates on what we've done (for eg Cloud).

You can set the two static properties RuntimeOptions.InstallMissingDatabase and RuntimeOptions.InstallEmptyDatabase to true in a PreApplicationStartMethod and then, the presence of the connection string does not cause a boot failure, but treats this as an install. Then, you need to have a composer/component run for install and do what's required (created database, etc).

This works - however, you'll have to trigger a restart after you're done, to get Umbraco to switch to Run mode.

Alternatively, you can use RuntimeOptions.OnRuntimeBoot() and RuntimeOptions.OnRuntimeEssentials() to register handlers that will run at two very precise moments of the boot process:

You probably would be interested in OnRuntimeEssentials(). Since you have a connection string, the database factory is configured (though of course it cannot connect to the database). You could use it to get an IUmbracoDatabase and create and initialize the database (as per #3866).

All this would happen before we try to figure out the runtime level, meaning that when we figure it out, the database is already there, so it's Run and everything is fine.

Will require experimenting, but should work.

aaronpowell commented 5 years ago

So I'd have to do a process restart, or at least restart the runtime? Hmm this could get a bit messy, but I'll start playing.

Assuming this stuff is all in the nightly builds now?

aaronpowell commented 5 years ago

I'm back to having some major problems with Client Dependency, I've created a PR to work around it: https://github.com/umbraco/Umbraco-CMS/pull/4534

zpqrtbnk commented 5 years ago

If you go the component way, Umbraco will boot into Install and you have to restart the app (use UmbracoApplication.Restart()) - OTOH if you use the RuntimeOptions hooks you can install your database before we detect the runtime level, and Umbraco will boot into Run directly, thus you don't need a restart.

aaronpowell commented 5 years ago

After dealing with a bunch of other runtime issues I can now confirm that using the properties on RuntimeOptions does allow you to boot Umbraco without a DB or a browser! 🎉

jtemperv commented 5 years ago

After dealing with a bunch of other runtime issues I can now confirm that using the properties on RuntimeOptions does allow you to boot Umbraco without a DB or a browser! 🎉

Hi @aaronpowell , can you share your implementation details on how you consumed the RuntimeOptions to get around your initial problem?

aaronpowell commented 5 years ago

It's been a while since I've touched Chauffeur for v8 so it may not be 100% accurate as of the current v8 release, but here's what I did:

I also have some custom components (such as https://github.com/aaronpowell/Chauffeur/blob/umbraco-v8/src/Chauffeur/Components/ChauffeurComponent.fs) which registers my own types in the container.