You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2016/04/02 20:44:11 UTC
Review Request 45620: Adjusted the status semantics in CNI isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45620/
-----------------------------------------------------------
Review request for mesos, Avinash sridharan and Qian Zhang.
Bugs: MESOS-4764
https://issues.apache.org/jira/browse/MESOS-4764
Repository: mesos
Description
-------
This patch also does a few minor cleanups.
Diffs
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b1173d5adee6b4eb45dbed5e693f40ffb5275210
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp af5f49007e0afe57006c9db940611e56eb75a621
Diff: https://reviews.apache.org/r/45620/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 45620: Adjusted the status semantics in CNI
isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45620/
-----------------------------------------------------------
(Updated April 5, 2016, 4:45 p.m.)
Review request for mesos, Avinash sridharan and Qian Zhang.
Changes
-------
Addressed review comments.
Bugs: MESOS-4764
https://issues.apache.org/jira/browse/MESOS-4764
Repository: mesos
Description
-------
This patch also does a few minor cleanups.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b1173d5adee6b4eb45dbed5e693f40ffb5275210
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp af5f49007e0afe57006c9db940611e56eb75a621
Diff: https://reviews.apache.org/r/45620/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 45620: Adjusted the status semantics in CNI
isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On April 3, 2016, 3:30 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 95
> > <https://reviews.apache.org/r/45620/diff/1/?file=1323272#file1323272line95>
> >
> > Can you please clarify why making `networkInfo` as an `Option`? Is there any chances that `status()` is called before `ContainerNetwork.networkInfo` is set in `prepare()` and `_recover()`?
I was mainly thinking about the orphan container case. We still create 'Info' struct for orphan containers. IN that case, the 'networkInfo' in the 'Info' struct does not make sense. Instead of leaving a 'default' networkInfo, I prefer to be more explicit using Option.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45620/#review126713
-----------------------------------------------------------
On April 2, 2016, 6:44 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45620/
> -----------------------------------------------------------
>
> (Updated April 2, 2016, 6:44 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Qian Zhang.
>
>
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch also does a few minor cleanups.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b1173d5adee6b4eb45dbed5e693f40ffb5275210
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp af5f49007e0afe57006c9db940611e56eb75a621
>
> Diff: https://reviews.apache.org/r/45620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 45620: Adjusted the status semantics in CNI
isolator.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45620/#review126713
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 95)
<https://reviews.apache.org/r/45620/#comment189747>
Can you please clarify why making `networkInfo` as an `Option`? Is there any chances that `status()` is called before `ContainerNetwork.networkInfo` is set in `prepare()` and `_recover()`?
- Qian Zhang
On April 3, 2016, 2:44 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45620/
> -----------------------------------------------------------
>
> (Updated April 3, 2016, 2:44 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Qian Zhang.
>
>
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch also does a few minor cleanups.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b1173d5adee6b4eb45dbed5e693f40ffb5275210
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp af5f49007e0afe57006c9db940611e56eb75a621
>
> Diff: https://reviews.apache.org/r/45620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 45620: Adjusted the status semantics in CNI
isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On April 2, 2016, 7:25 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 817
> > <https://reviews.apache.org/r/45620/diff/1/?file=1323273#file1323273line817>
> >
> > We should have a comment here explaining why we are not returning a failure. Primarily, because some containers might be on host network instead of CNI network.
Added a TODO. We should probably set ip addresses here for containers that join host network.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45620/#review126699
-----------------------------------------------------------
On April 2, 2016, 6:44 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45620/
> -----------------------------------------------------------
>
> (Updated April 2, 2016, 6:44 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Qian Zhang.
>
>
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch also does a few minor cleanups.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b1173d5adee6b4eb45dbed5e693f40ffb5275210
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp af5f49007e0afe57006c9db940611e56eb75a621
>
> Diff: https://reviews.apache.org/r/45620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 45620: Adjusted the status semantics in CNI
isolator.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45620/#review126699
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 817)
<https://reviews.apache.org/r/45620/#comment189716>
We should have a comment here explaining why we are not returning a failure. Primarily, because some containers might be on host network instead of CNI network.
- Avinash sridharan
On April 2, 2016, 6:44 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45620/
> -----------------------------------------------------------
>
> (Updated April 2, 2016, 6:44 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Qian Zhang.
>
>
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch also does a few minor cleanups.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b1173d5adee6b4eb45dbed5e693f40ffb5275210
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp af5f49007e0afe57006c9db940611e56eb75a621
>
> Diff: https://reviews.apache.org/r/45620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 45620: Adjusted the status semantics in CNI
isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On April 5, 2016, 6:32 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 824
> > <https://reviews.apache.org/r/45620/diff/1/?file=1323273#file1323273line824>
> >
> > Just wondering if there are any cases that `containerNetwork.networkInfo` is NONE? I think if we get here then `containerNetwork.networkInfo` must have been set, so maybe it should be a `CHECK(containerNetwork.networkInfo.isSome())`?
Agreed. Added a CHECK.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45620/#review127047
-----------------------------------------------------------
On April 2, 2016, 6:44 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45620/
> -----------------------------------------------------------
>
> (Updated April 2, 2016, 6:44 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Qian Zhang.
>
>
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch also does a few minor cleanups.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b1173d5adee6b4eb45dbed5e693f40ffb5275210
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp af5f49007e0afe57006c9db940611e56eb75a621
>
> Diff: https://reviews.apache.org/r/45620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 45620: Adjusted the status semantics in CNI
isolator.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45620/#review127047
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 824)
<https://reviews.apache.org/r/45620/#comment190187>
Just wondering if there are any cases that `containerNetwork.networkInfo` is NONE? I think if we get here then `containerNetwork.networkInfo` must have been set, so maybe it should be a `CHECK(containerNetwork.networkInfo.isSome())`?
- Qian Zhang
On April 3, 2016, 2:44 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45620/
> -----------------------------------------------------------
>
> (Updated April 3, 2016, 2:44 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Qian Zhang.
>
>
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch also does a few minor cleanups.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b1173d5adee6b4eb45dbed5e693f40ffb5275210
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp af5f49007e0afe57006c9db940611e56eb75a621
>
> Diff: https://reviews.apache.org/r/45620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 45620: Adjusted the status semantics in CNI
isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45620/#review126992
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45620]
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 2, 2016, 6:44 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45620/
> -----------------------------------------------------------
>
> (Updated April 2, 2016, 6:44 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Qian Zhang.
>
>
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch also does a few minor cleanups.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b1173d5adee6b4eb45dbed5e693f40ffb5275210
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp af5f49007e0afe57006c9db940611e56eb75a621
>
> Diff: https://reviews.apache.org/r/45620/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>