From 7e5392b600696c6878af277c7ec09e37514ac655 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 10 Jul 2025 17:19:04 -0700 Subject: [PATCH] Support Bundle: Add user comment support --- dev-tools/omdb/src/bin/omdb/nexus.rs | 2 + nexus/db-model/src/schema_versions.rs | 3 +- nexus/db-model/src/support_bundle.rs | 3 + .../src/db/datastore/support_bundle.rs | 36 +++++++ nexus/db-schema/src/schema.rs | 1 + nexus/external-api/output/nexus_tags.txt | 1 + nexus/external-api/src/lib.rs | 12 +++ nexus/internal-api/src/lib.rs | 11 +++ nexus/src/app/support_bundles.rs | 23 +++++ nexus/src/external_api/http_entrypoints.rs | 31 +++++++ nexus/src/internal_api/http_entrypoints.rs | 32 +++++++ nexus/tests/integration_tests/endpoints.rs | 6 ++ .../integration_tests/support_bundles.rs | 93 +++++++++++++++++++ nexus/types/src/external_api/params.rs | 6 ++ nexus/types/src/external_api/shared.rs | 1 + openapi/nexus-internal.json | 58 ++++++++++++ openapi/nexus.json | 61 ++++++++++++ schema/crdb/bundle-user-comment/up01.sql | 1 + schema/crdb/dbinit.sql | 7 +- 19 files changed, 385 insertions(+), 3 deletions(-) create mode 100644 schema/crdb/bundle-user-comment/up01.sql diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index fca3415534e..a8c816310d4 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -4009,6 +4009,7 @@ async fn cmd_nexus_support_bundles_list( reason_for_creation: String, reason_for_failure: String, state: String, + user_comment: String, } let rows = support_bundles.into_iter().map(|sb| SupportBundleInfo { id: *sb.id, @@ -4018,6 +4019,7 @@ async fn cmd_nexus_support_bundles_list( .reason_for_failure .unwrap_or_else(|| "-".to_string()), state: format!("{:?}", sb.state), + user_comment: sb.user_comment.unwrap_or_else(|| "-".to_string()), }); let table = tabled::Table::new(rows) .with(tabled::settings::Style::empty()) diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 08e75a7bf6c..ae17aece122 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(162, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(163, 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(163, "bundle-user-comment"), KnownVersion::new(162, "bundle-by-creation"), KnownVersion::new(161, "inv_cockroachdb_status"), KnownVersion::new(160, "tuf-trust-root"), diff --git a/nexus/db-model/src/support_bundle.rs b/nexus/db-model/src/support_bundle.rs index 6be06285126..95356305d76 100644 --- a/nexus/db-model/src/support_bundle.rs +++ b/nexus/db-model/src/support_bundle.rs @@ -95,6 +95,7 @@ pub struct SupportBundle { pub zpool_id: DbTypedUuid, pub dataset_id: DbTypedUuid, pub assigned_nexus: Option>, + pub user_comment: Option, } impl SupportBundle { @@ -113,6 +114,7 @@ impl SupportBundle { zpool_id: zpool_id.into(), dataset_id: dataset_id.into(), assigned_nexus: Some(nexus_id.into()), + user_comment: None, } } @@ -128,6 +130,7 @@ impl From for SupportBundleView { time_created: bundle.time_created, reason_for_creation: bundle.reason_for_creation, reason_for_failure: bundle.reason_for_failure, + user_comment: bundle.user_comment, state: bundle.state.into(), } } diff --git a/nexus/db-queries/src/db/datastore/support_bundle.rs b/nexus/db-queries/src/db/datastore/support_bundle.rs index 30e10aecc39..387c07a6949 100644 --- a/nexus/db-queries/src/db/datastore/support_bundle.rs +++ b/nexus/db-queries/src/db/datastore/support_bundle.rs @@ -438,6 +438,42 @@ impl DataStore { } } + /// Updates the user comment of a support bundle. + /// + /// Returns: + /// - "Ok" if the bundle was updated successfully. + /// - "Err::InvalidRequest" if the comment exceeds the maximum length. + pub async fn support_bundle_update_user_comment( + &self, + opctx: &OpContext, + authz_bundle: &authz::SupportBundle, + user_comment: Option, + ) -> Result<(), Error> { + opctx.authorize(authz::Action::Modify, authz_bundle).await?; + + if let Some(ref comment) = user_comment { + if comment.len() > 4096 { + return Err(Error::invalid_request( + "User comment cannot exceed 4096 bytes", + )); + } + } + + use nexus_db_schema::schema::support_bundle::dsl; + + let id = authz_bundle.id().into_untyped_uuid(); + let conn = self.pool_connection_authorized(opctx).await?; + diesel::update(dsl::support_bundle) + .filter(dsl::id.eq(id)) + .set(dsl::user_comment.eq(user_comment)) + .execute_async(&*conn) + .await + .map(|_rows_modified| ()) + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + Ok(()) + } + /// Deletes a support bundle. /// /// This should only be invoked after all storage for the support bundle has diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 99285a291f3..319c2af6659 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -1470,6 +1470,7 @@ table! { dataset_id -> Uuid, assigned_nexus -> Nullable, + user_comment -> Nullable, } } diff --git a/nexus/external-api/output/nexus_tags.txt b/nexus/external-api/output/nexus_tags.txt index c7def7557d3..aca84b0e334 100644 --- a/nexus/external-api/output/nexus_tags.txt +++ b/nexus/external-api/output/nexus_tags.txt @@ -62,6 +62,7 @@ support_bundle_head HEAD /experimental/v1/system/suppor support_bundle_head_file HEAD /experimental/v1/system/support-bundles/{bundle_id}/download/{file} support_bundle_index GET /experimental/v1/system/support-bundles/{bundle_id}/index support_bundle_list GET /experimental/v1/system/support-bundles +support_bundle_update PUT /experimental/v1/system/support-bundles/{bundle_id} support_bundle_view GET /experimental/v1/system/support-bundles/{bundle_id} system_update_get_repository GET /v1/system/update/repository/{system_version} system_update_put_repository PUT /v1/system/update/repository diff --git a/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs index 4361ea4ed38..557063ea6ba 100644 --- a/nexus/external-api/src/lib.rs +++ b/nexus/external-api/src/lib.rs @@ -3335,6 +3335,18 @@ pub trait NexusExternalApi { path_params: Path, ) -> Result; + /// Update a support bundle + #[endpoint { + method = PUT, + path = "/experimental/v1/system/support-bundles/{bundle_id}", + tags = ["experimental"], // system/support-bundles: only one tag is allowed + }] + async fn support_bundle_update( + rqctx: RequestContext, + path_params: Path, + body: TypedBody, + ) -> Result, HttpError>; + // Probes (experimental) /// List instrumentation probes diff --git a/nexus/internal-api/src/lib.rs b/nexus/internal-api/src/lib.rs index 69b4c214564..5b5fc5696b1 100644 --- a/nexus/internal-api/src/lib.rs +++ b/nexus/internal-api/src/lib.rs @@ -680,6 +680,17 @@ pub trait NexusInternalApi { path_params: Path, ) -> Result; + /// Update a support bundle + #[endpoint { + method = PUT, + path = "/experimental/v1/system/support-bundles/{bundle_id}", + }] + async fn support_bundle_update( + rqctx: RequestContext, + path_params: Path, + body: TypedBody, + ) -> Result, HttpError>; + /// Get all the probes associated with a given sled. #[endpoint { method = GET, diff --git a/nexus/src/app/support_bundles.rs b/nexus/src/app/support_bundles.rs index 69c5db77b07..70d52d5c750 100644 --- a/nexus/src/app/support_bundles.rs +++ b/nexus/src/app/support_bundles.rs @@ -224,4 +224,27 @@ impl super::Nexus { .await?; Ok(()) } + + pub async fn support_bundle_update_user_comment( + &self, + opctx: &OpContext, + id: SupportBundleUuid, + user_comment: Option, + ) -> LookupResult { + let (authz_bundle, ..) = LookupPath::new(opctx, &self.db_datastore) + .support_bundle(id) + .lookup_for(authz::Action::Modify) + .await?; + + self.db_datastore + .support_bundle_update_user_comment( + &opctx, + &authz_bundle, + user_comment, + ) + .await?; + + // Return the updated bundle + self.support_bundle_view(opctx, id).await + } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d2ba100882a..d48d60aad23 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7493,6 +7493,37 @@ impl NexusExternalApi for NexusExternalApiImpl { .await } + async fn support_bundle_update( + rqctx: RequestContext, + path_params: Path, + body: TypedBody, + ) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.context.nexus; + let path = path_params.into_inner(); + let update = body.into_inner(); + + let opctx = + crate::context::op_context_for_external_api(&rqctx).await?; + + let bundle = nexus + .support_bundle_update_user_comment( + &opctx, + SupportBundleUuid::from_untyped_uuid(path.bundle_id), + update.user_comment, + ) + .await?; + + Ok(HttpResponseOk(bundle.into())) + }; + apictx + .context + .external_latencies + .instrument_dropshot_handler(&rqctx, handler) + .await + } + async fn probe_list( rqctx: RequestContext, query_params: Query>, diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index c0e2a5a29f0..842d964f613 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -36,6 +36,7 @@ use nexus_types::external_api::params::PhysicalDiskPath; use nexus_types::external_api::params::SledSelector; use nexus_types::external_api::params::SupportBundleFilePath; use nexus_types::external_api::params::SupportBundlePath; +use nexus_types::external_api::params::SupportBundleUpdate; use nexus_types::external_api::params::UninitializedSledId; use nexus_types::external_api::shared::ProbeInfo; use nexus_types::external_api::shared::UninitializedSled; @@ -1321,6 +1322,37 @@ impl NexusInternalApi for NexusInternalApiImpl { .await } + async fn support_bundle_update( + rqctx: RequestContext, + path_params: Path, + body: TypedBody, + ) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.context.nexus; + let path = path_params.into_inner(); + let update = body.into_inner(); + + let opctx = + crate::context::op_context_for_internal_api(&rqctx).await; + + let bundle = nexus + .support_bundle_update_user_comment( + &opctx, + SupportBundleUuid::from_untyped_uuid(path.bundle_id), + update.user_comment, + ) + .await?; + + Ok(HttpResponseOk(bundle.into())) + }; + apictx + .context + .external_latencies + .instrument_dropshot_handler(&rqctx, handler) + .await + } + async fn probes_get( rqctx: RequestContext, path_params: Path, diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 0718f1c36fe..b00a21ecc09 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -2474,6 +2474,12 @@ pub static VERIFY_ENDPOINTS: LazyLock> = allowed_methods: vec![ AllowedMethod::Get, AllowedMethod::Delete, + AllowedMethod::Put( + serde_json::to_value(¶ms::SupportBundleUpdate { + user_comment: None, + }) + .unwrap(), + ), ], }, /* Updates */ diff --git a/nexus/tests/integration_tests/support_bundles.rs b/nexus/tests/integration_tests/support_bundles.rs index 878ae588350..909ec36bb06 100644 --- a/nexus/tests/integration_tests/support_bundles.rs +++ b/nexus/tests/integration_tests/support_bundles.rs @@ -279,6 +279,29 @@ async fn bundle_download_expect_fail( Ok(()) } +async fn bundle_update_comment( + client: &ClientTestContext, + id: SupportBundleUuid, + comment: Option, +) -> Result { + use nexus_types::external_api::params::SupportBundleUpdate; + + let url = format!("{BUNDLES_URL}/{id}"); + let update = SupportBundleUpdate { user_comment: comment }; + + NexusRequest::new( + RequestBuilder::new(client, Method::PUT, &url) + .body(Some(&update)) + .expect_status(Some(StatusCode::OK)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .context("failed to update bundle comment")? + .parsed_body() + .context("failed to parse 'update bundle comment' response") +} + // -- Background Task -- // // The following logic helps us trigger and observe the output of the support @@ -640,3 +663,73 @@ async fn test_support_bundle_list_time_ordering( ); } } + +// Test updating bundle comments +#[nexus_test] +async fn test_support_bundle_update_comment( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + let _disk_test = + DiskTestBuilder::new(&cptestctx).with_zpool_count(1).build().await; + + // Create a bundle + let bundle = bundle_create(&client).await.unwrap(); + assert_eq!(bundle.user_comment, None); + + // Update the comment + let comment = Some("Test comment".to_string()); + let updated_bundle = + bundle_update_comment(&client, bundle.id, comment.clone()) + .await + .unwrap(); + assert_eq!(updated_bundle.user_comment, comment); + + // Update with a different comment + let new_comment = Some("Updated comment".to_string()); + let updated_bundle = + bundle_update_comment(&client, bundle.id, new_comment.clone()) + .await + .unwrap(); + assert_eq!(updated_bundle.user_comment, new_comment); + + // Clear the comment + let updated_bundle = + bundle_update_comment(&client, bundle.id, None).await.unwrap(); + assert_eq!(updated_bundle.user_comment, None); + + // Test maximum length validation (4096 bytes) + let max_comment = "a".repeat(4096); + let updated_bundle = + bundle_update_comment(&client, bundle.id, Some(max_comment.clone())) + .await + .unwrap(); + assert_eq!(updated_bundle.user_comment, Some(max_comment)); + + // Test exceeding maximum length (4097 bytes) + let too_long_comment = "a".repeat(4097); + let url = format!("{BUNDLES_URL}/{}", bundle.id); + let update = nexus_types::external_api::params::SupportBundleUpdate { + user_comment: Some(too_long_comment), + }; + + let error = NexusRequest::new( + RequestBuilder::new(client, Method::PUT, &url) + .body(Some(&update)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .context("failed to update bundle comment") + .unwrap() + .parsed_body::() + .context("failed to parse error response") + .unwrap(); + + assert!(error.message.contains("cannot exceed 4096 bytes")); + + // Clean up + bundle_delete(&client, bundle.id).await.unwrap(); +} diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 05e1adc30aa..d64e4685d39 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -158,6 +158,12 @@ pub struct SupportBundleFilePath { pub file: String, } +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct SupportBundleUpdate { + /// User comment for the support bundle + pub user_comment: Option, +} + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] pub struct OptionalSiloSelector { /// Name or ID of the silo diff --git a/nexus/types/src/external_api/shared.rs b/nexus/types/src/external_api/shared.rs index 7688b969a34..6de268e6a45 100644 --- a/nexus/types/src/external_api/shared.rs +++ b/nexus/types/src/external_api/shared.rs @@ -645,6 +645,7 @@ pub struct SupportBundleInfo { pub time_created: DateTime, pub reason_for_creation: String, pub reason_for_failure: Option, + pub user_comment: Option, pub state: SupportBundleState, } diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 22be1853c81..def88298024 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -966,6 +966,50 @@ } } }, + "put": { + "summary": "Update a support bundle", + "operationId": "support_bundle_update", + "parameters": [ + { + "in": "path", + "name": "bundle_id", + "description": "ID of the support bundle", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleUpdate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleInfo" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, "delete": { "summary": "Delete an existing support bundle", "description": "May also be used to cancel a support bundle which is currently being collected, or to remove metadata for a support bundle that has failed.", @@ -7374,6 +7418,10 @@ "time_created": { "type": "string", "format": "date-time" + }, + "user_comment": { + "nullable": true, + "type": "string" } }, "required": [ @@ -7436,6 +7484,16 @@ } ] }, + "SupportBundleUpdate": { + "type": "object", + "properties": { + "user_comment": { + "nullable": true, + "description": "User comment for the support bundle", + "type": "string" + } + } + }, "SwitchLocation": { "description": "Identifies switch physical location", "oneOf": [ diff --git a/openapi/nexus.json b/openapi/nexus.json index 6584f45bb5b..2f62e1ed23c 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -423,6 +423,53 @@ } } }, + "put": { + "tags": [ + "experimental" + ], + "summary": "Update a support bundle", + "operationId": "support_bundle_update", + "parameters": [ + { + "in": "path", + "name": "bundle_id", + "description": "ID of the support bundle", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleUpdate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SupportBundleInfo" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, "delete": { "tags": [ "experimental" @@ -23937,6 +23984,10 @@ "time_created": { "type": "string", "format": "date-time" + }, + "user_comment": { + "nullable": true, + "type": "string" } }, "required": [ @@ -23999,6 +24050,16 @@ } ] }, + "SupportBundleUpdate": { + "type": "object", + "properties": { + "user_comment": { + "nullable": true, + "description": "User comment for the support bundle", + "type": "string" + } + } + }, "Switch": { "description": "An operator's view of a Switch.", "type": "object", diff --git a/schema/crdb/bundle-user-comment/up01.sql b/schema/crdb/bundle-user-comment/up01.sql new file mode 100644 index 00000000000..57b71841c52 --- /dev/null +++ b/schema/crdb/bundle-user-comment/up01.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.support_bundle ADD COLUMN IF NOT EXISTS user_comment TEXT; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index ac02bf92f30..2bc5132538a 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2650,7 +2650,10 @@ CREATE TABLE IF NOT EXISTS omicron.public.support_bundle ( -- The Nexus which is in charge of collecting the support bundle, -- and later managing its storage. - assigned_nexus UUID + assigned_nexus UUID, + + user_comment TEXT + ); -- The "UNIQUE" part of this index helps enforce that we allow one support bundle @@ -6202,7 +6205,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '162.0.0', NULL) + (TRUE, NOW(), NOW(), '163.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT;