You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Dmitry Zhuk <dz...@twopensource.com> on 2017/01/30 15:20:07 UTC

Review Request 56080: Fixed error handling when writing to a cgroup control file.

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
-------

Due to internal buffering in ofstream, actual write to the underlying
device may not be performed by the time of checking for failures.
This patch flushes written data to ensure fail check uses actual value.


Diffs
-----

  src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
  src/tests/containerizer/cgroups_tests.cpp 5c1ef70fb1feb76de602418286989b48563f26ff 

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


Testing
-------

sudo make check


Thanks,

Dmitry Zhuk

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

Posted by Dmitry Zhuk <dz...@twopensource.com>.

> On Jan. 30, 2017, 5:14 p.m., James Peach wrote:
> > Since you need to detect errors, why use `iostreams` at all? This code is just writing a buffer to a file, so `os::write` would let you do the same thing with lower overhead and improved error handling.
> > 
> > AFAICT this function could be written as:
> > ```C
> > static Try<Nothing> write(
> >     const string& hierarchy,
> >     const string& cgroup,
> >     const string& control,
> >     const string& value)
> > {
> >   string path = path::join(hierarchy, cgroup, control);
> >   return os::write(path, value);
> > }
> > 
> > ```

There's similar code in `read`, which can be replaced with `os::read` (issue in `TODO(benh)` seems to be resolved already).
Shall this be fixed in this review request? Or I can sumbit a separate review for `os::read` and `os::write`, though it probably worth merging this patch to update tests.


- Dmitry


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


On Jan. 30, 2017, 3:20 p.m., Dmitry Zhuk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
>     https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56080/#review163532
-----------------------------------------------------------



Since you need to detect errors, why use `iostreams` at all? This code is just writing a buffer to a file, so `os::write` would let you do the same thing with lower overhead and improved error handling.

AFAICT this function could be written as:
```C
static Try<Nothing> write(
    const string& hierarchy,
    const string& cgroup,
    const string& control,
    const string& value)
{
  string path = path::join(hierarchy, cgroup, control);
  return os::write(path, value);
}

```

- James Peach


On Jan. 30, 2017, 3:20 p.m., Dmitry Zhuk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
>     https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

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


Fix it, then Ship it!





src/tests/containerizer/cgroups_tests.cpp (lines 497 - 498)
<https://reviews.apache.org/r/56080/#comment234987>

    Let's move this up right below:
    ```
    ASSERT_SOME(cgroups::create(hierarchy, TEST_CGROUPS_ROOT));
    ```


- Jie Yu


On Jan. 30, 2017, 3:20 p.m., Dmitry Zhuk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
>     https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

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



Patch looks great!

Reviews applied: [56080]

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 Jan. 30, 2017, 3:20 p.m., Dmitry Zhuk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
>     https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

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



Patch looks great!

Reviews applied: [56080]

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 Jan. 30, 2017, 7:07 p.m., Dmitry Zhuk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56080/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2017, 7:07 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7020
>     https://issues.apache.org/jira/browse/MESOS-7020
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Due to internal buffering in ofstream, actual write to the underlying
> device may not be performed by the time of checking for failures.
> This patch flushes written data to ensure fail check uses actual value.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 5c1ef70fb1feb76de602418286989b48563f26ff 
> 
> Diff: https://reviews.apache.org/r/56080/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 56080: Fixed error handling when writing to a cgroup control file.

Posted by Dmitry Zhuk <dz...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56080/
-----------------------------------------------------------

(Updated Jan. 30, 2017, 7:07 p.m.)


Review request for mesos and Jie Yu.


Changes
-------

Updated test according to review comments.


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


Repository: mesos


Description
-------

Due to internal buffering in ofstream, actual write to the underlying
device may not be performed by the time of checking for failures.
This patch flushes written data to ensure fail check uses actual value.


Diffs (updated)
-----

  src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
  src/tests/containerizer/cgroups_tests.cpp 5c1ef70fb1feb76de602418286989b48563f26ff 

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


Testing
-------

sudo make check


Thanks,

Dmitry Zhuk