You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Jason Lai <ja...@jasonlai.net> on 2016/12/13 05:12:05 UTC

Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

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

Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.


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


Repository: mesos


Description
-------

Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.


Diffs
-----

  include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp PRE-CREATION 

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


Testing
-------


Thanks,

Jason Lai

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Gilbert Song <so...@gmail.com>.

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




include/mesos/mesos.proto
Lines 2178 (patched)
<https://reviews.apache.org/r/54693/#comment251574>

    could we add this link in comment?
    `https://www.kernel.org/doc/Documentation/cgroup-v1/blkio-controller.txt`



include/mesos/mesos.proto
Lines 2195 (patched)
<https://reviews.apache.org/r/54693/#comment251255>

    how about s/value/rate/g?


- Gilbert Song


On Dec. 21, 2016, 12:06 p.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> 
> Diff: https://reviews.apache.org/r/54693/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Joris Van Remoortere <jo...@gmail.com>.

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



Closing this review due to inactivity. Please see our [guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md) for reopening reviews.

- Joris Van Remoortere


On Dec. 21, 2016, 8:06 p.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 8:06 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> 
> Diff: https://reviews.apache.org/r/54693/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Jason Lai <ja...@jasonlai.net>.

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

(Updated June 26, 2017, 10:50 p.m.)


Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.


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


Repository: mesos


Description
-------

Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.


Diffs
-----

  include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 


Diff: https://reviews.apache.org/r/54693/diff/3/


Testing
-------


Thanks,

Jason Lai

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Gilbert Song <so...@gmail.com>.

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




include/mesos/mesos.proto
Lines 2244-2246 (patched)
<https://reviews.apache.org/r/54693/#comment251722>

    Could we elaborate more here, since we have two `Total`:
    1. Operation type `Total`.
    2. At the last line of some cgroup file, we have a summerized value `Total`.
    
    Also, seems like we are going to traverse a cgroup hierarchy only once and collect statistics all together. The statistics will be devided by devices (or it represents a general `Total` from the last line of a cgroup file). Then, for the `Cgroup::blkio::Statistics`, we have a structure like:
    1:0 -> all device stats (CFQ, CFQ_recursive, Throttle)
    1:1 -> all device stats (CFQ, CFQ_recursive, Throttle)
    2:0 -> all device stats (CFQ, CFQ_recursive, Throttle)
    ...
    8:2 -> all device stats (CFQ, CFQ_recursive, Throttle)
    Total -> all summerized stats (CFQ, CFQ_recursive, Throttle)
    
    I am fine with it but we need to be clear in comment. People may be confused about it.



include/mesos/mesos.proto
Lines 2260-2275 (patched)
<https://reviews.apache.org/r/54693/#comment251723>

    Will these fields be used in `ContainerStatus`?
    
    We only include `Blkio::ResourceStatistics` in `ResourceStatistics`. And these fields are inside of `CgroupInfo`, which means that they will only be set in `Blkio::Subsystem::status()`, right?
    
    Understand that we want to keep all `blkio` related proto grouped at one place, but we need to comment that explicitly.



include/mesos/mesos.proto
Lines 2287 (patched)
<https://reviews.apache.org/r/54693/#comment251724>

    As mentioned above, let's comment that this `CgruopInfo::ResourceStatistics` will only be consumed in `ResourceStatistics` protobuf message, which reports container usages.



include/mesos/mesos.proto
Lines 2287-2290 (patched)
<https://reviews.apache.org/r/54693/#comment251725>

    Move above `NetCls`?


- Gilbert Song


On Dec. 21, 2016, 12:06 p.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> 
> Diff: https://reviews.apache.org/r/54693/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Gilbert Song <so...@gmail.com>.

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



@Jason, could you reopen this patch for review? I want to apply it to my chain. Thanks.

- Gilbert Song


On Dec. 21, 2016, 12:06 p.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> 
> Diff: https://reviews.apache.org/r/54693/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Jason Lai <ja...@jasonlai.net>.


> On Dec. 21, 2016, 11:40 p.m., Zhitao Li wrote:
> > include/mesos/mesos.proto, lines 1107-1108
> > <https://reviews.apache.org/r/54693/diff/2-3/?file=1590188#file1590188line1107>
> >
> >     Why is this newly added field in the middle of the message rather than the end?

Make sense now since I just renamed the `blkio` field to a more generic field named `cgroups`. The initial placement was in purpose of putting `blkio` closer to `disk_*` fields.

I'll address this altogether after @jieyu's comment.


- Jason


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


On Dec. 21, 2016, 8:06 p.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 8:06 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Zhitao Li <zh...@gmail.com>.

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




include/mesos/mesos.proto (lines 1107 - 1108)
<https://reviews.apache.org/r/54693/#comment230933>

    Why is this newly added field in the middle of the message rather than the end?


- Zhitao Li


On Dec. 21, 2016, 8:06 p.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 8:06 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Jason Lai <ja...@jasonlai.net>.

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

(Updated Dec. 21, 2016, 8:06 p.m.)


Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.


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


Repository: mesos


Description
-------

Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.


Diffs (updated)
-----

  include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 

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


Testing
-------


Thanks,

Jason Lai

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [53546, 54693]

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 Dec. 21, 2016, 2:19 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 2:19 a.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Jason Lai <ja...@jasonlai.net>.

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

(Updated Dec. 21, 2016, 2:19 a.m.)


Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.


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


Repository: mesos


Description
-------

Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.


Diffs
-----

  include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 

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


Testing
-------


Thanks,

Jason Lai

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Jason Lai <ja...@jasonlai.net>.

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

(Updated Dec. 21, 2016, 2:16 a.m.)


Review request for .


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


Repository: mesos


Description
-------

Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.


Diffs (updated)
-----

  include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 

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


Testing
-------


Thanks,

Jason Lai

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Jason Lai <ja...@jasonlai.net>.


> On Dec. 13, 2016, 7:29 a.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp, lines 54-57
> > <https://reviews.apache.org/r/54693/diff/1/?file=1582392#file1582392line54>
> >
> >     Let's only add defintions for proto in this file.

Sure. I was initially hoping to add code for implementations, but seems like it will make the scope of this diff much bigger. I'll revert these 2 files in the diff.


- Jason


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


On Dec. 13, 2016, 5:12 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by haosdent huang <ha...@gmail.com>.

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




include/mesos/mesos.proto (line 1106)
<https://reviews.apache.org/r/54693/#comment229853>

    ```
    // Blkio statistics.
    ```



include/mesos/mesos.proto (line 2225)
<https://reviews.apache.org/r/54693/#comment229855>

    We need add a comment that when `device` is emtpy, it represent `All`.



include/mesos/mesos.proto (lines 2228 - 2229)
<https://reviews.apache.org/r/54693/#comment229856>

    I am not sure in which case we need the non-recursive one. If we don't have user cases which need this, maybe we could remove it?



src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp (lines 52 - 55)
<https://reviews.apache.org/r/54693/#comment229857>

    Let's only add defintions for proto in this file.


- haosdent huang


On Dec. 13, 2016, 5:12 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Mesos ReviewBot <re...@mesos.apache.org>.

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



Bad patch!

Reviews applied: [54693, 53546]

Failed command: ./support/apply-review.sh -n -r 53546

Error:
2016-12-13 15:17:50 URL:https://reviews.apache.org/r/53546/diff/raw/ [30079/30079] -> "53546.patch" [1]
error: patch failed: src/CMakeLists.txt:160
error: src/CMakeLists.txt: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/16438/console

- Mesos ReviewBot


On Dec. 13, 2016, 5:12 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Jason Lai <ja...@jasonlai.net>.


> On Dec. 17, 2016, 1:25 a.m., Jie Yu wrote:
> > include/mesos/mesos.proto, line 2188
> > <https://reviews.apache.org/r/54693/diff/1/?file=1582391#file1582391line2188>
> >
> >     I'd introduce CgroupInfo::Blkio::CFQ CgroupInfo::Blkio::Throttling message here. Looks like for the fields below, we should separate them. For instance, `read_bps_device` is only for throttling iosched.

I have some reservation on introducing message `CFQ` for both config and stats. CFQ may be good for the stats part, but the configuration-related control files involved only include are fairly straightforward:
* `blkio.weight` (also `_device`)
* `blkio.leaf_weight` (also `_device`).

Do you think we need to introduce an extra level as a field named `cfq` for weights?


> On Dec. 17, 2016, 1:25 a.m., Jie Yu wrote:
> > include/mesos/mesos.proto, line 2214
> > <https://reviews.apache.org/r/54693/diff/1/?file=1582391#file1582391line2214>
> >
> >     I am wondering if the following structure is better:
> >     
> >     ```
> >     message Blkio {
> >       message CFQ {
> >         message Statistics {
> >           optional Device device;
> >           optional uint64 sectors;
> >           optional uint64 time;
> >           repeated Entry io_serviced;
> >           ...
> >         }
> >         
> >         repeated DeviceWeight weight_devices;
> >         repeated DeviceWeight leaf_weight_devices;
> >       }
> >       
> >       message Throttling {
> >         message Statistics {
> >           ...
> >         }
> >         
> >         repeated DeviceLimit write_bps_devices;
> >         repeated DeviceLimit write_iops_devices;
> >       }
> >       
> >       message Statistics {
> >         repeated CFQ::Statistics cfq;
> >         repeated CFQ::Statistics cfq_recursive;
> >       }
> >       
> >       optional CFQ cfq;
> >       optional Throttling throttling;
> >     }
> >     
> >     message ResourceStatistics {
> >       optional Blkio::Statistics blkio;
> >     }
> >     ```
> 
> Jason Lai wrote:
>     By introducing `CgroupInfo::ResourceStatistics`, can I guess that you are suggesting that we have a field with this message named `cgroups` within the global `ResourceStatistics` like:
>     
>     ```
>     messge ResourceStatistics {
>       // ....
>       optional CgroupInfo.ResourceStatistics cgroups = 43;
>       // ....
>     }
>     
>     message CgroupInfo {
>       // ....
>     
>       message Blkio {
>         // ....
>         message Statistics {
>           // ....
>         }
>         // ....
>       }
>     
>       message ResourceStatistics {
>         optional Blkio.Statistics blkio = 1;
>       }
>     
>       // ....
>     }
>     ```

I have some further thoughts on the control fields for `CgroupInfo::Blkio`. I would like to see if it makes sense to you if we put the cgroup's `weight` and `leaf_weight` as direct fields and group the per-device configurations in the same way we do for stats?

i.e. Something like:

```
message Blkio
  message DeviceLimits {
    required Device device = 1;
    optional uint32 weight = 2;
    optional uint32 leaf_weight = 3;
    optional uint64 read_bps = 4;
    optional uint64 write_bps = 5;
    optional uint64 read_iops = 6;
    optional uint64 write_iops = 7;
  }

  message Statistics {
    // ....
  }

  optional uint32 weight = 1;
  optional uint32 leaf_weight = 2;
  repeated DeviceLimits device_limits = 3;
}
```


- Jason


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


On Dec. 21, 2016, 8:06 p.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 8:06 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

Posted by Jason Lai <ja...@jasonlai.net>.


> On Dec. 17, 2016, 1:25 a.m., Jie Yu wrote:
> > include/mesos/mesos.proto, lines 2226-2227
> > <https://reviews.apache.org/r/54693/diff/1/?file=1582391#file1582391line2226>
> >
> >     What are these two?

They are `blkio.throttle.io_serviced` and `blkio.throttle.io_service_bytes`. I'll add comments to them.


> On Dec. 17, 2016, 1:25 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp, lines 45-51
> > <https://reviews.apache.org/r/54693/diff/1/?file=1582393#file1582393line45>
> >
> >     Let's do that in the following patch where you actually get those statistics.

Sure. I was initially hoping to add code for implementations, but seems like it will make the scope of this diff much bigger. I'll revert these 2 files in the diff.


> On Dec. 17, 2016, 1:25 a.m., Jie Yu wrote:
> > include/mesos/mesos.proto, line 2214
> > <https://reviews.apache.org/r/54693/diff/1/?file=1582391#file1582391line2214>
> >
> >     I am wondering if the following structure is better:
> >     
> >     ```
> >     message Blkio {
> >       message CFQ {
> >         message Statistics {
> >           optional Device device;
> >           optional uint64 sectors;
> >           optional uint64 time;
> >           repeated Entry io_serviced;
> >           ...
> >         }
> >         
> >         repeated DeviceWeight weight_devices;
> >         repeated DeviceWeight leaf_weight_devices;
> >       }
> >       
> >       message Throttling {
> >         message Statistics {
> >           ...
> >         }
> >         
> >         repeated DeviceLimit write_bps_devices;
> >         repeated DeviceLimit write_iops_devices;
> >       }
> >       
> >       message Statistics {
> >         repeated CFQ::Statistics cfq;
> >         repeated CFQ::Statistics cfq_recursive;
> >       }
> >       
> >       optional CFQ cfq;
> >       optional Throttling throttling;
> >     }
> >     
> >     message ResourceStatistics {
> >       optional Blkio::Statistics blkio;
> >     }
> >     ```

By introducing `CgroupInfo::ResourceStatistics`, can I guess that you are suggesting that we have a field with this message named `cgroups` within the global `ResourceStatistics` like:

```
messge ResourceStatistics {
  // ....
  optional CgroupInfo.ResourceStatistics cgroups = 43;
  // ....
}

message CgroupInfo {
  // ....

  message Blkio {
    // ....
    message Statistics {
      // ....
    }
    // ....
  }

  message ResourceStatistics {
    optional Blkio.Statistics blkio = 1;
  }

  // ....
}
```


> On Dec. 17, 2016, 1:25 a.m., Jie Yu wrote:
> > include/mesos/mesos.proto, line 2199
> > <https://reviews.apache.org/r/54693/diff/1/?file=1582391#file1582391line2199>
> >
> >     Can this be nested inside CgroupInfo::Blkio?

Yes, sir. It is done :)


> On Dec. 17, 2016, 1:25 a.m., Jie Yu wrote:
> > include/mesos/mesos.proto, line 2209
> > <https://reviews.apache.org/r/54693/diff/1/?file=1582391#file1582391line2209>
> >
> >     s/StatEntry/Entry/

Done.


> On Dec. 17, 2016, 1:25 a.m., Jie Yu wrote:
> > include/mesos/mesos.proto, lines 2200-2207
> > <https://reviews.apache.org/r/54693/diff/1/?file=1582391#file1582391line2200>
> >
> >     I'd move this to CgroupInfo::Blkio::Operation

Done. I have instead moved it to `CgroupInfo::Blkio::Statistics::Operation` as it is stats-specific.


- Jason


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


On Dec. 21, 2016, 8:06 p.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 21, 2016, 8:06 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

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

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




include/mesos/mesos.proto (line 899)
<https://reviews.apache.org/r/54693/#comment230364>

    2 lines apart please.



include/mesos/mesos.proto (line 2188)
<https://reviews.apache.org/r/54693/#comment230551>

    I'd introduce CgroupInfo::Blkio::CFQ CgroupInfo::Blkio::Throttling message here. Looks like for the fields below, we should separate them. For instance, `read_bps_device` is only for throttling iosched.



include/mesos/mesos.proto (line 2199)
<https://reviews.apache.org/r/54693/#comment230542>

    Can this be nested inside CgroupInfo::Blkio?



include/mesos/mesos.proto (lines 2200 - 2207)
<https://reviews.apache.org/r/54693/#comment230548>

    I'd move this to CgroupInfo::Blkio::Operation



include/mesos/mesos.proto (line 2209)
<https://reviews.apache.org/r/54693/#comment230557>

    s/StatEntry/Entry/



include/mesos/mesos.proto (line 2214)
<https://reviews.apache.org/r/54693/#comment230555>

    I am wondering if the following structure is better:
    
    ```
    message Blkio {
      message CFQ {
        message Statistics {
          optional Device device;
          optional uint64 sectors;
          optional uint64 time;
          repeated Entry io_serviced;
          ...
        }
        
        repeated DeviceWeight weight_devices;
        repeated DeviceWeight leaf_weight_devices;
      }
      
      message Throttling {
        message Statistics {
          ...
        }
        
        repeated DeviceLimit write_bps_devices;
        repeated DeviceLimit write_iops_devices;
      }
      
      message Statistics {
        repeated CFQ::Statistics cfq;
        repeated CFQ::Statistics cfq_recursive;
      }
      
      optional CFQ cfq;
      optional Throttling throttling;
    }
    
    message ResourceStatistics {
      optional Blkio::Statistics blkio;
    }
    ```



include/mesos/mesos.proto (lines 2226 - 2227)
<https://reviews.apache.org/r/54693/#comment230554>

    What are these two?



src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp (lines 45 - 51)
<https://reviews.apache.org/r/54693/#comment230543>

    Let's do that in the following patch where you actually get those statistics.


- Jie Yu


On Dec. 13, 2016, 5:12 a.m., Jason Lai wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
>     https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Lai
> 
>