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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrations: Update backfill assumptions #55650

Merged
merged 2 commits into from Aug 8, 2023
Merged

migrations: Update backfill assumptions #55650

merged 2 commits into from Aug 8, 2023

Conversation

efritz
Copy link
Contributor

@efritz efritz commented Aug 8, 2023

Fixes the root cause of #55414, which broke upgrades between v5.0[3,6] and v5.1.5. We will need to provide instructions for users that did hit v5.1.5, as they will have already experienced the effect of this bug. They can simply run the missing migrations we can supply as a support packet. This should fix the issue from occurring in v5.1.6 and later.

The issue came from v5.1.5 incorrectly backfilling a set of migrations. When we initialize the database, we insert "backfilled" migration records for migrations that existed in the past but are no longer in-tree (after being squashed away). The logic for this assumed that version branches wouldn't redefine migration ancestry (we still shouldn't, but we can fix the effect of there).

Since we've started backporting Cody features, we've introduced at least one occurrence where the ancestry of a migration definition changes between the 5.0 and main branches. In these cases, a migration was backported without all of its ancestors. This creates a situation where moving from 5.0 to 5.1 branches a migration suddenly gets an (unapplied) parent added to it.

This breaks the assumption that any migration successfully applied has also had its ancestors applied. But sometimes Fry has to go back in time and become his own grandfather. 馃し

This PR fixes the issue by instead of backfilling _all ancestors of all applied migrations-, we only backfill the (get read for this, it's a mouth-full): all ancestors of the root of a version of which all leaf migrations have been applied. :clueless:

This is a less eager way to backfill, as we will only backfill definitions prior to a completely applied set of migrations for some particular version.

Test plan

Updated unit tests. Will backport these changes to a local 5.1 branch and test the v5.0.6->v5.1.5(+ this fix) upgrade to see if we still have issues.

@efritz efritz self-assigned this Aug 8, 2023
@cla-bot cla-bot bot added the cla-signed label Aug 8, 2023
@coury-clark
Copy link
Contributor

all ancestors of the root of a version of which all leaf migrations have been applied

馃槄

@efritz
Copy link
Contributor Author

efritz commented Aug 8, 2023

all ancestors of the root of a version of which all leaf migrations have been applied

馃槄

Correct response! Maybe this is helpful?

image

@coury-clark
Copy link
Contributor

@efritz very! I think this should go in the docs somewhere, so when we inevitably find something else we need to change we don't have to relearn how this all works 馃槃.

// version's migrations. We can backfill from its root.
root := bounds.RootID
if root < 0 {
root = -root
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this ever be negative?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squashed migrations become a pair of [-ID, ID] migrations where the negative one has privileged commands (extension installation). We have to do this because there's no known gaps between IDs when they were sequential.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically says "if it's the extension one, just go to its twin"

@efritz efritz merged commit a59fd1e into main Aug 8, 2023
12 checks passed
@efritz efritz deleted the ef/fix-backfill branch August 8, 2023 22:52
@efritz
Copy link
Contributor Author

efritz commented Aug 8, 2023

I think this should go in the docs somewhere, so when we inevitably find something else we need to change we don't have to relearn how this all works

@DaedalusG would you be game to make some internal doc pages (even if blank) to start filling out? I'm happy to write or brain dump about any of the internals, but the wide canvas is too daunting to start off on my own with.

github-actions bot pushed a commit that referenced this pull request Aug 8, 2023
coury-clark pushed a commit that referenced this pull request Aug 8, 2023
Fixes the root cause of #55414, which broke upgrades between `v5.0[3,6]`
and `v5.1.5`. We will need to provide instructions for users that did
hit v5.1.5, as they will have already experienced the effect of this
bug. They can simply run the missing
[migrations](#55414 (comment))
we can supply as a support packet. This should fix the issue from
occurring in `v5.1.6` and later.

The issue came from v5.1.5 incorrectly backfilling a set of migrations.
When we initialize the database, we insert &quot;backfilled&quot;
migration records for migrations that existed in the past but are no
longer in-tree (after being squashed away). The logic for this assumed
that version branches wouldn&#39;t redefine migration ancestry (we still
shouldn&#39;t, but we can fix the effect of there).

Since we&#39;ve started backporting Cody features, we&#39;ve introduced
[at least one](#50869) occurrence where the ancestry of a migration
definition changes between the `5.0` and `main` branches. In these
cases, a migration was backported without all of its ancestors. This
creates a situation where moving from `5.0` to `5.1` branches a
migration suddenly gets an (unapplied) parent added to it.

This breaks the assumption that any migration successfully applied has
also had its ancestors applied. But sometimes Fry has to go back in time
and become his own grandfather. 馃し

This PR fixes the issue by instead of backfilling _all ancestors of all
applied migrations-, we only backfill the (get read for this, it&#39;s a
mouth-full): _all ancestors of the root of a version of which all leaf
migrations have been applied_. :clueless:

This is a less eager way to backfill, as we will only backfill
definitions prior to a *completely applied* set of migrations for some
particular version.

## Test plan

Updated unit tests. Will backport these changes to a local 5.1 branch
and test the v5.0.6-&gt;v5.1.5(+ this fix) upgrade to see if we still
have issues. <br> Backport a59fd1e from
#55650

Co-authored-by: Eric Fritz <eric@sourcegraph.com>
@DaedalusG
Copy link
Contributor

@efritz @coury-clark I was actually thinking the same thing. As an exercise for knowledge transfer going through and writing comprehensive docs about migrator upgrades and the dependencies on the release docs is in order 馃憤, it'll be a great way to deeply learn this part of the codebase too 馃槑

@DaedalusG
Copy link
Contributor

I was actually planning on doing something like this with advisement as is 馃憤

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

Successfully merging this pull request may close these issues.

None yet

3 participants