Re: Review Request 51402: Added nested container check in provisioner destroy.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 425)


s/nested/child/



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 429)


s/nested/child/


- Jie Yu


On Sept. 9, 2016, 8:07 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 9, 2016, 8:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Please see the comments in the patch for detail.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

> On Sept. 9, 2016, 3:13 p.m., Joseph Wu wrote:
> > Suggestion for description:
> > ```
> > This forces the agent to fail fast if we attempt to destroy an outer
> > container without first destroying all nested containers.  It is
> > appropriate to CHECK as we control all the call sites.
> > ```

Thanks Joseph. I will update all of them.


- Gilbert


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


On Sept. 9, 2016, 1:07 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 9, 2016, 1:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Please see the comments in the patch for detail.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Joseph Wu]

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


Ship it!




Suggestion for description:
```
This forces the agent to fail fast if we attempt to destroy an outer
container without first destroying all nested containers.  It is
appropriate to CHECK as we control all the call sites.
```


src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 424 - 426)


Rewording a bit:
```
// In the above cases, we assume that the container being destroyed
// has no corresponding nested containers. We fail fast if this
// condition is not satisfied.
```



src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 428 - 431)


Rewording a bit:
```
// NOTE: This check is expensive since it traverses the entire
// `infos` hashmap. This is acceptable because we generally expect
// the number of containers on a single agent to be on the order of
// tens or hundreds of containers.
```


- Joseph Wu


On Sept. 9, 2016, 1:07 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 9, 2016, 1:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Please see the comments in the patch for detail.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Avinash sridharan]

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


Fix it, then Ship it!




Ship It!


src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 429)


s/infos/`infos`



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 437)


s/is/ has not been


- Avinash sridharan


On Sept. 9, 2016, 8:07 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 9, 2016, 8:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Please see the comments in the patch for detail.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

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

(Updated Sept. 9, 2016, 1:07 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description (updated)
---

Please see the comments in the patch for detail.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
8e35ff49ec99a242e764095dcfbb541c5e41ec71 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Jie Yu]

> On Sept. 9, 2016, 4:21 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp, lines 194-197
> > 
> >
> > I realized that it's not sufficient to just pass in top level orphan 
> > containers to provisioners/isolators. We also want to know about known 
> > child containers for both checkpointed containers and orphan containers so 
> > that provisioners/isolators can cleanup unknown child containers.
> > 
> > Consider the following case:
> > 1) containerizer launched a child container A/B under top level 
> > container A
> > 2) isolator prepare finishes for container A/B
> > 3) agent crashes before launcher fork is called
> > 4) agent recovers
> > 5) container A is checkpointed, thus considered alive
> > 6) however, provisioners/isolators need to cleanup for container A/B as 
> > it's unknown to the launcher
> > 
> > Therefore, I suggest we introduce a protobuf 'ContainerRecoverInfo' in 
> > `include/mesos/slave/containerizer.proto`:
> > 
> > ```
> > message ContainerRecoverInfo {
> >   repeated ContainerState checkpointed_containers;
> >   repeated ContainerID orphan_container_ids; // Deprecated. Top level 
> > orphans.
> >   repeated COntainerID known_container_ids; // All known containers, 
> > including child containers.
> > }
> > ```
> > 
> > And both Provisioner and Isolator recover interface will take this 
> > protobuf.
> 
> Gilbert Song wrote:
> Thanks for being in details. Sorry did not see this comment yesterday. In 
> my local implementation, `ContainerRecoverInfo` is only for 
> isolator::recover(), since we can just change the provisioner::recover 
> interface with only `knownContainers` set without breaking other parts. This 
> reduce the complication in provisioner::recover.

Sounds fine to me for now. Eventually, you'll use this protobuf i think. It is 
just more easy to construct one protobuf and send it to both provisioner and 
isolators.


- Jie


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


On Sept. 7, 2016, 6:49 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 7, 2016, 6:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

> On Sept. 8, 2016, 9:21 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp, lines 194-197
> > 
> >
> > I realized that it's not sufficient to just pass in top level orphan 
> > containers to provisioners/isolators. We also want to know about known 
> > child containers for both checkpointed containers and orphan containers so 
> > that provisioners/isolators can cleanup unknown child containers.
> > 
> > Consider the following case:
> > 1) containerizer launched a child container A/B under top level 
> > container A
> > 2) isolator prepare finishes for container A/B
> > 3) agent crashes before launcher fork is called
> > 4) agent recovers
> > 5) container A is checkpointed, thus considered alive
> > 6) however, provisioners/isolators need to cleanup for container A/B as 
> > it's unknown to the launcher
> > 
> > Therefore, I suggest we introduce a protobuf 'ContainerRecoverInfo' in 
> > `include/mesos/slave/containerizer.proto`:
> > 
> > ```
> > message ContainerRecoverInfo {
> >   repeated ContainerState checkpointed_containers;
> >   repeated ContainerID orphan_container_ids; // Deprecated. Top level 
> > orphans.
> >   repeated COntainerID known_container_ids; // All known containers, 
> > including child containers.
> > }
> > ```
> > 
> > And both Provisioner and Isolator recover interface will take this 
> > protobuf.

Thanks for being in details. Sorry did not see this comment yesterday. In my 
local implementation, `ContainerRecoverInfo` is only for isolator::recover(), 
since we can just change the provisioner::recover interface with only 
`knownContainers` set without breaking other parts. This reduce the 
complication in provisioner::recover.


- Gilbert


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


On Sept. 7, 2016, 11:49 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 7, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Jie Yu]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 194 - 197)


I realized that it's not sufficient to just pass in top level orphan 
containers to provisioners/isolators. We also want to know about known child 
containers for both checkpointed containers and orphan containers so that 
provisioners/isolators can cleanup unknown child containers.

Consider the following case:
1) containerizer launched a child container A/B under top level container A
2) isolator prepare finishes for container A/B
3) agent crashes before launcher fork is called
4) agent recovers
5) container A is checkpointed, thus considered alive
6) however, provisioners/isolators need to cleanup for container A/B as 
it's unknown to the launcher

Therefore, I suggest we introduce a protobuf 'ContainerRecoverInfo' in 
`include/mesos/slave/containerizer.proto`:

```
message ContainerRecoverInfo {
  repeated ContainerState checkpointed_containers;
  repeated ContainerID orphan_container_ids; // Deprecated. Top level 
orphans.
  repeated COntainerID known_container_ids; // All known containers, 
including child containers.
}
```

And both Provisioner and Isolator recover interface will take this protobuf.


- Jie Yu


On Sept. 7, 2016, 6:49 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 7, 2016, 6:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

> On Sept. 5, 2016, 2:51 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp, line 434
> > 
> >
> > s/sub-container/sub-containers/

Lets us nested containers consistantly.


- Gilbert


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


On Sept. 7, 2016, 11:49 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 7, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

> On Sept. 5, 2016, 7:38 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp, lines 432-434
> > 
> >
> > Just a question, how do we ensure when a container is being destroyed, 
> > all its sub-containers are already destroyed? Will we need to add some 
> > logic in the caller of provisioner destroy to ensure this?

This is ensured in containerizer::destroy().


- Gilbert


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


On Sept. 7, 2016, 11:49 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 7, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

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

(Updated Sept. 7, 2016, 11:49 a.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description
---

Added nested container check in provisioner destroy.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
8e35ff49ec99a242e764095dcfbb541c5e41ec71 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Avinash sridharan]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 181)


s/hierarchies/hierarchy



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 232)


s/dirrectly/directly
s/not necessarily  recursive/without needing to make a recursive call 
to destroy



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 428)


s/recover/`recover`
s/destroy/`destroy`



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 432)


both cases ? Aren't there three cases listed above?

Also remove this line (should ... sub-container). Its redundant.

s/The following check ../ The following check ensures this condition is 
satisfied.



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 438)


Well we are probably going to run O(100) containers on an agent. So don't 
think this check would ever be too expensive. 

I would say something like:
// NOTE: We generally expect the number of containers on an agent to be 
O(100), hence this check should not be expensive. However, in the event that 
the number of containers are much larger  than O(100) this check might become 
expensive.


- Avinash sridharan


On Aug. 29, 2016, 9:20 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Aug. 29, 2016, 9:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Qian Zhang]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 432 - 434)


Just a question, how do we ensure when a container is being destroyed, all 
its sub-containers are already destroyed? Will we need to add some logic in the 
caller of provisioner destroy to ensure this?


- Qian Zhang


On Aug. 30, 2016, 5:20 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Aug. 30, 2016, 5:20 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Qian Zhang]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 434)


s/sub-container/sub-containers/


- Qian Zhang


On Aug. 30, 2016, 5:20 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Aug. 30, 2016, 5:20 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Guangya Liu]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 432)


s/both/all



src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 433 - 434)


+1 to Joseph's comments, since here you already mentioned that all of the 
sub-containers were already destroyed, so in the followig check, it shoud use 
check-fail instead?


- Guangya Liu


On 八月 29, 2016, 9:20 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated 八月 29, 2016, 9:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Joseph Wu]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 443 - 446)


Is this case recoverable?  

If we try to destroy a parent without destroying a child first, that means 
our code is broken.  If that is the case, we should CHECK-fail here.


- Joseph Wu


On Aug. 29, 2016, 2:20 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Aug. 29, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

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

(Updated Aug. 29, 2016, 2:20 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description
---

Added nested container check in provisioner destroy.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
8e35ff49ec99a242e764095dcfbb541c5e41ec71 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Jie Yu]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 429)


i think there is a third case in the normal destroy path, right?



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 441)


no need for the extra `()`



src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 442 - 445)


I would return a Failure here instead.


- Jie Yu


On Aug. 25, 2016, 12:52 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Aug. 25, 2016, 12:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51323, 51343, 51358, 51359, 51392, 51393, 51402]

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

- Mesos ReviewBot


On Aug. 25, 2016, 12:52 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Aug. 25, 2016, 12:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
Kevin Klues.


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


Repository: mesos


Description
---

Added nested container check in provisioner destroy.


Diffs
-

  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
8e35ff49ec99a242e764095dcfbb541c5e41ec71 

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


Testing
---

make check


Thanks,

Gilbert Song