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

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

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

Conversation

javiertejero
Copy link

Summary

This pull request aims to resolve performance degradation and instability experienced on Docker Desktop for macOS, particularly when using VirtioFS file sharing and hot reloading on lambdas.

Root Cause

The issue is traced back to the file watch mechanism, which relies on a "polling" strategy as opposed to an "event" based approach. This results in excessive CPU usage that scales proportionally with the number of files in the shared folder. This is especially problematic when the folder contains a substantial number of files due to Python dependencies.

Changes Made

Switched from a polling-based to an event-based file watch mechanism when a linuxkit (assuming it is macOS) is detected

Impact

These changes are expected to significantly improve performance and stability for users working with Docker Desktop on macOS, no only those utilizing VirtioFS but any other file system sharing such as "gRPC FUSE".

// 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

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

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

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!

@javiertejero
Copy link
Author

I have added a comment to a somewhat related issue localstack/localstack#7466 (comment)

I will open a new issue if needed

@joe4dev
Copy link
Member

joe4dev commented Oct 31, 2023

Hi @javiertejero

Thank you for your PR and suggestions. We will discuss the next steps once our full Lambda team is back from vacation. We currently focus on our upcoming LocalStack v3 release.

@javiertejero
Copy link
Author

@dfangl hi, is there anything else I can do to help on the evaluation of this PR?

We have been using the resultant binary in localstack 2.x for some time more than 15 MacBook Pro M1/M2 and it's really useful.

@dfangl
Copy link
Member

dfangl commented Jan 30, 2024

Since this is a major change, and with potential impact on backwards compatibility with older clients, we cannot just merge this.
We want to do more investigation about the settings influencing this, and how to properly detect them ideally. Nevertheless, in the meantime I prepared #29, so you can manually configure the behavior you want using an environment variable. Once it is published, you can use LAMBDA_DOCKER_FLAGS=-e LOCALSTACK_FILE_WATCHER_STRATEGY=event to change the behavior, and can avoid shipping a custom init runtime to all users.

I want to go forward with this PR as well, but as I said, it needs some further work to determine the impact, and will probably be done with the next major release of LocalStack.

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