-
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
RPW for OPTE v2p Mappings #5568
Conversation
Ooh, will this also resolve #4259? We had talked about using an RPW for the v2p mappings in that ticket as well. |
@davepacheco Why yes, yes it will! I love sweeping up multiple bugs / issues into a single fix! |
ea311b8
to
ca42e1e
Compare
Quick tests are showing that our initial implementation is working Desired State
Push the v2p state away from the desired state
v2p state is now incorrect
rpw corrects the drift
v2p state is now correct again
|
Going to shift gears and finish up the |
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.
🚀
use omicron_common::api::external::ListResultVec; | ||
|
||
impl DataStore { | ||
pub async fn v2p_mappings( |
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.
does this also need a paginated query?
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.
Well... I admittedly tend to use DataPageParams::max_page()
in RPWs whenever using db queries that require pagination parameters, and that will return u32::MAX
(4_294_967_296
) entries, so I'd say it's kind of moot here, unless we want to standardize on a lower limit for db queries in background tasks (which honestly, it wouldn't shock me if we decide that it would be a good idea to do so!)
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.
Under @davepacheco's direction we've started using this pattern in reconfigurator RPWs:
omicron/nexus/db-queries/src/db/datastore/zpool.rs
Lines 125 to 134 in 8ffe0e1
opctx.check_complex_operations_allowed()?; | |
let mut zpools = Vec::new(); | |
let mut paginator = Paginator::new(SQL_BATCH_SIZE); | |
while let Some(p) = paginator.next() { | |
let batch = self | |
.zpool_list_all_external(opctx, &p.current_pagparams()) | |
.await?; | |
paginator = p.found_batch(&batch, &|(z, _)| z.id()); | |
zpools.extend(batch); | |
} |
check_complex_operations_allowed()
fails the opctx is associated with an external API client, and then we do still list all the items, but in paginated batches to avoid a giant result set coming from crdb.
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.
I have updated the query to use this pattern.
nexus/src/app/instance_network.rs
Outdated
// - it means that delete calls are required as well as set calls, | ||
// meaning that now the ordering of those matters (this may also | ||
// necessitate a generation number for V2P mappings) |
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.
I don't think generation numbers are required with the approach of using a periodic background task to correct drift... what do you think?
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.
I think it mostly depends on whether or not it is acceptable to send / read the entire configuration during a reconciliation loop. For things like NAT, the total configuration can be quite large, so we employ generation numbers so we can send only the required updates instead of the full set. For v2p mappings, the individual messages are much smaller. My napkin math from before showed that if we had a vm per core, with a vnic per vm, that would be something in the ballpark of 213KB for a full set of updates for a single sled, which seems reasonable.
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.
👍
All outstanding comments have been resolved, going ahead and setting this to auto-merge unless anyone has an objection. I'd like to get this soaking on Dogfood sooner rather than later for the upcoming release :) |
sigh looks like a test is flaky
|
Fixed the flaky test. I was just using the tooling wrong 😛 |
TODO
delete
functionality cleans up v2p mappingsPossible Optimizations
Related
Resolves #5214
Resolves #4259
Resolves #3107