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

Fix ec2_instance eventual consistency when wait: false #51885

Merged
merged 5 commits into from
Mar 6, 2019
Merged

Fix ec2_instance eventual consistency when wait: false #51885

merged 5 commits into from
Mar 6, 2019

Conversation

Shaps
Copy link
Contributor

@Shaps Shaps commented Feb 7, 2019

SUMMARY

As very well described in #51662, when wait: false is specified, the module may fail because the instance ids are not yet propagated.

Fixes #51662

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ec2_instance

ADDITIONAL INFORMATION

N/A

@Shaps Shaps changed the title Ec2 instance eventual consistency Fix ec2_instance eventual consistency when wait: false Feb 7, 2019
@Shaps Shaps mentioned this pull request Feb 7, 2019
Closed
@ansibot
Copy link
Contributor

ansibot commented Feb 7, 2019

cc @ryansb
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 aws bug This issue/PR relates to a bug. cloud community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Feb 7, 2019
@Shaps
Copy link
Contributor Author

Shaps commented Feb 11, 2019

cc @willthames

@willthames
Copy link
Contributor

The code looks good, I haven't yet run the test suite though.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Feb 14, 2019
Spredzy
Spredzy approved these changes Feb 18, 2019
View reviewed changes
Copy link
Contributor

@Spredzy Spredzy left a comment

Choose a reason for hiding this comment

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

LGTM. Haven't had run the test either - but so far I can't reproduce the issue with this patch in.

It was flaky, so I'll update/feel a new ticket if I see it spawning, but as per the content of this PR I don't see how it can happen.

Thanks @Shaps !

@Spredzy
Copy link
Contributor

Spredzy commented Feb 18, 2019

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Feb 18, 2019
@willthames
Copy link
Contributor

I'm still wrestling with the test suite (there seem to be a lot of issues accumulated with the testing-policies, not blaming this individual change), I think I'm nearly there - I'll ship once this passes tests.

Thanks for confirming it fixes your issue though, that's definitely a great start!

@willthames
Fix test suite to work with ec2_instance
1b9e031 
* Additional permissions
* Enforce boto3 version
* Fix broken tests
* Improve error messages
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed shipit This PR is ready to be merged by Core labels Feb 21, 2019
@ansibot
Copy link
Contributor

ansibot commented Feb 21, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/cloud/amazon/ec2_instance.py:1375:52: bad-whitespace Exactly one space required before assignment         changed, failed, instances, failure_reason  = change_instance_state(                                                     ^
lib/ansible/modules/cloud/amazon/ec2_instance.py:1489:33: too-many-format-args Too many arguments for format string

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/ec2_instance.py:1375:51: E221 multiple spaces before operator

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Feb 21, 2019
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Feb 21, 2019
@Shaps
Copy link
Contributor Author

Shaps commented Feb 28, 2019

@willthames happy to fix the 2 linter issues, just let me know if you want both the instances and failure_reason to be printed out or just the reason, for this error:

lib/ansible/modules/cloud/amazon/ec2_instance.py:1489:33: too-many-format-args Too many arguments for format string

L1489

@willthames
Copy link
Contributor

@Shaps prefer both I think

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 1, 2019
@willthames willthames merged commit 5c6b16e into ansible:devel Mar 6, 2019
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@willthames willthames willthames approved these changes

@Spredzy Spredzy Spredzy approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 aws bug This issue/PR relates to a bug. cloud core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ec2_instance: Instance creation faces Eventual Consistency and fails
4 participants
@Shaps @ansibot @willthames @Spredzy