Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encode resource limits into schema #3864

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion common/src/api/external/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ use std::num::{NonZeroU16, NonZeroU32};
use std::str::FromStr;
use uuid::Uuid;

use crate::limits::MAX_VCPU_PER_INSTANCE;

// The type aliases below exist primarily to ensure consistency among return
// types for functions in the `nexus::Nexus` and `nexus::DataStore`. The
// type argument `T` generally implements `Object`.
Expand Down Expand Up @@ -881,9 +883,39 @@ impl InstanceState {
}

/// The number of CPUs in an Instance
#[derive(Copy, Clone, Debug, Deserialize, Serialize, JsonSchema)]
#[derive(Copy, Clone, Debug, Deserialize, Serialize)]
pub struct InstanceCpuCount(pub u16);

impl JsonSchema for InstanceCpuCount {
fn schema_name() -> String {
"InstanceCpuCount".to_string()
}

fn json_schema(
_: &mut schemars::gen::SchemaGenerator,
) -> schemars::schema::Schema {
schemars::schema::SchemaObject {
metadata: Some(Box::new(schemars::schema::Metadata {
description: Some(
"The number of CPUs in an Instance".to_string(),
),
..Default::default()
})),
format: Some("uint16".to_string()),
instance_type: Some(schemars::schema::InstanceType::Integer.into()),
number: Some(Box::new(schemars::schema::NumberValidation {
multiple_of: None,
maximum: Some(MAX_VCPU_PER_INSTANCE.into()),
exclusive_maximum: None,
minimum: Some(1.0),
exclusive_minimum: None,
})),
..Default::default()
}
.into()
}
}

