search

Open-MSS / MSS

A QT application, a OGC web map server, a collaboration server to plan atmospheric research flights.
https://open-mss.github.io
Apache License 2.0
54 stars 68 forks source link

updated data base migration to current release #2410

Closed ReimarBauer closed 5 days ago

ReimarBauer commented 1 week ago

Purpose of PR?:

Fixes #2401

We need to update the documentation

matrss commented 1 week ago

I'm confused, how is the migration intended to be applied? Shouldn't the migration script be added to mslib/mscolab/migrations/versions or something?

ReimarBauer commented 1 week ago

I'm confused, how is the migration intended to be applied? Shouldn't the migration script be added to mslib/mscolab/migrations/versions or something?

We added the migration facility not from the beginning of mscolab.

The tool checks the existing database and builds from the model of the installation a migration script. It is important that a new database gets a revision id. This is implemented. The mig script has the revision id which it can work on. When we add them to the dir we would have revision ids which are different to an existing productive database. On that place you can't edit the migration files. Using them can give an error like

alembic/script/revision.py:242: UserWarning: Revision 27a026b0daec referenced from 27a026b0daec -> e62d08ce88a4 (head), To version 9.0.0 is not present```

We use them like in the video, having on the production system the database for the migrations. We use the documentation for hints. Sometimes the creation of the mig script is not all to do. This time the mig script was not able on a database with users to solve correctly: 

sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) Cannot add a NOT NULL column with default value NULL
[SQL: ALTER TABLE users ADD COLUMN authentication_backend VARCHAR(255) NOT NULL]

We can add them under revision control but I think we should document them too.

When we start with flask db init on a fresh system, we have no down_revision to rollback. I would have expected that.

revision = 'b7749653aa41'
down_revision = None
matrss commented 1 week ago

We added the migration facility not from the beginning of mscolab.

Doesn't matter.

The tool checks the existing database and builds from the model of the installation a migration script.

Yes. This should only happen once and the resulting (possibly fixed up, like in this case) migration script committed to the repository, so that the admin of a mscolab server only needs to run flask db upgrade (or maybe that should happen automatically on application startup). This is what the Flask-Migrate docs suggest on how the tool is intended to be used.

When we add them to the dir we would have revision ids which are different to an existing productive database.

This should never be the case. If this is the case then either we already skipped adding a migration script to the repository or the production database was messed with outside of what the mscolab application ships with and as a developer I am not interested in supporting this situation. The migration scripts in the repository should form a continuous chain (or actually more like a DAG, if we consider branching) from one database revision to the next, and any state outside of that should be unreachable.

On that place you can't edit the migration files.

This is by design, once the migration script is committed to the repository (or at least once it is part of a release) it should be considered immutable.

Using them can give an error like

alembic/script/revision.py:242: UserWarning: Revision 27a026b0daec referenced from 27a026b0daec -> e62d08ce88a4 (head), To version 9.0.0 is not present

This error looks like a previous migration script wasn't committed to the repository, which is a bug.

Sometimes the creation of the mig script is not all to do. This time the mig script was not able on a database with users to solve correctly:

sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) Cannot add a NOT NULL column with default value NULL
[SQL: ALTER TABLE users ADD COLUMN authentication_backend VARCHAR(255) NOT NULL]

That's why the fixed migration script is added to the repository, to avoid having to repeat manual steps.

We can add them under revision control but I think we should document them too.

Documentation should be some variant of "Run flask db upgrade to get your database up-to-date.", or that should be done automatically on startup. A user shouldn't have to touch migration scripts at all, unless they messed with their production database schema, in which case they are on their own.

ReimarBauer commented 6 days ago

The documentation needs a change. That was missing by the release.

Additionally we should try to understand why the migration scripts are not used from our library and how to solve this without breaking other functionality. On the server the created migration scripts are found because there the Migrate uses a relative path from the location of the wsgi script. This works as described forward and backwards.

When we have a solution which works always also on the server we could remove documenting this.

The documentation I linked is a good start for deploying from a directory, which we don't. We don't have an app.py in the users home dir. And when creating a wsgi script we have a new start path for the revision scripts which differs from the package.

I think we should work on this in a new issue. I assume that there is some refactoring needed. This PR should primary fix the documentation.

matrss commented 6 days ago

The documentation needs a change. That was missing by the release.

Maybe, but this change is not what is needed. An admin of an mscolab server never has to run flask db migrate, so we shouldn't document it as if they have to. That would be counterproductive, as in messing up production databases into a state in which we will never be able to do proper migrations.

What is missing in the release are the migration scripts in mslib/mscolab/migrations.

Additionally we should try to understand why the migration scripts are not used from our library and how to solve this without breaking other functionality.

I don't get what you mean. It is quite a mouthful, but you can get at the migrations with an installed version of mss without a problem, e.g.: flask --app mslib.mscolab.app db heads -d "$(python3 -c 'import mslib.mscolab; print(mslib.mscolab.__path__[0])')"/migrations.

On the server the created migration scripts

On a server no migration script should ever be created. That should solely happen in development.

ReimarBauer commented 6 days ago

Make an issue for this and provide the solution. This PR is for the documentation we have released.

matrss commented 6 days ago

OK, will do. This PR contains a documentation change that we shouldn't make, so I vote for just closing it.

ReimarBauer commented 6 days ago

OK, will do. This PR contains a documentation change that we shouldn't make, so I vote for just closing it.

I added an issues about the ideas I have on this https://github.com/Open-MSS/MSS/issues/2417 Maybe look into this too.

I don't think it is a good idea to have in the docs a migration to 8 script which is wrong compared to what to do for version 9. That is currently the only way a user can get help. It is not that easy to know how to fix the makemigrations failure.

matrss commented 6 days ago

It is not that easy to know how to fix the makemigrations failure.

The proper fix is actually rather easy, it just needs to be done.

matrss commented 6 days ago

I mean, we could merge this PR until the proper fix is done and no manual migrations will ever be necessary, but that will bring us into a situation in which we will have to document multiple migration paths depending on what people currently have in their production databases and it will be a pain to fix if people fail to apply that migration from v8 to v9 properly.

ReimarBauer commented 6 days ago

the easist solution I add to the PR section that this mig path documentation is deprecated and will be removed with the next major.

because of the skyfield-data problem I want to release soon.

btw. we will also learn on our servers first what it means to switch to the better path. Others having the same problems I might be a ressource for help.

matrss commented 6 days ago

OK, let's go with that. The "skyfield-data problem" is nothing that necessarily really requires a release though; since mamba does install time dependency resolution it should already pick up the correct version if it is released on conda-forge.

ReimarBauer commented 6 days ago

Do not expect that users type mamba update on theire existing Installation. They don't see this dependency.

Computation errors can be problematic.