Skip to content
Snippets Groups Projects

Introduce GCS adapter for remote cache

Merged Tomasz Maczukin requested to merge introduce-gcs-cache-support into master
1 unresolved thread

What does this MR do?

Adds GCS support for caching mechanism

Why was this MR needed?

GCS doesn't work well with minio-go library, because of differences in S3 protocol implementations. This MR adds support for GCS using the GCS own API to create presigned URLs for uploading and downloading.

Are there points in the code the reviewer needs to double check?

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #1773 (closed)

Edited by Tomasz Maczukin

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
  • Tomasz Maczukin resolved all discussions

    resolved all discussions

  • Tomasz Maczukin resolved all discussions

    resolved all discussions

  • Tomasz Maczukin added 6 commits

    added 6 commits

    • afb1eab0 - Introduce modular structure for shells/cache
    • 28cfc722 - Add GCS cache adapter
    • b63dcd9a - Update dependencies
    • baf09f6c - Support GCS credentials from file
    • 3088b59f - Update documentation
    • 998e1ebd - Refactor cache.Adapter interface

    Compare with previous version

  • Tomasz Maczukin added 4 commits

    added 4 commits

    • f9cb5d17 - Support GCS credentials from file
    • 8c63f05a - Update documentation
    • 4db514c6 - Refactor cache.Adapter interface
    • ad20934a - Print explicitly that there is no URL and cache will be not uploaded/downloaded

    Compare with previous version

  • Author Maintainer

    @ilyaf Are you still interested in testing this? If yes, then please wait for the pipeline to become green. After this you can download the binary/package from https://gitlab-runner-downloads.s3.amazonaws.com/introduce-gcs-cache-support/index.html. And the documentation preview is at http://review-docs-intro-xleo88.178.62.207.141.xip.io/runner/configuration/advanced-configuration.html#the-runners-cache-gcs-section.

    If you will need a docker image (don't know how your setup looks like), then let me know - I'll restart the pipeline with option to store the image at https://gitlab.com/gitlab-org/gitlab-runner/container_registry :wink:

  • Tomasz Maczukin added 9 commits

    added 9 commits

    • ad20934a...f9a9b5f4 - 2 commits from branch master
    • 689da775 - Introduce modular structure for shells/cache
    • ca011e7f - Add GCS cache adapter
    • 75b42ca3 - Update dependencies
    • b39893a0 - Support GCS credentials from file
    • 0608ac13 - Update documentation
    • 08af3482 - Refactor cache.Adapter interface
    • dbf3795a - Print explicitly that there is no URL and cache will be not uploaded/downloaded

    Compare with previous version

  • Tomasz Maczukin marked as a Work In Progress

    marked as a Work In Progress

  • Tomasz Maczukin changed the description

    changed the description

  • Author Maintainer

    Set back the WIP status, since I want to add one change before it will be merged. I'll update this until the end of this week and then remove the WIP :)

  • Tomasz Maczukin added 20 commits

    added 20 commits

    • dbf3795a...80396350 - 11 commits from branch master
    • 88bb6c7b - Introduce modular structure for shells/cache
    • 41bf1e92 - Add GCS cache adapter
    • ca8cc042 - Update dependencies
    • f55656a6 - Support GCS credentials from file
    • 77c02181 - Update documentation
    • 41252bca - Refactor cache.Adapter interface
    • c0915ff5 - Print explicitly that there is no URL and cache will be not uploaded/downloaded
    • 087d67db - Add BucketName explicitly to gcs config struct
    • c0ba6ccc - Create dedicated section for S3 cache configuration

    Compare with previous version

    • Author Maintainer
      Resolved by Alessio Caiazza

      OK, added changes that I wanted.

      I've introduced a [runners.cache.s3] configuration section. This will make things consistent with freshly introduced GCS configuratn, and it will make the configuration cleaner in the future (which will be especially useful, if we'll introduce another cache adapter).

      I've made current configuration format (with S3 options directly in [runners.cache]) deprecated. After we'll merge this MR, we should create an issue, pointed to %12.0, that will contain the cleanup part.

      I'm removing the WIP status - the MR may be reviewed again :wink:

  • Tomasz Maczukin unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Tomasz Maczukin changed milestone to %11.2

    changed milestone to %11.2

  • mentioned in issue #1773 (closed)

  • added 1 commit

    • eeb5e66f - Create dedicated section for S3 cache configuration

    Compare with previous version

  • Tomasz Maczukin changed the description

    changed the description

  • added 1 commit

    • 6e21a483 - Create dedicated section for S3 cache configuration

    Compare with previous version

  • Author Maintainer

    Interesting. In the MR it stays that there is no code quality changes, while I clearly see gocyclo warnings in code quality report.

    I'll fix them later today.

  • Author Maintainer

    @axil Could you take a look on documentation changes added in this MR? :)

  • Alessio Caiazza
  • Interesting. In the MR it stays that there is no code quality changes, while I clearly see gocyclo warnings in code quality report.

    I'll fix them later today.

    That's because we are showing only diffs from master, and we already merged those gocyclo offenders :(

  • @tmaczukin thanks! I pushed some edits for the docs :)

  • added 1 commit

    • 995e3a62 - Copyedit docs for GCS cache support

    Compare with previous version

  • Tomasz Maczukin changed milestone to %11.3

    changed milestone to %11.3

  • Tomasz Maczukin added 6 commits

    added 6 commits

    • 9b1be095 - Refactor cache.Adapter interface
    • f26723b0 - Print explicitly that there is no URL and cache will be not uploaded/downloaded
    • cd715a7c - Add BucketName explicitly to gcs config struct
    • e36e08af - Create dedicated section for S3 cache configuration
    • 8f4e80eb - Copyedit docs for GCS cache support
    • cd422efd - Refactorize structure and interface of ./shells/cache/...

    Compare with previous version

  • Tomasz Maczukin added 20 commits

    added 20 commits

    • cd422efd...edd7ee23 - 9 commits from branch master
    • 3b5368c0 - Introduce modular structure for shells/cache
    • ec528347 - Add GCS cache adapter
    • 330905f9 - Update dependencies
    • d3480624 - Support GCS credentials from file
    • 7fa65eff - Update documentation
    • 11773a8b - Refactor cache.Adapter interface
    • e9f1efac - Print explicitly that there is no URL and cache will be not uploaded/downloaded
    • b6b6fa4f - Add BucketName explicitly to gcs config struct
    • e027c5ec - Create dedicated section for S3 cache configuration
    • b4a5f937 - Copyedit docs for GCS cache support
    • 6b8f471e - Refactorize structure and interface of ./shells/cache/...

    Compare with previous version

  • Tomasz Maczukin added 20 commits

    added 20 commits

    • 6b8f471e...fb161916 - 9 commits from branch master
    • 147a7b3b - Introduce modular structure for shells/cache
    • 3943b641 - Add GCS cache adapter
    • cbd13b1b - Update dependencies
    • fc5cd3c9 - Support GCS credentials from file
    • 24e4375b - Update documentation
    • 190f35c5 - Refactor cache.Adapter interface
    • 9c4c03ea - Print explicitly that there is no URL and cache will be not uploaded/downloaded
    • 4451eb1c - Add BucketName explicitly to gcs config struct
    • 75656b81 - Create dedicated section for S3 cache configuration
    • fa209417 - Copyedit docs for GCS cache support
    • bbd835c5 - Refactorize structure and interface of ./shells/cache/...

    Compare with previous version

  • added 1 commit

    • 61d62b4b - Refactorize structure and interface of ./shells/cache/...

    Compare with previous version

  • @tmaczukin whats the status of this MR? Are we still on track for %11.3

  • Author Maintainer

    @nolith We should be able to merge this into %11.3. I've moved lot of things around to make it cleaner and still testable and just need to confirm that this is the final state that I'd like to work on.

  • Author Maintainer

    I'll try to push finall version before the Summit ends :)

  • Tomasz Maczukin added 40 commits

    added 40 commits

    • 61d62b4b...d06a9aa3 - 28 commits from branch master
    • 2f2abb41 - Introduce modular structure for shells/cache
    • 357bb53c - Add GCS cache adapter
    • d346e749 - Update dependencies
    • 94e35637 - Support GCS credentials from file
    • df4ebcab - Update documentation
    • 23180186 - Refactor cache.Adapter interface
    • fc8017ea - Print explicitly that there is no URL and cache will be not uploaded/downloaded
    • 0a0ba2d5 - Add BucketName explicitly to gcs config struct
    • 90bf4022 - Create dedicated section for S3 cache configuration
    • 58f31801 - Copyedit docs for GCS cache support
    • d690ff41 - Refactorize structure and interface of ./shells/cache/...
    • 32df4c77 - Move cache package out of shell package

    Compare with previous version

  • Tomasz Maczukin added 11 commits

    added 11 commits

    • c7a142d7 - Add GCS cache adapter
    • e7e9a98f - Update dependencies
    • d2c8c108 - Support GCS credentials from file
    • cf641c71 - Update documentation
    • 864e4fdd - Refactor cache.Adapter interface
    • 643ae12d - Print explicitly that there is no URL and cache will be not uploaded/downloaded
    • ac0b0bc9 - Add BucketName explicitly to gcs config struct
    • 3cce6df0 - Create dedicated section for S3 cache configuration
    • 3ad5c2bd - Copyedit docs for GCS cache support
    • 4b48d8d7 - Refactorize structure and interface of ./shells/cache/...
    • cf6a9b9c - Move cache package out of shell package

    Compare with previous version

  • Tomasz Maczukin added 10 commits

    added 10 commits

    • 649d98e6 - Support GCS credentials from file
    • 26475160 - Update documentation
    • 3f46641d - Refactor cache.Adapter interface
    • 263d4592 - Print explicitly that there is no URL and cache will be not uploaded/downloaded
    • 37ebf586 - Add BucketName explicitly to gcs config struct
    • 0b2dff30 - Create dedicated section for S3 cache configuration
    • a2e74780 - Copyedit docs for GCS cache support
    • 9f550be2 - Refactorize structure and interface of ./shells/cache/...
    • 6b30c15f - Move cache package out of shell package
    • 682cd202 - Update dependencies

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Tomasz Maczukin added 5 commits

    added 5 commits

    • 7c978d59 - Update docs with proper version for GCS support
    • aa77d3ba - Refactorize cache code
    • 39b5cc80 - Refactorize S3 cache adapter
    • aa14bb76 - Improve cache/adapter tests
    • d40af893 - Improve GCS adapter tests

    Compare with previous version

  • Author Maintainer

    @nolith It's ready for a next review!

    Few words of comments - while I can agree that some things I've proposed initially could be simplified I definitely can't agree with a statement that testability is not important.

    First, a possibility to write unit tests that are indeed testing the feature, not the feature's dependencies, from a long time is understood as a prove that the code has a good shape. But what's more important I remember many problems in the past caused because we:

    • didn't have tests (because it was too hard or even impossible to write them),
    • were testing not the things that we should (relaying on dependencies and their output instead of checking the behavior of our code).

    One of the reasons of why we still are finding many different problem that are hard to track is that we have currently only a ~50% of code coverage (https://gitlab.com/gitlab-org/gitlab-runner/-/jobs/91089797) - which is drastically small - and that we have a lot of the code messed together without any interface separation. Adding a new cache adapter was a good reason to refactorize the whole cache mechanism and make it easier to maintain in the future, and while during this I wanted to not introduce problems mentioned above.

    Having all of the above in mind - including your comments - I've updated the MR and:

    • moved some things around to put them in better places,
    • hidden things that should be internal and are not a part of the interfaces of cache mechanism nor cache adapters,
    • hidden mechanisms used for making the code more testable (so we will not have any global, public, mutable state that could be changed somewhere else than in tests happening internally in the package).

    What's also important in the new state of this MR is that I've moved the whole cache package and its subpackages out from the shell package. The fact that we're currently using this only from shell with the presigned URL feature doesn't mean that cache mechanism is part of a shell.

  • assigned to @nolith

  • Tomasz Maczukin added 8 commits

    added 8 commits

    • 00a21a8f - Refactorize structure and interface of ./shells/cache/...
    • 2cee2f6f - Move cache package out of shell package
    • f075e88f - Update dependencies
    • 42bf8742 - Update docs with proper version for GCS support
    • 4ed29e7d - Refactorize cache code
    • 006b194f - Refactorize S3 cache adapter
    • 2e9364fd - Improve cache/adapter tests
    • 12265ca6 - Improve GCS adapter tests

    Compare with previous version

  • Author Maintainer

    How good we have https://gitlab.com/gitlab-org/gitlab-runner/-/jobs/92303707 now :wink:

    I've fixed code style offenses.

    • Resolved by Alessio Caiazza

      Can we consider reducing the package name depth?

      1. I think that cache/adapter can be merged into cache
      2. adapter initialization should be done in main.go like we are already doing for executors (this will also remove the need to add a comment to make the linter happy)
      3. instead​ of using cache/adapters/provider use just cache/provider
  • @tmaczukin I've left some comments, please do not mark them as resolved, having them around will help me reviewing your changes.

  • Tomasz Maczukin mentioned in merge request !336 (closed)

    mentioned in merge request !336 (closed)

  • Tomasz Maczukin added 3 commits

    added 3 commits

    • 1f0d5174 - Improve naming, slightly refactor tests
    • f0dba647 - Change logrus mocking in tests
    • 612daeac - Reduce depth of cache package

    Compare with previous version

  • Tomasz Maczukin changed the description

    changed the description

  • Author Maintainer

    @nolith All comments addressed. Pipeline still passes green :wink:

  • assigned to @nolith

  • Alessio Caiazza
  • Alessio Caiazza
  • mentioned in issue #3473 (closed)

  • Thanks @tmaczukin, great job! I've left a couple of comments.

  • Tomasz Maczukin added 19 commits

    added 19 commits

    • 6f8b522c - Support GCS credentials from file
    • 3c1e5666 - Update documentation
    • fa6db101 - Refactor cache.Adapter interface
    • 289affb9 - Print explicitly that there is no URL and cache will be not uploaded/downloaded
    • d568792a - Add BucketName explicitly to gcs config struct
    • 3beda3fe - Create dedicated section for S3 cache configuration
    • a1efd003 - Copyedit docs for GCS cache support
    • a0872574 - Refactorize structure and interface of ./shells/cache/...
    • bc30159c - Move cache package out of shell package
    • 6a048bf1 - Update dependencies
    • c33d089f - Update docs with proper version for GCS support
    • 805262e0 - Refactorize cache code
    • f032534c - Refactorize S3 cache adapter
    • e6bca4f8 - Improve cache/adapter tests
    • b5fd0d0a - Improve GCS adapter tests
    • 6a4811ec - Improve naming, slightly refactor tests
    • 32aa86c6 - Change logrus mocking in tests
    • cde5fa67 - Reduce depth of cache package
    • e49e28e6 - Raplace syscall.Unlink with os.Remove

    Compare with previous version

  • Tomasz Maczukin added 14 commits

    added 14 commits

    • 16aab0cf - Create dedicated section for S3 cache configuration
    • 8d8600a2 - Copyedit docs for GCS cache support
    • 7c1c10dc - Refactorize structure and interface of ./shells/cache/...
    • 3f4e2460 - Move cache package out of shell package
    • f604694f - Update dependencies
    • 0eae36b2 - Update docs with proper version for GCS support
    • d9bef9aa - Refactorize cache code
    • 7d49aebf - Refactorize S3 cache adapter
    • b205a3ca - Improve cache/adapter tests
    • 2fa431a6 - Improve GCS adapter tests
    • ef68565d - Improve naming, slightly refactor tests
    • b1268e4a - Change logrus mocking in tests
    • 2f440bc2 - Reduce depth of cache package
    • a6b95027 - Raplace syscall.Unlink with os.Remove

    Compare with previous version

  • Author Maintainer

    @nolith Updated as requested :)

  • assigned to @nolith

  • Alessio Caiazza resolved all discussions

    resolved all discussions

  • Alessio Caiazza approved this merge request

    approved this merge request

  • Thanks @tmaczukin :rocket: it LGTM

  • Alessio Caiazza enabled an automatic merge when the pipeline for a6b95027 succeeds

    enabled an automatic merge when the pipeline for a6b95027 succeeds

  • Alessio Caiazza mentioned in commit 0177466a

    mentioned in commit 0177466a

  • Author Maintainer

    Yay! :rocket:

  • Tomasz Maczukin mentioned in merge request !1012 (merged)

    mentioned in merge request !1012 (merged)

  • Steve Xuereb mentioned in merge request !1014 (closed)

    mentioned in merge request !1014 (closed)

  • mentioned in issue #3573 (closed)

  • Tomasz Maczukin changed title from Introduce GCS cache support to Introduce GCS adapter for remote cache

    changed title from Introduce GCS cache support to Introduce GCS adapter for remote cache

  • mentioned in issue #3450 (closed)

  • Steve Xuereb mentioned in commit 6fa38409

    mentioned in commit 6fa38409

  • Steve Xuereb mentioned in merge request !1140 (merged)

    mentioned in merge request !1140 (merged)

  • mentioned in issue #4070 (closed)

  • mentioned in epic &1238 (closed)

  • mentioned in issue #6556 (closed)

  • mentioned in issue #5018 (closed)

  • Please register or sign in to reply
    Loading