mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 69938: Add resource decorator hook to implicitly allocate mandatory resources.
Date Mon, 11 Feb 2019 18:58:00 GMT

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



Nice patch, thanks Clément. Just a few minor comments below and we should be good to go.

Just as an aside, the current hook interfaces are rather inefficient (copies the entire task
info). If you're interested in improving the efficiency of the hook interface let me know
and we can tackle it separately.


include/mesos/hook.hpp
Lines 56-62 (patched)
<https://reviews.apache.org/r/69938/#comment298582>

    It's not clear from here what the semantics are, can you add:
    
    ```
    These new resources overwrite the previous ones in TaskInfo.
    ```



src/examples/test_hook_module.cpp
Lines 114-148 (patched)
<https://reviews.apache.org/r/69938/#comment298588>

    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;
      }
    ```



src/hook/manager.cpp
Lines 149 (patched)
<https://reviews.apache.org/r/69938/#comment298589>

    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?



src/master/master.cpp
Lines 4459-4461 (patched)
<https://reviews.apache.org/r/69938/#comment298590>

    Let's avoid the copy:
    
    ```
    *task.mutable_resources() =
      HookManager::masterLaunchTaskResourceDecorator(
          task,
          slave->totalResources));
    ```



src/master/master.cpp
Lines 4482-4484 (patched)
<https://reviews.apache.org/r/69938/#comment298591>

    Let's avoid the copy:
    
    ```
    *task.mutable_resources() =
      HookManager::masterLaunchTaskResourceDecorator(
          task,
          slave->totalResources));
    ```



src/tests/hook_tests.cpp
Lines 246-247 (patched)
<https://reviews.apache.org/r/69938/#comment298592>

    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?



src/tests/hook_tests.cpp
Lines 306 (patched)
<https://reviews.apache.org/r/69938/#comment298594>

    const Resources& (we put the reference next to the type)
    or
    const Resources
    or
    Resources
    
    would all be ok here



src/tests/hook_tests.cpp
Lines 307-309 (patched)
<https://reviews.apache.org/r/69938/#comment298593>

    Doesn't look like this is needed, the option being none will be caught below


- Benjamin Mahler


On Feb. 10, 2019, 11:16 a.m., Clement Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69938/
> -----------------------------------------------------------
> 
> (Updated Feb. 10, 2019, 11:16 a.m.)
> 
> 
> 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