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.

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

Add support to create or terminate AWS Elastic Map Reduce #35864

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@wmaramos

wmaramos commented Feb 7, 2018

SUMMARY
  • create or terminating EMR Clusters
  • steps (jobs spark for example) can be setup when Cluster is creating
  • because the name is not a unique field in AWS this module can't ensure the state from a cluster, so if you runs this module 10 times, 10 cluster will be created
  • basically this module only receive args and send to boto3, so any argument supported by method run_job_flow() can be used when state is create. Check http://boto3.readthedocs.io/en/latest/reference/services/emr.html#EMR.Client.run_job_flow for more information.
  • it depends on boto3
ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

emr_cluster

ANSIBLE VERSION
ansible 2.5.0 (wr-emr-cluster-find-module 0435c66e48) last updated 2018/02/07 19:10:57 (GMT -200)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/wellington.ramos/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/wellington.ramos/git/ansible/lib/ansible
  executable location = /home/wellington.ramos/git/ansible/bin/ansible
  python version = 2.7.12 (default, Nov 20 2017, 18:23:56) [GCC 5.4.0 20160609]
@ansibot

This comment has been minimized.

Contributor

ansibot commented Feb 7, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Feb 7, 2018

The test ansible-test sanity --test pep8 [?] failed with the following errors:

lib/ansible/modules/cloud/amazon/emr_cluster.py:193:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/cloud/amazon/emr_cluster.py:286:38: E231 missing whitespace after ','
lib/ansible/modules/cloud/amazon/emr_cluster.py:286:56: E231 missing whitespace after ','
lib/ansible/modules/cloud/amazon/emr_cluster.py:317:5: E303 too many blank lines (2)

The test ansible-test sanity --test validate-modules [?] failed with the following errors:

lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E105 GPLv3 license header not found in the first 20 lines of the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "access_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "access_token" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "additional_info" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "ami_version" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "auto_scaling_role" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "aws_access_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "aws_region" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "aws_secret_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "custom_ami_id" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "ec2_access_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "ec2_region" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "ec2_secret_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "ec2_url" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "kerberos_attributes" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "new_supported_products" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "profile" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "repo_upgrade_on_boot" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "scale_down_behavior" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "secret_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "security_configuration" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "security_token" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "supported_products" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "tags" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "validate_certs" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E322 "visible_to_all_users" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/emr_cluster.py:138:5: E313 RETURN is not valid YAML

The test ansible-test sanity --test yamllint [?] failed with the following error:

lib/ansible/modules/cloud/amazon/emr_cluster.py:138:5: error RETURN: syntax error: could not find expected ':'

The test ansible-test sanity --test pylint [?] failed with the following errors:

lib/ansible/modules/cloud/amazon/emr_cluster.py:203:49: undefined-variable Undefined variable 'job_flow_id'
lib/ansible/modules/cloud/amazon/emr_cluster.py:215:57: undefined-variable Undefined variable 'job_flow_id'
lib/ansible/modules/cloud/amazon/emr_cluster.py:309:50: undefined-variable Undefined variable 'traceback'

click here for bot help

@mkrizek mkrizek removed the needs_triage label Feb 9, 2018

@wmaramos wmaramos changed the title from Add support to AWS EMR to Add support to creating or terminating AWS Elastic Map Reduce Feb 9, 2018

@wmaramos wmaramos force-pushed the wmaramos:wr-emr-cluster-module branch 2 times, most recently Feb 9, 2018

@wmaramos wmaramos changed the title from Add support to creating or terminating AWS Elastic Map Reduce to Add support to create or terminate AWS Elastic Map Reduce Feb 9, 2018

@wmaramos wmaramos force-pushed the wmaramos:wr-emr-cluster-module branch Feb 9, 2018

@ansibot ansibot removed the ci_verified label Feb 9, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Feb 9, 2018

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/modules/cloud/amazon/emr_cluster.py:246:49: undefined-variable Undefined variable 'job_flow_id'
lib/ansible/modules/cloud/amazon/emr_cluster.py:258:57: undefined-variable Undefined variable 'job_flow_id'
lib/ansible/modules/cloud/amazon/emr_cluster.py:352:50: undefined-variable Undefined variable 'traceback'

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

lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E307 version_added should be 2.6. Currently 2.5

click here for bot help

