Add disk generation to inventory #7162
Conversation
This is the first PR related to fixing 7098
| @@ -0,0 +1,3 @@ | |||
| ALTER TABLE omicron.public.inv_sled_agent | |||
| ADD COLUMN IF NOT EXISTS omicrion_physical_disks_generation INT8 | |||
| NOT NULL DEFAULT 0; | |||
There was a problem hiding this comment.
I was wondering whether or not "zero" would be an invalid generation number -- since we start counting from "1":
https://github.com/oxidecomputer/omicron/blob/main/common/src/api/external/mod.rs#L755-L757
But weirdly this seems to be fine. A "zero value generation number" is still a valid u64, and would succeed when parsed out of the database as an i64:
https://github.com/oxidecomputer/omicron/blob/main/common/src/api/external/mod.rs#L808-L817
There was a problem hiding this comment.
Yes, this is intentional. Zero is less than any valid generation number, and therefore will always indicate that we don't know if the sled-agent has seen any OmicronPhysicalDisksConfig. We could have used a NULL column, but this is only for backfilling, and moving forward we won't want a NULL here at all.
There was a problem hiding this comment.
Yes, this is intentional. Zero is less than any valid generation number, and therefore will always indicate that we don't know if the sled-agent has seen any
OmicronPhysicalDisksConfig. We could have used a NULL column, but this is only for backfilling, and moving forward we won't want a NULL here at all.
I suppose since the StorageResources always returns Generation::new which will be in any new inventory, we may actually want to default this to 1.
There was a problem hiding this comment.
I changed it from 0 to 1 in 6d9616e
There was a problem hiding this comment.
eh, was gonna say, 0 would actually be okay to me, because any place we'd compare it with the "initial inventory collection, with value of 1", the real data would evaluate as "greater than" the stub value
There was a problem hiding this comment.
Yeah, I had the same thought. What convinced me to change it was that an empty inventory reports as Generation 1 after this PR and not 0. So we still need any "real" disk inventory to have Generation 2 to notice a change in the planner. This argues for some consistency, but is a somewhat weak argument.
There was a problem hiding this comment.
Light 👍 on 1 over 0; generation 1 is already "the initial/empty set" for zones and datasets, so I assume that's true for disks too? (Or should be if it isn't!). Also it shouldn't really matter much, I think? Inventory turns over pretty quickly, so whatever default we have here should be gone after a few minutes.
This is the first PR related to fixing #7098