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

Improve coordinator port config #520

Merged
merged 6 commits into from
Jun 4, 2024
Merged

Conversation

phil-opp
Copy link
Collaborator

@phil-opp phil-opp commented May 30, 2024

  • Allow setting custom control port in coordinator
  • Set default in CLI parsing -> default shown in --help output
  • Add support for custom coordinator control port numbers in CLI

Follow-up to #513

phil-opp added 2 commits May 30, 2024 13:30
This way, the default address is shown as part of the `--help` output.
@phil-opp phil-opp force-pushed the improve-coordinator-port-config branch from 3ab0210 to 142d218 Compare May 30, 2024 13:30
@Gege-Wang
Copy link
Collaborator

Now, we can do this.

  • To make coordinator_control_port configurable.
# remote
ubuntu@ip-172-31-30-172:~/dora$ dora coordinator --port 6014 --control-interface 0.0.0.0 --control-port 6013
Listening for incoming daemon connection on 6014

# local
$ ./target/release/dora check --coordinator-addr 54.234.111.197 --coordinator-port 6013
Dora Coordinator: ok
Dora Daemon: not running
  • Daemon connect to remote coordinator
# remote
ubuntu@ip-172-31-30-172:~/dora$ dora coordinator
Listening for incoming daemon connection on 53290

# local
$ ./target/release/dora daemon --coordinator-addr 54.234.111.197:53290

$ ./target/release/dora check --coordinator-addr 54.234.111.197           1 ↵
Dora Coordinator: ok
Dora Daemon: ok

@Gege-Wang
Copy link
Collaborator

What does it do when we use dora up --coordinator-addr?
When coordinator and daemon are running in remote machine, dora up --coordinator-addr <remote-ip> in local machine doesn't seem to do anything.
When only a coordinator is running in remote machine, the local dora up --coordinator-addr does not seem to be able to start the connection between daemon and coordinator.

# remote 
ubuntu@ip-172-31-30-172:~$ dora coordinator
Listening for incoming daemon connection on 53290

# local
$ ./target/release/dora up --coordinator-addr 54.234.111.197              2 ↵
started dora daemon
daemon not connected after 2.1000001s

Did I misunderstand anything?

@XxChang
Copy link
Collaborator

XxChang commented May 31, 2024

What does it do when we use dora up --coordinator-addr? When coordinator and daemon are running in remote machine, dora up --coordinator-addr <remote-ip> in local machine doesn't seem to do anything. When only a coordinator is running in remote machine, the local dora up --coordinator-addr does not seem to be able to start the connection between daemon and coordinator.

# remote 
ubuntu@ip-172-31-30-172:~$ dora coordinator
Listening for incoming daemon connection on 53290

# local
$ ./target/release/dora up --coordinator-addr 54.234.111.197              2 ↵
started dora daemon
daemon not connected after 2.1000001s

Did I misunderstand anything?

It seems like that start_daemon() use default command which is dora daemon, So querying remote coordinator session will be foiled

@XxChang
Copy link
Collaborator

XxChang commented May 31, 2024

In addition, we should replace

cmd.spawn()

by

    if !cmd.status().wrap_err("failed to run `dora daemon`")?.success() {
        eyre::bail!("failed to start dora daemon");
    }

to capture cli error

@phil-opp
Copy link
Collaborator Author

phil-opp commented May 31, 2024

Thanks for testing!

What does it do when we use dora up --coordinator-addr?

The coordinator-addr is currently only used for checking the connection after starting. So this doesn't really work (yet?).

For now, it's probably best to remove the coordinator-addr argument from dora up and instruct users to use the manual spawn commands for remote deployments. The commands dora coordinator for running the coordinator and dora daemon for running the daemon.

Edit: I just pushed 02b5c7d to remove the coordinator_addr and coordinator_port arguments from dora up.

phil-opp added 3 commits May 31, 2024 18:18
… up`

The `dora up` command always starts a coordinator and daemon in local mode using the default config. For non-standard config and for remote setups, users need to use `dora coordinator` and `dora daemon` directly.
@phil-opp
Copy link
Collaborator Author

In addition, we should replace

cmd.spawn()

by

    if !cmd.status().wrap_err("failed to run `dora daemon`")?.success() {
        eyre::bail!("failed to start dora daemon");
    }

to capture cli error

This would wait until the daemon exits, which we don't want to do here (we want to spawn it in the background).

@XxChang
Copy link
Collaborator

XxChang commented May 31, 2024

In addition, we should replace

cmd.spawn()

by

if !cmd.status().wrap_err("failed to run `dora daemon`")?.success() {
    eyre::bail!("failed to start dora daemon");
}

to capture cli error

This would wait until the daemon exits, which we don't want to do here (we want to spawn it in the background).

Copy that, i read it wrong, sorry

@phil-opp phil-opp requested review from haixuanTao and removed request for haixuanTao June 4, 2024 07:28
Copy link
Collaborator

@haixuanTao haixuanTao left a comment

Choose a reason for hiding this comment

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

Looks great thanks!

@haixuanTao haixuanTao merged commit d4ff586 into main Jun 4, 2024
17 checks passed
@haixuanTao haixuanTao deleted the improve-coordinator-port-config branch June 4, 2024 10:20
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