You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2017/05/04 20:07:29 UTC
Review Request 58999: Added --secret_fetcher flag for agent.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/
-----------------------------------------------------------
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Repository: mesos
Description
-------
Updated Containerizer to accept SecretFetcher.
Diffs
-----
src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d
src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f
src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
Diff: https://reviews.apache.org/r/58999/diff/1/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review174242
-----------------------------------------------------------
src/slave/containerizer/containerizer.hpp
Lines 33 (patched)
<https://reviews.apache.org/r/58999/#comment247363>
why this include?
src/slave/containerizer/containerizer.cpp
Lines 25 (patched)
<https://reviews.apache.org/r/58999/#comment247365>
ditto?
src/slave/containerizer/mesos/containerizer.cpp
Lines 335-338 (patched)
<https://reviews.apache.org/r/58999/#comment247366>
why is this introduced in this review?
src/slave/flags.cpp
Lines 963 (patched)
<https://reviews.apache.org/r/58999/#comment247359>
The name of the secret resolver to use for resolving enivornment and file based secrets.
- Vinod Kone
On May 8, 2017, 10:19 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 10:19 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp 58ab74571fb14c6dbb1907151dc421f93e324bb5
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp ed99fadbf1aa91f7f0a57c9fb351c0247a40c6f4
> src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review175258
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On May 16, 2017, 12:22 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 16, 2017, 12:22 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 04ab997454534b8e5e821b53b83e166e5018e11c
> src/slave/containerizer/mesos/containerizer.cpp 97837c83cc223950750e4cd088f4da067023c96c
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
> src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
>
>
> Diff: https://reviews.apache.org/r/58999/diff/7/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/
-----------------------------------------------------------
(Updated May 22, 2017, 4:54 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Changes
-------
Addressed Vinod's comment
Repository: mesos
Description
-------
Updated Containerizer to accept SecretResolver.
Diffs (updated)
-----
src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
src/slave/containerizer/mesos/containerizer.hpp 04ab997454534b8e5e821b53b83e166e5018e11c
src/slave/containerizer/mesos/containerizer.cpp 50a63b58b4960729316b0b5685793ce18ee5ce93
src/slave/flags.hpp 28f6482c38a2d388e624726d5e7bccc5cb260fce
src/slave/flags.cpp e172aa526cfd38f4f30efda22ef011146e5c1a7d
src/slave/main.cpp cc833279ae0a1c2e62166afe059cad14eb9978a4
Diff: https://reviews.apache.org/r/58999/diff/8/
Changes: https://reviews.apache.org/r/58999/diff/7-8/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review175327
-----------------------------------------------------------
Ship it!
Add a comment to the flag help saying the default value based resolver will be used if this is not set?
- Vinod Kone
On May 16, 2017, 7:22 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 16, 2017, 7:22 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 04ab997454534b8e5e821b53b83e166e5018e11c
> src/slave/containerizer/mesos/containerizer.cpp 97837c83cc223950750e4cd088f4da067023c96c
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
> src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
>
>
> Diff: https://reviews.apache.org/r/58999/diff/7/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/
-----------------------------------------------------------
(Updated May 16, 2017, 3:22 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Changes
-------
Addressed Gilbert's comments.
Repository: mesos
Description
-------
Updated Containerizer to accept SecretResolver.
Diffs (updated)
-----
src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
src/slave/containerizer/mesos/containerizer.hpp 04ab997454534b8e5e821b53b83e166e5018e11c
src/slave/containerizer/mesos/containerizer.cpp 97837c83cc223950750e4cd088f4da067023c96c
src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
Diff: https://reviews.apache.org/r/58999/diff/7/
Changes: https://reviews.apache.org/r/58999/diff/6-7/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On May 15, 2017, 7:57 a.m., Gilbert Song wrote:
> > src/slave/flags.cpp
> > Lines 961-964 (patched)
> > <https://reviews.apache.org/r/58999/diff/6/?file=1717011#file1717011line961>
> >
> > Should we consider make the `default secret resolver` as the default value for this flag? So that we can get rid of the logic in `SecretResolver::create()`.
I am not sure about it because the create logic will only change from `if (moduleName.isNone()) {...}` to `if (moduleName == DEFAULT_SECRET_RESOLVER) {...}`.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review174918
-----------------------------------------------------------
On May 16, 2017, 3:22 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 16, 2017, 3:22 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 04ab997454534b8e5e821b53b83e166e5018e11c
> src/slave/containerizer/mesos/containerizer.cpp 97837c83cc223950750e4cd088f4da067023c96c
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
> src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
>
>
> Diff: https://reviews.apache.org/r/58999/diff/7/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review174918
-----------------------------------------------------------
src/slave/flags.cpp
Lines 961-964 (patched)
<https://reviews.apache.org/r/58999/#comment248230>
Should we consider make the `default secret resolver` as the default value for this flag? So that we can get rid of the logic in `SecretResolver::create()`.
src/slave/flags.cpp
Lines 963 (patched)
<https://reviews.apache.org/r/58999/#comment248188>
s/secret resolve module/secret resolver module/g ?
- Gilbert Song
On May 12, 2017, 10:52 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 12, 2017, 10:52 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 04ab997454534b8e5e821b53b83e166e5018e11c
> src/slave/containerizer/mesos/containerizer.cpp 97837c83cc223950750e4cd088f4da067023c96c
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
> src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
>
>
> Diff: https://reviews.apache.org/r/58999/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/
-----------------------------------------------------------
(Updated May 12, 2017, 1:52 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Changes
-------
Rebased and fixed clang warnings.
Repository: mesos
Description
-------
Updated Containerizer to accept SecretResolver.
Diffs (updated)
-----
src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
src/slave/containerizer/mesos/containerizer.hpp 04ab997454534b8e5e821b53b83e166e5018e11c
src/slave/containerizer/mesos/containerizer.cpp 97837c83cc223950750e4cd088f4da067023c96c
src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
src/slave/flags.cpp bc63a6a4cb6115b4b4d592e67e34045f52b50d4c
src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
Diff: https://reviews.apache.org/r/58999/diff/6/
Changes: https://reviews.apache.org/r/58999/diff/5-6/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/
-----------------------------------------------------------
(Updated May 9, 2017, 2:11 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Changes
-------
Addressed Vinod's comments!
Repository: mesos
Description
-------
Updated Containerizer to accept SecretResolver.
Diffs (updated)
-----
src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
src/slave/containerizer/mesos/containerizer.cpp 58ab74571fb14c6dbb1907151dc421f93e324bb5
src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
src/slave/flags.cpp ed99fadbf1aa91f7f0a57c9fb351c0247a40c6f4
src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
Diff: https://reviews.apache.org/r/58999/diff/5/
Changes: https://reviews.apache.org/r/58999/diff/4-5/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On May 8, 2017, 8:05 p.m., Vinod Kone wrote:
> > src/slave/main.cpp
> > Line 450 (original), 463 (patched)
> > <https://reviews.apache.org/r/58999/diff/4/?file=1710998#file1710998line463>
> >
> > If the pointer to resolver is only passed to the containerizer, how will the agent get access to it to resolve executor auth token?
We'll be modifying the `executorEnvironment` signature to return `Try<Environment>` instead of `map<string, string>` so that all env vars can be resolved in the containerizer itself (via env isolator).
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review174248
-----------------------------------------------------------
On May 9, 2017, 2:11 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 9, 2017, 2:11 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp 58ab74571fb14c6dbb1907151dc421f93e324bb5
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp ed99fadbf1aa91f7f0a57c9fb351c0247a40c6f4
> src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review174248
-----------------------------------------------------------
src/slave/main.cpp
Line 450 (original), 463 (patched)
<https://reviews.apache.org/r/58999/#comment247379>
If the pointer to resolver is only passed to the containerizer, how will the agent get access to it to resolve executor auth token?
- Vinod Kone
On May 8, 2017, 10:19 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 10:19 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp 58ab74571fb14c6dbb1907151dc421f93e324bb5
> src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
> src/slave/flags.cpp ed99fadbf1aa91f7f0a57c9fb351c0247a40c6f4
> src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/
-----------------------------------------------------------
(Updated May 8, 2017, 6:19 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Changes
-------
Rebased
Repository: mesos
Description
-------
Updated Containerizer to accept SecretResolver.
Diffs (updated)
-----
src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
src/slave/containerizer/mesos/containerizer.cpp 58ab74571fb14c6dbb1907151dc421f93e324bb5
src/slave/flags.hpp e5784ef81ad0720c7ec061ee0b28b8fadae77afd
src/slave/flags.cpp ed99fadbf1aa91f7f0a57c9fb351c0247a40c6f4
src/slave/main.cpp 507d59996a90f51c7cd1e0f836a34c2e9c484791
src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
Diff: https://reviews.apache.org/r/58999/diff/4/
Changes: https://reviews.apache.org/r/58999/diff/3-4/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/
-----------------------------------------------------------
(Updated May 5, 2017, 9:26 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Changes
-------
Remove stale pointer copy.
Repository: mesos
Description
-------
Updated Containerizer to accept SecretResolver.
Diffs (updated)
-----
src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
Diff: https://reviews.apache.org/r/58999/diff/3/
Changes: https://reviews.apache.org/r/58999/diff/2-3/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/
-----------------------------------------------------------
(Updated May 5, 2017, 6:41 a.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Changes
-------
SecretFetcher -> SecretResolver; Option<SecretFetcher*> -> SecretResolver*
Summary (updated)
-----------------
Added --secret_resolver flag to agent.
Repository: mesos
Description (updated)
-------
Updated Containerizer to accept SecretResolver.
Diffs (updated)
-----
src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
Diff: https://reviews.apache.org/r/58999/diff/2/
Changes: https://reviews.apache.org/r/58999/diff/1-2/
Testing
-------
Thanks,
Kapil Arya
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On May 4, 2017, 6:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
>
> Kapil Arya wrote:
> That way we don't have to update the existing unit tests involving containerizers.
>
> Jie Yu wrote:
> Can you try using default parameter?
>
> Jie Yu wrote:
> Even default parameter is hacky. We should at least have a TODO somewhere. I saw the isolator will simply call secretFetcher.get() assuming it's Some(), while you're passing None() to containerizer in the test.
>
> I don't like the way we inject `Fetcher` in tests also. That's the reason why RAW pointer is evil. If you have a managed pointer, you probably don't have this issue. Maybe we should use a managed pointer in the interface? The fact that the module create returns a raw pointer is a bad decision in retrospect. It should have been unique_ptr or Owned pointer.
>
> Jie Yu wrote:
> Any reason this cannot be a Shared pointer? `Shared<SecretFetcher>`?
>
> just make the method in SecretFetcher const.
>
> Kapil Arya wrote:
> Are you suggesting `SecretFecther::fetch(...) const;` ?
>
> Kapil Arya wrote:
> I am not sure how we can use managed pointers with modules because a module might just return a pointer to a static object without ever calling `new`.
>
> I think we'll need to use raw pointer with a default parameter instead.
>
> Jie Yu wrote:
> As I said, the fact that module returns a raw pointer is a tech debt. We should force it to return an Owned pointer or unique_ptr in the future.
>
> Can you follow up with patches to fix that if possible? I think we can change the module interface at anytime?
>
> Jie Yu wrote:
> For the time being, can you just add a documentation to the SecretFetcher saying that the create must returns a dynamically allocated object whose lifecycle should be delegate to the caller?
Yes, I'll add a comment/TODO for now. I already spoke with Benjamin about making appropriate changes to the module interface in 1.4 timeframe. The module API is marked experimental and can change at any time.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 5, 2017, 6:41 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 6:41 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On May 4, 2017, 6:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
>
> Kapil Arya wrote:
> That way we don't have to update the existing unit tests involving containerizers.
>
> Jie Yu wrote:
> Can you try using default parameter?
>
> Jie Yu wrote:
> Even default parameter is hacky. We should at least have a TODO somewhere. I saw the isolator will simply call secretFetcher.get() assuming it's Some(), while you're passing None() to containerizer in the test.
>
> I don't like the way we inject `Fetcher` in tests also. That's the reason why RAW pointer is evil. If you have a managed pointer, you probably don't have this issue. Maybe we should use a managed pointer in the interface? The fact that the module create returns a raw pointer is a bad decision in retrospect. It should have been unique_ptr or Owned pointer.
>
> Jie Yu wrote:
> Any reason this cannot be a Shared pointer? `Shared<SecretFetcher>`?
>
> just make the method in SecretFetcher const.
>
> Kapil Arya wrote:
> Are you suggesting `SecretFecther::fetch(...) const;` ?
>
> Kapil Arya wrote:
> I am not sure how we can use managed pointers with modules because a module might just return a pointer to a static object without ever calling `new`.
>
> I think we'll need to use raw pointer with a default parameter instead.
>
> Jie Yu wrote:
> As I said, the fact that module returns a raw pointer is a tech debt. We should force it to return an Owned pointer or unique_ptr in the future.
>
> Can you follow up with patches to fix that if possible? I think we can change the module interface at anytime?
>
> Jie Yu wrote:
> For the time being, can you just add a documentation to the SecretFetcher saying that the create must returns a dynamically allocated object whose lifecycle should be delegate to the caller?
>
> Kapil Arya wrote:
> Yes, I'll add a comment/TODO for now. I already spoke with Benjamin about making appropriate changes to the module interface in 1.4 timeframe. The module API is marked experimental and can change at any time.
Added comments to `mesos/module.hpp` (see https://reviews.apache.org/r/58759/diff/5#0.2).
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 5, 2017, 6:41 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 6:41 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_fetcher flag for agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On May 4, 2017, 6:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
That way we don't have to update the existing unit tests involving containerizers.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 4, 2017, 4:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 4:07 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretFetcher.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d
> src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_fetcher flag for agent.
Posted by Jie Yu <yu...@gmail.com>.
> On May 4, 2017, 10:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
>
> Kapil Arya wrote:
> That way we don't have to update the existing unit tests involving containerizers.
Can you try using default parameter?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 4, 2017, 8:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 8:07 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretFetcher.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d
> src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_fetcher flag for agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On May 4, 2017, 6:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
>
> Kapil Arya wrote:
> That way we don't have to update the existing unit tests involving containerizers.
>
> Jie Yu wrote:
> Can you try using default parameter?
>
> Jie Yu wrote:
> Even default parameter is hacky. We should at least have a TODO somewhere. I saw the isolator will simply call secretFetcher.get() assuming it's Some(), while you're passing None() to containerizer in the test.
>
> I don't like the way we inject `Fetcher` in tests also. That's the reason why RAW pointer is evil. If you have a managed pointer, you probably don't have this issue. Maybe we should use a managed pointer in the interface? The fact that the module create returns a raw pointer is a bad decision in retrospect. It should have been unique_ptr or Owned pointer.
>
> Jie Yu wrote:
> Any reason this cannot be a Shared pointer? `Shared<SecretFetcher>`?
>
> just make the method in SecretFetcher const.
>
> Kapil Arya wrote:
> Are you suggesting `SecretFecther::fetch(...) const;` ?
I am not sure how we can use managed pointers with modules because a module might just return a pointer to a static object without ever calling `new`.
I think we'll need to use raw pointer with a default parameter instead.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 4, 2017, 4:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 4:07 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretFetcher.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d
> src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_fetcher flag for agent.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On May 4, 2017, 6:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
>
> Kapil Arya wrote:
> That way we don't have to update the existing unit tests involving containerizers.
>
> Jie Yu wrote:
> Can you try using default parameter?
>
> Jie Yu wrote:
> Even default parameter is hacky. We should at least have a TODO somewhere. I saw the isolator will simply call secretFetcher.get() assuming it's Some(), while you're passing None() to containerizer in the test.
>
> I don't like the way we inject `Fetcher` in tests also. That's the reason why RAW pointer is evil. If you have a managed pointer, you probably don't have this issue. Maybe we should use a managed pointer in the interface? The fact that the module create returns a raw pointer is a bad decision in retrospect. It should have been unique_ptr or Owned pointer.
>
> Jie Yu wrote:
> Any reason this cannot be a Shared pointer? `Shared<SecretFetcher>`?
>
> just make the method in SecretFetcher const.
Are you suggesting `SecretFecther::fetch(...) const;` ?
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 4, 2017, 4:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 4:07 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretFetcher.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d
> src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_fetcher flag for agent.
Posted by Jie Yu <yu...@gmail.com>.
> On May 4, 2017, 10:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
>
> Kapil Arya wrote:
> That way we don't have to update the existing unit tests involving containerizers.
>
> Jie Yu wrote:
> Can you try using default parameter?
>
> Jie Yu wrote:
> Even default parameter is hacky. We should at least have a TODO somewhere. I saw the isolator will simply call secretFetcher.get() assuming it's Some(), while you're passing None() to containerizer in the test.
>
> I don't like the way we inject `Fetcher` in tests also. That's the reason why RAW pointer is evil. If you have a managed pointer, you probably don't have this issue. Maybe we should use a managed pointer in the interface? The fact that the module create returns a raw pointer is a bad decision in retrospect. It should have been unique_ptr or Owned pointer.
Any reason this cannot be a Shared pointer? `Shared<SecretFetcher>`?
just make the method in SecretFetcher const.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 4, 2017, 8:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 8:07 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretFetcher.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d
> src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_fetcher flag for agent.
Posted by Jie Yu <yu...@gmail.com>.
> On May 4, 2017, 10:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
>
> Kapil Arya wrote:
> That way we don't have to update the existing unit tests involving containerizers.
>
> Jie Yu wrote:
> Can you try using default parameter?
Even default parameter is hacky. We should at least have a TODO somewhere. I saw the isolator will simply call secretFetcher.get() assuming it's Some(), while you're passing None() to containerizer in the test.
I don't like the way we inject `Fetcher` in tests also. That's the reason why RAW pointer is evil. If you have a managed pointer, you probably don't have this issue. Maybe we should use a managed pointer in the interface? The fact that the module create returns a raw pointer is a bad decision in retrospect. It should have been unique_ptr or Owned pointer.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 4, 2017, 8:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 8:07 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretFetcher.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d
> src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Jie Yu <yu...@gmail.com>.
> On May 4, 2017, 10:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
>
> Kapil Arya wrote:
> That way we don't have to update the existing unit tests involving containerizers.
>
> Jie Yu wrote:
> Can you try using default parameter?
>
> Jie Yu wrote:
> Even default parameter is hacky. We should at least have a TODO somewhere. I saw the isolator will simply call secretFetcher.get() assuming it's Some(), while you're passing None() to containerizer in the test.
>
> I don't like the way we inject `Fetcher` in tests also. That's the reason why RAW pointer is evil. If you have a managed pointer, you probably don't have this issue. Maybe we should use a managed pointer in the interface? The fact that the module create returns a raw pointer is a bad decision in retrospect. It should have been unique_ptr or Owned pointer.
>
> Jie Yu wrote:
> Any reason this cannot be a Shared pointer? `Shared<SecretFetcher>`?
>
> just make the method in SecretFetcher const.
>
> Kapil Arya wrote:
> Are you suggesting `SecretFecther::fetch(...) const;` ?
>
> Kapil Arya wrote:
> I am not sure how we can use managed pointers with modules because a module might just return a pointer to a static object without ever calling `new`.
>
> I think we'll need to use raw pointer with a default parameter instead.
As I said, the fact that module returns a raw pointer is a tech debt. We should force it to return an Owned pointer or unique_ptr in the future.
Can you follow up with patches to fix that if possible? I think we can change the module interface at anytime?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 5, 2017, 10:41 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 10:41 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_resolver flag to agent.
Posted by Jie Yu <yu...@gmail.com>.
> On May 4, 2017, 10:29 p.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 66 (patched)
> > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66>
> >
> > Why optional?
>
> Kapil Arya wrote:
> That way we don't have to update the existing unit tests involving containerizers.
>
> Jie Yu wrote:
> Can you try using default parameter?
>
> Jie Yu wrote:
> Even default parameter is hacky. We should at least have a TODO somewhere. I saw the isolator will simply call secretFetcher.get() assuming it's Some(), while you're passing None() to containerizer in the test.
>
> I don't like the way we inject `Fetcher` in tests also. That's the reason why RAW pointer is evil. If you have a managed pointer, you probably don't have this issue. Maybe we should use a managed pointer in the interface? The fact that the module create returns a raw pointer is a bad decision in retrospect. It should have been unique_ptr or Owned pointer.
>
> Jie Yu wrote:
> Any reason this cannot be a Shared pointer? `Shared<SecretFetcher>`?
>
> just make the method in SecretFetcher const.
>
> Kapil Arya wrote:
> Are you suggesting `SecretFecther::fetch(...) const;` ?
>
> Kapil Arya wrote:
> I am not sure how we can use managed pointers with modules because a module might just return a pointer to a static object without ever calling `new`.
>
> I think we'll need to use raw pointer with a default parameter instead.
>
> Jie Yu wrote:
> As I said, the fact that module returns a raw pointer is a tech debt. We should force it to return an Owned pointer or unique_ptr in the future.
>
> Can you follow up with patches to fix that if possible? I think we can change the module interface at anytime?
For the time being, can you just add a documentation to the SecretFetcher saying that the create must returns a dynamically allocated object whose lifecycle should be delegate to the caller?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
On May 5, 2017, 10:41 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 5, 2017, 10:41 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretResolver.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 58999: Added --secret_fetcher flag for agent.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58999/#review173969
-----------------------------------------------------------
src/slave/containerizer/containerizer.hpp
Lines 66 (patched)
<https://reviews.apache.org/r/58999/#comment247066>
Why optional?
- Jie Yu
On May 4, 2017, 8:07 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58999/
> -----------------------------------------------------------
>
> (Updated May 4, 2017, 8:07 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated Containerizer to accept SecretFetcher.
>
>
> Diffs
> -----
>
> src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
> src/slave/containerizer/containerizer.cpp 9024371b6c4228f0903cfeef3bbec736e1a425f8
> src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
> src/slave/containerizer/mesos/containerizer.cpp b58baed64480e22f640a4852537f85922ed382ae
> src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94
> src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173
> src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52
> src/tests/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d
> src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f
> src/tests/containerizer/docker_volume_isolator_tests.cpp b47a6b5081a63ac474ac4634701b1a572eb58137
> src/tests/containerizer/mesos_containerizer_tests.cpp 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4
>
>
> Diff: https://reviews.apache.org/r/58999/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kapil Arya
>
>