> 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:

- 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

- Depends on SSL availability
- Probably not optimum implementation

- Jojy

This is an automatically generated e-mail. To reply, visit:

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

Reply via email to