Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

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



I will discard this patch since I have broken it into two:
https://reviews.apache.org/r/44549/
https://reviews.apache.org/r/44555/

- Qian Zhang


On March 8, 2016, 10:42 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 8, 2016, 10:42 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 9, 2016, 2:09 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 95-288
> > 
> >
> > I would strongly recommend move all `parse` and `validation` operations 
> > to `spec.cpp`, because we may not want the `create` method to be that long.

I agree with you. However after discussed with Jie and Avinash, we all agree 
that we do not need to valid the fields in the CNI network configuration file 
since the CNI plugin will do the validation when "network/cni" isolator invokes 
it. So I removed all those field validation code from `create()` method which 
makes it much shorter. Please see https://reviews.apache.org/r/44555/ for 
details and let me know if you have further comments :-)


- Qian


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


On March 8, 2016, 10:42 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 8, 2016, 10:42 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Gilbert Song]

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




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


How about using `'--network_cni_plugins_dir'`?



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


ditto.



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 58 - 60)


return Error(
"The Network CNI plugins directory '" +
flags.network_cni_plugins_dir.get() + "' does not exist");



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 65 - 66)


ditto.



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 95 - 288)


I would strongly recommend move all `parse` and `validation` operations to 
`spec.cpp`, because we may not want the `create` method to be that long.


- Gilbert Song


On March 8, 2016, 6:42 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 8, 2016, 6:42 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

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

(Updated March 8, 2016, 10:42 p.m.)


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


Changes
---

Introduced a protobuf message "NetworkConfig"


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


Repository: mesos


Description
---

Added the framework of 'network/cni' isolator.


Diffs (updated)
-

  src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
  src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Abhishek Dasgupta]

