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: use correct path to toolcache #1494

Merged
merged 4 commits into from
Jan 30, 2024
Merged

fix: use correct path to toolcache #1494

merged 4 commits into from
Jan 30, 2024

Conversation

KnisterPeter
Copy link
Member

The toolcache on GitHub Actions need to be in
/opt/hostedtoolcache. This is the case for all
environment variables set by act, but it's not the case for the volume mounted into the container.

The toolcache on GitHub Actions need to be in
/opt/hostedtoolcache. This is the case for all
environment variables set by act, but it's not the
case for the volume mounted into the container.
@KnisterPeter KnisterPeter requested a review from a team as a code owner December 7, 2022 13:30
@KnisterPeter KnisterPeter self-assigned this Dec 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 1 0 0.02s
✅ REPOSITORY gitleaks yes no 2.27s
✅ REPOSITORY git_diff yes no 0.17s
✅ REPOSITORY grype yes no 7.48s
✅ REPOSITORY secretlint yes no 1.07s
✅ REPOSITORY trivy-sbom yes no 0.6s
✅ REPOSITORY trufflehog yes no 14.89s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@mergify
Copy link
Contributor

mergify bot commented Dec 7, 2022

@KnisterPeter this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Dec 7, 2022
@KnisterPeter KnisterPeter marked this pull request as draft December 7, 2022 14:56
@catthehacker
Copy link
Member

It fails because you can't have a mount over data and in my images I keep node in /opt/hostedtoolcache

@KnisterPeter
Copy link
Member Author

We have /opt/hostedtoolcache hardcoded in a few places in the act codebase. Not being able to mount a volume to that means that the toolcache is unusable and broken.
Tools will be reinstalled all the time.

@nektos/act-maintainers Would it be okay for your to remove the hardcoded paths and replace with with a configurable option (while still defaulting to the hardcoded value).

@ChristopherHX
Copy link
Contributor

ChristopherHX commented Dec 20, 2022

The failing test will be altered in #1503 to use a different docker image. This change should be sufficient for this change to pass all tests.

Would it be okay for your to remove the hardcoded paths and replace with with a configurable option (while still defaulting to the hardcoded value).

This option would need a --container- prefix, because it should only apply to the docker environment. I'm not shure how such an option would solve a problem, this PR wouldn't solve.

@KnisterPeter
Copy link
Member Author

Cool. I think this PR is obsolete then and I can close it.

@KnisterPeter KnisterPeter deleted the fix-toolcache branch December 20, 2022 17:05
@ChristopherHX
Copy link
Contributor

I think this PR is obsolete then and I can close it.

Not shure what make you believe that this PR is obsolete, my mentioned change only resolve the not passing tests not anything else

A workaround would be use
--container-options "-v toolcache:/opt/hostedtoolcache" to bypass the testsuite.

@KnisterPeter KnisterPeter restored the fix-toolcache branch December 20, 2022 17:08
@KnisterPeter KnisterPeter reopened this Dec 20, 2022
@KnisterPeter
Copy link
Member Author

I thought your changes will resolve the more general toolcache issue.
I'll keep this PR open then until #1503 is merged and see if I can continue on this.

@github-actions
Copy link
Contributor

PR is stale and will be closed in 14 days unless there is new activity

@github-actions github-actions bot added the stale label Jan 20, 2023
@github-actions github-actions bot closed this Feb 4, 2023
@catthehacker catthehacker reopened this Feb 4, 2023
@catthehacker catthehacker removed the stale label Feb 4, 2023
@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5a80a04) 61.56% compared to head (f029fdc) 61.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1494      +/-   ##
==========================================
- Coverage   61.56%   61.53%   -0.04%     
==========================================
  Files          53       53              
  Lines        9002     9002              
==========================================
- Hits         5542     5539       -3     
- Misses       3020     3022       +2     
- Partials      440      441       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mergify mergify bot removed the needs-work Extra attention is needed label Apr 17, 2023
@ChristopherHX
Copy link
Contributor

While the tests are now passing. We would still have to move the tools of the default docker images out of the mount point.

Otherwise these docker images end up with an empty tool_cache and the preinstalled node etc. are away.

@KnisterPeter
Copy link
Member Author

@ZauberNerd I think we can close that, can't we?

@github-actions
Copy link
Contributor

PR is stale and will be closed in 14 days unless there is new activity

@github-actions github-actions bot added the stale label Oct 26, 2023
@lawndoc
Copy link

lawndoc commented Jan 29, 2024

While the tests are now passing. We would still have to move the tools of the default docker images out of the mount point.

Otherwise these docker images end up with an empty tool_cache and the preinstalled node etc. are away.

Why do you need the mount point to be empty? Doesn't docker initialize new named volumes using the contents of the image? Or am I misunderstanding the issue?

@ChristopherHX
Copy link
Contributor

ChristopherHX commented Jan 29, 2024

@lawndoc Thanks to remind me about this, yes I should update the default image and get this merged.

Doesn't docker initialize new named volumes using the contents of the image? Or am I misunderstanding the issue?

the toolcache volume is long term persisting data. This volume already exists for all used act atleast once via docker backend without the data on that location of the image.

We have a default image with /opt/hostedtoolcache/.../node/bin in PATH, so the most likly already existing volume is empty then you see error node not in PATH.

Even setup-node needs node in PATH or the workflow fails.

The solution would be to not store any required files inside that location.

Using the small images of the image survey will also lead to an empty toolcache folder

@ChristopherHX ChristopherHX reopened this Jan 29, 2024
@ChristopherHX
Copy link
Contributor

The large images also have files on these location, but not the critical node binary so that is fine.

@ChristopherHX
Copy link
Contributor

This PR should make the location free of files catthehacker/docker_images#121

I believe the toolcache mounting side effects are currently not that small without changing the image. actions/runner uses /__t for some reasons, but the official setup-python action doesn't work using that path.

@lawndoc If you have a better idea let me know.

I don't feel comfortable to have luck with getting the correct content in /opt/hostedtoolcache for builtin tools like node.

My experience with named volumes in docker was, that they are always empty unless the Dockerfile uses the VOLUME keyword. (Could be a wrong knownledge base from my side)

@ChristopherHX
Copy link
Contributor

@lawndoc You are completely right the toolcache is initialized with the content of the container dir if it didn't exist yet...

However if it was initialized non empty (even a empty volume is initialized by docker via a copy from image) it throws node not found error, if the wrong node tool is present in the toolcache volume.

I'm merging this now, default image is patched and confirmed it reduces the problems caused by divergent tool-cache.

You as a potential gitea actions user, might need to wait longer for the next sync.

@ChristopherHX ChristopherHX marked this pull request as ready for review January 30, 2024 21:40
Copy link
Contributor

mergify bot commented Jan 30, 2024

KnisterPeter this pull request has failed checks 🛠

I want to delete these messages...

@mergify mergify bot added the needs-work Extra attention is needed label Jan 30, 2024
@nektos nektos deleted a comment from mergify bot Jan 30, 2024
@ChristopherHX
Copy link
Contributor

Wow good job docker:

2024-01-30T22:06:17.5231148Z     runner_test.go:203: 
2024-01-30T22:06:17.5231557Z         	Error Trace:	/home/runner/work/act/act/pkg/runner/runner_test.go:203
2024-01-30T22:06:17.5232114Z         	            				/home/runner/work/act/act/pkg/runner/runner_test.go:323
2024-01-30T22:06:17.5234123Z         	Error:      	Expected nil, but got: &fmt.wrapError{msg:"failed to create container: 'Error response from daemon: failed to mkdir /var/lib/docker/volumes/act-toolcache/_data/node: mkdir /var/lib/docker/volumes/act-toolcache/_data/node: file exists'", err:errdefs.errSystem{error:(*errors.withStack)(0xc0003fa828)}}
2024-01-30T22:06:17.5234335Z         	Test:       	TestRunEvent/shells/pwsh
2024-01-30T22:06:17.5234722Z         	Messages:   	/home/runner/work/act/act/pkg/runner/testdata/shells/pwsh

looks like the images with data in /opt/hostedtoolcache are affected by concurrency bugs.

Good to make this folder empty with only file permission applied to the volume

@mergify mergify bot removed the needs-work Extra attention is needed label Jan 30, 2024
@mergify mergify bot merged commit 054caec into master Jan 30, 2024
10 checks passed
@mergify mergify bot deleted the fix-toolcache branch January 30, 2024 22:43
@lawndoc
Copy link

lawndoc commented Feb 1, 2024

Thank you for your work on this! It's greatly appreciated!

jmikedupont2 pushed a commit to meta-introspector/act that referenced this pull request Mar 10, 2024
The toolcache on GitHub Actions need to be in
/opt/hostedtoolcache. This is the case for all
environment variables set by act, but it's not the
case for the volume mounted into the container.

Co-authored-by: Björn Brauer <[email protected]>
Co-authored-by: ChristopherHX <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants