iroh start & stop commands #360

b5 · 2022-10-18T03:25:29Z

supersedes #340. closes n0-computer/beetle#166. closes n0-computer/beetle#158, closes n0-computer/beetle#211

re-re-re worked start & stop commands that effectively do this on start:

#/bin/bash
nohup iroh-store > /dev/null &
nohup iroh-p2p > /dev/null &
nohup iroh-gateway > /dev/null &

all daemons now use lockfiles, and all lockfiles contain the PID of the owning process. iroh stop uses this PID to send SIGINT to each process. iroh stop also makes a best effort to remove stray lockfiles.

TODO:

cc @fabricedesre, this modifies lockfiles, writing the PID to the lockfile itself. I'd like to confirm you're ok with these changes. I'd also like to propose we exit with a non-zero status when a new process can't acquire the lock.

iroh-gateway/src/main.rs

b5 · 2022-10-18T03:27:27Z

iroh-util/src/lock.rs

+/// Attempt to remove a stray lock file that wasn't cleaned up, returns true
+/// if a lock is successfully deleted, and will only attempt to delete if the
+/// lock is not currently held
+pub fn try_cleanup_dead_lock(prog_name: &str) -> Result<bool> {


This needs a test, and I need to review it when I'm not braindead, please scrutinize.

iroh-util/src/lock.rs

localops/Cargo.toml

fabricedesre · 2022-10-18T04:58:53Z

cc @fabricedesre, this modifies lockfiles, writing the PID to the lockfile itself. I'd like to confirm you're ok with these changes. I'd also like to propose we exit with a non-zero status when a new process can't acquire the lock.

Sure, that works for me. I'm not a fan of the binary pid format, but not a hill I want to die on either. Exiting with a non-zero status totally makes sense.

b5 · 2022-10-18T16:57:39Z

@fabricedesre, sounds good. I've changed the format to be a serialized UFT-8 string representation.

b5 · 2022-10-18T16:58:44Z

@faassen I'd appreciate you smoke testing this on linux, iirc you have a fedora distro lying around, right? I'll also be testing on ubuntu

b5 · 2022-10-18T17:31:21Z

iroh-util/src/exitcodes.rs

@@ -0,0 +1,23 @@
+//! iroh exit codes


could also move this into an errors package. I've just crammed it here for now, and don't think it's a big deal to move later once we start getting more rigorous about error categorization. Happy to move it now if someone wants to name the crate this belongs in

faassen · 2022-10-18T17:36:29Z

@faassen I'd appreciate you smoke testing this on linux, iirc you have a fedora distro lying around, right?

Yes, Fedora is my main platform so will do some testing tomorrow. In fact this command will be handy when I do manual testing with the CLI.

b5 · 2022-10-18T17:37:59Z

note that it requires iroh binaries on $PATH. cargo xtask install-dev --build from project root should set you up

ramfox · 2022-10-18T18:15:33Z

Finding two bugs:

If you start up one of the binaries individually, the respective lock is not getting cleaned up on shutdown.
This ignores cfg input, so you can't currently have more than one process running on a single machine. Which puts a big damper on local testing. (previously, you could just pass in a path to a custom config file & have the different binaries use different configuration to ensure no port/rpc conflicts & have multiple iroh setups running at the same time)

The second problem is a way bigger conversation: do we want folks to be able to run more than one setup on their machine, if so how do we want them to do it, where would non-default locks live, etc

dignifiedquire · 2022-10-18T18:16:32Z

localops/Cargo.toml

@@ -0,0 +1,15 @@
+[package]
+name = "localops"


If this is not private, should be iroh-localops for now

can we for now just put this functionality into iroh-util?

I'd heavily prefer iroh-localops, even just for the amount of conditional compilation that needs to go into this crate. Most of this work is about vetting dependencies for cross-platform compatibility

how does that change when putting it in utils? we already have a lot of crates :/

localops/src/process.rs

iroh/src/services.rs

dignifiedquire · 2022-10-18T18:25:15Z

localops/src/process.rs

+}
+
+#[cfg(any(target_os = "macos", target_os = "linux"))]
+fn stop_process(pid: u32) -> Result<()> {


please make all apis that use u32s currently, use a new type Pid, that looks sth like this

#[cfg(unix)] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Pid(nix::Pid); #[cfg(not(unix))] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Pid; // TODO: fill in for each plattform when supported #[cfg(unix)] impl From nix::Pid for Pid { // .. } impl std::fmt::Display for Pid { // .. }

I've marked this as a TODO for later

iroh-gateway/src/main.rs

iroh-p2p/src/main.rs

iroh-rpc-client/src/status.rs

iroh-store/src/main.rs

dignifiedquire · 2022-10-18T18:28:34Z

iroh-util/src/exitcodes.rs

+//! ```
+
+/// Alias for the numeric type that holds system exit codes.
+pub type ExitCode = i32;


please use existing rust types instead of defining new ones :) https://doc.rust-lang.org/std/process/struct.ExitCode.html

From that page:

ExitCode is intended to be consumed only by the standard library (via Termination::report()), and intentionally does not provide accessors like PartialEq, Eq, or Hash. Instead the standard library provides the canonical SUCCESS and FAILURE exit codes as well as From for ExitCode for constructing other arbitrary exit codes.

the definition is irritating:

pub struct ExitCode(imp::ExitCode);

this means you can't define a nonstandard exit code as a constant. I'd argue that std authors are pointing us in the right direction: these are iroh exit codes, and should have their own type.

I'm going to rename the type to IrohExitCode to further disambiguate, and await your clarification on why this isn't the right move 😄

ramfox

mac bug:
iroh start says "starting" and "success", but doesn't seem to create any lock files or launch any processes.

the individual binaries create and remove their lock files appropriately

if I start the individual binaries up, I can use iroh stop to shut them down

iroh stop also seems to have 4 output lines, but should only be reporting about 3 processes.

ramfox · 2022-10-21T03:20:08Z

iroh-util/src/lock.rs

+    /// Failure to parse contents of lock file
+    #[error("Corrupt lock file contents at {0}")]
+    CorruptLock(PathBuf),
+    #[error("Cannot detrmine status of process holding this lock")]


detrmine -> determine

dignifiedquire · 2022-10-21T16:51:42Z

iroh-localops/src/process.rs

+    Err(anyhow!("daemonizing processes on windows is not supported"))
+}
+
+pub fn stop(pid: i32) -> Result<()> {


for later: you don't really need this indirection, you can just cfg the public method

dignifiedquire · 2022-10-21T16:53:11Z

iroh-rpc-client/src/status.rs

+
+        self.iter += 1;
+        if current == None {
+            self.iter = 0;


is it on purpose that this iterator is infinite?

actually it doesn't seem to be, I am confused about this logic here. what are you trying to do?

I mean yes, but now I realize that that doesn't make sense b/c why would you re-use iterators in Rust. I'll remove this reset entirely

dignifiedquire · 2022-10-21T16:55:19Z

iroh-util/src/lock.rs

-/// is already running.
-/// The lock is released either when the object is dropped
-/// or when the program stops.
+pub fn acquire_or_exit(lock: &mut ProgramLock) -> &mut ProgramLock {


this should be a method on ProgramLock

I am confused, why does this exist here and down below again?

tried a few variations, and couldn't get the runtime to hang onto the reference at the call site:

what I wanted:

fn main() { let lock = ProgramLock::new("iroh-gateway")?.acquire_or_exit();

which caused the compiler to yell about an unused variable. changing lock -> _lock means the lock is Dropd right away, an makes the lock not work at all

🤦 forgot to remove this

lock -> _lock means the lock is Dropd right away,

That is not true, this is how drop guards work, they are kept for the current scope. It is only dropped immediately if it is _.

iroh-util/src/lock.rs

dignifiedquire · 2022-10-21T16:57:08Z

iroh-util/src/lock.rs

+            .unwrap_or_else(|| std::ffi::OsStr::new(""))
+            .to_str()
+            .unwrap()
+            .split('.')


Why do you need to split on . here?

.lock extension in the file name. There's a forthcoming method in rust nightly that'll make this much cleaner

dignifiedquire · 2022-10-21T16:58:03Z

iroh-util/src/lock.rs

-        if let Ok(file) = File::create(&self.path) {
-            file_guard::try_lock(&file, Lock::Exclusive, 0, 1).is_err()
+        // path exists, examine lock PID
+        let pid = read_lock(&self.path)?;


this looks like a lot of duplication from is_locked

iroh-util/src/lock.rs

dignifiedquire · 2022-10-21T17:07:18Z

iroh-util/src/lock.rs

    #[test]
    fn test_locks() {
        use nix::unistd::{fork, ForkResult::*};
        use std::io::{Read, Write};
        use std::time::Duration;

-        // Start we no lock file.
+        // Start with no lock file.
        let _ = std::fs::remove_file("test1.lock");


here as well, please use a temp directory

this is more complicated than it seems. I'll make an issue & address in a follow up

dignifiedquire · 2022-10-21T17:08:12Z

iroh/src/services.rs

+/// running.
+pub async fn start(api: &impl Api) -> Result<()> {
+    // start_services(api, HashSet::from(["store"])).await
+    start_services(api, HashSet::from(["store", "p2p", "gateway"])).await


these should really not be strings but an enum

dignifiedquire · 2022-10-21T17:09:13Z

iroh/src/services.rs

+        });
+
+    // construct a new set from the intersection of missing & expected services
+    let missing_services: HashSet<&str> = services


HashSet::difference is your friend

iroh/src/services.rs

dignifiedquire · 2022-10-21T17:10:09Z

iroh/src/services.rs

+
+/// stop all services by sending SIGINT to any active daemons identified
+/// by lockfiles
+pub async fn stop(api: &impl Api) -> Result<()> {


shouldn't this take a configuration of services as well?

b5 added the c-ctl label Oct 18, 2022

b5 self-assigned this Oct 18, 2022

b5 added this to the v0.1.0 milestone Oct 18, 2022

b5 commented Oct 18, 2022

View reviewed changes

b5 mentioned this pull request Oct 18, 2022

feat(cli): start and stop commands #340

Closed

b5 force-pushed the b5/iroh_start_simple branch from f20fc57 to 046d430 Compare October 18, 2022 14:43

b5 marked this pull request as ready for review October 18, 2022 16:56

b5 requested review from dignifiedquire and faassen and removed request for dignifiedquire October 18, 2022 16:57

b5 commented Oct 18, 2022

View reviewed changes