mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Avinash sridharan <avin...@mesosphere.io>
Subject Re: Review Request 48527: Add NetworkInfo.labels to CNI Network before passing to CNI plugin.
Date Wed, 15 Jun 2016 06:08:32 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/48527/#comment202843>

    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)
<https://reviews.apache.org/r/48527/#comment202844>

    Don't need this comment.



src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 137)
<https://reviews.apache.org/r/48527/#comment202846>

    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<Nothing> attach(
          const ContainerID& containerId,
          const std::string& networkName,
          const std::string& netNsHandle,
           const TaskID& taskId
          const Option<hashmap<std::string, std::string>>& labels = None());



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 566)
<https://reviews.apache.org/r/48527/#comment202845>

    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)
<https://reviews.apache.org/r/48527/#comment202847>

    Should this be "Read CNI configuration from disk" ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 955)
<https://reviews.apache.org/r/48527/#comment202853>

    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)
<https://reviews.apache.org/r/48527/#comment202852>

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



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 967)
<https://reviews.apache.org/r/48527/#comment202849>

    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)
<https://reviews.apache.org/r/48527/#comment202850>

    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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message