Ecs service module health check settings #47217

stefanhorning · 2018-10-17T16:48:49Z

SUMMARY

Added health check grace period to ecs_service module. New attempt after closing this PR way back:
#39289 and none of the other PRs got merged.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

module ecs_service.py

ADDITIONAL INFORMATION

Added backwards compatibility this time with a check for the botocore version. Also makes sure the healthCheckGracePeriodSeconds param is only applied when loadbalancers are configured (as creation otherwise fails). Added some initial tests.

…time with a botocore version check and some initial testing

…ined

ansibot · 2018-10-17T16:52:55Z

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

click here for bot help

ansibot · 2018-10-17T16:52:55Z

cc @Java1Guy @Java1Guy @kaczynskid @willthames @zacblazic
click here for bot help

ansibot · 2018-10-17T16:52:56Z

@stefanhorning, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

ansibot · 2018-10-17T17:41:24Z

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/cloud/amazon/ecs_service.py:461:39: E272 multiple spaces before keyword
lib/ansible/modules/cloud/amazon/ecs_service.py:463:1: E302 expected 2 blank lines, found 1

click here for bot help

ryansb · 2018-10-17T17:58:53Z

lib/ansible/modules/cloud/amazon/ecs_service.py

+    def health_check_setable(self, params):
+        load_balancers = params.get('loadBalancers', [])
+        # check if botocore (and thus boto3) is new enough for using the healthCheckGracePeriodSeconds parameter
+        return len(load_balancers) > 0  and LooseVersion(botocore.__version__) >= LooseVersion('1.9.0')


AnsibleAWSModule has a function to check boto3/botocore versions you can use here.

Suggested change

return len(load_balancers) > 0 and LooseVersion(botocore.__version__) >= LooseVersion('1.9.0')

return len(load_balancers) > 0 and self.module.botocore_at_least('1.9.0')

stefanhorning · 2018-10-17T19:39:08Z

@ryansb applied your recommended changes, also build is green now. Anything else I should do?

ezmac · 2018-11-16T17:21:27Z

ready_for_review
@stefanhorning, thanks for your work. After reading https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md#commands, I was hoping to kick this back into the review queue, but you may need to be the one who issues the command. I'm not sure.

stefanhorning · 2018-11-16T17:32:38Z

ready_for_review

ezmac · 2018-11-22T01:02:41Z

lib/ansible/modules/cloud/amazon/ecs_service.py

@@ -466,7 +481,8 @@ def main():
            security_groups=dict(type='list'),
            assign_public_ip=dict(type='bool'),
        )),
-        launch_type=dict(required=False, choices=['EC2', 'FARGATE'])
+        launch_type=dict(required=False, choices=['EC2', 'FARGATE']),
+        health_check_grace_period_seconds=dict(required=False, type='int', default=30)


Isn't healthcheck grace period 0 seconds by default? Why would it default to 30 here?

Couldn't find any docs on what values are allowed. However I assume I had a reason setting it to 30, probably if setting it there is a minimum. It's long ago I actually implented this, but I can recheck if you want.

From https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ecs.html#ECS.Client.create_service

healthCheckGracePeriodSeconds (integer) -- The period of time, in seconds, that the Amazon ECS service scheduler should ignore unhealthy Elastic Load Balancing target health checks after a task has first started. This is only valid if your service is configured to use a load balancer. If your service's tasks take a while to start and respond to Elastic Load Balancing health checks, you can specify a health check grace period of up to 7,200 seconds during which the ECS service scheduler ignores health check status. This grace period can prevent the ECS service scheduler from marking tasks as unhealthy and stopping them before they have time to come up.

I've also checked some of my services that were created before health check grace period was added and the return for it is 0. Having 30 seconds be the default wouldn't be a "problem" for me, but it would be a change from what I think is current behavior and not what I would expect.

Yes, I saw those docs already unfortunaltely they didn't say anything about the minimum value. But if you say it's 0 I will simply change that.

However looking at the code again it's actually an optional value, hence if you don't provide the parameter health_check_grace_period_seconds it won't actually be set (no matter what the default value is). I guess I just followed the pattern of the few lines above to always set a sane default if the param is required=False.

Just to clarify, I ran through a quick test omitting health check grace period with default at 30 and it looks like it gets set even when I omit in yaml.

But I see you dropped it, so I think this looks good. I'll give it another look over shortly and put a full review in if I can.

Thanks.

ezmac

Everything looks good to me except the required botocore version.
I wouldn't have noticed except I looked at implementing this feature too. A little testing and searching through botocore commits suggested it needed 1.8.20 instead of 1.9.0. Not that it would break anything, but to be thorough :)

ezmac · 2018-11-22T16:44:42Z

lib/ansible/modules/cloud/amazon/ecs_service.py

@@ -121,6 +121,11 @@
        required: false
        version_added: 2.7
        choices: ["EC2", "FARGATE"]
+    health_check_grace_period_seconds:
+        description:
+          - Seconds to wait before health checking the freshly added/updated services. This option requires botocore >= 1.9.0.


Not sure if something else happened in 1.9.0, but I think health check grace period was added to botocore in 1.8.20.
boto/botocore@a110e8d#diff-48d02f400bc4e82bd35bf470ad45ef20R1145
This is the blame for 1.8.20 showing the addition of HealthCheckGracePeriodSeconds to ecs service. I couldn't find anything newer that would make it need 1.9.0.

ezmac · 2018-11-22T17:06:23Z

lib/ansible/modules/cloud/amazon/ecs_service.py

+    def health_check_setable(self, params):
+        load_balancers = params.get('loadBalancers', [])
+        # check if botocore (and thus boto3) is new enough for using the healthCheckGracePeriodSeconds parameter
+        return len(load_balancers) > 0 and self.module.botocore_at_least('1.9.0')


Same thing here as above. Testing with botocore 1.8.19 fails when trying to use healthcheck grace period and succeeds on 1.8.20
Otherwise, looks good.

Thanks for testing, I will update it now

ezmac

Looks good to me. Hopefully this helps.

shipit

stefanhorning · 2018-11-22T21:16:50Z

I hope so! It's probably the best reviewed handfull of lines by now, if you also include the initial PR #39289 😉

Also once this get's merged I guess we can also close #36035 and #38965

willthames

This looks good to me - there are a couple of longer lines in this PR which I'd welcome shortening just to keep consistent (they go off edge of the github PR review which makes them noticeable) but that is not a blocker.

I'll run the test suite before giving it my full approval, but if anyone does that before me, then I'm happy for this to be merged as is.

willthames · 2018-11-23T10:30:22Z

TASK [ecs_cluster : create ECS service definition with network configuration] ***
task path: /root/ansible/test/integration/targets/ecs_cluster/playbooks/roles/ecs_cluster/tasks/main.yml:417
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: root
<127.0.0.1> EXEC /bin/sh -c 'echo ~root && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390 `" && echo ansible-tmp-1542968685.9855068-144333392449390="` echo /root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390 `" ) && sleep 0'
Using module file /root/ansible/lib/ansible/modules/cloud/amazon/ecs_service.py
<127.0.0.1> PUT /root/.ansible/tmp/ansible-local-883hl1l4e9r/tmp6nkzmr_9 TO /root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/AnsiballZ_ecs_service.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/ /root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/AnsiballZ_ecs_service.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/tmp/ansible-test-coverage-r9e6n06y/coverage/injector.py /root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/AnsiballZ_ecs_service.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/ > /dev/null 2>&1 && sleep 0'
The full traceback is:
Traceback (most recent call last):
  File "/root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/AnsiballZ_ecs_service.py", line 113, in <module>
    _ansiballz_main()
  File "/root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/AnsiballZ_ecs_service.py", line 105, in _ansiballz_main
    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
  File "/root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/AnsiballZ_ecs_service.py", line 48, in invoke_module
    imp.load_module('__main__', mod, module, MOD_DESC)
  File "/usr/lib/python3.6/imp.py", line 235, in load_module
    return load_source(name, filename, file)
  File "/usr/lib/python3.6/imp.py", line 170, in load_source
    module = _exec(spec, sys.modules[name])
  File "<frozen importlib._bootstrap>", line 618, in _exec
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/tmp/ansible_ecs_service_payload_1qonwwiu/__main__.py", line 621, in <module>
  File "/tmp/ansible_ecs_service_payload_1qonwwiu/__main__.py", line 566, in main
  File "/tmp/ansible_ecs_service_payload_1qonwwiu/__main__.py", line 411, in create_service
  File "/usr/lib/python3.6/site-packages/botocore/client.py", line 320, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/usr/lib/python3.6/site-packages/botocore/client.py", line 597, in _make_api_call
    api_params, operation_model, context=request_context)
  File "/usr/lib/python3.6/site-packages/botocore/client.py", line 633, in _convert_to_request_dict
    api_params, operation_model)
  File "/usr/lib/python3.6/site-packages/botocore/validate.py", line 291, in serialize_to_request
    raise ParamValidationError(report=report.generate_report())
