Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

> On April 1, 2016, 6:22 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 781
> > 
> >
> > Can you add some LOG(INFO) here. Maybe in a subsequent patch.

Sure, fixed it in a new patch: https://reviews.apache.org/r/45580/


> On April 1, 2016, 6:22 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 788
> > 
> >
> > Ditto on adding some LOG(INFO) here.

Ditto.


- Qian


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


On March 31, 2016, 7:28 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 31, 2016, 7:28 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 873e0c52475f4868e611bd24a6782ad5eb261a99 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1c8e231813c0579b79681c5d18b1f799a727ead7 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Jie Yu]

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


Ship it!





src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 776)


Can you add some LOG(INFO) here. Maybe in a subsequent patch.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 783)


Ditto on adding some LOG(INFO) here.


- Jie Yu


On March 31, 2016, 11:28 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 31, 2016, 11:28 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 873e0c52475f4868e611bd24a6782ad5eb261a99 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1c8e231813c0579b79681c5d18b1f799a727ead7 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 31, 2016, 7:28 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented cleanup() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
873e0c52475f4868e611bd24a6782ad5eb261a99 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
1c8e231813c0579b79681c5d18b1f799a727ead7 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 625)


space after `foreachkey`. Please fix all occurances.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 629)


Should we use 'await' here to make sure all plugin DEL finishes/failed 
before returnning result? You can do:
```
return await(futures)
  .then(_cleanup);
```

No need for onAny here since await is guaranteed to have a suceeded future.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 631)


Please assign the parameters here. You're off by 1 space.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 685)


You can use `s->status()` and `s->out().get()` here.

ALso, no need for process:: namespace prefix.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 739)


I think in `_cleanup`, I think if any of the plugin DEL succeeds, we should 
remove it's corresponding directory so that we don't retry DEL again. 
Otherwise, we should not so that during recover, we can retry it's deletion.

I think we should not umount unless all plugin DEL succeeds.


- Jie Yu


On March 30, 2016, 8:36 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 30, 2016, 8:36 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 873e0c52475f4868e611bd24a6782ad5eb261a99 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1c8e231813c0579b79681c5d18b1f799a727ead7 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 30, 2016, 4:36 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented cleanup() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
873e0c52475f4868e611bd24a6782ad5eb261a99 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
1c8e231813c0579b79681c5d18b1f799a727ead7 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 30, 2016, 3:45 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented cleanup() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
873e0c52475f4868e611bd24a6782ad5eb261a99 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
1c8e231813c0579b79681c5d18b1f799a727ead7 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 30, 2016, 2:53 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented cleanup() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
873e0c52475f4868e611bd24a6782ad5eb261a99 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
1c8e231813c0579b79681c5d18b1f799a727ead7 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 644-645
> > 
> >
> > These two CHECKS don't make sense. What if the plugin got deleted or 
> > there was a bug in the plugin because of which it wasn't able to delete the 
> > interfaces or release the IP addresses. The Agent should not die because of 
> > an error in a 3rd party plugin.
> 
> Qian Zhang wrote:
> If the plugin got deleted, then `Subprocess.isError()` will be true and 
> we will return a `Failure`, if there was a bug in the plugin, then 
> `Subprocess.status()` will be non-zero and we will read its output for the 
> detailed error message. For either of these cases, agent will not die.
> 
> Avinash sridharan wrote:
> The fact that we are failing on the plugin returning an error seems very 
> dangerous. A malicious plugin can start crashing the Agent which is just bad 
> behavior. We should avoid this at all costs. CHECKS are more defensive to 
> test an internal logic, they should not be used for any external inputs. I 
> would rather throw an error and get out.

We do not have these two CHECKs in the latest patch anymore :-)


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 647-649
> > 
> >
> > This is a bit odd, __disconnect always returns an error ? The plugin 
> > can return error codes and error logs which we should be propagating 
> > upstream through failure semantics.
> 
> Qian Zhang wrote:
> The reason that `__disconnect()` always returns a `Failure` is, it will 
> be only called when the exit code of plugin is not 0 (i.e., plugin fails for 
> a reason), in case of plugin success, we will return `Nothing()` in 
> `_disconnect()`. So we only need `__disconnect()` in case of plugin failure 
> to get the output (i.e., detailed failure reason) of plugin and propagate it 
> upstream.
> 
> Avinash sridharan wrote:
> I would suggest s/Failed to execute the CNI plugin/CNI plugin failed to 
> detach network/ . "Failed to execute ..." implies that we not able to launch 
> the plugin, which might not be the case here.

Agree!


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 653
> > 
> >
> > Why are we returning a Future over here? We should be 
> > returning a Future there is no list since you are deletnig the 
> > entire container directory in one shot. This seems a bit odd.
> 
> Qian Zhang wrote:
> I tend to agree with you, however, I see CPU share isolator does the 
> similar thing: 
> https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp#L536:L547,
>  maybe we should change it as well?

I have made `_cleanup()` as a void method so we do not need to return 
`Future` anymore.


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 686
> > 
> >
> > This is dangerous we should be returning a const reference passed down 
> > in the call as a return argument. Just do a return list() over 
> > here. 
> > 
> > PS: Please see my comment on returning Future above.

Ditto.


- Qian


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


On March 29, 2016, 6:59 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 29, 2016, 6:59 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Avinash sridharan]

> On March 22, 2016, 3:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 644-645
> > 
> >
> > These two CHECKS don't make sense. What if the plugin got deleted or 
> > there was a bug in the plugin because of which it wasn't able to delete the 
> > interfaces or release the IP addresses. The Agent should not die because of 
> > an error in a 3rd party plugin.
> 
> Qian Zhang wrote:
> If the plugin got deleted, then `Subprocess.isError()` will be true and 
> we will return a `Failure`, if there was a bug in the plugin, then 
> `Subprocess.status()` will be non-zero and we will read its output for the 
> detailed error message. For either of these cases, agent will not die.

The fact that we are failing on the plugin returning an error seems very 
dangerous. A malicious plugin can start crashing the Agent which is just bad 
behavior. We should avoid this at all costs. CHECKS are more defensive to test 
an internal logic, they should not be used for any external inputs. I would 
rather throw an error and get out.


> On March 22, 2016, 3:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 647-649
> > 
> >
> > This is a bit odd, __disconnect always returns an error ? The plugin 
> > can return error codes and error logs which we should be propagating 
> > upstream through failure semantics.
> 
> Qian Zhang wrote:
> The reason that `__disconnect()` always returns a `Failure` is, it will 
> be only called when the exit code of plugin is not 0 (i.e., plugin fails for 
> a reason), in case of plugin success, we will return `Nothing()` in 
> `_disconnect()`. So we only need `__disconnect()` in case of plugin failure 
> to get the output (i.e., detailed failure reason) of plugin and propagate it 
> upstream.

I would suggest s/Failed to execute the CNI plugin/CNI plugin failed to detach 
network/ . "Failed to execute ..." implies that we not able to launch the 
plugin, which might not be the case here.


- Avinash


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


On March 29, 2016, 10:59 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 29, 2016, 10:59 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 29, 2016, 6:59 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented cleanup() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
7cda5715814a0cfc4b394eb04437831e6dc44e3f 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 29, 2016, 3:11 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented cleanup() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
7cda5715814a0cfc4b394eb04437831e6dc44e3f 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44004, 44200, 44549, 44555, 44622, 44514, 44706, 45082]

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 26, 2016, 2:59 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 26, 2016, 2:59 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 26, 2016, 10:59 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented cleanup() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
7cda5715814a0cfc4b394eb04437831e6dc44e3f 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 151
> > 
> >
> > I guess if we are going to use `attach` for connecting to networks, 
> > lets use `detach` over here.

Agree.


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 647-649
> > 
> >
> > This is a bit odd, __disconnect always returns an error ? The plugin 
> > can return error codes and error logs which we should be propagating 
> > upstream through failure semantics.

The reason that `__disconnect()` always returns a `Failure` is, it will be only 
called when the exit code of plugin is not 0 (i.e., plugin fails for a reason), 
in case of plugin success, we will return `Nothing()` in `_disconnect()`. So we 
only need `__disconnect()` in case of plugin failure to get the output (i.e., 
detailed failure reason) of plugin and propagate it upstream.


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 653
> > 
> >
> > Why are we returning a Future over here? We should be 
> > returning a Future there is no list since you are deletnig the 
> > entire container directory in one shot. This seems a bit odd.

I tend to agree with you, however, I see CPU share isolator does the similar 
thing: 
https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp#L536:L547,
 maybe we should change it as well?


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 644-645
> > 
> >
> > These two CHECKS don't make sense. What if the plugin got deleted or 
> > there was a bug in the plugin because of which it wasn't able to delete the 
> > interfaces or release the IP addresses. The Agent should not die because of 
> > an error in a 3rd party plugin.

If the plugin got deleted, then `Subprocess.isError()` will be true and we will 
return a `Failure`, if there was a bug in the plugin, then 
`Subprocess.status()` will be non-zero and we will read its output for the 
detailed error message. For either of these cases, agent will not die.


- Qian


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


On March 21, 2016, 12:30 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 21, 2016, 12:30 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Avinash sridharan]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 151)


I guess if we are going to use `attach` for connecting to networks, lets 
use `detach` over here.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 554)


Calling dispatch on itself I think can be avoided.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 644 - 645)


These two CHECKS don't make sense. What if the plugin got deleted or there 
was a bug in the plugin because of which it wasn't able to delete the 
interfaces or release the IP addresses. The Agent should not die because of an 
error in a 3rd party plugin.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 647 - 649)


This is a bit odd, __disconnect always returns an error ? The plugin can 
return error codes and error logs which we should be propagating upstream 
through failure semantics.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 653)


Why are we returning a Future over here? We should be 
returning a Future there is no list since you are deletnig the entire 
container directory in one shot. This seems a bit odd.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 686)


This is dangerous we should be returning a const reference passed down in 
the call as a return argument. Just do a return list() over here. 

PS: Please see my comment on returning Future above.


- Avinash sridharan


On March 20, 2016, 4:30 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 20, 2016, 4:30 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44004, 44200, 44549, 44555, 44622, 44514, 44706, 45082]

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 20, 2016, 4:30 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 20, 2016, 4:30 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>