-
Notifications
You must be signed in to change notification settings - Fork 284
Non-hypervised Docker CPU Environment #4235
Conversation
A temporary subclass of DockerCPUEnvironment that always uses DummyHypervisor and therefore performs no operations on Docker virtual machine. It is meant to enable usage of the new Environment API without removing DockerManager yet. Using standard DockerCPUEnvironment would cause potential conflicts during VM reconfiguration.
Setting up DockerCPUEnvironment in TaskComputer.__init__() requires two conditions to be met: * Twisted reactor running (because prepare() returns a Deferred) * ClientConfigDescriptor with proper memory and CPU settings
6b2d253
to
5791443
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
So many test patches.. at least it has good coverage :P
One small comment, still approving!
Codecov Report
@@ Coverage Diff @@
## develop #4235 +/- ##
===========================================
- Coverage 88.56% 84.87% -3.69%
===========================================
Files 224 224
Lines 19713 19725 +12
===========================================
- Hits 17458 16741 -717
- Misses 2255 2984 +729 |
Client.start() requires to be run with reactor.
1a0ed96
to
c490983
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4235 +/- ##
=========================================
+ Coverage 88.49% 88.7% +0.2%
=========================================
Files 224 225 +1
Lines 19715 19766 +51
=========================================
+ Hits 17447 17533 +86
+ Misses 2268 2233 -35 |
4318b05
to
5e868e6
Compare
When a task header with environment prerequistes is received provider will attempt to install the specified prerequisites and report positive support status only if the installation was succesful.
c464ef4
to
a068932
Compare
a068932
to
db39955
Compare
@mfranciszkiewicz @maaktweluit The code is ready for review. The only thing missing is Golem-Messages update. I'll update the requirements as soon as I get to release Golem-Messages with this change: golemfactory/golem-messages#349 |
Golem-Messages 3.8.0 version introduces environment_prerequisites field to TaskHeader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -5,6 +5,7 @@ | |||
from tests.golem.task.dummy import runner, task | |||
|
|||
|
|||
@mock.patch('golem.task.taskserver.NonHypervisedDockerCPUEnvironment') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, if you use mock.Mock()
as the second argument you won't need to put the extra _
argument in the test methods.
A temporary subclass of
DockerCPUEnvironment
that always usesDummyHypervisor
and therefore performs no operations on Docker virtual machine. It is meant to enable usage of the new Environment API without removingDockerManager
yet. Using standardDockerCPUEnvironment
would cause potential conflicts during VM reconfiguration.