Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 1 addition & 38 deletions nexus/db-queries/src/db/datastore/ereport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use nexus_db_errors::public_error_from_diesel;
use nexus_db_lookup::DbConnection;
use nexus_db_schema::schema::ereport::dsl;
use nexus_types::fm::ereport as fm;
use nexus_types::fm::ereport::EreportFilters;
use nexus_types::fm::ereport::EreportId;
use omicron_common::api::external::CreateResult;
use omicron_common::api::external::DataPageParams;
Expand All @@ -48,44 +49,6 @@ pub struct EreporterRestartBySerial {
pub ereports: u32,
}

/// A set of filters for fetching ereports.
#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub struct EreportFilters {
/// If present, include only ereports that were collected at the specified
/// timestamp or later.
///
/// If `end_time` is also present, this value *must* be earlier than
/// `end_time`.
pub start_time: Option<DateTime<Utc>>,
/// If present, include only ereports that were collected at the specified
/// timestamp or before.
///
/// If `start_time` is also present, this value *must* be later than
/// `start_time`.
pub end_time: Option<DateTime<Utc>>,
/// If this list is non-empty, include only ereports that were reported by
/// systems with the provided serial numbers.
pub only_serials: Vec<String>,
/// If this list is non-empty, include only ereports with the provided class
/// strings.
// TODO(eliza): globbing could be nice to add here eventually...
pub only_classes: Vec<String>,
}

impl EreportFilters {
fn check_time_range(&self) -> Result<(), Error> {
if let (Some(start), Some(end)) = (self.start_time, self.end_time) {
if start > end {
return Err(Error::invalid_request(
"start time must be before end time",
));
}
}

Ok(())
}
}

