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