Delete disk metrics endpoint (#8721)

Closes #8720 

We'll have to confirm no one is relying on this endpoint, but as far as
I know, the only thing relying on it was the instance disk metrics page
in console, and that was switched to OxQL in February in
oxidecomputer/console#2654.

Author: david-crespo

nexus/external-api/output/nexus_tags.txt

@@ -35,7 +35,6 @@ disk_create                              POST     /v1/disks
 disk_delete                              DELETE   /v1/disks/{disk}
 disk_finalize_import                     POST     /v1/disks/{disk}/finalize
 disk_list                                GET      /v1/disks
-disk_metrics_list                        GET      /v1/disks/{disk}/metrics/{metric}
 disk_view                                GET      /v1/disks/{disk}
 
 API operations found with tag "experimental"

nexus/external-api/src/lib.rs

@@ -1048,24 +1048,6 @@ pub trait NexusExternalApi {
         query_params: Query<params::OptionalProjectSelector>,
     ) -> Result<HttpResponseDeleted, HttpError>;
 
-    /// Fetch disk metrics
-    #[endpoint {
-        method = GET,
-        path = "/v1/disks/{disk}/metrics/{metric}",
-        tags = ["disks"],
-    }]
-    async fn disk_metrics_list(
-        rqctx: RequestContext<Self::Context>,
-        path_params: Path<params::DiskMetricsPath>,
-        query_params: Query<
-            PaginationParams<params::ResourceMetrics, params::ResourceMetrics>,
-        >,
-        selector_params: Query<params::OptionalProjectSelector>,
-    ) -> Result<
-        HttpResponseOk<ResultsPage<oximeter_types::Measurement>>,
-        HttpError,
-    >;
-
     /// Start importing blocks into disk
     ///
     /// Start the process of importing blocks into a disk

nexus/src/external_api/http_entrypoints.rs

@@ -1921,52 +1921,6 @@ impl NexusExternalApi for NexusExternalApiImpl {
             .await
     }
 
-    async fn disk_metrics_list(
-        rqctx: RequestContext<ApiContext>,
-        path_params: Path<params::DiskMetricsPath>,
-        query_params: Query<
-            PaginationParams<params::ResourceMetrics, params::ResourceMetrics>,
-        >,
-        selector_params: Query<params::OptionalProjectSelector>,
-    ) -> Result<HttpResponseOk<ResultsPage<oximeter_db::Measurement>>, HttpError>
-    {
-        let apictx = rqctx.context();
-        let handler = async {
-            let nexus = &apictx.context.nexus;
-            let path = path_params.into_inner();
-            let query = query_params.into_inner();
-
-            let selector = selector_params.into_inner();
-            let limit = rqctx.page_limit(&query)?;
-            let disk_selector = params::DiskSelector {
-                disk: path.disk,
-                project: selector.project,
-            };
-            let opctx =
-                crate::context::op_context_for_external_api(&rqctx).await?;
-            let (.., authz_disk) = nexus
-                .disk_lookup(&opctx, disk_selector)?
-                .lookup_for(authz::Action::Read)
-                .await?;
-
-            let result = nexus
-                .select_timeseries(
-                    &format!("crucible_upstairs:{}", path.metric),
-                    &[&format!("upstairs_uuid=={}", authz_disk.id())],
-                    query,
-                    limit,
-                )
-                .await?;
-
-            Ok(HttpResponseOk(result))
-        };
-        apictx
-            .context
-            .external_latencies
-            .instrument_dropshot_handler(&rqctx, handler)
-            .await
-    }
-
     async fn disk_bulk_write_import_start(
         rqctx: RequestContext<ApiContext>,
         path_params: Path<params::DiskPath>,

nexus/tests/integration_tests/disks.rs

@@ -5,9 +5,6 @@
 //! Tests basic disk support in the API
 
 use super::instances::instance_wait_for_state;
-use super::metrics_querier::MetricsNotYet;
-use super::metrics_querier::MetricsQuerier;
-use chrono::Utc;
 use dropshot::HttpErrorResponseBody;
 use dropshot::test_util::ClientTestContext;
 use http::StatusCode;
@@ -22,36 +19,30 @@ use nexus_db_queries::db::datastore::RegionAllocationParameters;
 use nexus_db_queries::db::fixed_data::FLEET_ID;
 use nexus_test_utils::SLED_AGENT_UUID;
 use nexus_test_utils::http_testing::AuthnMode;
-use nexus_test_utils::http_testing::Collection;
 use nexus_test_utils::http_testing::NexusRequest;
 use nexus_test_utils::http_testing::RequestBuilder;
 use nexus_test_utils::identity_eq;
 use nexus_test_utils::resource_helpers::create_default_ip_pool;
 use nexus_test_utils::resource_helpers::create_disk;
 use nexus_test_utils::resource_helpers::create_instance;
-use nexus_test_utils::resource_helpers::create_instance_with;
 use nexus_test_utils::resource_helpers::create_project;
-use nexus_test_utils::resource_helpers::objects_list_page_authz;
-use nexus_test_utils::wait_for_producer;
 use nexus_test_utils_macros::nexus_test;
 use nexus_types::external_api::params;
 use nexus_types::identity::Asset;
 use nexus_types::silo::DEFAULT_SILO_ID;
+use omicron_common::api::external::ByteCount;
 use omicron_common::api::external::Disk;
 use omicron_common::api::external::DiskState;
 use omicron_common::api::external::IdentityMetadataCreateParams;
 use omicron_common::api::external::Instance;
 use omicron_common::api::external::InstanceState;
 use omicron_common::api::external::Name;
 use omicron_common::api::external::NameOrId;
-use omicron_common::api::external::{ByteCount, SimpleIdentityOrName as _};
 use omicron_nexus::Nexus;
 use omicron_nexus::TestInterfaces as _;
 use omicron_nexus::app::{MAX_DISK_SIZE_BYTES, MIN_DISK_SIZE_BYTES};
 use omicron_uuid_kinds::VolumeUuid;
 use omicron_uuid_kinds::{GenericUuid, InstanceUuid};
-use oximeter::types::Datum;
-use oximeter::types::Measurement;
 use sled_agent_client::TestInterfaces as _;
 use std::collections::HashSet;
 use std::sync::Arc;
@@ -1752,193 +1743,6 @@ async fn test_multiple_disks_multiple_zpools(
     .unwrap();
 }
 
-async fn create_instance_with_disk(client: &ClientTestContext) {
-    create_instance_with(
-        &client,
-        PROJECT_NAME,
-        INSTANCE_NAME,
-        &params::InstanceNetworkInterfaceAttachment::Default,
-        vec![params::InstanceDiskAttachment::Attach(
-            params::InstanceDiskAttach { name: DISK_NAME.parse().unwrap() },
-        )],
-        Vec::<params::ExternalIpCreate>::new(),
-        true,
-        Default::default(),
-    )
-    .await;
-}
-
-const ALL_METRICS: [&'static str; 6] =
-    ["activated", "read", "write", "read_bytes", "write_bytes", "flush"];
-
-#[nexus_test]
-async fn test_disk_metrics(cptestctx: &ControlPlaneTestContext) {
-    let metrics_querier = MetricsQuerier::new(cptestctx);
-    let client = &cptestctx.external_client;
-    DiskTest::new(&cptestctx).await;
-    let project_id = create_project_and_pool(client).await;
-    let disk = create_disk(&client, PROJECT_NAME, DISK_NAME).await;
-
-    // When grabbing a metric, we look for data points going back to the
-    // start of this test all the way up to the current time.
-    let metric_url = |metric: &str| {
-        format!(
-            "/v1/disks/{}/metrics/{}?start_time={:?}&end_time={:?}&project={}",
-            DISK_NAME,
-            metric,
-            cptestctx.start_time,
-            Utc::now(),
-            PROJECT_NAME,
-        )
-    };
-
-    // Try accessing metrics before we attach the disk to an instance.
-    //
-    // Observe that no metrics exist yet; no "upstairs" should have been
-    // instantiated on a sled.
-    let measurements =
-        objects_list_page_authz::<Measurement>(client, &metric_url("read"))
-            .await;
-    assert!(measurements.items.is_empty());
-
-    metrics_querier
-        .wait_for_latest_silo_metric(
-            "virtual_disk_space_provisioned",
-            Some(project_id),
-            |measurement| {
-                if measurement == i64::from(disk.size) {
-                    Ok(())
-                } else {
-                    Err(MetricsNotYet::new(format!(
-                        "waiting for virtual_disk_space_provisioned={} \
-                         (currently {measurement})",
-                        disk.size,
-                    )))
-                }
-            },
-        )
-        .await;
-
-    // Create an instance, attach the disk to it.
-    create_instance_with_disk(client).await;
-    wait_for_producer(&cptestctx.oximeter, disk.id()).await;
-
-    for metric in &ALL_METRICS {
-        metrics_querier
-            .wait_for_disk_metric(PROJECT_NAME, DISK_NAME, metric, |items| {
-                if items.is_empty() {
-                    return Err(MetricsNotYet::new(format!(
-                        "waiting for at least one item for metric={metric}"
-                    )));
-                }
-                for item in &items {
-                    let cumulative = match item.datum() {
-                        Datum::CumulativeI64(c) => c,
-                        _ => panic!("Unexpected datum type {:?}", item.datum()),
-                    };
-                    assert!(cumulative.start_time() <= item.timestamp());
-                }
-                Ok(())
-            })
-            .await;
-    }
-
-    // Check the utilization info for the whole project too.
-    metrics_querier
-        .wait_for_latest_silo_metric(
-            "virtual_disk_space_provisioned",
-            Some(project_id),
-            |measurement| {
-                if measurement == i64::from(disk.size) {
-                    Ok(())
-                } else {
-                    Err(MetricsNotYet::new(format!(
-                        "waiting for virtual_disk_space_provisioned={} \
-                         (currently {measurement})",
-                        disk.size,
-                    )))
-                }
-            },
-        )
-        .await;
-}
-
-#[nexus_test]
-async fn test_disk_metrics_paginated(cptestctx: &ControlPlaneTestContext) {
-    let client = &cptestctx.external_client;
-    DiskTest::new(&cptestctx).await;
-    create_project_and_pool(client).await;
-    let disk = create_disk(&client, PROJECT_NAME, DISK_NAME).await;
-    create_instance_with_disk(client).await;
-    wait_for_producer(&cptestctx.oximeter, disk.id()).await;
-
-    let metrics_querier = MetricsQuerier::new(cptestctx);
-    for metric in &ALL_METRICS {
-        // Wait until we have at least two measurements.
-        metrics_querier
-            .wait_for_disk_metric(
-                PROJECT_NAME,
-                DISK_NAME,
-                metric,
-                |measurements| {
-                    let num_measurements = measurements.len();
-                    if num_measurements >= 2 {
-                        Ok(())
-                    } else {
-                        Err(MetricsNotYet::new(format!(
-                            "waiting for at least 2 measurements \
-                             (currently {num_measurements})"
-                        )))
-                    }
-                },
-            )
-            .await;
-
-        let collection_url = format!(
-            "/v1/disks/{}/metrics/{}?project={}",
-            DISK_NAME, metric, PROJECT_NAME
-        );
-        let initial_params = format!(
-            "start_time={:?}&end_time={:?}",
-            cptestctx.start_time,
-            Utc::now(),
-        );
-
-        let measurements_paginated: Collection<Measurement> =
-            NexusRequest::iter_collection_authn(
-                client,
-                &collection_url,
-                &initial_params,
-                Some(10),
-            )
-            .await
-            .expect("failed to iterate over metrics");
-        assert!(!measurements_paginated.all_items.is_empty());
-
-        let mut last_timestamp = None;
-        let mut last_value = None;
-        for item in &measurements_paginated.all_items {
-            let cumulative = match item.datum() {
-                Datum::CumulativeI64(c) => c,
-                _ => panic!("Unexpected datum type {:?}", item.datum()),
-            };
-            assert!(cumulative.start_time() <= item.timestamp());
-
-            // Validate that the timestamps are non-decreasing.
-            if let Some(last_ts) = last_timestamp {
-                assert!(last_ts <= item.timestamp());
-            }
-            // Validate that the values increase.
-            if let Some(last_value) = last_value {
-                assert!(last_value < cumulative.value());
-            }
-
-            last_timestamp = Some(item.timestamp());
-            last_value = Some(cumulative.value());
-        }
-    }
-}
-
 #[nexus_test]
 async fn test_disk_create_for_importing(cptestctx: &ControlPlaneTestContext) {
     let client = &cptestctx.external_client;

nexus/tests/integration_tests/endpoints.rs

@@ -401,15 +401,6 @@ pub static DEMO_DISK_CREATE: LazyLock<params::DiskCreate> =
             ),
         }
     });
-pub static DEMO_DISK_METRICS_URL: LazyLock<String> = LazyLock::new(|| {
-    format!(
-        "/v1/disks/{}/metrics/activated?start_time={:?}&end_time={:?}&{}",
-        *DEMO_DISK_NAME,
-        Utc::now(),
-        Utc::now(),
-        *DEMO_PROJECT_SELECTOR,
-    )
-});
 
 // Related to importing blocks from an external source
 pub static DEMO_IMPORT_DISK_NAME: LazyLock<Name> =
@@ -2048,12 +2039,6 @@ pub static VERIFY_ENDPOINTS: LazyLock<Vec<VerifyEndpoint>> = LazyLock::new(
                     AllowedMethod::Delete,
                 ],
             },
-            VerifyEndpoint {
-                url: &DEMO_DISK_METRICS_URL,
-                visibility: Visibility::Protected,
-                unprivileged_access: UnprivilegedAccess::None,
-                allowed_methods: vec![AllowedMethod::Get],
-            },
             VerifyEndpoint {
                 url: &DEMO_INSTANCE_DISKS_URL,
                 visibility: Visibility::Protected,

nexus/tests/integration_tests/metrics_querier.rs

@@ -143,27 +143,6 @@ impl<'a, N> MetricsQuerier<'a, N> {
         self.wait_for_objects(|| endpoint.to_string(), cond).await
     }
 
-    /// Repeatedly fetch a specific disk metric until `cond` returns `Ok(_)`.
-    pub async fn wait_for_disk_metric<F, T>(
-        &self,
-        project_name: &str,
-        disk_name: &str,
-        metric_name: &str,
-        cond: F,
-    ) -> T
-    where
-        F: Fn(Vec<Measurement>) -> Result<T, MetricsNotYet>,
-    {
-        let endpoint = || {
-            format!(
-                "/v1/disks/{disk_name}/metrics/{metric_name}?start_time={:?}&end_time={:?}&project={project_name}",
-                self.ctx.start_time,
-                Utc::now(),
-            )
-        };
-        self.wait_for_objects(endpoint, cond).await
-    }
-
     /// Repeatedly fetch the single newest system metric until `cond` returns
     /// `Ok(_)`.
     pub async fn wait_for_latest_system_metric<F, T>(