Skip to content

[nexus] Add DataStore::instance_fetch_all#5888

Merged
hawkw merged 3 commits into
mainfrom
eliza/instance-fetch-all
Jun 12, 2024
Merged

[nexus] Add DataStore::instance_fetch_all#5888
hawkw merged 3 commits into
mainfrom
eliza/instance-fetch-all

Conversation

@hawkw

@hawkw hawkw commented Jun 12, 2024

Copy link
Copy Markdown
Member

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

@hawkw hawkw Jun 12, 2024

Copy link
Copy Markdown
Member Author

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
@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