From b00724d744cb1659a63b46abae1166e6a8b180fe Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 10 Jul 2025 16:00:29 -0700 Subject: [PATCH] Order support bundles by creation time --- nexus/db-model/src/schema_versions.rs | 3 +- .../src/db/datastore/support_bundle.rs | 85 +++++++++++++++++-- nexus/external-api/src/lib.rs | 2 +- nexus/internal-api/src/lib.rs | 7 +- nexus/src/app/support_bundles.rs | 2 +- nexus/src/external_api/http_entrypoints.rs | 6 +- nexus/src/internal_api/http_entrypoints.rs | 8 +- .../integration_tests/support_bundles.rs | 49 +++++++++++ openapi/nexus-internal.json | 21 ++++- openapi/nexus.json | 26 +++--- schema/crdb/bundle-by-creation/up01.sql | 4 + schema/crdb/dbinit.sql | 6 +- 12 files changed, 186 insertions(+), 33 deletions(-) create mode 100644 schema/crdb/bundle-by-creation/up01.sql diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 7a67315c41e..08e75a7bf6c 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(161, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(162, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = LazyLock::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(162, "bundle-by-creation"), KnownVersion::new(161, "inv_cockroachdb_status"), KnownVersion::new(160, "tuf-trust-root"), KnownVersion::new(159, "sled-config-desired-host-phase-2"), diff --git a/nexus/db-queries/src/db/datastore/support_bundle.rs b/nexus/db-queries/src/db/datastore/support_bundle.rs index f45b3508f42..30e10aecc39 100644 --- a/nexus/db-queries/src/db/datastore/support_bundle.rs +++ b/nexus/db-queries/src/db/datastore/support_bundle.rs @@ -11,6 +11,7 @@ use crate::db::model::RendezvousDebugDataset; use crate::db::model::SupportBundle; use crate::db::model::SupportBundleState; use crate::db::pagination::paginated; +use crate::db::pagination::paginated_multicolumn; use crate::db::update_and_check::{UpdateAndCheck, UpdateStatus}; use async_bb8_diesel::AsyncRunQueryDsl; use diesel::prelude::*; @@ -166,21 +167,25 @@ impl DataStore { Ok(db_bundle) } - /// Lists one page of support bundles + /// Lists one page of support bundles ordered by creation time pub async fn support_bundle_list( &self, opctx: &OpContext, - pagparams: &DataPageParams<'_, Uuid>, + pagparams: &DataPageParams<'_, (chrono::DateTime, Uuid)>, ) -> ListResultVec { opctx.authorize(authz::Action::Read, &authz::FLEET).await?; use nexus_db_schema::schema::support_bundle::dsl; let conn = self.pool_connection_authorized(opctx).await?; - paginated(dsl::support_bundle, dsl::id, pagparams) - .select(SupportBundle::as_select()) - .load_async(&*conn) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + paginated_multicolumn( + dsl::support_bundle, + (dsl::time_created, dsl::id), + pagparams, + ) + .select(SupportBundle::as_select()) + .load_async(&*conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } /// Lists one page of support bundles in a particular state, assigned to @@ -1417,4 +1422,70 @@ mod test { db.terminate().await; logctx.cleanup_successful(); } + + #[tokio::test] + async fn test_bundle_list_time_ordering() { + let logctx = dev::test_setup_log("test_bundle_list_time_ordering"); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + let _test_sled = create_sled_and_zpools(&datastore, &opctx, 3).await; + let this_nexus_id = OmicronZoneUuid::new_v4(); + + // Create multiple bundles with slight time delays to ensure different creation times + let mut bundle_ids = Vec::new(); + let mut bundle_times = Vec::new(); + + for _i in 0..3 { + let bundle = datastore + .support_bundle_create( + &opctx, + "Bundle for time ordering test", + this_nexus_id, + ) + .await + .expect("Should be able to create bundle"); + bundle_ids.push(bundle.id); + bundle_times.push(bundle.time_created); + + // Small delay to ensure different creation times + tokio::time::sleep(tokio::time::Duration::from_millis(10)).await; + } + + // List bundles using time-based pagination + let pagparams = DataPageParams::max_page(); + let observed_bundles = datastore + .support_bundle_list(&opctx, &pagparams) + .await + .expect("Should be able to list bundles"); + + assert_eq!(3, observed_bundles.len()); + + // Verify bundles are ordered by creation time (ascending) + for i in 0..observed_bundles.len() - 1 { + assert!( + observed_bundles[i].time_created + <= observed_bundles[i + 1].time_created, + "Bundles should be ordered by creation time (ascending). Bundle at index {} has time {:?}, but bundle at index {} has time {:?}", + i, + observed_bundles[i].time_created, + i + 1, + observed_bundles[i + 1].time_created + ); + } + + // Verify that the bundles are our created bundles + let returned_ids: Vec<_> = + observed_bundles.iter().map(|b| b.id).collect(); + for bundle_id in &bundle_ids { + assert!( + returned_ids.contains(bundle_id), + "Bundle ID {:?} should be in the returned list", + bundle_id + ); + } + + db.terminate().await; + logctx.cleanup_successful(); + } } diff --git a/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs index adb7457fa16..d6f61ca6d6d 100644 --- a/nexus/external-api/src/lib.rs +++ b/nexus/external-api/src/lib.rs @@ -3237,7 +3237,7 @@ pub trait NexusExternalApi { }] async fn support_bundle_list( rqctx: RequestContext, - query_params: Query, + query_params: Query, ) -> Result>, HttpError>; /// View a support bundle diff --git a/nexus/internal-api/src/lib.rs b/nexus/internal-api/src/lib.rs index ffa6165828a..69b4c214564 100644 --- a/nexus/internal-api/src/lib.rs +++ b/nexus/internal-api/src/lib.rs @@ -34,7 +34,10 @@ use nexus_types::{ }, }; use omicron_common::api::{ - external::{Instance, http_pagination::PaginatedById}, + external::{ + Instance, + http_pagination::{PaginatedById, PaginatedByTimeAndId}, + }, internal::nexus::{ DiskRuntimeState, DownstairsClientStopRequest, DownstairsClientStopped, ProducerEndpoint, ProducerRegistrationResponse, RepairFinishInfo, @@ -587,7 +590,7 @@ pub trait NexusInternalApi { }] async fn support_bundle_list( rqctx: RequestContext, - query_params: Query, + query_params: Query, ) -> Result>, HttpError>; /// View a support bundle diff --git a/nexus/src/app/support_bundles.rs b/nexus/src/app/support_bundles.rs index 1451fd9cb25..69c5db77b07 100644 --- a/nexus/src/app/support_bundles.rs +++ b/nexus/src/app/support_bundles.rs @@ -39,7 +39,7 @@ impl super::Nexus { pub async fn support_bundle_list( &self, opctx: &OpContext, - pagparams: &DataPageParams<'_, Uuid>, + pagparams: &DataPageParams<'_, (chrono::DateTime, Uuid)>, ) -> ListResultVec { self.db_datastore.support_bundle_list(&opctx, pagparams).await } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 315088a634f..d2ba100882a 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7203,7 +7203,7 @@ impl NexusExternalApi for NexusExternalApiImpl { async fn support_bundle_list( rqctx: RequestContext, - query_params: Query, + query_params: Query, ) -> Result>, HttpError> { let apictx = rqctx.context(); @@ -7223,11 +7223,11 @@ impl NexusExternalApi for NexusExternalApiImpl { .map(|p| p.into()) .collect(); - Ok(HttpResponseOk(ScanById::results_page( + Ok(HttpResponseOk(ScanByTimeAndId::results_page( &query, bundles, &|_, bundle: &shared::SupportBundleInfo| { - bundle.id.into_untyped_uuid() + (bundle.time_created, bundle.id.into_untyped_uuid()) }, )?)) }; diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index c3afdbb856e..c0e2a5a29f0 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -53,7 +53,9 @@ use nexus_types::internal_api::views::UpdateStatus; use nexus_types::internal_api::views::to_list; use omicron_common::api::external::Instance; use omicron_common::api::external::http_pagination::PaginatedById; +use omicron_common::api::external::http_pagination::PaginatedByTimeAndId; use omicron_common::api::external::http_pagination::ScanById; +use omicron_common::api::external::http_pagination::ScanByTimeAndId; use omicron_common::api::external::http_pagination::ScanParams; use omicron_common::api::external::http_pagination::data_page_params_for; use omicron_common::api::internal::nexus::DiskRuntimeState; @@ -1029,7 +1031,7 @@ impl NexusInternalApi for NexusInternalApiImpl { async fn support_bundle_list( rqctx: RequestContext, - query_params: Query, + query_params: Query, ) -> Result>, HttpError> { let apictx = rqctx.context(); @@ -1049,11 +1051,11 @@ impl NexusInternalApi for NexusInternalApiImpl { .map(|p| p.into()) .collect(); - Ok(HttpResponseOk(ScanById::results_page( + Ok(HttpResponseOk(ScanByTimeAndId::results_page( &query, bundles, &|_, bundle: &shared::SupportBundleInfo| { - bundle.id.into_untyped_uuid() + (bundle.time_created, bundle.id.into_untyped_uuid()) }, )?)) }; diff --git a/nexus/tests/integration_tests/support_bundles.rs b/nexus/tests/integration_tests/support_bundles.rs index 305e551e25a..878ae588350 100644 --- a/nexus/tests/integration_tests/support_bundles.rs +++ b/nexus/tests/integration_tests/support_bundles.rs @@ -591,3 +591,52 @@ async fn test_support_bundle_range_requests( .unwrap(); assert_eq!(second_half, full_contents[first_half.len()..]); } + +// Test that support bundle listing returns bundles ordered by creation time +#[nexus_test] +async fn test_support_bundle_list_time_ordering( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + // Create a disk test with multiple zpools to allow multiple bundles + let _disk_test = + DiskTestBuilder::new(&cptestctx).with_zpool_count(3).build().await; + + // Create multiple bundles with delays to ensure different creation times + let mut bundle_ids = Vec::new(); + + for _ in 0..3 { + let bundle = bundle_create(&client).await.unwrap(); + bundle_ids.push(bundle.id); + + // Small delay to ensure different creation times + tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; + } + + // List all bundles + let bundles = bundles_list(&client).await.unwrap(); + assert_eq!(bundles.len(), 3, "Should have created 3 bundles"); + + // Verify bundles are ordered by creation time (ascending) + for i in 0..bundles.len() - 1 { + assert!( + bundles[i].time_created <= bundles[i + 1].time_created, + "Bundles should be ordered by creation time (ascending). Bundle at index {} has time {:?}, but bundle at index {} has time {:?}", + i, + bundles[i].time_created, + i + 1, + bundles[i + 1].time_created + ); + } + + // Verify that all our created bundles are present + let returned_ids: Vec<_> = bundles.iter().map(|b| b.id).collect(); + for bundle_id in &bundle_ids { + assert!( + returned_ids.contains(bundle_id), + "Bundle ID {:?} should be in the returned list", + bundle_id + ); + } +} diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 13e578cc2f3..22be1853c81 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -882,7 +882,7 @@ "in": "query", "name": "sort_by", "schema": { - "$ref": "#/components/schemas/IdSortMode" + "$ref": "#/components/schemas/TimeAndIdSortMode" } } ], @@ -7961,6 +7961,25 @@ } ] }, + "TimeAndIdSortMode": { + "description": "Supported set of sort modes for scanning by timestamp and ID", + "oneOf": [ + { + "description": "sort in increasing order of timestamp and ID, i.e., earliest first", + "type": "string", + "enum": [ + "ascending" + ] + }, + { + "description": "sort in increasing order of timestamp and ID, i.e., most recent first", + "type": "string", + "enum": [ + "descending" + ] + } + ] + }, "TypedUuidForPropolisKind": { "type": "string", "format": "uuid" diff --git a/openapi/nexus.json b/openapi/nexus.json index aa5a695947b..1422e372d82 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -333,7 +333,7 @@ "in": "query", "name": "sort_by", "schema": { - "$ref": "#/components/schemas/IdSortMode" + "$ref": "#/components/schemas/TimeAndIdSortMode" } } ], @@ -27147,18 +27147,6 @@ } ] }, - "IdSortMode": { - "description": "Supported set of sort modes for scanning by id only.\n\nCurrently, we only support scanning in ascending order.", - "oneOf": [ - { - "description": "sort in increasing order of \"id\"", - "type": "string", - "enum": [ - "id_ascending" - ] - } - ] - }, "TimeAndIdSortMode": { "description": "Supported set of sort modes for scanning by timestamp and ID", "oneOf": [ @@ -27197,6 +27185,18 @@ "descending" ] }, + "IdSortMode": { + "description": "Supported set of sort modes for scanning by id only.\n\nCurrently, we only support scanning in ascending order.", + "oneOf": [ + { + "description": "sort in increasing order of \"id\"", + "type": "string", + "enum": [ + "id_ascending" + ] + } + ] + }, "SystemMetricName": { "type": "string", "enum": [ diff --git a/schema/crdb/bundle-by-creation/up01.sql b/schema/crdb/bundle-by-creation/up01.sql new file mode 100644 index 00000000000..a49473ebe29 --- /dev/null +++ b/schema/crdb/bundle-by-creation/up01.sql @@ -0,0 +1,4 @@ +CREATE INDEX IF NOT EXISTS lookup_bundle_by_creation ON omicron.public.support_bundle ( + time_created +); + diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 39ea3574f13..ac02bf92f30 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2664,6 +2664,10 @@ CREATE INDEX IF NOT EXISTS lookup_bundle_by_nexus ON omicron.public.support_bund assigned_nexus ); +CREATE INDEX IF NOT EXISTS lookup_bundle_by_creation ON omicron.public.support_bundle ( + time_created +); + /*******************************************************************/ /* @@ -6198,7 +6202,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '161.0.0', NULL) + (TRUE, NOW(), NOW(), '162.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT;