Fix wrongly generated `Content-Range` header for `PATCH /api/v4/jobs/:id/trace` request
What does this MR do?
Fixes wrongly generated Content-Range
header for PATCH /api/v4/jobs/:id/trace
request.
Why was this MR needed?
Please look into #3275 (closed) for context.
Are there points in the code the reviewer needs to double check?
Does this MR meet the acceptance criteria?
-
Documentation created/updated - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Branch has no merge conflicts with master
(if you do - rebase it please)
What are the relevant issue numbers?
Closes #3275 (closed)
Merge request reports
Activity
First I've made the
Content-Range
to bebyte_start-byte_end
as requested. I've also moved this inside oftracePath
struct where it belongs (since this is a place whereoffset
andlimit
are used and we see what they are doing; by usingtracePatch.Limit()
inGitLabClient
we haven't got full image of the situation).I've also updated tests and... this shows that now we have a very strange behavior of
Content-Range
:when the trace is empty
- and we will send the patch, it will send empty patch,
0-0
asContent-Range
andContentLength
will be 0 - as expected (https://gitlab.com/gitlab-org/gitlab-runner/blob/3275-fix-content-range-for-trace-patch/network/gitlab_test.go#L837), - when we'll send a 1-byte update, the patch will contain this one byte,
0-0
asContent-Range
andContentLength
will be 1 - also expected (https://gitlab.com/gitlab-org/gitlab-runner/blob/3275-fix-content-range-for-trace-patch/network/gitlab_test.go#L838)
when the trace is not empty
- and we will send the patch (e.g.
test
), it will send first bytes,0-3
(sofirst_byte-last_byte
) asContent-Range
andContentLength
will be 3 - also expected (https://gitlab.com/gitlab-org/gitlab-runner/blob/3275-fix-content-range-for-trace-patch/network/gitlab_test.go#L802) - if we will then try to send a 1-byte update - again it will work: 1 byte (e.g.
4-4
inContent-Range
and 1 asContentLength
(https://gitlab.com/gitlab-org/gitlab-runner/blob/3275-fix-content-range-for-trace-patch/network/gitlab_test.go#L804) - but if we would try to send an empty update, then we will have empty trace,
ContentLengt
set to 0 - so as expected - butContentRange
will be...4-3
(https://gitlab.com/gitlab-org/gitlab-runner/blob/3275-fix-content-range-for-trace-patch/network/gitlab_test.go#L803)!!!
I'll look how to update
tracePatch.ContentRange()
to catch this case.- and we will send the patch, it will send empty patch,
added 1 commit
- ada72538 - Handle 0-byte patch when updating the non-empty trace
Done. @ayufan Could you review this?
Tests that help to understand the cases are
TestPatchTraceUpdatedTrace
,TestPatchTraceContentRangeAndLength
andTestPatchTraceContentRangeHeaderValues
innetwork/gitlab_test.go
.assigned to @ayufan
- Resolved by Tomasz Maczukin
- Resolved by Tomasz Maczukin
assigned to @tmaczukin
- Resolved by Tomasz Maczukin
Generally speaking,
Content-range: 0-0
should be with 1 byte because the end_byte is inclusive. So when a runner sends an empry patche for keep-alive purpose (e.g.Content-range: 100-100
,Content-range: 567-567
), it should contain a 1 byte.One idea is that pathces contain always 1 byte prefix (such as
&
or$
), and gitlab rails side discards it.Or we should separate keep-alive endpoint from patching trace.
If we're OK with forcing Runner 11.0 to use GitLab 11.0 - which I'm OK with, it's not a first time - the I would go with a specified keep-alive API endpoint.
We could then change the
tracePatch
implementation to send incremental update only if there is new output, but additionally, each 15/30/60s (better make it configurable) send a small request that will only bump theupdated_at
field. If we will not removebuild.touch if build.needs_touch?
from the PATCH endpoint, then all older Runners could still work with GitLab >= 11.0, but Runner 11.0 would be fixed.mentioned in issue #3275 (closed)
added 9 commits
- 06648a36 - Update CHANGELOG for v10.8.0-rc2
- 4e51e105 - Update CHANGELOG for v10.8.0-rc3
- cd512569 - Update CHANGELOG for v10.8.0
- 38e98cf1 - Bump version to 11.0.0
- 558a8ae0 - Do not send first PUT
- 2ac818ed - Rename CI_COMMIT_REF to CI_COMMIT_SHA
- 6ac595c8 - Update related tests
- b34ca979 - Move Content-Range header composition to network/gitlab.go
- 25e6c6a5 - Add dedicated keep-alive endpoint
Toggle commit list@ayuan I've updated the MR. Patch trace is now send only, when there is some new output. On the
forceSendInterval
instead of sending an empty patch, a request to new/api/v4/jobs/:id/keep-alive
endpoint is done.MR for the new endpoint in GitLab CE is quite small and it's at gitlab-ce!19543
assigned to @ayufan
assigned to @tmaczukin
added 1 commit
- 1bcc1801 - Use UpdateJob endpoint instead of dedicated one
@ayufan Done. It now sends a request to
UpdateJob
endpoint, without the trace, when there is no new output andforceSendInterval
was exceeded.assigned to @ayufan
added 29 commits
-
1bcc1801...c9496399 - 23 commits from branch
master
- fca7a930 - Fix Content-Range header for trace PATCH request
- a01a4646 - Handle 0-byte patch when updating the non-empty trace
- 0d424989 - Move Content-Range header composition to network/gitlab.go
- 23747451 - Add dedicated keep-alive endpoint
- b1f22be6 - Use UpdateJob endpoint instead of dedicated one
- 92c015ba - Remove unused constant
Toggle commit list-
1bcc1801...c9496399 - 23 commits from branch
added 2 commits
@ayufan Anything that should be added here?
@ayufan ping :)
- Resolved by Tomasz Maczukin
- Resolved by Tomasz Maczukin
- Resolved by Tomasz Maczukin
- Resolved by Tomasz Maczukin
- Resolved by Tomasz Maczukin
assigned to @tmaczukin
- Resolved by Tomasz Maczukin
- Resolved by Tomasz Maczukin
@ayufan Thanks for the review. I'll gather all the feedback and add fixes :)
added 2 commits
changed milestone to %11.1
added 49 commits
-
1ae69252...31dd3881 - 37 commits from branch
master
- 71ba80c6 - Fix Content-Range header for trace PATCH request
- 1675fb82 - Handle 0-byte patch when updating the non-empty trace
- aa43321e - Move Content-Range header composition to network/gitlab.go
- 4d8b0d0f - Add dedicated keep-alive endpoint
- 6071983d - Use UpdateJob endpoint instead of dedicated one
- 4de56a63 - Remove unused constant
- e7fb1480 - Fix a typo
- b378c8f3 - Rename struct to RemoteJobStateResponse
- 42d2b107 - Increase test coverage of ./network/trace.go
- bec13772 - Change the flow of PatchTrace and UpdateJob calls
- e9758ff3 - Disable sending empty PatchTrace call
- 5554c4ad - Regenerate mocks
Toggle commit list-
1ae69252...31dd3881 - 37 commits from branch
@ayufan I've pushed updates.
assigned to @ayufan
added 9 commits
- d69551e8 - Add dedicated keep-alive endpoint
- 7bc48375 - Use UpdateJob endpoint instead of dedicated one
- 8ba787ce - Remove unused constant
- 8f02564b - Fix a typo
- 1593b276 - Rename struct to RemoteJobStateResponse
- c1932118 - Increase test coverage of ./network/trace.go
- ea4d660e - Change the flow of PatchTrace and UpdateJob calls
- a1dfbdbd - Disable sending empty PatchTrace call
- e30cc67a - Regenerate mocks
Toggle commit list- Resolved by Tomasz Maczukin
- Resolved by Tomasz Maczukin
assigned to @tmaczukin
changed milestone to %11.3
added missed-deliverable label
cc @tmaczukin ?
added 95 commits
-
e30cc67a...fb161916 - 82 commits from branch
master
- d676fe37 - Fix Content-Range header for trace PATCH request
- 9e381d08 - Handle 0-byte patch when updating the non-empty trace
- c2fb033f - Move Content-Range header composition to network/gitlab.go
- f662a93f - Add dedicated keep-alive endpoint
- ae1615ec - Use UpdateJob endpoint instead of dedicated one
- b021d585 - Remove unused constant
- 107c4acc - Fix a typo
- 69b344e1 - Rename struct to RemoteJobStateResponse
- e7d1764b - Increase test coverage of ./network/trace.go
- 16eb0256 - Change the flow of PatchTrace and UpdateJob calls
- e99421ca - Disable sending empty PatchTrace call
- cd8ff13a - Regenerate mocks
- 14a1e1c2 - Remove PatchEmpty() from the interface
Toggle commit list-
e30cc67a...fb161916 - 82 commits from branch
@ayufan Done!
assigned to @ayufan
mentioned in commit 37fb9a0c