aurae/api/v0/ runtime.proto: use repeated fields for stringly-typed fields containing multiple items?

[matttproud]

I was looking over https://github.com/aurae-runtime/aurae/blob/main/api/v0/runtime.proto, and two instances of this caught my attention.

/// A comma-separated list of CPU IDs where the task in the control group /// can run. Dashes between numbers indicate ranges.

Would it be prudent to represent Cell.cpu_cpus and Cell.cpu_mems as repeated fields so that some of the stringly-typed input can be retired in favor of a structured data schema?

So the fields from Cell would look something like this:

repeated string cpu_cpus = 2;
...
repeated string cpu_mems = 4;

For the expression of ranges, I might consider using the oneof capability. A sketch:

message CPUSpec {
  message Range {
    string start = 1;
    string end = 2;
  }
  oneof spec {
    string id = 1;
    Range range = 2;
  }
}

repeated CPUSpec cpu_cpus = 2;

[dmah42]

this was discussed in the discord and we decided to keep it like this so it's close to the underlying API.

I agree repeated numbers might be clearer and more type safe but the format is so specific we didn't want to take things too far from that.

[matttproud]

I see. Would you want to add a note to the fields to indicate that the values should be provided as literals according to a format dictated by an external system?

[future-highway]

I agree with @dmah42 that we should mimic the underlying API, however, I was thinking of ways to have both.

We could have, for example:

message Cell {
  ...  
  oneof cpu_spec {
    string cpu_spec_native = 3;
    CPUSpecRange cpu_spec_range = 4;
  }
  ...

where native would match the underlying api, but the other choices would provide more typed options that we would convert to the native version. And we could always add more formats and it won't be a breaking change.

[dmah42]

i would prefer to have one way to define something in the API, just for simplicity.

i'm pretty torn about whether we should adhere to the underlying API. i don't think it matters a massive amount either way, but there is something nice about being able to just set repeated integers and (on our side) not needing as much validation. to convert to native it's just a join with ',' (we don't need to be sophisticated with '-' ranges).

i can't find the reasoning from the discord on why we settled on copying the underlying API. perhaps just "transparency with the cgroups v2 API" is enough.

[future-highway]

I didn't remember the discussion either, but my agreement of having a way that is equal to the underlying API has to do with familiarity (for those that have it) and docs that we can point at for those that don't.

Adding more strongly typed variants will definitely make validation more work and perhaps more complicated, so not something we need to do now, but I do think it makes the api easier to use for those not as familiar with the underlying API. As you know, wrapping a field in a oneof is not a breaking change, so we can introduce it later if we feel it's worth it (the naming might be weird though if we don't want to change those).

[dmah42]

i'm writing up a PR to implement this to make sure we understand the impact of the proposal.

[dmah42]

I updated docs for API style to indicate we want to stick to underlying format.