Support running in background #72

monthonk · 2023-02-07T16:03:19Z

This change allows the file connector to run in background and it will be running there by default. Users can change this behavior by passing in mount option --foreground or -f to run it in foreground instead.

I'm still figuring out how we can create some tests for this change. Any ideas are welcome. Another thing is that the logs are currently writing to the stdout directly even if the process is running in background. It's a bit annoying but I think that would be a part of PR that addresses #39.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

monthonk · 2023-02-09T14:23:11Z

From this PR, I think AutoUnmount should be set as a default config, otherwise the mount state would be left hanging when we kill the background process.

monthonk · 2023-02-09T16:57:36Z

ASan is not happy with the tests. Any thoughts?

s3-file-connector/Cargo.toml

s3-file-connector/src/main.rs

s3-file-connector/tests/common/mod.rs

s3-file-connector/tests/cli.rs

s3-file-connector/src/main.rs

s3-file-connector/tests/cli.rs

s3-file-connector/src/main.rs

s3-file-connector/tests/cli.rs

s3-file-connector/src/main.rs

Makefile

s3-file-connector/src/main.rs

dannycjones · 2023-02-13T15:04:12Z

s3-file-connector/src/main.rs

+                init_tracing_subscriber();
+
+                let (interrupt_tx, interrupt_rx) = crossbeam::channel::unbounded();
+                std::thread::spawn(move || await_interrupt(interrupt_tx));


Is there a risk this happens too late? What if the mount completes in the child thread?

That's not likely to happen for success case due to network latency. Could be possible in some error cases that send the signal very soon. I'm not sure we should move this to before forking because I think the child process would copy the channel too. @jamesbornholt do you have any suggestions here?

Hmm, yeah, this is unlikely but we should do it properly. That means creating a pipe (pipe2) before the fork, writing a character into the write side of the pipe when the child wants to send its status (probably File::from_raw_fd), and reading a character off the read side of the pipe in the parent to check the status (same). Doing timeouts will be more annoying this way, since you can't configure a timeout for file/pipe reads. Might need to spawn a thread to do it.

s3-file-connector/src/main.rs

Makefile

s3-file-connector/src/main.rs

jamesbornholt · 2023-02-13T16:24:50Z

s3-file-connector/src/main.rs

