mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 55887: Check task user before allowing a task to be launched on the agent.
Date Sat, 11 Mar 2017 08:44:56 GMT

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


Fix it, then Ship it!




Overall LGTM, just some nits.


src/slave/slave.hpp
Lines 1112 (patched)
<https://reviews.apache.org/r/55887/#comment240979>

    Yeah you are right, `executorId` doesn't necessarily come from `task.executor()`.
    
    But for symmetry, use `ExecutorInfo` instead?
    
    ```
    bool removePendingTask(
        const TaskInfo& task,
        const ExecutorInfo& executorInfo);
    ```



src/slave/slave.cpp
Lines 1808 (patched)
<https://reviews.apache.org/r/55887/#comment240980>

    s/list of//
    
    Nit-picking perhaps but we don't have to say "list" here and it's a map. :)



src/slave/slave.cpp
Line 1810 (original), 1811 (patched)
<https://reviews.apache.org/r/55887/#comment240969>

    I am not sure if we need to check the result of `removePendingTask` or add this log line.
It'll happen if it's killed and we have a log message for that in `Slave::killTask` so the
information here is not of much significance?



src/slave/slave.cpp
Lines 1812-1823 (original), 1813-1822 (patched)
<https://reviews.apache.org/r/55887/#comment240970>

    This comment to me is more misleading than helping. I don't understand why "Ideally we
would perform the following check here".
    
    AFAICT the comment in its original place means: normally when you erase a task from `pending`
you would check if the framework is fully cleaned up and if so remove it. However in the original
location of this comment, it's removing tasks from `pending` **not for the sake of cleaning
up** but rather because it's about to launch these tasks and store these in some other places,
**so you can't remove the framework**.
    
    Here the meaning above is lost entirely: we could've checked the condition within the
loop; there's no difference.
    
    We can check with @mpark (who added it) about how to improve the comment but for now let's
keep it at the original location: in `__run` where you remove tasks from `pending` before
you launch them.



src/slave/slave.cpp
Lines 1826-1827 (patched)
<https://reviews.apache.org/r/55887/#comment240981>

    Per the reason above, we probably don't need to keep repeating this reference in `_run`
(as the comment should be moved to `__run`).



src/slave/slave.cpp
Lines 1840 (patched)
<https://reviews.apache.org/r/55887/#comment240972>

    Althought it's safe in this case because we have checked, we are making an effort to switch
to `at()` in the codebase for read access so here let's use:
    
    ```
    framework->pending.at(executorId).contains(_task.task_id())
    ```



src/slave/slave.cpp
Lines 1852 (patched)
<https://reviews.apache.org/r/55887/#comment240983>

    I feel the name `removePendingTask` is self documenting enough?



src/slave/slave.cpp
Lines 1854 (patched)
<https://reviews.apache.org/r/55887/#comment240982>

    Ditto, no need to check or log here.



src/slave/slave.cpp
Lines 1898 (patched)
<https://reviews.apache.org/r/55887/#comment240984>

    Ditto, the method name is self-documenting.



src/slave/slave.cpp
Lines 1900 (patched)
<https://reviews.apache.org/r/55887/#comment240985>

    Ditto about log line.



src/slave/slave.cpp
Lines 1960 (patched)
<https://reviews.apache.org/r/55887/#comment240986>

    Could we add a TODO to acknowledge the issue of code duplication and say we should consider
refactoring it out?



src/slave/slave.cpp
Lines 2067-2070 (patched)
<https://reviews.apache.org/r/55887/#comment240977>

    For ExecutorInfo, we should look at `executorInfo` and not `_task.executor()` because
it might not be set?



src/slave/slave.cpp
Lines 2088 (patched)
<https://reviews.apache.org/r/55887/#comment240987>

    s/Failing/Ignoring running/?
    
    Failing may be implying `TASK_FAILED` when it's `TASK_ERROR`?



src/slave/slave.cpp
Lines 6883 (patched)
<https://reviews.apache.org/r/55887/#comment240988>

    I just realized it might be better to use an affirmative statement...
    
    ```
    // Return `true` if `task` was a pending task of this framework before the removal.
    ```



src/slave/slave.cpp
Lines 6889 (patched)
<https://reviews.apache.org/r/55887/#comment240978>

    Use `.at()`?
    
    Sorry my code example didn't update this.


- Jiang Yan Xu


On March 10, 2017, 2:15 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55887/
> -----------------------------------------------------------
> 
> (Updated March 10, 2017, 2:15 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Alexander Rojas, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6953
>     https://issues.apache.org/jira/browse/MESOS-6953
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added support for action `run_tasks` on the agent's flag `acl`. Based on
> the ACL configured for `run_tasks`, a task to be launched on the agent
> can be (dis)allowed to launch on the agent.
> If a task or task group cannot be launched due to failed authorization,
> a `TASK_ERROR` Status Update shall be sent with a reason code of
> `REASON_TASK_UNAUTHORIZED` or `REASON_TASK_GROUP_UNAUTHORIZED` as
> applicable.
> Note that in case of a task group, all tasks fail if any of the tasks
> within the task group encounter the authorization error.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 978edd6309dfbbde1058f9c44d5fac7083ff95fb 
>   src/slave/slave.cpp 2308d5bf1fef5e1a6458a3bb742a16935a127929 
> 
> 
> Diff: https://reviews.apache.org/r/55887/diff/10/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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