This is an automatically generated e-mail. To reply, visit:

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

     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)

    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

Reply via email to