From 06bcc8ca6a33fa4a884d05c606d537b1aef945f3 Mon Sep 17 00:00:00 2001 From: Marius Kittler Date: Wed, 7 Aug 2024 13:29:11 +0200 Subject: [PATCH 1/5] Use test labels in test for default prio assignment when posting jobs Related ticket: https://progress.opensuse.org/issues/162029 --- t/api/04-jobs.t | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/api/04-jobs.t b/t/api/04-jobs.t index 347e63ca980..4f6fde3abcc 100644 --- a/t/api/04-jobs.t +++ b/t/api/04-jobs.t @@ -915,16 +915,16 @@ subtest 'default priority correctly assigned when posting job' => sub { # post new job and check default priority $t->post_ok('/api/v1/jobs', form => \%jobs_post_params)->status_is(200); $t->get_ok('/api/v1/jobs/' . $t->tx->res->json->{id})->status_is(200); - $t->json_is('/job/group', 'opensuse'); - $t->json_is('/job/priority', 50); + $t->json_is('/job/group', 'opensuse', 'group assigned (1)'); + $t->json_is('/job/priority', 50, 'global default priority assigned'); # post new job in job group with customized default priority $schema->resultset('JobGroups')->find({name => 'opensuse test'})->update({default_priority => 42}); $jobs_post_params{_GROUP} = 'opensuse test'; $t->post_ok('/api/v1/jobs', form => \%jobs_post_params)->status_is(200); $t->get_ok('/api/v1/jobs/' . $t->tx->res->json->{id})->status_is(200); - $t->json_is('/job/group', 'opensuse test'); - $t->json_is('/job/priority', 42); + $t->json_is('/job/group', 'opensuse test', 'group assigned (2)'); + $t->json_is('/job/priority', 42, 'default priority from group assigned'); }; subtest 'specifying group by ID' => sub { From 08f8f88727eb2025452e6e8fda836c353f9d1a7c Mon Sep 17 00:00:00 2001 From: Marius Kittler Date: Wed, 7 Aug 2024 14:18:01 +0200 Subject: [PATCH 2/5] Allow specifying priority when creating a single set of jobs Related ticket: https://progress.opensuse.org/issues/162029 --- lib/OpenQA/Schema/ResultSet/Jobs.pm | 12 +++++++++++- t/api/04-jobs.t | 17 ++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/lib/OpenQA/Schema/ResultSet/Jobs.pm b/lib/OpenQA/Schema/ResultSet/Jobs.pm index 26cf07c92ac..0a911c359cd 100644 --- a/lib/OpenQA/Schema/ResultSet/Jobs.pm +++ b/lib/OpenQA/Schema/ResultSet/Jobs.pm @@ -16,6 +16,8 @@ use OpenQA::Utils 'testcasedir'; use Mojo::File 'path'; use Mojo::JSON 'encode_json'; use Mojo::URL; +use Mojolicious::Validator; +use Mojolicious::Validator::Validation; use Time::HiRes 'time'; use DateTime; @@ -121,11 +123,19 @@ sub create_from_settings { keys %settings; die 'The ' . join(',', @invalid_keys) . ' cannot include / in value' if @invalid_keys; + # validate special settings + my %special_settings = (_PRIORITY => delete $settings{_PRIORITY}); + my $validator = Mojolicious::Validator->new; + my $v = Mojolicious::Validator::Validation->new(validator => $validator, input => \%special_settings); + my $prio = $v->optional('_PRIORITY')->num->param; + die 'The following settings are invalid: ' . join(', ', @{$v->failed}) . "\n" if $v->has_error; + # assign group ID and priority my ($group_args, $group) = OpenQA::Schema::Result::Jobs::extract_group_args_from_settings(\%settings); + $new_job_args{priority} = $prio if defined $prio; if ($group) { $new_job_args{group_id} = $group->id; - $new_job_args{priority} = $group->default_priority; + $new_job_args{priority} //= $group->default_priority; } # handle dependencies diff --git a/t/api/04-jobs.t b/t/api/04-jobs.t index 4f6fde3abcc..6a82a7d72f1 100644 --- a/t/api/04-jobs.t +++ b/t/api/04-jobs.t @@ -911,7 +911,7 @@ subtest 'WORKER_CLASS correctly assigned when posting job' => sub { is $jobs->find($id)->settings_hash->{WORKER_CLASS}, 'svirt', 'specified WORKER_CLASS assigned'; }; -subtest 'default priority correctly assigned when posting job' => sub { +subtest 'priority correctly assigned when posting job' => sub { # post new job and check default priority $t->post_ok('/api/v1/jobs', form => \%jobs_post_params)->status_is(200); $t->get_ok('/api/v1/jobs/' . $t->tx->res->json->{id})->status_is(200); @@ -925,10 +925,25 @@ subtest 'default priority correctly assigned when posting job' => sub { $t->get_ok('/api/v1/jobs/' . $t->tx->res->json->{id})->status_is(200); $t->json_is('/job/group', 'opensuse test', 'group assigned (2)'); $t->json_is('/job/priority', 42, 'default priority from group assigned'); + + # post new job with explicitely specified _PRIORITY setting + $jobs_post_params{_PRIORITY} = 43; + $t->post_ok('/api/v1/jobs', form => \%jobs_post_params)->status_is(200); + $t->get_ok('/api/v1/jobs/' . $t->tx->res->json->{id})->status_is(200); + $t->json_is('/job/group', 'opensuse test', 'group assigned (3)'); + $t->json_is('/job/priority', 43, 'explicitely specified priority assigned'); + $t->json_is('/job/settings/_PRIORITY', undef, 'priority not added as setting'); + + # post new job with invalid explicitely specified _PRIORITY setting + $jobs_post_params{_PRIORITY} = 43.5; + $t->post_ok('/api/v1/jobs', form => \%jobs_post_params)->status_is(400); + $t->json_like('/error', qr/settings.*invalid.*_PRIORITY/, 'error returned'); + $t->json_is('/id', undef, 'no job ID returned'); }; subtest 'specifying group by ID' => sub { delete $jobs_post_params{_GROUP}; + delete $jobs_post_params{_PRIORITY}; $jobs_post_params{_GROUP_ID} = 1002; $t->post_ok('/api/v1/jobs', form => \%jobs_post_params)->status_is(200); $t->get_ok('/api/v1/jobs/' . $t->tx->res->json->{id})->status_is(200); From 00f9c3100fdec969db50e76a1272fa4d1643f06d Mon Sep 17 00:00:00 2001 From: Marius Kittler Date: Wed, 7 Aug 2024 14:18:51 +0200 Subject: [PATCH 3/5] Avoid including source code line number in API error message --- lib/OpenQA/Schema/ResultSet/Jobs.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/OpenQA/Schema/ResultSet/Jobs.pm b/lib/OpenQA/Schema/ResultSet/Jobs.pm index 0a911c359cd..22595ced6ee 100644 --- a/lib/OpenQA/Schema/ResultSet/Jobs.pm +++ b/lib/OpenQA/Schema/ResultSet/Jobs.pm @@ -121,7 +121,7 @@ sub create_from_settings { my @invalid_keys = grep { $_ =~ /^(PUBLISH_HDD|FORCE_PUBLISH_HDD|STORE_HDD)\S+(\d+)$/ && $settings{$_} =~ /\// } keys %settings; - die 'The ' . join(',', @invalid_keys) . ' cannot include / in value' if @invalid_keys; + die 'The ' . join(',', @invalid_keys) . " cannot include / in value\n" if @invalid_keys; # validate special settings my %special_settings = (_PRIORITY => delete $settings{_PRIORITY}); From 321977b7c4eafb10b6de39317851a515ad040164 Mon Sep 17 00:00:00 2001 From: Marius Kittler Date: Wed, 7 Aug 2024 14:27:43 +0200 Subject: [PATCH 4/5] Simplify priority handling for product scheduling After the priority handling has been added to `create_from_settings` in "Allow specifying priority when creating a single set of jobs" we no longer need an extra update of the created jobs when scheduling a product. Related ticket: https://progress.opensuse.org/issues/162029 --- lib/OpenQA/Schema/Result/ScheduledProducts.pm | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/OpenQA/Schema/Result/ScheduledProducts.pm b/lib/OpenQA/Schema/Result/ScheduledProducts.pm index 6c9d8d6fca5..f5dd31f0ca5 100644 --- a/lib/OpenQA/Schema/Result/ScheduledProducts.pm +++ b/lib/OpenQA/Schema/Result/ScheduledProducts.pm @@ -334,7 +334,6 @@ sub _schedule_iso { my %job_ids_by_test_machine; # key: "TEST@MACHINE", value: "array of job ids" for my $settings (@{$jobs || []}) { - my $prio = delete $settings->{PRIO}; $settings->{_GROUP_ID} = delete $settings->{GROUP_ID}; # create a new job with these parameters and count if successful, do not send job notifies yet @@ -348,10 +347,6 @@ sub _schedule_iso { my $j_id = $job->id; $job_ids_by_test_machine{_job_ref($settings)} //= []; push @{$job_ids_by_test_machine{_job_ref($settings)}}, $j_id; - - # set prio if defined explicitly (otherwise default prio is used) - $job->update({priority => $prio}) if (defined($prio)); - $self->_create_download_lists(\%tmp_downloads, $download_list, $j_id); } catch { @@ -603,7 +598,7 @@ sub _generate_jobs { my $error = OpenQA::JobSettings::generate_settings(\%params); $error_message .= $error if defined $error; - $settings{PRIO} = defined($priority) ? $priority : $job_template->prio; + $settings{_PRIORITY} = $priority // $job_template->prio; $settings{GROUP_ID} = $job_template->group_id; _populate_wanted_jobs_for_test_arg($args, \%settings, \%wanted); @@ -793,13 +788,13 @@ sub _schedule_from_yaml ($self, $args, $skip_chained_deps, $include_children, @l my $machine_settings = $mach->{settings} // {}; _merge_settings_and_worker_classes($machine_settings, $settings, \@worker_class); $settings->{BACKEND} = $mach->{backend} if $mach->{backend}; - $settings->{PRIO} = $mach->{priority} // DEFAULT_JOB_PRIORITY; + $settings->{_PRIORITY} = $mach->{priority} // DEFAULT_JOB_PRIORITY; } } # set priority of job if specified if (my $priority = $job_template->{priority}) { - $settings->{PRIO} = $priority; + $settings->{_PRIORITY} = $priority; } # handle further settings From 6fc4e627d140b429ecfdeff41b8b02c1f6b369d1 Mon Sep 17 00:00:00 2001 From: Marius Kittler Date: Wed, 7 Aug 2024 14:43:17 +0200 Subject: [PATCH 5/5] Add validation in route for updating job priority * Do the same validation as in newly introduced parsing of the `_PRIORITY` setting * Use less verbose coding style * See https://progress.opensuse.org/issues/162029 --- lib/OpenQA/WebAPI/Controller/API/V1/Job.pm | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm b/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm index 6f7dffe73e8..395a2459eef 100644 --- a/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm +++ b/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm @@ -487,11 +487,12 @@ Sets priority for a given job. sub prio ($self) { return unless my $job = $self->find_job_or_render_not_found($self->stash('jobid')); - my $res = $job->set_prio($self->param('prio')); + my $v = $self->validation; + my $prio = $v->required('prio')->num->param; + return $self->reply->validation_error({format => 'json'}) if $v->has_error; - # Referencing the scalar will result in true or false - # (see http://mojolicio.us/perldoc/Mojo/JSON) - $self->render(json => {result => \$res}); + # Referencing the scalar will result in true or false (see http://mojolicio.us/perldoc/Mojo/JSON) + $self->render(json => {result => \$job->set_prio($prio)}); } =over 4