Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stop logging cloud_init_bytes in sled-agent #6439

Merged
merged 4 commits into from
Aug 27, 2024
Merged

stop logging cloud_init_bytes in sled-agent #6439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10a2895
want sled-agent to scream cidata bytes less
iliana Aug 26, 2024
556c49b
fine, rustdoc. you're right
iliana Aug 26, 2024
102b1df
Merge remote-tracking branch 'origin/main' into iliana/cidata-screaming
iliana Aug 27, 2024
37188e9
elaborate why we're not using an upstream crate
iliana Aug 27, 2024
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
24 changes: 24 additions & 0 deletions common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,27 @@ where
async fn never_bail() -> Result<bool, Error> {
Ok(false)
}

/// A wrapper struct that does nothing other than elide the inner value from
Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote https://docs.rs/debug-ignore a bit ago which we use :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trick is that it must also implement JsonSchema for this particular use case, and anything else we ever want to use it in a struct for as well. For that reason it seems to make sense to keep it within Omicron so we can adjust it as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

sighhhh, yeah.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Could you add a comment about this?)

/// [`std::fmt::Debug`] output.
///
/// We define this within Omicron instead of using one of the many available
/// crates that do the same thing because it's trivial to do so, and we want the
/// flexibility to add traits to this type without needing to wait on upstream
/// to add an optional dependency.
///
/// If you want to use this for secrets, consider that it might not do
/// everything you expect (it does not zeroize memory on drop, nor get in the
/// way of you removing the inner value from this wrapper struct).
#[derive(
Clone, Copy, serde::Deserialize, serde::Serialize, schemars::JsonSchema,
)]
#[repr(transparent)]
#[serde(transparent)]
Comment on lines +136 to +137
Copy link
Member

Choose a reason for hiding this comment

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

world's most transparent type :)

pub struct NoDebug<T>(pub T);

impl<T> std::fmt::Debug for NoDebug<T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "..")
}
}
7 changes: 4 additions & 3 deletions sled-agent/src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use omicron_common::api::internal::shared::{
};
use omicron_common::backoff;
use omicron_common::zpool_name::ZpoolName;
use omicron_common::NoDebug;
use omicron_uuid_kinds::{GenericUuid, InstanceUuid, PropolisUuid};
use propolis_client::Client as PropolisClient;
use rand::prelude::IteratorRandom;
Expand Down Expand Up @@ -335,7 +336,7 @@ struct InstanceRunner {

// Disk related properties
requested_disks: Vec<propolis_client::types::DiskRequest>,
cloud_init_bytes: Option<String>,
cloud_init_bytes: Option<NoDebug<String>>,

// Internal State management
state: InstanceStates,
Expand Down Expand Up @@ -718,10 +719,10 @@ impl InstanceRunner {
.map(Into::into)
.collect(),
migrate,
cloud_init_bytes: self.cloud_init_bytes.clone(),
cloud_init_bytes: self.cloud_init_bytes.clone().map(|x| x.0),
};

info!(self.log, "Sending ensure request to propolis: {:?}", request);
debug!(self.log, "Sending ensure request to propolis: {:?}", request);
let result = client.instance_ensure().body(request).send().await;
info!(self.log, "result of instance_ensure call is {:?}", result);
result?;
Expand Down
3 changes: 2 additions & 1 deletion sled-agent/types/src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use omicron_common::api::internal::{
DhcpConfig, NetworkInterface, ResolvedVpcFirewallRule, SourceNatConfig,
},
};
use omicron_common::NoDebug;
use omicron_uuid_kinds::InstanceUuid;
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -60,7 +61,7 @@ pub struct InstanceHardware {
pub dhcp_config: DhcpConfig,
// TODO: replace `propolis_client::*` with locally-modeled request type
pub disks: Vec<propolis_client::types::DiskRequest>,
pub cloud_init_bytes: Option<String>,
pub cloud_init_bytes: Option<NoDebug<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

An alternative solution, rather than adding a no-debug wrapper type, could be to manually implement Debug for InstanceHardware using fmt::DebugStruct, and formatting everything normally but skipping the cloud_init_bytes field (or rendering it as Some(..)/None).

I'm not sure which is better off the top of my head --- the wrapper type seems a bit unfortunate: the "actual" type is Option<String>, and it's kinda sad to change the type just to control formatting behavior. But it might be better in the long run, since we don't have to remember to add any new fields to the manual debug implementation. So, I think this is fine, just felt like talking through alternatives.

Incidentally, there is nice technique for avoiding issues where new fields aren't added to a manual Debug implementation, though (credit to my friend @cratelyn for this one). You can use a destructuring assignment on the Self type in the Debug impl, like:

impl fmt::Debug for InstanceHardware {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        let Self {
            properties,
            nics,
            source_nat,
            ephemeral_ip,
            floating_ips,
            firewall_rules,
            dhcp_config,
            disks,
            cloud_init_bytes,
        } = self;
        f.debug_struct("InstanceHardware")
            .field("properties", properties)
            .field("nics", nics)
            // ... and so on ...
            .finish()
    }
}

and, because the destructuring pattern binds all the fields (since it doesn't have a ..), any newly-added field will cause it to fail to compile, serving as a nice reminder to add the new field to the Debug impl. But, this is still more work than #[derive(Debug)], so...

Copy link
Contributor

@sunshowers sunshowers Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW I prefer the wrapper struct over the two other alternatives I'm aware of, the manual impl you mentioned and the derive-more crate. But there are definitely places where I'd use one of the other approaches.

Copy link
Member

Choose a reason for hiding this comment

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

For the record, the wrapper struct is fine with me!

}

/// Metadata used to track statistics about an instance.
Expand Down
Loading