You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2015/10/01 20:23:06 UTC
Re: Review Request 38747: Adding digest utilities
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101257
-----------------------------------------------------------
Sorry I haven't chimed in earlier. I made one comment earlier with a reference to a pending review <https://reviews.apache.org/r/34138/> but didn't look at the review closely. I also have a ticket https://issues.apache.org/jira/browse/MESOS-3191 create before.
I have a few questions, just to help me understand the context.
1) Since the motivation for this util is for computing and verifying docker image hash, how will we use it? Given that this implementation requires USE_SSL_SOCKET, do we want to tie the docker provisioner to the --enable-ssl flag? Or should we create another flag or make `libssl-dev` or `openssl-devel` a dependency by default? What is the plan?
2) What's the advantage of using openssl APIs directly? This implementation is obviously more complex than the alternative <https://reviews.apache.org/r/34138/>, which simply calls out to a few widely distributed system binaries on our supported platforms. (sha256sum sha512sum are part of GNU coreutils while shasum is on every mac). The linked review needs to address some comments but it's not far from ready for shipit (it's not a priority for us right now but you can take it if you like).
Thanks!
- Jiang Yan Xu
On Sept. 30, 2015, 1:11 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 1:11 p.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added SHA256, SHA512 implementation.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/digest.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/CMakeLists.txt a14b5b8fe22d3e75bef3c716ae7865ddaf132927
> 3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38747: Adding digest utilities
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Oct. 1, 2015, 11:23 a.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a reference to a pending review <https://reviews.apache.org/r/34138/> but didn't look at the review closely. I also have a ticket https://issues.apache.org/jira/browse/MESOS-3191 create before.
> >
> > I have a few questions, just to help me understand the context.
> >
> > 1) Since the motivation for this util is for computing and verifying docker image hash, how will we use it? Given that this implementation requires USE_SSL_SOCKET, do we want to tie the docker provisioner to the --enable-ssl flag? Or should we create another flag or make `libssl-dev` or `openssl-devel` a dependency by default? What is the plan?
> >
> > 2) What's the advantage of using openssl APIs directly? This implementation is obviously more complex than the alternative <https://reviews.apache.org/r/34138/>, which simply calls out to a few widely distributed system binaries on our supported platforms. (sha256sum sha512sum are part of GNU coreutils while shasum is on every mac). The linked review needs to address some comments but it's not far from ready for shipit (it's not a priority for us right now but you can take it if you like).
> >
> > Thanks!
>
> Jojy Varghese wrote:
> Thanks for feedback. To answer your questions:
>
> - We currently depend on SSL for docker remote registry client as thats the authentication type we support. The code exposes simple APIs that can be called to get digest of a string, file etc. The test file serves as examples of usage.
> - The advantage is that we can use the API without spawning a process. Else you can image the number of spawns for a docker image with many layers. In short efficiency. Another reason is that the review you pointed me to also depends on those utils to be available. So we still need a fallback when those utils are not available.
> - The code is open to adding more fallback implementations that can incorporate the method in https://reviews.apache.org/r/34138/. So we can still add those fallback. I would see this implementation as a superset and not a replacement for the code presented in https://reviews.apache.org/r/34138/.
>
> -jojy
Sorry for the delay due to my travel. I realize much work has already been done in this and we should probably still push this forward so I'll comment on the code separately but the following is still high-level:
I guess I am still not fully sold on the necessity of this when we already do the rest of the image provisioning via subprocess commands. (i.e., `cp`, `tar`). Hashing using these commands look natual to me, especially because it doesn't interact with the rest of the codebase in anyway. It maybe something easier to do for the task at hand. Some of the thoughts below are applicable if we adopt the native implementation as well.
## SSL
Ok I see that SSL is required for for docker (or more precicsely docker when pulling from the registry). But how are we enabling this dependency? IIUC currently `USE_SSL_SOCKET` and `--enable-ssl` are tied to libevent which has nothing to do with SHA. I can understand that in order to use docker registry I have to turn on libevent + SSL but if I just want to use the hashing utility I have think I should be forced to switch from libev to libevent. Maybe we should come up with another flag and preprocessor macro for this (use openssl headers). FWIW I think openssl may have expected common usage in the future to be a hard/unconditional dependency. Which is easier?
I am all for exposing simple and generic APIs for this utility but I think we are discussing the implementation details here.
## Overhead of spawning a process
We already call `tar` and perhaps `cp` in subprocesses for each layer so this is definitely not a bottleneck. :)
## Dependency on the availability of the binary
See my comments above. I think sha256sum and sha512sum on Linux and shasum on OSX are widely distributed (more common than the openssl headers). They appear to be as common as `tar` and `mount`.
## Subprocess as a fallback.
Sure they can coexist. I am merely talking about complexity and priority. If openssl if a hard dependency then the subprocess implementation probably doesn't have to exist. On the other hand, this native implementation does data reading and processing in a serial manner, I don't know if these binaries have any optimization. Note that for large binaries we are using SHAing takes close to a minute so any optimization would be great to have.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101257
-----------------------------------------------------------
On Oct. 7, 2015, 7:37 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2015, 7:37 p.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added SHA256, SHA512 implementation.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/digest.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/CMakeLists.txt a14b5b8fe22d3e75bef3c716ae7865ddaf132927
> 3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38747: Adding digest utilities
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Oct. 1, 2015, 11:23 a.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a reference to a pending review <https://reviews.apache.org/r/34138/> but didn't look at the review closely. I also have a ticket https://issues.apache.org/jira/browse/MESOS-3191 create before.
> >
> > I have a few questions, just to help me understand the context.
> >
> > 1) Since the motivation for this util is for computing and verifying docker image hash, how will we use it? Given that this implementation requires USE_SSL_SOCKET, do we want to tie the docker provisioner to the --enable-ssl flag? Or should we create another flag or make `libssl-dev` or `openssl-devel` a dependency by default? What is the plan?
> >
> > 2) What's the advantage of using openssl APIs directly? This implementation is obviously more complex than the alternative <https://reviews.apache.org/r/34138/>, which simply calls out to a few widely distributed system binaries on our supported platforms. (sha256sum sha512sum are part of GNU coreutils while shasum is on every mac). The linked review needs to address some comments but it's not far from ready for shipit (it's not a priority for us right now but you can take it if you like).
> >
> > Thanks!
>
> Jojy Varghese wrote:
> Thanks for feedback. To answer your questions:
>
> - We currently depend on SSL for docker remote registry client as thats the authentication type we support. The code exposes simple APIs that can be called to get digest of a string, file etc. The test file serves as examples of usage.
> - The advantage is that we can use the API without spawning a process. Else you can image the number of spawns for a docker image with many layers. In short efficiency. Another reason is that the review you pointed me to also depends on those utils to be available. So we still need a fallback when those utils are not available.
> - The code is open to adding more fallback implementations that can incorporate the method in https://reviews.apache.org/r/34138/. So we can still add those fallback. I would see this implementation as a superset and not a replacement for the code presented in https://reviews.apache.org/r/34138/.
>
> -jojy
>
> Jiang Yan Xu wrote:
> Sorry for the delay due to my travel. I realize much work has already been done in this and we should probably still push this forward so I'll comment on the code separately but the following is still high-level:
>
> I guess I am still not fully sold on the necessity of this when we already do the rest of the image provisioning via subprocess commands. (i.e., `cp`, `tar`). Hashing using these commands look natual to me, especially because it doesn't interact with the rest of the codebase in anyway. It maybe something easier to do for the task at hand. Some of the thoughts below are applicable if we adopt the native implementation as well.
>
> ## SSL
> Ok I see that SSL is required for for docker (or more precicsely docker when pulling from the registry). But how are we enabling this dependency? IIUC currently `USE_SSL_SOCKET` and `--enable-ssl` are tied to libevent which has nothing to do with SHA. I can understand that in order to use docker registry I have to turn on libevent + SSL but if I just want to use the hashing utility I have think I should be forced to switch from libev to libevent. Maybe we should come up with another flag and preprocessor macro for this (use openssl headers). FWIW I think openssl may have expected common usage in the future to be a hard/unconditional dependency. Which is easier?
>
> I am all for exposing simple and generic APIs for this utility but I think we are discussing the implementation details here.
>
> ## Overhead of spawning a process
> We already call `tar` and perhaps `cp` in subprocesses for each layer so this is definitely not a bottleneck. :)
>
> ## Dependency on the availability of the binary
> See my comments above. I think sha256sum and sha512sum on Linux and shasum on OSX are widely distributed (more common than the openssl headers). They appear to be as common as `tar` and `mount`.
>
> ## Subprocess as a fallback.
> Sure they can coexist. I am merely talking about complexity and priority. If openssl if a hard dependency then the subprocess implementation probably doesn't have to exist. On the other hand, this native implementation does data reading and processing in a serial manner, I don't know if these binaries have any optimization. Note that for large binaries we are using SHAing takes close to a minute so any optimization would be great to have.
>
> Jojy Varghese wrote:
> Thanks for the comments Yan, inspite of your busy schedule.
>
> To summarize the "YEAs" and "NAYs" for this solution:
>
> YEA:
> - Finer control of the hashing API. This allows errors to be fine grained.
> - No separate "process"
> - Still supports other methods if SSL not available.
> - Does not depend on external tools
>
> NAY:
> - Depends on SSL availability
> - Probably not optimum implementation
Thanks for the response! Specifically how would you like the enable it (re my comment about SSL)?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101257
-----------------------------------------------------------
On Oct. 12, 2015, 2:14 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2015, 2:14 p.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added SHA256, SHA512 implementation.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/digest.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/CMakeLists.txt a14b5b8fe22d3e75bef3c716ae7865ddaf132927
> 3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38747: Adding digest utilities
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Oct. 1, 2015, 6:23 p.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a reference to a pending review <https://reviews.apache.org/r/34138/> but didn't look at the review closely. I also have a ticket https://issues.apache.org/jira/browse/MESOS-3191 create before.
> >
> > I have a few questions, just to help me understand the context.
> >
> > 1) Since the motivation for this util is for computing and verifying docker image hash, how will we use it? Given that this implementation requires USE_SSL_SOCKET, do we want to tie the docker provisioner to the --enable-ssl flag? Or should we create another flag or make `libssl-dev` or `openssl-devel` a dependency by default? What is the plan?
> >
> > 2) What's the advantage of using openssl APIs directly? This implementation is obviously more complex than the alternative <https://reviews.apache.org/r/34138/>, which simply calls out to a few widely distributed system binaries on our supported platforms. (sha256sum sha512sum are part of GNU coreutils while shasum is on every mac). The linked review needs to address some comments but it's not far from ready for shipit (it's not a priority for us right now but you can take it if you like).
> >
> > Thanks!
>
> Jojy Varghese wrote:
> Thanks for feedback. To answer your questions:
>
> - We currently depend on SSL for docker remote registry client as thats the authentication type we support. The code exposes simple APIs that can be called to get digest of a string, file etc. The test file serves as examples of usage.
> - The advantage is that we can use the API without spawning a process. Else you can image the number of spawns for a docker image with many layers. In short efficiency. Another reason is that the review you pointed me to also depends on those utils to be available. So we still need a fallback when those utils are not available.
> - The code is open to adding more fallback implementations that can incorporate the method in https://reviews.apache.org/r/34138/. So we can still add those fallback. I would see this implementation as a superset and not a replacement for the code presented in https://reviews.apache.org/r/34138/.
>
> -jojy
>
> Jiang Yan Xu wrote:
> Sorry for the delay due to my travel. I realize much work has already been done in this and we should probably still push this forward so I'll comment on the code separately but the following is still high-level:
>
> I guess I am still not fully sold on the necessity of this when we already do the rest of the image provisioning via subprocess commands. (i.e., `cp`, `tar`). Hashing using these commands look natual to me, especially because it doesn't interact with the rest of the codebase in anyway. It maybe something easier to do for the task at hand. Some of the thoughts below are applicable if we adopt the native implementation as well.
>
> ## SSL
> Ok I see that SSL is required for for docker (or more precicsely docker when pulling from the registry). But how are we enabling this dependency? IIUC currently `USE_SSL_SOCKET` and `--enable-ssl` are tied to libevent which has nothing to do with SHA. I can understand that in order to use docker registry I have to turn on libevent + SSL but if I just want to use the hashing utility I have think I should be forced to switch from libev to libevent. Maybe we should come up with another flag and preprocessor macro for this (use openssl headers). FWIW I think openssl may have expected common usage in the future to be a hard/unconditional dependency. Which is easier?
>
> I am all for exposing simple and generic APIs for this utility but I think we are discussing the implementation details here.
>
> ## Overhead of spawning a process
> We already call `tar` and perhaps `cp` in subprocesses for each layer so this is definitely not a bottleneck. :)
>
> ## Dependency on the availability of the binary
> See my comments above. I think sha256sum and sha512sum on Linux and shasum on OSX are widely distributed (more common than the openssl headers). They appear to be as common as `tar` and `mount`.
>
> ## Subprocess as a fallback.
> Sure they can coexist. I am merely talking about complexity and priority. If openssl if a hard dependency then the subprocess implementation probably doesn't have to exist. On the other hand, this native implementation does data reading and processing in a serial manner, I don't know if these binaries have any optimization. Note that for large binaries we are using SHAing takes close to a minute so any optimization would be great to have.
Thanks for the comments Yan, inspite of your busy schedule.
To summarize the "YEAs" and "NAYs" for this solution:
YEA:
- Finer control of the hashing API. This allows errors to be fine grained.
- No separate "process"
- Still supports other methods if SSL not available.
- Does not depend on external tools
NAY:
- Depends on SSL availability
- Probably not optimum implementation
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101257
-----------------------------------------------------------
On Oct. 8, 2015, 2:37 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2015, 2:37 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added SHA256, SHA512 implementation.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/digest.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/CMakeLists.txt a14b5b8fe22d3e75bef3c716ae7865ddaf132927
> 3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38747: Adding digest utilities
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Oct. 1, 2015, 6:23 p.m., Jiang Yan Xu wrote:
> > Sorry I haven't chimed in earlier. I made one comment earlier with a reference to a pending review <https://reviews.apache.org/r/34138/> but didn't look at the review closely. I also have a ticket https://issues.apache.org/jira/browse/MESOS-3191 create before.
> >
> > I have a few questions, just to help me understand the context.
> >
> > 1) Since the motivation for this util is for computing and verifying docker image hash, how will we use it? Given that this implementation requires USE_SSL_SOCKET, do we want to tie the docker provisioner to the --enable-ssl flag? Or should we create another flag or make `libssl-dev` or `openssl-devel` a dependency by default? What is the plan?
> >
> > 2) What's the advantage of using openssl APIs directly? This implementation is obviously more complex than the alternative <https://reviews.apache.org/r/34138/>, which simply calls out to a few widely distributed system binaries on our supported platforms. (sha256sum sha512sum are part of GNU coreutils while shasum is on every mac). The linked review needs to address some comments but it's not far from ready for shipit (it's not a priority for us right now but you can take it if you like).
> >
> > Thanks!
Thanks for feedback. To answer your questions:
- We currently depend on SSL for docker remote registry client as thats the authentication type we support. The code exposes simple APIs that can be called to get digest of a string, file etc. The test file serves as examples of usage.
- The advantage is that we can use the API without spawning a process. Else you can image the number of spawns for a docker image with many layers. In short efficiency. Another reason is that the review you pointed me to also depends on those utils to be available. So we still need a fallback when those utils are not available.
- The code is open to adding more fallback implementations that can incorporate the method in https://reviews.apache.org/r/34138/. So we can still add those fallback. I would see this implementation as a superset and not a replacement for the code presented in https://reviews.apache.org/r/34138/.
-jojy
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38747/#review101257
-----------------------------------------------------------
On Sept. 30, 2015, 8:11 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 8:11 p.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added SHA256, SHA512 implementation.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am c764717d447da39f78a9c74a756e611b63a814e1
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/digest.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/CMakeLists.txt a14b5b8fe22d3e75bef3c716ae7865ddaf132927
> 3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38747/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>