Re: Review Request 65182: Tested that agent resends unacknowledged op status updates on recovery.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65182 was successfully built and tested.

Reviews applied: `['64992', '64994', '65232', '65057', '65182']`

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

- Mesos Reviewbot Windows


On Jan. 17, 2018, 5 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65182/
> ---
> 
> (Updated Jan. 17, 2018, 5 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8362
> https://issues.apache.org/jira/browse/MESOS-8362
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a test to verify that the agent resends unacknowledged
> operation status updates after a recovery.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65182/diff/4/
> 
> 
> Testing
> ---
> 
> `GLOG_v=1 sudo bin/mesos-tests.sh 
> --gtest_filter='StorageLocalResourceProviderTest.ROOT_RetryOperationStatusUpdateAfterRecovery'
>  --verbose --gtest_repeat=1000 --gtest_break_on_failure` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65039: Tested reconciliation when operation is dropped en route to agent.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65039 was successfully built and tested.

Reviews applied: `['64992', '64994', '65232', '65039']`

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

- Mesos Reviewbot Windows


On Jan. 19, 2018, 12:42 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65039/
> ---
> 
> (Updated Jan. 19, 2018, 12:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> and Jie Yu.
> 
> 
> Bugs: MESOS-8373
> https://issues.apache.org/jira/browse/MESOS-8373
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds
> StorageLocalResourceProviderTest.ROOT_ReconcileDroppedOperation
> in order to verify that reconciliation is performed correctly
> when an operation is dropped on its way from the master to the
> agent.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65039/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Chun-Hung Hsiao]

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

(Updated Jan. 19, 2018, 2:46 a.m.)


Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.


Changes
---

Addressed Qian's comment.


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


Repository: mesos


Description
---

Fixed detaching task volume directories of destroyed frameworks.


Diffs (updated)
-

  src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
  src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On Jan. 19, 2018, 10:30 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65231/
> ---
> 
> (Updated Jan. 19, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8460
> https://issues.apache.org/jira/browse/MESOS-8460
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed detaching task volume directories of destroyed frameworks.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
>   src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 
> 
> 
> Diff: https://reviews.apache.org/r/65231/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Qian Zhang]

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




src/slave/slave.cpp
Lines 1030-1031 (patched)


Add a newline between.


- Qian Zhang


On Jan. 19, 2018, 10:30 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65231/
> ---
> 
> (Updated Jan. 19, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8460
> https://issues.apache.org/jira/browse/MESOS-8460
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed detaching task volume directories of destroyed frameworks.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
>   src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 
> 
> 
> Diff: https://reviews.apache.org/r/65231/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65231 was successfully built and tested.

Reviews applied: `['65231']`

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

- Mesos Reviewbot Windows


On Jan. 19, 2018, 2:30 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65231/
> ---
> 
> (Updated Jan. 19, 2018, 2:30 a.m.)
> 
> 
> Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8460
> https://issues.apache.org/jira/browse/MESOS-8460
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed detaching task volume directories of destroyed frameworks.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
>   src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 
> 
> 
> Diff: https://reviews.apache.org/r/65231/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Chun-Hung Hsiao]

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

(Updated Jan. 19, 2018, 2:30 a.m.)


Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.


Changes
---

Bug fix.


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


Repository: mesos


Description
---

Fixed detaching task volume directories of destroyed frameworks.


Diffs (updated)
-

  src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
  src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65231 was successfully built and tested.

Reviews applied: `['65231']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 6:22 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65231/
> ---
> 
> (Updated Jan. 18, 2018, 6:22 p.m.)
> 
> 
> Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8460
> https://issues.apache.org/jira/browse/MESOS-8460
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed detaching task volume directories of destroyed frameworks.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
>   src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 
> 
> 
> Diff: https://reviews.apache.org/r/65231/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Chun-Hung Hsiao]

> On Jan. 19, 2018, 1:36 a.m., Vinod Kone wrote:
> > Can you or @qian write a unit test that triggered this bug?

Let's do the unit test in another patch. Tracking with MESOS-8462.


- Chun-Hung


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


On Jan. 19, 2018, 2:22 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65231/
> ---
> 
> (Updated Jan. 19, 2018, 2:22 a.m.)
> 
> 
> Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8460
> https://issues.apache.org/jira/browse/MESOS-8460
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed detaching task volume directories of destroyed frameworks.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
>   src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 
> 
> 
> Diff: https://reviews.apache.org/r/65231/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Chun-Hung Hsiao]

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

(Updated Jan. 19, 2018, 2:22 a.m.)


Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.


Changes
---

Addressed Vinod's comments and removed the `executorDirectory` argument from 
the attach function.


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


Repository: mesos


Description
---

Fixed detaching task volume directories of destroyed frameworks.


Diffs (updated)
-

  src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
  src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Chun-Hung Hsiao]

> On Jan. 19, 2018, 1:36 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp
> > Lines 1029 (patched)
> > 
> >
> > Custom executors satisfy this check so it's not fool proof. Lets pass 
> > `ExecutorInfo` here and check the type instead.

I don't like the fact that we pass in the `ExecutorInfo` but don't use it at 
all in the functions, but I can live with it so I'll do it.


- Chun-Hung


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


On Jan. 19, 2018, 1:25 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65231/
> ---
> 
> (Updated Jan. 19, 2018, 1:25 a.m.)
> 
> 
> Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8460
> https://issues.apache.org/jira/browse/MESOS-8460
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed detaching task volume directories of destroyed frameworks.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
>   src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 
> 
> 
> Diff: https://reviews.apache.org/r/65231/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65057: Tested that op status updates dropped en route to master are resent.

[Gaston Kleiman]

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

(Updated Jan. 18, 2018, 6:19 p.m.)


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


Changes
---

Addressed feedback.


Bugs: MESOS-8363 and MESOS-8420
https://issues.apache.org/jira/browse/MESOS-8363
https://issues.apache.org/jira/browse/MESOS-8420


Repository: mesos


Description
---

This patch adds
`StorageLocalResourceProviderTest.ROOT_RetryOperationStatusUpdate` that
verifies that operation status updates are resent to the master after
being dropped en route to it.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
bbfe95e9818f25fdd5405db3ad2fe355e023f743 


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

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


Testing
---

`sudo bin/mesos-tests.sh 
--gtest_filter='StorageLocalResourceProviderTest.ROOT_RetryOperationStatusUpdate'
 --gtest_repeat=100 --gtest_break_on_failure` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Vinod Kone]

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



Can you or @qian write a unit test that triggered this bug?


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


Custom executors satisfy this check so it's not fool proof. Lets pass 
`ExecutorInfo` here and check the type instead.



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


ditto. lets pass `ExecutorInfo` here too.


- Vinod Kone


On Jan. 19, 2018, 1:25 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65231/
> ---
> 
> (Updated Jan. 19, 2018, 1:25 a.m.)
> 
> 
> Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8460
> https://issues.apache.org/jira/browse/MESOS-8460
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed detaching task volume directories of destroyed frameworks.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
>   src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 
> 
> 
> Diff: https://reviews.apache.org/r/65231/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Chun-Hung Hsiao]

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

(Updated Jan. 19, 2018, 1:25 a.m.)


Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.


Changes
---

Addressed Qian's comment.


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


Repository: mesos


Description
---

Fixed detaching task volume directories of destroyed frameworks.


Diffs (updated)
-

  src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
  src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Qian Zhang]

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




src/slave/slave.cpp
Lines 5915-5919 (original), 5998-6002 (patched)


What about changing these to something like below to be consistent with 
what you did in `recoverExecutor()`?
```
garbageCollect(path)
.onAny(defer(self(), ::detachFile, latestPath))
.onAny(defer(self(), ::detachFile, virtualLatestPath));
```


- Qian Zhang


On Jan. 19, 2018, 8:48 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65231/
> ---
> 
> (Updated Jan. 19, 2018, 8:48 a.m.)
> 
> 
> Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8460
> https://issues.apache.org/jira/browse/MESOS-8460
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed detaching task volume directories of destroyed frameworks.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
>   src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 
> 
> 
> Diff: https://reviews.apache.org/r/65231/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65090: Mesos flags related to ZooKeeper use SecurePathOrValue.

[Greg Mann]

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


Fix it, then Ship it!





src/master/flags.hpp
Line 105 (original), 105 (patched)


Nit: in the description, I would recommend:
s/This prevents this leakage/This patch prevents this leakage/

I think it's just a bit more clear.


- Greg Mann


On Jan. 18, 2018, 7:01 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65090/
> ---
> 
> (Updated Jan. 18, 2018, 7:01 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8413
> https://issues.apache.org/jira/browse/MESOS-8413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up until now the Mesos master flag `--zk` as well as the Mesos agent
> flag `--master` would leak ZooKeeper authentication credentials in
> both logs and results for the `/flags` endpoint, if the credentials
> were part of the configuration url.
> 
> This prevents this leakage if a user decides to store the ZooKeeper
> url in a file and pass the file as a value to the flags mentioned
> above (using the preffix `file://`).
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp dabb414560f506787b6e821a27af623c8da44b11 
>   src/master/main.cpp f65ce637d77ce183f83b70dce6da8d0b4b8b8e71 
>   src/slave/flags.hpp 42c4861b5ecdc808d04bfe8f5c35572074fd2bdc 
>   src/slave/main.cpp f38fec6028ade0e0a51fd2cce7470c5c36e66396 
> 
> 
> Diff: https://reviews.apache.org/r/65090/diff/2/
> 
> 
> Testing
> ---
> 
> ```sh
> make -j12 check
> 
> # We don't seem to test flags in unit tests anywhere,
> # so additionally I ran:
> 
> docker pull zookeeper
> 
> cat < /tmp/$USER/zk-stack.yml
> version: '3.1'
> services:
>   zoo1:
> image: zookeeper
> restart: always
> hostname: zoo1
> ports:
>   - 2181:2181
> environment:
>   ZOO_MY_ID: 1
>   ZOO_SERVERS: server.1=0.0.0.0:2888:3888 server.2=zoo2:2888:3888 
> server.3=zoo3:2888:3888
>   zoo2:
> image: zookeeper
> restart: always
> hostname: zoo2
> ports:
>   - 2182:2181
> environment:
>   ZOO_MY_ID: 2
>   ZOO_SERVERS: server.1=zoo1:2888:3888 server.2=0.0.0.0:2888:3888 
> server.3=zoo3:2888:3888
>   zoo3:
> image: zookeeper
> restart: always
> hostname: zoo3
> ports:
>   - 2183:2181
> environment:
>   ZOO_MY_ID: 2
>   ZOO_SERVERS: server.1=zoo1:2888:3888 server.2=zoo2:2888:3888 
> server.3=0.0.0.0:2888:3888
> EOF
> 
> docker-compose -f /tmp/$USER/zk-stack.yml up
> 
> cd ${MESOS_BUILD_DIR}
> 
> # This command should fail to launch because there is no file zk.conf
> ./bin/mesos-master.sh \
> --work_dir=/tmp/$USER/mesos/master \
> --log_dir=/tmp/$USER/mesos/master/log \
> --ip=$PUBLIC_IP \
> --quorum=1 \
> --zk=file:///tmp/$USER/zk/zk.conf
> 
> cat < /tmp/$USER/zk/zk.conf
> zk://$PUBLIC_IP:2181,$PUBLIC_IP:2182,$PUBLIC_IP:2183/mesos
> EOF
> 
> ./bin/mesos-master.sh \
> --work_dir=/tmp/$USER/mesos/master \
> --log_dir=/tmp/$USER/mesos/master/log \
> --ip=$PUBLIC_IP \
> --quorum=1 \
> --zk=`cat /tmp/$USER/zk/zk.conf`  &
> 
> [[ $(http -b $PUBLIC_IP:5050/flags | jq -r '.flags.zk') == `cat 
> /tmp/$USER/zk/zk.conf` ]]
> 
> kill %1
> 
> 
> ./bin/mesos-master.sh \
> --work_dir=/tmp/$USER/mesos/master \
> --log_dir=/tmp/$USER/mesos/master/log \
> --ip=$PUBLIC_IP \
> --quorum=1 \
> --zk=file:///tmp/$USER/zk/zk.conf &
> 
> [[ $(http -b $PUBLIC_IP:5050/flags | jq -r '.flags.zk') == 
> "/tmp/$USER/zk/zk.conf" ]]
> 
> ./bin/mesos-agent.sh \
> --work_dir=/tmp/$USER/mesos/agent \
> --log_dir=/tmp/$USER/mesos/agent/log \
> --master=file:///tmp/$USER/zk/zk.conf &
> 
> [[ $(http -b $PUBLIC_IP:5051/flags | jq -r '.flags.master') == 
> "/tmp/$USER/zk/zk.conf" ]]
> 
> kill %2
> 
> ./bin/mesos-agent.sh \
> --work_dir=/tmp/$USER/mesos/agent \
> --log_dir=/tmp/$USER/mesos/agent/log \
> --zk=`cat /tmp/$USER/zk/zk.conf`  &
> 
> [[ $(http -b $PUBLIC_IP:5051/flags | jq -r '.flags.master') == `cat 
> /tmp/$USER/zk/zk.conf` ]]
> 
> kill %2
> kill %1
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 65226: Added SecurePathOrValue for file flags which need not to leak contents.

[Greg Mann]

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


Fix it, then Ship it!




Awesome, thanks Alexander :)


3rdparty/stout/include/stout/flags/fetch.hpp
Lines 70-72 (patched)


Could you leave a comment here explaining why we don't fetch as normal for 
this type?



3rdparty/stout/include/stout/flags/flag.hpp
Lines 104-106 (patched)


This will fit on two lines.



3rdparty/stout/include/stout/flags/flag.hpp
Lines 114-115 (patched)


I think our usual wrapping here would be

```
inline std::ostream& operator<<(
std::ostream ,
const SecurePathOrValue& flag)
```

?


- Greg Mann


On Jan. 18, 2018, 6:53 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65226/
> ---
> 
> (Updated Jan. 18, 2018, 6:53 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8413
> https://issues.apache.org/jira/browse/MESOS-8413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default behavior when serializing a flag, if its value comes from
> a file (by using the prefix `file://`) is to show the contents of
> the file instead of the path to the file.
> 
> A common pattern for flags containing sensitive information is to
> store the values in a secure file, however the default behavior
> of serializing flags causes these values to be leaked when
> serializing a subclass of `FlagsBase`.
> 
> This patch introduces the structure `SecurePathOrValue` so that
> the default behavior is overriden and the path of the flag will
> be shown instead of the flag value itself.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/fetch.hpp 
> 788d09e0a2083ead7d6ccb0c233cfdfd5d1ddb81 
>   3rdparty/stout/include/stout/flags/flag.hpp 
> 6580c294fc5d06854eadc19b1515172a4a700e89 
>   3rdparty/stout/include/stout/flags/parse.hpp 
> 6112d699be4cbdcb7a7872da8f29d41aab0939a3 
> 
> 
> Diff: https://reviews.apache.org/r/65226/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> make check
> ```
> 
> Full testing in follow up patch.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 65227: Added specific flag loader for zookeeper urls preventing password leaks.

[Alexander Rojas]

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

Review request for mesos.


Repository: mesos


Description
---

Adds the struct `zookeeper::SafeUrlLoader` which is used to load
zookeeper url's from flags.

This new struct will not shown the contents of a file in logs or the
`/flags` endpoint if the urls are given using a file (with the
`file://` prefix). If the url's are given in the command line, not
changes are noticed.


Testing
---

```sh
make -j12 check

# We don't seem to test flags in unit tests anywhere,
# so additionally I ran:

docker pull zookeeper

cat < /tmp/$USER/zk-stack.yml
version: '3.1'
services:
  zoo1:
image: zookeeper
restart: always
hostname: zoo1
ports:
  - 2181:2181
environment:
  ZOO_MY_ID: 1
  ZOO_SERVERS: server.1=0.0.0.0:2888:3888 server.2=zoo2:2888:3888 
server.3=zoo3:2888:3888
  zoo2:
image: zookeeper
restart: always
hostname: zoo2
ports:
  - 2182:2181
environment:
  ZOO_MY_ID: 2
  ZOO_SERVERS: server.1=zoo1:2888:3888 server.2=0.0.0.0:2888:3888 
server.3=zoo3:2888:3888
  zoo3:
image: zookeeper
restart: always
hostname: zoo3
ports:
  - 2183:2181
environment:
  ZOO_MY_ID: 2
  ZOO_SERVERS: server.1=zoo1:2888:3888 server.2=zoo2:2888:3888 
server.3=0.0.0.0:2888:3888
EOF

docker-compose -f /tmp/$USER/zk-stack.yml up

cd ${MESOS_BUILD_DIR}

# This command should fail to launch because there is no file zk.conf
./bin/mesos-master.sh \
--work_dir=/tmp/$USER/mesos/master \
--log_dir=/tmp/$USER/mesos/master/log \
--ip=$PUBLIC_IP \
--quorum=1 \
--zk=file:///tmp/$USER/zk/zk.conf

cat < /tmp/$USER/zk/zk.conf
zk://$PUBLIC_IP:2181,$PUBLIC_IP:2182,$PUBLIC_IP:2183/mesos
EOF

./bin/mesos-master.sh \
--work_dir=/tmp/$USER/mesos/master \
--log_dir=/tmp/$USER/mesos/master/log \
--ip=$PUBLIC_IP \
--quorum=1 \
--zk=`cat /tmp/$USER/zk/zk.conf`  &

[[ $(http -b $PUBLIC_IP:5050/flags | jq -r '.flags.zk') == `cat 
/tmp/$USER/zk/zk.conf` ]]

kill %1


./bin/mesos-master.sh \
--work_dir=/tmp/$USER/mesos/master \
--log_dir=/tmp/$USER/mesos/master/log \
--ip=$PUBLIC_IP \
--quorum=1 \
--zk=file:///tmp/$USER/zk/zk.conf &

[[ $(http -b $PUBLIC_IP:5050/flags | jq -r '.flags.zk') == 
"/tmp/$USER/zk/zk.conf" ]]

./bin/mesos-agent.sh \
--work_dir=/tmp/$USER/mesos/agent \
--log_dir=/tmp/$USER/mesos/agent/log \
--master=file:///tmp/$USER/zk/zk.conf &

[[ $(http -b $PUBLIC_IP:5051/flags | jq -r '.flags.master') == 
"/tmp/$USER/zk/zk.conf" ]]

kill %2

./bin/mesos-agent.sh \
--work_dir=/tmp/$USER/mesos/agent \
--log_dir=/tmp/$USER/mesos/agent/log \
--zk=`cat /tmp/$USER/zk/zk.conf`  &

[[ $(http -b $PUBLIC_IP:5051/flags | jq -r '.flags.master') == `cat 
/tmp/$USER/zk/zk.conf` ]]

kill %2
kill %1
```


Thanks,

Alexander Rojas

Re: Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Chun-Hung Hsiao]

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

(Updated Jan. 19, 2018, 12:48 a.m.)


Review request for mesos, Michael Park, Qian Zhang, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Fixed detaching task volume directories of destroyed frameworks.


Diffs (updated)
-

  src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
  src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65232: Resumed the clock if necessary when destroying test agent.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On Jan. 18, 2018, 4:40 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65232/
> ---
> 
> (Updated Jan. 18, 2018, 4:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gaston Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Because the `cgroups::destroy()` code path makes use of `delay()`,
> the clock must not be paused in order for the destructor of the
> test `Slave` to reliably destroy all remaining containers.
> 
> This patch updates the destructor to check if the clock is paused
> and, if so, resume it before destroying containers.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 568c9c7eda9d69e85bc32ce3259548b348ed9f25 
> 
> 
> Diff: https://reviews.apache.org/r/65232/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 65039: Tested reconciliation when operation is dropped en route to agent.

[Greg Mann]

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

(Updated Jan. 19, 2018, 12:42 a.m.)


Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
and Jie Yu.


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


Repository: mesos


Description
---

This patch adds
StorageLocalResourceProviderTest.ROOT_ReconcileDroppedOperation
in order to verify that reconciliation is performed correctly
when an operation is dropped on its way from the master to the
agent.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
bbfe95e9818f25fdd5405db3ad2fe355e023f743 


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

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


Testing
---

sudo make check


Thanks,

Greg Mann

Re: Review Request 65039: Tested reconciliation when operation is dropped en route to agent.

[Greg Mann]

> On Jan. 17, 2018, 8:44 a.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 1797 (patched)
> > 
> >
> > This seems unneeded.
> 
> Greg Mann wrote:
> If the clock isn't resumed at the end of the test, teardown will not 
> complete successfully. The destruction of containers in `Slave::~Slave()` in 
> 'cluster.cpp' requires the clock to be running in order to finish.
> 
> Benjamin Bannier wrote:
> That sounds like a bug in `cluster::Slave::~Slave`, and I do not see this 
> explicit `resume` pattern elsewhere a lot either.
> 
> Adding an explicit `resume` also does not help in cases where an 
> assertion before the `resume` leads to the rest of the test being skipped. 
> The only way to make sure it is called is to invoke it in some destructor 
> RAII style.
> 
> I'd suggest to drop the `resume` here for consistency, and instead fix 
> the destructor of `~Slave` to make it self-sufficient (either file a ticket 
> or add a cleanup).

Thanks for pushing on this. After further investigation, it turns out that the 
`cgroups::destroy()` code path makes use of `delay()`. Thus, the clock cannot 
be paused in order for code like the tests' `Slave::~Slave()` to reliably 
destroy containers. In fact, we have other test fixtures like 
`ContainerizerTest` which already resume/pause the clock when necessary for 
this reason.

I posted a patch to add this behavior to the destructor, and removed the 
`Clock::resume()` call from this test: https://reviews.apache.org/r/65232/


- Greg


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


On Jan. 18, 2018, 1:57 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65039/
> ---
> 
> (Updated Jan. 18, 2018, 1:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> and Jie Yu.
> 
> 
> Bugs: MESOS-8373
> https://issues.apache.org/jira/browse/MESOS-8373
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds
> StorageLocalResourceProviderTest.ROOT_ReconcileDroppedOperation
> in order to verify that reconciliation is performed correctly
> when an operation is dropped on its way from the master to the
> agent.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65039/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 65232: Resumed the clock if necessary when destroying test agent.

[Greg Mann]

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

Review request for mesos, Benjamin Bannier and Gaston Kleiman.


Repository: mesos


Description
---

Because the `cgroups::destroy()` code path makes use of `delay()`,
the clock must not be paused in order for the destructor of the
test `Slave` to reliably destroy all remaining containers.

This patch updates the destructor to check if the clock is paused
and, if so, resume it before destroying containers.


Diffs
-

  src/tests/cluster.cpp 568c9c7eda9d69e85bc32ce3259548b348ed9f25 


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


Testing
---

make check


Thanks,

Greg Mann

Review Request 65231: Fixed detaching task volume directories of destroyed frameworks.

[Chun-Hung Hsiao]

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

Review request for mesos, Michael Park and Vinod Kone.


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


Repository: mesos


Description
---

This patch prevents deferencing destroyed executors when detaching task
volume directories from the `/files` endpoint.


Diffs
-

  src/slave/slave.hpp a07f046c7ed980bed3e3d0d2780295727b87ee44 
  src/slave/slave.cpp 1672c06894c25e7120bb8a1da15f7d6432eb5954 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 64970: Use tox for linting and testing code living uder src/python.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64970 was successfully built and tested.

Reviews applied: `['64970']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 11:46 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64970/
> ---
> 
> (Updated Jan. 18, 2018, 11:46 p.m.)
> 
> 
> Review request for mesos, Armand Grillet and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use tox for linting and testing code living uder src/python.
> 
> At the moment, all linting is done through the same `pylint`
> installation under support/.virtualenv, which requires ALL dependencies
> (i.e. pip-requirements.txt, requirements.in scattered in various
> directories) to be installed in the same virtualenv, making things
> really messy -- e.g. when I've changed some code under `src/python/lib`,
> but don't have the dev virtualenv activated, linting will fail since
> none of the dependencies under `src/python/lib` have been installed.
> 
> Using tox, we can solve this problem by distributing a "test spec"
> (tox.ini) in each of the python source directories which are aware of
> its local dependencies only. To test or lint the code there would be as
> simple as running `tox -e py27-lint `, and the corresponding
> virtualenv and test dependencies would automatically be setup.
> 
> This patch modifies `support/mesos-style.py` to install `tox` in
> `support/.virtualenv` and delegates linting to a `tox` call when it sees
> python directories that have tox setup for it. Linting for all other
> languages will not be effected.
> 
> Testing Done:
> 1. intentionally create a lint error, such as extra spaces before a
> parens in a python file
> 2. run the pre-commit hook and see tox in action
> 
> Reviewed at https://reviews.apache.org/r/64970/
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/tox.ini PRE-CREATION 
>   src/python/lib/requirements-test.in 
> b2b73aab65377d9310797203ea84c5150ae60805 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/mesos-style.py 1b34ea2d9afa8f17b545841cea7a6853a6e18144 
> 
> 
> Diff: https://reviews.apache.org/r/64970/diff/3/
> 
> 
> Testing
> ---
> 
> 1. intentionally create a lint error, such as extra spaces before a parens in 
> a python file
> 2. run the pre-commit hook and see tox in action
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 64970: Replace ad hoc venv under support/ with tox.

[Eric Chung]

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

(Updated Jan. 18, 2018, 11:46 p.m.)


Review request for mesos, Armand Grillet and Kevin Klues.


Repository: mesos


Description (updated)
---

Use tox for linting and testing code living uder src/python.

At the moment, all linting is done through the same `pylint`
installation under support/.virtualenv, which requires ALL dependencies
(i.e. pip-requirements.txt, requirements.in scattered in various
directories) to be installed in the same virtualenv, making things
really messy -- e.g. when I've changed some code under `src/python/lib`,
but don't have the dev virtualenv activated, linting will fail since
none of the dependencies under `src/python/lib` have been installed.

Using tox, we can solve this problem by distributing a "test spec"
(tox.ini) in each of the python source directories which are aware of
its local dependencies only. To test or lint the code there would be as
simple as running `tox -e py27-lint `, and the corresponding
virtualenv and test dependencies would automatically be setup.

This patch modifies `support/mesos-style.py` to install `tox` in
`support/.virtualenv` and delegates linting to a `tox` call when it sees
python directories that have tox setup for it. Linting for all other
languages will not be effected.

Testing Done:
1. intentionally create a lint error, such as extra spaces before a
parens in a python file
2. run the pre-commit hook and see tox in action

Reviewed at https://reviews.apache.org/r/64970/


Diffs (updated)
-

  src/python/cli_new/tox.ini PRE-CREATION 
  src/python/lib/requirements-test.in b2b73aab65377d9310797203ea84c5150ae60805 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
  support/mesos-style.py 1b34ea2d9afa8f17b545841cea7a6853a6e18144 


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

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


Testing
---

1. intentionally create a lint error, such as extra spaces before a parens in a 
python file
2. run the pre-commit hook and see tox in action


Thanks,

Eric Chung

Re: Review Request 64970: Replace ad hoc venv under support/ with tox.

[Eric Chung]

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

(Updated Jan. 18, 2018, 11:20 p.m.)


Review request for mesos, Armand Grillet and Kevin Klues.


Repository: mesos


Description (updated)
---

Use tox for linting and testing code living uder src/python.

At the moment, all linting is done through the same `pylint` installation under 
support/.virtualenv, which requires ALL dependencies (i.e. 
pip-requirements.txt, requirements.in scattered in various directories) to be 
installed in the same virtualenv, making things really messy -- e.g. when I've 
changed some code under `src/python/lib`, but don't have the dev virtualenv 
activated, linting will fail since none of the dependencies under 
`src/python/lib` have been installed.

Using tox, we can solve this problem by distributing a "test spec" (tox.ini) in 
each of the python source directories which are aware of its local dependencies 
only. To test or lint the code there would be as simple as running `tox -e 
py27-lint `, and the corresponding virtualenv and test dependencies 
would automatically be setup.

This patch modifies `support/mesos-style.py` to install `tox` in 
`support/.virtualenv` and delegates linting to a `tox` call when it sees python 
directories that have tox setup for it. Linting for all other languages will 
not be effected.


Diffs (updated)
-

  src/python/cli_new/tox.ini PRE-CREATION 
  src/python/lib/requirements-test.in b2b73aab65377d9310797203ea84c5150ae60805 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
  support/mesos-style.py 1b34ea2d9afa8f17b545841cea7a6853a6e18144 


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

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


Testing
---

1. intentionally create a lint error, such as extra spaces before a parens in a 
python file
2. run the pre-commit hook and see tox in action


Thanks,

Eric Chung

Re: Review Request 65229: Made `GetNodeID` call depend on `PUBLISH_UNPUBLISH_VOLUME` capability.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65229 was successfully built and tested.

Reviews applied: `['65229']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 8:58 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65229/
> ---
> 
> (Updated Jan. 18, 2018, 8:58 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-8461
> https://issues.apache.org/jira/browse/MESOS-8461
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `GetNodeID` call depend on `PUBLISH_UNPUBLISH_VOLUME` capability.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 9a322049843b282e070d05cb7ecae745f4b40145 
> 
> 
> Diff: https://reviews.apache.org/r/65229/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> Tested the chain of https://reviews.apache.org/r/65062/
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65229: Made `GetNodeID` call depend on `PUBLISH_UNPUBLISH_VOLUME` capability.

[Chun-Hung Hsiao]

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

(Updated Jan. 18, 2018, 8:58 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Made `GetNodeID` call depend on `PUBLISH_UNPUBLISH_VOLUME` capability.


Diffs
-

  src/resource_provider/storage/provider.cpp 
9a322049843b282e070d05cb7ecae745f4b40145 


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


Testing
---

sudo make check
Tested the chain of https://reviews.apache.org/r/65062/


Thanks,

Chun-Hung Hsiao

Re: Review Request 65229: Made `GetNodeID` call depend on `PUBLISH_UNPUBLISH_VOLUME` capability.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 18, 2018, 8:21 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65229/
> ---
> 
> (Updated Jan. 18, 2018, 8:21 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `GetNodeID` call depend on `PUBLISH_UNPUBLISH_VOLUME` capability.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 9a322049843b282e070d05cb7ecae745f4b40145 
> 
> 
> Diff: https://reviews.apache.org/r/65229/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> Tested the chain of https://reviews.apache.org/r/65062/
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 65229: Made `GetNodeID` call depend on `PUBLISH_UNPUBLISH_VOLUME` capability.

[Chun-Hung Hsiao]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Made `GetNodeID` call depend on `PUBLISH_UNPUBLISH_VOLUME` capability.


Diffs
-

  src/resource_provider/storage/provider.cpp 
9a322049843b282e070d05cb7ecae745f4b40145 


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


Testing
---

sudo make check
Tested the chain of https://reviews.apache.org/r/65062/


Thanks,

Chun-Hung Hsiao

Re: Review Request 65090: Mesos flags related to ZooKeeper use SecurePathOrValue.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65090 was successfully built and tested.

Reviews applied: `['65226', '65090']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 7:01 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65090/
> ---
> 
> (Updated Jan. 18, 2018, 7:01 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8413
> https://issues.apache.org/jira/browse/MESOS-8413
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up until now the Mesos master flag `--zk` as well as the Mesos agent
> flag `--master` would leak ZooKeeper authentication credentials in
> both logs and results for the `/flags` endpoint, if the credentials
> were part of the configuration url.
> 
> This prevents this leakage if a user decides to store the ZooKeeper
> url in a file and pass the file as a value to the flags mentioned
> above (using the preffix `file://`).
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp dabb414560f506787b6e821a27af623c8da44b11 
>   src/master/main.cpp f65ce637d77ce183f83b70dce6da8d0b4b8b8e71 
>   src/slave/flags.hpp 42c4861b5ecdc808d04bfe8f5c35572074fd2bdc 
>   src/slave/main.cpp f38fec6028ade0e0a51fd2cce7470c5c36e66396 
> 
> 
> Diff: https://reviews.apache.org/r/65090/diff/2/
> 
> 
> Testing
> ---
> 
> ```sh
> make -j12 check
> 
> # We don't seem to test flags in unit tests anywhere,
> # so additionally I ran:
> 
> docker pull zookeeper
> 
> cat < /tmp/$USER/zk-stack.yml
> version: '3.1'
> services:
>   zoo1:
> image: zookeeper
> restart: always
> hostname: zoo1
> ports:
>   - 2181:2181
> environment:
>   ZOO_MY_ID: 1
>   ZOO_SERVERS: server.1=0.0.0.0:2888:3888 server.2=zoo2:2888:3888 
> server.3=zoo3:2888:3888
>   zoo2:
> image: zookeeper
> restart: always
> hostname: zoo2
> ports:
>   - 2182:2181
> environment:
>   ZOO_MY_ID: 2
>   ZOO_SERVERS: server.1=zoo1:2888:3888 server.2=0.0.0.0:2888:3888 
> server.3=zoo3:2888:3888
>   zoo3:
> image: zookeeper
> restart: always
> hostname: zoo3
> ports:
>   - 2183:2181
> environment:
>   ZOO_MY_ID: 2
>   ZOO_SERVERS: server.1=zoo1:2888:3888 server.2=zoo2:2888:3888 
> server.3=0.0.0.0:2888:3888
> EOF
> 
> docker-compose -f /tmp/$USER/zk-stack.yml up
> 
> cd ${MESOS_BUILD_DIR}
> 
> # This command should fail to launch because there is no file zk.conf
> ./bin/mesos-master.sh \
> --work_dir=/tmp/$USER/mesos/master \
> --log_dir=/tmp/$USER/mesos/master/log \
> --ip=$PUBLIC_IP \
> --quorum=1 \
> --zk=file:///tmp/$USER/zk/zk.conf
> 
> cat < /tmp/$USER/zk/zk.conf
> zk://$PUBLIC_IP:2181,$PUBLIC_IP:2182,$PUBLIC_IP:2183/mesos
> EOF
> 
> ./bin/mesos-master.sh \
> --work_dir=/tmp/$USER/mesos/master \
> --log_dir=/tmp/$USER/mesos/master/log \
> --ip=$PUBLIC_IP \
> --quorum=1 \
> --zk=`cat /tmp/$USER/zk/zk.conf`  &
> 
> [[ $(http -b $PUBLIC_IP:5050/flags | jq -r '.flags.zk') == `cat 
> /tmp/$USER/zk/zk.conf` ]]
> 
> kill %1
> 
> 
> ./bin/mesos-master.sh \
> --work_dir=/tmp/$USER/mesos/master \
> --log_dir=/tmp/$USER/mesos/master/log \
> --ip=$PUBLIC_IP \
> --quorum=1 \
> --zk=file:///tmp/$USER/zk/zk.conf &
> 
> [[ $(http -b $PUBLIC_IP:5050/flags | jq -r '.flags.zk') == 
> "/tmp/$USER/zk/zk.conf" ]]
> 
> ./bin/mesos-agent.sh \
> --work_dir=/tmp/$USER/mesos/agent \
> --log_dir=/tmp/$USER/mesos/agent/log \
> --master=file:///tmp/$USER/zk/zk.conf &
> 
> [[ $(http -b $PUBLIC_IP:5051/flags | jq -r '.flags.master') == 
> "/tmp/$USER/zk/zk.conf" ]]
> 
> kill %2
> 
> ./bin/mesos-agent.sh \
> --work_dir=/tmp/$USER/mesos/agent \
> --log_dir=/tmp/$USER/mesos/agent/log \
> --zk=`cat /tmp/$USER/zk/zk.conf`  &
> 
> [[ $(http -b $PUBLIC_IP:5051/flags | jq -r '.flags.master') == `cat 
> /tmp/$USER/zk/zk.conf` ]]
> 
> kill %2
> kill %1
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 65059: Speeded up SLRP unit tests.

[Chun-Hung Hsiao]

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

(Updated Jan. 18, 2018, 7:30 p.m.)


Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.


Changes
---

Made changes for style consistency.


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


Repository: mesos


Description
---

This patch makes all SLRP unit tests to use an allocation interval of
50ms and decline offers with filters.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
bbfe95e9818f25fdd5405db3ad2fe355e023f743 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65057: Tested that op status updates dropped en route to master are resent.

[Greg Mann]

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




src/tests/storage_local_resource_provider_tests.cpp
Lines 2132 (patched)


Maybe "To accomplish this:"



src/tests/storage_local_resource_provider_tests.cpp
Lines 2134 (patched)


s/message//



src/tests/storage_local_resource_provider_tests.cpp
Lines 2172-2179 (patched)


Could you also add a note here regarding why the order of these two is 
reversed? Chun-Hung and I have such a comment in ours if you want to borrow it 
for consistency :)



src/tests/storage_local_resource_provider_tests.cpp
Lines 2189-2191 (patched)


Could you leave a more verbose comment here as to why this is necessary? 
Chun-Hung and my patches have one if you care to borrow for consistency :)



src/tests/storage_local_resource_provider_tests.cpp
Lines 2226 (patched)


Do we need this, or just being careful? If it is needed, it might not be if 
we add a long filter to the accept call.



src/tests/storage_local_resource_provider_tests.cpp
Lines 2278 (patched)


Benjamin suggested on my RR that we figure out the issue in 
`Slave::~Slave()` that creates the need for this resume. Looking at some other 
paused tests in the codebase, many of them do not need to resume.

I agree that it's a good idea for us to fix this in 'cluster.cpp' and 
eliminate the need for this resume.


- Greg Mann


On Jan. 10, 2018, 12:25 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65057/
> ---
> 
> (Updated Jan. 10, 2018, 12:25 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8363 and MESOS-8420
> https://issues.apache.org/jira/browse/MESOS-8363
> https://issues.apache.org/jira/browse/MESOS-8420
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds
> `StorageLocalResourceProviderTest.ROOT_RetryOperationStatusUpdate` that
> verifies that operation status updates are resent to the master after
> being dropped en route to it.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65057/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh 
> --gtest_filter='StorageLocalResourceProviderTest.ROOT_RetryOperationStatusUpdate'
>  --gtest_repeat=100 --gtest_break_on_failure` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65090: Mesos flags related to ZooKeeper use SecurePathOrValue.

[Alexander Rojas]

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

(Updated Jan. 18, 2018, 8:01 p.m.)


Review request for mesos and Greg Mann.


Summary (updated)
-

Mesos flags related to ZooKeeper use SecurePathOrValue.


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


Repository: mesos


Description (updated)
---

Up until now the Mesos master flag `--zk` as well as the Mesos agent
flag `--master` would leak ZooKeeper authentication credentials in
both logs and results for the `/flags` endpoint, if the credentials
were part of the configuration url.

This prevents this leakage if a user decides to store the ZooKeeper
url in a file and pass the file as a value to the flags mentioned
above (using the preffix `file://`).


Diffs (updated)
-

  src/master/flags.hpp dabb414560f506787b6e821a27af623c8da44b11 
  src/master/main.cpp f65ce637d77ce183f83b70dce6da8d0b4b8b8e71 
  src/slave/flags.hpp 42c4861b5ecdc808d04bfe8f5c35572074fd2bdc 
  src/slave/main.cpp f38fec6028ade0e0a51fd2cce7470c5c36e66396 


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

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


Testing
---

```sh
make -j12 check

# We don't seem to test flags in unit tests anywhere,
# so additionally I ran:

docker pull zookeeper

cat < /tmp/$USER/zk-stack.yml
version: '3.1'
services:
  zoo1:
image: zookeeper
restart: always
hostname: zoo1
ports:
  - 2181:2181
environment:
  ZOO_MY_ID: 1
  ZOO_SERVERS: server.1=0.0.0.0:2888:3888 server.2=zoo2:2888:3888 
server.3=zoo3:2888:3888
  zoo2:
image: zookeeper
restart: always
hostname: zoo2
ports:
  - 2182:2181
environment:
  ZOO_MY_ID: 2
  ZOO_SERVERS: server.1=zoo1:2888:3888 server.2=0.0.0.0:2888:3888 
server.3=zoo3:2888:3888
  zoo3:
image: zookeeper
restart: always
hostname: zoo3
ports:
  - 2183:2181
environment:
  ZOO_MY_ID: 2
  ZOO_SERVERS: server.1=zoo1:2888:3888 server.2=zoo2:2888:3888 
server.3=0.0.0.0:2888:3888
EOF

docker-compose -f /tmp/$USER/zk-stack.yml up

cd ${MESOS_BUILD_DIR}

# This command should fail to launch because there is no file zk.conf
./bin/mesos-master.sh \
--work_dir=/tmp/$USER/mesos/master \
--log_dir=/tmp/$USER/mesos/master/log \
--ip=$PUBLIC_IP \
--quorum=1 \
--zk=file:///tmp/$USER/zk/zk.conf

cat < /tmp/$USER/zk/zk.conf
zk://$PUBLIC_IP:2181,$PUBLIC_IP:2182,$PUBLIC_IP:2183/mesos
EOF

./bin/mesos-master.sh \
--work_dir=/tmp/$USER/mesos/master \
--log_dir=/tmp/$USER/mesos/master/log \
--ip=$PUBLIC_IP \
--quorum=1 \
--zk=`cat /tmp/$USER/zk/zk.conf`  &

[[ $(http -b $PUBLIC_IP:5050/flags | jq -r '.flags.zk') == `cat 
/tmp/$USER/zk/zk.conf` ]]

kill %1


./bin/mesos-master.sh \
--work_dir=/tmp/$USER/mesos/master \
--log_dir=/tmp/$USER/mesos/master/log \
--ip=$PUBLIC_IP \
--quorum=1 \
--zk=file:///tmp/$USER/zk/zk.conf &

[[ $(http -b $PUBLIC_IP:5050/flags | jq -r '.flags.zk') == 
"/tmp/$USER/zk/zk.conf" ]]

./bin/mesos-agent.sh \
--work_dir=/tmp/$USER/mesos/agent \
--log_dir=/tmp/$USER/mesos/agent/log \
--master=file:///tmp/$USER/zk/zk.conf &

[[ $(http -b $PUBLIC_IP:5051/flags | jq -r '.flags.master') == 
"/tmp/$USER/zk/zk.conf" ]]

kill %2

./bin/mesos-agent.sh \
--work_dir=/tmp/$USER/mesos/agent \
--log_dir=/tmp/$USER/mesos/agent/log \
--zk=`cat /tmp/$USER/zk/zk.conf`  &

[[ $(http -b $PUBLIC_IP:5051/flags | jq -r '.flags.master') == `cat 
/tmp/$USER/zk/zk.conf` ]]

kill %2
kill %1
```


Thanks,

Alexander Rojas

Review Request 65226: Added SecurePathOrValue for file flags which need not to leak contents.

[Alexander Rojas]

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

Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

The default behavior when serializing a flag, if its value comes from
a file (by using the prefix `file://`) is to show the contents of
the file instead of the path to the file.

A common pattern for flags containing sensitive information is to
store the values in a secure file, however the default behavior
of serializing flags causes these values to be leaked when
serializing a subclass of `FlagsBase`.

This patch introduces the structure `SecurePathOrValue` so that
the default behavior is overriden and the path of the flag will
be shown instead of the flag value itself.


Diffs
-

  3rdparty/stout/include/stout/flags/fetch.hpp 
788d09e0a2083ead7d6ccb0c233cfdfd5d1ddb81 
  3rdparty/stout/include/stout/flags/flag.hpp 
6580c294fc5d06854eadc19b1515172a4a700e89 
  3rdparty/stout/include/stout/flags/parse.hpp 
6112d699be4cbdcb7a7872da8f29d41aab0939a3 


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


Testing
---

```
make check
```

Full testing in follow up patch.


Thanks,

Alexander Rojas

Re: Review Request 65032: Added a SLRP unit test for agent reboot.

[Greg Mann]

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




src/tests/storage_local_resource_provider_tests.cpp
Lines 1697 (patched)


s/agentFlags/slaveFlags/g

Or, let's just follow up at the end of the chain with a patch. I'm fine 
either way.



src/tests/storage_local_resource_provider_tests.cpp
Lines 1703-1711 (patched)


Ditto here as in parent review - I think we can make this more concise?



src/tests/storage_local_resource_provider_tests.cpp
Lines 1749 (patched)


I'm curious why you place all expectations together in some tests, and 
spread them throughout the test here and elsewhere?



src/tests/storage_local_resource_provider_tests.cpp
Lines 1753 (patched)


s/agent/slave/



src/tests/storage_local_resource_provider_tests.cpp
Lines 1759 (patched)


s/volume/volumes/



src/tests/storage_local_resource_provider_tests.cpp
Lines 1780-1781 (patched)


Do we need the `.Times(Atleast(1))` here? I think we can rely on a single 
offer being rescinded when the agent is terminated?



src/tests/storage_local_resource_provider_tests.cpp
Lines 1867 (patched)


Could we move the registering of this expectation out of this scope, and 
directly above the `StartSlave()` call below?


- Greg Mann


On Jan. 10, 2018, 2:36 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65032/
> ---
> 
> (Updated Jan. 10, 2018, 2:36 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8415
> https://issues.apache.org/jira/browse/MESOS-8415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a SLRP unit test for agent reboot.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65032/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64574: Ensured trailing '/' in URL is insignificant.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64574 was successfully built and tested.

Reviews applied: `['64574']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 5:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> ---
> 
> (Updated Jan. 18, 2018, 5:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> f79d366baecc73e6b14df593a906264f5fd1d72b 
> 
> 
> Diff: https://reviews.apache.org/r/64574/diff/2/
> 
> 
> Testing
> ---
> 
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 65223: Removed duplicated "/help" prefix in links in /help response.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65223 was successfully built and tested.

Reviews applied: `['65223']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 5:46 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65223/
> ---
> 
> (Updated Jan. 18, 2018, 5:46 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8458
> https://issues.apache.org/jira/browse/MESOS-8458
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to that patch, links generated as part of the /help endpoint
> response had the "/help/help/" format, which is incorrect.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/help.cpp 4e1faff7e2a0fb73ed444ce645613c98fb513e1c 
> 
> 
> Diff: https://reviews.apache.org/r/65223/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` on MacOS 10.13.2
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 65212: Updated `mesos-tidy` docker build setup for benchmarks.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 18, 2018, 5:24 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65212/
> ---
> 
> (Updated Jan. 18, 2018, 5:24 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the `mesos-tidy` docker build setup to generate
> `benchmarks.pb.h` needed by the libprocess benchmark files added as
> part of `eb901173b8aa43b9fd1ab0f727ff5187982c4449`.
> 
> We also reorder the targets to be build in the script so that all
> proto targets are located close to each other and after manually
> triggered 3rdparty dependencies (like e.g., protobuf). This should
> reduce redudant build invocations.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/entrypoint.sh dd83d66f48968bce652bc78ccf2d807949f1725b 
> 
> 
> Diff: https://reviews.apache.org/r/65212/diff/2/
> 
> 
> Testing
> ---
> 
> Tested with a custom docker image and `support/mesos-tidy.sh` pointing to it.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 65223: Removed duplicated "/help" prefix in links in /help response.

[Alexander Rukletsov]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Prior to that patch, links generated as part of the /help endpoint
response had the "/help/help/" format, which is incorrect.


Diffs
-

  3rdparty/libprocess/src/help.cpp 4e1faff7e2a0fb73ed444ce645613c98fb513e1c 


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


Testing
---

`make check` on MacOS 10.13.2


Thanks,

Alexander Rukletsov

Re: Review Request 64574: Ensured trailing '/' in URL is insignificant.

[Alexander Rukletsov]

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

(Updated Jan. 18, 2018, 5:46 p.m.)


Review request for mesos, Benjamin Bannier and Benjamin Mahler.


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


Repository: mesos


Description
---

Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
"/state/", yielded a 404 response. This patch ensures that two URLs
which differ only in trailing '/' produce the same result.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad 
  3rdparty/libprocess/src/tests/http_tests.cpp 
f79d366baecc73e6b14df593a906264f5fd1d72b 


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

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


Testing
---

Ensured the modified test fails without the fix.
`make check` on Mac OS 10.11.6 and several Linux distributions.


Thanks,

Alexander Rukletsov

Re: Review Request 64992: Added SLRP unit tests for profile updates and corner cases.

[Chun-Hung Hsiao]

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

(Updated Jan. 18, 2018, 5:30 p.m.)


Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.


Changes
---

Made changes for style consistency.


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


Repository: mesos


Description
---

This patch adds the following tests:

`NoResource`: RP updates its state with no resource, and can recover
from a checkpointed state that contains no resource.

`ZeroSizedDisk`: CSI plugin reports a pre-existing volume with
zero capacity.

`SmallDisk`: CSI plugin reports a storage pool and a pre-existing volume
with sizes < 1MB.

`NewProfile`: A new profile is added after RP updates its state.


Diffs (updated)
-

  src/examples/test_csi_plugin.cpp f6b2c98c44366d5c752aa44bdbf8ae0374a7765c 
  src/tests/storage_local_resource_provider_tests.cpp 
bbfe95e9818f25fdd5405db3ad2fe355e023f743 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 61811: Flattened continuation chains in containerizer for readability.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 18, 2018, 5:11 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61811/
> ---
> 
> (Updated Jan. 18, 2018, 5:11 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f69f65a5033ce0c1fa2fe9591d8c6bbada5e09fb 
> 
> 
> Diff: https://reviews.apache.org/r/61811/diff/4/
> 
> 
> Testing
> ---
> 
> make check on:
> `Apple LLVM version 8.0.0 (clang-800.0.42.1)` on Mac OS 10.11.6
> `c++ (GCC) 6.2.1 20160916 (Red Hat 6.2.1-2)` on Fedora 24
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 65045: Tested correct operation handling during master failover.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65045 was successfully built and tested.

Reviews applied: `['65044', '65045']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 2:11 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65045/
> ---
> 
> (Updated Jan. 18, 2018, 2:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tested correct operation handling during master failover.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp d01f3fbdd688ddd31fb0c777f973928f5b5fa5e7 
> 
> 
> Diff: https://reviews.apache.org/r/65045/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 64574: Ensured trailing '/' in URL is insignificant.

[Alexander Rukletsov]

> On Dec. 19, 2017, 1:08 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 3530-3531 (patched)
> > 
> >
> > This approach doesn't seem quite right to me, since it prevents the 
> > user of libprocess from seeing what the original path was. Have you 
> > considered any other approaches? (also I think you only needed to trim from 
> > the tail?)
> 
> Alexander Rukletsov wrote:
> Why so? `name` is a local variable based on request path and used to find 
> appropriate handler. A user of libprocess can still access 
> `request->url.path`.
> 
> I do trim from the tail only (see two lines above).
> 
> I did consider returning 301 with a trimmed path, but decided not to 
> since I'm not sure we're allowed to return 301 codes in response to operator 
> API endpoints.
> 
> Benjamin Mahler wrote:
> Ah yes, I see that it's a local variable now, sorry about that!
> 
> A couple of questions:
> 
> (1) What happens on the `route()` side? Can a caller setup two routes for 
> "path" and "path/"? And if so, what happens when requests come in for "path" 
> and "path/"?
> 
> (2) Maybe we could preserve the original trimming of prefix (since it 
> belongs with the removal of the "ID" comment), and instead add the suffix 
> stripping along with a comment about the overall idea here? As it stands, I 
> think the reader will have a hard time realizing that we've decided to treat 
> trailing slash paths the same as non-trailing slash paths?

(1) Currently, libprocess allows to route both `"/path"` and `"/path/"` and 
moreover to different handlers. With this patch, route to `"/path/"` is not 
wired and hence has no effects. We can do following things to improve the logic 
here:
  - disallow `"/path/"` in `route()`
  - normalize `"/path/"` to `"/path"` in `route()` (might result in handler 
overwrite)
  - allow both `"/path/"` and `"/path"` and try both if `request.path` handler 
is not found
I have a light tendency to the first option because of its simplicity. Note 
that we already normalize `"/path/"` to `"/path"` on [the help 
page](https://github.com/apache/mesos/blob/634c8af2618c57a1405d20717fa909b399486f37/3rdparty/libprocess/src/help.cpp#L95-L98).

(2) Sure.


- Alexander


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


On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> ---
> 
> (Updated Dec. 13, 2017, 1:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 9daac715f0242921b7f9f5c20b3eb27f1be802d4 
> 
> 
> Diff: https://reviews.apache.org/r/64574/diff/1/
> 
> 
> Testing
> ---
> 
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 65212: Updated `mesos-tidy` docker build setup for benchmarks.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65212 was successfully built and tested.

Reviews applied: `['65212']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 1:24 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65212/
> ---
> 
> (Updated Jan. 18, 2018, 1:24 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the `mesos-tidy` docker build setup to generate
> `benchmarks.pb.h` needed by the libprocess benchmark files added as
> part of `eb901173b8aa43b9fd1ab0f727ff5187982c4449`.
> 
> We also reorder the targets to be build in the script so that all
> proto targets are located close to each other and after manually
> triggered 3rdparty dependencies (like e.g., protobuf). This should
> reduce redudant build invocations.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/entrypoint.sh dd83d66f48968bce652bc78ccf2d807949f1725b 
> 
> 
> Diff: https://reviews.apache.org/r/65212/diff/2/
> 
> 
> Testing
> ---
> 
> Tested with a custom docker image and `support/mesos-tidy.sh` pointing to it.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 64211: Added configure/make options to build the new CLI and run unit tests.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64211 was successfully built and tested.

Reviews applied: `['64211']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 2:03 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64211/
> ---
> 
> (Updated Jan. 18, 2018, 2:03 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-8240
> https://issues.apache.org/jira/browse/MESOS-8240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An update of the discarded review /r/52543.
> 
> 
> Diffs
> -
> 
>   configure.ac 6abec5f6d7255a6a166ce11fbedd002ecbfae965 
>   src/Makefile.am 30cd4d426e797e4c8ee556d1bc3de99830a5fe41 
> 
> 
> Diff: https://reviews.apache.org/r/64211/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ ../configure --enable-new-cli --disable-java
> $ make check
> ```
> Checked that the the CLI tests were run, that the content of the directory 
> `build/src/cli` was as expected, and that `build/src/mesos/mesos` was 
> correctly running.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61811: Flattened continuation chains in containerizer for readability.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 61811 was successfully built and tested.

Reviews applied: `['61811']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 1:11 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61811/
> ---
> 
> (Updated Jan. 18, 2018, 1:11 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f69f65a5033ce0c1fa2fe9591d8c6bbada5e09fb 
> 
> 
> Diff: https://reviews.apache.org/r/61811/diff/4/
> 
> 
> Testing
> ---
> 
> make check on:
> `Apple LLVM version 8.0.0 (clang-800.0.42.1)` on Mac OS 10.11.6
> `c++ (GCC) 6.2.1 20160916 (Red Hat 6.2.1-2)` on Fedora 24
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Jan Schlicht]

> On Jan. 17, 2018, 10:18 a.m., Benjamin Bannier wrote:
> > src/tests/api_tests.cpp
> > Line 181 (original), 181 (patched)
> > 
> >
> > Could you add something to the commit message explaining why this is 
> > needed?

No longer an issue, since we're using a separate test case now.


- Jan


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


On Jan. 18, 2018, 3:10 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65044/
> ---
> 
> (Updated Jan. 18, 2018, 3:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'GET_OPERATIONS' call lists all operations known to master or agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
>   include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
>   include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
>   include/mesos/v1/master/master.proto 
> 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
>   src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
>   src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
>   src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
>   src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
>   src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
>   src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
>   src/tests/api_tests.cpp bdacc30be4dc8656a41a0c47d0e350d48e59ad94 
> 
> 
> Diff: https://reviews.apache.org/r/65044/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 65045: Tested correct operation handling during master failover.

[Jan Schlicht]

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

(Updated Jan. 18, 2018, 3:11 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Use `GET_OPERATIONS` instead of `GET_AGENTS`.


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


Repository: mesos


Description
---

Tested correct operation handling during master failover.


Diffs (updated)
-

  src/tests/master_tests.cpp d01f3fbdd688ddd31fb0c777f973928f5b5fa5e7 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Jan Schlicht]

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

(Updated Jan. 18, 2018, 3:10 p.m.)


Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jie 
Yu.


Changes
---

Added a `GET_OPERATIONS` call (for master and agent) instead of listing 
operations as part of `GET_AGENTS`.


Summary (updated)
-

Added the v1 API 'GET_OPERATIONS' call for master and agent.


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


Repository: mesos


Description (updated)
---

The 'GET_OPERATIONS' call lists all operations known to master or agent.


Diffs (updated)
-

  include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
  include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
  include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
  include/mesos/v1/master/master.proto 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
  src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
  src/master/master.hpp 05136781b6b1539f37c283e8127e4bafb187a0d1 
  src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
  src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
  src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
  src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
  src/tests/api_tests.cpp bdacc30be4dc8656a41a0c47d0e350d48e59ad94 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Kevin Klues]

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




src/python/lib/mesos/exceptions.py
Lines 27-33 (patched)


I would prefer to do this at each call site instead of wrapping it up and 
hiding it in here. This will keep it consistent with the rest of the code base.

That said, I do like keeping track of the original exception. So something 
like the following would be better in my opinion:

```
def __init__(self, message=None, original_exception=None):
self.original_exception = original_exception
super(MesosException, self).__init__(message)
```

Is the name `original_exception` the best name here? Is there a more 
standard way of wrapping exceptions like this in python and keeping track of 
the original ones?



src/python/lib/mesos/exceptions.py
Line 50 (original), 61 (patched)


Can you add the text `The url '{url}' ...` to the beginning of the string 
so that it composes well with other exceptions. They should all begin with a 
capital letter.



src/python/lib/mesos/http.py
Lines 201-205 (original), 218-221 (patched)


Can we simplify this to:
```
if know_exception:
raise known_exception(response)
else:
raise MesosHTTPException(response)
```



src/python/lib/mesos/http.py
Line 278 (original), 290 (patched)


Error strings should beign with a capital letter.



src/python/lib/mesos/http.py
Line 322 (original), 333 (patched)


Error strings should beign with a capital letter.



src/python/lib/tests/test_http.py
Line 283 (original), 286 (patched)


The tmeout parameter should be moved to the next line


- Kevin Klues


On Jan. 18, 2018, 4:25 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> ---
> 
> (Updated Jan. 18, 2018, 4:25 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/9/
> 
> 
> Testing
> ---
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 65212: Updated `mesos-tidy` docker build setup for benchmarks.

[Benjamin Bannier]

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

(Updated Jan. 18, 2018, 2:24 p.m.)


Review request for mesos and Michael Park.


Summary (updated)
-

Updated `mesos-tidy` docker build setup for benchmarks.


Repository: mesos


Description
---

This patch updates the `mesos-tidy` docker build setup to generate
`benchmarks.pb.h` needed by the libprocess benchmark files added as
part of `eb901173b8aa43b9fd1ab0f727ff5187982c4449`.

We also reorder the targets to be build in the script so that all
proto targets are located close to each other and after manually
triggered 3rdparty dependencies (like e.g., protobuf). This should
reduce redudant build invocations.


Diffs (updated)
-

  support/mesos-tidy/entrypoint.sh dd83d66f48968bce652bc78ccf2d807949f1725b 


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

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


Testing (updated)
---

Tested with a custom docker image and `support/mesos-tidy.sh` pointing to it.


Thanks,

Benjamin Bannier

Re: Review Request 61811: Flattened continuation chains in containerizer for readability.

[Alexander Rukletsov]

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

(Updated Jan. 18, 2018, 1:11 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
f69f65a5033ce0c1fa2fe9591d8c6bbada5e09fb 


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

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


Testing
---

make check on:
`Apple LLVM version 8.0.0 (clang-800.0.42.1)` on Mac OS 10.11.6
`c++ (GCC) 6.2.1 20160916 (Red Hat 6.2.1-2)` on Fedora 24


Thanks,

Alexander Rukletsov

Re: Review Request 61811: Flattened continuation chains in containerizer for readability.

[Alexander Rukletsov]

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

(Updated Jan. 18, 2018, 1:10 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Rebased.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
f69f65a5033ce0c1fa2fe9591d8c6bbada5e09fb 


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

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


Testing
---

make check on:
`Apple LLVM version 8.0.0 (clang-800.0.42.1)` on Mac OS 10.11.6
`c++ (GCC) 6.2.1 20160916 (Red Hat 6.2.1-2)` on Fedora 24


Thanks,

Alexander Rukletsov

Re: Review Request 59746: Stopped accounting aborted container launches as failures.

[Alexander Rukletsov]

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

(Updated Jan. 18, 2018, 1:09 p.m.)


Review request for mesos, Greg Mann, Ian Downes, Jie Yu, Joseph Wu, Jan 
Schlicht, and Vinod Kone.


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


Repository: mesos


Description
---

The container launch future might be failed or discarded (depending
on the containerizer implementation) if the launch has been aborted,
for example, a framework might have stopped while its task are being
started. Such failures should not be accounted as launch errors.


Diffs
-

  src/slave/slave.cpp 50d2a10cd68f6611efd4e691e5325e6e0c06f33a 


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


Testing
---

`make check` on several Linux distros.

Additional manual tests for (1) mesos and (1) docker containerizers. The 
framework is asked to exit right after it submits the task to mesos.

(1) With mesos c-zer
m: `./bin/mesos-master.sh --work_dir=./m`
a: `GLOG_v=1 sudo ./bin/mesos-agent.sh --master=:5050 --work_dir=./a 
--containerizers=mesos --image_providers="DOCKER" 
--isolation=filesystem/linux,docker/runtime`
f: `./src/mesos-execute --master=:5050 --containerizer=mesos 
--docker_image=fedora:25 --name=pull-test --command="sleep 1000"`

(2) With docker c-zer
m: `./bin/mesos-master.sh --work_dir=./m`
a: `GLOG_v=1 sudo ./bin/mesos-agent.sh --master=:5050 --work_dir=./a 
--containerizers=docker`
f: `./src/mesos-execute --master=:5050 --containerizer=docker 
--docker_image=fedora:25 --name=pull-test --command="sleep 1000"`


Thanks,

Alexander Rukletsov

Re: Review Request 65208: Prevented redundant descriptions when applying reviewboard reviews.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65208 was successfully built and tested.

Reviews applied: `['65208']`

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

- Mesos Reviewbot Windows


On Jan. 18, 2018, 2:53 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65208/
> ---
> 
> (Updated Jan. 18, 2018, 2:53 a.m.)
> 
> 
> Review request for mesos, Armand Grillet and Till Toenshoff.
> 
> 
> Bugs: MESOS-7979
> https://issues.apache.org/jira/browse/MESOS-7979
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reviewboard's `rbt` can be configured to guess field when creating
> review requests. With that setting it e.g., uses the first line of a
> commit as description, and the remainder as description. If the commit
> message has no body it will use the summary as description as well,
> leading to redundant information in commit messages which committers
> usually remove again manually before committing.
> 
> This patch prevents reusing a reviewboard description which is
> identical to the summary when synthesising commit message with
> `apply_reviews.py`.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py 0ef28cb02bc65acfeb7ea6808f74e1620a8a85c4 
> 
> 
> Diff: https://reviews.apache.org/r/65208/diff/1/
> 
> 
> Testing
> ---
> 
> Applied a commit with redundant description; the commit message only contains 
> the reviewboard URL as body.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 65208: Prevented redundant descriptions when applying reviewboard reviews.

[Benjamin Bannier]

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

Review request for mesos, Armand Grillet and Till Toenshoff.


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


Repository: mesos


Description
---

Reviewboard's `rbt` can be configured to guess field when creating
review requests. With that setting it e.g., uses the first line of a
commit as description, and the remainder as description. If the commit
message has no body it will use the summary as description as well,
leading to redundant information in commit messages which committers
usually remove again manually before committing.

This patch prevents reusing a reviewboard description which is
identical to the summary when synthesising commit message with
`apply_reviews.py`.


Diffs
-

  support/apply-reviews.py 0ef28cb02bc65acfeb7ea6808f74e1620a8a85c4 


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


Testing
---

Applied a commit with redundant description; the commit message only contains 
the reviewboard URL as body.


Thanks,

Benjamin Bannier

Re: Review Request 65197: Added some missing email addresses to the contributors list.

[Andrei Budnik]

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


Ship it!




Ship It!

- Andrei Budnik


On Jan. 18, 2018, 12:16 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65197/
> ---
> 
> (Updated Jan. 18, 2018, 12:16 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, 
> Benjamin Hindman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added some missing email addresses to the contributors list.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 8bfeb89a9dea3de424b454dc0958515bb855f455 
> 
> 
> Diff: https://reviews.apache.org/r/65197/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65039: Tested reconciliation when operation is dropped en route to agent.

[Benjamin Bannier]

> On Jan. 17, 2018, 9:44 a.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 1797 (patched)
> > 
> >
> > This seems unneeded.
> 
> Greg Mann wrote:
> If the clock isn't resumed at the end of the test, teardown will not 
> complete successfully. The destruction of containers in `Slave::~Slave()` in 
> 'cluster.cpp' requires the clock to be running in order to finish.

That sounds like a bug in `cluster::Slave::~Slave`, and I do not see this 
explicit `resume` pattern elsewhere a lot either.

Adding an explicit `resume` also does not help in cases where an assertion 
before the `resume` leads to the rest of the test being skipped. The only way 
to make sure it is called is to invoke it in some destructor RAII style.

I'd suggest to drop the `resume` here for consistency, and instead fix the 
destructor of `~Slave` to make it self-sufficient (either file a ticket or add 
a cleanup).


- Benjamin


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


On Jan. 18, 2018, 2:57 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65039/
> ---
> 
> (Updated Jan. 18, 2018, 2:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> and Jie Yu.
> 
> 
> Bugs: MESOS-8373
> https://issues.apache.org/jira/browse/MESOS-8373
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds
> StorageLocalResourceProviderTest.ROOT_ReconcileDroppedOperation
> in order to verify that reconciliation is performed correctly
> when an operation is dropped on its way from the master to the
> agent.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65039/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 65197: Added some missing email addresses to the contributors list.

[Armand Grillet]

> On Jan. 18, 2018, 12:23 a.m., Joseph Wu wrote:
> > docs/contributors.yaml
> > Lines 118 (patched)
> > 
> >
> > Only one here too:
> > ```
> > commit 30f7f3e1965885406d6d3e49348bee7e7d0df9d5
> > Author: Armand Grillet 
> > Date:   Mon Feb 6 22:45:43 2017 +0800
> > 
> > Optimized the size of the favicon.
> > 
> > The favicon now only contains two images: 16x16 and 32x32 (for 
> > Retina
> > displays). The size has reduced from 364 KB to 8 KB.
> > 
> > This closes #202
> > ```
> > 
> > And both are Github PRs :|

Please don't list me with that email address, it adds noise in my opinion.


- Armand


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


On Jan. 18, 2018, 12:16 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65197/
> ---
> 
> (Updated Jan. 18, 2018, 12:16 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, 
> Benjamin Hindman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added some missing email addresses to the contributors list.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 8bfeb89a9dea3de424b454dc0958515bb855f455 
> 
> 
> Diff: https://reviews.apache.org/r/65197/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>