You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anindya Sinha <an...@apple.com> on 2016/12/10 01:19:27 UTC
Re: Review Request 51880: Added unit tests to determine disk size for
MOUNT or PATH disks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51880/
-----------------------------------------------------------
(Updated Dec. 10, 2016, 1:19 a.m.)
Review request for mesos and Jiang Yan Xu.
Changes
-------
Updates based on change in Resources::parse().
Summary (updated)
-----------------
Added unit tests to determine disk size for MOUNT or PATH disks.
Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062
Repository: mesos
Description (updated)
-------
Added unit tests to determine disk size for MOUNT or PATH disks.
Diffs (updated)
-----
src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION
src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87
Diff: https://reviews.apache.org/r/51880/diff/
Testing
-------
All tests including the additional tests in this RR passed.
Thanks,
Anindya Sinha
Re: Review Request 51880: Added unit tests to determine disk size for
MOUNT or PATH disks.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51880/#review167531
-----------------------------------------------------------
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. 20, 2016, 2:04 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51880/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2016, 2:04 a.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added unit tests to determine disk size for MOUNT or PATH disks.
>
>
> Diffs
> -----
>
> src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
> src/tests/containerizer/resources_containerizer_tests.cpp PRE-CREATION
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
>
>
> Diff: https://reviews.apache.org/r/51880/diff/15/
>
>
> Testing
> -------
>
> All tests including the additional tests in this RR passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 51880: Added unit tests to determine disk size for
MOUNT or PATH disks.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51880/
-----------------------------------------------------------
(Updated May 24, 2017, 7:56 p.m.)
Review request for mesos and Jiang Yan Xu.
Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062
Repository: mesos
Description
-------
Added unit tests to determine disk size for MOUNT or PATH disks.
Diffs (updated)
-----
src/Makefile.am 7e4ce8532ec2c1b4216c2aa8d4262b2091e21c4a
src/tests/containerizer/resources_containerizer_tests.cpp PRE-CREATION
src/tests/resources_tests.cpp 5dcbce2dd4944194b59790551929d6d75b805ba3
Diff: https://reviews.apache.org/r/51880/diff/16/
Changes: https://reviews.apache.org/r/51880/diff/15-16/
Testing
-------
All tests including the additional tests in this RR passed.
Thanks,
Anindya Sinha
Re: Review Request 51880: Added unit tests to determine disk size for
MOUNT or PATH disks.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51880/
-----------------------------------------------------------
(Updated Dec. 20, 2016, 2:04 a.m.)
Review request for mesos and Jiang Yan Xu.
Changes
-------
Addressed review comments.
Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062
Repository: mesos
Description
-------
Added unit tests to determine disk size for MOUNT or PATH disks.
Diffs (updated)
-----
src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
src/tests/containerizer/resources_containerizer_tests.cpp PRE-CREATION
src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
Diff: https://reviews.apache.org/r/51880/diff/
Testing
-------
All tests including the additional tests in this RR passed.
Thanks,
Anindya Sinha
Re: Review Request 51880: Added unit tests to determine disk size for
MOUNT or PATH disks.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51880/
-----------------------------------------------------------
(Updated Dec. 18, 2016, 8:38 a.m.)
Review request for mesos and Jiang Yan Xu.
Changes
-------
Rebased.
Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062
Repository: mesos
Description
-------
Added unit tests to determine disk size for MOUNT or PATH disks.
Diffs (updated)
-----
src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION
src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
Diff: https://reviews.apache.org/r/51880/diff/
Testing
-------
All tests including the additional tests in this RR passed.
Thanks,
Anindya Sinha
Re: Review Request 51880: Added unit tests to determine disk size for
MOUNT or PATH disks.
Posted by Anindya Sinha <an...@apple.com>.
> On Dec. 17, 2016, 2:28 a.m., Jiang Yan Xu wrote:
> > src/Makefile.am, line 2263
> > <https://reviews.apache.org/r/51880/diff/13/?file=1581277#file1581277line2263>
> >
> > How about `constainerizer_tests.cpp` so it's easy to establish the mapping from <component> to <component>_tests.cpp.
> >
> > `common_containerizer_tests.cpp` reads like there's a containerizer called common containerizer.
All these tests are within the path `src/tests/containerizer`, so naming it `containerizer_tests.cpp` is probably not great as well. Agreed that `common_containerizer_tests.cpp` is not the right name as well. Should we name it `src/tests/containerizer/resources_containerizer_tests.cpp` instead?
> On Dec. 17, 2016, 2:28 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/common_containerizer_tests.cpp, line 46
> > <https://reviews.apache.org/r/51880/diff/13/?file=1581278#file1581278line46>
> >
> > Here we are really just testing `resources()`. How about naming it `ContainerizerResourcesTests`.
> >
> > In the future if more logic goes into the `Containerizer` code, we can add sepearate test fixtures. (Probably more likely for the resource detection code to move out first.)
Named it `ResourcesContainerizerTest` and `DiskResourcesContainerizerTest` to follow the existing pattern.
> On Dec. 17, 2016, 2:28 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/common_containerizer_tests.cpp, lines 64-89
> > <https://reviews.apache.org/r/51880/diff/13/?file=1581278#file1581278line64>
> >
> > Why are they not SetUp and TearDown? These special methods are executed even when tests fail which is what we want. If the thinking is that these mounts don't need to be in tests that don't involve disks with `DiskInfo`, then they can be a separate test, e.g., `ConainerizerDiskResourcesTests`.
Ok. Separated into 2 separate classes, viz. `ResourcesContainerizerTest` and `DiskResourcesContainerizerTest`, and moved setting up of mounts in `SetUp()` and unmounting in `TearDown()`.
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51880/#review151611
-----------------------------------------------------------
On Dec. 20, 2016, 2:04 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51880/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2016, 2:04 a.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added unit tests to determine disk size for MOUNT or PATH disks.
>
>
> Diffs
> -----
>
> src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
> src/tests/containerizer/resources_containerizer_tests.cpp PRE-CREATION
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
>
> Diff: https://reviews.apache.org/r/51880/diff/
>
>
> Testing
> -------
>
> All tests including the additional tests in this RR passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 51880: Added unit tests to determine disk size for
MOUNT or PATH disks.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51880/#review151611
-----------------------------------------------------------
Overall I think the tests here can be made simpler and more direct and focus on auto-detection.
- Tests like `PersistentVolumeTest` use `getSlaveResources()` to `getDiskResource()` (way simpler than ours here) to construct the agent resources but it's as a way to configure the agent. Here we are testing whether resource parsing and detection work so we should just directly construct the input text (simple string or JSON string).
- Given `ResourcesTest` already covers/should cover the various edge cases for resource parsing, in this tests we can just focus on detection (by constructing input strings that require auto-detection). The logic that glues parsed resources together are also exercised by it.
src/Makefile.am (line 2263)
<https://reviews.apache.org/r/51880/#comment230439>
How about `constainerizer_tests.cpp` so it's easy to establish the mapping from <component> to <component>_tests.cpp.
`common_containerizer_tests.cpp` reads like there's a containerizer called common containerizer.
src/tests/containerizer/common_containerizer_tests.cpp (line 40)
<https://reviews.apache.org/r/51880/#comment230441>
One empty line.
src/tests/containerizer/common_containerizer_tests.cpp (line 46)
<https://reviews.apache.org/r/51880/#comment230442>
Here we are really just testing `resources()`. How about naming it `ContainerizerResourcesTests`.
In the future if more logic goes into the `Containerizer` code, we can add sepearate test fixtures. (Probably more likely for the resource detection code to move out first.)
src/tests/containerizer/common_containerizer_tests.cpp (lines 64 - 89)
<https://reviews.apache.org/r/51880/#comment230458>
Why are they not SetUp and TearDown? These special methods are executed even when tests fail which is what we want. If the thinking is that these mounts don't need to be in tests that don't involve disks with `DiskInfo`, then they can be a separate test, e.g., `ConainerizerDiskResourcesTests`.
src/tests/containerizer/common_containerizer_tests.cpp (lines 67 - 71)
<https://reviews.apache.org/r/51880/#comment230468>
Why are they parameterized? Can we just set up a MOUNT disk and a PATH disk in `SetUp()`?
src/tests/containerizer/common_containerizer_tests.cpp (line 70)
<https://reviews.apache.org/r/51880/#comment230477>
If we know the size is 10M, in the test we can verify the detected size (inside a `#ifdef __linux__`) right?
src/tests/containerizer/common_containerizer_tests.cpp (line 73)
<https://reviews.apache.org/r/51880/#comment230469>
Add a caveat: the detected values for these mocked mounts are basically bogus but we will just check whether *a* value is detected.
src/tests/containerizer/common_containerizer_tests.cpp (lines 101 - 110)
<https://reviews.apache.org/r/51880/#comment230455>
Instead of having this many knobs, which is very hard to decipher, we could just specify the input string directly in the tests.
src/tests/containerizer/common_containerizer_tests.cpp (line 289)
<https://reviews.apache.org/r/51880/#comment230456>
We can just directly construct these JSON strings.
1) Missing resource: trivial, an empty string misses all predefined resources.
2) Missing value: e.g.,
```
string jsonString = R"~(
[
{
"name": "mem",
"type": "SCALAR"
}
])~";
```
3)-5) touches no JSON specific logic so if they are tested with simple strings we don't strictly need to test them here.
3) Resources with value zero are not auto-detected.
4) Custom resources are not auto-detected.
5) Resources specified with normal values are not changed.
With these JSON string it's very obvious what the input looks like, as opposed to have a helper method with many knobs.
If we construct JSON directly here, we can basically in one JSON input have all of the cases (except complex disks which are tested separately) right?
e.g., missing cpu, mem with no value, disk with explicit zero, explicitly specified ports, etc.
This way we don't have to repeate the same test lines over and over.
src/tests/containerizer/common_containerizer_tests.cpp (lines 371 - 377)
<https://reviews.apache.org/r/51880/#comment230476>
I don't see the need to have this many combinations of disk types (this test and the ones below). If the test case sets up a mount disk and a path disk, can we just test this setup with missing values from the JSON input?
Sample input:
```
strings::format(R"~(
[
{
"name": "disk",
"type": "SCALAR",
"disk": {
"source": {
"type": "MOUNT",
"mount": {
"root": "%s"
}
}
}
},
{
"name": "disk",
"type": "SCALAR",
"disk": {
"source": {
"type": "PATH",
"mount": {
"root": "%s"
}
}
}
}
])~",
mountRoot,
pathRoot);
```
src/tests/resources_tests.cpp (lines 606 - 634)
<https://reviews.apache.org/r/51880/#comment230440>
Two space indentation.
- Jiang Yan Xu
On Dec. 9, 2016, 5:19 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51880/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 5:19 p.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added unit tests to determine disk size for MOUNT or PATH disks.
>
>
> Diffs
> -----
>
> src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
> src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION
> src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87
>
> Diff: https://reviews.apache.org/r/51880/diff/
>
>
> Testing
> -------
>
> All tests including the additional tests in this RR passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>