@ansibot ansibot added the ci_verified label Feb 9, 2018

@wmaramos wmaramos force-pushed the wmaramos:wr-emr-cluster-module branch to e2fe202 Feb 15, 2018

@ansibot ansibot removed the ci_verified label Feb 15, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Feb 15, 2018

@Constantin007 @Constantin07 @Deepakkothandan @Etherdaemon @Java1Guy @Lujeni @Madhura-CSI @MichaelBaydoun @Sodki @adq @akazakov @alachaum @amir343 @anryko @bekelchik @bpennypacker @brandond @carsongee @defunctio @dkhenry @fiunchinho @fivethreeo @garethr @gunzy83 @gurumaia @hsingh @hyperized @iiibrad @infectsoldier @j-carl @jarv @Java1Guy @jimbydamonk @jmenga @joelthompson @jonhadfield @jonmer85 @joshsouza @jsdalton @jsmartin @kaczynskid @leedm777 @linuxdynasty @loia @lwade @MichaelBaydoun @michaeljs1990 @minichate @mjschultz @mmochan @nadirollo @nand0p @naslanidis @nickball @orthanc @piontas @pjodouin @prasadkatti @psykotox @pwnall @raags @rickmendes @roadmapper @ryansydnor @scicoin-project @scottanderson42 @shepdelacreme @silviud @steynovich @tastychutney @tedder @tgerla @timmahoney @tombamford @tsiganenok @viper233 @whiter @willricardo @wilvk @wimnat @zacblazic @zbal @zeekin @zimbatm

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

state:
description:
- ensure cluster is create or terminate
- default create

This comment has been minimized.

@s-hertel

s-hertel Apr 6, 2018

Contributor

State should have the choices 'present' or 'absent' to be like other Ansible modules.

description: list of the applications installed on this cluster
type: list
tags:
description: list of tags

This comment has been minimized.

@s-hertel

s-hertel Apr 6, 2018

Contributor

It should be a dict, flattened like other AWS Ansible modules. You can use boto3_tag_list_to_ansible_dict for the transformation.

'''
try:
from botocore.exception import ClientError

This comment has been minimized.

@s-hertel

s-hertel Apr 6, 2018

Contributor

Should also handle BotoCoreError

import traceback
import time
from ansible.module_utils.basic import AnsibleModule

This comment has been minimized.

@s-hertel

s-hertel Apr 6, 2018

Contributor

This can be updated to AnsibleAWSModule from ansible.module_utils.aws.core

That will also mean you can remove the imports and uses of get_aws_connection_info, boto3_conn, and HAS_BOTO3. You can create the client with connection = module.client('emr') (which does not need exception handling around it)

if not cluster_id:
module.fail_json(msg='cluster_id required for state absent')
client.terminate_job_flows(JobFlowIds=[cluster_id])

This comment has been minimized.

@s-hertel

s-hertel Apr 6, 2018

Contributor

This needs BotoCoreError and ClientError exception handling:

except (BotoCoreError, ClientError) as e:
    module.fail_json_aws(e, msg="Unable to terminate job flows for cluster {0}".format(cluster_id))
client.terminate_job_flows(JobFlowIds=[cluster_id])
resource = client.describe_cluster(ClusterId=cluster_id)['Cluster']

This comment has been minimized.

@s-hertel

s-hertel Apr 6, 2018

Contributor

Needs exception handling here too.

resource = client.describe_cluster(ClusterId=cluster_id)['Cluster']
if wait:

This comment has been minimized.

@s-hertel

s-hertel Apr 6, 2018

Contributor

There is a built-in waiter you can use for this: http://boto3.readthedocs.io/en/latest/reference/services/emr.html#EMR.Waiter.ClusterTerminated

You can modify the waiter config to wait the correct amount of time by adding the WaiterConfig keyword:

waiter.wait(
    ClusterId=cluster_id,
    WaiterConfig={'Delay': 5, 'MaxAttempts': wait_timeout // 5}
)['Cluster']
resource = client.describe_cluster(ClusterId=job_flow_id)['Cluster']
if wait:

This comment has been minimized.

params = snake_dict_to_camel_dict(params, capitalize_first=True)
job_flow_id = client.run_job_flow(**params)['JobFlowId']

This comment has been minimized.

@s-hertel

s-hertel Apr 6, 2018

Contributor

This needs exception handling.

@ansibot ansibot added affects_2.6 and removed stale_ci labels May 17, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented May 18, 2018

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

lib/ansible/modules/cloud/amazon/emr_cluster.py:281:32: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/cloud/amazon/emr_cluster.py:281:119: undefined-variable Undefined variable 'emr'
lib/ansible/modules/cloud/amazon/emr_cluster.py:313:34: undefined-variable Undefined variable 'job_flow_id'
lib/ansible/modules/cloud/amazon/emr_cluster.py:314:57: undefined-variable Undefined variable 'job_flow_id'

The test ansible-test sanity --test validate-modules [explain] failed with 12 errors:

lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E324 Value for "default" from the argument_spec ('EMR_DefaultRole') for "service_role" does not match the documentation (None)
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E324 Value for "default" from the argument_spec ('EMR_EC2_DefaultRole') for "job_flow_role" does not match the documentation (None)
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E324 Value for "default" from the argument_spec ('emr-5.11.0') for "release_label" does not match the documentation (None)
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E324 Value for "default" from the argument_spec ('present') for "state" does not match the documentation (None)
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E324 Value for "default" from the argument_spec (30) for "wait_delay" does not match the documentation (None)
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E324 Value for "default" from the argument_spec (60) for "wait_max_attempts" does not match the documentation (None)
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E324 Value for "default" from the argument_spec (True) for "visible_to_all_users" does not match the documentation (False)
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E325 argument_spec for "ebs_optimized" defines type="bool" but documentation does not
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E325 argument_spec for "visible_to_all_users" defines type="bool" but documentation does not
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E325 argument_spec for "wait" defines type="bool" but documentation does not
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E326 Value for "choices" from the argument_spec (['instance_hour', 'task_completion']) for "scale_down_behavior" does not match the documentation ([])
lib/ansible/modules/cloud/amazon/emr_cluster.py:0:0: E326 Value for "choices" from the argument_spec (['present', 'create', 'absent']) for "state" does not match the documentation ([])

click here for bot help

@ansibot ansibot added the ci_verified label May 18, 2018

@ansibot ansibot added the stale_ci label May 26, 2018

@s-hertel

@wmaramos Thanks for updating some things and using the waiter! Are you able to fix the things the Shippable found (that the bot commented)? Also, as per policy, all new modules after 2.5 need integration tests https://github.com/ansible/community/blob/master/group-aws/integration.md#policy. They don't need to be complicated, but should test creating/modifying/removing a cluster. Here's an example of the integration test structure for the ec2_group module: https://github.com/ansible/ansible/tree/devel/test/integration/targets/ec2_group

If you need any help with anything, feel welcome to ping me on IRC (username shertel).

ebs_volumes:
description:
- list to create extra volumes see section EbsBlockDeviceConfigs in
http://boto3.readthedocs.io/en/latest/reference/services/emr.html#EMR.Client.run_job_flow

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

Rather than link to other docs, you can document complex options.

ebs_volumes:
    description: A list of volumes to create
    type: complex
    contains:
        VolumeSpecification:
            description: a dict ...
            type: complex
            contains:
                VolumeType:
                    ...
                Iops:
                    ...
                SizeInGB:
        VolumesPerInstance: ...

Then in the argument spec this would look like:

ebs_volumes=dict(
    type='list', elements='dict', options=dict(
        VolumeSpecification=dict(type='dict', elements='dict', options=dict(
            VolumeType=dict(....<document like normal options>),
            Iops=dict(....),
            SizeInGB=dict(....),
        )
        VolumesPerInstance=dict(...),
))

Here's a module example you can look at:

listeners=dict(type='list',
elements='dict',
options=dict(
Protocol=dict(type='str', required=True),
Port=dict(type='int', required=True),
SslPolicy=dict(type='str'),
Certificates=dict(type='list'),
DefaultActions=dict(type='list', required=True)

Another nice thing would be to add aliases to the suboptions, so people can provide VolumesPerInstance as the usual snake_case volumes_per_instance, since that is the typical pattern for Ansible AWS modules.

state:
description:
- ensure cluster is create or terminate
- default present

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

- default present isn't the same as default: present. The latter gets parsed correctly. You'll fix a lot of the shippable errors by correcting that. All the others too. You should also document choices for the options that have them. So it should look like:

state:
  description:
    - Whether the cluster should exist or be absent
  default: present
  choices: ['absent', 'present']
supported_products:
description:
- a list set by third-party when job is launched.
ec2_tags:

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

This should be called tags for consistency with the other AWS modules. It also appears tags are only changed/added by this module when first creating the job flow but it should be modifiable since the API supports this with add_tags() and remove_tags(). Here are some general guidelines for dealing with tags. https://github.com/ansible/ansible/blob/bf304832ffb8fc723c4d2f755e2e45ba17809f73/lib/ansible/modules/cloud/amazon/GUIDELINES.md#dealing-with-tags

Here are a couple module examples, though there are many:

desired_tags = module.params.get('tags')
if desired_tags is not None:
current_tags = boto3_tag_list_to_ansible_dict(image.get('Tags'))
tags_to_add, tags_to_remove = compare_aws_tags(current_tags, desired_tags, purge_tags=module.params.get('purge_tags'))
if tags_to_remove:
try:
connection.delete_tags(Resources=[image_id], Tags=[dict(Key=tagkey) for tagkey in tags_to_remove])
changed = True
except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
module.fail_json_aws(e, msg="Error updating tags")
if tags_to_add:
try:
connection.create_tags(Resources=[image_id], Tags=ansible_dict_to_boto3_tag_list(tags_to_add))
changed = True
except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
module.fail_json_aws(e, msg="Error updating tags")

try:
cur_tags = describe_tags_with_backoff(connection, resource_id)
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
module.fail_json_aws(e, msg='Unable to list tags for VPC')
to_add, to_delete = compare_aws_tags(cur_tags, tags, purge_tags)
if not to_add and not to_delete:
return {'changed': False, 'tags': cur_tags}
if check_mode:
if not purge_tags:
tags = cur_tags.update(tags)
return {'changed': True, 'tags': tags}
if to_delete:
try:
connection.delete_tags(Resources=[resource_id], Tags=[{'Key': k} for k in to_delete])
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
module.fail_json_aws(e, msg="Couldn't delete tags")
if to_add:
try:
connection.create_tags(Resources=[resource_id], Tags=ansible_dict_to_boto3_tag_list(to_add))
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
module.fail_json_aws(e, msg="Couldn't create tags")

custom_ami_id=dict(required=False),
repo_upgrade_on_boot=dict(required=False),
ebs_root_volume_size=dict(required=False, type='int'),
state=dict(required=False, choices=['present', 'create', 'absent'], default='present'),

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

state shouldn't be an action. This should just have the choices present and absent.

pass # handled by AnsibleAWSModule
class ElasticMapReduceError(Exception):

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

This never gets raised so you can remove it and where it's used in main().

changed = False
if len(response) > 1:
module.warn(warning='More than one Cluster match with name {} just only the first will be describe'.format(emr['Name']))

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

This should have {0} rather than {} for Python 2.6 compatibility.

module.fail_json(msg='name is required')
try:
emr_clusters = client.list_clusters(ClusterStates=['STARTING',

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

This has a paginator that should be used in case there are too many clusters to return with list_clusters() alone. https://boto3.readthedocs.io/en/latest/reference/services/emr.html#EMR.Paginator.ListClusters

name = module.params.get('name')
if not name:
module.fail_json(msg='name is required')

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

Rather than fail here, you can use a required_if in AnsibleAWSModule. Same for cluster_id for removing the cluster:

... = AnsibleAWSModule(...
                       required_if=[
                           ['state', 'present', ['name']],
                           ['state', 'absent', ['cluster_id']],
                       ]
)
except (ClientError, BotoCoreError, WaiterError) as e:
module.fail_json_aws(e)
changed = True

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

What if the cluster has been deleted in the previous playbook run? This should be idempotent. Right now it looks like it will always attempt to delete it, and with most AWS services trying to delete something that isn't there fails.

applications:
description: list of the applications installed on this cluster
type: list
ec2_tags:

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

This should also be returned as tags for consistency with other AWS modules. And it shouldn't be a list of [{'Key', name1, 'Value', value1}, {'Key': name2, 'Value': value2},], but a dictionary like {name1: value1, name2: value2}. You can use the boto3_tag_list_to_ansible_dict function from ansible.module_utils.ec2 to make that transformation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment