-
Notifications
You must be signed in to change notification settings - Fork 197
Add filesystem notification (FSNotify) support #294
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
base: main
Are you sure you want to change the base?
Conversation
f89a058
to
128421e
Compare
128421e
to
816c698
Compare
@@ -60,6 +60,8 @@ service SandboxContext { | |||
rpc Sync(SyncRequest) returns (SyncResponse); | |||
// Send a signal to a process via the PID. | |||
rpc Kill(KillRequest) returns (KillResponse); | |||
// Notify guest of filesystem events from host. | |||
rpc NotifyFileSystemEvent(NotifyFileSystemEventRequest) returns (NotifyFileSystemEventResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should have messages for FileSystemEvent and FileSystemEventList, and have the request type deal with lists for performance. We'd need to work out how do deal with errors... perhaps a list of error responses, where each contains a FileSystemEvent, and an error string. An empty response list means no errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or change it to a stream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A stream is likely the right answer.
|
||
// Test 1: CREATE event | ||
print("Testing CREATE event...") | ||
let createResponse = try await agent.notifyFileSystemEvent(path: "/mnt/test/new_file.txt", eventType: .create) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably test create event on an existing file.
The reasoning is that from the point of view of your macOS filesystem monitor it will:
- Get an event from macOS that a file was created.
- At this point the file exists and should be accessible to the container (not sure what the latency for this is)
- Trigger a create event for the already-existing file.
case .modify: | ||
// Touch file to update timestamp -> generates IN_ATTRIB event | ||
let now = Date() | ||
try FileManager.default.setAttributes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't change the modification date. The modification date was already changed, we just want to notify.
) | ||
|
||
case .create: | ||
// Use chmod with same permissions to generate IN_ATTRIB event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to error out if we can't get permissions.
chmodding a file from 400 to 644 wouldn't be ideal, nor would changing a file from 755 to 644.
@realrajaryan Can you take a peek at the CI fail, it's for the new test added |
Addresses apple/container#141, where containers don't receive filesystem events on mounted volumes, preventing incremental rebuilds and other file-watching features. This PR implements the guest-side components for FSNotify. Host-side implementation in the container repo will complete the pipeline.
Summary:
cctl fsnotify
) and integration test infrastructure