> On March 4, 2016, 6:55 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 61
> > 
> >
> > Add a `+` at the end. While not necessary, makes it more readable.
> 
> Qian Zhang wrote:
> Agree.
> 
> Qian Zhang wrote:
> There will be a compile error if adding a `+` at the end :-(

Probably, you have to add this way:

return Error(
"Failed to find location of the CNI network" +
std::string(" configuration files '") + 
flags.network_cni_config_dir.get() + "'");


- Abhishek


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


On March 6, 2016, 4:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 6, 2016, 4:03 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.
> 
> Qian Zhang wrote:
> My point is, if we can not find a plugin for a named network during 
> initilization, log a warning message to let operator know this issue, and 
> afterward operator can put the plugin in the plugin directory without 
> restarting agent, then the named network can still work.
> 
> Avinash sridharan wrote:
> Lets not rely on the operator heeding WARNING messages and fixing the 
> problem. My concern is that this is a `FATAL` error since before the operator 
> can rectify the error if containers are launched the behavior becomes 
> undefined.
> 
> Qian Zhang wrote:
> Agree, let's return an error :-)
> 
> Qian Zhang wrote:
> After more thinking, I think in this case, it makes more sense to log a 
> warning message and ignore the network config file rather than bail at this 
> point, because there can be other valid network config files. If in the end 
> there is no any valid network config files, we should definitely bail.
> 
> Avinash sridharan wrote:
> I think we should not allow any errors in the configs/plugins passed by 
> the operator . Reason being that frameworks are going to learn about networks 
> out-of-band, and if there are config/plugin errors we will have to throw 
> errors during task launch. Why should we allow the system to proceed knowing 
> that this is going to lead to erroneous situation? The only way the operator 
> can fix this error is by restarting the slave (and fixing the config), so 
> might as well bail out sooner rather than later.
> 
> Qian Zhang wrote:
> Can you please let me know how this can lead to erroneous situation? If a 
> network config file is invalid for whatever reason, "network/isolator" will 
> NOT load it and just ignore it, so how can framework launches a task to join 
> an invalid network which is not loaded by the isolator? I do not think 
> framework user has such knowledge, or you think framework user will know all 
> the network config files (valid or invalid) under "--network_cni_config_dir" 
> in some way?
> 
> Avinash sridharan wrote:
> Frameworks would know only the network name. Its the responsibility of 
> the operator to install the right config for the given `name`. Hence the 
> erroneous case. The fact that config was not loaded for valid network name 
> cause inconsistency between the frameworks view of what is available and the 
> isolators view of what is configurable.
> 
> Qian Zhang wrote:
> What about framework specifies a wrong network name by mistake? Even in 
> `create()` method we ensure every network config file is valid and agent is 
> started successfuly, there is still a chance for framework to specify a wrong 
> network name (which is actually out of our control), right? So my point is, 
> we have to handle this erroneous in launching task case anyway.
> 
> And I do not quite understand what is `the frameworks view of what is 
> available`, can you please elaborate how framework can know what is 
> available? My thinking is, in future we may expose the available CNI networks 
> to framework via an HTTP endpoint or even via an offer as shared resources, 
> but the networks exposed in this way must be the ones which are valid, the 
> invalids will not be loaded by isolator, hense will not be exposed.
> 
> Avinash sridharan wrote:
> Let me try explaining my perspective differently. If the operator was to 
> provide erroneous parameter to the isolator, the isolator would  bail out. My 
> perspective is that an erroneous network config is an erroneous parameter 
> being provided to the isolator and we should treat it so. I believe this 
> would simplify any error handling that needs to be done when frameworks 
> launch tasks on misconfigured networks.

Can you please elaborate how this would simplify any error handling that needs 
to be done when frameworks launch tasks on misconfigured networks? How can 
framework launches task on a misconfigured network which will actually not be 
loaded by isolator?


- Qian


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


On March 7, 2016, 12:03 a.m., Qian Zhang wrote:
> 

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.
> 
> Qian Zhang wrote:
> My point is, if we can not find a plugin for a named network during 
> initilization, log a warning message to let operator know this issue, and 
> afterward operator can put the plugin in the plugin directory without 
> restarting agent, then the named network can still work.
> 
> Avinash sridharan wrote:
> Lets not rely on the operator heeding WARNING messages and fixing the 
> problem. My concern is that this is a `FATAL` error since before the operator 
> can rectify the error if containers are launched the behavior becomes 
> undefined.
> 
> Qian Zhang wrote:
> Agree, let's return an error :-)
> 
> Qian Zhang wrote:
> After more thinking, I think in this case, it makes more sense to log a 
> warning message and ignore the network config file rather than bail at this 
> point, because there can be other valid network config files. If in the end 
> there is no any valid network config files, we should definitely bail.
> 
> Avinash sridharan wrote:
> I think we should not allow any errors in the configs/plugins passed by 
> the operator . Reason being that frameworks are going to learn about networks 
> out-of-band, and if there are config/plugin errors we will have to throw 
> errors during task launch. Why should we allow the system to proceed knowing 
> that this is going to lead to erroneous situation? The only way the operator 
> can fix this error is by restarting the slave (and fixing the config), so 
> might as well bail out sooner rather than later.
> 
> Qian Zhang wrote:
> Can you please let me know how this can lead to erroneous situation? If a 
> network config file is invalid for whatever reason, "network/isolator" will 
> NOT load it and just ignore it, so how can framework launches a task to join 
> an invalid network which is not loaded by the isolator? I do not think 
> framework user has such knowledge, or you think framework user will know all 
> the network config files (valid or invalid) under "--network_cni_config_dir" 
> in some way?
> 
> Avinash sridharan wrote:
> Frameworks would know only the network name. Its the responsibility of 
> the operator to install the right config for the given `name`. Hence the 
> erroneous case. The fact that config was not loaded for valid network name 
> cause inconsistency between the frameworks view of what is available and the 
> isolators view of what is configurable.
> 
> Qian Zhang wrote:
> What about framework specifies a wrong network name by mistake? Even in 
> `create()` method we ensure every network config file is valid and agent is 
> started successfuly, there is still a chance for framework to specify a wrong 
> network name (which is actually out of our control), right? So my point is, 
> we have to handle this erroneous in launching task case anyway.
> 
> And I do not quite understand what is `the frameworks view of what is 
> available`, can you please elaborate how framework can know what is 
> available? My thinking is, in future we may expose the available CNI networks 
> to framework via an HTTP endpoint or even via an offer as shared resources, 
> but the networks exposed in this way must be the ones which are valid, the 
> invalids will not be loaded by isolator, hense will not be exposed.

Let me try explaining my perspective differently. If the operator was to 
provide erroneous parameter to the isolator, the isolator would  bail out. My 
perspective is that an erroneous network config is an erroneous parameter being 
provided to the isolator and we should treat it so. I believe this would 
simplify any error handling that needs to be done when frameworks launch tasks 
on misconfigured networks.


- Avinash


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


On March 6, 2016, 4:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 6, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos, 

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.
> 
> Qian Zhang wrote:
> My point is, if we can not find a plugin for a named network during 
> initilization, log a warning message to let operator know this issue, and 
> afterward operator can put the plugin in the plugin directory without 
> restarting agent, then the named network can still work.
> 
> Avinash sridharan wrote:
> Lets not rely on the operator heeding WARNING messages and fixing the 
> problem. My concern is that this is a `FATAL` error since before the operator 
> can rectify the error if containers are launched the behavior becomes 
> undefined.
> 
> Qian Zhang wrote:
> Agree, let's return an error :-)
> 
> Qian Zhang wrote:
> After more thinking, I think in this case, it makes more sense to log a 
> warning message and ignore the network config file rather than bail at this 
> point, because there can be other valid network config files. If in the end 
> there is no any valid network config files, we should definitely bail.
> 
> Avinash sridharan wrote:
> I think we should not allow any errors in the configs/plugins passed by 
> the operator . Reason being that frameworks are going to learn about networks 
> out-of-band, and if there are config/plugin errors we will have to throw 
> errors during task launch. Why should we allow the system to proceed knowing 
> that this is going to lead to erroneous situation? The only way the operator 
> can fix this error is by restarting the slave (and fixing the config), so 
> might as well bail out sooner rather than later.
> 
> Qian Zhang wrote:
> Can you please let me know how this can lead to erroneous situation? If a 
> network config file is invalid for whatever reason, "network/isolator" will 
> NOT load it and just ignore it, so how can framework launches a task to join 
> an invalid network which is not loaded by the isolator? I do not think 
> framework user has such knowledge, or you think framework user will know all 
> the network config files (valid or invalid) under "--network_cni_config_dir" 
> in some way?
> 
> Avinash sridharan wrote:
> Frameworks would know only the network name. Its the responsibility of 
> the operator to install the right config for the given `name`. Hence the 
> erroneous case. The fact that config was not loaded for valid network name 
> cause inconsistency between the frameworks view of what is available and the 
> isolators view of what is configurable.

What about framework specifies a wrong network name by mistake? Even in 
`create()` method we ensure every network config file is valid and agent is 
started successfuly, there is still a chance for framework to specify a wrong 
network name (which is actually out of our control), right? So my point is, we 
have to handle this erroneous in launching task case anyway.

And I do not quite understand what is `the frameworks view of what is 
available`, can you please elaborate how framework can know what is available? 
My thinking is, in future we may expose the available CNI networks to framework 
via an HTTP endpoint or even via an offer as shared resources, but the networks 
exposed in this way must be the ones which are valid, the invalids will not be 
loaded by isolator, hense will not be exposed.


- Qian


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


On March 7, 2016, 12:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 7, 2016, 12:03 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.
> 
> Qian Zhang wrote:
> My point is, if we can not find a plugin for a named network during 
> initilization, log a warning message to let operator know this issue, and 
> afterward operator can put the plugin in the plugin directory without 
> restarting agent, then the named network can still work.
> 
> Avinash sridharan wrote:
> Lets not rely on the operator heeding WARNING messages and fixing the 
> problem. My concern is that this is a `FATAL` error since before the operator 
> can rectify the error if containers are launched the behavior becomes 
> undefined.
> 
> Qian Zhang wrote:
> Agree, let's return an error :-)
> 
> Qian Zhang wrote:
> After more thinking, I think in this case, it makes more sense to log a 
> warning message and ignore the network config file rather than bail at this 
> point, because there can be other valid network config files. If in the end 
> there is no any valid network config files, we should definitely bail.
> 
> Avinash sridharan wrote:
> I think we should not allow any errors in the configs/plugins passed by 
> the operator . Reason being that frameworks are going to learn about networks 
> out-of-band, and if there are config/plugin errors we will have to throw 
> errors during task launch. Why should we allow the system to proceed knowing 
> that this is going to lead to erroneous situation? The only way the operator 
> can fix this error is by restarting the slave (and fixing the config), so 
> might as well bail out sooner rather than later.
> 
> Qian Zhang wrote:
> Can you please let me know how this can lead to erroneous situation? If a 
> network config file is invalid for whatever reason, "network/isolator" will 
> NOT load it and just ignore it, so how can framework launches a task to join 
> an invalid network which is not loaded by the isolator? I do not think 
> framework user has such knowledge, or you think framework user will know all 
> the network config files (valid or invalid) under "--network_cni_config_dir" 
> in some way?

Frameworks would know only the network name. Its the responsibility of the 
operator to install the right config for the given `name`. Hence the erroneous 
case. The fact that config was not loaded for valid network name cause 
inconsistency between the frameworks view of what is available and the 
isolators view of what is configurable.


- Avinash


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


On March 6, 2016, 4:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 6, 2016, 4:03 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.
> 
> Qian Zhang wrote:
> My point is, if we can not find a plugin for a named network during 
> initilization, log a warning message to let operator know this issue, and 
> afterward operator can put the plugin in the plugin directory without 
> restarting agent, then the named network can still work.
> 
> Avinash sridharan wrote:
> Lets not rely on the operator heeding WARNING messages and fixing the 
> problem. My concern is that this is a `FATAL` error since before the operator 
> can rectify the error if containers are launched the behavior becomes 
> undefined.
> 
> Qian Zhang wrote:
> Agree, let's return an error :-)
> 
> Qian Zhang wrote:
> After more thinking, I think in this case, it makes more sense to log a 
> warning message and ignore the network config file rather than bail at this 
> point, because there can be other valid network config files. If in the end 
> there is no any valid network config files, we should definitely bail.
> 
> Avinash sridharan wrote:
> I think we should not allow any errors in the configs/plugins passed by 
> the operator . Reason being that frameworks are going to learn about networks 
> out-of-band, and if there are config/plugin errors we will have to throw 
> errors during task launch. Why should we allow the system to proceed knowing 
> that this is going to lead to erroneous situation? The only way the operator 
> can fix this error is by restarting the slave (and fixing the config), so 
> might as well bail out sooner rather than later.

Can you please let me know how this can lead to erroneous situation? If a 
network config file is invalid for whatever reason, "network/isolator" will NOT 
load it and just ignore it, so how can framework launches a task to join an 
invalid network which is not loaded by the isolator? I do not think framework 
user has such knowledge, or you think framework user will know all the network 
config files (valid or invalid) under "--network_cni_config_dir" in some way?


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 108-115
> > 
> >
> > isn't this the same as !nameValue.isSome() ?
> > 
> > Also LOG(WARNING) should be LOG(ERROR)
> 
> Qian Zhang wrote:
> Please see 
> https://github.com/apache/mesos/blob/0.27.1/src/docker/docker.cpp#L255:L260 
> for the existing way to handle the return value of `json.get().find()`.
> 
> Avinash sridharan wrote:
> Well... !isSome implies isNone or isError is true which is what you are 
> aiming for ? Will simplify things I guess.
> 
> Qian Zhang wrote:
> Let's follow the existing way, I think it is better to not introduce 
> inconsistence :-)
> 
> Avinash sridharan wrote:
> Inconsistence in terms of coding style is bad. But copying logic just for 
> the sake of consistency doesn't make sense. Just because a logic is 
> implemented a certain way doesn't mean we can't improve on it. I will leave 
> it up to you, but would prefer if we can shorten the code if possible.

Can you please elaborate how to shorten the code? Do you mean instead of 
calling `isNone()` and `isError()` we can simply call `isSome()`? I do not 
think we can do it, because `isNone()` and `isError()` have different purposes, 
we need `isNone()` to handle the case that there is no "name" in the JSON, and 
we need `isError()` to handle the case that there is an error to find "name" in 
the JSON. That's why we return different error for `isNone()` and `isError()` 
respectively.


- Qian


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


On March 7, 2016, 12:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 7, 2016, 12:03 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> 

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 108-115
> > 
> >
> > isn't this the same as !nameValue.isSome() ?
> > 
> > Also LOG(WARNING) should be LOG(ERROR)
> 
> Qian Zhang wrote:
> Please see 
> https://github.com/apache/mesos/blob/0.27.1/src/docker/docker.cpp#L255:L260 
> for the existing way to handle the return value of `json.get().find()`.
> 
> Avinash sridharan wrote:
> Well... !isSome implies isNone or isError is true which is what you are 
> aiming for ? Will simplify things I guess.
> 
> Qian Zhang wrote:
> Let's follow the existing way, I think it is better to not introduce 
> inconsistence :-)

Inconsistence in terms of coding style is bad. But copying logic just for the 
sake of consistency doesn't make sense. Just because a logic is implemented a 
certain way doesn't mean we can't improve on it. I will leave it up to you, but 
would prefer if we can shorten the code if possible.


> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 108
> > 
> >
> > Conver this to LOG(ERROR)
> 
> Qian Zhang wrote:
> I think it should be LOG(WARNING) if we are going to ignore something in 
> the `create()` method of an isolator, please see 
> `PortMappingIsolatorProcess::create()`:
> 
> https://github.com/apache/mesos/blob/0.27.1/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L1321

Semantically this is an error in the network configuration file. Please see my 
comments below about bailing out when a config/plugin error occurs.


> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.
> 
> Qian Zhang wrote:
> My point is, if we can not find a plugin for a named network during 
> initilization, log a warning message to let operator know this issue, and 
> afterward operator can put the plugin in the plugin directory without 
> restarting agent, then the named network can still work.
> 
> Avinash sridharan wrote:
> Lets not rely on the operator heeding WARNING messages and fixing the 
> problem. My concern is that this is a `FATAL` error since before the operator 
> can rectify the error if containers are launched the behavior becomes 
> undefined.
> 
> Qian Zhang wrote:
> Agree, let's return an error :-)
> 
> Qian Zhang wrote:
> After more thinking, I think in this case, it makes more sense to log a 
> warning message and ignore the network config file rather than bail at this 
> point, because there can be other valid network config files. If in the end 
> there is no any valid network config files, we should definitely bail.

I think we should not allow any errors in the configs/plugins passed by the 
operator . Reason being that frameworks are going to learn about networks 
out-of-band, and if there are config/plugin errors we will have to throw errors 
during task launch. Why should we allow the system to proceed knowing that this 
is going to lead to erroneous situation? The only way the operator can fix this 
error is by restarting the slave (and fixing the config), so might as well bail 
out sooner rather than later.


- Avinash


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


On March 6, 2016, 4:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 6, 2016, 4:03 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44004, 44200, 44269]

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, 4:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 6, 2016, 4:03 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

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

(Updated March 7, 2016, 12:03 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
---

Added the framework of 'network/cni' isolator.


Diffs (updated)
-

  src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
  src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.
> 
> Qian Zhang wrote:
> My point is, if we can not find a plugin for a named network during 
> initilization, log a warning message to let operator know this issue, and 
> afterward operator can put the plugin in the plugin directory without 
> restarting agent, then the named network can still work.
> 
> Avinash sridharan wrote:
> Lets not rely on the operator heeding WARNING messages and fixing the 
> problem. My concern is that this is a `FATAL` error since before the operator 
> can rectify the error if containers are launched the behavior becomes 
> undefined.
> 
> Qian Zhang wrote:
> Agree, let's return an error :-)

After more thinking, I think in this case, it makes more sense to log a warning 
message and ignore the network config file rather than bail at this point, 
because there can be other valid network config files. If in the end there is 
no any valid network config files, we should definitely bail.


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 108
> > 
> >
> > Conver this to LOG(ERROR)

I think it should be LOG(WARNING) if we are going to ignore something in the 
`create()` method of an isolator, please see 
`PortMappingIsolatorProcess::create()`:
https://github.com/apache/mesos/blob/0.27.1/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L1321


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 129
> > 
> >
> > Can we also check if the plugin is executable? Since later we are going 
> > to try and execute the plugin.
> 
> Avinash sridharan wrote:
> This might be slightly more involved then I thought. You will have to 
> figure out if the owner or group of the binary is root (since the assumption 
> is agent is running with sudo). If yes then check the permission of the user 
> and the group. If no then check the permission of others. os::permissions 
> should allow you to check all the permissions.

Yes, we should check it. And since the assumption is agent is running with root 
permission, we only need to check if any of user/group/other has "x" 
permission, if yes, root will be able to execute the plugin.


- Qian


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


On March 5, 2016, 1:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 5, 2016, 1:07 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 5, 2016, 2:55 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 61
> > 
> >
> > Add a `+` at the end. While not necessary, makes it more readable.
> 
> Qian Zhang wrote:
> Agree.

There will be a compile error if adding a `+` at the end :-(


- Qian


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


On March 5, 2016, 1:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 5, 2016, 1:07 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 5, 2016, 2:55 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 45
> > 
> >
> > Remove the period at the end. We don't have terminal sentences in ERROR 
> > strings.

Nice catch!


> On March 5, 2016, 2:55 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 50
> > 
> >
> > Remove period.

Nice catch!


> On March 5, 2016, 2:55 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 61
> > 
> >
> > Add a `+` at the end. While not necessary, makes it more readable.

Agree.


- Qian


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


On March 5, 2016, 1:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 5, 2016, 1:07 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 129
> > 
> >
> > Can we also check if the plugin is executable? Since later we are going 
> > to try and execute the plugin.

This might be slightly more involved then I thought. You will have to figure 
out if the owner or group of the binary is root (since the assumption is agent 
is running with sudo). If yes then check the permission of the user and the 
group. If no then check the permission of others. os::permissions should allow 
you to check all the permissions.


- Avinash


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


On March 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44004, 44200, 44269]

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 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

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




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


Remove the period at the end. We don't have terminal sentences in ERROR 
strings.



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


Remove period.



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


Add a `+` at the end. While not necessary, makes it more readable.


- Avinash sridharan


On March 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 80
> > 
> >
> > Shouldn't we do a size check here as well ? Bail out if we don't have 
> > any files in this directory ?
> 
> Qian Zhang wrote:
> The check `if (netConfigs.size() == 0) {` below it has already covered 
> that case.

Yeah I see that. Was just wondering if it makes sense to do the `foreach` if 
size is 0?


- Avinash


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


On March 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 108-115
> > 
> >
> > isn't this the same as !nameValue.isSome() ?
> > 
> > Also LOG(WARNING) should be LOG(ERROR)
> 
> Qian Zhang wrote:
> Please see 
> https://github.com/apache/mesos/blob/0.27.1/src/docker/docker.cpp#L255:L260 
> for the existing way to handle the return value of `json.get().find()`.

Well... !isSome implies isNone or isError is true which is what you are aiming 
for ? Will simplify things I guess.


> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.
> 
> Qian Zhang wrote:
> My point is, if we can not find a plugin for a named network during 
> initilization, log a warning message to let operator know this issue, and 
> afterward operator can put the plugin in the plugin directory without 
> restarting agent, then the named network can still work.

Lets not rely on the operator heeding WARNING messages and fixing the problem. 
My concern is that this is a `FATAL` error since before the operator can 
rectify the error if containers are launched the behavior becomes undefined.


- Avinash


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


On March 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

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

(Updated March 5, 2016, 1:07 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
---

Added the framework of 'network/cni' isolator.


Diffs (updated)
-

  src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
  src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 28
> > 
> >
> > Maybe:
> >  The isolator implements support for Container Network Interface (CNI) 
> > specification  . It provides network isolation to 
> > containers by creating a network namespace for each container, and then 
> > adding the container to the CNI network sepcified in the `NetworkInfo` for 
> > the container. It adds the container to a CNI network by using CNI plugins 
> > specified by the operator for the corresponding CNI network.

Agree.


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 80
> > 
> >
> > Shouldn't we do a size check here as well ? Bail out if we don't have 
> > any files in this directory ?

The check `if (netConfigs.size() == 0) {` below it has already covered that 
case.


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 108-115
> > 
> >
> > isn't this the same as !nameValue.isSome() ?
> > 
> > Also LOG(WARNING) should be LOG(ERROR)

Please see 
https://github.com/apache/mesos/blob/0.27.1/src/docker/docker.cpp#L255:L260 for 
the existing way to handle the return value of `json.get().find()`.


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.

My point is, if we can not find a plugin for a named network during 
initilization, log a warning message to let operator know this issue, and 
afterward operator can put the plugin in the plugin directory without 
restarting agent, then the named network can still work.


- Qian


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


On March 4, 2016, 10:34 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 10:34 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44200, 44269]

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 4, 2016, 2:34 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 2:34 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

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




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


Maybe:
 The isolator implements support for Container Network Interface (CNI) 
specification  . It provides network isolation to 
containers by creating a network namespace for each container, and then adding 
the container to the CNI network sepcified in the `NetworkInfo` for the 
container. It adds the container to a CNI network by using CNI plugins 
specified by the operator for the corresponding CNI network.



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


We use camel case for private variables. 

Why the const qualifier? I see that currently you are creating the hashmap 
during create and I guess you are assuming you are going to update it during 
the lifetime of the isolator, but what if in the future we allow updates to 
configs? Lets remove the const over here.



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


shouldn't we be returning an error here as well ? Without any plugins CNI 
can't do much. Or can we still do operations (port forwarding, --net=none) ?



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


Shouldn't we do a size check here as well ? Bail out if we don't have any 
files in this directory ?



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


We use camel case for variables.



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


Conver this to LOG(ERROR)



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 108 - 115)


isn't this the same as !nameValue.isSome() ?

Also LOG(WARNING) should be LOG(ERROR)



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 118 - 126)


Ditto to my comment above on `isSome`



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


Can we also check if the plugin is executable? Since later we are going to 
try and execute the plugin.



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


I don't understand this comment. We just made sure the plugin does not 
exist? So what does the comment imply "it can
// still be valid as long as operator puts the CNI plugin binary
// that it uses under '--network_cni_plugins_dir'." ?

I think at this point we should return an error. If can't find an 
executable for a named network, the behavior will become undefined. We should 
bail at this point.



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 142 - 150)


we can use !isSome?



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


Ditto comment on bailing out if we can't verify the plugin?


- Avinash sridharan


On March 4, 2016, 2:34 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 2:34 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

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

(Updated March 4, 2016, 10:34 p.m.)


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


Changes
---

Implemented NetworkCniIsolatorProcess::create().


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


Repository: mesos


Description
---

Added the framework of 'network/cni' isolator.


Diffs (updated)
-

  src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
  src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 4, 2016, 1:15 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 24
> > 
> >
> > We try to avoid using this now. Let's use 'using process::XXX' 
> > explicitly here. In your case:
> > 
> > ```
> > using process::Future;
> > using process::Owned;
> > using process::PID;
> > ```

