Store "last allocated IP" in BlueprintSledConfig#9550
Conversation
Blippy has had checks on sled subnet consistency for a while, but they were implemented by inferring each sled's subnet from its zones' IPs. We added explicit sled subnets to blueprints in #9416; this changes blippy's sled-subnet-related checks to use that new field. (This is work that fell out of a larger PR that isn't ready yet, and I'm opening it separately to reduce diff clutter in that PR.)
| @@ -0,0 +1,59 @@ | |||
| -- Working with INET values in SQL is "fun". | |||
There was a problem hiding this comment.
This migration query is quite horrifying IMO. I would love suggestions to make it less so.
common/src/address.rs
Outdated
| // to assume that addresses in this subnet are available. | ||
| pub const SLED_RESERVED_ADDRESSES: u16 = 32; | ||
|
|
||
| static_assertions::const_assert_eq!( |
There was a problem hiding this comment.
I added this because I was concerned some places for the new field semantically wanted to use RSS_RESERVED_ADDRESSES and others were using SLED_RESERVED_ADDRESSES, and those mixed uses only worked if they both had the same value. With the switch to the newtype, I don't think that's true anymore (because it enforces a minimum, and if RSS_RESERVED_ADDRESSES went higher that would be fine), so I just took this back out.
| BlueprintSledConfig { | ||
| state: SledState::Active, | ||
| subnet: Ipv6Subnet::new(Ipv6Addr::LOCALHOST), | ||
| last_allocated_ip_subnet_offset: RSS_RESERVED_ADDRESSES, |
There was a problem hiding this comment.
Since this is used in a lot of tests and the connection feels pretty non-obvious, I wonder if we could document this better somehow? Maybe another constant (TESTING_INITIAL_LAST_ALLOCATED_IP_SUBNET_OFFSET is maybe long...) with a comment about it?
There was a problem hiding this comment.
I really don't love another constant; we already have both RSS_RESERVED_ADDRESSES and SLED_RESERVED_ADDRESSES that both mean "the starting point of what Reconfigurator can use". I think we only have two because SLED_RESERVED_ADDRESSES is supposed to be for special IPs like "gz" and "switch zone", except RSS stomps all over it when assigning zone IPs.
I did briefly consider making this a newtype instead of a raw u16; then these could all be something like LastAllocatedIpOffset::initial(). Would that be better?
There was a problem hiding this comment.
I don't think it's too bad to have a third constant, in that they're all right near each other, documented together, and have static asserts. In fact, I wouldn't define this one to be 32 -- I'd define it as the value of that other constant explicitly. More like an alias.
A different type with a named constructor would also work. You don't need a separate type to make it a free function... but then it may as well be a constant.
There was a problem hiding this comment.
I went with a newtype in 27a7e0d - what do you think?
schema/crdb/dbinit.sql
Outdated
| subnet INET NOT NULL, | ||
|
|
||
| -- the last allocated IP within `subnet` used by the blueprint | ||
| last_allocated_ip_subnet_offset INT4 NOT NULL, |
There was a problem hiding this comment.
What do you think about a constraint between 0 and 65K?
| -- Final arg to `substr()`: this trims off the leading | ||
| -- `::` in the `::NNNN` we produced above. | ||
| 3), | ||
| -- Final args to `lpad()`: this ensure we always have | ||
| -- exactly 4 hex digits, left padding with 0 if needed. | ||
| 4, '0') |
There was a problem hiding this comment.
These seem aligned weirdly for the functions that they're arguments to?
| SELECT ('x' || | ||
| lpad( | ||
| substr( | ||
| -- bitwise & the IP with a /112 hostmask, which | ||
| -- squishes this IP down to the string '::NNNN', | ||
| -- where there will be 1-4 hex-valued X characters. | ||
| host(primary_service_ip & hostmask('::/112')), | ||
| -- Final arg to `substr()`: this trims off the leading | ||
| -- `::` in the `::NNNN` we produced above. | ||
| 3), | ||
| -- Final args to `lpad()`: this ensure we always have | ||
| -- exactly 4 hex digits, left padding with 0 if needed. | ||
| 4, '0') | ||
| -- We concatenated a literal 'x' prefix onto the front of the | ||
| -- `NNNN` value we just produced, which causes `bit(16)` to | ||
| -- interpret it as a 16-bit hex value (which it is!). Then | ||
| -- convert that bitstring into an integer. | ||
| )::bit(16)::int AS final_hextet |
There was a problem hiding this comment.
ChatGPT tells me and I've casually confirmed that you can treat inet values as integers. So this works in dogfood to compute the offset for a given row:
root@[fd00:1122:3344:10b::3]:32221/omicron> select sled_id,primary_service_ip,((primary_service_ip & hostmask('::/112')) - '::'::inet) as offset from bp_omicron_zone where blueprint_id = '01155c9f-f124-41b1-aafd-0bc132d22598';
sled_id | primary_service_ip | offset
---------------------------------------+------------------------+---------
b886b58a-1e3f-4be1-b9f2-0c2e66c6bc88 | fd00:1122:3344:106::c | 12
0c7011f7-a4bf-4daf-90cc-1c2410103300 | fd00:1122:3344:2::1 | 1
f15774c1-b8e5-434f-a493-ec43f96cba06 | fd00:1122:3344:105::28 | 40
...
7b862eb6-7f50-4c2f-b9a6-0d12ac913d3c | fd00:1122:3344:101::45 | 69
f15774c1-b8e5-434f-a493-ec43f96cba06 | fd00:1122:3344:105::22 | 34
b886b58a-1e3f-4be1-b9f2-0c2e66c6bc88 | fd00:1122:3344:106::d | 13
(698 rows)
Assuming I did the right thing, maybe that could clean a bunch of this up?
There was a problem hiding this comment.
... yeah that's way better. Thanks
…es (#9608) This is a followup to #9521, specifically driven by @smklein's [note](#9521 (comment)) that the planner _also_ accesses expunged zones, and we need to track those reasons too. Doing this work revealed one case where the planner was accessing them in a way that was going to cause problems with pruning; that's fixed by #9550, and this PR is staged on top of it.
3 participants
This PR is large-ish but very boilerplatey. This is a prerequisite for the planner work to avoid reading expunged zones @smklein pointed out in #9521 (comment).
We add a
last_allocated_ip_subnet_offsetfield toBlueprintSledConfig, along with alast_allocated_ip()method that adds the offset to the sled's subnet. The rest is fallout of using that field:Staged on top of #9549.