Encode resource limits into schema #3864
Conversation
openapi/nexus.json
Outdated
| "description": "The amount of memory to allocate to the instance, in bytes.\n\nMust be between 1 and 256 GiB.", | ||
| "type": "integer", | ||
| "format": "uint64", | ||
| "minimum": 1073741824, | ||
| "maximum": 274877906944 |
There was a problem hiding this comment.
This worked out nicely! The numbers are a bit hard to read but I think the calculated description is helpful in that regard.
| "minimum": 1073741824, | ||
| "maximum": 1098437885952 |
There was a problem hiding this comment.
Cool to see this in the API, I hope this can make our clients better!
There was a problem hiding this comment.
So, this actually isn't right. It's not the min/max that's wrong, it's the schema. We still want ByteCount to be preserved but we also want the limits. It took me a little digging to figure out what to do here.
According to the spec two models can be composed into a single object via allOf. So what we actually want here is an allOf with the ByteCount and these limited definitions. My latest commit has something that should produce that, but typify pukes on it here:
I plan to look at this more tomorrow.
There was a problem hiding this comment.
I mean... the unimplemented! isn't wrong...
| pub const MIN_MEMORY_BYTES_PER_INSTANCE: u32 = 1 << 30; // 1 GiB | ||
| pub const MAX_MEMORY_BYTES_PER_INSTANCE: u64 = 256 * (1 << 30); // 256 GiB |
There was a problem hiding this comment.
Nitpick: For all these "_BYTES" constants, could we use u64?
There was a problem hiding this comment.
Actually, could we hold off on that until another PR? It has some decently wide ranging repercussions. There are a lot of cases that we have to move from try to try_from. Further there are situations where we're adding two u32 ints together which require them to both be transformed. We can do it, it's just going to touch a lot of places and it'd be nice to not have that all in this PR.
There was a problem hiding this comment.
@smklein you and your love of u64s....
There was a problem hiding this comment.
I propose a simple plan, charge $0.005 / hour (USD) effective February 1st for all u32 usage until we migrate to a fully u64 world.
Is that valuable? Why is that valuable? How do you envision the user experience changing / improving for what kinds of (erroneous) operations? It seems that there are other situations where we might kick back a value as being too large e.g. if the number of CPUs indicated is larger than what remains in one's allocation. In other words, there is often additional validation that is done beyond what's expressed in the schema. In addition, one might imagine additional limits that vary for different users. For example, customers might have policy limits to say "you can't have a VM larger than 4 vCPUS (i.e. because I'm cruel)" that I can imagine us supporting. To be clear: I'm ambivalent about this change, and can see the clarity this provides in the SDKs. I also see the potential additional complexity and future stumbling blocks. |
We hard code upper and lower limits for resources in several places. We should clearly communicate that those limits exist. Having a hard upper boundary doesn't mean that a submission in the acceptable range will always be successful. I don't believe making these constraints explicit will indicate that. There are going to be arbitrary computed constraints that of course shouldn't be in the schema. I understand the relative value of this is low compared to other on-going work but I think it's sweating the details that makes our product excellent. The goal here is to provide clarity at what I hope is only a marginal complexity cost. This PR can't be merged until there's a PR on typify to enable the schema to be correct here, so I'll move it back to draft for the time being. |
|
I think, eventually, it would be good for someone to tackle something along similar lines. If we have hard code limits, showing them in the schema does seem to make sense. Also, wrangling all the consts to a semi-shared place seems like it would be an improvement. |
This PR aims to encode resource limits like max memory, max vCPU, etc explicitly into the schema so that clients can auto generate validation code for those checks.
As a consequence of taking on this work I had to shuffle around where we were defining these constant limits. Schema definitions for things like vCPU count actually live in common and (most) of the limits where in app. App uses common but not vice versa so it seemed to make sense to move these values over to common. An alternative here could be to create a new top level crate called
omicron-limitsor something. I chatted with @smklein about these options and we both think the addition to common is a lighter touch.