Skip to content

API: swarm: move PidsLimit to TaskTemplate.Resources#40938

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
thaJeztah:move_pidslimit
Jun 11, 2020
Merged

API: swarm: move PidsLimit to TaskTemplate.Resources#40938
cpuguy83 merged 1 commit intomoby:masterfrom
thaJeztah:move_pidslimit

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented May 9, 2020

This is a follow up to #39882 (Add API support for PidsLimit on services), which implementation followed the Swarm API, where PidsLimit is located in ContainerSpec. This is not the desired place for this property, so moving the field to TaskTemplate.Resources in our API.

After discussing in the maintainers meeting, we decided that we should update our API to move PidsLimit to the right location before the 20.x release; a similar change should be made in the SwarmKit API (likely keeping the old field for backward compatibility, because it was merged some releases back (moby/swarmkit#2415))

depends on

addresses the API side of #28618

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

Comment thread api/types/swarm/task.go Outdated
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.

Should we name this Pids or PidsLimit ? Wasn't sure (and naming is hard)

@cpuguy83 @tonistiigi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pids

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.

updated 👍

The initial implementation followed the Swarm API, where
PidsLimit is located in ContainerSpec. This is not the
desired place for this property, so moving the field to
TaskTemplate.Resources in our API.

A similar change should be made in the SwarmKit API (likely
keeping the old field for backward compatibility, because
it was merged some releases back)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah marked this pull request as ready for review June 5, 2020 10:51
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

@cpuguy83 cpuguy83 merged commit 7fa2026 into moby:master Jun 11, 2020
@thaJeztah thaJeztah deleted the move_pidslimit branch June 11, 2020 19:46
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.

3 participants