mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Clement Michaud <clement.michau...@gmail.com>
Subject Re: Review Request 69938: Add resource decorator hook to implicitly allocate mandatory resources.
Date Mon, 11 Feb 2019 20:44:04 GMT


> On fév. 11, 2019, 6:58 après-midi, Benjamin Mahler wrote:
> > include/mesos/hook.hpp
> > Lines 56-62 (patched)
> > <https://reviews.apache.org/r/69938/diff/1/?file=2124662#file2124662line56>
> >
> >     It's not clear from here what the semantics are, can you add:
> >     
> >     ```
> >     These new resources overwrite the previous ones in TaskInfo.
> >     ```

sure.


> On fév. 11, 2019, 6:58 après-midi, Benjamin Mahler wrote:
> > src/examples/test_hook_module.cpp
> > Lines 114-148 (patched)
> > <https://reviews.apache.org/r/69938/diff/1/?file=2124663#file2124663line114>
> >
> >     how about:
> >     
> >     ```
> >       // The test logic here is based on the use case of injecting
> >       // default network bandwidth resource.
> >       Result<Resources> masterLaunchTaskResourceDecorator(
> >           const TaskInfo& taskInfo,
> >           const Resources& slaveResources) override
> >       {      
> >         LOG(INFO) << "Executing 'masterLaunchTaskResourceDecorator' hook";
> >     
> >         // If slave does not declare network bandwidth resource,
> >         // don't set a default value for it.
> >         if (slaveResources.names().count("network_bandwidth") == 0) {
> >           return taskInfo.resources();
> >         }
> >     
> >         Resources taskResources = taskInfo.resources();
> >     
> >         // Add a default value for network bandwidth if absent.
> >         if (taskResources.names().count("network_bandwidth") == 0) {
> >           taskResources += CHECK_NOTERROR(Resources::parse("network_bandwidth:10"));
> >         }
> >     
> >         return taskResources;
> >       }
> >     ```

Much shorter, I like that.


> On fév. 11, 2019, 6:58 après-midi, Benjamin Mahler wrote:
> > src/hook/manager.cpp
> > Lines 149 (patched)
> > <https://reviews.apache.org/r/69938/diff/1/?file=2124665#file2124665line149>
> >
> >     Since we want to move `result`, it should not be const? I'm surprised it compiles,
I suspect the const renders the move below into a copy operation?

You are right!


> On fév. 11, 2019, 6:58 après-midi, Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 4459-4461 (patched)
> > <https://reviews.apache.org/r/69938/diff/1/?file=2124666#file2124666line4459>
> >
> >     Let's avoid the copy:
> >     
> >     ```
> >     *task.mutable_resources() =
> >       HookManager::masterLaunchTaskResourceDecorator(
> >           task,
> >           slave->totalResources));
> >     ```

done


> On fév. 11, 2019, 6:58 après-midi, Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 4482-4484 (patched)
> > <https://reviews.apache.org/r/69938/diff/1/?file=2124666#file2124666line4482>
> >
> >     Let's avoid the copy:
> >     
> >     ```
> >     *task.mutable_resources() =
> >       HookManager::masterLaunchTaskResourceDecorator(
> >           task,
> >           slave->totalResources));
> >     ```

done


> On fév. 11, 2019, 6:58 après-midi, Benjamin Mahler wrote:
> > src/tests/hook_tests.cpp
> > Lines 246-247 (patched)
> > <https://reviews.apache.org/r/69938/diff/1/?file=2124667#file2124667line246>
> >
> >     We can simplify this test by not using the mock executor and test containerizer,
and removing all the expectations around it below.
> >     
> >     Was it added because of copy / paste?

This was indeed a copy paste.


- Clement


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69938/#review212704
-----------------------------------------------------------


On fév. 10, 2019, 11:16 matin, Clement Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69938/
> -----------------------------------------------------------
> 
> (Updated fév. 10, 2019, 11:16 matin)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9315
>     https://issues.apache.org/jira/browse/MESOS-9315
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit introduces a master hook to decorate task resources in
> order to allocate a given amount of custom resource if the framework
> does not support it yet.
> 
> For instance, if one introduces a new custom resource in a cluster
> running frameworks not supporting this resource, there will be a mixed
> set of tasks consuming and not consuming this resource leading to
> isolation issues. By implementing this hook, a default amount can be
> allocated for a custom resources on behalf of the framework so that
> every tasks end up consuming this resource and Mesos can take it into
> account.
> 
> This implicit allocation of resource helps introducing a new custom
> resource in the clusters because, before this patch, all frameworks
> needed to be patched before introducing the new resource while now a
> default value can be applied for the frameworks not supporting the
> resource yet meaning the patches can be done later.
> 
> https://issues.apache.org/jira/browse/MESOS-9315
> 
> 
> Diffs
> -----
> 
>   include/mesos/hook.hpp 019887095e7845d5a65d133b0f58091d262ec55b 
>   src/examples/test_hook_module.cpp c4f449512a4cc150de8a99f44a525b96a2fc1ae2 
>   src/hook/manager.hpp b3d4f5198588068d3b28a57cffb3754b55e33b51 
>   src/hook/manager.cpp 3e71a26f8c0fcfefecc93d70f8a9d6c2d7fdcc6c 
>   src/master/master.cpp b4faf2b077a0288ba36195b7a21402932489d316 
>   src/tests/hook_tests.cpp d8aa35e0027d589044bb131b460311721bd36609 
> 
> 
> Diff: https://reviews.apache.org/r/69938/diff/1/
> 
> 
> Testing
> -------
> 
> I added a test showing that if a task was missing network_bandwidth resource in the TaskInfo,
the hook injects a default value on behalf of the framework.
> 
> 
> Thanks,
> 
> Clement Michaud
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message