Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 7, 2016, 9:49 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 71c4308cccfa5c56b93f6c3928dd2a1cf3ba9741 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp acadb5bb0a1b1a9b0cee0345035b93147bf7164c 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 6, 2016, 10:35 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am ba9cc8b683bba2ae8fe9d9c58642690f5b80afaf 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp acadb5bb0a1b1a9b0cee0345035b93147bf7164c 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 6, 2016, 6:42 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp acadb5bb0a1b1a9b0cee0345035b93147bf7164c 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/tests/containerizer/xfs_quota_tests.cpp (line 154)


Methods inside a class should be separated by one blank line. Here and 
below.



src/tests/containerizer/xfs_quota_tests.cpp (line 163)


We changed this to "xfs/disk". Strictly speaking this belongs to 
 as it's not used here.



src/tests/containerizer/xfs_quota_tests.cpp (line 215)


Move this one to the bottom as it is the most complex test.



src/tests/containerizer/xfs_quota_tests.cpp (lines 271 - 282)


These checks are not related to the rest of the test above and should be in 
a separate test. Maybe

```
TEST_F(ROOT_XFS_QuotaTest, ProjectIdErrors)
{
}

```

It can be the placed at the top as the first test.



src/tests/containerizer/xfs_quota_tests.cpp (line 306)


Add a check that expects mkfile() to return an error when it goes past the 
limit.


- Jiang Yan Xu


On April 5, 2016, 4:40 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated April 5, 2016, 4:40 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 5, 2016, 11:40 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 5, 2016, 11 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[Jiang Yan Xu]

> On April 1, 2016, 12:27 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp, line 71
> > 
> >
> > We have 
> > 
> > `makeQuotaInfo` vs. `mkfile` & `mkloop`. Can we make the use of the 
> > word `make` or `mk` consistent? Generally we avoid abbreviations so using 
> > `make` (and camelCasing) is preferred.
> 
> James Peach wrote:
> ``mkfile`` and ``mkloop`` are named after ``mkfile(1)``, ``mknod(1)``, 
> etc. I expected that would be a fairly familiar nomenclature.
> 
> ``makeQuotaInfo`` is substantially different so there's no good reason to 
> use the same naming scheme. It is following the ``std::make_pair`` pattern, 
> but using Mesos naming conventions.

SGTM!


- Jiang Yan


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


On April 4, 2016, 10:27 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated April 4, 2016, 10:27 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

> On April 1, 2016, 7:27 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp, lines 292-294
> > 
> >
> > Instead of doing cleanups here, we can register the projectIds with a 
> > member variable:
> > 
> > ```
> > vector projectIds;
> > ```
> > 
> > This way in the `tearDown()` method we can go through all the them and 
> > call `clearProjectQuota()` on each.

I don't think we need this anymore since we are constructing a fresh filesystem 
each time.


> On April 1, 2016, 7:27 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp, lines 273-280
> > 
> >
> > So for this we can also create
> > 
> > ```
> > directory
> >  |
> >  |- link
> > ```
> > 
> > Then:
> > ```
> > getProjectId(link);
> > setProjectId(directory);
> > getProjectId(link);
> > clearProjectId(directory);
> > getProjectId(link);
> > ```
> > 
> > Even if the reason for `EXPECT_ERROR(getProjectId(path));` to pass is 
> > due to FTS_PHYSICAL (so we didn't attempt to set it on the link instead of 
> > we cannot set it on the link), that's the situation we are ever going to 
> > run into in real scnearios anyways so it's OK.

These tests were restructured due to feedback in other reviews.


> On April 1, 2016, 7:27 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp, line 215
> > 
> >
> > This test can be extended to include files:
> > 
> > ```
> > directory
> >   |
> >   |- file
> > ```
> > 
> > You can call `set|clearProjectId()` on the directory and verify 
> > `getProjectId()` on the file inside (and the directory).

These tests were restructured due to feedback in other reviews.


> On April 1, 2016, 7:27 a.m., Jiang Yan Xu wrote:
> > src/tests/cluster.cpp, lines 434-436
> > 
> >
> > This can be tackled in another review because a) the tests here don't 
> > use cluster.cpp and b) I see other issuse here as well (see below).
> 
> James Peach wrote:
> The isolator tests use ``cluster.cpp``. I hit this when I had a bug in 
> those tests that passed invalid flags arguments to the Slave. Let me know 
> whether you want me to move or just drop this diff. However, it is a fairly 
> annoying problem to debug.

Moved this change to a separate review.


- James


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


On April 1, 2016, midnight, James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated April 1, 2016, midnight)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 4, 2016, 5:27 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and addressed review comments.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

> On April 1, 2016, 7:27 a.m., Jiang Yan Xu wrote:
> > src/tests/cluster.cpp, lines 434-436
> > 
> >
> > This can be tackled in another review because a) the tests here don't 
> > use cluster.cpp and b) I see other issuse here as well (see below).

The isolator tests use ``cluster.cpp``. I hit this when I had a bug in those 
tests that passed invalid flags arguments to the Slave. Let me know whether you 
want me to move or just drop this diff. However, it is a fairly annoying 
problem to debug.


- James


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


On April 1, 2016, midnight, James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated April 1, 2016, midnight)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

> On April 1, 2016, 7:27 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp, line 71
> > 
> >
> > We have 
> > 
> > `makeQuotaInfo` vs. `mkfile` & `mkloop`. Can we make the use of the 
> > word `make` or `mk` consistent? Generally we avoid abbreviations so using 
> > `make` (and camelCasing) is preferred.

``mkfile`` and ``mkloop`` are named after ``mkfile(1)``, ``mknod(1)``, etc. I 
expected that would be a fairly familiar nomenclature.

``makeQuotaInfo`` is substantially different so there's no good reason to use 
the same naming scheme. It is following the ``std::make_pair`` pattern, but 
using Mesos naming conventions.


- James


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


On April 1, 2016, midnight, James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated April 1, 2016, midnight)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[Jiang Yan Xu]

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




src/tests/cluster.cpp (lines 434 - 436)


This can be tackled in another review because a) the tests here don't use 
cluster.cpp and b) I see other issuse here as well (see below).



src/tests/cluster.cpp (line 455)


Here AWAIT could result in a failure that terminates the lambda early and 
leave behind orphan containers.



src/tests/containerizer/xfs_quota_tests.cpp (line 71)


We have 

`makeQuotaInfo` vs. `mkfile` & `mkloop`. Can we make the use of the word 
`make` or `mk` consistent? Generally we avoid abbreviations so using `make` 
(and camelCasing) is preferred.



src/tests/containerizer/xfs_quota_tests.cpp (line 75)


No space: `{limit, used};`



src/tests/containerizer/xfs_quota_tests.cpp (line 215)


This test can be extended to include files:

```
directory
  |
  |- file
```

You can call `set|clearProjectId()` on the directory and verify 
`getProjectId()` on the file inside (and the directory).



src/tests/containerizer/xfs_quota_tests.cpp (line 217)


`s/p/prid/` or `s/p/project/` here and elsewhere. `project` is probably 
better because you use `projectA` and `projectB` below in another test.

When there are more variables in scope, single letter variables can be 
confusing: e.g., `p` vs. `path` which also starts with `p`.



src/tests/containerizer/xfs_quota_tests.cpp (line 218)


In your `setUp()` you have `chdir`d so it should save us from needing to 
spell out the full path here.

Fix other occurrences as well.



src/tests/containerizer/xfs_quota_tests.cpp (lines 222 - 223)


This can be done in one line:

```
EXPECT_NONE(getProjectId(path));
```

Please fix other occurences at well.



src/tests/containerizer/xfs_quota_tests.cpp (line 239)


It's now hard to do this test with the public `setProjectId()` no longer 
applicable to individual files but I think it's OK because you can construct 
the file hiearachies like above to achieve all achievable cases we'll run into.

In principle testing against public APIs is sufficient and if we have 
enumerated all possiblilties of public API usage and still cannot cover all 
100% paths in the private methods, then these paths are probably invalid and we 
should actually remove them. :)



src/tests/containerizer/xfs_quota_tests.cpp (lines 263 - 264)


I guess this is saying that when O_NOFOLLOW is used along, `openPath()` 
would fail; when O_NOFOLLOW + O_PATH is used, `openPath()` would succeed but 
the resulting fd is very limited.

