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

fix: Change checkout cache strategy #35259

Closed
wants to merge 1 commit into from

Conversation

cipolleschi
Copy link
Contributor

Summary

This PR updates he cache strategy for the checkout_with_cache command.

The previous strategy was using three keys in descending priority order to restore from the cache:

  • << parameters.checkout_base_cache_key >>-{{ arch }}-{{ .Branch }}-{{ .Revision }}
  • << parameters.checkout_base_cache_key >>-{{ arch }}-{{ .Branch }}
    *<< parameters.checkout_base_cache_key >>-{{ arch }}

When it saves, it always saves using the first key.

The restore works as it follows:

  1. It tries to restore the cache using the first key
  2. If it fails, it checks whether there is a cache hit for a key that matches the second key as a pattern
  3. If it fails, it checks whether there is a cache hit for a key that matches the third pattern
  4. Otherwise, it is a cache miss.

This does not work well. Imagine that you submit some code in commit xxxx for branch abc.
The CI run the first time, it misses all the three keys and checks out the code normally.
Then, it stores the checked out code in the checkout_key-abc-xxxx key.

Then, you submit a commit yyyy in the same branch.
The CI starts, it tries with the key checkout_key-abc-yyyy but it misses
It tries with the key checkout_key-abc and it finds the cache for checkout_key-abc-xxxx and it restores it, forgetting about your changes.

While doing the release, we created a tag in a commit X. Then we manually had to remove it, but the CI had a cached version of .git with the tag for
the 0.71-stable branch. And the release failed because the tag was already existing.

Why this should work

With this solution, we are going to cache using the commit. If there is no cache for a specific commit, it will be a miss. It won't try to be smart and
retrieve the code from previous caches.

This should prevent stale caches and if we manually remove a tag, and then we do a new commit, it should work.

This is a good trade-off that allows to pay the checkout cost only for the first batch of jobs of the pipeline.

NOTE: This still won't work if we don't do a new commit.

Changelog

[General] [Fixed] - Change Cache strategy to avoid cache bumps in Release

Test Plan

  1. CircleCI must be green

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Nov 8, 2022
@cipolleschi cipolleschi force-pushed the cipolleschi/fix-release-checkout branch from d576dac to 4a01025 Compare November 8, 2022 12:48
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: dac6806
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,068,113 +0
android hermes armeabi-v7a 6,440,497 +0
android hermes x86 7,483,431 +0
android hermes x86_64 7,342,828 +0
android jsc arm64-v8a 8,933,067 +0
android jsc armeabi-v7a 7,667,418 +0
android jsc x86 8,993,443 +0
android jsc x86_64 9,472,201 +0

Base commit: dac6806
Branch: main

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @cipolleschi in ddba780.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Nov 8, 2022
@kelset kelset deleted the cipolleschi/fix-release-checkout branch November 8, 2022 15:16
kelset pushed a commit that referenced this pull request Nov 22, 2022
Summary:
This PR updates he cache strategy for the `checkout_with_cache command`.

The previous strategy was using three keys in descending priority order to restore from the cache:
* `<< parameters.checkout_base_cache_key >>-{{ arch }}-{{ .Branch }}-{{ .Revision }}`
* `<< parameters.checkout_base_cache_key >>-{{ arch }}-{{ .Branch }}`
*`<< parameters.checkout_base_cache_key >>-{{ arch }}`

When it saves, it always saves using the first key.

The restore works as it follows:
1. It tries to restore the cache using the first key
2. If it fails, it checks whether there is a cache hit for a key that matches the second key as a pattern
3. If it fails, it checks whether there is a cache hit for a key that matches the third pattern
4. Otherwise, it is a cache miss.

This does not work well. Imagine that you submit some code in commit `xxxx` for branch `abc`.
The CI run the first time, it misses all the three keys and checks out the code normally.
Then, it stores the checked out code in the `checkout_key-abc-xxxx` key.

Then, you submit a commit `yyyy` in the same branch.
The CI starts, it tries with the key `checkout_key-abc-yyyy` but it misses
It tries with the key `checkout_key-abc` and it finds the cache for `checkout_key-abc-xxxx` and it restores it, forgetting about your changes.

While doing the release, we created a tag in a commit X. Then we manually had to remove it, but the CI had a cached version of .git with the tag for
the `0.71-stable` branch. And the release failed because the tag was already existing.

### Why this should work

With this solution, we are going to cache using the commit. If there is no cache for a specific commit, it will be a miss. It won't try to be smart and
retrieve the code from previous caches.

This should prevent stale caches and if we manually remove a tag, and then we do a new commit, it should work.

This is a good trade-off that allows to pay the checkout cost only for the first batch of jobs of the pipeline.

**NOTE:** This still won't work if we don't do a new commit.

## Changelog

[General] [Fixed] - Change Cache strategy to avoid cache bumps in Release

Pull Request resolved: #35259

Test Plan: 1. CircleCI must be green

Reviewed By: jacdebug

Differential Revision: D41120895

Pulled By: cipolleschi

fbshipit-source-id: 2b45da01803197dbe4a25a313a9dfc53a976d096
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This PR updates he cache strategy for the `checkout_with_cache command`.

The previous strategy was using three keys in descending priority order to restore from the cache:
* `<< parameters.checkout_base_cache_key >>-{{ arch }}-{{ .Branch }}-{{ .Revision }}`
* `<< parameters.checkout_base_cache_key >>-{{ arch }}-{{ .Branch }}`
*`<< parameters.checkout_base_cache_key >>-{{ arch }}`

When it saves, it always saves using the first key.

The restore works as it follows:
1. It tries to restore the cache using the first key
2. If it fails, it checks whether there is a cache hit for a key that matches the second key as a pattern
3. If it fails, it checks whether there is a cache hit for a key that matches the third pattern
4. Otherwise, it is a cache miss.

This does not work well. Imagine that you submit some code in commit `xxxx` for branch `abc`.
The CI run the first time, it misses all the three keys and checks out the code normally.
Then, it stores the checked out code in the `checkout_key-abc-xxxx` key.

Then, you submit a commit `yyyy` in the same branch.
The CI starts, it tries with the key `checkout_key-abc-yyyy` but it misses
It tries with the key `checkout_key-abc` and it finds the cache for `checkout_key-abc-xxxx` and it restores it, forgetting about your changes.

While doing the release, we created a tag in a commit X. Then we manually had to remove it, but the CI had a cached version of .git with the tag for
the `0.71-stable` branch. And the release failed because the tag was already existing.

### Why this should work

With this solution, we are going to cache using the commit. If there is no cache for a specific commit, it will be a miss. It won't try to be smart and
retrieve the code from previous caches.

This should prevent stale caches and if we manually remove a tag, and then we do a new commit, it should work.

This is a good trade-off that allows to pay the checkout cost only for the first batch of jobs of the pipeline.

**NOTE:** This still won't work if we don't do a new commit.

## Changelog

[General] [Fixed] - Change Cache strategy to avoid cache bumps in Release

Pull Request resolved: facebook#35259

Test Plan: 1. CircleCI must be green

Reviewed By: jacdebug

Differential Revision: D41120895

Pulled By: cipolleschi

fbshipit-source-id: 2b45da01803197dbe4a25a313a9dfc53a976d096
@cipolleschi cipolleschi mentioned this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants