Support Bundles: Order by creation time (#8575)

Fixes #8551

Author: smklein

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<Vec<KnownVersion>> = 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"),

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<chrono::Utc>, Uuid)>,
     ) -> ListResultVec<SupportBundle> {
         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();
+    }
 }

nexus/external-api/src/lib.rs

@@ -3237,7 +3237,7 @@ pub trait NexusExternalApi {
     }]
     async fn support_bundle_list(
         rqctx: RequestContext<Self::Context>,
-        query_params: Query<PaginatedById>,
+        query_params: Query<PaginatedByTimeAndId>,
     ) -> Result<HttpResponseOk<ResultsPage<shared::SupportBundleInfo>>, HttpError>;
 
     /// View a support bundle

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<Self::Context>,
-        query_params: Query<PaginatedById>,
+        query_params: Query<PaginatedByTimeAndId>,
     ) -> Result<HttpResponseOk<ResultsPage<shared::SupportBundleInfo>>, HttpError>;
 
     /// View a support bundle

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<chrono::Utc>, Uuid)>,
     ) -> ListResultVec<SupportBundle> {
         self.db_datastore.support_bundle_list(&opctx, pagparams).await
     }

nexus/src/external_api/http_entrypoints.rs

@@ -7203,7 +7203,7 @@ impl NexusExternalApi for NexusExternalApiImpl {
 
     async fn support_bundle_list(
         rqctx: RequestContext<ApiContext>,
-        query_params: Query<PaginatedById>,
+        query_params: Query<PaginatedByTimeAndId>,
     ) -> Result<HttpResponseOk<ResultsPage<shared::SupportBundleInfo>>, 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())
                 },
             )?))
         };

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<ApiContext>,
-        query_params: Query<PaginatedById>,
+        query_params: Query<PaginatedByTimeAndId>,
     ) -> Result<HttpResponseOk<ResultsPage<shared::SupportBundleInfo>>, 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())
                 },
             )?))
         };

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
+        );
+    }
+}

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"

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": [