-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use needles from correct ref of NEEDLES_DIR #5175
base: master
Are you sure you want to change the base?
Changes from all commits
2bdf709
87b9fdc
60bdc81
52367a6
124af5c
64cd052
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
# Copyright SUSE LLC | ||
# SPDX-License-Identifier: GPL-2.0-or-later | ||
|
||
package OpenQA::Needles; | ||
use Mojo::Base -strict, -signatures; | ||
|
||
use Exporter qw(import); | ||
use File::Basename; | ||
use File::Spec; | ||
use File::Spec::Functions qw(catdir); | ||
use OpenQA::Git; | ||
use OpenQA::Log qw(log_error); | ||
use OpenQA::Utils qw(prjdir sharedir); | ||
use Mojo::File qw(path); | ||
|
||
our @EXPORT = qw(temp_dir is_in_temp_dir needle_temp_dir locate_needle); | ||
|
||
my $tmp_dir = prjdir() . '/webui/cache/needle-refs'; | ||
|
||
sub temp_dir () { $tmp_dir } | ||
|
||
sub is_in_temp_dir ($file_path) { index($file_path, $tmp_dir) == 0 } | ||
|
||
sub needle_temp_dir ($dir, $ref) { path($tmp_dir, basename(dirname($dir)), $ref, 'needles') } | ||
|
||
sub _locate_needle_for_ref ($relative_needle_path, $needles_dir, $needles_ref) { | ||
return undef unless defined $needles_ref; | ||
|
||
my $temp_needles_dir = needle_temp_dir($needles_dir, $needles_ref); | ||
my $subdir = dirname($relative_needle_path); | ||
path($temp_needles_dir, $subdir)->make_path if File::Spec->splitdir($relative_needle_path) > 1; | ||
|
||
my $git = OpenQA::Git->new(dir => $needles_dir); | ||
my $temp_json_path = "$temp_needles_dir/$relative_needle_path"; | ||
my $basename = basename($relative_needle_path, '.json'); | ||
my $relative_png_path = "$subdir/$basename.png"; | ||
my $temp_png_path = "$temp_needles_dir/$relative_png_path"; | ||
my $error = $git->cache_ref($needles_ref, $relative_needle_path, $temp_json_path) | ||
// $git->cache_ref($needles_ref, $relative_png_path, $temp_png_path); | ||
return $temp_json_path unless defined $error; | ||
log_error "An error occurred when looking for ref '$needles_ref' of '$relative_needle_path': $error"; | ||
return undef; | ||
} | ||
|
||
sub locate_needle ($relative_needle_path, $needles_dir, $needles_ref = undef) { | ||
my $location_for_ref = _locate_needle_for_ref($relative_needle_path, $needles_dir, $needles_ref); | ||
return $location_for_ref if defined $location_for_ref; | ||
my $absolute_filename = catdir($needles_dir, $relative_needle_path); | ||
my $needle_exists = -f $absolute_filename; | ||
if (!$needle_exists) { | ||
$absolute_filename = catdir(sharedir(), $relative_needle_path); | ||
$needle_exists = -f $absolute_filename; | ||
} | ||
return $absolute_filename if $needle_exists; | ||
log_error "Needle file $relative_needle_path not found within $needles_dir."; | ||
return undef; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# Copyright SUSE LLC | ||
# SPDX-License-Identifier: GPL-2.0-or-later | ||
|
||
package OpenQA::Task::Needle::LimitTempRefs; | ||
use Mojo::Base 'Mojolicious::Plugin', -signatures; | ||
|
||
use File::Find; | ||
use File::stat; | ||
use Fcntl qw(S_ISDIR); | ||
use OpenQA::Needles; | ||
use OpenQA::Task::SignalGuard; | ||
use Time::Seconds; | ||
|
||
my $retention; | ||
|
||
sub register ($self, $app, $job) { | ||
$retention = $app->config->{'scm git'}->{temp_needle_refs_retention} * ONE_MINUTE; | ||
$app->minion->add_task(limit_temp_needle_refs => sub ($job) { _limit($app, $job) }); | ||
} | ||
|
||
sub _limit ($app, $job) { | ||
my $ensure_task_retry_on_termination_signal_guard = OpenQA::Task::SignalGuard->new($job); | ||
|
||
return $job->finish({error => 'Another job to remove needle versions is running. Try again later.'}) | ||
unless my $guard = $app->minion->guard('limit_needle_versions_task', 7200); | ||
|
||
# remove all temporary needles which haven't been accessed in time period specified in config | ||
my $temp_dir = OpenQA::Needles::temp_dir; | ||
return undef unless -d $temp_dir; | ||
my $now = time; | ||
my $wanted = sub { | ||
return undef unless my $lstat = lstat $File::Find::name; | ||
return rmdir $File::Find::name if S_ISDIR($lstat->mode); # remove all empty dirs | ||
return unlink $File::Find::name if ($now - $lstat->mtime) > $retention; | ||
}; | ||
find({no_chdir => 1, bydepth => 1, wanted => $wanted}, $temp_dir); | ||
} | ||
|
||
1; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,12 @@ | |
use Mojo::File 'path'; | ||
use Mojo::URL; | ||
use Mojo::Util 'decode'; | ||
use OpenQA::Utils qw(ensure_timestamp_appended find_bug_number locate_needle needledir testcasedir); | ||
use OpenQA::Needles qw(needle_temp_dir locate_needle); | ||
use OpenQA::Utils qw(ensure_timestamp_appended find_bug_number needledir testcasedir | ||
run_cmd_with_log run_cmd_with_log_return_error); | ||
use OpenQA::Jobs::Constants; | ||
use File::Basename; | ||
use File::Path 'make_path'; | ||
use File::Which 'which'; | ||
use POSIX 'strftime'; | ||
use Mojo::JSON 'decode_json'; | ||
|
@@ -91,6 +94,34 @@ | |
$self->viewimg; | ||
} | ||
|
||
sub _determine_needles_dir_for_job ($self, $job) { | ||
return undef unless $self->app->config->{'scm git'}->{checkout_needles_sha} eq 'yes'; | ||
return undef unless my $needle_dirs = realpath($job->needle_dir); | ||
my $settings = $job->settings; | ||
return ($needle_dirs, $settings->single({key => 'NEEDLES_DIR'}) // $settings->single({key => 'CASEDIR'})); | ||
} | ||
|
||
sub _create_tmpdir_for_needles_refspec ($self, $job) { | ||
my ($needle_dirs, $needles_dir_var) = $self->_determine_needles_dir_for_job($job); | ||
return undef unless $needles_dir_var; | ||
my $needles_url = Mojo::URL->new($needles_dir_var->value); | ||
return undef unless $needles_url->scheme; | ||
my $needles_ref = $needles_url->fragment; | ||
eval { | ||
my $vars = decode_json(path($job->result_dir, 'vars.json')->slurp); | ||
$needles_ref = $vars->{NEEDLES_GIT_HASH} if ref $vars eq 'HASH'; | ||
}; | ||
chomp $needles_ref; | ||
return undef unless $needles_ref; | ||
return undef unless run_cmd_with_log ['git', '-C', $needle_dirs, 'fetch', '--depth', 1, 'origin', $needles_ref]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sdclarke We have not forgotten about this. We are currently working on some git features and fixes, and I just wanted to note that we have a That would make this Would you consider to use the I'm not sure if there are any cases which There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that Even then it is not guaranteed to fetch the refs the test actually runs on. The test might specify a branch name (or no branch which means "default branch") and when the test is finally executed on the worker host the ref that branch points to might differ from the ref checked out by the web UI at the time the test was scheduled. Additionally, we probably want to have some cleanup for the fetching done by the So I think we need that fetch in any case.
I don't think we need a lockfile here. We can probably arrange it so that it would not matter if any other commands are executed in the middle. (The use of
That's true. So we could (and probably should) get rid of it. If one specifies a branch name via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
And this is all we need, no? The only thing could be forks, but as far as I can see this PR also doesn't handle forks.
What's checked out on the webui does not matter. The important thing is that it has been fetched at some point, so we can do a
Yes. I'm not sure what git already offers there and how to know what we can cleanup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I meant "fetched by the web UI". And I meant the following sequence of events:
And this is not very contrived. That someone specifies a branch where many commits happen (e.g. the default branch of some repo with many contributors) is probably a common use case. That it can take between the scheduling of a job and the time it actually gets executed is also something we need to expect. |
||
my $rev_parse_res = run_cmd_with_log_return_error ['git', '-C', $needle_dirs, 'rev-parse', 'FETCH_HEAD']; | ||
return undef unless $rev_parse_res->{status}; | ||
$needles_ref = $rev_parse_res->{stdout}; | ||
chomp $needles_ref; | ||
needle_temp_dir($needle_dirs, $needles_ref)->make_path; | ||
return $needles_ref; | ||
} | ||
|
||
# Needle editor | ||
sub edit ($self) { | ||
return $self->reply->not_found unless $self->_init && $self->check_tabmode(); | ||
|
@@ -101,6 +132,7 @@ | |
my $distri = $job->DISTRI; | ||
my $dversion = $job->VERSION || ''; | ||
my $needle_dir = $job->needle_dir; | ||
my $needle_ref = $self->_create_tmpdir_for_needles_refspec($job); | ||
my $app = $self->app; | ||
my $needles_rs = $app->schema->resultset('Needles'); | ||
|
||
|
@@ -130,7 +162,7 @@ | |
# Second position: the only needle (with the same matches) | ||
my $needle_info | ||
= $self->_extended_needle_info($needle_dir, $needle_name, \%basic_needle_data, $module_detail->{json}, | ||
0, \@error_messages); | ||
0, \@error_messages, $needle_ref); | ||
if ($needle_info) { | ||
$needle_info->{matches} = $screenshot->{matches}; | ||
push(@needles, $needle_info); | ||
|
@@ -144,10 +176,10 @@ | |
# $needle contains information from result, in which 'areas' refers to the best matches. | ||
# We also use $area for transforming the match information into a real area | ||
for my $needle (@$module_detail_needles) { | ||
my $needle_info = $self->_extended_needle_info( | ||
$needle_dir, $needle->{name}, \%basic_needle_data, | ||
$needle->{json}, $needle->{error}, \@error_messages | ||
) || next; | ||
my $needle_info | ||
= $self->_extended_needle_info($needle_dir, $needle->{name}, \%basic_needle_data, | ||
$needle->{json}, $needle->{error}, \@error_messages, $needle_ref) | ||
|| next; | ||
my $matches = $needle_info->{matches}; | ||
for my $match (@{$needle->{area}}) { | ||
my %area = ( | ||
|
@@ -188,7 +220,7 @@ | |
# get needle info to show the needle also in selection | ||
my $needle_info | ||
= $self->_extended_needle_info($needle_dir, $new_needle->name, \%basic_needle_data, $new_needle->path, | ||
undef, \@error_messages) | ||
undef, \@error_messages, $needle_ref) | ||
|| next; | ||
$needle_info->{title} = 'new: ' . $needle_info->{title}; | ||
push(@needles, $needle_info); | ||
|
@@ -274,9 +306,9 @@ | |
return \%screenshot; | ||
} | ||
|
||
sub _basic_needle_info ($self, $name, $distri, $version, $file_name, $needles_dir) { | ||
sub _basic_needle_info ($self, $name, $distri, $version, $file_name, $needles_dir, $needle_ref) { | ||
$file_name //= "$name.json"; | ||
$file_name = locate_needle($file_name, $needles_dir) if !-f $file_name; | ||
$file_name = locate_needle($file_name, $needles_dir, $needle_ref) if !-f $file_name; | ||
return (undef, 'File not found') unless defined $file_name; | ||
|
||
my $needle; | ||
|
@@ -303,11 +335,14 @@ | |
return ($needle, undef); | ||
} | ||
|
||
sub _extended_needle_info ($self, $needle_dir, $needle_name, $basic_needle_data, $file_name, $error, $error_messages) { | ||
sub _extended_needle_info ($self, $needle_dir, $needle_name, $basic_needle_data, $file_name, $error, $error_messages, | ||
$needle_ref) | ||
{ | ||
my $overall_list_of_tags = $basic_needle_data->{tags}; | ||
my $distri = $basic_needle_data->{distri}; | ||
my $version = $basic_needle_data->{version}; | ||
my ($needle_info, $err) = $self->_basic_needle_info($needle_name, $distri, $version, $file_name, $needle_dir); | ||
my ($needle_info, $err) | ||
= $self->_basic_needle_info($needle_name, $distri, $version, $file_name, $needle_dir, $needle_ref); | ||
unless (defined $needle_info) { | ||
push(@$error_messages, "Could not parse needle $needle_name for $distri $version: $err"); | ||
return undef; | ||
|
@@ -467,6 +502,7 @@ | |
my $distri = $job->DISTRI; | ||
my $dversion = $job->VERSION || ''; | ||
my $needle_dir = $job->needle_dir; | ||
my $needle_ref = $self->_create_tmpdir_for_needles_refspec($job); | ||
my $real_needle_dir = realpath($needle_dir) // $needle_dir; | ||
my $needles_rs = $self->app->schema->resultset('Needles'); | ||
|
||
|
@@ -502,7 +538,8 @@ | |
# load primary needle match | ||
my $primary_match; | ||
if (my $needle = $module_detail->{needle}) { | ||
my ($needleinfo) = $self->_basic_needle_info($needle, $distri, $dversion, $module_detail->{json}, $needle_dir); | ||
my ($needleinfo) | ||
= $self->_basic_needle_info($needle, $distri, $dversion, $module_detail->{json}, $needle_dir, $needle_ref); | ||
if ($needleinfo) { | ||
my $info = { | ||
name => $needle, | ||
|
@@ -524,7 +561,8 @@ | |
if ($module_detail->{needles}) { | ||
for my $needle (@{$module_detail->{needles}}) { | ||
my $needlename = $needle->{name}; | ||
my ($needleinfo) = $self->_basic_needle_info($needlename, $distri, $dversion, $needle->{json}, $needle_dir); | ||
my ($needleinfo) | ||
= $self->_basic_needle_info($needlename, $distri, $dversion, $needle->{json}, $needle_dir, $needle_ref); | ||
next unless $needleinfo; | ||
my $info = { | ||
name => $needlename, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
#!/bin/sh -e | ||
exec "$(dirname "$0")"/openqa eval -m production -V 'app->gru->enqueue(limit_temp_needle_refs => [], {priority => 5, ttl => 172800, limit => 1})' "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling that the comment is a bit cryptic from users' side. What does it mean
attempt
? Alsocorrect version
I think is not very explanatory. I am talking from the POV of someone who do not know much about the openQA or a new user.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'll be able to explain this feature within the config file. I think this comment is good enough for people trying to enable this feature after reading documentation about it elsewhere. So we could document this feature as part of the normal openQA documentation. However, I'd be fine keeping this as an almost undocumented "expert feature" for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe we can still change the word "correct". Who wouldn't want "correct versions"? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not about the branch that was used (branches are moving targets) but really about the exact version that was used (not a moving target). This lookup will probably also be best effort. So I find the version of this comment as it is right now much better than the suggested change.
Someone who doesn't want to pay the price for it. We could state the disadvantages here, especially that the feature is still experimental.