db/migrate/20160412173417_update_ci_commit.rb requires downtime or manual steps, this needs to be automated

The migration db/migrate/20160412173417_update_ci_commit.rb was added in commit 872bbb9f. This migration requires users to either take their GitLab instance offline or re-run the migration after the code has been deployed.

Both solutions are not ideal. Downtime is a no-go, certainly for 8.8 (if we're not strict about it now we never will be) and re-running migrations is something that can't be done easily. We should automate this so that both we (as in GitLab itself) and other users/customers can upgrade to 8.8 without having to run any extra manual steps.

This is currently preventing me from releasing 8.8.0 RC1 (and any further 8.8 releases).

cc @ayufan @dzaporozhets cc @marin (since this may at some point lead to changes in omnibus)

@ayufan can we change migration or logic to avoid second run here?

@dzaporozhets It's hard to do it, because otherwise we will have some numbers of objects left in inconsistent state. The ones between running migration and between restarting Unicorns.

@ayufan What exactly is the data that can be left in an inconsistent state? Judging by the migration it seems to modify ci_builds, is it perhaps possible to pause running builds during the deployment?

@yorickpeterse

Thanks you just show me that this will not even work for second execution :)

This is the scenario that will be broken (and solved by double-run):

1. We run migration, and calculate status of the ci_commits
2. Someone pushes a commits, a new ci_commit is created with status=nil
3. Build is run and succeeds, the ci_builds is updated
4. We restart Unicorn, the ci_commits status is still nil

This scenario will be broken (and solved by double-run, but without the status=nil):

1. Build is running
2. We run migration, update ci_commit.status=running
3. Build has finished
4. We restart Unicorn
5. The is still ci_commits.status=running, even when ci_builds.status=success (the old GitLab did not update the status)
6. We will eventually update ci_commits.status, but only when the ci_commit gets touched, which will happen if a new build is run for that commit

Given that we should unconditionally (currently the condition is status=nil) update all ci_commits.status after the GitLab app restart.

Second option is do not care, but we will be left with some ci_commits that have status inconsistent with builds.

Stopping a sidekiq processing would help with 1. Stopping a build processing (access to /ci/api) would help with second, but it's not 100% true, because some users could also modify the state of builds from GitLab interface.

@ayufan Is it important that the build status of ci_commits is up to date? Is this displayed anywhere or used in queries and such?

Alternatively, is it possible to correct these statuses on the fly, or will that slow things down a lot?

@yorickpeterse yes, it's used in queries, it's used to show a status of latest branch

They are calculated after a change of related objects. Trying to calculate "everything" on fly will kill the performance :)

@ayufan and I had a call about this, and we couldn't really come up with a solution that doesn't involve any manual steps. Automatically re-running certain migrations after upgrading to a new version may break in the future (e.g. a post-deploy migration depends on columns removed by a pre-deploy migration). For a lack of a better solution we think the following is the best:

Provide a Rake task for every minor version of GitLab that re-runs a list of migrations, for example something like rake gitlab:post_deploy. This Rake task just uses the migrations directly and is updated for every minor version. This task would look something like:

task :post_deploy do
  SomeMigration.new.up # or whatever we'd need to do to re-run it
end

This setup however doesn't work if you upgrade from say 7.x to 9.x as you'd miss out on the migrations to re-run for 8.x. This is not ideal but still better than telling people to run various SQL queries/Ruby scripts manually.

Alternatively we can still decide having an inconsistent state is good enough. @ayufan mentioned it doesn't really do any harm in this case so maybe this is better, though I foresee future migrations where we can't have an inconsistent state (but perhaps we should solve the problem then).

@dzaporozhets @DouweM @rspeicher: any thoughts on the above?

@yorickpeterse Maybe it would be just good to say that it will work only if you are upgrading from 8.x to 8.(x+1), but it'll not work if you are upgrading for further version (8.x to 8.(x+2)). Then you would need to do it offline.

Status changed to closed

I gave this some thought and we'll go with the following:

Since @ayufan said there's no harm in leaving some ci commits in an inconsistent state we'll just tell people they can deploy online. If we ever need to correct the state we'll take care of it then. Right now we don't have any proper tools for post-deploy migrations and build those will delay 8.8.x releases.

@ayufan: For the blog post, could you add a snippet that tells people about this while providing some Ruby snippet they can run if they absolutely want to guarantee a proper state? This can be something as simple as bundle exec rails r "...".

OK

@yorickpeterse Inconsistent states suck, but in this case, I agree it's better than having an offline/manual migration.

Agree with https://gitlab.com/gitlab-org/gitlab-ce/issues/17384#note_11757890