You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2015/09/22 02:58:26 UTC
Review Request 38584: Added a few filesystem isolation and
provisioning metrics.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38584/
-----------------------------------------------------------
Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
Bugs: MESOS-3466
https://issues.apache.org/jira/browse/MESOS-3466
Repository: mesos
Description
-------
Added a few filesystem isolation and provisioning metrics.
Diffs
-----
src/slave/containerizer/isolators/filesystem/linux.hpp 041af7b51cc7da933a929d3beb05c419321ddd73
src/slave/containerizer/isolators/filesystem/linux.cpp e821b27d5cba324e434132d884d25a526c11a9c5
src/slave/containerizer/provisioner/backends/bind.cpp d853b49891360fbc2eb46f2e7859fa296fd36beb
src/slave/containerizer/provisioner/provisioner.cpp 213f8a6cbcfa198920dbb83c42e58e8aef6d6946
src/tests/containerizer/filesystem_isolator_tests.cpp e2eb049ae477578caeedaa4251f7ddf9028d3fb5
Diff: https://reviews.apache.org/r/38584/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 38584: Added a few filesystem isolation and
provisioning metrics.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Sept. 21, 2015, 11:09 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioner/backends/bind.cpp, line 194
> > <https://reviews.apache.org/r/38584/diff/1/?file=1079553#file1079553line194>
> >
> > Why only when errno == EBUSY?
Because other errors are not masked by the backend and propagted uptowards and will be reflected in other metrics.
It's no big deal and we'll likely deprecated these some time later.
Currently we need these to see how often we get errors due to race condition between containers cleaning up their mount table and the host destroying these folders.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38584/#review99930
-----------------------------------------------------------
On Sept. 21, 2015, 5:58 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38584/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 5:58 p.m.)
>
>
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
>
>
> Bugs: MESOS-3466
> https://issues.apache.org/jira/browse/MESOS-3466
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a few filesystem isolation and provisioning metrics.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.hpp 041af7b51cc7da933a929d3beb05c419321ddd73
> src/slave/containerizer/isolators/filesystem/linux.cpp e821b27d5cba324e434132d884d25a526c11a9c5
> src/slave/containerizer/provisioner/backends/bind.cpp d853b49891360fbc2eb46f2e7859fa296fd36beb
> src/slave/containerizer/provisioner/provisioner.cpp 213f8a6cbcfa198920dbb83c42e58e8aef6d6946
> src/tests/containerizer/filesystem_isolator_tests.cpp e2eb049ae477578caeedaa4251f7ddf9028d3fb5
>
> Diff: https://reviews.apache.org/r/38584/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 38584: Added a few filesystem isolation and
provisioning metrics.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38584/#review99930
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.hpp (line 138)
<https://reviews.apache.org/r/38584/#comment156945>
I actually like new rootfs more, but open to either.
src/slave/containerizer/isolators/filesystem/linux.cpp (line 860)
<https://reviews.apache.org/r/38584/#comment156946>
Add a blank line above
src/slave/containerizer/provisioner/backends/bind.cpp (line 194)
<https://reviews.apache.org/r/38584/#comment156947>
Why only when errno == EBUSY?
- Timothy Chen
On Sept. 22, 2015, 12:58 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38584/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 12:58 a.m.)
>
>
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
>
>
> Bugs: MESOS-3466
> https://issues.apache.org/jira/browse/MESOS-3466
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a few filesystem isolation and provisioning metrics.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.hpp 041af7b51cc7da933a929d3beb05c419321ddd73
> src/slave/containerizer/isolators/filesystem/linux.cpp e821b27d5cba324e434132d884d25a526c11a9c5
> src/slave/containerizer/provisioner/backends/bind.cpp d853b49891360fbc2eb46f2e7859fa296fd36beb
> src/slave/containerizer/provisioner/provisioner.cpp 213f8a6cbcfa198920dbb83c42e58e8aef6d6946
> src/tests/containerizer/filesystem_isolator_tests.cpp e2eb049ae477578caeedaa4251f7ddf9028d3fb5
>
> Diff: https://reviews.apache.org/r/38584/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 38584: Added a few filesystem isolation and
provisioning metrics.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38584/#review99953
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38584]
All tests passed.
- Mesos ReviewBot
On Sept. 22, 2015, 12:58 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38584/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 12:58 a.m.)
>
>
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
>
>
> Bugs: MESOS-3466
> https://issues.apache.org/jira/browse/MESOS-3466
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a few filesystem isolation and provisioning metrics.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.hpp 041af7b51cc7da933a929d3beb05c419321ddd73
> src/slave/containerizer/isolators/filesystem/linux.cpp e821b27d5cba324e434132d884d25a526c11a9c5
> src/slave/containerizer/provisioner/backends/bind.cpp d853b49891360fbc2eb46f2e7859fa296fd36beb
> src/slave/containerizer/provisioner/provisioner.cpp 213f8a6cbcfa198920dbb83c42e58e8aef6d6946
> src/tests/containerizer/filesystem_isolator_tests.cpp e2eb049ae477578caeedaa4251f7ddf9028d3fb5
>
> Diff: https://reviews.apache.org/r/38584/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 38584: Added a few filesystem isolation and
provisioning metrics.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Sept. 21, 2015, 9:56 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.hpp, line 138
> > <https://reviews.apache.org/r/38584/diff/1/?file=1079551#file1079551line138>
> >
> > `containers_changing_rootfs`?
Keeping `containers_new_rootfs` because I think it should be a shorthand for `containers_that_are_using_a_changed_rootfs` or `containers_that_are_using_a_new_rootfs`. And indeed a *new* rootfs is created for this container.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38584/#review99917
-----------------------------------------------------------
On Sept. 21, 2015, 5:58 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38584/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 5:58 p.m.)
>
>
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
>
>
> Bugs: MESOS-3466
> https://issues.apache.org/jira/browse/MESOS-3466
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a few filesystem isolation and provisioning metrics.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.hpp 041af7b51cc7da933a929d3beb05c419321ddd73
> src/slave/containerizer/isolators/filesystem/linux.cpp e821b27d5cba324e434132d884d25a526c11a9c5
> src/slave/containerizer/provisioner/backends/bind.cpp d853b49891360fbc2eb46f2e7859fa296fd36beb
> src/slave/containerizer/provisioner/provisioner.cpp 213f8a6cbcfa198920dbb83c42e58e8aef6d6946
> src/tests/containerizer/filesystem_isolator_tests.cpp e2eb049ae477578caeedaa4251f7ddf9028d3fb5
>
> Diff: https://reviews.apache.org/r/38584/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 38584: Added a few filesystem isolation and
provisioning metrics.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38584/#review99917
-----------------------------------------------------------
Ship it!
src/slave/containerizer/isolators/filesystem/linux.hpp (line 135)
<https://reviews.apache.org/r/38584/#comment156923>
const PID<T>& isolator?
src/slave/containerizer/isolators/filesystem/linux.hpp (line 138)
<https://reviews.apache.org/r/38584/#comment156924>
`containers_changing_rootfs`?
src/slave/containerizer/provisioner/provisioner.cpp (line 416)
<https://reviews.apache.org/r/38584/#comment156926>
Add a blank line.
src/slave/containerizer/provisioner/provisioner.cpp (line 420)
<https://reviews.apache.org/r/38584/#comment156927>
Add a blank line above
- Jie Yu
On Sept. 22, 2015, 12:58 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38584/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 12:58 a.m.)
>
>
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
>
>
> Bugs: MESOS-3466
> https://issues.apache.org/jira/browse/MESOS-3466
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a few filesystem isolation and provisioning metrics.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.hpp 041af7b51cc7da933a929d3beb05c419321ddd73
> src/slave/containerizer/isolators/filesystem/linux.cpp e821b27d5cba324e434132d884d25a526c11a9c5
> src/slave/containerizer/provisioner/backends/bind.cpp d853b49891360fbc2eb46f2e7859fa296fd36beb
> src/slave/containerizer/provisioner/provisioner.cpp 213f8a6cbcfa198920dbb83c42e58e8aef6d6946
> src/tests/containerizer/filesystem_isolator_tests.cpp e2eb049ae477578caeedaa4251f7ddf9028d3fb5
>
> Diff: https://reviews.apache.org/r/38584/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 38584: Added a few filesystem isolation and
provisioning metrics.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Sept. 22, 2015, 9:19 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/isolators/filesystem/linux.hpp, line 141
> > <https://reviews.apache.org/r/38584/diff/1/?file=1079551#file1079551line141>
> >
> > is this proper accordng to style guide (Since this is a public method)?
It's under the private section and the convention used for metrics gauges. e.g. https://github.com/apache/mesos/blob/master/src/master/master.hpp#L1237
> On Sept. 22, 2015, 9:19 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 839
> > <https://reviews.apache.org/r/38584/diff/1/?file=1079552#file1079552line839>
> >
> > const?
You mean "const PID<LinuxFilesystemIsolatorProcess>& isolator"? It's fixed already. Thanks!
> On Sept. 22, 2015, 9:19 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner/provisioner.cpp, line 435
> > <https://reviews.apache.org/r/38584/diff/1/?file=1079554#file1079554line435>
> >
> > const?
What being const?
> On Sept. 22, 2015, 9:19 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner/provisioner.cpp, line 419
> > <https://reviews.apache.org/r/38584/diff/1/?file=1079554#file1079554line419>
> >
> > const?
Yeah I guess it won't hurt to use const but the entire usage and lifecyle of this variable is within 5 lines I didn't feel the need. For more complex logic, for sure. Thanks!
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38584/#review100005
-----------------------------------------------------------
On Sept. 21, 2015, 5:58 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38584/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 5:58 p.m.)
>
>
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
>
>
> Bugs: MESOS-3466
> https://issues.apache.org/jira/browse/MESOS-3466
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a few filesystem isolation and provisioning metrics.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.hpp 041af7b51cc7da933a929d3beb05c419321ddd73
> src/slave/containerizer/isolators/filesystem/linux.cpp e821b27d5cba324e434132d884d25a526c11a9c5
> src/slave/containerizer/provisioner/backends/bind.cpp d853b49891360fbc2eb46f2e7859fa296fd36beb
> src/slave/containerizer/provisioner/provisioner.cpp 213f8a6cbcfa198920dbb83c42e58e8aef6d6946
> src/tests/containerizer/filesystem_isolator_tests.cpp e2eb049ae477578caeedaa4251f7ddf9028d3fb5
>
> Diff: https://reviews.apache.org/r/38584/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 38584: Added a few filesystem isolation and
provisioning metrics.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38584/#review100005
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.hpp (line 141)
<https://reviews.apache.org/r/38584/#comment157077>
is this proper accordng to style guide (Since this is a public method)?
src/slave/containerizer/isolators/filesystem/linux.cpp (line 839)
<https://reviews.apache.org/r/38584/#comment157080>
const?
src/slave/containerizer/provisioner/provisioner.cpp (line 414)
<https://reviews.apache.org/r/38584/#comment157082>
const?
src/slave/containerizer/provisioner/provisioner.cpp (line 429)
<https://reviews.apache.org/r/38584/#comment157083>
const?
- Jojy Varghese
On Sept. 22, 2015, 12:58 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38584/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2015, 12:58 a.m.)
>
>
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
>
>
> Bugs: MESOS-3466
> https://issues.apache.org/jira/browse/MESOS-3466
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a few filesystem isolation and provisioning metrics.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.hpp 041af7b51cc7da933a929d3beb05c419321ddd73
> src/slave/containerizer/isolators/filesystem/linux.cpp e821b27d5cba324e434132d884d25a526c11a9c5
> src/slave/containerizer/provisioner/backends/bind.cpp d853b49891360fbc2eb46f2e7859fa296fd36beb
> src/slave/containerizer/provisioner/provisioner.cpp 213f8a6cbcfa198920dbb83c42e58e8aef6d6946
> src/tests/containerizer/filesystem_isolator_tests.cpp e2eb049ae477578caeedaa4251f7ddf9028d3fb5
>
> Diff: https://reviews.apache.org/r/38584/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>