Skip to content
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

ec2_vol doesn't report volume changes properly #482

Closed
1 task done
lowlydba opened this issue Aug 30, 2021 · 4 comments · Fixed by #483
Closed
1 task done

ec2_vol doesn't report volume changes properly #482

lowlydba opened this issue Aug 30, 2021 · 4 comments · Fixed by #483
Labels
bug This issue/PR relates to a bug module module plugins plugin (any type) python3

Comments

@lowlydba
Copy link
Contributor

lowlydba commented Aug 30, 2021

Summary

When modify_volume is used but no new disk is being attached, the module incorrectly reports that no change has occurred even when disks have been modified (iops, throughput, type, etc.). This is due to the attach function overwriting the changed variable even if no disks are attached:

if volume:
volume, changed = update_volume(module, ec2_conn, volume)
else:
volume, changed = create_volume(module, ec2_conn, zone=zone)
if name:
tags['Name'] = name
final_tags, tags_changed = ensure_tags(module, ec2_conn, volume['volume_id'], 'volume', tags, module.params.get('purge_tags'))
if detach_vol_flag:
volume, changed = detach_volume(module, ec2_conn, volume_dict=volume)
elif inst is not None:
volume, changed = attach_volume(module, ec2_conn, volume_dict=volume, instance_dict=inst, device_name=device_name)
# Add device, volume_id and volume_type parameters separately to maintain backward compatibility
volume_info = get_volume_info(module, volume, tags=final_tags)
if tags_changed:
changed = True

Locally I have used the following workaround as a fix:

        if detach_vol_flag:
            volume, changed = detach_volume(module, ec2_conn, volume_dict=volume)
        elif inst is not None:
            volume, vol_attached = attach_volume(module, ec2_conn, volume_dict=volume, instance_dict=inst, device_name=device_name)

        # Add device, volume_id and volume_type parameters separately to maintain backward compatibility
        volume_info = get_volume_info(module, volume, tags=final_tags)

        if tags_changed or vol_attached:
            changed = True

I am happy to make a PR with this change if this is an acceptable route to go down.

Issue Type

Bug Report

Component Name

ec2_vol

Ansible Version

ansible 2.9.9

Collection Versions

Collection        Version
----------------- -------
amazon.aws        1.5.0

AWS SDK versions

Name: boto
Version: 2.49.0
Summary: Amazon Web Services Library
Home-page: https://github.com/boto/boto/
Author: Mitch Garnaat
Author-email: [email protected]
License: MIT
Location: /Users/john.mccall/.pyenv/versions/3.6.5/lib/python3.6/site-packages
Requires: 
Required-by: 
---
Name: boto3
Version: 1.18.31
Summary: The AWS SDK for Python
Home-page: https://github.com/boto/boto3
Author: Amazon Web Services
Author-email: 
License: Apache License 2.0
Location: /Users/john.mccall/.pyenv/versions/3.6.5/lib/python3.6/site-packages
Requires: botocore, jmespath, s3transfer
Required-by: 
---
Name: botocore
Version: 1.21.31
Summary: Low-level, data-driven core of boto 3.
Home-page: https://github.com/boto/botocore
Author: Amazon Web Services
Author-email: 
License: Apache License 2.0
Location: /Users/john.mccall/.pyenv/versions/3.6.5/lib/python3.6/site-packages
Requires: jmespath, urllib3, python-dateutil
Required-by: s3transfer, boto3

Configuration

N/A

OS / Environment

MacOS

Steps to Reproduce

amazon.aws.ec2_vol:
        region: "region-name"
        name: "disk-name"
        device_name: "device-name"
        volume_type: gp3
        volume_size: 50
        iops: 126
        throughput: 3001
        modify_volume: true

Run something similar to the above example to upgrade an existing attached gp2 volume to gp3, along with non-default iops and throughput. The module will make the changes correctly but mark the volume as "ok" instead of "changed" in the results.

Expected Results

I expect the module to report back changed: true when modifying the type, iops, etc. of an already-attached ebs volume.

Actual Results

Module reports changed: false when modifying the type, iops, etc. of an already-attached ebs volume.

Code of Conduct

  • I agree to follow the Ansible Code of Conduct
@ansibullbot
Copy link

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module needs_triage plugins plugin (any type) python3 labels Aug 30, 2021
@abikouo
Copy link
Contributor

abikouo commented Aug 31, 2021

Hi @lowlydba
Thanks for taking the time to report this issue.
You can submit a PR and we will review it.

Thanks again

@tremble
Copy link
Contributor

tremble commented Aug 31, 2021

@lowlydba,

PRs are always welcome. If you've taken "the wrong" approach we'll generally try to explain why and what the better approach would be.

A couple of things to speed up the approval/merging of the PR:

  1. For bugfixes and features, please add a changelog fragment Note: We use fragments rather than updating the main changelog doc, and the release process will merge them into the main changelog.
  2. Please update the ec2_vol integration tests to help avoid a regression creeping in over time. These tests are just an Ansible role which is run in a CI AWS account.

