Register scalable target prior to creating/deleting a scaling policy #35632

chenl87 · 2018-02-01T23:23:18Z

SUMMARY

Added essential step of creating scalable target registration before attempting to create (and delete)
a scaling policy.

up until now, this module has always failed with the following message (example message):
"An error occurred (ObjectNotFoundException) when calling the PutScalingPolicy operation: No scalable target registered for service namespace: ecs, resource ID: service/poc-pricing/test-as, scalable dimension: ecs:service:DesiredCount"

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

aws_application_scaling_policy

ADDITIONAL INFORMATION

From boto3 documentation:
"
Registers or updates a scalable target. A scalable target is a resource that Application Auto Scaling can scale out or scale in. After you have registered a scalable target, you can use this operation to update the minimum and maximum values for its scalable dimension.

After you register a scalable target, you can create and apply scaling policies using PutScalingPolicy
"

http://boto3.readthedocs.io/en/latest/reference/services/application-autoscaling.html#ApplicationAutoScaling.Client.register_scalable_target

ansibot · 2018-02-01T23:26:49Z

cc @gurumaia @willthames
click here for bot help

ansibot · 2018-02-04T15:50:41Z

The test ansible-test sanity --test ansible-doc --python 2.6 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/aws_application_scaling_policy.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/aws_application_scaling_policy.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/aws_application_scaling_policy.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/aws_application_scaling_policy.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [?] failed with the following error:

lib/ansible/modules/cloud/amazon/aws_application_scaling_policy.py:0:0: has a documentation error formatting or is missing documentation.

click here for bot help

chenl87 · 2018-02-04T16:57:40Z

ready_for_review

chenl87 · 2018-02-06T15:07:09Z

@gurumaia , Could you please take a look at the changes?
Thanks!

s-hertel · 2018-02-07T18:45:22Z

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

@@ -170,7 +178,56 @@ def delete_scaling_policy(connection, module):
        except Exception as e:
            module.fail_json(msg=str(e), exception=traceback.format_exc())

-    module.exit_json(changed=changed)
+    result = {"changed": changed}


You could just return {'changed': changed}

s-hertel · 2018-02-07T18:46:08Z

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

+def create_scalable_target(connection, module):
+    changed = False
+
+    scalable_targets = connection.describe_scalable_targets(


This call needs exception handling.

s-hertel · 2018-02-07T18:51:10Z

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

+                MinCapacity=module.params.get('minimum_tasks'),
+                MaxCapacity=module.params.get('maximum_tasks')
+            )
+        except Exception as e:


It looks like the rest of the module is using except Exception so you've probably done this for consistency, but it would be better to catch only the specific exceptions, which would be the botocore.exceptions ClientError and BotoCoreError. Same for the one below. At some point the other 'except Exception's need to be removed.
Guidelines on exception handling: https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/GUIDELINES.md#using-fail_json-and-avoiding-ansiblemodule_utilsawscore

s-hertel · 2018-02-07T18:52:52Z

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

+    else:
+        snaked_response = {}
+
+    result = {"changed": changed, "response": snaked_response}


You could just return this dict, same below.

s-hertel · 2018-02-07T18:59:02Z

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

@@ -284,11 +344,19 @@ def main():
    except botocore.exceptions.ProfileNotFound as e:
        module.fail_json(msg=str(e))

+    # A scalable target must be registered prior to creating a scaling policy
+    scalable_target_result = create_scalable_target(connection, module)


For backwards compatibility should there be a way to turn this on or off?

s-hertel · 2018-02-07T19:00:04Z

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


+    # return the scalable target result only if it has changed and there were no policy changes
+    if scalable_target_result['changed'] is True and policy_result['changed'] is False:
+        module.exit_json(**scalable_target_result)


The contents of this needs to be added to the RETURN doc.

…ption handling

chenl87 · 2018-02-20T18:28:25Z

ready_for_review

chenl87 · 2018-02-21T12:39:19Z

@s-hertel , thanks for the review and the comments. I've pushed the necessary fixes and some other enhancements. I'd really appreciate if you could give it another look.

chenl87 · 2018-03-05T10:26:49Z

@s-hertel @ryansb , any chance you guys can review it anytime soon?
I'm already running with this in our production environment and I would really prefer to run with an Ansible devel installation instead of my fork.