impl DataStore {
/// Fetch an ereport by its restart ID and ENA.
///
Expand Down
1 change: 0 additions & 1 deletion nexus/db-queries/src/db/datastore/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ pub use disk::LocalStorageAllocation;
pub use disk::LocalStorageDisk;
pub use dns::DataStoreDnsTest;
pub use dns::DnsVersionUpdateBuilder;
pub use ereport::EreportFilters;
pub use external_ip::FloatingIpAllocation;
pub use external_subnet::ExternalSubnetBeginOpResult;
pub use external_subnet::ExternalSubnetCompleteOpResult;
Expand Down
123 changes: 5 additions & 118 deletions nexus/src/app/background/tasks/support_bundle/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@

//! Support bundle request types and data selection

use nexus_db_queries::db::datastore::EreportFilters;
use nexus_types::fm::ereport::EreportFilters;
use nexus_types::support_bundle::{
BundleData, BundleDataCategory, BundleDataSelection, SledSelection,
};

use omicron_uuid_kinds::SledUuid;
use std::collections::HashMap;
use std::collections::HashSet;
use std::num::NonZeroU64;

/// We use "/var/tmp" to use Nexus' filesystem for temporary storage,
Expand All @@ -18,121 +20,6 @@ pub const TEMPDIR: &str = "/var/tmp";
/// within a single streaming request.
pub const CHUNK_SIZE: NonZeroU64 = NonZeroU64::new(1024 * 1024 * 1024).unwrap();

/// Describes the category of support bundle data.
#[derive(Debug, Clone, Copy, Hash, Eq, PartialEq)]
pub enum BundleDataCategory {
/// Collects reconfigurator state (some of the latest blueprints,
/// information about the target blueprint).
Reconfigurator,
/// Collects info from sled agents, running a handful of
/// diagnostic commands (e.g., zoneadm, dladm, etc).
HostInfo,
/// Collects sled serial numbers, cubby numbers, and UUIDs.
SledCubbyInfo,
/// Saves task dumps from SPs.
SpDumps,
/// Collects ereports.
Ereports,
}

/// Specifies what data to collect for a bundle data category.
///
/// Each variant corresponds to a BundleDataCategory.
/// For categories without additional parameters, the variant is a unit variant.
/// For categories that can be filtered or configured, the variant contains
/// that configuration data.
#[derive(Debug, Clone, Eq, PartialEq)]
pub enum BundleData {
Reconfigurator,
HostInfo(HashSet<SledSelection>),
SledCubbyInfo,
SpDumps,
Ereports(EreportFilters),
}

impl BundleData {
fn category(&self) -> BundleDataCategory {
match self {
Self::Reconfigurator => BundleDataCategory::Reconfigurator,
Self::HostInfo(_) => BundleDataCategory::HostInfo,
Self::SledCubbyInfo => BundleDataCategory::SledCubbyInfo,
Self::SpDumps => BundleDataCategory::SpDumps,
Self::Ereports(_) => BundleDataCategory::Ereports,
}
}
}

/// A collection of bundle data specifications.
///
/// This wrapper ensures that categories and data always match - you can't
/// insert (BundleDataCategory::Reconfigurator, BundleData::SpDumps)
/// because each BundleData determines its own category.
#[derive(Debug, Clone)]
pub struct BundleDataSelection {
data: HashMap<BundleDataCategory, BundleData>,
}

impl BundleDataSelection {
pub fn new() -> Self {
Self { data: HashMap::new() }
}

/// Inserts BundleData to be queried for a particular category within the
/// bundle.
///
/// Each category of data can only be specified once (e.g., inserting
/// BundleData::HostInfo multiple times will only use the most-recently
/// inserted specification)
pub fn insert(&mut self, bundle_data: BundleData) {
self.data.insert(bundle_data.category(), bundle_data);
}

pub fn contains(&self, category: BundleDataCategory) -> bool {
self.data.contains_key(&category)
}

pub fn get(&self, category: BundleDataCategory) -> Option<&BundleData> {
self.data.get(&category)
}
}

impl FromIterator<BundleData> for BundleDataSelection {
fn from_iter<T: IntoIterator<Item = BundleData>>(iter: T) -> Self {
let mut selection = Self::new();
for bundle_data in iter {
selection.insert(bundle_data);
}
selection
}
}

impl Default for BundleDataSelection {
fn default() -> Self {
[
BundleData::Reconfigurator,
BundleData::HostInfo(HashSet::from([SledSelection::All])),
BundleData::SledCubbyInfo,
BundleData::SpDumps,
BundleData::Ereports(EreportFilters {
start_time: Some(chrono::Utc::now() - chrono::Days::new(7)),
..EreportFilters::default()
}),
]
.into_iter()
.collect()
}
}

/// The set of sleds to include
///
/// Multiple values of this enum are joined together into a HashSet.
/// Therefore "SledSelection::All" overrides specific sleds.
#[derive(Debug, Clone, Hash, Eq, PartialEq)]
pub enum SledSelection {
All,
Specific(SledUuid),
}

/// Specifies the data to be collected within the Support Bundle.
#[derive(Clone)]
pub struct BundleRequest {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@
use crate::app::background::tasks::support_bundle::collection::BundleCollection;
use crate::app::background::tasks::support_bundle::step::CollectionStepOutput;
use nexus_types::fm::ereport::EreportFilters;

use anyhow::Context;
use camino::Utf8Path;
use camino::Utf8PathBuf;
use nexus_db_queries::context::OpContext;
use nexus_db_queries::db::DataStore;
use nexus_db_queries::db::datastore;
use nexus_db_queries::db::datastore::EreportFilters;
use nexus_db_queries::db::pagination::Paginator;
use nexus_types::fm::Ereport;
use nexus_types::internal_api::background::SupportBundleEreportStatus;
Expand Down
2 changes: 1 addition & 1 deletion nexus/src/app/background/tasks/support_bundle_collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,6 @@ mod test {
use super::*;

use crate::app::background::tasks::support_bundle::perfetto;
use crate::app::background::tasks::support_bundle::request::BundleData;
use crate::app::support_bundles::SupportBundleQueryType;
use http_body_util::BodyExt;
use illumos_utils::zpool::ZpoolHealth;
Expand All @@ -456,6 +455,7 @@ mod test {
use nexus_types::internal_api::background::SupportBundleCollectionStep;
use nexus_types::internal_api::background::SupportBundleEreportStatus;
use nexus_types::inventory::SpType;
use nexus_types::support_bundle::BundleData;
use omicron_common::api::external::ByteCount;
use omicron_common::api::internal::shared::DatasetKind;
use omicron_common::disk::DatasetConfig;
Expand Down
39 changes: 39 additions & 0 deletions nexus/types/src/fm/ereport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use crate::inventory::SpType;
use chrono::{DateTime, Utc};
use omicron_common::api::external::Error;
use omicron_uuid_kinds::EreporterRestartUuid;
use omicron_uuid_kinds::OmicronZoneUuid;
use omicron_uuid_kinds::SledUuid;
Expand Down Expand Up @@ -214,3 +215,41 @@ fn get_sp_metadata_string(
}
}
}

/// A set of filters for fetching ereports.
#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub struct EreportFilters {
/// If present, include only ereports that were collected at the specified
/// timestamp or later.
///
/// If `end_time` is also present, this value *must* be earlier than
/// `end_time`.
pub start_time: Option<DateTime<Utc>>,
/// If present, include only ereports that were collected at the specified
/// timestamp or before.
///
/// If `start_time` is also present, this value *must* be later than
/// `start_time`.
pub end_time: Option<DateTime<Utc>>,
/// If this list is non-empty, include only ereports that were reported by
/// systems with the provided serial numbers.
pub only_serials: Vec<String>,
/// If this list is non-empty, include only ereports with the provided class
/// strings.
// TODO(eliza): globbing could be nice to add here eventually...
pub only_classes: Vec<String>,
}

impl EreportFilters {
pub fn check_time_range(&self) -> Result<(), Error> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of me wants to make the fields private and do the validation in a constructor, so all instances of the type are validated...but, looking back, I think the reason I didn't do it that way originally is that I wanted the start and end timestamps to be provided as named fields, rather than as unnamed function parameters, so it was obvious to the reader which is which. Given that, I don't really think it's worth messing with.

If you really wanted to do it the Right Way, you could have the constructor take one of the std range types as a single argument, so you could call it with start...end or start... or ...end and it would always be obvious to the reader. But let's treat that as a fun side quest you can do if you want, rather than something I care deeply about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that too, actually. But I figure I should probably do that in a separate PR just to make it super obvious that I'm purely moving things here.

if let (Some(start), Some(end)) = (self.start_time, self.end_time) {
if start > end {
return Err(Error::invalid_request(
"start time must be before end time",
));
}
}

Ok(())
}
}
1 change: 1 addition & 0 deletions nexus/types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,5 @@ pub mod multicast;
pub mod quiesce;
pub mod saga;
pub mod silo;
pub mod support_bundle;
pub mod trust_quorum;
Loading
Loading