fix(dashmate): invalid drive status check #2248
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,15 @@ | |
| //! RS-Drive-ABCI server starts a single-threaded server and listens to connections from Tenderdash. | ||
|
|
||
| use clap::{Parser, Subcommand}; | ||
| use dapi_grpc::platform::v0::get_status_request; | ||
| use dapi_grpc::platform::v0::get_status_request::GetStatusRequestV0; | ||
| use dapi_grpc::platform::v0::platform_client::PlatformClient; | ||
| use dapi_grpc::tonic::transport::Uri; | ||
| use dpp::version::PlatformVersion; | ||
| use drive_abci::config::{FromEnv, PlatformConfig}; | ||
| use drive_abci::core::wait_for_core_to_sync::v0::wait_for_core_to_sync_v0; | ||
| use drive_abci::logging::{LogBuilder, LogConfig, LogDestination, Loggers}; | ||
| use drive_abci::metrics::Prometheus; | ||
| use drive_abci::platform_types::platform::Platform; | ||
| use drive_abci::rpc::core::DefaultCoreRPC; | ||
| use drive_abci::{logging, server}; | ||
|
|
@@ -17,6 +21,7 @@ use std::fs::remove_file; | |
| use std::net::SocketAddr; | ||
| use std::path::PathBuf; | ||
| use std::process::ExitCode; | ||
| use std::str::FromStr; | ||
| use std::sync::Arc; | ||
| use tokio::runtime::{Builder, Runtime}; | ||
| use tokio::signal::unix::{signal, SignalKind}; | ||
|
|
@@ -98,6 +103,13 @@ impl Cli { | |
| ) -> Result<(), String> { | ||
| match self.command { | ||
| Commands::Start => { | ||
| tracing::info!( | ||
| version = env!("CARGO_PKG_VERSION"), | ||
| features = list_enabled_features().join(","), | ||
| rust = env!("CARGO_PKG_RUST_VERSION"), | ||
| "drive-abci server initializing", | ||
| ); | ||
|
|
||
| if config.drive.grovedb_verify_on_startup { | ||
| verify_grovedb(&config.db_path, false)?; | ||
| } | ||
|
|
@@ -129,10 +141,12 @@ impl Cli { | |
|
|
||
| server::start(runtime, Arc::new(platform), config, cancel); | ||
|
|
||
| tracing::info!("drive-abci server is stopped"); | ||
|
|
||
| return Ok(()); | ||
| } | ||
| Commands::Config => dump_config(&config)?, | ||
| Commands::Status => runtime.block_on(check_status(&config))?, | ||
| Commands::Verify => verify_grovedb(&config.db_path, true)?, | ||
| }; | ||
|
|
||
|
|
@@ -202,13 +216,6 @@ fn main() -> Result<(), ExitCode> { | |
| install_panic_hook(cancel.clone()); | ||
|
|
||
| // Start runtime in the main thread | ||
| let runtime_guard = runtime.enter(); | ||
|
|
||
| runtime.spawn(handle_signals(cancel.clone(), loggers)); | ||
|
|
@@ -219,15 +226,13 @@ fn main() -> Result<(), ExitCode> { | |
| Ok(()) | ||
| } | ||
| Err(e) => { | ||
| tracing::error!(error = e, "drive-abci failed: {e}"); | ||
| Err(ExitCode::FAILURE) | ||
| } | ||
| }; | ||
|
|
||
| drop(runtime_guard); | ||
| runtime.shutdown_timeout(Duration::from_millis(SHUTDOWN_TIMEOUT_MILIS)); | ||
| result | ||
| } | ||
|
|
||
|
|
@@ -303,31 +308,27 @@ fn list_enabled_features() -> Vec<&'static str> { | |
| } | ||
|
|
||
| /// Check status of ABCI server. | ||
| async fn check_status(config: &PlatformConfig) -> Result<(), String> { | ||
| // Convert the gRPC bind address string to a Uri | ||
| let uri = Uri::from_str(&format!("http://{}", config.grpc_bind_address)) | ||
| .map_err(|e| format!("invalid url: {e}"))?; | ||
|
|
||
| // Connect to the gRPC server | ||
| let mut client = PlatformClient::connect(uri.clone()) | ||
| .await | ||
| .map_err(|e| format!("can't connect to grpc server {uri}: {e}"))?; | ||
|
|
||
| // Make a request to the server | ||
| let request = dapi_grpc::platform::v0::GetStatusRequest { | ||
| version: Some(get_status_request::Version::V0(GetStatusRequestV0 {})), | ||
| }; | ||
|
|
||
| // Should return non-zero error code if Drive is not responding | ||
| client | ||
| .get_status(request) | ||
| .await | ||
| .map(|_| ()) | ||
| .map_err(|e| format!("can't request status: {e}")) | ||
|
Comment on lines
+311
to
+331
There was a problem hiding this comment. 🛠️ Refactor suggestion Enhance error handling in The current error handling in the Consider implementing timeouts and more detailed error messages: use tokio::time::{timeout, Duration};
async fn check_status(config: &PlatformConfig) -> Result<(), String> {
// Convert the gRPC bind address string to a Uri
let uri = Uri::from_str(&format!("http://{}", config.grpc_bind_address))
.map_err(|e| format!("Invalid gRPC URL '{}': {}", config.grpc_bind_address, e))?;
// Set a timeout duration
let timeout_duration = Duration::from_secs(5);
// Connect to the gRPC server within the timeout period
let mut client = timeout(
timeout_duration,
PlatformClient::connect(uri.clone())
)
.await
.map_err(|_| format!("Timeout connecting to gRPC server at {}", uri))?
.map_err(|e| format!("Can't connect to gRPC server {}: {}", uri, e))?;
// Create the request
let request = dapi_grpc::platform::v0::GetStatusRequest {
version: Some(get_status_request::Version::V0(GetStatusRequestV0 {})),
};
// Send the request within the timeout period
timeout(timeout_duration, client.get_status(request))
.await
.map_err(|_| "Timeout requesting status from gRPC server".to_string())?
.map(|_| ())
.map_err(|e| format!("Can't request status: {}", e))
}This adds a timeout to both the connection and the request, providing better control over potential network issues. There was a problem hiding this comment. This connection happening in local network. Either port is listening or not. We do not expect timeouts. There was a problem hiding this comment. @shumkov, understood. Since the connection is within the local network and timeouts are not expected, the current implementation should suffice. ✏️ Learnings added
|
||
| } | ||
|
|
||
| /// Verify GroveDB integrity. | ||
|
|
@@ -415,7 +416,7 @@ fn configure_logging(cli: &Cli, config: &PlatformConfig) -> Result<Loggers, logg | |
| if configs.is_empty() || cli.verbose > 0 { | ||
| let cli_config = LogConfig { | ||
| destination: LogDestination::StdOut, | ||
| level: cli.verbose.try_into()?, | ||
| color: cli.color, | ||
| ..Default::default() | ||
| }; | ||
|
|
@@ -442,15 +443,14 @@ fn install_panic_hook(cancel: CancellationToken) { | |
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use ::drive::{drive::Drive, query::Element}; | ||
| use dpp::block::epoch::Epoch; | ||
| use drive::drive::credit_pools::epochs::epoch_key_constants; | ||
| use std::{ | ||
| fs, | ||
| path::{Path, PathBuf}, | ||
| }; | ||
|
|
||
| use dpp::version::PlatformVersion; | ||
| use drive::drive::credit_pools::epochs::paths::EpochProposers; | ||
| use drive_abci::logging::LogLevel; | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Potential undefined environment variable
CARGO_PKG_RUST_VERSIONThe use of
env!("CARGO_PKG_RUST_VERSION")may result in a compilation error becauseCARGO_PKG_RUST_VERSIONis not a standard Cargo-provided environment variable. This could cause the build to fail if the environment variable isn't defined.Consider using the
rustc_versioncrate to obtain the Rust compiler version:Add
rustc_versionto yourCargo.tomldependencies:Modify the code to use
rustc_version:tracing::info!( version = env!("CARGO_PKG_VERSION"), features = list_enabled_features().join(","), - rust = env!("CARGO_PKG_RUST_VERSION"), + rust = rustc_version::version().unwrap().to_string(), "drive-abci server initializing", );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.
This is a standard env.
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.
@shumkov, understood. Thank you for the clarification.
（・ω・）?
✏️ Learnings added