I think these comments are better suitable to be put above the definition 
of `openPath()` than here because the test here doesn't have access to the 
`open` flags and the reader of the test aren't necessarily aware of the 
internal details.



src/tests/containerizer/xfs_quota_tests.cpp (line 265)


s/used/use/



src/tests/containerizer/xfs_quota_tests.cpp (lines 273 - 280)


So for this we can also create

```
directory
 |
 |- link
```

Then:
```
getProjectId(link);
setProjectId(directory);
getProjectId(link);
clearProjectId(directory);
getProjectId(link);
```

Even if the reason for `EXPECT_ERROR(getProjectId(path));` to pass is due 
to FTS_PHYSICAL (so we didn't attempt to set it on the link instead of we 
cannot set it on the link), that's the situation we are ever going to run into 
in real scnearios anyways so it's OK.



src/tests/containerizer/xfs_quota_tests.cpp (line 284)


This is the most comprehensive test in this file and it has covered all 
methods and not just for `AssignProjectId`: call it `DirectoryTree`?



src/tests/containerizer/xfs_quota_tests.cpp (lines 292 - 294)


Instead of doing cleanups here, we can register the projectIds with a 
member variable:

```
vector projectIds;
```

This way in the `tearDown()` method we can go through all the them and call 
`clearProjectQuota()` on each.

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 1, 2016, midnight)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and updated for review comments.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 30, 2016, 10:18 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 7617e43587cb81104786d06f753f08565a6c2d0a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 29, 2016, 8:55 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and updated.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 21e2965bb7cd9b88a5c787dd3efe0673c71cdc4f 
  src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 7617e43587cb81104786d06f753f08565a6c2d0a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[Jiang Yan Xu]

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



Minor comments. More thorough review once the tests reflect the proposed new 
APIs.


src/tests/containerizer/xfs_quota_tests.cpp (line 17)


Insert a blank line below.



src/tests/containerizer/xfs_quota_tests.cpp (line 51)


`mesos:internal::xfs` goes alphabetically before `process` and leave a 
blank line in between.



src/tests/containerizer/xfs_quota_tests.cpp (line 102)


End the sentence with '.'.



src/tests/containerizer/xfs_quota_tests.cpp (line 115)


In the Mesos codebase C++ style argument comment is preferred: `0, // 
Flags.`



src/tests/containerizer/xfs_quota_tests.cpp (lines 133 - 135)


The indentation here treats `subprocess(` as if it was a method argument.

Suggestion:

```
Try s = subprocess(
"losetup -d " + loopDevice.get(),
Subprocess::PATH("/dev/null"));
```



src/tests/containerizer/xfs_quota_tests.cpp (line 138)


We should give the wait a timeout. e.g., Seconds(15) as we don't want the 
tests to block forever under any circumstances.



src/tests/containerizer/xfs_quota_tests.cpp (line 150)


s/a/an/



src/tests/containerizer/xfs_quota_tests.cpp (line 167)


Doesn't look like we need `ftruncate` if we use `posix_fallocate`?

And you are not checking the exit status here.

IIUC, `os::ftruncate()` is not going to trigger ENOSPC but 
`::posix_fallocate` is. However the method is going to return `Nothing()`.



src/tests/containerizer/xfs_quota_tests.cpp (line 180)


s/ctlfd/fd/



src/tests/containerizer/xfs_quota_tests.cpp (line 189)


s/r//



src/tests/environment.cpp (line 467)


Looks like #if also works but it's more standard to use #ifdef 

http://www.faqs.org/docs/Linux-HOWTO/GCC-HOWTO.html


- Jiang Yan Xu


On March 26, 2016, 10:05 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated March 26, 2016, 10:05 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 7617e43587cb81104786d06f753f08565a6c2d0a 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 26, 2016, 5:05 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 7617e43587cb81104786d06f753f08565a6c2d0a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 22, 2016, 11:21 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp ee59a09c44242a2d31ff539106edbcb316b120aa 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 22, 2016, 1:20 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and fixed review comments.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 21, 2016, 9:46 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and fixed review comments.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 21, 2016, 6:18 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased. Added --enable-xfs-disk-isolator.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 21, 2016, 2 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 18, 2016, 3:46 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated March 17, 2016, 10:43 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach