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

Rework migration system to have independent migrations #768

Open
wants to merge 11 commits into
base: stretch-unstable
from

Conversation

@alexAubin
Copy link
Member

commented Aug 5, 2019

The problem

This follows discussions during meetings about #585 which ships a critical migration (half of yunohost won't be able to ran as long as this migration ain't ran).

The current migration system is too constraining as if a migration C or D fails (or has to be ran manually), then critical migration E won't be ran even though it's independent of C and D.

So in the case of group permissions, the migration would / could not be ran just because some other pending / failed migration.

Solution

The proposed solution is to rework the migration system to make migrations independent from each other (though we can still have dependencies between migration via self.dependencies = [ ...list of required migrations to be done before this one ...] inside the migration)

So if migrations C, D, and E are pending, running yunohost tools migrations migrate will attempt to run every of those pending migrations even if C or D fails.

This requires to change the way we store the state of migrations, as only the name/number of the last ran migrations was stored so far. Now instead, a yaml json containing the list of known migrations with a status skipped, done or pending.

In this PR, I also included many tweaks following past experiences with the migration mechanism, such as :

  • ability to re-run a migration even if it was already ran
  • the -t option was confusing when you just want to revert the last migration (e.g. during development) so instead there's now a --revert option
  • instead of json, yaml is probably better (i can remember many situation where I had to - or tell users to - manually edit the file for some reasons)

PR Status

Tested on my side but should run more tests ...

Also this is blocking for 3.7 since it's a must have to be sure upgrades will happen smoothly even for people upgrading from old versions

How to test

  • Pull the branch
  • If you run yunohost tools migrations list or yunohost tools migrations state, the old json should be transparently migrated to a new yaml file, you should be able to see this in /etc/yunohost
  • Then you can try to revert some migrations such as 0007_ssh_conf_managed_by_yunohost_step1 and 0008_ssh_conf_managed_by_yunohost_step2 (though that last one you gotta do it by hand in the .yaml file because reverting aint supported for it)
  • Then attempt to run just 0008_ssh_conf_managed_by_yunohost_step2, should tell you that first you gotta run part1
  • Then if you just run yunohost tools migrations migrate it should run both 0007 and 0008

... though there are many other options and use case to test, not sure I can list them all here é_è

Validation

  • Principle agreement 1/2 : ljf
  • Quick review 0/1 :
  • Simple test 1/1 : ljf
  • Deep review 1/1 : ljf
@decentral1se
Copy link
Contributor

left a comment

Quick fly by ... hopefully get time to manually test this!

Good Work 🚀

logger.warn("Falling back on the standard shell.")

import readline # will allow Up/Down/History in the console
readline # to please pyflakes

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 6, 2019

Contributor

Can just use # noqa on the line above to avoid, I think.

# If asked, filter pending or done migrations
if pending or done:
if done:
migrations = [m for m in migrations if m["state"] != "pending"]

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 6, 2019

Contributor
Suggested change
migrations = [m for m in migrations if m["state"] != "pending"]
migrations = [m for m in migrations if m["state"] != "done"]

?

This comment has been minimized.

Copy link
@zamentur

zamentur Aug 14, 2019

Contributor

like that --done return "done" and "skip" but i think webadmin want to display done and skipped migrations together.

# If explicit targets are provided, we shall validate them
else:
targets = [get_matching_migration(t) for t in targets]
done = [t.id for t in targets if t.state != "pending"]

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 6, 2019

Contributor
Suggested change
done = [t.id for t in targets if t.state != "pending"]
done = [t.id for t in targets if t.state != "done"]

?

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Aug 10, 2019

Member

Current code looks right, to have "done" target you just want exclude the "pending" ones (other states are "done" too)

logger.warn("You don't have IPython installed, consider installing it as it is way better than the standard shell.")
logger.warn("Falling back on the standard shell.")
# Well ugh what are we supposed to do if we can't do this >.>...
pass

This comment has been minimized.

Copy link
@decentral1se

decentral1se Aug 6, 2019

Contributor

Probably should explode here!

This comment has been minimized.

Copy link
@zamentur

zamentur Aug 14, 2019

Contributor

Personally i think we should remove the try except here

This comment has been minimized.

Copy link
@alexAubin

alexAubin Aug 14, 2019

Author Member

You mean we kind safely assume that we can perform the import because that would be really strange otherwise ? I kind of agree, e.g. we don't do try ... except for the usual imports at the top of the file either ..

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

A bit hard to do a good review with my current life schedule (and commits are quite mixed up 😅) but it looks good from a quick review :)

raise YunohostError("migration_backward_impossible", name=self.name)

def migrate(self):
def forward(self):

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Aug 14, 2019

Member

You've renamed everything to forward (which is not really part of this commit if we look at the commit message :/) but it's still the migrate method that is called so this will break everything 🤐

This comment has been minimized.

Copy link
@zamentur

zamentur Aug 14, 2019

Contributor

I have renamed it just because some migration use forward and other migrate.

Migrate method is marked as to not override in the class Migration. this migrate method call forward, and i didn't find other part of the code calling this method. But i am currently doing an other pr derivated of this one with other stuff.

@zamentur
Copy link
Contributor

left a comment

Reviewed and tested, LGTM
I have tested force-run, i suggest to remove --revert because --revert don't manage dependencies, and after discussion it's not so usefull and it's not relevant developper take time to create backward function. It should be better if they implement a rollback mechanism in their migrations (see #774 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.