Thanks!

s-hertel

This looks great @chenl87, thanks for the patience. Feel free to add yourself as an author, and then you'll get updates and be able to use the power of "shipit" to help get community contributions merged.

s-hertel · 2018-03-21T17:29:35Z

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

    ))

-    module = AnsibleModule(argument_spec=argument_spec, supports_check_mode=True)
+    module = AnsibleAWSModule(argument_spec=argument_spec, supports_check_mode=True)

    if not HAS_BOTO3:


AnsibleAWSModule checks this for you so you can remove this and the import.

s-hertel · 2018-03-21T17:33:32Z

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


    if not HAS_BOTO3:
-        module.fail_json(msg='boto3 is required.')
+        module.fail_json_aws(msg='boto3 is required.')

    try:
        region, ec2_url, aws_connect_kwargs = get_aws_connection_info(module, boto3=True)
        if not region:


boto3_conn handles this for you. You can remove this, as well as the try/except around boto3_conn. It handles ProfileNotFound. Since you're using AnsibleAWSModule you can replace this entire try/except with: connection = module.client('application-autoscaling').

ansibot · 2018-03-21T20:16:25Z

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

lib/ansible/modules/cloud/amazon/aws_application_scaling_policy.py:472:0: trailing-whitespace Trailing whitespace

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

lib/ansible/modules/cloud/amazon/aws_application_scaling_policy.py:472:1: W293 blank line contains whitespace

click here for bot help

chen-leibovich added 3 commits February 1, 2018 23:10

Added missing scalable target creation

e6b4eb3

Changed if statement

95fea77

Added support to results of all actions

89b1673

mkrizek removed the needs_triage Needs a first human triage before being processed. label Feb 2, 2018

Fixed line lengths, whitespaces and blank lines between functions

01f5ab7

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

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Feb 4, 2018

Fixed documentation formatting

191ae74

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. labels Feb 4, 2018

ryansb self-requested a review February 7, 2018 18:31

s-hertel reviewed Feb 7, 2018

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 15, 2018

Work in progress, fixed returns from functions, still need to do exce…

c59ec0c

…ption handling

fixed unnecessary blank line

ab204ad

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. labels Feb 20, 2018

ansibot added bug This issue/PR relates to a bug. 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 bugfix_pull_request labels Mar 1, 2018

s-hertel approved these changes Mar 21, 2018

View reviewed changes

removed imports and unnecessary checks handled by AnsibleAWSModule

c1904c5

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 21, 2018

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 Mar 21, 2018

removed whitespace

5f2d287

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. labels Mar 21, 2018

ansible deleted a comment from ansibot Mar 22, 2018

ansible deleted a comment from chenl87 Mar 22, 2018

ansible deleted a comment from ansibot Mar 22, 2018

ryansb approved these changes Mar 22, 2018

View reviewed changes

ryansb merged commit e501134 into ansible:devel Mar 22, 2018

ansible locked and limited conversation to collaborators Apr 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register scalable target prior to creating/deleting a scaling policy #35632

Register scalable target prior to creating/deleting a scaling policy #35632

chenl87 commented Feb 1, 2018

ansibot commented Feb 1, 2018

ansibot commented Feb 4, 2018

chenl87 commented Feb 4, 2018

chenl87 commented Feb 6, 2018

s-hertel Feb 7, 2018

s-hertel Feb 7, 2018

s-hertel Feb 7, 2018

s-hertel Feb 7, 2018

s-hertel Feb 7, 2018

s-hertel Feb 7, 2018

chenl87 commented Feb 20, 2018

chenl87 commented Feb 21, 2018

chenl87 commented Mar 5, 2018

s-hertel left a comment

s-hertel Mar 21, 2018

s-hertel Mar 21, 2018

ansibot commented Mar 21, 2018

Register scalable target prior to creating/deleting a scaling policy #35632

Register scalable target prior to creating/deleting a scaling policy #35632

Conversation

chenl87 commented Feb 1, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibot commented Feb 1, 2018

ansibot commented Feb 4, 2018

chenl87 commented Feb 4, 2018

chenl87 commented Feb 6, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenl87 commented Feb 20, 2018

chenl87 commented Feb 21, 2018

chenl87 commented Mar 5, 2018

s-hertel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented Mar 21, 2018