You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2017/08/25 23:45:38 UTC
Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/
-----------------------------------------------------------
Review request for mesos and Greg Mann.
Bugs: MESOS-7916
https://issues.apache.org/jira/browse/MESOS-7916
Repository: mesos
Description
-------
These tests verifies that sibling tasks can share a Volume owned by
their parent executor using 'sandbox_path' volumes.
Diffs
-----
src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
Diff: https://reviews.apache.org/r/61921/diff/1/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/#review184222
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Aug. 31, 2017, 12:07 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61921/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 12:07 a.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-7916
> https://issues.apache.org/jira/browse/MESOS-7916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These tests verifies that sibling tasks can share a Volume owned by
> their parent executor using 'sandbox_path' volumes.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
>
>
> Diff: https://reviews.apache.org/r/61921/diff/4/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/
-----------------------------------------------------------
(Updated Aug. 31, 2017, 12:07 a.m.)
Review request for mesos and Greg Mann.
Changes
-------
Improved a comment.
Bugs: MESOS-7916
https://issues.apache.org/jira/browse/MESOS-7916
Repository: mesos
Description
-------
These tests verifies that sibling tasks can share a Volume owned by
their parent executor using 'sandbox_path' volumes.
Diffs (updated)
-----
src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
Diff: https://reviews.apache.org/r/61921/diff/4/
Changes: https://reviews.apache.org/r/61921/diff/3-4/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/
-----------------------------------------------------------
(Updated Aug. 30, 2017, 8:39 p.m.)
Review request for mesos and Greg Mann.
Changes
-------
Addressed feedback.
Bugs: MESOS-7916
https://issues.apache.org/jira/browse/MESOS-7916
Repository: mesos
Description
-------
These tests verifies that sibling tasks can share a Volume owned by
their parent executor using 'sandbox_path' volumes.
Diffs (updated)
-----
src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
Diff: https://reviews.apache.org/r/61921/diff/3/
Changes: https://reviews.apache.org/r/61921/diff/2-3/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 1982 (patched)
> > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line1982>
> >
> > Use backticks consistently around `sandbox_path`; here and below.
Updated the patch to use single quotes to be consistent with the the already existing tests (`PersistentVolumeDefaultExecutor.ROOT_PersistentResources`).
> On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 1984 (patched)
> > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line1984>
> >
> > Nice test!! :)
Thanks! =)
> On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 2014 (patched)
> > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2014>
> >
> > Is this needed?
This avoids getting gtest warnings if for some reason the nested containers take long to execute and the driver gets another offer.
> On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 2072-2073 (patched)
> > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2072>
> >
> > This comment is a little confusing. Maybe something like:
> >
> > "The test will only succeed if the executor and tasks share the same volume."
I think that tecnically there are 3 volumes, and that even though each one of them is different, the test will pass only if they all map to the same path.
I updated the patch using your suggested wording, but we might be able to come up with something better =).
> On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 2107 (patched)
> > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2107>
> >
> > s/for for/for/
Nooo, not again! :$
> On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 2133-2134 (patched)
> > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2133>
> >
> > Is this needed?
Nope, removed it from the tests.
> On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 2204-2205 (patched)
> > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2204>
> >
> > Do you think we should combine these tests into one? The tests are nearly the same, so it would eliminate some duplicated code if we combined them, at the cost of some ambiguity in the case of test failure.
> >
> > What do you think?
I think that we'd still need to add some code, and in the end I don't think that the file would be more readable:
This are the things that we'd need to do after the first two tasks run in different task groups:
1) Remove the file created by the "producer".
2) Clear the `taskStages` map between launches.
2) Wait for the executor to commit suicide.
3) Wait for another offer.
4) Set expectations for more updates.
5) Repeat the loop processing the updates.
- Gastón
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/#review184109
-----------------------------------------------------------
On Aug. 30, 2017, 8:39 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61921/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 8:39 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-7916
> https://issues.apache.org/jira/browse/MESOS-7916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These tests verifies that sibling tasks can share a Volume owned by
> their parent executor using 'sandbox_path' volumes.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
>
>
> Diff: https://reviews.apache.org/r/61921/diff/3/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 2072-2073 (patched)
> > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2072>
> >
> > This comment is a little confusing. Maybe something like:
> >
> > "The test will only succeed if the executor and tasks share the same volume."
>
> Gastón Kleiman wrote:
> I think that tecnically there are 3 volumes, and that even though each one of them is different, the test will pass only if they all map to the same path.
>
> I updated the patch using your suggested wording, but we might be able to come up with something better =).
Ah yea that's fair. How about:
"The test will only succeed if the task volume's source path is set to the path of the executor's persistent volume."
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/#review184109
-----------------------------------------------------------
On Aug. 30, 2017, 8:39 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61921/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 8:39 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-7916
> https://issues.apache.org/jira/browse/MESOS-7916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These tests verifies that sibling tasks can share a Volume owned by
> their parent executor using 'sandbox_path' volumes.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
>
>
> Diff: https://reviews.apache.org/r/61921/diff/3/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/#review184109
-----------------------------------------------------------
src/tests/default_executor_tests.cpp
Lines 1981 (patched)
<https://reviews.apache.org/r/61921/#comment260165>
Could you say explicitly here that the tasks in this test are in the same task group?
src/tests/default_executor_tests.cpp
Lines 1982 (patched)
<https://reviews.apache.org/r/61921/#comment260149>
Use backticks consistently around `sandbox_path`; here and below.
src/tests/default_executor_tests.cpp
Lines 1984 (patched)
<https://reviews.apache.org/r/61921/#comment260167>
Nice test!! :)
src/tests/default_executor_tests.cpp
Lines 2014 (patched)
<https://reviews.apache.org/r/61921/#comment260148>
Is this needed?
src/tests/default_executor_tests.cpp
Lines 2072-2073 (patched)
<https://reviews.apache.org/r/61921/#comment260150>
This comment is a little confusing. Maybe something like:
"The test will only succeed if the executor and tasks share the same volume."
src/tests/default_executor_tests.cpp
Lines 2106-2108 (patched)
<https://reviews.apache.org/r/61921/#comment260154>
Fits on two lines.
src/tests/default_executor_tests.cpp
Lines 2107 (patched)
<https://reviews.apache.org/r/61921/#comment260153>
s/for for/for/
src/tests/default_executor_tests.cpp
Lines 2112-2113 (patched)
<https://reviews.apache.org/r/61921/#comment260156>
Could you either merge "true" into the first line of the command, or indent the line containing "true" to make it clear that it is part of a single function parameter?
src/tests/default_executor_tests.cpp
Lines 2119 (patched)
<https://reviews.apache.org/r/61921/#comment260158>
Could we use a `std::vector` here instead of a C-style array?
i.e.,
```
vector<Future<v1::scheduler::Event::Update>> updates(4);
```
src/tests/default_executor_tests.cpp
Lines 2122-2123 (patched)
<https://reviews.apache.org/r/61921/#comment260160>
Could you elaborate on why this variable is necessary?
src/tests/default_executor_tests.cpp
Lines 2133-2134 (patched)
<https://reviews.apache.org/r/61921/#comment260163>
Is this needed?
src/tests/default_executor_tests.cpp
Lines 2204-2205 (patched)
<https://reviews.apache.org/r/61921/#comment260166>
Do you think we should combine these tests into one? The tests are nearly the same, so it would eliminate some duplicated code if we combined them, at the cost of some ambiguity in the case of test failure.
What do you think?
- Greg Mann
On Aug. 30, 2017, 12:39 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61921/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 12:39 a.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-7916
> https://issues.apache.org/jira/browse/MESOS-7916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These tests verifies that sibling tasks can share a Volume owned by
> their parent executor using 'sandbox_path' volumes.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
>
>
> Diff: https://reviews.apache.org/r/61921/diff/2/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/
-----------------------------------------------------------
(Updated Aug. 30, 2017, 12:39 a.m.)
Review request for mesos and Greg Mann.
Changes
-------
Rebsae.
Bugs: MESOS-7916
https://issues.apache.org/jira/browse/MESOS-7916
Repository: mesos
Description
-------
These tests verifies that sibling tasks can share a Volume owned by
their parent executor using 'sandbox_path' volumes.
Diffs (updated)
-----
src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
Diff: https://reviews.apache.org/r/61921/diff/2/
Changes: https://reviews.apache.org/r/61921/diff/1-2/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/#review183895
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61921]
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 Aug. 25, 2017, 11:45 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61921/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 11:45 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-7916
> https://issues.apache.org/jira/browse/MESOS-7916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These tests verifies that sibling tasks can share a Volume owned by
> their parent executor using 'sandbox_path' volumes.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
>
>
> Diff: https://reviews.apache.org/r/61921/diff/1/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61921: Added tests to ensure that tasks can access
their parent's volumes.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61921/#review183911
-----------------------------------------------------------
Failed to apply patch!
Reviews applied: [61921]
Logs available here: http://104.210.40.105/logs/master/61921
- Mesos Reviewbot Windows
On Aug. 25, 2017, 11:45 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61921/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2017, 11:45 p.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Bugs: MESOS-7916
> https://issues.apache.org/jira/browse/MESOS-7916
>
>
> Repository: mesos
>
>
> Description
> -------
>
> These tests verifies that sibling tasks can share a Volume owned by
> their parent executor using 'sandbox_path' volumes.
>
>
> Diffs
> -----
>
> src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1
>
>
> Diff: https://reviews.apache.org/r/61921/diff/1/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>