Skip to content

fix: Wasm cache directory on macOS host#5466

Merged
arkodg merged 3 commits intoenvoyproxy:mainfrom
mathetake:wasmdirectory
Mar 12, 2025
Merged

fix: Wasm cache directory on macOS host#5466
arkodg merged 3 commits intoenvoyproxy:mainfrom
mathetake:wasmdirectory

Conversation

@mathetake
Copy link
Copy Markdown
Member

What type of PR is this?

fix: permission error on startWasmCache

What this PR does / why we need it:

Previously, the cache directory for Wasm binaries is hardcoded to /var/lib/eg/wasm but it's not writable without root permission on macOS. This fixes it by making it default to ~/.eg/wasm.

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake mathetake marked this pull request as ready for review March 11, 2025 15:52
@mathetake mathetake requested a review from a team as a code owner March 11, 2025 15:52
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 11, 2025

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 65.22%. Comparing base (36060cb) to head (d884a32).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
internal/gatewayapi/runner/runner.go 0.00% 10 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5466      +/-   ##
==========================================
- Coverage   65.25%   65.22%   -0.03%     
==========================================
  Files         213      213              
  Lines       33923    33928       +5     
==========================================
- Hits        22135    22131       -4     
- Misses      10455    10463       +8     
- Partials     1333     1334       +1     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@arkodg arkodg requested a review from zhaohuabing March 11, 2025 20:55
Comment thread internal/gatewayapi/runner/runner.go Outdated
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team March 11, 2025 21:39
Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@arkodg arkodg merged commit 7e1f57d into envoyproxy:main Mar 12, 2025
@mathetake
Copy link
Copy Markdown
Member Author

@zhaohuabing @arkodg /var/lib/eg seems like a bad choice on non default Envoy container so I think I would like to make it configurable via a command line arg like --wasm-cache-dir. wdyt

2025-03-12T17:13:58.812Z ERROR gateway-api runner/runner.go:110 Failed to create Wasm cache directory {"runner": "gateway-api", "error": "mkdir /var/lib/eg: permission denied"}

https://github.com/envoyproxy/ai-gateway/actions/runs/13817304087/job/38653772222

@mathetake mathetake deleted the wasmdirectory branch March 12, 2025 17:42
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Mar 12, 2025

@zhaohuabing has more context on the design, can you help with this, relates to

RUN mkdir -p /var/lib/eg

@mathetake
Copy link
Copy Markdown
Member Author

or is it possible to disable the file cache at all when the dir is not available (it should be easy if the abstraction is right at the moment? )

@zhaohuabing
Copy link
Copy Markdown
Member

@zhaohuabing has more context on the design, can you help with this, relates to

The design considerations are outlined in this PR description: #3564 (comment) . Some of them definitely have room for improvement.

or is it possible to disable the file cache at all when the dir is not available (it should be easy if the abstraction is right at the moment? )

Just disabling the cache will break the Wasm feature, it can not function without the cache. In the initial design, we don't want to put Wasm in memory because it can lead to unexpected OOM of EG.

Our choices are:

  • Make the cache directory configurable, or
  • Disable the Wasm feature as a whole

I prefer the first option, as we need to make this feature work in VM deployment where this directory may not exist.

@mathetake
Copy link
Copy Markdown
Member Author

Yeah let's make it configurable. Passing it as an argument of envoy-gateway CLI makes sense to you ?

@zhaohuabing
Copy link
Copy Markdown
Member

Yeah let's make it configurable. Passing it as an argument of envoy-gateway CLI makes sense to you ?

Should we put this in the EnvoyGateway API? cc @arkodg ?

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Mar 13, 2025

yeah EnvoyGateway is fine, since macOS is a dev only case, can we just use /tmp/eg for it ?

@mathetake
Copy link
Copy Markdown
Member Author

If /tmo/eg works here then we won't need to add an option in the first place ?

@mathetake
Copy link
Copy Markdown
Member Author

Just to clarify that it's not only about Mac but also non root Linux env as well (see the CI failure I pasted above, it's on Ubuntu GHA)

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Mar 13, 2025

Ah in that case we can switch on host/k8s provider

@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Mar 13, 2025

On host mode defaulting to ~/.eg/wasm (or /tmp/eg) and On k8s, /var/eg/wasm as before; how's that sound

@mathetake
Copy link
Copy Markdown
Member Author

(deleted the confusing comment)

@zhaohuabing
Copy link
Copy Markdown
Member

Should be ~/.eg/wasm as the cached Wasm files are not temp files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants