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

Add step to delete all migrations images from docker cache #748

Conversation

mikaylathompson
Copy link
Collaborator

@mikaylathompson mikaylathompson commented Jun 19, 2024

Description

We believe that the docker cache is causing the e2e tests to fail, possibly by taking up too much disk space and forcing the target cluster to lock indices.

One solution (assuming a correct diagnosis) is to delete docker caching altogether. This, of course, also means we lose the benefits of caching the external images (namely, not being rate limited). This PR is an attempt to split the difference--keep the cache for external images, but don't cache all the migrations/ images, which we're rebuilding anyways.

Issues Resolved

Hopefully https://opensearch.atlassian.net/browse/MIGRATIONS-1797

Testing

Several tests in the commits to this PR.

Before merging, I intend to roll back the final two commits.

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 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.90%. Comparing base (1488d90) to head (0501b56).
Report is 6 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #748      +/-   ##
============================================
+ Coverage     64.75%   64.90%   +0.14%     
- Complexity     1585     1586       +1     
============================================
  Files           238      239       +1     
  Lines          9880     9919      +39     
  Branches        771      771              
============================================
+ Hits           6398     6438      +40     
  Misses         3072     3072              
+ Partials        410      409       -1     
Flag Coverage Δ
gradle-test 64.03% <ø> (+0.04%) ⬆️
python-test 74.45% <ø> (+0.88%) ⬆️

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.

@mikaylathompson
Copy link
Collaborator Author

This PR itself seems like additional evidence towards docker cache being the issue. Testing with a cache hit failed, cache miss succeeded.

The cleanup step worked as intended: https://github.com/opensearch-project/opensearch-migrations/actions/runs/9588463478/job/26440545131?pr=748#step:18:28

Signed-off-by: Mikayla Thompson <[email protected]>
@mikaylathompson mikaylathompson marked this pull request as ready for review June 19, 2024 22:19
@mikaylathompson mikaylathompson changed the title Add step to delete all migrations images Add step to delete all migrations images from docker cache Jun 19, 2024
@peternied peternied merged commit e805ed7 into opensearch-project:main Jun 19, 2024
11 checks passed
@mikaylathompson mikaylathompson deleted the disable-e2e-test-docker-image-caching branch June 20, 2024 00:50
@mikaylathompson mikaylathompson restored the disable-e2e-test-docker-image-caching branch June 25, 2024 08:28
@mikaylathompson mikaylathompson deleted the disable-e2e-test-docker-image-caching branch June 25, 2024 08:28
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.

2 participants