OpenStack datasource should not retry user-data on 404

sources/helpers/openstack.py has the exception_cb logic reversed.

Attached patch fixes it.

Note: I special cased 'meta_data.json' as this is required and we might want to have retries for this file even though we have a response code >= 400. This is not because I believe that this is the best possible approach, but because there might be environments out there that relied on the unintended retrials and I want to avoid this patch possibly causing a regression anywhere.

Patch is against 0.7.9 from Xenial (where I tested this), let me know if you need this rediffed against some current git tree.

Two more notes:
1. This saves up to 18s boot time (when no user_data, network_data.json and vendor_data.json is provided)
2. The logic in url_helper.readurl() was different before, so one might argue that it should be reverted back there and the helper/openstack.py logic be left as is ... Would need to check DataSourceScaleway.py and ec2_utils.py to ensure we don't break anything. I did not go through the commit logs to understand what the author intended with this reversion and whether it was intentional.

Curt,
Thanks for the patch.
In order to accept it you will need to sign the Canonical Contributors Agreement, see http://cloudinit.readthedocs.io/en/latest/topics/hacking.html for more information.

Also, a git merge proposal is ideal rather than a patch on the bug (see also in HACKING)

thanks!

Kurt,

Any updates on signing the CCA? Would love to see this patch land!

Paul

commit e9e86164198993aca13148872afdeebaae751c2c
Author: Scott Moser <email address hidden>
Date: Tue Sep 29 17:17:49 2015 -0400

reverted the logic in url_helper.py and broke the OpenStack datasource rety logic that was working just fine before.

It references LP: #1499869.
but that does not give me a clue why the logic was reverted in readurl().

When e9e8616 was committed, the only user of the exception_cb parameter in url_helper.readurl() was the OpenStack DS.

It called it's cb should_retry_cb, clearly assuming that a True ish return value would result in a retry and a False in an abort (reraising the exception).

This was true until e9e8616 broke it with
- if exception_cb and not exception_cb(req_args.copy(), excps[-1]):
+ if exception_cb and exception_cb(req_args.copy(), excps[-1]):
+ # if an exception callback was given it should return None
+ # a true-ish value means to break and re-raise the exception
                 break

The old logic aborted the retry loop on a False return value, the new one on True.

I thus conclude that the commit was doing the reversion by mistake as it broke the only user of it back then.

These days, there is one other user of exception_cb: DataSourceScaleway.
It uses
            exception_cb=lambda _, exc: exc.code == 404 or (
                isinstance(exc.cause, requests.exceptions.ConnectionError)
            )
which returns True on ConnectionErrors and on 404, causing it NOT to retry on 404 according to the current logi in readurl() which is probably the intended behavior. (A file not found error won't change by retrying ...)

Given the history, my preferred resolution of the issue would be to leave the OpenStack helper alone, reinstate the old logic in url_helper.readurl, update the commnet and revert the logic in DataSourceScaleway.

Merge request
https://code.launchpad.net/~kgarloff/cloud-init/+git/cloud-init/+merge/341844

An upstream commit landed for this bug.

To view that commit see the following URL:
https://git.launchpad.net/cloud-init/commit/?id=097a2967

This bug is believed to be fixed in cloud-init in 18.2. If this is still a problem for you, please make a comment and set the state back to New

Thank you.

Tracked in Github Issues as https://github.com/canonical/cloud-init/issues/2942