[nexus] Add DataStore::instance_fetch_all#5888
Merged
Merged
Conversation
In order to implement the `instance-update` saga ongoing in #5749, it's necessary to have a way to query the database for the state of an instance, its active VMM, its target VMM, and active migration in a single, atomic query. This will be used as a snapshot of the instance state that the update saga will operate on. This commit adds an implementation of such a query in the form of a `DataStore::instance_fetch_all` method, returning an `InstanceSnapshot` struct that bundles together all these records. We do this by `LEFT JOIN`ing the `instance` table with the `vmm` and `migration` table on the `instance`'s various ID columns, with a query that should look something like: ```sql SELECT * FROM instance LEFT JOIN vmm ON ((instance.active_propolis_id = vmm.id) OR (instance.target_propolis_id = vmm.id)) LEFT JOIN migration ON (instance.migration_id = migration.id) WHERE instance.id = instance_id ``` Ideally, we would instead do two subqueries that join `instance` with `vmm` on the active and destination IDs separately and return a single row which contains both in a way that can be distinguished in the result set, but I wasn't able to figure out how to do that in Diesel. Instead, the query may return two rows, if the instance has both an active and target VMM, and iterate over the result set to find both VMMs in Rust code. This is a bit uglier, but it works fine.
gjcolombo
approved these changes
Jun 12, 2024
Comment on lines
+423
to
+425
| // Yes, this code is a bit unfortunate, but Diesel doesn't like the idea | ||
| // of joining with the same table twice on different columns...so, this, | ||
| // at least, works. |
Contributor
There was a problem hiding this comment.
Yeah, this is probably the best way of doing this while coloring inside the lines Diesel has set out. (I could imagine having some kind of CTE that conjures up a temporary table that contains the VMM IDs and a "sequence" column that can be used to order the returned rows unambiguously, but I've found it very hard to do this kind of thing without dropping out to raw SQL.)
Member
Author
There was a problem hiding this comment.
That's sort of what I thought. Writing the CTE seemed like it would be enough of a pain that I didn't really think it would be worth the effort, but we can revisit this later.
hawkw
added a commit
that referenced
this pull request
Jun 13, 2024
This commit rewrites the `DataStore::instance_fetch_all` query added in #5888 to perform two separate left joins with the `vmm` table, so that the result set only contains a single row. This is a bit more aesthetically pleasing than having to iterate over a multi-row result set, but should still return the same values. Thanks to @jgallagher and @luqmana for figuring out how to do the Diesel alias!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to implement the
instance-updatesaga ongoing in #5749, it's necessary to have a way to query the database for the state of an instance, its active VMM, its target VMM, and active migration in a single, atomic query. This will be used as a snapshot of the instance state that the update saga will operate on.This commit adds an implementation of such a query in the form of a
DataStore::instance_fetch_allmethod, returning anInstanceSnapshotstruct that bundles together all these records. We do this byLEFT JOINing theinstancetable with thevmmandmigrationtable on theinstance's various ID columns, with a query that should look something like:Ideally, we would instead do two subqueries that join
instancewithvmmon the active and destination IDs separately and return a single row which contains both in a way that can be distinguished in the result set, but I wasn't able to figure out how to do that in Diesel. Instead, the query may return two rows, if the instance has both an active and target VMM, and iterate over the result set to find both VMMs in Rust code. This is a bit uglier, but it works fine.