You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2016/12/06 21:21:09 UTC
Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Jiang Yan Xu.
Bugs: MESOS-6732
https://issues.apache.org/jira/browse/MESOS-6732
Repository: mesos
Description
-------
The XFS disk isolator checks that the filesystem is XFS, but doesn't
check whether project quotas are actually enabled. This means that
an invalid configuration will start but will always fail when tasks
are launched.
Add a check to test whether project quotas are enabled on the work
directory and fail hard if they are not.
Diffs
-----
configure.ac 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4
src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
Diff: https://reviews.apache.org/r/54449/diff/
Testing
-------
Make check on Fedora 25. Manual test on F25 with mesos-execute.
Thanks,
James Peach
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
> On Dec. 10, 2016, 12:37 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp, line 407
> > <https://reviews.apache.org/r/54449/diff/1/?file=1578143#file1578143line407>
> >
> > Remove redundant space:
> >
> > ```
> > struct fs_quota_statv statv = {FS_QSTATV_VERSION1, 0};
> > ```
> >
> > What's the `0` here? Do we need it? If we do, comment on it?
It's zero-initializing the remaining fields of the struct.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158517
-----------------------------------------------------------
On Dec. 6, 2016, 9:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 9:21 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
> On Dec. 10, 2016, 12:37 a.m., Jiang Yan Xu wrote:
> > Can we add a simple test for `isQuotaEnabled()` in ROOT_XFS_QuotaTest?
>
> James Peach wrote:
> I didn't add a new test because I reasoned that this is implicitly tested already, like `pathIsXfs`.
>
> Jiang Yan Xu wrote:
> Implicitly testing it is fine and I was trying to find where both of them are implicitly tested but all the tests only have the "successful case": the work_dir is indeed on the XFS mountpoint with quota enabled. There are no tests where it's not?
To really test the negative case you'd have to construct a non-XFS filesystem and also an XFS filesystem where quotas are disabled. We could probably do that similarly what the tests do now, but it didn't seem worth it :)
> On Dec. 10, 2016, 12:37 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp, line 407
> > <https://reviews.apache.org/r/54449/diff/1/?file=1578143#file1578143line407>
> >
> > Remove redundant space:
> >
> > ```
> > struct fs_quota_statv statv = {FS_QSTATV_VERSION1, 0};
> > ```
> >
> > What's the `0` here? Do we need it? If we do, comment on it?
>
> James Peach wrote:
> It's zero-initializing the remaining fields of the struct.
>
> Jiang Yan Xu wrote:
> Is this a well-defined pattern?
>
> I thought zero-initialization doesn't requires the number `0`?
> http://en.cppreference.com/w/cpp/language/zero_initialization
> http://en.cppreference.com/w/cpp/language/aggregate_initialization
>
> e.g.,
>
> `struct fs_quota_statv statv = {FS_QSTATV_VERSION1};` is the same as `struct fs_quota_statv statv = {FS_QSTATV_VERSION1, 0};`
>
> `fs_disk_quota_t quota = {0};` is the same as `fs_disk_quota_t quota = {};`
>
> Does it look right?
>
> I mean, in term of style, the less "magic numbers" the better?
In terms of style, zero-initializing structs with `{0}` has been common for decades. I belive that the `{}` style was introduced in C++11, which is why it feels less recognizeable to me.
http://en.cppreference.com/w/c/language/struct_initialization
I believe the relevant language is:
```
All members that are not initialized explicitly are initialized implicitly the same way as objects that have static storage duration.
```
In which case the trailing 0 is not needed, so you are right that `{FS_QSTATV_VERSION1}` is sufficient here.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158517
-----------------------------------------------------------
On Dec. 10, 2016, 1:14 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 1:14 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
> On Dec. 10, 2016, 12:37 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp, lines 110-120
> > <https://reviews.apache.org/r/54449/diff/1/?file=1578141#file1578141line110>
> >
> > We check
> >
> > ```
> > if (!xfs::pathIsXfs(flags.work_dir)) {
> > return Error("'" + flags.work_dir + "' is not an XFS filesystem");
> > }
> > ```
> >
> > in `XfsDiskIsolatorProcess::create`. Seems like the similar check should go there if we want to "fail fast".
This check is in `XfsDiskIsolatorProcess::create`. The diff viewer is misleading ;)
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158517
-----------------------------------------------------------
On Dec. 6, 2016, 9:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 9:21 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
> On Dec. 10, 2016, 12:37 a.m., Jiang Yan Xu wrote:
> > Can we add a simple test for `isQuotaEnabled()` in ROOT_XFS_QuotaTest?
I didn't add a new test because I reasoned that this is implicitly tested already, like `pathIsXfs`.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158517
-----------------------------------------------------------
On Dec. 6, 2016, 9:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 9:21 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Dec. 9, 2016, 4:37 p.m., Jiang Yan Xu wrote:
> > Can we add a simple test for `isQuotaEnabled()` in ROOT_XFS_QuotaTest?
>
> James Peach wrote:
> I didn't add a new test because I reasoned that this is implicitly tested already, like `pathIsXfs`.
Implicitly testing it is fine and I was trying to find where both of them are implicitly tested but all the tests only have the "successful case": the work_dir is indeed on the XFS mountpoint with quota enabled. There are no tests where it's not?
> On Dec. 9, 2016, 4:37 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp, line 407
> > <https://reviews.apache.org/r/54449/diff/1/?file=1578143#file1578143line407>
> >
> > Remove redundant space:
> >
> > ```
> > struct fs_quota_statv statv = {FS_QSTATV_VERSION1, 0};
> > ```
> >
> > What's the `0` here? Do we need it? If we do, comment on it?
>
> James Peach wrote:
> It's zero-initializing the remaining fields of the struct.
Is this a well-defined pattern?
I thought zero-initialization doesn't requires the number `0`?
http://en.cppreference.com/w/cpp/language/zero_initialization
http://en.cppreference.com/w/cpp/language/aggregate_initialization
e.g.,
`struct fs_quota_statv statv = {FS_QSTATV_VERSION1};` is the same as `struct fs_quota_statv statv = {FS_QSTATV_VERSION1, 0};`
`fs_disk_quota_t quota = {0};` is the same as `fs_disk_quota_t quota = {};`
Does it look right?
I mean, in term of style, the less "magic numbers" the better?
> On Dec. 9, 2016, 4:37 p.m., Jiang Yan Xu wrote:
> > configure.ac, line 1768
> > <https://reviews.apache.org/r/54449/diff/1/?file=1578140#file1578140line1768>
> >
> > Is this change for `Q_XGETQSTATV` and `FS_QSTATV_VERSION1`?
> >
> > At least explain this (and the change of variable names XFS_PROJ_QUOTA -> FS_PROJ_QUOTA in the review summary?
>
> James Peach wrote:
> Updated summary. The short story is that `linux/dqblk_xfs.h` is a superset and the two headers are mutually incompatible.
You updated the comment summary via git? It's not synced over to RB. Could you paste it here? Yeah the short story is good enough.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158517
-----------------------------------------------------------
On Dec. 9, 2016, 5:14 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 5:14 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
> On Dec. 10, 2016, 12:37 a.m., Jiang Yan Xu wrote:
> > configure.ac, line 1768
> > <https://reviews.apache.org/r/54449/diff/1/?file=1578140#file1578140line1768>
> >
> > Is this change for `Q_XGETQSTATV` and `FS_QSTATV_VERSION1`?
> >
> > At least explain this (and the change of variable names XFS_PROJ_QUOTA -> FS_PROJ_QUOTA in the review summary?
Updated summary. The short story is that `linux/dqblk_xfs.h` is a superset and the two headers are mutually incompatible.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158517
-----------------------------------------------------------
On Dec. 6, 2016, 9:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 9:21 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158517
-----------------------------------------------------------
Can we add a simple test for `isQuotaEnabled()` in ROOT_XFS_QuotaTest?
configure.ac (line 1768)
<https://reviews.apache.org/r/54449/#comment229535>
Is this change for `Q_XGETQSTATV` and `FS_QSTATV_VERSION1`?
At least explain this (and the change of variable names XFS_PROJ_QUOTA -> FS_PROJ_QUOTA in the review summary?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 110 - 120)
<https://reviews.apache.org/r/54449/#comment229537>
We check
```
if (!xfs::pathIsXfs(flags.work_dir)) {
return Error("'" + flags.work_dir + "' is not an XFS filesystem");
}
```
in `XfsDiskIsolatorProcess::create`. Seems like the similar check should go there if we want to "fail fast".
src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 112 - 113)
<https://reviews.apache.org/r/54449/#comment229538>
Indent like
```
return Error(
"Failed to get quota status for '" +
flags.work_dir + "': " + enabled.error());
```
or
```
return Error("Failed to get quota status for '" +
flags.work_dir + "': " + enabled.error());
```
src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 52)
<https://reviews.apache.org/r/54449/#comment229536>
Would `isQuotaEnabled` be a better name?
It's obvious that the method above has set up the example but I guess `isPathXfs` would have been a better name too?
src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 406)
<https://reviews.apache.org/r/54449/#comment229539>
Remove redundant space:
```
struct fs_quota_statv statv = {FS_QSTATV_VERSION1, 0};
```
What's the `0` here? Do we need it? If we do, comment on it?
src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 420)
<https://reviews.apache.org/r/54449/#comment229540>
We need a brief comment about "both accounting mode and enforcement mode count as quota enabled" but it probalby can go into the method declaration comment.
- Jiang Yan Xu
On Dec. 6, 2016, 1:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 1:21 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158970
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54449]
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. 13, 2016, 12:15 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 12:15 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
> On March 9, 2017, 11:17 p.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp
> > Lines 229 (patched)
> > <https://reviews.apache.org/r/54449/diff/4/?file=1656794#file1656794line229>
> >
> > IMO `ROOT_XFS_NoQuota` reads better: it's literally the mount option and it's pretty clear. :)
It's done this way so gtest globs glob better, but sure :)
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review168207
-----------------------------------------------------------
On March 6, 2017, 7:10 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 7:10 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
> src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
>
>
> Diff: https://reviews.apache.org/r/54449/diff/4/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review168207
-----------------------------------------------------------
Fix it, then Ship it!
The test refactor looks great!
src/tests/containerizer/xfs_quota_tests.cpp
Line 82 (original), 82 (patched)
<https://reviews.apache.org/r/54449/#comment240402>
How about ROOT_XFS_TestBase here and use the name ROOT_XFS_QuotaTest for the "standard test" below?
Although not required by gtest, it's a convention to end test fixtures with Test or TestBase (the latter when it is purely a base class with no tests of itself).
src/tests/containerizer/xfs_quota_tests.cpp
Lines 85 (patched)
<https://reviews.apache.org/r/54449/#comment240403>
s/quotaOpt/quotaOptions/ (full spelling) (or s/quotaOpt/xfsOptions/ which is more generic since this class is just a base class)
src/tests/containerizer/xfs_quota_tests.cpp
Lines 227 (patched)
<https://reviews.apache.org/r/54449/#comment240785>
s/a/an/ here and below?
src/tests/containerizer/xfs_quota_tests.cpp
Lines 229 (patched)
<https://reviews.apache.org/r/54449/#comment240399>
IMO `ROOT_XFS_NoQuota` reads better: it's literally the mount option and it's pretty clear. :)
src/tests/containerizer/xfs_quota_tests.cpp
Lines 237 (patched)
<https://reviews.apache.org/r/54449/#comment240394>
s/oeoject/project/?
src/tests/containerizer/xfs_quota_tests.cpp
Lines 238 (patched)
<https://reviews.apache.org/r/54449/#comment240396>
The comment says `ROOT_XFS_QuotaNoProj` but here it's `ROOT_XFS_QuotaOther`.
IMO `ROOT_XFS_NoProjectQuota` is more straighforward than "QuotaOther".
src/tests/containerizer/xfs_quota_tests.cpp
Lines 916 (patched)
<https://reviews.apache.org/r/54449/#comment240781>
Testing `xfs::isQuotaEnabled` is approximate, but the most direct test would be on `XfsDiskIsolatorProcess::create` or even `MesosContainerizer::create`?
- Jiang Yan Xu
On March 6, 2017, 11:10 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 11:10 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
> src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
>
>
> Diff: https://reviews.apache.org/r/54449/diff/4/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review168055
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54449]
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 March 6, 2017, 11:10 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 11:10 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
> src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
>
>
> Diff: https://reviews.apache.org/r/54449/diff/4/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review168649
-----------------------------------------------------------
Ship it!
LGTM. I'll fix the following when committing.
src/tests/containerizer/xfs_quota_tests.cpp
Line 82 (original), 84 (patched)
<https://reviews.apache.org/r/54449/#comment240888>
I suggested to use `TestBase` which confirms to our naming convention better and the issue was marked resolved so I am assuming this is a typo?
s/ROOT_XFS_QuotaBase/ROOT_XFS_TestBase/
src/tests/containerizer/xfs_quota_tests.cpp
Lines 87 (patched)
<https://reviews.apache.org/r/54449/#comment240890>
s/xfsOptions/_xfsOptions/ per convention.
- Jiang Yan Xu
On March 10, 2017, 7:51 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 7:51 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
> src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
>
>
> Diff: https://reviews.apache.org/r/54449/diff/6/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/
-----------------------------------------------------------
(Updated March 10, 2017, 3:51 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Rebased.
Bugs: MESOS-6732
https://issues.apache.org/jira/browse/MESOS-6732
Repository: mesos
Description
-------
The XFS disk isolator checks that the filesystem is XFS, but doesn't
check whether project quotas are actually enabled. This means that
an invalid configuration will start but will always fail when tasks
are launched.
Add a check to test whether project quotas are enabled on the work
directory and fail hard if they are not.
Diffs (updated)
-----
configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671
src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
Diff: https://reviews.apache.org/r/54449/diff/6/
Changes: https://reviews.apache.org/r/54449/diff/5-6/
Testing
-------
Make check on Fedora 25. Manual test on F25 with mesos-execute.
Thanks,
James Peach
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
> On March 10, 2017, 8:10 a.m., Jiang Yan Xu wrote:
> > Rebase necessary?
Probably :)
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review168588
-----------------------------------------------------------
On March 10, 2017, 1:16 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 1:16 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/authenticator.hpp 00660f42cd4b707d955745bbfea5ffec73f690d6
> 3rdparty/libprocess/include/process/http.hpp 9b89d31d60f7977f30f623a7315e67f97f5940e7
> 3rdparty/libprocess/include/process/logging.hpp f9997677d69d54f5723d4fc0a495008d3ce11cc5
> 3rdparty/libprocess/include/process/metrics/metrics.hpp 9c32a88d851c884a5025edb6ea1e27939b484546
> 3rdparty/libprocess/include/process/process.hpp 401510c8f7b7d546d5364756dcab3245207ca5ab
> 3rdparty/libprocess/include/process/profiler.hpp 2991dd2033d68802a813de91babb47679c807aa0
> 3rdparty/libprocess/src/authenticator.cpp 8d1756f0a2e124440bc7c69a1b25576f607c2d03
> 3rdparty/libprocess/src/authenticator_manager.cpp 5cbed53e7085f227d90679e1b56ad803d9b74a47
> 3rdparty/libprocess/src/logging.cpp cfaf1793dfcb404448319c1eddd7a971b3374296
> 3rdparty/libprocess/src/metrics/metrics.cpp 7184aa4d0294c20466646c9aa61d90973eca22e1
> 3rdparty/libprocess/src/process.cpp 9eb7fe3d20aa9416db5162fa275fcf116f5d6477
> 3rdparty/libprocess/src/profiler.cpp a29e8d1e18136ec2b88cce19f03ee3073f8d6340
> 3rdparty/libprocess/src/tests/http_tests.cpp a0e23c2300f9f6b9d1143ee1eb115bbf24adf92e
> configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671
> include/mesos/agent/agent.proto 9149724159485ea2265e1494c1ce7ef989dad20a
> include/mesos/authorizer/authorizer.proto fdc4817ce74c45d792fc47f064f7909a83b1657d
> include/mesos/v1/agent/agent.proto 34210c30ca58f50b14ff3e5a01c54003c9705121
> src/common/http.hpp a3cfc5d8f0b2e453d5f6c3e485e92dbd643737a3
> src/common/http.cpp 0848f70ad4fa9e67c74c9fbdd882d7ab5ed6eabf
> src/files/files.hpp 1e123fac21ab98c508ca96112e080bd7d48389cb
> src/files/files.cpp f066146b7cbff35c452717d179b79039bc603cc8
> src/master/allocator/mesos/hierarchical.hpp 646f66e67d9c6b8c61fc6e6558a1db976a44c126
> src/master/allocator/mesos/hierarchical.cpp 0059ccead90f32491591990c791e7fa905a864b7
> src/master/http.cpp de2cc03e6c7a3b75224113f56ce268937ead2929
> src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413
> src/master/master.cpp dd1e4cd7c2da32949ffc69ce4f7b169b153fc736
> src/master/quota_handler.cpp ce1f0644a56e85a99d8c3742d00940a1bfae3be3
> src/master/registrar.cpp 0029cc77628b5bb2a7b1ff551fb42b3eaf7b4fb1
> src/master/validation.hpp d96287de73ddb30ae2ed841c1b910b0ac6cfa74e
> src/master/validation.cpp 3f70875484bbd856ac79a7d6070ac313d69782fa
> src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
> src/slave/http.cpp bc8209cb81194ebc8b604c9ba0d4a9e176cff2f6
> src/slave/slave.hpp 978edd6309dfbbde1058f9c44d5fac7083ff95fb
> src/slave/slave.cpp 4319f841fbdc7ad39eb60eb52ae2a764b133cfbd
> src/slave/validation.cpp 3dbd0fa6ec27b38f40c7922c256859b4d29059ac
> src/tests/api_tests.cpp 52f58a4d6b1ea75744de1c3d2f0f064d9299fe1d
> src/tests/containerizer/runtime_isolator_tests.cpp fbca31a4da1c83574cce7414fe5e03b1f86591cb
> src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
> src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856
> src/tests/files_tests.cpp d492adf71ecb22c433f0eba4d974e99f610b5dd3
> src/tests/http_authentication_tests.cpp d5fabf0058755502f19eb6385bd99a0d45419508
> src/tests/master_validation_tests.cpp 11dfbc612ee2c9a39a17dc1ca9bde04eeb65b550
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/54449/diff/5/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review168588
-----------------------------------------------------------
Rebase necessary?
- Jiang Yan Xu
On March 9, 2017, 5:16 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated March 9, 2017, 5:16 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/authenticator.hpp 00660f42cd4b707d955745bbfea5ffec73f690d6
> 3rdparty/libprocess/include/process/http.hpp 9b89d31d60f7977f30f623a7315e67f97f5940e7
> 3rdparty/libprocess/include/process/logging.hpp f9997677d69d54f5723d4fc0a495008d3ce11cc5
> 3rdparty/libprocess/include/process/metrics/metrics.hpp 9c32a88d851c884a5025edb6ea1e27939b484546
> 3rdparty/libprocess/include/process/process.hpp 401510c8f7b7d546d5364756dcab3245207ca5ab
> 3rdparty/libprocess/include/process/profiler.hpp 2991dd2033d68802a813de91babb47679c807aa0
> 3rdparty/libprocess/src/authenticator.cpp 8d1756f0a2e124440bc7c69a1b25576f607c2d03
> 3rdparty/libprocess/src/authenticator_manager.cpp 5cbed53e7085f227d90679e1b56ad803d9b74a47
> 3rdparty/libprocess/src/logging.cpp cfaf1793dfcb404448319c1eddd7a971b3374296
> 3rdparty/libprocess/src/metrics/metrics.cpp 7184aa4d0294c20466646c9aa61d90973eca22e1
> 3rdparty/libprocess/src/process.cpp 9eb7fe3d20aa9416db5162fa275fcf116f5d6477
> 3rdparty/libprocess/src/profiler.cpp a29e8d1e18136ec2b88cce19f03ee3073f8d6340
> 3rdparty/libprocess/src/tests/http_tests.cpp a0e23c2300f9f6b9d1143ee1eb115bbf24adf92e
> configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671
> include/mesos/agent/agent.proto 9149724159485ea2265e1494c1ce7ef989dad20a
> include/mesos/authorizer/authorizer.proto fdc4817ce74c45d792fc47f064f7909a83b1657d
> include/mesos/v1/agent/agent.proto 34210c30ca58f50b14ff3e5a01c54003c9705121
> src/common/http.hpp a3cfc5d8f0b2e453d5f6c3e485e92dbd643737a3
> src/common/http.cpp 0848f70ad4fa9e67c74c9fbdd882d7ab5ed6eabf
> src/files/files.hpp 1e123fac21ab98c508ca96112e080bd7d48389cb
> src/files/files.cpp f066146b7cbff35c452717d179b79039bc603cc8
> src/master/allocator/mesos/hierarchical.hpp 646f66e67d9c6b8c61fc6e6558a1db976a44c126
> src/master/allocator/mesos/hierarchical.cpp 0059ccead90f32491591990c791e7fa905a864b7
> src/master/http.cpp de2cc03e6c7a3b75224113f56ce268937ead2929
> src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413
> src/master/master.cpp dd1e4cd7c2da32949ffc69ce4f7b169b153fc736
> src/master/quota_handler.cpp ce1f0644a56e85a99d8c3742d00940a1bfae3be3
> src/master/registrar.cpp 0029cc77628b5bb2a7b1ff551fb42b3eaf7b4fb1
> src/master/validation.hpp d96287de73ddb30ae2ed841c1b910b0ac6cfa74e
> src/master/validation.cpp 3f70875484bbd856ac79a7d6070ac313d69782fa
> src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
> src/slave/http.cpp bc8209cb81194ebc8b604c9ba0d4a9e176cff2f6
> src/slave/slave.hpp 978edd6309dfbbde1058f9c44d5fac7083ff95fb
> src/slave/slave.cpp 4319f841fbdc7ad39eb60eb52ae2a764b133cfbd
> src/slave/validation.cpp 3dbd0fa6ec27b38f40c7922c256859b4d29059ac
> src/tests/api_tests.cpp 52f58a4d6b1ea75744de1c3d2f0f064d9299fe1d
> src/tests/containerizer/runtime_isolator_tests.cpp fbca31a4da1c83574cce7414fe5e03b1f86591cb
> src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
> src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856
> src/tests/files_tests.cpp d492adf71ecb22c433f0eba4d974e99f610b5dd3
> src/tests/http_authentication_tests.cpp d5fabf0058755502f19eb6385bd99a0d45419508
> src/tests/master_validation_tests.cpp 11dfbc612ee2c9a39a17dc1ca9bde04eeb65b550
> src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
>
>
> Diff: https://reviews.apache.org/r/54449/diff/5/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/
-----------------------------------------------------------
(Updated March 10, 2017, 1:16 a.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Addressed review feedback.
Bugs: MESOS-6732
https://issues.apache.org/jira/browse/MESOS-6732
Repository: mesos
Description
-------
The XFS disk isolator checks that the filesystem is XFS, but doesn't
check whether project quotas are actually enabled. This means that
an invalid configuration will start but will always fail when tasks
are launched.
Add a check to test whether project quotas are enabled on the work
directory and fail hard if they are not.
Diffs (updated)
-----
3rdparty/libprocess/include/process/authenticator.hpp 00660f42cd4b707d955745bbfea5ffec73f690d6
3rdparty/libprocess/include/process/http.hpp 9b89d31d60f7977f30f623a7315e67f97f5940e7
3rdparty/libprocess/include/process/logging.hpp f9997677d69d54f5723d4fc0a495008d3ce11cc5
3rdparty/libprocess/include/process/metrics/metrics.hpp 9c32a88d851c884a5025edb6ea1e27939b484546
3rdparty/libprocess/include/process/process.hpp 401510c8f7b7d546d5364756dcab3245207ca5ab
3rdparty/libprocess/include/process/profiler.hpp 2991dd2033d68802a813de91babb47679c807aa0
3rdparty/libprocess/src/authenticator.cpp 8d1756f0a2e124440bc7c69a1b25576f607c2d03
3rdparty/libprocess/src/authenticator_manager.cpp 5cbed53e7085f227d90679e1b56ad803d9b74a47
3rdparty/libprocess/src/logging.cpp cfaf1793dfcb404448319c1eddd7a971b3374296
3rdparty/libprocess/src/metrics/metrics.cpp 7184aa4d0294c20466646c9aa61d90973eca22e1
3rdparty/libprocess/src/process.cpp 9eb7fe3d20aa9416db5162fa275fcf116f5d6477
3rdparty/libprocess/src/profiler.cpp a29e8d1e18136ec2b88cce19f03ee3073f8d6340
3rdparty/libprocess/src/tests/http_tests.cpp a0e23c2300f9f6b9d1143ee1eb115bbf24adf92e
configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671
include/mesos/agent/agent.proto 9149724159485ea2265e1494c1ce7ef989dad20a
include/mesos/authorizer/authorizer.proto fdc4817ce74c45d792fc47f064f7909a83b1657d
include/mesos/v1/agent/agent.proto 34210c30ca58f50b14ff3e5a01c54003c9705121
src/common/http.hpp a3cfc5d8f0b2e453d5f6c3e485e92dbd643737a3
src/common/http.cpp 0848f70ad4fa9e67c74c9fbdd882d7ab5ed6eabf
src/files/files.hpp 1e123fac21ab98c508ca96112e080bd7d48389cb
src/files/files.cpp f066146b7cbff35c452717d179b79039bc603cc8
src/master/allocator/mesos/hierarchical.hpp 646f66e67d9c6b8c61fc6e6558a1db976a44c126
src/master/allocator/mesos/hierarchical.cpp 0059ccead90f32491591990c791e7fa905a864b7
src/master/http.cpp de2cc03e6c7a3b75224113f56ce268937ead2929
src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413
src/master/master.cpp dd1e4cd7c2da32949ffc69ce4f7b169b153fc736
src/master/quota_handler.cpp ce1f0644a56e85a99d8c3742d00940a1bfae3be3
src/master/registrar.cpp 0029cc77628b5bb2a7b1ff551fb42b3eaf7b4fb1
src/master/validation.hpp d96287de73ddb30ae2ed841c1b910b0ac6cfa74e
src/master/validation.cpp 3f70875484bbd856ac79a7d6070ac313d69782fa
src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c
src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
src/slave/http.cpp bc8209cb81194ebc8b604c9ba0d4a9e176cff2f6
src/slave/slave.hpp 978edd6309dfbbde1058f9c44d5fac7083ff95fb
src/slave/slave.cpp 4319f841fbdc7ad39eb60eb52ae2a764b133cfbd
src/slave/validation.cpp 3dbd0fa6ec27b38f40c7922c256859b4d29059ac
src/tests/api_tests.cpp 52f58a4d6b1ea75744de1c3d2f0f064d9299fe1d
src/tests/containerizer/runtime_isolator_tests.cpp fbca31a4da1c83574cce7414fe5e03b1f86591cb
src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856
src/tests/files_tests.cpp d492adf71ecb22c433f0eba4d974e99f610b5dd3
src/tests/http_authentication_tests.cpp d5fabf0058755502f19eb6385bd99a0d45419508
src/tests/master_validation_tests.cpp 11dfbc612ee2c9a39a17dc1ca9bde04eeb65b550
src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9
Diff: https://reviews.apache.org/r/54449/diff/5/
Changes: https://reviews.apache.org/r/54449/diff/4-5/
Testing
-------
Make check on Fedora 25. Manual test on F25 with mesos-execute.
Thanks,
James Peach
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/
-----------------------------------------------------------
(Updated March 6, 2017, 7:10 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Rebase and address review feedback.
Bugs: MESOS-6732
https://issues.apache.org/jira/browse/MESOS-6732
Repository: mesos
Description
-------
The XFS disk isolator checks that the filesystem is XFS, but doesn't
check whether project quotas are actually enabled. This means that
an invalid configuration will start but will always fail when tasks
are launched.
Add a check to test whether project quotas are enabled on the work
directory and fail hard if they are not.
Diffs (updated)
-----
configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671
src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
src/tests/containerizer/xfs_quota_tests.cpp 0fbaddd68af55c51c106962377be20afa599fb97
Diff: https://reviews.apache.org/r/54449/diff/4/
Changes: https://reviews.apache.org/r/54449/diff/3-4/
Testing
-------
Make check on Fedora 25. Manual test on F25 with mesos-execute.
Thanks,
James Peach
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review167666
-----------------------------------------------------------
Fix it, then Ship it!
Sorry this was mostly done but got overlooked due to holidays...
We have discussed the unit test before and it's probably fine if we punt on that (I still think a negative test case is useful) but could you update the "Testing Done" section with a manual test negative case result? The negative case is the whole point of this patch after all. :)
src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 408-412 (patched)
<https://reviews.apache.org/r/54449/#comment239583>
This is very thorough, thanks. :)
I originally just meant that we could comment on what each `0` means on their own line, much like (this)[https://github.com/apache/mesos/blob/cc15db4b10af343d7aefc838bb626f4579289375/src/tests/containerizer/xfs_quota_tests.cpp#L128] and perhaps copy some snippets from the external doc for the sake of "explaining obscure APIs/terms".
Would it make sense to s/we are getting/it is for/? The difference to me is whether the API has to be used this way or we are using the API in this particular way. Sounds like the former is true?
src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 412 (patched)
<https://reviews.apache.org/r/54449/#comment239584>
s/identity/identity (e.g., projectId)/?
In this file we have been using `projectId` throughout and I as a reader became more familiar with that term than `identity`.
- Jiang Yan Xu
On March 1, 2017, 10:50 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated March 1, 2017, 10:50 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
>
> Diff: https://reviews.apache.org/r/54449/diff/3/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/
-----------------------------------------------------------
(Updated March 1, 2017, 6:50 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Bugs: MESOS-6732
https://issues.apache.org/jira/browse/MESOS-6732
Repository: mesos
Description
-------
The XFS disk isolator checks that the filesystem is XFS, but doesn't
check whether project quotas are actually enabled. This means that
an invalid configuration will start but will always fail when tasks
are launched.
Add a check to test whether project quotas are enabled on the work
directory and fail hard if they are not.
Diffs
-----
configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d
src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
Diff: https://reviews.apache.org/r/54449/diff/3/
Testing
-------
Make check on Fedora 25. Manual test on F25 with mesos-execute.
Thanks,
James Peach
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review167522
-----------------------------------------------------------
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. 13, 2016, 12:15 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 12:15 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
>
> Diff: https://reviews.apache.org/r/54449/diff/3/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/
-----------------------------------------------------------
(Updated Dec. 13, 2016, 12:15 a.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Bugs: MESOS-6732
https://issues.apache.org/jira/browse/MESOS-6732
Repository: mesos
Description
-------
The XFS disk isolator checks that the filesystem is XFS, but doesn't
check whether project quotas are actually enabled. This means that
an invalid configuration will start but will always fail when tasks
are launched.
Add a check to test whether project quotas are enabled on the work
directory and fail hard if they are not.
Diffs (updated)
-----
configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d
src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
Diff: https://reviews.apache.org/r/54449/diff/
Testing
-------
Make check on Fedora 25. Manual test on F25 with mesos-execute.
Thanks,
James Peach
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158750
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 408)
<https://reviews.apache.org/r/54449/#comment229552>
Not that we are on the subject of readability, mind explaning these zeros?
```
if (::quotactl(QCMD(Q_XGETQSTATV, 0), // `type` is ignored for Q_XGETQSTATV.
```
This would help people like me in understanding how it works. :)
src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 410)
<https://reviews.apache.org/r/54449/#comment229551>
0, // The `id` argument is ignored for `Q_XGETQSTATV`.
- Jiang Yan Xu
On Dec. 9, 2016, 5:14 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 5:14 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/
-----------------------------------------------------------
(Updated Dec. 10, 2016, 1:14 a.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Updated with review fixes.
Bugs: MESOS-6732
https://issues.apache.org/jira/browse/MESOS-6732
Repository: mesos
Description
-------
The XFS disk isolator checks that the filesystem is XFS, but doesn't
check whether project quotas are actually enabled. This means that
an invalid configuration will start but will always fail when tasks
are launched.
Add a check to test whether project quotas are enabled on the work
directory and fail hard if they are not.
Diffs (updated)
-----
configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d
src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
Diff: https://reviews.apache.org/r/54449/diff/
Testing
-------
Make check on Fedora 25. Manual test on F25 with mesos-execute.
Thanks,
James Peach
Re: Review Request 54449: Check quotas are enabled in the XFS disk
isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54449/#review158292
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54449]
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. 6, 2016, 9:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 9:21 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
>
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
>
>
> Diffs
> -----
>
> configure.ac 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374
> src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a
> src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8
>
> Diff: https://reviews.apache.org/r/54449/diff/
>
>
> Testing
> -------
>
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
>
>
> Thanks,
>
> James Peach
>
>