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

test: optimize caching of downloaded assets #5019

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

gilbertsoft
Copy link
Member

@gilbertsoft gilbertsoft commented Jun 25, 2023

The Issue

Download of assets for tests fails regularly and tests have to be restarted.

How This PR Solves The Issue

This patch avoids the cleanup of assets between the test runs and introduces a unique hash for every assets which are now properly shared between the test runs and should minimize failures.

On GH the cache is always loaded from the master branch and updated once a week. This avoids we use more than the 10 GB available for caching and makes caching more stable. At the time of writing this patch we used almost 40 GB. Because big caches I have avoided the usage of hashFiles() function and implemented a key based on the date instead.

Caches can be removed manually at https://github.com/ddev/ddev/actions/caches.

Manual Testing Instructions

Caching behavior must be observed once this PR is merged. Important points are:

  • cache on master is updated once a week
  • branches are using the master cache
  • asset download errors do not longer occur.

Automated Testing Overview

The changes in the internal asset caching is covered by the existing tests.

Related Issue Link(s)

Release/Deployment Notes

@github-actions
Copy link

github-actions bot commented Jun 25, 2023

@gilbertsoft gilbertsoft force-pushed the tests/optimize-cached-files branch 2 times, most recently from 5961158 to 12edc52 Compare June 25, 2023 17:19
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

We don't have that much trouble with the caching of CMS tarballs, but improving it is fine. Only one test does the full set of these (Github Pull/TestDdevFullSiteSetup).

The most common failure is the node test, which absolutely needs improvement and caching.

The real approach to caching the CMS tarballs though is to cache them in the github test.

@gilbertsoft
Copy link
Member Author

We don't have that much trouble with the caching of CMS tarballs, but improving it is fine. Only one test does the full set of these (Github Pull/TestDdevFullSiteSetup).

The most common failure is the node test, which absolutely needs improvement and caching.

The real approach to caching the CMS tarballs though is to cache them in the github test.

About 90% of the failures I have is because of failed downloads!

@gilbertsoft
Copy link
Member Author

Happened again in this patch
grafik

@rfay
Copy link
Member

rfay commented Jun 25, 2023

I see more of the nodejs stuff, but every improvement is welcome.

However, improving the caching in the go code won't help much here unless we actually cache the testcache in the github tests.

@gilbertsoft
Copy link
Member Author

I see more of the nodejs stuff, but every improvement is welcome.

However, improving the caching in the go code won't help much here unless we actually cache the testcache in the github tests.

Will check for more improvments e.g. GH once the tests are green again...

.github/workflows/tests.yml Outdated Show resolved Hide resolved
cmd/ddev/cmd/clean.go Outdated Show resolved Hide resolved
@gilbertsoft gilbertsoft force-pushed the tests/optimize-cached-files branch 2 times, most recently from abc4302 to 69d82db Compare June 26, 2023 10:18
@gilbertsoft gilbertsoft changed the title [tests] Optimize caching of downloaded assets test: optimize caching of downloaded assets Jun 28, 2023
@gilbertsoft gilbertsoft force-pushed the tests/optimize-cached-files branch 7 times, most recently from d8f3c8f to 11a1e92 Compare July 2, 2023 09:11
@gilbertsoft gilbertsoft marked this pull request as ready for review July 2, 2023 11:45
@gilbertsoft gilbertsoft requested a review from a team as a code owner July 2, 2023 11:45
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

This seems to dump the tarball into two places (root of testcache and also inside tarballs directory):

$ ls -l . tarballs/
.:
total 0
drwxr-xr-x  21 rfay  staff  672 Jul  3 10:04 82f32c881c851f923c3442367116fccc8617c7bcc4efa35ea2e649363a3d1b27.tar.gz
drwxr-xr-x   3 rfay  staff   96 Jul  3 10:04 tarballs

tarballs/:
total 43056
-rw-r--r--  1 rfay  staff  21171263 Jul  3 10:04 82f32c881c851f923c3442367116fccc8617c7bcc4efa35ea2e649363a3d1b27.tar.gz

The one in the root is the extraction of the one in the tarballs directory I guess. It's pretty confusing.

However, the output shows otherwise, I guess it shows the old destination?

Downloading https://wordpress.org/wordpress-6.0.1.tar.gz
Downloaded https://wordpress.org/wordpress-6.0.1.tar.gz into /Users/rfay/.ddev/testcache/tarballs/82f32c881c851f923c3442367116fccc8617c7bcc4efa35ea2e649363a3d1b27.tar.gz
Extracted /Users/rfay/.ddev/testcache/tarballs/82f32c881c851f923c3442367116fccc8617c7bcc4efa35ea2e649363a3d1b27.tar.gz into /Users/rfay/.ddev/testcache/82f32c881c851f923c3442367116fccc8617c7bcc4efa35ea2e649363a3d1b27.tar.gz
Copying directory /Users/rfay/.ddev/testcache/82f32c881c851f923c3442367116fccc8617c7bcc4efa35ea2e649363a3d1b27.tar.gz to /Users/rfay/tmp/ddevtest/TestPkgWordpress517092105

Please make the tarball have a name that can be studied and understood. For example, this one is for wordpress. Can we keep a name and date on it? I'm ok with it having a hash in the name of course.

My results of the testcache directory after running TestDdevStart with DDEV_DEBUG=true: https://gist.github.com/rfay/585cccdcb0a8814c371ea2655e862df2

.github/workflows/colima-tests.yml Outdated Show resolved Hide resolved
.github/workflows/colima-tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
cmd/ddev/cmd/clean.go Outdated Show resolved Hide resolved
.github/workflows/cancel-previous-workflow.yml Outdated Show resolved Hide resolved
.github/workflows/colima-tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
pkg/testcommon/testcommon.go Show resolved Hide resolved
@gilbertsoft gilbertsoft force-pushed the tests/optimize-cached-files branch 4 times, most recently from 71f1685 to 660d10b Compare July 4, 2023 08:43
@gilbertsoft gilbertsoft requested a review from rfay July 4, 2023 11:46
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I sure like it better now! I'm very willing to give this a try. Should be a win for us.

New testcache:

testcache
├── 00fbfea3_drupal-9.4.2.tar.gz
│   ├── INSTALL.txt
│   ├── LICENSE.txt
│   ├── README.md
│   ├── autoload.php
│   ├── composer.json
│   ├── composer.lock
│   ├── core
│   ├── example.gitignore
│   ├── index.php
│   ├── modules
│   ├── profiles
│   ├── robots.txt
│   ├── sites
│   ├── themes
│   ├── update.php
│   ├── vendor
│   └── web.config
├── 82f32c88_wordpress-6.0.1.tar.gz
│   ├── index.php
│   ├── license.txt
│   ├── readme.html
│   ├── wp-activate.php
│   ├── wp-admin
│   ├── wp-blog-header.php
│   ├── wp-comments-post.php
│   ├── wp-config-sample.php
│   ├── wp-content
│   ├── wp-cron.php
│   ├── wp-includes
│   ├── wp-links-opml.php
│   ├── wp-load.php
│   ├── wp-login.php
│   ├── wp-mail.php
│   ├── wp-settings.php
│   ├── wp-signup.php
│   ├── wp-trackback.php
│   └── xmlrpc.php
└── tarballs
    ├── 00fbfea3_drupal-9.4.2.tar.gz
    └── 82f32c88_wordpress-6.0.1.tar.gz

13 directories, 29 files

@rfay rfay merged commit b931885 into ddev:master Jul 4, 2023
27 checks passed
@rfay rfay deleted the tests/optimize-cached-files branch July 4, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants