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

Qorb integration as connection pool for database #5876

Merged
merged 21 commits into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion nexus/db-queries/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ strum.workspace = true
swrite.workspace = true
thiserror.workspace = true
tokio = { workspace = true, features = ["full"] }
uuid.workspace = true
url.workspace = true
usdt.workspace = true
uuid.workspace = true

db-macros.workspace = true
nexus-auth.workspace = true
Expand Down
8 changes: 5 additions & 3 deletions nexus/db-queries/src/db/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ fn make_postgres_connector(
// - Creating async_bb8_diesel connections that also wrap DTraceConnections.
let user = "root";
let db = "omicron";
let args = Some("sslmode=disable");
let args = vec![("sslmode", "disable")];
Arc::new(DieselPgConnector::new(
log,
DieselPgConnectorArgs { user, db, args },
Expand Down Expand Up @@ -125,9 +125,11 @@ impl Pool {
}

/// Creates a new qorb-backed connection pool which returns an error
/// if claims are not quickly available.
/// if claims are not available within one millisecond.
///
/// This is intended for test-only usage.
/// This is intended for test-only usage, in particular for tests where
/// claim requests should rapidly return errors when a backend has been
/// intentionally disabled.
#[cfg(any(test, feature = "testing"))]
pub fn new_single_host_failfast(
log: &Logger,
Expand Down
41 changes: 26 additions & 15 deletions nexus/db-queries/src/db/pool_connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use diesel::PgConnection;
use diesel_dtrace::DTraceConnection;
use qorb::backend::{self, Backend, Error};
use slog::Logger;
use url::Url;

pub type DbConnection = DTraceConnection<PgConnection>;

Expand All @@ -22,14 +23,15 @@ pub const DISALLOW_FULL_TABLE_SCAN_SQL: &str =
/// A [backend::Connector] which provides access to [PgConnection].
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This whole implementation is taken from https://github.com/oxidecomputer/qorb/blob/master/src/connectors/diesel_pg.rs -- I originally made the implementation there as an example, and to make sure I didn't break anything, but IMO it makes sense for Omicron to have total control over this (e.g., the layering, the control of full table scans, etc).

pub(crate) struct DieselPgConnector {
log: Logger,
prefix: String,
suffix: String,
user: String,
db: String,
args: Vec<(String, String)>,
}

pub(crate) struct DieselPgConnectorArgs<'a> {
pub(crate) user: &'a str,
pub(crate) db: &'a str,
pub(crate) args: Option<&'a str>,
pub(crate) args: Vec<(&'a str, &'a str)>,
}

impl DieselPgConnector {
Expand All @@ -47,20 +49,29 @@ impl DieselPgConnector {
let DieselPgConnectorArgs { user, db, args } = args;
Self {
log: log.clone(),
prefix: format!("postgresql://{user}@"),
suffix: format!(
"/{db}{}",
args.map(|args| format!("?{args}")).unwrap_or("".to_string())
),
user: user.to_string(),
db: db.to_string(),
args: args
.into_iter()
.map(|(k, v)| (k.to_string(), v.to_string()))
.collect(),
}
}

fn to_url(&self, address: std::net::SocketAddr) -> String {
format!(
"{prefix}{address}{suffix}",
prefix = self.prefix,
suffix = self.suffix,
)
fn to_url(
&self,
address: std::net::SocketAddr,
) -> Result<String, anyhow::Error> {
let user = &self.user;
let db = &self.db;
let mut url =
Url::parse(&format!("postgresql://{user}@{address}/{db}"))?;
Comment on lines +67 to +68
Copy link
Member

Choose a reason for hiding this comment

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

i really thought there was a builder-style interface for constructing URLs from parts like this, but looking at the url docs, i think it must be the other Rust URL library that has that...


for (k, v) in &self.args {
url.query_pairs_mut().append_pair(k, v);
}

Ok(url.as_str().to_string())
}
}

Expand All @@ -72,7 +83,7 @@ impl backend::Connector for DieselPgConnector {
&self,
backend: &Backend,
) -> Result<Self::Connection, Error> {
let url = self.to_url(backend.address);
let url = self.to_url(backend.address).map_err(Error::Other)?;

let conn = tokio::task::spawn_blocking(move || {
let pg_conn = DbConnection::establish(&url)
Expand Down
Loading