+                match session {
+                    Ok(_) => {
+                        let _ = nix::sys::signal::kill(parent_pid, Signal::SIGCONT);
+                        thread::park();
+
+                        return Ok(());


This is kind of subtle and we should be more explicit -- the session stays running because its lifetime is bound to the match statement, so doesn't drop until after the park. Probably we should write Ok(_session) and put a comment above the park explaining this.

s3-file-connector/src/main.rs

s3-file-connector/tests/common/mod.rs

s3-file-connector/tests/fuse_tests/fork_test.rs

s3-file-connector/src/main.rs

jamesbornholt · 2023-02-13T18:11:32Z

s3-file-connector/src/main.rs

+                init_tracing_subscriber();
+
+                let (interrupt_tx, interrupt_rx) = crossbeam::channel::unbounded();
+                std::thread::spawn(move || await_interrupt(interrupt_tx));


Hmm, yeah, this is unlikely but we should do it properly. That means creating a pipe (pipe2) before the fork, writing a character into the write side of the pipe when the child wants to send its status (probably File::from_raw_fd), and reading a character off the read side of the pipe in the parent to check the status (same). Doing timeouts will be more annoying this way, since you can't configure a timeout for file/pipe reads. Might need to spawn a thread to do it.

dannycjones

I'm a little uneasy on some of the multi-process logic, not that I understand the area well at this point.

Also, I expect the logic to change anyway with the pipe change.

s3-file-connector/src/main.rs

s3-file-connector/tests/fuse_tests/fork_test.rs

s3-file-connector/src/main.rs

dannycjones · 2023-02-15T17:09:33Z

s3-file-connector/src/main.rs

+                        // also `park()` does not guarantee to remain parked forever. so, we put it inside a loop.
+                        loop {
+                            thread::park();
+                        }


I think we need some kind of stopping condition.

When we receive SIGINT, we should exit. Maybe another thread waits for SIGINT, and sets an atomic bool?

Does this sound right, @jamesbornholt?

I think the standard library already handle that. I have tested this by running kill -2 {pid} which send SIGINT to the running child process and it works just fine.

We need to figure something different out here, otherwise the process runs forever even if the user unmounts it. But we can do that as a followup, I think, because we'll probably have to make fuser changes.

Yeah, that is existing problem, it keep running forever even for foreground process. It's just less visible when run in the background. Let's do it as a follow up.

Let's open an issue and then we can resolve this.

Opened #93.

s3-file-connector/src/main.rs

jamesbornholt · 2023-02-15T18:53:34Z

s3-file-connector/src/main.rs

+                        // also `park()` does not guarantee to remain parked forever. so, we put it inside a loop.
+                        loop {
+                            thread::park();
+                        }


We need to figure something different out here, otherwise the process runs forever even if the user unmounts it. But we can do that as a followup, I think, because we'll probably have to make fuser changes.

s3-file-connector/src/main.rs

jamesbornholt · 2023-02-15T18:57:33Z

s3-file-connector/src/main.rs

+                    // SAFETY: `read_fd` is a valid file descriptor.
+                    let mut f = unsafe { File::from_raw_fd(read_fd) };


Maybe do this conversion outside the thread and right below closing the write_fd, to make clearer the fate of the pipe (and give f a better name)

This change allows the file connector to run in background and it will be running there by default. Users can change this behavior by passing in mount option `--foreground` or `-f` to run it in foreground instead. Signed-off-by: Monthon Klongklaew <[email protected]>

dannycjones

LGTM, pending new issue to follow up on failing to exit if unmounted.

After #72, our file connector will be running in background by default. This breaks the benchmark clean up process because we're now using the wrong pid to kill the process after unmount. Technically, running in background should not be a problem but because of #93 we will have to use `--foreground` flag until we have it fixed. Signed-off-by: Monthon Klongklaew <[email protected]>

* Fix benchmark script After #72, our file connector will be running in background by default. This breaks the benchmark clean up process because we're now using the wrong pid to kill the process after unmount. Technically, running in background should not be a problem but because of #93 we will have to use `--foreground` flag until we have it fixed. Signed-off-by: Monthon Klongklaew <[email protected]> * Update file system name in bench script Signed-off-by: Monthon Klongklaew <[email protected]> --------- Signed-off-by: Monthon Klongklaew <[email protected]>

monthonk force-pushed the running_in_background branch from 3939a18 to 147d0bd Compare February 7, 2023 16:23

dannycjones self-requested a review February 8, 2023 18:04

monthonk force-pushed the running_in_background branch 3 times, most recently from e308c2f to 9b7b556 Compare February 9, 2023 14:14

monthonk force-pushed the running_in_background branch 4 times, most recently from 707c137 to 56f2f9e Compare February 9, 2023 16:39

monthonk requested a review from jamesbornholt February 9, 2023 16:58

dannycjones reviewed Feb 9, 2023

View reviewed changes

jamesbornholt reviewed Feb 9, 2023

View reviewed changes

monthonk force-pushed the running_in_background branch 11 times, most recently from 131cb22 to 9817ad8 Compare February 13, 2023 10:08

monthonk commented Feb 13, 2023

View reviewed changes

s3-file-connector/src/main.rs Outdated Show resolved Hide resolved

monthonk commented Feb 13, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

monthonk requested review from jamesbornholt and dannycjones February 13, 2023 10:23

monthonk force-pushed the running_in_background branch from 9817ad8 to 43703f5 Compare February 13, 2023 11:12

monthonk force-pushed the running_in_background branch from 43703f5 to fdc740d Compare February 13, 2023 12:02

dannycjones reviewed Feb 13, 2023

View reviewed changes

jamesbornholt reviewed Feb 13, 2023

View reviewed changes

monthonk force-pushed the running_in_background branch from fdc740d to 88d87ab Compare February 13, 2023 17:59

jamesbornholt reviewed Feb 13, 2023

View reviewed changes

monthonk force-pushed the running_in_background branch from 88d87ab to d6dc79c Compare February 14, 2023 10:04

dannycjones requested changes Feb 14, 2023

View reviewed changes

s3-file-connector/src/main.rs Show resolved Hide resolved

s3-file-connector/src/main.rs Outdated Show resolved Hide resolved

s3-file-connector/tests/fuse_tests/fork_test.rs Show resolved Hide resolved

monthonk force-pushed the running_in_background branch 5 times, most recently from 00ac0ea to 67cedc7 Compare February 15, 2023 14:48

monthonk requested review from dannycjones and jamesbornholt February 15, 2023 15:28

monthonk force-pushed the running_in_background branch from 67cedc7 to 6e52bb5 Compare February 15, 2023 16:22

dannycjones reviewed Feb 15, 2023

View reviewed changes

monthonk force-pushed the running_in_background branch from 6e52bb5 to d465085 Compare February 15, 2023 18:19

jamesbornholt reviewed Feb 15, 2023

View reviewed changes

monthonk force-pushed the running_in_background branch from d465085 to 17b1af2 Compare February 16, 2023 10:47

dannycjones approved these changes Feb 16, 2023

View reviewed changes

jamesbornholt approved these changes Feb 16, 2023

View reviewed changes

jamesbornholt merged commit f6e936f into main Feb 16, 2023

jamesbornholt deleted the running_in_background branch February 16, 2023 15:42

monthonk mentioned this pull request Feb 16, 2023

Fix benchmark script #94

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support running in background #72

Support running in background #72

monthonk commented Feb 7, 2023

monthonk commented Feb 9, 2023

monthonk commented Feb 9, 2023

dannycjones Feb 13, 2023

monthonk Feb 13, 2023

jamesbornholt Feb 13, 2023

jamesbornholt Feb 13, 2023

jamesbornholt Feb 13, 2023

dannycjones left a comment

dannycjones Feb 15, 2023

monthonk Feb 15, 2023

jamesbornholt Feb 15, 2023

monthonk Feb 16, 2023

dannycjones Feb 16, 2023

monthonk Feb 16, 2023

jamesbornholt Feb 15, 2023

jamesbornholt Feb 15, 2023

dannycjones left a comment

		// SAFETY: `read_fd` is a valid file descriptor.
		let mut f = unsafe { File::from_raw_fd(read_fd) };

Support running in background #72

Support running in background #72

Conversation

monthonk commented Feb 7, 2023

monthonk commented Feb 9, 2023

monthonk commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannycjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannycjones left a comment

Choose a reason for hiding this comment