From cd3a71d1cd02804971cdec857d8913fab5e081e8 Mon Sep 17 00:00:00 2001 From: Prithvi Shahi Date: Tue, 10 Nov 2020 17:31:32 -0800 Subject: [PATCH 1/2] refactor: write replica pid to file to fix flaky test --- e2e/bats/start.bash | 11 +---------- e2e/bats/utils/_.bash | 10 +++++----- src/dfx/src/actors/replica.rs | 8 ++++++++ src/dfx/src/commands/replica.rs | 1 + src/dfx/src/commands/start.rs | 15 ++++++++------- 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/e2e/bats/start.bash b/e2e/bats/start.bash index b51cfb7cfa..522022a5f4 100644 --- a/e2e/bats/start.bash +++ b/e2e/bats/start.bash @@ -23,16 +23,7 @@ teardown() { assert_command dfx canister call hello greet '("Alpha")' assert_eq '("Hello, Alpha!")' - # ps "isn't a robust solution": https://dfinity.atlassian.net/browse/OPS-166 - # - # I guess we could make dfx write the replica pid somewhere. - # - # Anyway, if this test ends up sometimes killing a replica other than - # the one created by dfx for this test, then some other test might fail. - # - # Also, this does not work on linux: - # ps -o "ppid, pid, comm" - REPLICA_PID=$(ps x | grep [/[:space:]]replica | awk '{print $1}') + REPLICA_PID=$(cat .dfx/replica-configuration/replica-pid) echo "replica pid is $REPLICA_PID" diff --git a/e2e/bats/utils/_.bash b/e2e/bats/utils/_.bash index 2306f26910..00135654cc 100644 --- a/e2e/bats/utils/_.bash +++ b/e2e/bats/utils/_.bash @@ -31,7 +31,7 @@ dfx_new() { echo PWD: $(pwd) >&2 } -# Start the client in the background. +# Start the replica in the background. dfx_start() { if [ "$USE_IC_REF" ] then @@ -62,10 +62,10 @@ dfx_start() { dfx start --background "$@" 3>&- fi local project_dir=${pwd} - local dfx_config_root=.dfx/client-configuration + local dfx_config_root=.dfx/replica-configuration printf "Configuration Root for DFX: %s\n" "${dfx_config_root}" - test -f ${dfx_config_root}/client-1.port - local port=$(cat ${dfx_config_root}/client-1.port) + test -f ${dfx_config_root}/replica-1.port + local port=$(cat ${dfx_config_root}/replica-1.port) # Overwrite the default networks.local.bind 127.0.0.1:8000 with allocated port local webserver_port=$(cat .dfx/webserver-port) @@ -80,7 +80,7 @@ dfx_start() { || (echo "could not connect to replica on port ${port}" && exit 1) } -# Stop the client and verify it is very very stopped. +# Stop the replica and verify it is very very stopped. dfx_stop() { if [ "$USE_IC_REF" ] then diff --git a/src/dfx/src/actors/replica.rs b/src/dfx/src/actors/replica.rs index c79e0e2375..d3ce6e90d8 100644 --- a/src/dfx/src/actors/replica.rs +++ b/src/dfx/src/actors/replica.rs @@ -49,6 +49,7 @@ pub struct Config { pub replica_path: PathBuf, pub shutdown_controller: Addr, pub logger: Option, + pub replica_configuration_dir: PathBuf, } /// A replica actor. Starts the replica, can subscribe to a Ready signal and a @@ -116,6 +117,7 @@ impl Replica { // Create a replica config. let config = &self.config.replica_config; + let replica_pid_path = self.config.replica_configuration_dir.join("replica-pid"); let port = config.http_handler.port; let write_port_to = config.http_handler.write_port_to.clone(); @@ -131,6 +133,7 @@ impl Replica { write_port_to, ic_starter_path, replica_path, + replica_pid_path, addr, receiver, )?; @@ -253,6 +256,7 @@ fn replica_start_thread( write_port_to: Option, ic_starter_path: PathBuf, replica_path: PathBuf, + replica_pid_path: PathBuf, addr: Addr, receiver: Receiver<()>, ) -> DfxResult> { @@ -298,6 +302,10 @@ fn replica_start_thread( debug!(logger, "Starting replica..."); let mut child = cmd.spawn().expect("Could not start replica."); + std::fs::write(&replica_pid_path, "").expect("Could not write to replica-pid file."); + std::fs::write(&replica_pid_path, child.id().to_string()) + .expect("Could not write to replica-pid file."); + let port = port.unwrap_or_else(|| { Replica::wait_for_port_file(write_port_to.as_ref().unwrap()).unwrap() }); diff --git a/src/dfx/src/commands/replica.rs b/src/dfx/src/commands/replica.rs index 909a5d7d0a..4bcd3d7919 100644 --- a/src/dfx/src/commands/replica.rs +++ b/src/dfx/src/commands/replica.rs @@ -136,6 +136,7 @@ pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { replica_path: replica_pathbuf, shutdown_controller, logger: Some(env.get_logger().clone()), + replica_configuration_dir: env.get_temp_dir().join("replica-configuration"), }) .start(); diff --git a/src/dfx/src/commands/start.rs b/src/dfx/src/commands/start.rs index dc620ed82c..8c11969dca 100644 --- a/src/dfx/src/commands/start.rs +++ b/src/dfx/src/commands/start.rs @@ -197,25 +197,26 @@ fn start_replica( let ic_starter_path = env.get_cache().get_binary_command_path("ic-starter")?; let temp_dir = env.get_temp_dir(); - let client_configuration_dir = temp_dir.join("client-configuration"); - fs::create_dir_all(&client_configuration_dir)?; + let replica_configuration_dir = temp_dir.join("replica-configuration"); + fs::create_dir_all(&replica_configuration_dir)?; let state_dir = temp_dir.join("state/replicated_state"); fs::create_dir_all(&state_dir)?; - let client_port_path = client_configuration_dir.join("client-1.port"); + let replica_port_path = replica_configuration_dir.join("replica-1.port"); - // Touch the client port file. This ensures it is empty prior to + // Touch the replica port file. This ensures it is empty prior to // handing it over to the replica. If we read the file and it has - // contents we shall assume it is due to our spawned client + // contents we shall assume it is due to our spawned replica // process. - std::fs::write(&client_port_path, "")?; + std::fs::write(&replica_port_path, "")?; - let replica_config = ReplicaConfig::new(state_root).with_random_port(&client_port_path); + let replica_config = ReplicaConfig::new(state_root).with_random_port(&replica_port_path); let actor_config = actors::replica::Config { ic_starter_path, replica_config, replica_path, shutdown_controller, logger: Some(env.get_logger().clone()), + replica_configuration_dir, }; Ok(actors::replica::Replica::new(actor_config).start()) } From 16560b99072d3a99cea9249bf1cf30f475d06b64 Mon Sep 17 00:00:00 2001 From: Prithvi Shahi Date: Tue, 10 Nov 2020 17:44:25 -0800 Subject: [PATCH 2/2] give path to replia-pid file in assert_no_dfx_start_or_replica_processes --- e2e/bats/utils/assertions.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/bats/utils/assertions.bash b/e2e/bats/utils/assertions.bash index 0d726ea911..4faca9a149 100644 --- a/e2e/bats/utils/assertions.bash +++ b/e2e/bats/utils/assertions.bash @@ -149,7 +149,7 @@ assert_process_exits() { # Asserts that `dfx start` and `replica` are no longer running assert_no_dfx_start_or_replica_processes() { ! ( ps | grep "[/[:space:]]dfx start" ) - ! ( ps | grep "[/[:space:]]replica" ) + ! ( ps | cat .dfx/replica-configuration/replica-pid ) } assert_file_eventually_exists() {