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 Mon, 27 Feb 2017 17:43:10 GMT

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




src/slave/slave.cpp (line 1706)
<https://reviews.apache.org/r/55887/#comment238967>

    The code below doesn't do "reporting", move to where it's done?



src/slave/slave.cpp (line 1717)
<https://reviews.apache.org/r/55887/#comment238973>

    w.r.t the `unschedule` here. This looks problematic: 
    
    1. we are invoking some asynchronous work (captured by `unschedule`) without checking
it's result but only to pass it along, then why not do it afterwards?
    2. we are not cleaning up the work that is initiated above (as well as some internal state
changes like `framework->pending`) if `authorizations` fails. We don't look at `unschedule`
at all for that case.
    3. semantically we should authorize the task before a whole bunch of work with side-effects
are initiated. It should probably be the first thing we do after some simple checking. We
should do the unscheduling after we have authorize the tasks.



src/slave/slave.cpp (lines 1747 - 1748)
<https://reviews.apache.org/r/55887/#comment238977>

    Move this to before we do it? At this point authorization is already done.



src/slave/slave.cpp (line 1759)
<https://reviews.apache.org/r/55887/#comment238982>

    There's 3rd state here, since we don't expect anyone to discard the future, we should
    
    ```
    CHECK(!future.isDiscarded());
    ```
    
    before assuming `future.failure()` works.



src/slave/slave.cpp (line 6258)
<https://reviews.apache.org/r/55887/#comment238984>

    authorize is a transitive verb so we need 
    
    s/authorized/authorized it/?



src/slave/slave.cpp (line 6266)
<https://reviews.apache.org/r/55887/#comment238985>

    In case it becomes an issue to construct `Framework` before `authorizeTask`, we actually
only need `FrameworkInfo` here.


- Jiang Yan Xu


On Feb. 20, 2017, 11:20 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55887/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2017, 11:20 p.m.)
> 
> 
> Review request for mesos, Adam B, 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_FAILED` 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 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp 7564e8d39530794131dbbc928fcbc59fb65ef471 
>   src/tests/master_slave_reconciliation_tests.cpp 1c7a3d686e2f924ad14c75fcab2ccafaab6d7b81

>   src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 
>   src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf 
>   src/tests/slave_tests.cpp 16bb14b170d724bd8424778a76de28b0efccc6ed 
> 
> Diff: https://reviews.apache.org/r/55887/diff/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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