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

Update BuildKit for cache key memory usage patch #3505

Closed
wants to merge 2 commits into from

Conversation

mikejholly
Copy link
Contributor

@mikejholly mikejholly commented Nov 17, 2023

Updates BK for memory usage fixes in earthly/buildkit#33

@mikejholly mikejholly changed the title Update BK Update BuildKit for cache key memory usage patch Nov 28, 2023
@mikejholly
Copy link
Contributor Author

Note: I ran the repro case for issue #2957 and did not encounter the error.

date > my-file; earthly-dev +foo & earthly-dev +foo

@mikejholly mikejholly marked this pull request as ready for review November 28, 2023 22:53
@mikejholly mikejholly requested a review from a team as a code owner November 28, 2023 22:53
Comment on lines +30 to +31
# ARG bazel_cache = "/root/.cache/bazel"
# CACHE "$bazel_cache"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intended change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I'm not sure why these lines were leading to this failure: https://github.com/earthly/earthly/actions/runs/7023739400/job/19112169282

./e/bazel+build *failed* | --> expandargs readlink -f ./bazel-out
./e/bazel+build *failed* | [no output]
./e/bazel+build *failed* | ERROR examples/bazel/Earthfile line 33:4
./e/bazel+build *failed* |       The command
./e/bazel+build *failed* |           expandargs readlink -f ./bazel-out
./e/bazel+build *failed* |       did not complete successfully. Exit code 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should just remove this example altogether. It's not very polished anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either or. The target seems to work without that CACHE command.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah.. well the CACHE was the most important point of the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can look into it further.

@alexcb
Copy link
Collaborator

alexcb commented Nov 29, 2023

I gave this some some more stress testing; by running:

VERSION 0.7

test1:
  FROM alpine:3.17
  COPY my-file .
  RUN true
  RUN md5sum my-file > data
  SAVE ARTIFACT data data

test2:
  FROM alpine:3.17
  COPY my-file .
  RUN true

test3:
  FROM alpine:3.17
  COPY +test1/data .
  RUN cat data

test4:
  FROM +test3
  IF ls data
    BUILD +test1
  END

all:
  BUILD +test1
  BUILD +test2
  BUILD +test3
  BUILD +test4

along with

for i in $(seq 1 100); do /home/alex/gh/earthly/earthly/build/linux/amd64/earthly +all & done

and another process periodically changing the contents of my-file:

while sleep 0.1; do uuidgen > my-file; done

and didn't see any solver issues, so I'm feeling less worried about this change :)

@mikejholly
Copy link
Contributor Author

@alexcb Cool. Thanks for doing that. I've also been running our Core tests against this branch with no apparent issues. https://github.com/earthly/earthly/actions/runs/7024593005?pr=3505

@mikejholly mikejholly marked this pull request as draft December 31, 2023 00:01
@mikejholly mikejholly closed this May 17, 2024
@mikejholly mikejholly deleted the mh/bk-cache-key branch May 17, 2024 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants