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.

Sign up for GitHub

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

Jump to bottom

Update inventory cache fix + tests #38229

Merged
merged 13 commits into from May 24, 2018
Merged

Update inventory cache fix + tests #38229

merged 13 commits into from May 24, 2018

Conversation

s-hertel
Copy link
Contributor

@s-hertel s-hertel commented Apr 3, 2018 •
edited

SUMMARY

#37689 should be resolved first and I will rebase. <- Done.

  • When inventory caching is on fix updating the cache when refresh_inventory or --flush-cache are used (currently inventory is updated but the cache is not)
  • Fix a bug finding terminated instances for custom hostnames (with this will find only running instances by default; use a filter if you want stopped instances)
  • Replaced jsonify in the jsonfile cache plugin to work with datetime objects
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

aws_ec2

ANSIBLE VERSION
2.6

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels Apr 3, 2018
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Apr 4, 2018
@s-hertel s-hertel force-pushed the update_inventory_cache_fix branch 6 times, most recently from 10edafb to 97f77c4 Compare April 10, 2018 14:58
@ansible ansible deleted a comment from ansibot Apr 10, 2018
@ansible ansible deleted a comment from ansibot Apr 10, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 18, 2018
@s-hertel s-hertel changed the title [WIP] Update inventory cache fix + tests Update inventory cache fix + tests Apr 19, 2018
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 19, 2018
@s-hertel
Copy link
Contributor Author

The changes to lib/ansible/plugins/inventory/aws_ec2.py should be cherry-picked to 2.5.

@ansibot ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Apr 19, 2018
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 19, 2018
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 21, 2018
willthames
willthames reviewed Apr 23, 2018
View reviewed changes
test/integration/targets/aws_ec2_inventory/playbooks/create_inventory_config.yml Outdated
- name: write inventory config file
copy:
dest: ../test.aws_ec2.yml
content: |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a template! I'm all for the occasional use of copy with content but this is a little extreme!

willthames
willthames reviewed Apr 23, 2018
View reviewed changes
...integration/targets/aws_ec2_inventory/playbooks/create_inventory_config_with_constructed.yml Outdated
prefix: 'security_groups'
- key: 'tags'
prefix: 'tag'
# why no work
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because region is not returned by DescribeInstances.

This works though:

  - key: 'placement.availability_zone[:-1]'
    prefix: aws_region

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Maybe it would be good to include a "virtual key" or something that would pull out the region automatically and put it under placement so users could have a placement.region without needing to know quite as much Python.

ryansb
ryansb suggested changes Apr 23, 2018
View reviewed changes
lib/ansible/plugins/inventory/aws_ec2.py Outdated
# is terminated those attributes no longer exist, but others may:
# if an instance-state-name filter has not been specified, find the running instances
if not any([f['Name'] == 'instance-state-name' for f in filters]):
filters.append({'Name': 'instance-state-name', 'Values': ['running']})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to assume running. I can see never collecting terminated/shutting-down instances because instances in those states will never be available https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-lifecycle.html

I think a better default would be to collect pending/running/stopping/stopped hosts, since all of those may become available during a play, or have host vars that are needed.

test/integration/targets/aws_ec2_inventory/playbooks/populate_cache.yml Outdated
vars:
images:
us-east-1: ami-4fffc834
us-east-2: ami-ea87a78f
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be done region-independently with ec2_ami_facts. For CentOS I have a canned task:

- ec2_ami_facts:
    # https://wiki.centos.org/Cloud/AWS collected 2018-01-10
    filters:
      architecture: x86_64
      product-code: aw0evgkw8e5c1q413zgy5pjce
      virtualization-type: hvm 
      root-device-type: ebs 
      name: 'CentOS Linux 7*'

@s-hertel s-hertel force-pushed the update_inventory_cache_fix branch 3 times, most recently from ca64876 to 8f70fe6 Compare April 23, 2018 23:48
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels May 9, 2018
@s-hertel s-hertel force-pushed the update_inventory_cache_fix branch from 9881d1c to 0d7b36b Compare May 17, 2018 17:32
@ansibot ansibot added affects_2.6 This issue/PR affects Ansible v2.6 and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels May 17, 2018
@ryansb ryansb merged commit cba64f5 into ansible:devel May 24, 2018
gothicx pushed a commit to gothicx/ansible that referenced this pull request Jun 9, 2018
@s-hertel @gothicx
Fix inventory plugin cache + add tests (ansible#38229)
c28f9fa 
* Fix setting the cache when refresh_cache or --flush-cache are used

* Use jsonify function that handles datetime objects in jsonfile cache plugin

* Don't access self._options directly

* Add initial integration tests for aws_ec2 inventory plugin

* Add CI alias

* Fix and add a few more unit tests

* Add integration tests for constructed

* Fix typo

* Use inventory config templates

* Collect all instances that are not terminated by default

* Create separate playbook for setting up the VPC, subnet, security group, and finding an image for the host

Create a separate playbook for removing the resources

* Allow easier grouping by region and add an example

* use a unified json encode/decode that can handle unsafe and vault
jacum pushed a commit to jacum/ansible that referenced this pull request Jun 26, 2018
@s-hertel
Fix inventory plugin cache + add tests (ansible#38229)
99a28b8 
* Fix setting the cache when refresh_cache or --flush-cache are used

* Use jsonify function that handles datetime objects in jsonfile cache plugin

* Don't access self._options directly

* Add initial integration tests for aws_ec2 inventory plugin

* Add CI alias

* Fix and add a few more unit tests

* Add integration tests for constructed

* Fix typo

* Use inventory config templates

* Collect all instances that are not terminated by default

* Create separate playbook for setting up the VPC, subnet, security group, and finding an image for the host

Create a separate playbook for removing the resources

* Allow easier grouping by region and add an example

* use a unified json encode/decode that can handle unsafe and vault
@ansible ansible locked and limited conversation to collaborators May 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@willthames willthames willthames left review comments

@bcoca bcoca bcoca left review comments

@ryansb ryansb ryansb approved these changes

Assignees
No one assigned
Labels
affects_2.6 This issue/PR affects Ansible v2.6 bug This issue/PR relates to a bug. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@s-hertel @willthames @ryansb @bcoca @ansibot