Skip to content
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

Merged
merged 3 commits into from
Jun 12, 2024
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jun 12, 2024

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 JOINing the instance table with the vmm and migration table on the instance's various ID columns, with a query that should look something like:

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.

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.
@hawkw hawkw requested a review from gjcolombo June 12, 2024 19:22
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.
Copy link
Contributor

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.)

Copy link
Member Author

@hawkw hawkw Jun 12, 2024

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.

@hawkw hawkw enabled auto-merge (squash) June 12, 2024 19:58
@hawkw hawkw merged commit 1b89771 into main Jun 12, 2024
19 checks passed
@hawkw hawkw deleted the eliza/instance-fetch-all branch June 12, 2024 23:53
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!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants