Auto merge of #604 - Mark-Simulacrum:background-reports, r=Mark-Simulacrum

Discover necessity of reporting in the background

This will increase the latency to starting a report by ~10 minutes in the worst case, but those take long enough to generate (and experiments are slow enough) that this doesn't seem like a big deal. In practice, my hope is that it will:

* Avoid needing retry-report quite as frequently, as OOMs and other abnormal crashes will be gracefully recovered from (albeit likely sending the server into a death spiral if we *really* can't generate the report; in practice this shouldn't happen). It was already somewhat the case that a server restart would start the generation again, just that a thread *panic* didn't since the report generation thread died in that case. So this should be essentially as-before.
* Avoid fairly heavy piece of work done on *every* new result, which hopefully improved result endpoint performance and reduces normal server load.

See also commit messages for some additional details.

Author: bors

src/cli.rs

@@ -506,8 +506,6 @@ impl Crater {
                     );
                     workspace.purge_all_build_dirs()?;
                     res?;
-
-                    experiment.set_status(&db, Status::NeedsReport)?;
                 } else {
                     bail!("missing experiment {}", ex.0);
                 }
@@ -522,18 +520,18 @@ impl Crater {
                 let db = Database::open()?;
 
                 if let Some(mut experiment) = Experiment::get(&db, &ex.0)? {
-                    // Update the status
-                    match (experiment.status, force) {
-                        (Status::NeedsReport, _) | (Status::ReportFailed, _) | (_, true) => {
-                            experiment.set_status(&db, Status::GeneratingReport)?;
-                        }
-                        (other, false) => bail!(
-                            "can't generate the report of an experiment with status {} \
+                    let (completed, all) = experiment.raw_progress(&db)?;
+                    if !force && completed != all {
+                        bail!(
+                            "can't generate the report of an incomplete experiment: {}/{} results \
                              (use --force to override)",
-                            other
-                        ),
+                            completed,
+                            all,
+                        );
                     }
 
+                    experiment.set_status(&db, Status::GeneratingReport)?;
+
                     let result_db = DatabaseDB::new(&db);
                     let res = report::gen(
                         &result_db,
@@ -564,18 +562,18 @@ impl Crater {
                 let db = Database::open()?;
 
                 if let Some(mut experiment) = Experiment::get(&db, &ex.0)? {
-                    // Update the status
-                    match (experiment.status, force) {
-                        (Status::NeedsReport, _) | (Status::ReportFailed, _) | (_, true) => {
-                            experiment.set_status(&db, Status::GeneratingReport)?;
-                        }
-                        (other, false) => bail!(
-                            "can't publish the report of an experiment with status {} \
+                    let (completed, all) = experiment.raw_progress(&db)?;
+                    if !force && completed != all {
+                        bail!(
+                            "can't publish the report of an incomplete experiment: {}/{} results \
                              (use --force to override)",
-                            other
-                        ),
+                            completed,
+                            all,
+                        );
                     }
 
+                    experiment.set_status(&db, Status::GeneratingReport)?;
+
                     let result_db = DatabaseDB::new(&db);
                     let client = report::get_client_for_bucket(&s3_prefix.bucket)?;
 

src/experiments.rs

@@ -298,20 +298,31 @@ impl Experiment {
         }
     }
 
-    pub fn first_by_status(db: &Database, status: Status) -> Fallible<Option<Experiment>> {
-        let record = db.get_row(
-            "SELECT * FROM experiments \
-             WHERE status = ?1 \
-             ORDER BY priority DESC, created_at;",
-            &[&status.to_str()],
-            |r| ExperimentDBRecord::from_row(r),
-        )?;
-
-        if let Some(record) = record {
-            Ok(Some(record.into_experiment()?))
-        } else {
-            Ok(None)
+    // Returns the first experiment which has all results ready (and so can
+    // produce a complete report). However, the experiment should not be
+    // *completed* yet. Note that this may return an experiment which has had
+    // report generation already start.
+    pub fn ready_for_report(db: &Database) -> Fallible<Option<Experiment>> {
+        let unfinished = Self::unfinished(db)?;
+        for ex in unfinished {
+            if ex.status == Status::ReportFailed {
+                // Skip experiments whose report failed to generate. This avoids
+                // constantly retrying reports (and posting a message each time
+                // about the attempt); the retry-report command can override the
+                // failure state. In practice we rarely *fail* to generate
+                // reports in a clean way (instead OOMing or panicking, in which
+                // case it is fine to automatically retry the report, as we've
+                // not posted anything on GitHub -- it may be a problem from a
+                // performance perspective but no more than that).
+                continue;
+            }
+            let (completed, all) = ex.raw_progress(db)?;
+            if completed == all {
+                return Ok(Some(ex));
+            }
         }
+
+        Ok(None)
     }
 
     pub fn find_next(db: &Database, assignee: &Assignee) -> Fallible<Option<Experiment>> {

src/server/reports.rs

@@ -37,7 +37,7 @@ fn reports_thread(data: &Data, github_data: Option<&GithubData>) -> Fallible<()>
     let results = DatabaseDB::new(&data.db);
 
     loop {
-        let mut ex = match Experiment::first_by_status(&data.db, Status::NeedsReport)? {
+        let mut ex = match Experiment::ready_for_report(&data.db)? {
             Some(ex) => ex,
             None => {
                 // This will sleep AUTOMATIC_THREAD_WAKEUP seconds *or* until a wake is received

src/server/routes/agent.rs

@@ -1,5 +1,5 @@
 use crate::agent::Capabilities;
-use crate::experiments::{Assignee, Experiment, Status};
+use crate::experiments::{Assignee, Experiment};
 use crate::prelude::*;
 use crate::results::{DatabaseDB, EncodingType, ProgressData};
 use crate::server::api_types::{AgentConfig, ApiResponse};
@@ -160,7 +160,7 @@ fn endpoint_record_progress(
     auth: AuthDetails,
 ) -> Fallible<Response<Body>> {
     let data = mutex.lock().unwrap();
-    let mut ex = Experiment::get(&data.db, &result.experiment_name)?
+    let ex = Experiment::get(&data.db, &result.experiment_name)?
         .ok_or_else(|| err_msg("no experiment run by this agent"))?;
 
     data.metrics
@@ -169,13 +169,6 @@ fn endpoint_record_progress(
     let db = DatabaseDB::new(&data.db);
     db.store(&ex, &result.data, EncodingType::Gzip)?;
 
-    let (completed, all) = ex.raw_progress(&data.db)?;
-    if completed == all {
-        ex.set_status(&data.db, Status::NeedsReport)?;
-        info!("experiment {} completed, marked as needs-report", ex.name);
-        data.reports_worker.wake(); // Ensure the reports worker is awake
-    }
-
     Ok(ApiResponse::Success { result: true }.into_response()?)
 }