Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for pids_limit parameter in docker_container module #49319

Merged
merged 4 commits into from Dec 3, 2018
Merged

Support for pids_limit parameter in docker_container module #49319

merged 4 commits into from Dec 3, 2018

Conversation

akshay196
Copy link
Contributor

SUMMARY

This add pids_limit parameter support in docker_container module

Fixes #43337

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

docker_container

ADDITIONAL INFORMATION

Set pids limit as,

- name: Run container with limiting pids
  docker_container:
     name: example
     image: nginx:latest
     state: started
     pids_limit: 100

@ansibot
Copy link
Contributor

ansibot commented Nov 29, 2018

Hi @akshay196, thank you for submitting this pull-request!

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Nov 29, 2018

cc @danihodovic @dariko @felixfontein @jwitko @kassiansun @tbouvet
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 cloud community_review In order to be merged, this PR must follow the community review workflow. docker feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels Nov 29, 2018
@ansibot
Copy link
Contributor

ansibot commented Nov 29, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_container.py:1052:9: E303 too many blank lines (2)

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Nov 29, 2018
felixfontein
felixfontein reviewed Nov 29, 2018
View reviewed changes
lib/ansible/modules/cloud/docker/docker_container.py Outdated
@@ -408,6 +408,12 @@
description:
- Set the PID namespace mode for the container.
- Note that docker-py < 2.0 only supports 'host'. Newer versions allow all values supported by the docker daemon.
pids_limit:
description:
- Set PID limit for the container. It accepts integer value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Set PID limit for the container. It accepts integer value.
- Set PID limit for the container. It accepts an integer value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@felixfontein felixfontein mentioned this pull request Nov 29, 2018
Closed
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. labels Nov 30, 2018
felixfontein
felixfontein approved these changes Nov 30, 2018
View reviewed changes
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks good; I'll run some tests tomorrow, but I think this is ready to go!

lib/ansible/modules/cloud/docker/docker_container.py Outdated
@@ -1276,6 +1283,7 @@ def _host_config(self):
device_write_bps='device_write_bps',
device_read_iops='device_read_iops',
device_write_iops='device_write_iops',
pids_limit='pids_limit'
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit: can you add a trailing comma, so that the next time someone adds something, there's no need to modify this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thank for pointing.

@akshay196
Support for pids_limit parameter in docker_container module
501a158 
This add pids_limit parameter support in docker_container module

Fixes ansible#43337

Signed-off-by: Akshay <akshay@localhost.localdomain>
@akshay196
Add changelog for pids_limit parameter
45e10cc 
Signed-off-by: Akshay Gaikwad <akgaikwad001@gmail.com>
felixfontein
felixfontein reviewed Dec 1, 2018
View reviewed changes
lib/ansible/modules/cloud/docker/docker_container.py Outdated
@@ -1690,6 +1698,7 @@ def __init__(self, container, parameters):
self.parameters_map['device_write_bps'] = 'device_write_bps'
self.parameters_map['device_read_iops'] = 'device_read_iops'
self.parameters_map['device_write_iops'] = 'device_write_iops'
self.parameters_map['pids_limit'] = 'pids_limit'
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for not noticing this earlier, but this isn't necessary (and the four lines above aren't, either). The map is needed if the names are different (expected_* vs. *), and is not defined for most options. Can you remove this (and the four lines above)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. Thank you for letting me know. I have made the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

felixfontein
felixfontein reviewed Dec 1, 2018
View reviewed changes
test/integration/targets/docker_container/tasks/tests/options.yml
state: started
pids_limit: 10
force_kill: yes
register: pids_limit_1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you adjust the tests simliar to the ones for dns_opts, so that they also run through for docker-py < 1.10.0?

@felixfontein
Copy link
Contributor

I tested this with docker-py 1.9.0, docker-py 1.10.0, and docker-3.6.0 (by running all integration tests). With the latter two, it worked fine. With docker-py 1.9.0, it (correctly) failed because pids_limit is only supported from docker-py 1.10.0 on. It would be nice if the whole testsuite won't die in this case, but besides that everything's fine! :)

@akshay196
Remove unnecessary lines of code
24d2c85 
The map is needed if the names are different.

Signed-off-by: Akshay Gaikwad <akgaikwad001@gmail.com>
@akshay196
Update pids_limit option tests
d21bcc1 
It also run for docker-py < 1.10.0

Signed-off-by: Akshay Gaikwad <akgaikwad001@gmail.com>
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 1, 2018
felixfontein
felixfontein approved these changes Dec 1, 2018
View reviewed changes
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks good! I'll retest with docker-py 1.9.0 and 1.10.0 and if it doesn't show something strange, it's shipit time ;)

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Dec 1, 2018
@felixfontein
Copy link
Contributor

It works flawlessly!

shipit

(ah, looks like ansibot already accepts shipit when it shows up in a sentence...)

@gundalow
Copy link
Contributor

gundalow commented Dec 3, 2018

@akshay196 Thank you for this Feature PR
@felixfontein Thank you as always for your reviews.

Merged into devel for release in Ansible 2.8

@gundalow gundalow merged commit 597e449 into ansible:devel Dec 3, 2018
@akshay196
Copy link
Contributor Author

@felixfontein Thank you for review.
@gundalow Thank you for merging.

kbreit pushed a commit to kbreit/ansible that referenced this pull request Jan 11, 2019
@akshay196 @kbreit
Support for pids_limit parameter in docker_container module (ansible#…
cdd81d9 
…49319)

* Support for pids_limit parameter in docker_container module

This add pids_limit parameter support in docker_container module

Fixes ansible#43337

Signed-off-by: Akshay <akshay@localhost.localdomain>

* Add changelog for pids_limit parameter

Signed-off-by: Akshay Gaikwad <akgaikwad001@gmail.com>

* Remove unnecessary lines of code

The map is needed if the names are different.

Signed-off-by: Akshay Gaikwad <akgaikwad001@gmail.com>

* Update pids_limit option tests

It also run for docker-py < 1.10.0

Signed-off-by: Akshay Gaikwad <akgaikwad001@gmail.com>
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 cloud docker feature This issue/PR relates to a feature request. module This issue/PR relates to a module. shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support for pids_limit parameter in docker_container module
4 participants
@akshay196 @ansibot @felixfontein @gundalow