FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix macOS VirtioFS issues and hotreload/watcher performance problems by javiertejero · Pull Request #28 · localstack/lambda-runtime-init · GitHub

/ lambda-runtime-init Public
forked from aws/aws-lambda-runtime-interface-emulator

Fix macOS VirtioFS issues and hotreload/watcher performance problems #28

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.

Sign up for GitHub

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

Closed
javiertejero wants to merge 1 commit into from
Conversation 11 Commits 1 Checks 0 Files changed
Closed

Fix macOS VirtioFS issues and hotreload/watcher performance problems #28

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension .go  (1) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
4 changes: 2 additions & 2 deletions cmd/localstack/filenotify/filenotify.go
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ func shouldUseEventWatcher() bool {
err := unix.Uname(&utsname)
release := strings.TrimRight(string(utsname.Release[:]), "\x00")
log.Println("Release detected: ", release)
// cheap check if we are in Docker desktop or not.
// cheap check if we are in Docker desktop for Windows (WSL2) or not.
// We could also inspect the mounts, but that would be more complicated and needs more parsing
return err == nil && !(strings.Contains(release, "linuxkit") || strings.Contains(release, "WSL2"))
return err == nil && !(strings.Contains(release, "WSL2"))

javiertejero Oct 31, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

in my macos I saw above log to print Release detected: 6.4.16-linuxkit so that's why we removed linuxkit - I wonder if this can break any other system such as windows if using linuxkit but it feels safe to me

dfangl Nov 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is here on purpose, as inotify did not work with docker desktop at the time of implementation of this.
Maybe this changed with the default usage of virtioFS, but this would still be a problem for people still on grpc-fuse.
I will test this more thoroughly on our macos machine tomorrow, I just wanted to provide some context here.

Even if it works with virtioFS, but not grpc, we need some mechanism to enable it in one but not the other.

javiertejero Nov 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

thank you @dfangl !

I confirm it works for me with both VirtioFS and gRPC FUSE in my macosx (Apple M2 Pro) and with Docker Desktop 4.25.0 (126437)

hope it works for you too

javiertejero Nov 12, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

perhaps 1 year ago when the code was written it was not working as expected, but it doesn't seem to be the case with the last versions of Docker Desktop

uriyakadmon Nov 15, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi @javiertejero :) This looks exactly what I need to solve a similar issue. How can I apply this in my machine?

javiertejero Nov 16, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@uriyakadmon first compile this package, you can use the Makefile in the repo, then you can replace the binary in the localstack but it requires some hacks, and it depends whether you use docker-compose or not... using docker-compose you can replace the old binary with the new in the volume folder ... and that would solve the problem on that machine

uriyakadmon Nov 22, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's exactly what I ended up doing. Thank you!
}

// New tries to use a fs-event watcher, and falls back to the poller if there is an error
Expand Down
Back | FazBrowse Home | New Git URL