Support overwriting caches #60

earl-warren · 2024-11-07T15:30:08Z

earl-warren commented

2024-11-07 15:30:08 +00:00

Port of https://github.com/nektos/act/pull/2265

In the old implementation, it assumes that there will be no duplicate uploads of caches with the same key.

But in fact, some actions like actions/setup-go will do something like:
1. Download the cache with the generated key.

2. Add more contents to the cache dir by downloading and building.

3. Upload the cache dir with the same key to replace the old cache.
It will result in a failure (though it will actually be ignored) with the following log:
/root/go/pkg/mod
/root/.cache/go-build
[command]/bin/tar --posix -cf cache.tgz --exclude cache.tgz -P -C /workspace/gitea/act_runner --files-from manifest.txt -z
::warning::Failed to save: {"error":"already exist"}
To support it, this PR introduces some changes.
* Uploading caches with same key (and the version which indicates the env) will be allowed.

* Return the latest completed one when retrieving a cache with a key.

* The old caches with the same key will be deleted regularly, keep the latest only.
I have tested the code with a custom act_runner. It works well and is compatible with old data.

Port of https://github.com/nektos/act/pull/2265 > In the old implementation, it assumes that there will be no duplicate uploads of caches with the same key. > > But in fact, some actions like `actions/setup-go` will do something like: > > 1. Download the cache with the generated key. > > 2. Add more contents to the cache dir by downloading and building. > > 3. Upload the cache dir with the same key to replace the old cache. > > > It will result in a failure (though it will actually be ignored) with the following log: > > ``` > /root/go/pkg/mod > /root/.cache/go-build > [command]/bin/tar --posix -cf cache.tgz --exclude cache.tgz -P -C /workspace/gitea/act_runner --files-from manifest.txt -z > ::warning::Failed to save: {"error":"already exist"} > ``` > > To support it, this PR introduces some changes. > > * Uploading caches with same key (and the version which indicates the env) will be allowed. > > * Return the latest completed one when retrieving a cache with a key. > > * The old caches with the same key will be deleted regularly, keep the latest only. > > > I have tested the code with a custom act_runner. It works well and is compatible with old data.

earl-warren self-assigned this

2024-11-07 15:30:08 +00:00

earl-warren added 3 commits

2024-11-07 15:30:09 +00:00

fix: docker buildx cache restore not working (#2236 ) d42e8f05fc

* To take effect artifacts v4 pr is needed with adjusted claims

(cherry picked from commit 75e4ad93f4)

Support overwriting caches (#2265 ) 71cfa868fa

* feat: support overwrite caches

* test: fix case

* test: fix get_with_multiple_keys

* chore: use atomic.Bool

* test: improve get_with_multiple_keys

* chore: use ping to improve path

* fix: wrong CompareAndSwap

* test: TestHandler_gcCache

* chore: lint code

* chore: lint code

(cherry picked from commit b9382a2c4e)

fix: cache adjust restore order of exact key matches (#2267 )

checks / check and test (pull_request) Successful in 1m23s

Details

/ cascade (pull_request_target) Failing after 42s

Details

c04850088f

* wip: adjust restore order

* fixup

* add tests

* cleanup

* fix typo

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit f825e42ce2)

earl-warren requested review from Kwonunn

2024-11-07 15:31:21 +00:00

cascading-pr commented

2024-11-07 15:32:52 +00:00

cascading-pr updated at forgejo/runner#314

cascading-pr updated at https://code.forgejo.org/forgejo/runner/pulls/314

Kwonunn requested changes

2024-11-10 16:33:02 +00:00

Dismissed

Kwonunn left a comment

i haven't actually tested it because i don't really have a good way to test caching

mostly everything looks good, i've requested some legibility changes

i haven't actually tested it because i don't really have a good way to test caching mostly everything looks good, i've requested some legibility changes

pkg/artifactcache/handler.go

					
				@ -36,3 +36,3 @@

					logger   logrus.FieldLogger

					gcing int32 // TODO: use atomic.Bool when we can use Go 1.19

					gcing atomic.Bool

gcing is kind of a bad name, i suggest gcIsRunning or something similar

`gcing` is kind of a bad name, i suggest `gcIsRunning` or something similar

viceice marked this conversation as resolved

pkg/artifactcache/handler.go

					
				@ -382,0 +357,4 @@

							bolthold.Where("Key").Eq(prefix).

								And("Version").Eq(version).

								And("Complete").Eq(true).

								SortBy("CreatedAt").Reverse()); err == nil || !errors.Is(err, bolthold.ErrNotFound) {

i think this if statement construction is really hard to understand

earl-warren marked this conversation as resolved

pkg/artifactcache/handler.go

					
				@ -406,2 +371,2 @@

							if !errors.Is(err, stop) {

								return nil, err

						if err := db.FindOne(cache,

							bolthold.Where("Key").RegExp(re).

same here

viceice marked this conversation as resolved

earl-warren commented

2024-11-10 17:32:48 +00:00

i haven't actually tested it because i don't really have a good way to test caching

There is a test https://code.forgejo.org/forgejo/end-to-end/src/branch/main/actions/example-cache that was run successfully at https://code.forgejo.org/forgejo/act/actions/runs/267/jobs/0 (you can get to it following the cascading-pr breadcrums).

mostly everything looks good, i've requested some legibility changes

I agree on all counts of naming and code readability. If you don't mind, I'd rather leave them as is because it helps with cherry-picking from Gitea & ACT. If you feel strongly about it, I'll make the change.

> i haven't actually tested it because i don't really have a good way to test caching There is a test https://code.forgejo.org/forgejo/end-to-end/src/branch/main/actions/example-cache that was run successfully at https://code.forgejo.org/forgejo/act/actions/runs/267/jobs/0 (you can get to it following the [cascading-pr breadcrums](https://code.forgejo.org/forgejo/act/pulls/60#issuecomment-18628)). > mostly everything looks good, i've requested some legibility changes I agree on all counts of naming and code readability. If you don't mind, I'd rather leave them as is because it helps with cherry-picking from Gitea & ACT. If you feel strongly about it, I'll make the change.

Kwonunn approved these changes

2024-11-11 09:15:56 +00:00

Kwonunn left a comment

alright, looks good to me then ^-^

❤️ 1

earl-warren merged commit 050bc604f8 into main

2024-11-11 16:43:24 +00:00

earl-warren deleted branch wip-artifactcache

2024-11-11 16:43:24 +00:00

earl-warren referenced this pull request from a commit

2024-11-11 16:43:26 +00:00

Merge pull request 'Support overwriting caches' (#60) from wip-artifactcache into main