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 44269: Added the framework of 'network/cni' isolator.
Date Fri, 04 Mar 2016 15:30:24 GMT

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

    Maybe:
     The isolator implements support for Container Network Interface (CNI) specification <Add
github link to spec> . 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)
<https://reviews.apache.org/r/44269/#comment183963>

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

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

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

    We use camel case for variables.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 108)
<https://reviews.apache.org/r/44269/#comment183964>

    Conver this to LOG(ERROR)



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 108 - 115)
<https://reviews.apache.org/r/44269/#comment183967>

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

    Ditto to my comment above on `isSome`



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 129)
<https://reviews.apache.org/r/44269/#comment183969>

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

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

    we can use !isSome?



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 160)
<https://reviews.apache.org/r/44269/#comment183972>

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


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