Support versioned migrations

[eaon]

Description

Implements freedomofpress/securedrop-workstation#673, a way to ensure all migrations that would be necessary to run are executed in succession, even if there are multiple, and that at time of package install/upgrade. It is a test balloon 🎈 intended to be ported to securedrop-workstation after the next release.

This is work also goes towards Qubes OS R4.2 support, which will have its own updater policy system, which would otherwise be incompatible with our current upgrade/migrate strategy.

Note: this PR only contains the migration engine and its respective tests, it does not contain any actual migrations. This will follow in a separate PR.

Review

• Documentation in migrations/README.md is helpful enough to start writing new migrations
• Test coverage is adequate

Testing

• CI successfully runs
• make test (in a dev environment)

No dom0 testing necessary.

[rocodes]

Had a meeting with @eaon about this work today, and just documenting some of our conversation here.

istm there are 3 main somewhat independent areas for review:

• the versioning/parsing logic
• the actual upgrade process (can we build in robust testing around any migration plans we write, to ensure that they do what is intended and don't break anything?)
• the containerization portion and changes to rpm build logic

I feel comfortable in the first two areas, but would appreciate the additional eye of someone more familiar with packaging and building to look at the rpm build and test process (maybe a @legoktm 😄 )?

For next steps: @eaon and have set aside some time next week to pair and document test scenarios, particularly for the second point (identifying edge cases/more robust migration testing).

[legoktm]

Did a first pass of just visual review, will poke at the packaging/rpm/etc. stuff tomorrow.

[legoktm]

My main concern about the use of inheritance is that it's just difficult to trace what is happening, given any action. You have to be aware of the entire hierarchy, and what each class does for each step.

versus something like:

def remove(path: Path):
    snapshot(path)
    if path.exists():
        try:
            path.unlink()
        except Exception as e:
            raise Rollback(e)

Yes, there will probably be some duplication because of it, but it's very straightforward to understand what's happening and in what order.

[legoktm]

Since these are Python files, what's the advantage gained from shelling out? I know earlier in the PR these were bash scripts, but now shelling out doesn't seem necessary.

I think it would be cleaner if each migration file returned a list of steps to run, and then the main migration entrypoint ran the necessary ones.

[eaon]

Very good question that I missed to think about when you originally commented… and you're right that's basically just organic growth, it would certainly make logging less icky as well. However I don't quite see a good way to do this yet, do you have any suggestions?

[eaon]

Refactored to use importlib which seemed like the least worst option to continue to use the version file name convention.

[legoktm]

I'm skeptical how useful snapshotting will be. For example, adding/moving/deleting a file will fail because of some I/O error (disk full, permissions, etc.) or a logic issue (code has the wrong name).

In the first case, if we're having I/O errors, copying or moving the tree has no better chance at success and really a lot more ways to go wrong. It could be useful in the second case, but then it seems better to have a global snapshot, rather than per-step ones.

[eaon]

Apparently I lost a response to this message, but for posterity: as discussed in person, I agree with your view here, the implementation for simple file operations is more of a showcase, as the primary target of this functionality would be for making potential destructive changes to VMs and templates, where this functionality would be much more appropriate.

[rocodes]

Just chatted with @eaon today about testing + documenting. I suggested either additions to the README that explain the process of writing a migration, or a separate README for this purpose in the migrations directory. I'm happy to help write up any docs that need writing.

My understanding of the main points (to get this readme/explainer started/make sure I understand correctly) is:

• To write a migration, create a new python file in the migration directory, $version.py, where $version is correct semver. In this file, include an ordered list of steps ([MigrationStep]).
• Use existing migration steps for commonly-used actions such as remove, rename, etc
• If a custom action is needed, build it by inheriting from MigrationStep. Implement at minimum, the run method, but also optionally other methods as shown in migration_steps.py. (We should include advice on if/when writing rollback is useful)
• Error-handling is managed already, so the run method does not need to implement error handling. The premise of the current error handling is try migration -> if error, roll back and quit. (The migrate method should have a comment and the approach to error handling should be documented)
• If writing any additional MigrationSteps, add unit tests
• We should also add a quick "When do I write a migration?" FAQ

lmk if you think I'm missing any key info!

[eaon]

That's pretty much it! Small nits:

• There's not just error handling for run, but for all the steps (I even added one for validation now even though that seems a bit overkill 😜)
• Per-migration custom MigrationSteps that are specific to system state won't support unit tests because we don't have an automated install/testing infrastructure that would support that. So add unit tests please only applies to generic solutions that would get added to migration_steps.py
• As for when to write a migration, I don't think there's more to it than we need the system state to be different than it was before.

[legoktm]

OK! I hope the number of comments isn't overwhelming, like half of them are pretty minor/trivial issues.

I did not yet get to reviewing the test framework and the tests themselves, will try to do that tomorrow.

[legoktm]

nit: Typically I'd name functions as verb-noun, so remove_path, validate_path, etc.

[eaon]

Aye, having a style guide for that sort of thing might be good 😄 My approach to this is typically to start names of things with the part that is shared, sort of sharedbase_variant[_detail] etc. Not sure if there's a name for that though. Totally open to renaming them

[legoktm]

Overall the tests look good, the main remaining issue is the types being passed to the MigrationStep constructors being Path or str.

Also a general nit is that the read_text()/write_text() functions are nicer than .open.read() and .open("w").write() :-)

[legoktm]

One very minor documentation update is needed, otherwise it looks ready to me!!

[legoktm]

Awesome work!