From 60cc55aa03583988751144a9f47024a110b4a9bb Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Mon, 19 Dec 2022 10:52:28 -0800
Subject: [PATCH] Don't restart scheduled or running chained parents

This is a follow-up to @martchus #4498. The intent of that PR,
as the commit message says, is to "Restart failed chained parents
"up the chain"". However, it actually does more than this: it
also restarts chained parents that are queued or running, because
they also do not have "OK" results. This tweaks the check a bit
so we only restart parents if they have a result which is known
to be not OK. This leaves out NONE, which is the result that
scheduled or running tests have.

In a way we cannot know whether restarting a chained parent that
is still running is the right thing to do, but doing so definitely
causes problems in one real-world case:
https://progress.opensuse.org/projects/openqav3/activity?from=2022-12-19
and from the examples backing the original change here, it seems
we only really intended to add the restart behaviour when the
parent test definitely failed, not when its result was not yet
decided. So on the principle of making minimal changes to the
previous behaviour, it seems reasonable to make this change,
which in fact restores the behaviour from before #4498 for
still-running parent jobs. The cases #4498 intended to address
with complex investigation jobs should still be addressed.

This also updates the comments for `duplicate` to more accurately
reflect current behaviour, and highlight the problem that
ignoring children of a parent job can ignore not only other jobs
in the same parallel group as the child job, but other children of
the same parent that are *not* in the same parallel group as the
child job.

Related progress issue: https://progress.opensuse.org/issues/110458
Related progress issue: https://progress.opensuse.org/issues/112256

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 lib/OpenQA/Schema/Result/Jobs.pm       | 6 +++---
 t/05-scheduler-restart-and-duplicate.t | 5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/OpenQA/Schema/Result/Jobs.pm b/lib/OpenQA/Schema/Result/Jobs.pm
index b49a60661..0084777ea 100644
--- a/lib/OpenQA/Schema/Result/Jobs.pm
+++ b/lib/OpenQA/Schema/Result/Jobs.pm
@@ -745,7 +744,7 @@ sub cluster_jobs ($self, @args) {
             unless ($skip_parents || $cancelmode) {
                 my $parent_result = $p->result;
                 $p->cluster_jobs(jobs => $jobs, skip_children => 1, cancelmode => $cancelmode)
-                  if !$parent_result || !OpenQA::Jobs::Constants::is_ok_result($parent_result);
+                  if !$parent_result || grep { $parent_result eq $_ } NOT_OK_RESULTS;
             }
             next;
         }
@@ -861,13 +860,13 @@ for PARALLEL dependencies:
  + if child is clone, find the latest clone and clone it
 
 for CHAINED dependencies:
-- do NOT clone parents
+- only clone failed parents, ignoring their children (our siblings, but also potentially our cousins from other child groups)
  + create new dependency - duplicit cloning is prevented by ignorelist, webui will show multiple chained deps though
 - clone children
  + if child is clone, find the latest clone and clone it
 
 for DIRECTLY_CHAINED dependencies:
-- clone parents recursively but ignore their children (our siblings)
+- clone parents recursively but ignore their children (our siblings, but also potentially our cousins from other child groups)
  + if parent is clone, find the latest clone and clone it
 - clone children
  + if child is clone, find the latest clone and clone it
diff --git a/t/05-scheduler-restart-and-duplicate.t b/t/05-scheduler-restart-and-duplicate.t
index 03a8b9c40..a4296184a 100644
--- a/t/05-scheduler-restart-and-duplicate.t
+++ b/t/05-scheduler-restart-and-duplicate.t
@@ -133,12 +133,13 @@ subtest 'restart with (directly) chained child' => sub {
     );
     my $job_data_99926;
     subtest 'cluster jobs for 99937 which has one chained child and one chained parent' => sub {
+        job_get_rs(99926)->update({result => FAILED});
         is_deeply job_get_rs(99937)->cluster_jobs, \%expected_cluster,
-          'chained parent considered for restarting as its result is not ok';
+          'chained parent considered for restarting as its result is failed';
         job_get_rs(99926)->update({result => SOFTFAILED});
         $job_data_99926 = delete $expected_cluster{99926};
         is_deeply job_get_rs(99937)->cluster_jobs, \%expected_cluster,
-          'only child considered for restarting as the parent result is ok';
+          'only child considered for restarting as the parent result is not failed';
     };
 
     # restart the job
-- 
2.39.0