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.

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

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

Open
wants to merge 1 commit into
base: localstack
Choose a base branch
from
Open

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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/localstack/filenotify/filenotify.go
View file
Open in desktop
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"))
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@javiertejero javiertejero Nov 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@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

Copy link

Choose a reason for hiding this comment

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

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