Merge pull request #4136 from Martchus/fix-eary-asset-cleanup

Prevent assets from being cleaned up too early
os-autoinst · Aug 26, 2021 · 5ffe7a2 · 5ffe7a2
2 parents de4dd07 + 98e9f63
commit 5ffe7a2
Show file tree

Hide file tree

Showing 20 changed files with 257 additions and 207 deletions.
diff --git a/cpanfile b/cpanfile
@@ -9,7 +9,7 @@ requires 'BSD::Resource';
 requires 'CSS::Minifier::XS', '>= 0.01';
 requires 'Capture::Tiny';
 requires 'Carp';
-requires 'Carp::Always';
+requires 'Carp::Always', '>= 0.14.02';
 requires 'CommonMark';
 requires 'Config::IniFiles';
 requires 'Config::Tiny';

diff --git a/dependencies.yaml b/dependencies.yaml
@@ -29,7 +29,7 @@ build_requires:
 common_requires:
   perl: '>= 5.20.0'
   perl(Archive::Extract): '> 0.7'
-  perl(Carp::Always):
+  perl(Carp::Always): '>= 0.14.02'
   perl(Config::IniFiles):
   perl(Cpanel::JSON::XS): '>= 4.09'
   perl(Cwd):

diff --git a/dist/rpm/openQA.spec b/dist/rpm/openQA.spec
@@ -49,7 +49,7 @@
 # The following line is generated from dependencies.yaml
 %define assetpack_requires perl(CSS::Minifier::XS) >= 0.01 perl(JavaScript::Minifier::XS) >= 0.11 perl(Mojolicious::Plugin::AssetPack) >= 1.36
 # The following line is generated from dependencies.yaml
-%define common_requires perl >= 5.20.0 perl(Archive::Extract) > 0.7 perl(Carp::Always) perl(Config::IniFiles) perl(Cpanel::JSON::XS) >= 4.09 perl(Cwd) perl(Data::Dump) perl(Data::Dumper) perl(Digest::MD5) perl(Filesys::Df) perl(Getopt::Long) perl(Minion) >= 10.22 perl(Mojolicious) >= 9.11 perl(Regexp::Common) perl(Storable) perl(Try::Tiny)
+%define common_requires perl >= 5.20.0 perl(Archive::Extract) > 0.7 perl(Carp::Always) >= 0.14.02 perl(Config::IniFiles) perl(Cpanel::JSON::XS) >= 4.09 perl(Cwd) perl(Data::Dump) perl(Data::Dumper) perl(Digest::MD5) perl(Filesys::Df) perl(Getopt::Long) perl(Minion) >= 10.22 perl(Mojolicious) >= 9.11 perl(Regexp::Common) perl(Storable) perl(Try::Tiny)
 # runtime requirements for the main package that are not required by other sub-packages
 # The following line is generated from dependencies.yaml
 %define main_requires %assetpack_requires git-core hostname perl(BSD::Resource) perl(Carp) perl(CommonMark) perl(Config::Tiny) perl(DBD::Pg) >= 3.7.4 perl(DBI) >= 1.632 perl(DBIx::Class) >= 0.082801 perl(DBIx::Class::DeploymentHandler) perl(DBIx::Class::DynamicDefault) perl(DBIx::Class::OptimisticLocking) perl(DBIx::Class::ResultClass::HashRefInflator) perl(DBIx::Class::Schema::Config) perl(DBIx::Class::Storage::Statistics) perl(Date::Format) perl(DateTime) perl(DateTime::Duration) perl(DateTime::Format::Pg) perl(Exporter) perl(Fcntl) perl(File::Basename) perl(File::Copy) perl(File::Copy::Recursive) perl(File::Path) perl(File::Spec) perl(FindBin) perl(Getopt::Long::Descriptive) perl(IO::Handle) perl(IPC::Run) perl(JSON::Validator) perl(LWP::UserAgent) perl(Module::Load::Conditional) perl(Module::Pluggable) perl(Mojo::Base) perl(Mojo::ByteStream) perl(Mojo::IOLoop) perl(Mojo::JSON) perl(Mojo::Pg) perl(Mojo::RabbitMQ::Client) >= 0.2 perl(Mojo::URL) perl(Mojo::Util) perl(Mojolicious::Commands) perl(Mojolicious::Plugin) perl(Mojolicious::Static) perl(Net::OpenID::Consumer) perl(POSIX) perl(Pod::POM) perl(SQL::Translator) perl(Scalar::Util) perl(Sort::Versions) perl(Text::Diff) perl(Time::HiRes) perl(Time::ParseDate) perl(Time::Piece) perl(Time::Seconds) perl(URI::Escape) perl(YAML::PP) >= 0.026 perl(YAML::XS) perl(aliased) perl(base) perl(constant) perl(diagnostics) perl(strict) perl(warnings)

