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

Don't treat parsing problems as async task timeout #16458

Merged
merged 4 commits into from
Jul 6, 2016

Conversation

emonty
Copy link
Contributor

@emonty emonty commented Jun 27, 2016

ISSUE TYPE
  • Bugfix Pull Request
ANSIBLE VERSION
ansible 2.1.0.0
  config file = 
  configured module search path = Default w/o overrides
SUMMARY

If there is a problem reading/writing the status file that manifests as
not being able to parse the data, that doesn't mean the task timed out,
it means there was what was likely a tempoarary problem. Move on and
keep polling for success. The only things that should cause the async
status to not be parseable are bugs in the async_runner.

@pabelanger
Copy link
Contributor

+1

@abadger
Copy link
Contributor

abadger commented Jun 27, 2016

Tentative +1 to this. We talked about it in IRC. The drawback of doing this is that parsing errors will no longer fail the task... we'll end up waiting the full timeout period instead.

@emonty we should add a comment explaining the condtions that we exit here, though, and your explanation of why we are ignoring parsing errors. (otherwise some later refactoring may remove it accidentally.)

@bcoca bcoca added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jun 28, 2016
@bcoca
Copy link
Member

bcoca commented Jun 28, 2016

looks good, but you got submodule ref updates that should not be here

If there is a problem reading/writing the status file that manifests as
not being able to parse the data, that doesn't mean the task timed out,
it means there was what was likely a tempoarary problem. Move on and
keep polling for success. The only things that should cause the async
status to not be parseable are bugs in the async_runner.
return dict(failed=True, msg="async task did not complete within the requested time")
else:
return dict(failed=True, msg="async task produced unparseable results", async_result=async_result)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious else:
Breaking the tests.

@abadger abadger merged commit 229d8f6 into ansible:devel Jul 6, 2016
abadger pushed a commit that referenced this pull request Jul 6, 2016
* Don't treat parsing problems as async task timeout

If there is a problem reading/writing the status file that manifests as
not being able to parse the data, that doesn't mean the task timed out,
it means there was what was likely a tempoarary problem. Move on and
keep polling for success. The only things that should cause the async
status to not be parseable are bugs in the async_runner.

* Add comment explaining not bailing out of loop

* Return different error when result is unparseable

* Remove extraneous else
bcoca pushed a commit that referenced this pull request Aug 15, 2016
We want to NOT consider the async task as failed if the result is
not parsed, which was the intent of:

  #16458

However, the logic doesn't actually do that because we default
the 'parsed' value to True. It should default to False so that
we continue waiting, as intended.
bcoca pushed a commit that referenced this pull request Aug 15, 2016
We want to NOT consider the async task as failed if the result is
not parsed, which was the intent of:

  #16458

However, the logic doesn't actually do that because we default
the 'parsed' value to True. It should default to False so that
we continue waiting, as intended.
(cherry picked from commit bf8c871)
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 5, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants