Merge pull request #6850 from Martchus/overview-limit

Fix possibly excessive memory use when computer test result overview

Author: mergify[bot]

lib/OpenQA/Schema/Result/Jobs.pm

@@ -418,7 +418,7 @@ sub settings_hash ($self, $prefetched = undef) {
             $settings->{$_->key} = $_->value;
         }
     }
-    for my $column (qw(DISTRI VERSION FLAVOR MACHINE ARCH BUILD TEST)) {
+    for my $column (MAIN_SETTINGS) {
         if (my $value = $self->$column) { $settings->{$column} = $value }
     }
     $settings->{NAME} = sprintf '%08d-%s', $self->id, $self->name;

lib/OpenQA/Schema/ResultSet/Jobs.pm

@@ -23,6 +23,7 @@ use Mojolicious::Validator;
 use Mojolicious::Validator::Validation;
 use Time::HiRes 'time';
 use DateTime;
+use List::Util qw(any);
 use Scalar::Util qw(looks_like_number);
 
 =head2 latest_build
@@ -55,20 +56,16 @@ sub latest_build ($self, %args) {
     $attrs{order_by} = {-desc => 'me.id'};    # More reliable for tests than t_created
     $attrs{columns} = qw(BUILD);
 
+    my $job_settings = $schema->resultset('JobSettings');
     foreach my $key (keys %args) {
+        my $key_uc = uc $key;
         my $value = $args{$key};
-
-        if (grep { $key eq $_ } qw(distri version flavor machine arch build test)) {
-            push(@conds, {'me.' . uc($key) => $value});
+        if (any { $key_uc eq $_ } OpenQA::Schema::Result::Jobs::MAIN_SETTINGS) {
+            push @conds, {'me.' . $key_uc => $value};
         }
         else {
-
-            my $subquery = $schema->resultset('JobSettings')->search(
-                {
-                    key => uc($key),
-                    value => $value
-                });
-            push(@conds, {'me.id' => {-in => $subquery->get_column('job_id')->as_query}});
+            my $subquery = $job_settings->search({key => $key_uc, value => $value});
+            push @conds, {'me.id' => {-in => $subquery->get_column('job_id')->as_query}};
         }
     }
 
@@ -97,16 +94,8 @@ sub latest_jobs ($self, $until = undef) {
     my @latest;
     my %seen;
     foreach my $job (@jobs) {
-        my $test = $job->TEST;
-        my $distri = $job->DISTRI;
-        my $version = $job->VERSION;
-        my $build = $job->BUILD;
-        my $flavor = $job->FLAVOR;
-        my $arch = $job->ARCH;
-        my $machine = $job->MACHINE // '';
-        my $key = "$distri-$version-$build-$test-$flavor-$arch-$machine";
-        next if $seen{$key}++;
-        push(@latest, $job);
+        my $key = join('-', map { $job->$_ // '' } OpenQA::Schema::Result::Jobs::MAIN_SETTINGS);
+        push @latest, $job unless $seen{$key}++;
     }
 
     return @latest;
@@ -321,7 +310,7 @@ sub _prepare_complex_query_search_args ($self, $args) {
     else {
         my %js_settings;
         # Check if the settings are between the arguments passed via query url
-        # they come in lowercase, so mace sure $key is lc'ed
+        # they come in lowercase, so make sure $key is lc'ed
         for my $key (qw(ISO HDD_1 WORKER_CLASS)) {
             $js_settings{$key} = $args->{lc $key} if defined $args->{lc $key};
         }
@@ -353,16 +342,53 @@ sub _prepare_complex_query_search_args ($self, $args) {
     return (\@conds, \%attrs);
 }
 
-sub complex_query ($self, %args) {
-    # For args where we accept a list of values, allow passing either an
-    # array ref or a comma-separated list
+sub _accept_comma_separated_arg_values ($args) {
     for my $arg (qw(state ids result modules modules_result)) {
-        next unless $args{$arg};
-        $args{$arg} = [split(',', $args{$arg})] unless (ref($args{$arg}) eq 'ARRAY');
+        next unless my $value = $args->{$arg};
+        $args->{$arg} = [split ',', $value] unless ref $value eq 'ARRAY';
     }
+}
+
+sub complex_query ($self, %args) {
+    _accept_comma_separated_arg_values(\%args);
     my ($conds, $attrs) = $self->_prepare_complex_query_search_args(\%args);
-    my $jobs = $self->search({-and => $conds}, $attrs);
-    return $jobs;
+    return $self->search({-and => $conds}, $attrs);
+}
+
+sub complex_query_latest_ids ($self, %args) {
+    # prepare basic search conditions and attributes
+    _accept_comma_separated_arg_values(\%args);
+    my ($conds, $attrs) = $self->_prepare_complex_query_search_args(\%args);
+    my $filters = $args{filters};
+    my $has_filters = $filters && @$filters > 0;
+    my $rows = $has_filters ? delete $attrs->{rows} : undef;  # when filtering, limit rows only in outer/filtering query
+    if (my $until = $args{until}) { push @$conds, {'me.t_created' => {'<=' => $until}} }
+
+    # set attributes to return only the latest job IDs for a certain combination of TEST, DISTRI, VERSION, …
+    $attrs->{order_by} = \['max(me.id) DESC'];
+    $attrs->{select} = ['max(me.id)'];
+    $attrs->{as} = ['id'];
+    $attrs->{group_by} = [OpenQA::Schema::Result::Jobs::MAIN_SETTINGS];
+
+    # execute the search; use a sub query if filtering is enabled
+    my $search = $self->search({-and => $conds}, $attrs);
+    if ($has_filters) {
+        # add another layer of querying for filters
+        # note: The filtering cannot be applied in the same query we do the grouping to return only the latest job IDs.
+        #       Otherwise adding filter parameters would lead to old jobs showing up. That is not wanted (and we have
+        #       therefore the test "filtering does not reveal old jobs" in `10-tests_overview.t` to test this).
+        my %filter_attrs = %$attrs;
+        $filter_attrs{rows} = $rows;
+        push @$filters, {'me.id' => {-in => $search->as_query}};
+        $search = $self->search({-and => $filters}, \%filter_attrs);
+    }
+    return [map { $_->id } $search->all];
+}
+
+sub latest_jobs_from_ids ($self, $latest_job_ids, $limit_from_initial_search) {
+    my %search_args = (id => {-in => $latest_job_ids});
+    my %options = (order_by => {-desc => 'id'}, rows => $limit_from_initial_search - 1);
+    return $self->search(\%search_args, \%options);
 }
 
 sub cancel_by_settings (
@@ -380,7 +406,7 @@ sub cancel_by_settings (
     my %precond = %{$settings};
     my %cond;
 
-    for my $key (qw(DISTRI VERSION FLAVOR MACHINE ARCH BUILD TEST)) {
+    for my $key (OpenQA::Schema::Result::Jobs::MAIN_SETTINGS) {
         $cond{$key} = delete $precond{$key} if defined $precond{$key};
     }
     if (keys %precond) {

lib/OpenQA/WebAPI/Controller/API/V1/Job.pm

@@ -226,29 +226,15 @@ So this works in the same way as the test results overview in the GUI.
 sub overview ($self) {
     my ($search_args, $groups) = $self->compose_job_overview_search_args;
     my $failed_modules = $self->param_hash('failed_modules');
-    my $states = $self->param_hash('state');
-    my $results = $self->param_hash('result');
-    my $archs = $self->param_hash('arch');
-    my $machines = $self->param_hash('machine');
-
-    my @jobs = $self->schema->resultset('Jobs')->complex_query(%$search_args)->latest_jobs;
+    my $jobs_rs = $self->schema->resultset('Jobs');
+    my $latest_job_ids = $jobs_rs->complex_query_latest_ids(%$search_args);
+    my @jobs = $jobs_rs->latest_jobs_from_ids($latest_job_ids, $search_args->{limit})->all;
 
     my @job_hashes;
     for my $job (@jobs) {
-        next if $states && !$states->{$job->state};
-        next if $results && !$results->{$job->result};
-        next if $archs && !$archs->{$job->ARCH};
-        next if $machines && !$machines->{$job->MACHINE};
-        if ($failed_modules) {
-            next if $job->result ne OpenQA::Jobs::Constants::FAILED;
-            next unless OpenQA::Utils::any_array_item_contained_by_hash($job->failed_modules, $failed_modules);
-        }
-        push(
-            @job_hashes,
-            {
-                id => $job->id,
-                name => $job->name,
-            });
+        next
+          if $failed_modules && !OpenQA::Utils::any_array_item_contained_by_hash($job->failed_modules, $failed_modules);
+        push @job_hashes, {id => $job->id, name => $job->name};
     }
     $self->render(json => \@job_hashes);
 }

lib/OpenQA/WebAPI/Controller/Test.pm

@@ -685,10 +685,9 @@ sub _calculate_preferred_machines ($jobs) {
 }
 
 # Take an job objects arrayref and prepare data structures for 'overview'
-sub _prepare_job_results ($self, $all_jobs, $limit) {
+sub _prepare_job_results ($self, $jobs, $job_ids) {
     my %archs;
     my %results;
-    my @job_ids;
     my $aggregated = {
         none => 0,
         passed => 0,
@@ -699,25 +698,8 @@ sub _prepare_job_results ($self, $all_jobs, $limit) {
         running => 0,
         unknown => 0
     };
-    my $preferred_machines = _calculate_preferred_machines($all_jobs);
-
-    # read parameter for additional filtering
-    my $failed_modules = $self->param_hash('failed_modules');
-    my $states = $self->param_hash('state');
-    my $results = $self->param_hash('result');
-    my $archs = $self->param_hash('arch');
-    my $machines = $self->param_hash('machine');
-
-    my @jobs = grep {
-              (not $states or $states->{$_->state})
-          and (not $results or $results->{$_->result})
-          and (not $archs or $archs->{$_->ARCH})
-          and (not $machines or $machines->{$_->MACHINE})
-          and (not $failed_modules or $_->result eq OpenQA::Jobs::Constants::FAILED)
-    } @$all_jobs;
-    my $limit_exceeded = @jobs >= $limit;
-    @jobs = @jobs[0 .. ($limit - 1)] if $limit_exceeded;
-    my @jobids = map { $_->id } @jobs;
+    my @jobs = $jobs->all;
+    my $preferred_machines = _calculate_preferred_machines(\@jobs);
 
     # prefetch the number of available labels for those jobs
     my $schema = $self->schema;
@@ -726,7 +708,7 @@ sub _prepare_job_results ($self, $all_jobs, $limit) {
     # prefetch test suite names from job settings
     my $job_settings
       = $schema->resultset('JobSettings')
-      ->search({job_id => {-in => [map { $_->id } @jobs]}, key => {-in => [qw(JOB_DESCRIPTION TEST_SUITE_NAME)]}});
+      ->search({job_id => {-in => $job_ids}, key => {-in => [qw(JOB_DESCRIPTION TEST_SUITE_NAME)]}});
     my %settings_by_job_id;
     for my $js ($job_settings->all) {
         $settings_by_job_id{$js->job_id}->{$js->key} = $js->value;
@@ -740,12 +722,13 @@ sub _prepare_job_results ($self, $all_jobs, $limit) {
       = $schema->resultset('TestSuites')->search({name => \%desc_args}, {columns => [qw(name description)]});
     my %descriptions = map { $_->name => $_->description } @descriptions;
 
-    my $failed_modules_by_job = $self->_fetch_failed_modules_by_jobs(\@jobids);
-    my ($children_by_job, $parents_by_job) = $self->_fetch_dependencies_by_jobs(\@jobids);
+    my $failed_modules_by_job = $self->_fetch_failed_modules_by_jobs($job_ids);
+    my ($children_by_job, $parents_by_job) = $self->_fetch_dependencies_by_jobs($job_ids);
     foreach my $job (@jobs) {
         my $id = $job->id;
         my $result = $job->overview_result(
-            $comment_data, $aggregated, $failed_modules,
+            $comment_data, $aggregated,
+            $self->param_hash('failed_modules'),
             $failed_modules_by_job->{$id} || [],
             $self->param('todo')) or next;
         my $test = $job->TEST;
@@ -780,14 +763,11 @@ sub _prepare_job_results ($self, $all_jobs, $limit) {
         $results{$distri}{$version}{$flavor}{$test} //= {};
         $results{$distri}{$version}{$flavor}{$test}{$arch} = $result;
 
-        # populate job IDs for adding multiple comments
-        push @job_ids, $id;
-
         # add description
         my $description = $settings_by_job_id{$id}->{JOB_DESCRIPTION} // $descriptions{$test_suite_names{$id}};
         $results{$distri}{$version}{$flavor}{$test}{description} //= $description;
     }
-    return ($limit_exceeded, \%archs, \%results, \@job_ids, $aggregated);
+    return (\%archs, \%results, $aggregated);
 }
 
 sub _prepare_groupids ($self) {
@@ -857,32 +837,26 @@ sub overview ($self) {
     my $validation = $self->validation;
     $validation->optional('t')->datetime;
     my $until = $validation->param('t');
-    my %stash = (
-        # build, version, distri are not mandatory and therefore not
-        # necessarily come from the search args so they can be undefined.
-        build => ref $search_args->{build} eq 'ARRAY' ? join(',', @{$search_args->{build}}) : $search_args->{build},
-        version => $search_args->{version},
-        distri => $search_args->{distri},
-        groups => $groups,
-        until => $until,
-        parallel_children_collapsable_results_sel => $config->{global}->{parallel_children_collapsable_results_sel},
-    );
-    my @jobs = $self->schema->resultset('Jobs')->complex_query(%$search_args)->latest_jobs($until);
-
-    my $limit = $config->{misc_limits}->{tests_overview_max_jobs};
-    (my $limit_exceeded, $stash{archs}, $stash{results}, $stash{job_ids}, $stash{aggregated})
-      = $self->_prepare_job_results(\@jobs, $limit);
+    $search_args->{until} = $until;
+    my $distri = $search_args->{distri};
+    my $version = $search_args->{version};
+    my $jobs_rs = $self->schema->resultset('Jobs');
+    my $latest_job_ids = $jobs_rs->complex_query_latest_ids(%$search_args);
+    my $limit = $search_args->{limit};    # one more than actual limit so we can check whether the limit was exceeded
+    my $exceeded_limit = @$latest_job_ids >= $limit ? $limit - 1 : 0;
+    my $jobs = $jobs_rs->latest_jobs_from_ids($latest_job_ids, $limit);
+    my ($archs, $results, $aggregated) = $self->_prepare_job_results($jobs, $latest_job_ids);
 
     # determine distri/version from job results if not explicitly specified via search args
-    my @distris = keys %{$stash{results}};
+    my @distris = keys %$results;
     my $formatted_distri;
     my $formatted_version;
     my $only_distri = scalar @distris == 1;
-    if (!defined $stash{distri} && $only_distri) {
+    if (!defined $distri && $only_distri) {
         $formatted_distri = $distris[0];
-        if (!defined $stash{version}) {
-            my @versions = keys %{$stash{results}->{$formatted_distri}};
-            $formatted_version = $versions[0] if (scalar @versions == 1);
+        if (!defined $version) {
+            my @versions = keys %{$results->{$formatted_distri}};
+            $formatted_version = $versions[0] if scalar @versions == 1;
         }
     }
 
@@ -899,18 +873,28 @@ sub overview ($self) {
         _add_distri_and_version_to_summary(\@summary_parts, $formatted_distri, $formatted_version, 1);
 
         # add distri and version from query parameters as regular strings
-        _add_distri_and_version_to_summary(\@summary_parts, $stash{distri}, $stash{version}, 0);
+        _add_distri_and_version_to_summary(\@summary_parts, $distri, $version, 0);
     }
 
-    $self->stash(
-        %stash,
+    my %stash = (
+        # build, version, distri are not mandatory and therefore not
+        # necessarily come from the search args so they can be undefined.
+        build => ref $search_args->{build} eq 'ARRAY' ? join(',', @{$search_args->{build}}) : $search_args->{build},
+        distri => $distri,
+        version => $version,
+        groups => $groups,
+        archs => $archs,
+        results => $results,
+        aggregated => $aggregated,
+        job_ids => $latest_job_ids,
+        until => $until,
+        parallel_children_collapsable_results_sel => $config->{global}->{parallel_children_collapsable_results_sel},
         summary_parts => \@summary_parts,
         only_distri => $only_distri,
-        limit_exceeded => $limit_exceeded ? $limit : undef
+        limit_exceeded => $exceeded_limit,
     );
-    $self->respond_to(
-        json => {json => \%stash},
-        html => {template => 'test/overview'});
+    $self->stash(\%stash);
+    $self->respond_to(json => {json => \%stash}, html => {template => 'test/overview'});
 }
 
 sub _get_latest_job ($self) {