Reconfigurator: Teach planner about SNAT IPs #6195
Conversation
Prior to this change, the planner expected all blueprints to have fully-exclusive external IP addresses. This isn't compatible with #6037, where RSS now hands out SNAT IPs with distinct port ranges but the same IP address. A big chunk of this work is necessary to support boundary NTP zone planning, but that isn't included in this PR, so those bits are marked with `#[cfg(test)]`. Fixes #6194.
jgallagher
force-pushed
the
john/reconfigurator-external-ip-allocator
branch
from
85c8ba8 to
4678f7d
Compare
| if self.used_snat_ips.contains_key(&ip) | ||
| || !self.used_exclusive_ips.insert(ip) | ||
| { | ||
| bail!("duplicate external IP: {external_ip:?}"); |
There was a problem hiding this comment.
Is this something we'll ever hit beyond a logic error?
IIRC there was an error enum in this code where internal logic errors could be put -- could this use that enum to be explicit?
There was a problem hiding this comment.
Hmm, it looks like we've been weirdly consistent about this - all anyhow::Errors get converted to Error::Planner(anyhow::Error), so lots of existing methods that can only fail due to internal logic errors return anyhow::Result, and any methods that can fail for any other reason return Result<_, Error>. (E.g., changing this would also require changing the return type of BuilderExternalNetworking::new(), which would require changing the half dozen bail!s it has and the return type of BlueprintBuilder::new_based_on(), ...). It might be worth doing for clarity, but I'm not sure?
There was a problem hiding this comment.
Ah if it's already the prevailing standard then this is okay. I'd say it's definitely worth doing at some point though, anyhow errors floating around just make me uncomfortable because of how untyped they are.
| OmicronZoneExternalIp::Floating(ip) => { | ||
| let ip = ip.ip; | ||
| if self.used_snat_ips.contains_key(&ip) { | ||
| bail!( |
There was a problem hiding this comment.
same comment here -- internal enum for logic error?
| Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, strum::EnumIter, | ||
| )] | ||
| #[cfg_attr(test, derive(test_strategy::Arbitrary))] | ||
| enum SnatPortRange { |
There was a problem hiding this comment.
Oh hey, this is what we talked about at some point right?
There was a problem hiding this comment.
Almost - we talked about it in the bigger context of omicron-common; this is more of a local trial run of that idea. I think I like it, but it's not entirely frictionless.
| // here. We'll use an explicit match to guard against `SourceNatConfig` | ||
| // gaining other kinds of validation we're currently not aware of. |
Prior to this change, the planner expected all blueprints to have fully-exclusive external IP addresses. This isn't compatible with #6037, where RSS now hands out SNAT IPs with distinct port ranges but the same IP address.
A big chunk of this work is necessary to support boundary NTP zone planning, but that isn't included in this PR, so those bits are marked with
#[cfg(test)].Fixes #6194.