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

Adding module to manage AWS Storage gateways #40494

Open
wants to merge 6 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@slapula
Contributor

slapula commented May 21, 2018

SUMMARY

This is the first pull request of several that will add support for AWS Storage Gateway resources. This one manages the activation/deactivation of storage gateway instances. Right now, the module makes a big assumption that instance is fresh and that detected disks can be used for cache or buffer storage depending on the type of the gateway that's being defined. I'm open to alternative ways to handle this. This way just seemed to be the easiest without getting bogged down in local disk micromanagement.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

storage_gateway

ANSIBLE VERSION
ansible 2.4.1.0
  config file = None
  configured module search path = [u'/Users/asmith/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/asmith/.pyenv/versions/2.7.13/lib/python2.7/site-packages/ansible
  executable location = /Users/asmith/.pyenv/versions/2.7.13/bin/ansible
  python version = 2.7.13 (default, Sep  8 2017, 15:16:38) [GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)]

slapula added some commits May 21, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented May 21, 2018

@Constantin007 @Constantin07 @Deepakkothandan @Etherdaemon @Java1Guy @Lujeni @Madhura-CSI @MichaelBaydoun @Sodki @adq @akazakov @alachaum @amir343 @anryko @bekelchik @bpennypacker @brandond @carsongee @chenl87 @defunctio @dennisconrad @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 @nathanwebsterdotme @nerzhul @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

@s-hertel

Thanks for the work you're putting into these @slapula :-)

aws_connection_info: &aws_connection_info
aws_access_key: "{{ aws_access_key }}"
aws_secret_key: "{{ aws_secret_key }}"
#security_token: "{{ security_token }}"

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

You can use "{{ security_token | omit() }}" or "{{ security_token | default() }}" so it doesn't break for non-STS credentials.

- name: Send storage gateway activation code request
uri:
url: "http://{{ gateway_instance.instances[0].public_ip_address }}/?activationRegion={{ aws_region }}"
retries: 10

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

Bumping this up to 30 and adding delay: 5 and until: code_request is not failed allows this to pass for me.

result['gateway_arn'] = response['GatewayARN']
result['Disks'] = disks_response['Disks']
result['changed'] = True
return result

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

This return and the one on line 174 don't do anything (same for returns in delete_gateway())

result['Disks'] = disks_response['Disks']
return True
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError):
return False

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

It looks like list_gateways() returns an empty list rather than failing if no gateways exist, so I think fail_json_aws should probably be used here. Right now valid ClientError and BotoCoreError exceptions are masked. If there's a specific boto3 exception that needs to be handled, use except is_boto3_error_code(... right before line 109 .

'''
import time
import ast

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

These imports are unused.

from ansible.module_utils.aws.core import AnsibleAWSModule
from ansible.module_utils.ec2 import boto3_conn, get_aws_connection_info, AWSRetry
from ansible.module_utils.ec2 import camel_dict_to_snake_dict, boto3_tag_list_to_ansible_dict

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

Line 94 and 95 are also unused imports.

def create_gateway(client, module, params, result):
try:
gw_response = client.activate_gateway(**params)
time.sleep(15) # Need a waiter here but it doesn't exist yet in the StorageGateway API

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

Can you verify that the gateway is active here before moving on? Maybe with https://boto3.readthedocs.io/en/latest/reference/services/storagegateway.html#StorageGateway.Client.describe_gateway_information. We've been adding waiters for things here https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/aws/waiters.py and at a glance looks like this could use that.

result['gateway_arn'] = gw_response['GatewayARN']
result['Disks'] = disks_response['Disks']
result['changed'] = True
return result

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

Unused return here and on line 149.

if gateway_status:
delete_gateway(client, module, result)
module.exit_json(changed=result['changed'], gateway_arn=result['gateway_arn'])

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

Not a blocker, but it seems like returning tuples could be an alternative to passing around a reference to a result dictionary that gets modified and not returned.

ec2_instance_name: '{{resource_prefix}}-node'
ec2_instance_owner: 'integration-run-{{resource_prefix}}'
ec2_file_ami_image:
# amazon/aws-storage-gateway-file-2018-03-21

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

If there's a way to filter for these you could use ec2_ami_facts so we always get the latest ones.

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