Add Amazon Elastic File System (efs) module #2305

ryansydnor · 2016-05-25T18:03:17Z

ISSUE TYPE

New Modules Pull Request

COMPONENT NAME

efs (elastic file system)
efs_facts (elastic file system facts)

ANSIBLE VERSION

Tested against: ansible 2.1.1.0

SUMMARY

Adds the ability to create, manage, and delete elastic file systems

gregdek · 2016-05-25T22:25:13Z

Thanks @ryansydnor for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

ryansydnor · 2016-05-27T00:11:42Z

@gregdek - are the travis errors show stoppers?

gregdek · 2016-05-27T04:07:56Z

Looks like we're trying to do a simple boto3 detection in travis and failing, so I would say no.

sivel · 2016-05-27T11:35:14Z

Is the boto import needed? New modules are not permitted to have a dependency on boto2.

ryansydnor · 2016-05-27T20:15:35Z

No. I was testing on an older version (1.9.5) and mixing boto/boto3 to connect. I've removed all dependencies on boto and tested against ansible 2.1 and boto3. Looks good!

ryansydnor · 2016-05-30T19:14:41Z

@gregdek - I believe we can remove the "needs_revision" label and add "community_review" (or equivalent).

gregdek · 2016-06-01T21:31:37Z

Thanks @ryansydnor :)

jonhadfield · 2016-06-29T12:39:48Z

Hi @ryansydnor

EFS has just gone live in eu-west-1 so it would be good to get this functionality added.

I'm testing this against latest devel:(ansible/ansible@216a845)
and on the first run I get the error below. The next run succeeds and subsequent runs are successfully idempotent. Deletion also works successfully. Will try and debug if I get time.

