Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache docker images for E2E testing workflow in Github Actions #698

Conversation

mikaylathompson
Copy link
Collaborator

Description

We’re frequently encountering issues where builds fail due to rate limiting in the github actions workflows for pulling docker images.

For example, see #695 (comment).
While we’ve considered multiple ways to resolve this, a quick one (suggested in the comment above) is to use the github actions caching mechanism to cache our docker images.

One issue I hit along the way was that I was defining the action as:

      - name: Cache Docker Images
        uses: ScribeMD/[email protected]
        with:
          key: docker-${{ runner.os }}-${{ hashFiles('**/docker-compose.yml', '**/Dockerfile') }}

which does the same thing as now--a hash of the file contents of all docker-compose and Dockerfiles in the repo. However, it turns out that this key is evaluated at runtime--both during the original action (loading from cache) and the post-action (saving to cache). However, our gradle build process includes generating new dockerfiles, so the value of this key was changing from pre to post--meaning that there would almost by definition never be a cache hit. Calculating the hash in a separate step and storing it resolves this issue.

Issues Resolved

MIGRATIONS-1761

Testing

A lot of manual testing -- I left my former branch open as a PR in my fork to preserve the testing history: mikaylathompson#1

Check List

  • New functionality includes testing
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.46%. Comparing base (d6e249d) to head (ad56463).
Report is 19 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #698      +/-   ##
============================================
+ Coverage     63.41%   63.46%   +0.04%     
- Complexity     1551     1563      +12     
============================================
  Files           215      216       +1     
  Lines          8340     8414      +74     
  Branches        753      758       +5     
============================================
+ Hits           5289     5340      +51     
- Misses         2649     2673      +24     
+ Partials        402      401       -1     
Flag Coverage Δ
unittests 63.46% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@gregschohn gregschohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this works, please put this in the gradle-build-and-test.yml file too

@@ -22,6 +22,19 @@ jobs:
java-version: '11'
distribution: 'corretto'

- name: Generate Cache Key from Dockerfiles
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it turns out that this key is evaluated at runtime

Good find and workaround, since this is a bug in the action, could you create an issue so they can fix it in a future release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@mikaylathompson mikaylathompson Jun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I have most of a PR for them, but the tests are pretty brittle and I'm not sure how much time I want to put into fixing them before I get an indication about whether they'd be open to it anyways)

@mikaylathompson mikaylathompson merged commit e18fe96 into opensearch-project:main Jun 6, 2024
7 checks passed
@mikaylathompson
Copy link
Collaborator Author

I want to give this a few days of bake time so we can spot any issues, but issue for adding it to the other pipeline created here: https://opensearch.atlassian.net/browse/MIGRATIONS-1766

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants