galaxy: make retryable HTTP status codes configurable

[nejch]

SUMMARY

Extends the current list of server HTTP codes to retry client-side during galaxy installs, some of which have been listed in ansible/galaxy#3086 and the issues linked. This doesn't solve the root cause, which AFAIK should be resolved once Galaxy NG is ready. IMO these still make sense to be included by default but open to ideas.

This list is now similar to what we have in python-gitlab, which we've been extending based on feedback from users over the years:
https://github.com/python-gitlab/python-gitlab/blob/main/gitlab/const.py#L117

ISSUE TYPE

• Bugfix Pull Request

ADDITIONAL INFORMATION

For the full list of Cloudflare 52x status codes, see:
https://developers.cloudflare.com/support/troubleshooting/cloudflare-errors/troubleshooting-cloudflare-5xx-errors/#error-520-web-server-returns-an-unknown-error

/cc @dlouzan @fgreinacher

[bcoca]

First, not all those codes seem to be wise to add to 'retryable' list.

Second, since that can vary by context, why not make this an option instead (defaulting to currrent list), so the user can configure the codes as they see fit?

[nejch]

Not sure if this should be handled here at all or if we assume they'll always be validated before. Just added just in case.

[webknjaz]

Yeah, this seems like too late for the validation. But if you do keep this, make sure to chain the exception cause explicitly.

Suggested change

	try:
	retry_codes = [int(code) for code in C.GALAXY_RETRY_HTTP_ERROR_CODES]
	except ValueError as e:
	raise AnsibleError("Invalid value for HTTP retry code: %s. Only integer values are supported." % e)
	raise AnsibleError("Invalid value for HTTP retry code: %s. Only integer values are supported." % e) from e

[nejch]

First, not all those codes seem to be wise to add to 'retryable' list.

Second, since that can vary by context, why not make this an option instead (defaulting to currrent list), so the user can configure the codes as they see fit?

Thanks @bcoca @webknjaz, I added those initially as I saw 520 was on there which also doesn't necessarily mean retryable but I've removed them now 👍

I started adding a simple config item, would you be able to take another look if this is going in the right direction? Wasn't 100% sure on the config approach.

[webknjaz]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[nejch]

/azp run

@webknjaz CI is green here but I'm not sure if there's anything else to for this PR before the next round of review? 🙇

[webknjaz]

Since it's a regex, it'd be useful to be extra explicit about it:

Suggested change

	expected = "Invalid value for HTTP retry code"
	expected = r"^Invalid value for HTTP retry code: invalid\. Only integer values are supported\.$"

[webknjaz]

Is it really necessary to use string literals here?

Suggested change

	- "429" # Too many requests
	- "520" # Galaxy rate limit error code (Cloudflare unknown error)
	- "502" # Common error from galaxy that may represent any number of transient backend issues
	- 429 # Too many requests
	- 520 # Galaxy rate limit error code (Cloudflare unknown error)
	- 502 # Common error from galaxy that may represent any number of transient backend issues

[webknjaz]

@nejch I'm mostly going through the CI queue to make sure no statuses are stuck and the CI is healthy. Sometimes GitHub thinks it didn't receive some status updates and it needs to be triggered again. Hence the restarts.

You'll have to wait for somebody to do a functional review.