Add instance ID column to migration table #5909
Conversation
| @@ -38,6 +42,22 @@ impl DataStore { | |||
| .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) | |||
| } | |||
|
|
|||
| /// List all migrations associated with the provided instance ID. | |||
| pub async fn migration_list_by_instance( | |||
There was a problem hiding this comment.
Just curious, is there ever more than one entry here? We're filtering to non-deleted migrations, so I naively expected to see just a single value returned here.
There was a problem hiding this comment.
There's never more than one entry yet. Right now, migrations are marked as deleted as soon as the migration completes. However, this will probably change in the future, and migrations will be deleted only when the instance itself is deleted (and possibly once they have been completed for a 'fairly long' period of time). I didn't make that part of the change in this PR to keep the diff small, but I suppose I could...
There was a problem hiding this comment.
Interesting, thanks for the explanation! Is the idea to keep them around in the future as an auditable log of migrations? I have no opinion on whether that should be added now, but I do think a quick comment that there may be more than one in the future might be helpful. Thanks!
There was a problem hiding this comment.
That's part of the motivation, yeah. Additionally, though, migration records will be used by the instance-update saga to do things like determining which Propolis processes are no longer in use because their instance has been migrated out.
hawkw
force-pushed
the
eliza/historical-migration
branch
from
82345ea to
1e6cb9c
Compare
|
Okay, @bnaecker and @gjcolombo, if you have a moment, I'd love another review, as I've made some additional changes: now, we no longer mark migrations as deleted when they complete or fail, and instead mark all migrations for an instance as deleted when the instance itself is deleted. Migration records are still marked as deleted when an |
Currently, the
migrationtable does not contain a way to directlyassociate migrations with instances. An
instancerow'smigration_idis a foreign key into the
migrationtable pointing at the currentlyactive migration, if one exists, but past migrations cannot be easily
associated with migrations. This is unfortunate, as it seems useful to
be able to easily list completed/failed migrations for an instance. This
is particularly important as we would like to change the behavior around
marking migration records as deleted, so that deleting an instance also
deletes any corresponding migration records. It may also be useful for
debugging tools and UIs that expose a migration's history.
This branch adds an instance ID column to the migration table. To avoid
having
NULLs in this column, I've added a migration1 that deletesthe old
migrationtable and creates a new one, rather than backfillingmigration IDs by trying to figure them out using the
vmmrecords forthe migration records. This seemed fine, because (1) the
migrationtable hasn't been released to customers yet, and (2), we don't actually
use the data in that table for anything yet, so nuking it should be
okay. I've also added a query for listing migration records by instance
ID.
Now, we no longer mark migrations as deleted when they complete or fail,
and instead mark all migrations for an instance as deleted when the
instance itself is deleted.
Migration records are still marked as deleted when an
instance-migratesaga unwinds. I'm on the fence as to whether this is the Right Thing or
not, but I think it's fine, as those migration states won't be needed by
the
instance-updatesaga (AFAIK so far). If that doesn't end up beingthe case, we might consider adding a
SagaUnwoundstate, similar to theone for VMMs, so that we can distinguish between migrations that failed
because a VMM failed and migrations whose sagas unwound...
Footnotes
A database migration, not a live migration migration. :) ↩