diff --git a/Cargo.lock b/Cargo.lock index 746eea100f8..c32393fc905 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8927,6 +8927,7 @@ dependencies = [ "indoc", "itertools 0.14.0", "jsonwebtoken", + "libc", "nix 0.29.0", "openssl", "pg-client-config", diff --git a/crates/pipeline-manager/Cargo.toml b/crates/pipeline-manager/Cargo.toml index 71fb96d8ba9..fbe1bfd1323 100644 --- a/crates/pipeline-manager/Cargo.toml +++ b/crates/pipeline-manager/Cargo.toml @@ -5,7 +5,7 @@ keywords = ["DBSP", "streaming", "analytics", "database", "webui"] categories = ["database", "gui"] publish = false -edition = { workspace = true } +edition = "2024" version = { workspace = true } homepage = { workspace = true } repository = { workspace = true } @@ -151,6 +151,7 @@ wiremock = { workspace = true } feldera-types = { workspace = true, features = ["testing"] } itertools = { workspace = true } tokio = { workspace = true, features = ["sync"] } +libc = { workspace = true } # For: asserting errno-to-ErrorKind mapping [package.metadata.cargo-machete] ignored = ["static-files", "compare", "tikv-jemallocator"] diff --git a/crates/pipeline-manager/build.rs b/crates/pipeline-manager/build.rs index a2356761c1c..b79b67fa7cb 100644 --- a/crates/pipeline-manager/build.rs +++ b/crates/pipeline-manager/build.rs @@ -1,5 +1,5 @@ use change_detection::ChangeDetection; -use static_files::{resource_dir, NpmBuild}; +use static_files::{NpmBuild, resource_dir}; use std::env; use std::path::{Path, PathBuf}; use vergen_gitcl::*; diff --git a/crates/pipeline-manager/src/api/demo.rs b/crates/pipeline-manager/src/api/demo.rs index f6afb8e5b64..2fb135c44b5 100644 --- a/crates/pipeline-manager/src/api/demo.rs +++ b/crates/pipeline-manager/src/api/demo.rs @@ -250,7 +250,7 @@ pub fn read_demos_from_directories(demos_dir: &Vec) -> Vec { #[cfg(test)] mod test { - use super::{parse_demo, read_demos_from_directories, Demo, DemoError}; + use super::{Demo, DemoError, parse_demo, read_demos_from_directories}; use std::fs; use std::fs::File; use std::io::Write; diff --git a/crates/pipeline-manager/src/api/endpoints/api_key.rs b/crates/pipeline-manager/src/api/endpoints/api_key.rs index 3f09a02766e..5c888e0492b 100644 --- a/crates/pipeline-manager/src/api/endpoints/api_key.rs +++ b/crates/pipeline-manager/src/api/endpoints/api_key.rs @@ -9,11 +9,10 @@ use crate::db::types::tenant::TenantId; use crate::error::ManagerError; use crate::{api::examples, db::storage::Storage}; use actix_web::{ - delete, get, + HttpRequest, HttpResponse, delete, get, http::header::{CacheControl, CacheDirective}, post, web::{self, Data as WebData, ReqData}, - HttpRequest, HttpResponse, }; use serde::{Deserialize, Serialize}; use tracing::info; diff --git a/crates/pipeline-manager/src/api/endpoints/cluster.rs b/crates/pipeline-manager/src/api/endpoints/cluster.rs index 326b17b2b36..d12001478af 100644 --- a/crates/pipeline-manager/src/api/endpoints/cluster.rs +++ b/crates/pipeline-manager/src/api/endpoints/cluster.rs @@ -7,7 +7,7 @@ use crate::{ error::ManagerError, }; use actix_web::http::header::{CacheControl, CacheDirective}; -use actix_web::{get, web, web::Data as WebData, HttpResponse}; +use actix_web::{HttpResponse, get, web, web::Data as WebData}; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use std::str::FromStr; diff --git a/crates/pipeline-manager/src/api/endpoints/config.rs b/crates/pipeline-manager/src/api/endpoints/config.rs index 4d82cb40d87..1835726701f 100644 --- a/crates/pipeline-manager/src/api/endpoints/config.rs +++ b/crates/pipeline-manager/src/api/endpoints/config.rs @@ -1,8 +1,7 @@ // Configuration API to retrieve the current authentication configuration and list of demos use actix_web::{ - get, + HttpRequest, HttpResponse, get, web::{Data as WebData, ReqData}, - HttpRequest, HttpResponse, }; use feldera_cloud1_client::license::DisplaySchedule; use serde::Serialize; diff --git a/crates/pipeline-manager/src/api/endpoints/metrics.rs b/crates/pipeline-manager/src/api/endpoints/metrics.rs index bfc53bbd0d1..fe38689f9e1 100644 --- a/crates/pipeline-manager/src/api/endpoints/metrics.rs +++ b/crates/pipeline-manager/src/api/endpoints/metrics.rs @@ -2,10 +2,9 @@ use crate::api::main::ServerState; use crate::db::{storage::Storage as _, types::tenant::TenantId}; use crate::error::ManagerError; use actix_web::{ - get, + HttpResponse, get, http::Method, web::{Data as WebData, ReqData}, - HttpResponse, }; use awc::body::MessageBody as _; use feldera_types::runtime_status::RuntimeStatus; @@ -45,10 +44,9 @@ pub(crate) async fn get_metrics( let mut result = Vec::new(); for pipeline in pipelines { - if pipeline.deployment_runtime_status == Some(RuntimeStatus::Running) - || pipeline.deployment_runtime_status == Some(RuntimeStatus::Paused) - { - if let Ok(res) = state + if (pipeline.deployment_runtime_status == Some(RuntimeStatus::Running) + || pipeline.deployment_runtime_status == Some(RuntimeStatus::Paused)) + && let Ok(res) = state .runner .forward_http_request_to_pipeline_by_name( client.as_ref(), @@ -61,14 +59,11 @@ pub(crate) async fn get_metrics( None, ) .await - { - if res.status().is_success() { - if let Ok(bytes) = res.into_body().try_into_bytes() { - result.extend(bytes); - result.push(NEWLINE); - } - } - } + && res.status().is_success() + && let Ok(bytes) = res.into_body().try_into_bytes() + { + result.extend(bytes); + result.push(NEWLINE); } } diff --git a/crates/pipeline-manager/src/api/endpoints/oidc_trust.rs b/crates/pipeline-manager/src/api/endpoints/oidc_trust.rs index e5ffbef7ab6..3fc44ea82c1 100644 --- a/crates/pipeline-manager/src/api/endpoints/oidc_trust.rs +++ b/crates/pipeline-manager/src/api/endpoints/oidc_trust.rs @@ -19,11 +19,10 @@ use crate::db::types::role::{MemberRole, Role}; use crate::db::types::tenant::TenantId; use crate::error::ManagerError; use actix_web::{ - delete, get, + HttpRequest, HttpResponse, delete, get, http::header::{CacheControl, CacheDirective}, post, web::{self, Data as WebData, ReqData}, - HttpRequest, HttpResponse, }; use serde::{Deserialize, Serialize}; use tracing::info; diff --git a/crates/pipeline-manager/src/api/endpoints/pipeline_interaction.rs b/crates/pipeline-manager/src/api/endpoints/pipeline_interaction.rs index e301a571304..df9a7fa3918 100644 --- a/crates/pipeline-manager/src/api/endpoints/pipeline_interaction.rs +++ b/crates/pipeline-manager/src/api/endpoints/pipeline_interaction.rs @@ -9,11 +9,10 @@ use crate::db::types::tenant::TenantId; use crate::error::ManagerError; use actix_http::StatusCode; use actix_web::{ - get, - http::{header, Method}, + HttpRequest, HttpResponse, get, + http::{Method, header}, post, web::{self, Data as WebData, ReqData}, - HttpRequest, HttpResponse, }; #[allow(unused_imports)] use feldera_types::checkpoint::RemoteCheckpoint; diff --git a/crates/pipeline-manager/src/api/endpoints/pipeline_interaction/support_bundle.rs b/crates/pipeline-manager/src/api/endpoints/pipeline_interaction/support_bundle.rs index 6f82421fb29..243eb3bd796 100644 --- a/crates/pipeline-manager/src/api/endpoints/pipeline_interaction/support_bundle.rs +++ b/crates/pipeline-manager/src/api/endpoints/pipeline_interaction/support_bundle.rs @@ -9,14 +9,13 @@ use crate::api::main::ServerState; use crate::api::support_data_collector::{ CollectionSummary, SupportBundleData, SupportBundleParameters, }; -use crate::db::types::combined_status::{combine_since, CombinedDesiredStatus, CombinedStatus}; +use crate::db::types::combined_status::{CombinedDesiredStatus, CombinedStatus, combine_since}; use crate::db::types::pipeline::ExtendedPipelineDescrMonitoring; use crate::db::{storage::Storage, types::tenant::TenantId}; use crate::error::ManagerError; use actix_web::{ - get, + HttpResponse, get, web::{self, Data as WebData, ReqData}, - HttpResponse, }; use chrono::{DateTime, Utc}; use serde::Serialize; @@ -459,11 +458,13 @@ mod tests { assert_eq!(collections.len(), 2); for c in collections { assert!(dirs.contains(c["directory"].as_str().unwrap())); - assert!(c["collected"] - .as_array() - .unwrap() - .iter() - .any(|f| f == "pipeline_events.json")); + assert!( + c["collected"] + .as_array() + .unwrap() + .iter() + .any(|f| f == "pipeline_events.json") + ); } } diff --git a/crates/pipeline-manager/src/api/endpoints/pipeline_management.rs b/crates/pipeline-manager/src/api/endpoints/pipeline_management.rs index ea1616a3922..f06cdf2f356 100644 --- a/crates/pipeline-manager/src/api/endpoints/pipeline_management.rs +++ b/crates/pipeline-manager/src/api/endpoints/pipeline_management.rs @@ -7,7 +7,7 @@ use crate::compiler::{ProgramValidationRequest, ValidateProgramResponse}; use crate::config::CommonConfig; use crate::db::error::DBError; use crate::db::storage::Storage; -use crate::db::types::combined_status::{combine_since, CombinedDesiredStatus, CombinedStatus}; +use crate::db::types::combined_status::{CombinedDesiredStatus, CombinedStatus, combine_since}; use crate::db::types::pipeline::{ ClientMetadata, ExtendedPipelineDescr, ExtendedPipelineDescrMonitoring, PatchClientMetadata, PipelineDescr, PipelineId, @@ -24,17 +24,16 @@ use crate::has_unstable_feature; #[cfg(feature = "feldera-enterprise")] use actix_web::http::Method; use actix_web::{ - delete, get, + HttpResponse, delete, get, http::header::{CacheControl, CacheDirective}, patch, post, put, web::{self, Data as WebData, ReqData}, - HttpResponse, }; use chrono::{DateTime, Utc}; use feldera_types::adapter_stats::PipelineStatsErrorsResponse; use feldera_types::config::{InputEndpointConfig, OutputEndpointConfig, RuntimeConfig}; use feldera_types::error::ErrorResponse; -use feldera_types::pipeline_diff::{compute_pipeline_diff, PipelineDiff}; +use feldera_types::pipeline_diff::{PipelineDiff, compute_pipeline_diff}; use feldera_types::program_schema::ProgramSchema; use feldera_types::runtime_status::{ BootstrapConfig, BootstrapPolicy, ConnectorStats, RuntimeDesiredStatus, RuntimeStatus, @@ -878,12 +877,11 @@ async fn fetch_connector_error_stats( let details = backward_compatible_runtime_status_details( pipeline.deployment_runtime_status_details.clone(), ); - if let Some(value) = details { - if let Ok(details) = serde_json::from_value::(value) { - if let Some(connector_stats) = details.connector_stats { - return Some(connector_stats); - } - } + if let Some(value) = details + && let Ok(details) = serde_json::from_value::(value) + && let Some(connector_stats) = details.connector_stats + { + return Some(connector_stats); }; // Only forward the request if the pipeline is in a valid runtime status diff --git a/crates/pipeline-manager/src/api/endpoints/pipeline_management/pipeline_events.rs b/crates/pipeline-manager/src/api/endpoints/pipeline_management/pipeline_events.rs index 0d5d6385229..b8a1f76998b 100644 --- a/crates/pipeline-manager/src/api/endpoints/pipeline_management/pipeline_events.rs +++ b/crates/pipeline-manager/src/api/endpoints/pipeline_management/pipeline_events.rs @@ -10,10 +10,9 @@ use crate::db::types::storage::StorageStatus; use crate::db::types::tenant::TenantId; use crate::error::ManagerError; use actix_web::{ - get, + HttpResponse, get, http::header::{CacheControl, CacheDirective}, web::{self, Data as WebData, ReqData}, - HttpResponse, }; use chrono::{DateTime, Utc}; use feldera_types::error::ErrorResponse; diff --git a/crates/pipeline-manager/src/api/endpoints/tenant.rs b/crates/pipeline-manager/src/api/endpoints/tenant.rs index 24aae9a2f3b..5333077319e 100644 --- a/crates/pipeline-manager/src/api/endpoints/tenant.rs +++ b/crates/pipeline-manager/src/api/endpoints/tenant.rs @@ -15,11 +15,10 @@ use crate::db::types::tenant::TenantId; use crate::db::types::user::{TenantInfo, UserId}; use crate::error::ManagerError; use actix_web::{ - delete, get, + HttpRequest, HttpResponse, delete, get, http::header::{CacheControl, CacheDirective}, patch, post, put, web::{self, Data as WebData, ReqData}, - HttpRequest, HttpResponse, }; use serde::{Deserialize, Serialize}; use tracing::info; diff --git a/crates/pipeline-manager/src/api/error.rs b/crates/pipeline-manager/src/api/error.rs index 74ccb0add12..2ac7ddb4da6 100644 --- a/crates/pipeline-manager/src/api/error.rs +++ b/crates/pipeline-manager/src/api/error.rs @@ -1,5 +1,5 @@ use actix_web::{ - body::BoxBody, http::StatusCode, HttpResponse, HttpResponseBuilder, ResponseError, + HttpResponse, HttpResponseBuilder, ResponseError, body::BoxBody, http::StatusCode, }; use feldera_types::error::{DetailedError, ErrorResponse}; use serde::Serialize; diff --git a/crates/pipeline-manager/src/api/examples.rs b/crates/pipeline-manager/src/api/examples.rs index 9a6362e18cf..94be63679b9 100644 --- a/crates/pipeline-manager/src/api/examples.rs +++ b/crates/pipeline-manager/src/api/examples.rs @@ -14,13 +14,13 @@ use crate::db::types::program::{CompilationProfile, ProgramConfig, ProgramError, use crate::db::types::resources_status::{ResourcesDesiredStatus, ResourcesStatus}; use crate::db::types::storage::StorageStatus; use crate::db::types::utils::{ - validate_program_config, validate_program_info, validate_runtime_config, PATTERN_KUBERNETES_LABEL_VALUE, PATTERN_KUBERNETES_LABEL_VALUE_DESCRIPTION, + validate_program_config, validate_program_info, validate_runtime_config, }; use crate::db::types::version::Version; use crate::runner::error::RunnerError; use crate::runner::interaction::{ - format_disconnected_error_message, format_timeout_error_message, RunnerInteraction, + RunnerInteraction, format_disconnected_error_message, format_timeout_error_message, }; use feldera_types::config::{DevTweaks, FtConfig, ResourceConfig, StorageOptions}; use feldera_types::runtime_status::{RuntimeStatusDetails, StorageStatusDetails}; diff --git a/crates/pipeline-manager/src/api/main.rs b/crates/pipeline-manager/src/api/main.rs index 8f8d33614f3..a05b3fd1d8d 100644 --- a/crates/pipeline-manager/src/api/main.rs +++ b/crates/pipeline-manager/src/api/main.rs @@ -1,4 +1,4 @@ -use crate::api::demo::{read_demos_from_directories, Demo}; +use crate::api::demo::{Demo, read_demos_from_directories}; use crate::api::endpoints; use crate::api::support_data_collector::SupportDataCollector; use crate::auth::{IssuerJwkCache, JwkCache}; @@ -9,17 +9,16 @@ use crate::error::ManagerError; use crate::license::LicenseCheck; use crate::runner::interaction::RunnerInteraction; use crate::unstable_features; -use actix_http::body::BoxBody; use actix_http::StatusCode; +use actix_http::body::BoxBody; +use actix_web::Scope; use actix_web::body::MessageBody; use actix_web::dev::{Service, ServiceResponse}; -use actix_web::http::{header, Method}; -use actix_web::Scope; +use actix_web::http::{Method, header}; use actix_web::{ - get, middleware, + App, HttpResponse, HttpServer, get, middleware, web::Data as WebData, web::{self}, - App, HttpResponse, HttpServer, }; use actix_web_httpauth::middleware::HttpAuthentication; use actix_web_static_files::ResourceFiles; @@ -29,11 +28,11 @@ use futures_util::FutureExt; use std::io::Write; use std::time::Duration; use std::{env, io, net::TcpListener, sync::Arc}; -use termbg::{theme, Theme}; +use termbg::{Theme, theme}; use tokio::signal; use tokio::sync::watch; use tokio::sync::{Mutex, RwLock}; -use tracing::{error, info, Level}; +use tracing::{Level, error, info}; use utoipa::openapi::security::{HttpAuthScheme, HttpBuilder, SecurityScheme}; use utoipa::{Modify, OpenApi}; use utoipa_swagger_ui::SwaggerUi; @@ -608,13 +607,16 @@ fn build_app( api_config: &ApiServerConfig, auth_configuration: &Option, ) -> App< + // `use<>`: the returned app owns everything it needs, so it must not + // capture the argument lifetimes that edition 2024 would capture by + // default. Callers pass short-lived config to an app that outlives it. impl actix_web::dev::ServiceFactory< actix_web::dev::ServiceRequest, Config = (), - Response = actix_web::dev::ServiceResponse, + Response = actix_web::dev::ServiceResponse>, Error = actix_web::Error, InitError = (), - >, + > + use<>, > { let cors = api_config.cors(); let app = App::new() @@ -1119,7 +1121,7 @@ Version: {} v{}{} }); #[cfg(unix)] { - use tokio::signal::unix::{signal as unix_signal, SignalKind}; + use tokio::signal::unix::{SignalKind, signal as unix_signal}; let mut term_stream = unix_signal(SignalKind::terminate()).expect("SIGTERM handler"); let server_handle_term = server.handle(); tokio::spawn(async move { @@ -1221,10 +1223,11 @@ mod tests { "public, max-age=86400", ); // Only the immutable branch emits ACAO/EXPIRES. - assert!(res - .headers() - .get(header::ACCESS_CONTROL_ALLOW_ORIGIN) - .is_none()); + assert!( + res.headers() + .get(header::ACCESS_CONTROL_ALLOW_ORIGIN) + .is_none() + ); assert!(res.headers().get(header::EXPIRES).is_none()); } @@ -1288,7 +1291,9 @@ mod tests { let res = test::call_service(&app, req).await; assert!( - res.headers().get(header::ACCESS_CONTROL_ALLOW_CREDENTIALS).is_none(), + res.headers() + .get(header::ACCESS_CONTROL_ALLOW_CREDENTIALS) + .is_none(), "static asset leaked Access-Control-Allow-Credentials — actix-cors regressed onto the static scope", ); if let Some(vary) = res.headers().get(header::VARY) { diff --git a/crates/pipeline-manager/src/api/rbac.rs b/crates/pipeline-manager/src/api/rbac.rs index 1fc1b11f53a..acf5b2c3e26 100644 --- a/crates/pipeline-manager/src/api/rbac.rs +++ b/crates/pipeline-manager/src/api/rbac.rs @@ -510,7 +510,7 @@ mod test { #[actix_web::test] async fn middleware_enforces_in_a_real_pipeline() { use actix_web::middleware::from_fn; - use actix_web::{test, web, App, HttpResponse}; + use actix_web::{App, HttpResponse, test, web}; use std::str::FromStr; // Installs a principal whose role comes from the `x-test-role` header, @@ -603,8 +603,8 @@ mod test { /// Paths outside `/v0` are unauthenticated (public scope) and not gated. fn openapi_v0_routes() -> Vec<(String, String)> { use crate::api::main::ApiDoc; - use utoipa::openapi::PathItemType; use utoipa::OpenApi; + use utoipa::openapi::PathItemType; let method = |t: &PathItemType| match t { PathItemType::Get => "GET", @@ -701,8 +701,8 @@ mod test { #[test] fn every_v0_operation_documents_its_min_role() { use crate::api::main::ApiDoc; - use utoipa::openapi::PathItemType; use utoipa::OpenApi; + use utoipa::openapi::PathItemType; let method = |t: &PathItemType| match t { PathItemType::Get => "GET", diff --git a/crates/pipeline-manager/src/api/support_data_collector.rs b/crates/pipeline-manager/src/api/support_data_collector.rs index f2df1986cfc..1c179c8f1f1 100644 --- a/crates/pipeline-manager/src/api/support_data_collector.rs +++ b/crates/pipeline-manager/src/api/support_data_collector.rs @@ -11,9 +11,9 @@ use crate::db::types::combined_status::CombinedStatus; use crate::db::types::pipeline::PipelineId; use crate::db::types::tenant::TenantId; use crate::error::ManagerError; +use actix_web::HttpResponse; use actix_web::http::Method; use actix_web::rt::time::timeout; -use actix_web::HttpResponse; use awc::Client; use chrono::{DateTime, Utc}; use feldera_types::error::ErrorResponse; @@ -24,7 +24,7 @@ use std::collections::BTreeMap; use std::io::Write; use std::sync::Arc; use tokio::sync::watch; -use tokio::time::{sleep, Duration, Instant}; +use tokio::time::{Duration, Instant, sleep}; use tracing::{debug, error, info}; use utoipa::{IntoParams, ToSchema}; @@ -1367,7 +1367,7 @@ mod tests { use serde_json::json; use std::sync::Arc; use tokio::sync::Mutex; - use tokio::time::{sleep, Duration}; + use tokio::time::{Duration, sleep}; use uuid::Uuid; #[test] diff --git a/crates/pipeline-manager/src/auth.rs b/crates/pipeline-manager/src/auth.rs index 5d6eca2de5f..dd6688f6774 100644 --- a/crates/pipeline-manager/src/auth.rs +++ b/crates/pipeline-manager/src/auth.rs @@ -48,24 +48,24 @@ use std::{collections::HashMap, env, sync::Arc}; +use actix_web::HttpMessage; use actix_web::body::MessageBody; use actix_web::http::header::{self, HeaderMap, HeaderName, HeaderValue}; use actix_web::middleware::Next; -use actix_web::HttpMessage; use actix_web::{ dev::{ServiceRequest, ServiceResponse}, error::ErrorUnauthorized, web::Data, }; use actix_web_httpauth::extractors::{ - bearer::{BearerAuth, Config}, AuthenticationError, + bearer::{BearerAuth, Config}, }; -use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _}; +use base64::{Engine as _, engine::general_purpose::URL_SAFE_NO_PAD}; use cached::{Cached, TimedCache}; -use jsonwebtoken::{decode, decode_header, Algorithm, DecodingKey, TokenData, Validation}; +use jsonwebtoken::{Algorithm, DecodingKey, TokenData, Validation, decode, decode_header}; use rand::rngs::ThreadRng; -use rand::{distributions::Alphanumeric, Rng}; +use rand::{Rng, distributions::Alphanumeric}; use serde::{Deserialize, Serialize}; use serde_json::Value; use static_assertions::assert_impl_any; @@ -82,7 +82,7 @@ use crate::db::storage_postgres::StoragePostgres; use crate::db::types::role::Role; use crate::db::types::tenant::TenantId; use crate::oidc::fetch::{ - fetch_issuer_jwks, fetch_jwks_uri_from_discovery, oidc_http_client, OidcDestination, + OidcDestination, fetch_issuer_jwks, fetch_jwks_uri_from_discovery, oidc_http_client, }; use reqwest::Certificate; @@ -191,20 +191,17 @@ pub(crate) async fn promote_websocket_subprotocol_auth( mut req: ServiceRequest, next: Next, ) -> Result, actix_web::Error> { - if !req.headers().contains_key(header::AUTHORIZATION) { - if let Some(token) = decode_ws_subprotocol(req.headers(), WS_BEARER_PROTOCOL_PREFIX) { - if let Ok(authorization) = HeaderValue::from_str(&format!("Bearer {token}")) { - req.headers_mut() - .insert(header::AUTHORIZATION, authorization); - if let Some(tenant) = - decode_ws_subprotocol(req.headers(), WS_TENANT_PROTOCOL_PREFIX) - { - if let Ok(tenant) = HeaderValue::from_str(&tenant) { - req.headers_mut() - .insert(HeaderName::from_static(TENANT_HEADER), tenant); - } - } - } + if !req.headers().contains_key(header::AUTHORIZATION) + && let Some(token) = decode_ws_subprotocol(req.headers(), WS_BEARER_PROTOCOL_PREFIX) + && let Ok(authorization) = HeaderValue::from_str(&format!("Bearer {token}")) + { + req.headers_mut() + .insert(header::AUTHORIZATION, authorization); + if let Some(tenant) = decode_ws_subprotocol(req.headers(), WS_TENANT_PROTOCOL_PREFIX) + && let Ok(tenant) = HeaderValue::from_str(&tenant) + { + req.headers_mut() + .insert(HeaderName::from_static(TENANT_HEADER), tenant); } } next.call(req).await @@ -571,7 +568,7 @@ async fn bearer_auth( "Database error while fetching tenant: {e}" )), req, - )) + )); } } } @@ -772,27 +769,27 @@ impl OidcClaimExt for TokenData { let sub = &self.claims.sub; // Check if we have explicit tenant authorization in the claim - if let Some(authorized) = self.authorized_tenants() { - if !authorized.is_empty() { - let selected = headers - .get(TENANT_HEADER) - .and_then(|h| h.to_str().ok()) - .filter(|s| !s.is_empty()); - // A selector is always checked against the claim, never ignored. - // Honouring it only when the claim lists several tenants would - // silently place a caller in its one authorized tenant while it - // believes it named another. - return match selected { - Some(selected) if authorized.iter().any(|t| t == selected) => { - Ok(selected.to_string()) - } - Some(selected) => Err(AuthError::UnauthorizedTenant(selected.to_string())), - None if authorized.len() == 1 => Ok(authorized[0].clone()), - None => Err(AuthError::MissingTenantHeader), - }; - } - // Empty array falls through to fallback logic + if let Some(authorized) = self.authorized_tenants() + && !authorized.is_empty() + { + let selected = headers + .get(TENANT_HEADER) + .and_then(|h| h.to_str().ok()) + .filter(|s| !s.is_empty()); + // A selector is always checked against the claim, never ignored. + // Honouring it only when the claim lists several tenants would + // silently place a caller in its one authorized tenant while it + // believes it named another. + return match selected { + Some(selected) if authorized.iter().any(|t| t == selected) => { + Ok(selected.to_string()) + } + Some(selected) => Err(AuthError::UnauthorizedTenant(selected.to_string())), + None if authorized.len() == 1 => Ok(authorized[0].clone()), + None => Err(AuthError::MissingTenantHeader), + }; } + // Empty array falls through to fallback logic // Fallback when the token claims no tenant at all: derive one. // Priority: issuer-domain > sub (if enabled) @@ -1212,17 +1209,17 @@ async fn decode_token_with_validation( // Provider-specific validations based on optional fields // Validate client_id if present (AWS Cognito puts it in a separate field) - if let Some(ref client_id) = token_data.claims.client_id { - if configuration.client_id != *client_id { - return Err(jsonwebtoken::errors::ErrorKind::InvalidAudience.into()); - } + if let Some(ref client_id) = token_data.claims.client_id + && configuration.client_id != *client_id + { + return Err(jsonwebtoken::errors::ErrorKind::InvalidAudience.into()); } // Validate token_use if present (AWS Cognito requires "access" for access tokens) - if let Some(ref token_use) = token_data.claims.token_use { - if token_use != "access" { - return Err(jsonwebtoken::errors::ErrorKind::InvalidToken.into()); - } + if let Some(ref token_use) = token_data.claims.token_use + && token_use != "access" + { + return Err(jsonwebtoken::errors::ErrorKind::InvalidToken.into()); } Ok(token_data) @@ -1442,10 +1439,10 @@ pub(crate) fn parse_rsa_jwks(value: &Value) -> Result(key: &str, check: &str, json: &'a Value) -> Option<&'a Value> { - if let Some(value) = validate_field_is_str(key, json) { - if value == check { - return Some(json); - } + if let Some(value) = validate_field_is_str(key, json) + && value == check + { + return Some(json); } debug!( "Skipping JWK key because it did not match the required shape {} {}", @@ -1456,10 +1453,10 @@ fn check_key_as_str<'a>(key: &str, check: &str, json: &'a Value) -> Option<&'a V fn validate_field_is_str<'a>(key: &str, json: &'a Value) -> Option<&'a str> { let value = json.get(key); - if let Some(value) = value { - if let Some(value) = value.as_str() { - return Some(value); - } + if let Some(value) = value + && let Some(value) = value.as_str() + { + return Some(value); } None } @@ -1487,16 +1484,17 @@ mod test { use actix_http::{HttpMessage, StatusCode}; use actix_web::{ + App, HttpRequest, HttpResponse, body::{BoxBody, EitherBody}, dev::ServiceResponse, http::{self}, - test, web, App, HttpRequest, HttpResponse, + test, web, }; use actix_web_httpauth::middleware::HttpAuthentication; use base64::Engine; use cached::Cached; use chrono::Utc; - use jsonwebtoken::{encode, Algorithm, DecodingKey, EncodingKey, Header, Validation}; + use jsonwebtoken::{Algorithm, DecodingKey, EncodingKey, Header, Validation, encode}; use tokio::sync::{Mutex, RwLock}; use uuid::Uuid; diff --git a/crates/pipeline-manager/src/bin/pipeline-manager.rs b/crates/pipeline-manager/src/bin/pipeline-manager.rs index 1f139b56907..b53b48c78db 100644 --- a/crates/pipeline-manager/src/bin/pipeline-manager.rs +++ b/crates/pipeline-manager/src/bin/pipeline-manager.rs @@ -5,7 +5,7 @@ use clap::{Args, Command, FromArgMatches}; use colored::Colorize; use feldera_observability as observability; use pipeline_manager::api::main::ApiDoc; -use pipeline_manager::cluster_monitor::{cluster_monitor, LocalResourcesPoller}; +use pipeline_manager::cluster_monitor::{LocalResourcesPoller, cluster_monitor}; use pipeline_manager::compiler::main::{compiler_main, compiler_precompile}; #[cfg(feature = "postgresql_embedded")] use pipeline_manager::config::PgEmbedConfig; @@ -25,7 +25,11 @@ use utoipa::OpenApi; fn main() -> anyhow::Result<()> { ensure_default_crypto_provider(); init_fd_limit(); - let _guard = observability::init("https://18aa37ae23e7130b57b91aaad432bc18@o4510219052253184.ingest.us.sentry.io/4510298809827328", "pipeline-manager", env!("CARGO_PKG_VERSION")); + let _guard = observability::init( + "https://18aa37ae23e7130b57b91aaad432bc18@o4510219052253184.ingest.us.sentry.io/4510298809827328", + "pipeline-manager", + env!("CARGO_PKG_VERSION"), + ); pipeline_manager::logging::init_service_logging( "[manager]".cyan(), feldera_observability::json_logging::ServiceName::Manager, diff --git a/crates/pipeline-manager/src/cluster_monitor.rs b/crates/pipeline-manager/src/cluster_monitor.rs index 0fbc3f57e43..b91b28a88ed 100644 --- a/crates/pipeline-manager/src/cluster_monitor.rs +++ b/crates/pipeline-manager/src/cluster_monitor.rs @@ -72,8 +72,10 @@ pub async fn cluster_monitor( "{protocol}://{}:{}/healthz", common_config.api_host, common_config.api_port ); + // `check_storage` additionally fails on storage pressure of the compiler + // working directory, which would make binary uploads fail with ENOSPC. let compiler_url = format!( - "{protocol}://{}:{}/healthz", + "{protocol}://{}:{}/healthz?check_storage=true", common_config.compiler_host, common_config.compiler_port ); let runner_url = format!( @@ -99,7 +101,9 @@ pub async fn cluster_monitor( if matches!(e, DBError::NoClusterMonitorEventsAvailable) { None } else { - error!("Cluster monitor cannot perform monitoring because it is unable to retrieve the latest event due to: {e}"); + error!( + "Cluster monitor cannot perform monitoring because it is unable to retrieve the latest event due to: {e}" + ); tokio::time::sleep(MONITOR_INTERVAL).await; continue; } @@ -232,8 +236,8 @@ pub async fn cluster_monitor( }; // Clean up events that no longer need to be retained - if stored { - if let Err(e) = db + if stored + && let Err(e) = db .lock() .await .delete_cluster_monitor_events_beyond_retention( @@ -241,9 +245,8 @@ pub async fn cluster_monitor( MONITOR_RETENTION_NUM, ) .await - { - error!("Cluster monitor is unable to clean up based on retention due to: {e}"); - } + { + error!("Cluster monitor is unable to clean up based on retention due to: {e}"); } } else { iterations_without_insert += 1; @@ -279,7 +282,9 @@ async fn poll_service_health_endpoint( { Ok(resp) if resp.status().is_success() => ( true, - format!("Healthy: The {service_name} service responded successfully to the last health check."), + format!( + "Healthy: The {service_name} service responded successfully to the last health check." + ), ), Ok(resp) => { let status = resp.status(); @@ -321,7 +326,6 @@ async fn poll_service_health_endpoint( source: {}. Please check the {service_name} logs for more information.", source_error(&e) ), - ), } } diff --git a/crates/pipeline-manager/src/common_error.rs b/crates/pipeline-manager/src/common_error.rs index 36c99db1a6e..26efb841836 100644 --- a/crates/pipeline-manager/src/common_error.rs +++ b/crates/pipeline-manager/src/common_error.rs @@ -1,9 +1,9 @@ use actix_web::{ - body::BoxBody, http::StatusCode, HttpResponse, HttpResponseBuilder, ResponseError, + HttpResponse, HttpResponseBuilder, ResponseError, body::BoxBody, http::StatusCode, }; use feldera_types::error::{DetailedError, ErrorResponse}; use serde::Serialize; -use serde::{ser::SerializeStruct, Serializer}; +use serde::{Serializer, ser::SerializeStruct}; use std::backtrace::Backtrace; use std::io::Error as IOError; use std::{borrow::Cow, error::Error as StdError, fmt, fmt::Display}; diff --git a/crates/pipeline-manager/src/compiler/error.rs b/crates/pipeline-manager/src/compiler/error.rs index 5ec4fbff2c0..da88aeedc4f 100644 --- a/crates/pipeline-manager/src/compiler/error.rs +++ b/crates/pipeline-manager/src/compiler/error.rs @@ -1,5 +1,5 @@ use actix_web::{ - body::BoxBody, http::StatusCode, HttpResponse, HttpResponseBuilder, ResponseError, + HttpResponse, HttpResponseBuilder, ResponseError, body::BoxBody, http::StatusCode, }; use feldera_types::error::{DetailedError, ErrorResponse}; use serde::Serialize; diff --git a/crates/pipeline-manager/src/compiler/main.rs b/crates/pipeline-manager/src/compiler/main.rs index d3a666c59f3..2eadfa3af18 100644 --- a/crates/pipeline-manager/src/compiler/main.rs +++ b/crates/pipeline-manager/src/compiler/main.rs @@ -2,13 +2,16 @@ use crate::api::error::ApiError; use crate::common_error::CommonError; use crate::compiler::error::CompilerError; use crate::compiler::rust_compiler::{ - perform_rust_compilation, rust_compiler_task, RustCompilationError, RustCompilationResult, + CLEANUP_INTERVAL, RustCompilationError, RustCompilationResult, cleanup_pipeline_binaries, + perform_rust_compilation, rust_compiler_task, }; use crate::compiler::sql_compiler::{ - ephemeral_compilation_dir, perform_sql_compilation, sql_compiler_task, validate_program, - ProgramValidationRequest, SqlCompilationError, SqlCompilationOutput, + ProgramValidationRequest, SqlCompilationError, SqlCompilationOutput, decide_stale_jar, + ephemeral_compilation_dir, jar_cache_dir, perform_sql_compilation, sql_compiler_task, + validate_program, }; use crate::compiler::util::{ + CleanupDecision, DiskSpace, cleanup_specific_directories, cleanup_specific_files, pipeline_binary_filename, program_info_filename, validate_is_sha256_checksum, }; use crate::config::{CommonConfig, CompilerConfig}; @@ -20,14 +23,18 @@ use crate::db::types::tenant::TenantId; use crate::db::types::version::Version; use crate::error::ManagerError; use actix_files::NamedFile; -use actix_web::{get, post, web, HttpRequest, HttpResponse, HttpServer, Responder}; -use futures_util::StreamExt; +use actix_web::error::PayloadError; +use actix_web::{HttpRequest, HttpResponse, HttpServer, Responder, get, post, web}; +use futures_util::{Stream, StreamExt}; use std::net::TcpListener; use std::path::Path; use std::str::FromStr; use std::sync::Arc; +use std::time::Duration; +use tokio::task::JoinHandle; +use tokio::time::sleep; use tokio::{fs, io::AsyncWriteExt, spawn, sync::Mutex}; -use tracing::{error, info}; +use tracing::{error, info, warn}; use uuid::Uuid; /// Decodes the URL encoded parameter value as a string. @@ -45,7 +52,9 @@ fn decode_url_encoded_parameter( /// Checks if the required compilation artifacts exist for the specified pipeline and version. /// If `program_info_integrity_checksum` is "none", only the binary existence is checked. -#[get("/artifacts/{pipeline_id}/{program_version}/{source_checksum}/{binary_integrity_checksum}/{program_info_integrity_checksum}")] +#[get( + "/artifacts/{pipeline_id}/{program_version}/{source_checksum}/{binary_integrity_checksum}/{program_info_integrity_checksum}" +)] async fn check_compilation_artifacts( config: web::Data, req: HttpRequest, @@ -357,7 +366,14 @@ async fn upload_binary( &expected_integrity_checksum, )); - let total_size = save_file(&target_file_path, payload, &expected_integrity_checksum).await?; + let total_size = match save_file(&target_file_path, payload, &expected_integrity_checksum).await + { + Ok(total_size) => total_size, + Err(error) => match unwritable_store_cause(&error) { + Some(cause) => return Ok(insufficient_storage_response(cause, &error)), + None => return Err(error), + }, + }; info!( pipeline_id = %pipeline_id, @@ -452,7 +468,14 @@ async fn upload_program_info( &expected_integrity_checksum, )); - let total_size = save_file(&target_file_path, payload, &expected_integrity_checksum).await?; + let total_size = match save_file(&target_file_path, payload, &expected_integrity_checksum).await + { + Ok(total_size) => total_size, + Err(error) => match unwritable_store_cause(&error) { + Some(cause) => return Ok(insufficient_storage_response(cause, &error)), + None => return Err(error), + }, + }; info!( pipeline_id = %pipeline_id, @@ -496,15 +519,77 @@ async fn validate_program_endpoint( Ok(HttpResponse::Ok().json(response)) } +/// Streams the payload to a temp file next to `target_file_path`, verifies the +/// sha256 checksum, and only then renames the temp file onto the final path. +/// The final path therefore never holds a partially written or corrupt file; +/// on any error the temp file is removed. async fn save_file( target_file_path: &Path, - mut payload: web::Payload, + payload: impl Stream> + Unpin, + expected_integrity_checksum: &str, +) -> Result { + let temp_file_path = target_file_path.with_added_extension(format!("tmp-{}", Uuid::now_v7())); + + let write_result = + stream_to_file_and_verify(&temp_file_path, payload, expected_integrity_checksum).await; + + match write_result { + Ok(total_size) => match fs::rename(&temp_file_path, target_file_path).await { + Ok(()) => { + // Persist the rename: without a directory fsync a crash can + // lose the entry while the database already says Success. + fsync_parent_dir(target_file_path).await?; + Ok(total_size) + } + Err(e) => { + remove_temp_upload_file(&temp_file_path).await; + Err(ManagerError::from(CommonError::io_error( + format!( + "renaming '{}' to '{}'", + temp_file_path.display(), + target_file_path.display() + ), + e, + ))) + } + }, + Err(e) => { + remove_temp_upload_file(&temp_file_path).await; + Err(e) + } + } +} + +/// Fsyncs the directory containing `path` so that a rename into it is durable. +async fn fsync_parent_dir(path: &Path) -> Result<(), ManagerError> { + let Some(parent_dir) = path.parent() else { + return Ok(()); + }; + let parent_dir_handle = fs::File::open(parent_dir).await.map_err(|e| { + ManagerError::from(CommonError::io_error( + format!("opening directory '{}'", parent_dir.display()), + e, + )) + })?; + parent_dir_handle.sync_all().await.map_err(|e| { + ManagerError::from(CommonError::io_error( + format!("syncing directory '{}'", parent_dir.display()), + e, + )) + }) +} + +/// Streams the payload to `file_path` and validates the sha256 checksum after +/// flushing. Returns the total size in bytes. The caller removes the file on +/// any error. +async fn stream_to_file_and_verify( + file_path: &Path, + mut payload: impl Stream> + Unpin, expected_integrity_checksum: &str, ) -> Result { - // Stream the binary directly to disk with integrity checksum validation - let mut file = fs::File::create(&target_file_path).await.map_err(|e| { + let mut file = fs::File::create(&file_path).await.map_err(|e| { ManagerError::from(CommonError::io_error( - format!("creating file '{}'", target_file_path.display()), + format!("creating file '{}'", file_path.display()), e, )) })?; @@ -527,16 +612,23 @@ async fn save_file( // Write chunk to file file.write_all(&chunk).await.map_err(|e| { ManagerError::from(CommonError::io_error( - format!("writing to file '{}'", target_file_path.display()), + format!("writing to file '{}'", file_path.display()), e, )) })?; } - // Flush and close file + // Flush and persist to disk before the rename makes the file visible + // under its final name. file.flush().await.map_err(|e| { ManagerError::from(CommonError::io_error( - format!("flushing file '{}'", target_file_path.display()), + format!("flushing file '{}'", file_path.display()), + e, + )) + })?; + file.sync_all().await.map_err(|e| { + ManagerError::from(CommonError::io_error( + format!("syncing file '{}'", file_path.display()), e, )) })?; @@ -545,8 +637,6 @@ async fn save_file( // Validate integrity checksum let actual_integrity_checksum = hex::encode(hasher.finish()); if actual_integrity_checksum != expected_integrity_checksum { - // Remove the invalid file - let _ = fs::remove_file(&target_file_path).await; return Err(ManagerError::from(ApiError::InvalidChecksumParam { value: format!( "Expected integrity checksum '{}', but calculated '{}'", @@ -559,9 +649,115 @@ async fn save_file( Ok(total_size) } +/// Operator documentation on resolving a full or read-only storage volume. +const OUT_OF_STORAGE_DOCS_URL: &str = + "https://docs.feldera.com/operations/guide/#out-of-storage-errors"; + +/// Why the binary store cannot accept writes, phrased as the cause plus the +/// action that resolves it, or `None` when the error is something else. Only +/// ENOSPC and EROFS qualify: both need an operator, so an upload that hits +/// either must fail rather than retry. +fn unwritable_store_cause(error: &ManagerError) -> Option<&'static str> { + let ManagerError::CommonError { + common_error: CommonError::IoError { io_error, .. }, + } = error + else { + return None; + }; + match io_error.kind() { + std::io::ErrorKind::StorageFull => Some( + "the storage volume is full; an operator must grow it or delete unused pipelines \ + to reclaim space", + ), + // EROFS in practice means the underlying disk failed and the kernel + // remounted the filesystem read-only. + std::io::ErrorKind::ReadOnlyFilesystem => Some( + "the storage volume is read-only, which usually means the underlying disk failed; \ + an operator must repair or replace it", + ), + _ => None, + } +} + +/// 507 Insufficient Storage response naming the cause and how to resolve it. +/// The distinct status lets workers fail the compilation fast with this +/// message instead of burning their retry budget on a volume that only an +/// operator can grow or repair. +fn insufficient_storage_response(cause: &str, error: &ManagerError) -> HttpResponse { + HttpResponse::InsufficientStorage().json(serde_json::json!({ + "message": format!( + "Unable to write to the binary store: {cause}. \ + See {OUT_OF_STORAGE_DOCS_URL}. Underlying error: {error}" + ), + })) +} + +/// Removes a temp upload file. Failure only leaves an orphan file behind, so +/// it is logged rather than propagated. +async fn remove_temp_upload_file(temp_file_path: &Path) { + if let Err(e) = fs::remove_file(temp_file_path).await + && e.kind() != std::io::ErrorKind::NotFound + { + warn!( + "Unable to remove temp upload file '{}': {e}", + temp_file_path.display() + ); + } +} + +/// Fraction of the working-directory filesystem above which `/healthz` reports +/// storage pressure: binary uploads are about to fail with ENOSPC and an +/// operator must grow the volume or reclaim space. +const STORAGE_PRESSURE_THRESHOLD: f64 = 0.95; + +/// Message reported when the working-directory filesystem is at or above +/// `used_fraction_threshold`, `None` when it is below. +fn storage_pressure_message( + disk_space: &DiskSpace, + used_fraction_threshold: f64, +) -> Option { + if disk_space.used_fraction < used_fraction_threshold { + return None; + } + Some(format!( + "unhealthy: compiler working directory filesystem is {:.1}% full ({} of {} bytes used); \ + binary uploads will fail until an operator grows the volume or storage is reclaimed", + disk_space.used_fraction * 100.0, + disk_space.used_byte, + disk_space.total_byte + )) +} + +/// Query parameters of the `/healthz` endpoint. +#[derive(serde::Deserialize)] +struct HealthzQuery { + /// Also report storage pressure of the working-directory filesystem. + #[serde(default)] + check_storage: bool, +} + /// Health check which returns success if it is able to reach the database. +/// With `?check_storage=true` it also fails when the working-directory +/// filesystem is nearly full. +/// +/// Kubernetes probes must omit the parameter, because restarting the pod +/// cannot free disk space and the pod still serves already compiled binaries. +/// The cluster monitor passes it so that /v0/cluster_healthz surfaces the +/// condition to operators, who can act on it. #[get("/healthz")] -async fn healthz(probe: web::Data>>) -> Result { +async fn healthz( + probe: web::Data>>, + config: web::Data, + query: web::Query, +) -> Result { + if query.check_storage + && let Some(disk_space) = DiskSpace::new_from_path(&config.working_dir()) + && let Some(message) = storage_pressure_message(&disk_space, STORAGE_PRESSURE_THRESHOLD) + { + return Ok( + HttpResponse::ServiceUnavailable().json(serde_json::json!({ "status": message })) + ); + } Ok(probe.lock().await.as_http_response()) } @@ -742,6 +938,54 @@ pub async fn compiler_main( )); // Spawn HTTP server thread + let http_server = spawn_compiler_http_server(&common_config, &config, &db).await; + + // All threads should run indefinitely + let error = tokio::select! { + _ = sql_task => "Compiler SQL task ended prematurely", + _ = rust_task => "Compiler Rust task ended prematurely", + _ = http_server => "Compiler HTTP(S) server task ended prematurely", + }; + error!("{error}"); + error!("Returning compiler thread"); + Err(ManagerError::from(CompilerError::TaskFailed { + error: error.to_string(), + })) +} + +/// Runs the artifact-store variant of the compiler server: the full HTTP +/// surface plus a janitor, but no SQL or Rust compilation tasks. Serves +/// deployments where compiler workers are ephemeral and this process is the +/// durable binary store (see enterprise compiler autoscaling). +pub async fn artifact_server_main( + common_config: CommonConfig, + config: CompilerConfig, + db: Arc>, +) -> Result<(), ManagerError> { + create_working_directory_if_not_exists(&config).await?; + + // Spawn janitor and HTTP server threads + let janitor_task = spawn(artifact_server_janitor_task(config.clone(), db.clone())); + let http_server = spawn_compiler_http_server(&common_config, &config, &db).await; + + // Both threads should run indefinitely + let error = tokio::select! { + _ = janitor_task => "Artifact server janitor task ended prematurely", + _ = http_server => "Artifact server HTTP(S) server task ended prematurely", + }; + error!("{error}"); + Err(ManagerError::from(CompilerError::TaskFailed { + error: error.to_string(), + })) +} + +/// Spawns the compiler HTTP(S) server serving artifacts, uploads, program +/// validation, and health checks. Panics if the listener cannot be bound. +async fn spawn_compiler_http_server( + common_config: &CommonConfig, + config: &CompilerConfig, + db: &Arc>, +) -> JoinHandle> { let config = web::Data::new(config.clone()); let common_config_data = web::Data::new(common_config.clone()); let probe = web::Data::new(DbProbe::new(db.clone()).await); @@ -793,34 +1037,92 @@ pub async fn compiler_main( common_config.compiler_port, common_config.http_workers, ); + http_server +} - // All threads should run indefinitely - let error = tokio::select! { - _ = sql_task => "Compiler SQL task ended prematurely", - _ = rust_task => "Compiler Rust task ended prematurely", - _ = http_server => "Compiler HTTP(S) server task ended prematurely", +/// Age above which an ephemeral validation directory is an orphan of a crashed +/// validation; live validations finish within seconds. +const ORPHANED_EPHEMERAL_DIR_MAX_AGE: Duration = Duration::from_secs(3600); + +/// Removes an ephemeral validation directory whose modification time exceeds +/// [`ORPHANED_EPHEMERAL_DIR_MAX_AGE`]. Missing or future modification times +/// never remove. +fn decide_orphaned_ephemeral_dir( + _dir_name: &str, + metadata: Option, +) -> CleanupDecision { + let Some(modified_time) = metadata.and_then(|metadata| metadata.modified().ok()) else { + return CleanupDecision::Ignore; }; - error!("{error}"); - error!("Returning compiler thread"); - Err(ManagerError::from(CompilerError::TaskFailed { - error: error.to_string(), - })) + if modified_time + .elapsed() + .is_ok_and(|age| age >= ORPHANED_EPHEMERAL_DIR_MAX_AGE) + { + CleanupDecision::Remove + } else { + CleanupDecision::Keep { + motivation: "Validation may still be in progress".to_string(), + } + } +} + +/// Janitor of the artifact server: garbage-collects pipeline binaries of +/// deleted or recompiled pipelines, ephemeral validation directories orphaned +/// by crashed validations, and stale SQL compiler jars. Errors within a pass +/// are logged and the loop continues. +async fn artifact_server_janitor_task(config: CompilerConfig, db: Arc>) { + loop { + if let Err(e) = cleanup_pipeline_binaries(&config, db.clone()).await { + error!("Artifact server janitor: pipeline binaries cleanup failed: {e}"); + } + let ephemeral_dir = ephemeral_compilation_dir(&config); + if ephemeral_dir.is_dir() + && let Err(e) = cleanup_specific_directories( + "Ephemeral validation directories", + &ephemeral_dir, + Arc::new(decide_orphaned_ephemeral_dir), + false, + true, + ) + .await + { + error!("Artifact server janitor: ephemeral validation directory cleanup failed: {e}"); + } + let jar_cache_dir = jar_cache_dir(&config); + if jar_cache_dir.is_dir() + && let Err(e) = cleanup_specific_files( + "SQL JAR cache", + &jar_cache_dir, + Arc::new(decide_stale_jar), + true, + true, + ) + .await + { + error!("Artifact server janitor: SQL compiler jar cache cleanup failed: {e}"); + } + sleep(CLEANUP_INTERVAL).await; + } } #[cfg(test)] mod test { use crate::api::error::ApiError; use crate::compiler::main::{ - create_working_directory_if_not_exists, decode_url_encoded_parameter, upload_binary, + create_working_directory_if_not_exists, decide_orphaned_ephemeral_dir, + decode_url_encoded_parameter, save_file, upload_binary, }; + use crate::compiler::util::CleanupDecision; use crate::compiler::util::pipeline_binary_filename; use crate::config::CompilerConfig; use crate::db::types::pipeline::PipelineId; use crate::db::types::program::CompilationProfile; use crate::db::types::version::Version; use crate::error::ManagerError; - use actix_web::{test as actix_test, web, App}; + use actix_web::error::PayloadError; + use actix_web::{App, test as actix_test, web}; use openssl::sha::sha256; + use std::time::Duration; use tokio::fs; use uuid::Uuid; @@ -961,6 +1263,14 @@ mod test { "Checksum should match for test case: {}", test_case.name ); + + // No temp file may be left behind + let dir_names = + list_file_names(expected_path.parent().expect("path must have a parent")).await; + assert!( + dir_names.iter().all(|name| !name.contains(".tmp-")), + "No temp file may remain, found: {dir_names:?}" + ); } } @@ -1004,6 +1314,158 @@ mod test { !expected_path.exists(), "Binary file should not exist after checksum failure" ); + + // Neither the final file nor a temp file may remain + let dir_names = + list_file_names(expected_path.parent().expect("path must have a parent")).await; + assert!( + dir_names.is_empty(), + "No file may remain after checksum failure, found: {dir_names:?}" + ); + } + + /// An upload whose payload errors mid-stream leaves no file under the + /// final name and no temp file behind. + #[tokio::test] + async fn test_interrupted_upload_leaves_no_file() { + let tempdir = tempfile::tempdir().unwrap(); + let pipeline_binaries_dir = tempdir.path().join("pipeline-binaries"); + fs::create_dir_all(&pipeline_binaries_dir).await.unwrap(); + let target_file_path = pipeline_binaries_dir.join("pipeline_example_binary"); + + let data = b"partial data"; + let interrupted_payload = futures_util::stream::iter(vec![ + Ok(web::Bytes::from_static(data)), + Err(PayloadError::Incomplete(None)), + ]); + let result = save_file( + &target_file_path, + interrupted_payload, + &hex::encode(sha256(data)), + ) + .await; + assert!(result.is_err(), "Interrupted upload should fail"); + + let dir_names = list_file_names(&pipeline_binaries_dir).await; + assert!( + dir_names.is_empty(), + "No file may remain after an interrupted upload, found: {dir_names:?}" + ); + } + + /// A full or read-only store yields a cause naming the fix; other I/O + /// errors yield none. + #[test] + fn unwritable_store_cause_detection() { + let io_error = |kind| { + ManagerError::from(crate::common_error::CommonError::io_error( + "writing".to_string(), + std::io::Error::new(kind, "test"), + )) + }; + let full = super::unwritable_store_cause(&io_error(std::io::ErrorKind::StorageFull)) + .expect("a full volume is unwritable"); + assert!(full.contains("full"), "unexpected cause: {full}"); + let read_only = + super::unwritable_store_cause(&io_error(std::io::ErrorKind::ReadOnlyFilesystem)) + .expect("a read-only volume is unwritable"); + assert!( + read_only.contains("read-only"), + "unexpected cause: {read_only}" + ); + assert!( + super::unwritable_store_cause(&io_error(std::io::ErrorKind::PermissionDenied)) + .is_none() + ); + } + + /// The 507 response names the cause and links the operator documentation. + #[actix_web::test] + async fn insufficient_storage_response_is_actionable() { + let error = ManagerError::from(crate::common_error::CommonError::io_error( + "writing".to_string(), + std::io::Error::new(std::io::ErrorKind::StorageFull, "test"), + )); + let cause = super::unwritable_store_cause(&error).unwrap(); + let response = super::insufficient_storage_response(cause, &error); + assert_eq!( + response.status(), + actix_web::http::StatusCode::INSUFFICIENT_STORAGE + ); + let body = actix_web::body::to_bytes(response.into_body()) + .await + .unwrap(); + let message = serde_json::from_slice::(&body).unwrap()["message"] + .as_str() + .unwrap() + .to_string(); + assert!( + message.starts_with("Unable to write to the binary store:"), + "unexpected message: {message}" + ); + assert!(message.contains(super::OUT_OF_STORAGE_DOCS_URL)); + } + + /// The kernel errnos the upload path actually sees map to the + /// `ErrorKind`s `unwritable_store_cause` matches on. + #[cfg(unix)] + #[test] + fn unwritable_store_errnos_map_to_expected_error_kinds() { + assert_eq!( + std::io::Error::from_raw_os_error(libc::ENOSPC).kind(), + std::io::ErrorKind::StorageFull + ); + assert_eq!( + std::io::Error::from_raw_os_error(libc::EROFS).kind(), + std::io::ErrorKind::ReadOnlyFilesystem + ); + } + + /// Storage pressure is reported at or above the threshold and stays silent + /// below it. + #[test] + fn storage_pressure_threshold() { + let disk_space = |used_fraction: f64| crate::compiler::util::DiskSpace { + total_byte: 100, + used_byte: (used_fraction * 100.0) as u64, + used_fraction, + available_byte: 100 - (used_fraction * 100.0) as u64, + available_fraction: 1.0 - used_fraction, + }; + assert!(super::storage_pressure_message(&disk_space(0.5), 0.95).is_none()); + assert!(super::storage_pressure_message(&disk_space(0.95), 0.95).is_some()); + let message = super::storage_pressure_message(&disk_space(1.0), 0.95).unwrap(); + assert!(message.contains("100.0% full")); + } + + /// An old ephemeral validation directory is removed, a fresh one is kept, + /// and missing metadata never removes. + #[test] + fn orphaned_ephemeral_dir_decision() { + let tempdir = tempfile::tempdir().unwrap(); + let dir_path = tempdir.path(); + let recent = std::fs::metadata(dir_path).unwrap(); + assert!(matches!( + decide_orphaned_ephemeral_dir("d", Some(recent)), + CleanupDecision::Keep { .. } + )); + let old_time = std::time::SystemTime::now() - Duration::from_secs(2 * 3600); + let times = std::fs::FileTimes::new() + .set_accessed(old_time) + .set_modified(old_time); + std::fs::File::open(dir_path) + .unwrap() + .set_times(times) + .unwrap(); + let old = std::fs::metadata(dir_path).unwrap(); + assert_eq!( + decide_orphaned_ephemeral_dir("d", Some(old)), + CleanupDecision::Remove + ); + assert_eq!( + decide_orphaned_ephemeral_dir("d", None), + CleanupDecision::Ignore + ); } #[tokio::test] @@ -1101,6 +1563,16 @@ mod test { (0..size).map(|i| (i % 256) as u8).collect() } + /// Lists the file names in a directory + async fn list_file_names(dir: &std::path::Path) -> Vec { + let mut file_names = vec![]; + let mut entries = fs::read_dir(dir).await.unwrap(); + while let Some(entry) = entries.next_entry().await.unwrap() { + file_names.push(entry.file_name().to_string_lossy().to_string()); + } + file_names + } + /// Builds upload URL from parameters fn build_upload_url( pipeline_id: &PipelineId, diff --git a/crates/pipeline-manager/src/compiler/rust_compiler.rs b/crates/pipeline-manager/src/compiler/rust_compiler.rs index ea6de2232d6..464a922b852 100644 --- a/crates/pipeline-manager/src/compiler/rust_compiler.rs +++ b/crates/pipeline-manager/src/compiler/rust_compiler.rs @@ -1,10 +1,10 @@ use crate::compiler::util::{ + CleanupDecision, DirectoryContent, DiskSpace, ProcessGroupTerminator, UtilError, checksum_buffer, checksum_file, cleanup_specific_directories, cleanup_specific_files, copy_file, copy_file_if_checksum_differs, crate_name_pipeline_globals, crate_name_pipeline_main, create_dir_if_not_exists, create_new_file, decode_string_as_dir, pipeline_binary_filename, program_info_filename, read_file_content, recreate_dir, - recreate_file_with_content, truncate_sha256_checksum, write_file, CleanupDecision, - DirectoryContent, DiskSpace, ProcessGroupTerminator, UtilError, + recreate_file_with_content, truncate_sha256_checksum, write_file, }; use crate::config::{CommonConfig, CompilerConfig}; use crate::db::error::DBError; @@ -32,7 +32,7 @@ use tokio::{ io::AsyncReadExt, process::Command, sync::Mutex, - time::{sleep, Duration}, + time::{Duration, sleep}, }; use tracing::{debug, error, info, trace, warn}; @@ -56,7 +56,11 @@ const POLL_ERROR_INTERVAL: Duration = Duration::from_secs(30); const COMPILATION_CHECK_INTERVAL: Duration = Duration::from_millis(250); /// The frequency at which Rust cleanup is performed. -const CLEANUP_INTERVAL: Duration = Duration::from_secs(120); +pub(crate) const CLEANUP_INTERVAL: Duration = Duration::from_secs(120); + +/// Age above which a `.tmp-` upload file is an orphan of a crashed upload +/// rather than an upload in flight, and is removed by the binaries cleanup. +const STALE_TEMP_UPLOAD_MAX_AGE: Duration = Duration::from_secs(3600); /// Minimum time between when the Rust cleanup has detected a pipeline to be deleted until /// its compilation artifacts are actually cleaned up. It is a minimum, as cleanup both @@ -98,18 +102,26 @@ pub async fn rust_compiler_task( if let Err(e) = cleanup_rust_compilation(&config, db.clone()).await { match e { RustCompilationCleanupError::Database(e) => { - error!("Rust worker {worker_id}: compilation cleanup failed: database error occurred: {e}"); + error!( + "Rust worker {worker_id}: compilation cleanup failed: database error occurred: {e}" + ); } RustCompilationCleanupError::Utility(e) => { - error!("Rust worker {worker_id}: compilation cleanup failed: utility error occurred: {e}"); + error!( + "Rust worker {worker_id}: compilation cleanup failed: utility error occurred: {e}" + ); } RustCompilationCleanupError::TargetCleared => { if allow_exit_upon_target_cleared { // This restart behavior only occurs for a standalone compiler server - warn!("Rust worker {worker_id}: target directory has been cleared due to lack of space -- restarting to have any precompilation re-applied"); + warn!( + "Rust worker {worker_id}: target directory has been cleared due to lack of space -- restarting to have any precompilation re-applied" + ); return Err(()); } else { - warn!("Rust worker {worker_id}: target directory has been cleared due to lack of space -- the next compilation will be slow"); + warn!( + "Rust worker {worker_id}: target directory has been cleared due to lack of space -- the next compilation will be slow" + ); } } } @@ -140,11 +152,15 @@ pub async fn rust_compiler_task( outdated_version, latest_version, } => { - debug!("Rust worker {worker_id}: compilation canceled: pipeline program version ({outdated_version}) is outdated by latest ({latest_version})"); + debug!( + "Rust worker {worker_id}: compilation canceled: pipeline program version ({outdated_version}) is outdated by latest ({latest_version})" + ); } e => { unexpected_error = true; - error!("Rust worker {worker_id}: compilation canceled: unexpected database error occurred: {e}"); + error!( + "Rust worker {worker_id}: compilation canceled: unexpected database error occurred: {e}" + ); } } } @@ -338,6 +354,9 @@ async fn attempt_end_to_end_rust_compilation( ); } RustCompilationError::FileUploadError(upload_error) => { + // The in-cycle retry budget already absorbed transient blips, so a + // failure here (endpoint down for long, disk full) needs an operator; + // a terminal status tells the user why their compile failed. db.lock() .await .transit_program_status_to_system_error( @@ -578,7 +597,7 @@ async fn upload_binary_to_endpoint_with_retries( _ => { return Err(RustCompilationError::SystemError( "Invalid delivery mode for HTTP upload".to_string(), - )) + )); } }; @@ -607,6 +626,10 @@ async fn upload_binary_to_endpoint_with_retries( return Ok(result); } Err(e) => { + // Permanent rejections skip the retry budget. + if matches!(e, RustCompilationError::SystemError(_)) { + return Err(e); + } if attempts > max_retries { error!( pipeline_id = %metadata.pipeline_id, @@ -656,7 +679,7 @@ async fn upload_program_info_to_endpoint_with_retries( _ => { return Err(RustCompilationError::SystemError( "Invalid delivery mode for HTTP upload".to_string(), - )) + )); } }; @@ -685,6 +708,10 @@ async fn upload_program_info_to_endpoint_with_retries( return Ok(result); } Err(e) => { + // Permanent rejections skip the retry budget. + if matches!(e, RustCompilationError::SystemError(_)) { + return Err(e); + } if attempts > max_retries { error!( pipeline_id = %metadata.pipeline_id, @@ -715,6 +742,30 @@ async fn upload_program_info_to_endpoint_with_retries( } } +/// Returns true when the upload response status indicates a permanent +/// rejection that no retry can fix: a 4xx other than 408 and 429, or 507 +/// Insufficient Storage (the binary store cannot accept writes and only an +/// operator can fix that, so retrying just delays the user-visible error). +fn is_permanent_upload_rejection(status: reqwest::StatusCode) -> bool { + if status == reqwest::StatusCode::INSUFFICIENT_STORAGE { + return true; + } + status.is_client_error() + && status != reqwest::StatusCode::REQUEST_TIMEOUT + && status != reqwest::StatusCode::TOO_MANY_REQUESTS +} + +/// Classifies a non-2xx upload response: a permanent rejection becomes a +/// terminal `SystemError` because retrying or recompiling cannot succeed; +/// anything else stays a retryable `FileUploadError`. +fn upload_failure_error(status: reqwest::StatusCode, message: String) -> RustCompilationError { + if is_permanent_upload_rejection(status) { + RustCompilationError::SystemError(message) + } else { + RustCompilationError::FileUploadError(message) + } +} + /// Uploads the compiled binary to an HTTP endpoint (single attempt) using streaming. async fn upload_binary_to_endpoint( common_config: &CommonConfig, @@ -794,10 +845,8 @@ async fn upload_binary_to_endpoint( .text() .await .unwrap_or_else(|_| "Unknown error".to_string()); - return Err(RustCompilationError::FileUploadError(format!( - "Binary upload failed with status {}: {}", - status, error_text - ))); + let message = format!("Binary upload failed with status {status}: {error_text}"); + return Err(upload_failure_error(status, message)); } // Get response body for any location information @@ -862,10 +911,8 @@ async fn upload_program_info_to_endpoint( .text() .await .unwrap_or_else(|_| "Unknown error".to_string()); - return Err(RustCompilationError::FileUploadError(format!( - "Program info upload failed with status {}: {}", - status, error_text - ))); + let message = format!("Program info upload failed with status {status}: {error_text}"); + return Err(upload_failure_error(status, message)); } // Get response body for any location information @@ -1120,15 +1167,19 @@ async fn checkout_runtime_version( match clone(repo_location).await { Ok(output) => { if !output.status.success() { - return Err(RustCompilationError::SystemError(format!("Unable to clone latest runtime version for '{requested_runtime_version}' for compilation.\n`git clone` failed with exit code: {}\nstderr:\n{}\nstdout:\n{}", + return Err(RustCompilationError::SystemError(format!( + "Unable to clone latest runtime version for '{requested_runtime_version}' for compilation.\n`git clone` failed with exit code: {}\nstderr:\n{}\nstdout:\n{}", output.status.code().unwrap_or(-1), String::from_utf8_lossy(&output.stderr), - String::from_utf8_lossy(&output.stdout)))); + String::from_utf8_lossy(&output.stdout) + ))); } } - Err(e) => return Err(RustCompilationError::SystemError(format!( - "Unable to clone repo for runtime version override to '{requested_runtime_version}' for compilation, `git clone` failed: {e}", - ))), + Err(e) => { + return Err(RustCompilationError::SystemError(format!( + "Unable to clone repo for runtime version override to '{requested_runtime_version}' for compilation, `git clone` failed: {e}", + ))); + } } } @@ -1184,14 +1235,18 @@ async fn checkout_runtime_version( if output.status.success() { Ok(()) } else { - debug!("Failed to checkout requested runtime, trying to sync with latest git repository."); + debug!( + "Failed to checkout requested runtime, trying to sync with latest git repository." + ); let fetch_result = fetch(repo_location, requested_runtime_version).await.map_err(|e| { RustCompilationError::SystemError(format!("Unable to switch runtime version to '{requested_runtime_version}' for compilation, `git fetch` failed: {e}"))})?; if !fetch_result.status.success() { - return Err(RustCompilationError::SystemError(format!("Unable to fetch latest runtime version for '{requested_runtime_version}' for compilation.\nGit command failed with exit code: {}\nstderr:\n{}\nstdout:\n{}", + return Err(RustCompilationError::SystemError(format!( + "Unable to fetch latest runtime version for '{requested_runtime_version}' for compilation.\nGit command failed with exit code: {}\nstderr:\n{}\nstdout:\n{}", fetch_result.status.code().unwrap_or(-1), String::from_utf8_lossy(&fetch_result.stderr), - String::from_utf8_lossy(&fetch_result.stdout)))); + String::from_utf8_lossy(&fetch_result.stdout) + ))); } let output = checkout(repo_location, requested_runtime_version) @@ -1228,12 +1283,7 @@ pub async fn resolve_runtime_sha( let repo_location = runtime_version.runtime_sources(config); match Command::new("git") .current_dir(&repo_location) - .args([ - "-c", - "protocol.version=2", - "rev-parse", - version, - ]) + .args(["-c", "protocol.version=2", "rev-parse", version]) .output() .await { @@ -1753,7 +1803,7 @@ async fn call_compiler( } /// Rust compilation cleanup possible error outcomes. -enum RustCompilationCleanupError { +pub(crate) enum RustCompilationCleanupError { /// Database error occurred (e.g., lost connectivity). Database(DBError), /// Utility function problem occurred (e.g., I/O error) @@ -1776,6 +1826,18 @@ impl From for RustCompilationCleanupError { } } +impl std::fmt::Display for RustCompilationCleanupError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + RustCompilationCleanupError::Database(e) => write!(f, "database error occurred: {e}"), + RustCompilationCleanupError::Utility(e) => write!(f, "utility error occurred: {e}"), + RustCompilationCleanupError::TargetCleared => { + write!(f, "target directory was cleared") + } + } + } +} + /// Makes the cleanup decision based on the provided name of the file or directory. fn decide_cleanup( name: &str, @@ -1817,27 +1879,74 @@ fn decide_cleanup( } } -/// Cleans up the Rust compilation working directory by removing binaries and compilation artifacts. -/// If the working directory `target` directory is about to become full (due to old dependencies), -/// it will be fully cleared. Otherwise, it will only clear compilation artifacts of pipeline -/// programs that no longer exist. -async fn cleanup_rust_compilation( +/// Decides whether a file in the pipeline-binaries directory is kept, +/// removed, or ignored by the binaries cleanup. +fn decide_pipeline_binary_cleanup( + filename: &str, + metadata: Option, + valid_pipeline_binary_filenames: &[String], + valid_program_info_filenames: &[String], +) -> CleanupDecision { + // Temp upload files share the binary name prefixes, so they must + // be decided before the prefix matches below would keep them. + if filename.contains(".tmp-") { + let is_stale_orphan = metadata + .and_then(|metadata| metadata.modified().ok()) + .and_then(|modified_time| modified_time.elapsed().ok()) + .is_some_and(|age| age >= STALE_TEMP_UPLOAD_MAX_AGE); + return if is_stale_orphan { + CleanupDecision::Remove + } else { + // Keep rather than Ignore: an upload may still be writing this + // file, and Ignore would warn on every cleanup pass. + CleanupDecision::Keep { + motivation: filename.to_string(), + } + }; + } + if filename.starts_with("pipeline_") { + if valid_pipeline_binary_filenames + .iter() + .any(|f| filename.starts_with(f)) + { + CleanupDecision::Keep { + motivation: filename.to_string(), + } + } else { + CleanupDecision::Remove + } + } else if filename.starts_with("program_info_") { + if valid_program_info_filenames + .iter() + .any(|f| filename.starts_with(f)) + { + CleanupDecision::Keep { + motivation: filename.to_string(), + } + } else { + CleanupDecision::Remove + } + } else { + CleanupDecision::Ignore + } +} + +/// Removes pipeline binaries and program info files that no longer correspond +/// to an existing pipeline program; only the latest version of each program is +/// retained. A row still `CompilingRust` has no checksums yet, so its files +/// are matched on a checksum-less prefix and retained. +pub(crate) async fn cleanup_pipeline_binaries( config: &CompilerConfig, db: Arc>, ) -> Result<(), RustCompilationCleanupError> { - trace!("Performing Rust cleanup..."); - - // Rust compilation directory - let rust_compilation_dir = config.working_dir().join("rust-compilation"); - if !rust_compilation_dir.exists() { + let pipeline_binaries_dir = config + .working_dir() + .join("rust-compilation") + .join("pipeline-binaries"); + if !pipeline_binaries_dir.is_dir() { return Ok(()); } - /////////////////////////////// - // PHASE 1: PIPELINE BINARIES - // Only the latest version binaries of successfully compiled pipeline - // programs are retained. Older version binaries are deleted. - // Retrieve existing pipeline programs // (pipeline_id, program_version, program_binary_source_checksum, program_binary_integrity_checksum, program_info_integrity_checksum) let existing_pipeline_programs = db @@ -1848,7 +1957,6 @@ async fn cleanup_rust_compilation( // Clean up pipeline binaries // These are not subject to the retention period. - let pipeline_binaries_dir = rust_compilation_dir.join("pipeline-binaries"); let valid_pipeline_binary_filenames: Vec = existing_pipeline_programs .iter() .map( @@ -1905,45 +2013,45 @@ async fn cleanup_rust_compilation( ) .collect(); - if pipeline_binaries_dir.is_dir() { - cleanup_specific_files( - "Rust compilation pipeline binaries", - &pipeline_binaries_dir, - Arc::new( - move |filename: &str, _metadata: Option| { - if filename.starts_with("pipeline_") { - if valid_pipeline_binary_filenames - .iter() - .any(|f| filename.starts_with(f)) - { - CleanupDecision::Keep { - motivation: filename.to_string(), - } - } else { - CleanupDecision::Remove - } - } else if filename.starts_with("program_info_") { - if valid_program_info_filenames - .iter() - .any(|f| filename.starts_with(f)) - { - CleanupDecision::Keep { - motivation: filename.to_string(), - } - } else { - CleanupDecision::Remove - } - } else { - CleanupDecision::Ignore - } - }, - ), - true, - false, - ) - .await?; + cleanup_specific_files( + "Rust compilation pipeline binaries", + &pipeline_binaries_dir, + Arc::new(move |filename: &str, metadata: Option| { + decide_pipeline_binary_cleanup( + filename, + metadata, + &valid_pipeline_binary_filenames, + &valid_program_info_filenames, + ) + }), + true, + true, + ) + .await?; + + Ok(()) +} + +/// Cleans up the Rust compilation working directory by removing binaries and compilation artifacts. +/// If the working directory `target` directory is about to become full (due to old dependencies), +/// it will be fully cleared. Otherwise, it will only clear compilation artifacts of pipeline +/// programs that no longer exist. +async fn cleanup_rust_compilation( + config: &CompilerConfig, + db: Arc>, +) -> Result<(), RustCompilationCleanupError> { + trace!("Performing Rust cleanup..."); + + // Rust compilation directory + let rust_compilation_dir = config.working_dir().join("rust-compilation"); + if !rust_compilation_dir.exists() { + return Ok(()); } + /////////////////////////////// + // PHASE 1: PIPELINE BINARIES + cleanup_pipeline_binaries(config, db.clone()).await?; + /////////////////////////////////// // PHASE 2: COMPILATION ARTIFACTS // Remove the artifacts used during compilation itself, notably @@ -2053,7 +2161,10 @@ async fn cleanup_rust_compilation( .is_ok_and(|duration| duration >= CLEANUP_RETENTION) { deletion.insert(artifact_name.clone()); - trace!("Rust compilation cleanup: retention for artifact '{}' has expired -- marked for deletion", artifact_name); + trace!( + "Rust compilation cleanup: retention for artifact '{}' has expired -- marked for deletion", + artifact_name + ); } } let deletion: Vec = deletion.into_iter().collect(); @@ -2069,8 +2180,11 @@ async fn cleanup_rust_compilation( &mut cleanup_specific_directories( "Rust compilation crates", &crates_dir, - Arc::new(move |name: &str| decide_cleanup(name, None, &deletion_clone)), + Arc::new(move |name: &str, _metadata: Option| { + decide_cleanup(name, None, &deletion_clone) + }), true, + false, ) .await?, ); @@ -2100,10 +2214,14 @@ async fn cleanup_rust_compilation( match target_size_byte { Ok(Ok(target_size_byte)) => { if target_size_byte < disk_space.total_byte / 20 { - error!("Not clearing `target` directory because its size ({target_size_byte} byte) is less than 5%. Please reduce disk usage in another way."); + error!( + "Not clearing `target` directory because its size ({target_size_byte} byte) is less than 5%. Please reduce disk usage in another way." + ); false } else { - error!("Removing `target` directory to make space (should clear up {target_size_byte} byte)..."); + error!( + "Removing `target` directory to make space (should clear up {target_size_byte} byte)..." + ); if let Err(e) = fs::remove_dir_all(&target_dir).await { error!( "Unable to remove `target` directory to make space due to: {e}" @@ -2126,11 +2244,15 @@ async fn cleanup_rust_compilation( } } Ok(Err(e)) => { - error!("Not clearing `target` directory because its size cannot be determined. Reduce disk usage in another way. Due to error: {e}"); + error!( + "Not clearing `target` directory because its size cannot be determined. Reduce disk usage in another way. Due to error: {e}" + ); false } Err(e) => { - error!("Not clearing `target` directory because its size cannot be determined due to a thread join error: {e}"); + error!( + "Not clearing `target` directory because its size cannot be determined due to a thread join error: {e}" + ); false } } @@ -2153,7 +2275,10 @@ async fn cleanup_rust_compilation( false } } else { - warn!("Unable to determine disk space remaining: unable to find disk corresponding to '{}'", target_dir.display()); + warn!( + "Unable to determine disk space remaining: unable to find disk corresponding to '{}'", + target_dir.display() + ); false } } else { @@ -2215,10 +2340,11 @@ async fn cleanup_rust_compilation( &mut cleanup_specific_directories( &format!("Rust compilation target/{target_profile_folder}/.fingerprint"), &fingerprint_dir, - Arc::new(move |name: &str| { + Arc::new(move |name: &str, _metadata: Option| { decide_cleanup(name, Some((true, '-')), &deletion_clone) }), false, + false, ) .await?, ); @@ -2232,10 +2358,11 @@ async fn cleanup_rust_compilation( &mut cleanup_specific_directories( &format!("Rust compilation target/{target_profile_folder}/incremental"), &incremental_dir, - Arc::new(move |name: &str| { + Arc::new(move |name: &str, _metadata: Option| { decide_cleanup(name, Some((true, '-')), &deletion_clone) }), false, + false, ) .await?, ); @@ -2298,10 +2425,13 @@ async fn cleanup_rust_compilation( mod test { use crate::auth::TenantRecord; use crate::compiler::rust_compiler::prepare_workspace; - use crate::compiler::rust_compiler::{calculate_source_checksum, decide_cleanup}; - use crate::compiler::test::{list_content_as_sorted_names, CompilerTest}; + use crate::compiler::rust_compiler::{ + STALE_TEMP_UPLOAD_MAX_AGE, calculate_source_checksum, decide_cleanup, + decide_pipeline_binary_cleanup, is_permanent_upload_rejection, + }; + use crate::compiler::test::{CompilerTest, list_content_as_sorted_names}; use crate::compiler::util::{ - crate_name_pipeline_globals, crate_name_pipeline_main, read_file_content, CleanupDecision, + CleanupDecision, crate_name_pipeline_globals, crate_name_pipeline_main, read_file_content, }; use crate::db::types::program::{CompilationProfile, ProgramStatus, RuntimeSelector}; use crate::db::types::utils::validate_program_info; @@ -2448,18 +2578,22 @@ mod test { .unwrap(), pipeline_descr.udf_rust ); - assert!(read_file_content(&globals_crate_path.join("Cargo.toml")) - .await - .unwrap() - .contains(&pipeline_descr.udf_toml)); + assert!( + read_file_content(&globals_crate_path.join("Cargo.toml")) + .await + .unwrap() + .contains(&pipeline_descr.udf_toml) + ); // Workspace-wide Cargo.toml let workspace_toml_file = test.rust_workdir.join("Cargo.toml"); assert!(workspace_toml_file.is_file()); - assert!(read_file_content(&workspace_toml_file) - .await - .unwrap() - .contains(&format!("members = [\n \"crates/{main_crate_name}\"\n]"))); + assert!( + read_file_content(&workspace_toml_file) + .await + .unwrap() + .contains(&format!("members = [\n \"crates/{main_crate_name}\"\n]")) + ); } /// Tests the binary delivery mode configuration. @@ -2544,4 +2678,98 @@ mod test { ); } } + + /// 4xx (except 408 and 429) and 507 fail an upload permanently; transient + /// statuses stay retryable. + #[test] + fn permanent_upload_rejection_classification() { + use reqwest::StatusCode; + assert!(is_permanent_upload_rejection(StatusCode::BAD_REQUEST)); + assert!(is_permanent_upload_rejection(StatusCode::NOT_FOUND)); + assert!(is_permanent_upload_rejection(StatusCode::PAYLOAD_TOO_LARGE)); + assert!(is_permanent_upload_rejection( + StatusCode::INSUFFICIENT_STORAGE + )); + assert!(!is_permanent_upload_rejection(StatusCode::REQUEST_TIMEOUT)); + assert!(!is_permanent_upload_rejection( + StatusCode::TOO_MANY_REQUESTS + )); + assert!(!is_permanent_upload_rejection( + StatusCode::INTERNAL_SERVER_ERROR + )); + assert!(!is_permanent_upload_rejection(StatusCode::BAD_GATEWAY)); + } + + /// Stale `.tmp-` uploads are removed, fresh or unstat-able ones are kept, + /// and prefix matching decides the rest. + #[test] + fn pipeline_binary_cleanup_decision() { + let valid_binaries = vec!["pipeline_a_v1_".to_string()]; + let valid_program_infos = vec!["program_info_a_v1_".to_string()]; + let decide = |filename: &str, metadata: Option| { + decide_pipeline_binary_cleanup( + filename, + metadata, + &valid_binaries, + &valid_program_infos, + ) + }; + + // Metadata with an mtime older than the staleness threshold + let temp_dir = tempfile::tempdir().unwrap(); + let stale_file_path = temp_dir.path().join("stale"); + let stale_file = std::fs::File::create(&stale_file_path).unwrap(); + let stale_mtime = std::time::SystemTime::now() + - (STALE_TEMP_UPLOAD_MAX_AGE + std::time::Duration::from_secs(60)); + stale_file + .set_times(std::fs::FileTimes::new().set_modified(stale_mtime)) + .unwrap(); + let stale_metadata = std::fs::metadata(&stale_file_path).unwrap(); + + // Metadata with a current mtime + let fresh_file_path = temp_dir.path().join("fresh"); + std::fs::File::create(&fresh_file_path).unwrap(); + let fresh_metadata = std::fs::metadata(&fresh_file_path).unwrap(); + + // Stale temp upload is removed + assert_eq!( + decide("pipeline_a_v1_x.tmp-123", Some(stale_metadata)), + CleanupDecision::Remove + ); + // Fresh temp upload is kept (upload may still be in flight) + assert_eq!( + decide("pipeline_a_v1_x.tmp-123", Some(fresh_metadata)), + CleanupDecision::Keep { + motivation: "pipeline_a_v1_x.tmp-123".to_string() + } + ); + // Missing metadata never removes a temp upload + assert_eq!( + decide("pipeline_a_v1_x.tmp-123", None), + CleanupDecision::Keep { + motivation: "pipeline_a_v1_x.tmp-123".to_string() + } + ); + // Binary with a valid prefix is kept, unknown is removed + assert_eq!( + decide("pipeline_a_v1_abc", None), + CleanupDecision::Keep { + motivation: "pipeline_a_v1_abc".to_string() + } + ); + assert_eq!(decide("pipeline_b_v1_abc", None), CleanupDecision::Remove); + // Program info with a valid prefix is kept, unknown is removed + assert_eq!( + decide("program_info_a_v1_abc", None), + CleanupDecision::Keep { + motivation: "program_info_a_v1_abc".to_string() + } + ); + assert_eq!( + decide("program_info_b_v1_abc", None), + CleanupDecision::Remove + ); + // Unrelated file is ignored + assert_eq!(decide("unrelated.txt", None), CleanupDecision::Ignore); + } } diff --git a/crates/pipeline-manager/src/compiler/sql_compiler.rs b/crates/pipeline-manager/src/compiler/sql_compiler.rs index e07dc02e54b..7e98453e565 100644 --- a/crates/pipeline-manager/src/compiler/sql_compiler.rs +++ b/crates/pipeline-manager/src/compiler/sql_compiler.rs @@ -1,9 +1,8 @@ use crate::common_error::CommonError; use crate::compiler::util::{ - cleanup_specific_directories, cleanup_specific_files, crate_name_pipeline_base, - crate_name_pipeline_globals, create_new_file, create_new_file_with_content, - encode_dir_as_string, read_file_content, recreate_dir, CleanupDecision, ProcessGroupTerminator, - UtilError, + CleanupDecision, ProcessGroupTerminator, UtilError, cleanup_specific_directories, + cleanup_specific_files, crate_name_pipeline_base, crate_name_pipeline_globals, create_new_file, + create_new_file_with_content, encode_dir_as_string, read_file_content, recreate_dir, }; use crate::config::{CommonConfig, CompilerConfig}; use crate::db::error::DBError; @@ -11,7 +10,7 @@ use crate::db::storage::Storage; use crate::db::storage_postgres::StoragePostgres; use crate::db::types::pipeline::PipelineId; use crate::db::types::program::{ - generate_program_info, RuntimeSelector, SqlCompilationInfo, SqlCompilerMessage, + RuntimeSelector, SqlCompilationInfo, SqlCompilerMessage, generate_program_info, }; use crate::db::types::tenant::TenantId; use crate::db::types::utils::validate_program_config; @@ -33,7 +32,7 @@ use tokio::{ fs, process::Command, sync::Mutex, - time::{sleep, Duration}, + time::{Duration, sleep}, }; use tracing::{debug, error, info, trace, warn}; use utoipa::ToSchema; @@ -81,10 +80,14 @@ pub async fn sql_compiler_task( if let Err(e) = cleanup_sql_compilation(&config, db.clone()).await { match e { SqlCompilationCleanupError::Database(e) => { - error!("SQL worker {worker_id}: compilation cleanup failed: database error occurred: {e}"); + error!( + "SQL worker {worker_id}: compilation cleanup failed: database error occurred: {e}" + ); } SqlCompilationCleanupError::Utility(e) => { - error!("SQL worker {worker_id}: compilation cleanup failed: filesystem operation error occurred: {e}"); + error!( + "SQL worker {worker_id}: compilation cleanup failed: filesystem operation error occurred: {e}" + ); } } unexpected_error = true; @@ -114,11 +117,15 @@ pub async fn sql_compiler_task( outdated_version, latest_version, } => { - debug!("SQL worker {worker_id}: compilation canceled: pipeline program version ({outdated_version}) is outdated by latest ({latest_version})"); + debug!( + "SQL worker {worker_id}: compilation canceled: pipeline program version ({outdated_version}) is outdated by latest ({latest_version})" + ); } e => { unexpected_error = true; - error!("SQL worker {worker_id}: compilation canceled: unexpected database error occurred: {e}"); + error!( + "SQL worker {worker_id}: compilation canceled: unexpected database error occurred: {e}" + ); } } } @@ -334,6 +341,51 @@ impl From for SqlCompilationError { } } +/// How long a cached SQL compiler jar is retained after its last use. +pub(crate) const JAR_CACHE_RETENTION: Duration = Duration::from_secs(7 * 24 * 3600); + +/// Removes a cached SQL compiler jar that was not accessed within +/// [`JAR_CACHE_RETENTION`]; jars still read by compilations or validations +/// stay cached. Missing metadata or access times never remove. +pub(crate) fn decide_stale_jar(jar_name: &str, metadata: Option) -> CleanupDecision { + let Some(metadata) = metadata else { + debug!("Failed to get metadata for JAR file"); + return CleanupDecision::Ignore; + }; + let atime = match metadata.accessed() { + Ok(atime) => atime, + Err(e) => { + debug!("Failed to get access time for JAR file: {:?}", e); + return CleanupDecision::Ignore; + } + }; + let Ok(elapsed) = atime.elapsed() else { + warn!( + "Unable to determine access time for JAR file, your system clock may be set incorrectly." + ); + return CleanupDecision::Ignore; + }; + if elapsed < JAR_CACHE_RETENTION { + trace!( + "Keeping {jar_name} because it was accessed within the retention window ({elapsed:?} ago)" + ); + CleanupDecision::Keep { + motivation: "Accessed within the retention window".to_string(), + } + } else { + CleanupDecision::Remove + } +} + +/// Directory in which downloaded SQL compiler jars for non-platform runtime +/// versions are cached. +pub(crate) fn jar_cache_dir(config: &CompilerConfig) -> PathBuf { + config + .working_dir() + .join("sql-compilation") + .join("jar-cache") +} + /// Determines the path to the SQL compiler executable based on the runtime selector. fn determine_sql_compiler_path( config: &CompilerConfig, @@ -341,16 +393,12 @@ fn determine_sql_compiler_path( ) -> PathBuf { match runtime_selector { RuntimeSelector::Platform(_) => PathBuf::from(&config.sql_compiler_path), - RuntimeSelector::Sha(sha) => config - .working_dir() - .join("sql-compilation") - .join("jar-cache") - .join(format!("sql2dbsp-jar-with-dependencies-{sha}.jar")), - RuntimeSelector::Version(version) => config - .working_dir() - .join("sql-compilation") - .join("jar-cache") - .join(format!("sql2dbsp-jar-with-dependencies-{version}.jar")), + RuntimeSelector::Sha(sha) => { + jar_cache_dir(config).join(format!("sql2dbsp-jar-with-dependencies-{sha}.jar")) + } + RuntimeSelector::Version(version) => { + jar_cache_dir(config).join(format!("sql2dbsp-jar-with-dependencies-{version}.jar")) + } } } @@ -363,10 +411,7 @@ async fn fetch_sql_compiler( "This code-path is only enabled in unstable mode" ); - let jar_cache_dir = config - .working_dir() - .join("sql-compilation") - .join("jar-cache"); + let jar_cache_dir = jar_cache_dir(config); fs::create_dir_all(&jar_cache_dir).await.map_err(|e| SqlCompilationError::SystemError(format!( "Unable initialize JAR cache directory '{}': {}. If possible, fall-back to platform version by removing `runtime_version` in the program config.", jar_cache_dir.display(), @@ -755,9 +800,9 @@ pub(crate) async fn perform_sql_compilation( Ok(messages) => messages, Err(e) => { if !exit_status.success() { - return Err(SqlCompilationError::SystemError( - format!("SQL compiler process returned with exit status code ({exit_code}) and stderr which cannot be deserialized due to {e}:\n{stderr_str}") - )); + return Err(SqlCompilationError::SystemError(format!( + "SQL compiler process returned with exit status code ({exit_code}) and stderr which cannot be deserialized due to {e}:\n{stderr_str}" + ))); } else { error!( pipeline_id = %pipeline_id, @@ -1003,7 +1048,7 @@ pub(crate) async fn cleanup_sql_compilation( cleanup_specific_directories( "SQL compilation directories", &sql_pipelines_dir, - Arc::new(move |dirname: &str| { + Arc::new(move |dirname: &str, _metadata: Option| { let spl: Vec<&str> = dirname.splitn(2, '-').collect(); if spl.len() == 2 && spl[0] == "pipeline" { if let Ok(uuid) = Uuid::parse_str(spl[1]) { @@ -1027,49 +1072,18 @@ pub(crate) async fn cleanup_sql_compilation( } }), true, + false, ) .await?; } // (3) Clean up JAR cache to make sure it does not grow unboundedly - let jar_cache_dir = config - .working_dir() - .join("sql-compilation") - .join("jar-cache"); + let jar_cache_dir = jar_cache_dir(config); if jar_cache_dir.is_dir() { cleanup_specific_files( "SQL JAR cache", &jar_cache_dir, - Arc::new(move |_jar_name: &str, metadata: Option| { - // Get rid of JAR files that have not been accessed within the last week - const MAX_AGE: Duration = Duration::from_secs(7*24*60*60); - if let Some(metadata) = metadata { - match metadata.accessed() { - Ok(atime) => { - if let Ok(elapsed) = atime.elapsed() { - if elapsed < MAX_AGE { - trace!("Keeping {_jar_name} because it was accessed within the last 7 days ({elapsed:?} ago)"); - CleanupDecision::Keep { - motivation: "Accessed within the last week".to_string(), - } - } else { - CleanupDecision::Remove - } - } else { - warn!("Unable to determine access time for JAR file, your system clock may be set incorrectly."); - CleanupDecision::Ignore - } - } - Err(e) => { - debug!("Failed to get access time for JAR file: {:?}", e); - CleanupDecision::Ignore - } - } - } else { - debug!("Failed to get metadata for JAR file"); - CleanupDecision::Ignore - } - }), + Arc::new(decide_stale_jar), true, true, ) @@ -1082,7 +1096,40 @@ pub(crate) async fn cleanup_sql_compilation( #[cfg(test)] mod test { use crate::auth::TenantRecord; - use crate::compiler::test::{list_content_as_sorted_names, CompilerTest}; + + /// A jar unaccessed past the retention window is removed, a recently + /// accessed one is kept, and missing metadata never removes. + #[test] + fn stale_jar_decision() { + use crate::compiler::sql_compiler::{JAR_CACHE_RETENTION, decide_stale_jar}; + use crate::compiler::util::CleanupDecision; + let tempdir = tempfile::tempdir().unwrap(); + let jar_path = tempdir.path().join("a.jar"); + std::fs::write(&jar_path, b"jar").unwrap(); + let recent = std::fs::metadata(&jar_path).unwrap(); + assert!(matches!( + decide_stale_jar("a.jar", Some(recent)), + CleanupDecision::Keep { .. } + )); + let old_time = std::time::SystemTime::now() - JAR_CACHE_RETENTION - JAR_CACHE_RETENTION; + let times = std::fs::FileTimes::new() + .set_accessed(old_time) + .set_modified(old_time); + std::fs::File::options() + .write(true) + .open(&jar_path) + .unwrap() + .set_times(times) + .unwrap(); + let old = std::fs::metadata(&jar_path).unwrap(); + assert_eq!( + decide_stale_jar("a.jar", Some(old)), + CleanupDecision::Remove + ); + assert_eq!(decide_stale_jar("a.jar", None), CleanupDecision::Ignore); + } + + use crate::compiler::test::{CompilerTest, list_content_as_sorted_names}; use crate::compiler::util::{create_new_file, recreate_dir}; use crate::db::types::program::ProgramStatus; use crate::db::types::utils::validate_program_info; @@ -1448,9 +1495,11 @@ mod test { assert_eq!(table_properties_only.name, "t1"); assert_eq!(table_properties_only.properties.len(), 1); assert!(table_properties_only.properties.contains_key("connectors")); - assert!(table_properties_only.properties["connectors"] - .value - .contains("\"name\": \"c1\"")); + assert!( + table_properties_only.properties["connectors"] + .value + .contains("\"name\": \"c1\"") + ); } /// Tests that SQL compiler recovers from an incorrect platform version. @@ -1534,11 +1583,13 @@ mod test { test.sql_compiler_tick().await; let pipeline_descr = test.get_pipeline(tenant_id, pipeline_id).await; assert_eq!(pipeline_descr.program_status, ProgramStatus::SqlError); - assert!(pipeline_descr - .program_error - .sql_compilation - .is_some_and(|info| info.messages.len() == 1 - && info.messages[0].to_owned().error_type == "Error parsing SQL")); + assert!( + pipeline_descr + .program_error + .sql_compilation + .is_some_and(|info| info.messages.len() == 1 + && info.messages[0].to_owned().error_type == "Error parsing SQL") + ); } /// Tests that compilation fails with an invalid connector. @@ -1561,11 +1612,13 @@ mod test { assert_eq!(pipeline_descr.program_status, ProgramStatus::SqlError); // First message is a warning about the connector missing a name // Second message is an error - assert!(pipeline_descr - .program_error - .sql_compilation - .is_some_and(|info| info.messages.len() == 2 - && info.messages[1].to_owned().error_type == "ConnectorGenerationError")); + assert!( + pipeline_descr + .program_error + .sql_compilation + .is_some_and(|info| info.messages.len() == 2 + && info.messages[1].to_owned().error_type == "ConnectorGenerationError") + ); } /// Tests that the cleanup ignores files and directories that do not follow the pattern. diff --git a/crates/pipeline-manager/src/compiler/test.rs b/crates/pipeline-manager/src/compiler/test.rs index a237ef109cf..0815641ebdc 100644 --- a/crates/pipeline-manager/src/compiler/test.rs +++ b/crates/pipeline-manager/src/compiler/test.rs @@ -1,5 +1,5 @@ use crate::compiler::sql_compiler::{attempt_end_to_end_sql_compilation, cleanup_sql_compilation}; -use crate::compiler::util::{encode_dir_as_string, read_file_content, DirectoryContent}; +use crate::compiler::util::{DirectoryContent, encode_dir_as_string, read_file_content}; use crate::config::{CommonConfig, CompilerConfig}; use crate::db::storage::Storage; use crate::db::storage_postgres::StoragePostgres; diff --git a/crates/pipeline-manager/src/compiler/util.rs b/crates/pipeline-manager/src/compiler/util.rs index 92baafbd7b8..7eafec1d092 100644 --- a/crates/pipeline-manager/src/compiler/util.rs +++ b/crates/pipeline-manager/src/compiler/util.rs @@ -1,12 +1,12 @@ use crate::db::types::pipeline::PipelineId; use crate::db::types::version::Version; -use base64::prelude::{Engine, BASE64_STANDARD}; +use base64::prelude::{BASE64_STANDARD, Engine}; use flate2::Compression; use hex; +use nix::NixPath; use nix::libc::pid_t; -use nix::sys::signal::{killpg, Signal}; +use nix::sys::signal::{Signal, killpg}; use nix::unistd::Pid; -use nix::NixPath; use openssl::sha::sha256; use sha2::{Digest, Sha256}; use std::fs::Metadata; @@ -69,7 +69,10 @@ impl Drop for ProcessGroupTerminator { }; // Send the SIGKILL to the PGRP if let Err(e) = killpg(Pid::from_raw(pgrp), Signal::SIGKILL) { - error!("Failed to cancel {}: attempt to kill the process and its subprocesses (PGRP: {}) failed: {e}", self.subject, self.process_group); + error!( + "Failed to cancel {}: attempt to kill the process and its subprocesses (PGRP: {}) failed: {e}", + self.subject, self.process_group + ); } debug!( "Successfully cancelled {} by killing its process group", @@ -518,7 +521,10 @@ pub async fn cleanup_specific_files( } } } else if warn_ignore { - warn!("{cleanup_name} cleanup: ignoring '{}' (unexpected directory entry which is not a file)", path.display()); + warn!( + "{cleanup_name} cleanup: ignoring '{}' (unexpected directory entry which is not a file)", + path.display() + ); } } Ok(keep_motivations) @@ -530,14 +536,20 @@ pub async fn cleanup_specific_files( pub async fn cleanup_specific_directories( cleanup_name: &str, dir: &Path, - decide: Arc CleanupDecision + Send + Sync>, + decide: DecisionFn, warn_ignore: bool, + add_metadata: bool, ) -> Result, UtilError> { let content = DirectoryContent::new(dir).await?.content; let mut keep_motivations = vec![]; for (path, name, is_file) in content { if !is_file { - match decide(&name) { + let metadata = if add_metadata { + fs::metadata(&path).await.ok() + } else { + None + }; + match decide(&name, metadata) { CleanupDecision::Keep { motivation } => { // If it should be kept, nothing needs to happen to the directory keep_motivations.push(motivation); @@ -561,7 +573,10 @@ pub async fn cleanup_specific_directories( } } } else if warn_ignore { - warn!("{cleanup_name} cleanup: ignoring '{}' (unexpected directory entry which is not a directory)", path.display()); + warn!( + "{cleanup_name} cleanup: ignoring '{}' (unexpected directory entry which is not a directory)", + path.display() + ); } } Ok(keep_motivations) @@ -710,13 +725,12 @@ impl DiskSpace { #[cfg(test)] mod test { use crate::compiler::util::{ - cleanup_specific_directories, cleanup_specific_files, copy_file, - copy_file_if_checksum_differs, crate_name_pipeline_base, crate_name_pipeline_globals, - crate_name_pipeline_main, create_dir_if_not_exists, create_new_file, - create_new_file_with_content, decode_string_as_dir, encode_dir_as_string, + CleanupDecision, DirectoryContent, cleanup_specific_directories, cleanup_specific_files, + copy_file, copy_file_if_checksum_differs, crate_name_pipeline_base, + crate_name_pipeline_globals, crate_name_pipeline_main, create_dir_if_not_exists, + create_new_file, create_new_file_with_content, decode_string_as_dir, encode_dir_as_string, pipeline_binary_filename, read_file_content, read_file_content_bytes, recreate_dir, recreate_file_with_content, truncate_sha256_checksum, validate_is_sha256_checksum, - CleanupDecision, DirectoryContent, }; use crate::db::types::pipeline::PipelineId; use crate::db::types::version::Version; @@ -1135,7 +1149,7 @@ mod test { cleanup_specific_directories( "", &dir_path, - Arc::new(|name: &str| { + Arc::new(|name: &str, _metadata: Option| { if name.starts_with("dir-") { CleanupDecision::Remove } else { @@ -1145,6 +1159,7 @@ mod test { } }), true, + false, ) .await .unwrap(); diff --git a/crates/pipeline-manager/src/config.rs b/crates/pipeline-manager/src/config.rs index cff01b60b66..5d7f1fef9ee 100644 --- a/crates/pipeline-manager/src/config.rs +++ b/crates/pipeline-manager/src/config.rs @@ -16,7 +16,7 @@ use openssl::pkey::PKey; use openssl::rsa::Rsa; use openssl::ssl::{SslConnector, SslMethod, SslVerifyMode}; use openssl::x509::extension::SubjectAlternativeName; -use openssl::x509::{X509NameBuilder, X509}; +use openssl::x509::{X509, X509NameBuilder}; use postgres_openssl::MakeTlsConnector; use reqwest::Certificate; use rustls::pki_types::pem::PemObject; @@ -812,7 +812,9 @@ impl DatabaseConfig { let mut connector = MakeTlsConnector::new(builder.build()); if self.disable_tls_hostname_verify { - warn!("PostgreSQL TLS hostname verification is disabled. The PostgreSQL server's hostname may not match the one specified in the SSL certificate."); + warn!( + "PostgreSQL TLS hostname verification is disabled. The PostgreSQL server's hostname may not match the one specified in the SSL certificate." + ); connector.set_callback(|ctx, _| { ctx.set_verify_hostname(false); Ok(()) @@ -1411,9 +1413,11 @@ mod tests { // A trust that names no issuer or no subject would match on the wrong // half of the pair, so it is refused at startup rather than at auth time. assert!(r#"[{"issuer": "", "subject": "x"}]"#.parse::().is_err()); - assert!(r#"[{"issuer": "https://idp.example", "subject": ""}]"# - .parse::() - .is_err()); + assert!( + r#"[{"issuer": "https://idp.example", "subject": ""}]"# + .parse::() + .is_err() + ); assert!("not json".parse::().is_err()); } diff --git a/crates/pipeline-manager/src/db/error.rs b/crates/pipeline-manager/src/db/error.rs index ba914cb803d..d3b4eb88792 100644 --- a/crates/pipeline-manager/src/db/error.rs +++ b/crates/pipeline-manager/src/db/error.rs @@ -9,13 +9,13 @@ use crate::db::types::tenant::TenantId; use crate::db::types::utils::ValidationError; use crate::db::types::version::Version; use actix_web::{ - body::BoxBody, http::StatusCode, HttpResponse, HttpResponseBuilder, ResponseError, + HttpResponse, HttpResponseBuilder, ResponseError, body::BoxBody, http::StatusCode, }; use deadpool_postgres::PoolError; use feldera_types::error::DetailedError; use feldera_types::error::ErrorResponse; use refinery::Error as RefineryError; -use serde::{ser::SerializeStruct, Serialize, Serializer}; +use serde::{Serialize, Serializer, ser::SerializeStruct}; use std::{backtrace::Backtrace, borrow::Cow, error::Error as StdError, fmt, fmt::Display}; use tokio_postgres::error::{Error as PgError, SqlState}; @@ -551,7 +551,10 @@ impl Display for DBError { ) } DBError::InvalidProgramError { value, error } => { - write!(f, "JSON for 'program_error' field:\n{value:#}\n\n... is not valid due to: {error}") + write!( + f, + "JSON for 'program_error' field:\n{value:#}\n\n... is not valid due to: {error}" + ) } DBError::EditRestrictedToClearedStorage { not_allowed } => { write!( @@ -561,19 +564,34 @@ impl Display for DBError { ) } DBError::InvalidErrorResponse { value, error } => { - write!(f, "JSON for 'deployment_error' field:\n{value:#}\n\n... is not valid due to: {error}") + write!( + f, + "JSON for 'deployment_error' field:\n{value:#}\n\n... is not valid due to: {error}" + ) } DBError::FailedToSerializeRuntimeConfig { error } => { - write!(f, "Unable to serialize runtime configuration for 'runtime_config' field as JSON due to: {error}") + write!( + f, + "Unable to serialize runtime configuration for 'runtime_config' field as JSON due to: {error}" + ) } DBError::FailedToSerializeProgramConfig { error } => { - write!(f, "Unable to serialize program configuration for 'program_config' field as JSON due to: {error}") + write!( + f, + "Unable to serialize program configuration for 'program_config' field as JSON due to: {error}" + ) } DBError::FailedToSerializeProgramError { error } => { - write!(f, "Unable to serialize program error for 'program_error' field as JSON due to: {error}") + write!( + f, + "Unable to serialize program error for 'program_error' field as JSON due to: {error}" + ) } DBError::FailedToSerializeErrorResponse { error } => { - write!(f, "Unable to serialize error response for 'deployment_error' field as JSON due to: {error}") + write!( + f, + "Unable to serialize error response for 'deployment_error' field as JSON due to: {error}" + ) } DBError::UniqueKeyViolation { constraint, .. } => { write!(f, "Unique key violation for '{constraint}'") @@ -720,16 +738,25 @@ impl Display for DBError { write!(f, "Unknown pipeline name '{pipeline_name}'") } DBError::UpdateRestrictedToStopped => { - write!(f, "Pipeline can only be updated while stopped. Stop it first by invoking '/stop'.") + write!( + f, + "Pipeline can only be updated while stopped. Stop it first by invoking '/stop'." + ) } DBError::ProgramStatusUpdateRestrictedToStopped => { write!(f, "Program status can only be updated while stopped.") } DBError::DeleteRestrictedToFullyStopped => { - write!(f, "Cannot delete a pipeline which is not fully stopped. Stop the pipeline first fully by invoking the '/stop' endpoint.") + write!( + f, + "Cannot delete a pipeline which is not fully stopped. Stop the pipeline first fully by invoking the '/stop' endpoint." + ) } DBError::CannotRenameNonExistingPipeline => { - write!(f, "The pipeline name in the request body does not match the one provided in the URL path. This is not allowed when no pipeline with the name provided in the URL path exists.") + write!( + f, + "The pipeline name in the request body does not match the one provided in the URL path. This is not allowed when no pipeline with the name provided in the URL path exists." + ) } DBError::OutdatedProgramVersion { outdated_version, @@ -930,7 +957,11 @@ impl Display for DBError { write!(f, "Invalid monitor status: '{value}'") } DBError::UnknownClusterMonitorEvent { event_id } => { - write!(f, "Cluster monitor event with identifier '{event_id}' does not exist -- it might have been deleted as monitor events are only retained for {}h and at most {}", MONITOR_RETENTION_HOURS, MONITOR_RETENTION_NUM) + write!( + f, + "Cluster monitor event with identifier '{event_id}' does not exist -- it might have been deleted as monitor events are only retained for {}h and at most {}", + MONITOR_RETENTION_HOURS, MONITOR_RETENTION_NUM + ) } DBError::NoClusterMonitorEventsAvailable => { write!(f, "There are not yet any cluster monitor events recorded") @@ -946,16 +977,25 @@ impl Display for DBError { ) } DBError::UnknownPipelineMonitorEvent { event_id } => { - write!(f, "Pipeline monitor event with identifier '{event_id}' does not exist -- it might have been deleted as only a limited number of events are retained") + write!( + f, + "Pipeline monitor event with identifier '{event_id}' does not exist -- it might have been deleted as only a limited number of events are retained" + ) } DBError::NoPipelineMonitorEventsAvailable => { write!(f, "There are not yet any pipeline monitor events recorded") } DBError::LockTookTooLong => { - write!(f, "The lock required for this operation took too long to acquire. Try this operation again later.") + write!( + f, + "The lock required for this operation took too long to acquire. Try this operation again later." + ) } DBError::DeadlockDetected => { - write!(f, "A deadlock was detected while performing the operation. Try this operation again later. Please also file a bug report, as this error should not happen.") + write!( + f, + "A deadlock was detected while performing the operation. Try this operation again later. Please also file a bug report, as this error should not happen." + ) } } } diff --git a/crates/pipeline-manager/src/db/listen_table.rs b/crates/pipeline-manager/src/db/listen_table.rs index 213661e5c92..1826698b263 100644 --- a/crates/pipeline-manager/src/db/listen_table.rs +++ b/crates/pipeline-manager/src/db/listen_table.rs @@ -2,7 +2,7 @@ use crate::db::error::DBError; use crate::db::storage_postgres::StoragePostgres; use crate::db::types::pipeline::PipelineId; use crate::db::types::tenant::TenantId; -use futures_util::{stream, StreamExt}; +use futures_util::{StreamExt, stream}; use std::sync::Arc; use std::time::Duration; use thiserror::Error as ThisError; @@ -128,7 +128,9 @@ async fn attempt_listen_table( if let Err(e) = notification_sender.try_send(n) { match e { TrySendError::Full(_n) => { - error!("Notifier is unable to send notification out on channel because it has reached capacity"); + error!( + "Notifier is unable to send notification out on channel because it has reached capacity" + ); } TrySendError::Closed(_n) => { break ListenError::NotificationReceiverClosed; @@ -148,7 +150,9 @@ async fn attempt_listen_table( } // AsyncMessage is marked non-exhaustive _ => { - error!("Notifier received AsyncMessage that isn't a notification or notice") + error!( + "Notifier received AsyncMessage that isn't a notification or notice" + ) } } } @@ -248,9 +252,9 @@ fn parse_notification( #[cfg(test)] mod test { use super::{ - listen_table, NotificationError, PipelineNotification, PIPELINE_NOTIFY_CHANNEL_CAPACITY, + NotificationError, PIPELINE_NOTIFY_CHANNEL_CAPACITY, PipelineNotification, listen_table, }; - use super::{parse_notification, Operation}; + use super::{Operation, parse_notification}; use crate::db::types::pipeline::{PatchClientMetadata, PipelineDescr, PipelineId}; use crate::db::types::program::ProgramConfig; use crate::db::types::tenant::TenantId; diff --git a/crates/pipeline-manager/src/db/operations/oidc_trust.rs b/crates/pipeline-manager/src/db/operations/oidc_trust.rs index 69a5f13f974..d756c292b45 100644 --- a/crates/pipeline-manager/src/db/operations/oidc_trust.rs +++ b/crates/pipeline-manager/src/db/operations/oidc_trust.rs @@ -2,10 +2,10 @@ use crate::db::error::DBError; use crate::db::operations::utils::{ maybe_tenant_id_foreign_key_constraint_err, maybe_unique_violation, }; -use crate::db::types::oidc_trust::{claim_matches, OidcTrustDescr, OidcTrustId}; +use crate::db::types::oidc_trust::{OidcTrustDescr, OidcTrustId, claim_matches}; use crate::db::types::role::Role; use crate::db::types::tenant::TenantId; -use crate::oidc::destination::{validate_tenant_oidc_url, TenantIssuerPolicy}; +use crate::oidc::destination::{TenantIssuerPolicy, validate_tenant_oidc_url}; use crate::oidc::trust_name::validate_oidc_trust_name; use deadpool_postgres::Transaction; use std::str::FromStr; @@ -211,10 +211,10 @@ pub async fn match_oidc_trust( if !claim_matches(&pattern_subject, subject) { continue; } - if let Some(aud_pattern) = &pattern_audience { - if !audiences.iter().any(|a| claim_matches(aud_pattern, a)) { - continue; - } + if let Some(aud_pattern) = &pattern_audience + && !audiences.iter().any(|a| claim_matches(aud_pattern, a)) + { + continue; } match matched.iter_mut().find(|(t, _)| *t == tenant_id) { Some(entry) => entry.1 = entry.1.max(role), diff --git a/crates/pipeline-manager/src/db/operations/pipeline.rs b/crates/pipeline-manager/src/db/operations/pipeline.rs index 89d370b61bd..059b0294df8 100644 --- a/crates/pipeline-manager/src/db/operations/pipeline.rs +++ b/crates/pipeline-manager/src/db/operations/pipeline.rs @@ -2,27 +2,27 @@ use crate::api::support_data_collector::SupportBundleData; use crate::db::error::DBError; use crate::db::operations::pipeline_monitor::new_pipeline_monitor_event; use crate::db::operations::pipeline_parsing::{ + PIPELINE_COLUMNS_ALL, PIPELINE_COLUMNS_EVENT_INFO, PIPELINE_COLUMNS_MONITORING, parse_pipeline_row_all, parse_pipeline_row_event_info, parse_pipeline_row_monitoring, - serialize_error_response, serialize_program_error, PIPELINE_COLUMNS_ALL, - PIPELINE_COLUMNS_EVENT_INFO, PIPELINE_COLUMNS_MONITORING, + serialize_error_response, serialize_program_error, }; use crate::db::operations::utils::{ maybe_tenant_id_foreign_key_constraint_err, maybe_unique_violation, }; use crate::db::types::pipeline::{ - bootstrap_config_to_string, runtime_desired_status_to_string, runtime_status_to_string, ExtendedPipelineDescr, ExtendedPipelineDescrEventInfo, ExtendedPipelineDescrMonitoring, - PatchClientMetadata, PipelineDescr, PipelineId, + PatchClientMetadata, PipelineDescr, PipelineId, bootstrap_config_to_string, + runtime_desired_status_to_string, runtime_status_to_string, }; use crate::db::types::program::{ - validate_program_status_transition, ProgramError, ProgramStatus, RustCompilationInfo, - SqlCompilationInfo, + ProgramError, ProgramStatus, RustCompilationInfo, SqlCompilationInfo, + validate_program_status_transition, }; use crate::db::types::resources_status::{ - validate_resources_desired_status_transition, validate_resources_status_transition, - ResourcesDesiredStatus, ResourcesStatus, + ResourcesDesiredStatus, ResourcesStatus, validate_resources_desired_status_transition, + validate_resources_status_transition, }; -use crate::db::types::storage::{validate_storage_status_transition, StorageStatus}; +use crate::db::types::storage::{StorageStatus, validate_storage_status_transition}; use crate::db::types::tenant::TenantId; use crate::db::types::utils::{ validate_deployment_config, validate_pipeline_name, validate_program_config, @@ -1128,21 +1128,19 @@ pub(crate) async fn set_deployment_resources_desired_status( }; // If the current initial desired runtime status is already set, it cannot be changed - if let Some(current_initial) = current.deployment_initial { - if let Some(new_initial) = final_deployment_initial { - if current_initial != new_initial { - return Err(DBError::InitialImmutableUnlessStopped); - } - } + if let Some(current_initial) = current.deployment_initial + && let Some(new_initial) = final_deployment_initial + && current_initial != new_initial + { + return Err(DBError::InitialImmutableUnlessStopped); } // If the current bootstrap policy is already set, it cannot be changed - if let Some(current_bootstrap_config) = current.bootstrap_policy { - if let Some(new_bootstrap_config) = final_bootstrap_config { - if current_bootstrap_config != new_bootstrap_config { - return Err(DBError::BootstrapPolicyImmutableUnlessStopped); - } - } + if let Some(current_bootstrap_config) = current.bootstrap_policy + && let Some(new_bootstrap_config) = final_bootstrap_config + && current_bootstrap_config != new_bootstrap_config + { + return Err(DBError::BootstrapPolicyImmutableUnlessStopped); } // Desired status cannot be set to standby if no file backend is configured @@ -1859,6 +1857,7 @@ pub(crate) async fn list_pipelines_across_all_tenants_needing_sql_compilation_cl worker_id: usize, total_workers: usize, ) -> Result, DBError> { + // Predicate changes must be mirrored in count_pipelines_needing_compilation. let stmt = txn .prepare_cached(&format!( "SELECT p.tenant_id, {PIPELINE_COLUMNS_MONITORING} @@ -1908,6 +1907,7 @@ pub(crate) async fn list_pipelines_across_all_tenants_needing_rust_compilation_c worker_id: usize, total_workers: usize, ) -> Result, DBError> { + // Predicate changes must be mirrored in count_pipelines_needing_compilation. let stmt = txn .prepare_cached(&format!( "SELECT p.tenant_id, {PIPELINE_COLUMNS_MONITORING} @@ -1952,6 +1952,7 @@ pub(crate) async fn get_next_sql_compilation( worker_id: usize, total_workers: usize, ) -> Result, DBError> { + // Predicate changes must be mirrored in count_pipelines_needing_compilation. let stmt = txn .prepare_cached(&format!( "SELECT p.tenant_id, {PIPELINE_COLUMNS_ALL} @@ -1992,6 +1993,7 @@ pub(crate) async fn get_next_rust_compilation( worker_id: usize, total_workers: usize, ) -> Result, DBError> { + // Predicate changes must be mirrored in count_pipelines_needing_compilation. let stmt = txn .prepare_cached(&format!( "SELECT p.tenant_id, {PIPELINE_COLUMNS_ALL} @@ -2024,6 +2026,30 @@ pub(crate) async fn get_next_rust_compilation( } } +/// Counts pipelines with outstanding compilation work. +/// +/// The predicate must remain the union of the predicates of the four worker +/// queries above (the two `_needing_*_compilation_clear` lists and the two +/// `get_next_*_compilation` pickers) across all shards and platform versions: +/// the count is greater than zero if and only if some compiler worker would +/// act. It drives compiler autoscaling in the enterprise runner. +pub(crate) async fn count_pipelines_needing_compilation( + txn: &Transaction<'_>, +) -> Result { + let stmt = txn + .prepare_cached( + "SELECT COUNT(*) + FROM pipeline AS p + WHERE p.deployment_resources_status = 'stopped' + AND p.program_status IN ('pending', 'compiling_sql', 'sql_compiled', 'compiling_rust') + ", + ) + .await?; + let row = txn.query_one(&stmt, &[]).await?; + let count: i64 = row.get(0); + Ok(count as u64) +} + /// Retrieves the list of successfully compiled pipeline programs (pipeline identifier, program version, /// program binary source checksum, program binary integrity checksum, program info integrity checksum) AND pipeline programs that /// are currently being compiled (pipeline identifier, program version) across all tenants. diff --git a/crates/pipeline-manager/src/db/operations/pipeline_monitor.rs b/crates/pipeline-manager/src/db/operations/pipeline_monitor.rs index deab05c97cf..4cd7723f5db 100644 --- a/crates/pipeline-manager/src/db/operations/pipeline_monitor.rs +++ b/crates/pipeline-manager/src/db/operations/pipeline_monitor.rs @@ -3,15 +3,15 @@ use crate::db::operations::pipeline::{ get_pipeline_by_id_for_event_info, get_pipeline_for_monitoring, }; use crate::db::operations::pipeline_parsing::{ - parse_pipeline_event_row_extended, parse_pipeline_event_row_short, serialize_error_response, - PIPELINE_EVENT_COLUMNS_ALL, PIPELINE_EVENT_COLUMNS_SHORT, + PIPELINE_EVENT_COLUMNS_ALL, PIPELINE_EVENT_COLUMNS_SHORT, parse_pipeline_event_row_extended, + parse_pipeline_event_row_short, serialize_error_response, }; use crate::db::operations::utils::maybe_unique_violation; use crate::db::types::monitor::{ ExtendedPipelineMonitorEvent, PipelineMonitorEvent, PipelineMonitorEventId, }; use crate::db::types::pipeline::{ - runtime_desired_status_to_string, runtime_status_to_string, PipelineId, + PipelineId, runtime_desired_status_to_string, runtime_status_to_string, }; use crate::db::types::tenant::TenantId; use deadpool_postgres::Transaction; diff --git a/crates/pipeline-manager/src/db/operations/pipeline_parsing.rs b/crates/pipeline-manager/src/db/operations/pipeline_parsing.rs index 937a93b3e77..b328b782851 100644 --- a/crates/pipeline-manager/src/db/operations/pipeline_parsing.rs +++ b/crates/pipeline-manager/src/db/operations/pipeline_parsing.rs @@ -3,9 +3,9 @@ use crate::db::types::monitor::{ ExtendedPipelineMonitorEvent, PipelineMonitorEvent, PipelineMonitorEventId, }; use crate::db::types::pipeline::{ - parse_string_as_bootstrap_config, parse_string_as_runtime_desired_status, - parse_string_as_runtime_status, ClientMetadata, ExtendedPipelineDescr, - ExtendedPipelineDescrEventInfo, ExtendedPipelineDescrMonitoring, PipelineId, + ClientMetadata, ExtendedPipelineDescr, ExtendedPipelineDescrEventInfo, + ExtendedPipelineDescrMonitoring, PipelineId, parse_string_as_bootstrap_config, + parse_string_as_runtime_desired_status, parse_string_as_runtime_status, }; use crate::db::types::program::{ProgramError, ProgramStatus}; use crate::db::types::resources_status::{ResourcesDesiredStatus, ResourcesStatus}; @@ -524,10 +524,10 @@ fn deserialize_program_error_with_default(s: &str) -> ProgramError { #[cfg(test)] mod tests { use super::{ - deserialize_error_response, deserialize_json_value, deserialize_program_error, - deserialize_program_error_with_default, serialize_error_response, serialize_program_error, PIPELINE_COLUMNS_ALL, PIPELINE_COLUMNS_EVENT_INFO, PIPELINE_COLUMNS_MONITORING, - PIPELINE_EVENT_COLUMNS_ALL, PIPELINE_EVENT_COLUMNS_SHORT, + PIPELINE_EVENT_COLUMNS_ALL, PIPELINE_EVENT_COLUMNS_SHORT, deserialize_error_response, + deserialize_json_value, deserialize_program_error, deserialize_program_error_with_default, + serialize_error_response, serialize_program_error, }; use crate::db::error::DBError; use crate::db::types::monitor::{ diff --git a/crates/pipeline-manager/src/db/operations/utils.rs b/crates/pipeline-manager/src/db/operations/utils.rs index 7ed46c05ca2..ead34de7dda 100644 --- a/crates/pipeline-manager/src/db/operations/utils.rs +++ b/crates/pipeline-manager/src/db/operations/utils.rs @@ -43,14 +43,12 @@ pub(crate) fn maybe_tenant_id_foreign_key_constraint_err( ) -> DBError { if let DBError::PostgresError { error, .. } = &err { let db_err = error.as_db_error(); - if let Some(db_err) = db_err { - if db_err.code() == &tokio_postgres::error::SqlState::FOREIGN_KEY_VIOLATION { - if let Some(constraint_name) = db_err.constraint() { - if constraint_name.ends_with("tenant_id_fkey") { - return DBError::UnknownTenant { tenant_id }; - } - } - } + if let Some(db_err) = db_err + && db_err.code() == &tokio_postgres::error::SqlState::FOREIGN_KEY_VIOLATION + && let Some(constraint_name) = db_err.constraint() + && constraint_name.ends_with("tenant_id_fkey") + { + return DBError::UnknownTenant { tenant_id }; } } err @@ -61,18 +59,15 @@ pub(crate) fn maybe_tenant_id_foreign_key_constraint_err( /// instead of surfacing a raw Postgres error. Other errors pass through /// unchanged. pub(crate) fn maybe_user_id_foreign_key_constraint_err(err: DBError, user_id: UserId) -> DBError { - if let DBError::PostgresError { error, .. } = &err { - if let Some(db_err) = error.as_db_error() { - if db_err.code() == &tokio_postgres::error::SqlState::FOREIGN_KEY_VIOLATION { - if let Some(constraint_name) = db_err.constraint() { - if constraint_name.ends_with("user_id_fkey") { - return DBError::UnknownUser { - user_id: user_id.to_string(), - }; - } - } - } - } + if let DBError::PostgresError { error, .. } = &err + && let Some(db_err) = error.as_db_error() + && db_err.code() == &tokio_postgres::error::SqlState::FOREIGN_KEY_VIOLATION + && let Some(constraint_name) = db_err.constraint() + && constraint_name.ends_with("user_id_fkey") + { + return DBError::UnknownUser { + user_id: user_id.to_string(), + }; } err } diff --git a/crates/pipeline-manager/src/db/storage.rs b/crates/pipeline-manager/src/db/storage.rs index 2930734a0ee..6269f369c48 100644 --- a/crates/pipeline-manager/src/db/storage.rs +++ b/crates/pipeline-manager/src/db/storage.rs @@ -654,6 +654,13 @@ pub(crate) trait Storage { total_workers: usize, ) -> Result, DBError>; + /// Counts pipelines with outstanding compilation work: stopped pipelines + /// whose program status is `Pending`, `CompilingSql`, `SqlCompiled`, or + /// `CompilingRust`. The predicate must stay the union of the four worker + /// queries; the full invariant is documented at + /// `operations::pipeline::count_pipelines_needing_compilation`. + async fn count_pipelines_needing_compilation(&self) -> Result; + /// Retrieves the list of fully compiled pipeline programs (pipeline identifier, program version, /// program binary source checksum, program binary integrity checksum) AND pipeline programs that /// are currently being compiled (pipeline identifier, program version) across all tenants. diff --git a/crates/pipeline-manager/src/db/storage_postgres.rs b/crates/pipeline-manager/src/db/storage_postgres.rs index 5f355abafa7..39cc6d15a22 100644 --- a/crates/pipeline-manager/src/db/storage_postgres.rs +++ b/crates/pipeline-manager/src/db/storage_postgres.rs @@ -1542,6 +1542,14 @@ impl Storage for StoragePostgres { Ok(next_pipeline_program) } + async fn count_pipelines_needing_compilation(&self) -> Result { + let mut client = self.pool.get().await?; + let txn = client.transaction().await?; + let count = operations::pipeline::count_pipelines_needing_compilation(&txn).await?; + txn.commit().await?; + Ok(count) + } + async fn list_pipeline_programs_across_all_tenants( &self, ) -> Result< @@ -1824,6 +1832,13 @@ impl Storage for StoragePostgres { } impl StoragePostgres { + /// Public mirror of the `Storage::count_pipelines_needing_compilation` + /// trait method: the enterprise runner derives compiler autoscaling demand + /// from it but cannot name the crate-private `Storage` trait. + pub async fn count_pipelines_needing_compilation(&self) -> Result { + ::count_pipelines_needing_compilation(self).await + } + pub async fn connect( db_config: &DatabaseConfig, #[cfg(feature = "postgresql_embedded")] pg_embed_config: PgEmbedConfig, diff --git a/crates/pipeline-manager/src/db/test.rs b/crates/pipeline-manager/src/db/test.rs index ca852a05c07..e3bdecca54d 100644 --- a/crates/pipeline-manager/src/db/test.rs +++ b/crates/pipeline-manager/src/db/test.rs @@ -1,41 +1,41 @@ use crate::api::support_data_collector::SupportBundleData; -use crate::auth::{generate_api_key, TenantRecord}; +use crate::auth::{TenantRecord, generate_api_key}; use crate::db::error::DBError; use crate::db::error::DBError::InvalidResourcesStatusNotRemain; use crate::db::operations::pipeline::get_pipeline_by_id_for_monitoring; use crate::db::storage::{ExtendedPipelineDescrRunner, Storage}; -use crate::db::storage_postgres::{is_pipeline_assigned_to_worker, StoragePostgres}; +use crate::db::storage_postgres::{StoragePostgres, is_pipeline_assigned_to_worker}; use crate::db::types::api_key::{ApiKeyDescr, ApiKeyId}; use crate::db::types::monitor::{ ClusterMonitorEvent, ClusterMonitorEventId, ExtendedClusterMonitorEvent, ExtendedPipelineMonitorEvent, MonitorStatus, NewClusterMonitorEvent, PipelineMonitorEvent, PipelineMonitorEventId, }; -use crate::db::types::oidc_trust::{claim_matches, OidcTrustDescr, OidcTrustId}; +use crate::db::types::oidc_trust::{OidcTrustDescr, OidcTrustId, claim_matches}; use crate::db::types::pipeline::{ ClientMetadata, ExtendedPipelineDescr, ExtendedPipelineDescrMonitoring, PatchClientMetadata, PipelineDescr, PipelineId, }; use crate::db::types::program::{ - generate_pipeline_config, validate_program_status_transition, CompilationProfile, - ProgramConfig, ProgramError, ProgramInfo, ProgramStatus, RustCompilationInfo, - SqlCompilationInfo, + CompilationProfile, ProgramConfig, ProgramError, ProgramInfo, ProgramStatus, + RustCompilationInfo, SqlCompilationInfo, generate_pipeline_config, + validate_program_status_transition, }; use crate::db::types::resources_status::{ - validate_resources_desired_status_transition, validate_resources_status_transition, - ResourcesDesiredStatus, ResourcesStatus, + ResourcesDesiredStatus, ResourcesStatus, validate_resources_desired_status_transition, + validate_resources_status_transition, }; use crate::db::types::role::{MemberRole, MintableKeyRole, Role}; -use crate::db::types::storage::{validate_storage_status_transition, StorageStatus}; +use crate::db::types::storage::{StorageStatus, validate_storage_status_transition}; use crate::db::types::tenant::TenantId; use crate::db::types::user::{TenantInfo, TenantMember, UserId}; use crate::db::types::utils::{ - validate_api_key_name, validate_deployment_config, validate_pipeline_name, + MAXIMUM_TAG_LENGTH, validate_api_key_name, validate_deployment_config, validate_pipeline_name, validate_program_config, validate_program_info, validate_runtime_config, - validate_storage_status_details, MAXIMUM_TAG_LENGTH, + validate_storage_status_details, }; use crate::db::types::version::Version; -use crate::oidc::destination::{validate_tenant_oidc_url, TenantIssuerPolicy}; +use crate::oidc::destination::{TenantIssuerPolicy, validate_tenant_oidc_url}; use crate::oidc::trust_name::validate_oidc_trust_name; use async_trait::async_trait; use chrono::{TimeZone, Utc}; @@ -61,7 +61,7 @@ use std::sync::Arc; use std::time::{Duration, Instant}; use std::vec; use tokio::spawn; -use tokio::sync::{oneshot, Mutex}; +use tokio::sync::{Mutex, oneshot}; use tokio::time::sleep; use tracing::info; use uuid::Uuid; @@ -91,7 +91,7 @@ impl Drop for DbHandle { #[cfg(not(feature = "postgresql_embedded"))] fn drop(&mut self) { use postgres_openssl::TlsStream; - use tokio_postgres::{tls::NoTlsStream, Connection, Socket}; + use tokio_postgres::{Connection, Socket, tls::NoTlsStream}; enum ConnWrapper { Tls(Connection>), NoTls(Connection), @@ -173,7 +173,10 @@ pub(crate) async fn setup_pg() -> (StoragePostgres, tempfile::TempDir) { match crate::db::pg_setup::install(temp_path.into(), false, Some(port)).await { Ok(pg) => break (_temp_dir, pg), Err(e) => { - info!("Unable to install test database on port {port} ({} attempts left) -- port might have become occupied in the meanwhile. Original error: {e}", 10 - attempt); + info!( + "Unable to install test database on port {port} ({} attempts left) -- port might have become occupied in the meanwhile. Original error: {e}", + 10 - attempt + ); sleep(Duration::from_millis(100)).await; } } @@ -637,7 +640,7 @@ fn limited_pipeline_config() -> impl Strategy { } else { val.2.invalid0 = 1; // Prevent it from being invalid let runtime_config = map_val_to_limited_runtime_config(val.2); - val.3 .0 = 1; // Prevent it from being invalid + val.3.0 = 1; // Prevent it from being invalid let program_info: ProgramInfo = serde_json::from_value(map_val_to_limited_program_info(val.3)).unwrap(); serde_json::to_value(PipelineConfig { @@ -729,15 +732,14 @@ fn limited_optional_storage_status_details() -> impl Strategy ExtendedPipelineDescr { + db.new_pipeline( + tenant_id, + Uuid::now_v7(), + "v0", + PipelineDescr { + name: name.to_string(), + description: "".to_string(), + tags: vec![], + runtime_config: json!({}), + program_code: "".to_string(), + udf_rust: "".to_string(), + udf_toml: "".to_string(), + program_config: json!({}), + }, + ) + .await + .unwrap() + } + + async fn advance_program_status( + db: &StoragePostgres, + tenant_id: TenantId, + pipeline_id: PipelineId, + statuses: &[ProgramStatus], + program_info: &serde_json::Value, + ) { + let sql_compilation = SqlCompilationInfo { + exit_code: 0, + messages: vec![], + }; + let rust_compilation = RustCompilationInfo { + exit_code: 0, + stdout: "".to_string(), + stderr: "".to_string(), + }; + for status in statuses { + match status { + ProgramStatus::Pending => db + .transit_program_status_to_pending(tenant_id, pipeline_id, Version(1)) + .await + .unwrap(), + ProgramStatus::CompilingSql => db + .transit_program_status_to_compiling_sql(tenant_id, pipeline_id, Version(1)) + .await + .unwrap(), + ProgramStatus::SqlCompiled => db + .transit_program_status_to_sql_compiled( + tenant_id, + pipeline_id, + Version(1), + &sql_compilation, + program_info, + ) + .await + .unwrap(), + ProgramStatus::CompilingRust => db + .transit_program_status_to_compiling_rust(tenant_id, pipeline_id, Version(1)) + .await + .unwrap(), + ProgramStatus::Success => db + .transit_program_status_to_success( + tenant_id, + pipeline_id, + Version(1), + &rust_compilation, + "abc", + "123", + "456", + ) + .await + .unwrap(), + ProgramStatus::SqlError => db + .transit_program_status_to_sql_error( + tenant_id, + pipeline_id, + Version(1), + &sql_compilation, + ) + .await + .unwrap(), + ProgramStatus::RustError => db + .transit_program_status_to_rust_error( + tenant_id, + pipeline_id, + Version(1), + &rust_compilation, + ) + .await + .unwrap(), + ProgramStatus::SystemError => db + .transit_program_status_to_system_error( + tenant_id, + pipeline_id, + Version(1), + "system error", + ) + .await + .unwrap(), + } + } + } + + let handle = test_setup().await; + let tenant_id = TenantRecord::default().id; + let program_info = serde_json::to_value(ProgramInfo { + schema: serde_json::to_value(ProgramSchema { + inputs: vec![], + outputs: vec![], + }) + .unwrap(), + main_rust: "".to_string(), + udf_stubs: "".to_string(), + input_connectors: BTreeMap::new(), + output_connectors: BTreeMap::new(), + dataflow: None, + }) + .unwrap(); + + assert_eq!( + handle + .db + .count_pipelines_needing_compilation() + .await + .unwrap(), + 0 + ); + + // One counted pipeline per status with outstanding compilation work + let p_pending = new_test_pipeline(&handle.db, tenant_id, "counted-pending").await; + for (name, statuses) in [ + ("counted-compiling-sql", vec![ProgramStatus::CompilingSql]), + ( + "counted-sql-compiled", + vec![ProgramStatus::CompilingSql, ProgramStatus::SqlCompiled], + ), + ( + "counted-compiling-rust", + vec![ + ProgramStatus::CompilingSql, + ProgramStatus::SqlCompiled, + ProgramStatus::CompilingRust, + ], + ), + ] { + let pipeline = new_test_pipeline(&handle.db, tenant_id, name).await; + advance_program_status(&handle.db, tenant_id, pipeline.id, &statuses, &program_info).await; + } + assert_eq!( + handle + .db + .count_pipelines_needing_compilation() + .await + .unwrap(), + 4 + ); + + // Terminal statuses are counted while in flight, no longer once reached + for (name, statuses) in [ + ( + "done-success", + vec![ + ProgramStatus::CompilingSql, + ProgramStatus::SqlCompiled, + ProgramStatus::CompilingRust, + ProgramStatus::Success, + ], + ), + ( + "done-sql-error", + vec![ProgramStatus::CompilingSql, ProgramStatus::SqlError], + ), + ( + "done-rust-error", + vec![ + ProgramStatus::CompilingSql, + ProgramStatus::SqlCompiled, + ProgramStatus::CompilingRust, + ProgramStatus::RustError, + ], + ), + ("done-system-error", vec![ProgramStatus::SystemError]), + ] { + let pipeline = new_test_pipeline(&handle.db, tenant_id, name).await; + assert_eq!( + handle + .db + .count_pipelines_needing_compilation() + .await + .unwrap(), + 5 + ); + advance_program_status(&handle.db, tenant_id, pipeline.id, &statuses, &program_info).await; + assert_eq!( + handle + .db + .count_pipelines_needing_compilation() + .await + .unwrap(), + 4 + ); + } + + // A non-stopped deployment is not counted. Non-stopped resources with an + // unfinished program is unreachable through the Storage API (provisioning + // requires a compiled program), so force the state to pin the predicate. + for (resources_status, expected_count) in [("provisioning", 3), ("stopped", 4)] { + handle + .db + .pool + .get() + .await + .unwrap() + .execute( + "UPDATE pipeline SET deployment_resources_status = $1 WHERE id = $2", + &[&resources_status, &p_pending.id.0], + ) + .await + .unwrap(); + assert_eq!( + handle + .db + .count_pipelines_needing_compilation() + .await + .unwrap(), + expected_count + ); + } +} + /// Tests the following sequence of events: /// - Pipeline is compiled /// - User calls /start @@ -3938,16 +4187,12 @@ async fn pipeline_concurrent_access_deadlock() { return Some(e); } txn1.commit().await.unwrap(); - return None; + None }); rx.await.unwrap(); - let t2_error = if let Err(e) = - get_pipeline_by_id_for_monitoring(&txn2, tenant_id, pipeline1.id, true).await - { - Some(e) - } else { - None - }; + let t2_error = get_pipeline_by_id_for_monitoring(&txn2, tenant_id, pipeline1.id, true) + .await + .err(); let t1_error = join_handle.await.unwrap(); assert!(!(t1_error.is_some() && t2_error.is_some())); let error = t1_error.unwrap_or_else(|| { @@ -4380,6 +4625,7 @@ enum StorageAction { GetNextSqlCompilation(#[proptest(strategy = "limited_platform_version()")] String), ClearOngoingRustCompilation(#[proptest(strategy = "limited_platform_version()")] String), GetNextRustCompilation(#[proptest(strategy = "limited_platform_version()")] String), + CountPipelinesNeedingCompilation, ListPipelineProgramsAcrossAllTenants, ListClusterMonitorEvents, GetClusterMonitorEventShort(ClusterMonitorEventId), @@ -4533,10 +4779,14 @@ fn check_responses( "mismatch detected with model (left) and impl (right)" ), (Err(err_model), Ok(val_impl)) => { - panic!("step({step}): model returned error: {err_model:?}, but impl returned result: {val_impl:?}"); + panic!( + "step({step}): model returned error: {err_model:?}, but impl returned result: {val_impl:?}" + ); } (Ok(val_model), Err(err_impl)) => { - panic!("step({step}): model returned result: {val_model:?}, but impl returned error: {err_impl:?}"); + panic!( + "step({step}): model returned result: {val_model:?}, but impl returned error: {err_impl:?}" + ); } (Err(err_model), Err(err_impl)) => { assert_eq!( @@ -5174,6 +5424,11 @@ fn db_impl_behaves_like_model() { let impl_response = handle.db.get_next_rust_compilation(&platform_version, 0, 1).await; check_response_optional_pipeline_with_tenant_id(i, model_response, impl_response); } + StorageAction::CountPipelinesNeedingCompilation => { + let model_response = model.count_pipelines_needing_compilation().await; + let impl_response = handle.db.count_pipelines_needing_compilation().await; + check_responses(i, model_response, impl_response); + } StorageAction::ListPipelineProgramsAcrossAllTenants => { let model_response = model.list_pipeline_programs_across_all_tenants().await; let impl_response = handle.db.list_pipeline_programs_across_all_tenants().await; @@ -5812,7 +6067,9 @@ impl Storage for Mutex { _tenant_name: String, _provider: String, ) -> DBResult { - panic!("For model-based tests, we generate the TenantID using proptest, as opposed to generating a claim that we then get or create an ID for"); + panic!( + "For model-based tests, we generate the TenantID using proptest, as opposed to generating a claim that we then get or create an ID for" + ); } async fn create_tenant(&self, id: Uuid, _name: &str, _provider: &str) -> DBResult { @@ -5831,11 +6088,11 @@ impl Storage for Mutex { let s = self.lock().await; Ok(s.api_keys .iter() - .filter(|k| k.0 .0 == tenant_id) + .filter(|k| k.0.0 == tenant_id) .map(|k| ApiKeyDescr { - id: k.1 .0, - name: k.0 .1.clone(), - role: k.1 .2, + id: k.1.0, + name: k.0.1.clone(), + role: k.1.2, }) .collect()) } @@ -5879,13 +6136,13 @@ impl Storage for Mutex { let mut hasher = sha::Sha256::new(); hasher.update(key.as_bytes()); let hash = openssl::base64::encode_block(&hasher.finish()); - if s.api_keys.iter().any(|k| k.1 .0 == ApiKeyId(id)) { + if s.api_keys.iter().any(|k| k.1.0 == ApiKeyId(id)) { return Err(DBError::unique_key_violation("api_key_pkey")); } if s.api_keys.contains_key(&(tenant_id, name.to_string())) { return Err(DBError::DuplicateName); } - if s.api_keys.iter().any(|k| k.1 .1 == hash) { + if s.api_keys.iter().any(|k| k.1.1 == hash) { return Err(DBError::duplicate_key()); } s.api_keys.insert( @@ -5903,8 +6160,8 @@ impl Storage for Mutex { let record: Vec<(TenantId, Role)> = s .api_keys .iter() - .filter(|k| k.1 .1 == hash) - .map(|k| (k.0 .0, k.1 .2)) + .filter(|k| k.1.1 == hash) + .map(|k| (k.0.0, k.1.2)) .collect(); assert!(record.len() <= 1); match record.first() { @@ -6023,10 +6280,10 @@ impl Storage for Mutex { if descr.issuer != issuer || !claim_matches(&descr.subject, subject) { continue; } - if let Some(pattern) = &descr.audience { - if !audiences.iter().any(|a| claim_matches(pattern, a)) { - continue; - } + if let Some(pattern) = &descr.audience + && !audiences.iter().any(|a| claim_matches(pattern, a)) + { + continue; } match matched.iter_mut().find(|(t, _)| t == scope) { Some(entry) => entry.1 = entry.1.max(descr.role), @@ -7641,6 +7898,25 @@ impl Storage for Mutex { Ok(Some((chosen.0, chosen.1))) } + async fn count_pipelines_needing_compilation(&self) -> Result { + Ok(self + .lock() + .await + .pipelines + .values() + .filter(|p| { + p.deployment_resources_status == ResourcesStatus::Stopped + && matches!( + p.program_status, + ProgramStatus::Pending + | ProgramStatus::CompilingSql + | ProgramStatus::SqlCompiled + | ProgramStatus::CompilingRust + ) + }) + .count() as u64) + } + async fn list_pipeline_programs_across_all_tenants( &self, ) -> Result< @@ -8015,7 +8291,7 @@ impl Storage for Mutex { ) -> Result { let mut state = self.lock().await; let mut num_deleted: usize = 0; - let keys: Vec<(TenantId, PipelineId)> = state.pipelines.keys().map(|v| v.clone()).collect(); + let keys: Vec<(TenantId, PipelineId)> = state.pipelines.keys().copied().collect(); for (tenant_id, pipeline_id) in keys { let events = state .pipeline_events diff --git a/crates/pipeline-manager/src/db/types/combined_status.rs b/crates/pipeline-manager/src/db/types/combined_status.rs index 766bfc4cbd4..96a45c245cd 100644 --- a/crates/pipeline-manager/src/db/types/combined_status.rs +++ b/crates/pipeline-manager/src/db/types/combined_status.rs @@ -61,7 +61,9 @@ impl CombinedStatus { RuntimeStatus::Suspended => Self::Suspended, } } else { - error!("Generating combined status encountered unexpected scenario: resource status is Provisioned but runtime status is None -- falling back to Unavailable"); + error!( + "Generating combined status encountered unexpected scenario: resource status is Provisioned but runtime status is None -- falling back to Unavailable" + ); Self::Unavailable } } @@ -114,7 +116,9 @@ impl CombinedDesiredStatus { RuntimeDesiredStatus::Suspended => Self::Suspended, } } else { - error!("Generating combined desired status encountered unexpected scenario: resource desired status is Provisioned but initial and current runtime desired status is None -- falling back to Unavailable"); + error!( + "Generating combined desired status encountered unexpected scenario: resource desired status is Provisioned but initial and current runtime desired status is None -- falling back to Unavailable" + ); Self::Unavailable } } diff --git a/crates/pipeline-manager/src/db/types/program.rs b/crates/pipeline-manager/src/db/types/program.rs index b04ac8c1ed5..1ddf0c924a1 100644 --- a/crates/pipeline-manager/src/db/types/program.rs +++ b/crates/pipeline-manager/src/db/types/program.rs @@ -859,7 +859,7 @@ pub fn generate_pipeline_config( #[cfg(test)] mod tests { - use super::{determine_connector_endpoint_names, RuntimeSelector}; + use super::{RuntimeSelector, determine_connector_endpoint_names}; use crate::db::types::program::ConnectorGenerationError::RelationConnectorNameCollision; use feldera_types::config::{ConnectorConfig, TransportConfig}; use feldera_types::program_schema::{PropertyValue, SourcePosition}; diff --git a/crates/pipeline-manager/src/db/types/utils.rs b/crates/pipeline-manager/src/db/types/utils.rs index e6387ac3d31..41965d87587 100644 --- a/crates/pipeline-manager/src/db/types/utils.rs +++ b/crates/pipeline-manager/src/db/types/utils.rs @@ -16,8 +16,7 @@ use tracing::error; pub(crate) const PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN: &str = r"^[a-zA-Z0-9_-]+$"; /// Description of the non-empty alphanumeric-underscore-hyphen pattern. -pub(crate) const PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN_DESCRIPTION: &str = - "be non-empty and only \ +pub(crate) const PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN_DESCRIPTION: &str = "be non-empty and only \ contain lowercase (a-z), uppercase (A-Z), number (0-9), underscore (_) or hyphen (-) characters"; /// The pattern is almost the same as for Kubernetes label values but slightly stricter. @@ -202,14 +201,18 @@ pub(crate) fn validate_runtime_config( if runtime_config.fault_tolerance.is_enabled() { let e = ValidationError::EnterpriseFeature("fault tolerance".to_string()); if log_if_invalid { - error!("Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid runtime configuration due to: {e}"); + error!( + "Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid runtime configuration due to: {e}" + ); } return Err(e); } if let Err(e) = validate_pipeline_env(&runtime_config.env) { let e = ValidationError::InvalidPipelineEnv(e); if log_if_invalid { - error!("Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid runtime configuration due to: {e}"); + error!( + "Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid runtime configuration due to: {e}" + ); } return Err(e); } @@ -217,7 +220,9 @@ pub(crate) fn validate_runtime_config( } Err(e) => { if log_if_invalid { - error!("Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid runtime configuration due to: {e}"); + error!( + "Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid runtime configuration due to: {e}" + ); } Err(e) } @@ -232,10 +237,12 @@ pub(crate) fn validate_program_config( ) -> Result { let deserialize_result = serde_json::from_value(value.clone()) .map_err(|e| ValidationError::DeserializationFailed(e.to_string())); - if let Err(e) = &deserialize_result { - if log_if_invalid { - error!("Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid program configuration due to: {e}"); - } + if let Err(e) = &deserialize_result + && log_if_invalid + { + error!( + "Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid program configuration due to: {e}" + ); } deserialize_result } @@ -247,7 +254,9 @@ pub(crate) fn validate_program_info( let deserialize_result = serde_json::from_value(value.clone()) .map_err(|e| ValidationError::DeserializationFailed(e.to_string())); if let Err(e) = &deserialize_result { - error!("Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid program information due to: {e}"); + error!( + "Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid program information due to: {e}" + ); } deserialize_result } @@ -262,14 +271,18 @@ pub(crate) fn validate_deployment_config( Ok(deployment_config) => { if let Err(e) = validate_pipeline_env(&deployment_config.global.env) { let e = ValidationError::InvalidPipelineEnv(e); - error!("Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid deployment configuration due to: {e}"); + error!( + "Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid deployment configuration due to: {e}" + ); Err(e) } else { Ok(deployment_config) } } Err(e) => { - error!("Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid deployment configuration due to: {e}"); + error!( + "Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid deployment configuration due to: {e}" + ); Err(e) } } @@ -282,7 +295,9 @@ pub(crate) fn validate_storage_status_details( let deserialize_result = serde_json::from_value(value.clone()) .map_err(|e| ValidationError::DeserializationFailed(e.to_string())); if let Err(e) = &deserialize_result { - error!("Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid storage status details due to: {e}"); + error!( + "Backward incompatibility detected: the following JSON:\n{value:#}\n\n... is no longer a valid storage status details due to: {e}" + ); } deserialize_result } @@ -290,14 +305,14 @@ pub(crate) fn validate_storage_status_details( #[cfg(test)] mod tests { use super::{ - validate_api_key_name, validate_connector_name, validate_deployment_config, - validate_description, validate_pipeline_name, validate_program_config, - validate_program_info, validate_runtime_config, validate_tags, ValidationError, MAXIMUM_API_KEY_NAME_LENGTH, MAXIMUM_CONNECTOR_NAME_LENGTH, MAXIMUM_DESCRIPTION_LENGTH, - MAXIMUM_PIPELINE_NAME_LENGTH, MAXIMUM_TAGS_PER_PIPELINE, MAXIMUM_TAG_LENGTH, + MAXIMUM_PIPELINE_NAME_LENGTH, MAXIMUM_TAG_LENGTH, MAXIMUM_TAGS_PER_PIPELINE, PATTERN_KUBERNETES_LABEL_VALUE, PATTERN_KUBERNETES_LABEL_VALUE_DESCRIPTION, PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN, - PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN_DESCRIPTION, + PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN_DESCRIPTION, ValidationError, + validate_api_key_name, validate_connector_name, validate_deployment_config, + validate_description, validate_pipeline_name, validate_program_config, + validate_program_info, validate_runtime_config, validate_tags, }; use crate::db::error::DBError; use crate::db::types::program::{CompilationProfile, ProgramConfig, ProgramInfo}; @@ -517,7 +532,7 @@ mod tests { // Too long. let too_long = "a".repeat(MAXIMUM_TAG_LENGTH + 1); assert!(matches!( - validate_tags(&[too_long.clone()]), + validate_tags(std::slice::from_ref(&too_long)), Err(DBError::InvalidTag { tag, .. }) if tag == too_long )); // Disallowed characters. diff --git a/crates/pipeline-manager/src/error.rs b/crates/pipeline-manager/src/error.rs index 01e365a2f3e..ce6ea61d903 100644 --- a/crates/pipeline-manager/src/error.rs +++ b/crates/pipeline-manager/src/error.rs @@ -27,7 +27,7 @@ use crate::compiler::error::CompilerError; use crate::db::error::DBError; use crate::runner::error::RunnerError; use actix_web::{ - body::BoxBody, http::StatusCode, HttpResponse, HttpResponseBuilder, ResponseError, + HttpResponse, HttpResponseBuilder, ResponseError, body::BoxBody, http::StatusCode, }; use feldera_types::error::{DetailedError, ErrorResponse}; use openssl::error::ErrorStack; diff --git a/crates/pipeline-manager/src/events_cleaner.rs b/crates/pipeline-manager/src/events_cleaner.rs index 6dcfbbfe334..76e123bd571 100644 --- a/crates/pipeline-manager/src/events_cleaner.rs +++ b/crates/pipeline-manager/src/events_cleaner.rs @@ -26,7 +26,9 @@ pub async fn events_cleaner(db: Arc>, common_config: Comm { Ok(num_deleted) => { if num_deleted > 0 { - debug!("Pipeline monitor events cleanup: deleted {num_deleted} events that exceeded retention limits"); + debug!( + "Pipeline monitor events cleanup: deleted {num_deleted} events that exceeded retention limits" + ); } } Err(e) => { diff --git a/crates/pipeline-manager/src/lib.rs b/crates/pipeline-manager/src/lib.rs index c8e3a826553..fcb2e2b2b8b 100644 --- a/crates/pipeline-manager/src/lib.rs +++ b/crates/pipeline-manager/src/lib.rs @@ -35,7 +35,9 @@ pub fn platform_enable_unstable(requested_features: &str) { if let Some(supported_feature) = all_features.get(requested_feature) { enabled.insert(*supported_feature); } else { - warn!("Requested unstable feature '{requested_feature}' is not supported by the platform."); + warn!( + "Requested unstable feature '{requested_feature}' is not supported by the platform." + ); } } UNSTABLE_FEATURES diff --git a/crates/pipeline-manager/src/license.rs b/crates/pipeline-manager/src/license.rs index 3db8fdc7bb3..68e9f760acc 100644 --- a/crates/pipeline-manager/src/license.rs +++ b/crates/pipeline-manager/src/license.rs @@ -43,10 +43,10 @@ impl LicenseCheck { } }; - if let Some(license_check) = &mut license_check { - if let LicenseValidity::Exists(license_info) = &mut license_check.check_outcome { - license_info.current += license_check.checked_at.elapsed(); - } + if let Some(license_check) = &mut license_check + && let LicenseValidity::Exists(license_info) = &mut license_check.check_outcome + { + license_info.current += license_check.checked_at.elapsed(); } Ok(license_check) diff --git a/crates/pipeline-manager/src/logging.rs b/crates/pipeline-manager/src/logging.rs index 52749e711a7..d0878a1692a 100644 --- a/crates/pipeline-manager/src/logging.rs +++ b/crates/pipeline-manager/src/logging.rs @@ -1,6 +1,6 @@ use colored::ColoredString; use feldera_observability::json_logging::{ - init_pipeline_logging, init_service_logging as init_service_logging_subscriber, ServiceName, + ServiceName, init_pipeline_logging, init_service_logging as init_service_logging_subscriber, }; use tracing::warn; use tracing_subscriber::EnvFilter; diff --git a/crates/pipeline-manager/src/oidc/fetch.rs b/crates/pipeline-manager/src/oidc/fetch.rs index 3f93629c3bc..1fc04b40ff7 100644 --- a/crates/pipeline-manager/src/oidc/fetch.rs +++ b/crates/pipeline-manager/src/oidc/fetch.rs @@ -4,8 +4,8 @@ //! verify the token's signature, so the fetch happens on behalf of whoever //! presented it. -use crate::auth::{parse_rsa_jwks, AuthError}; -use crate::oidc::destination::{is_public_ip, validate_tenant_oidc_url, TenantIssuerPolicy}; +use crate::auth::{AuthError, parse_rsa_jwks}; +use crate::oidc::destination::{TenantIssuerPolicy, is_public_ip, validate_tenant_oidc_url}; use jsonwebtoken::DecodingKey; use serde::Deserialize; use serde_json::Value; diff --git a/crates/pipeline-manager/src/oidc/trust_name.rs b/crates/pipeline-manager/src/oidc/trust_name.rs index a1abfd5673d..249871c384d 100644 --- a/crates/pipeline-manager/src/oidc/trust_name.rs +++ b/crates/pipeline-manager/src/oidc/trust_name.rs @@ -1,7 +1,7 @@ use crate::db::error::DBError; use crate::db::types::utils::{ - validate_name, PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN, - PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN_DESCRIPTION, + PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN, + PATTERN_NON_EMPTY_ALPHANUMERIC_UNDERSCORE_HYPHEN_DESCRIPTION, validate_name, }; /// Longest permitted name for a trust relationship. diff --git a/crates/pipeline-manager/src/runner/error.rs b/crates/pipeline-manager/src/runner/error.rs index 3bbbe89fb76..45f82d6033c 100644 --- a/crates/pipeline-manager/src/runner/error.rs +++ b/crates/pipeline-manager/src/runner/error.rs @@ -1,7 +1,7 @@ use crate::db::types::resources_status::{ResourcesDesiredStatus, ResourcesStatus}; use crate::db::types::utils::ValidationError; use actix_web::{ - body::BoxBody, http::StatusCode, HttpResponse, HttpResponseBuilder, ResponseError, + HttpResponse, HttpResponseBuilder, ResponseError, body::BoxBody, http::StatusCode, }; use feldera_types::error::{DetailedError, ErrorResponse}; use indoc::writedoc; @@ -319,7 +319,10 @@ impl Display for RunnerError { write!(f, "Pipeline provision failed: {error}") } Self::RunnerCheckError { error } => { - write!(f, "Pipeline check failed: compute and/or storage resources encountered a fatal error.\n\n{error}") + write!( + f, + "Pipeline check failed: compute and/or storage resources encountered a fatal error.\n\n{error}" + ) } Self::RunnerStopError { error } => { write!(f, "Pipeline stop failed (will retry): {error}") @@ -337,10 +340,16 @@ impl Display for RunnerError { ) } Self::RunnerInteractionLogFollowRequestChannelFull => { - write!(f, "Log follow request channel is full -- this indicates that the runner logging is overwhelmed") + write!( + f, + "Log follow request channel is full -- this indicates that the runner logging is overwhelmed" + ) } Self::RunnerInteractionLogFollowRequestChannelClosed => { - write!(f, "Log follow request channel is closed -- this indicates that the runner crashed unexpectedly") + write!( + f, + "Log follow request channel is closed -- this indicates that the runner crashed unexpectedly" + ) } Self::PipelineInteractionNotDeployed { pipeline_name, diff --git a/crates/pipeline-manager/src/runner/interaction.rs b/crates/pipeline-manager/src/runner/interaction.rs index 483188b4aa4..c37b541c2bf 100644 --- a/crates/pipeline-manager/src/runner/interaction.rs +++ b/crates/pipeline-manager/src/runner/interaction.rs @@ -7,7 +7,7 @@ use crate::db::types::tenant::TenantId; use crate::error::ManagerError; use crate::runner::error::RunnerError; use actix_web::http::header::{self, HeaderValue}; -use actix_web::{http::Method, web::Payload, HttpRequest, HttpResponse, HttpResponseBuilder}; +use actix_web::{HttpRequest, HttpResponse, HttpResponseBuilder, http::Method, web::Payload}; use actix_ws::{CloseCode, CloseReason}; use awc::error::{ConnectError, SendRequestError}; use awc::{ClientRequest, ClientResponse}; @@ -866,7 +866,7 @@ mod tests { /// connection), while a client offering none gets no header back. #[actix_web::test] async fn ws_handshake_echoes_offered_subprotocol() { - use actix_web::{web, App}; + use actix_web::{App, web}; setup(); // awc's TLS connector needs a rustls CryptoProvider installed. // Mirrors the production handshake: complete the upgrade, then echo. diff --git a/crates/pipeline-manager/src/runner/local_runner.rs b/crates/pipeline-manager/src/runner/local_runner.rs index 949deef9951..af7c58d5742 100644 --- a/crates/pipeline-manager/src/runner/local_runner.rs +++ b/crates/pipeline-manager/src/runner/local_runner.rs @@ -4,17 +4,17 @@ use crate::common_error::CommonError; use crate::config::{CommonConfig, LocalRunnerConfig}; use crate::db::types::pipeline::{ - bootstrap_policy_to_string, runtime_desired_status_to_string, PipelineId, + PipelineId, bootstrap_policy_to_string, runtime_desired_status_to_string, }; use crate::db::types::version::Version; -use crate::error::{source_error, ManagerError}; +use crate::error::{ManagerError, source_error}; use crate::pipeline_env::validate_pipeline_env; use crate::runner::error::RunnerError; use crate::runner::pipeline_executor::{PipelineExecutor, ProvisionStatus}; use crate::runner::pipeline_logs::{LogMessage, LogsSender}; use async_trait::async_trait; -use feldera_observability::system::total_memory_megabyte; use feldera_observability::ReqwestTracingExt; +use feldera_observability::system::total_memory_megabyte; use feldera_types::config::{ PipelineConfig, PipelineConfigProgramInfo, RuntimeConfig, StorageCacheConfig, StorageConfig, }; @@ -37,7 +37,7 @@ use tokio::task::JoinHandle; use tokio::time::{sleep, timeout}; use tokio::{fs, fs::create_dir_all, select, spawn}; use tokio_stream::StreamExt; -use tracing::{error, info, warn, Level}; +use tracing::{Level, error, info, warn}; use uuid::Uuid; /// How many times to attempt to retrieve the pipeline binary. @@ -1226,7 +1226,7 @@ impl PipelineExecutor for LocalRunner { return Err(RunnerError::RunnerProvisionError { error: format!("unable to spawn process due to: {e}"), } - .into()) + .into()); } } }; @@ -1413,14 +1413,14 @@ fn per_host_share(available_mb: Option, n_hosts: usize) -> Option { /// Sets `global.resources.memory_mb_max` to `available_mb` when the pipeline /// has no memory budget. fn apply_default_memory_limit(global: &mut RuntimeConfig, available_mb: Option) { - if global.effective_memory_mb().is_none() { - if let Some(available_mb) = available_mb { - info!( - "pipeline has no memory limit ('max_rss_mb' or 'resources.memory_mb_max'): \ + if global.effective_memory_mb().is_none() + && let Some(available_mb) = available_mb + { + info!( + "pipeline has no memory limit ('max_rss_mb' or 'resources.memory_mb_max'): \ defaulting 'resources.memory_mb_max' to {available_mb} MB" - ); - global.resources.memory_mb_max = Some(available_mb); - } + ); + global.resources.memory_mb_max = Some(available_mb); } } @@ -1441,8 +1441,8 @@ mod memory_limit_tests { #[cfg(test)] mod multihost_tests { use super::{ - multihost_coordinator_ip, multihost_host_ip, multihost_host_template, MAX_MULTIHOST_HOSTS, - MULTIHOST_LOOPBACK_OCTET, + MAX_MULTIHOST_HOSTS, MULTIHOST_LOOPBACK_OCTET, multihost_coordinator_ip, multihost_host_ip, + multihost_host_template, }; use std::net::Ipv4Addr; diff --git a/crates/pipeline-manager/src/runner/main.rs b/crates/pipeline-manager/src/runner/main.rs index 270ac4d8cb6..a3122ee4eab 100644 --- a/crates/pipeline-manager/src/runner/main.rs +++ b/crates/pipeline-manager/src/runner/main.rs @@ -13,7 +13,7 @@ use crate::runner::pipeline_executor::PipelineExecutor; use crate::runner::pipeline_logs::{LogMessage, LogsSender}; use actix_web::HttpResponse; use actix_web::Responder; -use actix_web::{get, web, HttpRequest, HttpServer}; +use actix_web::{HttpRequest, HttpServer, get, web}; use async_stream::try_stream; use std::collections::BTreeMap; use std::net::TcpListener; @@ -22,7 +22,7 @@ use std::sync::Arc; use std::time::Duration; use tokio::spawn; use tokio::sync::mpsc::error::TrySendError; -use tokio::sync::mpsc::{channel, Receiver, Sender}; +use tokio::sync::mpsc::{Receiver, Sender, channel}; use tokio::sync::{Mutex, Notify}; use tokio::task::JoinHandle; use tokio::time::timeout; @@ -107,22 +107,24 @@ async fn get_logs( .append_header(("X-Content-Type-Options", "nosniff")) .streaming(logs_stream(receiver).await)) } - Err(e) => { - match e { - TrySendError::Full(_) => { - error!("Unable to follow pipeline logs because the request channel is full"); - Err(ManagerError::from( - RunnerError::RunnerInteractionLogFollowRequestChannelFull, - )) - } - TrySendError::Closed(_) => { - error!("Unable to follow pipeline logs because the request channel is closed"); - Err(ManagerError::from( - RunnerError::RunnerInteractionLogFollowRequestChannelClosed, - )) - } + Err(e) => match e { + TrySendError::Full(_) => { + error!( + "Unable to follow pipeline logs because the request channel is full" + ); + Err(ManagerError::from( + RunnerError::RunnerInteractionLogFollowRequestChannelFull, + )) } - } + TrySendError::Closed(_) => { + error!( + "Unable to follow pipeline logs because the request channel is closed" + ); + Err(ManagerError::from( + RunnerError::RunnerInteractionLogFollowRequestChannelClosed, + )) + } + }, } } } @@ -249,7 +251,9 @@ async fn reconcile( } } None => { - error!("Runner main: listen notifier sending side has disconnected -- no longer able to send notifications"); + error!( + "Runner main: listen notifier sending side has disconnected -- no longer able to send notifications" + ); break; } } @@ -269,7 +273,9 @@ async fn reconcile( match db.lock().await.list_pipeline_ids_across_all_tenants().await { Ok(pipeline_ids) => { if db_error_previously { - info!("Runner main: again able to retrieve pipeline identifiers from the database. Any new pipelines will be retroactively detected."); + info!( + "Runner main: again able to retrieve pipeline identifiers from the database. Any new pipelines will be retroactively detected." + ); db_error_previously = false; } for (tenant_id, pipeline_id) in pipeline_ids { @@ -313,7 +319,9 @@ async fn reconcile( } } Err(e) => { - error!("Runner main: unable to retrieve pipeline identifiers from the database. Any new pipelines are not detected until again able to. Error: {e}"); + error!( + "Runner main: unable to retrieve pipeline identifiers from the database. Any new pipelines are not detected until again able to. Error: {e}" + ); db_error_previously = true; } } diff --git a/crates/pipeline-manager/src/runner/pipeline_automata.rs b/crates/pipeline-manager/src/runner/pipeline_automata.rs index e9e48e47a54..f72106b8dd0 100644 --- a/crates/pipeline-manager/src/runner/pipeline_automata.rs +++ b/crates/pipeline-manager/src/runner/pipeline_automata.rs @@ -3,10 +3,10 @@ use crate::db::error::DBError; use crate::db::storage::{ExtendedPipelineDescrRunner, Storage}; use crate::db::storage_postgres::StoragePostgres; use crate::db::types::pipeline::{ - runtime_desired_status_to_string, runtime_status_to_string, ExtendedPipelineDescr, - ExtendedPipelineDescrMonitoring, PipelineId, + ExtendedPipelineDescr, ExtendedPipelineDescrMonitoring, PipelineId, + runtime_desired_status_to_string, runtime_status_to_string, }; -use crate::db::types::program::{generate_pipeline_config, ProgramStatus}; +use crate::db::types::program::{ProgramStatus, generate_pipeline_config}; use crate::db::types::resources_status::{ResourcesDesiredStatus, ResourcesStatus}; use crate::db::types::storage::StorageStatus; use crate::db::types::tenant::TenantId; @@ -18,7 +18,7 @@ use crate::is_supported_runtime; use crate::runner::error::RunnerError; use crate::runner::interaction::{format_pipeline_url, format_timeout_error_message}; use crate::runner::pipeline_executor::{PipelineExecutor, ProvisionStatus}; -use crate::runner::pipeline_logs::{start_thread_pipeline_logs, LogMessage, LogsSender}; +use crate::runner::pipeline_logs::{LogMessage, LogsSender, start_thread_pipeline_logs}; use chrono::Utc; use feldera_observability::ReqwestTracingExt; use feldera_types::error::ErrorResponse; @@ -36,7 +36,7 @@ use tokio::task::JoinHandle; use tokio::time::Instant; use tokio::{sync::Mutex, time::Duration}; use tokio::{sync::Notify, time::timeout}; -use tracing::{debug, error, info, warn, Level}; +use tracing::{Level, debug, error, info, warn}; use uuid::Uuid; /// Every cycle, the automaton decides one of these actions to undertake. @@ -851,7 +851,10 @@ impl PipelineAutomaton { match serde_json::to_value(details) { Ok(storage_status_details) => Some(storage_status_details), Err(e) => { - error!("Automaton of pipeline {} is unable to serialize storage status details due to: {e}", self.pipeline_id); + error!( + "Automaton of pipeline {} is unable to serialize storage status details due to: {e}", + self.pipeline_id + ); None } } @@ -871,7 +874,10 @@ impl PipelineAutomaton { ProgramStatus::Success if !is_supported_runtime(&self.platform_version, &pipeline.platform_version) => { - info!("Runner cannot start pipeline {} because its runtime version ({}) is incompatible with current ({})", pipeline.id, pipeline.platform_version, self.platform_version); + info!( + "Runner cannot start pipeline {} because its runtime version ({}) is incompatible with current ({})", + pipeline.id, pipeline.platform_version, self.platform_version + ); Ok(Action::RemainStoppedUpdateError { error: ErrorResponse::from_error_nolog( @@ -1207,7 +1213,7 @@ impl PipelineAutomaton { return Action::TransitionToStopping { error: Some(RunnerError::AutomatonMissingDeploymentInitial.into()), storage_status_details: None, - } + }; } Some(deployment_initial) => *deployment_initial, }; @@ -1218,7 +1224,7 @@ impl PipelineAutomaton { return Action::TransitionToStopping { error: Some(RunnerError::AutomatonMissingDeploymentId.into()), storage_status_details: None, - } + }; } Some(deployment_id) => *deployment_id, }; @@ -1229,7 +1235,7 @@ impl PipelineAutomaton { return Action::TransitionToStopping { error: Some(RunnerError::AutomatonMissingDeploymentConfig.into()), storage_status_details: None, - } + }; } Some(deployment_config) => match validate_deployment_config(deployment_config) { Ok(deployment_config) => deployment_config, @@ -1407,7 +1413,7 @@ impl PipelineAutomaton { return Action::TransitionToStopping { error: Some(RunnerError::AutomatonMissingDeploymentInitial.into()), storage_status_details: None, - } + }; } Some(deployment_initial) => *deployment_initial, }; @@ -1658,7 +1664,7 @@ impl PipelineAutomaton { return Action::TransitionToStopping { error: Some(e.into()), storage_status_details: None, - } + }; } }; @@ -1669,7 +1675,7 @@ impl PipelineAutomaton { return Action::TransitionToStopping { error: Some(RunnerError::AutomatonMissingDeploymentLocation.into()), storage_status_details: None, - } + }; } }; @@ -1680,7 +1686,7 @@ impl PipelineAutomaton { return Action::TransitionToStopping { error: Some(RunnerError::AutomatonMissingDeploymentInitial.into()), storage_status_details: None, - } + }; } }; @@ -1700,7 +1706,7 @@ impl PipelineAutomaton { return Action::TransitionToStopping { error: Some(e), storage_status_details: None, - } + }; } }; @@ -1876,11 +1882,11 @@ mod test { use std::str::FromStr; use std::sync::Arc; use std::time::Duration; - use tokio::sync::mpsc::{channel, Sender}; + use tokio::sync::mpsc::{Sender, channel}; use tokio::sync::{Mutex, Notify}; use uuid::Uuid; use wiremock::matchers::{method, path}; - use wiremock::{http, Mock, MockServer, ResponseTemplate}; + use wiremock::{Mock, MockServer, ResponseTemplate, http}; struct MockRunner { deployment_location: String, diff --git a/crates/pipeline-manager/src/runner/pipeline_logs.rs b/crates/pipeline-manager/src/runner/pipeline_logs.rs index 876eabc45ec..b5c943b564f 100644 --- a/crates/pipeline-manager/src/runner/pipeline_logs.rs +++ b/crates/pipeline-manager/src/runner/pipeline_logs.rs @@ -8,7 +8,7 @@ use tokio::sync::{mpsc, oneshot}; use tokio::task::JoinHandle; use tokio::time::interval; use tokio::{select, spawn}; -use tracing::{debug, error, warn, Level}; +use tracing::{Level, debug, error, warn}; // Logs buffer size limit constants. const LOGS_BUFFER_LIMIT_BYTE: usize = 1_000_000; // 1 MB @@ -171,7 +171,10 @@ async fn catch_up_and_add_follower( // First line mentions the number of discarded lines due to the circular buffer if logs.num_discarded_lines() > 0 { - let first_line = format!("... {} prior log lines were discarded due to buffer constraints and are thus not shown.", logs.num_discarded_lines()); + let first_line = format!( + "... {} prior log lines were discarded due to buffer constraints and are thus not shown.", + logs.num_discarded_lines() + ); // Tag as control-plane metadata so the notice is formatted (text or JSON) consistently with other runner messages. let formatted_notice = format_log_line(&LogMessage::new_from_control_plane( module_path!(), @@ -184,7 +187,9 @@ async fn catch_up_and_add_follower( if let Err(e) = new_follower.try_send(formatted_notice) { match e { TrySendError::Full(_) => { - error!("Unable to catch up new follower because buffer is full, the follower will be dropped"); + error!( + "Unable to catch up new follower because buffer is full, the follower will be dropped" + ); } TrySendError::Closed(_) => {} } @@ -198,7 +203,9 @@ async fn catch_up_and_add_follower( if let Err(e) = new_follower.try_send(line.clone()) { match e { TrySendError::Full(_) => { - error!("Unable to catch up new follower because buffer is full, the follower will be dropped") + error!( + "Unable to catch up new follower because buffer is full, the follower will be dropped" + ) } TrySendError::Closed(_) => {} } @@ -240,7 +247,9 @@ async fn process_log_line_with_followers( // There exists a buffer to give a follower the chance to catch up. // However, if the limit of the buffer is reached and thus unable to send new, // the log follower will be removed to prevent it from slowing down the rest. - error!("Unable to send log line to follower because buffer is full: the follower will be removed") + error!( + "Unable to send log line to follower because buffer is full: the follower will be removed" + ) } TrySendError::Closed(_) => {} }, @@ -344,12 +353,16 @@ impl LogsSender { SendTimeoutError::Timeout(unsent_message) => { warn!( "Unable to send logs message because receiver buffer is full -- trying again in {}ms (attempt {} / {})", - SEND_LOG_MESSAGE_TIMEOUT.as_millis(), i, SEND_LOG_MESSAGE_TRIES + SEND_LOG_MESSAGE_TIMEOUT.as_millis(), + i, + SEND_LOG_MESSAGE_TRIES ); unsent_message } SendTimeoutError::Closed(_) => { - debug!("Unable to send logs message because receiver is closed -- this can happen when the pipeline is deleted"); + debug!( + "Unable to send logs message because receiver is closed -- this can happen when the pipeline is deleted" + ); return; } }, diff --git a/crates/pipeline-manager/tests/logging_demo.rs b/crates/pipeline-manager/tests/logging_demo.rs index 4b6bd0bf3df..a59827e9b94 100644 --- a/crates/pipeline-manager/tests/logging_demo.rs +++ b/crates/pipeline-manager/tests/logging_demo.rs @@ -7,8 +7,8 @@ use tracing::info; #[test] fn emits_sample_log() { // When invoked as a subprocess, just emit the logs and exit so the parent can assert on output. - if let Ok(mode) = std::env::var("LOGGING_DEMO_CHILD") { - emit_logs(mode == "json"); + if std::env::var("LOGGING_DEMO_CHILD").is_ok() { + emit_logs(); return; } @@ -82,15 +82,9 @@ fn run_child(mode: &str) -> String { String::from_utf8(output.stdout).expect("child stdout to be utf-8") } -fn emit_logs(json: bool) { - if json { - std::env::set_var("FELDERA_LOG_JSON", "1"); - } else { - std::env::remove_var("FELDERA_LOG_JSON"); - } - // Force INFO output for the demo regardless of upstream defaults. - std::env::set_var("RUST_LOG", "info"); - std::env::set_var("NO_COLOR", "1"); +/// `run_child` passes the log format, `RUST_LOG` and `NO_COLOR` in the child's +/// environment, so this only has to initialize logging and emit. +fn emit_logs() { init_logging("[logging-demo]".cyan()); info!("logging demo event"); info!( diff --git a/docs.feldera.com/docs/get-started/enterprise/parallel-compilation.md b/docs.feldera.com/docs/get-started/enterprise/parallel-compilation.md index c1b09e6cdc6..c673434723b 100644 --- a/docs.feldera.com/docs/get-started/enterprise/parallel-compilation.md +++ b/docs.feldera.com/docs/get-started/enterprise/parallel-compilation.md @@ -17,7 +17,7 @@ Feldera deploys the compiler server as a Kubernetes StatefulSet with **N** repli To further accelerate builds, Feldera optionally supports [sccache](https://github.com/mozilla/sccache) with an S3-compatible backend. This allows workers to share compiled operator artifacts instead of rebuilding identical code. :::info -Autoscaling based on workload is not yet supported. You must set the number of compiler server replicas at install time or scale them manually later. +Workload-based autoscaling is available as an experimental feature; see [Autoscaling (experimental)](#autoscaling-experimental). Without it, you set the number of compiler server replicas at install time or scale them manually later. ::: --- @@ -131,6 +131,127 @@ parallelCompilation: ``` +--- + +## Autoscaling (experimental) + +Compiler autoscaling scales the compiler server StatefulSet between 0 and N replicas so that idle deployments stop paying for compiler nodes. The kubernetes-runner drives the scaling: + +- Every `pollIntervalSeconds` the runner checks whether there are outstanding compilation requests. +- When there are, the runner scales the StatefulSet to N. N is `parallelCompilation.replicas` when parallel compilation is enabled, otherwise 1. +- After `idleTimeoutSeconds` without pending compilation work, the runner scales the StatefulSet to zero. + +A small always-on artifact server Deployment (`-compiler-artifact-server`) owns the binary store and serves compiled pipeline binaries and SQL program validation. The `-compiler-server-0` service routes to it, so the api-server, the runner, and pipeline pods keep working while the compiler workers are scaled to zero. All compiler workers, including worker 0, upload their binaries to the artifact server. + +### Configuration Defaults + +```yaml +compilerAutoscaling: + # Experimental: scale the compiler server StatefulSet to zero when idle. + enabled: false + # Seconds without pending compilation work before scaling to zero. + idleTimeoutSeconds: 1800 + # Seconds between checks for pending compilation work. + pollIntervalSeconds: 10 + artifactServer: + httpWorkers: 2 + # Budget at least 200 to 300 MB per optimized program version. + pvcSize: 50Gi + # The artifact server also serves SQL program validation (a JVM); the CPU + # limit lets the JVM size its heap. + resources: + requests: + cpu: "1" + memory: 2000Mi + limits: + cpu: "1" + memory: 2000Mi + # Seed the artifact store once from the compiler-server-0 PVC of an + # existing installation. Keep false on fresh installs. + seedFromCompilerServer0: false +``` + +### Latency Expectations + +A compilation submitted while the compiler workers are scaled to zero waits for the full cold start: the autoscaler poll (up to `pollIntervalSeconds`), node provisioning if the cluster must add compiler nodes, image pull, and compiler server startup including precompiled dependency extraction. The pipeline shows `Pending` during this time; the cold start usually adds 1 to 3 minutes of latency to the compilation, more if node provisioning is needed. Compilations submitted while workers are already up behave exactly as without autoscaling. + +Starting or restarting a pipeline whose binary is already compiled does not wake the compiler workers; the artifact server serves the stored binary directly. + +### Enabling on an Existing Installation + +Toggling the feature changes the StatefulSet `podManagementPolicy`, an immutable field. A pre-upgrade hook (`compilerAutoscaling.kubectlImage`) handles that automatically: on the one upgrade that flips the policy it deletes the compiler StatefulSet, and the same upgrade recreates it with the new policy while the retained per-replica volumes re-attach. The hook renders only on such transition upgrades; steady-state upgrades and installations that never toggle the feature run no hook at all. The hook runs `kubectl` from the image configured in `compilerAutoscaling.kubectlImage`, which must be pullable from your cluster. For air-gapped installations, copy the image into your registry (for example `crane copy docker.io/rancher/kubectl:v1.33.13 registry.example.com/kubectl:v1.33.13`) and set `compilerAutoscaling.kubectlImage` to the mirrored reference. + +1. Upgrade to images that support autoscaling, with `compilerAutoscaling.enabled` still `false`. +2. Run the enabling upgrade with `compilerAutoscaling.enabled=true` and `compilerAutoscaling.artifactServer.seedFromCompilerServer0=true`. While the seed setting is true the chart omits the compiler StatefulSet and frees its ReadWriteOnce `compiler-storage--compiler-server-0` volume; the artifact server then copies the compiled binaries from that volume into the artifact store, so existing pipelines keep their binaries. +3. Wait until the artifact server pod is `Running` (the copy happens in its init container). +4. Run a follow-up upgrade with `seedFromCompilerServer0` back to `false`. This recreates the compiler StatefulSet and detaches the old volume from the artifact server. + +Setting `seedFromCompilerServer0=true` is optional: skipping it means a single upgrade with `compilerAutoscaling.enabled=true` suffices, and the existing binaries are recompiled instead of carried over. The artifact store starts empty, stopped pipelines recompile on their next start, and a running pipeline whose pod restarts cannot fetch its binary until its program is recompiled (stop and start it once). Stop running pipelines first if that is not acceptable. + +Fresh installations need no procedure: set `compilerAutoscaling.enabled=true` at install time. + +### Disabling + +Without preserving binaries compiled while autoscaling was enabled, disabling is a single upgrade; stopped pipelines recompile on their next start: + +1. Run `helm upgrade` with `compilerAutoscaling.enabled=false`. The pre-upgrade hook recreates the StatefulSet and helm restores the configured replica count. The artifact store PVC carries `helm.sh/resource-policy: keep`, so it survives disabling and is re-adopted if you re-enable later. +2. Verify with `kubectl get statefulset -compiler-server -n ` that the replica count matches your configuration. + +To preserve the binaries (required if pipelines are running and must survive pod restarts without a recompile), copy them back before the upgrade: + +1. Wait until the compiler workers are parked at zero replicas (or scale the StatefulSet to zero), then scale the artifact server down so both ReadWriteOnce volumes are free: + + ```bash + kubectl scale deployment -compiler-artifact-server -n --replicas=0 + ``` + +2. Run a one-shot pod that mounts the `-compiler-artifact-server` PVC (source) and the `compiler-storage--compiler-server-0` PVC (target) and copies the `rust-compilation/pipeline-binaries` directory from source to target. Both paths are relative to the volume root: + + ```yaml + apiVersion: v1 + kind: Pod + metadata: + name: binary-restore + spec: + restartPolicy: Never + containers: + - name: copy + image: busybox:1.37.0 + command: ["sh", "-c", "mkdir -p /target/rust-compilation/pipeline-binaries && cp -a /source/rust-compilation/pipeline-binaries/. /target/rust-compilation/pipeline-binaries/"] + volumeMounts: + - {name: source, mountPath: /source, readOnly: true} + - {name: target, mountPath: /target} + volumes: + - {name: source, persistentVolumeClaim: {claimName: -compiler-artifact-server}} + - {name: target, persistentVolumeClaim: {claimName: compiler-storage--compiler-server-0}} + ``` + + Wait for the pod to reach `Succeeded`, then delete it. +3. Run `helm upgrade` with `compilerAutoscaling.enabled=false` and verify the replica count as above. + + +### Autoscaling Troubleshooting + +- **`/cluster_healthz` reports `scaled_to_zero`:** + +While the compiler workers are parked at zero, the health endpoint reports the compiler section as healthy with a `"scaled_to_zero": true` marker. + +- **`/cluster_healthz` reports the compiler not ready during scale-up:** + +During a 0 to N cold start the compiler section reports not ready together with a note that autoscaling is active; this resolves once the compiler pods pass their startup probes. Only a not-ready state that persists well beyond the expected cold start indicates a real problem (for example unschedulable pods or exhausted quota). + +- **`SCALING DETECTED` restarts during transitions:** + +Compiler pods that observe a replica change exit with `SCALING DETECTED` and restart with the new worker count. During 0 to N and N to 0 transitions these restarts are expected and bounded; the pods converge as soon as the transition completes. + +- **`/cluster_healthz` reports the compiler unhealthy with a storage message:** + +The compiler health check fails once the binary store filesystem is 95% full, before uploads start failing with `No space left on device`. Grow the artifact server PVC (the storage class must support volume expansion) or delete unused pipelines so the garbage collector reclaims their binaries. Budget at least 200 to 300 MB per optimized program version when sizing `compilerAutoscaling.artifactServer.pvcSize`. + +- **Compilers never scale down:** + +A compilation that never reaches a terminal status keeps demand pending and keeps the workers up. The typical cause is a compile that is OOM-killed on every attempt: the pipeline cycles between `SqlCompiled` and `CompilingRust` forever. Give the compiler pods more memory or remove the offending pipeline. + --- ## Troubleshooting & FAQs @@ -143,11 +264,15 @@ Ensure your cluster nodes have enough resources to run the desired number of com If a pipeline is assigned to a worker pod that is not yet running or is unhealthy, it will not be compiled until that pod is available and running. Make sure to validate all pods are running. -- **SystemError: Failed to upload binary:** +- **Failed to upload binary:** + +Upload failures fall into three classes: -If the pipeline gets this status, that means pod N failed to upload its binary to `<_>-compiler-server-0`. + - Transient (network errors, HTTP 5xx): retried with exponential backoff inside the compilation attempt; the default retry settings absorb roughly half an hour of outage, for example a restart of the upload target during an upgrade. + - Permanent: surface immediately as `SystemError` with the underlying cause and skip the retry budget: an HTTP 4xx rejection (for example a proxy body-size limit), or HTTP 507 when the binary store volume is full or has remounted itself read-only after a storage failure (`Unable to write to the binary store`, see [Out-of-storage Errors](/operations/guide#out-of-storage-errors)). + - Retry budget exhausted: a retryable failure that outlives the retry budget also ends in `SystemError`. -You can check if `<_>-compiler-server-0` is healthy or not by `/cluster_healthz` endpoint. Make sure to adjust binary upload related configuration as per your needs, e.g. if your _upgrade_ takes a while, we should configure retries and backoff interval to sane values such that pods get time to come up to receive the binary. +After fixing the cause (for example growing the artifact store PVC), recompile affected pipelines by editing or re-saving their program. Check `-compiler-server-0` health via the `/cluster_healthz` endpoint. - **error: process didn't exit successfully: `sccache .. rustc -vV`:** @@ -179,4 +304,15 @@ Comman causes can be misconfigured S3 bucket / endpoint / credentials. | `parallelCompilation.sccache.s3.serverSideEncryption` | Enable server-side encryption with s3 managed key (SSE-S3) | `false` | | `parallelCompilation.sccache.s3.endpoint` | Custom endpoint (e.g. MinIO) | `minio.mydomain.com:9000` | +**Autoscaling (experimental)** +| Key | Description | Default/Example | +|-----|-------------|-----------------| +| `compilerAutoscaling.enabled` | Scale the compiler server StatefulSet to zero when idle | `false` (ex: `true`) | +| `compilerAutoscaling.idleTimeoutSeconds` | Seconds without pending compilation work before scaling to zero | `1800` | +| `compilerAutoscaling.pollIntervalSeconds` | Seconds between checks for pending compilation work | `10` | +| `compilerAutoscaling.artifactServer.httpWorkers` | HTTP worker threads of the artifact server | `2` | +| `compilerAutoscaling.artifactServer.pvcSize` | Size of the artifact store volume | `50Gi` | +| `compilerAutoscaling.artifactServer.resources` | Artifact server pod resources | `1` CPU, `2000Mi` memory | +| `compilerAutoscaling.artifactServer.seedFromCompilerServer0` | Seed the artifact store from the compiler-server-0 PVC during the enabling upgrade | `false` (ex: `true`) | + --- diff --git a/docs.feldera.com/docs/operations/guide.md b/docs.feldera.com/docs/operations/guide.md index 0b15740dce8..907156d96fe 100644 --- a/docs.feldera.com/docs/operations/guide.md +++ b/docs.feldera.com/docs/operations/guide.md @@ -119,6 +119,30 @@ explicitly request larger volumes for each pipeline: } ``` +#### Compiler binary store + +**Error**: a pipeline fails to compile with: +``` +Unable to write to the binary store: the storage volume is full; an operator +must grow it or delete unused pipelines to reclaim space +``` + +The compiler stores every compiled pipeline binary on its own volume, separate +from the pipeline storage above. Compilation fails immediately rather than +retrying, because only an operator can resolve it. + +**Solution**: grow the compiler volume or reclaim space. Budget at least 200 to +300 MB per compiled program version. Deleting unused pipelines lets the +compiler's garbage collector reclaim their binaries. The compiler health check +reports this condition through `/cluster_healthz` once the volume is 95% full, +before uploads start failing. + +If the message instead says the volume is read-only, the kernel remounted the +filesystem after a disk failure; repair or replace the underlying disk. + +After resolving the cause, recompile affected pipelines by editing or re-saving +their program. + ### Kubernetes evictions **Error**: the pipeline becomes `UNAVAILABLE` with no errors in the logs.