[2/8] Require project argument to be specified for daemon mode
diff mbox series

Message ID 20180720115728.19312-2-ruscur@russell.cc
State Accepted
Headers show
Series
  • [1/8] Convert from hyper to reqwest
Related show

Commit Message

Russell Currey July 20, 2018, 11:57 a.m. UTC
So this might seem out of place, but bear with me here.

Right now, the way that we check for patches is to get the latest
patches "page" from the API.  For each of them, we see if the project
the patch is from matches one we have settings for, and if so we
test it.  We exhaust the patch list page we got, and then we get a
new one.

There are lots of problems with this.  The first one is that if we're
testing lots of patches and the Patchwork instance is high traffic,
we risk missing patches that fall to page 2 or beyond in that time.
We currently don't have a solution for this.

The next issue is that when I originally implemented this, filtering
by project didn't work properly in the patches API, making it
difficult to track all patches, especially for niche projects with
less activity (since running into one patch of a series will lead
to them all being tested, even if snowpatch doesn't find them all
individually in the patch list).

This is no longer the case however, and we can filter by project
on the patch list and actually get a full page of patches in that
project.  As a result, until both the following conditions are true:

 - patchwork has a suitable events API and/or better pagination
 - snowpatch has a proper threading model for distributed work

Limit snowpatch to one project at a time to minimise the chance of
missing patches, and users should just spin up more than one
instance if they want to test multiple projects at once.

Signed-off-by: Russell Currey <ruscur@russell.cc>
---
 src/main.rs      | 130 +++++++++++++++++++++--------------------------
 src/patchwork.rs |   8 ++-
 2 files changed, 64 insertions(+), 74 deletions(-)

Comments

Andrew Donnellan July 23, 2018, 12:36 a.m. UTC | #1
On 20/07/18 21:57, Russell Currey wrote:
> So this might seem out of place, but bear with me here.
> 
> Right now, the way that we check for patches is to get the latest
> patches "page" from the API.  For each of them, we see if the project
> the patch is from matches one we have settings for, and if so we
> test it.  We exhaust the patch list page we got, and then we get a
> new one.
> 
> There are lots of problems with this.  The first one is that if we're
> testing lots of patches and the Patchwork instance is high traffic,
> we risk missing patches that fall to page 2 or beyond in that time.
> We currently don't have a solution for this.
> 
> The next issue is that when I originally implemented this, filtering
> by project didn't work properly in the patches API, making it
> difficult to track all patches, especially for niche projects with
> less activity (since running into one patch of a series will lead
> to them all being tested, even if snowpatch doesn't find them all
> individually in the patch list).
> 
> This is no longer the case however, and we can filter by project
> on the patch list and actually get a full page of patches in that
> project.  As a result, until both the following conditions are true:
> 
>   - patchwork has a suitable events API and/or better pagination
>   - snowpatch has a proper threading model for distributed work
> 
> Limit snowpatch to one project at a time to minimise the chance of
> missing patches, and users should just spin up more than one
> instance if they want to test multiple projects at once.
> 
> Signed-off-by: Russell Currey <ruscur@russell.cc>

This looks like a sufficiently sensible decision for now.

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

Patch
diff mbox series

diff --git a/src/main.rs b/src/main.rs
index 44bc013..5bf6187 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -67,8 +67,8 @@  mod utils;
 
 static USAGE: &'static str = "
 Usage:
-  snowpatch <config-file> [--count=<count>] [--project <name>]
-  snowpatch <config-file> --mbox <mbox> --project <name>
+  snowpatch <config-file> --project <name> [--count=<count>]
+  snowpatch <config-file> --project <name> --mbox <mbox>
   snowpatch <config-file> --patch <id>
   snowpatch <config-file> --series <id>
   snowpatch -v | --version
@@ -77,11 +77,11 @@  Usage:
 By default, snowpatch runs as a long-running daemon.
 
 Options:
+  --project <name>          Test patches for the given project.
   --count <count>           Run tests on <count> recent series.
   --patch <id>              Run tests on the given Patchwork patch.
   --series <id>             Run tests on the given Patchwork series.
   --mbox <mbox>             Run tests on the given mbox file. Requires --project
-  --project <name>          Test patches for the given project.
   -v, --version             Output version information.
   -h, --help                Output this help text.
 ";
@@ -349,19 +349,6 @@  fn main() {
         panic!("Can't specify both --series and --patch");
     }
 
-    if args.flag_mbox != "" && args.flag_project != "" {
-        info!("snowpatch is testing a local patch.");
-        let patch = Path::new(&args.flag_mbox);
-        match settings.projects.get(&args.flag_project) {
-            None => panic!("Couldn't find project {}", args.flag_project),
-            Some(project) => {
-                test_patch(&settings, &client, project, patch, true);
-            }
-        }
-
-        return;
-    }
-
     if args.flag_patch > 0 {
         info!("snowpatch is testing a patch from Patchwork.");
         let patch = patchwork.get_patch(&(args.flag_patch as u64)).unwrap();
@@ -400,6 +387,17 @@  fn main() {
         return;
     }
 
+    // At this point, specifying a project is required
+    let project = settings.projects.get(&args.flag_project).unwrap();
+
+    if args.flag_mbox != "" {
+        info!("snowpatch is testing a local patch.");
+        let patch = Path::new(&args.flag_mbox);
+        test_patch(&settings, &client, project, patch, true);
+
+        return;
+    }
+
     // The number of patches tested so far.  If --count isn't provided, this is unused.
     let mut patch_count = 0;
 
@@ -411,7 +409,7 @@  fn main() {
      */
     'daemon: loop {
         let patch_list = patchwork
-            .get_patch_query()
+            .get_patch_query(&args.flag_project)
             .unwrap_or_else(|err| panic!("Failed to obtain patch list: {}", err));
         info!("snowpatch is ready to test new revisions from Patchwork.");
         for patch in patch_list {
@@ -426,72 +424,60 @@  fn main() {
                 continue;
             }
 
-            //let project = patchwork.get_project(&patch.project).unwrap();
-            // Skip if we're using -p and it's the wrong project
-            if args.flag_project != "" && patch.project.link_name != args.flag_project {
-                debug!(
+            // Skip if it's the wrong project
+            if patch.project.link_name != args.flag_project {
+                warn!(
                     "Skipping patch {} ({}) (wrong project: {})",
                     patch.name, patch.id, patch.project.link_name
                 );
                 continue;
             }
 
-            match settings.projects.get(&patch.project.link_name) {
-                None => {
-                    debug!(
-                        "Project {} not configured for patch {}",
-                        &patch.project.link_name, patch.name
-                    );
-                    continue;
-                }
-                Some(project) => {
-                    // TODO(ajd): Refactor this.
-                    let hefty_tests;
-                    let mbox = if patch.has_series() {
-                        debug!(
-                            "Patch {} has a series at {}!",
-                            &patch.name, &patch.series[0].url
-                        );
-                        let series = patchwork.get_series_by_url(&patch.series[0].url);
-                        match series {
-                            Ok(series) => {
-                                if !series.received_all {
-                                    debug!("Series is incomplete, skipping patch for now");
-                                    continue;
-                                }
-                                let dependencies = patchwork.get_patch_dependencies(&patch);
-                                hefty_tests = dependencies.len() == series.patches.len();
-                                patchwork.get_patches_mbox(dependencies)
-                            }
-                            Err(e) => {
-                                debug!("Series is not OK: {}", e);
-                                hefty_tests = true;
-                                patchwork.get_patch_mbox(&patch)
-                            }
+            // TODO(ajd): Refactor this.
+            let hefty_tests;
+            let mbox = if patch.has_series() {
+                debug!(
+                    "Patch {} has a series at {}!",
+                    &patch.name, &patch.series[0].url
+                );
+                let series = patchwork.get_series_by_url(&patch.series[0].url);
+                match series {
+                    Ok(series) => {
+                        if !series.received_all {
+                            debug!("Series is incomplete, skipping patch for now");
+                            continue;
                         }
-                    } else {
+                        let dependencies = patchwork.get_patch_dependencies(&patch);
+                        hefty_tests = dependencies.len() == series.patches.len();
+                        patchwork.get_patches_mbox(dependencies)
+                    }
+                    Err(e) => {
+                        debug!("Series is not OK: {}", e);
                         hefty_tests = true;
                         patchwork.get_patch_mbox(&patch)
-                    };
-
-                    let results = test_patch(&settings, &client, project, &mbox, hefty_tests);
-
-                    // Delete the temporary directory with the patch in it
-                    fs::remove_dir_all(mbox.parent().unwrap())
-                        .unwrap_or_else(|err| error!("Couldn't delete temp directory: {}", err));
-                    if project.push_results {
-                        for result in results {
-                            patchwork.post_test_result(result, &patch.checks).unwrap();
-                        }
-                    }
-                    if args.flag_count > 0 {
-                        patch_count += 1;
-                        debug!("Tested {} patches out of {}", patch_count, args.flag_count);
-                        if patch_count >= args.flag_count {
-                            break 'daemon;
-                        }
                     }
                 }
+            } else {
+                hefty_tests = true;
+                patchwork.get_patch_mbox(&patch)
+            };
+
+            let results = test_patch(&settings, &client, project, &mbox, hefty_tests);
+
+            // Delete the temporary directory with the patch in it
+            fs::remove_dir_all(mbox.parent().unwrap())
+                .unwrap_or_else(|err| error!("Couldn't delete temp directory: {}", err));
+            if project.push_results {
+                for result in results {
+                    patchwork.post_test_result(result, &patch.checks).unwrap();
+                }
+            }
+            if args.flag_count > 0 {
+                patch_count += 1;
+                debug!("Tested {} patches out of {}", patch_count, args.flag_count);
+                if patch_count >= args.flag_count {
+                    break 'daemon;
+                }
             }
         }
         info!("Finished testing new revisions, sleeping.");
diff --git a/src/patchwork.rs b/src/patchwork.rs
index d59fc40..378c6ca 100644
--- a/src/patchwork.rs
+++ b/src/patchwork.rs
@@ -300,8 +300,12 @@  impl PatchworkServer {
         serde_json::from_str(&self.get_url_string(url).unwrap())
     }
 
-    pub fn get_patch_query(&self) -> Result<Vec<Patch>, serde_json::Error> {
-        let url = format!("{}{}/patches/{}", &self.url, PATCHWORK_API, PATCHWORK_QUERY);
+    pub fn get_patch_query(&self, project: &str) -> Result<Vec<Patch>, serde_json::Error> {
+        let url = format!(
+            "{}{}/patches/{}&project={}",
+            &self.url, PATCHWORK_API, PATCHWORK_QUERY, project
+        );
+
         serde_json::from_str(&self.get_url_string(&url)
             .unwrap_or_else(|err| panic!("Failed to connect to Patchwork: {}", err)))
     }