[db-queries] Convert virtual provisioning CTE to use raw SQL #5089
Conversation
This file contains 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
hawkw
approved these changes
May 28, 2024
nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs
Outdated
Show resolved
Hide resolved
Comment on lines
+116
to
+118
| WITH | ||
| parent_silo AS (SELECT project.silo_id AS id FROM project WHERE project.id = ").param().sql("),") | ||
| .bind::<sql_types::Uuid, _>(project_id).sql(" |
There was a problem hiding this comment.
IIUC it shouldn't really be necessary to format the query strings here identically to the format_sql output, since we're still going to call format_sql on them prior to testing that they match?
There was a problem hiding this comment.
Yeah, that's correct. My goal with the layout of code here is "make it as easy to grok as possible", which is super subjective with non-formatted interpolated strings, but I'm open to suggestions.
Regardless, yes, updating whitespace won't break the tests / change any saved outputs.
smklein
added a commit
that referenced
this pull request
May 30, 2024
…#5830) Builds on #5081 and #5089 , but more out of convenience than necessity. # Summary This PR attempts to validate that, for the "virtual provisioning collection {insert, delete}" operations, they are idempotent. Currently, our usage of `max_instance_gen` only **partially** prevents updates during instance provisioning deletions: - If `max_instance_gen` is smaller than the observed instance generation number... - ... we avoid deleting the `virtual_provisioning_resource` record (which is great) - ... but we still decrement the `virtual_provisioning_collection` values (which is really not great). This basically means that we can "only cause the project/silo/fleet usage values to decrement arbitrarily, with no other changes". This has been, mechanically, the root cause of our observed underflows (e.g, #5525). # Details of this change - All the changes in `nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs` are tests validating idempotency of these operations. - All the changes in `nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs` are actually changes to the query which change functionality. The objective of these changes is to preserve idempotency of the newly added tests, and to prevent undercounting of virtual provisioning resources. If these changes are reverted, the newly added tests start failing, showing a lack of coverage.
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.
Builds on:
Converts the virtual provisioning CTE to use SQL more directly