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




src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (lines 108 - 109)
<https://reviews.apache.org/r/45956/#comment192060>

    can you follow style guide here?



src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 123)
<https://reviews.apache.org/r/45956/#comment192061>

    `const Flags& _flags`



src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 128)
<https://reviews.apache.org/r/45956/#comment192062>

    move up. Please following the original style.



src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 170)
<https://reviews.apache.org/r/45956/#comment192063>

    const Flags flags;



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 80 - 81)
<https://reviews.apache.org/r/45956/#comment192064>

    ```
    new NetworkCniIsolatorProcess(
        flags,
        hashmap<string, NetworkConfigInfo>())));
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 568 - 571)
<https://reviews.apache.org/r/45956/#comment192066>

    ```
    Owned<info> info(new Info(
        containerNetworks,
        containerConfig.rootfs()));
        
    infos.put(containerId, info);
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 661 - 673)
<https://reviews.apache.org/r/45956/#comment192067>

    Can you move that to `_isolate` as well?
    
    ```
    return await(futures)
      .then(defer(
          self(),
          &Self::_isolate,
          containerId,
          pid,
          const list<Future<Nothing>>& attaches));
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 675)
<https://reviews.apache.org/r/45956/#comment192068>

    See my above comments.
    
    Also, capture `this` pointer is in general bad. If this isolator terminates 
(e.g., during teardown in tests) while the above future is still pending, when 
the future becomes ready, it'll cause a segfault.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 689 - 693)
<https://reviews.apache.org/r/45956/#comment192070>

    This should be a CHECK instead?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 702)
<https://reviews.apache.org/r/45956/#comment192071>

    Kill this line.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 706)
<https://reviews.apache.org/r/45956/#comment192072>

    No need to print containerId in the error message here since the caller 
will print it anyway.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 709)
<https://reviews.apache.org/r/45956/#comment192073>

    Add a TODO saying that we only support ipv4 for now



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 714)
<https://reviews.apache.org/r/45956/#comment192074>

    Please align the parameters.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 716 - 717)
<https://reviews.apache.org/r/45956/#comment192077>

    What if IP is not found? Should we use 127.0.0.1?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 722 - 725)
<https://reviews.apache.org/r/45956/#comment192075>

    ```
    Try<net::IPNetwork> ipNetwork = net::IPNetwork::parse(
        network.cniNetworkInfo->ip4().ip(),
        AF_INET);
    
    if (ipNetwork.isError()) {
      return Failure("");
    }
    
    hosts << ipNetwork->address() << " " << containerId << endl;
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 732 - 733)
<https://reviews.apache.org/r/45956/#comment192076>

    This fits in one line?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 750 - 751)
<https://reviews.apache.org/r/45956/#comment192078>

    DItto on alignment.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 752 - 761)
<https://reviews.apache.org/r/45956/#comment192079>

    I probably do
    ```
    if (network.cniNetworkInfo.isNone() ||
        !network.cniNetworkInfo->has_dns()) {
      continue;
    }
    
    foreach (const string& nameserver,
             network.cniNetwork->dns().nameservers()) {
      resolv << "nameserver " << nameserver << endl;
    }
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 754)
<https://reviews.apache.org/r/45956/#comment192080>

    THis is not needed.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 771 - 773)
<https://reviews.apache.org/r/45956/#comment192081>

    Please align `<<`. See other files in the code base.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 776)
<https://reviews.apache.org/r/45956/#comment192082>

    Ditto. Fix all such cases.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 788 - 797)
<https://reviews.apache.org/r/45956/#comment192083>

    I think subprocess supports 'Flags' directly. Please see how to use that in 
port mapping isolator.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 807 - 808)
<https://reviews.apache.org/r/45956/#comment192084>

    Fix the style here.


- Jie Yu


On April 13, 2016, 4:40 a.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
> 
> (Updated April 13, 2016, 4:40 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-4922
>     https://issues.apache.org/jira/browse/MESOS-4922
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 654137c552a7c416f394365e43ea80770fe1ef8d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 159152a01b68a667dbd57fa6452c6c2a3422787c 
> 
> Diff: https://reviews.apache.org/r/45956/diff/
> 
> 
> Testing
> -------
> 
> make
> 
> *Ran mesos_execute with single master/slave setup to verify that containers 
> get the right hostname and network files when attached to a CNI network.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Reply via email to