Merge pull request #5146 from Martchus/build-tagging

Consider a tag's version when cleaning up job results
os-autoinst · May 24, 2023 · 13deaff · 13deaff
2 parents 6dc41fa + 1b6b050
commit 13deaff
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 60 deletions.
diff --git a/lib/OpenQA/Jobs/Constants.pm b/lib/OpenQA/Jobs/Constants.pm
@@ -99,6 +99,9 @@ use constant
 # defaults for new jobs that are useful outside the schema
 use constant DEFAULT_JOB_PRIORITY => 50;
 
+# the "column" to query for a tag ID in accordance with parse_tags_from_comments() and _important_builds()
+use constant TAG_ID_COLUMN => "concat(VERSION, '-', BUILD)";
+
 our @EXPORT = qw(
   ASSIGNED
   CANCELLED
@@ -136,6 +139,7 @@ our @EXPORT = qw(
   TIMEOUT_EXCEEDED
   DEFAULT_JOB_PRIORITY
   RESULT_CLEANUP_LOG_FILES
+  TAG_ID_COLUMN
 );
 
 # mapping from any specific job state/result to a meta state/result

diff --git a/lib/OpenQA/Schema/Result/JobGroups.pm b/lib/OpenQA/Schema/Result/JobGroups.pm
@@ -7,6 +7,7 @@ use Mojo::Base 'DBIx::Class::Core', -signatures;
 use OpenQA::App;
 use OpenQA::Markdown 'markdown_to_html';
 use OpenQA::JobGroupDefaults;
+use OpenQA::Jobs::Constants;
 use Class::Method::Modifiers;
 use OpenQA::Log qw(log_debug);
 use OpenQA::Utils qw(parse_tags_from_comments);
@@ -174,7 +175,9 @@ sub matches_nested {
 }
 
 # check the group comments for important builds
-sub _important_builds ($self) {
+sub important_builds ($self) {
+    if (my $cached = $self->{_important_builds}) { return $cached }
+
     # determine relevant comments including those on the parent-level
     # note: Assigning to scalar first because ->comments would return all results at once when
     #       called in an array-context.
@@ -186,27 +189,25 @@ sub _important_builds ($self) {
     }
 
     # look for "important" tags in the comments
-    my %importants;
+    my (%with_version, %without_version);
     for my $comments (@comments) {
         while (my $comment = $comments->next) {
-            my @tag = $comment->tag;
-            next unless $tag[0];
-            if ($tag[1] eq 'important') {
-                $importants{$tag[0]} = 1;
+            my ($build, $type, $desc, $version) = $comment->tag;
+            next unless $build;
+            my $tag_id = $version ? "$version-$build" : $build;
+            my $res = $version ? \%with_version : \%without_version;
+            if ($type eq 'important') {
+                $res->{$tag_id} = 1;
             }
-            elsif ($tag[1] eq '-important') {
-                delete $importants{$tag[0]};
+            elsif ($type eq '-important') {
+                delete $res->{$tag_id};
             }
         }
     }
-    return \%importants;
+    return $self->{_important_builds} = [[sort keys %with_version], [sort keys %without_version]];
 }
 
-sub important_builds ($self) { [sort keys %{$self->_important_builds}] }
-
-sub _find_expired_jobs ($self, $important_builds, $keep_in_days, $keep_important_in_days,
-    $preserved_important_jobs_out = undef)
-{
+sub _find_expired_jobs ($self, $keep_in_days, $keep_important_in_days, $preserved_important_jobs_out = undef) {
     return undef unless $keep_in_days;    # 0 means forever
 
     my $now = time;
@@ -216,10 +217,14 @@ sub _find_expired_jobs ($self, $important_builds, $keep_in_days, $keep_important
     # note: As we use this function also for the homeless group (with id=null), we can't use $self->jobs, but
     #       need to add it directly.
     my $jobs = $self->result_source->schema->resultset('Jobs');
+    my ($important_builds_with_version, $important_builds_without_version) = @{$self->important_builds};
     my @group_cond = ('me.group_id' => $self->id);
+    my @not_important_cond = (
+        TAG_ID_COLUMN, => {-not_in => $important_builds_with_version},
+        BUILD => {-not_in => $important_builds_without_version});
     my $expired_jobs = $jobs->search(
         {
-            BUILD => {-not_in => $important_builds},
+            @not_important_cond,
             text => {like => 'label:linked%'},
             t_finished => $timecond,
             @group_cond,
@@ -228,13 +233,17 @@ sub _find_expired_jobs ($self, $important_builds, $keep_in_days, $keep_important
     my @linked_jobs = map { $_->id } $expired_jobs->all;
 
     # define condition for expired jobs in unimportant builds
-    my @ors = ({BUILD => {-not_in => $important_builds}, t_finished => $timecond, id => {-not_in => \@linked_jobs}});
+    my @ors = ({@not_important_cond, t_finished => $timecond, id => {-not_in => \@linked_jobs}});
 
     # define condition for expired jobs in important builds
     my ($important_timestamp, @important_cond);
     if ($keep_important_in_days && $keep_important_in_days > $keep_in_days) {
         $important_timestamp = time2str('%Y-%m-%d %H:%M:%S', $now - ONE_DAY * $keep_important_in_days, 'UTC');
-        @important_cond = (-or => [{BUILD => {-in => $important_builds}}, {id => {-in => \@linked_jobs}}]);
+        @important_cond = (
+            -or => [
+                TAG_ID_COLUMN, => {-in => $important_builds_with_version},
+                BUILD => {-in => $important_builds_without_version},
+                id => {-in => \@linked_jobs}]);
         push @ors, {@important_cond, t_finished => {'<' => $important_timestamp}};
     }
 
@@ -249,31 +258,25 @@ sub _find_expired_jobs ($self, $important_builds, $keep_in_days, $keep_important
     return $jobs->search({-and => {@group_cond, -or => \@ors}}, {order_by => qw(id)});
 }
 
-sub find_jobs_with_expired_results ($self, $important_builds = undef) {
-    $important_builds //= $self->important_builds;
-    my $expired = $self->_find_expired_jobs($important_builds, $self->keep_results_in_days,
-        $self->keep_important_results_in_days);
+sub find_jobs_with_expired_results ($self) {
+    my $expired = $self->_find_expired_jobs($self->keep_results_in_days, $self->keep_important_results_in_days);
     return $expired ? [$expired->all] : [];
 }
 
-sub find_jobs_with_expired_logs ($self, $important_builds = undef, $preserved_important_jobs_out = undef) {
-    $important_builds //= $self->important_builds;
-    my $expired
-      = $self->_find_expired_jobs($important_builds, $self->keep_logs_in_days, $self->keep_important_logs_in_days,
+sub find_jobs_with_expired_logs ($self, $preserved_important_jobs_out = undef) {
+    my $expired = $self->_find_expired_jobs($self->keep_logs_in_days, $self->keep_important_logs_in_days,
         $preserved_important_jobs_out);
     return $expired ? [$expired->search({logs_present => 1})->all] : [];
 }
 
 # helper function for cleanup task
 sub limit_results_and_logs ($self, $preserved_important_jobs_out = undef) {
-    my $important_builds_hash = $self->_important_builds;
-    my @important_builds = keys %$important_builds_hash;
-    my $expired_jobs = $self->find_jobs_with_expired_results(\@important_builds);
+    my $expired_jobs = $self->find_jobs_with_expired_results;
     $_->delete for @$expired_jobs;
 
     my $config = OpenQA::App->singleton->config;
     my $preserved = $config->{archiving}->{archive_preserved_important_jobs} ? $preserved_important_jobs_out : undef;
-    my $jobs_with_expired_logs = $self->find_jobs_with_expired_logs(\@important_builds, $preserved);
+    my $jobs_with_expired_logs = $self->find_jobs_with_expired_logs($preserved);
     $_->delete_logs for @$jobs_with_expired_logs;
 }
 

diff --git a/lib/OpenQA/Task/Job/Limit.pm b/lib/OpenQA/Task/Job/Limit.pm
@@ -4,6 +4,7 @@
 package OpenQA::Task::Job::Limit;
 use Mojo::Base 'Mojolicious::Plugin', -signatures;
 
+use OpenQA::Jobs::Constants;
 use OpenQA::Log 'log_debug';
 use OpenQA::ScreenshotDeletion;
 use OpenQA::Utils qw(:DEFAULT resultdir check_df);
@@ -193,12 +194,24 @@ sub _ensure_results_below_threshold ($job, @) {
 
     # determine important builds (for each group)
     my $job_groups = $schema->resultset('JobGroups');
-    my %important_builds_hash;
+    my %important_builds_with_version;
+    my %important_builds_without_version;
     for my $job_group ($job_groups->all) {
-        $important_builds_hash{$_} = 1 for @{$job_group->important_builds};
+        my ($important_builds_with_version, $important_builds_without_version) = @{$job_group->important_builds};
+        $important_builds_with_version{$_} = 1 for @$important_builds_with_version;
+        $important_builds_without_version{$_} = 1 for @$important_builds_without_version;
     }
-    my @important_builds = keys %important_builds_hash;
-    $job->note(important_builds => \@important_builds);
+    my @important_builds_with_version = keys %important_builds_with_version;
+    my @important_builds_without_version = keys %important_builds_without_version;
+    my @important_cond = (
+        -or => [
+            TAG_ID_COLUMN, => {-in => \@important_builds_with_version},
+            BUILD => {-in => \@important_builds_without_version}]);
+    my @not_important_cond = (
+        TAG_ID_COLUMN, => {-not_in => \@important_builds_with_version},
+        BUILD => {-not_in => \@important_builds_without_version});
+    $job->note(important_builds_with_version => \@important_builds_with_version);
+    $job->note(important_builds_without_version => \@important_builds_without_version);
 
     # caveat: The subsequent cleanup simply deletes stuff from old jobs first. It does not take the retention periods
     #         configured on job group level into account anymore.
@@ -209,33 +222,31 @@ sub _ensure_results_below_threshold ($job, @) {
     my $jobs = $schema->resultset('Jobs');
     my @job_id_args = (id => {'<=' => $max_job_id});
     my %jobs_params = (order_by => {-asc => 'id'});
-    my $relevant_jobs
-      = $jobs->search({@job_id_args, logs_present => 1, BUILD => {-not_in => \@important_builds}}, \%jobs_params);
+    my $relevant_jobs = $jobs->search({@job_id_args, @not_important_cond, logs_present => 1}, \%jobs_params);
     while (my $openqa_job = $relevant_jobs->next) {
         log_debug 'Deleting video of job ' . $openqa_job->id;
         return $job->finish('Done after deleting videos from non-important jobs')
           if ($margin_bytes += $openqa_job->delete_videos) >= 0;
     }
 
     log_debug "Deleting results from non-important jobs startinng from oldest job (balance is $margin_bytes)";
-    $relevant_jobs = $jobs->search({@job_id_args, BUILD => {-not_in => \@important_builds}}, \%jobs_params);
+    $relevant_jobs = $jobs->search({@job_id_args, @not_important_cond}, \%jobs_params);
     while (my $openqa_job = $relevant_jobs->next) {
         log_debug 'Deleting results of job ' . $openqa_job->id;
         return $job->finish('Done after deleting results from non-important jobs')
           if ($margin_bytes += $openqa_job->delete_results) >= 0;
     }
 
     log_debug "Deleting videos from important jobs startinng from oldest job (balance is $margin_bytes)";
-    $relevant_jobs
-      = $jobs->search({@job_id_args, logs_present => 1, BUILD => {-in => \@important_builds}}, \%jobs_params);
+    $relevant_jobs = $jobs->search({@job_id_args, @important_cond, logs_present => 1}, \%jobs_params);
     while (my $openqa_job = $relevant_jobs->next) {
         log_debug 'Deleting video of important job ' . $openqa_job->id;
         return $job->finish('Done after deleting videos from important jobs')
           if ($margin_bytes += $openqa_job->delete_videos) >= 0;
     }
 
     log_debug "Deleting results from important jobs startinng from oldest job (balance is $margin_bytes)";
-    $relevant_jobs = $jobs->search({@job_id_args, BUILD => {-in => \@important_builds}}, \%jobs_params);
+    $relevant_jobs = $jobs->search({@job_id_args, @important_cond}, \%jobs_params);
     while (my $openqa_job = $relevant_jobs->next) {
         log_debug 'Deleting results of important job ' . $openqa_job->id;
         return $job->finish('Done after deleting results from important jobs')

diff --git a/t/17-build_tagging.t b/t/17-build_tagging.t
@@ -40,12 +40,7 @@ sub post_comment_1001 ($comment) {
 }
 
 sub post_parent_group_comment ($parent_group_id, $comment) {
-    return $comments->create(
-        {
-            parent_group_id => $parent_group_id,
-            user_id => 1,
-            text => $comment
-        });
+    return $comments->create({parent_group_id => $parent_group_id, user_id => 1, text => $comment});
 }
 
 # this and 'create_job_version_build' are for adding jobs on the fly,
@@ -170,29 +165,21 @@ subtest 'test tags for Fedora update-style BUILD values' => sub {
 
 sub query_important_builds {
     my %important_builds_by_group = (0 => $job_groups->new({})->important_builds);
-    my $job_groups = $schema->resultset('JobGroups');
-    while (my $job_group = $job_groups->next) {
-        $important_builds_by_group{$job_group->id} = $job_group->important_builds;
-    }
+    $important_builds_by_group{$_->id} = $_->important_builds for $job_groups->all;
     return \%important_builds_by_group;
 }
 
 subtest 'tagging builds via parent group comments' => sub {
     my %expected_important_builds = (
-        0 => [],
-        1001 => [qw(0048 0066 20170329.n.0 3456ba4c93)],
-        1002 => [],
+        0 => [[], []],
+        1001 => [[qw(FEDORA-2017-3456ba4c93 Fedora-26-20170329.n.0)], [qw(0048 0066)]],
+        1002 => [[], []],
     );
 
     # create a parent group and move all job groups into it
     my $parent_group = $parent_groups->create({name => 'Test parent', sort_order => 0});
     my $parent_group_id = $parent_group->id;
-    while (my $job_group = $job_groups->next) {
-        $job_group->update(
-            {
-                parent_id => $parent_group->id
-            });
-    }
+    $_->update({parent_id => $parent_group->id}) for $job_groups->all;
 
     # create job with DISTRI=Arch, VERSION=2018 and BUILD=08
     create_job_version_build('1', 'Arch-2018-08');
@@ -218,12 +205,34 @@ subtest 'tagging builds via parent group comments' => sub {
     is(scalar @tags, 4, 'four builds tagged');
 
     # check whether the build is considered important now
-    $expected_important_builds{1001} = [qw(0048 0066 08 20170329.n.0 3456ba4c93)];
-    $expected_important_builds{1002} = [qw(08)];
+    $expected_important_builds{1001}
+      = [[qw(Arch-2018-08 FEDORA-2017-3456ba4c93 Fedora-26-20170329.n.0)], [qw(0048 0066)]];
+    $expected_important_builds{1002} = [[qw(Arch-2018-08)], []];
     $important_builds = query_important_builds;
     is_deeply($important_builds, \%expected_important_builds, 'tag on parent level marks build as important')
       or diag explain $important_builds;
 
+    $schema->txn_begin;
+    subtest 'the version of "important" tags is considered when determining expired jobs' => sub {
+        # create tag without version constraint, setup group's retention
+        post_comment_1001 'tag:0066:important:foo';
+        my $group = $job_groups->find(1001);
+        $group->keep_results_in_days(1);
+        $group->keep_important_results_in_days(0);
+        # create job to be expired despite "tag:Arch-2018-08:important:fromparent"; version specified but does not match
+        my @common_args = (BUILD => '08', t_finished => '0001-01-01 00:00:00', group_id => $group->id);
+        my $expired_job = $jobs->create({TEST => 'expired-1', VERSION => 'Arch-2019', @common_args});
+        # create job to be preserved via "tag:0066:important:foo"; no version specified but build matches
+        $jobs->create({TEST => 'preservable-1', VERSION => 'Arch-2018', @common_args, BUILD => '0066'});
+        # create job to be preserved via "tag:Arch-2018-08:important:fromparent"; version specified and matches
+        $jobs->create({TEST => 'preservable-2', VERSION => 'Arch-2018', @common_args});
+        # determine and check expired jobs
+        my @expired_jobs = sort map { $_->id } @{$group->find_jobs_with_expired_results};
+        is_deeply \@expired_jobs, [$expired_job->id], 'only job with mismatching VERSION is expired'
+          or diag explain \@expired_jobs;
+    };
+    $schema->txn_rollback;
+
     # create a tag for the same build on child level
     post_comment_1001('tag:Arch-2018-08:important:fromchild');