Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[haosdent huang]

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


Fix it, then Ship it!




Ship It!


src/tests/containerizer/mesos_containerizer_tests.cpp (line 798)


Should we `AWAIT_FAILED(launch)` return by `.launch` here? Because 
`provisioner->provision` return `Failure`.


- haosdent huang


On March 6, 2016, 9:23 p.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 6, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On March 19, 2016, 3:35 a.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 19, 2016, 3:35 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4638d08328127a8c4ae37555f2be74fe9695da31 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> e849932ab558ccd792080433169ae50dc208a12e 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44435]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 19, 2016, 3:35 a.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 19, 2016, 3:35 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4638d08328127a8c4ae37555f2be74fe9695da31 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> e849932ab558ccd792080433169ae50dc208a12e 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Shuai Lin]

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

(Updated March 19, 2016, 2:29 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Address review comments, and added comments/TODOS.


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


Repository: mesos


Description
---

Fixed a bug that causes the task stuck in staging state.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
4638d08328127a8c4ae37555f2be74fe9695da31 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
e849932ab558ccd792080433169ae50dc208a12e 

Diff: https://reviews.apache.org/r/44435/diff/


Testing
---

- Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
- make check


Thanks,

Shuai Lin

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Shuai Lin]

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

(Updated March 19, 2016, 3:35 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed a bug that causes the task stuck in staging state.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
4638d08328127a8c4ae37555f2be74fe9695da31 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
e849932ab558ccd792080433169ae50dc208a12e 

Diff: https://reviews.apache.org/r/44435/diff/


Testing
---

- Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
- make check


Thanks,

Shuai Lin

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp (line 674)


Thanks for catching the bug. I think this is a pretty bad bug which might 
worth cutting 0.28.1.

Your fix for this issue does reveal another issue in the code: It's 
possible that we will call provisioner->destroy while provisioner->provision is 
in progress. This could cause leaking of the provisioned directory. I'll create 
another ticket for that. Can you add a TODO here?

For this patch, can you add a comment here stating why you need to set it 
to be a ready future here? E.g.:
```
We need to set the 'launchInfos' to be a ready future initially before we 
starting calling isolator->prepare() because otherwise, the destroy will wait 
forever trying to wait for this future to be ready, which it never will.
```



src/tests/containerizer/mesos_containerizer_tests.cpp (line 776)


Can you use a UUID::random() here?



src/tests/containerizer/mesos_containerizer_tests.cpp (line 819)


This is not accurate. We need to improve that. Can you add a TODO here. I 
think we should introduce a new state PROVISIONING.


- Jie Yu


On March 6, 2016, 9:23 p.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 6, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Jie Yu]

> On March 18, 2016, 7 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 674
> > 
> >
> > Thanks for catching the bug. I think this is a pretty bad bug which 
> > might worth cutting 0.28.1.
> > 
> > Your fix for this issue does reveal another issue in the code: It's 
> > possible that we will call provisioner->destroy while 
> > provisioner->provision is in progress. This could cause leaking of the 
> > provisioned directory. I'll create another ticket for that. Can you add a 
> > TODO here?
> > 
> > For this patch, can you add a comment here stating why you need to set 
> > it to be a ready future here? E.g.:
> > ```
> > We need to set the 'launchInfos' to be a ready future initially before 
> > we starting calling isolator->prepare() because otherwise, the destroy will 
> > wait forever trying to wait for this future to be ready, which it never 
> > will.
> > ```

Please include the ticket number in the TODO.


- Jie


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


On March 6, 2016, 9:23 p.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 6, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Jie Yu]

> On March 18, 2016, 7 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 674
> > 
> >
> > Thanks for catching the bug. I think this is a pretty bad bug which 
> > might worth cutting 0.28.1.
> > 
> > Your fix for this issue does reveal another issue in the code: It's 
> > possible that we will call provisioner->destroy while 
> > provisioner->provision is in progress. This could cause leaking of the 
> > provisioned directory. I'll create another ticket for that. Can you add a 
> > TODO here?
> > 
> > For this patch, can you add a comment here stating why you need to set 
> > it to be a ready future here? E.g.:
> > ```
> > We need to set the 'launchInfos' to be a ready future initially before 
> > we starting calling isolator->prepare() because otherwise, the destroy will 
> > wait forever trying to wait for this future to be ready, which it never 
> > will.
> > ```
> 
> Jie Yu wrote:
> Please include the ticket number in the TODO.

BTW, I create a ticket for the other bug:
https://issues.apache.org/jira/browse/MESOS-4985


- Jie


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


On March 6, 2016, 9:23 p.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 6, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Guangya Liu]

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




src/slave/containerizer/mesos/containerizer.cpp (line 679)


s/'launchInfos'/`launchInfos`



src/slave/containerizer/mesos/containerizer.cpp (line 680)


s/because otherwise,/because otherwise, if the `prepare` in isolator did 
not return `launchInfos` in some failure cases (MESOS-4878), ?



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 713 - 719)


The jagged format is not good and a patch 
https://reviews.apache.org/r/44653/diff/5#index_header is already trying to fix 
such issues.

It is better use following format:
MOCK_METHOD2(
recover,
Future(
const list&,
const hashset&));



src/tests/containerizer/mesos_containerizer_tests.cpp (line 740)


a period


- Guangya Liu


On 三月 19, 2016, 2:29 a.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated 三月 19, 2016, 2:29 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4638d08328127a8c4ae37555f2be74fe9695da31 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> e849932ab558ccd792080433169ae50dc208a12e 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44435]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 6, 2016, 9:23 p.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44435/
> ---
> 
> (Updated March 6, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4878
> https://issues.apache.org/jira/browse/MESOS-4878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that causes the task stuck in staging state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
> 
> Diff: https://reviews.apache.org/r/44435/diff/
> 
> 
> Testing
> ---
> 
> - Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
> - make check
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Review Request 44435: Fixed a bug that causes the task stuck in staging state.

[Shuai Lin]

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

Review request for mesos.


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


Repository: mesos


Description
---

Fixed a bug that causes the task stuck in staging state.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
af3ff5750649497d8852b4761c78d4cae5455a02 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 

Diff: https://reviews.apache.org/r/44435/diff/


Testing
---

- Added a new test `"MesosContainerizerProvisionerTest.ProvisionFailed"`
- make check


Thanks,

Shuai Lin