Skip to content

Commit 001a5e8

Browse files
smkleinsmklein
committed
Order support bundles by creation time
1 parent 0c4b5a8 commit 001a5e8

File tree

12 files changed

+186
-33
lines changed

12 files changed

+186
-33
lines changed

‎nexus/db-model/src/schema_versions.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock};
1616
///
1717
/// This must be updated when you change the database schema. Refer to
1818
/// schema/crdb/README.adoc in the root of this repository for details.
19-
pub const SCHEMA_VERSION: Version = Version::new(161, 0, 0);
19+
pub const SCHEMA_VERSION: Version = Version::new(162, 0, 0);
2020

2121
/// List of all past database schema versions, in *reverse* order
2222
///
@@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = LazyLock::new(|| {
2828
// | leaving the first copy as an example for the next person.
2929
// v
3030
// KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
31+
KnownVersion::new(162, "bundle-by-creation"),
3132
KnownVersion::new(161, "inv_cockroachdb_status"),
3233
KnownVersion::new(160, "tuf-trust-root"),
3334
KnownVersion::new(159, "sled-config-desired-host-phase-2"),

‎nexus/db-queries/src/db/datastore/support_bundle.rs

Lines changed: 78 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ use crate::db::model::RendezvousDebugDataset;
1111
use crate::db::model::SupportBundle;
1212
use crate::db::model::SupportBundleState;
1313
use crate::db::pagination::paginated;
14+
use crate::db::pagination::paginated_multicolumn;
1415
use crate::db::update_and_check::{UpdateAndCheck, UpdateStatus};
1516
use async_bb8_diesel::AsyncRunQueryDsl;
1617
use diesel::prelude::*;
@@ -166,21 +167,25 @@ impl DataStore {
166167
Ok(db_bundle)
167168
}
168169

169-
/// Lists one page of support bundles
170+
/// Lists one page of support bundles ordered by creation time
170171
pub async fn support_bundle_list(
171172
&self,
172173
opctx: &OpContext,
173-
pagparams: &DataPageParams<'_, Uuid>,
174+
pagparams: &DataPageParams<'_, (chrono::DateTime<chrono::Utc>, Uuid)>,
174175
) -> ListResultVec<SupportBundle> {
175176
opctx.authorize(authz::Action::Read, &authz::FLEET).await?;
176177
use nexus_db_schema::schema::support_bundle::dsl;
177178

178179
let conn = self.pool_connection_authorized(opctx).await?;
179-
paginated(dsl::support_bundle, dsl::id, pagparams)
180-
.select(SupportBundle::as_select())
181-
.load_async(&*conn)
182-
.await
183-
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
180+
paginated_multicolumn(
181+
dsl::support_bundle,
182+
(dsl::time_created, dsl::id),
183+
pagparams,
184+
)
185+
.select(SupportBundle::as_select())
186+
.load_async(&*conn)
187+
.await
188+
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
184189
}
185190

186191
/// Lists one page of support bundles in a particular state, assigned to
@@ -1417,4 +1422,70 @@ mod test {
14171422
db.terminate().await;
14181423
logctx.cleanup_successful();
14191424
}
1425+
1426+
#[tokio::test]
1427+
async fn test_bundle_list_time_ordering() {
1428+
let logctx = dev::test_setup_log("test_bundle_list_time_ordering");
1429+
let db = TestDatabase::new_with_datastore(&logctx.log).await;
1430+
let (opctx, datastore) = (db.opctx(), db.datastore());
1431+
1432+
let _test_sled = create_sled_and_zpools(&datastore, &opctx, 3).await;
1433+
let this_nexus_id = OmicronZoneUuid::new_v4();
1434+
1435+
// Create multiple bundles with slight time delays to ensure different creation times
1436+
let mut bundle_ids = Vec::new();
1437+
let mut bundle_times = Vec::new();
1438+
1439+
for _i in 0..3 {
1440+
let bundle = datastore
1441+
.support_bundle_create(
1442+
&opctx,
1443+
"Bundle for time ordering test",
1444+
this_nexus_id,
1445+
)
1446+
.await
1447+
.expect("Should be able to create bundle");
1448+
bundle_ids.push(bundle.id);
1449+
bundle_times.push(bundle.time_created);
1450+
1451+
// Small delay to ensure different creation times
1452+
tokio::time::sleep(tokio::time::Duration::from_millis(10)).await;
1453+
}
1454+
1455+
// List bundles using time-based pagination
1456+
let pagparams = DataPageParams::max_page();
1457+
let observed_bundles = datastore
1458+
.support_bundle_list(&opctx, &pagparams)
1459+
.await
1460+
.expect("Should be able to list bundles");
1461+
1462+
assert_eq!(3, observed_bundles.len());
1463+
1464+
// Verify bundles are ordered by creation time (ascending)
1465+
for i in 0..observed_bundles.len() - 1 {
1466+
assert!(
1467+
observed_bundles[i].time_created
1468+
<= observed_bundles[i + 1].time_created,
1469+
"Bundles should be ordered by creation time (ascending). Bundle at index {} has time {:?}, but bundle at index {} has time {:?}",
1470+
i,
1471+
observed_bundles[i].time_created,
1472+
i + 1,
1473+
observed_bundles[i + 1].time_created
1474+
);
1475+
}
1476+
1477+
// Verify that the bundles are our created bundles
1478+
let returned_ids: Vec<_> =
1479+
observed_bundles.iter().map(|b| b.id).collect();
1480+
for bundle_id in &bundle_ids {
1481+
assert!(
1482+
returned_ids.contains(bundle_id),
1483+
"Bundle ID {:?} should be in the returned list",
1484+
bundle_id
1485+
);
1486+
}
1487+
1488+
db.terminate().await;
1489+
logctx.cleanup_successful();
1490+
}
14201491
}

‎nexus/external-api/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3237,7 +3237,7 @@ pub trait NexusExternalApi {
32373237
}]
32383238
async fn support_bundle_list(
32393239
rqctx: RequestContext<Self::Context>,
3240-
query_params: Query<PaginatedById>,
3240+
query_params: Query<PaginatedByTimeAndId>,
32413241
) -> Result<HttpResponseOk<ResultsPage<shared::SupportBundleInfo>>, HttpError>;
32423242

32433243
/// View a support bundle

‎nexus/internal-api/src/lib.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,10 @@ use nexus_types::{
3434
},
3535
};
3636
use omicron_common::api::{
37-
external::{Instance, http_pagination::PaginatedById},
37+
external::{
38+
Instance,
39+
http_pagination::{PaginatedById, PaginatedByTimeAndId},
40+
},
3841
internal::nexus::{
3942
DiskRuntimeState, DownstairsClientStopRequest, DownstairsClientStopped,
4043
ProducerEndpoint, ProducerRegistrationResponse, RepairFinishInfo,
@@ -587,7 +590,7 @@ pub trait NexusInternalApi {
587590
}]
588591
async fn support_bundle_list(
589592
rqctx: RequestContext<Self::Context>,
590-
query_params: Query<PaginatedById>,
593+
query_params: Query<PaginatedByTimeAndId>,
591594
) -> Result<HttpResponseOk<ResultsPage<shared::SupportBundleInfo>>, HttpError>;
592595

593596
/// View a support bundle

‎nexus/src/app/support_bundles.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ impl super::Nexus {
3939
pub async fn support_bundle_list(
4040
&self,
4141
opctx: &OpContext,
42-
pagparams: &DataPageParams<'_, Uuid>,
42+
pagparams: &DataPageParams<'_, (chrono::DateTime<chrono::Utc>, Uuid)>,
4343
) -> ListResultVec<SupportBundle> {
4444
self.db_datastore.support_bundle_list(&opctx, pagparams).await
4545
}

‎nexus/src/external_api/http_entrypoints.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7203,7 +7203,7 @@ impl NexusExternalApi for NexusExternalApiImpl {
72037203

72047204
async fn support_bundle_list(
72057205
rqctx: RequestContext<ApiContext>,
7206-
query_params: Query<PaginatedById>,
7206+
query_params: Query<PaginatedByTimeAndId>,
72077207
) -> Result<HttpResponseOk<ResultsPage<shared::SupportBundleInfo>>, HttpError>
72087208
{
72097209
let apictx = rqctx.context();
@@ -7223,11 +7223,11 @@ impl NexusExternalApi for NexusExternalApiImpl {
72237223
.map(|p| p.into())
72247224
.collect();
72257225

7226-
Ok(HttpResponseOk(ScanById::results_page(
7226+
Ok(HttpResponseOk(ScanByTimeAndId::results_page(
72277227
&query,
72287228
bundles,
72297229
&|_, bundle: &shared::SupportBundleInfo| {
7230-
bundle.id.into_untyped_uuid()
7230+
(bundle.time_created, bundle.id.into_untyped_uuid())
72317231
},
72327232
)?))
72337233
};

‎nexus/src/internal_api/http_entrypoints.rs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,9 @@ use nexus_types::internal_api::views::UpdateStatus;
5353
use nexus_types::internal_api::views::to_list;
5454
use omicron_common::api::external::Instance;
5555
use omicron_common::api::external::http_pagination::PaginatedById;
56+
use omicron_common::api::external::http_pagination::PaginatedByTimeAndId;
5657
use omicron_common::api::external::http_pagination::ScanById;
58+
use omicron_common::api::external::http_pagination::ScanByTimeAndId;
5759
use omicron_common::api::external::http_pagination::ScanParams;
5860
use omicron_common::api::external::http_pagination::data_page_params_for;
5961
use omicron_common::api::internal::nexus::DiskRuntimeState;
@@ -1029,7 +1031,7 @@ impl NexusInternalApi for NexusInternalApiImpl {
10291031

10301032
async fn support_bundle_list(
10311033
rqctx: RequestContext<ApiContext>,
1032-
query_params: Query<PaginatedById>,
1034+
query_params: Query<PaginatedByTimeAndId>,
10331035
) -> Result<HttpResponseOk<ResultsPage<shared::SupportBundleInfo>>, HttpError>
10341036
{
10351037
let apictx = rqctx.context();
@@ -1049,11 +1051,11 @@ impl NexusInternalApi for NexusInternalApiImpl {
10491051
.map(|p| p.into())
10501052
.collect();
10511053

1052-
Ok(HttpResponseOk(ScanById::results_page(
1054+
Ok(HttpResponseOk(ScanByTimeAndId::results_page(
10531055
&query,
10541056
bundles,
10551057
&|_, bundle: &shared::SupportBundleInfo| {
1056-
bundle.id.into_untyped_uuid()
1058+
(bundle.time_created, bundle.id.into_untyped_uuid())
10571059
},
10581060
)?))
10591061
};

‎nexus/tests/integration_tests/support_bundles.rs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -591,3 +591,52 @@ async fn test_support_bundle_range_requests(
591591
.unwrap();
592592
assert_eq!(second_half, full_contents[first_half.len()..]);
593593
}
594+
595+
// Test that support bundle listing returns bundles ordered by creation time
596+
#[nexus_test]
597+
async fn test_support_bundle_list_time_ordering(
598+
cptestctx: &ControlPlaneTestContext,
599+
) {
600+
let client = &cptestctx.external_client;
601+
602+
// Create a disk test with multiple zpools to allow multiple bundles
603+
let _disk_test =
604+
DiskTestBuilder::new(&cptestctx).with_zpool_count(3).build().await;
605+
606+
// Create multiple bundles with delays to ensure different creation times
607+
let mut bundle_ids = Vec::new();
608+
609+
for _ in 0..3 {
610+
let bundle = bundle_create(&client).await.unwrap();
611+
bundle_ids.push(bundle.id);
612+
613+
// Small delay to ensure different creation times
614+
tokio::time::sleep(tokio::time::Duration::from_millis(50)).await;
615+
}
616+
617+
// List all bundles
618+
let bundles = bundles_list(&client).await.unwrap();
619+
assert_eq!(bundles.len(), 3, "Should have created 3 bundles");
620+
621+
// Verify bundles are ordered by creation time (ascending)
622+
for i in 0..bundles.len() - 1 {
623+
assert!(
624+
bundles[i].time_created <= bundles[i + 1].time_created,
625+
"Bundles should be ordered by creation time (ascending). Bundle at index {} has time {:?}, but bundle at index {} has time {:?}",
626+
i,
627+
bundles[i].time_created,
628+
i + 1,
629+
bundles[i + 1].time_created
630+
);
631+
}
632+
633+
// Verify that all our created bundles are present
634+
let returned_ids: Vec<_> = bundles.iter().map(|b| b.id).collect();
635+
for bundle_id in &bundle_ids {
636+
assert!(
637+
returned_ids.contains(bundle_id),
638+
"Bundle ID {:?} should be in the returned list",
639+
bundle_id
640+
);
641+
}
642+
}

‎openapi/nexus-internal.json

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -882,7 +882,7 @@
882882
"in": "query",
883883
"name": "sort_by",
884884
"schema": {
885-
"$ref": "#/components/schemas/IdSortMode"
885+
"$ref": "#/components/schemas/TimeAndIdSortMode"
886886
}
887887
}
888888
],
@@ -7961,6 +7961,25 @@
79617961
}
79627962
]
79637963
},
7964+
"TimeAndIdSortMode": {
7965+
"description": "Supported set of sort modes for scanning by timestamp and ID",
7966+
"oneOf": [
7967+
{
7968+
"description": "sort in increasing order of timestamp and ID, i.e., earliest first",
7969+
"type": "string",
7970+
"enum": [
7971+
"ascending"
7972+
]
7973+
},
7974+
{
7975+
"description": "sort in increasing order of timestamp and ID, i.e., most recent first",
7976+
"type": "string",
7977+
"enum": [
7978+
"descending"
7979+
]
7980+
}
7981+
]
7982+
},
79647983
"TypedUuidForPropolisKind": {
79657984
"type": "string",
79667985
"format": "uuid"

‎openapi/nexus.json

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@
333333
"in": "query",
334334
"name": "sort_by",
335335
"schema": {
336-
"$ref": "#/components/schemas/IdSortMode"
336+
"$ref": "#/components/schemas/TimeAndIdSortMode"
337337
}
338338
}
339339
],
@@ -27147,18 +27147,6 @@
2714727147
}
2714827148
]
2714927149
},
27150-
"IdSortMode": {
27151-
"description": "Supported set of sort modes for scanning by id only.\n\nCurrently, we only support scanning in ascending order.",
27152-
"oneOf": [
27153-
{
27154-
"description": "sort in increasing order of \"id\"",
27155-
"type": "string",
27156-
"enum": [
27157-
"id_ascending"
27158-
]
27159-
}
27160-
]
27161-
},
2716227150
"TimeAndIdSortMode": {
2716327151
"description": "Supported set of sort modes for scanning by timestamp and ID",
2716427152
"oneOf": [
@@ -27197,6 +27185,18 @@
2719727185
"descending"
2719827186
]
2719927187
},
27188+
"IdSortMode": {
27189+
"description": "Supported set of sort modes for scanning by id only.\n\nCurrently, we only support scanning in ascending order.",
27190+
"oneOf": [
27191+
{
27192+
"description": "sort in increasing order of \"id\"",
27193+
"type": "string",
27194+
"enum": [
27195+
"id_ascending"
27196+
]
27197+
}
27198+
]
27199+
},
2720027200
"SystemMetricName": {
2720127201
"type": "string",
2720227202
"enum": [

0 commit comments

Comments
 (0)