ec2_vol_facts: moved to boto3 #43348
Conversation
|
Hi @hryamzik, Thank you for the pullrequest, just so you are aware we have a dedicated Working Group for aws. |
ansibot
added
affects_2.7
|
The test The test |
ansibot
added
ci_verified
|
The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test The test |
|
Love these formatting requirements, 2 imports instead of a single multiline one. from ansible.module_utils.ec2 import connect_to_aws, ec2_argument_spec, get_aws_connection_info, boto3_conn, HAS_BOTO3, boto3_tag_list_to_ansible_dict
from ansible.module_utils.ec2 import ansible_dict_to_boto3_filter_list, camel_dict_to_snake_dict |
ansibot
added
community_review
mkrizek
removed
the
needs_triage
ansibot
added
the
stale_ci
|
@s-hertel is there anything preventing this from being merged? |
|
cc @mattclay Is there any way to avoid stuck PRs like this? |
|
@hryamzik Since this is a community module, the best thing to do is try pinging the maintainer of the module for a review. If the maintainer is unavailable, you may be able to find others in the community to review and test. Since this is an AWS module you may want to check in the #ansible-aws IRC channel on Freenode. |
|
Hey @wimnat, can you take a look at this PR, please? |
|
|
||
| if region: | ||
| try: | ||
| connection = connect_to_aws(boto.ec2, region, **aws_connect_params) | ||
| except (boto.exception.NoAuthHandlerFound, Exception) as e: | ||
| connection = boto3_conn( |
There was a problem hiding this comment.
boto3_conn is calling module.fail_json for exceptions and the lack of region, so you can remove the exception handling and unnest this and remove the ProfileNotFound import
| import boto.ec2 | ||
| from boto.exception import BotoServerError | ||
| HAS_BOTO = True | ||
| from botocore.exceptions import ClientError |
There was a problem hiding this comment.
This module should handle BotoCoreError and ClientError. ProfileNotFound should be handled by boto3_conn and doesn't need to be imported by the module.
|
|
||
| from ansible.module_utils.basic import AnsibleModule | ||
| from ansible.module_utils.ec2 import connect_to_aws, ec2_argument_spec, get_aws_connection_info | ||
| from ansible.module_utils.ec2 import connect_to_aws, ec2_argument_spec, get_aws_connection_info, boto3_conn, HAS_BOTO3, boto3_tag_list_to_ansible_dict |
There was a problem hiding this comment.
You could update this module to AnsibleAWSModule if you wanted. That would simplify correcting the exception handling and eliminate the need to use/import all of:
connect_to_aws, ec2_argument_spec, get_aws_connection_info, boto3_conn, and HAS_BOTO3
Doing this would end up looking something like:
from ansible.module_utils.aws.core import AnsibleAWSModule
...
module = AnsibleAWSModule(argument_spec=argument_spec)
connection = module.client('ec2')
And your exception handling would look like:
try:
connection.describe_volumes(Filters=ansible_dict_to_boto3_filter_list(sanitized_filters))
except (BotoCoreError, ClientError) as e:
module.fail_json_aws(e, msg="Unable to describe volumes with filters {0}".format(sanitized_filters))
Otherwise you end up having a mess handling the different cases of what attributes exist for what exception because they don't inherit the same properties (such as, you should include e.message and e.response if they exist, but e.message does not exist on Python 3 and BotoCoreErrors never have a response attribute).
There was a problem hiding this comment.
I didn't manage to get value for region in this case and it has to be presented in module output.
There was a problem hiding this comment.
Okay, no problem either way. You could still get region via get_aws_connection_info, which is how AnsibleAWSModule gets it too.
| attachment = volume.attach_data | ||
| attachment = volume["attachments"] | ||
| tags = {} | ||
| for tag in volume["tags"]: |
There was a problem hiding this comment.
You've imported boto3_tag_list_to_ansible_dict so you could use that.
line 99: 'tags': boto3_tag_list_to_ansible_dict(volume['tags'])
|
|
||
| for volume in all_volumes["Volumes"]: | ||
| volume = camel_dict_to_snake_dict(volume) |
There was a problem hiding this comment.
Do you need camel_dict_to_snake_dict(volume, ignore_list=['Tags'])? If 'Tags' is a key in volumes it will snake_dict the tags too.
There was a problem hiding this comment.
Good point, I didn't manage find any tags issues but going to give this change a try.
|
|
||
| filters = module.params.get("filters") | ||
| # Replace filter key underscores with dashes, for compatibility, except if we're dealing with tags |
There was a problem hiding this comment.
It would be nice to have integration tests to make sure this module stays backward compatible. It's not a blocker, but there should be tests for features even for preview modules. https://github.com/ansible/community/blob/master/group-aws/integration.md#policy
There was a problem hiding this comment.
I'm working on an integration test but it's hard to compare everything so let me post results here:
current version:
"volumes": [
{
"attachment_set": {
"attach_time": "2018-02-12T12:43:22.000Z",
"device": "/dev/sda1",
"instance_id": "i-88888888888888885",
"status": "attached"
},
"create_time": "2018-02-12T12:43:22.461Z",
"encrypted": false,
"id": "vol-88888888888888888",
"iops": 100,
"region": "us-west-2",
"size": 8,
"snapshot_id": "snap-88888888888888887",
"status": "in-use",
"tags": {
"Name": "test",
},
"type": "gp2",
"zone": "us-west-2c"
}
]
new version:
"volumes": [
{
"attachment_set": {
"attach_time": "2018-02-12T12:43:22+00:00",
"delete_on_termination": true,
"device": "/dev/sda1",
"instance_id": "i-88888888888888885",
"status": "attached"
},
"create_time": "2018-02-12T12:43:22.461000+00:00",
"encrypted": false,
"id": "vol-88888888888888888",
"iops": 100,
"region": "us-west-2",
"size": 8,
"snapshot_id": "snap-88888888888888887",
"status": "in-use",
"tags": {
"Name": "test",
},
"type": "gp2",
"zone": "us-west-2c"
},
]
So as you can see date format is a bit different, is it something that should be checked/fixed?
|
|
||
| for volume in all_volumes: | ||
| volume_dict_array.append(get_volume_info(volume)) | ||
| all_volumes = connection.describe_volumes(Filters=ansible_dict_to_boto3_filter_list(sanitized_filters)) |
There was a problem hiding this comment.
This should handle pagination https://boto3.readthedocs.io/en/latest/reference/services/ec2.html#EC2.Paginator.DescribeVolumes
There was a problem hiding this comment.
I don't see any pagination support in the original code, is it something different in boto3 approach?
| volume_dict_array.append(get_volume_info(volume)) | ||
| all_volumes = connection.describe_volumes(Filters=ansible_dict_to_boto3_filter_list(sanitized_filters)) | ||
| except ClientError as e: | ||
| module.fail_json(msg=e.message, exception=traceback.format_exc()) |
There was a problem hiding this comment.
e.message does not exist on Python 3. e.response does exist for ClientErrors and should be included if so.
This is wired as I've copied setup from other module test. Any ideas how to fix this? |
|
The test |
s-hertel
added
the
needs_ci_update
|
|
||
| - name: Create test instance | ||
| ec2_instance: | ||
| name: "ansible_ec2_vol_facts_test" |
There was a problem hiding this comment.
This (and any other resources) need to be using {{ resource_prefix }} on resource names, it's part of the requirements for the CI IAM policy.
| ec2_vol: | ||
| instance: "{{ instance.instance_ids[0] }}" | ||
| volume_size: 4 | ||
| name: ansible_ec2_vol_facts_test.db |
There was a problem hiding this comment.
Same here - make sure to use {{ resource_prefix }}
| @@ -0,0 +1,2 @@ | |||
| --- | |||
There was a problem hiding this comment.
I prefer not to include effectively empty defaults and vars files
| - "v.snapshot_id is defined and v.snapshot_id == ''" | ||
| - "v.status is defined and v.status == 'in-use'" | ||
| - "v.tags.Name is defined and v.tags.Name == resource_prefix + '_ansible_ec2_vol_facts_test.db'" | ||
| - "v.tags['Tag Name with Space-and-dash'] == 'Tag Value with Space-and-dash'" |
There was a problem hiding this comment.
This test fails - this is the only reference to this tag.
|
The test |
| @@ -0,0 +1,2 @@ | |||
| cloud/aws | |||
| unsupported | |||
There was a problem hiding this comment.
It looks like testing this PR is blocked on CI updates that @s-hertel is working on.
We should wait for those updates rather than disabling the tests.
ansibot
removed
the
ci_verified
|
+1 to merge from me, and I will update CI for these when I have time. The tests look good and pass locally for me with Python 2 and 3. And it's pretty low risk since it's a preview community facts module. |
|
Merged, thanks @hryamzik |
|
Thanks to @s-hertel for the secondary testing too |
|
Thanks guys! |
s-hertel
removed
the
needs_ci_update
SUMMARY
Moved to boto3 to support role-based profiles.
ISSUE TYPE
COMPONENT NAME
ec2_vol_facts
ANSIBLE VERSION