Skip to content
Snippets Groups Projects

Fix wrongly generated `Content-Range` header for `PATCH /api/v4/jobs/:id/trace` request

Merged Tomasz Maczukin requested to merge 3275-fix-content-range-for-trace-patch into master
All threads resolved!

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)

Edited by Kamil Trzciński

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Kamil Trzciński
  • 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.

  • @dosuken123 No. We should probably send patch without trace, or use something different for keep-alive purposes.

  • Author Maintainer

    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 the updated_at field. If we will not remove build.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.

  • If we change drastically in 11.0, probably it's worth creating a new database column for tracking keep alive status. updated_at is a mixed concept as it can be updated by other reasons. For example, last_pinged_at is more explicit than that.

  • mentioned in issue #3275 (closed)

  • Tomasz Maczukin added 9 commits

    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

    Compare with previous version

  • Author Maintainer

    @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

  • added 1 commit

    • 1bcc1801 - Use UpdateJob endpoint instead of dedicated one

    Compare with previous version

  • Tomasz Maczukin resolved all discussions

    resolved all discussions

  • Author Maintainer

    @ayufan Done. It now sends a request to UpdateJob endpoint, without the trace, when there is no new output and forceSendInterval was exceeded.

  • assigned to @ayufan

  • Tomasz Maczukin added 29 commits

    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

    Compare with previous version

  • Tomasz Maczukin added 2 commits

    added 2 commits

    • 62f494de - Use UpdateJob endpoint instead of dedicated one
    • ee20e976 - Remove unused constant

    Compare with previous version

  • Author Maintainer

    I've cleaned up the commits and added one update in the tests, to better simulate real usage.

  • Author Maintainer

    @ayufan Anything that should be added here?

  • Author Maintainer

    @ayufan ping :)

  • Generally, it seems OK. I have a bunch of comments.

  • Kamil Trzciński
  • Author Maintainer

    @ayufan Thanks for the review. I'll gather all the feedback and add fixes :)

  • Tomasz Maczukin added 2 commits

    added 2 commits

    Compare with previous version

  • Tomasz Maczukin changed milestone to %11.1

    changed milestone to %11.1

  • Tomasz Maczukin resolved all discussions

    resolved all discussions

  • Tomasz Maczukin added 4 commits

    added 4 commits

    • a3901926 - Rename struct to RemoteJobStateResponse
    • 57f69faa - Increase test coverage of ./network/trace.go
    • 83318e31 - Change the flow of PatchTrace and UpdateJob calls
    • 1ae69252 - Disable sending empty PatchTrace call

    Compare with previous version

  • Tomasz Maczukin added 49 commits

    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

    Compare with previous version

  • Author Maintainer

    @ayufan I've pushed updates.

  • assigned to @ayufan

  • Tomasz Maczukin added 9 commits

    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

    Compare with previous version

  • Kamil Trzciński
  • Jason Yavorska changed milestone to %11.3

    changed milestone to %11.3

  • Tomasz Maczukin added 95 commits

    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

    Compare with previous version

  • Tomasz Maczukin resolved all discussions

    resolved all discussions

  • added 1 commit

    • db196768 - Remove PatchEmpty() from the interface

    Compare with previous version

  • Author Maintainer

    @ayufan Done!

  • assigned to @ayufan

  • Kamil Trzciński marked the checklist item Documentation created/updated as completed

    marked the checklist item Documentation created/updated as completed

  • Kamil Trzciński marked the checklist item Added for this feature/bug as completed

    marked the checklist item Added for this feature/bug as completed

  • Kamil Trzciński marked the checklist item All builds are passing as completed

    marked the checklist item All builds are passing as completed

  • Kamil Trzciński approved this merge request

    approved this merge request

  • Kamil Trzciński marked the checklist item Branch has no merge conflicts with master (if you do - rebase it please) as completed

    marked the checklist item Branch has no merge conflicts with master (if you do - rebase it please) as completed

  • mentioned in commit 37fb9a0c

  • Please register or sign in to reply
    Loading