Skip to content

API: split types for Resources Reservations and Limits#40937

Merged
tiborvass merged 1 commit intomoby:masterfrom
thaJeztah:split_resource_types
Jun 5, 2020
Merged

API: split types for Resources Reservations and Limits#40937
tiborvass merged 1 commit intomoby:masterfrom
thaJeztah:split_resource_types

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

This introduces A new type (ResourceLimit), which allows "Limits" and "Reservations" to have different options, as it's not possible to make "Reservations" for some kind of limits.

The GenericResources have been removed from the new type (and thus from Limits); the API did not handle specifying GenericResources as a Limit (only as Reservations), and this field would therefore always be empty (omitted) in the Limits case.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Copy Markdown
Member Author

@cpuguy83 @tonistiigi @tiborvass PTAL

@tonistiigi
Copy link
Copy Markdown
Member

I'm not strictly against it but I don't think this is critical for #40938 . On swarm side, the design uses the same type for limits, reservation and node description. I'd imagine when this is added on the swarm it will just be a new key in the same type with no exceptions. On swarm side it probably even makes sense to make the reservations work for it as it is just the comparison with the field in node description. On moby side, we could just ignore it by not setting the value in node description until someone has a use case.

@tonistiigi
Copy link
Copy Markdown
Member

Actually, I think I was wrong guessing the intentions of the design in swarm. There are already similar fields like this in https://github.com/docker/swarmkit/blob/master/api/types.proto#L82-L91 that are not exposed in moby afaics. If this is the model then PidsLimit should be a key directly in ResourceRequirements. But if we do think it might make sense to use it for reservations as well (in the future) then Pids directly in Resources.

@thaJeztah
Copy link
Copy Markdown
Member Author

My main motivation for this is to express what we support as Limit, and what we support as Reservation, and allow to be flexible in that. Currently the types are shared but marked with omitempty, and the field description would have to cover "don't use this for ("limit or reservation") as it's only used for ("limit" or "reservation")); in addition there's no real equivalent in OpenAPI / Swagger to express omitempty other than -x-nullable, which is also confusing if the field does not accept nil.

For example, currently the documentation looks like this;

Screenshot 2020-05-11 at 08 50 03

GenericResources is listed in both, but only supported in Reservation, not in Limit. If it's impossible on the swarm side to split these types, I think we can continue using the same type in the Swarm API (as it's currently not user-facing)

There are already similar fields like this in https://github.com/docker/swarmkit/blob/master/api/types.proto#L82-L91 that are not exposed in moby afaics.

We had a similar discussion on #37872, where some options could be set as a "limit" but not as a "reservation" (can't "reserve" swap space or swappiness ). Not sure if they're in the right place directly in Resources though (given that memory swap is a limit).

This introduces A new type (`Limit`), which allows Limits
and "Reservations" to have different options, as it's not
possible to make "Reservations" for some kind of limits.

The `GenericResources` have been removed from the new type;
the API did not handle specifying `GenericResources` as a
_Limit_ (only as _Reservations_), and this field would
therefore always be empty (omitted) in the `Limits` case.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the split_resource_types branch from a8b405c to 84748c7 Compare May 18, 2020 12:22
Comment thread api/types/swarm/task.go
}

// Limit describes limits on resources which can be requested by a task.
type Limit struct {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonistiigi @cpuguy83 I updated this from ResourceLimit to Limit as we discussed, but I kept Resources (we discussed renaming to Reservation, but the same type is used both for nodes as well as for "reservation" for Services.

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tiborvass tiborvass merged commit fa38a6c into moby:master Jun 5, 2020
@thaJeztah thaJeztah deleted the split_resource_types branch June 5, 2020 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants