Re: Review Request 64998: Added a SLRP test for CSI plugin restart.

2018-01-19 Thread Greg Mann 

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


Fix it, then Ship it!





src/slave/container_daemon_process.hpp
Lines 76-79 (patched)


Nit: one fewer newline here. I can fix this while committing.


- Greg Mann


On Jan. 18, 2018, 2:53 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64998/
> ---
> 
> (Updated Jan. 18, 2018, 2:53 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8408
> https://issues.apache.org/jira/browse/MESOS-8408
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test does the same as the `PublishResources` test, but it kills the
> CSI plugin contaier between each operation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 191594b9d67f1aa1342efbaad5e7501004332d74 
>   src/slave/container_daemon.cpp d74fa5105c61a6cadfb2610790e559d387ca13a0 
>   src/slave/container_daemon_process.hpp PRE-CREATION 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/64998/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64998: Added a SLRP test for CSI plugin restart.

2018-01-17 Thread Chun-Hung Hsiao 

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

(Updated Jan. 18, 2018, 2:53 a.m.)


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


Changes
---

Addressed Greg's comments and made some changes for consistency.


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


Repository: mesos


Description
---

The test does the same as the `PublishResources` test, but it kills the
CSI plugin contaier between each operation.


Diffs (updated)
-

  src/Makefile.am 191594b9d67f1aa1342efbaad5e7501004332d74 
  src/slave/container_daemon.cpp d74fa5105c61a6cadfb2610790e559d387ca13a0 
  src/slave/container_daemon_process.hpp PRE-CREATION 
  src/tests/storage_local_resource_provider_tests.cpp 
bbfe95e9818f25fdd5405db3ad2fe355e023f743 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 64998: Added a SLRP test for CSI plugin restart.

2018-01-17 Thread Chun-Hung Hsiao 


> On Jan. 17, 2018, 2:34 a.m., Greg Mann wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 1552-1559 (patched)
> > 
> >
> > Could this be racy? Since we're not awaiting on the UpdateSlaveMessage 
> > containing the RP resources, is it possible that the first offer the 
> > scheduler receives will not contain any raw disk?

Offers that does not match the `OffersHaveAnyResource(...)` criteria will be 
declined immediately by Line 1538. But I should use another set of filters 
which a long `refuse_seconds` for declining such offers.


- Chun-Hung


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


On Jan. 10, 2018, 2:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64998/
> ---
> 
> (Updated Jan. 10, 2018, 2:33 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8408
> https://issues.apache.org/jira/browse/MESOS-8408
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test does the same as the `PublishResources` test, but it kills the
> CSI plugin contaier between each operation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bfe9eb1609c71c980c11e7cb0149caf5bdd7ab9b 
>   src/slave/container_daemon.cpp d74fa5105c61a6cadfb2610790e559d387ca13a0 
>   src/slave/container_daemon_process.hpp PRE-CREATION 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/64998/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64998: Added a SLRP test for CSI plugin restart.

2018-01-16 Thread Greg Mann 

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



Nice test!!


src/slave/container_daemon_process.hpp
Lines 26-27 (patched)


Let's include  as well, to bring in the declaration of 
`process::http::URL`, which you use below.



src/tests/storage_local_resource_provider_tests.cpp
Lines 1552-1559 (patched)


Could this be racy? Since we're not awaiting on the UpdateSlaveMessage 
containing the RP resources, is it possible that the first offer the scheduler 
receives will not contain any raw disk?



src/tests/storage_local_resource_provider_tests.cpp
Lines 1562-1565 (patched)


Do you really need the lambda here? What about:

```
Future pluginContainers = containerizer->containers();

AWAIT_READY(pluginContainers);
ASSERT_EQ(1u, pluginContainers->size());

const ContainerID containerId = *(pluginContainers.begin());
```


- Greg Mann


On Jan. 10, 2018, 2:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64998/
> ---
> 
> (Updated Jan. 10, 2018, 2:33 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8408
> https://issues.apache.org/jira/browse/MESOS-8408
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test does the same as the `PublishResources` test, but it kills the
> CSI plugin contaier between each operation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bfe9eb1609c71c980c11e7cb0149caf5bdd7ab9b 
>   src/slave/container_daemon.cpp d74fa5105c61a6cadfb2610790e559d387ca13a0 
>   src/slave/container_daemon_process.hpp PRE-CREATION 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/64998/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64998: Added a SLRP test for CSI plugin restart.

2018-01-09 Thread Chun-Hung Hsiao 

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

(Updated Jan. 10, 2018, 2:33 a.m.)


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


Changes
---

Speeded up the test.


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


Repository: mesos


Description
---

The test does the same as the `PublishResources` test, but it kills the
CSI plugin contaier between each operation.


Diffs (updated)
-

  src/Makefile.am bfe9eb1609c71c980c11e7cb0149caf5bdd7ab9b 
  src/slave/container_daemon.cpp d74fa5105c61a6cadfb2610790e559d387ca13a0 
  src/slave/container_daemon_process.hpp PRE-CREATION 
  src/tests/storage_local_resource_provider_tests.cpp 
bbfe95e9818f25fdd5405db3ad2fe355e023f743 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 64998: Added a SLRP test for CSI plugin restart.

2018-01-09 Thread Chun-Hung Hsiao 

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

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


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


Changes
---

Fixed a typo.


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


Repository: mesos


Description
---

The test does the same as the `PublishResources` test, but it kills the
CSI plugin contaier between each operation.


Diffs (updated)
-

  src/Makefile.am bfe9eb1609c71c980c11e7cb0149caf5bdd7ab9b 
  src/slave/container_daemon.cpp d74fa5105c61a6cadfb2610790e559d387ca13a0 
  src/slave/container_daemon_process.hpp PRE-CREATION 
  src/tests/storage_local_resource_provider_tests.cpp 
bbfe95e9818f25fdd5405db3ad2fe355e023f743 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Review Request 64998: Added a SLRP test for CSI plugin restart.

2018-01-05 Thread Chun-Hung Hsiao 

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

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


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


Repository: mesos


Description
---

The test does the same as the `PublishResources` test, but it kills the
CSI plugin contaier between each operation.


Diffs
-

  src/Makefile.am 30cd4d426e797e4c8ee556d1bc3de99830a5fe41 
  src/slave/container_daemon.cpp d74fa5105c61a6cadfb2610790e559d387ca13a0 
  src/tests/storage_local_resource_provider_tests.cpp 
e9ea021867bf2175d5ee53a89a0619563ab8f329 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao