Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Get parent class as one row to avoid warnings #5822

Closed
wants to merge 1 commit into from
Closed

Get parent class as one row to avoid warnings #5822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions lib/OpenQA/Schema/Result/ScheduledProducts.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,14 +704,7 @@ sub _create_dependencies_for_parents ($self, $job, $created_jobs, $deptype, $par
$worker_class = $job->settings->find({key => 'WORKER_CLASS'});
$worker_class = $worker_class ? $worker_class->value : '';
}
my $parent_worker_class
= $schema->resultset('JobSettings')->find({job_id => $parent, key => 'WORKER_CLASS'});
$parent_worker_class = $parent_worker_class ? $parent_worker_class->value : '';
if ($worker_class ne $parent_worker_class) {
my $test_name = $job->TEST;
die
"Worker class of $test_name ($worker_class) does not match the worker class of its directly chained parent ($parent_worker_class)";
}
my $parent_worker_class = $schema->resultset('JobSettings')->parent_worker_class($parent, $worker_class);
}
$job_dependencies->create(
{
Expand Down
10 changes: 10 additions & 0 deletions lib/OpenQA/Schema/ResultSet/JobSettings.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,14 @@ sub query_for_settings ($self, $args) {
return $self->search({-and => \@conds});
}

sub parent_worker_class ($self, $parent_job_id, $worker_class) {
my $result = $self->search({parent_job_id => $job_id, key => 'WORKER_CLASS'})->next;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we only want one row, it's better to tell that the database

Suggested change
my $result = $self->search({parent_job_id => $job_id, key => 'WORKER_CLASS'})->next;
my $result = $self->search({parent_job_id => $job_id, key => 'WORKER_CLASS'}, {rows => 1})->single;

since there can be multiple WORKER_CLASS entries, we're still relying on the internal order of postgres here I guess.

my $parent_worker_class = $result ? $result->value : '';
die
"Worker class $worker_class does not match the worker class of its directly chained parent ($parent_worker_class)"
unless $worker_class eq $parent_worker_class;
die "Ambiguous job settings for dependent jobs" if $result->next;
Copy link
Contributor

@perlpunk perlpunk Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be multiple entries, that's valid

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have multiple WORKER_CLASS settings per job and that's perfectly fine. This code should actually get all of them and then compare whether the full set of worker classes is identical to the full set of worker classes of the parent job (the order doesn't matter).

Note that this means that already the line $job->settings->find({key => 'WORKER_CLASS'}); is problematic. Also in this case we need to handle that there can be multiple worker classes (and then compare it to the multiple worker classes the call you're trying to fix might return).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, like I wrote. And in the case of ResultSet::Jobs::create_from_settings we probably want to copy every WORKER_CLASS to the child

return $parent_worker_class;
}

1;
19 changes: 3 additions & 16 deletions lib/OpenQA/Schema/ResultSet/Jobs.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,9 @@ sub create_from_settings {
my $dependency_type = $dependency_definition->{dependency_type};
for my $id (@$ids) {
if ($dependency_type eq OpenQA::JobDependencies::Constants::DIRECTLY_CHAINED) {
my $parent_worker_class = $job_settings->find({job_id => $id, key => 'WORKER_CLASS'});
_handle_directly_chained_dep($parent_worker_class, $id, \%settings);
my $parent_worker_class = $job_settings->parent_worker_class($id, $settings->{WORKER_CLASS});
# assume we want to use the worker class from the parent here (and not the default which is otherwise assumed)
$settings->{WORKER_CLASS} = $parent_worker_class;
perlpunk marked this conversation as resolved.
Show resolved Hide resolved
}
push(@{$new_job_args{parents}}, {parent_job_id => $id, dependency => $dependency_type});
}
Expand Down Expand Up @@ -193,20 +194,6 @@ sub create_from_settings {
return $job;
}

sub _handle_directly_chained_dep ($parent_worker_class, $id, $settings) {
if ($parent_worker_class = $parent_worker_class ? $parent_worker_class->value : '') {
if (!$settings->{WORKER_CLASS}) {
# assume we want to use the worker class from the parent here (and not the default which
# is otherwise assumed)
$settings->{WORKER_CLASS} = $parent_worker_class;
}
elsif ($settings->{WORKER_CLASS} ne $parent_worker_class) {
die "Specified WORKER_CLASS ($settings->{WORKER_CLASS}) does not match the one from"
. " directly chained parent $id ($parent_worker_class)";
}
}
}

sub _search_modules ($self, $module_re) {
my $distris = path(testcasedir);
my @results;
Expand Down
Loading