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

Support for inotify in mounted directories #1913

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

balajiv113
Copy link
Member

@balajiv113 balajiv113 commented Oct 13, 2023

Fixes #615

Tasks

  • Basic testing with vz (reverse-sshfs, virtiofs)
  • Basic testing with qemu macOS (reverse-sshfs, 9p)
  • Basic testing with qemu linux (Need help!!)
  • Basic testing with wsl windows (Need help!!, Need to see if this is needed/wsl mount already supports this)
  • Explore alternate for chmod

Approaches tried for triggering inotify in guest

chtimes/touch (Currently used)

  • No recursion issue / vim events are also triggered
  • But have issues with IDE (intellij keeps telling load file from system). Fixed by passing through host time

chmod

  • No recursion issue / vim events are also triggered
  • IDE don't have any problem
  • This deals with permissions of files in host (It would be better if we can find a alternate)

Do suggest if there are someother approach for this.

go.mod Outdated Show resolved Hide resolved
@aeifn
Copy link

aeifn commented Oct 22, 2023

One drawback I noticed: sometimes — say twice an hour — VM (vz,virtfs) crashes silently while intensive haskell build inside VM in mounted directory.

No such problem is observed on the stable branch.

@balajiv113
Copy link
Member Author

@aeifn Yes yes i am working on optimising it and address these crashes on high load

@balajiv113
Copy link
Member Author

@aeifn - Can you give a try now.
Hopefully the silent crash should not happen now

@balajiv113 balajiv113 force-pushed the inotify branch 2 times, most recently from a36e2fd to 61eea4f Compare November 7, 2023 05:37
@balajiv113 balajiv113 marked this pull request as ready for review November 7, 2023 05:37
@balajiv113
Copy link
Member Author

@AkihiroSuda
Marking this as ready for review. As all the required features are added.

I have tested this feature with reverse-sshfs, 9p, virtiofs on macOS. All these cases are working fine without issues.
Also done long running and average load test, as of now i don't see any crashes.

But might some help to test on linux and windows.

@AkihiroSuda AkihiroSuda modified the milestones: v1.0, v0.19.0 Nov 17, 2023
@balajiv113
Copy link
Member Author

Just a FYI,
In terms of CPU. With inotify enabled am seeing some slight increase in CPU during normal usage.

Am testing out GRPC for communication. I will create a follow-up PR to address this issue. I will keep performance out of scope for this PR.

@jandubois
Copy link
Member

I tried to test it on macOS 12.7.3 (Monterey) on Intel, and it didn't work for me at all, both using Alpine and Ubuntu (default), with either reverse-sshfs or 9p:

jan@lima-default:/Users/jan/suse/lima$ sudo -i
root@lima-default:~# inotifywait -m /tmp/lima
Setting up watches.
Watches established.

I then run touch /tmp/lima/foo on the host, and the file is visible inside the guest, but inotifywait doesn't report anything.

limactl start confirms that mountInotify has been set:

$ l edit
Waiting for Emacs...
WARN[0021] `mountType: 9p` is experimental
WARN[0021] `mountInotify` is experimental
INFO[0021] Instance "default" configuration edited
? Do you want to start the instance now?  Yes

@jandubois
Copy link
Member

I tried to test it on macOS 12.7.3 (Monterey) on Intel,

Same thing happens for me on macOS 14.3.1 (Sonoma) on M1: I don't get any inotify events (inside the VM) for files touched on the host, but I do get events for files touched inside the VM.

Do I misunderstand how this is supposed to work? I expected that touch a file on the host would cause the guestagent to touch the file inside the VM, triggering an inotify event that could be captured by applications supporting hot-reload.

I guess I can give it one more try using VZ, but I suspect I must be doing something else wrong.

@jandubois
Copy link
Member

I guess I can give it one more try using VZ, but I suspect I must be doing something else wrong.

Same results with VZ.

Only interesting observation was seeing these 2 lines during startup:

ERRO[0055] [hostagent] r.CreateEndpoint() = connection was refused
ERRO[0056] [hostagent] r.CreateEndpoint() = connection was refused

I don't think I've seen them before; could they be related?

@balajiv113
Copy link
Member Author

@jandubois Do you see any logs like
enable inotify for writable mount <path> in ha logs ??

@jandubois
Copy link
Member

