Skip to content

Commit

Permalink
Merge pull request #5450 from b10n1k/perlcritic_deeply_nested
Browse files Browse the repository at this point in the history 
Refactor methods which violate the deeply nested loops
  • Loading branch information
@okurz
okurz committed Feb 6, 2024
2 parents 656da5a + de41fba commit dfb33b1
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 97 deletions.
32 changes: 17 additions & 15 deletions lib/OpenQA/Scheduler/Model/Jobs.pm
View file
Expand Up @@ -101,21 +101,7 @@ sub _allocate_jobs ($self, $free_workers) {
my $prio = $j->{priority}; # we only consider the priority of the main job
for my $worker (keys %taken) {
my $ji = $taken{$worker};
if ($prio > 0) {
# this means we will by default increase the offset per half-assigned job,
# so if we miss 1/25 jobs, we'll bump by +24
log_debug "Discarding job $ji->{id} (with priority $prio) due to incomplete parallel cluster"
. ', reducing priority by '
. STARVATION_PROTECTION_PRIORITY_OFFSET;
$j->{priority_offset} += STARVATION_PROTECTION_PRIORITY_OFFSET;
}
else {
# don't "take" the worker, but make sure it's not
# used for another job and stays around
log_debug "Holding worker $worker for job $ji->{id} to avoid starvation";
$allocated_workers->{$worker} = $ji->{id};
}

_allocate_worker_with_priority($prio, $ji, $j, $allocated_workers, $worker);
}
%taken = ();
last;
Expand All @@ -141,6 +127,22 @@ sub _allocate_jobs ($self, $free_workers) {
return ($allocated_workers, $allocated_jobs);
}

sub _allocate_worker_with_priority ($prio, $ji, $j, $allocated_workers, $worker) {
if ($prio > 0) {
# this means we will by default increase the offset per half-assigned job,
# so if we miss 1/25 jobs, we'll bump by +24
log_debug "Discarding job $ji->{id} (with priority $prio) due to incomplete parallel cluster"
. ', reducing priority by '
. STARVATION_PROTECTION_PRIORITY_OFFSET;
$j->{priority_offset} += STARVATION_PROTECTION_PRIORITY_OFFSET;
}
else {
# don't "take" the worker, but make sure it's not
# used for another job and stays around
log_debug "Holding worker $worker for job $ji->{id} to avoid starvation";
$allocated_workers->{$worker} = $ji->{id};
}
}

sub schedule ($self) {
my $start_time = time;
Expand Down
169 changes: 98 additions & 71 deletions lib/OpenQA/Schema/Result/JobGroups.pm
View file
Expand Up @@ -338,28 +338,32 @@ sub to_template {

foreach my $product (sort keys %{$group{scenarios}->{$arch}}) {
my @scenarios;
foreach my $test_suite (@{$group{scenarios}->{$arch}->{$product}}) {
foreach my $name (sort keys %$test_suite) {
my $attr = $test_suite->{$name};
if ($attr->{machine} eq $default_machine) {
delete $attr->{machine} if $test_suites{$arch}{$name} == 1;
}
if (keys %$attr) {
$test_suite->{$name} = $attr;
push @scenarios, $test_suite;
}
else {
push @scenarios, $name;
}
}
}
$group{scenarios}{$arch}{$product} = \@scenarios;
_remove_test_suite_defaults($product, $default_machine, $arch, \%group, \%test_suites, \@scenarios);
}
}

return \%group;
}

sub _remove_test_suite_defaults ($product, $default_machine, $arch, $group, $test_suites, $scenarios) {
foreach my $test_suite (@{$group->{scenarios}->{$arch}->{$product}}) {
foreach my $name (sort keys %$test_suite) {
my $attr = $test_suite->{$name};
if ($attr->{machine} eq $default_machine) {
delete $attr->{machine} if $test_suites->{$arch}{$name} == 1;
}
if (keys %$attr) {
$test_suite->{$name} = $attr;
push @$scenarios, $test_suite;
}
else {
push @$scenarios, $name;
}
}
}
$group->{scenarios}{$arch}{$product} = $scenarios;
}

sub to_yaml {
my ($self) = @_;
if ($self->template) {
Expand All @@ -382,68 +386,91 @@ sub template_data_from_yaml {
foreach my $arch (sort keys %$yaml_archs) {
my $yaml_products_for_arch = $yaml_archs->{$arch};
my $yaml_defaults_for_arch = $yaml_defaults->{$arch};
foreach my $product_name (sort keys %$yaml_products_for_arch) {
foreach my $spec (@{$yaml_products_for_arch->{$product_name}}) {
# Get testsuite, machine, prio and job template settings from YAML data
my $testsuite_name;
my $job_template_name;
# Assign defaults
my $prio = $yaml_defaults_for_arch->{priority};
my $machine_names = $yaml_defaults_for_arch->{machine};
my $settings = dclone($yaml_defaults_for_arch->{settings} // {});
my $description = '';
if (ref $spec eq 'HASH') {
# We only have one key. Asserted by schema
$testsuite_name = (keys %$spec)[0];
my $attr = $spec->{$testsuite_name};
if ($attr->{priority}) {
$prio = $attr->{priority};
}
if ($attr->{machine}) {
$machine_names = $attr->{machine};
}
if (exists $attr->{testsuite}) {
$job_template_name = $testsuite_name;
$testsuite_name = $attr->{testsuite};
}
if ($attr->{settings}) {
%$settings = (%{$settings // {}}, %{$attr->{settings}});
}
if (defined $attr->{description}) {
$description = $attr->{description};
}
my $ret = _parse_job_template_products($yaml_products_for_arch, $yaml_defaults_for_arch, $arch, $yaml_products,
$yaml_defaults, \%job_template_names);
return $ret if defined $ret;
}

return \%job_template_names;
}

sub _parse_job_template_products ($yaml_products_for_arch, $yaml_defaults_for_arch, $arch, $yaml_products,
$yaml_defaults, $job_template_names)
{
foreach my $product_name (sort keys %$yaml_products_for_arch) {
foreach my $spec (@{$yaml_products_for_arch->{$product_name}}) {
# Get testsuite, machine, prio and job template settings from YAML data
my $testsuite_name;
my $job_template_name;
# Assign defaults
my $prio = $yaml_defaults_for_arch->{priority};
my $machine_names = $yaml_defaults_for_arch->{machine};
my $settings = dclone($yaml_defaults_for_arch->{settings} // {});
my $description = '';
if (ref $spec eq 'HASH') {
# We only have one key. Asserted by schema
$testsuite_name = (keys %$spec)[0];
my $attr = $spec->{$testsuite_name};
if ($attr->{priority}) {
$prio = $attr->{priority};
}
else {
$testsuite_name = $spec;
if ($attr->{machine}) {
$machine_names = $attr->{machine};
}

$machine_names = [$machine_names] if ref($machine_names) ne 'ARRAY';
foreach my $machine_name (@{$machine_names}) {
my $job_template_key
= $arch . $product_name . $machine_name . ($testsuite_name // '') . ($job_template_name // '');
if ($job_template_names{$job_template_key}) {
my $name = $job_template_name // $testsuite_name;
my $error = "Job template name '$name' is defined more than once. "
. "Use a unique name and specify 'testsuite' to re-use test suites in multiple scenarios.";
return {error => $error};
}
$job_template_names{$job_template_key} = {
prio => $prio,
machine_name => $machine_name,
arch => $arch,
product_name => $product_name,
product_spec => $yaml_products->{$product_name},
job_template_name => $job_template_name,
testsuite_name => $testsuite_name,
settings => $settings,
length $description ? (description => $description) : (),
};
if (exists $attr->{testsuite}) {
$job_template_name = $testsuite_name;
$testsuite_name = $attr->{testsuite};
}
if ($attr->{settings}) {
%$settings = (%{$settings // {}}, %{$attr->{settings}});
}
if (defined $attr->{description}) {
$description = $attr->{description};
}
}
else {
$testsuite_name = $spec;
}

$machine_names = [$machine_names] if ref($machine_names) ne 'ARRAY';
my $ret = _parse_job_template_machines(
$machine_names, $job_template_names, $prio, $arch, $product_name,
$yaml_products, $job_template_name, $testsuite_name, $settings, $description
);
return $ret if defined $ret;
}
}
return undef;
}

return \%job_template_names;
sub _parse_job_template_machines (
$machine_names, $job_template_names, $prio, $arch, $product_name,
$yaml_products, $job_template_name, $testsuite_name, $settings, $description
)
{

foreach my $machine_name (@{$machine_names}) {
my $job_template_key
= $arch . $product_name . $machine_name . ($testsuite_name // '') . ($job_template_name // '');
if ($job_template_names->{$job_template_key}) {
my $name = $job_template_name // $testsuite_name;
my $error = "Job template name '$name' is defined more than once. "
. "Use a unique name and specify 'testsuite' to re-use test suites in multiple scenarios.";
return {error => $error};
}
$job_template_names->{$job_template_key} = {
prio => $prio,
machine_name => $machine_name,
arch => $arch,
product_name => $product_name,
product_spec => $yaml_products->{$product_name},
job_template_name => $job_template_name,
testsuite_name => $testsuite_name,
settings => $settings,
length $description ? (description => $description) : (),
};
}
return undef;
}

sub expand_yaml {
Expand Down
26 changes: 15 additions & 11 deletions lib/OpenQA/Schema/ResultSet/Jobs.pm
View file
Expand Up @@ -155,17 +155,7 @@ sub create_from_settings {
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'});
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)";
}
}
_handle_directly_chained_dep($parent_worker_class, $id, \%settings);
}
push(@{$new_job_args{parents}}, {parent_job_id => $id, dependency => $dependency_type});
}
Expand Down Expand Up @@ -203,6 +193,20 @@ 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

0 comments on commit dfb33b1

Please sign in to comment.