ansible-zuul bot pushed a commit that referenced this issue Aug 31, 2021
ec2_vol: Fix incorrectly returned changed result

SUMMARY
When modify_volume is used but no new disk is being attached, the module incorrectly reports that no change has occurred even when disks have been modified (iops, throughput, type, etc.). This is due to the attach function overwriting the changed variable even if no disks are attached.
Fixes #482
The integration test has been fixed so that when the gp3 modifications are tested, the volume is already in an attached state (previously detached) when reporting back changed. The detach tests are moved further down now, allowing this case to be properly covered by the existing assert:

  
    
      amazon.aws/tests/integration/targets/ec2_vol/tasks/tests.yml
    
    
        Lines 384 to 387
      in
      e8df917
    
    
    
    

        
          
           - name: check that volume_type has changed 
        

        
          
             assert: 
        

        
          
               that: 
        

        
          
                 - changed_gp3_volume.changed 
        
    
  


ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
ec2_vol

Reviewed-by: Alina Buzachis <None>
Reviewed-by: None <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: None <None>
ansible-zuul bot pushed a commit that referenced this issue Sep 9, 2021
ec2_vol: backport fix incorrectly returned changed result

SUMMARY
Backport version of #483
When modify_volume is used but no new disk is being attached, the module incorrectly reports that no change has occurred even when disks have been modified (iops, throughput, type, etc.). This is due to the attach function overwriting the changed variable even if no disks are attached.
Fixes #482
The integration test has been fixed so that when the gp3 modifications are tested, the volume is already in an attached state (previously detached) when reporting back changed. The detach tests are moved further down now, allowing this case to be properly covered by the existing assert:

  
    
      amazon.aws/tests/integration/targets/ec2_vol/tasks/tests.yml
    
    
        Lines 384 to 387
      in
      e8df917
    
    
    
    

        
          
           - name: check that volume_type has changed 
        

        
          
             assert: 
        

        
          
               that: 
        

        
          
                 - changed_gp3_volume.changed 
        
    
  


ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
ec2_vol
Depends-On: #491

Reviewed-by: Mark Chappell <None>
Reviewed-by: Alina Buzachis <None>
Reviewed-by: None <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this issue Sep 18, 2023
ec2_asg: Add purge_tags to AutoScalingGroups.

SUMMARY

Add purge_tags to ec2_asg module.

Fixes ansible-collections#481.
ISSUE TYPE


Feature Pull Request

COMPONENT NAME

ec2_asg
ADDITIONAL INFORMATION

There was another PR (currently closed) ansible-collections#482 - with similar functionality but I'm not sure if having modules to handle tags for individual services/modules is a good way to have this functionality. It will certainly cause increase in number of modules.
Hence tried modifying existing ec2_asg module to be able to do this.

This utilizes underlying API calls to:
https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DescribeTags.html
https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_CreateOrUpdateTags.html
https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DeleteTags.html

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Jill R <None>
Reviewed-by: Mandar Kulkarni <[email protected]>
abikouo pushed a commit to abikouo/amazon.aws that referenced this issue Sep 18, 2023
ec2_asg: Add purge_tags to AutoScalingGroups.

SUMMARY

Add purge_tags to ec2_asg module.

Fixes ansible-collections#481.
ISSUE TYPE


Feature Pull Request

COMPONENT NAME

ec2_asg
ADDITIONAL INFORMATION

There was another PR (currently closed) ansible-collections#482 - with similar functionality but I'm not sure if having modules to handle tags for individual services/modules is a good way to have this functionality. It will certainly cause increase in number of modules.
Hence tried modifying existing ec2_asg module to be able to do this.

This utilizes underlying API calls to:
https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DescribeTags.html
https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_CreateOrUpdateTags.html
https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DeleteTags.html

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Jill R <None>
Reviewed-by: Mandar Kulkarni <[email protected]>
abikouo pushed a commit to abikouo/amazon.aws that referenced this issue Oct 24, 2023
ec2_asg: Add purge_tags to AutoScalingGroups.

SUMMARY

Add purge_tags to ec2_asg module.

Fixes ansible-collections#481.
ISSUE TYPE


Feature Pull Request

COMPONENT NAME

ec2_asg
ADDITIONAL INFORMATION

There was another PR (currently closed) ansible-collections#482 - with similar functionality but I'm not sure if having modules to handle tags for individual services/modules is a good way to have this functionality. It will certainly cause increase in number of modules.
Hence tried modifying existing ec2_asg module to be able to do this.

This utilizes underlying API calls to:
https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DescribeTags.html
https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_CreateOrUpdateTags.html
https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DeleteTags.html

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Jill R <None>
Reviewed-by: Mandar Kulkarni <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug module module plugins plugin (any type) python3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants