You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Pierre Cheynier <me...@pierre-cheynier.net> on 2017/02/09 13:00:06 UTC

Review Request 56493: Port_mapping isolator: do not depend on interface speed.

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
-------

Since a lot of environments do not provide interface speed in
/sys/class/net/<ifname>/speed`.


Diffs
-----

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 

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


Testing
-------

./configure
make -j 8
make check


Thanks,

Pierre Cheynier

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56493/#review164970
-----------------------------------------------------------



Patch looks great!

Reviews applied: [56493]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Feb. 9, 2017, 1 p.m., Pierre Cheynier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2017, 1 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
>     https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net/<ifname>/speed`.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> -------
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56493/#review165359
-----------------------------------------------------------


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (line 1588)
<https://reviews.apache.org/r/56493/#comment237191>

    Please use full sentense for comments. For this particular case, please add a tailing period.



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (lines 1593 - 1600)
<https://reviews.apache.org/r/56493/#comment237192>

    Instead of relying on error, can we explicitly check if `/sys/class/net/<ifname>/speed` exists.
    
    ```
    const string eth0SpeedPath =
      path::join("/sys/class/net", eth0.get(), "speed");
    
    if (!os::exists(eth0SpeedPath)) {
      LOG(WARNING) << "...";
    }
    
    Try<string> value = os::read(eth0SpeedPath);
    if (value.isError()) {
      ...
    }
    
    ...
    ```



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (lines 1600 - 1601)
<https://reviews.apache.org/r/56493/#comment237193>

    ```
    } else {
    ```



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (line 1602)
<https://reviews.apache.org/r/56493/#comment237194>

    Please fix the indentation.



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (line 1610)
<https://reviews.apache.org/r/56493/#comment237195>

    ```
    LOG(WARNING) << "Link speed reporting is not supported for '"
                 << eth0.get() << "'";
    ```



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (lines 1612 - 1613)
<https://reviews.apache.org/r/56493/#comment237196>

    Ditto.


- Jie Yu


On Feb. 9, 2017, 1 p.m., Pierre Cheynier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2017, 1 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
>     https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net/<ifname>/speed`.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> -------
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56493/#review165957
-----------------------------------------------------------



Patch looks great!

Reviews applied: [56493]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Feb. 17, 2017, 3:51 p.m., Pierre Cheynier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2017, 3:51 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
>     https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net/<ifname>/speed`.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> -------
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Pierre Cheynier <me...@pierre-cheynier.net>.

> On Feb. 17, 2017, 10:51 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp, line 1597
> > <https://reviews.apache.org/r/56493/diff/4/?file=1638725#file1638725line1597>
> >
> >     We don't add period in log lines.

Sorry for that. I see that you've already merged.


> On Feb. 17, 2017, 10:51 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp, lines 1601-1602
> > <https://reviews.apache.org/r/56493/diff/4/?file=1638725#file1638725line1601>
> >
> >     I would still return Error here.

I do not agree on that part. Here is what I experience and it's the reason of this review:
```
$ ls -la /sys/class/net/team0/speed
-r--r--r-- 1 root root 4096 Feb 20 12:17 /sys/class/net/team0/speed
# file /sys/class/net/team0/speed
/sys/class/net/team0/speed: ERROR: cannot read (Invalid argument)
# cat /sys/class/net/team0/speed
cat: /sys/class/net/team0/speed: Invalid argument
```


- Pierre


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


On Feb. 17, 2017, 3:51 p.m., Pierre Cheynier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2017, 3:51 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
>     https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net/<ifname>/speed`.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> -------
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Jie Yu <yu...@gmail.com>.

> On Feb. 17, 2017, 10:51 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp, lines 1601-1602
> > <https://reviews.apache.org/r/56493/diff/4/?file=1638725#file1638725line1601>
> >
> >     I would still return Error here.
> 
> Pierre Cheynier wrote:
>     I do not agree on that part. Here is what I experience and it's the reason of this review:
>     ```
>     $ ls -la /sys/class/net/team0/speed
>     -r--r--r-- 1 root root 4096 Feb 20 12:17 /sys/class/net/team0/speed
>     # file /sys/class/net/team0/speed
>     /sys/class/net/team0/speed: ERROR: cannot read (Invalid argument)
>     # cat /sys/class/net/team0/speed
>     cat: /sys/class/net/team0/speed: Invalid argument
>     ```

Ah, ok, let me change this back.


- Jie


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


On Feb. 17, 2017, 3:51 p.m., Pierre Cheynier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2017, 3:51 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
>     https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net/<ifname>/speed`.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> -------
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Jie Yu <yu...@gmail.com>.

> On Feb. 17, 2017, 10:51 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp, lines 1601-1602
> > <https://reviews.apache.org/r/56493/diff/4/?file=1638725#file1638725line1601>
> >
> >     I would still return Error here.
> 
> Pierre Cheynier wrote:
>     I do not agree on that part. Here is what I experience and it's the reason of this review:
>     ```
>     $ ls -la /sys/class/net/team0/speed
>     -r--r--r-- 1 root root 4096 Feb 20 12:17 /sys/class/net/team0/speed
>     # file /sys/class/net/team0/speed
>     /sys/class/net/team0/speed: ERROR: cannot read (Invalid argument)
>     # cat /sys/class/net/team0/speed
>     cat: /sys/class/net/team0/speed: Invalid argument
>     ```
> 
> Jie Yu wrote:
>     Ah, ok, let me change this back.

Fixed.


- Jie


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


On Feb. 17, 2017, 3:51 p.m., Pierre Cheynier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2017, 3:51 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
>     https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net/<ifname>/speed`.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> -------
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56493/#review165973
-----------------------------------------------------------


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (line 1597)
<https://reviews.apache.org/r/56493/#comment237833>

    We don't add period in log lines.



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (lines 1601 - 1602)
<https://reviews.apache.org/r/56493/#comment237834>

    I would still return Error here.


- Jie Yu


On Feb. 17, 2017, 3:51 p.m., Pierre Cheynier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2017, 3:51 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
>     https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net/<ifname>/speed`.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> -------
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Pierre Cheynier <me...@pierre-cheynier.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56493/
-----------------------------------------------------------

(Updated Feb. 17, 2017, 3:51 p.m.)


Review request for mesos and Jie Yu.


Changes
-------

One of the case was not well handled due to a syntax error (missing comma)


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


Repository: mesos


Description
-------

Since a lot of environments do not provide interface speed in
/sys/class/net/<ifname>/speed`.


Diffs (updated)
-----

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 

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


Testing
-------

./configure
make -j 8
make check


Thanks,

Pierre Cheynier

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56493/#review165697
-----------------------------------------------------------



Patch looks great!

Reviews applied: [56493]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Feb. 15, 2017, 11:25 a.m., Pierre Cheynier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> -----------------------------------------------------------
> 
> (Updated Feb. 15, 2017, 11:25 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
>     https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net/<ifname>/speed`.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> -------
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Pierre Cheynier <me...@pierre-cheynier.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56493/
-----------------------------------------------------------

(Updated Feb. 15, 2017, 11:25 a.m.)


Review request for mesos and Jie Yu.


Changes
-------

Missed some fixes because of a mess during rbt post.


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


Repository: mesos


Description
-------

Since a lot of environments do not provide interface speed in
/sys/class/net/<ifname>/speed`.


Diffs (updated)
-----

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 

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


Testing
-------

./configure
make -j 8
make check


Thanks,

Pierre Cheynier

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56493/#review165572
-----------------------------------------------------------



Patch looks great!

Reviews applied: [56493]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Feb. 14, 2017, 6:35 p.m., Pierre Cheynier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56493/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2017, 6:35 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7074
>     https://issues.apache.org/jira/browse/MESOS-7074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since a lot of environments do not provide interface speed in
> /sys/class/net/<ifname>/speed`.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 
> 
> Diff: https://reviews.apache.org/r/56493/diff/
> 
> 
> Testing
> -------
> 
> ./configure
> make -j 8
> make check
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 56493: Port_mapping isolator: do not depend on interface speed.

Posted by Pierre Cheynier <me...@pierre-cheynier.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56493/
-----------------------------------------------------------

(Updated Feb. 14, 2017, 6:35 p.m.)


Review request for mesos and Jie Yu.


Changes
-------

Fixed issues reported by @jieyu.


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


Repository: mesos


Description
-------

Since a lot of environments do not provide interface speed in
/sys/class/net/<ifname>/speed`.


Diffs (updated)
-----

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp f6f2bfe1d5d3c113036ad44a480f97bbb462a269 

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


Testing
-------

./configure
make -j 8
make check


Thanks,

Pierre Cheynier