dns update during blueprint execution uses wrong version number for precondition#10252
dns update during blueprint execution uses wrong version number for precondition#10252davepacheco wants to merge 2 commits intomainfrom
Conversation
| info!( | ||
| log, | ||
| "attempting to update from generation {} to generation {}", | ||
| dns_config_current.generation, |
There was a problem hiding this comment.
Should the from log here be blueprint_generation?
| // the blueprint was generated. But we have to check later when we try to | ||
| // update the database anyway. And we're not wasting much effort allowing | ||
| // this proceed for now. This way, we have only one code path for this and | ||
| // we know it's being hit when we exercise this condition. |
There was a problem hiding this comment.
Is this comment still accurate? Specifically the "we're not wasting much effort allowing this to proceed" bit - it looks like we could know at this point that we're already guaranteed to fail (by comparing the blueprint DNS version against what we just loaded from the DB), right? By not checking this, we have to do what looks like a nontrivial amount of work including a bunch of db queries (list all silos, list all dns zones, then try an insert that we know is doomed)?
| .blueprint_target_get_current_full(&opctx) | ||
| .await | ||
| .expect("failed to read current target blueprint"); | ||
| // Override the CockroachDB settings so that we don't try to set them. |
There was a problem hiding this comment.
Not really here or there, but - why do we need to do this?
| // above. | ||
| let ip_pool_range_rows = | ||
| fetch_all_service_ip_pool_ranges(&datastore, &opctx).await; | ||
| let external_ip_policy = { |
There was a problem hiding this comment.
If we're testing internal DNS, I think we could trim the boilerplate quite a bit here by picking a zone type that doesn't need as much configuration (e.g., add another crucible pantry or oximeter instead of Nexus).
smklein
left a comment
smklein
left a comment
There was a problem hiding this comment.
Agree with the things john said here, but also, the one line that actually changes the generation supplied LGTM
3 participants
Fixes #10251.