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

Gru tasks cleanup #2011

Merged
merged 7 commits into from Mar 5, 2019
Merged

Gru tasks cleanup #2011

merged 7 commits into from Mar 5, 2019

Conversation

kraih
Copy link
Member

@kraih kraih commented Mar 4, 2019

This should be the correct solution for #2008. It's ready for merging, but i might still add a few more test cases.

Progress: https://progress.opensuse.org/issues/48554

@Martchus
Copy link
Contributor

Martchus commented Mar 4, 2019

Looks good so far.

@AdamWill
Copy link
Contributor

AdamWill commented Mar 4, 2019

By eyeball this looks fine. I'm gonna do a build with it and deploy it on Fedora staging so I can verify that it does address the specific case that was going wrong there (I expect it will).

$self->_fail_gru($gru_id => $err);
}
elsif ($state eq 'active') {
$self->_delete_gru($gru_id) if $self->finish(defined $buffer ? $buffer : 'Job successfully executed');
Copy link
Member Author

Choose a reason for hiding this comment

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

In case you were wondering why this is a separate case from the finished state. Here we avoid a possible race condition where a task calls $job->retry() and the retried job gets picked up by a worker fast enough to be in the active state before reaching this line of code. Due to the built-in versioning of Minion jobs the $self->finish call here would fail, preventing the Gru task from getting deleted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now, testing this race condition is hard, and something i'm still thinking about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is worth adding this as a comment in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #2011 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2011      +/-   ##
=========================================
- Coverage   89.17%   89.1%   -0.08%     
=========================================
  Files         153     153              
  Lines       10358   10364       +6     
=========================================
- Hits         9237    9235       -2     
- Misses       1121    1129       +8
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/GruJob.pm 5.88% <0%> (-1.27%) ⬇️
lib/OpenQA/Worker/Commands.pm 81.17% <0%> (-2.36%) ⬇️

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 e27d66d...5df588d. Read the comment docs.

@AdamWill
Copy link
Contributor

AdamWill commented Mar 5, 2019

Confirmed this seems to fix the issue observed in Fedora just fine; I updated to a build with this patch applied, wiped an ISO, re-scheduled the tests for that ISO on two flavors, and the tests for both flavors correctly waited for the download before commencing (but then did commence once it was done :>). Thanks, @kraih .

@kraih
Copy link
Member Author

kraih commented Mar 5, 2019

@AdamWill Thanks, i will merge the patch later today then if there are no objections.

@kraih kraih requested a review from Martchus March 5, 2019 11:22
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

Since @AdamWill already tested it I didn't run any local tests. From looking at the code I'd say it can be merged.

@kraih kraih merged commit a5aadf2 into os-autoinst:master Mar 5, 2019
coolo pushed a commit that referenced this pull request Mar 5, 2019
commit a5aadf2
Merge: 7fd90ad 5df588d
Author:     Sebastian Riedel <kraihx@gmail.com>
AuthorDate: Tue Mar 5 15:58:52 2019 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Tue Mar 5 15:58:52 2019 +0100

    Merge pull request #2011 from kraih/gru_tasks_cleanup

    Gru tasks cleanup
@kraih kraih deleted the gru_tasks_cleanup branch May 12, 2020 13:19
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