Skip to content

spin doctor prototype #1435

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.

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

Merged
merged 6 commits into from
May 17, 2023
Merged
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
78 changes: 72 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ spin-bindle = { path = "crates/bindle" }
spin-build = { path = "crates/build" }
spin-common = { path = "crates/common" }
spin-config = { path = "crates/config" }
spin-doctor = { path = "crates/doctor" }
spin-http = { path = "crates/http" }
spin-trigger-http = { path = "crates/trigger-http" }
spin-loader = { path = "crates/loader" }
Expand Down
18 changes: 18 additions & 0 deletions crates/doctor/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
[package]
name = "spin-doctor"
version = "0.1.0"
edition = "2021"

[dependencies]
anyhow = "1"
async-trait = "0.1"
serde = { version = "1", features = ["derive"] }
similar = "2"
spin-loader = { path = "../loader" }
tokio = "1"
toml = "0.7"
toml_edit = "0.19"
tracing = { workspace = true }

[dev-dependencies]
tempfile = "3"
184 changes: 184 additions & 0 deletions crates/doctor/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
//! Spin doctor: check and automatically fix problems with Spin apps.
#![deny(missing_docs)]

use std::{fmt::Debug, fs, future::Future, path::PathBuf, pin::Pin, sync::Arc};

use anyhow::{ensure, Context, Result};
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I know we do this everywhere, but I really prefer not using Result unqualified. Unqualified Result should always be std::result::Result. Everywhere else we should use it qualified (e.g., anyhow::Result, io::Result, etc.). You don't need to address that here though since this is far from the only place where this occurs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While I understand the rationale, I don't think it's necessary for anyhow::Result because 1) it is ubiquitous across the codebase (for better or worse) and 2) its definition makes it compatible with std::result::Result:

pub type Result<T, E = Error> = core::result::Result<T, E>;

use async_trait::async_trait;
use tokio::sync::Mutex;
use toml_edit::Document;

/// Diagnoses for app manifest format problems.
pub mod manifest;
/// Test helpers.
pub mod test;
/// Diagnoses for Wasm source problems.
pub mod wasm;

/// Configuration for an app to be checked for problems.
pub struct Checkup {
manifest_path: PathBuf,
diagnostics: Vec<Box<dyn BoxingDiagnostic>>,
}

impl Checkup {
/// Return a new checkup for the app manifest at the given path.
pub fn new(manifest_path: impl Into<PathBuf>) -> Self {
let mut checkup = Self {
manifest_path: manifest_path.into(),
diagnostics: vec![],
};
checkup.add_diagnostic::<manifest::version::VersionDiagnostic>();
checkup.add_diagnostic::<manifest::trigger::TriggerDiagnostic>();
checkup.add_diagnostic::<wasm::missing::WasmMissingDiagnostic>();
checkup
}

/// Add a detectable problem to this checkup.
pub fn add_diagnostic<D: Diagnostic + Default + 'static>(&mut self) -> &mut Self {
self.diagnostics.push(Box::<D>::default());
self
}

fn patient(&self) -> Result<PatientApp> {
let path = &self.manifest_path;
ensure!(
path.is_file(),
"No Spin app manifest file found at {path:?}"
);

let contents = fs::read_to_string(path)
.with_context(|| format!("Couldn't read Spin app manifest file at {path:?}"))?;

let manifest_doc: Document = contents
.parse()
.with_context(|| format!("Couldn't parse manifest file at {path:?} as valid TOML"))?;

Ok(PatientApp {
manifest_path: path.into(),
manifest_doc,
})
}

/// Find problems with the configured app, calling the given closure with
/// each problem found.
pub async fn for_each_diagnosis<F>(&self, mut f: F) -> Result<usize>
where
F: for<'a> FnMut(
Box<dyn Diagnosis + 'static>,
&'a mut PatientApp,
) -> Pin<Box<dyn Future<Output = Result<()>> + 'a>>,
{
let patient = Arc::new(Mutex::new(self.patient()?));
let mut count = 0;
for diagnostic in &self.diagnostics {
let patient = patient.clone();
let diags = diagnostic
.diagnose_boxed(&*patient.lock().await)
.await
.unwrap_or_else(|err| {
tracing::debug!("Diagnose failed: {err:?}");
vec![]
});
count += diags.len();
for diag in diags {
let mut patient = patient.lock().await;
f(diag, &mut patient).await?;
}
}
Ok(count)
}
}

/// An app "patient" to be checked for problems.
#[derive(Clone)]
pub struct PatientApp {
/// Path to an app manifest file.
pub manifest_path: PathBuf,
/// Parsed app manifest TOML document.
pub manifest_doc: Document,
}

/// The Diagnose trait implements the detection of a particular Spin app problem.
#[async_trait]
pub trait Diagnostic: Send + Sync {
/// A [`Diagnosis`] representing the problem(s) this can detect.
type Diagnosis: Diagnosis;

/// Check the given [`Patient`], returning any problem(s) found.
///
/// If multiple _independently addressable_ problems are found, this may
/// return multiple instances. If two "logically separate" problems would
/// have the same fix, they should be represented with the same instance.
async fn diagnose(&self, patient: &PatientApp) -> Result<Vec<Self::Diagnosis>>;
}

/// The Diagnosis trait represents a detected problem with a Spin app.
pub trait Diagnosis: Debug + Send + Sync + 'static {
/// Return a human-friendly description of this problem.
fn description(&self) -> String;

/// Return true if this problem is "critical", i.e. if the app's
/// configuration or environment is invalid. Return false for
/// "non-critical" problems like deprecations.
fn is_critical(&self) -> bool {
true
}

/// Return a [`Treatment`] that can (potentially) fix this problem, or
/// None if there is no automatic fix.
fn treatment(&self) -> Option<&dyn Treatment> {
None
}
}

/// The Treatment trait represents a (potential) fix for a detected problem.
#[async_trait]
pub trait Treatment: Sync {
/// Return a short (single line) description of what this fix will do, as
/// an imperative, e.g. "Upgrade the library".
fn summary(&self) -> String;

/// Return a detailed description of what this fix will do, such as a file
/// diff or list of commands to be executed.
///
/// May return `Err(DryRunNotSupported.into())` if no such description is
/// available, which is the default implementation.
async fn dry_run(&self, patient: &PatientApp) -> Result<String> {
let _ = patient;
Err(DryRunNotSupported.into())
}

/// Attempt to fix this problem. Return Ok only if the problem is
/// successfully fixed.
async fn treat(&self, patient: &mut PatientApp) -> Result<()>;
}

/// Error returned by [`Treatment::dry_run`] if dry run isn't supported.
#[derive(Debug)]
pub struct DryRunNotSupported;

impl std::fmt::Display for DryRunNotSupported {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "dry run not implemented for this treatment")
}
}

impl std::error::Error for DryRunNotSupported {}

#[async_trait]
trait BoxingDiagnostic {
async fn diagnose_boxed(&self, patient: &PatientApp) -> Result<Vec<Box<dyn Diagnosis>>>;
}

#[async_trait]
impl<Factory: Diagnostic> BoxingDiagnostic for Factory {
async fn diagnose_boxed(&self, patient: &PatientApp) -> Result<Vec<Box<dyn Diagnosis>>> {
Ok(self
.diagnose(patient)
.await?
.into_iter()
.map(|diag| Box::new(diag) as Box<dyn Diagnosis>)
.collect())
}
}
Loading