Skip to content

Commit

Permalink
Refactor methods which violate the deeply nested loops
Browse files Browse the repository at this point in the history 
Simply extract parts of the subroutines which causes perlcritic to complain
about deeply nested loops.

https://progress.opensuse.org/issues/138416

Signed-off-by: ybonatakis <[email protected]>
  • Loading branch information
@b10n1k
b10n1k committed Feb 5, 2024
1 parent 0d50a81 commit b767891
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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};
}
_parse_job_template_products($yaml_products_for_arch,
$yaml_defaults_for_arch,
$arch,
$yaml_products,
$yaml_defaults,
\%job_template_names);
}

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';
_parse_job_template_machines($machine_names,
$job_template_names,
$prio,
$arch,
$product_name,
$yaml_products,
$job_template_name,
$testsuite_name,
$settings,
$description);
}
}
}

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) {
# my ($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) : (),
};
}
}

sub expand_yaml {
Expand Down
26 changes: 15 additions & 11 deletions lib/OpenQA/Schema/ResultSet/Jobs.pm
View file
Original file line number Diff line number Diff line change
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 b767891

Please sign in to comment.