-
Notifications
You must be signed in to change notification settings - Fork 4.1k
chore(ci): implement best practices for CI caching #17873
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
chore(ci): implement best practices for CI caching #17873
Conversation
it was previously on the sliding v2 tag and could update to unknown code without notice
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
|
I made a quick first attempt to merely post these as suggestions to the PR @SelaseKay had open and which this should be applied on top of, but the github web UI did not handle the formatting of the YAML correctly, and spacing in YAML has semantic meaning so that just didn't work. And then I realized that the amount of changes were large enough and needed enough context to pass review that I thought a separate PR off that one made more sense / was more easily reviewable This should save a lot of CI time, between Pods actually having valid caches and the AVD actually being cached in a warm state most of the time, plus main having any caches...should be a lot better. |
|
Oh - testing note: the specific workflow logs plus the cache storage link I put in the description are how you will judge if this worked. However, there is no way it will take affect until it has landed on main unfortunately. So you have to merge it without evidence, just based on review. Once on main, and once it starts to be used in branches (based on main with these changes) then it will be possible to judge the effectiveness. Unfortunately that's the way changing workflows and changing caching behavior works. I will be available to verify it is working as intended and/or make changes as a follow-on though. I maintain the workflows for all of the Invertase repositories in the react-native ecosystem and I'm happy to help here in Flutter Fire. Cheers |
| key: avd-${{ runner.os }} | ||
| - name: Start AVD then run E2E tests | ||
| uses: reactivecircus/android-emulator-runner@v2 | ||
| uses: reactivecircus/android-emulator-runner@b530d96654c385303d652368551fb075bc2f0b6b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verify visually by noticing the v2 tag is currently attached to this SHA here: ReactiveCircus/android-emulator-runner@b530d96
| # Branches can read main cache but main cannot read branch cache. Avoid LRU eviction with main-only cache. | ||
| cache-read-only: ${{ github.ref != 'refs/heads/main' }} | ||
| # The firebase emulators are pure javascript and java, OS-independent | ||
| enableCrossOsArchive: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation if interested https://github.com/actions/cache/blob/main/tips-and-workarounds.md#cross-os-cache
complete listing of files in ~/.cache/firebase/emulators for verification:
mike@krakatoa:~/work/invertase/flutterfire (fix-avd-storage) % find /Users/mike/.cache/firebase/emulators
/Users/mike/.cache/firebase/emulators
/Users/mike/.cache/firebase/emulators/ui-v1.15.0.zip
/Users/mike/.cache/firebase/emulators/firebase-database-emulator-v4.11.2.jar
/Users/mike/.cache/firebase/emulators/cloud-storage-rules-runtime-v1.1.3.jar
/Users/mike/.cache/firebase/emulators/cloud-firestore-emulator-v1.19.8.jar
/Users/mike/.cache/firebase/emulators/ui-v1.15.0
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/server
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/server/server.mjs.map
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/server/server.mjs
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/favicon-16x16.png
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/safari-pinned-tab.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/favicon.ico
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/index.html
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/android-chrome-192x192.png
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/apple-touch-icon.png
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/android-chrome-512x512.png
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/manifest.json
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/robots.txt
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/mstile-150x150.png
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/index-BX-A56oj.js
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/index-CjB9C900.css
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/index-BX-A56oj.js.map
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/extensions
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/extensions/default-extension.png
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/img
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/img/database.png
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_saml.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_phone.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_facebook.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_game_center.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_apple.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_github.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_mslive.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_yahoo.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_twitter.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_play_games.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_email.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_google.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/assets/provider-icons/auth_service_oidc.svg
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/browserconfig.xml
/Users/mike/.cache/firebase/emulators/ui-v1.15.0/client/favicon-32x32.png
mike@krakatoa:~/work/invertase/flutterfire (@mikehardy/ci-caching-best-practices) %
| - name: 'Install Tools' | ||
| run: | | ||
| sudo npm i -g firebase-tools | ||
| echo "FIREBASE_TOOLS_VERSION=$(npm firebase --version)" >> $GITHUB_ENV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussion on how to share information between workflow steps, including syntax for command interpolation while setting an environment variable: https://stackoverflow.com/a/57989070/9910298
example output of command executed so you may verify it is valid for an environment variable (no spaces or similar):
mike@krakatoa:~/work/invertase/react-native-firebase (main) % yarn firebase --version
14.24.2
| melos-version: '5.3.0' | ||
| - name: 'Bootstrap package' | ||
| run: melos bootstrap --scope tests && melos bootstrap --scope "cloud_firestore*" | ||
| - name: 'Install Tools' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to move tools install above tools cache restore so it could get the version for cache key
The tools install makes no use of the cache, so the move should do no harm
| env: | ||
| AVD_ARCH: x86_64 | ||
| AVD_API_LEVEL: 34 | ||
| AVD_TARGET: google_apis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env var used to follow "Don't Repeat Yourself" programming law since these values will be used both for cache key and emulator action now
.github/workflows/android.yaml
Outdated
| cache-read-only: ${{ github.ref != 'refs/heads/main' }} | ||
| path: ~/.cache/firebase/emulators | ||
| key: firebase-emulators-v3-${{ runner.os }} | ||
| key: firebase-emulators-v3-${{ runner.os }}-${{ env.FIREBASE_TOOLS_VERSION }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussion around "caches are immutable and may never be updated", necessitating the unique key https://github.com/actions/cache/blob/main/tips-and-workarounds.md#update-a-cache
use the separate actions/cache "restore" and "save" actions so we may only save optional based on whether we are on main or not actions/cache isolates branch caches from main to avoid branch cache poison attacks on main runs but branches may see main cache as those caches are considered trustworthy branch caches do consume available LRU storage though, meaning branch caches could evict main caches and in high traffic / large cache repositories like this one that means caches LRU out frequently and cache hit rates are terrible unless you cache on main only
very occasionally, there are errors in the `hashFiles` function used to calculate cache keys, and this causes a build error that is a flake, not a valid CI failure signal so, continue the workflow even if the caching setup has errors, as that is a recoverable problem and the workflow may still have a valid success outcome
github workflow caches are considered immutable, they will never update if there was a hit on the primary cache key ergo, if the primary cache key never changes the caches will never update and you will only ever get fresh caches by chance if stale caches are evicted via LRU cache storage limit management So, all cache primary keys are unique now according to the requirements of the specific cache - the AVD cache is now unique on the api-level/target/arch tuple, and that is set and referenced in an environment variable since it is now used in two places - the firebase emulator cache is set based on the firebase-tools version, which always changes when new emulator versions are downloaded
f6110bb to
51dcddc
Compare
| - name: Firebase Emulator Cache | ||
| uses: actions/cache@1bd1e32a3bdc45362d1e726936510720a7c30a57 | ||
| id: firebase-emulator-cache | ||
| uses: actions/cache/restore@1bd1e32a3bdc45362d1e726936510720a7c30a57 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
splitting to the separate restore/save actions vs the combo 'cache' action so we may restore every time but only save conditionally
| pgrep -f appium && pkill -f appium || echo "No Appium process found" | ||
| - name: Save Firestore Emulator Cache | ||
| # Branches can read main cache but main cannot read branch cache. Avoid LRU eviction with main-only cache. | ||
| if: github.ref == 'refs/heads/main' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example of conditional step based on branch ref https://stackoverflow.com/a/58142412/9910298
the downloaded/cached files are pure javascript and java, they may be shared across operating systems safely
51dcddc to
ef66fc8
Compare
Description
👋 this is based on #17871 from @SelaseKay vs main because Jude was working on that file already and it will likely suffer a conflict otherwise
I worked with Jude on that change in 17871 and while I was in there I noticed that the caching setup for CI in this repo was likely to be subject to severe LRU thrashing and very very low hit rates, with low useful caches even with hits.
In fact when I went to verify that, it was true: https://github.com/firebase/flutterfire/actions/caches?query=sort%3Acreated-asc
The oldest cache is only 4 days old, the majority of the cache storage is taken up with caches only 4 hours old - and those are from a branch which means main builds can't even see them to use them.
Each commit in this series is a fully separate idea to improve things, and each should be reviewed separately by reading the commit message explaining that specific change then checking the diff for that commit
The basic idea is this though:
In addition, I pinned the Android Emulator runner to a concrete SHA as is practice in this repo
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]).This will ensure a smooth and quick review process. Updating the
pubspec.yamland changelogs is not required.///).melos run analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?