Auto merge of #602 - Mark-Simulacrum:recover-from-error, r=pietroalbini

Recover gracefully from agent failure

Previously, any agent reporting an error would cause an experiment to come to a
hard stop. This is noisy (leads to a user-visible report) and, at least lately,
not actually representative of a terminal failure. Instead, we take the approach
of re-queueing any crates assigned to that agent and logging an error on the
server host specifying which agent failed.

As the error message is now not typically posted publicly (and no longer needs
to be short for being in a GitHub comment), we will also have more opportunity
to add extra metadata over time to that message (in future PRs) enabling
diagnosing and fixing these errors.

In the future, we will likely also want some tracking/reporting of these
failures such that hard errors across all agents lead to alerting the infra
team, but this may be best done through Prometheus metrics rather than GitHub
"end-user" visible comments (which are likely not the best way to reach infra
team members regardless).

r? `@pietroalbini`

Author: bors

src/experiments.rs

@@ -485,8 +485,9 @@ impl Experiment {
         }
     }
 
-    pub fn report_failure(&mut self, db: &Database, agent: &Assignee) -> Fallible<()> {
-        // Mark all the running crates from this agent as failed as well if the experiment failed
+    pub fn handle_failure(&mut self, db: &Database, agent: &Assignee) -> Fallible<()> {
+        // Mark all the running crates from this agent as queued (so that they
+        // run again)
         db.execute(
             "
             UPDATE experiment_crates
@@ -495,14 +496,13 @@ impl Experiment {
             AND assigned_to = ?4
             ",
             &[
-                &Status::Failed.to_string(),
+                &Status::Queued.to_string(),
                 &self.name,
                 &Status::Running.to_string(),
                 &agent.to_string(),
             ],
         )?;
-
-        self.set_status(db, Status::Failed)
+        Ok(())
     }
 
     pub fn set_status(&mut self, db: &Database, status: Status) -> Fallible<()> {
@@ -1063,6 +1063,8 @@ mod tests {
         assert_eq!(uncompleted_crates.len(), 0);
     }
 
+    // A failure is handled by re-queueing any running crates for a given agent,
+    // to be picked up by the next agent to ask for them.
     #[test]
     fn test_failed_experiment() {
         let db = Database::temp().unwrap();
@@ -1079,9 +1081,12 @@ mod tests {
             .get_uncompleted_crates(&db, &config, &agent1)
             .unwrap()
             .is_empty());
-        ex.report_failure(&db, &agent1).unwrap();
-        assert!(!Experiment::next(&db, &agent1).unwrap().is_some());
-        assert_eq!(ex.status, Status::Failed);
-        assert!(ex.get_running_crates(&db, &agent1).unwrap().is_empty());
+        ex.handle_failure(&db, &agent1).unwrap();
+        assert!(Experiment::next(&db, &agent1).unwrap().is_some());
+        assert_eq!(ex.status, Status::Running);
+        assert!(!ex
+            .get_uncompleted_crates(&db, &config, &agent1)
+            .unwrap()
+            .is_empty());
     }
 }

src/report/analyzer.rs

@@ -40,7 +40,7 @@ fn analyze_detailed(toolchain: usize, crates: Vec<CrateResult>) -> ReportCrates
     let mut root = Vec::new();
     for krate in crates {
         if let BuildFail(FailureReason::DependsOn(ref deps)) =
-            (&krate.runs[toolchain]).as_ref().unwrap().res
+            krate.runs[toolchain].as_ref().unwrap().res
         {
             for dep in deps {
                 tree.entry(dep.clone())

src/report/markdown.rs

@@ -31,7 +31,7 @@ struct ResultsContext<'a> {
 }
 
 fn write_crate(
-    mut rendered: &mut String,
+    rendered: &mut String,
     krate: &CrateResult,
     comparison: Comparison,
     is_child: bool,
@@ -76,7 +76,7 @@ fn write_crate(
         };
 
         writeln!(
-            &mut rendered,
+            rendered,
             "{}[{}{}]({}) {} {} **{}** [start]({}/log.txt) | [end]({}/log.txt)",
             prefix,
             krate.name,
@@ -90,7 +90,7 @@ fn write_crate(
         )?;
     } else {
         writeln!(
-            &mut rendered,
+            rendered,
             "{}[{}{}]({}) {} [start]({}/log.txt) | [end]({}/log.txt)",
             prefix, krate.name, status_warning, krate.url, comparison, runs[1], runs[3]
         )?;
@@ -103,10 +103,10 @@ fn render_markdown(context: &ResultsContext) -> Fallible<String> {
     let mut rendered = String::new();
 
     //add title
-    writeln!(&mut rendered, "# Crater report for {}\n\n", context.ex.name)?;
+    writeln!(rendered, "# Crater report for {}\n\n", context.ex.name)?;
 
     for (comparison, results) in context.categories.iter() {
-        writeln!(&mut rendered, "\n### {}", comparison)?;
+        writeln!(rendered, "\n### {}", comparison)?;
         match results {
             ReportCratesMD::Plain(crates) => {
                 for krate in crates {
@@ -123,7 +123,7 @@ fn render_markdown(context: &ResultsContext) -> Fallible<String> {
 
                 for (krate, deps) in orphans {
                     writeln!(
-                        &mut rendered,
+                        rendered,
                         "* [{}]({}) (not covered in crater testing)",
                         krate,
                         crate_to_url(krate)

src/server/metrics.rs

@@ -11,11 +11,13 @@ use prometheus::{
 
 const JOBS_METRIC: &str = "crater_completed_jobs_total";
 const AGENT_WORK_METRIC: &str = "crater_agent_supposed_to_work";
+const AGENT_FAILED: &str = "crater_agent_failure";
 const LAST_CRATES_UPDATE_METRIC: &str = "crater_last_crates_update";
 
 #[derive(Clone)]
 pub struct Metrics {
     crater_completed_jobs_total: IntCounterVec,
+    crater_agent_failure: IntCounterVec,
     crater_work_status: IntGaugeVec,
     crater_last_crates_update: IntGauge,
 }
@@ -25,6 +27,9 @@ impl Metrics {
         let jobs_opts = prometheus::opts!(JOBS_METRIC, "total completed jobs");
         let crater_completed_jobs_total =
             prometheus::register_int_counter_vec!(jobs_opts, &["agent", "experiment"])?;
+        let failure_opts = prometheus::opts!(AGENT_FAILED, "total completed jobs");
+        let crater_agent_failure =
+            prometheus::register_int_counter_vec!(failure_opts, &["agent", "experiment"])?;
         let agent_opts = prometheus::opts!(AGENT_WORK_METRIC, "is agent supposed to work");
         let crater_work_status = prometheus::register_int_gauge_vec!(agent_opts, &["agent"])?;
         let crates_update_opts =
@@ -33,11 +38,18 @@ impl Metrics {
 
         Ok(Metrics {
             crater_completed_jobs_total,
+            crater_agent_failure,
             crater_work_status,
             crater_last_crates_update,
         })
     }
 
+    pub fn record_error(&self, agent: &str, experiment: &str) {
+        self.crater_agent_failure
+            .with_label_values(&[agent, experiment])
+            .inc_by(1);
+    }
+
     pub fn record_completed_jobs(&self, agent: &str, experiment: &str, amount: i64) {
         self.crater_completed_jobs_total
             .with_label_values(&[agent, experiment])

src/server/routes/agent.rs

@@ -51,7 +51,7 @@ pub fn routes(
         .and(warp::path("next-experiment"))
         .and(warp::path::end())
         .and(mutex_filter.clone())
-        .and(github_data_filter.clone())
+        .and(github_data_filter)
         .and(auth_filter(data.clone(), TokenType::Agent))
         .map(endpoint_next_experiment);
 
@@ -75,7 +75,6 @@ pub fn routes(
         .and(warp::path::end())
         .and(warp::body::json())
         .and(mutex_filter)
-        .and(github_data_filter)
         .and(auth_filter(data, TokenType::Agent))
         .map(endpoint_error);
 
@@ -197,38 +196,22 @@ fn endpoint_heartbeat(data: Arc<Data>, auth: AuthDetails) -> Fallible<Response<B
 fn endpoint_error(
     error: ExperimentData<HashMap<String, String>>,
     mutex: Arc<Mutex<Data>>,
-    github_data: Option<Arc<GithubData>>,
     auth: AuthDetails,
 ) -> Fallible<Response<Body>> {
+    log::error!(
+        "agent {} failed while running {}: {:?}",
+        auth.name,
+        error.experiment_name,
+        error.data.get("error")
+    );
+
     let data = mutex.lock().unwrap();
     let mut ex = Experiment::get(&data.db, &error.experiment_name)?
         .ok_or_else(|| err_msg("no experiment run by this agent"))?;
 
-    //also set status to failed
-    ex.report_failure(&data.db, &Assignee::Agent(auth.name))?;
+    data.metrics.record_error(&auth.name, &ex.name);
+    ex.handle_failure(&data.db, &Assignee::Agent(auth.name))?;
 
-    if let Some(github_data) = github_data.as_ref() {
-        if let Some(ref github_issue) = ex.github_issue {
-            Message::new()
-                .line(
-                    "rotating_light",
-                    format!(
-                        "Experiment **`{}`** has encountered an error: {}",
-                        ex.name,
-                        error.data.get("error").unwrap_or(&String::from("no error")),
-                    ),
-                )
-                .line(
-                    "hammer_and_wrench",
-                    "If the error is fixed use the `retry` command.",
-                )
-                .note(
-                    "sos",
-                    "Can someone from the infra team check in on this? @rust-lang/infra",
-                )
-                .send(&github_issue.api_url, &data, github_data)?;
-        }
-    }
     Ok(ApiResponse::Success { result: true }.into_response()?)
 }
 

src/toolchain.rs

@@ -118,7 +118,7 @@ impl FromStr for Toolchain {
         for part in parts {
             if let Some(equal_idx) = part.find('=') {
                 let (flag, value_with_equal) = part.split_at(equal_idx);
-                let value = (&value_with_equal[1..]).to_string();
+                let value = value_with_equal[1..].to_string();
 
                 if value.is_empty() {
                     return Err(ToolchainParseError::InvalidFlag(flag.to_string()));