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

Remove resource allocator #1990

Merged
merged 7 commits into from Mar 1, 2019

Conversation

kraih
Copy link
Member

@kraih kraih commented Feb 7, 2019

When i started replacing the dbus methods in OpenQA::ResourceAllocator with a redis alternative i noticed that dbus was only used for blocking RPC calls. And that means that we could have just as well used OpenQA::Resource::Locks/Jobs directly from the API controllers, making the resource allocator obsolete. So that's what i ended up doing. This pull request removes the resource allocator completely, and i will have to use another service for introducing redis into openQA (most likely the scheduler).

The only noticeable difference should be the removal of the single process bottleneck that blocking RPC calls to OpenQA::ResouceAllocator caused. And that means there is a small risk that new race conditions will be introduced once locks/barriers can be managed with the prefork daemon. But looking through the code in OpenQA::Resource::Locks/Jobs (and having done a few tests) it seems rather defensive and i'm cautiously optimistic that it will "just work". 😉 Worst case, we need to add one or two transactions to OpenQA::Resource::Locks later.

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

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #1990 into master will increase coverage by 17.09%.
The diff coverage is 96%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1990       +/-   ##
===========================================
+ Coverage    71.8%   88.89%   +17.09%     
===========================================
  Files         132      153       +21     
  Lines        9614    10356      +742     
===========================================
+ Hits         6903     9206     +2303     
+ Misses       2711     1150     -1561
Impacted Files Coverage Δ
lib/OpenQA/IPC.pm 83.76% <ø> (-0.28%) ⬇️
lib/OpenQA/WebAPI/Controller/API/V1/Command.pm 25% <ø> (ø)
lib/OpenQA/Task/Asset/Download.pm 16.94% <ø> (-5.78%) ⬇️
lib/OpenQA.pm 70.27% <ø> (ø)
lib/OpenQA/Scheduler/Scheduler.pm 90.69% <ø> (+4.98%) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Iso.pm 95.34% <ø> (+15.98%) ⬆️
lib/OpenQA/WebAPI.pm 97.14% <ø> (+0.33%) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm 87.18% <100%> (+38.06%) ⬆️
lib/OpenQA/Resource/Jobs.pm 100% <100%> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Asset.pm 98.07% <100%> (+64.74%) ⬆️
... and 83 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 7179521...893ae52. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #1990 into master will increase coverage by 17.17%.
The diff coverage is 96%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1990       +/-   ##
===========================================
+ Coverage    71.8%   88.97%   +17.17%     
===========================================
  Files         132      153       +21     
  Lines        9614    10340      +726     
===========================================
+ Hits         6903     9200     +2297     
+ Misses       2711     1140     -1571
Impacted Files Coverage Δ
lib/OpenQA/IPC.pm 83.76% <ø> (-0.28%) ⬇️
lib/OpenQA/WebAPI/Controller/API/V1/Command.pm 25% <ø> (ø)
lib/OpenQA/Task/Asset/Download.pm 23.25% <ø> (+0.52%) ⬆️
lib/OpenQA.pm 70.27% <ø> (ø)
lib/OpenQA/Scheduler/Scheduler.pm 88.37% <ø> (+2.65%) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Iso.pm 95.34% <ø> (+15.98%) ⬆️
lib/OpenQA/WebAPI.pm 97.14% <ø> (+0.33%) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm 87.18% <100%> (+38.06%) ⬆️
lib/OpenQA/Resource/Jobs.pm 100% <100%> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Asset.pm 98.07% <100%> (+64.74%) ⬆️
... and 82 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 7179521...893ae52. Read the comment docs.

@coolo coolo added the acceptance-tests-needed Needed for code that is required to be tested on a production-like environment label Feb 7, 2019
@coolo
Copy link
Contributor

coolo commented Feb 7, 2019

please give a good test with multi machine jobs on staging

@Martchus
Copy link
Contributor

Martchus commented Feb 7, 2019

This looks already good and also complete with all the adjustments to the spec file and bootstrap script. Let's see how it works on staging.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

+1

@kraih
Copy link
Member Author

kraih commented Feb 8, 2019

Multi machine tests will take a little more time i'm afraid, since our staging machines aren't working properly at the moment (and i'm still learning while trying to fix them 😉).

@coolo
Copy link
Contributor

coolo commented Feb 9, 2019

you learning about them has priority over merging this :)

@kraih kraih mentioned this pull request Feb 12, 2019
@kraih
Copy link
Member Author

kraih commented Feb 12, 2019

Small progress update, ran CaaSP multi machine tests so far and everything looks fine.

@Martchus
Copy link
Contributor

I'm wondering on which instance. Only openqa-staging-1 has CaaSP jobs but they failed as incomplete.

@kraih
Copy link
Member Author

kraih commented Feb 13, 2019

@Martchus I restarted the CaaSP jobs right away yesterday to try again after they were successful, but it looks like the API keys expired around the same time...

@kraih
Copy link
Member Author

kraih commented Feb 13, 2019

@ldevulder also ran some independent HA/HPC tests successfully on his lab with this pull request applied.

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.

Not sure why http://openqa-staging-1.qa.suse.de/tests/756 failed but I guess it is beyond the scope of removing the resource allocator :-)

@Martchus Martchus merged commit 0ef8f6a into os-autoinst:master Mar 1, 2019
coolo pushed a commit that referenced this pull request Mar 1, 2019
commit 0ef8f6a
Merge: 40a74f5 893ae52
Author:     Martchus <martchus@gmx.net>
AuthorDate: Fri Mar 1 16:17:22 2019 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Fri Mar 1 16:17:22 2019 +0100

    Merge pull request #1990 from kraih/remove_resource_allocator

    Remove resource allocator
@kraih kraih deleted the remove_resource_allocator branch May 12, 2020 13:19
perlpunk added a commit to perlpunk/openQA that referenced this pull request Mar 19, 2022
perlpunk added a commit to perlpunk/openQA that referenced this pull request Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acceptance-tests-needed Needed for code that is required to be tested on a production-like environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants