Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-17 Thread Jie Yu 

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


Ship it!




I'll make necessary changes and commit this for you.

- Jie Yu


On June 17, 2016, 12:31 a.m., Dan Osborne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48527/
> ---
> 
> (Updated June 17, 2016, 12:31 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5592
> https://issues.apache.org/jira/browse/MESOS-5592
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/48527
> 
> Fixes.
> 
> Review: https://reviews.apache.org/r/48754
> 
> Code review fixes.
> 
> 
> Checkpointed mutated networkinfo.
> 
> 
> Added error message if args is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 106ff35320f37b2e75ed60381de1406459e6d515 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
> 8af50c11b00240ac1d24605b119acbd96aaa50be 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
> b3fb0a32e3e74ca859ca58085efed5a87e4e626b 
> 
> Diff: https://reviews.apache.org/r/48527/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dan Osborne
> 
>

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-16 Thread Dan Osborne 

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

(Updated June 17, 2016, 12:31 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Review: https://reviews.apache.org/r/48527

Fixes.

Review: https://reviews.apache.org/r/48754

Code review fixes.


Checkpointed mutated networkinfo.


Added error message if args is specified.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
09890cedf2e7a1846bd1cb250e117be1680a1b80 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
106ff35320f37b2e75ed60381de1406459e6d515 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
8af50c11b00240ac1d24605b119acbd96aaa50be 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
b3fb0a32e3e74ca859ca58085efed5a87e4e626b 

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


Testing
---


Thanks,

Dan Osborne

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-16 Thread Jie Yu 


> On June 15, 2016, 6:08 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 987
> > 
> >
> > Is there a guarantee that the plugin would be waiting for the isolator 
> > to `write` into the PIPE ?
> > Also, this can potentially block the `network/cni` isolator if the 
> > plugin is not fast enough in reading the pipe.
> > 
> > Can we follow the existing idiom of writing mutated JSON into a temp 
> > file? Probably checkpoint the JSON into the containers checkpoint DIR and 
> > set the stdinof the plugin to the checkpointed JSON config. This would also 
> > serve the dual purpose of checkpointing the mutated JSON to be used when 
> > deleteing the container.

See my comments below. I won't be too worried about the the pipe full issue as 
network config is typically pretty small and the kernel buffer for a pipe is 
usually 64K. Also, the spec says that the plugin will read the stdin for config.

If you're really worried about the blocking issue, use async process::io::write 
should solve the issue.


- Jie


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


On June 15, 2016, 10:23 p.m., Dan Osborne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48527/
> ---
> 
> (Updated June 15, 2016, 10:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/48527
> 
> Fixes.
> 
> Review: https://reviews.apache.org/r/48754
> 
> Code review fixes.
> 
> 
> Checkpointed mutated networkinfo.
> 
> 
> Added error message if args is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 106ff35320f37b2e75ed60381de1406459e6d515 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
> 8af50c11b00240ac1d24605b119acbd96aaa50be 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
> b3fb0a32e3e74ca859ca58085efed5a87e4e626b 
> 
> Diff: https://reviews.apache.org/r/48527/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dan Osborne
> 
>

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-16 Thread Jie Yu 

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




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


Why 'taskId' here? If it's a command task, that's clear what taskId is. But 
what if this is a custom executor with multiple tasks, what is this id for?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 968 - 971)


I would prefer the following for the `args` json:
```
{
  ...
  "args" : {
"mesos" : {
  "network_info" : {
... // JSON rep. of NetworkInfo.
  }
}
  }
}
```



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


I would prefer not introducing a new checkpointed file which we will 
deprecate soon. In the future, we need to checkpoint the network config json 
anyway because technically, when we call `plugin DEL`, we need to provide an 
identical network config as we provide to `plugin ADD`.

I think the previous way of using a PIPE for stdin sounds fine to me.


- Jie Yu


On June 15, 2016, 10:23 p.m., Dan Osborne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48527/
> ---
> 
> (Updated June 15, 2016, 10:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/48527
> 
> Fixes.
> 
> Review: https://reviews.apache.org/r/48754
> 
> Code review fixes.
> 
> 
> Checkpointed mutated networkinfo.
> 
> 
> Added error message if args is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 106ff35320f37b2e75ed60381de1406459e6d515 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
> 8af50c11b00240ac1d24605b119acbd96aaa50be 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
> b3fb0a32e3e74ca859ca58085efed5a87e4e626b 
> 
> Diff: https://reviews.apache.org/r/48527/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dan Osborne
> 
>

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-15 Thread Dan Osborne 

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




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


I'm not a fan of the number of JSON::Objects that were initialized here, 
but couldn't figure out a way to more dynamically create entries in 
`config['args']['labels']`. Any suggestions?


- Dan Osborne


On June 15, 2016, 10:23 p.m., Dan Osborne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48527/
> ---
> 
> (Updated June 15, 2016, 10:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/48527
> 
> Fixes.
> 
> Review: https://reviews.apache.org/r/48754
> 
> Code review fixes.
> 
> 
> Checkpointed mutated networkinfo.
> 
> 
> Added error message if args is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 106ff35320f37b2e75ed60381de1406459e6d515 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
> 8af50c11b00240ac1d24605b119acbd96aaa50be 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
> b3fb0a32e3e74ca859ca58085efed5a87e4e626b 
> 
> Diff: https://reviews.apache.org/r/48527/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dan Osborne
> 
>

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-15 Thread Dan Osborne 


> On June 15, 2016, 6:08 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 967
> > 
> >
> > We should make these keys ("labels", "task-id", "args") as `constexpr` 
> > in spec.hpp.

Can you point me to a similar constexpr definition that I can mirror?


- Dan


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


On June 15, 2016, 10:23 p.m., Dan Osborne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48527/
> ---
> 
> (Updated June 15, 2016, 10:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/48527
> 
> Fixes.
> 
> Review: https://reviews.apache.org/r/48754
> 
> Code review fixes.
> 
> 
> Checkpointed mutated networkinfo.
> 
> 
> Added error message if args is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 106ff35320f37b2e75ed60381de1406459e6d515 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
> 8af50c11b00240ac1d24605b119acbd96aaa50be 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
> b3fb0a32e3e74ca859ca58085efed5a87e4e626b 
> 
> Diff: https://reviews.apache.org/r/48527/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dan Osborne
> 
>

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-15 Thread Dan Osborne 

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

(Updated June 15, 2016, 10:23 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description (updated)
---

Review: https://reviews.apache.org/r/48527

Fixes.

Review: https://reviews.apache.org/r/48754

Code review fixes.


Checkpointed mutated networkinfo.


Added error message if args is specified.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
09890cedf2e7a1846bd1cb250e117be1680a1b80 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
106ff35320f37b2e75ed60381de1406459e6d515 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
8af50c11b00240ac1d24605b119acbd96aaa50be 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
b3fb0a32e3e74ca859ca58085efed5a87e4e626b 

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


Testing
---


Thanks,

Dan Osborne

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-15 Thread Dan Osborne 

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

(Updated June 15, 2016, 9:41 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description (updated)
---

Review: https://reviews.apache.org/r/48527

Fixes.

Review: https://reviews.apache.org/r/48754

Code review fixes.


Checkpointed mutated networkinfo.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
09890cedf2e7a1846bd1cb250e117be1680a1b80 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
106ff35320f37b2e75ed60381de1406459e6d515 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
8af50c11b00240ac1d24605b119acbd96aaa50be 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
b3fb0a32e3e74ca859ca58085efed5a87e4e626b 

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


Testing
---


Thanks,

Dan Osborne

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-15 Thread Dan Osborne 

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

(Updated June 15, 2016, 9:10 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description (updated)
---

Review: https://reviews.apache.org/r/48527

Fixes.

Review: https://reviews.apache.org/r/48754

Code review fixes.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
09890cedf2e7a1846bd1cb250e117be1680a1b80 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
106ff35320f37b2e75ed60381de1406459e6d515 

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


Testing
---


Thanks,

Dan Osborne

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-15 Thread Avinash sridharan 

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




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


Should this be an `Option` ? Since, not all containers would have lable 
set? Also, why have a separate field here if it's already contained in 
`NetworkInfo`?



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


Don't need this comment.



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


This could be an `Option` since not all `NetworkInfo` would have their 
`labels` set. `Labels` is an optional field in `NetworkInfo`.

Also, the signature could be:
process::Future attach(
  const ContainerID& containerId,
  const std::string& networkName,
  const std::string& netNsHandle,
   const TaskID& taskId
  const Option>& labels = None());



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


This is not required. 
We could just do :
const mesos::TaskID taskId = containerConfig.task_info().task_id();



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


Should this be "Read CNI configuration from disk" ?



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


Can we add a comment here for the purposed of the `args` key in the CNI 
config. Maybe a brife description and point to a github issue if reader is 
interested in understanding the purpose of the `args` key?



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


Should there be an `else` clause ? What happens if the `labels` key exists?



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


We should make these keys ("labels", "task-id", "args") as `constexpr` in 
spec.hpp.



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


Is there a guarantee that the plugin would be waiting for the isolator to 
`write` into the PIPE ?
Also, this can potentially block the `network/cni` isolator if the plugin 
is not fast enough in reading the pipe.

Can we follow the existing idiom of writing mutated JSON into a temp file? 
Probably checkpoint the JSON into the containers checkpoint DIR and set the 
stdinof the plugin to the checkpointed JSON config. This would also serve the 
dual purpose of checkpointing the mutated JSON to be used when deleteing the 
container.


- Avinash sridharan


On June 14, 2016, 7:55 p.m., Dan Osborne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48527/
> ---
> 
> (Updated June 14, 2016, 7:55 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add NetworkInfo.labels to CNI Network before passing to CNI plugin.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 106ff35320f37b2e75ed60381de1406459e6d515 
> 
> Diff: https://reviews.apache.org/r/48527/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dan Osborne
> 
>

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-13 Thread Mesos ReviewBot 

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos ReviewBot


On June 14, 2016, 1:05 a.m., Dan Osborne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48527/
> ---
> 
> (Updated June 14, 2016, 1:05 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add NetworkInfo.labels to CNI Network before passing to CNI plugin.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 106ff35320f37b2e75ed60381de1406459e6d515 
> 
> Diff: https://reviews.apache.org/r/48527/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dan Osborne
> 
>

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-13 Thread Dan Osborne 

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

(Updated June 14, 2016, 1:05 a.m.)


Review request for mesos.


Repository: mesos


Description
---

Add NetworkInfo.labels to CNI Network before passing to CNI plugin.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
09890cedf2e7a1846bd1cb250e117be1680a1b80 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
106ff35320f37b2e75ed60381de1406459e6d515 

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


Testing
---


Thanks,

Dan Osborne

Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.

2016-06-09 Thread Mesos ReviewBot 

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos ReviewBot


On June 10, 2016, 12:16 a.m., Dan Osborne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48527/
> ---
> 
> (Updated June 10, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add NetworkInfo.labels to CNI Network before passing to CNI plugin.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 106ff35320f37b2e75ed60381de1406459e6d515 
> 
> Diff: https://reviews.apache.org/r/48527/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dan Osborne
> 
>