Do you see any logs like enable inotify for writable mount <path> in ha logs ??

@balajiv113 Yes, I do:

$ ag inoti
lima.yaml
97:# Enable inotify support for mounted directories (EXPERIMENTAL)
99:mountInotify: true

ha.stderr.log
36:{"level":"info","msg":"enable inotify for writable mount: /tmp/lima","time":"2024-02-29T17:30:42-08:00"}
43:{"level":"info","msg":"enable inotify for writable mount: /Volumes/ThunderBlade","time":"2024-02-29T17:30:42-08:00"}

@balajiv113
Copy link
Member Author

Found the issue, /tmp/lima is a symlink to /private/tmp/lima.
Due to this it is not able to find a similar file in guest

Thanks for the find, Will fix it. I was trying with home folder didn't encounter this :(

@balajiv113
Copy link
Member Author

Fixed the symlink issue.

@jandubois - Can you try actually editing the file and see if that triggers a event. Instead of just touch.

The problem is we are relying on notify.Create|notify.Write events. These are internally FSEventsCreated|FSEventsModified.

When we do a touch, it triggers FSEventsInodeMetaMod and not FSEventsModified. If we rely on FSEventsInodeMetaMod we will be getting too many events (As we are doing touch in guest, which triggers a cycle of events).

For now, I believe FSEventsModified should be enough to triggers events on file change.

Note: Am not adding it to document, Because at some usecase, even touch will work. For instance,
edit a file and save -> notice a event is triggered -> now do touch file -> notice a event is triggered

@AkihiroSuda
Copy link
Member

@jandubois PTAL 🙏

@jandubois
Copy link
Member

@balajiv113 I can confirm that the symlink issue seems to be fixed.

I do get different results for touch vs. edit though.

E.g. when I touch foo on the host, I get /tmp/lima/ ATTRIB foo from inotifywait.

But when I then write to the file with e.g. cp ~/foo /tmp/lima/foo, I don't get any event even though the file content changed. This looks wrong.

This only happens for empty files. If the file contains data, and it is overwritten, then I do get a notification.

Also it looks like ATTRIB is the only notification I get. When I modify files in the same shared directory from a second shell from inside the VM, then I get the usual ACCESS, OPEN, CREATE, MODIFY, CLOSE notifications as well. So I wonder if ATTRIB will be enough to trigger hot-reload with the various applications using inotify.

I'm sorry, I only got to try the PR, I haven't yet found a big enough chunk of time to review the code in one sitting... 😞

@balajiv113
Copy link
Member Author

Also it looks like ATTRIB is the only notification I get.

This is intended. The reason is when we need to replicate the same pattern we will have complexity of handling the duplicate event in host. All we do is just touch the file in guest agent

So I wonder if ATTRIB will be enough to trigger hot-reload with the various applications using inotify

I checked with some of the frontend hot reload like nextjs and it worked fine with attrib

But when I then write to the file with e.g. cp ~/foo /tmp/lima/foo, I don't get any event

I will check this case

@balajiv113 balajiv113 force-pushed the inotify branch 2 times, most recently from 266143d to d25f948 Compare March 8, 2024 06:58
@balajiv113
Copy link
Member Author

@jandubois
I have done changes to support FSEventsInodeMetaMod. All use-cases should work fine. Even touch with empty file should work now

Note: As said above, We will always receive ATTRIB notification only.

@balajiv113 balajiv113 force-pushed the inotify branch 2 times, most recently from a471954 to 6724132 Compare March 8, 2024 07:10
@AkihiroSuda
Copy link
Member

@jandubois PTAL

@jandubois
Copy link
Member

Even touch with empty file should work now

I can confirm that it works now.

I wonder if removing a file should send a notification or not. Because for me it doesn't. And if I remove a file from a webapp, shouldn't it trigger a hot reload?

If this is hard/complex, then maybe documenting this as a limitation would be fine too.

@balajiv113
Copy link
Member Author

I wonder if removing a file should send a notification or not

Our current model is to touch the file so that the guest consider something changed.
With this approach we cannot support remove file logic. As file is removed and we cannot trigger a touch

maybe documenting this as a limitation would be fine too

Will document as a limitation

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks, let's merge this

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

Successfully merging this pull request may close these issues.

File system change events (inotify) support for mounted directories
5 participants