Add exponential retry on fetch

[go-dima]

What does this PR do?

• Add a retry for 500 server error using exponential backoff mechanism

[go-dima]

PR fails due to missing dependency -backoff module.
What is the correct way to add this in the project? The options I found weer requirements.txt (didn't work) or in the dockerfile itself.

[rpothier]

Hi @go-dima
Looking at the error message.
ERROR: Found 1 import issue(s) on python 2.7 which need to be resolved:
ERROR: plugins/lookup/conjur_variable.py:100:0: traceback: ImportError: No module named backoff
See documentation for help: https://docs.ansible.com/ansible/devel/dev_guide/testing/sanity/import.html

The issue seems to be that Ansible is requiring the import in a try/except block.
I just added comments on the review.

[rpothier]

This should be a try/except block as described in https://docs.ansible.com/ansible/devel/dev_guide/testing/sanity/import.html

try:
    import backoff
except ImportError as imp_exc:
    BACKOFF_IMPORT_ERROR = imp_exc
else:
    BACKOFF_IMPORT_ERROR = None

[go-dima]

I'll take a look. Thanks.

[go-dima]

@rpothier, Same result. It looks like the module isn't found, although it is supported officially.

[rpothier]

I'm looking into it also. When you say it's officially supported, do you mean by python? or Ansible?

[rpothier]

Hi @go-dima I did some research with Ansible and it looks like they do not support non standard libraries.
Ansible has ansible.module_utils.api.retry can you use that instead?

[go-dima]

Looks like ansible retry doesn't fit - it expects an exception to identify failure for retry. I'll keep looking for a suitable solution

[go-dima]

@rpothier
I decided to implement it myself, care to tae a look?
I also need an approval to run workflows now :)

[go-dima]

@rpothier
Looks like I have yml issues, although my change didn't involve such.
I'll be glad to get some help on the matter.

[rpothier]

Hi @go-dima, I tried re-running it, but same results. I'll try a baseline run.

I'm seeing the same errors on another branch, so the errors are not due to your changes.
You can check #61 to verify
the errors are the same.

[rpothier]

@go-dima I have posted #61 with a fix for the sanity errors.

[go-dima]

Thanks for the update! I'll wait for #61 and update this one.

[go-dima]

@rpothier can you re-trigger the build please? (I don't have permissions)

[rpothier]

@go-dima Did you rebase with the latest?

[rpothier]

Can you summarize the testing that was done to validate this?

[rpothier]

@go-dima Woohoo! tests all pass. Changes look good.
Please squash the commits and can you describe the testing that was done?

[go-dima]

@rpothier
Latest was merged 👍

Regarding the tests:
We work with a docker worker to run our plays, so I changed the image to include the plugin from my branch. I triggered a workflow that uses the plugin multiple times in different places, to make sure the change doesn't break the successful flow.

I don't have a proper way to make sure that the retry happens during the plugin (since I cannot force 500 reply), but I did test the code in a standalone scenario with a simulated 500 error.

Regarding the squash - I plan to perform the merge itself with 'squash and merge' option.

[rpothier]

LGTM

[go-dima]

@rpothier
I don't have write access (no surprising actually...)
Can you merge please? 🥇

[rpothier]

@go-dima Can you sign off your commit ? https://github.com/cyberark/community/blob/master/CONTRIBUTING.md#signing-off-a-commit

[go-dima]

@rpothier Done
Since I already making changes, I squashed everything to a single commit

[rpothier]

LGTM