diff --git a/lib/OpenQA/Schema/Result/Assets.pm b/lib/OpenQA/Schema/Result/Assets.pm
@@ -1,4 +1,4 @@
-# Copyright (C) 2014-2019 SUSE LLC
+# Copyright (C) 2014-2021 SUSE LLC
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -47,9 +47,9 @@ __PACKAGE__->add_columns(
     name => {
         data_type => 'text',
     },
-    size => {
+    size => {    # initialized when registering assets from job settings, refreshed when scanning assets
         data_type   => 'bigint',
-        is_nullable => 1
+        is_nullable => 1           # is null for assets which do not exist
     },
     checksum => {
         data_type     => 'text',
@@ -140,15 +140,15 @@ sub delete {
 sub ensure_size {
     my ($self) = @_;
     my $size = $self->size;
-    return $size if (defined($size));
+    return $size if defined $size;
     return $self->refresh_size($size);
 }
 
 sub refresh_size {
     my ($self, $current_size) = @_;
     $current_size //= $self->size;
 
-    my $new_size = 0;
+    my $new_size = undef;
     my @stat     = stat(my $disk_file = $self->disk_file);
     if (@stat) {
         if ($self->type eq 'repo') {
@@ -159,9 +159,8 @@ sub refresh_size {
             $new_size = $stat[7];
         }
     }
-    if (!defined($current_size) || $current_size != $new_size) {
-        $self->update({size => $new_size});
-    }
+    $self->update({size => $new_size})
+      if (!defined $new_size ^ !defined $current_size) || ($current_size // 0) != ($new_size // 0);
     return $new_size;
 }
 

diff --git a/lib/OpenQA/Schema/Result/Jobs.pm b/lib/OpenQA/Schema/Result/Jobs.pm
@@ -579,24 +579,25 @@ sub _strip_parent_job_id {
 sub missing_assets {
     my ($self) = @_;
 
-    my $assets = parse_assets_from_settings($self->settings_hash);
-    return [] unless keys %$assets;
-
+    my $assets_settings = parse_assets_from_settings($self->settings_hash);
+    return [] unless keys %$assets_settings;
 
     # ignore UEFI_PFLASH_VARS; to keep scheduling simple it is present in lots of jobs which actually don't need it
-    delete $assets->{UEFI_PFLASH_VARS};
+    delete $assets_settings->{UEFI_PFLASH_VARS};
 
     my $parent_job_ids = $self->_parent_job_ids;
     # ignore repos, as they're not really clonable: see
     # https://github.com/os-autoinst/openQA/pull/2676#issuecomment-616312026
     my @relevant_assets
-      = grep { $_->{name} ne '' && $_->{type} ne 'repo' } values %$assets;
+      = grep { $_->{name} ne '' && $_->{type} ne 'repo' } values %$assets_settings;
     my @assets_query = map {
         {
             type => $_->{type},
-            name => {-in => _compute_asset_names_considering_parent_jobs($parent_job_ids, $_->{name})}}
+            name => {-in => _compute_asset_names_considering_parent_jobs($parent_job_ids, $_->{name})},
+        }
     } @relevant_assets;
-    my @existing_assets = $self->result_source->schema->resultset('Assets')->search(\@assets_query);
+    my $assets          = $self->result_source->schema->resultset('Assets');
+    my @existing_assets = $assets->search({-or => \@assets_query, size => \'is not null'});
     return [] if scalar @$parent_job_ids == 0 && scalar @assets_query == scalar @existing_assets;
     my %missing_assets = map { ("$_->{type}/$_->{name}" => 1) } @relevant_assets;
     delete $missing_assets{$_->type . '/' . _strip_parent_job_id($parent_job_ids, $_->name)} for @existing_assets;
@@ -1572,10 +1573,10 @@ sub update_status {
     return $ret;
 }
 
-sub _parent_job_ids {
-    my ($self)    = @_;
-    my %condition = (dependency => {-in => [OpenQA::JobDependencies::Constants::CHAINED_DEPENDENCIES]});
-    my @parents   = $self->parents->search(\%condition, {columns => ['parent_job_id']});
+my %CHAINED_DEPENDENCY_QUERY = (dependency => {-in => [OpenQA::JobDependencies::Constants::CHAINED_DEPENDENCIES]});
+
+sub _parent_job_ids ($self) {
+    my @parents = $self->parents->search(\%CHAINED_DEPENDENCY_QUERY, {columns => ['parent_job_id']});
     return [map { $_->parent_job_id } @parents];
 }
 
@@ -1593,6 +1594,7 @@ sub register_assets_from_settings {
     my %updated;
 
     # check assets and fix the file names
+    my $assets = $self->result_source->schema->resultset('Assets');
     for my $k (keys %assets) {
         my $asset = $assets{$k};
         my ($name, $type) = ($asset->{name}, $asset->{type});
@@ -1606,26 +1608,34 @@ sub register_assets_from_settings {
             delete $assets{$k};
             next;
         }
-        my $f_asset = _asset_find($name, $type, $parent_job_ids);
-        unless (defined $f_asset) {
-            # don't register asset not yet available
-            delete $assets{$k};
-            next;
+        my $existing_asset = _asset_find($name, $type, $parent_job_ids);
+        if (defined $existing_asset && $existing_asset ne $name) {
+            # remove possibly previously registered asset
+            # note: This is expected to happen if an asset turns out to be a private asset.
+            $assets->untie_asset_from_job_and_unregister_if_unused($type, $name, $self);
+            $name = $asset->{name} = $existing_asset;
         }
-        $asset->{name} = $f_asset;
-        $updated{$k} = $f_asset;
+        $updated{$k} = $name;
     }
 
-    for my $asset (values %assets) {
+    for my $asset_info (values %assets) {
         # avoid plain create or we will get unique constraint problems
         # in case ISO_1 and ISO_2 point to the same ISO
-        my $aid = $self->result_source->schema->resultset('Assets')->find_or_create($asset);
-        $self->jobs_assets->find_or_create({asset_id => $aid->id});
+        my $asset = $assets->find_or_create($asset_info);
+        $asset->ensure_size;
+        $self->jobs_assets->find_or_create({asset_id => $asset->id});
     }
 
     return \%updated;
 }
 
+# calls `register_assets_from_settings` on all children to re-evaluate associated assets
+sub reevaluate_children_asset_settings ($self, $include_self = 0, $visited = {}) {
+    return undef                         if $visited->{$self->id}++;    # uncoverable statement
+    $self->register_assets_from_settings if $include_self;
+    $_->child->reevaluate_children_asset_settings(1, $visited) for $self->children->search(\%CHAINED_DEPENDENCY_QUERY);
+}
+
 sub _asset_find {
     my ($name, $type, $parents) = @_;
 

diff --git a/lib/OpenQA/Schema/Result/ScheduledProducts.pm b/lib/OpenQA/Schema/Result/ScheduledProducts.pm
@@ -217,7 +217,7 @@ sub _schedule_iso {
     for my $asset (values %{parse_assets_from_settings($args)}) {
         my ($name, $type) = ($asset->{name}, $asset->{type});
         return {error => 'Asset type and name must not be empty.'} unless $name && $type;
-        return {error => "Failed to register asset $name."}        unless $assets->register($type, $name, 1);
+        return {error => "Failed to register asset $name."} unless $assets->register($type, $name, {missing_ok => 1});
     }
 
     # read arguments for deprioritization and obsoleten

diff --git a/lib/OpenQA/Schema/ResultSet/Assets.pm b/lib/OpenQA/Schema/ResultSet/Assets.pm
@@ -1,4 +1,4 @@
-# Copyright (C) 2014-2019 SUSE LLC
+# Copyright (C) 2014-2021 SUSE LLC
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -18,6 +18,7 @@ package OpenQA::Schema::ResultSet::Assets;
 use strict;
 use warnings;
 
+use Mojo::Base -strict, -signatures;
 use base 'DBIx::Class::ResultSet';
 
 use DBIx::Class::Timestamps 'now';
@@ -37,35 +38,22 @@ sub status_cache_file {
 }
 
 # called when uploading an asset or finding one in scanning
-sub register {
-    my ($self, $type, $name, $missingok) = @_;
-    $missingok //= 0;
-
-    unless ($name) {
-        log_warning "attempt to register asset with empty name";
-        return;
-    }
-    unless (grep /^$type$/, TYPES) {
-        log_warning "asset type '$type' invalid";
-        return;
-    }
-    unless (locate_asset $type, $name, mustexist => 1) {
-        return 'missing' if ($missingok);
-
+sub register ($self, $type, $name, $options = {}) {
+    unless ($name)                 { log_warning 'attempt to register asset with empty name'; return undef }
+    unless (grep /^$type$/, TYPES) { log_warning "asset type '$type' invalid";                return undef }
+    if     (!$options->{missing_ok} && !locate_asset $type, $name, mustexist => 1) {
         log_warning "no file found for asset '$name' type '$type'";
-        return;
+        return undef;
     }
-
-    return $self->result_source->schema->txn_do(
+    $self->result_source->schema->txn_do(
         sub {
-            return $self->find_or_create(
-                {
-                    type => $type,
-                    name => $name,
-                },
-                {
-                    key => 'assets_type_name',
-                });
+            my $asset = $self->find_or_create({type => $type, name => $name}, {key => 'assets_type_name'});
+            if (my $created_by = $options->{created_by}) {
+                my $scope = $options->{scope} // 'public';
+                $created_by->jobs_assets->create({asset_id => $asset->id, created_by => 1});
+                $created_by->reevaluate_children_asset_settings if $scope ne 'public';
+            }
+            return $asset;
         });
 }
 
@@ -371,4 +359,16 @@ END_SQL
     return {assets => \@assets, groups => \%group_info, parents => \%parent_group_info};
 }
 
+sub untie_asset_from_job_and_unregister_if_unused ($self, $type, $name, $job) {
+    $self->result_source->schema->txn_do(
+        sub {
+            my %query = (type => $type, name => $name, fixed => 0);
+            return 0 unless my $asset = $self->find(\%query, {join => 'jobs_assets'});
+            $job->jobs_assets->search({asset_id => $asset->id})->delete;
+            return 0 if defined $asset->size || $asset->jobs->count;
+            $asset->delete;
+            return 1;
+        });
+}
+
 1;
diff --git a/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm b/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm
@@ -533,8 +533,9 @@ Used by the worker to upload files to the test.
 sub create_artefact {
     my ($self) = @_;
 
-    my $jobid = int($self->stash('jobid'));
-    my $job   = $self->schema->resultset('Jobs')->find($jobid);
+    my $jobid  = int($self->stash('jobid'));
+    my $schema = $self->schema;
+    my $job    = $schema->resultset('Jobs')->find($jobid);
     if (!$job) {
         log_info('Got artefact for non-existing job: ' . $jobid);
         return $self->render(json => {error => "Specified job $jobid does not exist"}, status => 404);
@@ -570,7 +571,7 @@ sub create_artefact {
             $validation->param('script'));
         return $self->render(text => "FAILED");
     }
-    elsif ($self->param('asset')) {
+    elsif (my $scope = $self->param('asset')) {
         $self->render_later;    # XXX: Not really needed, but in case of upstream changes
 
         # See: https://mojolicious.org/perldoc/Mojolicious/Guides/FAQ#What-does-Connection-already-closed-mean
@@ -580,25 +581,23 @@ sub create_artefact {
             sub {
                 die "Transaction empty" if $tx->is_empty;
                 OpenQA::Events->singleton->emit('chunk_upload.start' => $self);
-                my ($e, $fname, $type, $last)
-                  = $job->create_asset($validation->param('file'), $self->param('asset'), $self->param('local'));
-                OpenQA::Events->singleton->emit('chunk_upload.end' => ($self, $e, $fname, $type, $last));
-                die "$e" if $e;
+                my ($error, $fname, $type, $last)
+                  = $job->create_asset($validation->param('file'), $scope, $self->param('local'));
+                OpenQA::Events->singleton->emit('chunk_upload.end' => ($self, $error, $fname, $type, $last));
+                die $error if $error;
                 return $fname, $type, $last;
             },
             sub {
-                my ($subprocess, $e, @results) = @_;
-                # Even if most probably it is an error on client side, we return 500
-                # So worker can keep retrying if it was caused by network failures
-                $self->app->log->debug($e) if $e;
-
-                $self->render(json => {error => 'Failed receiving asset: ' . $e}, status => 500) and return if $e;
-                my $fname = $results[0];
-                my $type  = $results[1];
-                my $last  = $results[2];
-
-                $job->jobs_assets->create({job => $job, asset => {name => $fname, type => $type}, created_by => 1})
-                  if $last && !$e;
+                my ($subprocess, $error, @results) = @_;
+                if ($error) {
+                    # return 500 even if most probably it is an error on client side so the worker can keep
+                    # retrying if it was caused by network failures
+                    $self->app->log->debug($error);
+                    return $self->render(json => {error => "Failed receiving asset: $error"}, status => 500);
+                }
+
+                my ($fname, $type, $last) = @results;
+                $schema->resultset('Assets')->register($type, $fname, {scope => $scope, created_by => $job}) if $last;
                 return $self->render(json => {status => 'ok'});
             });
     }