Traceback (most recent call last):
  File "/var/folders/00/drbkx48d6cjd_pmvdnd3dq1w0000gn/T/ansible_7BJtvE/ansible_module_efs.py", line 689, in <module>
    main()
  File "/var/folders/00/drbkx48d6cjd_pmvdnd3dq1w0000gn/T/ansible_7BJtvE/ansible_module_efs.py", line 659, in main
    changed = connection.converge_file_system(name=name, tags=tags, targets=targets) or changed
  File "/var/folders/00/drbkx48d6cjd_pmvdnd3dq1w0000gn/T/ansible_7BJtvE/ansible_module_efs.py", line 432, in converge_file_system
    **targets[sid]
  File "/usr/local/var/pyenv/versions/2.7.10/lib/python2.7/site-packages/botocore/client.py", line 278, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/usr/local/var/pyenv/versions/2.7.10/lib/python2.7/site-packages/botocore/client.py", line 572, in _make_api_call
    raise ClientError(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (IncorrectFileSystemLifeCycleState) when calling the CreateMountTarget operation: None

fatal: [localhost]: FAILED! => {"changed": false, "failed": true, "invocation": {"module_name": "efs"}, "module_stderr": "Traceback (most recent call last):\n  File \"/var/folders/00/drbkx48d6cjd_pmvdnd3dq1w0000gn/T/ansible_7BJtvE/ansible_module_efs.py\", line 689, in <module>\n    main()\n  File \"/var/folders/00/drbkx48d6cjd_pmvdnd3dq1w0000gn/T/ansible_7BJtvE/ansible_module_efs.py\", line 659, in main\n    changed = connection.converge_file_system(name=name, tags=tags, targets=targets) or changed\n  File \"/var/folders/00/drbkx48d6cjd_pmvdnd3dq1w0000gn/T/ansible_7BJtvE/ansible_module_efs.py\", line 432, in converge_file_system\n    **targets[sid]\n  File \"/usr/local/var/pyenv/versions/2.7.10/lib/python2.7/site-packages/botocore/client.py\", line 278, in _api_call\n    return self._make_api_call(operation_name, kwargs)\n  File \"/usr/local/var/pyenv/versions/2.7.10/lib/python2.7/site-packages/botocore/client.py\", line 572, in _make_api_call\n    raise ClientError(parsed_response, operation_name)\nbotocore.exceptions.ClientError: An error occurred (IncorrectFileSystemLifeCycleState) when calling the CreateMountTarget operation: None\n", "module_stdout": "", "msg": "MODULE FAILURE", "parsed": false}

I'm using the following playbook:

---
- name: Test EFS Module - Create
  hosts: localhost
  connection: local
  gather_facts: False
  tasks:
  - name: create efs
    efs:
      state: present
      name: my_efs
      #tags:
      #    Name: efs_name
      targets:
         - SubnetId: subnet-97bc71e1
           SecurityGroups: ["sg-60197c07"]
         - SubnetId: subnet-55eb3131
           SecurityGroups: ["sg-60197c07", "sg-cd197caa"]
    register: my_efs_details

  - name: efs creation output
    debug: var=my_efs_details

- name: Test EFS Module - Delete
  hosts: localhost
  connection: local
  gather_facts: False
  tasks:
  - name: delete efs
    efs:
      state: absent
      name: my_efs
    register: my_efs_deletion_details

  - name: efs deletion output
    debug: var=my_efs_deletion_details

ryansydnor · 2016-07-02T09:57:53Z

Hi @jonhadfield!

First of all, thanks so much for testing!

Unfortunately, I'm not going to be able to investigate as I'm on vacation with limited access to computers until August.

I wonder if the API changed between beta and release leading to that exception, as it did not occur for me while testing (I have a similar set of test playbooks). I'd also be curious if we tested with different versions of boto3 (this PR could potentially be a culprit : boto/botocore@36df38d)

I'd greatly appreciate if you could help me look in to it. Completely understand if you cannot, but expect no progress on this PR from me for about a month.

jonhadfield · 2016-07-04T19:40:57Z

The issue is that you're calling 'create_mount_target()' without checking the state being available first.
You only exit 'create_file_system()' with state of the filesystem being equal to available if wait is set to True. You'll see I didn't have wait set in my example playbooks, so it defaulted to False.

        if self.wait:
            wait_for(
                lambda: self.get_file_system_state(name),
                self.STATE_AVAILABLE,
                self.wait_timeout
            )

I'm assuming you've set wait to True in your test cases?
This is why it would always succeed on subsequent runs in my tests as it only took a few seconds to become available and that was at least the gap between runs.

I can think of a number of fixes, but not currently got time to try them out.
Are there any scenarios where you wouldn't want to wait for the filesystem being available considering the short amount of time it takes?
Alternatively, before you create targets, you could add another wait to ensure the filesystem is available?

ryansydnor · 2016-07-15T15:27:57Z

@jonhadfield - thanks for the debugging help! You were spot-on with wait: True in my test.

To answer your question:

Are there any scenarios where you wouldn't want to wait for the filesystem being available considering the short amount of time it takes?

No.

I don't think the added complexity of a wait here is worthwhile. I've removed it and updated the PR. Let me know if things are working for you now.

Thanks!

jonhadfield · 2016-07-23T13:55:02Z

Creation now working, but deletion is only working by id and not name.
Just returns without any errors.

ryansydnor · 2016-08-01T16:37:57Z

@jonhadfield - I am not able to reproduce. Deletion with wait:yes and wait:no using both ID and name are working for me. Can you show me what you're doing that is failing?

Code snippets:

- name: Setting test efs instance name
  set_fact:
    efs_instance_name: "efs-test-{{ 99999 | random | to_uuid }}"

- name: Creating test EFS instance
  efs:
    state: present
    name: "{{ efs_instance_name }}"
    wait: yes
  register: result

- name: Saving ID of EFS instance
  set_fact:
    efs_instance_id: "{{ result.efs.FileSystemId }}"

- name: Deleting test EFS instance
  efs:
    state: absent
    id: "{{ efs_instance_id }}"
    #name: "{{ efs_instance_name }}"
    wait: no
    #wait: yes
  register: result

ryansydnor · 2016-08-19T14:14:19Z

@gregdek - is there anything else I can do to push this along outside of wait for community members to test and sign off?

ryansb · 2016-08-23T20:22:11Z

Works for me - 🚢 ship_it 🚢

ryansb · 2016-08-23T20:23:10Z

cloud/amazon/efs.py

+        description:
+            - Allows to create, search and destroy Amazon EFS file system
+        required: true
+        choices: ['present', 'absent', 'list']


I think list would be better off as its own facts module, not a "state" on this one.

gregdek · 2016-08-24T22:26:53Z

@ryansydnor A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

ryansydnor · 2016-08-26T15:36:02Z

@gregdek - addressed the comments from ryansb. split the "list" functionality into its own module (included in this PR for testing purposes).

fvant · 2016-09-13T12:50:51Z

Can state please be optional and default to present as with other modules ?

And can you add the performanceMode parameter too ?

fvant · 2016-09-13T13:27:47Z

cloud/amazon/efs_facts.py

+
+DOCUMENTATION = '''
+---
+module: efs


Done. Thanks for pointing that out.

Use snake_case instead of CamelCase Remove unused code Update docs Make python 2.6 compatible Add performanceMode parameter

ryansydnor · 2016-09-13T15:37:23Z

@fvant - state is now optional (default to present). added performance mode as a parameter.

thanks again for taking a look!

fvant · 2016-09-13T21:26:28Z

shipit

paprins · 2016-09-14T07:43:01Z

LGTM

tima · 2016-09-14T16:22:00Z

@paprins: The ansibots doesn't understand LGTM. ;)

shipit

gregdek · 2016-09-14T16:31:47Z

Thanks again to @ryansydnor for this PR, and thanks @ansible/core for reviewing. Marking for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

ryansb · 2016-09-14T17:21:30Z

cloud/amazon/efs_facts.py

+
+    def __init__(self, module, region, **aws_connect_params):
+        try:
+            session = Session(


You need to be using boto3_conn from EC2 module_utils, this connection method doesn't work for users that have multiple AWS profiles set up.

ryansb · 2016-09-14T19:51:25Z

cloud/amazon/efs_facts.py

+    file_systems_info = connection.get_file_systems(FileSystemId=fs_id, CreationToken=name)
+
+    if tags:
+        file_systems_info = filter(lambda item: has_tags(item['Tags'], tags), file_systems_info)


i'd really prefer [item for item in file_systems_info if has_tags(item['Tags'], tags)] here. Filter with lambdas is a bit clunky.

ryansb · 2016-09-14T19:51:42Z

cloud/amazon/efs_facts.py

+    if targets:
+        targets = [(item, prefix_to_attr(item)) for item in targets]
+        file_systems_info = filter(lambda item:
+                                   has_targets(item['MountTargets'], targets), file_systems_info)


Same here - list comprehension would be cleaner.

ryansb · 2016-09-14T19:54:16Z

cloud/amazon/efs_facts.py

+                                   has_targets(item['MountTargets'], targets), file_systems_info)
+
+    file_systems_info = [camel_dict_to_snake_dict(x) for x in file_systems_info]
+    module.exit_json(changed=False, efs=file_systems_info)


For facts modules we encourage returning your info as part of ansible_facts so users don't have to register your output, for example:

module.exit_json(changed=False, ansible_facts={'efs': file_systems_info})

That way usage could look like:

- efs_facts: profile: my-aws-account - command: "echo my EFS has {{ efs.number_of_mount_targets }} targets associated"

ryansb · 2016-09-14T19:55:21Z

cloud/amazon/efs.py

+            CreationToken=name,
+            FileSystemId=file_system_id
+        ))
+        return info and info['LifeCycleState'] or self.STATE_DELETED


To avoid this boolean you can instead use info.get('LifeCycleState', self.STATE_DELETED)

ryansb · 2016-09-14T20:02:46Z

cloud/amazon/efs.py

+        'security_groups': 'SecurityGroups',
+        'subnet_id': 'SubnetId'
+    }
+    targets = [dict((target_translations[key], value) for (key, value) in x.items()) for x in module.params.get('targets')]


Can you add comments to explain what's going on here and what data goes in/out of this?

ryansb · 2016-09-14T20:03:54Z

cloud/amazon/efs.py

+
+    def __init__(self, module, region, **aws_connect_params):
+        try:
+            session = Session(


Same boto3_conn connection comment as below. Just replace with:

try: - session = Session( - aws_access_key_id=aws_connect_params['aws_access_key_id'], - aws_secret_access_key=aws_connect_params['aws_secret_access_key'], - aws_session_token=aws_connect_params['aws_session_token'], - region_name=region - ) - self.connection = session.client('efs') + self.connection = boto3_conn(module, conn_type='client', + resource='efs', region=region, + **aws_connect_params) except Exception as e: - module.fail_json(msg=repr(e)) + module.fail_json(msg="Failed to connect to AWS " + str(e), exception=traceback.format_exc(e))

ryansydnor · 2016-09-14T23:10:22Z

@gregdek & @ryansb - what's going on with the waiting_on_contributor label?

Are ryansb's most recent comments blocking? Or should we move forward and revisit later?

ryansb · 2016-09-16T20:28:39Z

Going to go ahead with shipping this, but I'd really appreciate if you could follow up on these changes in another PR

ryansb · 2016-09-16T20:38:02Z

Merged as 412af42 and c03e26b

tima · 2016-09-16T21:27:52Z

Nice work @ryansydnor. Thanks.

wimnat · 2016-09-17T01:27:03Z

@ryansydnor @ryansb state should also be explicit not optional - core team request from a while back. Also, there isn't a single try catch block around any of the boto3 calls.... :(

tima · 2016-09-17T05:06:06Z

@wimnat I see #2305 (comment). Where's the core team request from awhile ago?

wimnat · 2016-09-17T10:43:21Z

@tima i had a look around and couldn't find it. It'll be either in the google devel group or a comment on a PR somewhere! Pretty much untraceable but i didn't imagine it :)

I'll do a PR to update the docs

gundalow · 2016-10-19T18:49:12Z

Did a follow up PR get raised, I don't see a link here

gregdek added cloud new_plugin needs_revision labels May 25, 2016

gregdek added community_review and removed needs_revision labels Jun 1, 2016

ryansb added the aws label Jul 8, 2016

gregdek removed the aws label Jul 8, 2016

gregdek mentioned this pull request Aug 19, 2016

Standing Agenda, New Modules Meeting ansible/community#92

Closed

ryansb reviewed Aug 23, 2016
View reviewed changes

ryansb added needs_revision and removed community_review labels Aug 24, 2016

fvant reviewed Sep 13, 2016
View reviewed changes

Clean up efs and efs_facts based on community comments

98d5e93

Use snake_case instead of CamelCase Remove unused code Update docs Make python 2.6 compatible Add performanceMode parameter

gregdek added shipit and removed community_review labels Sep 14, 2016

ryansb reviewed Sep 14, 2016
View reviewed changes

abadger removed the pending_action label Sep 14, 2016

ryansb reviewed Sep 14, 2016

View reviewed changes

ryansb added the waiting_on_contributor label Sep 14, 2016

gregdek removed the waiting_on_contributor label Sep 14, 2016

ryansb closed this Sep 16, 2016

Add Amazon Elastic File System (efs) module #2305

Add Amazon Elastic File System (efs) module #2305

Conversation

ryansydnor commented May 25, 2016 • edited

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

SUMMARY

gregdek commented May 25, 2016

ryansydnor commented May 27, 2016

gregdek commented May 27, 2016

sivel commented May 27, 2016

ryansydnor commented May 27, 2016

ryansydnor commented May 30, 2016

gregdek commented Jun 1, 2016

jonhadfield commented Jun 29, 2016 • edited

ryansydnor commented Jul 2, 2016

jonhadfield commented Jul 4, 2016

ryansydnor commented Jul 15, 2016

jonhadfield commented Jul 23, 2016 • edited

ryansydnor commented Aug 1, 2016 • edited

ryansydnor commented Aug 19, 2016

ryansb commented Aug 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregdek commented Aug 24, 2016

ryansydnor commented Aug 26, 2016

fvant commented Sep 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryansydnor commented Sep 13, 2016

fvant commented Sep 13, 2016

paprins commented Sep 14, 2016

tima commented Sep 14, 2016

gregdek commented Sep 14, 2016

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

Choose a reason for hiding this comment

ryansydnor commented Sep 14, 2016

ryansb commented Sep 16, 2016

ryansb commented Sep 16, 2016

tima commented Sep 16, 2016

wimnat commented Sep 17, 2016

tima commented Sep 17, 2016

wimnat commented Sep 17, 2016

gundalow commented Oct 19, 2016

ryansydnor commented May 25, 2016 •

edited

jonhadfield commented Jun 29, 2016 •

edited

jonhadfield commented Jul 23, 2016 •

edited

ryansydnor commented Aug 1, 2016 •

edited

ryansb commented Aug 23, 2016 •

edited

fvant commented Sep 13, 2016 •

edited