Agree!


- Qian


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


On March 3, 2016, 9:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 3, 2016, 9:57 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 4, 2016, 1:19 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 26-27
> > 
> >
> > Depending on the comment above, please move `using std::XX` above 
> > `using process::XX`. Thanks.

OK.


- Qian


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


On March 3, 2016, 9:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 3, 2016, 9:57 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44200, 44269]

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 3, 2016, 1:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 3, 2016, 1:57 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Gilbert Song]

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




src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 26 - 27)


Depending on the comment above, please move `using std::XX` above `using 
process::XX`. Thanks.


- Gilbert Song


On March 3, 2016, 5:57 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 3, 2016, 5:57 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Gilbert Song]

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




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


We try to avoid using this now. Let's use 'using process::XXX' explicitly 
here. In your case:

```
using process::Future;
using process::Owned;
using process::PID;
```


- Gilbert Song


On March 3, 2016, 5:57 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 3, 2016, 5:57 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

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

(Updated March 3, 2016, 9:57 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
---

Added the framework of 'network/cni' isolator.


Diffs (updated)
-

  src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
  src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

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

(Updated March 3, 2016, 5:58 p.m.)


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


Changes
---

Addressed Avinash's comments.


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


Repository: mesos


Description
---

Added the framework of 'network/cni' isolator.


Diffs (updated)
-

  src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
  src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 3, 2016, 12:24 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 81-84
> > 
> >
> > Lets do this in a separate patch once the implementation for the 
> > isolator is complete.

Agree, the reason I did this was, I wanted to do some tests by starting agent 
with "network/cni" isolator created.


> On March 3, 2016, 12:24 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 223
> > 
> >
> > We should introduce the `network/cni` isolator into the containerizer 
> > till the implementation is complete. Lets remove this and submit it in a 
> > separate patch once the implementation is complete.

Agree, the reason I did this was, I wanted to do some tests by starting agent 
with "network/cni" isolator created.


> On March 3, 2016, 12:24 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 49
> > 
> >
> > Doesn't this fit in a single line?

No, it will be more than 80 chars in a single line :-(


> On March 3, 2016, 12:24 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 41
> > 
> >
> > explicit checks? (==0)

I think the convention is implicit check, you can check other codes in Mesos 
which call os::exists(), e.g., 
https://github.com/apache/mesos/blob/0.27.0/src/slave/slave.cpp#L418


> On March 3, 2016, 12:24 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 42
> > 
> >
> > s/directory/location
> > 
> > Shouldn't the indentation be:
> > return Error(
> >  "Failed to ..." +
> >  "..."

Agree!


> On March 3, 2016, 12:24 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 50
> > 
> >
> > Should we be listing all the files in the config directory and reading 
> > the configs over here?

Yes, we should. However I see Gilbert raised a comment in 
https://reviews.apache.org/r/44200/ about directly passing JSON to agent, so 
let's wait for the decision for that comment :-)


- Qian


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


On March 2, 2016, 11:25 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 2, 2016, 11:25 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9d29fad396cdac0f263c5d52460030630fc2dfaf 
>   src/Makefile.am fe549a881e778e094d3f957e5119eeb2f55e7915 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44200, 44269]

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 2, 2016, 3:25 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 2, 2016, 3:25 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9d29fad396cdac0f263c5d52460030630fc2dfaf 
>   src/Makefile.am fe549a881e778e094d3f957e5119eeb2f55e7915 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

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




src/slave/containerizer/mesos/containerizer.cpp (lines 81 - 84)


Lets do this in a separate patch once the implementation for the isolator 
is complete.



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


We should introduce the `network/cni` isolator into the containerizer till 
the implementation is complete. Lets remove this and submit it in a separate 
patch once the implementation is complete.



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


Doesn't this fit in a single line?



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


explicit checks? (==0)



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


s/directory/location

Shouldn't the indentation be:
return Error(
 "Failed to ..." +
 "..."



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


Ditto on explicit checks.



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


s/directory/location

Ditto with the indentation.



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


Should we be listing all the files in the config directory and reading the 
configs over here?


- Avinash sridharan


On March 2, 2016, 3:25 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 2, 2016, 3:25 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
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9d29fad396cdac0f263c5d52460030630fc2dfaf 
>   src/Makefile.am fe549a881e778e094d3f957e5119eeb2f55e7915 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>