feat(client): automatically retry on HTTP 409 Resource lock#2326
feat(client): automatically retry on HTTP 409 Resource lock#2326nejch merged 2 commits intopython-gitlab:mainfrom
Conversation
|
Unit tests might increase the confidence that the code is working as expected. Would you mind adding unit tests? |
nejch
left a comment
nejch
left a comment
There was a problem hiding this comment.
@pspacek thanks for the quick work here! Just a few nits.
Regarding unit tests and your question in #2325 (comment).
We have some vendored code from httmock for a similar reason (redirects). I think you might be able to reuse it here?
https://github.com/python-gitlab/python-gitlab/blob/main/tests/unit/helpers.py
See how it's used in the redirect tests:
https://github.com/python-gitlab/python-gitlab/blob/main/tests/unit/test_gitlab_http_methods.py#L310-L317
Let us know if you need help with that :) That's a shame about responses though.
|
And @pspacek we're also ok to go ahead without the unit tests if it's too complicated and we open a follow-up issue to resolve on our end :) |
|
Hi @pspacek! Let us know if you'd like to finish this PR, or if we should push to your branch to get this merged (either way you'll be credited as the commit author). Thanks again for the contribution! |
|
@nejch I'm sorry, I have no time to work on this. Feel free to do whatever you like. |
nejch
force-pushed
the
http-409-lock-retry
branch
from
fdb1d2c to
25446cf
Compare
| def response_callback( | ||
| response: requests.models.Response, | ||
| ) -> requests.models.Response: | ||
| """We need a callback that adds a resource lock reason only on first call""" | ||
| nonlocal retried | ||
|
|
||
| if not retried: | ||
| response.reason = "Resource lock" | ||
|
|
||
| retried = True | ||
| return response |
There was a problem hiding this comment.
We're mostly doing this because responses doesn't support mocking Response.reason as @pspacek already found out in #2325 (comment).
nejch
changed the title
Codecov Report
@@ Coverage Diff @@
## main #2326 +/- ##
=======================================
Coverage 96.13% 96.13%
=======================================
Files 84 85 +1
Lines 5534 5569 +35
=======================================
+ Hits 5320 5354 +34
- Misses 214 215 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
nejch
force-pushed
the
http-409-lock-retry
branch
from
25446cf to
1a37bab
Compare
| return False | ||
| if result.status_code in gitlab.const.RETRYABLE_TRANSIENT_ERROR_CODES: | ||
| return True | ||
| if result.status_code == 409 and "Resource lock" in result.reason: |
There was a problem hiding this comment.
Doesn't seem to work for us - could it be because Resource lock is in the response body rather than the HTTP reason (Conflict)?
…3648) ### Motivation The python-gitlab fix (python-gitlab/python-gitlab#2326) for 409 Resource lock retries checks `result.reason`, but GitLab sends `Conflict` (standard HTTP reason phrase) while `Resource lock` is in the response body. The check never matches, so retries never happen: ``` $ dda inv -- pipeline.auto-cancel-previous-pipelines [...] gitlab.exceptions.GitlabHttpError: 409: 409 Conflict: Resource lock ^ reason ^ response body ``` Example: https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1262109099 ### What does this PR do? This patch adds HTTP `409` to GitLab's API retriable transient error codes as a lightweight workaround, allowing all 409 errors to be retried when `retry_transient_errors=True` (our case). ### Additional Notes Will file an issue upstream. Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
…taDog#43648) ### Motivation The python-gitlab fix (python-gitlab/python-gitlab#2326) for 409 Resource lock retries checks `result.reason`, but GitLab sends `Conflict` (standard HTTP reason phrase) while `Resource lock` is in the response body. The check never matches, so retries never happen: ``` $ dda inv -- pipeline.auto-cancel-previous-pipelines [...] gitlab.exceptions.GitlabHttpError: 409: 409 Conflict: Resource lock ^ reason ^ response body ``` Example: https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1262109099 ### What does this PR do? This patch adds HTTP `409` to GitLab's API retriable transient error codes as a lightweight workaround, allowing all 409 errors to be retried when `retry_transient_errors=True` (our case). ### Additional Notes Will file an issue upstream. Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com> fcb5852
7 participants
Fixes: #2325