Re: Review Request 46229: Add unit tests for adding a user for persistent volumes.

[Joris Van Remoortere]

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



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 Nov. 20, 2016, 4:51 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46229/
> ---
> 
> (Updated Nov. 20, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add unit tests for adding a user for persistent volumes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 24d1a6971d412aaf7bdc20efcc1197b120de51ce 
>   src/tests/hierarchical_allocator_tests.cpp 
> edd5cea8996d7c616cf9428f0f1c05d70c37c307 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
>   src/tests/mesos.hpp f94882f44e7fd35f6e9aaa381656af5c5a58ff9e 
>   src/tests/persistent_volume_tests.cpp 
> 8651b2c5455089041f16d091c90a7e0d948191b8 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 
> 
> 
> Diff: https://reviews.apache.org/r/46229/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed (including the newly added tests).
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 46229: Add unit tests for adding a user for persistent volumes.

[Greg Mann]

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



I was just trying to apply this to test and it looks like it needs a rebase?

- Greg Mann


On Nov. 20, 2016, 4:51 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46229/
> ---
> 
> (Updated Nov. 20, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add unit tests for adding a user for persistent volumes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 24d1a6971d412aaf7bdc20efcc1197b120de51ce 
>   src/tests/hierarchical_allocator_tests.cpp 
> edd5cea8996d7c616cf9428f0f1c05d70c37c307 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
>   src/tests/mesos.hpp f94882f44e7fd35f6e9aaa381656af5c5a58ff9e 
>   src/tests/persistent_volume_tests.cpp 
> 8651b2c5455089041f16d091c90a7e0d948191b8 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 
> 
> Diff: https://reviews.apache.org/r/46229/diff/
> 
> 
> Testing
> ---
> 
> All tests passed (including the newly added tests).
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 46229: Add unit tests for adding a user for persistent volumes.

[Greg Mann]

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




src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1531)


Nit:
s/use (and assume the presence) of/use (and assume the presence of)/



src/tests/persistent_volume_tests.cpp (lines 554 - 559)


I think that providing some validation of the nobody user is a good idea, 
but I wonder if this is the best way to accomplish it (and in the test below as 
well). It looks like this block also appears in 
`SlaveTest.DISABLED_ROOT_RunTaskWithCommandInfoWithUser`, but other tests which 
make use of 'nobody' (`SlaveTest.ROOT_LaunchTaskInfoWithContainerInfo`, for 
example) do not have such a check.

We could instead add a new filter to the test filters already present in 
`src/tests/environment.cpp` which would filter out all tests which make use of 
the `nobody` user when it's not present.

I would advocate a more general solution like that, I think. If you opt for 
such an approach, you could do it as part of this work, or just place a TODO 
here which points to a Mesos JIRA for that task.

Thoughts?


- Greg Mann


On Nov. 20, 2016, 4:51 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46229/
> ---
> 
> (Updated Nov. 20, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add unit tests for adding a user for persistent volumes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 24d1a6971d412aaf7bdc20efcc1197b120de51ce 
>   src/tests/hierarchical_allocator_tests.cpp 
> edd5cea8996d7c616cf9428f0f1c05d70c37c307 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
>   src/tests/mesos.hpp f94882f44e7fd35f6e9aaa381656af5c5a58ff9e 
>   src/tests/persistent_volume_tests.cpp 
> 8651b2c5455089041f16d091c90a7e0d948191b8 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 
> 
> Diff: https://reviews.apache.org/r/46229/diff/
> 
> 
> Testing
> ---
> 
> All tests passed (including the newly added tests).
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 46229: Add unit tests for adding a user for persistent volumes.

[Anindya Sinha]

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

(Updated Nov. 20, 2016, 4:51 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

addressed review comments, and rebased.


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


Repository: mesos


Description
---

Add unit tests for adding a user for persistent volumes.


Diffs (updated)
-

  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
24d1a6971d412aaf7bdc20efcc1197b120de51ce 
  src/tests/hierarchical_allocator_tests.cpp 
edd5cea8996d7c616cf9428f0f1c05d70c37c307 
  src/tests/master_validation_tests.cpp 
f893067859425967654401f3226149268b51cf57 
  src/tests/mesos.hpp f94882f44e7fd35f6e9aaa381656af5c5a58ff9e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 
  src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
  src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 

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


Testing
---

All tests passed (including the newly added tests).


Thanks,

Anindya Sinha

Re: Review Request 46229: Add unit tests for adding a user for persistent volumes.

[Greg Mann]

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




src/tests/containerizer/linux_filesystem_isolator_tests.cpp (lines 1527 - 1531)


Could you expand this test to include both a successful and an unsuccessful 
attempt to access the volume? We already make use of the user "nobody" in the 
agent tests, so we could use it here as well.



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1532)


Since this test doesn't use a shared volume, should probably remove 
"Shared" from the test name?



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (lines 1595 - 1596)


This comment is incorrect, right? Looks like the task is successfully 
writing.



src/tests/persistent_volume_tests.cpp (lines 547 - 548)


Could you include a short comment preceding the test which explains the 
test's purpose?



src/tests/persistent_volume_tests.cpp (line 587)


Perhaps to more comprehensively verify the intended behavior here, you 
could create the volume as the "nobody" user and check the volume's owner after 
the directory has been created.


- Greg Mann


On Oct. 18, 2016, 7:46 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46229/
> ---
> 
> (Updated Oct. 18, 2016, 7:46 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add unit tests for adding a user for persistent volumes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> f17ed4437a5e1366f85803ce7e29bee24162504c 
>   src/tests/hierarchical_allocator_tests.cpp 
> 2e979d784b8e6cdacebac78a67498b5f4d023540 
>   src/tests/master_validation_tests.cpp 
> da43f990e3b8f61e27b22d551cd3e07638c7ff37 
>   src/tests/mesos.hpp 9309b5a985c0d7136a2ee5aa1598b4fee6194816 
>   src/tests/persistent_volume_tests.cpp 
> 6289009fe9ed0a57ba5eff46dbbe0633a75d2616 
>   src/tests/resources_tests.cpp 6a12783c26f359dda835b4866b299a8fcfb3f972 
>   src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 
> 
> Diff: https://reviews.apache.org/r/46229/diff/
> 
> 
> Testing
> ---
> 
> All tests passed (including the newly added tests).
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 46229: Add unit tests for adding a user for persistent volumes.

[Greg Mann]

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



Looks like this needs a rebase.

- Greg Mann


On Oct. 18, 2016, 7:46 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46229/
> ---
> 
> (Updated Oct. 18, 2016, 7:46 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add unit tests for adding a user for persistent volumes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> f17ed4437a5e1366f85803ce7e29bee24162504c 
>   src/tests/hierarchical_allocator_tests.cpp 
> 2e979d784b8e6cdacebac78a67498b5f4d023540 
>   src/tests/master_validation_tests.cpp 
> da43f990e3b8f61e27b22d551cd3e07638c7ff37 
>   src/tests/mesos.hpp 9309b5a985c0d7136a2ee5aa1598b4fee6194816 
>   src/tests/persistent_volume_tests.cpp 
> 6289009fe9ed0a57ba5eff46dbbe0633a75d2616 
>   src/tests/resources_tests.cpp 6a12783c26f359dda835b4866b299a8fcfb3f972 
>   src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 
> 
> Diff: https://reviews.apache.org/r/46229/diff/
> 
> 
> Testing
> ---
> 
> All tests passed (including the newly added tests).
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 46229: Add unit tests for adding a user for persistent volumes.

[Anindya Sinha]

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

(Updated Oct. 18, 2016, 7:46 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add unit tests for adding a user for persistent volumes.


Diffs (updated)
-

  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
f17ed4437a5e1366f85803ce7e29bee24162504c 
  src/tests/hierarchical_allocator_tests.cpp 
2e979d784b8e6cdacebac78a67498b5f4d023540 
  src/tests/master_validation_tests.cpp 
da43f990e3b8f61e27b22d551cd3e07638c7ff37 
  src/tests/mesos.hpp 9309b5a985c0d7136a2ee5aa1598b4fee6194816 
  src/tests/persistent_volume_tests.cpp 
6289009fe9ed0a57ba5eff46dbbe0633a75d2616 
  src/tests/resources_tests.cpp 6a12783c26f359dda835b4866b299a8fcfb3f972 
  src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 

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


Testing
---

All tests passed (including the newly added tests).


Thanks,

Anindya Sinha

Review Request 46229: Add unit tests for adding a user for persistent volumes.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Add unit tests for adding a user for persistent volumes.


Diffs
-

  src/tests/containerizer/filesystem_isolator_tests.cpp 
51e60c2e8c6c8b76b51de0e7761ecbb0ca9c3304 
  src/tests/mesos.hpp 20370a277d55efeea8daae7ea5e2f6575b5a2d62 
  src/tests/persistent_volume_tests.cpp 
d246f35046fff469b847c908de2b305ae629212f 
  src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 

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


Testing
---

All tests passed (including the newly added tests).


Thanks,

Anindya Sinha