botocore.exceptions.ParamValidationError: Parameter validation failed:
Invalid type for parameter healthCheckGracePeriodSeconds, value: None, type: <class 'NoneType'>, valid types: <class 'int'>

fatal: [localhost]: FAILED! => {
    "changed": false,
    "module_stderr": "Traceback (most recent call last):\n  File \"/root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/AnsiballZ_ecs_service.py\", line 113, in <module>\n    _ansiballz_main()\n  File \"/root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/AnsiballZ_ecs_service.py\", line 105, in _ansiballz_main\n    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\n  File \"/root/.ansible/tmp/ansible-tmp-1542968685.9855068-144333392449390/AnsiballZ_ecs_service.py\", line 48, in invoke_module\n    imp.load_module('__main__', mod, module, MOD_DESC)\n  File \"/usr/lib/python3.6/imp.py\", line 235, in load_module\n    return load_source(name, filename, file)\n  File \"/usr/lib/python3.6/imp.py\", line 170, in load_source\n    module = _exec(spec, sys.modules[name])\n  File \"<frozen importlib._bootstrap>\", line 618, in _exec\n  File \"<frozen importlib._bootstrap_external>\", line 678, in exec_module\n  File \"<frozen importlib._bootstrap>\", line 219, in _call_with_frames_removed\n  File \"/tmp/ansible_ecs_service_payload_1qonwwiu/__main__.py\", line 621, in <module>\n  File \"/tmp/ansible_ecs_service_payload_1qonwwiu/__main__.py\", line 566, in main\n  File \"/tmp/ansible_ecs_service_payload_1qonwwiu/__main__.py\", line 411, in create_service\n  File \"/usr/lib/python3.6/site-packages/botocore/client.py\", line 320, in _api_call\n    return self._make_api_call(operation_name, kwargs)\n  File \"/usr/lib/python3.6/site-packages/botocore/client.py\", line 597, in _make_api_call\n    api_params, operation_model, context=request_context)\n  File \"/usr/lib/python3.6/site-packages/botocore/client.py\", line 633, in _convert_to_request_dict\n    api_params, operation_model)\n  File \"/usr/lib/python3.6/site-packages/botocore/validate.py\", line 291, in serialize_to_request\n    raise ParamValidationError(report=report.generate_report())\nbotocore.exceptions.ParamValidationError: Parameter validation failed:\nInvalid type for parameter healthCheckGracePeriodSeconds, value: None, type: <class 'NoneType'>, valid types: <class 'int'>\n",
    "module_stdout": "",
    "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error",
    "rc": 1
}

willthames · 2018-11-23T10:34:42Z

lib/ansible/modules/cloud/amazon/ecs_service.py

@@ -401,19 +406,24 @@ def create_service(self, service_name, cluster_name, task_definition, load_balan
            params['networkConfiguration'] = network_configuration
        if launch_type:
            params['launchType'] = launch_type
+        if self.health_check_setable(params):
+            params['healthCheckGracePeriodSeconds'] = health_check_grace_period_seconds


I suspect this would pass tests if this only set the value if the parameter wasn't None

I guess this was broken by removing the default value as requested by @ezmac

Will try to add a better check soonish.

willthames · 2018-11-23T11:05:46Z

No worries @stefanhorning - I'm just testing the fix now, I'll push it to your branch if it fixes it

willthames · 2018-11-23T11:08:32Z

rebuild_merge

willthames · 2018-11-23T12:36:15Z

Merged for inclusion in 2.8. Thanks @stefanhorning for getting this over the line and @ezmac for the reviews

* Added feature health_check_grace_period_seconds to ecs_service, this time with a botocore version check and some initial testing * Only set health_check_grace_period_seconds when loadbalancers are defined * Removed leftover commas and fix in test * Removed blank line * Minor improvements for ecs_service module * Removed default (30) for health_check_grace_period_seconds param * Changed botocore version allowed to 1.8.20 for health check param. * Fix empty healthcheck failure

stefanhorning added 3 commits October 17, 2018 18:09

Added feature health_check_grace_period_seconds to ecs_service, this …

cec0661

…time with a botocore version check and some initial testing

Only set health_check_grace_period_seconds when loadbalancers are def…

9daa196

…ined

Removed leftover commas and fix in test

208f55c

Removed blank line

5979d51

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 Oct 17, 2018

ryansb suggested changes Oct 17, 2018

View reviewed changes

ansibot removed the needs_triage Needs a first human triage before being processed. label Oct 17, 2018

Minor improvements for ecs_service module

8ecadd9

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Oct 17, 2018

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Nov 1, 2018

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 16, 2018

ezmac reviewed Nov 22, 2018

View reviewed changes

Removed default (30) for health_check_grace_period_seconds param

2d0a3dc

ansibot removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Nov 22, 2018

ezmac suggested changes Nov 22, 2018

View reviewed changes

ansibot added 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 22, 2018

Changed botocore version allowed to 1.8.20 for health check param.

324dfdc

ezmac approved these changes Nov 22, 2018

View reviewed changes

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 22, 2018

willthames reviewed Nov 22, 2018

View reviewed changes

willthames suggested changes Nov 23, 2018

View reviewed changes

ansibot added 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 23, 2018

Fix empty healthcheck failure

a9968b3

willthames approved these changes Nov 23, 2018

View reviewed changes

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 23, 2018

willthames merged commit c3b059d into ansible:devel Nov 23, 2018

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ecs service module health check settings #47217

Ecs service module health check settings #47217

stefanhorning commented Oct 17, 2018

ansibot commented Oct 17, 2018

ansibot commented Oct 17, 2018

ansibot commented Oct 17, 2018

ansibot commented Oct 17, 2018

ryansb Oct 17, 2018

stefanhorning commented Oct 17, 2018 •

edited

ezmac commented Nov 16, 2018 •

edited

stefanhorning commented Nov 16, 2018

ezmac Nov 22, 2018

stefanhorning Nov 22, 2018

ezmac Nov 22, 2018

stefanhorning Nov 22, 2018 •

edited

stefanhorning Nov 22, 2018

ezmac Nov 22, 2018

ezmac left a comment

ezmac Nov 22, 2018

ezmac Nov 22, 2018

stefanhorning Nov 22, 2018

ezmac left a comment

stefanhorning commented Nov 22, 2018 •

edited

willthames left a comment

willthames commented Nov 23, 2018

willthames Nov 23, 2018

stefanhorning Nov 23, 2018

willthames commented Nov 23, 2018

willthames commented Nov 23, 2018

willthames commented Nov 23, 2018

	return len(load_balancers) > 0 and LooseVersion(botocore.__version__) >= LooseVersion('1.9.0')
	return len(load_balancers) > 0 and self.module.botocore_at_least('1.9.0')

Ecs service module health check settings #47217

Ecs service module health check settings #47217

Conversation

stefanhorning commented Oct 17, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibot commented Oct 17, 2018

ansibot commented Oct 17, 2018

ansibot commented Oct 17, 2018

ansibot commented Oct 17, 2018

Choose a reason for hiding this comment

stefanhorning commented Oct 17, 2018 • edited

ezmac commented Nov 16, 2018 • edited

stefanhorning commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhorning Nov 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezmac left a comment

Choose a reason for hiding this comment

stefanhorning commented Nov 22, 2018 • edited

willthames left a comment

Choose a reason for hiding this comment

willthames commented Nov 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willthames commented Nov 23, 2018

willthames commented Nov 23, 2018

willthames commented Nov 23, 2018

stefanhorning commented Oct 17, 2018 •

edited

ezmac commented Nov 16, 2018 •

edited

stefanhorning Nov 22, 2018 •

edited

stefanhorning commented Nov 22, 2018 •

edited