Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45571]

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 April 1, 2016, 5:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 5:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On April 1, 2016, 5:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 5:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 429
> > 
> >
> > You had mentioned that if we return an error during `recover` the agent 
> > will restart. This state from checkpointed information that is not 
> > changing. After restart would the Agent end up hitting this condition again 
> > ?
> 
> Jie Yu wrote:
> Yes, but this is not possible, right? If that happens, human needs to be 
> involved.
> 
> Avinash sridharan wrote:
> Yeah, we can't handle this condition. Should we add a `LOG(ERROR)` here 
> or would this be highlighted upstream? Wanted to see if we can explicitly 
> indicate this condition to the operator.

I think the Error message will be visible to the operator? Why you need a 
LOG(ERROR) here. The agent (or containerizer) will do the LOG(ERROR).


- Jie


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


On April 1, 2016, 5:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 5:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Avinash sridharan]

> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 429
> > 
> >
> > You had mentioned that if we return an error during `recover` the agent 
> > will restart. This state from checkpointed information that is not 
> > changing. After restart would the Agent end up hitting this condition again 
> > ?
> 
> Jie Yu wrote:
> Yes, but this is not possible, right? If that happens, human needs to be 
> involved.

Yeah, we can't handle this condition. Should we add a `LOG(ERROR)` here or 
would this be highlighted upstream? Wanted to see if we can explicitly indicate 
this condition to the operator.


- Avinash


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


On April 1, 2016, 5:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 5:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

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

(Updated April 1, 2016, 5:36 p.m.)


Review request for mesos, Avinash sridharan and Qian Zhang.


Changes
---

Review comments.


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


Repository: mesos


Description
---

A few cleanups and simplifications in CNI isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
3a07540909ed771d1bd3b22888e04d5fb451710d 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
486c382365d5293cd9d53b8b239f70a543c46792 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 374
> > 
> >
> > s/on/only
> > instead of `non-host network(s)` maybe `joined a CNI network(s) ?
> > 
> > Didn't follow the last part of the sentence "and cleanup _might_ be 
> > required for that container." What does this mean ?

Add a few more to explain that does that mean.


> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 429
> > 
> >
> > You had mentioned that if we return an error during `recover` the agent 
> > will restart. This state from checkpointed information that is not 
> > changing. After restart would the Agent end up hitting this condition again 
> > ?

Yes, but this is not possible, right? If that happens, human needs to be 
involved.


> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 475
> > 
> >
> > s/crashes/crashed/
> > Why would cleanup be called on this container? Cause they are `orphans`?

No, the agent hasn't realized that the container cleanup has done. So after 
restart, it'll call cleanup again.


> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1016
> > 
> >
> > Maybe "failed to detech container " + containerId + " from network '" 
> > .

'containerId' is not needed here because the caller will print it.


- Jie


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


On April 1, 2016, 1:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 1:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

> On April 1, 2016, 2:55 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 418-424
> > 
> >
> > I do not think we need this. If agent crashes after removing the 
> > interface directory in `_detach`, then we should not get into this 
> > `foreach` loop because `networkNames` returned by `paths::getNetworkNames` 
> > should be empty.
> 
> Jie Yu wrote:
> Hum, I don't think we delete network dir, do we? We delete ifdir first, 
> if all are successful, we delete containerDir. But it's possible that we 
> delete the ifdir, but agent crashes before the containerDir is deleted, right?
> 
> Qian Zhang wrote:
> Yes, you are right. However, since there is only one ifdir in a network 
> dir, I am thinking we may change `_detach` to remove the network dir rather 
> than remove the ifdir and leave an empty network dir there, and then here we 
> will not need this code:
>   if (interfaces->empty()) {
> continue;
>   }

We will support adding more than one interface to a network dir, so I think the 
approach in my patch is better.


- Jie


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


On April 1, 2016, 1:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 1:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Qian Zhang]

> On April 1, 2016, 10:55 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 418-424
> > 
> >
> > I do not think we need this. If agent crashes after removing the 
> > interface directory in `_detach`, then we should not get into this 
> > `foreach` loop because `networkNames` returned by `paths::getNetworkNames` 
> > should be empty.
> 
> Jie Yu wrote:
> Hum, I don't think we delete network dir, do we? We delete ifdir first, 
> if all are successful, we delete containerDir. But it's possible that we 
> delete the ifdir, but agent crashes before the containerDir is deleted, right?

Yes, you are right. However, since there is only one ifdir in a network dir, I 
am thinking we may change `_detach` to remove the network dir rather than 
remove the ifdir and leave an empty network dir there, and then here we will 
not need this code:
  if (interfaces->empty()) {
continue;
  }


- Qian


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


On April 1, 2016, 9:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 9:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45571]

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 April 1, 2016, 1:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 1:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

> On April 1, 2016, 2:55 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 462-467
> > 
> >
> > Is it possible that agent crashes when checkpointing the output of CNI 
> > plugin in `_attach`? Will it cause this file corrupt or only contain 
> > partial output of plugin? If so, then we may log a warning message and just 
> > continue here.

Yeah, I'll add a TODO.


- Jie


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


On April 1, 2016, 1:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 1:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

> On April 1, 2016, 2:55 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 418-424
> > 
> >
> > I do not think we need this. If agent crashes after removing the 
> > interface directory in `_detach`, then we should not get into this 
> > `foreach` loop because `networkNames` returned by `paths::getNetworkNames` 
> > should be empty.

Hum, I don't think we delete network dir, do we? We delete ifdir first, if all 
are successful, we delete containerDir. But it's possible that we delete the 
ifdir, but agent crashes before the containerDir is deleted, right?


- Jie


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


On April 1, 2016, 1:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 1:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>