-
Notifications
You must be signed in to change notification settings - Fork 40
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
[nexus] Add DataStore::instance_fetch_all
#5888
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.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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!
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 anInstanceSnapshot
struct that bundles together all these records. We do this byLEFT JOIN
ing theinstance
table with thevmm
andmigration
table on theinstance
's various ID columns, with a query that should look something like:Ideally, we would instead do two subqueries that join
instance
withvmm
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.