From 5dbd332b6d77a3851e17261f9ba319e437ab2315 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 21 Jul 2023 09:56:57 -0400 Subject: [PATCH] repair: Rework to be more introspectable I plan to "productize" this repair code a bit more in OpenShift at least, and I think other admins may want to do similar outside of that too. In order to make that more reliable: - Better split the "fsck/--dry-run" path from "repair" i.e mutation - Introduce a `--write-result-to` argument that outputs JSON. This allows us to better distinguish the tristate of "OK" from "corruption detected" to "tool failed for some other reason" Further: - Drop the check for derived commits, let's just *always* check the inodes because it's not very expensive in the end and it's just really useful to do. - Add checks for whether the booted deployment is potentially affected, as this is important information; we'll need to redeploy and reboot if so --- lib/src/cli.rs | 60 +++++++-- lib/src/container/store.rs | 8 +- lib/src/repair.rs | 247 +++++++++++++++++++++---------------- 3 files changed, 195 insertions(+), 120 deletions(-) diff --git a/lib/src/cli.rs b/lib/src/cli.rs index be17ac4a..6f42d622 100644 --- a/lib/src/cli.rs +++ b/lib/src/cli.rs @@ -12,6 +12,7 @@ use fn_error_context::context; use ostree::{cap_std, gio, glib}; use std::collections::BTreeMap; use std::ffi::OsString; +use std::io::BufWriter; use std::path::PathBuf; use std::process::Command; use tokio::sync::mpsc::Receiver; @@ -348,7 +349,7 @@ pub(crate) enum ContainerImageOpts { } /// Options for deployment repair. -#[derive(Debug, Subcommand)] +#[derive(Debug, Parser)] pub(crate) enum ProvisionalRepairOpts { AnalyzeInodes { /// Path to the repository @@ -358,6 +359,10 @@ pub(crate) enum ProvisionalRepairOpts { /// Print additional information #[clap(long)] verbose: bool, + + /// Serialize the repair result to this file as JSON + #[clap(long)] + write_result_to: Option, }, Repair { @@ -369,6 +374,10 @@ pub(crate) enum ProvisionalRepairOpts { #[clap(long)] dry_run: bool, + /// Serialize the repair result to this file as JSON + #[clap(long)] + write_result_to: Option, + /// Print additional information #[clap(long)] verbose: bool, @@ -787,6 +796,17 @@ fn container_remount_sysroot(sysroot: &Utf8Path) -> Result<()> { Ok(()) } +#[context("Serializing to output file")] +fn handle_serialize_to_file(path: Option<&Utf8Path>, obj: T) -> Result<()> { + if let Some(path) = path { + let mut out = std::fs::File::create(path) + .map(BufWriter::new) + .with_context(|| anyhow::anyhow!("Opening {path} for writing"))?; + serde_json::to_writer(&mut out, &obj).context("Serializing output")?; + } + Ok(()) +} + /// Parse the provided arguments and execute. /// Calls [`structopt::clap::Error::exit`] on failure, printing the error message and aborting the program. pub async fn run_from_iter(args: I) -> Result<()> @@ -1027,15 +1047,21 @@ where #[cfg(feature = "docgen")] Opt::Man(manopts) => crate::docgen::generate_manpages(&manopts.directory), Opt::ProvisionalRepair(opts) => match opts { - ProvisionalRepairOpts::AnalyzeInodes { repo, verbose } => { + ProvisionalRepairOpts::AnalyzeInodes { + repo, + verbose, + write_result_to, + } => { let repo = parse_repo(&repo)?; - match crate::repair::check_inode_collision(&repo, verbose)? { - crate::repair::InodeCheckResult::Okay => { - println!("OK: No colliding objects found."); - } - crate::repair::InodeCheckResult::PotentialCorruption(n) => { - eprintln!("warning: {} potentially colliding inodes found", n.len()); - } + let check_res = crate::repair::check_inode_collision(&repo, verbose)?; + handle_serialize_to_file(write_result_to.as_deref(), &check_res)?; + if check_res.collisions.is_empty() { + println!("OK: No colliding objects found."); + } else { + eprintln!( + "warning: {} potentially colliding inodes found", + check_res.collisions.len() + ); } Ok(()) } @@ -1043,12 +1069,26 @@ where sysroot, verbose, dry_run, + write_result_to, } => { container_remount_sysroot(&sysroot)?; let sysroot = &ostree::Sysroot::new(Some(&gio::File::for_path(&sysroot))); sysroot.load(gio::Cancellable::NONE)?; let sysroot = &SysrootLock::new_from_sysroot(sysroot).await?; - crate::repair::auto_repair_inode_collision(sysroot, dry_run, verbose) + let result = crate::repair::analyze_for_repair(sysroot, verbose)?; + if let Some(write_result_to) = write_result_to.as_deref() { + let mut out = std::fs::File::create(write_result_to) + .map(BufWriter::new) + .with_context(|| { + anyhow::anyhow!("Opening {write_result_to} for writing") + })?; + serde_json::to_writer(&mut out, &result).context("Serializing output")?; + } + if dry_run { + result.check() + } else { + result.repair(sysroot) + } } }, } diff --git a/lib/src/container/store.rs b/lib/src/container/store.rs index 913b0167..ba5ec3e7 100644 --- a/lib/src/container/store.rs +++ b/lib/src/container/store.rs @@ -1385,13 +1385,12 @@ fn compare_commit_trees( pub(crate) fn verify_container_image( sysroot: &SysrootLock, imgref: &ImageReference, + state: &LayeredImageState, colliding_inodes: &BTreeSet, verbose: bool, ) -> Result { let cancellable = gio::Cancellable::NONE; let repo = &sysroot.repo(); - let state = - query_image_ref(repo, imgref)?.ok_or_else(|| anyhow!("Expected present image {imgref}"))?; let merge_commit = state.merge_commit.as_str(); let merge_commit_root = repo.read_commit(merge_commit, gio::Cancellable::NONE)?.0; let merge_commit_root = merge_commit_root @@ -1402,9 +1401,10 @@ pub(crate) fn verify_container_image( // This shouldn't happen anymore let config = state .configuration - .ok_or_else(|| anyhow!("Missing configuration for image {imgref}"))?; + .as_ref() + .ok_or_else(|| anyhow!("Missing configuration for image"))?; let (commit_layer, _component_layers, remaining_layers) = - parse_manifest_layout(&state.manifest, &config)?; + parse_manifest_layout(&state.manifest, config)?; let mut comparison_state = CompareState::default(); diff --git a/lib/src/repair.rs b/lib/src/repair.rs index 5adddf45..e6f4e275 100644 --- a/lib/src/repair.rs +++ b/lib/src/repair.rs @@ -1,16 +1,13 @@ //! System repair functionality -use std::{ - collections::{BTreeMap, BTreeSet}, - process::Command, -}; +use std::collections::{BTreeMap, BTreeSet}; +use std::fmt::Display; use anyhow::{anyhow, Context, Result}; use cap_std::fs::Dir; -use cap_std_ext::prelude::CapStdExtCommandExt; use cap_tempfile::cap_std; use fn_error_context::context; -use ostree::{gio, glib}; +use serde::{Deserialize, Serialize}; use std::os::unix::fs::MetadataExt; use crate::sysroot::SysrootLock; @@ -47,37 +44,52 @@ fn gather_inodes( Ok(()) } -#[context("Analyzing commit for derivation")] -fn commit_is_derived(commit: &glib::Variant) -> Result { - let commit_meta = &glib::VariantDict::new(Some(&commit.child_value(0))); - if commit_meta - .lookup::(crate::container::store::META_MANIFEST_DIGEST)? - .is_some() - { - return Ok(true); - } - if commit_meta - .lookup::("rpmostree.clientlayer")? - .is_some() - { - return Ok(true); +#[derive(Default, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub struct RepairResult { + /// Result of inode checking + pub inodes: InodeCheck, + // Whether we detected a likely corrupted merge commit + pub likely_corrupted_container_image_merges: Vec, + // Whether the booted deployment is likely corrupted + pub booted_is_likely_corrupted: bool, + // Whether the staged deployment is likely corrupted + pub staged_is_likely_corrupted: bool, +} + +#[derive(Default, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub struct InodeCheck { + // Number of >32 bit inodes found + pub inode64: u64, + // Number of <= 32 bit inodes found + pub inode32: u64, + // Number of collisions found (when 64 bit inode is truncated to 32 bit) + pub collisions: BTreeSet, +} + +impl Display for InodeCheck { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "ostree inode check:\n 64bit inodes: {}\n 32 bit inodes: {}\n collisions: {}\n", + self.inode64, + self.inode32, + self.collisions.len() + ) } - Ok(false) } -/// The result of a check_repair operation -#[derive(Debug, PartialEq, Eq)] -pub enum InodeCheckResult { - /// Problems are unlikely. - Okay, - /// There is potential corruption - PotentialCorruption(BTreeSet), +impl InodeCheck { + pub fn is_ok(&self) -> bool { + self.collisions.is_empty() + } } #[context("Checking inodes")] #[doc(hidden)] /// Detect if any commits are potentially incorrect due to inode truncations. -pub fn check_inode_collision(repo: &ostree::Repo, verbose: bool) -> Result { +pub fn check_inode_collision(repo: &ostree::Repo, verbose: bool) -> Result { let repo_dir = repo.dfd_as_dir()?; let objects = repo_dir.open_dir("objects")?; @@ -129,99 +141,122 @@ For more information, see https://github.com/ostreedev/ostree/pull/2874/commits/ } } - let n_big = big_inodes.len(); - let n_small = little_inodes.len(); - println!("Analyzed {n_big} objects with > 32 bit inode numbers and {n_small} objects with <= 32 bit inode numbers"); - if !colliding_inodes.is_empty() { - return Ok(InodeCheckResult::PotentialCorruption( - colliding_inodes - .keys() - .map(|&&v| v) - .collect::>(), - )); - } + // From here let's just track the possibly-colliding 64 bit inode, not also + // the checksum. + let collisions = colliding_inodes + .keys() + .map(|&&v| v) + .collect::>(); - Ok(InodeCheckResult::Okay) + let inode32 = little_inodes.len() as u64; + let inode64 = big_inodes.len() as u64; + Ok(InodeCheck { + inode32, + inode64, + collisions, + }) } /// Attempt to automatically repair any corruption from inode collisions. #[doc(hidden)] -pub fn auto_repair_inode_collision( - sysroot: &SysrootLock, - dry_run: bool, - verbose: bool, -) -> Result<()> { +pub fn analyze_for_repair(sysroot: &SysrootLock, verbose: bool) -> Result { use crate::container::store as container_store; let repo = &sysroot.repo(); - let repo_dir = repo.dfd_as_dir()?; - let mut derived_commits = BTreeSet::new(); - for (_refname, digest) in repo.list_refs(None, gio::Cancellable::NONE)? { - let commit = repo.load_commit(&digest)?.0; - if commit_is_derived(&commit)? { - if verbose { - eprintln!("Found derived commit: {commit}"); - } - derived_commits.insert(digest); - } - } + // Query booted and pending state + let booted_deployment = sysroot.booted_deployment(); + let booted_checksum = booted_deployment.as_ref().map(|b| b.csum()); + let booted_checksum = booted_checksum.as_ref().map(|s| s.as_str()); + let staged_deployment = sysroot.staged_deployment(); + let staged_checksum = staged_deployment.as_ref().map(|b| b.csum()); + let staged_checksum = staged_checksum.as_ref().map(|s| s.as_str()); - // This is not an ironclad guarantee...however, I am pretty confident that there's - // no exposure without derivation today. - if derived_commits.is_empty() { - println!("OK no derived commits found."); - return Ok(()); + let inodes = check_inode_collision(repo, verbose)?; + println!("{}", inodes); + if inodes.is_ok() { + println!("OK no colliding inodes found"); + return Ok(RepairResult { + inodes, + ..Default::default() + }); } - let n_derived = derived_commits.len(); - println!("Found {n_derived} derived commits"); - println!("Backing filesystem information:"); - { - let st = Command::new("stat") - .args(["-f", "."]) - .cwd_dir(repo_dir.try_clone()?) - .status()?; - if !st.success() { - eprintln!("failed to spawn stat: {st:?}"); + + let all_images = container_store::list_images(repo)?; + let all_images = all_images + .into_iter() + .map(|img| crate::container::ImageReference::try_from(img.as_str())) + .collect::>>()?; + println!("Verifying ostree-container images: {}", all_images.len()); + let mut likely_corrupted_container_image_merges = Vec::new(); + let mut booted_is_likely_corrupted = false; + let mut staged_is_likely_corrupted = false; + for imgref in all_images { + if let Some(state) = container_store::query_image_ref(repo, &imgref)? { + if !container_store::verify_container_image( + sysroot, + &imgref, + &state, + &inodes.collisions, + verbose, + )? { + eprintln!("warning: Corrupted image {imgref}"); + likely_corrupted_container_image_merges.push(imgref.to_string()); + let merge_commit = state.merge_commit.as_str(); + if booted_checksum == Some(merge_commit) { + booted_is_likely_corrupted = true; + eprintln!("warning: booted deployment is likely corrupted"); + } else if staged_checksum == Some(merge_commit) { + staged_is_likely_corrupted = true; + eprintln!("warning: staged deployment is likely corrupted"); + } + } + } else { + // This really shouldn't happen + eprintln!("warning: Image was removed from underneath us: {imgref}"); + std::thread::sleep(std::time::Duration::from_secs(1)); } } + Ok(RepairResult { + inodes, + likely_corrupted_container_image_merges, + booted_is_likely_corrupted, + staged_is_likely_corrupted, + }) +} - match check_inode_collision(repo, verbose)? { - InodeCheckResult::Okay => { - println!("OK no colliding inodes found"); - Ok(()) +impl RepairResult { + pub fn check(&self) -> anyhow::Result<()> { + if self.booted_is_likely_corrupted { + eprintln!("warning: booted deployment is likely corrupted"); } - InodeCheckResult::PotentialCorruption(colliding_inodes) => { - eprintln!( - "warning: {} potentially colliding inodes found", - colliding_inodes.len() - ); - let all_images = container_store::list_images(repo)?; - let all_images = all_images - .into_iter() - .map(|img| crate::container::ImageReference::try_from(img.as_str())) - .collect::>>()?; - println!("Verifying {} ostree-container images", all_images.len()); - let mut corrupted_images = Vec::new(); - for imgref in all_images { - if !container_store::verify_container_image( - sysroot, - &imgref, - &colliding_inodes, - verbose, - )? { - eprintln!("warning: Corrupted image {imgref}"); - corrupted_images.push(imgref); - } - } - if corrupted_images.is_empty() { - println!("OK no corrupted images found"); - return Ok(()); + if self.booted_is_likely_corrupted { + eprintln!("warning: staged deployment is likely corrupted"); + } + match self.likely_corrupted_container_image_merges.len() { + 0 => { + println!("OK no corruption found"); + Ok(()) } - if dry_run { - anyhow::bail!("Found potential corruption, dry-run mode enabled"); + n => { + anyhow::bail!("Found corruption in images: {n}") } - container_store::remove_images(repo, corrupted_images.iter())?; - Ok(()) } } + + #[context("Repairing")] + pub fn repair(self, sysroot: &SysrootLock) -> Result<()> { + let repo = &sysroot.repo(); + for imgref in self.likely_corrupted_container_image_merges { + let imgref = crate::container::ImageReference::try_from(imgref.as_str())?; + eprintln!("Flushing cached state for corrupted merged image: {imgref}"); + crate::container::store::remove_images(repo, [&imgref])?; + } + if self.booted_is_likely_corrupted { + anyhow::bail!("TODO redeploy and reboot for booted deployment corruption"); + } + if self.staged_is_likely_corrupted { + anyhow::bail!("TODO undeploy for staged deployment corruption"); + } + Ok(()) + } }