You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2019/05/29 04:29:04 UTC
Review Request 70741: Adopted container file operations for secrets
volumes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70741/
-----------------------------------------------------------
Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and Jiang Yan Xu.
Bugs: MESOS-9799
https://issues.apache.org/jira/browse/MESOS-9799
Repository: mesos
Description
-------
Switched the `volumes/secrets` isolator from using container
pre-exec commands to using file operations. This allows us to
reduce the number of forked child processes, consolidate code
and leaves the `network/port_mapping` isolator as the last
consumer of pre-exec commands.
Diffs
-----
include/mesos/slave/containerizer.proto b2e35cbf01caea6c1e4f45c7b7d833bc7f065099
src/common/protobuf_utils.hpp 8aa9aff71cbbd8e98573d0095cfc007cdea163bc
src/common/protobuf_utils.cpp 870859de93b44a1debc6786a562b4bc28955ddab
src/slave/containerizer/mesos/isolators/volume/secret.cpp 7a9bb82b23b35728408fb37bac53d79883c0a19f
src/slave/containerizer/mesos/launch.cpp 0c482f46a97063133edfe29ae3c6a2721d29f6c6
Diff: https://reviews.apache.org/r/70741/diff/1/
Testing
-------
sudo make check (Fedora 30)
Thanks,
James Peach
Re: Review Request 70741: Adopted container file operations for
secrets volumes.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70741/#review215708
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70741]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On June 5, 2019, 8:44 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70741/
> -----------------------------------------------------------
>
> (Updated June 5, 2019, 8:44 a.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9799
> https://issues.apache.org/jira/browse/MESOS-9799
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Switched the `volumes/secrets` isolator from using container
> pre-exec commands to using file operations. This allows us to
> reduce the number of forked child processes, consolidate code
> and leaves the `network/port_mapping` isolator as the last
> consumer of pre-exec commands.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto b2e35cbf01caea6c1e4f45c7b7d833bc7f065099
> src/common/protobuf_utils.hpp 8aa9aff71cbbd8e98573d0095cfc007cdea163bc
> src/common/protobuf_utils.cpp 870859de93b44a1debc6786a562b4bc28955ddab
> src/slave/containerizer/mesos/isolators/volume/secret.cpp 7a9bb82b23b35728408fb37bac53d79883c0a19f
> src/slave/containerizer/mesos/launch.cpp 0c482f46a97063133edfe29ae3c6a2721d29f6c6
>
>
> Diff: https://reviews.apache.org/r/70741/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70741: Adopted container file operations for
secrets volumes.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70741/#review215714
-----------------------------------------------------------
Ship it!
Ship It!
- Jiang Yan Xu
On June 5, 2019, 1:44 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70741/
> -----------------------------------------------------------
>
> (Updated June 5, 2019, 1:44 a.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9799
> https://issues.apache.org/jira/browse/MESOS-9799
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Switched the `volumes/secrets` isolator from using container
> pre-exec commands to using file operations. This allows us to
> reduce the number of forked child processes, consolidate code
> and leaves the `network/port_mapping` isolator as the last
> consumer of pre-exec commands.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto b2e35cbf01caea6c1e4f45c7b7d833bc7f065099
> src/common/protobuf_utils.hpp 8aa9aff71cbbd8e98573d0095cfc007cdea163bc
> src/common/protobuf_utils.cpp 870859de93b44a1debc6786a562b4bc28955ddab
> src/slave/containerizer/mesos/isolators/volume/secret.cpp 7a9bb82b23b35728408fb37bac53d79883c0a19f
> src/slave/containerizer/mesos/launch.cpp 0c482f46a97063133edfe29ae3c6a2721d29f6c6
>
>
> Diff: https://reviews.apache.org/r/70741/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70741: Adopted container file operations for
secrets volumes.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70741/
-----------------------------------------------------------
(Updated June 5, 2019, 8:44 a.m.)
Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and Jiang Yan Xu.
Bugs: MESOS-9799
https://issues.apache.org/jira/browse/MESOS-9799
Repository: mesos
Description
-------
Switched the `volumes/secrets` isolator from using container
pre-exec commands to using file operations. This allows us to
reduce the number of forked child processes, consolidate code
and leaves the `network/port_mapping` isolator as the last
consumer of pre-exec commands.
Diffs (updated)
-----
include/mesos/slave/containerizer.proto b2e35cbf01caea6c1e4f45c7b7d833bc7f065099
src/common/protobuf_utils.hpp 8aa9aff71cbbd8e98573d0095cfc007cdea163bc
src/common/protobuf_utils.cpp 870859de93b44a1debc6786a562b4bc28955ddab
src/slave/containerizer/mesos/isolators/volume/secret.cpp 7a9bb82b23b35728408fb37bac53d79883c0a19f
src/slave/containerizer/mesos/launch.cpp 0c482f46a97063133edfe29ae3c6a2721d29f6c6
Diff: https://reviews.apache.org/r/70741/diff/3/
Changes: https://reviews.apache.org/r/70741/diff/2-3/
Testing
-------
sudo make check (Fedora 30)
Thanks,
James Peach
Re: Review Request 70741: Adopted container file operations for
secrets volumes.
Posted by James Peach <jp...@apache.org>.
> On June 5, 2019, 7:16 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/volume/secret.cpp
> > Line 138 (original), 147 (patched)
> > <https://reviews.apache.org/r/70741/diff/2/?file=2147411#file2147411line147>
> >
> > Do you know why `launchInfo.mounts` was not used (and for the same reason it couldn't be used here)?
You need to sequence the mount with other operations, so you can't use the container mounts. We can revisit coalescing these in the future, but for now I opted for compatibility and simplicity.
> On June 5, 2019, 7:16 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 516 (patched)
> > <https://reviews.apache.org/r/70741/diff/2/?file=2147412#file2147412line524>
> >
> > Is it more explicit if you just name the operation `ContainerFileOperation::MV` or `ContainerFileOperation::MOVE` and implement it as such (rename + fallback) either here or in stout since you are essentially implementing how we use a `mv` command?
`rename` vs. `move` seems pretty similar. To me, `rename` seemed clear so I prefer it. I didn't add a stout implementation because I didn't have the time to handle all the corner cases and write proper tests for a general utility.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70741/#review215696
-----------------------------------------------------------
On May 29, 2019, 4:29 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70741/
> -----------------------------------------------------------
>
> (Updated May 29, 2019, 4:29 a.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9799
> https://issues.apache.org/jira/browse/MESOS-9799
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Switched the `volumes/secrets` isolator from using container
> pre-exec commands to using file operations. This allows us to
> reduce the number of forked child processes, consolidate code
> and leaves the `network/port_mapping` isolator as the last
> consumer of pre-exec commands.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto b2e35cbf01caea6c1e4f45c7b7d833bc7f065099
> src/common/protobuf_utils.hpp 8aa9aff71cbbd8e98573d0095cfc007cdea163bc
> src/common/protobuf_utils.cpp 870859de93b44a1debc6786a562b4bc28955ddab
> src/slave/containerizer/mesos/isolators/volume/secret.cpp 7a9bb82b23b35728408fb37bac53d79883c0a19f
> src/slave/containerizer/mesos/launch.cpp 0c482f46a97063133edfe29ae3c6a2721d29f6c6
>
>
> Diff: https://reviews.apache.org/r/70741/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70741: Adopted container file operations for
secrets volumes.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 5, 2019, 12:16 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 516 (patched)
> > <https://reviews.apache.org/r/70741/diff/2/?file=2147412#file2147412line524>
> >
> > Is it more explicit if you just name the operation `ContainerFileOperation::MV` or `ContainerFileOperation::MOVE` and implement it as such (rename + fallback) either here or in stout since you are essentially implementing how we use a `mv` command?
>
> James Peach wrote:
> `rename` vs. `move` seems pretty similar. To me, `rename` seemed clear so I prefer it. I didn't add a stout implementation because I didn't have the time to handle all the corner cases and write proper tests for a general utility.
I think people tend to map them to linux syscall or commands so renaming doesn't equal potentially copying and deleting?
Just my opinion. Still giving you the ship it.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70741/#review215696
-----------------------------------------------------------
On June 5, 2019, 1:44 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70741/
> -----------------------------------------------------------
>
> (Updated June 5, 2019, 1:44 a.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9799
> https://issues.apache.org/jira/browse/MESOS-9799
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Switched the `volumes/secrets` isolator from using container
> pre-exec commands to using file operations. This allows us to
> reduce the number of forked child processes, consolidate code
> and leaves the `network/port_mapping` isolator as the last
> consumer of pre-exec commands.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto b2e35cbf01caea6c1e4f45c7b7d833bc7f065099
> src/common/protobuf_utils.hpp 8aa9aff71cbbd8e98573d0095cfc007cdea163bc
> src/common/protobuf_utils.cpp 870859de93b44a1debc6786a562b4bc28955ddab
> src/slave/containerizer/mesos/isolators/volume/secret.cpp 7a9bb82b23b35728408fb37bac53d79883c0a19f
> src/slave/containerizer/mesos/launch.cpp 0c482f46a97063133edfe29ae3c6a2721d29f6c6
>
>
> Diff: https://reviews.apache.org/r/70741/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70741: Adopted container file operations for
secrets volumes.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70741/#review215696
-----------------------------------------------------------
src/common/protobuf_utils.hpp
Lines 100 (patched)
<https://reviews.apache.org/r/70741/#comment302508>
Why this? The paragraphs above are still about `validateProtobufUnion` right?
src/common/protobuf_utils.hpp
Lines 436 (patched)
<https://reviews.apache.org/r/70741/#comment302509>
One empty line between the last element and the ending `}`
src/slave/containerizer/mesos/isolators/volume/secret.cpp
Line 138 (original), 147 (patched)
<https://reviews.apache.org/r/70741/#comment302511>
Do you know why `launchInfo.mounts` was not used (and for the same reason it couldn't be used here)?
src/slave/containerizer/mesos/launch.cpp
Lines 516 (patched)
<https://reviews.apache.org/r/70741/#comment302510>
Is it more explicit if you just name the operation `ContainerFileOperation::MV` or `ContainerFileOperation::MOVE` and implement it as such (rename + fallback) either here or in stout since you are essentially implementing how we use a `mv` command?
- Jiang Yan Xu
On May 28, 2019, 9:29 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70741/
> -----------------------------------------------------------
>
> (Updated May 28, 2019, 9:29 p.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9799
> https://issues.apache.org/jira/browse/MESOS-9799
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Switched the `volumes/secrets` isolator from using container
> pre-exec commands to using file operations. This allows us to
> reduce the number of forked child processes, consolidate code
> and leaves the `network/port_mapping` isolator as the last
> consumer of pre-exec commands.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto b2e35cbf01caea6c1e4f45c7b7d833bc7f065099
> src/common/protobuf_utils.hpp 8aa9aff71cbbd8e98573d0095cfc007cdea163bc
> src/common/protobuf_utils.cpp 870859de93b44a1debc6786a562b4bc28955ddab
> src/slave/containerizer/mesos/isolators/volume/secret.cpp 7a9bb82b23b35728408fb37bac53d79883c0a19f
> src/slave/containerizer/mesos/launch.cpp 0c482f46a97063133edfe29ae3c6a2721d29f6c6
>
>
> Diff: https://reviews.apache.org/r/70741/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70741: Adopted container file operations for
secrets volumes.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70741/#review215610
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70741]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 29, 2019, 4:29 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70741/
> -----------------------------------------------------------
>
> (Updated May 29, 2019, 4:29 a.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9799
> https://issues.apache.org/jira/browse/MESOS-9799
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Switched the `volumes/secrets` isolator from using container
> pre-exec commands to using file operations. This allows us to
> reduce the number of forked child processes, consolidate code
> and leaves the `network/port_mapping` isolator as the last
> consumer of pre-exec commands.
>
>
> Diffs
> -----
>
> include/mesos/slave/containerizer.proto b2e35cbf01caea6c1e4f45c7b7d833bc7f065099
> src/common/protobuf_utils.hpp 8aa9aff71cbbd8e98573d0095cfc007cdea163bc
> src/common/protobuf_utils.cpp 870859de93b44a1debc6786a562b4bc28955ddab
> src/slave/containerizer/mesos/isolators/volume/secret.cpp 7a9bb82b23b35728408fb37bac53d79883c0a19f
> src/slave/containerizer/mesos/launch.cpp 0c482f46a97063133edfe29ae3c6a2721d29f6c6
>
>
> Diff: https://reviews.apache.org/r/70741/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>