Re: Review Request 62472: Fixed the ordering of Mesos containerizer isolators.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [62472]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 19, 2018, 9:22 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62472/
> ---
> 
> (Updated April 19, 2018, 9:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gilbert Song, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-7643
> https://issues.apache.org/jira/browse/MESOS-7643
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Historically, isolators were invoked in the order listed by
> the operator in the `--isolation` flag. This changed to an
> internal ordering in 6fb9c024, back to the operator ordering
> in af474a6fa, and then to an undefined ordering in 9642d3c67b.
> 
> This commit switches back to an internal ordering for all the built-in
> isolators. Custom isolators loaded in modules are run in operator order
> after all the built-in ones. The rationale for an internal ordering is
> expressed in MESOS-5581; basically we should not burden the operator
> with having to figure out how to make the order correct. In the case of
> custom isolators there's no way for us to know the correct ordering so
> we make an arbitrary choice.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6568126252a0a8ff5f6095bedae7828a9a1ba0fd 
> 
> 
> Diff: https://reviews.apache.org/r/62472/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> Posting this as an RFC. If reviewers are OK with the approach, I'll float it 
> on users@ and dev@ and update the documentation.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66311: Implement recovery of resource provider manager.

[Chun-Hung Hsiao]

> On April 19, 2018, 6 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Lines 193-203 (original), 194-203 (patched)
> > 
> >
> > How about moving this into `GenericRegistrarProcess::initialize()` to 
> > start recovery early?
> > 
> > If we do this and keep `recovered` (See below) then this function 
> > should return
> > ```
> > recovered->then([=] { return registry.get(); })
> > ```
> 
> Benjamin Bannier wrote:
> We already require users to `recover` registrars before being able to 
> perform operations against them (like for other registrars), so I am not 
> really sure I understand how what you suggest would help. Could you explain?

Oh what I mean is doing the following:
```
virtual void initialize() override {
  registry = ... // start the recovery.
}

Future recover() {
  return registry;
}
```

The second part of my comment above is just illustrating the code if we keep 
`registry` as an `Option` and `recovered` as a `Future`.


> On April 19, 2018, 6 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Line 205 (original), 205 (patched)
> > 
> >
> > Why do we need the `undiscardable` here? Could you add some comments?
> > 
> > Also, should we prevent the recovery being discarded due to a caller 
> > discarding an `apply` call? If yes then we should do
> > ```
> > return undiscardable(registry.get()).then(... ::_apply ...);
> > ```
> > in the following `apply()` function.
> 
> Benjamin Bannier wrote:
> I added a comment and also fixed below `apply` to use `recover()` which 
> would return the cached result, already guarded by `undiscarable`.

I was condering that we could do `undiscardable` in `apply` so that the caller 
can actually discard the recovery if they want to. Not sure if this is a valid 
use case though. Please drop it if you don't think so.


> On April 19, 2018, 6 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Line 216 (original), 216 (patched)
> > 
> >
> > The overall logic is correct, but it takes a bit of inference to know 
> > that overwriting `registry` with a new `Future` in an earlier `_apply` does 
> > not affect a later `_apply` that is chained with the overwritten `Future`. 
> > So it seems more readible to me if we keep the original 
> > `Option recovered` (or make it just a `Promise`), 
> > and chain `_apply` with `recovered` here.
> 
> Benjamin Bannier wrote:
> I replaced the direct access to `registry` with `recover` here which once 
> recovered would just serve a cached result. Does that look more reasonable to 
> you?

The thing that seems unintuitive to me is that `recover()` would return 
different futures at different times. May I ask what the reason to make 
`registry` a `Future` instead of an `Option`?


- Chun-Hung


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


On April 19, 2018, 12:29 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> ---
> 
> (Updated April 19, 2018, 12:29 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adjusts the control flow of the resource provider manager
> so that we can in the future make use of persisted resource provider
> information.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 92ef9aecb1e93d10f46e53984391558f9901a60b 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66311/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66310: Passed on registrar when constructing resource provider manager.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On April 19, 2018, 12:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66310/
> ---
> 
> (Updated April 19, 2018, 12:28 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to support recovering resource provider manager information
> in the future, we need to adjust the construction of the manager to be
> able to consume a registrar.
> 
> This patch lays the groundwork by adjusting interfaces and their
> usage; we will make use of the passed on information in a following
> patch.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2 
>   src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66310/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66308: Delayed construction of the agent's resource provider manager.

[Chun-Hung Hsiao]

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




src/slave/slave.cpp
Lines 8849 (patched)


How about adding a comment to explain why this might be called twice and we 
should ignore the subsequent call?


- Chun-Hung Hsiao


On April 19, 2018, 9:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66308/
> ---
> 
> (Updated April 19, 2018, 9:19 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By delaying the construction of the agent's resource provider manager
> we prepare for a following patch introducing a dependency on the
> resource provider manager on the agent's ID.
> 
> Depending on whether the agent was able to recover an agent ID from
> its log or still needs to obtain on in a first registration with the
> master, we can only construct the resource provider manager after
> different points in the initialization of the agent. To capture the
> common code we introduce a helper function performing the necessary
> steps.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66308/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66526: Renamed resource provider `AgentRegistrar` to `GenericRegistrar`.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On April 19, 2018, 12:29 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66526/
> ---
> 
> (Updated April 19, 2018, 12:29 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This registrar and its matching process can work on generic storage
> and is currently used to work on an agent's persist store.
> 
> Its name should not be tied the agent, so we rename the registrar in
> this patch.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 92ef9aecb1e93d10f46e53984391558f9901a60b 
> 
> 
> Diff: https://reviews.apache.org/r/66526/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66309: Externalized creation of resource provider manager backing storage.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On April 19, 2018, 9:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> ---
> 
> (Updated April 19, 2018, 9:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
> 
> Being able to explicitly inject the used storage simplifies testing.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 92ef9aecb1e93d10f46e53984391558f9901a60b 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66309/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66309: Externalized creation of resource provider manager backing storage.

[Chun-Hung Hsiao]

> On April 18, 2018, 9:31 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Lines 71-73 (original), 73-75 (patched)
> > 
> >
> > I was wondering that, since we have a generic storage-backed registrar, 
> > could we also use this for the master's RP registrar and remove 
> > `MasterRegistrar`?
> 
> Benjamin Bannier wrote:
> The master registrar is different in that it is already recovered and we 
> need to use it differently when retrieving its recovered state via above 
> interface, see https://reviews.apache.org/r/66311//

I get that we need this interface to retrieve data from the master's registrar. 
I was wondering if it is necessary or has a lot of advantages to store the RP 
manager registry in the master's registrar.


- Chun-Hung


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


On April 19, 2018, 9:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> ---
> 
> (Updated April 19, 2018, 9:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
> 
> Being able to explicitly inject the used storage simplifies testing.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 92ef9aecb1e93d10f46e53984391558f9901a60b 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66309/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66163: Built storage local resource provider with CMake.

[Chun-Hung Hsiao]

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

(Updated April 20, 2018, 2:34 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
Joseph Wu.


Changes
---

Enabled Windows Build.


Bugs: MESOS-8698
https://issues.apache.org/jira/browse/MESOS-8698


Repository: mesos


Description
---

This patch adds CMake rules for compiling necessary source code for
building storage local resource provider and related tests.


Diffs (updated)
-

  cmake/CompilationConfigure.cmake 173089a1ebf80a6288431075770a807235524ac1 
  src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
  src/examples/CMakeLists.txt e85eed607715d2b3ee596c3c0b51f16682491900 
  src/resource_provider/storage/CMakeLists.txt PRE-CREATION 
  src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 


Diff: https://reviews.apache.org/r/66163/diff/5/

Changes: https://reviews.apache.org/r/66163/diff/4-5/


Testing
---

`sudo make check` with CMake


Thanks,

Chun-Hung Hsiao

Re: Review Request 61118: Building gRPC support in libprocess with CMake.

[Chun-Hung Hsiao]

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

(Updated April 20, 2018, 2:31 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
Joseph Wu.


Changes
---

Enabled Windows build.


Bugs: MESOS-7881
https://issues.apache.org/jira/browse/MESOS-7881


Repository: mesos


Description
---

This patch enables CMake to build code for gRPC support in libprocess
along with tests.


Diffs (updated)
-

  3rdparty/libprocess/src/CMakeLists.txt 
1eca50a4d513b2fead111e453565edff0a3ee497 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
d624402bc9efb43a130a2afdf27cfc1aa69010e4 


Diff: https://reviews.apache.org/r/61118/diff/6/

Changes: https://reviews.apache.org/r/61118/diff/5-6/


Testing (updated)
---

`cmake .. -DENABLE_GRPC=1` and then `cmake --build . --target check` on Linux 
and Windows.


Thanks,

Chun-Hung Hsiao

Re: Review Request 61096: Building gRPC with CMake.

[Chun-Hung Hsiao]

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

(Updated April 20, 2018, 2:28 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
Joseph Wu.


Changes
---

Enabled Windows build.


Bugs: MESOS-7881
https://issues.apache.org/jira/browse/MESOS-7881


Repository: mesos


Description
---

This patch adds an `ENABLE_GRPC` option and builds the bundled gRPC
3rdparty library in CMake.


Diffs (updated)
-

  3rdparty/CMakeLists.txt e6bb4076e630d78e73a82a93b1f64b3bc5d859b9 
  3rdparty/cmake/Versions.cmake 33577cc86d3ef31e129b8ea55d44d3fee02d4a30 
  cmake/CompilationConfigure.cmake 173089a1ebf80a6288431075770a807235524ac1 
  src/cmake/MesosProtobuf.cmake dde034fe4d4251dd7c7397d422136bfa3c9c3ebc 


Diff: https://reviews.apache.org/r/61096/diff/8/

Changes: https://reviews.apache.org/r/61096/diff/7-8/


Testing (updated)
---

`cmake .. -DENABLE_GRPC=1` on both Linux and Windows.


Thanks,

Chun-Hung Hsiao

Review Request 66727: Cherry-picked gRPC PR15128 for Windows compilation.

[Chun-Hung Hsiao]

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

Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph Wu.


Bugs: MESOS-7881
https://issues.apache.org/jira/browse/MESOS-7881


Repository: mesos


Description
---

We added a gRPC patch file that includes
https://github.com/grpc/grpc/pull/15128 to fix Windows compilation, and
updated `grpc.md` to match the format of `protobuf.md`.


Diffs
-

  3rdparty/grpc-1.10.0.patch PRE-CREATION 
  3rdparty/grpc-1.10.0.readme d33a80881f9660e95cf3c2b032b3053d5ed9c7ff 
  3rdparty/grpc.md PRE-CREATION 


Diff: https://reviews.apache.org/r/66727/diff/1/


Testing
---

N/A


Thanks,

Chun-Hung Hsiao

Review Request 66726: Made CMake's `FindZLIB` module be able to find Zlib on Windows.

[Chun-Hung Hsiao]

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

Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph Wu.


Bugs: MESOS-7881
https://issues.apache.org/jira/browse/MESOS-7881


Repository: mesos


Description
---

This patch "install" ZLib in `build/3rdparty/zlib-1.2.8/src/zlib-1.2.8`,
so we can set `ZLIB_ROOT` to the directory to make `FindZLIB` be able to
find the include directory containing both `zlib.h` and `zconf.h`, as
well as to find the library directory containing the library files.


Diffs
-

  3rdparty/CMakeLists.txt e6bb4076e630d78e73a82a93b1f64b3bc5d859b9 


Diff: https://reviews.apache.org/r/66726/diff/1/


Testing
---

cmake --build .


Thanks,

Chun-Hung Hsiao

Re: Review Request 66696: Updated documentation for operation feedback.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66696]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 19, 2018, 11:12 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66696/
> ---
> 
> (Updated April 19, 2018, 11:12 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated documentation for operation feedback.
> 
> 
> Diffs
> -
> 
>   docs/csi.md 2d96ee754b3fdcfb18a7516f30abf0086fa75137 
>   docs/scheduler-http-api.md 3929c33781a152428338c4cdaf7dbc47da7c875e 
> 
> 
> Diff: https://reviews.apache.org/r/66696/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66163: Built storage local resource provider with CMake.

[Chun-Hung Hsiao]

> On April 12, 2018, 5:06 p.m., Andrew Schwartzmeyer wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 387-390 (patched)
> > 
> >
> > Do you know which targets actually require `ENABLE_GRPC` set as a 
> > pre-processor definition? Since it's new code being added, we should be 
> > able to resovle this TODO before committing.
> 
> Chun-Hung Hsiao wrote:
> For now it's just `src/resource_provider/local.cpp`. What's the best way 
> to achieve this? IMHO it's not that bad to make this global. ;)
> 
> Andrew Schwartzmeyer wrote:
> Change
> ```
> target_compile_definitions(mesos PUBLIC USE_CMAKE_BUILD_CONFIG)
> ```
> to
> ```
> target_compile_definitions(
>   mesos
>   PUBLIC USE_CMAKE_BUILD_CONFIG
>   PRIVATE ENABLE_GRPC)
> ```
> 
> But yeah, we still have `add_definitions(-DUSE_SSL_SOCKET=1)`, so this 
> can be a `TODO`.

Actually I'm not sure if having an explicit list of targets that the definition 
takes effects would reduce the cost of code maintanance or it would increase 
the cost. It seems to me that people that are unfamiliar with this list could 
easily run into a case where a definition doesn't take effects on a new target 
and they will spend a lot of time debugging it.


- Chun-Hung


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


On April 17, 2018, 3:20 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66163/
> ---
> 
> (Updated April 17, 2018, 3:20 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8698
> https://issues.apache.org/jira/browse/MESOS-8698
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds CMake rules for compiling necessary source code for
> building storage local resource provider and related tests.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 08d154e6e4b718b6059afb70e0b0235d1725d72d 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/examples/CMakeLists.txt e85eed607715d2b3ee596c3c0b51f16682491900 
>   src/resource_provider/storage/CMakeLists.txt PRE-CREATION 
>   src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
> 
> 
> Diff: https://reviews.apache.org/r/66163/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66227: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operator API.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 66049.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66049`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66227

Relevant logs:

- 
[apply-review-66049-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66227/logs/apply-review-66049-stdout.log):

```
error: patch failed: include/mesos/mesos.proto:1917
error: include/mesos/mesos.proto: patch does not apply
error: patch failed: include/mesos/v1/mesos.proto:1909
error: include/mesos/v1/mesos.proto: patch does not apply
```

- Mesos Reviewbot Windows


On April 11, 2018, 2:21 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66227/
> ---
> 
> (Updated April 11, 2018, 2:21 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-8747
> https://issues.apache.org/jira/browse/MESOS-8747
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operator API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 
> 
> 
> Diff: https://reviews.apache.org/r/66227/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66725: Improved error message in the fetcher.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66725 was successfully built and tested.

Reviews applied: `['66725']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66725

- Mesos Reviewbot Windows


On April 19, 2018, 4:08 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66725/
> ---
> 
> (Updated April 19, 2018, 4:08 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Included error message from `mkdir` when
> returning error.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 8a8d7c3311c6d282648940bc4440f4c6d9d5e918 
> 
> 
> Diff: https://reviews.apache.org/r/66725/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66708: Refactored sending a TASK_DROPPED status update.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On April 20, 2018, 2:09 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66708/
> ---
> 
> (Updated April 20, 2018, 2:09 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored how the agent sends a TASK_DROPPED status update to
> remove code duplication. This also makes the agent consistently
> send the executor ID in all these status updates.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66708/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66705: Propagated executor sandbox creation errors.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On April 20, 2018, 1:52 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66705/
> ---
> 
> (Updated April 20, 2018, 1:52 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than crashing if the agent fails to create the executor
> directory, propagate the error to the caller so that it can
> handle it appropriately.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp fe5ab9e7f96d69069406e2714ab676a5bb070534 
>   src/slave/paths.cpp 690bfe3587e6d728ab6eb712a913de23c4abe353 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/paths_tests.cpp dc765ed9db7a8ac7ca0bcb4af5cf353547ba881f 
> 
> 
> Diff: https://reviews.apache.org/r/66705/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66621`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621

Relevant logs:

- 
[apply-review-66621-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621/logs/apply-review-66621-stderr.log):

```
Traceback (most recent call last):
  File ".\support\apply-reviews.py", line 447, in 
main()
  File ".\support\apply-reviews.py", line 442, in main
reviewboard(options)
  File ".\support\apply-reviews.py", line 432, in reviewboard
apply_review(options)
  File ".\support\apply-reviews.py", line 163, in apply_review
commit_patch(options)
  File ".\support\apply-reviews.py", line 292, in commit_patch
shell(cmd, options['dry_run'])
  File ".\support\apply-reviews.py", line 147, in shell
error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
  File "C:\Program Files\Python27\lib\subprocess.py", line 168, in call
return Popen(*popenargs, **kwargs).wait()
  File "C:\Program Files\Python27\lib\subprocess.py", line 390, in __init__
errread, errwrite)
  File "C:\Program Files\Python27\lib\subprocess.py", line 610, in 
_execute_child
args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 
23: ordinal not in range(128)
```

- Mesos Reviewbot Windows


On April 19, 2018, 4:10 p.m., Clément Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> ---
> 
> (Updated April 19, 2018, 4:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Till Toenshoff.
> 
> 
> Bugs: MESOS-8788
> https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for alg RS256 to JWT library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/jwt.hpp 
> 768cbf6fa91537ff9f45f236f4033097c5cea959 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> b7cc31c33fd35c93754407f8b350eeb993177f1d 
>   3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 4d3727daf53ec62a19255da5a9804d342e770ec2 
>   3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp 
> eb36a9aed3b11208c7cdc6f20b5347f46821a207 
> 
> 
> Diff: https://reviews.apache.org/r/66621/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> I added the same tests than the ones for HS256 (i.e., validation in following 
> cases: bad header, bad payload, unknown alg, unsupported alg, valid token 
> etc.. and creation of a valid token). I also added a test to verify that the 
> validation of a RS256 token fails when using the wrong public key.
> 
> 
> Thanks,
> 
> Clément Michaud
> 
>

Re: Review Request 66683: Updated address field of new CLI config to accept URLs.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66683]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 18, 2018, 6:14 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66683/
> ---
> 
> (Updated April 18, 2018, 6:14 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-8025
> https://issues.apache.org/jira/browse/MESOS-8025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated address field of new CLI config to accept URLs.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/http.py 03d6031cb3273575f41d4d06d9a409f74488a16b 
>   src/python/cli_new/lib/cli/util.py 307b22293a9c7199ad7088dfd0db6dff83a08ac8 
> 
> 
> Diff: https://reviews.apache.org/r/66683/diff/2/
> 
> 
> Testing
> ---
> 
> Ran `mesos agent list` successfully with config:
> ```
> [master]
>   address = "http://.com:5061"
> ```
> On the server providing `http://.com` I had 
> a Mesos master running with `bash mesos-master.sh --port='5061' 
> --work_dir='/tmp/master1' --log_dir='/tmp/master1-log' --registry=in_memory`.
> 
> Also ran `mesos-cli-tests` successfully.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 66227: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operator API.

[Zhitao Li]

> On April 17, 2018, 6:25 p.m., Chun-Hung Hsiao wrote:
> > src/tests/api_tests.cpp
> > Lines 3688 (patched)
> > 
> >
> > Let's try to split the test into two so each test verifies one 
> > operation.
> > 
> > Also, what's the reason of disabling the test on Windows?
> > 
> > And do you think it is reasonable to have tests that leave `agent_id` 
> > unset, to expose the code path of validating the existence of `agent_id`?

Persistent volume creation is not working on Windows yet.

I'm okay to add some check about the validation of mising `agent_id`.


> On April 17, 2018, 6:25 p.m., Chun-Hung Hsiao wrote:
> > src/tests/api_tests.cpp
> > Lines 3766 (patched)
> > 
> >
> > Not sure if this is necessary. Could you justify the need of this 
> > synchronization?

I'll see whether this is necessary. In the other test, this is necessary if we 
rely on offers to reflect operation result: to work around possible race 
condition between calling `allocation->updateAllocation()` and clock::advance.


- Zhitao


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


On April 11, 2018, 2:21 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66227/
> ---
> 
> (Updated April 11, 2018, 2:21 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-8747
> https://issues.apache.org/jira/browse/MESOS-8747
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operator API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 
> 
> 
> Diff: https://reviews.apache.org/r/66227/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66439: Windows: Made `protobuf::write()` use CRT file descriptor explicitly.

[Andrew Schwartzmeyer]

> On April 19, 2018, 2:07 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 83 (original), 87-95 (patched)
> > 
> >
> > I'm a bit surprised this doesn't cause problems with tests like 
> > `ProtobufIOTest.Basic` and `OperationStatusUpdateManagerTests`.
> > 
> > Since you're dup-ing the FDs for each write, if the caller passes in 
> > the same FD for multiple calls, the original FD will retain its offset 
> > (near the beginning of the file) and should repeatedly overwrite the data 
> > each time `protobuf::write` is called.
> 
> Akash Gupta wrote:
> `dup` creates another file *descriptor* to the same file *description*, 
> which stores the offset. So, if you read from a dup'd handle, it changes the 
> offset of the original handle. The behavior is the same on Windows and Linux.

Yup, thanks Akash.

`man 2 dup`:
>  After a successful return, the old and new file descriptors may be used 
> interchangeably.  They refer to the same open file description (see open(2)) 
> and thus share file offset and file status flags; for example, if the file 
> offset is modified by using lseek(2) on one of the file descriptors,  the  
> offset  is  also changed for the other.

`DuplicateHandle` on MSDN:
> The duplicate handle refers to the same object as the original handle. 
> Therefore, any changes to the object are reflected through both handles. For 
> example, if you duplicate a file handle, the current file position is always 
> the same for both handles.
This explains why the tests are passing ;)


- Andrew


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


On April 6, 2018, 4:16 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66439/
> ---
> 
> (Updated April 6, 2018, 4:16 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Bugs: MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is another edge case where a third-party library (protobuf)
> requires a CRT integer file descriptor. Thus we duplicate the `int_fd`
> and then explicitly allocate via `crt()`, which requires that we also
> manually close it via `_close()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 2fa5072e3c62c487da0dccffdd38d2fa1a615dc0 
> 
> 
> Diff: https://reviews.apache.org/r/66439/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66568: Dropped combined operations with GROW and SHRINK volumes.

[Greg Mann]

> On April 19, 2018, 9:58 p.m., Greg Mann wrote:
> > src/master/master.cpp
> > Line 4512 (original), 4515 (patched)
> > 
> >
> > Nit: there's an extra space after the comma.

after the colon I mean :)


- Greg


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


On April 17, 2018, 10:53 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66568/
> ---
> 
> (Updated April 17, 2018, 10:53 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two operations are intended to be non-speculative eventually but
> are implemented as speculative right now. To avoid frameworks opt-in to
> dangerous behavior, we require that accept can only contain one
> `GROW_VOLUME` or `SHRINK_VOLUME` and no other operations.
> 
> This is implemented by reuse existing code which already drops `LAUNCH`
> or `LAUNCH_GROUP` with proper reason and message.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66568/diff/3/
> 
> 
> Testing
> ---
> 
> Behavior tested in https://reviews.apache.org/r/66569/.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66219: Added helper functions to create grow and shrink volume in test.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On April 11, 2018, 9:18 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66219/
> ---
> 
> (Updated April 11, 2018, 9:18 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper functions to create grow and shrink volume in test.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
> 
> 
> Diff: https://reviews.apache.org/r/66219/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

[Clément Michaud]

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

(Updated avr. 19, 2018, 11:10 après-midi)


Review request for mesos, Alexander Rojas and Till Toenshoff.


Changes
---

Fixed all issues reported by Alexander


Bugs: MESOS-8788
https://issues.apache.org/jira/browse/MESOS-8788


Repository: mesos


Description
---

Add support for alg RS256 to JWT library.


Diffs (updated)
-

  3rdparty/libprocess/include/process/jwt.hpp 
768cbf6fa91537ff9f45f236f4033097c5cea959 
  3rdparty/libprocess/include/process/ssl/utilities.hpp 
b7cc31c33fd35c93754407f8b350eeb993177f1d 
  3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
  3rdparty/libprocess/src/ssl/utilities.cpp 
4d3727daf53ec62a19255da5a9804d342e770ec2 
  3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/jwt_tests.cpp 
eb36a9aed3b11208c7cdc6f20b5347f46821a207 


Diff: https://reviews.apache.org/r/66621/diff/3/

Changes: https://reviews.apache.org/r/66621/diff/2-3/


Testing
---

make check

I added the same tests than the ones for HS256 (i.e., validation in following 
cases: bad header, bad payload, unknown alg, unsupported alg, valid token etc.. 
and creation of a valid token). I also added a test to verify that the 
validation of a RS256 token fails when using the wrong public key.


Thanks,

Clément Michaud

Review Request 66725: Improved error message in the fetcher.

[Meng Zhu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

Included error message from `mkdir` when
returning error.


Diffs
-

  src/launcher/fetcher.cpp 8a8d7c3311c6d282648940bc4440f4c6d9d5e918 


Diff: https://reviews.apache.org/r/66725/diff/1/


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 62472: Fixed the ordering of Mesos containerizer isolators.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 62472 was successfully built and tested.

Reviews applied: `['62472']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62472

- Mesos Reviewbot Windows


On April 19, 2018, 2:22 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62472/
> ---
> 
> (Updated April 19, 2018, 2:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gilbert Song, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-7643
> https://issues.apache.org/jira/browse/MESOS-7643
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Historically, isolators were invoked in the order listed by
> the operator in the `--isolation` flag. This changed to an
> internal ordering in 6fb9c024, back to the operator ordering
> in af474a6fa, and then to an undefined ordering in 9642d3c67b.
> 
> This commit switches back to an internal ordering for all the built-in
> isolators. Custom isolators loaded in modules are run in operator order
> after all the built-in ones. The rationale for an internal ordering is
> expressed in MESOS-5581; basically we should not burden the operator
> with having to figure out how to make the order correct. In the case of
> custom isolators there's no way for us to know the correct ordering so
> we make an arbitrary choice.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6568126252a0a8ff5f6095bedae7828a9a1ba0fd 
> 
> 
> Diff: https://reviews.apache.org/r/62472/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> Posting this as an RFC. If reviewers are OK with the approach, I'll float it 
> on users@ and dev@ and update the documentation.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66431: Windows: Fixed `os::read()` to use `ReadFile()`.

[Andrew Schwartzmeyer]

> On April 18, 2018, 11:55 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/windows/read.hpp
> > Lines 39-44 (patched)
> > 
> >
> > So you're saying that `ReadFile` reads all the existing data on the 
> > handle prior to returning False?It would feel safer if we verify this 
> > with a unit test.

I think that `TEST_F(SubprocessTest, PipeOutputToFileDescriptor)` adequately 
ensures this is working as inspected. Let me know what you think...


- Andrew


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


On April 9, 2018, 3:53 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66431/
> ---
> 
> (Updated April 9, 2018, 3:53 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Bugs: MESOS-8676
> https://issues.apache.org/jira/browse/MESOS-8676
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This can eventually support overlapped I/O.
> 
> The Windows API `ReadFile()` returns an error if the pipe is broken,
> where `_read()` did not, but this is not an error for us as the data
> is still read correctly. So we ignore it.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/read.hpp 
> 49878e499209fa2f91fede0ebdabb8f088a9d018 
>   3rdparty/stout/include/stout/os/windows/read.hpp 
> 8047ad590fcc46d3ec46b551472d8c518ae49cc1 
> 
> 
> Diff: https://reviews.apache.org/r/66431/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On April 19, 2018, 4:28 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 19, 2018, 4:28 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66568: Dropped combined operations with GROW and SHRINK volumes.

[Greg Mann]

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




src/master/master.cpp
Line 4512 (original), 4515 (patched)


Nit: there's an extra space after the comma.



src/master/master.cpp
Lines 4525 (patched)


Does it make sense to use `TaskStatus::REASON_RESOURCES_UNKNOWN` here?



src/master/master.cpp
Lines 4528 (patched)


s/operation/operations/



src/master/master.cpp
Lines 4518-4522 (original), 4543-4547 (patched)


Hmmm... it's not clear to me why we don't drop other operations here as 
well. At the moment, this just means that we lose some logs, but once we land 
Gaston's patch (https://reviews.apache.org/r/66679/) dropping non-launch 
operations will also send operation feedback.

Could you update this block to drop non-launch operations?


- Greg Mann


On April 17, 2018, 10:53 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66568/
> ---
> 
> (Updated April 17, 2018, 10:53 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two operations are intended to be non-speculative eventually but
> are implemented as speculative right now. To avoid frameworks opt-in to
> dangerous behavior, we require that accept can only contain one
> `GROW_VOLUME` or `SHRINK_VOLUME` and no other operations.
> 
> This is implemented by reuse existing code which already drops `LAUNCH`
> or `LAUNCH_GROUP` with proper reason and message.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66568/diff/3/
> 
> 
> Testing
> ---
> 
> Behavior tested in https://reviews.apache.org/r/66569/.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66531: Added new authorization for `UpdateVolume`.

[Greg Mann]

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


Fix it, then Ship it!





include/mesos/authorizer/acls.proto
Lines 118 (patched)


Let's change this to `ResizeVolume`. If we want to generalize to other 
types of update in the future, we can do so.


- Greg Mann


On April 19, 2018, 7:57 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66531/
> ---
> 
> (Updated April 19, 2018, 7:57 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new authorization action is modelled after `CreateVolume`, and will
> be shared by both `GROW_VOLUME` and `SHRINK_VOLUME` operations and
> corresponding operator APIs.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 8ef33210644e7d2924b402a3158b1b38c1fdb464 
>   include/mesos/authorizer/authorizer.proto 
> 1508c01130013d74ed2b2574a2428f38e3d2c064 
>   src/authorizer/local/authorizer.cpp 
> c098ba9ded1b29a2e079cf09ab80b61f6fa4af05 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66531/diff/2/
> 
> 
> Testing
> ---
> 
> Manually created ACL and verified that untrusted principals will not allow to 
> grow/shrink volumes.
> Also created unit test in next patch.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66531: Added new authorization for `UpdateVolume`.

[Greg Mann]

> On April 17, 2018, 11:14 p.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 4924-4925 (original), 4980-4981 (patched)
> > 
> >
> > You need to finish implementing authorization here, and below for 
> > SHRINK_VOLUME.
> 
> Zhitao Li wrote:
> Will fix, but I have question/comments on operation authorization 
> handling in `Master::_accept`:
> 1. We don't really check that `authorizations.size() == 
> operations.size()`. My feeling is that this at least leaves space for error 
> when devs adding new operations;
> 2. we allow part of the operations which are authorized to proceed while 
> drop unauthorized operations. I'm not convinced it's either safe from 
> security perspective or clear from user perspective;
> 3. I see some comments about `We may want to retry this failed 
> authorization request rather than dropping it immediately.`. If authorization 
> result is "Unavaiable" rather than "Rejected", should that be reflected from 
> result type?
> 
> Zhitao Li wrote:
> Reading this a lot, I do not see we can safely retry failed authorization 
> when operations are chained. In fact, I feel that we should look into 
> refactoring the large code blob and extract authorization handling into its 
> own block (rather than scattered through the `switch/case`).
> 
> That should be a separate patch though.

Agreed, the authorization handling here would definitely benefit from a 
refactor.


- Greg


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


On April 19, 2018, 7:57 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66531/
> ---
> 
> (Updated April 19, 2018, 7:57 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new authorization action is modelled after `CreateVolume`, and will
> be shared by both `GROW_VOLUME` and `SHRINK_VOLUME` operations and
> corresponding operator APIs.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 8ef33210644e7d2924b402a3158b1b38c1fdb464 
>   include/mesos/authorizer/authorizer.proto 
> 1508c01130013d74ed2b2574a2428f38e3d2c064 
>   src/authorizer/local/authorizer.cpp 
> c098ba9ded1b29a2e079cf09ab80b61f6fa4af05 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66531/diff/2/
> 
> 
> Testing
> ---
> 
> Manually created ACL and verified that untrusted principals will not allow to 
> grow/shrink volumes.
> Also created unit test in next patch.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66532: Added test for authorization actions for `UPDATE_VOLUME`.

[Greg Mann]

> On April 17, 2018, 11:24 p.m., Greg Mann wrote:
> > src/tests/authorization_tests.cpp
> > Line 1979 (original), 1979 (patched)
> > 
> >
> > Could you also add an end-to-end test of authorization for these 
> > operations?
> 
> Zhitao Li wrote:
> Sure. Do you have an example or existing test to add this?

There are some similar tests in both 'persistent_volume_tests.cpp' as well as 
'master_authorization_tests.cpp'.

Tests which do something very similar in the persistent volume tests are 
'PersistentVolumeTest.BadACLDropCreateAndDestroy' and 
'PersistentVolumeTest.GoodACLCreateThenDestroy'. It would be good to verify 
both the successful and failed authorization cases.


- Greg


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


On April 19, 2018, 8:01 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated April 19, 2018, 8:01 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `UPDATE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 62472: Fixed the ordering of Mesos containerizer isolators.

[James Peach]

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

(Updated April 19, 2018, 9:22 p.m.)


Review request for mesos, Benjamin Mahler, Gilbert Song, Jie Yu, and Kevin 
Klues.


Bugs: MESOS-7643
https://issues.apache.org/jira/browse/MESOS-7643


Repository: mesos


Description
---

Historically, isolators were invoked in the order listed by
the operator in the `--isolation` flag. This changed to an
internal ordering in 6fb9c024, back to the operator ordering
in af474a6fa, and then to an undefined ordering in 9642d3c67b.

This commit switches back to an internal ordering for all the built-in
isolators. Custom isolators loaded in modules are run in operator order
after all the built-in ones. The rationale for an internal ordering is
expressed in MESOS-5581; basically we should not burden the operator
with having to figure out how to make the order correct. In the case of
custom isolators there's no way for us to know the correct ordering so
we make an arbitrary choice.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
6568126252a0a8ff5f6095bedae7828a9a1ba0fd 


Diff: https://reviews.apache.org/r/62472/diff/2/

Changes: https://reviews.apache.org/r/62472/diff/1-2/


Testing
---

make check (Fedora 26)

Posting this as an RFC. If reviewers are OK with the approach, I'll float it on 
users@ and dev@ and update the documentation.


Thanks,

James Peach

Re: Review Request 66439: Windows: Made `protobuf::write()` use CRT file descriptor explicitly.

[Joseph Wu]

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




3rdparty/stout/include/stout/protobuf.hpp
Line 83 (original), 87-95 (patched)


I'm a bit surprised this doesn't cause problems with tests like 
`ProtobufIOTest.Basic` and `OperationStatusUpdateManagerTests`.

Since you're dup-ing the FDs for each write, if the caller passes in the 
same FD for multiple calls, the original FD will retain its offset (near the 
beginning of the file) and should repeatedly overwrite the data each time 
`protobuf::write` is called.


- Joseph Wu


On April 6, 2018, 4:16 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66439/
> ---
> 
> (Updated April 6, 2018, 4:16 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Bugs: MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is another edge case where a third-party library (protobuf)
> requires a CRT integer file descriptor. Thus we duplicate the `int_fd`
> and then explicitly allocate via `crt()`, which requires that we also
> manually close it via `_close()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 2fa5072e3c62c487da0dccffdd38d2fa1a615dc0 
> 
> 
> Diff: https://reviews.apache.org/r/66439/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66568: Dropped combined operations with GROW and SHRINK volumes.

[Chun-Hung Hsiao]

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




src/master/master.cpp
Lines 4511 (patched)


How about `Option dropMessage;` and have `CHECK_SOME(dropMessage);` 
after Line 4537?


- Chun-Hung Hsiao


On April 17, 2018, 10:53 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66568/
> ---
> 
> (Updated April 17, 2018, 10:53 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two operations are intended to be non-speculative eventually but
> are implemented as speculative right now. To avoid frameworks opt-in to
> dangerous behavior, we require that accept can only contain one
> `GROW_VOLUME` or `SHRINK_VOLUME` and no other operations.
> 
> This is implemented by reuse existing code which already drops `LAUNCH`
> or `LAUNCH_GROUP` with proper reason and message.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66568/diff/3/
> 
> 
> Testing
> ---
> 
> Behavior tested in https://reviews.apache.org/r/66569/.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66438: Windows: Made `libevent` use CRT file descriptor explicitly.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On April 4, 2018, 12:19 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66438/
> ---
> 
> (Updated April 4, 2018, 12:19 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Bugs: MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is an edge case where a third-party library (libevent) requires a
> CRT integer file descriptor. Thus we duplicate the `int_fd` and then
> explicitly allocate via `crt()`, which requires that we also manually
> close it via `_close()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 4de161dbf9198e9c74b1e80838b8a5d52006a562 
> 
> 
> Diff: https://reviews.apache.org/r/66438/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66708: Refactored sending a TASK_DROPPED status update.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66704, 66705, 66706, 66707, 66708]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 19, 2018, 6:09 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66708/
> ---
> 
> (Updated April 19, 2018, 6:09 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored how the agent sends a TASK_DROPPED status update to
> remove code duplication. This also makes the agent consistently
> send the executor ID in all these status updates.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66708/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66433: Windows: Made `net::download()` use CRT file descriptor explicitly.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On April 4, 2018, 12:18 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66433/
> ---
> 
> (Updated April 4, 2018, 12:18 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> and Michael Park.
> 
> 
> Bugs: MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is an edge case where a third-party library (libcurl) requires a
> CRT integer file descriptor. Thus we explicitly allocate one via
> `crt()`, which requires that we also manually close it via `_close()`,
> not `os::close()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/net.hpp 
> d2992c05b221ea90dae1c06d27753932f7411925 
> 
> 
> Diff: https://reviews.apache.org/r/66433/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66696: Updated documentation for operation feedback.

[Mesos Reviewbot Windows]

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



FAIL: Mesos binaries failed to build.

Reviews applied: `['66696']`

Failed command: `cmake.exe --build . --config Release -- /maxcpucount`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66696

Relevant logs:

- 
[mesos-binaries-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66696/logs/mesos-binaries-cmake-stdout.log):

```
   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\src\docker\mesos-docker-executor.vcxproj" (default 
target) (22) ->
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\exit.hpp(69): warning 
C4722: '__Exit::~__Exit': destructor never returns, potential memory leak 
[D:\DCOS\mesos\src\docker\mesos-docker-executor.vcxproj]


   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\src\launcher\mesos-default-executor.vcxproj" (default 
target) (24) ->
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\exit.hpp(69): warning 
C4722: '__Exit::~__Exit': destructor never returns, potential memory leak 
[D:\DCOS\mesos\src\launcher\mesos-default-executor.vcxproj]


   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\src\master\mesos-master.vcxproj" (default target) (34) ->
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\exit.hpp(69): warning 
C4722: '__Exit::~__Exit': destructor never returns, potential memory leak 
[D:\DCOS\mesos\src\master\mesos-master.vcxproj]


   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\src\slave\mesos-agent.vcxproj" (default target) (25) ->
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\exit.hpp(69): warning 
C4722: '__Exit::~__Exit': destructor never returns, potential memory leak 
[D:\DCOS\mesos\src\slave\mesos-agent.vcxproj]


   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\3rdparty\jemalloc-5.0.1.vcxproj" (default target) (16) ->
   (CustomBuild target) -> 
 C:\Program Files (x86)\Microsoft Visual 
Studio\2017\Community\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(171,5):
 error MSB6006: "cmd.exe" exited with code 9009. 
[D:\DCOS\mesos\3rdparty\jemalloc-5.0.1.vcxproj]

358 Warning(s)
1 Error(s)

Time Elapsed 00:23:20.73
```

- Mesos Reviewbot Windows


On April 19, 2018, 6:12 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66696/
> ---
> 
> (Updated April 19, 2018, 6:12 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated documentation for operation feedback.
> 
> 
> Diffs
> -
> 
>   docs/csi.md 2d96ee754b3fdcfb18a7516f30abf0086fa75137 
>   docs/scheduler-http-api.md 3929c33781a152428338c4cdaf7dbc47da7c875e 
> 
> 
> Diff: https://reviews.apache.org/r/66696/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66532: Added test for authorization actions for `UPDATE_VOLUME`.

[Zhitao Li]

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

(Updated April 19, 2018, 1:01 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Bugs: MESOS-8748
https://issues.apache.org/jira/browse/MESOS-8748


Repository: mesos


Description
---

Added test for authorization actions for `UPDATE_VOLUME`.


Diffs (updated)
-

  src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 


Diff: https://reviews.apache.org/r/66532/diff/3/

Changes: https://reviews.apache.org/r/66532/diff/2-3/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66532: Added test for authorization actions for `UPDATE_VOLUME`.

[Zhitao Li]

> On April 17, 2018, 4:24 p.m., Greg Mann wrote:
> > src/tests/authorization_tests.cpp
> > Line 1979 (original), 1979 (patched)
> > 
> >
> > Could you also add an end-to-end test of authorization for these 
> > operations?

Sure. Do you have an example or existing test to add this?


- Zhitao


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


On April 10, 2018, 12:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated April 10, 2018, 12:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `UPDATE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66051: Implemented operator API to grow and shrink persistent volume.

[Zhitao Li]

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

(Updated April 19, 2018, 1 p.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Added authorization.


Bugs: MESOS-8747
https://issues.apache.org/jira/browse/MESOS-8747


Repository: mesos


Description
---

These operator APIs is implemented as speculative for now, but
we plan to convert them to non-speculative in the future.


Diffs (updated)
-

  src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 


Diff: https://reviews.apache.org/r/66051/diff/12/

Changes: https://reviews.apache.org/r/66051/diff/11-12/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66531: Added new authorization for `UpdateVolume`.

[Zhitao Li]

> On April 18, 2018, 8:04 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Line 4432 (original), 4474 (patched)
> > 
> >
> > We should also add authorization of these operations in the operator 
> > API.

Fixed in https://reviews.apache.org/r/66051/


- Zhitao


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


On April 19, 2018, 12:57 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66531/
> ---
> 
> (Updated April 19, 2018, 12:57 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new authorization action is modelled after `CreateVolume`, and will
> be shared by both `GROW_VOLUME` and `SHRINK_VOLUME` operations and
> corresponding operator APIs.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 8ef33210644e7d2924b402a3158b1b38c1fdb464 
>   include/mesos/authorizer/authorizer.proto 
> 1508c01130013d74ed2b2574a2428f38e3d2c064 
>   src/authorizer/local/authorizer.cpp 
> c098ba9ded1b29a2e079cf09ab80b61f6fa4af05 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
> 
> 
> Diff: https://reviews.apache.org/r/66531/diff/2/
> 
> 
> Testing
> ---
> 
> Manually created ACL and verified that untrusted principals will not allow to 
> grow/shrink volumes.
> Also created unit test in next patch.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66531: Added new authorization for `UpdateVolume`.

[Zhitao Li]

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

(Updated April 19, 2018, 12:57 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Bugs: MESOS-8748
https://issues.apache.org/jira/browse/MESOS-8748


Repository: mesos


Description
---

The new authorization action is modelled after `CreateVolume`, and will
be shared by both `GROW_VOLUME` and `SHRINK_VOLUME` operations and
corresponding operator APIs.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto 8ef33210644e7d2924b402a3158b1b38c1fdb464 
  include/mesos/authorizer/authorizer.proto 
1508c01130013d74ed2b2574a2428f38e3d2c064 
  src/authorizer/local/authorizer.cpp c098ba9ded1b29a2e079cf09ab80b61f6fa4af05 
  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 


Diff: https://reviews.apache.org/r/66531/diff/2/

Changes: https://reviews.apache.org/r/66531/diff/1-2/


Testing
---

Manually created ACL and verified that untrusted principals will not allow to 
grow/shrink volumes.
Also created unit test in next patch.


Thanks,

Zhitao Li

Re: Review Request 66708: Refactored sending a TASK_DROPPED status update.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66704', '66705', '66706', '66707', '66708']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66708

Relevant logs:

- 
[mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66708/logs/mesos-tests-cmake-stdout.log):

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3426):
 warning C4996: 'strerror': This function or variable may be unsafe. Consider 
using strerror_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3500):
 warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define 
_WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\recordio.c(170):
 warning C4267: '=': conversion from 'size_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 

Re: Review Request 66683: Updated address field of new CLI config to accept URLs.

[Mesos Reviewbot Windows]

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



FAIL: Mesos binaries failed to build.

Reviews applied: `['66683']`

Failed command: `cmake.exe --build . --config Release -- /maxcpucount`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66683

Relevant logs:

- 
[mesos-binaries-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66683/logs/mesos-binaries-cmake-stdout.log):

```
   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\src\docker\mesos-docker-executor.vcxproj" (default 
target) (20) ->
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\exit.hpp(69): warning 
C4722: '__Exit::~__Exit': destructor never returns, potential memory leak 
[D:\DCOS\mesos\src\docker\mesos-docker-executor.vcxproj]


   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\src\launcher\mesos-default-executor.vcxproj" (default 
target) (26) ->
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\exit.hpp(69): warning 
C4722: '__Exit::~__Exit': destructor never returns, potential memory leak 
[D:\DCOS\mesos\src\launcher\mesos-default-executor.vcxproj]


   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\src\master\mesos-master.vcxproj" (default target) (35) ->
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\exit.hpp(69): warning 
C4722: '__Exit::~__Exit': destructor never returns, potential memory leak 
[D:\DCOS\mesos\src\master\mesos-master.vcxproj]


   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\src\slave\mesos-agent.vcxproj" (default target) (21) ->
 d:\dcos\mesos\mesos\3rdparty\stout\include\stout\exit.hpp(69): warning 
C4722: '__Exit::~__Exit': destructor never returns, potential memory leak 
[D:\DCOS\mesos\src\slave\mesos-agent.vcxproj]


   "D:\DCOS\mesos\ALL_BUILD.vcxproj" (default target) (1) ->
   "D:\DCOS\mesos\3rdparty\jemalloc-5.0.1.vcxproj" (default target) (7) ->
   (CustomBuild target) -> 
 C:\Program Files (x86)\Microsoft Visual 
Studio\2017\Community\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(171,5):
 error MSB6006: "cmd.exe" exited with code 9009. 
[D:\DCOS\mesos\3rdparty\jemalloc-5.0.1.vcxproj]

358 Warning(s)
1 Error(s)

Time Elapsed 00:23:20.12
```

- Mesos Reviewbot Windows


On April 18, 2018, 6:14 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66683/
> ---
> 
> (Updated April 18, 2018, 6:14 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-8025
> https://issues.apache.org/jira/browse/MESOS-8025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated address field of new CLI config to accept URLs.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/http.py 03d6031cb3273575f41d4d06d9a409f74488a16b 
>   src/python/cli_new/lib/cli/util.py 307b22293a9c7199ad7088dfd0db6dff83a08ac8 
> 
> 
> Diff: https://reviews.apache.org/r/66683/diff/2/
> 
> 
> Testing
> ---
> 
> Ran `mesos agent list` successfully with config:
> ```
> [master]
>   address = "http://.com:5061"
> ```
> On the server providing `http://.com` I had 
> a Mesos master running with `bash mesos-master.sh --port='5061' 
> --work_dir='/tmp/master1' --log_dir='/tmp/master1-log' --registry=in_memory`.
> 
> Also ran `mesos-cli-tests` successfully.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 66531: Added new authorization for `UpdateVolume`.

[Zhitao Li]

> On April 17, 2018, 4:14 p.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 4924-4925 (original), 4980-4981 (patched)
> > 
> >
> > You need to finish implementing authorization here, and below for 
> > SHRINK_VOLUME.
> 
> Zhitao Li wrote:
> Will fix, but I have question/comments on operation authorization 
> handling in `Master::_accept`:
> 1. We don't really check that `authorizations.size() == 
> operations.size()`. My feeling is that this at least leaves space for error 
> when devs adding new operations;
> 2. we allow part of the operations which are authorized to proceed while 
> drop unauthorized operations. I'm not convinced it's either safe from 
> security perspective or clear from user perspective;
> 3. I see some comments about `We may want to retry this failed 
> authorization request rather than dropping it immediately.`. If authorization 
> result is "Unavaiable" rather than "Rejected", should that be reflected from 
> result type?

Reading this a lot, I do not see we can safely retry failed authorization when 
operations are chained. In fact, I feel that we should look into refactoring 
the large code blob and extract authorization handling into its own block 
(rather than scattered through the `switch/case`).

That should be a separate patch though.


- Zhitao


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


On April 10, 2018, 12:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66531/
> ---
> 
> (Updated April 10, 2018, 12:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new authorization action is modelled after `CreateVolume`, and will
> be shared by both `GROW_VOLUME` and `SHRINK_VOLUME` operations and
> corresponding operator APIs.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 8ef33210644e7d2924b402a3158b1b38c1fdb464 
>   include/mesos/authorizer/authorizer.proto 
> 1508c01130013d74ed2b2574a2428f38e3d2c064 
>   src/authorizer/local/authorizer.cpp 
> c098ba9ded1b29a2e079cf09ab80b61f6fa4af05 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
> 
> 
> Diff: https://reviews.apache.org/r/66531/diff/1/
> 
> 
> Testing
> ---
> 
> Manually created ACL and verified that untrusted principals will not allow to 
> grow/shrink volumes.
> Also created unit test in next patch.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66696: Updated documentation for operation feedback.

[Gaston Kleiman]

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

(Updated April 19, 2018, 11:12 a.m.)


Review request for mesos and Greg Mann.


Changes
---

Addressed feedback, updated the CSI doc.


Summary (updated)
-

Updated documentation for operation feedback.


Repository: mesos


Description (updated)
---

Updated documentation for operation feedback.


Diffs (updated)
-

  docs/csi.md 2d96ee754b3fdcfb18a7516f30abf0086fa75137 
  docs/scheduler-http-api.md 3929c33781a152428338c4cdaf7dbc47da7c875e 


Diff: https://reviews.apache.org/r/66696/diff/3/

Changes: https://reviews.apache.org/r/66696/diff/2-3/


Testing
---


Thanks,

Gaston Kleiman

Re: Review Request 66708: Refactored sending a TASK_DROPPED status update.

[James Peach]

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

(Updated April 19, 2018, 6:09 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.


Bugs: MESOS-8585
https://issues.apache.org/jira/browse/MESOS-8585


Repository: mesos


Description
---

Refactored how the agent sends a TASK_DROPPED status update to
remove code duplication. This also makes the agent consistently
send the executor ID in all these status updates.


Diffs (updated)
-

  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 


Diff: https://reviews.apache.org/r/66708/diff/2/

Changes: https://reviews.apache.org/r/66708/diff/1-2/


Testing
---

make check (Fedora 27)


Thanks,

James Peach

Re: Review Request 66531: Added new authorization for `UpdateVolume`.

[Zhitao Li]

> On April 17, 2018, 4:14 p.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 4924-4925 (original), 4980-4981 (patched)
> > 
> >
> > You need to finish implementing authorization here, and below for 
> > SHRINK_VOLUME.

Will fix, but I have question/comments on operation authorization handling in 
`Master::_accept`:
1. We don't really check that `authorizations.size() == operations.size()`. My 
feeling is that this at least leaves space for error when devs adding new 
operations;
2. we allow part of the operations which are authorized to proceed while drop 
unauthorized operations. I'm not convinced it's either safe from security 
perspective or clear from user perspective;
3. I see some comments about `We may want to retry this failed authorization 
request rather than dropping it immediately.`. If authorization result is 
"Unavaiable" rather than "Rejected", should that be reflected from result type?


- Zhitao


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


On April 10, 2018, 12:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66531/
> ---
> 
> (Updated April 10, 2018, 12:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new authorization action is modelled after `CreateVolume`, and will
> be shared by both `GROW_VOLUME` and `SHRINK_VOLUME` operations and
> corresponding operator APIs.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 8ef33210644e7d2924b402a3158b1b38c1fdb464 
>   include/mesos/authorizer/authorizer.proto 
> 1508c01130013d74ed2b2574a2428f38e3d2c064 
>   src/authorizer/local/authorizer.cpp 
> c098ba9ded1b29a2e079cf09ab80b61f6fa4af05 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
> 
> 
> Diff: https://reviews.apache.org/r/66531/diff/1/
> 
> 
> Testing
> ---
> 
> Manually created ACL and verified that untrusted principals will not allow to 
> grow/shrink volumes.
> Also created unit test in next patch.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66708: Refactored sending a TASK_DROPPED status update.

[James Peach]

> On April 19, 2018, 1:58 p.m., Qian Zhang wrote:
> > src/slave/slave.cpp
> > Lines 2543 (patched)
> > 
> >
> > Nit: Switch the order of these two parameters since we use `message` 
> > before `reason` in the code below.
> > 
> > And can we change `[=]` to `[&]`?

I fixed the '[&]', but I'd like to leave the parameter order as is. When you 
have a long message, this ordering makes the call site much easier to read.


- James


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


On April 18, 2018, 10:13 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66708/
> ---
> 
> (Updated April 18, 2018, 10:13 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored how the agent sends a TASK_DROPPED status update to
> remove code duplication. This also makes the agent consistently
> send the executor ID in all these status updates.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66708/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66707: Added a test for launching a task as an unknown user.

[James Peach]

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

(Updated April 19, 2018, 5:56 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.


Bugs: MESOS-8585
https://issues.apache.org/jira/browse/MESOS-8585


Repository: mesos


Description
---

Added a test that launches a task as an unknown user and verifies
that an appropriate status update is generated by the agent.


Diffs (updated)
-

  src/tests/slave_tests.cpp 04f7acad47aa980d507bbe29b29db5eb050c02f3 


Diff: https://reviews.apache.org/r/66707/diff/2/

Changes: https://reviews.apache.org/r/66707/diff/1-2/


Testing
---

make check (Fedora 27)


Thanks,

James Peach

Re: Review Request 66706: Handled failing to create the executor sandbox.

[James Peach]

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

(Updated April 19, 2018, 5:54 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.


Bugs: MESOS-8585
https://issues.apache.org/jira/browse/MESOS-8585


Repository: mesos


Description
---

When the agents adds a new executor, creating the sandbox might
fail (most commonly because the requested task user is not present
on the agent). Rather than crashing the agent with a `CHECK`,
we report a `TASK_DROPPED` status update. This makes the behavior
more consistent with the nested containers API, which also reports
an error in this case.


Diffs (updated)
-

  src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 


Diff: https://reviews.apache.org/r/66706/diff/2/

Changes: https://reviews.apache.org/r/66706/diff/1-2/


Testing
---

make check (Fedora 27)


Thanks,

James Peach

Re: Review Request 66706: Handled failing to create the executor sandbox.

[James Peach]

> On April 19, 2018, 1:58 p.m., Qian Zhang wrote:
> > src/slave/slave.cpp
> > Lines 2994 (patched)
> > 
> >
> > Kill this newline.

I actually prefer this newline. Because the error handling is a big block I 
think it does make a difference to the legibility.


- James


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


On April 18, 2018, 10:12 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66706/
> ---
> 
> (Updated April 18, 2018, 10:12 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the agents adds a new executor, creating the sandbox might
> fail (most commonly because the requested task user is not present
> on the agent). Rather than crashing the agent with a `CHECK`,
> we report a `TASK_DROPPED` status update. This makes the behavior
> more consistent with the nested containers API, which also reports
> an error in this case.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66706/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66705: Propagated executor sandbox creation errors.

[James Peach]

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

(Updated April 19, 2018, 5:52 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.


Bugs: MESOS-8585
https://issues.apache.org/jira/browse/MESOS-8585


Repository: mesos


Description
---

Rather than crashing if the agent fails to create the executor
directory, propagate the error to the caller so that it can
handle it appropriately.


Diffs (updated)
-

  src/slave/paths.hpp fe5ab9e7f96d69069406e2714ab676a5bb070534 
  src/slave/paths.cpp 690bfe3587e6d728ab6eb712a913de23c4abe353 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
  src/tests/paths_tests.cpp dc765ed9db7a8ac7ca0bcb4af5cf353547ba881f 


Diff: https://reviews.apache.org/r/66705/diff/2/

Changes: https://reviews.apache.org/r/66705/diff/1-2/


Testing
---

make check (Fedora 27)


Thanks,

James Peach

Re: Review Request 66705: Propagated executor sandbox creation errors.

[James Peach]

> On April 19, 2018, 1:56 p.m., Qian Zhang wrote:
> > src/slave/paths.cpp
> > Line 761 (original), 763 (patched)
> > 
> >
> > Do you want to kill this `CHECK_SOME` too?

We could return the error for consistency, but we really don't expect this to 
fail. We know the target file exists, so the `rm` failing would be a 
consistency error.


> On April 19, 2018, 1:56 p.m., Qian Zhang wrote:
> > src/slave/slave.cpp
> > Lines 8891-8906 (original), 8892-8907 (patched)
> > 
> >
> > I see you have the code below in the patch 
> > https://reviews.apache.org/r/66706 , but I think it should be part of this 
> > patch because in any cases we should not use the data of a `Try` object 
> > before making sure it has no error. Or can we just merge this patch and 
> > r66706 into one patch since they are closely related?
> > ```
> >   if (directory.isError()) {
> > return Error(directory.error());
> >   }
> > ```

I want to keep these two patches separate because it makes it a lot easier to 
reason about the changes in behavior. I've added a `CHECK_SOME` to this patch 
to preserve the original behavior (until the next patch).


> On April 19, 2018, 1:56 p.m., Qian Zhang wrote:
> > src/slave/slave.cpp
> > Lines 9614 (patched)
> > 
> >
> > Is it possible for agent to crash here?

Not in any new ways. Previously an I/O error creating the directory would cause 
the crash there. I didn't want to silently drop that error, so we would now 
crash here instead. There's sometehing fatally wrong if we can't write the 
checkpoint successfully.


- James


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


On April 18, 2018, 10:12 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66705/
> ---
> 
> (Updated April 18, 2018, 10:12 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than crashing if the agent fails to create the executor
> directory, propagate the error to the caller so that it can
> handle it appropriately.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp fe5ab9e7f96d69069406e2714ab676a5bb070534 
>   src/slave/paths.cpp 690bfe3587e6d728ab6eb712a913de23c4abe353 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/paths_tests.cpp dc765ed9db7a8ac7ca0bcb4af5cf353547ba881f 
> 
> 
> Diff: https://reviews.apache.org/r/66705/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66704: Refactored the executor launch path.

[James Peach]

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

(Updated April 19, 2018, 5:52 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.


Bugs: MESOS-8585
https://issues.apache.org/jira/browse/MESOS-8585


Repository: mesos


Description
---

Refactored the executor launch path so to make the conditional
logic clearer. After this there is just one place that adds and
launches a new executor and we will subsequently be able to add
error handling there.


Diffs (updated)
-

  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 


Diff: https://reviews.apache.org/r/66704/diff/3/

Changes: https://reviews.apache.org/r/66704/diff/2-3/


Testing
---

make check (Fedora 27)


Thanks,

James Peach

Re: Review Request 66649: Added pb2gen.sh for generating python protobuf bindings.

[Eric Chung]

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




src/python/lib/pb2gen.sh
Lines 66 (patched)


remove blank line



src/python/lib/pb2gen.sh
Lines 72 (patched)


remove blank line



src/python/lib/requirements.in
Lines 1 (patched)


how to include protobuf version defined in makefile?


- Eric Chung


On April 17, 2018, 8:22 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66649/
> ---
> 
> (Updated April 17, 2018, 8:22 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current state of support for python in protoc has two serious
> issues:
> 1. The `__init__.py` files necessary to mark a directory as a python
> package aren't generated.
> 2. The import paths in each of the generated .py files do not reflect
> the `--python_path` option passed to `protoc`.
> 
> This results in incomplete code generated, preventing it from being used
> out of the box. To address this issue, we're adding a `pb2gen.sh` script
> to do end-to-end code generation for python protobuf bindings: the
> script generates the bindings based on what's in the `include` dir, then
> postprocesses the generated code to add proper import paths and the
> `__init__.py` files.
> 
> 
> Diffs
> -
> 
>   src/python/lib/pb2gen.sh PRE-CREATION 
>   src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 
> 
> 
> Diff: https://reviews.apache.org/r/66649/diff/3/
> 
> 
> Testing
> ---
> 
> 1. under `src/python/lib`, run `pb2gen.sh`
> 2. initialize and activate virtualenv: `virtualenv env && . env/bin/activate`
> 3. install reqs: `pip install -r requirements.in`
> 4. try to import modules from generated python code: `python -c 'from 
> mesos.pb2.mesos.v1.master import master_pb2'`
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66621]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 16, 2018, 9 p.m., Clément Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> ---
> 
> (Updated April 16, 2018, 9 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Till Toenshoff.
> 
> 
> Bugs: MESOS-8788
> https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for alg RS256 to JWT library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/jwt.hpp 
> 768cbf6fa91537ff9f45f236f4033097c5cea959 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> b7cc31c33fd35c93754407f8b350eeb993177f1d 
>   3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 4d3727daf53ec62a19255da5a9804d342e770ec2 
>   3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp 
> eb36a9aed3b11208c7cdc6f20b5347f46821a207 
> 
> 
> Diff: https://reviews.apache.org/r/66621/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> I added the same tests than the ones for HS256 (i.e., validation in following 
> cases: bad header, bad payload, unknown alg, unsupported alg, valid token 
> etc.. and creation of a valid token). I also added a test to verify that the 
> validation of a RS256 token fails when using the wrong public key.
> 
> 
> Thanks,
> 
> Clément Michaud
> 
>

Re: Review Request 66696: Updated Scheduler HTTP API doc for operation feedback.

[Greg Mann]

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


Fix it, then Ship it!





docs/scheduler-http-api.md
Lines 179 (patched)


Could you make the 'resource provider' phrase a link to the CSI docs? It 
may not be obvious to the uninitiated what this means.


- Greg Mann


On April 18, 2018, 10:15 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66696/
> ---
> 
> (Updated April 18, 2018, 10:15 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Scheduler HTTP API doc for operation feedback.
> 
> 
> Diffs
> -
> 
>   docs/scheduler-http-api.md 3929c33781a152428338c4cdaf7dbc47da7c875e 
> 
> 
> Diff: https://reviews.apache.org/r/66696/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66694: Updated the 1.6.0 CHANGELOG.

[James Peach]

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


Ship it!




Looks good to me

- James Peach


On April 18, 2018, 6:45 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66694/
> ---
> 
> (Updated April 18, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, 
> Alexander Rojas, Benjamin Bannier, and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the 1.6.0 CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 09fff1d50504de158b6c77f51bd7968746e31371 
> 
> 
> Diff: https://reviews.apache.org/r/66694/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 66049.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66049`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66220

Relevant logs:

- 
[apply-review-66049-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66220/logs/apply-review-66049-stdout.log):

```
error: patch failed: include/mesos/mesos.proto:1917
error: include/mesos/mesos.proto: patch does not apply
error: patch failed: include/mesos/v1/mesos.proto:1909
error: include/mesos/v1/mesos.proto: patch does not apply
```

- Mesos Reviewbot Windows


On April 19, 2018, 4:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 19, 2018, 4:39 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

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

(Updated April 19, 2018, 9:39 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Remove unnecessary `Ignore subsequent offers.`


Bugs: MESOS-4965
https://issues.apache.org/jira/browse/MESOS-4965


Repository: mesos


Description
---

Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


Diff: https://reviews.apache.org/r/66220/diff/6/

Changes: https://reviews.apache.org/r/66220/diff/5-6/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

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

(Updated April 19, 2018, 9:30 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Review comments.


Summary (updated)
-

Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.


Bugs: MESOS-4965
https://issues.apache.org/jira/browse/MESOS-4965


Repository: mesos


Description (updated)
---

Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


Diff: https://reviews.apache.org/r/66220/diff/5/

Changes: https://reviews.apache.org/r/66220/diff/4-5/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated April 19, 2018, 9:28 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Add comment on scalar precision.


Bugs: MESOS-4965
https://issues.apache.org/jira/browse/MESOS-4965


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
  src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
  src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
  src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 
  src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 


Diff: https://reviews.apache.org/r/66049/diff/9/

Changes: https://reviews.apache.org/r/66049/diff/8-9/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 17, 2018, 3:52 p.m., Gaston Kleiman wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 474-475 (patched)
> > 
> >
> > I see that Benjamin recently added this line and the corresponding 
> > `AWAIT_READY` to many tests.
> > 
> > However I don't think it is necessary here; I'll check with Benjamin to 
> > see if we can also remove it from other tests in which it is not necessary.

I'd like to keep this (see reasons above) because I've seen race condition on 
offer otherwise. We can remove with other `UpdateSlaveMessage` in this test all 
together.


> On April 17, 2018, 3:52 p.m., Gaston Kleiman wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 502 (patched)
> > 
> >
> > Nit: `Offer offer = offersBeforeCreate->at(0);`
> > 
> > If this test is split into smaller ones, then you can also probable 
> > make `offer` a const ref.

We need to obtain offer at least twice.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 455-459 (patched)
> > 
> >
> > Or alternatively, we can do the following:
> > ```
> > class PersistentVolumeResizeTest : public PersistentVolumeTest {};
> > 
> > INSTANTIATE_TEST_CASE_P(
> > DiskResource, /* Not sure if the same name can be used twice */
> > PersistentVolumeResizeTest,
> > ::testing::Values(
> > PersistentVolumeSourceType::NONE,
> > PersistentVolumeSourceType::PATH));
> > 
> > TEST_P(PersistentVolumeResizeTest, GrowAndShrink)
> > {
> >   ...
> > }
> > ```
> > And remove the conditional return.

I feel keeping current form is easier to reason, and the condition upon `MOUNT` 
is reduced to only one test case.


> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 650-655 (patched)
> > 
> >
> > Since the test checks if the subsequent offer contains the shrunk 
> > volume, there is no need to check the internal `ApplyOperationMessage`.

See comments above: this is used to eliminate race condition between master and 
allocator.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 453 (patched)
> > 
> >
> > If I'm not mistaken, this test does the following steps:
> > 
> > 1. Create a persistent volume
> > 2. Launch a task to write a file in the volume
> > 3. Grow the volume
> > 4. Check if the file exists in the volume
> > 5. Shrink the volume
> > 6. Check if the file exists in the volume
> > 
> > The majority of this test is to implement Step 1 and 2, which are 
> > already testsed in `AccessPersistentVolume` and not really relevent to the 
> > growing and shrinking functions. How about splitting this test into two 
> > tests, `GrowPersistentVolume` and `ShrinkPersistentVolume`, where the first 
> > one only does Step 1, 3, 4 and the second only does Step 1, 5, 6? The 
> > creation of the file can be done with a simple `os::write()` to the volume 
> > path instead of using a task.
> > 
> > Also, I'm not sure if the file check is really necessary. If not, we 
> > can just check the existence of the volume path in Step 4 and 6. 
> > Furthermore, we could even choose not to check the existence of the volume 
> > path. In this case, we could make the persistent volume in 
> > `slaveFlags.resources` from the very beginning, and completely get rid of 
> > Step 4 and 6, and thus the whole test would be much easier to read.

The file check is definitely necessary IMO, because we are guaranteed that 
content of volume is not affected during grow/shrink, but I'm okay to use a 
file operation directly rather than a task to perform the check.

+1 for splitting the test.

w.r.t to directly creating volume from the very beginning, isn't persistent 
volume controlled by checkpointed info rather than `slaveFlags.resources`?


> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 519-570 (patched)
> > 
> >
> > I feel the whole task launch doesn't need to be tested in this unit 
> > test. See my comments above.

Will convert to file operation directly.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 455-459 (patched)
> > 
> >
> > Is this enforced somewhere in validation code? Can we check for 
> > expected behavior when a GROW/SHRINK operation is submitted for a MOUNT 
> > volume, rather than simply returning?
> 
> Zhitao Li wrote:
> I added validation in r/66050 so we drop shrink volume operation on 
> MOUNT. There is no logical path for triggering `GROW` on `MOUNT` disk type so 
> I'm not writing validation.
> 
> For testing, we can either keep the current structure and check that 
> `GROW`/`SHRINK` do not work on `MOUNT` (operation will be dropped), or take 
> Chun's suggestion to not test them for `MOUNT`. Please let me know.

I think this will be the only test case which needs to skip `MOUNT`. It also 
makes some sense because there is no logical starting point for a framework to 
even construct a `GROW_VOLUME` message for `MOUNT`.

I still feel that the handling here could be better. we can discuss on next 
patch revision.


> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 541-542 (patched)
> > 
> >
> > Is this `Future` necessary? Since the task consumes the volume, it may 
> > be sufficient to await on the task status updates?

Yes this is necessary if we do not launch task in this test. we need to 
reliably know that `Allocator::updateAllocation` is called from master, and 
this message happens before that, so this `Future` ensures allocator has 
properly processed all operation conversions and next offer will contain the 
host and updated resources.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66569: Added a test to verify that grow and shrink cannot be combined.

[Zhitao Li]

> On April 16, 2018, 3:05 p.m., Chun-Hung Hsiao wrote:
> >

Thanks for the review. I'll combine this patch into r/66220 and address your 
comment in that process.


> On April 16, 2018, 3:05 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 691-695 (patched)
> > 
> >
> > Since this test just verifies that the grow and shrink operations 
> > cannot be combined with other operations, I was wondering if there's a way 
> > to check this without worry about the different behaviors produced by PATH 
> > or MOUNT.
> > 
> > Alternatively, if you take my suggestion to create another 
> > `PersistentVolumeResizeTest` fixture in the previous patch, we can move 
> > this test into that fixture.

Actually the check happens before type of `MOUNT` so this conditional check can 
be safely removed.


> On April 16, 2018, 3:05 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 775-781 (patched)
> > 
> >
> > As discussed, I'm not sure if this is the semantics we want to enforce. 
> > If we end up rejecting the whole `ACCEPT` call, then we could simplify the 
> > test by:
> > 
> > 1. Starting with a persistent volume, some free disk and some CPU.
> > 2. Open receiving an offer, accept it with reserving the CPU and 
> > growing the persistent volume. Different type of resources are used here to 
> > make sure it's not just for preventing pipelining.
> > 3. Verify that the `ACCEPT` call fails by checking that the next offer 
> > contains the original resources.

Semantic already fixed in the other patch. I will fix the test to reflect your 
suggested semantic.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66569/
> ---
> 
> (Updated April 11, 2018, 2:19 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to verify that grow and shrink cannot be combined.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66569/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66711: Made 3rdparty jemalloc only build when enabled.

[Andrew Schwartzmeyer]

> On April 19, 2018, 5:43 a.m., Benno Evers wrote:
> > I'm a bit surprised by the behaviour you describe, i.e. cmake downloading 
> > and configuring third-party dependencies that are not going to be used. Am 
> > I correct in thinking that for a proper fix, we should wrap *every* 
> > third-party dependency into an analogous if-block?

I am quite surprised at this, because yes that would be the implication. I'm 
going to take a second look with some coffee in me.


- Andrew


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


On April 18, 2018, 7:12 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66711/
> ---
> 
> (Updated April 18, 2018, 7:12 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benno Evers, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ExternalProjectAdd` CMake module defines
> download/update/patch/configure steps unconditionally as default
> targets. This meant that `cmake --build .` on Windows attempted to run
> the `configure` script of jemalloc, which obviously fails.
> 
> By wrapping the 3rdparty import with `ENABLE_JEMALLOC_ALLOCATOR` we
> fix this bug on Windows, and avoid unnecessary build steps on Linux.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 70affa3550d91b48b97417d857a0eeacd60f9b2c 
> 
> 
> Diff: https://reviews.apache.org/r/66711/diff/1/
> 
> 
> Testing
> ---
> 
> `ninja` on Windows (no target arguments); now the `jemalloc` configuration 
> step is entirely skipped. Previously it resulted in:
> 
> ```
> FAILED: 
> 3rdparty/jemalloc-5.0.1/src/jemalloc-5.0.1-stamp/jemalloc-5.0.1-configure
> cmd.exe /C "cd /D 
> C:\Users\andschwa\src\mesos\build\master\3rdparty\jemalloc-5.0.1\src\jemalloc-5.0.1-build
>  && 
> C:\Users\andschwa\src\mesos\build\master\3rdparty\jemalloc-5.0.1\src\jemalloc-5.0.1\configure
>  --enable-stats --enable-prof --with-malloc-conf=prof:true,prof_active:false 
> && "C:\Program Files\CMake\bin\cmake.exe" -E touch 
> C:/Users/andschwa/src/mesos/build/master/3rdparty/jemalloc-5.0.1/src/jemalloc-5.0.1-stamp/jemalloc-5.0.1-configure"
> ```
> 
> Please correct me if I'm wrong about this.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66708: Refactored sending a TASK_DROPPED status update.

[Qian Zhang]

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




src/slave/slave.cpp
Lines 2543 (patched)


Nit: Switch the order of these two parameters since we use `message` before 
`reason` in the code below.

And can we change `[=]` to `[&]`?


- Qian Zhang


On April 19, 2018, 6:13 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66708/
> ---
> 
> (Updated April 19, 2018, 6:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored how the agent sends a TASK_DROPPED status update to
> remove code duplication. This also makes the agent consistently
> send the executor ID in all these status updates.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66708/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66707: Added a test for launching a task as an unknown user.

[Qian Zhang]

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


Fix it, then Ship it!





src/tests/slave_tests.cpp
Lines 1271 (patched)


Better to name it as `ROOT_RunTaskWithCommandInfoWithInvalidUser` to be 
consistent with the existing test `ROOT_RunTaskWithCommandInfoWithoutUser`.



src/tests/slave_tests.cpp
Lines 1293-1294 (patched)


A newline between.


- Qian Zhang


On April 19, 2018, 6:12 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66707/
> ---
> 
> (Updated April 19, 2018, 6:12 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that launches a task as an unknown user and verifies
> that an appropriate status update is generated by the agent.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 04f7acad47aa980d507bbe29b29db5eb050c02f3 
> 
> 
> Diff: https://reviews.apache.org/r/66707/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66706: Handled failing to create the executor sandbox.

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 2994 (patched)


Kill this newline.


- Qian Zhang


On April 19, 2018, 6:12 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66706/
> ---
> 
> (Updated April 19, 2018, 6:12 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the agents adds a new executor, creating the sandbox might
> fail (most commonly because the requested task user is not present
> on the agent). Rather than crashing the agent with a `CHECK`,
> we report a `TASK_DROPPED` status update. This makes the behavior
> more consistent with the nested containers API, which also reports
> an error in this case.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66706/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66705: Propagated executor sandbox creation errors.

[Qian Zhang]

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




src/slave/paths.cpp
Line 761 (original), 763 (patched)


Do you want to kill this `CHECK_SOME` too?



src/slave/slave.cpp
Lines 8891-8906 (original), 8892-8907 (patched)


I see you have the code below in the patch 
https://reviews.apache.org/r/66706 , but I think it should be part of this 
patch because in any cases we should not use the data of a `Try` object before 
making sure it has no error. Or can we just merge this patch and r66706 into 
one patch since they are closely related?
```
  if (directory.isError()) {
return Error(directory.error());
  }
```



src/slave/slave.cpp
Lines 9614 (patched)


Is it possible for agent to crash here?


- Qian Zhang


On April 19, 2018, 6:12 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66705/
> ---
> 
> (Updated April 19, 2018, 6:12 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than crashing if the agent fails to create the executor
> directory, propagate the error to the caller so that it can
> handle it appropriately.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp fe5ab9e7f96d69069406e2714ab676a5bb070534 
>   src/slave/paths.cpp 690bfe3587e6d728ab6eb712a913de23c4abe353 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/paths_tests.cpp dc765ed9db7a8ac7ca0bcb4af5cf353547ba881f 
> 
> 
> Diff: https://reviews.apache.org/r/66705/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66704: Refactored the executor launch path.

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/slave.cpp
Line 2912 (original), 2903-2904 (patched)


Can you add the comment below between this two lines?
```
// Master set the `launch_executor` flag and this is not a command task.
```



src/slave/slave.cpp
Lines 3014 (patched)


This comment seems not accurate, because we can also reach here in the 
legacy case where the master did not set the `launch_executor` flag, i.e., 
launching a new executor is not requested by master.


- Qian Zhang


On April 19, 2018, 6:11 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66704/
> ---
> 
> (Updated April 19, 2018, 6:11 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored the executor launch path so to make the conditional
> logic clearer. After this there is just one place that adds and
> launches a new executor and we will subsequently be able to add
> error handling there.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66704/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66546: Prevent resubscription of resource providers with unknown IDs.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66308', '66309', '66526', '66310', '66311', '66544', 
'66545', '66546']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66546

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66546/logs/mesos-tests-stdout.log):

```
[--] 9 tests from Endpoint/SlaveEndpointTest (1000 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (40 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (44 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (87 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (760 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (785 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (727 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (751 ms total)

[--] Global test environment tear-down
[==] 958 tests from 94 test cases ran. (453925 ms total)
[  PASSED  ] 956 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] 
ContentType/ResourceProviderManagerHttpApiTest.ResubscribeResourceProvider/0, 
where GetParam() = application/x-protobuf
[  FAILED  ] 
ContentType/ResourceProviderManagerHttpApiTest.ResubscribeResourceProvider/1, 
where GetParam() = application/json

 2 FAILED TESTS
  YOU HAVE 214 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66546/logs/mesos-tests-stderr.log):

```
I0419 13:25:54.049172  5424 master.cpp:10453] Updating the state of task 
d854c9c4-9679-4252-8fa8-6b123aeed47b of framework 
ca52eb1e-ad85-4aee-baf1-5ba31abacedf- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0419 13:25:54.049172  3828 slave.cpp:3985] Shutting down framework 
ca52eb1e-ad85-4aee-baf1-5ba31abacedf-
I0419 13:25:54.050384  3828 slave.cpp:6694] Shutting down executor 
'd854c9c4-9679-4252-8fa8-6b123aeed47b' of framework 
ca52eb1e-ad85-4aee-baf1-5ba31abacedf- at executor(1)@10.3.1.5:56368
I0419 13:2I0419 13:25:53.879165  5172 exec.cpp:162] Version: 1.6.0
I0419 13:25:53.906172  2056 exec.cpp:236] Executor registered on agent 
ca52eb1e-ad85-4aee-baf1-5ba31abacedf-S0
I0419 13:25:53.910169 13952 executor.cpp:177] Received SUBSCRIBED event
I0419 13:25:53.915170 13952 executor.cpp:181] Subscribed executor on 
winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0419 13:25:53.915170 13952 executor.cpp:177] Received LAUNCH event
I0419 13:25:53.920166 13952 executor.cpp:649] Starting task 
d854c9c4-9679-4252-8fa8-6b123aeed47b
I0419 13:25:54.012176 13952 executor.cpp:484] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0419 13:25:54.025168 13952 executor.cpp:662] Forked command at 9248
I0419 13:25:54.052179 11032 exec.cpp:445] Executor asked to shutdown
I0419 13:25:54.052179   556 executor.cpp:177] Received SHUTDOWN event
I0419 13:25:54.052179   556 executor.cpp:759] Shutting down
I0419 13:25:54.052179   556 executor.cpp:869] Sending SIGTERM to process tree 
at pid 95:54.051192  3828 slave.cpp:933] Agent terminating
W0419 13:25:54.052179  3828 slave.cpp:3981] Ignoring shutdown framework 
ca52eb1e-ad85-4aee-baf1-5ba31abacedf- because it is terminating
I0419 13:25:54.052179  5424 master.cpp:10552] Removing task 
d854c9c4-9679-4252-8fa8-6b123aeed47b with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework ca52eb1e-ad85-4aee-baf1-5ba31abacedf- on 
agent ca52eb1e-ad85-4aee-baf1-5ba31abacedf-S0 at slave(428)@10.3.1.5:56347 
(winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0419 13:25:54.054172 11948 containerizer.cpp:2341] Destroying container 
f99272d9-f42e-44f7-9188-d380f834a605 in RUNNING state
I0419 13:25:54.054172 11948 containerizer.cpp:2955] Transitioning the state of 
container f99272d9-f42e-44f7-9188-d380f834a605 from RUNNING to DESTROYING
I0419 13:25:54.055178 11948 launcher.cpp:156] Asked to destroy container 
f99272d9-f42e-44f7-9188-d380f834a605
I0419 13:25:54.055178  5424 master.cpp:1296] Agent 

Re: Review Request 66711: Made 3rdparty jemalloc only build when enabled.

[Benno Evers]

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


Ship it!




I'm a bit surprised by the behaviour you describe, i.e. cmake downloading and 
configuring third-party dependencies that are not going to be used. Am I 
correct in thinking that for a proper fix, we should wrap *every* third-party 
dependency into an analogous if-block?

- Benno Evers


On April 19, 2018, 2:12 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66711/
> ---
> 
> (Updated April 19, 2018, 2:12 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benno Evers, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ExternalProjectAdd` CMake module defines
> download/update/patch/configure steps unconditionally as default
> targets. This meant that `cmake --build .` on Windows attempted to run
> the `configure` script of jemalloc, which obviously fails.
> 
> By wrapping the 3rdparty import with `ENABLE_JEMALLOC_ALLOCATOR` we
> fix this bug on Windows, and avoid unnecessary build steps on Linux.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 70affa3550d91b48b97417d857a0eeacd60f9b2c 
> 
> 
> Diff: https://reviews.apache.org/r/66711/diff/1/
> 
> 
> Testing
> ---
> 
> `ninja` on Windows (no target arguments); now the `jemalloc` configuration 
> step is entirely skipped. Previously it resulted in:
> 
> ```
> FAILED: 
> 3rdparty/jemalloc-5.0.1/src/jemalloc-5.0.1-stamp/jemalloc-5.0.1-configure
> cmd.exe /C "cd /D 
> C:\Users\andschwa\src\mesos\build\master\3rdparty\jemalloc-5.0.1\src\jemalloc-5.0.1-build
>  && 
> C:\Users\andschwa\src\mesos\build\master\3rdparty\jemalloc-5.0.1\src\jemalloc-5.0.1\configure
>  --enable-stats --enable-prof --with-malloc-conf=prof:true,prof_active:false 
> && "C:\Program Files\CMake\bin\cmake.exe" -E touch 
> C:/Users/andschwa/src/mesos/build/master/3rdparty/jemalloc-5.0.1/src/jemalloc-5.0.1-stamp/jemalloc-5.0.1-configure"
> ```
> 
> Please correct me if I'm wrong about this.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

[Alexander Rojas]

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




3rdparty/libprocess/include/process/jwt.hpp
Lines 19 (patched)


I really would prefer, if possible, to forward declare `RSA_shared_ptr` 
here and leave in include in the cpp.



3rdparty/libprocess/include/process/ssl/utilities.hpp
Lines 140 (patched)


I'm not so sure we gain too much by declaring this typedef. Moreover, we 
use typedefs rather sparecely in the code base, so I would get away with it.



3rdparty/libprocess/include/process/ssl/utilities.hpp
Lines 189-190 (patched)


This comment needs an update.



3rdparty/libprocess/src/jwt.cpp
Lines 303 (patched)


We don't use `auto` unless the type is obvious from the lhs (like a call to 
`new Type` or `make_shared`) but in this case it is not obvious.



3rdparty/libprocess/src/jwt.cpp
Lines 364-365 (patched)


This could be better formatted:

```c++
  return JWT(
  header,
  payload,
  base64::encode_url_safe(signarure.ger(), false));
```



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 374 (patched)


**Here and below**

This files apparently uses snake case everywhere instead of snake case. 
Could you rectify that here in your function names and the variable names 
withing these functions?



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 382 (patched)


missed an space between `if` and `(`.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 401-406 (patched)


Note that we tend to avoid using `std::string` or `std::vector` in cpp 
files. Instead, at the top of the file add:

```c++
using std::string;
using std::vector;
```



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 405 (patched)


missed `#include `



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 415 (patched)


let's avoid one letter variable names.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 423 (patched)


missed an space between `if` and `(`.



3rdparty/libprocess/src/ssl/utilities.cpp
Lines 427 (patched)


I'd add an empty line here.


- Alexander Rojas


On April 16, 2018, 11 p.m., Clément Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> ---
> 
> (Updated April 16, 2018, 11 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Till Toenshoff.
> 
> 
> Bugs: MESOS-8788
> https://issues.apache.org/jira/browse/MESOS-8788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for alg RS256 to JWT library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/jwt.hpp 
> 768cbf6fa91537ff9f45f236f4033097c5cea959 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> b7cc31c33fd35c93754407f8b350eeb993177f1d 
>   3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 4d3727daf53ec62a19255da5a9804d342e770ec2 
>   3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp 
> eb36a9aed3b11208c7cdc6f20b5347f46821a207 
> 
> 
> Diff: https://reviews.apache.org/r/66621/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> I added the same tests than the ones for HS256 (i.e., validation in following 
> cases: bad header, bad payload, unknown alg, unsupported alg, valid token 
> etc.. and creation of a valid token). I also added a test to verify that the 
> validation of a RS256 token fails when using the wrong public key.
> 
> 
> Thanks,
> 
> Clément Michaud
> 
>

Re: Review Request 66310: Passed on registrar when constructing resource provider manager.

[Benjamin Bannier]

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

(Updated April 19, 2018, 2:28 p.m.)


Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.


Changes
---

Addressed issue raised by Chun.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

In order to support recovering resource provider manager information
in the future, we need to adjust the construction of the manager to be
able to consume a registrar.

This patch lays the groundwork by adjusting interfaces and their
usage; we will make use of the passed on information in a following
patch.


Diffs (updated)
-

  src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2 
  src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66310/diff/5/

Changes: https://reviews.apache.org/r/66310/diff/4-5/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66526: Renamed resource provider `AgentRegistrar` to `GenericRegistrar`.

[Benjamin Bannier]

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

(Updated April 19, 2018, 2:29 p.m.)


Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jan Schlicht.


Changes
---

Addressed issue raised by Chun.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

This registrar and its matching process can work on generic storage
and is currently used to work on an agent's persist store.

Its name should not be tied the agent, so we rename the registrar in
this patch.


Diffs (updated)
-

  src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
  src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b 


Diff: https://reviews.apache.org/r/66526/diff/3/

Changes: https://reviews.apache.org/r/66526/diff/2-3/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66311: Implement recovery of resource provider manager.

[Benjamin Bannier]

> On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 237 (patched)
> > 
> >
> > `::recover`

This is not used anywhere else in this file, let's just keep this instance as 
is?

Dropping.


> On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 264 (patched)
> > 
> >
> > This means that if the registrar itself takes a while to recover, and 
> > during recovery a lot of RPs subscribe to the manager, then all requests 
> > (along with potentially huge `ResourceProviderInfo`s) will be stored in the 
> > memory until the recovery is finished.
> > 
> > I was wondering if we should consider just rejecting all requests 
> > before the reccovery is finished, and have a function returning the future 
> > for recovery so the caller can do synchronization if they want to. Thoughts?

That's what I wanted to do initially as well, but it really complicates 
resource providers as they would need to implement some retry logic.

As we currently do assume not too large numbers of resource providers elsewhere 
as well, I believe keeping pending subscribe requests in memory should be fine 
for now. I added a `TODO` regarding your suggested improvement.


> On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Line 304 (original), 346 (patched)
> > 
> >
> > I might be missing something, but why is `this` necessary? Ditto below.

We are in a lambda body here and capture `this` which in general is not safe 
(unless one uses `dispatch` or `defer`). We often do make member access or 
method calls explicit in such contexts to call out the "dangerous parts".


> On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Lines 193-203 (original), 194-203 (patched)
> > 
> >
> > How about moving this into `GenericRegistrarProcess::initialize()` to 
> > start recovery early?
> > 
> > If we do this and keep `recovered` (See below) then this function 
> > should return
> > ```
> > recovered->then([=] { return registry.get(); })
> > ```

We already require users to `recover` registrars before being able to perform 
operations against them (like for other registrars), so I am not really sure I 
understand how what you suggest would help. Could you explain?


> On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Line 205 (original), 205 (patched)
> > 
> >
> > Why do we need the `undiscardable` here? Could you add some comments?
> > 
> > Also, should we prevent the recovery being discarded due to a caller 
> > discarding an `apply` call? If yes then we should do
> > ```
> > return undiscardable(registry.get()).then(... ::_apply ...);
> > ```
> > in the following `apply()` function.

I added a comment and also fixed below `apply` to use `recover()` which would 
return the cached result, already guarded by `undiscarable`.


> On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Line 216 (original), 216 (patched)
> > 
> >
> > The overall logic is correct, but it takes a bit of inference to know 
> > that overwriting `registry` with a new `Future` in an earlier `_apply` does 
> > not affect a later `_apply` that is chained with the overwritten `Future`. 
> > So it seems more readible to me if we keep the original 
> > `Option recovered` (or make it just a `Promise`), 
> > and chain `_apply` with `recovered` here.

I replaced the direct access to `registry` with `recover` here which once 
recovered would just serve a cached result. Does that look more reasonable to 
you?


> On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Line 250 (original), 250 (patched)
> > 
> >
> > The overall logic is correct, but it takes a bit of inference to know 
> > that the `Future` obtained from `registry.get()` is guaranteed to be ready. 
> > So it seems more readible to me if we keep `registry` as an `Option` rather 
> > than a `Future`.

It is pretty explicit on the `apply` path above -- how about adding a 
`CHECK(registry->isReady())` for documentation here?

Updated the code like that.


> On April 19, 2018, 8 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Line 381 (original), 387 (patched)
> > 

Re: Review Request 66311: Implement recovery of resource provider manager.

[Benjamin Bannier]

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

(Updated April 19, 2018, 2:29 p.m.)


Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.


Changes
---

Addressed some issues raised by Chun.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

This patch adjusts the control flow of the resource provider manager
so that we can in the future make use of persisted resource provider
information.


Diffs (updated)
-

  src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
  src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
  src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66311/diff/5/

Changes: https://reviews.apache.org/r/66311/diff/4-5/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66310: Passed on registrar when constructing resource provider manager.

[Benjamin Bannier]

> On April 19, 2018, 12:25 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Line 160 (original), 161 (patched)
> > 
> >
> > `Owned&& _registrar`?

https://reviews.apache.org/r/66309/#comment_rc282702-201502


> On April 19, 2018, 12:25 a.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8869-8871 (patched)
> > 
> >
> > The current implementation makes this assertion never fail ;)

That's exactly why this is an assertion, not a branch :D


- Benjamin


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


On April 19, 2018, 2:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66310/
> ---
> 
> (Updated April 19, 2018, 2:28 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to support recovering resource provider manager information
> in the future, we need to adjust the construction of the manager to be
> able to consume a registrar.
> 
> This patch lays the groundwork by adjusting interfaces and their
> usage; we will make use of the passed on information in a following
> patch.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2 
>   src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66310/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66711: Made 3rdparty jemalloc only build when enabled.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66711]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 18, 2018, 7:12 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66711/
> ---
> 
> (Updated April 18, 2018, 7:12 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benno Evers, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ExternalProjectAdd` CMake module defines
> download/update/patch/configure steps unconditionally as default
> targets. This meant that `cmake --build .` on Windows attempted to run
> the `configure` script of jemalloc, which obviously fails.
> 
> By wrapping the 3rdparty import with `ENABLE_JEMALLOC_ALLOCATOR` we
> fix this bug on Windows, and avoid unnecessary build steps on Linux.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 70affa3550d91b48b97417d857a0eeacd60f9b2c 
> 
> 
> Diff: https://reviews.apache.org/r/66711/diff/1/
> 
> 
> Testing
> ---
> 
> `ninja` on Windows (no target arguments); now the `jemalloc` configuration 
> step is entirely skipped. Previously it resulted in:
> 
> ```
> FAILED: 
> 3rdparty/jemalloc-5.0.1/src/jemalloc-5.0.1-stamp/jemalloc-5.0.1-configure
> cmd.exe /C "cd /D 
> C:\Users\andschwa\src\mesos\build\master\3rdparty\jemalloc-5.0.1\src\jemalloc-5.0.1-build
>  && 
> C:\Users\andschwa\src\mesos\build\master\3rdparty\jemalloc-5.0.1\src\jemalloc-5.0.1\configure
>  --enable-stats --enable-prof --with-malloc-conf=prof:true,prof_active:false 
> && "C:\Program Files\CMake\bin\cmake.exe" -E touch 
> C:/Users/andschwa/src/mesos/build/master/3rdparty/jemalloc-5.0.1/src/jemalloc-5.0.1-stamp/jemalloc-5.0.1-configure"
> ```
> 
> Please correct me if I'm wrong about this.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66309: Externalized creation of resource provider manager backing storage.

[Benjamin Bannier]

> On April 18, 2018, 11:31 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Line 69 (original), 71 (patched)
> > 
> >
> > Does it make sense to use `process::Owned&& storage`?

I don't think so. Taking a value already implies that we take ownership of the 
passed `storage`, and to call this function any user would need to provide us 
with that exclusive ownership (e.g., by casting some local with `std::move`). 
All that restricting ourself to rvalues here would add is that it would enforce 
correct usage even for broken types which are only semantically non-copyable, 
but still define a copy constructor. Unfortunately `Owned` falls into that 
category (see MESOS-5122), but I prefer to explicitly move args when passing 
instead of polluting interfaces because of tech debt we will fix eventually.


> On April 18, 2018, 11:31 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Lines 71-73 (original), 73-75 (patched)
> > 
> >
> > I was wondering that, since we have a generic storage-backed registrar, 
> > could we also use this for the master's RP registrar and remove 
> > `MasterRegistrar`?

The master registrar is different in that it is already recovered and we need 
to use it differently when retrieving its recovered state via above interface, 
see https://reviews.apache.org/r/66311//


- Benjamin


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


On April 19, 2018, 11:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> ---
> 
> (Updated April 19, 2018, 11:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
> 
> Being able to explicitly inject the used storage simplifies testing.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 92ef9aecb1e93d10f46e53984391558f9901a60b 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66309/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66309: Externalized creation of resource provider manager backing storage.

[Benjamin Bannier]

> On April 19, 2018, 12:19 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Line 113 (original), 114 (patched)
> > 
> >
> > `process::Owned&& storage`?

See above.


> On April 19, 2018, 12:19 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.cpp
> > Line 163 (original), 151 (patched)
> > 
> >
> > `Owned&& _storage`? The parameter has an underscore below.

We use the underscore to disambiguate the different names to readers. It is 
required below, but not here.


- Benjamin


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


On April 19, 2018, 11:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> ---
> 
> (Updated April 19, 2018, 11:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
> 
> Being able to explicitly inject the used storage simplifies testing.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 92ef9aecb1e93d10f46e53984391558f9901a60b 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66309/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66309: Externalized creation of resource provider manager backing storage.

[Benjamin Bannier]

> On April 18, 2018, 11:51 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/registrar.hpp
> > Lines 67-73 (original), 69-75 (patched)
> > 
> >
> > Not yours, but if each `create()` corresponds to a different type of 
> > registrar, how about moving these functions into their corresponding 
> > `MasterRegistrar` and `AgentRegistrar`?
> > 
> > I guess it depends on how meanful it is to encapsulate the actual type 
> > of registrar from the callers.
> 
> Chun-Hung Hsiao wrote:
> Also, if there is no need to encapsulate the actual type of registrar, 
> then it is meanleass to have these `create()` functions, since we don't do 
> extra checks and return errors.

I think having these factory functions still adds value as they hide away 
non-trivial details.  The arguments pretty clearly communicate how they can be 
used, e.g., in the master we would not be able to transfer ownership of the 
underlying storage leaving only one possible factory to use.

Let's keep this as is, dropping.


- Benjamin


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


On April 19, 2018, 11:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> ---
> 
> (Updated April 19, 2018, 11:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
> 
> Being able to explicitly inject the used storage simplifies testing.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 92ef9aecb1e93d10f46e53984391558f9901a60b 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66309/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66308: Delayed construction of the agent's resource provider manager.

[Benjamin Bannier]

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

(Updated April 19, 2018, 11:19 a.m.)


Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.


Changes
---

Addressed more issues raised by Chun.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

By delaying the construction of the agent's resource provider manager
we prepare for a following patch introducing a dependency on the
resource provider manager on the agent's ID.

Depending on whether the agent was able to recover an agent ID from
its log or still needs to obtain on in a first registration with the
master, we can only construct the resource provider manager after
different points in the initialization of the agent. To capture the
common code we introduce a helper function performing the necessary
steps.


Diffs (updated)
-

  src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66308/diff/5/

Changes: https://reviews.apache.org/r/66308/diff/4-5/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66308: Delayed construction of the agent's resource provider manager.

[Benjamin Bannier]

> On April 18, 2018, 8:58 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 839 (patched)
> > 
> >
> > One line apart. Actually, let's move this back to it's original place 
> > so all `/api/v1/*` endpoints are declared together.

I moved this up to be grouped with the routes of `/api/v1` and 
`/api/v1/executor`.


> On April 18, 2018, 8:58 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8189 (patched)
> > 
> >
> > Maybe add a comment saying that checking `additionalResources` is 
> > enough because all resources needs to be published must have been checked 
> > before?

I am not sure that is what is happening here. The assertion here just validates 
that no new additional resources from RPs enter while we haven't even 
initialized the RP manager. This should hold as RP resources can only appear 
after a RP has subscribed which requires the manager. I don't believe that this 
performs a very stringent check, and we just perform the easy check here.

I added a comment to that effect, PTAL.


- Benjamin


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


On April 18, 2018, 4:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66308/
> ---
> 
> (Updated April 18, 2018, 4:28 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By delaying the construction of the agent's resource provider manager
> we prepare for a following patch introducing a dependency on the
> resource provider manager on the agent's ID.
> 
> Depending on whether the agent was able to recover an agent ID from
> its log or still needs to obtain on in a first registration with the
> master, we can only construct the resource provider manager after
> different points in the initialization of the agent. To capture the
> common code we introduce a helper function performing the necessary
> steps.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66308/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66309: Externalized creation of resource provider manager backing storage.

[Benjamin Bannier]

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

(Updated April 19, 2018, 11:19 a.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Addressed issue raised by Chun.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

This patch changes the way the storage backing an agent's resource
provider registrar is created: while before we created it implicitly
when constructing the registrar, we now consume storage passed on
construction.

Being able to explicitly inject the used storage simplifies testing.


Diffs (updated)
-

  src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
  src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66309/diff/4/

Changes: https://reviews.apache.org/r/66309/diff/3-4/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66708: Refactored sending a TASK_DROPPED status update.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66704, 66705, 66706, 66707, 66708]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 18, 2018, 10:13 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66708/
> ---
> 
> (Updated April 18, 2018, 10:13 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8585
> https://issues.apache.org/jira/browse/MESOS-8585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored how the agent sends a TASK_DROPPED status update to
> remove code duplication. This also makes the agent consistently
> send the executor ID in all these status updates.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66708/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 66311: Implement recovery of resource provider manager.

[Chun-Hung Hsiao]

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




src/resource_provider/manager.cpp
Lines 89 (patched)


This is not used.



src/resource_provider/manager.cpp
Lines 234-247 (patched)


Let's move this into `ResourceProviderManagerProcess::initialize()` to keep 
this running in the actor's context (although I think this is safe because we 
spawn the process after the constructor finishes).



src/resource_provider/manager.cpp
Lines 237 (patched)


`::recover`



src/resource_provider/manager.cpp
Lines 242 (patched)


I actually don't know we have `operator<<` defined for a future! Learned 
something :)



src/resource_provider/manager.cpp
Lines 264 (patched)


This means that if the registrar itself takes a while to recover, and 
during recovery a lot of RPs subscribe to the manager, then all requests (along 
with potentially huge `ResourceProviderInfo`s) will be stored in the memory 
until the recovery is finished.

I was wondering if we should consider just rejecting all requests before 
the reccovery is finished, and have a function returning the future for 
recovery so the caller can do synchronization if they want to. Thoughts?



src/resource_provider/manager.cpp
Line 304 (original), 346 (patched)


I might be missing something, but why is `this` necessary? Ditto below.



src/resource_provider/registrar.cpp
Lines 193-203 (original), 194-203 (patched)


How about moving this into `GenericRegistrarProcess::initialize()` to start 
recovery early?

If we do this and keep `recovered` (See below) then this function should 
return
```
recovered->then([=] { return registry.get(); })
```



src/resource_provider/registrar.cpp
Line 205 (original), 205 (patched)


Why do we need the `undiscardable` here? Could you add some comments?

Also, should we prevent the recovery being discarded due to a caller 
discarding an `apply` call? If yes then we should do
```
return undiscardable(registry.get()).then(... ::_apply ...);
```
in the following `apply()` function.



src/resource_provider/registrar.cpp
Line 216 (original), 216 (patched)


The overall logic is correct, but it takes a bit of inference to know that 
overwriting `registry` with a new `Future` in an earlier `_apply` does not 
affect a later `_apply` that is chained with the overwritten `Future`. So it 
seems more readible to me if we keep the original `Option 
recovered` (or make it just a `Promise`), and chain `_apply` with 
`recovered` here.



src/resource_provider/registrar.cpp
Line 250 (original), 250 (patched)


The overall logic is correct, but it takes a bit of inference to know that 
the `Future` obtained from `registry.get()` is guaranteed to be ready. So it 
seems more readible to me if we keep `registry` as an `Option` rather than a 
`Future`.



src/resource_provider/registrar.cpp
Line 381 (original), 387 (patched)


Not yours, but the use of pointer indicates that this 
`resource_provider::MasterRegistrarProcess` relies on the lifecycle of the 
backed `master::Registrar` without any guarantee. Let's add some comments about 
this.


- Chun-Hung Hsiao


On April 11, 2018, 7:30 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> ---
> 
> (Updated April 11, 2018, 7:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adjusts the control flow of the resource provider manager
> so that we can in the future make use of persisted resource provider
> information.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 92ef9aecb1e93d10f46e53984391558f9901a60b 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: