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

WITH DOCKER --cache-id does not expand the --cache-id value #4136

Open
jrodrigv opened this issue May 20, 2024 · 7 comments
Open

WITH DOCKER --cache-id does not expand the --cache-id value #4136

jrodrigv opened this issue May 20, 2024 · 7 comments
Assignees
Labels
type:bug Something isn't working

Comments

@jrodrigv
Copy link

What went wrong?

When using WITH DOCKER --cache-id=id within a target that it is executed N times in parallel all targets are executed sequentially instead due to the locking mechanism.

Code example:

VERSION --docker-cache 0.8

FROM earthly/dind:alpine

test:
    BUILD +test-executor --PROJECT=1 --ID=1
    BUILD +test-executor --PROJECT=2 --ID=2
    BUILD +test-executor --PROJECT=3 --ID=3
    
test-executor:
    ARG PROJECT
    ARG ID

    WITH DOCKER \
        --cache-id=test \
        --pull hello-world
        RUN for i in {1..5}; do sleep 1; echo "hello $PROJECT"; done
    END

What should have happened?

Ideally I think the 1st execution should be locked and executed sequentially but once the docker context has all the required images pulled then the subsequent usages via cache should not be locked because they just need "read access" of the context to reuse the image layers.

What earthly version?

earthly8 version v0.8.11 5caed35 linux/amd64; Ubuntu 20.04.6 LTS (Focal Fossa)

Buildkit Logs

No response

Other Helpful Information

No response

@jrodrigv jrodrigv added the type:bug Something isn't working label May 20, 2024
@alexcb
Copy link
Contributor

alexcb commented May 21, 2024

Thanks for the report.

It looks like your code example defines ARG ID, however never uses it, and instead passes --cache-id=test.

If it's changed to --cache-id=$ID, I I noticed the following:

      +test-executor | aquiring flock for /var/earthly/dind/cache_/.earthly-docker-lock
      +test-executor | aquiring flock for /var/earthly/dind/cache_/.earthly-docker-lock
      +test-executor | aquiring flock for /var/earthly/dind/cache_/.earthly-docker-lock

it appears that the $ID isn't being expanded correctly. Instead I would expect to see 3 different locations:

      +test-executor | aquiring flock for /var/earthly/dind/cache_1/.earthly-docker-lock
      +test-executor | aquiring flock for /var/earthly/dind/cache_2/.earthly-docker-lock
      +test-executor | aquiring flock for /var/earthly/dind/cache_3/.earthly-docker-lock

(note the _1, _2, _3 suffixes)

@alexcb alexcb changed the title WITH DOCKER --cache-id Does not support parallel execution WITH DOCKER --cache-id does not expand the --cache-id value May 21, 2024
@alexcb
Copy link
Contributor

alexcb commented May 21, 2024

I should mention that when the cache-id has the same value, it's intentional that WITH DOCKER does not run in parallel -- this is to avoid corrupting docker's data-root.

@jrodrigv
Copy link
Author

I did it in that way on purpose given that the 3 tests executors are going to use the exact same docker context I thought that it would be a good place to use the cache-id and avoid pulling and loading the very same image layers N times

@alexcb
Copy link
Contributor

alexcb commented May 22, 2024

We still perform a pull each time to make sure the image hasn't changed; however the underlying image layers are cached. We chose to do this deliberately as a way to achieve speedup while still having the process repeatable.

We have a test for the existing behaviour under https://github.com/earthly/earthly/blob/main/tests/with-docker-cache/Earthfile -- the second time it's called we set CHECK_CACHED_TAG_EXISTS=true which validates that the image layer data already exists in the cache.

@jrodrigv
Copy link
Author

Thanks for the information! With "having the process repeatable" you mean across Earthly pipeline executions with caching right? But not within the scope of a single earthly execution.
I was thinking that within the scope of a single earthly execution if you know in advance that the pulled and loaded image layers are not going to change then we could potentially speedup even more by having a flag to avoid pulling the images again? This could be even assumed if earthly is executed with --no-cache

@alexcb
Copy link
Contributor

alexcb commented May 22, 2024

With "having the process repeatable" you mean across Earthly pipeline executions with caching right?

Yes. This also includes cases where you run a target, edit your Earthfile (e.g. changing the --load or --pull flags), and then run it again. If the edit changes what's being loaded it will still persist in the cache, which could cause your RUN command to work on a warm cache but not when there's a cache-miss.

if you know in advance ....

easier said than done :)

@jrodrigv
Copy link
Author

jrodrigv commented May 22, 2024

easier said than done :)

Sorry maybe I didn't express myself clear enough. I meant that if I know in advance (as a dev writing the Earthfile) that the --load and --pull are not going to change during a single earthly execution and on top of that I'm running earthly with --no-cache then if for example I have a FOR that runs the a target 100 times with its 100 WITH DOCKER. By not pulling/loading all the images again I guess that it would speedup the pipeline execution and reduce I/O a lot right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
Status: In Progress
Development

No branches or pull requests

2 participants