From b59092ce40c8b67b420b9ef264153697f2efeaf7 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 20 Nov 2023 23:05:30 -0500 Subject: [PATCH 01/47] Outline basic CRUD actions for quotas --- nexus/src/app/mod.rs | 1 + nexus/src/app/quota.rs | 68 ++++++++ nexus/src/external_api/http_entrypoints.rs | 176 +++++++++++++++++++++ 3 files changed, 245 insertions(+) create mode 100644 nexus/src/app/quota.rs diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index 18c9dae841..3a6f378ea2 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -51,6 +51,7 @@ mod metrics; mod network_interface; mod oximeter; mod project; +mod quota; mod rack; pub(crate) mod saga; mod session; diff --git a/nexus/src/app/quota.rs b/nexus/src/app/quota.rs new file mode 100644 index 0000000000..3845fc00de --- /dev/null +++ b/nexus/src/app/quota.rs @@ -0,0 +1,68 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Resource limits and system quotas + +use diesel::pg::TypeOidLookup; +use nexus_db_queries::context::OpContext; + +impl super::Nexus { + pub async fn quotas_list( + &self, + opctx: &OpContext, + pagparams: &PaginatedBy<'_>, + ) -> ListResultVec { + self.db_datastore.quota_list(opctx, pagparams).await + } + + pub(crate) async fn fleet_list_quotas( + &self, + opctx: &OpContext, + pagparams: &PaginatedBy<'_>, + ) -> ListResultVec { + self.db_datastore.fleet_list_quotas(opctx, pagparams).await + } + + pub(crate) async fn silo_fetch_quota( + &self, + opctx: &OpContext, + silo_lookup: &lookup::Silo<'_>, + ) -> Result { + let (.., authz_silo) = + silo_lookup.lookup_for(authz::Action::Read).await?; + self.db_datastore.silo_fetch_quota(opctx, authz_silo).await + } + + pub(crate) async fn silo_create_quota( + &self, + opctx: &OpContext, + silo_lookup: &lookup::Silo<'_>, + quota: &db::model::Quota, + ) -> Result { + let (.., authz_silo) = + silo_lookup.lookup_for(authz::Action::Modify).await?; + self.db_datastore.silo_create_quota(opctx, authz_silo, quota).await + } + + pub(crate) async fn silo_update_quota( + &self, + opctx: &OpContext, + silo_lookup: &lookup::Silo<'_>, + updates: ¶ms::QuotaUpdate, + ) -> Result { + let (.., authz_silo) = + silo_lookup.lookup_for(authz::Action::Modify).await?; + self.db_datastore.silo_update_quota(opctx, authz_silo, updates).await + } + + pub(crate) async fn silo_quota_delete( + &self, + opctx: &OpContext, + silo_lookup: &lookup::Silo<'_>, + ) -> Result { + let (.., authz_silo) = + silo_lookup.lookup_for(authz::Action::Delete).await?; + self.db_datastore.silo_quota_delete(opctx, authz_silo).await + } +} diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 428632bcf5..f3c3d67a31 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -265,6 +265,8 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(networking_bgp_announce_set_list)?; api.register(networking_bgp_announce_set_delete)?; + api.register(quota_list)?; + // Fleet-wide API operations api.register(silo_list)?; api.register(silo_create)?; @@ -273,6 +275,13 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(silo_policy_view)?; api.register(silo_policy_update)?; + api.register(system_quota_list)?; + + api.register(silo_quota_view)?; + api.register(silo_quota_create)?; + api.register(silo_quota_update)?; + api.register(silo_quota_delete)?; + api.register(silo_identity_provider_list)?; api.register(saml_identity_provider_create)?; @@ -503,6 +512,173 @@ async fn policy_update( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +#[endpoint { + method = GET, + path = "/v1/quotas", + tags = ["quotas"], +}] +async fn quota_list( + rqctx: RequestContext>, + query_params: Query, +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.nexus; + + let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query)?; + let scan_params = ScanByNameOrId::from_query(&query)?; + let paginated_by = name_or_id_pagination(&pag_params, scan_params)?; + + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + let quotas = nexus + .quotas_list(&opctx, &paginated_by) + .await? + .into_iter() + .map(|p| p.try_into()) + .collect::, Error>>()?; + + Ok(HttpResponseOk(ScanByNameOrId::results_page( + &query, + quotas, + &marker_for_name_or_id, + )?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +#[endpoint { + method = GET, + path = "/v1/system/quotas", + tags = ["system/quotas"], +}] +async fn system_quota_list( + rqctx: RequestContext>, + query_params: Query, +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.nexus; + + let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query)?; + let scan_params = ScanByNameOrId::from_query(&query)?; + let paginated_by = name_or_id_pagination(&pag_params, scan_params)?; + + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + let quotas = nexus + .fleet_list_quotas(&opctx, &paginated_by) + .await? + .into_iter() + .map(|p| p.try_into()) + .collect::, Error>>()?; + + Ok(HttpResponseOk(ScanByNameOrId::results_page( + &query, + quotas, + &marker_for_name_or_id, + )?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +#[endpoint { + method = GET, + path = "/v1/system/silos/{silo}/quotas", + tags = ["system/quotas"], +}] +async fn silo_quota_view( + rqctx: RequestContext>, + path_params: Path, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.nexus; + + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + let silo_lookup = + nexus.silo_lookup(&opctx, path_params.into_inner().silo)?; + let quota = + nexus.silo_fetch_quota(&opctx, &silo_lookup).await?.try_into()?; + Ok(HttpResponseOk(quota)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +#[endpoint { + method = POST, + path = "/v1/system/silos/{silo}/quotas", + tags = ["system/quotas"], +}] +async fn silo_quota_create( + rqctx: RequestContext>, + path_params: Path, + new_quota: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.nexus; + + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + let silo_lookup = + nexus.silo_lookup(&opctx, path_params.into_inner().silo)?; + let quota = nexus + .silo_create_quota(&opctx, &silo_lookup, new_quota.into_inner()) + .await? + .try_into()?; + Ok(HttpResponseCreated(quota)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +#[endpoint { + method = PUT, + path = "/v1/system/silos/{silo}/quotas", + tags = ["system/quotas"], +}] +async fn silo_quota_update( + rqctx: RequestContext>, + path_params: Path, + new_quota: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.nexus; + + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + let silo_lookup = + nexus.silo_lookup(&opctx, path_params.into_inner().silo)?; + let quota = nexus + .silo_update_quota(&opctx, &silo_lookup, new_quota.into_inner()) + .await? + .try_into()?; + Ok(HttpResponseOk(quota)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +#[endpoint { + method = DELETE, + path = "/v1/system/silos/{silo}/quotas", + tags = ["system/quotas"], +}] +async fn silo_quota_delete( + rqctx: RequestContext>, + path_params: Path, +) -> Result { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.nexus; + + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + let silo_lookup = + nexus.silo_lookup(&opctx, path_params.into_inner().silo)?; + nexus.silo_delete_quota(&opctx, &silo_lookup).await?; + Ok(HttpResponseDeleted()) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + /// List silos /// /// Lists silos that are discoverable based on the current permissions. From 159644c1577946d897f56f03a372dfb0a6575a80 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 21 Nov 2023 16:18:19 -0500 Subject: [PATCH 02/47] Only allow viewing and updating quotas In RFD-427 we specify that for the initial implementation we want to require that all silos have a quota. In considering the API and implementaiton more carefully I decided that at least temporarily we should drop create/delete given that deletions should never happen and creations should only happen when the silo is created (or via some internal process which creates quotas for pre-existing silos) --- nexus/src/external_api/http_entrypoints.rs | 50 ---------------------- 1 file changed, 50 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index f3c3d67a31..da9f343022 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -278,9 +278,7 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(system_quota_list)?; api.register(silo_quota_view)?; - api.register(silo_quota_create)?; api.register(silo_quota_update)?; - api.register(silo_quota_delete)?; api.register(silo_identity_provider_list)?; @@ -605,32 +603,6 @@ async fn silo_quota_view( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -#[endpoint { - method = POST, - path = "/v1/system/silos/{silo}/quotas", - tags = ["system/quotas"], -}] -async fn silo_quota_create( - rqctx: RequestContext>, - path_params: Path, - new_quota: TypedBody, -) -> Result, HttpError> { - let apictx = rqctx.context(); - let handler = async { - let nexus = &apictx.nexus; - - let opctx = crate::context::op_context_for_external_api(&rqctx).await?; - let silo_lookup = - nexus.silo_lookup(&opctx, path_params.into_inner().silo)?; - let quota = nexus - .silo_create_quota(&opctx, &silo_lookup, new_quota.into_inner()) - .await? - .try_into()?; - Ok(HttpResponseCreated(quota)) - }; - apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await -} - #[endpoint { method = PUT, path = "/v1/system/silos/{silo}/quotas", @@ -657,28 +629,6 @@ async fn silo_quota_update( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -#[endpoint { - method = DELETE, - path = "/v1/system/silos/{silo}/quotas", - tags = ["system/quotas"], -}] -async fn silo_quota_delete( - rqctx: RequestContext>, - path_params: Path, -) -> Result { - let apictx = rqctx.context(); - let handler = async { - let nexus = &apictx.nexus; - - let opctx = crate::context::op_context_for_external_api(&rqctx).await?; - let silo_lookup = - nexus.silo_lookup(&opctx, path_params.into_inner().silo)?; - nexus.silo_delete_quota(&opctx, &silo_lookup).await?; - Ok(HttpResponseDeleted()) - }; - apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await -} - /// List silos /// /// Lists silos that are discoverable based on the current permissions. From 21ba99cdf24ea5bae6a72497dd1e03951896a5e6 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 21 Nov 2023 19:52:57 -0500 Subject: [PATCH 03/47] Start outlining quota database type --- nexus/db-model/src/lib.rs | 4 +++ nexus/db-model/src/quota.rs | 29 +++++++++++++++++ nexus/db-model/src/quota_resource_kind.rs | 36 ++++++++++++++++++++++ nexus/db-queries/src/db/datastore/quota.rs | 19 ++++++++++++ 4 files changed, 88 insertions(+) create mode 100644 nexus/db-model/src/quota.rs create mode 100644 nexus/db-model/src/quota_resource_kind.rs create mode 100644 nexus/db-queries/src/db/datastore/quota.rs diff --git a/nexus/db-model/src/lib.rs b/nexus/db-model/src/lib.rs index 6b65eb87ec..1ec0aa016d 100644 --- a/nexus/db-model/src/lib.rs +++ b/nexus/db-model/src/lib.rs @@ -55,6 +55,8 @@ mod system_update; // for join-based marker trait generation. mod ipv4_nat_entry; pub mod queries; +mod quota; +mod quota_resource_kind; mod rack; mod region; mod region_snapshot; @@ -137,6 +139,8 @@ pub use physical_disk::*; pub use physical_disk_kind::*; pub use producer_endpoint::*; pub use project::*; +pub use quota::*; +pub use quota_resource_kind::*; pub use rack::*; pub use region::*; pub use region_snapshot::*; diff --git a/nexus/db-model/src/quota.rs b/nexus/db-model/src/quota.rs new file mode 100644 index 0000000000..9edd60c058 --- /dev/null +++ b/nexus/db-model/src/quota.rs @@ -0,0 +1,29 @@ +use db_macros::Asset; +use uuid::Uuid; + +#[derive(Queryable, Insertable, Debug, Clone, Selectable, Asset)] +#[diesel(table_name = quota)] +pub struct Quota { + #[diesel(embed)] + identity: QuotaIdentity, + + pub silo_id: Uuid, + + pub resource_type: QuotaResourceKind, + pub limit: i64, +} + +impl Quota { + pub fn new( + silo_id: Uuid, + resource_type: QuotaResourceKind, + limit: i64, + ) -> Self { + Self { + identity: QuotaIdentity::new(Uuid::new_v4()), + silo_id, + resource_type, + limit, + } + } +} diff --git a/nexus/db-model/src/quota_resource_kind.rs b/nexus/db-model/src/quota_resource_kind.rs new file mode 100644 index 0000000000..6c54aeaab6 --- /dev/null +++ b/nexus/db-model/src/quota_resource_kind.rs @@ -0,0 +1,36 @@ +use super::impl_enum_type; +use nexus_types::external_api::params; + +impl_enum_type!( + #[derive(Clone, SqlType, Debug, QueryId)] + #[diesel(postgres_type(name = "quota_resource_kind"))] + pub struct QuotaResourceTypeEnum; + + #[derive(Copy, Clone, Debug, AsExpression, FromSqlRow, PartialEq, Serialize, Deserialize)] + #[diesel(sql_type = QuotaResourceTypeEnum)] + pub enum QuotaResourceKind; + + Cpu => b"Cpu", + Memory => b"Memory", + Storage => b"Storage", +); + +impl From for QuotaResourceKind { + fn from(k: params::QuotaResourceKind) -> Self { + match k { + params::QuotaResourceKind::Cpu => QuotaResourceKind::Cpu, + params::QuotaResourceKind::Memory => QuotaResourceKind::Memory, + params::QuotaResourceKind::Storage => QuotaResourceKind::Storage, + } + } +} + +impl From for params::QuotaResourceKind { + fn from(value: QuotaResourceKind) -> Self { + match value { + QuotaResourceKind::Cpu => params::QuotaResourceKind::Cpu, + QuotaResourceKind::Memory => params::QuotaResourceKind::Memory, + QuotaResourceKind::Storage => params::QuotaResourceKind::Storage, + } + } +} diff --git a/nexus/db-queries/src/db/datastore/quota.rs b/nexus/db-queries/src/db/datastore/quota.rs new file mode 100644 index 0000000000..14084b1002 --- /dev/null +++ b/nexus/db-queries/src/db/datastore/quota.rs @@ -0,0 +1,19 @@ +impl DataStore { + pub async fn quota_list( + &self, + opctx: &OpContext, + pagparams: &PaginatedBy<'_>, + ) -> ListResultVec { + opctx + .authorize(authz::Action::ListChildren, authz::Resource::FLEET) + .await?; + let mut query = db::model::Quota::query(); + query = query.filter(db::schema::quotas::silo_id.eq(opctx.silo_id)); + query = query.order_by(db::schema::quotas::id.asc()); + query = pagparams.paginate(query); + query + .load_async::(&self.pool) + .await + .map_err(Error::from) + } +} From 0127dd1188d5b088e46620a289d773a602169079 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Sat, 2 Dec 2023 13:27:05 -0500 Subject: [PATCH 04/47] Simplify quota model to be flatter --- nexus/db-model/src/lib.rs | 2 - nexus/db-model/src/quota.rs | 34 +++++----- nexus/db-model/src/quota_resource_kind.rs | 36 ----------- nexus/db-model/src/schema.rs | 11 ++++ nexus/db-queries/src/db/datastore/mod.rs | 1 + nexus/db-queries/src/db/datastore/quota.rs | 74 +++++++++++++++++++++- nexus/src/app/quota.rs | 25 +++++--- nexus/src/external_api/http_entrypoints.rs | 6 +- nexus/types/src/external_api/params.rs | 7 ++ nexus/types/src/external_api/views.rs | 10 +++ schema/crdb/dbinit.sql | 9 +++ 11 files changed, 147 insertions(+), 68 deletions(-) delete mode 100644 nexus/db-model/src/quota_resource_kind.rs diff --git a/nexus/db-model/src/lib.rs b/nexus/db-model/src/lib.rs index 1ec0aa016d..74de8b4cfe 100644 --- a/nexus/db-model/src/lib.rs +++ b/nexus/db-model/src/lib.rs @@ -56,7 +56,6 @@ mod system_update; mod ipv4_nat_entry; pub mod queries; mod quota; -mod quota_resource_kind; mod rack; mod region; mod region_snapshot; @@ -140,7 +139,6 @@ pub use physical_disk_kind::*; pub use producer_endpoint::*; pub use project::*; pub use quota::*; -pub use quota_resource_kind::*; pub use rack::*; pub use region::*; pub use region_snapshot::*; diff --git a/nexus/db-model/src/quota.rs b/nexus/db-model/src/quota.rs index 9edd60c058..514f5aa5e2 100644 --- a/nexus/db-model/src/quota.rs +++ b/nexus/db-model/src/quota.rs @@ -1,29 +1,29 @@ -use db_macros::Asset; +use chrono::{DateTime, Utc}; use uuid::Uuid; -#[derive(Queryable, Insertable, Debug, Clone, Selectable, Asset)] -#[diesel(table_name = quota)] -pub struct Quota { - #[diesel(embed)] - identity: QuotaIdentity, +use crate::schema::silo_quotas; +#[derive(Queryable, Insertable, Debug, Clone, Selectable)] +#[diesel(table_name = silo_quotas)] +pub struct SiloQuotas { pub silo_id: Uuid, + pub time_created: DateTime, + pub time_modified: DateTime, - pub resource_type: QuotaResourceKind, - pub limit: i64, + pub cpus: i64, + pub memory: i64, + pub storage: i64, } -impl Quota { - pub fn new( - silo_id: Uuid, - resource_type: QuotaResourceKind, - limit: i64, - ) -> Self { +impl SiloQuotas { + pub fn new(silo_id: Uuid, cpus: i64, memory: i64, storage: i64) -> Self { Self { - identity: QuotaIdentity::new(Uuid::new_v4()), silo_id, - resource_type, - limit, + time_created: Utc::now(), + time_modified: Utc::now(), + cpus, + memory, + storage, } } } diff --git a/nexus/db-model/src/quota_resource_kind.rs b/nexus/db-model/src/quota_resource_kind.rs deleted file mode 100644 index 6c54aeaab6..0000000000 --- a/nexus/db-model/src/quota_resource_kind.rs +++ /dev/null @@ -1,36 +0,0 @@ -use super::impl_enum_type; -use nexus_types::external_api::params; - -impl_enum_type!( - #[derive(Clone, SqlType, Debug, QueryId)] - #[diesel(postgres_type(name = "quota_resource_kind"))] - pub struct QuotaResourceTypeEnum; - - #[derive(Copy, Clone, Debug, AsExpression, FromSqlRow, PartialEq, Serialize, Deserialize)] - #[diesel(sql_type = QuotaResourceTypeEnum)] - pub enum QuotaResourceKind; - - Cpu => b"Cpu", - Memory => b"Memory", - Storage => b"Storage", -); - -impl From for QuotaResourceKind { - fn from(k: params::QuotaResourceKind) -> Self { - match k { - params::QuotaResourceKind::Cpu => QuotaResourceKind::Cpu, - params::QuotaResourceKind::Memory => QuotaResourceKind::Memory, - params::QuotaResourceKind::Storage => QuotaResourceKind::Storage, - } - } -} - -impl From for params::QuotaResourceKind { - fn from(value: QuotaResourceKind) -> Self { - match value { - QuotaResourceKind::Cpu => params::QuotaResourceKind::Cpu, - QuotaResourceKind::Memory => params::QuotaResourceKind::Memory, - QuotaResourceKind::Storage => params::QuotaResourceKind::Storage, - } - } -} diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index e7d625e854..878289bc69 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -408,6 +408,17 @@ table! { } } +table! { + silo_quotas(silo_id) { + silo_id -> Uuid, + time_created -> Timestamptz, + time_modified -> Timestamptz, + cpus -> Int8, + memory -> Int8, + storage -> Int8, + } +} + table! { network_interface (id) { id -> Uuid, diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 0612b960c9..5a68b159e4 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -68,6 +68,7 @@ mod network_interface; mod oximeter; mod physical_disk; mod project; +mod quota; mod rack; mod region; mod region_snapshot; diff --git a/nexus/db-queries/src/db/datastore/quota.rs b/nexus/db-queries/src/db/datastore/quota.rs index 14084b1002..b955c5636e 100644 --- a/nexus/db-queries/src/db/datastore/quota.rs +++ b/nexus/db-queries/src/db/datastore/quota.rs @@ -1,5 +1,77 @@ +use super::DataStore; +use crate::authz; +use crate::context::OpContext; +use crate::db; +use crate::db::collection_insert::AsyncInsertError; +use crate::db::error::public_error_from_diesel; +use crate::db::error::ErrorHandler; +use chrono::Utc; +use diesel::upsert::excluded; +use nexus_db_model::SiloQuotas; +use omicron_common::api::external::CreateResult; +use omicron_common::api::external::Error; +use omicron_common::api::external::LookupType; +use omicron_common::api::external::ResourceType; + impl DataStore { - pub async fn quota_list( + pub async fn silo_quotas_upsert( + &self, + opctx: &OpContext, + authz_silo: &authz::Silo, + quotas: SiloQuotas, + ) -> CreateResult { + opctx.authorize(authz::Action::Modify, authz_silo).await?; + let silo_id = authz_silo.id(); + use db::schema::silo_quotas::dsl; + + let now = Utc::now(); + let quota_db = SiloQuotas::insert_resource( + silo_id, + diesel::insert_into(dsl::silo_quotas) + .values(quotas.clone()) + .on_conflict(dsl::silo_id) + .do_update() + .set(( + dsl::time_created.eq(excluded(dsl::time_created)), + dsl::time_modified.eq(now), + )), + ) + .insert_and_get_result_async( + &*self.pool_connection_authorized(&opctx).await?, + ) + .await + .map_err(|e| match e { + AsyncInsertError::CollectionNotFound => Error::ObjectNotFound { + type_name: ResourceType::SiloQuotas, + lookup_type: LookupType::ById(silo_id), + }, + AsyncInsertError::DatabaseError(e) => { + public_error_from_diesel(e, ErrorHandler::Server) + } + })?; + + Ok(quota_db) + } + + pub async fn quotas_list( + &self, + opctx: &OpContext, + pagparams: &PaginatedBy<'_>, + ) -> ListResultVec { + opctx + .authorize(authz::Action::ListChildren, authz::Resource::FLEET) + .await?; + let mut query = db::model::Quota::query(); + query = query.filter(db::schema::quotas::silo_id.eq(opctx.silo_id)); + query = query.order_by(db::schema::quotas::id.asc()); + query = pagparams.paginate(query); + query + .load_async::(&self.pool) + .await + .map_err(Error::from) + } + + pub async fn fleet_list_quotas( &self, opctx: &OpContext, pagparams: &PaginatedBy<'_>, diff --git a/nexus/src/app/quota.rs b/nexus/src/app/quota.rs index 3845fc00de..4a52abce6e 100644 --- a/nexus/src/app/quota.rs +++ b/nexus/src/app/quota.rs @@ -4,23 +4,30 @@ //! Resource limits and system quotas -use diesel::pg::TypeOidLookup; +use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; +use nexus_db_queries::db; +use nexus_db_queries::db::lookup; +use nexus_types::external_api::params; +use omicron_common::api::external::http_pagination::PaginatedBy; +use omicron_common::api::external::CreateResult; +use omicron_common::api::external::Error; +use omicron_common::api::external::ListResultVec; impl super::Nexus { pub async fn quotas_list( &self, opctx: &OpContext, pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { - self.db_datastore.quota_list(opctx, pagparams).await + ) -> Result { + self.db_datastore.quotas_list(opctx, pagparams).await } pub(crate) async fn fleet_list_quotas( &self, opctx: &OpContext, pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { + ) -> ListResultVec { self.db_datastore.fleet_list_quotas(opctx, pagparams).await } @@ -28,21 +35,21 @@ impl super::Nexus { &self, opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, - ) -> Result { + ) -> Result { let (.., authz_silo) = silo_lookup.lookup_for(authz::Action::Read).await?; self.db_datastore.silo_fetch_quota(opctx, authz_silo).await } - pub(crate) async fn silo_create_quota( + pub(crate) async fn silo_create_quotas( &self, opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, - quota: &db::model::Quota, - ) -> Result { + quotas: ¶ms::SiloQuotasCreate, + ) -> CreateResult { let (.., authz_silo) = silo_lookup.lookup_for(authz::Action::Modify).await?; - self.db_datastore.silo_create_quota(opctx, authz_silo, quota).await + self.db_datastore.silo_create_quota(opctx, authz_silo, quotas).await } pub(crate) async fn silo_update_quota( diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index da9f343022..89850c5675 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -44,7 +44,7 @@ use nexus_db_queries::db::model::Name; use nexus_db_queries::{ authz::ApiResource, db::fixed_data::silo::INTERNAL_SILO_ID, }; -use nexus_types::external_api::params::ProjectSelector; +use nexus_types::external_api::{params::ProjectSelector, views::SiloQuotas}; use nexus_types::{ external_api::views::{SledInstance, Switch}, identity::AssetIdentityMetadata, @@ -553,7 +553,7 @@ async fn quota_list( async fn system_quota_list( rqctx: RequestContext>, query_params: Query, -) -> Result>, HttpError> { +) -> Result>, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; @@ -588,7 +588,7 @@ async fn system_quota_list( async fn silo_quota_view( rqctx: RequestContext>, path_params: Path, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index a0169ae777..af3679723d 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -271,6 +271,13 @@ pub struct SiloCreate { BTreeMap>, } +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct SiloQuotasCreate { + pub cpus: i64, + pub memory: i64, + pub storage: i64, +} + /// Create-time parameters for a `User` #[derive(Clone, Deserialize, Serialize, JsonSchema)] pub struct UserCreate { diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index b34fc7a542..e1c790d28f 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -49,6 +49,16 @@ pub struct Silo { BTreeMap>, } +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct SiloQuotas { + #[serde(flatten)] + pub identity: AssetIdentityMetadata, + pub silo_id: Uuid, + pub cpus: i64, + pub memory: i64, + pub storage: i64, +} + // IDENTITY PROVIDER #[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 7bd83439e8..f3be3ee258 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -772,6 +772,15 @@ CREATE UNIQUE INDEX IF NOT EXISTS lookup_ssh_key_by_silo_user ON omicron.public. ) WHERE time_deleted IS NULL; +CREATE TABLE IF NOT EXISTS omicron.public.silo_quotas ( + silo_id UUID PRIMARY KEY, + time_created TIMESTAMPTZ NOT NULL, + time_modified TIMESTAMPTZ NOT NULL, + cpus INT8 NOT NULL, + memory INT8 NOT NULL, + storage INT8 NOT NULL, +) + /* * Projects */ From 5c50ce5fb81b0ab3d5fea14813b579f7775a611b Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 4 Dec 2023 09:24:29 -0500 Subject: [PATCH 05/47] Rough API shape of resource quotas --- common/src/api/external/mod.rs | 1 + nexus/db-model/src/quota.rs | 33 ++- nexus/db-queries/src/db/datastore/quota.rs | 127 ++++++----- nexus/src/app/quota.rs | 54 ++--- nexus/src/external_api/http_entrypoints.rs | 79 +++---- nexus/src/external_api/tag-config.json | 6 + nexus/tests/output/nexus_tags.txt | 7 + nexus/types/src/external_api/params.rs | 2 +- nexus/types/src/external_api/views.rs | 2 - openapi/nexus.json | 247 +++++++++++++++++++++ schema/crdb/dbinit.sql | 4 +- 11 files changed, 413 insertions(+), 149 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index adf661516a..9d661417da 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -704,6 +704,7 @@ pub enum ResourceType { Silo, SiloUser, SiloGroup, + SiloQuotas, IdentityProvider, SamlIdentityProvider, SshKey, diff --git a/nexus/db-model/src/quota.rs b/nexus/db-model/src/quota.rs index 514f5aa5e2..da0d2fdc64 100644 --- a/nexus/db-model/src/quota.rs +++ b/nexus/db-model/src/quota.rs @@ -1,9 +1,12 @@ +use crate::schema::silo_quotas; use chrono::{DateTime, Utc}; +use nexus_types::external_api::views; +use serde::{Deserialize, Serialize}; use uuid::Uuid; -use crate::schema::silo_quotas; - -#[derive(Queryable, Insertable, Debug, Clone, Selectable)] +#[derive( + Queryable, Insertable, Debug, Clone, Selectable, Serialize, Deserialize, +)] #[diesel(table_name = silo_quotas)] pub struct SiloQuotas { pub silo_id: Uuid, @@ -27,3 +30,27 @@ impl SiloQuotas { } } } + +impl From for views::SiloQuotas { + fn from(silo_quotas: SiloQuotas) -> Self { + Self { + silo_id: silo_quotas.silo_id, + cpus: silo_quotas.cpus, + memory: silo_quotas.memory, + storage: silo_quotas.storage, + } + } +} + +impl From for SiloQuotas { + fn from(silo_quotas: views::SiloQuotas) -> Self { + Self { + silo_id: silo_quotas.silo_id, + time_created: Utc::now(), + time_modified: Utc::now(), + cpus: silo_quotas.cpus, + memory: silo_quotas.memory, + storage: silo_quotas.storage, + } + } +} diff --git a/nexus/db-queries/src/db/datastore/quota.rs b/nexus/db-queries/src/db/datastore/quota.rs index b955c5636e..07cd4c1f86 100644 --- a/nexus/db-queries/src/db/datastore/quota.rs +++ b/nexus/db-queries/src/db/datastore/quota.rs @@ -2,19 +2,26 @@ use super::DataStore; use crate::authz; use crate::context::OpContext; use crate::db; -use crate::db::collection_insert::AsyncInsertError; use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; +use crate::db::pagination::paginated; +use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; -use diesel::upsert::excluded; +use diesel::prelude::*; use nexus_db_model::SiloQuotas; +use nexus_types::external_api::params; use omicron_common::api::external::CreateResult; +use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; -use omicron_common::api::external::LookupType; +use omicron_common::api::external::ListResultVec; use omicron_common::api::external::ResourceType; +use omicron_common::api::external::UpdateResult; +use uuid::Uuid; impl DataStore { - pub async fn silo_quotas_upsert( + /// Creates new quotas for a silo. This is grouped with silo creation + /// and shouldn't be called directly by the user. + pub async fn silo_quotas_create( &self, opctx: &OpContext, authz_silo: &authz::Silo, @@ -24,68 +31,78 @@ impl DataStore { let silo_id = authz_silo.id(); use db::schema::silo_quotas::dsl; - let now = Utc::now(); - let quota_db = SiloQuotas::insert_resource( - silo_id, - diesel::insert_into(dsl::silo_quotas) - .values(quotas.clone()) - .on_conflict(dsl::silo_id) - .do_update() - .set(( - dsl::time_created.eq(excluded(dsl::time_created)), - dsl::time_modified.eq(now), - )), - ) - .insert_and_get_result_async( - &*self.pool_connection_authorized(&opctx).await?, - ) - .await - .map_err(|e| match e { - AsyncInsertError::CollectionNotFound => Error::ObjectNotFound { - type_name: ResourceType::SiloQuotas, - lookup_type: LookupType::ById(silo_id), - }, - AsyncInsertError::DatabaseError(e) => { - public_error_from_diesel(e, ErrorHandler::Server) - } - })?; + diesel::insert_into(dsl::silo_quotas) + .values(quotas) + .returning(SiloQuotas::as_returning()) + .get_result_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::Conflict( + ResourceType::SiloQuotas, + &silo_id.to_string(), + ), + ) + }) + } - Ok(quota_db) + pub async fn silo_update_quota( + &self, + opctx: &OpContext, + authz_silo: &authz::Silo, + updates: params::SiloQuotasUpdate, + ) -> UpdateResult { + opctx.authorize(authz::Action::Modify, authz_silo).await?; + use db::schema::silo_quotas::dsl; + let silo_id = authz_silo.id(); + diesel::update(dsl::silo_quotas) + .filter(dsl::silo_id.eq(silo_id)) + .set(( + dsl::time_modified.eq(Utc::now()), + dsl::cpus.eq(updates.cpus), + dsl::memory.eq(updates.memory), + dsl::storage.eq(updates.storage), + )) + .returning(SiloQuotas::as_returning()) + .get_result_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::Conflict( + ResourceType::SiloQuotas, + &silo_id.to_string(), + ), + ) + }) } - pub async fn quotas_list( + pub async fn silo_quotas_view( &self, opctx: &OpContext, - pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { - opctx - .authorize(authz::Action::ListChildren, authz::Resource::FLEET) - .await?; - let mut query = db::model::Quota::query(); - query = query.filter(db::schema::quotas::silo_id.eq(opctx.silo_id)); - query = query.order_by(db::schema::quotas::id.asc()); - query = pagparams.paginate(query); - query - .load_async::(&self.pool) + authz_silo: &authz::Silo, + ) -> Result { + opctx.authorize(authz::Action::Read, authz_silo).await?; + use db::schema::silo_quotas::dsl; + dsl::silo_quotas + .filter(dsl::silo_id.eq(authz_silo.id())) + .first_async(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(Error::from) + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } pub async fn fleet_list_quotas( &self, opctx: &OpContext, - pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { - opctx - .authorize(authz::Action::ListChildren, authz::Resource::FLEET) - .await?; - let mut query = db::model::Quota::query(); - query = query.filter(db::schema::quotas::silo_id.eq(opctx.silo_id)); - query = query.order_by(db::schema::quotas::id.asc()); - query = pagparams.paginate(query); - query - .load_async::(&self.pool) + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; + use db::schema::silo_quotas::dsl; + paginated(dsl::silo_quotas, dsl::silo_id, pagparams) + .select(SiloQuotas::as_select()) + .load_async(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(Error::from) + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } } diff --git a/nexus/src/app/quota.rs b/nexus/src/app/quota.rs index 4a52abce6e..fa76af9f24 100644 --- a/nexus/src/app/quota.rs +++ b/nexus/src/app/quota.rs @@ -9,67 +9,41 @@ use nexus_db_queries::context::OpContext; use nexus_db_queries::db; use nexus_db_queries::db::lookup; use nexus_types::external_api::params; -use omicron_common::api::external::http_pagination::PaginatedBy; -use omicron_common::api::external::CreateResult; +use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; +use omicron_common::api::external::UpdateResult; +use uuid::Uuid; impl super::Nexus { - pub async fn quotas_list( - &self, - opctx: &OpContext, - pagparams: &PaginatedBy<'_>, - ) -> Result { - self.db_datastore.quotas_list(opctx, pagparams).await - } - - pub(crate) async fn fleet_list_quotas( - &self, - opctx: &OpContext, - pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { - self.db_datastore.fleet_list_quotas(opctx, pagparams).await - } - - pub(crate) async fn silo_fetch_quota( + pub async fn silo_quotas_view( &self, opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, ) -> Result { let (.., authz_silo) = silo_lookup.lookup_for(authz::Action::Read).await?; - self.db_datastore.silo_fetch_quota(opctx, authz_silo).await + self.db_datastore.silo_quotas_view(opctx, &authz_silo).await } - pub(crate) async fn silo_create_quotas( + pub(crate) async fn fleet_list_quotas( &self, opctx: &OpContext, - silo_lookup: &lookup::Silo<'_>, - quotas: ¶ms::SiloQuotasCreate, - ) -> CreateResult { - let (.., authz_silo) = - silo_lookup.lookup_for(authz::Action::Modify).await?; - self.db_datastore.silo_create_quota(opctx, authz_silo, quotas).await + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + self.db_datastore.fleet_list_quotas(opctx, pagparams).await } pub(crate) async fn silo_update_quota( &self, opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, - updates: ¶ms::QuotaUpdate, - ) -> Result { + updates: ¶ms::SiloQuotasUpdate, + ) -> UpdateResult { let (.., authz_silo) = silo_lookup.lookup_for(authz::Action::Modify).await?; - self.db_datastore.silo_update_quota(opctx, authz_silo, updates).await - } - - pub(crate) async fn silo_quota_delete( - &self, - opctx: &OpContext, - silo_lookup: &lookup::Silo<'_>, - ) -> Result { - let (.., authz_silo) = - silo_lookup.lookup_for(authz::Action::Delete).await?; - self.db_datastore.silo_quota_delete(opctx, authz_silo).await + self.db_datastore + .silo_update_quota(opctx, &authz_silo, updates.clone()) + .await } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 89850c5675..dea5ea291b 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -265,7 +265,7 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(networking_bgp_announce_set_list)?; api.register(networking_bgp_announce_set_delete)?; - api.register(quota_list)?; + api.register(quotas_view)?; // Fleet-wide API operations api.register(silo_list)?; @@ -275,10 +275,10 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(silo_policy_view)?; api.register(silo_policy_update)?; - api.register(system_quota_list)?; + api.register(system_quotas_list)?; - api.register(silo_quota_view)?; - api.register(silo_quota_update)?; + api.register(silo_quotas_view)?; + api.register(silo_quotas_update)?; api.register(silo_identity_provider_list)?; @@ -510,82 +510,70 @@ async fn policy_update( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// View the resource quotas of the user's current silo #[endpoint { method = GET, path = "/v1/quotas", - tags = ["quotas"], + tags = ["silos"], }] -async fn quota_list( +async fn quotas_view( rqctx: RequestContext>, - query_params: Query, -) -> Result>, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; - - let query = query_params.into_inner(); - let pag_params = data_page_params_for(&rqctx, &query)?; - let scan_params = ScanByNameOrId::from_query(&query)?; - let paginated_by = name_or_id_pagination(&pag_params, scan_params)?; - let opctx = crate::context::op_context_for_external_api(&rqctx).await?; - let quotas = nexus - .quotas_list(&opctx, &paginated_by) - .await? - .into_iter() - .map(|p| p.try_into()) - .collect::, Error>>()?; + let authz_silo = + opctx.authn.silo_required().internal_context("listing quotas")?; + let silo_lookup = nexus.silo_lookup(&opctx, authz_silo.id().into())?; + let quotas = nexus.silo_quotas_view(&opctx, &silo_lookup).await?; - Ok(HttpResponseOk(ScanByNameOrId::results_page( - &query, - quotas, - &marker_for_name_or_id, - )?)) + Ok(HttpResponseOk(quotas.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// Lists resource quotas for all silos #[endpoint { method = GET, path = "/v1/system/quotas", tags = ["system/quotas"], }] -async fn system_quota_list( +async fn system_quotas_list( rqctx: RequestContext>, - query_params: Query, + query_params: Query, ) -> Result>, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; let query = query_params.into_inner(); - let pag_params = data_page_params_for(&rqctx, &query)?; - let scan_params = ScanByNameOrId::from_query(&query)?; - let paginated_by = name_or_id_pagination(&pag_params, scan_params)?; + let pagparams = data_page_params_for(&rqctx, &query)?; let opctx = crate::context::op_context_for_external_api(&rqctx).await?; let quotas = nexus - .fleet_list_quotas(&opctx, &paginated_by) + .fleet_list_quotas(&opctx, &pagparams) .await? .into_iter() - .map(|p| p.try_into()) - .collect::, Error>>()?; + .map(|p| p.into()) + .collect(); - Ok(HttpResponseOk(ScanByNameOrId::results_page( + Ok(HttpResponseOk(ScanById::results_page( &query, quotas, - &marker_for_name_or_id, + &|_, quota: &SiloQuotas| quota.silo_id, )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// View the resource quotas of a given silo #[endpoint { method = GET, path = "/v1/system/silos/{silo}/quotas", tags = ["system/quotas"], }] -async fn silo_quota_view( +async fn silo_quotas_view( rqctx: RequestContext>, path_params: Path, ) -> Result, HttpError> { @@ -596,23 +584,23 @@ async fn silo_quota_view( let opctx = crate::context::op_context_for_external_api(&rqctx).await?; let silo_lookup = nexus.silo_lookup(&opctx, path_params.into_inner().silo)?; - let quota = - nexus.silo_fetch_quota(&opctx, &silo_lookup).await?.try_into()?; - Ok(HttpResponseOk(quota)) + let quota = nexus.silo_quotas_view(&opctx, &silo_lookup).await?; + Ok(HttpResponseOk(quota.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// Update the resource quotas of a given silo #[endpoint { method = PUT, path = "/v1/system/silos/{silo}/quotas", tags = ["system/quotas"], }] -async fn silo_quota_update( +async fn silo_quotas_update( rqctx: RequestContext>, path_params: Path, - new_quota: TypedBody, -) -> Result, HttpError> { + new_quota: TypedBody, +) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; @@ -621,10 +609,9 @@ async fn silo_quota_update( let silo_lookup = nexus.silo_lookup(&opctx, path_params.into_inner().silo)?; let quota = nexus - .silo_update_quota(&opctx, &silo_lookup, new_quota.into_inner()) - .await? - .try_into()?; - Ok(HttpResponseOk(quota)) + .silo_update_quota(&opctx, &silo_lookup, &new_quota.into_inner()) + .await?; + Ok(HttpResponseOk(quota.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } diff --git a/nexus/src/external_api/tag-config.json b/nexus/src/external_api/tag-config.json index 07eb198016..dc1a9cf28d 100644 --- a/nexus/src/external_api/tag-config.json +++ b/nexus/src/external_api/tag-config.json @@ -98,6 +98,12 @@ "url": "http://docs.oxide.computer/api/system-metrics" } }, + "system/quotas": { + "description": "Quotas set resource allocation limits for a silo allowing operators to control the amount of resources a silo can consume.", + "external_docs": { + "url": "http://docs.oxide.computer/api/system-quotas" + } + }, "system/networking": { "description": "This provides rack-level network configuration.", "external_docs": { diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 7f0c30c471..6ec8fafbc5 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -99,6 +99,7 @@ group_list GET /v1/groups group_view GET /v1/groups/{group_id} policy_update PUT /v1/policy policy_view GET /v1/policy +quotas_view GET /v1/quotas user_list GET /v1/users API operations found with tag "snapshots" @@ -162,6 +163,12 @@ networking_switch_port_settings_delete DELETE /v1/system/networking/switch-p networking_switch_port_settings_list GET /v1/system/networking/switch-port-settings networking_switch_port_settings_view GET /v1/system/networking/switch-port-settings/{port} +API operations found with tag "system/quotas" +OPERATION ID METHOD URL PATH +silo_quotas_update PUT /v1/system/silos/{silo}/quotas +silo_quotas_view GET /v1/system/silos/{silo}/quotas +system_quotas_list GET /v1/system/quotas + API operations found with tag "system/silos" OPERATION ID METHOD URL PATH local_idp_user_create POST /v1/system/identity-providers/local/users diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index af3679723d..c4feace386 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -272,7 +272,7 @@ pub struct SiloCreate { } #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct SiloQuotasCreate { +pub struct SiloQuotasUpdate { pub cpus: i64, pub memory: i64, pub storage: i64, diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index e1c790d28f..8db0a4833c 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -51,8 +51,6 @@ pub struct Silo { #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct SiloQuotas { - #[serde(flatten)] - pub identity: AssetIdentityMetadata, pub silo_id: Uuid, pub cpus: i64, pub memory: i64, diff --git a/openapi/nexus.json b/openapi/nexus.json index 0d19e81d9a..3ef4be9eda 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -3197,6 +3197,33 @@ } } }, + "/v1/quotas": { + "get": { + "tags": [ + "silos" + ], + "summary": "View the resource quotas of the user's current silo", + "operationId": "quotas_view", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SiloQuotas" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/v1/snapshots": { "get": { "tags": [ @@ -5894,6 +5921,65 @@ } } }, + "/v1/system/quotas": { + "get": { + "tags": [ + "system/quotas" + ], + "summary": "Lists resource quotas for all silos", + "operationId": "system_quotas_list", + "parameters": [ + { + "in": "query", + "name": "limit", + "description": "Maximum number of items returned by a single call", + "schema": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 1 + } + }, + { + "in": "query", + "name": "page_token", + "description": "Token returned by previous call to retrieve the subsequent page", + "schema": { + "nullable": true, + "type": "string" + } + }, + { + "in": "query", + "name": "sort_by", + "schema": { + "$ref": "#/components/schemas/IdSortMode" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SiloQuotasResultsPage" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + }, + "x-dropshot-pagination": { + "required": [] + } + } + }, "/v1/system/roles": { "get": { "tags": [ @@ -6232,6 +6318,90 @@ } } }, + "/v1/system/silos/{silo}/quotas": { + "get": { + "tags": [ + "system/quotas" + ], + "summary": "View the resource quotas of a given silo", + "operationId": "silo_quotas_view", + "parameters": [ + { + "in": "path", + "name": "silo", + "description": "Name or ID of the silo", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SiloQuotas" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "put": { + "tags": [ + "system/quotas" + ], + "summary": "Update the resource quotas of a given silo", + "operationId": "silo_quotas_update", + "parameters": [ + { + "in": "path", + "name": "silo", + "description": "Name or ID of the silo", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SiloQuotasUpdate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SiloQuotas" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/v1/system/users": { "get": { "tags": [ @@ -12870,6 +13040,76 @@ } ] }, + "SiloQuotas": { + "type": "object", + "properties": { + "cpus": { + "type": "integer", + "format": "int64" + }, + "memory": { + "type": "integer", + "format": "int64" + }, + "silo_id": { + "type": "string", + "format": "uuid" + }, + "storage": { + "type": "integer", + "format": "int64" + } + }, + "required": [ + "cpus", + "memory", + "silo_id", + "storage" + ] + }, + "SiloQuotasResultsPage": { + "description": "A single page of results", + "type": "object", + "properties": { + "items": { + "description": "list of items on this page of results", + "type": "array", + "items": { + "$ref": "#/components/schemas/SiloQuotas" + } + }, + "next_page": { + "nullable": true, + "description": "token used to fetch the next page of results (if any)", + "type": "string" + } + }, + "required": [ + "items" + ] + }, + "SiloQuotasUpdate": { + "type": "object", + "properties": { + "cpus": { + "type": "integer", + "format": "int64" + }, + "memory": { + "type": "integer", + "format": "int64" + }, + "storage": { + "type": "integer", + "format": "int64" + } + }, + "required": [ + "cpus", + "memory", + "storage" + ] + }, "SiloResultsPage": { "description": "A single page of results", "type": "object", @@ -15130,6 +15370,13 @@ "url": "http://docs.oxide.computer/api/system-networking" } }, + { + "name": "system/quotas", + "description": "Quotas set resource allocation limits for a silo allowing operators to control the amount of resources a silo can consume.", + "externalDocs": { + "url": "http://docs.oxide.computer/api/system-quotas" + } + }, { "name": "system/silos", "description": "Silos represent a logical partition of users and resources.", diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index f3be3ee258..2a41a181df 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -778,8 +778,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.silo_quotas ( time_modified TIMESTAMPTZ NOT NULL, cpus INT8 NOT NULL, memory INT8 NOT NULL, - storage INT8 NOT NULL, -) + storage INT8 NOT NULL +); /* * Projects From 3346a33b262334f9bccfffe3aed5202972c34711 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 4 Dec 2023 11:29:52 -0500 Subject: [PATCH 06/47] Create quotas during silo creation --- nexus/db-model/src/quota.rs | 32 +++++++++++++- nexus/db-queries/src/db/datastore/quota.rs | 49 +++++++++++++--------- nexus/db-queries/src/db/datastore/silo.rs | 14 +++++++ nexus/db-queries/src/db/fixed_data/silo.rs | 12 ++++++ nexus/src/app/quota.rs | 2 +- nexus/src/app/rack.rs | 2 + nexus/test-utils/src/resource_helpers.rs | 5 +++ nexus/types/src/external_api/params.rs | 12 +++++- 8 files changed, 104 insertions(+), 24 deletions(-) diff --git a/nexus/db-model/src/quota.rs b/nexus/db-model/src/quota.rs index da0d2fdc64..277ebb7888 100644 --- a/nexus/db-model/src/quota.rs +++ b/nexus/db-model/src/quota.rs @@ -1,11 +1,18 @@ use crate::schema::silo_quotas; use chrono::{DateTime, Utc}; -use nexus_types::external_api::views; +use nexus_types::external_api::{params, views}; use serde::{Deserialize, Serialize}; use uuid::Uuid; #[derive( - Queryable, Insertable, Debug, Clone, Selectable, Serialize, Deserialize, + Queryable, + Insertable, + Debug, + Clone, + Selectable, + Serialize, + Deserialize, + AsChangeset, )] #[diesel(table_name = silo_quotas)] pub struct SiloQuotas { @@ -54,3 +61,24 @@ impl From for SiloQuotas { } } } + +// Describes a set of updates for the [`SiloQuotas`] model. +#[derive(AsChangeset)] +#[diesel(table_name = silo_quotas)] +pub struct SiloQuotasUpdate { + pub cpus: Option, + pub memory: Option, + pub storage: Option, + pub time_modified: DateTime, +} + +impl From for SiloQuotasUpdate { + fn from(params: params::SiloQuotasUpdate) -> Self { + Self { + cpus: params.cpus, + memory: params.memory, + storage: params.storage, + time_modified: Utc::now(), + } + } +} diff --git a/nexus/db-queries/src/db/datastore/quota.rs b/nexus/db-queries/src/db/datastore/quota.rs index 07cd4c1f86..894fd94ff3 100644 --- a/nexus/db-queries/src/db/datastore/quota.rs +++ b/nexus/db-queries/src/db/datastore/quota.rs @@ -5,12 +5,13 @@ use crate::db; use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::pagination::paginated; +use crate::db::pool::DbConnection; +use crate::db::TransactionError; +use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; -use chrono::Utc; use diesel::prelude::*; use nexus_db_model::SiloQuotas; -use nexus_types::external_api::params; -use omicron_common::api::external::CreateResult; +use nexus_db_model::SiloQuotasUpdate; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; @@ -24,46 +25,54 @@ impl DataStore { pub async fn silo_quotas_create( &self, opctx: &OpContext, + conn: &async_bb8_diesel::Connection, authz_silo: &authz::Silo, quotas: SiloQuotas, - ) -> CreateResult { + ) -> Result<(), Error> { opctx.authorize(authz::Action::Modify, authz_silo).await?; let silo_id = authz_silo.id(); use db::schema::silo_quotas::dsl; - diesel::insert_into(dsl::silo_quotas) - .values(quotas) - .returning(SiloQuotas::as_returning()) - .get_result_async(&*self.pool_connection_authorized(opctx).await?) - .await - .map_err(|e| { - public_error_from_diesel( + let result = conn + .transaction_async(|c| async move { + diesel::insert_into(dsl::silo_quotas) + .values(quotas) + .execute_async(&c) + .await + .map_err(TransactionError::CustomError) + }) + .await; + + match result { + Ok(_) => Ok(()), + Err(TransactionError::CustomError(e)) => { + // TODO: Is this the right error handler? + Err(public_error_from_diesel(e, ErrorHandler::Server)) + } + Err(TransactionError::Database(e)) => { + Err(public_error_from_diesel( e, ErrorHandler::Conflict( ResourceType::SiloQuotas, &silo_id.to_string(), ), - ) - }) + )) + } + } } pub async fn silo_update_quota( &self, opctx: &OpContext, authz_silo: &authz::Silo, - updates: params::SiloQuotasUpdate, + updates: SiloQuotasUpdate, ) -> UpdateResult { opctx.authorize(authz::Action::Modify, authz_silo).await?; use db::schema::silo_quotas::dsl; let silo_id = authz_silo.id(); diesel::update(dsl::silo_quotas) .filter(dsl::silo_id.eq(silo_id)) - .set(( - dsl::time_modified.eq(Utc::now()), - dsl::cpus.eq(updates.cpus), - dsl::memory.eq(updates.memory), - dsl::storage.eq(updates.storage), - )) + .set(updates) .returning(SiloQuotas::as_returning()) .get_result_async(&*self.pool_connection_authorized(opctx).await?) .await diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index ec3658c067..9d53b18496 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -27,6 +27,7 @@ use chrono::Utc; use diesel::prelude::*; use nexus_db_model::Certificate; use nexus_db_model::ServiceKind; +use nexus_db_model::SiloQuotas; use nexus_types::external_api::params; use nexus_types::external_api::shared; use nexus_types::external_api::shared::SiloRole; @@ -255,6 +256,19 @@ impl DataStore { self.dns_update(nexus_opctx, &conn, dns_update).await?; + self.silo_quotas_create( + opctx, + &conn, + &authz_silo, + SiloQuotas::new( + authz_silo.id(), + new_silo_params.quotas.cpus, + new_silo_params.quotas.memory, + new_silo_params.quotas.storage, + ), + ) + .await?; + Ok(silo) }) .await diff --git a/nexus/db-queries/src/db/fixed_data/silo.rs b/nexus/db-queries/src/db/fixed_data/silo.rs index d32c4211e9..d63c719567 100644 --- a/nexus/db-queries/src/db/fixed_data/silo.rs +++ b/nexus/db-queries/src/db/fixed_data/silo.rs @@ -24,6 +24,12 @@ lazy_static! { name: "default-silo".parse().unwrap(), description: "default silo".to_string(), }, + // TODO: Should the default silo have a quota? If so, what should the defaults be? + quotas: params::SiloQuotasCreate { + cpus: 0, + memory: 0, + storage: 0, + }, discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, @@ -49,6 +55,12 @@ lazy_static! { name: "oxide-internal".parse().unwrap(), description: "Built-in internal Silo.".to_string(), }, + // TODO: Should the internal silo have a quota? If so, what should the defaults be? + quotas: params::SiloQuotasCreate { + cpus: 0, + memory: 0, + storage: 0, + }, discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/src/app/quota.rs b/nexus/src/app/quota.rs index fa76af9f24..f59069a9ab 100644 --- a/nexus/src/app/quota.rs +++ b/nexus/src/app/quota.rs @@ -43,7 +43,7 @@ impl super::Nexus { let (.., authz_silo) = silo_lookup.lookup_for(authz::Action::Modify).await?; self.db_datastore - .silo_update_quota(opctx, &authz_silo, updates.clone()) + .silo_update_quota(opctx, &authz_silo, updates.clone().into()) .await } } diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 1c2e49e260..9e452aaeb1 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -193,6 +193,8 @@ impl super::Nexus { name: request.recovery_silo.silo_name, description: "built-in recovery Silo".to_string(), }, + // TODO: Should the recovery silo have a quota? If so, what should the defaults be? + quotas: params::SiloQuotasCreate { cpus: 0, memory: 0, storage: 0 }, discoverable: false, identity_mode: SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 2368c3f568..5687e19c69 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -262,6 +262,11 @@ pub async fn create_silo( name: silo_name.parse().unwrap(), description: "a silo".to_string(), }, + quotas: params::SiloQuotasCreate { + cpus: 36, + memory: 1000, + storage: 100000, + }, discoverable, identity_mode, admin_group_name: None, diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index c4feace386..bfc9b318c1 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -262,6 +262,9 @@ pub struct SiloCreate { /// endpoints. These should be valid for the Silo's DNS name(s). pub tls_certificates: Vec, + /// Initial quotas for the new Silo + pub quotas: SiloQuotasCreate, + /// Mapping of which Fleet roles are conferred by each Silo role /// /// The default is that no Fleet roles are conferred by any Silo roles @@ -272,12 +275,19 @@ pub struct SiloCreate { } #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct SiloQuotasUpdate { +pub struct SiloQuotasCreate { pub cpus: i64, pub memory: i64, pub storage: i64, } +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct SiloQuotasUpdate { + pub cpus: Option, + pub memory: Option, + pub storage: Option, +} + /// Create-time parameters for a `User` #[derive(Clone, Deserialize, Serialize, JsonSchema)] pub struct UserCreate { From 501f5ac50871bd9dc695c768844142c812ddd4fa Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 4 Dec 2023 13:36:34 -0500 Subject: [PATCH 07/47] Update comment for internal silo --- nexus/db-queries/src/db/fixed_data/silo.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/fixed_data/silo.rs b/nexus/db-queries/src/db/fixed_data/silo.rs index d63c719567..ea9a9778da 100644 --- a/nexus/db-queries/src/db/fixed_data/silo.rs +++ b/nexus/db-queries/src/db/fixed_data/silo.rs @@ -55,7 +55,7 @@ lazy_static! { name: "oxide-internal".parse().unwrap(), description: "Built-in internal Silo.".to_string(), }, - // TODO: Should the internal silo have a quota? If so, what should the defaults be? + // The internal silo contains no virtual resources, so it has no allotted capacity. quotas: params::SiloQuotasCreate { cpus: 0, memory: 0, From 9e51431b5ecd210a95c119b89d5c3206810a06a9 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 5 Dec 2023 14:39:27 -0500 Subject: [PATCH 08/47] Drop inner silo quota view There will be a follow up PR to add capacity/utilization to the API both at the silo and system levels. Given that, and the general lack of actionability of quotas to silo users, I've just dropped the quota view from the API. --- nexus/src/external_api/http_entrypoints.rs | 25 ------- nexus/tests/output/nexus_tags.txt | 1 - openapi/nexus.json | 84 +++++++++++----------- 3 files changed, 44 insertions(+), 66 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d4843e8684..88a749554d 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -268,8 +268,6 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(networking_bgp_announce_set_list)?; api.register(networking_bgp_announce_set_delete)?; - api.register(quotas_view)?; - // Fleet-wide API operations api.register(silo_list)?; api.register(silo_create)?; @@ -513,29 +511,6 @@ async fn policy_update( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -/// View the resource quotas of the user's current silo -#[endpoint { - method = GET, - path = "/v1/quotas", - tags = ["silos"], -}] -async fn quotas_view( - rqctx: RequestContext>, -) -> Result, HttpError> { - let apictx = rqctx.context(); - let handler = async { - let nexus = &apictx.nexus; - let opctx = crate::context::op_context_for_external_api(&rqctx).await?; - let authz_silo = - opctx.authn.silo_required().internal_context("listing quotas")?; - let silo_lookup = nexus.silo_lookup(&opctx, authz_silo.id().into())?; - let quotas = nexus.silo_quotas_view(&opctx, &silo_lookup).await?; - - Ok(HttpResponseOk(quotas.into())) - }; - apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await -} - /// Lists resource quotas for all silos #[endpoint { method = GET, diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 2dcadeec8a..50d23981e6 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -99,7 +99,6 @@ group_list GET /v1/groups group_view GET /v1/groups/{group_id} policy_update PUT /v1/policy policy_view GET /v1/policy -quotas_view GET /v1/quotas user_list GET /v1/users API operations found with tag "snapshots" diff --git a/openapi/nexus.json b/openapi/nexus.json index 6e933bc458..d904064fee 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -3197,33 +3197,6 @@ } } }, - "/v1/quotas": { - "get": { - "tags": [ - "silos" - ], - "summary": "View the resource quotas of the user's current silo", - "operationId": "quotas_view", - "responses": { - "200": { - "description": "successful operation", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/SiloQuotas" - } - } - } - }, - "4XX": { - "$ref": "#/components/responses/Error" - }, - "5XX": { - "$ref": "#/components/responses/Error" - } - } - } - }, "/v1/snapshots": { "get": { "tags": [ @@ -13168,6 +13141,14 @@ "name": { "$ref": "#/components/schemas/Name" }, + "quotas": { + "description": "Initial quotas for the new Silo", + "allOf": [ + { + "$ref": "#/components/schemas/SiloQuotasCreate" + } + ] + }, "tls_certificates": { "description": "Initial TLS certificates to be used for the new Silo's console and API endpoints. These should be valid for the Silo's DNS name(s).", "type": "array", @@ -13181,6 +13162,7 @@ "discoverable", "identity_mode", "name", + "quotas", "tls_certificates" ] }, @@ -13211,22 +13193,40 @@ "format": "int64" }, "memory": { - "type": "integer", - "format": "int64" + "$ref": "#/components/schemas/ByteCount" }, "silo_id": { "type": "string", "format": "uuid" }, "storage": { + "$ref": "#/components/schemas/ByteCount" + } + }, + "required": [ + "cpus", + "memory", + "silo_id", + "storage" + ] + }, + "SiloQuotasCreate": { + "type": "object", + "properties": { + "cpus": { "type": "integer", "format": "int64" + }, + "memory": { + "$ref": "#/components/schemas/ByteCount" + }, + "storage": { + "$ref": "#/components/schemas/ByteCount" } }, "required": [ "cpus", "memory", - "silo_id", "storage" ] }, @@ -13255,23 +13255,27 @@ "type": "object", "properties": { "cpus": { + "nullable": true, "type": "integer", "format": "int64" }, "memory": { - "type": "integer", - "format": "int64" + "nullable": true, + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] }, "storage": { - "type": "integer", - "format": "int64" + "nullable": true, + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] } - }, - "required": [ - "cpus", - "memory", - "storage" - ] + } }, "SiloResultsPage": { "description": "A single page of results", From caf371b902a5257cff3bedb327b5eaf53b3bf51d Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 5 Dec 2023 14:44:05 -0500 Subject: [PATCH 09/47] Convert storage/memory to bytecount instead of ints --- nexus/db-model/src/quota.rs | 33 ++++++++++++++++------ nexus/db-model/src/schema.rs | 4 +-- nexus/db-queries/src/db/datastore/silo.rs | 4 +-- nexus/db-queries/src/db/fixed_data/silo.rs | 12 ++------ nexus/src/app/external_endpoints.rs | 1 + nexus/src/app/rack.rs | 2 +- nexus/test-utils/src/resource_helpers.rs | 4 +-- nexus/tests/integration_tests/endpoints.rs | 5 ++++ nexus/tests/integration_tests/silos.rs | 7 +++++ nexus/types/src/external_api/params.rs | 18 +++++++++--- nexus/types/src/external_api/views.rs | 4 +-- schema/crdb/dbinit.sql | 4 +-- 12 files changed, 64 insertions(+), 34 deletions(-) diff --git a/nexus/db-model/src/quota.rs b/nexus/db-model/src/quota.rs index 277ebb7888..b42456b330 100644 --- a/nexus/db-model/src/quota.rs +++ b/nexus/db-model/src/quota.rs @@ -1,3 +1,4 @@ +use super::ByteCount; use crate::schema::silo_quotas; use chrono::{DateTime, Utc}; use nexus_types::external_api::{params, views}; @@ -20,13 +21,25 @@ pub struct SiloQuotas { pub time_created: DateTime, pub time_modified: DateTime, + /// The number of CPUs that this silo is allowed to use pub cpus: i64, - pub memory: i64, - pub storage: i64, + + /// The amount of memory (in bytes) that this silo is allowed to use + #[diesel(column_name = memory_bytes)] + pub memory: ByteCount, + + /// The amount of storage (in bytes) that this silo is allowed to use + #[diesel(column_name = storage_bytes)] + pub storage: ByteCount, } impl SiloQuotas { - pub fn new(silo_id: Uuid, cpus: i64, memory: i64, storage: i64) -> Self { + pub fn new( + silo_id: Uuid, + cpus: i64, + memory: ByteCount, + storage: ByteCount, + ) -> Self { Self { silo_id, time_created: Utc::now(), @@ -43,8 +56,8 @@ impl From for views::SiloQuotas { Self { silo_id: silo_quotas.silo_id, cpus: silo_quotas.cpus, - memory: silo_quotas.memory, - storage: silo_quotas.storage, + memory: silo_quotas.memory.into(), + storage: silo_quotas.storage.into(), } } } @@ -56,8 +69,8 @@ impl From for SiloQuotas { time_created: Utc::now(), time_modified: Utc::now(), cpus: silo_quotas.cpus, - memory: silo_quotas.memory, - storage: silo_quotas.storage, + memory: silo_quotas.memory.into(), + storage: silo_quotas.storage.into(), } } } @@ -67,7 +80,9 @@ impl From for SiloQuotas { #[diesel(table_name = silo_quotas)] pub struct SiloQuotasUpdate { pub cpus: Option, + #[diesel(column_name = memory_bytes)] pub memory: Option, + #[diesel(column_name = storage_bytes)] pub storage: Option, pub time_modified: DateTime, } @@ -76,8 +91,8 @@ impl From for SiloQuotasUpdate { fn from(params: params::SiloQuotasUpdate) -> Self { Self { cpus: params.cpus, - memory: params.memory, - storage: params.storage, + memory: params.memory.map(|f| f.into()), + storage: params.storage.map(|f| f.into()), time_modified: Utc::now(), } } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index d2367db33b..a266a54b6e 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -415,8 +415,8 @@ table! { time_created -> Timestamptz, time_modified -> Timestamptz, cpus -> Int8, - memory -> Int8, - storage -> Int8, + memory_bytes -> Int8, + storage_bytes -> Int8, } } diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index 9d53b18496..6e36f163d9 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -263,8 +263,8 @@ impl DataStore { SiloQuotas::new( authz_silo.id(), new_silo_params.quotas.cpus, - new_silo_params.quotas.memory, - new_silo_params.quotas.storage, + new_silo_params.quotas.memory.into(), + new_silo_params.quotas.storage.into(), ), ) .await?; diff --git a/nexus/db-queries/src/db/fixed_data/silo.rs b/nexus/db-queries/src/db/fixed_data/silo.rs index ea9a9778da..d02db783a3 100644 --- a/nexus/db-queries/src/db/fixed_data/silo.rs +++ b/nexus/db-queries/src/db/fixed_data/silo.rs @@ -25,11 +25,7 @@ lazy_static! { description: "default silo".to_string(), }, // TODO: Should the default silo have a quota? If so, what should the defaults be? - quotas: params::SiloQuotasCreate { - cpus: 0, - memory: 0, - storage: 0, - }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, @@ -56,11 +52,7 @@ lazy_static! { description: "Built-in internal Silo.".to_string(), }, // The internal silo contains no virtual resources, so it has no allotted capacity. - quotas: params::SiloQuotasCreate { - cpus: 0, - memory: 0, - storage: 0, - }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/src/app/external_endpoints.rs b/nexus/src/app/external_endpoints.rs index f95c64d3eb..62238a651b 100644 --- a/nexus/src/app/external_endpoints.rs +++ b/nexus/src/app/external_endpoints.rs @@ -827,6 +827,7 @@ mod test { name: name.parse().unwrap(), description: String::new(), }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode, admin_group_name: None, diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index be0934afdf..9f63e312ae 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -204,7 +204,7 @@ impl super::Nexus { description: "built-in recovery Silo".to_string(), }, // TODO: Should the recovery silo have a quota? If so, what should the defaults be? - quotas: params::SiloQuotasCreate { cpus: 0, memory: 0, storage: 0 }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 5687e19c69..2ec41830bc 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -264,8 +264,8 @@ pub async fn create_silo( }, quotas: params::SiloQuotasCreate { cpus: 36, - memory: 1000, - storage: 100000, + memory: ByteCount::from_gibibytes_u32(1000), + storage: ByteCount::from_gibibytes_u32(100000), }, discoverable, identity_mode, diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 536b96f7ae..f5450fc633 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -91,6 +91,11 @@ lazy_static! { name: DEMO_SILO_NAME.clone(), description: String::from(""), }, + quotas: params::SiloQuotasCreate { + cpus: 100, + memory: ByteCount::from_gibibytes_u32(100), + storage: ByteCount::from_gibibytes_u32(100), + }, discoverable: true, identity_mode: shared::SiloIdentityMode::SamlJit, admin_group_name: None, diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index e5f41d294d..84199c48cc 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -65,6 +65,7 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { name: cptestctx.silo_name.clone(), description: "a silo".to_string(), }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, @@ -281,6 +282,7 @@ async fn test_silo_admin_group(cptestctx: &ControlPlaneTestContext) { name: "silo-name".parse().unwrap(), description: "a silo".to_string(), }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::SamlJit, admin_group_name: Some("administrator".into()), @@ -2253,6 +2255,7 @@ async fn test_silo_authn_policy(cptestctx: &ControlPlaneTestContext) { name: silo_name, description: String::new(), }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, @@ -2329,6 +2332,7 @@ async fn check_fleet_privileges( name: SILO_NAME.parse().unwrap(), description: String::new(), }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, @@ -2357,6 +2361,7 @@ async fn check_fleet_privileges( name: SILO_NAME.parse().unwrap(), description: String::new(), }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, @@ -2384,6 +2389,7 @@ async fn check_fleet_privileges( name: SILO_NAME.parse().unwrap(), description: String::new(), }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, @@ -2416,6 +2422,7 @@ async fn check_fleet_privileges( name: SILO_NAME.parse().unwrap(), description: String::new(), }, + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 293a139506..0604fa72d6 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -294,15 +294,25 @@ pub struct SiloCreate { #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct SiloQuotasCreate { pub cpus: i64, - pub memory: i64, - pub storage: i64, + pub memory: ByteCount, + pub storage: ByteCount, +} + +impl SiloQuotasCreate { + pub fn empty() -> Self { + Self { + cpus: 0, + memory: ByteCount::from(0), + storage: ByteCount::from(0), + } + } } #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct SiloQuotasUpdate { pub cpus: Option, - pub memory: Option, - pub storage: Option, + pub memory: Option, + pub storage: Option, } /// Create-time parameters for a `User` diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index d00e095150..8cb1595112 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -53,8 +53,8 @@ pub struct Silo { pub struct SiloQuotas { pub silo_id: Uuid, pub cpus: i64, - pub memory: i64, - pub storage: i64, + pub memory: ByteCount, + pub storage: ByteCount, } // IDENTITY PROVIDER diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 214a783434..c1316d4e61 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -832,8 +832,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.silo_quotas ( time_created TIMESTAMPTZ NOT NULL, time_modified TIMESTAMPTZ NOT NULL, cpus INT8 NOT NULL, - memory INT8 NOT NULL, - storage INT8 NOT NULL + memory_bytes INT8 NOT NULL, + storage_bytes INT8 NOT NULL ); /* From 96877b217d79259380928a9b48d17a93f9f10e98 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 5 Dec 2023 20:44:27 -0500 Subject: [PATCH 10/47] WIP quota check CTE --- .../virtual_provisioning_collection_update.rs | 25 ++++ .../virtual_provisioning_collection_update.rs | 131 ++++++++++++++++-- 2 files changed, 146 insertions(+), 10 deletions(-) diff --git a/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs b/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs index 6c684016b4..95eaafe8f4 100644 --- a/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs @@ -28,6 +28,30 @@ table! { } } +table! { + quotas (silo_id) { + silo_id -> Uuid, + cpus -> Int8, + memory -> Int8, + storage -> Int8, + } +} + +table! { + silo_provisioned { + id -> Uuid, + virtual_disk_bytes_provisioned -> Int8, + cpus_provisioned -> Int8, + ram_provisioned -> Int8, + } +} + +table! { + quota_check (passed) { + passed -> Bool, + } +} + diesel::allow_tables_to_appear_in_same_query!(silo, parent_silo,); diesel::allow_tables_to_appear_in_same_query!( @@ -35,4 +59,5 @@ diesel::allow_tables_to_appear_in_same_query!( parent_silo, all_collections, do_update, + quotas ); diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index 0a383eb6f1..bd84aa1681 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -13,16 +13,50 @@ use crate::db::pool::DbConnection; use crate::db::schema::virtual_provisioning_collection; use crate::db::schema::virtual_provisioning_resource; use crate::db::subquery::{AsQuerySource, Cte, CteBuilder, CteQuery}; +use crate::db::true_or_cast_error::matches_sentinel; +use crate::db::true_or_cast_error::TrueOrCastError; use db_macros::Subquery; use diesel::pg::Pg; use diesel::query_builder::{AstPass, Query, QueryFragment, QueryId}; +use diesel::result::Error as DieselError; use diesel::{ sql_types, CombineDsl, ExpressionMethods, IntoSql, NullableExpressionMethods, QueryDsl, RunQueryDsl, SelectableHelper, }; use nexus_db_model::queries::virtual_provisioning_collection_update::{ - all_collections, do_update, parent_silo, + all_collections, do_update, parent_silo, quota_check, quotas, + silo_provisioned, }; +use omicron_common::api::external; + +const NOT_ENOUGH_CPUS_SENTINEL: &'static str = "Not enough cpus"; +const NOT_ENOUGH_MEMORY_SENTINEL: &'static str = "Not enough memory"; +const NOT_ENOUGH_STORAGE_SENTINEL: &'static str = "Not enough storage"; + +pub fn from_diesel(e: DieselError) -> external::Error { + use crate::db::error; + + let sentinels = [ + NOT_ENOUGH_CPUS_SENTINEL, + NOT_ENOUGH_MEMORY_SENTINEL, + NOT_ENOUGH_STORAGE_SENTINEL, + ]; + if let Some(sentinel) = matches_sentinel(&e, &sentinels) { + match sentinel { + NOT_ENOUGH_CPUS_SENTINEL => { + return external::Error::InvalidRequest { message: "Insufficient Capacity: Not enough CPUs to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string() } + } + NOT_ENOUGH_MEMORY_SENTINEL => { + return external::Error::InvalidRequest { message: "Insufficient Capacity: Not enough memory to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string() } + } + NOT_ENOUGH_STORAGE_SENTINEL => { + return external::Error::InvalidRequest { message: "Insufficient Capacity: Not enough storage to complete request. Either remove unneeded disks and snapshots to free up resources or contact the rack operator to request a capacity increase.".to_string() } + } + _ => {} + } + } + error::public_error_from_diesel(e, error::ErrorHandler::Server) +} #[derive(Subquery, QueryId)] #[subquery(name = parent_silo)] @@ -161,6 +195,67 @@ impl UpdatedProvisions { } } +#[derive(Subquery, QueryId)] +#[subquery(name = quotas)] +struct Quotas { + query: Box>, +} + +impl Quotas { + fn new(parent_silo: &ParentSilo, update_kind: UpdateKind) -> Self { + use crate::db::schema::silo_quotas::dsl; + Self { + query: Box::new( + dsl::silo_quotas.filter(dsl::silo_id.eq(parent_silo::id)), + ), + } + } +} + +#[derive(Subquery, QueryId)] +#[subquery(name = silo_provisioned)] +struct SiloProvisioned { + query: Box>, +} + +impl SiloProvisioned { + fn new(parent_silo: &ParentSilo) -> Self { + use virtual_provisioning_collection::dsl; + Self { + query: Box::new( + dsl::virtual_provisioning_collection + .filter(dsl::id.eq(parent_silo::id)), + ), + } + } +} + +#[derive(Subquery, QueryId)] +#[subquery(name = quota_check)] +struct QuotaCheck { + query: Box>, +} + +impl QuotaCheck { + fn new(silo_provisioned: &SiloProvisioned, quotas: &Quotas) -> Self { + Self { + query: Box::new(diesel::select( + (ExpressionAlias::new::( + TrueOrCastError::new(enough_cpus, NOT_ENOUGH_CPUS_SENTINEL) + .and(TrueOrCastError::new( + enough_memory, + NOT_ENOUGH_MEMORY_SENTINEL, + )) + .and(TrueOrCastError::new( + enough_storage, + NOT_ENOUGH_STORAGE_SENTINEL, + )), + )), + )), + } + } +} + // This structure wraps a query, such that it can be used within a CTE. // // It generates a name that can be used by the "CteBuilder", but does not @@ -195,6 +290,13 @@ where } } +/// The virtual resource collection is only updated when a resource is inserted +/// or deleted from the resource provisioning table for idempotency. +enum UpdateKind { + Insert(VirtualProvisioningResource), + Delete(uuid::Uuid), +} + /// Constructs a CTE for updating resource provisioning information in all /// collections for a particular object. #[derive(QueryId)] @@ -220,7 +322,7 @@ impl VirtualProvisioningCollectionUpdate { // - values: The updated values to propagate through collections (iff // "do_update" evaluates to "true"). fn apply_update( - do_update: DoUpdate, + update_kind: UpdateKind, update: U, project_id: uuid::Uuid, values: V, @@ -231,14 +333,24 @@ impl VirtualProvisioningCollectionUpdate { ::Changeset: QueryFragment + Send + 'static, { + let do_update = match update_kind { + UpdateKind::Insert(resource) => { + DoUpdate::new_for_insert(resource.id) + } + UpdateKind::Delete(id) => DoUpdate::new_for_delete(id), + }; let parent_silo = ParentSilo::new(project_id); let all_collections = AllCollections::new( project_id, &parent_silo, *crate::db::fixed_data::FLEET_ID, ); + let updated_collections = UpdatedProvisions::new(&all_collections, &do_update, values); + let quotas = Quotas::new(&parent_silo, update_kind); + let silo_provisioned = SiloProvisioned::new(&parent_silo); + let quota_check = QuotaCheck::new(&silo_provisioned, "as); // TODO: Do we want to select from "all_collections" instead? Seems more // idempotent; it'll work even when we don't update anything... @@ -251,6 +363,9 @@ impl VirtualProvisioningCollectionUpdate { let cte = CteBuilder::new() .add_subquery(parent_silo) .add_subquery(all_collections) + .add_subquery(quotas) + .add_subquery(silo_provisioned) + .add_subquery(quota_check) .add_subquery(do_update) .add_subquery(update) .add_subquery(updated_collections) @@ -273,8 +388,7 @@ impl VirtualProvisioningCollectionUpdate { provision.virtual_disk_bytes_provisioned = disk_byte_diff; Self::apply_update( - // We should insert the record if it does not already exist. - DoUpdate::new_for_insert(id), + UpdateKind::Insert(provision), // The query to actually insert the record. UnreferenceableSubquery( diesel::insert_into( @@ -305,8 +419,7 @@ impl VirtualProvisioningCollectionUpdate { use virtual_provisioning_resource::dsl as resource_dsl; Self::apply_update( - // We should delete the record if it exists. - DoUpdate::new_for_delete(id), + UpdateKind::Delete(id), // The query to actually delete the record. UnreferenceableSubquery( diesel::delete(resource_dsl::virtual_provisioning_resource) @@ -342,8 +455,7 @@ impl VirtualProvisioningCollectionUpdate { provision.ram_provisioned = ram_diff; Self::apply_update( - // We should insert the record if it does not already exist. - DoUpdate::new_for_insert(id), + UpdateKind::Insert(provision), // The query to actually insert the record. UnreferenceableSubquery( diesel::insert_into( @@ -378,8 +490,7 @@ impl VirtualProvisioningCollectionUpdate { use virtual_provisioning_resource::dsl as resource_dsl; Self::apply_update( - // We should delete the record if it exists. - DoUpdate::new_for_delete(id), + UpdateKind::Delete(id), // The query to actually delete the record. // // The filter condition here ensures that the provisioning record is From 8025899445a214c45c81670148c7271d99a01153 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 5 Dec 2023 21:10:39 -0500 Subject: [PATCH 11/47] Add quota checks for cpu, memory, storage --- .../virtual_provisioning_collection_update.rs | 56 +++++++++++++++---- 1 file changed, 45 insertions(+), 11 deletions(-) diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index bd84aa1681..67b484d936 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -237,19 +237,53 @@ struct QuotaCheck { } impl QuotaCheck { - fn new(silo_provisioned: &SiloProvisioned, quotas: &Quotas) -> Self { + fn new( + silo_provisioned: &SiloProvisioned, + quotas: &Quotas, + resource: VirtualProvisioningResource, + ) -> Self { + let has_sufficient_cpus = + quotas.query_source().select(quotas::cpus).single_value().ge( + silo_provisioned + .query_source() + .select(silo_provisioned::cpus_provisioned) + .single_value() + + resource.cpus_provisioned, + ); + + let has_sufficient_memory = + quotas.query_source().select(quotas::memory).single_value().ge( + silo_provisioned + .query_source() + .select(silo_provisioned::ram_provisioned) + .single_value() + + resource.ram_provisioned, + ); + + let has_sufficient_storage = + quotas.query_source().select(quotas::storage).single_value().ge( + silo_provisioned + .query_source() + .select(silo_provisioned::virtual_disk_bytes_provisioned) + .single_value() + + resource.virtual_disk_bytes_provisioned, + ); + Self { query: Box::new(diesel::select( (ExpressionAlias::new::( - TrueOrCastError::new(enough_cpus, NOT_ENOUGH_CPUS_SENTINEL) - .and(TrueOrCastError::new( - enough_memory, - NOT_ENOUGH_MEMORY_SENTINEL, - )) - .and(TrueOrCastError::new( - enough_storage, - NOT_ENOUGH_STORAGE_SENTINEL, - )), + TrueOrCastError::new( + has_sufficient_cpus, + NOT_ENOUGH_CPUS_SENTINEL, + ) + .and(TrueOrCastError::new( + has_sufficient_memory, + NOT_ENOUGH_MEMORY_SENTINEL, + )) + .and(TrueOrCastError::new( + has_sufficient_storage, + NOT_ENOUGH_STORAGE_SENTINEL, + )), )), )), } @@ -350,7 +384,7 @@ impl VirtualProvisioningCollectionUpdate { UpdatedProvisions::new(&all_collections, &do_update, values); let quotas = Quotas::new(&parent_silo, update_kind); let silo_provisioned = SiloProvisioned::new(&parent_silo); - let quota_check = QuotaCheck::new(&silo_provisioned, "as); + let quota_check = QuotaCheck::new(&silo_provisioned, "as, resource); // TODO: Do we want to select from "all_collections" instead? Seems more // idempotent; it'll work even when we don't update anything... From a4002ffd6f7e46677a6d3bff402dd1e934f43c91 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 5 Dec 2023 22:12:12 -0500 Subject: [PATCH 12/47] Join do_update and quota_check into a single CTE step --- .../virtual_provisioning_collection_update.rs | 9 +- .../virtual_provisioning_collection_update.rs | 137 ++++++++---------- 2 files changed, 63 insertions(+), 83 deletions(-) diff --git a/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs b/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs index 95eaafe8f4..c774466fdb 100644 --- a/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs @@ -46,12 +46,6 @@ table! { } } -table! { - quota_check (passed) { - passed -> Bool, - } -} - diesel::allow_tables_to_appear_in_same_query!(silo, parent_silo,); diesel::allow_tables_to_appear_in_same_query!( @@ -59,5 +53,6 @@ diesel::allow_tables_to_appear_in_same_query!( parent_silo, all_collections, do_update, - quotas + quotas, + silo_provisioned ); diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index 67b484d936..ef02c1e23a 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -24,8 +24,7 @@ use diesel::{ NullableExpressionMethods, QueryDsl, RunQueryDsl, SelectableHelper, }; use nexus_db_model::queries::virtual_provisioning_collection_update::{ - all_collections, do_update, parent_silo, quota_check, quotas, - silo_provisioned, + all_collections, do_update, parent_silo, quotas, silo_provisioned, }; use omicron_common::api::external; @@ -116,20 +115,65 @@ struct DoUpdate { } impl DoUpdate { - fn new_for_insert(id: uuid::Uuid) -> Self { + fn new_for_insert( + silo_provisioned: &SiloProvisioned, + quotas: &Quotas, + resource: VirtualProvisioningResource, + ) -> Self { use virtual_provisioning_resource::dsl; let not_allocted = dsl::virtual_provisioning_resource - .find(id) + .find(resource.id) .count() .single_value() .assume_not_null() .eq(0); + let has_sufficient_cpus = + quotas.query_source().select(quotas::cpus).single_value().ge( + silo_provisioned + .query_source() + .select(silo_provisioned::cpus_provisioned) + .single_value() + + resource.cpus_provisioned, + ); + + let has_sufficient_memory = + quotas.query_source().select(quotas::memory).single_value().ge( + silo_provisioned + .query_source() + .select(silo_provisioned::ram_provisioned) + .single_value() + + resource.ram_provisioned, + ); + + let has_sufficient_storage = + quotas.query_source().select(quotas::storage).single_value().ge( + silo_provisioned + .query_source() + .select(silo_provisioned::virtual_disk_bytes_provisioned) + .single_value() + + resource.virtual_disk_bytes_provisioned, + ); + Self { query: Box::new(diesel::select((ExpressionAlias::new::< do_update::update, - >(not_allocted),))), + >( + not_allocted + .and(TrueOrCastError::new( + has_sufficient_cpus, + NOT_ENOUGH_CPUS_SENTINEL, + )) + .and(TrueOrCastError::new( + has_sufficient_memory, + NOT_ENOUGH_MEMORY_SENTINEL, + )) + .and(TrueOrCastError::new( + has_sufficient_storage, + NOT_ENOUGH_STORAGE_SENTINEL, + )), + ),))), } } @@ -224,72 +268,13 @@ impl SiloProvisioned { Self { query: Box::new( dsl::virtual_provisioning_collection - .filter(dsl::id.eq(parent_silo::id)), + .filter(dsl::id.eq(parent_silo::id)) + .select(silo_provisioned::all_columns), ), } } } -#[derive(Subquery, QueryId)] -#[subquery(name = quota_check)] -struct QuotaCheck { - query: Box>, -} - -impl QuotaCheck { - fn new( - silo_provisioned: &SiloProvisioned, - quotas: &Quotas, - resource: VirtualProvisioningResource, - ) -> Self { - let has_sufficient_cpus = - quotas.query_source().select(quotas::cpus).single_value().ge( - silo_provisioned - .query_source() - .select(silo_provisioned::cpus_provisioned) - .single_value() - + resource.cpus_provisioned, - ); - - let has_sufficient_memory = - quotas.query_source().select(quotas::memory).single_value().ge( - silo_provisioned - .query_source() - .select(silo_provisioned::ram_provisioned) - .single_value() - + resource.ram_provisioned, - ); - - let has_sufficient_storage = - quotas.query_source().select(quotas::storage).single_value().ge( - silo_provisioned - .query_source() - .select(silo_provisioned::virtual_disk_bytes_provisioned) - .single_value() - + resource.virtual_disk_bytes_provisioned, - ); - - Self { - query: Box::new(diesel::select( - (ExpressionAlias::new::( - TrueOrCastError::new( - has_sufficient_cpus, - NOT_ENOUGH_CPUS_SENTINEL, - ) - .and(TrueOrCastError::new( - has_sufficient_memory, - NOT_ENOUGH_MEMORY_SENTINEL, - )) - .and(TrueOrCastError::new( - has_sufficient_storage, - NOT_ENOUGH_STORAGE_SENTINEL, - )), - )), - )), - } - } -} - // This structure wraps a query, such that it can be used within a CTE. // // It generates a name that can be used by the "CteBuilder", but does not @@ -367,12 +352,6 @@ impl VirtualProvisioningCollectionUpdate { ::Changeset: QueryFragment + Send + 'static, { - let do_update = match update_kind { - UpdateKind::Insert(resource) => { - DoUpdate::new_for_insert(resource.id) - } - UpdateKind::Delete(id) => DoUpdate::new_for_delete(id), - }; let parent_silo = ParentSilo::new(project_id); let all_collections = AllCollections::new( project_id, @@ -380,11 +359,18 @@ impl VirtualProvisioningCollectionUpdate { *crate::db::fixed_data::FLEET_ID, ); - let updated_collections = - UpdatedProvisions::new(&all_collections, &do_update, values); let quotas = Quotas::new(&parent_silo, update_kind); let silo_provisioned = SiloProvisioned::new(&parent_silo); - let quota_check = QuotaCheck::new(&silo_provisioned, "as, resource); + + let do_update = match update_kind { + UpdateKind::Insert(resource) => { + DoUpdate::new_for_insert(&silo_provisioned, "as, resource) + } + UpdateKind::Delete(id) => DoUpdate::new_for_delete(id), + }; + + let updated_collections = + UpdatedProvisions::new(&all_collections, &do_update, values); // TODO: Do we want to select from "all_collections" instead? Seems more // idempotent; it'll work even when we don't update anything... @@ -399,7 +385,6 @@ impl VirtualProvisioningCollectionUpdate { .add_subquery(all_collections) .add_subquery(quotas) .add_subquery(silo_provisioned) - .add_subquery(quota_check) .add_subquery(do_update) .add_subquery(update) .add_subquery(updated_collections) From 76c722f9cbf8b15430ef785768e6a4c2e56f3b03 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 5 Dec 2023 22:42:49 -0500 Subject: [PATCH 13/47] Try (and fail) to resolve CTE type issues --- .../virtual_provisioning_collection_update.rs | 2 ++ .../virtual_provisioning_collection_update.rs | 20 +++++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs b/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs index c774466fdb..124ffe4db6 100644 --- a/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-model/src/queries/virtual_provisioning_collection_update.rs @@ -8,6 +8,7 @@ //! for the construction of this query. use crate::schema::silo; +use crate::schema::silo_quotas; use crate::schema::virtual_provisioning_collection; table! { @@ -50,6 +51,7 @@ diesel::allow_tables_to_appear_in_same_query!(silo, parent_silo,); diesel::allow_tables_to_appear_in_same_query!( virtual_provisioning_collection, + silo_quotas, parent_silo, all_collections, do_update, diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index ef02c1e23a..9e6ef3e804 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -250,7 +250,18 @@ impl Quotas { use crate::db::schema::silo_quotas::dsl; Self { query: Box::new( - dsl::silo_quotas.filter(dsl::silo_id.eq(parent_silo::id)), + dsl::silo_quotas + .filter(dsl::silo_id.eq(parent_silo::id)) + .select(( + dsl::silo_id, + dsl::cpus, + ExpressionAlias::new::( + dsl::memory_bytes, + ), + ExpressionAlias::new::( + dsl::storage_bytes, + ), + )), ), } } @@ -269,7 +280,12 @@ impl SiloProvisioned { query: Box::new( dsl::virtual_provisioning_collection .filter(dsl::id.eq(parent_silo::id)) - .select(silo_provisioned::all_columns), + .select(( + dsl::id, + dsl::cpus_provisioned, + dsl::ram_provisioned, + dsl::virtual_disk_bytes_provisioned, + )), ), } } From f8e2afc83aac7bb175615783da2d4b8bd919957b Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 5 Dec 2023 23:01:08 -0500 Subject: [PATCH 14/47] Resolve CTE compliation issues... ??? --- .../virtual_provisioning_collection_update.rs | 79 ++++++++++++------- 1 file changed, 50 insertions(+), 29 deletions(-) diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index 9e6ef3e804..3246c75dac 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -20,7 +20,7 @@ use diesel::pg::Pg; use diesel::query_builder::{AstPass, Query, QueryFragment, QueryId}; use diesel::result::Error as DieselError; use diesel::{ - sql_types, CombineDsl, ExpressionMethods, IntoSql, + sql_types, BoolExpressionMethods, CombineDsl, ExpressionMethods, IntoSql, NullableExpressionMethods, QueryDsl, RunQueryDsl, SelectableHelper, }; use nexus_db_model::queries::virtual_provisioning_collection_update::{ @@ -129,32 +129,41 @@ impl DoUpdate { .assume_not_null() .eq(0); - let has_sufficient_cpus = - quotas.query_source().select(quotas::cpus).single_value().ge( - silo_provisioned - .query_source() - .select(silo_provisioned::cpus_provisioned) - .single_value() - + resource.cpus_provisioned, - ); - - let has_sufficient_memory = - quotas.query_source().select(quotas::memory).single_value().ge( - silo_provisioned - .query_source() - .select(silo_provisioned::ram_provisioned) - .single_value() - + resource.ram_provisioned, - ); - - let has_sufficient_storage = - quotas.query_source().select(quotas::storage).single_value().ge( - silo_provisioned - .query_source() - .select(silo_provisioned::virtual_disk_bytes_provisioned) - .single_value() - + resource.virtual_disk_bytes_provisioned, - ); + let has_sufficient_cpus = quotas + .query_source() + .select(quotas::cpus) + .single_value() + .assume_not_null() + .ge(silo_provisioned + .query_source() + .select(silo_provisioned::cpus_provisioned) + .single_value() + .assume_not_null() + + resource.cpus_provisioned.into()); + + let has_sufficient_memory = quotas + .query_source() + .select(quotas::memory) + .single_value() + .assume_not_null() + .ge(silo_provisioned + .query_source() + .select(silo_provisioned::ram_provisioned) + .single_value() + .assume_not_null() + + resource.ram_provisioned.into()); + + let has_sufficient_storage = quotas + .query_source() + .select(quotas::storage) + .single_value() + .assume_not_null() + .ge(silo_provisioned + .query_source() + .select(silo_provisioned::virtual_disk_bytes_provisioned) + .single_value() + .assume_not_null() + + resource.virtual_disk_bytes_provisioned.into()); Self { query: Box::new(diesel::select((ExpressionAlias::new::< @@ -251,7 +260,13 @@ impl Quotas { Self { query: Box::new( dsl::silo_quotas - .filter(dsl::silo_id.eq(parent_silo::id)) + .filter( + dsl::silo_id.eq(parent_silo + .query_source() + .select(parent_silo::id) + .single_value() + .assume_not_null()), + ) .select(( dsl::silo_id, dsl::cpus, @@ -279,7 +294,13 @@ impl SiloProvisioned { Self { query: Box::new( dsl::virtual_provisioning_collection - .filter(dsl::id.eq(parent_silo::id)) + .filter( + dsl::id.eq(parent_silo + .query_source() + .select(parent_silo::id) + .single_value() + .assume_not_null()), + ) .select(( dsl::id, dsl::cpus_provisioned, From 3d4ebe211066a156dd156a97550423f51ffb9c5d Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 5 Dec 2023 23:37:17 -0500 Subject: [PATCH 15/47] Convert quota detlas to sql types; fix borrow issues --- .../virtual_provisioning_collection_update.rs | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index 3246c75dac..41c50bd593 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -122,6 +122,14 @@ impl DoUpdate { ) -> Self { use virtual_provisioning_resource::dsl; + let cpus_provisioned_delta = + resource.cpus_provisioned.into_sql::(); + let memory_provisioned_delta = + i64::from(resource.ram_provisioned).into_sql::(); + let storage_provisioned_delta = + i64::from(resource.virtual_disk_bytes_provisioned) + .into_sql::(); + let not_allocted = dsl::virtual_provisioning_resource .find(resource.id) .count() @@ -139,7 +147,7 @@ impl DoUpdate { .select(silo_provisioned::cpus_provisioned) .single_value() .assume_not_null() - + resource.cpus_provisioned.into()); + + cpus_provisioned_delta); let has_sufficient_memory = quotas .query_source() @@ -151,7 +159,7 @@ impl DoUpdate { .select(silo_provisioned::ram_provisioned) .single_value() .assume_not_null() - + resource.ram_provisioned.into()); + + memory_provisioned_delta); let has_sufficient_storage = quotas .query_source() @@ -163,7 +171,7 @@ impl DoUpdate { .select(silo_provisioned::virtual_disk_bytes_provisioned) .single_value() .assume_not_null() - + resource.virtual_disk_bytes_provisioned.into()); + + storage_provisioned_delta); Self { query: Box::new(diesel::select((ExpressionAlias::new::< @@ -255,7 +263,8 @@ struct Quotas { } impl Quotas { - fn new(parent_silo: &ParentSilo, update_kind: UpdateKind) -> Self { + // TODO: We could potentially skip this in cases where we know we're removing a resource instead of inserting + fn new(parent_silo: &ParentSilo) -> Self { use crate::db::schema::silo_quotas::dsl; Self { query: Box::new( @@ -396,7 +405,7 @@ impl VirtualProvisioningCollectionUpdate { *crate::db::fixed_data::FLEET_ID, ); - let quotas = Quotas::new(&parent_silo, update_kind); + let quotas = Quotas::new(&parent_silo); let silo_provisioned = SiloProvisioned::new(&parent_silo); let do_update = match update_kind { @@ -444,7 +453,7 @@ impl VirtualProvisioningCollectionUpdate { provision.virtual_disk_bytes_provisioned = disk_byte_diff; Self::apply_update( - UpdateKind::Insert(provision), + UpdateKind::Insert(provision.clone()), // The query to actually insert the record. UnreferenceableSubquery( diesel::insert_into( @@ -511,7 +520,7 @@ impl VirtualProvisioningCollectionUpdate { provision.ram_provisioned = ram_diff; Self::apply_update( - UpdateKind::Insert(provision), + UpdateKind::Insert(provision.clone()), // The query to actually insert the record. UnreferenceableSubquery( diesel::insert_into( From dcf974ea4750cf384eea1e775f38d2562899fb96 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Wed, 6 Dec 2023 00:00:01 -0500 Subject: [PATCH 16/47] Remove a todo --- nexus/src/app/rack.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 9f63e312ae..39c0c6f05e 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -203,7 +203,9 @@ impl super::Nexus { name: request.recovery_silo.silo_name, description: "built-in recovery Silo".to_string(), }, - // TODO: Should the recovery silo have a quota? If so, what should the defaults be? + // The recovery silo is initialized with no allocated capacity given it's + // not intended to be used to deploy workloads. Operators can add capacity + // after the fact if they want to use it for that purpose. quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: SiloIdentityMode::LocalOnly, From 83c98724b1b410a86f0fd75e6a77d3d2c63e8fa7 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Wed, 6 Dec 2023 00:10:29 -0500 Subject: [PATCH 17/47] Wire up quota limit error handling --- .../db/datastore/virtual_provisioning_collection.rs | 10 ++++++---- .../queries/virtual_provisioning_collection_update.rs | 3 +++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs b/nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs index 230c3941ff..348d277ddf 100644 --- a/nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs +++ b/nexus/db-queries/src/db/datastore/virtual_provisioning_collection.rs @@ -195,7 +195,9 @@ impl DataStore { ) .get_results_async(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + .map_err(|e| { + crate::db::queries::virtual_provisioning_collection_update::from_diesel(e) + })?; self.virtual_provisioning_collection_producer .append_disk_metrics(&provisions)?; Ok(provisions) @@ -249,7 +251,7 @@ impl DataStore { ) .get_results_async(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + .map_err(|e| crate::db::queries::virtual_provisioning_collection_update::from_diesel(e))?; self.virtual_provisioning_collection_producer .append_disk_metrics(&provisions)?; Ok(provisions) @@ -270,7 +272,7 @@ impl DataStore { ) .get_results_async(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + .map_err(|e| crate::db::queries::virtual_provisioning_collection_update::from_diesel(e))?; self.virtual_provisioning_collection_producer .append_cpu_metrics(&provisions)?; Ok(provisions) @@ -300,7 +302,7 @@ impl DataStore { ) .get_results_async(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + .map_err(|e| crate::db::queries::virtual_provisioning_collection_update::from_diesel(e))?; self.virtual_provisioning_collection_producer .append_cpu_metrics(&provisions)?; Ok(provisions) diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index 41c50bd593..c55ed0bd68 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -32,6 +32,9 @@ const NOT_ENOUGH_CPUS_SENTINEL: &'static str = "Not enough cpus"; const NOT_ENOUGH_MEMORY_SENTINEL: &'static str = "Not enough memory"; const NOT_ENOUGH_STORAGE_SENTINEL: &'static str = "Not enough storage"; +/// Translates a generic pool error to an external error based +/// on messages which may be emitted when provisioning virtual resources +/// such as instances and disks. pub fn from_diesel(e: DieselError) -> external::Error { use crate::db::error; From 7e779a0923b9f679841773a5e3392a3bb775f31e Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Wed, 6 Dec 2023 10:14:15 -0500 Subject: [PATCH 18/47] Fix missing quota specifier --- nexus/db-queries/src/db/datastore/rack.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index a69386cfd0..6200b307a3 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -912,6 +912,7 @@ mod test { name: "test-silo".parse().unwrap(), description: String::new(), }, + quotas: external_params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: SiloIdentityMode::LocalOnly, admin_group_name: None, From 9859e12b86b4f73fd4e0592787f480972d83c8d1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 6 Dec 2023 13:59:40 -0800 Subject: [PATCH 19/47] SELECT got you down? Try JOIN today! --- .../virtual_provisioning_collection_update.rs | 19 ++++----- nexus/preprocessed_configs/config.xml | 41 +++++++++++++++++++ 2 files changed, 49 insertions(+), 11 deletions(-) create mode 100644 nexus/preprocessed_configs/config.xml diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index c55ed0bd68..7cb2f340d4 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -21,7 +21,8 @@ use diesel::query_builder::{AstPass, Query, QueryFragment, QueryId}; use diesel::result::Error as DieselError; use diesel::{ sql_types, BoolExpressionMethods, CombineDsl, ExpressionMethods, IntoSql, - NullableExpressionMethods, QueryDsl, RunQueryDsl, SelectableHelper, + JoinOnDsl, NullableExpressionMethods, QueryDsl, RunQueryDsl, + SelectableHelper, }; use nexus_db_model::queries::virtual_provisioning_collection_update::{ all_collections, do_update, parent_silo, quotas, silo_provisioned, @@ -272,12 +273,10 @@ impl Quotas { Self { query: Box::new( dsl::silo_quotas - .filter( - dsl::silo_id.eq(parent_silo + .inner_join( + parent_silo .query_source() - .select(parent_silo::id) - .single_value() - .assume_not_null()), + .on(dsl::silo_id.eq(parent_silo::id)), ) .select(( dsl::silo_id, @@ -306,12 +305,10 @@ impl SiloProvisioned { Self { query: Box::new( dsl::virtual_provisioning_collection - .filter( - dsl::id.eq(parent_silo + .inner_join( + parent_silo .query_source() - .select(parent_silo::id) - .single_value() - .assume_not_null()), + .on(dsl::id.eq(parent_silo::id)), ) .select(( dsl::id, diff --git a/nexus/preprocessed_configs/config.xml b/nexus/preprocessed_configs/config.xml new file mode 100644 index 0000000000..9b13f12aea --- /dev/null +++ b/nexus/preprocessed_configs/config.xml @@ -0,0 +1,41 @@ + + + + + trace + true + + + 8123 + 9000 + 9004 + + ./ + + true + + + + + + + ::/0 + + + default + default + 1 + + + + + + + + + + + \ No newline at end of file From 45eb0efaae95afb000a37cfd54aec0e80dac4857 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 6 Dec 2023 14:00:18 -0800 Subject: [PATCH 20/47] Whoops, removing test cruft --- nexus/preprocessed_configs/config.xml | 41 --------------------------- 1 file changed, 41 deletions(-) delete mode 100644 nexus/preprocessed_configs/config.xml diff --git a/nexus/preprocessed_configs/config.xml b/nexus/preprocessed_configs/config.xml deleted file mode 100644 index 9b13f12aea..0000000000 --- a/nexus/preprocessed_configs/config.xml +++ /dev/null @@ -1,41 +0,0 @@ - - - - - trace - true - - - 8123 - 9000 - 9004 - - ./ - - true - - - - - - - ::/0 - - - default - default - 1 - - - - - - - - - - - \ No newline at end of file From 55ff0b2ed7d651c9eb7a2dd3eb6457ba4707d1c6 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Wed, 6 Dec 2023 21:01:35 -0500 Subject: [PATCH 21/47] Only perform quota checks if allocating resources --- .../queries/virtual_provisioning_collection_update.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index 7cb2f340d4..0d44b5f45c 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -183,15 +183,19 @@ impl DoUpdate { >( not_allocted .and(TrueOrCastError::new( - has_sufficient_cpus, + cpus_provisioned_delta.eq(0).or(has_sufficient_cpus), NOT_ENOUGH_CPUS_SENTINEL, )) .and(TrueOrCastError::new( - has_sufficient_memory, + memory_provisioned_delta + .eq(0) + .or(has_sufficient_memory), NOT_ENOUGH_MEMORY_SENTINEL, )) .and(TrueOrCastError::new( - has_sufficient_storage, + storage_provisioned_delta + .eq(0) + .or(has_sufficient_storage), NOT_ENOUGH_STORAGE_SENTINEL, )), ),))), From 2ee6d36399df55e9a3936dd78bbe717999812dfc Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 7 Dec 2023 00:47:56 -0500 Subject: [PATCH 22/47] Adjust some quotas in hopes of not failing all the tests --- nexus/db-queries/src/db/datastore/rack.rs | 8 ++++++-- nexus/db-queries/src/db/fixed_data/silo.rs | 9 ++++++--- nexus/test-utils/src/resource_helpers.rs | 4 ++-- nexus/tests/integration_tests/endpoints.rs | 4 ++-- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 6200b307a3..3ea785aa4c 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -874,7 +874,7 @@ mod test { }; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::{ - IdentityMetadataCreateParams, MacAddr, + ByteCount, IdentityMetadataCreateParams, MacAddr, }; use omicron_common::api::internal::shared::SourceNatConfig; use omicron_common::nexus_config::NUM_INITIAL_RESERVED_IP_ADDRESSES; @@ -912,7 +912,11 @@ mod test { name: "test-silo".parse().unwrap(), description: String::new(), }, - quotas: external_params::SiloQuotasCreate::empty(), + quotas: external_params::SiloQuotasCreate { + cpus: 128, + memory: ByteCount::from_gibibytes_u32(1000), + storage: ByteCount::from_gibibytes_u32(1000000), + }, discoverable: false, identity_mode: SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/db-queries/src/db/fixed_data/silo.rs b/nexus/db-queries/src/db/fixed_data/silo.rs index d02db783a3..70a7a79ab3 100644 --- a/nexus/db-queries/src/db/fixed_data/silo.rs +++ b/nexus/db-queries/src/db/fixed_data/silo.rs @@ -5,7 +5,7 @@ use crate::db; use lazy_static::lazy_static; use nexus_types::external_api::{params, shared}; -use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::{ByteCount, IdentityMetadataCreateParams}; lazy_static! { pub static ref SILO_ID: uuid::Uuid = "001de000-5110-4000-8000-000000000000" @@ -24,8 +24,11 @@ lazy_static! { name: "default-silo".parse().unwrap(), description: "default silo".to_string(), }, - // TODO: Should the default silo have a quota? If so, what should the defaults be? - quotas: params::SiloQuotasCreate::empty(), + quotas: params::SiloQuotasCreate { + cpus: 128, + memory: ByteCount::from_gibibytes_u32(1000), + storage: ByteCount::from_gibibytes_u32(1000000), + }, discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 2ec41830bc..01a6415503 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -263,9 +263,9 @@ pub async fn create_silo( description: "a silo".to_string(), }, quotas: params::SiloQuotasCreate { - cpus: 36, + cpus: 128, memory: ByteCount::from_gibibytes_u32(1000), - storage: ByteCount::from_gibibytes_u32(100000), + storage: ByteCount::from_gibibytes_u32(1000000), }, discoverable, identity_mode, diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index f5450fc633..443ffb2d68 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -93,8 +93,8 @@ lazy_static! { }, quotas: params::SiloQuotasCreate { cpus: 100, - memory: ByteCount::from_gibibytes_u32(100), - storage: ByteCount::from_gibibytes_u32(100), + memory: ByteCount::from_gibibytes_u32(1000), + storage: ByteCount::from_gibibytes_u32(100000), }, discoverable: true, identity_mode: shared::SiloIdentityMode::SamlJit, From 4c58f9eae13e47e535595366a36d345833fc6a25 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 7 Dec 2023 02:19:26 -0500 Subject: [PATCH 23/47] Add WIP integration test setup for quotas --- nexus/tests/integration_tests/mod.rs | 1 + nexus/tests/integration_tests/quotas.rs | 159 ++++++++++++++++++++++++ 2 files changed, 160 insertions(+) create mode 100644 nexus/tests/integration_tests/quotas.rs diff --git a/nexus/tests/integration_tests/mod.rs b/nexus/tests/integration_tests/mod.rs index 4d7b41cfa8..58d2fcfbcc 100644 --- a/nexus/tests/integration_tests/mod.rs +++ b/nexus/tests/integration_tests/mod.rs @@ -23,6 +23,7 @@ mod oximeter; mod pantry; mod password_login; mod projects; +mod quotas; mod rack; mod role_assignments; mod roles_builtin; diff --git a/nexus/tests/integration_tests/quotas.rs b/nexus/tests/integration_tests/quotas.rs new file mode 100644 index 0000000000..9c6f581277 --- /dev/null +++ b/nexus/tests/integration_tests/quotas.rs @@ -0,0 +1,159 @@ +use anyhow::Error; +use dropshot::test_util::ClientTestContext; +use nexus_test_utils::http_testing::AuthnMode; +use nexus_test_utils::http_testing::NexusRequest; +use nexus_test_utils::http_testing::TestResponse; +use nexus_test_utils::resource_helpers::create_local_user; +use nexus_test_utils::resource_helpers::grant_iam; +use nexus_test_utils::resource_helpers::object_create; +use nexus_test_utils_macros::nexus_test; +use nexus_types::external_api::params; +use nexus_types::external_api::shared; +use nexus_types::external_api::shared::SiloRole; +use omicron_common::api::external::ByteCount; +use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::InstanceCpuCount; + +type ControlPlaneTestContext = + nexus_test_utils::ControlPlaneTestContext; + +struct ResourceAllocator { + auth: AuthnMode, +} + +impl ResourceAllocator { + fn new(auth: AuthnMode) -> Self { + Self { auth } + } + + async fn provision_instance( + &self, + client: &ClientTestContext, + name: &str, + cpus: u16, + memory: u32, + ) -> Result { + NexusRequest::objects_post( + client, + "/v1/instances?project=project", + ¶ms::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: name.parse().unwrap(), + description: "".into(), + }, + ncpus: InstanceCpuCount(cpus), + memory: ByteCount::from_gibibytes_u32(memory), + hostname: "host".to_string(), + user_data: b"#cloud-config\nsystem_info:\n default_user:\n name: oxide" + .to_vec(), + network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, + external_ips: Vec::::new(), + disks: Vec::::new(), + start: true, + }, + ) + .authn_as(self.auth.clone()) + .execute() + .await + } + + async fn provision_disk( + &self, + client: &ClientTestContext, + name: &str, + size: u32, + ) -> Result { + NexusRequest::objects_post( + client, + "/v1/disks?project=project", + ¶ms::DiskCreate { + identity: IdentityMetadataCreateParams { + name: name.parse().unwrap(), + description: "".into(), + }, + size: ByteCount::from_gibibytes_u32(size), + disk_source: params::DiskSource::Blank { + block_size: params::BlockSize::try_from(512).unwrap(), + }, + }, + ) + .authn_as(self.auth.clone()) + .execute() + .await + } +} + +async fn setup_silo_with_quota( + client: &ClientTestContext, + silo_name: &str, + quotas: params::SiloQuotasCreate, +) -> ResourceAllocator { + let silo = object_create( + client, + "/v1/system/silos", + ¶ms::SiloCreate { + identity: IdentityMetadataCreateParams { + name: silo_name.parse().unwrap(), + description: "".into(), + }, + quotas, + discoverable: true, + identity_mode: shared::SiloIdentityMode::LocalOnly, + admin_group_name: None, + tls_certificates: vec![], + mapped_fleet_roles: Default::default(), + }, + ) + .await; + + // Create a silo user + let user = create_local_user( + client, + &silo, + &"user".parse().unwrap(), + params::UserPassword::LoginDisallowed, + ) + .await; + + // Make silo admin + grant_iam( + client, + format!("/v1/system/silos/{}", silo_name).as_str(), + SiloRole::Admin, + user.id, + AuthnMode::PrivilegedUser, + ) + .await; + + let auth_mode = AuthnMode::SiloUser(user.id); + + NexusRequest::objects_post( + client, + "/v1/projects", + ¶ms::ProjectCreate { + identity: IdentityMetadataCreateParams { + name: "project".parse().unwrap(), + description: "".into(), + }, + }, + ) + .authn_as(auth_mode.clone()) + .execute() + .await?; + + ResourceAllocator::new(auth_mode) +} + +#[nexus_test] +async fn test_quotas(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + // let nexus = &cptestctx.server.apictx().nexus; + + let system = setup_silo_with_quota( + &client, + "rationed_silo", + params::SiloQuotasCreate::empty(), + ) + .await; + system.provision_instance(client, "instance", 1, 1).await; +} From 3b014c80aedb7ecb59d4d04878754564861e77c4 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 7 Dec 2023 17:59:16 -0500 Subject: [PATCH 24/47] Show that quota checks are failing when they shouldn't --- nexus/tests/integration_tests/quotas.rs | 143 +++++++++++++++++++++++- 1 file changed, 138 insertions(+), 5 deletions(-) diff --git a/nexus/tests/integration_tests/quotas.rs b/nexus/tests/integration_tests/quotas.rs index 9c6f581277..ba0aba9008 100644 --- a/nexus/tests/integration_tests/quotas.rs +++ b/nexus/tests/integration_tests/quotas.rs @@ -1,18 +1,23 @@ use anyhow::Error; use dropshot::test_util::ClientTestContext; +use http::Method; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; +use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::http_testing::TestResponse; use nexus_test_utils::resource_helpers::create_local_user; use nexus_test_utils::resource_helpers::grant_iam; use nexus_test_utils::resource_helpers::object_create; +use nexus_test_utils::resource_helpers::populate_ip_pool; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::params; use nexus_types::external_api::shared; use nexus_types::external_api::shared::SiloRole; +use nexus_types::external_api::views::SiloQuotas; use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::InstanceCpuCount; +use semver::Op; type ControlPlaneTestContext = nexus_test_utils::ControlPlaneTestContext; @@ -26,6 +31,34 @@ impl ResourceAllocator { Self { auth } } + async fn set_quotas( + &self, + client: &ClientTestContext, + quotas: params::SiloQuotasUpdate, + ) -> Result { + NexusRequest::object_put( + client, + "/v1/system/silos/quota-test-silo/quotas", + Some("as), + ) + .authn_as(self.auth.clone()) + .execute() + .await + } + + async fn get_quotas(&self, client: &ClientTestContext) -> SiloQuotas { + NexusRequest::object_get( + client, + "/v1/system/silos/quota-test-silo/quotas", + ) + .authn_as(self.auth.clone()) + .execute() + .await + .expect("failed to fetch quotas") + .parsed_body() + .expect("failed to parse quotas") + } + async fn provision_instance( &self, client: &ClientTestContext, @@ -49,12 +82,54 @@ impl ResourceAllocator { network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, external_ips: Vec::::new(), disks: Vec::::new(), - start: true, + start: false, }, ) .authn_as(self.auth.clone()) .execute() .await + .expect("Instance should be created regardless of quotas"); + + NexusRequest::new( + RequestBuilder::new( + client, + Method::POST, + format!("/v1/instances/{}/start?project=project", name) + .as_str(), + ) + .body(None as Option<&serde_json::Value>), + ) + .authn_as(self.auth.clone()) + .execute() + .await + } + + async fn cleanup_instance( + &self, + client: &ClientTestContext, + name: &str, + ) -> TestResponse { + // Stop instance if it's started... can probably ignore errors here + NexusRequest::new( + RequestBuilder::new( + client, + Method::POST, + format!("/v1/instances/{}/stop?project=project", name).as_str(), + ) + .body(None as Option<&serde_json::Value>), + ) + .authn_as(self.auth.clone()) + .execute() + .await; + + NexusRequest::object_delete( + client, + format!("/v1/instances/{}?project=project", name).as_str(), + ) + .authn_as(self.auth.clone()) + .execute() + .await + .expect("failed to delete instance") } async fn provision_disk( @@ -106,6 +181,8 @@ async fn setup_silo_with_quota( ) .await; + populate_ip_pool(&client, "default", None).await; + // Create a silo user let user = create_local_user( client, @@ -139,7 +216,8 @@ async fn setup_silo_with_quota( ) .authn_as(auth_mode.clone()) .execute() - .await?; + .await + .unwrap(); ResourceAllocator::new(auth_mode) } @@ -147,13 +225,68 @@ async fn setup_silo_with_quota( #[nexus_test] async fn test_quotas(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; - // let nexus = &cptestctx.server.apictx().nexus; + let nexus = &cptestctx.server.apictx().nexus; let system = setup_silo_with_quota( &client, - "rationed_silo", + "quota-test-silo", params::SiloQuotasCreate::empty(), ) .await; - system.provision_instance(client, "instance", 1, 1).await; + + // Ensure trying to provision an instance with empty quotas fails + system + .provision_instance(client, "instance", 1, 1) + .await + .expect_err("should've failed with insufficient CPU quota"); + system.cleanup_instance(client, "instance").await; + + // Up the storage quota + system + .set_quotas( + client, + params::SiloQuotasUpdate { + cpus: None, + memory: None, + storage: Some(ByteCount::from_gibibytes_u32(100)), + }, + ) + .await + .expect("failed to set quotas"); + + let quotas = system.get_quotas(client).await; + assert_eq!(quotas.cpus, 0); + assert_eq!(quotas.memory, ByteCount::from(0)); + assert_eq!(quotas.storage, ByteCount::from_gibibytes_u32(100)); + + // Ensure trying to provision an instance still fails with only storage quota updated + system + .provision_instance(client, "instance", 1, 1) + .await + .expect_err("should've failed with insufficient CPU quota"); + system.cleanup_instance(client, "instance").await; + + // Up the CPU, memory quotas + system + .set_quotas( + client, + params::SiloQuotasUpdate { + cpus: Some(4), + memory: Some(ByteCount::from_gibibytes_u32(100)), + storage: Some(ByteCount::from_gibibytes_u32(0)), + }, + ) + .await + .expect("failed to set quotas"); + + let quotas = system.get_quotas(client).await; + assert_eq!(quotas.cpus, 4); + assert_eq!(quotas.memory, ByteCount::from_gibibytes_u32(100)); + assert_eq!(quotas.storage, ByteCount::from(0)); + + // Allocating instance should now succeed + system + .provision_instance(client, "instance", 2, 80) + .await + .expect("Instance should've had enough resources to be provisioned"); } From bd950bc6ab42ad8bbc895a4bb272f02e5af17818 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 7 Dec 2023 23:29:30 -0500 Subject: [PATCH 25/47] Ensure default silo gets constructed w/ a quota --- nexus/db-model/src/quota.rs | 10 ++++++++ nexus/db-queries/src/db/datastore/silo.rs | 29 +++++++++++++++++----- nexus/db-queries/src/db/fixed_data/silo.rs | 10 +++----- nexus/types/src/external_api/params.rs | 10 ++++++++ 4 files changed, 47 insertions(+), 12 deletions(-) diff --git a/nexus/db-model/src/quota.rs b/nexus/db-model/src/quota.rs index b42456b330..adf0612e8f 100644 --- a/nexus/db-model/src/quota.rs +++ b/nexus/db-model/src/quota.rs @@ -49,6 +49,16 @@ impl SiloQuotas { storage, } } + + pub fn half_rack(silo_id: Uuid) -> Self { + let count = params::SiloQuotasCreate::half_rack(); + Self::new( + silo_id, + count.cpus, + count.memory.into(), + count.storage.into(), + ) + } } impl From for views::SiloQuotas { diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index a42702e95a..bf8b99451d 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -62,13 +62,30 @@ impl DataStore { debug!(opctx.log, "attempting to create built-in silos"); use db::schema::silo::dsl; - let count = diesel::insert_into(dsl::silo) - .values([&*DEFAULT_SILO, &*INTERNAL_SILO]) - .on_conflict(dsl::id) - .do_nothing() - .execute_async(&*self.pool_connection_authorized(opctx).await?) + use db::schema::silo_quotas::dsl as quotas_dsl; + let count = self + .pool_connection_authorized(opctx) + .await? + .transaction_async(|conn| async move { + diesel::insert_into(quotas_dsl::silo_quotas) + .values(SiloQuotas::half_rack(DEFAULT_SILO.id())) + .on_conflict(quotas_dsl::silo_id) + .do_nothing() + .execute_async(&conn) + .await + .map_err(TransactionError::CustomError) + .unwrap(); + diesel::insert_into(dsl::silo) + .values([&*DEFAULT_SILO, &*INTERNAL_SILO]) + .on_conflict(dsl::id) + .do_nothing() + .execute_async(&conn) + .await + .map_err(TransactionError::CustomError) + }) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + .unwrap(); + info!(opctx.log, "created {} built-in silos", count); self.virtual_provisioning_collection_create( diff --git a/nexus/db-queries/src/db/fixed_data/silo.rs b/nexus/db-queries/src/db/fixed_data/silo.rs index 70a7a79ab3..6eba849ee3 100644 --- a/nexus/db-queries/src/db/fixed_data/silo.rs +++ b/nexus/db-queries/src/db/fixed_data/silo.rs @@ -5,7 +5,7 @@ use crate::db; use lazy_static::lazy_static; use nexus_types::external_api::{params, shared}; -use omicron_common::api::external::{ByteCount, IdentityMetadataCreateParams}; +use omicron_common::api::external::IdentityMetadataCreateParams; lazy_static! { pub static ref SILO_ID: uuid::Uuid = "001de000-5110-4000-8000-000000000000" @@ -24,11 +24,9 @@ lazy_static! { name: "default-silo".parse().unwrap(), description: "default silo".to_string(), }, - quotas: params::SiloQuotasCreate { - cpus: 128, - memory: ByteCount::from_gibibytes_u32(1000), - storage: ByteCount::from_gibibytes_u32(1000000), - }, + // This quota is actually _unused_ because the default silo + // isn't constructed in the same way a normal silo would be. + quotas: params::SiloQuotasCreate::empty(), discoverable: false, identity_mode: shared::SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 0604fa72d6..7a88d4493a 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -306,6 +306,16 @@ impl SiloQuotasCreate { storage: ByteCount::from(0), } } + + /// 30% of CPUs and memory reserved for internal usage. + /// Storage calculated at (total / 3.5) to account for redundancy / bookkeeping. + pub fn half_rack() -> Self { + Self { + cpus: 90, + memory: ByteCount::from_gibibytes_u32(708), + storage: ByteCount::from_gibibytes_u32(850), + } + } } #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] From 4b0d236bb3df71d2e15bde2659b8f514e4e971d9 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 8 Dec 2023 00:07:06 -0500 Subject: [PATCH 26/47] Flesh out basic quota tests --- nexus/tests/integration_tests/quotas.rs | 56 ++++++++++--------------- 1 file changed, 22 insertions(+), 34 deletions(-) diff --git a/nexus/tests/integration_tests/quotas.rs b/nexus/tests/integration_tests/quotas.rs index ba0aba9008..0fe6ebdcf2 100644 --- a/nexus/tests/integration_tests/quotas.rs +++ b/nexus/tests/integration_tests/quotas.rs @@ -9,6 +9,7 @@ use nexus_test_utils::resource_helpers::create_local_user; use nexus_test_utils::resource_helpers::grant_iam; use nexus_test_utils::resource_helpers::object_create; use nexus_test_utils::resource_helpers::populate_ip_pool; +use nexus_test_utils::resource_helpers::DiskTest; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::params; use nexus_types::external_api::shared; @@ -17,7 +18,6 @@ use nexus_types::external_api::views::SiloQuotas; use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::InstanceCpuCount; -use semver::Op; type ControlPlaneTestContext = nexus_test_utils::ControlPlaneTestContext; @@ -97,7 +97,8 @@ impl ResourceAllocator { format!("/v1/instances/{}/start?project=project", name) .as_str(), ) - .body(None as Option<&serde_json::Value>), + .body(None as Option<&serde_json::Value>) + .expect_status(Some(http::StatusCode::ACCEPTED)), ) .authn_as(self.auth.clone()) .execute() @@ -120,7 +121,8 @@ impl ResourceAllocator { ) .authn_as(self.auth.clone()) .execute() - .await; + .await + .expect("failed to stop instance"); NexusRequest::object_delete( client, @@ -225,7 +227,9 @@ async fn setup_silo_with_quota( #[nexus_test] async fn test_quotas(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; - let nexus = &cptestctx.server.apictx().nexus; + + // Simulate space for disks + DiskTest::new(&cptestctx).await; let system = setup_silo_with_quota( &client, @@ -241,39 +245,14 @@ async fn test_quotas(cptestctx: &ControlPlaneTestContext) { .expect_err("should've failed with insufficient CPU quota"); system.cleanup_instance(client, "instance").await; - // Up the storage quota - system - .set_quotas( - client, - params::SiloQuotasUpdate { - cpus: None, - memory: None, - storage: Some(ByteCount::from_gibibytes_u32(100)), - }, - ) - .await - .expect("failed to set quotas"); - - let quotas = system.get_quotas(client).await; - assert_eq!(quotas.cpus, 0); - assert_eq!(quotas.memory, ByteCount::from(0)); - assert_eq!(quotas.storage, ByteCount::from_gibibytes_u32(100)); - - // Ensure trying to provision an instance still fails with only storage quota updated - system - .provision_instance(client, "instance", 1, 1) - .await - .expect_err("should've failed with insufficient CPU quota"); - system.cleanup_instance(client, "instance").await; - // Up the CPU, memory quotas system .set_quotas( client, params::SiloQuotasUpdate { cpus: Some(4), - memory: Some(ByteCount::from_gibibytes_u32(100)), - storage: Some(ByteCount::from_gibibytes_u32(0)), + memory: Some(ByteCount::from_gibibytes_u32(15)), + storage: Some(ByteCount::from_gibibytes_u32(2)), }, ) .await @@ -281,12 +260,21 @@ async fn test_quotas(cptestctx: &ControlPlaneTestContext) { let quotas = system.get_quotas(client).await; assert_eq!(quotas.cpus, 4); - assert_eq!(quotas.memory, ByteCount::from_gibibytes_u32(100)); - assert_eq!(quotas.storage, ByteCount::from(0)); + assert_eq!(quotas.memory, ByteCount::from_gibibytes_u32(15)); + assert_eq!(quotas.storage, ByteCount::from_gibibytes_u32(2)); // Allocating instance should now succeed system - .provision_instance(client, "instance", 2, 80) + .provision_instance(client, "instance", 2, 10) .await .expect("Instance should've had enough resources to be provisioned"); + + system.provision_disk(client, "disk", 3).await.expect_err( + "Disk should not be provisioned because it exceeds the quota", + ); + + system + .provision_disk(client, "disk", 1) + .await + .expect("Disk should be provisioned"); } From 0ad4340dcd3ed9de5c693e822e957c2fd215a6dd Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 8 Dec 2023 00:17:51 -0500 Subject: [PATCH 27/47] Remove unnecessary transaction --- nexus/db-queries/src/db/datastore/quota.rs | 32 ++++++---------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/quota.rs b/nexus/db-queries/src/db/datastore/quota.rs index 894fd94ff3..148948c1c7 100644 --- a/nexus/db-queries/src/db/datastore/quota.rs +++ b/nexus/db-queries/src/db/datastore/quota.rs @@ -6,8 +6,6 @@ use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::pagination::paginated; use crate::db::pool::DbConnection; -use crate::db::TransactionError; -use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; use diesel::prelude::*; use nexus_db_model::SiloQuotas; @@ -33,32 +31,20 @@ impl DataStore { let silo_id = authz_silo.id(); use db::schema::silo_quotas::dsl; - let result = conn - .transaction_async(|c| async move { - diesel::insert_into(dsl::silo_quotas) - .values(quotas) - .execute_async(&c) - .await - .map_err(TransactionError::CustomError) - }) - .await; - - match result { - Ok(_) => Ok(()), - Err(TransactionError::CustomError(e)) => { - // TODO: Is this the right error handler? - Err(public_error_from_diesel(e, ErrorHandler::Server)) - } - Err(TransactionError::Database(e)) => { - Err(public_error_from_diesel( + diesel::insert_into(dsl::silo_quotas) + .values(quotas) + .execute_async(conn) + .await + .map_err(|e| { + public_error_from_diesel( e, ErrorHandler::Conflict( ResourceType::SiloQuotas, &silo_id.to_string(), ), - )) - } - } + ) + }) + .map(|_| ()) } pub async fn silo_update_quota( From 4f6d49f4eb17b25dd430b43557083e10ec3c3034 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 8 Dec 2023 00:24:42 -0500 Subject: [PATCH 28/47] Remove quotas tag, rename system silo list --- nexus/src/external_api/http_entrypoints.rs | 8 +- nexus/src/external_api/tag-config.json | 6 -- nexus/tests/output/nexus_tags.txt | 9 +-- openapi/nexus.json | 90 +++++++++++----------- 4 files changed, 52 insertions(+), 61 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 88a749554d..2bc163a8d4 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -514,8 +514,8 @@ async fn policy_update( /// Lists resource quotas for all silos #[endpoint { method = GET, - path = "/v1/system/quotas", - tags = ["system/quotas"], + path = "/v1/system/silo-quotas", + tags = ["system/silos"], }] async fn system_quotas_list( rqctx: RequestContext>, @@ -549,7 +549,7 @@ async fn system_quotas_list( #[endpoint { method = GET, path = "/v1/system/silos/{silo}/quotas", - tags = ["system/quotas"], + tags = ["system/silos"], }] async fn silo_quotas_view( rqctx: RequestContext>, @@ -572,7 +572,7 @@ async fn silo_quotas_view( #[endpoint { method = PUT, path = "/v1/system/silos/{silo}/quotas", - tags = ["system/quotas"], + tags = ["system/silos"], }] async fn silo_quotas_update( rqctx: RequestContext>, diff --git a/nexus/src/external_api/tag-config.json b/nexus/src/external_api/tag-config.json index dc1a9cf28d..07eb198016 100644 --- a/nexus/src/external_api/tag-config.json +++ b/nexus/src/external_api/tag-config.json @@ -98,12 +98,6 @@ "url": "http://docs.oxide.computer/api/system-metrics" } }, - "system/quotas": { - "description": "Quotas set resource allocation limits for a silo allowing operators to control the amount of resources a silo can consume.", - "external_docs": { - "url": "http://docs.oxide.computer/api/system-quotas" - } - }, "system/networking": { "description": "This provides rack-level network configuration.", "external_docs": { diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 50d23981e6..f5653f466f 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -164,12 +164,6 @@ networking_switch_port_settings_delete DELETE /v1/system/networking/switch-p networking_switch_port_settings_list GET /v1/system/networking/switch-port-settings networking_switch_port_settings_view GET /v1/system/networking/switch-port-settings/{port} -API operations found with tag "system/quotas" -OPERATION ID METHOD URL PATH -silo_quotas_update PUT /v1/system/silos/{silo}/quotas -silo_quotas_view GET /v1/system/silos/{silo}/quotas -system_quotas_list GET /v1/system/quotas - API operations found with tag "system/silos" OPERATION ID METHOD URL PATH local_idp_user_create POST /v1/system/identity-providers/local/users @@ -183,9 +177,12 @@ silo_identity_provider_list GET /v1/system/identity-providers silo_list GET /v1/system/silos silo_policy_update PUT /v1/system/silos/{silo}/policy silo_policy_view GET /v1/system/silos/{silo}/policy +silo_quotas_update PUT /v1/system/silos/{silo}/quotas +silo_quotas_view GET /v1/system/silos/{silo}/quotas silo_user_list GET /v1/system/users silo_user_view GET /v1/system/users/{user_id} silo_view GET /v1/system/silos/{silo} +system_quotas_list GET /v1/system/silo-quotas user_builtin_list GET /v1/system/users-builtin user_builtin_view GET /v1/system/users-builtin/{user} diff --git a/openapi/nexus.json b/openapi/nexus.json index d904064fee..22234fbac2 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5971,13 +5971,13 @@ } } }, - "/v1/system/quotas": { + "/v1/system/roles": { "get": { "tags": [ - "system/quotas" + "roles" ], - "summary": "Lists resource quotas for all silos", - "operationId": "system_quotas_list", + "summary": "List built-in roles", + "operationId": "role_list", "parameters": [ { "in": "query", @@ -5998,13 +5998,6 @@ "nullable": true, "type": "string" } - }, - { - "in": "query", - "name": "sort_by", - "schema": { - "$ref": "#/components/schemas/IdSortMode" - } } ], "responses": { @@ -6013,7 +6006,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/SiloQuotasResultsPage" + "$ref": "#/components/schemas/RoleResultsPage" } } } @@ -6030,31 +6023,20 @@ } } }, - "/v1/system/roles": { + "/v1/system/roles/{role_name}": { "get": { "tags": [ "roles" ], - "summary": "List built-in roles", - "operationId": "role_list", + "summary": "Fetch a built-in role", + "operationId": "role_view", "parameters": [ { - "in": "query", - "name": "limit", - "description": "Maximum number of items returned by a single call", - "schema": { - "nullable": true, - "type": "integer", - "format": "uint32", - "minimum": 1 - } - }, - { - "in": "query", - "name": "page_token", - "description": "Token returned by previous call to retrieve the subsequent page", + "in": "path", + "name": "role_name", + "description": "The built-in role's unique name.", + "required": true, "schema": { - "nullable": true, "type": "string" } } @@ -6065,7 +6047,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/RoleResultsPage" + "$ref": "#/components/schemas/Role" } } } @@ -6076,28 +6058,43 @@ "5XX": { "$ref": "#/components/responses/Error" } - }, - "x-dropshot-pagination": { - "required": [] } } }, - "/v1/system/roles/{role_name}": { + "/v1/system/silo-quotas": { "get": { "tags": [ - "roles" + "system/silos" ], - "summary": "Fetch a built-in role", - "operationId": "role_view", + "summary": "Lists resource quotas for all silos", + "operationId": "system_quotas_list", "parameters": [ { - "in": "path", - "name": "role_name", - "description": "The built-in role's unique name.", - "required": true, + "in": "query", + "name": "limit", + "description": "Maximum number of items returned by a single call", + "schema": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 1 + } + }, + { + "in": "query", + "name": "page_token", + "description": "Token returned by previous call to retrieve the subsequent page", "schema": { + "nullable": true, "type": "string" } + }, + { + "in": "query", + "name": "sort_by", + "schema": { + "$ref": "#/components/schemas/IdSortMode" + } } ], "responses": { @@ -6106,7 +6103,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/Role" + "$ref": "#/components/schemas/SiloQuotasResultsPage" } } } @@ -6117,6 +6114,9 @@ "5XX": { "$ref": "#/components/responses/Error" } + }, + "x-dropshot-pagination": { + "required": [] } } }, @@ -6371,7 +6371,7 @@ "/v1/system/silos/{silo}/quotas": { "get": { "tags": [ - "system/quotas" + "system/silos" ], "summary": "View the resource quotas of a given silo", "operationId": "silo_quotas_view", @@ -6407,7 +6407,7 @@ }, "put": { "tags": [ - "system/quotas" + "system/silos" ], "summary": "Update the resource quotas of a given silo", "operationId": "silo_quotas_update", From 4111cc2d40fb7bd86c5fb2fc03321c579eab7014 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 8 Dec 2023 00:43:03 -0500 Subject: [PATCH 29/47] Drop custom quota configuration for standard half-rack config --- nexus/db-queries/src/db/datastore/rack.rs | 6 +----- nexus/test-utils/src/resource_helpers.rs | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 3ea785aa4c..b6b208bde9 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -912,11 +912,7 @@ mod test { name: "test-silo".parse().unwrap(), description: String::new(), }, - quotas: external_params::SiloQuotasCreate { - cpus: 128, - memory: ByteCount::from_gibibytes_u32(1000), - storage: ByteCount::from_gibibytes_u32(1000000), - }, + quotas: external_params::SiloQuotasCreate::half_rack(), discoverable: false, identity_mode: SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 01a6415503..b0eb6ad595 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -262,11 +262,7 @@ pub async fn create_silo( name: silo_name.parse().unwrap(), description: "a silo".to_string(), }, - quotas: params::SiloQuotasCreate { - cpus: 128, - memory: ByteCount::from_gibibytes_u32(1000), - storage: ByteCount::from_gibibytes_u32(1000000), - }, + quotas: params::SiloQuotasCreate::half_rack(), discoverable, identity_mode, admin_group_name: None, From 501563bf07593415e40a8da75d6a9e87949c90c8 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 8 Dec 2023 00:43:17 -0500 Subject: [PATCH 30/47] Fixup auth tests --- nexus/tests/integration_tests/endpoints.rs | 29 ++++++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 443ffb2d68..22664d7ebe 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -85,17 +85,15 @@ lazy_static! { format!("/v1/system/silos/{}", *DEMO_SILO_NAME); pub static ref DEMO_SILO_POLICY_URL: String = format!("/v1/system/silos/{}/policy", *DEMO_SILO_NAME); + pub static ref DEMO_SILO_QUOTAS_URL: String = + format!("/v1/system/silos/{}/quotas", *DEMO_SILO_NAME); pub static ref DEMO_SILO_CREATE: params::SiloCreate = params::SiloCreate { identity: IdentityMetadataCreateParams { name: DEMO_SILO_NAME.clone(), description: String::from(""), }, - quotas: params::SiloQuotasCreate { - cpus: 100, - memory: ByteCount::from_gibibytes_u32(1000), - storage: ByteCount::from_gibibytes_u32(100000), - }, + quotas: params::SiloQuotasCreate::half_rack(), discoverable: true, identity_mode: shared::SiloIdentityMode::SamlJit, admin_group_name: None, @@ -943,6 +941,27 @@ lazy_static! { ), ], }, + VerifyEndpoint { + url: &DEMO_SILO_QUOTAS_URL, + visibility: Visibility::Protected, + unprivileged_access: UnprivilegedAccess::None, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Put( + serde_json::to_value( + params::SiloQuotasCreate::empty() + ).unwrap() + ) + ], + }, + VerifyEndpoint { + url: "/v1/system/silo-quotas", + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::None, + allowed_methods: vec![ + AllowedMethod::Get + ], + }, VerifyEndpoint { url: "/v1/policy", visibility: Visibility::Public, From f15a451c59e04af2fa8a5865e70fcdc6bf5a6d2e Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 8 Dec 2023 01:03:20 -0500 Subject: [PATCH 31/47] Better docs --- nexus/src/external_api/http_entrypoints.rs | 2 ++ nexus/types/src/external_api/params.rs | 15 ++++++++++- openapi/nexus.json | 30 ++++++++++++++-------- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 2bc163a8d4..60d2a9d9c4 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -569,6 +569,8 @@ async fn silo_quotas_view( } /// Update the resource quotas of a given silo +/// +/// If a quota value is not specified, it will remain unchanged. #[endpoint { method = PUT, path = "/v1/system/silos/{silo}/quotas", diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 7a88d4493a..2eb8dee73b 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -279,7 +279,10 @@ pub struct SiloCreate { /// endpoints. These should be valid for the Silo's DNS name(s). pub tls_certificates: Vec, - /// Initial quotas for the new Silo + /// Limits the amount of provisionable CPU, memory, and storage in the Silo. + /// CPU and memory are only consumed by running instances, while storage is + /// consumed by any disk or snapshot. A value of 0 means that resource is + /// *not* provisionable. pub quotas: SiloQuotasCreate, /// Mapping of which Fleet roles are conferred by each Silo role @@ -291,14 +294,19 @@ pub struct SiloCreate { BTreeMap>, } +/// The amount of provisionable resources for a Silo #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct SiloQuotasCreate { + /// The amount of virtual CPUs available for running instances in the Silo pub cpus: i64, + /// The amount of RAM (in bytes) available for running instances in the Silo pub memory: ByteCount, + /// The amount of storage (in bytes) available for disks or snapshots pub storage: ByteCount, } impl SiloQuotasCreate { + /// All quotas set to 0 pub fn empty() -> Self { Self { cpus: 0, @@ -318,10 +326,15 @@ impl SiloQuotasCreate { } } +/// Updateable properties of a Silo's resource limits. +/// If a value is omitted it will not be updated. #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct SiloQuotasUpdate { + /// The amount of virtual CPUs available for running instances in the Silo pub cpus: Option, + /// The amount of RAM (in bytes) available for running instances in the Silo pub memory: Option, + /// The amount of storage (in bytes) available for disks or snapshots pub storage: Option, } diff --git a/openapi/nexus.json b/openapi/nexus.json index 22234fbac2..499403961e 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -6410,6 +6410,7 @@ "system/silos" ], "summary": "Update the resource quotas of a given silo", + "description": "If a quota value is not specified, it will remain unchanged.", "operationId": "silo_quotas_update", "parameters": [ { @@ -13142,7 +13143,7 @@ "$ref": "#/components/schemas/Name" }, "quotas": { - "description": "Initial quotas for the new Silo", + "description": "Limits the amount of provisionable CPU, memory, and storage in the Silo. CPU and memory are only consumed by running instances, while storage is consumed by any disk or snapshot. A value of 0 means that resource is *not* provisionable.", "allOf": [ { "$ref": "#/components/schemas/SiloQuotasCreate" @@ -13211,17 +13212,29 @@ ] }, "SiloQuotasCreate": { + "description": "The amount of provisionable resources for a Silo", "type": "object", "properties": { "cpus": { + "description": "The amount of virtual CPUs available for running instances in the Silo", "type": "integer", "format": "int64" }, "memory": { - "$ref": "#/components/schemas/ByteCount" + "description": "The amount of RAM (in bytes) available for running instances in the Silo", + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] }, "storage": { - "$ref": "#/components/schemas/ByteCount" + "description": "The amount of storage (in bytes) available for disks or snapshots", + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] } }, "required": [ @@ -13252,15 +13265,18 @@ ] }, "SiloQuotasUpdate": { + "description": "Updateable properties of a Silo's resource limits. If a value is omitted it will not be updated.", "type": "object", "properties": { "cpus": { "nullable": true, + "description": "The amount of virtual CPUs available for running instances in the Silo", "type": "integer", "format": "int64" }, "memory": { "nullable": true, + "description": "The amount of RAM (in bytes) available for running instances in the Silo", "allOf": [ { "$ref": "#/components/schemas/ByteCount" @@ -13269,6 +13285,7 @@ }, "storage": { "nullable": true, + "description": "The amount of storage (in bytes) available for disks or snapshots", "allOf": [ { "$ref": "#/components/schemas/ByteCount" @@ -15608,13 +15625,6 @@ "url": "http://docs.oxide.computer/api/system-networking" } }, - { - "name": "system/quotas", - "description": "Quotas set resource allocation limits for a silo allowing operators to control the amount of resources a silo can consume.", - "externalDocs": { - "url": "http://docs.oxide.computer/api/system-quotas" - } - }, { "name": "system/silos", "description": "Silos represent a logical partition of users and resources.", From 569b9958c8ccee66e5fe46807d19691639afc85c Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 8 Dec 2023 01:22:34 -0500 Subject: [PATCH 32/47] Delete quotas if the silo is deleted --- nexus/db-queries/src/db/datastore/quota.rs | 21 +++++++++++++++++++++ nexus/db-queries/src/db/datastore/rack.rs | 2 +- nexus/db-queries/src/db/datastore/silo.rs | 2 ++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/quota.rs b/nexus/db-queries/src/db/datastore/quota.rs index 148948c1c7..573627cc67 100644 --- a/nexus/db-queries/src/db/datastore/quota.rs +++ b/nexus/db-queries/src/db/datastore/quota.rs @@ -11,6 +11,7 @@ use diesel::prelude::*; use nexus_db_model::SiloQuotas; use nexus_db_model::SiloQuotasUpdate; use omicron_common::api::external::DataPageParams; +use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::ResourceType; @@ -47,6 +48,26 @@ impl DataStore { .map(|_| ()) } + pub async fn silo_quotas_delete( + &self, + opctx: &OpContext, + conn: &async_bb8_diesel::Connection, + authz_silo: &authz::Silo, + ) -> DeleteResult { + // Given that the quotas right now are somewhat of an extension of the + // Silo we just check for delete permission on the silo itself. + opctx.authorize(authz::Action::Delete, authz_silo).await?; + + use db::schema::silo_quotas::dsl; + diesel::delete(dsl::silo_quotas) + .filter(dsl::silo_id.eq(authz_silo.id())) + .execute_async(conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + Ok(()) + } + pub async fn silo_update_quota( &self, opctx: &OpContext, diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index b6b208bde9..b5ac2ec65a 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -874,7 +874,7 @@ mod test { }; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::{ - ByteCount, IdentityMetadataCreateParams, MacAddr, + IdentityMetadataCreateParams, MacAddr, }; use omicron_common::api::internal::shared::SourceNatConfig; use omicron_common::nexus_config::NUM_INITIAL_RESERVED_IP_ADDRESSES; diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index bf8b99451d..034ab90413 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -413,6 +413,8 @@ impl DataStore { })); } + self.silo_quotas_delete(opctx, &conn, &authz_silo).await?; + self.virtual_provisioning_collection_delete_on_connection( &opctx.log, &conn, id, ) From 43beca80008ac76de2a7c8001ff5c18276bb4464 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 8 Dec 2023 02:09:54 -0500 Subject: [PATCH 33/47] Add a migration for silo_quotas and backfill data --- schema/crdb/20.0.0/up01.sql | 8 ++++++++ schema/crdb/20.0.0/up02.sql | 30 ++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 schema/crdb/20.0.0/up01.sql create mode 100644 schema/crdb/20.0.0/up02.sql diff --git a/schema/crdb/20.0.0/up01.sql b/schema/crdb/20.0.0/up01.sql new file mode 100644 index 0000000000..6a95c41e48 --- /dev/null +++ b/schema/crdb/20.0.0/up01.sql @@ -0,0 +1,8 @@ +CREATE TABLE IF NOT EXISTS omicron.public.silo_quotas ( + silo_id UUID PRIMARY KEY, + time_created TIMESTAMPTZ NOT NULL, + time_modified TIMESTAMPTZ NOT NULL, + cpus INT8 NOT NULL, + memory_bytes INT8 NOT NULL, + storage_bytes INT8 NOT NULL +); \ No newline at end of file diff --git a/schema/crdb/20.0.0/up02.sql b/schema/crdb/20.0.0/up02.sql new file mode 100644 index 0000000000..889c8d6f3b --- /dev/null +++ b/schema/crdb/20.0.0/up02.sql @@ -0,0 +1,30 @@ +set local disallow_full_table_scans = off; + +-- Adds quotas for any existing silos without them. +-- The selected quotas are based on the resources of a half rack +-- with 30% CPU and memory reserved for internal use and a 3.5x tax +-- on storage for replication, etc. +INSERT INTO + silo_quotas ( + silo_id, + time_created, + time_modified, + cpus, + memory_bytes, + storage_bytes + ) +SELECT + s.id AS silo_id, + NOW() AS time_created, + NOW() AS time_modified, + -- ~70% of 128 threads leaving 30% for internal use + 90 AS cpus, + -- 708 GiB (~70% of memory leaving 30% for internal use) + 760209211392 AS memory_bytes, + -- 850 GiB (total storage / 3.5) + 912680550400 AS storage_bytes +FROM + silo s + LEFT JOIN silo_quotas sq ON s.id = sq.silo_id +WHERE + sq.silo_id IS NULL; \ No newline at end of file From 2c11210444a5f471f62a0ed52ffa8696f3485d20 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 8 Dec 2023 02:39:03 -0500 Subject: [PATCH 34/47] Bump schema version --- nexus/db-model/src/schema.rs | 2 +- schema/crdb/dbinit.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 963b625058..10fa8dcfac 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1333,7 +1333,7 @@ table! { /// /// This should be updated whenever the schema is changed. For more details, /// refer to: schema/crdb/README.adoc -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(19, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(20, 0, 0); allow_tables_to_appear_in_same_query!( system_update, diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 20dd27156d..be7291b4e4 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3071,7 +3071,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - ( TRUE, NOW(), NOW(), '19.0.0', NULL) + ( TRUE, NOW(), NOW(), '20.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From 8a7794ba26d8439c1293fd4c0f4a8fb8759f68fd Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 11 Dec 2023 14:50:21 -0500 Subject: [PATCH 35/47] Use insufficient capacity error --- .../virtual_provisioning_collection_update.rs | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index 0d44b5f45c..f7b506ac14 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -28,6 +28,7 @@ use nexus_db_model::queries::virtual_provisioning_collection_update::{ all_collections, do_update, parent_silo, quotas, silo_provisioned, }; use omicron_common::api::external; +use omicron_common::api::external::MessagePair; const NOT_ENOUGH_CPUS_SENTINEL: &'static str = "Not enough cpus"; const NOT_ENOUGH_MEMORY_SENTINEL: &'static str = "Not enough memory"; @@ -47,13 +48,28 @@ pub fn from_diesel(e: DieselError) -> external::Error { if let Some(sentinel) = matches_sentinel(&e, &sentinels) { match sentinel { NOT_ENOUGH_CPUS_SENTINEL => { - return external::Error::InvalidRequest { message: "Insufficient Capacity: Not enough CPUs to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string() } + return external::Error::InsufficientCapacity { + message: MessagePair::new_full( + "Insufficient Capacity: Not enough CPUs to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string(), + "User tried to allocate an instance but the virtual provisioning resource table indicated that there were not enough CPUs available to satisfy the request.".to_string(), + ) + } } NOT_ENOUGH_MEMORY_SENTINEL => { - return external::Error::InvalidRequest { message: "Insufficient Capacity: Not enough memory to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string() } + return external::Error::InsufficientCapacity { + message: MessagePair::new_full( + "Insufficient Capacity: Not enough memory to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string(), + "User tried to allocate an instance but the virtual provisioning resource table indicated that there were not enough RAM available to satisfy the request.".to_string(), + ) + } } NOT_ENOUGH_STORAGE_SENTINEL => { - return external::Error::InvalidRequest { message: "Insufficient Capacity: Not enough storage to complete request. Either remove unneeded disks and snapshots to free up resources or contact the rack operator to request a capacity increase.".to_string() } + return external::Error::InsufficientCapacity { + message: MessagePair::new_full( + "Insufficient Capacity: Not enough storage to complete request. Either remove unneeded disks and snapshots to free up resources or contact the rack operator to request a capacity increase.".to_string(), + "User tried to allocate a disk or snapshot but the virtual provisioning resource table indicated that there were not enough storage available to satisfy the request.".to_string(), + ) + } } _ => {} } From f8955a5216c8203c987bd66b560f95de6e79c5e0 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Mon, 11 Dec 2023 16:20:42 -0500 Subject: [PATCH 36/47] half_rack -> from_sled_count --- nexus/db-model/src/quota.rs | 4 ++-- nexus/db-queries/src/db/datastore/rack.rs | 5 ++++- nexus/db-queries/src/db/datastore/silo.rs | 2 +- nexus/test-utils/src/resource_helpers.rs | 2 +- nexus/tests/integration_tests/endpoints.rs | 2 +- nexus/types/src/external_api/params.rs | 12 ++++++++---- 6 files changed, 17 insertions(+), 10 deletions(-) diff --git a/nexus/db-model/src/quota.rs b/nexus/db-model/src/quota.rs index adf0612e8f..c9a256cb8c 100644 --- a/nexus/db-model/src/quota.rs +++ b/nexus/db-model/src/quota.rs @@ -50,8 +50,8 @@ impl SiloQuotas { } } - pub fn half_rack(silo_id: Uuid) -> Self { - let count = params::SiloQuotasCreate::half_rack(); + pub fn from_sled_count(silo_id: Uuid, num_sleds: u32) -> Self { + let count = params::SiloQuotasCreate::from_sled_count(num_sleds); Self::new( silo_id, count.cpus, diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index b5ac2ec65a..f1afbebf8c 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -912,7 +912,10 @@ mod test { name: "test-silo".parse().unwrap(), description: String::new(), }, - quotas: external_params::SiloQuotasCreate::half_rack(), + // Set a default quota of a half rack's worth of resources + quotas: external_params::SiloQuotasCreate::from_sled_count( + 16, + ), discoverable: false, identity_mode: SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index c9c03ee4cc..a4481c39db 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -68,7 +68,7 @@ impl DataStore { .await? .transaction_async(|conn| async move { diesel::insert_into(quotas_dsl::silo_quotas) - .values(SiloQuotas::half_rack(DEFAULT_SILO.id())) + .values(SiloQuotas::from_sled_count(DEFAULT_SILO.id(), 16)) .on_conflict(quotas_dsl::silo_id) .do_nothing() .execute_async(&conn) diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index c4ea33abb8..cd4aecfaee 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -287,7 +287,7 @@ pub async fn create_silo( name: silo_name.parse().unwrap(), description: "a silo".to_string(), }, - quotas: params::SiloQuotasCreate::half_rack(), + quotas: params::SiloQuotasCreate::from_sled_count(16), discoverable, identity_mode, admin_group_name: None, diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 37eee1e90e..93a57c5832 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -93,7 +93,7 @@ lazy_static! { name: DEMO_SILO_NAME.clone(), description: String::from(""), }, - quotas: params::SiloQuotasCreate::half_rack(), + quotas: params::SiloQuotasCreate::from_sled_count(16), discoverable: true, identity_mode: shared::SiloIdentityMode::SamlJit, admin_group_name: None, diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 3fd4217765..a7dbd1bd67 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -324,13 +324,17 @@ impl SiloQuotasCreate { } } + /// Generates a conservative maximum set of quotas for a Silo with the + /// given number of sleds. Useful for situations where a non-zero quota + /// must be set, but the actual value is not known. + /// /// 30% of CPUs and memory reserved for internal usage. /// Storage calculated at (total / 3.5) to account for redundancy / bookkeeping. - pub fn half_rack() -> Self { + pub fn from_sled_count(num_sleds: u32) -> Self { Self { - cpus: 90, - memory: ByteCount::from_gibibytes_u32(708), - storage: ByteCount::from_gibibytes_u32(850), + cpus: 90 * i64::from(num_sleds), + memory: ByteCount::from_gibibytes_u32(708 * num_sleds), + storage: ByteCount::from_gibibytes_u32(850 * num_sleds), } } } From 371bc98f8be801c2afb565cb86ec04ff1fb28734 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 14:54:11 -0500 Subject: [PATCH 37/47] Fix db deadlock caused by checking authz before silo is created --- nexus/db-queries/src/db/datastore/quota.rs | 9 ++++++--- nexus/db-queries/src/db/datastore/silo.rs | 1 - 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/quota.rs b/nexus/db-queries/src/db/datastore/quota.rs index 573627cc67..6aba919e73 100644 --- a/nexus/db-queries/src/db/datastore/quota.rs +++ b/nexus/db-queries/src/db/datastore/quota.rs @@ -20,15 +20,18 @@ use uuid::Uuid; impl DataStore { /// Creates new quotas for a silo. This is grouped with silo creation - /// and shouldn't be called directly by the user. + /// and shouldn't be called outside of that flow. + /// + /// An authz check _cannot_ be performed here because the authz initialization + /// isn't complete and will lead to a db deadlock. + /// + /// See https://github.com/oxidecomputer/omicron/blob/07eb7dafc20e35e44edf429fcbb759cbb33edd5f/nexus/db-queries/src/db/datastore/rack.rs#L407-L410 pub async fn silo_quotas_create( &self, - opctx: &OpContext, conn: &async_bb8_diesel::Connection, authz_silo: &authz::Silo, quotas: SiloQuotas, ) -> Result<(), Error> { - opctx.authorize(authz::Action::Modify, authz_silo).await?; let silo_id = authz_silo.id(); use db::schema::silo_quotas::dsl; diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index a4481c39db..a483ced62e 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -282,7 +282,6 @@ impl DataStore { self.dns_update(nexus_opctx, &conn, dns_update).await?; self.silo_quotas_create( - opctx, &conn, &authz_silo, SiloQuotas::new( From de951a76eba214c144d1911773b93a667b011819 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 15:00:19 -0500 Subject: [PATCH 38/47] Add quotas for silo cert tests --- nexus/tests/integration_tests/certificates.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nexus/tests/integration_tests/certificates.rs b/nexus/tests/integration_tests/certificates.rs index 1843fc28c8..598585f80c 100644 --- a/nexus/tests/integration_tests/certificates.rs +++ b/nexus/tests/integration_tests/certificates.rs @@ -394,6 +394,7 @@ async fn test_silo_certificates() { .name(silo2.silo_name.clone()) .description("") .discoverable(false) + .quotas(params::SiloQuotasCreate::empty()) .identity_mode(oxide_client::types::SiloIdentityMode::LocalOnly) .tls_certificates(vec![silo2_cert.try_into().unwrap()]), ) @@ -454,6 +455,7 @@ async fn test_silo_certificates() { .name(silo3.silo_name.clone()) .description("") .discoverable(false) + .quotas(params::SiloQuotasCreate::empty()) .identity_mode(oxide_client::types::SiloIdentityMode::LocalOnly) .tls_certificates(vec![silo3_cert.try_into().unwrap()]), ) From 410d3ae35f554ea71a5a8016154f952f84605aec Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 15:31:27 -0500 Subject: [PATCH 39/47] Fix type failures --- nexus/tests/integration_tests/certificates.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/nexus/tests/integration_tests/certificates.rs b/nexus/tests/integration_tests/certificates.rs index 598585f80c..5a34caab49 100644 --- a/nexus/tests/integration_tests/certificates.rs +++ b/nexus/tests/integration_tests/certificates.rs @@ -394,7 +394,11 @@ async fn test_silo_certificates() { .name(silo2.silo_name.clone()) .description("") .discoverable(false) - .quotas(params::SiloQuotasCreate::empty()) + .quotas(oxide_client::types::SiloQuotasCreate { + cpus: 0, + memory: oxide_client::types::ByteCount(0), + storage: oxide_client::types::ByteCount(0), + }) .identity_mode(oxide_client::types::SiloIdentityMode::LocalOnly) .tls_certificates(vec![silo2_cert.try_into().unwrap()]), ) @@ -455,7 +459,11 @@ async fn test_silo_certificates() { .name(silo3.silo_name.clone()) .description("") .discoverable(false) - .quotas(params::SiloQuotasCreate::empty()) + .quotas(oxide_client::types::SiloQuotasCreate { + cpus: 0, + memory: oxide_client::types::ByteCount(0), + storage: oxide_client::types::ByteCount(0), + }) .identity_mode(oxide_client::types::SiloIdentityMode::LocalOnly) .tls_certificates(vec![silo3_cert.try_into().unwrap()]), ) From cd223885862a31637edef062342ed8d170330089 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 15:53:32 -0500 Subject: [PATCH 40/47] Fix docs build --- nexus/db-queries/src/db/datastore/quota.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/quota.rs b/nexus/db-queries/src/db/datastore/quota.rs index 6aba919e73..2066781e6b 100644 --- a/nexus/db-queries/src/db/datastore/quota.rs +++ b/nexus/db-queries/src/db/datastore/quota.rs @@ -25,7 +25,7 @@ impl DataStore { /// An authz check _cannot_ be performed here because the authz initialization /// isn't complete and will lead to a db deadlock. /// - /// See https://github.com/oxidecomputer/omicron/blob/07eb7dafc20e35e44edf429fcbb759cbb33edd5f/nexus/db-queries/src/db/datastore/rack.rs#L407-L410 + /// See pub async fn silo_quotas_create( &self, conn: &async_bb8_diesel::Connection, From e86ec72b1d46fcb919497baad804fb2f767e6fb5 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 16:51:10 -0500 Subject: [PATCH 41/47] Update nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs Co-authored-by: Sean Klein --- .../src/db/queries/virtual_provisioning_collection_update.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index f7b506ac14..22b51ede12 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -376,7 +376,9 @@ where } /// The virtual resource collection is only updated when a resource is inserted -/// or deleted from the resource provisioning table for idempotency. +/// or deleted from the resource provisioning table. By probing for the presence +/// or absence of a resource, we can update collections at the same time as we +/// create or destroy the resource, which helps make the operation idempotent. enum UpdateKind { Insert(VirtualProvisioningResource), Delete(uuid::Uuid), From 59743257b39350f8476b29d614d1c2fadeebc467 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 17:20:50 -0500 Subject: [PATCH 42/47] Use an arbitrarily high value over something that looks real but isn't --- nexus/db-model/src/quota.rs | 4 ++-- nexus/db-queries/src/db/datastore/rack.rs | 4 +--- nexus/db-queries/src/db/datastore/silo.rs | 4 +++- nexus/test-utils/src/resource_helpers.rs | 2 +- nexus/tests/integration_tests/endpoints.rs | 2 +- nexus/types/src/external_api/params.rs | 18 +++++++++--------- schema/crdb/20.0.0/up02.sql | 9 +++------ 7 files changed, 20 insertions(+), 23 deletions(-) diff --git a/nexus/db-model/src/quota.rs b/nexus/db-model/src/quota.rs index c9a256cb8c..70a8ffa1fd 100644 --- a/nexus/db-model/src/quota.rs +++ b/nexus/db-model/src/quota.rs @@ -50,8 +50,8 @@ impl SiloQuotas { } } - pub fn from_sled_count(silo_id: Uuid, num_sleds: u32) -> Self { - let count = params::SiloQuotasCreate::from_sled_count(num_sleds); + pub fn arbitrarily_high_default(silo_id: Uuid) -> Self { + let count = params::SiloQuotasCreate::arbitrarily_high_default(); Self::new( silo_id, count.cpus, diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index f1afbebf8c..728da0b0d1 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -913,9 +913,7 @@ mod test { description: String::new(), }, // Set a default quota of a half rack's worth of resources - quotas: external_params::SiloQuotasCreate::from_sled_count( - 16, - ), + quotas: external_params::SiloQuotasCreate::arbitrarily_high_default(), discoverable: false, identity_mode: SiloIdentityMode::LocalOnly, admin_group_name: None, diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index a483ced62e..2c0c5f3c47 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -68,7 +68,9 @@ impl DataStore { .await? .transaction_async(|conn| async move { diesel::insert_into(quotas_dsl::silo_quotas) - .values(SiloQuotas::from_sled_count(DEFAULT_SILO.id(), 16)) + .values(SiloQuotas::arbitrarily_high_default( + DEFAULT_SILO.id(), + )) .on_conflict(quotas_dsl::silo_id) .do_nothing() .execute_async(&conn) diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index cd4aecfaee..0527d99490 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -287,7 +287,7 @@ pub async fn create_silo( name: silo_name.parse().unwrap(), description: "a silo".to_string(), }, - quotas: params::SiloQuotasCreate::from_sled_count(16), + quotas: params::SiloQuotasCreate::arbitrarily_high_default(), discoverable, identity_mode, admin_group_name: None, diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 93a57c5832..bd6df210c0 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -93,7 +93,7 @@ lazy_static! { name: DEMO_SILO_NAME.clone(), description: String::from(""), }, - quotas: params::SiloQuotasCreate::from_sled_count(16), + quotas: params::SiloQuotasCreate::arbitrarily_high_default(), discoverable: true, identity_mode: shared::SiloIdentityMode::SamlJit, admin_group_name: None, diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index a7dbd1bd67..0626d064b8 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -324,17 +324,17 @@ impl SiloQuotasCreate { } } - /// Generates a conservative maximum set of quotas for a Silo with the - /// given number of sleds. Useful for situations where a non-zero quota - /// must be set, but the actual value is not known. + /// An arbitrarily high but identifiable default for quotas + /// that can be used for creating a Silo for testing /// - /// 30% of CPUs and memory reserved for internal usage. - /// Storage calculated at (total / 3.5) to account for redundancy / bookkeeping. - pub fn from_sled_count(num_sleds: u32) -> Self { + /// The only silo that customers will see that this should be set on is the default + /// silo. Ultimately the default silo should only be initialized with an empty quota, + /// but as tests currently relying on it having a quota, we need to set something. + pub fn arbitrarily_high_default() -> Self { Self { - cpus: 90 * i64::from(num_sleds), - memory: ByteCount::from_gibibytes_u32(708 * num_sleds), - storage: ByteCount::from_gibibytes_u32(850 * num_sleds), + cpus: 9999999999, + memory: ByteCount::try_from(9999999999999999999 as u64).unwrap(), + storage: ByteCount::try_from(9999999999999999999 as u64).unwrap(), } } } diff --git a/schema/crdb/20.0.0/up02.sql b/schema/crdb/20.0.0/up02.sql index 889c8d6f3b..2dafd3a396 100644 --- a/schema/crdb/20.0.0/up02.sql +++ b/schema/crdb/20.0.0/up02.sql @@ -17,12 +17,9 @@ SELECT s.id AS silo_id, NOW() AS time_created, NOW() AS time_modified, - -- ~70% of 128 threads leaving 30% for internal use - 90 AS cpus, - -- 708 GiB (~70% of memory leaving 30% for internal use) - 760209211392 AS memory_bytes, - -- 850 GiB (total storage / 3.5) - 912680550400 AS storage_bytes + 9999999999 AS cpus, + 9999999999999999999 AS memory_bytes, + 9999999999999999999 AS storage_bytes FROM silo s LEFT JOIN silo_quotas sq ON s.id = sq.silo_id From c602e29c13d12c0f9cb7084fa3dd1bc2cfbb2fcd Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 19:45:42 -0500 Subject: [PATCH 43/47] Enhance tests to check errors; fix overflow --- .../virtual_provisioning_collection_update.rs | 6 +-- nexus/tests/integration_tests/quotas.rs | 54 +++++++++++++++---- nexus/types/src/external_api/params.rs | 4 +- schema/crdb/20.0.0/up02.sql | 7 +-- 4 files changed, 52 insertions(+), 19 deletions(-) diff --git a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs index 22b51ede12..7672d5af9a 100644 --- a/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs +++ b/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs @@ -50,7 +50,7 @@ pub fn from_diesel(e: DieselError) -> external::Error { NOT_ENOUGH_CPUS_SENTINEL => { return external::Error::InsufficientCapacity { message: MessagePair::new_full( - "Insufficient Capacity: Not enough CPUs to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string(), + "vCPU Limit Exceeded: Not enough vCPUs to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string(), "User tried to allocate an instance but the virtual provisioning resource table indicated that there were not enough CPUs available to satisfy the request.".to_string(), ) } @@ -58,7 +58,7 @@ pub fn from_diesel(e: DieselError) -> external::Error { NOT_ENOUGH_MEMORY_SENTINEL => { return external::Error::InsufficientCapacity { message: MessagePair::new_full( - "Insufficient Capacity: Not enough memory to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string(), + "Memory Limit Exceeded: Not enough memory to complete request. Either stop unused instances to free up resources or contact the rack operator to request a capacity increase.".to_string(), "User tried to allocate an instance but the virtual provisioning resource table indicated that there were not enough RAM available to satisfy the request.".to_string(), ) } @@ -66,7 +66,7 @@ pub fn from_diesel(e: DieselError) -> external::Error { NOT_ENOUGH_STORAGE_SENTINEL => { return external::Error::InsufficientCapacity { message: MessagePair::new_full( - "Insufficient Capacity: Not enough storage to complete request. Either remove unneeded disks and snapshots to free up resources or contact the rack operator to request a capacity increase.".to_string(), + "Storage Limit Exceeded: Not enough storage to complete request. Either remove unneeded disks and snapshots to free up resources or contact the rack operator to request a capacity increase.".to_string(), "User tried to allocate a disk or snapshot but the virtual provisioning resource table indicated that there were not enough storage available to satisfy the request.".to_string(), ) } diff --git a/nexus/tests/integration_tests/quotas.rs b/nexus/tests/integration_tests/quotas.rs index 0fe6ebdcf2..c52a573096 100644 --- a/nexus/tests/integration_tests/quotas.rs +++ b/nexus/tests/integration_tests/quotas.rs @@ -1,5 +1,6 @@ use anyhow::Error; use dropshot::test_util::ClientTestContext; +use dropshot::HttpErrorResponseBody; use http::Method; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; @@ -97,8 +98,7 @@ impl ResourceAllocator { format!("/v1/instances/{}/start?project=project", name) .as_str(), ) - .body(None as Option<&serde_json::Value>) - .expect_status(Some(http::StatusCode::ACCEPTED)), + .body(None as Option<&serde_json::Value>), ) .authn_as(self.auth.clone()) .execute() @@ -140,10 +140,13 @@ impl ResourceAllocator { name: &str, size: u32, ) -> Result { - NexusRequest::objects_post( - client, - "/v1/disks?project=project", - ¶ms::DiskCreate { + NexusRequest::new( + RequestBuilder::new( + client, + Method::POST, + "/v1/disks?project=project", + ) + .body(Some(¶ms::DiskCreate { identity: IdentityMetadataCreateParams { name: name.parse().unwrap(), description: "".into(), @@ -152,7 +155,7 @@ impl ResourceAllocator { disk_source: params::DiskSource::Blank { block_size: params::BlockSize::try_from(512).unwrap(), }, - }, + })), ) .authn_as(self.auth.clone()) .execute() @@ -239,10 +242,17 @@ async fn test_quotas(cptestctx: &ControlPlaneTestContext) { .await; // Ensure trying to provision an instance with empty quotas fails - system + let err = system .provision_instance(client, "instance", 1, 1) .await - .expect_err("should've failed with insufficient CPU quota"); + .expect("should've failed with insufficient CPU quota") + .parsed_body::() + .expect("failed to parse error body"); + assert!( + err.message.contains("vCPU Limit Exceeded"), + "Unexpected error: {0}", + err.message + ); system.cleanup_instance(client, "instance").await; // Up the CPU, memory quotas @@ -263,14 +273,36 @@ async fn test_quotas(cptestctx: &ControlPlaneTestContext) { assert_eq!(quotas.memory, ByteCount::from_gibibytes_u32(15)); assert_eq!(quotas.storage, ByteCount::from_gibibytes_u32(2)); + // Ensure memory quota is enforced + let err = system + .provision_instance(client, "instance", 1, 16) + .await + .expect("should've failed with insufficient memory") + .parsed_body::() + .expect("failed to parse error body"); + assert!( + err.message.contains("Memory Limit Exceeded"), + "Unexpected error: {0}", + err.message + ); + system.cleanup_instance(client, "instance").await; + // Allocating instance should now succeed system .provision_instance(client, "instance", 2, 10) .await .expect("Instance should've had enough resources to be provisioned"); - system.provision_disk(client, "disk", 3).await.expect_err( - "Disk should not be provisioned because it exceeds the quota", + let err = system + .provision_disk(client, "disk", 3) + .await + .expect("should've failed w/ disk quota exceeded error") + .parsed_body::() + .expect("failed to parse error body"); + assert!( + err.message.contains("Storage Limit Exceeded"), + "Unexpected error: {0}", + err.message ); system diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 0626d064b8..49616689d4 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -333,8 +333,8 @@ impl SiloQuotasCreate { pub fn arbitrarily_high_default() -> Self { Self { cpus: 9999999999, - memory: ByteCount::try_from(9999999999999999999 as u64).unwrap(), - storage: ByteCount::try_from(9999999999999999999 as u64).unwrap(), + memory: ByteCount::try_from(999999999999999999 as u64).unwrap(), + storage: ByteCount::try_from(999999999999999999 as u64).unwrap(), } } } diff --git a/schema/crdb/20.0.0/up02.sql b/schema/crdb/20.0.0/up02.sql index 2dafd3a396..2909e379ca 100644 --- a/schema/crdb/20.0.0/up02.sql +++ b/schema/crdb/20.0.0/up02.sql @@ -1,4 +1,5 @@ -set local disallow_full_table_scans = off; +set + local disallow_full_table_scans = off; -- Adds quotas for any existing silos without them. -- The selected quotas are based on the resources of a half rack @@ -18,8 +19,8 @@ SELECT NOW() AS time_created, NOW() AS time_modified, 9999999999 AS cpus, - 9999999999999999999 AS memory_bytes, - 9999999999999999999 AS storage_bytes + 999999999999999999 AS memory_bytes, + 999999999999999999 AS storage_bytes FROM silo s LEFT JOIN silo_quotas sq ON s.id = sq.silo_id From fe87ca8e0444401a61be245ed9e2b76df5bdbbe0 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 19:47:23 -0500 Subject: [PATCH 44/47] Drop expects for unwraps when we don't expect them to fail --- nexus/tests/integration_tests/quotas.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nexus/tests/integration_tests/quotas.rs b/nexus/tests/integration_tests/quotas.rs index c52a573096..e9c1b74237 100644 --- a/nexus/tests/integration_tests/quotas.rs +++ b/nexus/tests/integration_tests/quotas.rs @@ -245,7 +245,7 @@ async fn test_quotas(cptestctx: &ControlPlaneTestContext) { let err = system .provision_instance(client, "instance", 1, 1) .await - .expect("should've failed with insufficient CPU quota") + .unwrap() .parsed_body::() .expect("failed to parse error body"); assert!( @@ -277,7 +277,7 @@ async fn test_quotas(cptestctx: &ControlPlaneTestContext) { let err = system .provision_instance(client, "instance", 1, 16) .await - .expect("should've failed with insufficient memory") + .unwrap() .parsed_body::() .expect("failed to parse error body"); assert!( @@ -296,7 +296,7 @@ async fn test_quotas(cptestctx: &ControlPlaneTestContext) { let err = system .provision_disk(client, "disk", 3) .await - .expect("should've failed w/ disk quota exceeded error") + .unwrap() .parsed_body::() .expect("failed to parse error body"); assert!( From 11ab3263eaeff4fb2deeecd7a4ce6f624f16663d Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 19:48:40 -0500 Subject: [PATCH 45/47] Update comment --- nexus/tests/integration_tests/quotas.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/tests/integration_tests/quotas.rs b/nexus/tests/integration_tests/quotas.rs index e9c1b74237..2fddf4e05c 100644 --- a/nexus/tests/integration_tests/quotas.rs +++ b/nexus/tests/integration_tests/quotas.rs @@ -110,7 +110,7 @@ impl ResourceAllocator { client: &ClientTestContext, name: &str, ) -> TestResponse { - // Stop instance if it's started... can probably ignore errors here + // Try to stop the instance NexusRequest::new( RequestBuilder::new( client, From e369851809e7770df7b28378eab2b6fa3b125d1c Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 12 Dec 2023 19:56:07 -0500 Subject: [PATCH 46/47] Fix clippy errors --- nexus/types/src/external_api/params.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 49616689d4..f27a6619e2 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -333,8 +333,8 @@ impl SiloQuotasCreate { pub fn arbitrarily_high_default() -> Self { Self { cpus: 9999999999, - memory: ByteCount::try_from(999999999999999999 as u64).unwrap(), - storage: ByteCount::try_from(999999999999999999 as u64).unwrap(), + memory: ByteCount::try_from(999999999999999999_u64).unwrap(), + storage: ByteCount::try_from(999999999999999999_u64).unwrap(), } } } From 635d1bad0322075db5b87fe63ecb7a4e3eeb090d Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Tue, 12 Dec 2023 19:28:58 -0800 Subject: [PATCH 47/47] ci: add quota setup to deploy test --- end-to-end-tests/src/bin/bootstrap.rs | 17 +++++++++++++++-- end-to-end-tests/src/helpers/ctx.rs | 2 +- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/end-to-end-tests/src/bin/bootstrap.rs b/end-to-end-tests/src/bin/bootstrap.rs index 83a37b8c21..9ddd872bc2 100644 --- a/end-to-end-tests/src/bin/bootstrap.rs +++ b/end-to-end-tests/src/bin/bootstrap.rs @@ -4,11 +4,11 @@ use end_to_end_tests::helpers::{generate_name, get_system_ip_pool}; use omicron_test_utils::dev::poll::{wait_for_condition, CondCheckError}; use oxide_client::types::{ ByteCount, DeviceAccessTokenRequest, DeviceAuthRequest, DeviceAuthVerify, - DiskCreate, DiskSource, IpRange, Ipv4Range, + DiskCreate, DiskSource, IpRange, Ipv4Range, SiloQuotasUpdate, }; use oxide_client::{ ClientDisksExt, ClientHiddenExt, ClientProjectsExt, - ClientSystemNetworkingExt, + ClientSystemNetworkingExt, ClientSystemSilosExt, }; use serde::{de::DeserializeOwned, Deserialize}; use std::time::Duration; @@ -45,6 +45,19 @@ async fn main() -> Result<()> { .send() .await?; + // ===== SET UP QUOTAS ===== // + eprintln!("setting up quotas..."); + client + .silo_quotas_update() + .silo("recovery") + .body(SiloQuotasUpdate { + cpus: Some(16), + memory: Some(ByteCount(1024 * 1024 * 1024 * 10)), + storage: Some(ByteCount(1024 * 1024 * 1024 * 1024)), + }) + .send() + .await?; + // ===== ENSURE DATASETS ARE READY ===== // eprintln!("ensuring datasets are ready..."); let ctx = Context::from_client(client).await?; diff --git a/end-to-end-tests/src/helpers/ctx.rs b/end-to-end-tests/src/helpers/ctx.rs index 2c66bd4724..0132feafeb 100644 --- a/end-to-end-tests/src/helpers/ctx.rs +++ b/end-to-end-tests/src/helpers/ctx.rs @@ -78,7 +78,7 @@ fn rss_config() -> Result { let content = std::fs::read_to_string(&path).unwrap_or(RSS_CONFIG_STR.to_string()); toml::from_str(&content) - .with_context(|| format!("parsing config-rss as TOML")) + .with_context(|| "parsing config-rss as TOML".to_string()) } fn nexus_external_dns_name(config: &SetupServiceConfig) -> String {