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

Add watch dir recursive option #210

Closed
wants to merge 5 commits into from
Closed

Conversation

dobrac
Copy link

@dobrac dobrac commented Dec 2, 2024

Add recursive directory watch support to the envd. It uses not yet released recursive feature of fsnotify library. This library was forked to https://github.com/e2b-dev/fsnotify and is now included instead of the original one.

Updated API with the recursive parameter as:

message CreateWatcherRequest {
    string path = 1;
    bool recursive = 2;
}

It should be backwards compatible, as all fields in protobuf are optional. The default value is recursive=false.

Important restriction
This change limits the compatibility to Linux, Windows only.

Description by Callstackai

This PR adds recursive directory watch support to the envd, utilizing a forked version of the fsnotify library. It introduces a new recursive parameter in the WatchDirRequest and CreateWatcherRequest messages, ensuring backward compatibility.

Diagrams of code changes
sequenceDiagram
    participant Client
    participant FileSystem Service
    participant FSNotify Watcher

    Client->>FileSystem Service: CreateWatcher(path, recursive)
    FileSystem Service->>FSNotify Watcher: Create new watcher
    FSNotify Watcher-->>FileSystem Service: Return watcher instance
    
    alt recursive watching
        FileSystem Service->>FSNotify Watcher: Add path with "..." suffix
    else non-recursive watching
        FileSystem Service->>FSNotify Watcher: Add path without suffix
    end
    
    FileSystem Service-->>Client: Return watcher_id
    
    loop File System Events
        FSNotify Watcher->>FileSystem Service: Emit events (Create/Write/Remove/Rename/Chmod)
        FileSystem Service->>Client: Stream events
    end
    
    Client->>FileSystem Service: RemoveWatcher(watcher_id)
    FileSystem Service->>FSNotify Watcher: Close watcher
    FileSystem Service-->>Client: Confirm removal
Loading
Files Changed
FileSummary
go.work.sumUpdated dependencies to include the new fsnotify library.
packages/envd/internal/services/filesystem/watch.goModified the WatchDir function to use the new recursive path utility.
packages/envd/internal/services/filesystem/watch_sync.goUpdated CreateFileWatcher to accept a recursive parameter.
packages/envd/internal/services/spec/filesystem/filesystem.pb.goAdded recursive field to WatchDirRequest and CreateWatcherRequest messages.
packages/envd/internal/utils/rfsnotify.goCreated a utility function to handle recursive path creation.
packages/envd/main.goUpdated version number to reflect changes.
packages/envd/spec/filesystem/filesystem.protoUpdated proto definitions to include recursive parameter.

This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions .sum, .go, .proto. See list of supported languages.

Copy link
Contributor

@0div 0div left a comment

Choose a reason for hiding this comment

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

Same comment as for SDK PR:

From a product perspective, do you think it would be better not to make the recursive an option always and just always watch recursively ? Also taking into account that the instructions said:

Make as few changes to the communication between envd and SDKs as possible

packages/envd/internal/utils/rfsnotify.go Show resolved Hide resolved
@dobrac dobrac force-pushed the watch-dir-recursive branch from 73bca37 to 3f9967f Compare December 19, 2024 08:55
@jakubno jakubno changed the base branch from main to dev December 19, 2024 16:21
@jakubno jakubno deleted the branch e2b-dev:dev January 1, 2025 17:09
@jakubno jakubno closed this Jan 1, 2025
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