impl TryFrom<i64> for InstanceCpuCount {
type Error = anyhow::Error;

Expand Down
1 change: 1 addition & 0 deletions common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub mod backoff;
pub mod cmd;
pub mod disk;
pub mod ledger;
pub mod limits;
pub mod nexus_config;
pub mod postgres_config;
pub mod update;
Expand Down
19 changes: 19 additions & 0 deletions common/src/limits.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// 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/.

// Contains constants that define the hard limits of Nexus

pub const MAX_VCPU_PER_INSTANCE: u16 = 64;

pub const MIN_MEMORY_BYTES_PER_INSTANCE: u32 = 1 << 30; // 1 GiB
pub const MAX_MEMORY_BYTES_PER_INSTANCE: u64 = 256 * (1 << 30); // 256 GiB
Comment on lines +9 to +10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: For all these "_BYTES" constants, could we use u64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, could we hold off on that until another PR? It has some decently wide ranging repercussions. There are a lot of cases that we have to move from try to try_from. Further there are situations where we're adding two u32 ints together which require them to both be transformed. We can do it, it's just going to touch a lot of places and it'd be nice to not have that all in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, we can punt

Copy link
Contributor

@ahl ahl Jan 25, 2024

Choose a reason for hiding this comment

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

@smklein you and your love of u64s....

Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose a simple plan, charge $0.005 / hour (USD) effective February 1st for all u32 usage until we migrate to a fully u64 world.


pub const MAX_DISKS_PER_INSTANCE: u32 = 8;
pub const MIN_DISK_SIZE_BYTES: u32 = 1 << 30; // 1 GiB
pub const MAX_DISK_SIZE_BYTES: u64 = 1023 * (1 << 30); // 1023 GiB

pub const MAX_NICS_PER_INSTANCE: usize = 8;

// TODO-completeness: Support multiple external IPs
pub const MAX_EXTERNAL_IPS_PER_INSTANCE: usize = 1;
2 changes: 1 addition & 1 deletion nexus-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ impl From<omicron_common::api::external::InstanceCpuCount>
for types::InstanceCpuCount
{
fn from(s: omicron_common::api::external::InstanceCpuCount) -> Self {
Self(s.0)
Self(s.0.into())
zephraph marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
8 changes: 1 addition & 7 deletions nexus/db-queries/src/db/queries/disk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,9 @@ use diesel::{
query_builder::{AstPass, QueryFragment, QueryId},
sql_types, Column, QueryResult,
};
use omicron_common::limits::MAX_DISKS_PER_INSTANCE;
use uuid::Uuid;

/// The maximum number of disks that can be attached to an instance.
//
// This is defined here for layering reasons: the main Nexus crate depends on
// the db-queries crate, so the disk-per-instance limit lives here and Nexus
// proper re-exports it.
pub const MAX_DISKS_PER_INSTANCE: u32 = 8;

/// A wrapper for the query that selects a PCI slot for a newly-attached disk.
///
/// The general idea is to produce a query that left joins a single-column table
Expand Down
5 changes: 2 additions & 3 deletions nexus/src/app/disk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,12 @@ use omicron_common::api::external::LookupResult;
use omicron_common::api::external::NameOrId;
use omicron_common::api::external::UpdateResult;
use omicron_common::api::internal::nexus::DiskRuntimeState;
use omicron_common::limits::MAX_DISK_SIZE_BYTES;
use omicron_common::limits::MIN_DISK_SIZE_BYTES;
use sled_agent_client::Client as SledAgentClient;
use std::sync::Arc;
use uuid::Uuid;

use super::MAX_DISK_SIZE_BYTES;
use super::MIN_DISK_SIZE_BYTES;

impl super::Nexus {
// Disks
pub fn disk_lookup<'a>(
Expand Down
12 changes: 6 additions & 6 deletions nexus/src/app/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,6 @@

//! Virtual Machine Instances

use super::MAX_DISKS_PER_INSTANCE;
use super::MAX_EXTERNAL_IPS_PER_INSTANCE;
use super::MAX_MEMORY_BYTES_PER_INSTANCE;
use super::MAX_NICS_PER_INSTANCE;
use super::MAX_VCPU_PER_INSTANCE;
use super::MIN_MEMORY_BYTES_PER_INSTANCE;
use crate::app::sagas;
use crate::app::sagas::retry_until_known_result;
use crate::authn;
Expand Down Expand Up @@ -42,6 +36,12 @@ use omicron_common::api::external::UpdateResult;
use omicron_common::api::external::Vni;
use omicron_common::api::internal::nexus;
use omicron_common::api::internal::shared::SwitchLocation;
use omicron_common::limits::MAX_DISKS_PER_INSTANCE;
use omicron_common::limits::MAX_EXTERNAL_IPS_PER_INSTANCE;
use omicron_common::limits::MAX_MEMORY_BYTES_PER_INSTANCE;
use omicron_common::limits::MAX_NICS_PER_INSTANCE;
use omicron_common::limits::MAX_VCPU_PER_INSTANCE;
use omicron_common::limits::MIN_MEMORY_BYTES_PER_INSTANCE;
use propolis_client::support::tungstenite::protocol::frame::coding::CloseCode;
use propolis_client::support::tungstenite::protocol::CloseFrame;
use propolis_client::support::tungstenite::Message as WebSocketMessage;
Expand Down
18 changes: 0 additions & 18 deletions nexus/src/app/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,24 +68,6 @@ mod vpc_subnet;
// application logic.
pub mod sagas;

// TODO: When referring to API types, we should try to include
// the prefix unless it is unambiguous.

pub(crate) use nexus_db_queries::db::queries::disk::MAX_DISKS_PER_INSTANCE;

pub(crate) const MAX_NICS_PER_INSTANCE: usize = 8;

// TODO-completeness: Support multiple external IPs
pub(crate) const MAX_EXTERNAL_IPS_PER_INSTANCE: usize = 1;

pub const MAX_VCPU_PER_INSTANCE: u16 = 64;

pub const MIN_MEMORY_BYTES_PER_INSTANCE: u32 = 1 << 30; // 1 GiB
pub const MAX_MEMORY_BYTES_PER_INSTANCE: u64 = 256 * (1 << 30); // 256 GiB

pub const MIN_DISK_SIZE_BYTES: u32 = 1 << 30; // 1 GiB
pub const MAX_DISK_SIZE_BYTES: u64 = 1023 * (1 << 30); // 1023 GiB

/// Manages an Oxide fleet -- the heart of the control plane
pub struct Nexus {
/// uuid for this nexus instance.
Expand Down
8 changes: 4 additions & 4 deletions nexus/src/app/sagas/instance_create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@ use crate::app::instance::WriteBackUpdatedInstance;
use crate::app::sagas::declare_saga_actions;
use crate::app::sagas::disk_create::{self, SagaDiskCreate};
use crate::app::sagas::retry_until_known_result;
use crate::app::{
MAX_DISKS_PER_INSTANCE, MAX_EXTERNAL_IPS_PER_INSTANCE,
MAX_NICS_PER_INSTANCE,
};
use crate::db::identity::Resource;
use crate::db::lookup::LookupPath;
use crate::db::model::ByteCount as DbByteCount;
Expand All @@ -29,6 +25,10 @@ use omicron_common::api::external::InstanceState;
use omicron_common::api::external::Name;
use omicron_common::api::internal::nexus::InstanceRuntimeState;
use omicron_common::api::internal::shared::SwitchLocation;
use omicron_common::limits::{
MAX_DISKS_PER_INSTANCE, MAX_EXTERNAL_IPS_PER_INSTANCE,
MAX_NICS_PER_INSTANCE,
};
use serde::Deserialize;
use serde::Serialize;
use sled_agent_client::types::InstanceStateRequested;
Expand Down
3 changes: 2 additions & 1 deletion nexus/tests/integration_tests/disks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ use omicron_common::api::external::IdentityMetadataCreateParams;
use omicron_common::api::external::Instance;
use omicron_common::api::external::Name;
use omicron_common::api::external::NameOrId;
use omicron_nexus::app::{MAX_DISK_SIZE_BYTES, MIN_DISK_SIZE_BYTES};
use omicron_common::limits::MAX_DISK_SIZE_BYTES;
use omicron_common::limits::MIN_DISK_SIZE_BYTES;
use omicron_nexus::db::fixed_data::{silo::SILO_ID, FLEET_ID};
use omicron_nexus::db::lookup::LookupPath;
use omicron_nexus::TestInterfaces as _;
Expand Down
6 changes: 3 additions & 3 deletions nexus/tests/integration_tests/instances.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ use omicron_common::api::external::InstanceState;
use omicron_common::api::external::Ipv4Net;
use omicron_common::api::external::Name;
use omicron_common::api::external::Vni;
use omicron_nexus::app::MAX_MEMORY_BYTES_PER_INSTANCE;
use omicron_nexus::app::MAX_VCPU_PER_INSTANCE;
use omicron_nexus::app::MIN_MEMORY_BYTES_PER_INSTANCE;
use omicron_common::limits::MAX_MEMORY_BYTES_PER_INSTANCE;
use omicron_common::limits::MAX_VCPU_PER_INSTANCE;
use omicron_common::limits::MIN_MEMORY_BYTES_PER_INSTANCE;
use omicron_nexus::db::fixed_data::silo::SILO_ID;
use omicron_nexus::db::lookup::LookupPath;
use omicron_nexus::external_api::shared::IpKind;
Expand Down
2 changes: 1 addition & 1 deletion nexus/tests/integration_tests/snapshots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use omicron_common::api::external::IdentityMetadataCreateParams;
use omicron_common::api::external::Instance;
use omicron_common::api::external::InstanceCpuCount;
use omicron_common::api::external::Name;
use omicron_nexus::app::MIN_DISK_SIZE_BYTES;
use omicron_common::limits::MIN_DISK_SIZE_BYTES;
use omicron_nexus::authz;
use omicron_nexus::db;
use omicron_nexus::db::identity::Resource;
Expand Down
48 changes: 43 additions & 5 deletions nexus/types/src/external_api/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,17 @@
use crate::external_api::shared;
use base64::Engine;
use chrono::{DateTime, Utc};
use omicron_common::api::external::{
AddressLotKind, ByteCount, IdentityMetadataCreateParams,
IdentityMetadataUpdateParams, InstanceCpuCount, IpNet, Ipv4Net, Ipv6Net,
Name, NameOrId, PaginationOrder, RouteDestination, RouteTarget,
SemverVersion,
use omicron_common::{
api::external::{
AddressLotKind, ByteCount, IdentityMetadataCreateParams,
IdentityMetadataUpdateParams, InstanceCpuCount, IpNet, Ipv4Net,
Ipv6Net, Name, NameOrId, PaginationOrder, RouteDestination,
RouteTarget, SemverVersion,
},
limits::{
MAX_DISK_SIZE_BYTES, MAX_MEMORY_BYTES_PER_INSTANCE,
MIN_DISK_SIZE_BYTES, MIN_MEMORY_BYTES_PER_INSTANCE,
},
};
use schemars::JsonSchema;
use serde::{
Expand Down Expand Up @@ -816,6 +822,7 @@ pub struct InstanceCreate {
#[serde(flatten)]
pub identity: IdentityMetadataCreateParams,
pub ncpus: InstanceCpuCount,
#[schemars(schema_with = "memory_limits")]
pub memory: ByteCount,
pub hostname: String, // TODO-cleanup different type?

Expand Down Expand Up @@ -859,6 +866,21 @@ fn bool_true() -> bool {
true
}

fn memory_limits(
gen: &mut schemars::gen::SchemaGenerator,
) -> schemars::schema::Schema {
let mut schema: schemars::schema::SchemaObject =
<ByteCount>::json_schema(gen).into();
let min_mem = MIN_MEMORY_BYTES_PER_INSTANCE / (1 << 30);
let max_mem = MAX_MEMORY_BYTES_PER_INSTANCE / (1 << 30);
schema.metadata().description = Some(
format!("the amount of memory to allocate to the instance, in bytes.\n\nMust be between {min_mem} and {max_mem} GiB.")
);
schema.number().minimum = Some(MIN_MEMORY_BYTES_PER_INSTANCE as f64);
schema.number().maximum = Some(MAX_MEMORY_BYTES_PER_INSTANCE as f64);
schema.into()
}

// If you change this, also update the error message in
// `UserData::deserialize()` below.
pub const MAX_USER_DATA_BYTES: usize = 32 * 1024; // 32 KiB
Expand Down Expand Up @@ -1151,9 +1173,25 @@ pub struct DiskCreate {
/// initial source for this disk
pub disk_source: DiskSource,
/// total size of the Disk in bytes
#[schemars(schema_with = "disk_size_limits")]
pub size: ByteCount,
}

fn disk_size_limits(
gen: &mut schemars::gen::SchemaGenerator,
) -> schemars::schema::Schema {
let mut schema: schemars::schema::SchemaObject =
ByteCount::json_schema(gen).into();
let min_disk = MIN_DISK_SIZE_BYTES / (1 << 30);
let max_disk = MAX_DISK_SIZE_BYTES / (1 << 30);
schema.metadata().description = Some(
format!("total size of the disk in bytes.\n\nMust be between {min_disk} and {max_disk} GiB.")
);
schema.number().minimum = Some(MIN_DISK_SIZE_BYTES as f64);
schema.number().maximum = Some(MAX_DISK_SIZE_BYTES as f64);
schema.into()
}

// equivalent to crucible_pantry_client::types::ExpectedDigest
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
#[serde(rename_all = "snake_case")]
Expand Down
3 changes: 2 additions & 1 deletion openapi/nexus-internal.json
Original file line number Diff line number Diff line change
Expand Up @@ -1850,7 +1850,8 @@
"description": "The number of CPUs in an Instance",
"type": "integer",
"format": "uint16",
"minimum": 0
"minimum": 1,
"maximum": 64
},
"InstanceRuntimeState": {
"description": "Runtime state of the Instance, including the actual running state and minimal metadata\n\nThis state is owned by the sled agent running that Instance.",
Expand Down
27 changes: 15 additions & 12 deletions openapi/nexus.json
Original file line number Diff line number Diff line change
Expand Up @@ -8544,11 +8544,10 @@
},
"size": {
"description": "total size of the Disk in bytes",
"allOf": [
{
"$ref": "#/components/schemas/ByteCount"
}
]
"type": "integer",
"format": "uint64",
"minimum": 1073741824,
"maximum": 1098437885952
}
},
"required": [
Expand Down Expand Up @@ -9601,7 +9600,8 @@
"description": "The number of CPUs in an Instance",
"type": "integer",
"format": "uint16",
"minimum": 0
"minimum": 1,
"maximum": 64
},
"InstanceCreate": {
"description": "Create-time parameters for an `Instance`",
Expand Down Expand Up @@ -9630,7 +9630,11 @@
"type": "string"
},
"memory": {
"$ref": "#/components/schemas/ByteCount"
"description": "the amount of memory to allocate to the instance, in bytes.\n\nMust be between 1 and 256 GiB.",
"type": "integer",
"format": "uint64",
"minimum": 1073741824,
"maximum": 274877906944
},
"name": {
"$ref": "#/components/schemas/Name"
Expand Down Expand Up @@ -9692,11 +9696,10 @@
},
"size": {
"description": "total size of the Disk in bytes",
"allOf": [
{
"$ref": "#/components/schemas/ByteCount"
}
]
"type": "integer",
"format": "uint64",
"minimum": 1073741824,
"maximum": 1098437885952
Comment on lines +12090 to +12091
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool to see this in the API, I hope this can make our clients better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this actually isn't right. It's not the min/max that's wrong, it's the schema. We still want ByteCount to be preserved but we also want the limits. It took me a little digging to figure out what to do here.

According to the spec two models can be composed into a single object via allOf. So what we actually want here is an allOf with the ByteCount and these limited definitions. My latest commit has something that should produce that, but typify pukes on it here:

https://github.com/oxidecomputer/typify/blob/1f97f167923f001818d461b1286f8a5242abf8b1/typify-impl/src/merge.rs#L690

I plan to look at this more tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean... the unimplemented! isn't wrong...

},
"type": {
"type": "string",
Expand Down
3 changes: 2 additions & 1 deletion openapi/sled-agent.json
Original file line number Diff line number Diff line change
Expand Up @@ -1344,7 +1344,8 @@
"description": "The number of CPUs in an Instance",
"type": "integer",
"format": "uint16",
"minimum": 0
"minimum": 1,
"maximum": 64
},
"InstanceEnsureBody": {
"description": "The body of a request to ensure that an instance is known to a sled agent.",
Expand Down