Optimising our GitHub workflows

[jenbutongit]

Occasionally our 5GB repository cache will fill before our weekly keys rotate.

This will block any PRs from having successful builds until the end of the week.
https://github.com/XGovFormBuilder/digital-form-builder/runs/2793269408?check_suite_focus=true

in my most humble of onions, we are not building and testing efficiently for our workflows.

what is currently happening, when a PR is opened:

• .github/workflows/branch--lint-unit-and-smoke-test.yml is kicked off
• Lint, building, and unit tests are run
  • the cache is hit here, for the node dependencies
• once the unit tests pass, we do a docker-compose build and run
  • the cache is hit another time, to grab the cached docker layers from previous builds
  • these images are around 1gb each, even with alpine. The images we build here are not slim! The **/src directory is not deleted, so we have a bunch of junk adding to the cache
• we then run the smoke tests in a node environment, against the docker images

Please post and discuss proposals below

[jenbutongit]

preferably, we stop building images on PRs. It takes far too long in the first place!

The reason why are building images, is that we can more closely replicate the environment the smoke tests would run on.

I don't think this is suitable..

1. the built images are discarded - and they are 1gb each. they do not affect the images built on merge to main at all.
2. I don't see how much of a difference running against an ubuntu container/VM (the GitHub test runner) would make against running an alpine image (linux distro).
3. we are also running headlessly
4. we run the tests on a "real" environment nightly. Smoke tests in my experience, have always been "run a slim suite on PRs", then run a more complete suite nightly.

Solution 1

on PRs:

• Build and run the designer and runner on the test runner, do not build images
• run the smoke tests

then on merge:

• the images are built and tagged, with the suffix -rc (release candidate) 3.1.xx-rc

then on successful publish to GHCR (there is an event we can hook into for GitHub Actions):

• run the smoke tests against these images
• if they are successful, retag the images as not rc. 3.1.xx

OR

• We can also run the smoke tests after we deploy to heroku, and remove -rc when the smoke tests complete on the heroku instance.

IF there are any concerns about using "bad" images, we/you can filter tags/notifications depending on if you care about -rc builds.

[jenbutongit]

we could give it a try. I've got a suspicion that we'd run into out-of-memory issues first when we try docker pull.

It's worth adding to the workflow a step which does docker images, so we can see what the GitHub Actions runner has pulled down

[jenbutongit]

https://github.com/XGovFormBuilder/digital-form-builder/pull/579/checks?check_run_id=2819788105#step:11:1

@MCrawleyHomeOffice has just put a debug line to print the images we're pulling down. There are a few images in there that don't need to be, but they're only taking up disk space.

if we look at this, it shows us what layers are getting cached. It does look like it's only caching the layers for the images that we're building.
https://github.com/XGovFormBuilder/digital-form-builder/pull/579/checks?check_run_id=2819788105#step:34:7

[MCrawleyHomeOffice]

I've just had a quick chat with @gautampachnandahmo who's also suggested we run a "docker system prune". He guided me through the creation of a new github action cron job that will run overnight that will run the prune after the smoke tests in heroku have completed - The new workflow is called "Git Cache Prune - Nightly" but it will only run on my test branch for now.

He has suggested we continue our investigation into what the cache is containing though, as it looks like there are multiple versions of the same images in our cache.

[jenbutongit]

I'm about 90% sure it's not the issue because we can see what is being cached in this step. Again, the issue is with the allocated 5GB shared cache - not the CI runner (the ubuntu box that we are using to run our CI commands). If it was an issue, we would have out of disk or out of memory issues before we get to the caching step, which happens AFTER we do docker pull. We would expect OOD/OOM issues at this point.

https://github.com/XGovFormBuilder/digital-form-builder/pull/579/checks?check_run_id=2819788105#step:34:7

[jenbutongit]

addendum: you cannot delete the shared cache either, at least not manually. And if you were to do do any cache clearing, or docker pruning, we might as well just change our docker cache key every day rather than every week.

[jenbutongit]

Solution 1.5?

on PRs:

• Build and run the designer and runner on the test runner, do not build images
• run the smoke tests
• hook into the PR approve event. If the non docker based smoke tests pass, AND there is an approval (i.e. its ready to merge)
• build the designer/runner docker images and run the smoke tests against this. If they pass, Tag and publish the images with the tag containing some sort of PR identifier, or calculated version with -rc.
• run Smoke tests against these images

then on merge to main:

• somehow pass the PR identifier to the main workflow (maybe there is an event, or information on the merge event we can leverage)
• optionally run the smoke tests against these images (they have already been run), or run them against heroku instance
• if they are successful, retag the images as not rc. 3.1.xx

[jenbutongit]

Solution 2

If we still want to run tests against docker images, we can also try this action (instead of docker-layer-caching). From what I understand, this action will

1. cache layers
2. build and publish the stages of the docker images, which means we do not have to cache the layers at these stages. For example:
   in stage 1 FROM node:12-alpine3.12 AS base, this is very unlikely to change, unless an apline/linux package needs to be updated. We can publish this, so it does not clog up our cache

https://github.com/marketplace/actions/build-docker-images-using-cache

[MCrawleyHomeOffice]

I've had further conversations with @gautampachnandahmo who has kindly assisted me in investigating this issue.

The problem lies with the github action we're using for the caching - Due to limitations with monitoring hits it must pull through all previously restored images each time meaning the cache will always fill up after a certain number of runs.

@gautampachnandahmo believes we should remove the caching altogether as there's very little return on investment using this, given the size of the team and amount of PRs actually raised per week. We then do proper investigation into other alternatives but only introduce once we know we won't hit similar limitations further down the line.

The alternative is to make our caches renew daily rather than weekly, but the issue will continue as soon as the development team grows so is not a solution, and again - The benefits from caching are negligible.

@gautampachnandahmo has agreed to discuss if you need further convincing.

@jenbutongit , @endamccormack , @gstevenson - Your thoughts?

[jenbutongit]

The reason why we added the caching was because it was making a huge difference in build times. It takes about 15 minutes to build the images from scratch (check how long the first smoke test run takes on a PR, vs the second) and about 6-8 without.

I will accept renewing the cache to daily (%u), or appending a string to the cache value when that fills, I'm not too sure about removing it completely.

[MCrawleyHomeOffice]

In that case how abouts we spike for the initial investigation of a replacement, perhaps the replacement you've suggested as Solution 2. In the meantime we can reduce the cache to daily.

The alternative is we decide to no longer test against docker images until we have a replacement that's more fit for purpose, though there's a good chance we'll still hit cache issues there I suspect.

I'm open for any and all suggestions, you folks know this system and tech stack far better than myself.

[jenbutongit]

Caches for node dependencies are a lot more predictable. We also use the yarn 2 cache (see your .yarn directory) which is a lot smaller than if you tried to DIY tarball the node_modules directory. It will be easier to evict old dependencies. Think how often you have upgraded or added a node dependency, vs with Docker Layer Caching, the layers will change on every code change.

If you want to spike finding a better caching solution, feel free to hijack my PR/branch. The approach is detailed in this discussion as solution 2.

[MCrawleyHomeOffice]

Just as a quick update, myself and Jen have agreed that for now we're going to shorten the life of the cache. I will see that this is done in the coming weeks.

This discussion will be kept open while we come to an agreement on what the end goal is - Whether that's removing docker image testing, removing caching altogether or swapping out the caching for an alternative (see solution 2 for a suggested alternative).

My personal thoughts are that we do not test against a docker image, this would negate the need for docker layer caching altogether but we can still make use of other caching (eg. yarn cache)

@endamccormack , @jenbutongit , @TheSpartan1980 , @gstevenson , if you could provide your thoughts on this it'd be much appreciated.

EDIT: The bug has been raised to change the cache life span - https://collaboration.homeoffice.gov.uk/jira/browse/FORMS-808

[jenbutongit]

fyi - full uncached builds are now ~4min but cached builds are now at ~30s. Enjoy!