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.

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

Fix removal of scheduling assets #1811

Merged
merged 4 commits into from
Oct 1, 2018
Merged

Conversation

coolo
Copy link
Contributor

@coolo coolo commented Sep 28, 2018

See https://progress.opensuse.org/issues/41483#note-17 - the webui
schedules assets while GRU tries to calculate if the asset should
be removed. And in between 2 queries the situation changes - causing
an early removal

See https://progress.opensuse.org/issues/41483#note-17 - the webui
schedules assets while GRU tries to calculate if the asset should
be removed. And in between 2 queries the situation changes - causing
an early removal
@coolo
Copy link
Contributor Author

coolo commented Sep 28, 2018

Hotpatched on osd

@@ -239,6 +239,9 @@ END_SQL
while (my $result = $max_job_by_group_prepared_query->fetchrow_hashref) {
my $asset_info = $asset_info{$result->{asset_id}} or next;
$asset_info->{groups}->{$group_id} = $result->{max_job};
if ($result->{max_job} > ($asset_info->{max_job} || 0)) {
die "$asset_info->{name} was scheduled during cleanup, we are in troubled water";
Copy link
Member

Choose a reason for hiding this comment

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

I'd add $result->{max_job}

@coolo
Copy link
Contributor Author

coolo commented Sep 28, 2018

this lead to quite some failing jobs in about no time. There are enough other cases, but it shows that it happens way more often than what is considered safe for failure.

So another option is to rerun the job - or run the whole function in a transaction, which might be the easiest solution.

@codecov
Copy link

codecov bot commented Oct 1, 2018

Codecov Report

Merging #1811 into master will decrease coverage by 18.66%.
The diff coverage is 96.96%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1811       +/-   ##
===========================================
- Coverage    90.4%   71.73%   -18.67%     
===========================================
  Files         139      139               
  Lines        9878     9882        +4     
===========================================
- Hits         8930     7089     -1841     
- Misses        948     2793     +1845
Impacted Files Coverage Δ
lib/OpenQA/Task/Asset/Limit.pm 100% <ø> (ø) ⬆️
lib/OpenQA/Schema/ResultSet/Assets.pm 99.11% <96.96%> (-0.89%) ⬇️
lib/OpenQA/ServerSideDataTable.pm 3.03% <0%> (-96.97%) ⬇️
lib/OpenQA/WebAPI/Controller/ApiKey.pm 6.25% <0%> (-93.75%) ⬇️
lib/OpenQA/WebAPI/Controller/Admin/User.pm 4.76% <0%> (-90.48%) ⬇️
lib/OpenQA/WebAPI/Auth/Fake.pm 10% <0%> (-90%) ⬇️
lib/OpenQA/WebAPI/Controller/Admin/JobTemplate.pm 11.11% <0%> (-88.89%) ⬇️
lib/OpenQA/WebAPI/Controller/Admin/Asset.pm 14.28% <0%> (-85.72%) ⬇️
lib/OpenQA/WebAPI/Controller/Admin/JobGroup.pm 6.25% <0%> (-81.25%) ⬇️
lib/OpenQA/WebAPI/Controller/Step.pm 4.45% <0%> (-79.83%) ⬇️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d8d061...3f5eceb. Read the comment docs.

So we don't work with inconsistent data when the asset cleanup
is done when scheduling a new ISO.
@Martchus
Copy link
Contributor

Martchus commented Oct 1, 2018

The diff of the last commit affects a lot of lines but I just moved the actual database operations together and put them into a $schema->txn_do. And since I had to move the variable declaration of %group_infos anyways I changed the name to %group_info.

@coolo coolo merged commit 2689489 into os-autoinst:master Oct 1, 2018
@coolo coolo deleted the fix_issue_41483 branch October 1, 2018 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants