Skip to content

Commit

Permalink
proxy fix wake compute rate limit (#7902)
Browse files Browse the repository at this point in the history
## Problem

We were rate limiting wake_compute in the wrong place

## Summary of changes

Move wake_compute rate limit to after the permit is acquired. Also makes
a slight refactor on normalize, as it caught my eye
  • Loading branch information
conradludgate committed May 30, 2024
1 parent b0a954b commit 238fa47
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 19 deletions.
2 changes: 1 addition & 1 deletion proxy/src/auth/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use crate::{
},
stream, url,
};
use crate::{scram, EndpointCacheKey, EndpointId, Normalize, RoleName};
use crate::{scram, EndpointCacheKey, EndpointId, RoleName};

/// Alternative to [`std::borrow::Cow`] but doesn't need `T: ToOwned` as we don't need that functionality
pub enum MaybeOwned<'a, T> {
Expand Down
19 changes: 10 additions & 9 deletions proxy/src/console/provider/neon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::{
http,
metrics::{CacheOutcome, Metrics},
rate_limiter::EndpointRateLimiter,
scram, EndpointCacheKey, Normalize,
scram, EndpointCacheKey,
};
use crate::{cache::Cached, context::RequestMonitoring};
use futures::TryFutureExt;
Expand Down Expand Up @@ -281,14 +281,6 @@ impl super::Api for Api {
return Ok(cached);
}

// check rate limit
if !self
.wake_compute_endpoint_rate_limiter
.check(user_info.endpoint.normalize().into(), 1)
{
return Err(WakeComputeError::TooManyConnections);
}

let permit = self.locks.get_permit(&key).await?;

// after getting back a permit - it's possible the cache was filled
Expand All @@ -301,6 +293,15 @@ impl super::Api for Api {
}
}

// check rate limit
if !self
.wake_compute_endpoint_rate_limiter
.check(user_info.endpoint.normalize_intern(), 1)
{
info!(key = &*key, "found cached compute node info");
return Err(WakeComputeError::TooManyConnections);
}

let mut node = permit.release_result(self.do_wake_compute(ctx, user_info).await)?;
ctx.set_project(node.aux.clone());
let cold_start_info = node.aux.cold_start_info;
Expand Down
21 changes: 12 additions & 9 deletions proxy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use std::convert::Infallible;

use anyhow::{bail, Context};
use intern::{EndpointIdInt, EndpointIdTag, InternId};
use tokio::task::JoinError;
use tokio_util::sync::CancellationToken;
use tracing::warn;
Expand Down Expand Up @@ -129,20 +130,22 @@ macro_rules! smol_str_wrapper {

const POOLER_SUFFIX: &str = "-pooler";

pub trait Normalize {
fn normalize(&self) -> Self;
}

impl<S: Clone + AsRef<str> + From<String>> Normalize for S {
impl EndpointId {
fn normalize(&self) -> Self {
if self.as_ref().ends_with(POOLER_SUFFIX) {
let mut s = self.as_ref().to_string();
s.truncate(s.len() - POOLER_SUFFIX.len());
s.into()
if let Some(stripped) = self.as_ref().strip_suffix(POOLER_SUFFIX) {
stripped.into()
} else {
self.clone()
}
}

fn normalize_intern(&self) -> EndpointIdInt {
if let Some(stripped) = self.as_ref().strip_suffix(POOLER_SUFFIX) {
EndpointIdTag::get_interner().get_or_intern(stripped)
} else {
self.into()
}
}
}

// 90% of role name strings are 20 characters or less.
Expand Down

1 comment on commit 238fa47

@github-actions
Copy link

Choose a reason for hiding this comment

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

3238 tests run: 3092 passed, 0 failed, 146 skipped (full report)


Flaky tests (2)

Postgres 16

  • test_vm_bit_clear_on_heap_lock: debug

Postgres 14

  • test_empty_branch_remote_storage_upload: debug

Code coverage* (full report)

  • functions: 31.4% (6489 of 20664 functions)
  • lines: 48.4% (50213 of 103727 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
238fa47 at 2024-05-30T11:28:05.542Z :recycle:

Please sign in to comment.