Skip to content

Commit

Permalink
Add a producer kind to oximeter metric producers (#4497)
Browse files Browse the repository at this point in the history
- Adds the `kind` enum to metric producer information, including DB
schema, model, and various client parameter types. This records the
supported types of metric producers, and is intended to aid debugging
and future work around updates and instance lifecycle management.
- Add schema update files which create the DB enum type and add it as a
column to the `metric_producer` table. This currently _drops_ the
existing table and recreates it with the new column, rather than adding
the column using `ALTER TABLE`. That is intended to remove old entries
in bulk, since nothing previously removed the records for Propolis
servers when their instance was stopped.

This is the initial PR in a sequence that will eventually make this
field _required_ in both the database and API requests. As there are
consumers of this API outside of the Omicron repository, this field
needs to start as optional, to avoid introducing a commit with
incompatible clients.
  • Loading branch information
bnaecker authored Nov 20, 2023
1 parent cd2d23b commit 08041d6
Show file tree
Hide file tree
Showing 20 changed files with 272 additions and 4 deletions.
14 changes: 14 additions & 0 deletions clients/nexus-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,19 @@ impl From<&types::InstanceState>
}
}

impl From<omicron_common::api::internal::nexus::ProducerKind>
for types::ProducerKind
{
fn from(kind: omicron_common::api::internal::nexus::ProducerKind) -> Self {
use omicron_common::api::internal::nexus::ProducerKind;
match kind {
ProducerKind::SledAgent => Self::SledAgent,
ProducerKind::Service => Self::Service,
ProducerKind::Instance => Self::Instance,
}
}
}

impl From<&omicron_common::api::internal::nexus::ProducerEndpoint>
for types::ProducerEndpoint
{
Expand All @@ -212,6 +225,7 @@ impl From<&omicron_common::api::internal::nexus::ProducerEndpoint>
address: s.address.to_string(),
base_route: s.base_route.clone(),
id: s.id,
kind: s.kind.map(Into::into),
interval: s.interval.into(),
}
}
Expand Down
14 changes: 14 additions & 0 deletions clients/oximeter-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,19 @@ impl From<std::time::Duration> for types::Duration {
}
}

impl From<omicron_common::api::internal::nexus::ProducerKind>
for types::ProducerKind
{
fn from(kind: omicron_common::api::internal::nexus::ProducerKind) -> Self {
use omicron_common::api::internal::nexus;
match kind {
nexus::ProducerKind::Service => Self::Service,
nexus::ProducerKind::SledAgent => Self::SledAgent,
nexus::ProducerKind::Instance => Self::Instance,
}
}
}

impl From<&omicron_common::api::internal::nexus::ProducerEndpoint>
for types::ProducerEndpoint
{
Expand All @@ -30,6 +43,7 @@ impl From<&omicron_common::api::internal::nexus::ProducerEndpoint>
address: s.address.to_string(),
base_route: s.base_route.clone(),
id: s.id,
kind: s.kind.map(Into::into),
interval: s.interval.into(),
}
}
Expand Down
21 changes: 21 additions & 0 deletions common/src/api/internal/nexus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,34 @@ pub struct SledInstanceState {

// Oximeter producer/collector objects.

/// The kind of metric producer this is.
#[derive(Clone, Copy, Debug, Deserialize, JsonSchema, PartialEq, Serialize)]
#[serde(rename_all = "snake_case")]
pub enum ProducerKind {
/// The producer is a sled-agent.
SledAgent,
/// The producer is an Omicron-managed service.
Service,
/// The producer is a Propolis VMM managing a guest instance.
Instance,
}

/// Information announced by a metric server, used so that clients can contact it and collect
/// available metric data from it.
#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize, PartialEq)]
pub struct ProducerEndpoint {
/// A unique ID for this producer.
pub id: Uuid,
/// The kind of producer.
pub kind: Option<ProducerKind>,
/// The IP address and port at which `oximeter` can collect metrics from the
/// producer.
pub address: SocketAddr,
/// The API base route from which `oximeter` can collect metrics.
///
/// The full route is `{base_route}/{id}`.
pub base_route: String,
/// The interval on which `oximeter` should collect metrics.
pub interval: Duration,
}

Expand Down
37 changes: 37 additions & 0 deletions nexus/db-model/src/producer_endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,47 @@
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use super::SqlU16;
use crate::impl_enum_type;
use crate::schema::metric_producer;
use db_macros::Asset;
use nexus_types::identity::Asset;
use omicron_common::api::internal;
use uuid::Uuid;

impl_enum_type!(
#[derive(SqlType, Copy, Clone, Debug, QueryId)]
#[diesel(postgres_type(name = "producer_kind"))]
pub struct ProducerKindEnum;

#[derive(AsExpression, Copy, Clone, Debug, FromSqlRow, PartialEq)]
#[diesel(sql_type = ProducerKindEnum)]
pub enum ProducerKind;

SledAgent => b"sled_agent"
Service => b"service"
Instance => b"instance"
);

impl From<internal::nexus::ProducerKind> for ProducerKind {
fn from(kind: internal::nexus::ProducerKind) -> Self {
match kind {
internal::nexus::ProducerKind::SledAgent => ProducerKind::SledAgent,
internal::nexus::ProducerKind::Service => ProducerKind::Service,
internal::nexus::ProducerKind::Instance => ProducerKind::Instance,
}
}
}

impl From<ProducerKind> for internal::nexus::ProducerKind {
fn from(kind: ProducerKind) -> Self {
match kind {
ProducerKind::SledAgent => internal::nexus::ProducerKind::SledAgent,
ProducerKind::Service => internal::nexus::ProducerKind::Service,
ProducerKind::Instance => internal::nexus::ProducerKind::Instance,
}
}
}

/// Information announced by a metric server, used so that clients can contact it and collect
/// available metric data from it.
#[derive(Queryable, Insertable, Debug, Clone, Selectable, Asset)]
Expand All @@ -17,6 +52,7 @@ pub struct ProducerEndpoint {
#[diesel(embed)]
identity: ProducerEndpointIdentity,

pub kind: Option<ProducerKind>,
pub ip: ipnetwork::IpNetwork,
pub port: SqlU16,
pub interval: f64,
Expand All @@ -33,6 +69,7 @@ impl ProducerEndpoint {
) -> Self {
Self {
identity: ProducerEndpointIdentity::new(endpoint.id),
kind: endpoint.kind.map(Into::into),
ip: endpoint.address.ip().into(),
port: endpoint.address.port().into(),
base_route: endpoint.base_route.clone(),
Expand Down
3 changes: 2 additions & 1 deletion nexus/db-model/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ table! {
id -> Uuid,
time_created -> Timestamptz,
time_modified -> Timestamptz,
kind -> Nullable<crate::ProducerKindEnum>,
ip -> Inet,
port -> Int4,
interval -> Float8,
Expand Down Expand Up @@ -1269,7 +1270,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(11, 0, 0);
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(12, 0, 0);

allow_tables_to_appear_in_same_query!(
system_update,
Expand Down
1 change: 1 addition & 0 deletions nexus/db-queries/src/db/datastore/oximeter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ impl DataStore {
.do_update()
.set((
dsl::time_modified.eq(Utc::now()),
dsl::kind.eq(producer.kind),
dsl::ip.eq(producer.ip),
dsl::port.eq(producer.port),
dsl::interval.eq(producer.interval),
Expand Down
4 changes: 4 additions & 0 deletions nexus/src/app/oximeter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ impl super::Nexus {
for producer in producers.into_iter() {
let producer_info = oximeter_client::types::ProducerEndpoint {
id: producer.id(),
kind: producer
.kind
.map(|kind| nexus::ProducerKind::from(kind).into()),
address: SocketAddr::new(
producer.ip.ip(),
producer.port.try_into().unwrap(),
Expand All @@ -149,6 +152,7 @@ impl super::Nexus {
pub(crate) async fn register_as_producer(&self, address: SocketAddr) {
let producer_endpoint = nexus::ProducerEndpoint {
id: self.id,
kind: Some(nexus::ProducerKind::Service),
address,
base_route: String::from("/metrics/collect"),
interval: Duration::from_secs(10),
Expand Down
2 changes: 2 additions & 0 deletions nexus/test-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use omicron_common::address::NEXUS_OPTE_IPV4_SUBNET;
use omicron_common::api::external::MacAddr;
use omicron_common::api::external::{IdentityMetadata, Name};
use omicron_common::api::internal::nexus::ProducerEndpoint;
use omicron_common::api::internal::nexus::ProducerKind;
use omicron_common::api::internal::shared::SwitchLocation;
use omicron_common::nexus_config;
use omicron_common::nexus_config::NUM_INITIAL_RESERVED_IP_ADDRESSES;
Expand Down Expand Up @@ -1092,6 +1093,7 @@ pub async fn start_producer_server(
let producer_address = SocketAddr::new(Ipv6Addr::LOCALHOST.into(), 0);
let server_info = ProducerEndpoint {
id,
kind: Some(ProducerKind::Service),
address: producer_address,
base_route: "/collect".to_string(),
interval: Duration::from_secs(1),
Expand Down
2 changes: 2 additions & 0 deletions nexus/tests/integration_tests/oximeter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use http::StatusCode;
use nexus_test_interface::NexusServer;
use nexus_test_utils_macros::nexus_test;
use omicron_common::api::internal::nexus::ProducerEndpoint;
use omicron_common::api::internal::nexus::ProducerKind;
use omicron_test_utils::dev::poll::{wait_for_condition, CondCheckError};
use oximeter_db::DbWrite;
use std::collections::BTreeSet;
Expand Down Expand Up @@ -360,6 +361,7 @@ async fn test_oximeter_collector_reregistration_gets_all_assignments() {
ids.insert(id);
let info = ProducerEndpoint {
id,
kind: Some(ProducerKind::Service),
address: SocketAddr::new(Ipv6Addr::LOCALHOST.into(), 12345),
base_route: String::from("/collect"),
interval: Duration::from_secs(1),
Expand Down
45 changes: 44 additions & 1 deletion openapi/nexus-internal.json
Original file line number Diff line number Diff line change
Expand Up @@ -4322,17 +4322,34 @@
"type": "object",
"properties": {
"address": {
"description": "The IP address and port at which `oximeter` can collect metrics from the producer.",
"type": "string"
},
"base_route": {
"description": "The API base route from which `oximeter` can collect metrics.\n\nThe full route is `{base_route}/{id}`.",
"type": "string"
},
"id": {
"description": "A unique ID for this producer.",
"type": "string",
"format": "uuid"
},
"interval": {
"$ref": "#/components/schemas/Duration"
"description": "The interval on which `oximeter` should collect metrics.",
"allOf": [
{
"$ref": "#/components/schemas/Duration"
}
]
},
"kind": {
"nullable": true,
"description": "The kind of producer.",
"allOf": [
{
"$ref": "#/components/schemas/ProducerKind"
}
]
}
},
"required": [
Expand All @@ -4342,6 +4359,32 @@
"interval"
]
},
"ProducerKind": {
"description": "The kind of metric producer this is.",
"oneOf": [
{
"description": "The producer is a sled-agent.",
"type": "string",
"enum": [
"sled_agent"
]
},
{
"description": "The producer is an Omicron-managed service.",
"type": "string",
"enum": [
"service"
]
},
{
"description": "The producer is a Propolis VMM managing a guest instance.",
"type": "string",
"enum": [
"instance"
]
}
]
},
"ProducerResultsItem": {
"oneOf": [
{
Expand Down
45 changes: 44 additions & 1 deletion openapi/oximeter.json
Original file line number Diff line number Diff line change
Expand Up @@ -191,17 +191,34 @@
"type": "object",
"properties": {
"address": {
"description": "The IP address and port at which `oximeter` can collect metrics from the producer.",
"type": "string"
},
"base_route": {
"description": "The API base route from which `oximeter` can collect metrics.\n\nThe full route is `{base_route}/{id}`.",
"type": "string"
},
"id": {
"description": "A unique ID for this producer.",
"type": "string",
"format": "uuid"
},
"interval": {
"$ref": "#/components/schemas/Duration"
"description": "The interval on which `oximeter` should collect metrics.",
"allOf": [
{
"$ref": "#/components/schemas/Duration"
}
]
},
"kind": {
"nullable": true,
"description": "The kind of producer.",
"allOf": [
{
"$ref": "#/components/schemas/ProducerKind"
}
]
}
},
"required": [
Expand Down Expand Up @@ -231,6 +248,32 @@
"required": [
"items"
]
},
"ProducerKind": {
"description": "The kind of metric producer this is.",
"oneOf": [
{
"description": "The producer is a sled-agent.",
"type": "string",
"enum": [
"sled_agent"
]
},
{
"description": "The producer is an Omicron-managed service.",
"type": "string",
"enum": [
"service"
]
},
{
"description": "The producer is a Propolis VMM managing a guest instance.",
"type": "string",
"enum": [
"instance"
]
}
]
}
},
"responses": {
Expand Down
4 changes: 4 additions & 0 deletions oximeter/collector/src/agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,7 @@ mod tests {
use hyper::Response;
use hyper::Server;
use hyper::StatusCode;
use omicron_common::api::internal::nexus::ProducerKind;
use omicron_test_utils::dev::test_setup_log;
use std::convert::Infallible;
use std::net::Ipv6Addr;
Expand Down Expand Up @@ -694,6 +695,7 @@ mod tests {
let interval = Duration::from_secs(1);
let endpoint = ProducerEndpoint {
id: Uuid::new_v4(),
kind: Some(ProducerKind::Service),
address,
base_route: String::from("/"),
interval,
Expand Down Expand Up @@ -752,6 +754,7 @@ mod tests {
let interval = Duration::from_secs(1);
let endpoint = ProducerEndpoint {
id: Uuid::new_v4(),
kind: Some(ProducerKind::Service),
address: SocketAddr::V6(SocketAddrV6::new(
Ipv6Addr::LOCALHOST,
0,
Expand Down Expand Up @@ -840,6 +843,7 @@ mod tests {
let interval = Duration::from_secs(1);
let endpoint = ProducerEndpoint {
id: Uuid::new_v4(),
kind: Some(ProducerKind::Service),
address,
base_route: String::from("/"),
interval,
Expand Down
Loading

0 comments on commit 08041d6

Please sign in to comment.