Re: Review Request 39839: RegistryClient refactor: Changed getManifest interface
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39839/ --- (Updated Nov. 5, 2015, 3:50 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- rebased. Repository: mesos Description --- Replaced path and tag parameters with Image::Name Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359 src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8 Diff: https://reviews.apache.org/r/39839/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39840: RegistryClient refactor: pulled up streaming read
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39840/ --- (Updated Nov. 6, 2015, 6:56 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- safer version of saveblob Repository: mesos Description --- Made reading the stream from the PIPE common. Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 29d4d4d6d60ec634efb21b951cf20ff90f6ffed6 Diff: https://reviews.apache.org/r/39840/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39840: RegistryClient refactor: pulled up streaming read
> On Nov. 6, 2015, 6:05 p.m., Timothy Chen wrote: > > src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp, line > > 394 > > <https://reviews.apache.org/r/39840/diff/8/?file=1118149#file1118149line394> > > > > We used Shared<> instead of std::shared_ptr in our code base. dropping based on our discussion - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39840/#review105451 --- On Nov. 6, 2015, 6:56 p.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39840/ > --- > > (Updated Nov. 6, 2015, 6:56 p.m.) > > > Review request for mesos, Ben Mahler and Timothy Chen. > > > Repository: mesos > > > Description > --- > > Made reading the stream from the PIPE common. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp > 29d4d4d6d60ec634efb21b951cf20ff90f6ffed6 > > Diff: https://reviews.apache.org/r/39840/diff/ > > > Testing > --- > > make check. > > > Thanks, > > Jojy Varghese > >
Re: Review Request 39340: RegistryClient: Added streaming response read
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39340/ --- (Updated Nov. 6, 2015, 3:50 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- reviews Repository: mesos Description --- RegistryClient: Added streaming response read Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c Diff: https://reviews.apache.org/r/39340/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39840: RegistryClient refactor: pulled up streaming read
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39840/ --- (Updated Nov. 6, 2015, 3:50 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- rebased Repository: mesos Description --- Made reading the stream from the PIPE common. Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c Diff: https://reviews.apache.org/r/39840/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39340: RegistryClient: Added streaming response read
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39340/ --- (Updated Nov. 6, 2015, 5:15 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- review Repository: mesos Description --- RegistryClient: Added streaming response read Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c Diff: https://reviews.apache.org/r/39340/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39840: RegistryClient refactor: pulled up streaming read
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39840/ --- (Updated Nov. 6, 2015, 5:17 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- rebased Repository: mesos Description --- Made reading the stream from the PIPE common. Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c Diff: https://reviews.apache.org/r/39840/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38580/ --- (Updated Nov. 6, 2015, 5:36 p.m.) Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu. Changes --- rebased Repository: mesos Description --- Integrated remote puller with store. Tests will follow. Diffs (updated) - src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289 src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546 src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5 src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63 src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359 src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8 Diff: https://reviews.apache.org/r/38580/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39840: RegistryClient refactor: pulled up streaming read
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39840/ --- (Updated Nov. 6, 2015, 7:39 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- made saveBlob same as ```internal::_write ``` Repository: mesos Description --- Made reading the stream from the PIPE common. Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 29d4d4d6d60ec634efb21b951cf20ff90f6ffed6 Diff: https://reviews.apache.org/r/39840/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39156: RegistryClient refactor: changed getManifest interface
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39156/ --- (Updated Oct. 14, 2015, 4:05 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- RegistryClient refactor: changed getManifest interface Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp 1d3377e7462908246dfac90aa0c56314406529c9 src/slave/containerizer/provisioner/docker/registry_client.cpp 471783d88b73b62afacac3d7952ebb5d5f442097 src/tests/containerizer/provisioner_docker_tests.cpp 9c3c45a81be6398722a37911788e347a4e91cce8 Diff: https://reviews.apache.org/r/39156/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38579: Refactored registry client
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38579/ --- (Updated Oct. 14, 2015, 4:06 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- rebased. Repository: mesos Description --- - Cleanup and broke big methods into smaller chunks. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 471783d88b73b62afacac3d7952ebb5d5f442097 src/tests/containerizer/provisioner_docker_tests.cpp 9c3c45a81be6398722a37911788e347a4e91cce8 Diff: https://reviews.apache.org/r/38579/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39155: RegistryClient refactor: removed nested namespace references
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39155/ --- (Updated Oct. 14, 2015, 4:05 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- RegistryClient refactor: removed nested namespace references Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 471783d88b73b62afacac3d7952ebb5d5f442097 Diff: https://reviews.apache.org/r/39155/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39014: RegistryClient refactor: renamed ManifestResponse
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39014/ --- (Updated Oct. 14, 2015, 4:06 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased. Repository: mesos Description --- RegistryClient refactor: renamed ManifestResponse as per review comments. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp 1d3377e7462908246dfac90aa0c56314406529c9 src/slave/containerizer/provisioner/docker/registry_client.cpp 471783d88b73b62afacac3d7952ebb5d5f442097 src/tests/containerizer/provisioner_docker_tests.cpp 9c3c45a81be6398722a37911788e347a4e91cce8 Diff: https://reviews.apache.org/r/39014/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39068: RegistryClient refactor: Renamed fsLayerInfoList
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39068/ --- (Updated Oct. 14, 2015, 4:07 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased. Repository: mesos Description --- RegistryClient refactor: renamed fsLayerInfoList Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp 1d3377e7462908246dfac90aa0c56314406529c9 src/slave/containerizer/provisioner/docker/registry_client.cpp 471783d88b73b62afacac3d7952ebb5d5f442097 src/tests/containerizer/provisioner_docker_tests.cpp 9c3c45a81be6398722a37911788e347a4e91cce8 Diff: https://reviews.apache.org/r/39068/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39331: Support docker local store pull image simultaneously
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39331/#review102945 --- src/slave/containerizer/provisioner/docker/store.cpp (line 169) <https://reviews.apache.org/r/39331/#comment160771> Does this mean that any request to get an image A will always return the same image? I thought the idea was to prevent simultaneous downloads/fetching of the same image. - Jojy Varghese On Oct. 14, 2015, 10:54 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39331/ > --- > > (Updated Oct. 14, 2015, 10:54 p.m.) > > > Review request for mesos, Anand Mazumdar, Jojy Varghese, and Timothy Chen. > > > Bugs: MESOS-3736 > https://issues.apache.org/jira/browse/MESOS-3736 > > > Repository: mesos > > > Description > --- > > Support docker local store pull image simultaneously > > > Diffs > - > > src/slave/containerizer/provisioner/docker/metadata_manager.cpp > 2b2de5245bccbd01a856b214ac6525278d794537 > src/slave/containerizer/provisioner/docker/store.cpp > 637c97c0973bda492826803a962c99635647692d > src/tests/containerizer/provisioner_docker_tests.cpp > 9c3c45a81be6398722a37911788e347a4e91cce8 > > Diff: https://reviews.apache.org/r/39331/diff/ > > > Testing > --- > > make check (ubuntu14.04 + clang-3.6) > > > Thanks, > > Gilbert Song > >
Review Request 39340: RegistryClient: Added streaming response read
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39340/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- RegistryClient: Added streaming response read Diffs - src/slave/containerizer/provisioner/docker/registry_client.cpp 471783d88b73b62afacac3d7952ebb5d5f442097 Diff: https://reviews.apache.org/r/39340/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38747: Adding digest utilities
> On Oct. 13, 2015, 12:54 a.m., Jiang Yan Xu wrote: > > 3rdparty/libprocess/include/process/digest.hpp, line 71 > > <https://reviews.apache.org/r/38747/diff/14/?file=1092722#file1092722line71> > > > > I imagine `fn` is expensive, after all, it's cryptography. :) > > > > Serially doing read and compute seems to be suboptimal but I simply > > parallelize read and compute can cause OOM when read is faster than > > compute. Of couse this is a classic producer and consumer problem and we > > can use a queue. We probably don't want to make things too complicated here > > in a single review but again, this is the issue of reimplemting a generic > > and widely used utility: we don't get to leverage state of the art for > > free... > > > > Just a comment. Added comment on preliminary performance test. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38747/#review102302 --- On Oct. 13, 2015, 7:21 p.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38747/ > --- > > (Updated Oct. 13, 2015, 7:21 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 38580: Added docker registry RemotePuller
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38580/ --- (Updated Oct. 8, 2015, 2:37 a.m.) Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu. Changes --- rebased. Repository: mesos Description --- Integrated remote puller with store. Tests will follow. Diffs (updated) - src/Makefile.am e69892736b0edc8c264eaccd52a04d44d01f53ba src/slave/containerizer/provisioner/docker/local_puller.hpp 4574e8a04663482625d7b54f765741f221ec13e0 src/slave/containerizer/provisioner/docker/local_puller.cpp 4a0b7d11f013941084571f2d89d835a4668a3d8b src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION src/slave/containerizer/provisioner/docker/store.cpp cbb67686d45513f0395a0cf1bc5c43cb4935adae src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38580/diff/ Testing --- make check. Thanks, Jojy Varghese
Review Request 39112: RegistryClient refactor: fixed variable names
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39112/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- RegistryClient refactor: fixed variable names. This patch will serve as catch-all for any variable name related changes in the refactor. Diffs - src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39112/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38747: Adding digest utilities
--- 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. Changes --- replaced raw pointers with smart ptrs. Repository: mesos Description --- Added SHA256, SHA512 implementation. Diffs (updated) - 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 39340: RegistryClient: Added streaming response read
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39340/ --- (Updated Oct. 16, 2015, 12:15 a.m.) Review request for mesos and Ben Mahler. Changes --- Fixed io::write to use the correct write overload. Repository: mesos Description --- RegistryClient: Added streaming response read Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 471783d88b73b62afacac3d7952ebb5d5f442097 Diff: https://reviews.apache.org/r/39340/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39250: Puller refactor: moved untar to a common place
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39250/ --- (Updated Oct. 15, 2015, 10:54 p.m.) Review request for mesos and Timothy Chen. Repository: mesos Description --- Puller refactor: moved untar to a common place Diffs - src/slave/containerizer/provisioner/docker/local_puller.cpp 74d0e1ead7d630e65a7e75cb6123139b9197efef src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/39250/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39250: Puller refactor: moved untar to a common place
> On Oct. 15, 2015, 10:51 p.m., Timothy Chen wrote: > > Ship It! > > Timothy Chen wrote: > This doesn't seem to depend on anything from 38747. I'll be merging this > without the previous one. > > Timothy Chen wrote: > Btw please rebase this patch. The dependency is because RemotePuller has not yet landed. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39250/#review102827 --- On Oct. 15, 2015, 10:54 p.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39250/ > --- > > (Updated Oct. 15, 2015, 10:54 p.m.) > > > Review request for mesos and Timothy Chen. > > > Repository: mesos > > > Description > --- > > Puller refactor: moved untar to a common place > > > Diffs > - > > src/slave/containerizer/provisioner/docker/local_puller.cpp > 74d0e1ead7d630e65a7e75cb6123139b9197efef > src/slave/containerizer/provisioner/docker/puller.hpp > 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 > src/slave/containerizer/provisioner/docker/puller.cpp > cb05324689ffa26ce830b513e2d71b55517da3cb > src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/39250/diff/ > > > Testing > --- > > make check. > > > Thanks, > > Jojy Varghese > >
Re: Review Request 39250: Puller refactor: moved untar to a common place
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39250/ --- (Updated Oct. 16, 2015, 12:17 a.m.) Review request for mesos and Timothy Chen. Repository: mesos Description --- Puller refactor: moved untar to a common place Diffs - src/slave/containerizer/provisioner/docker/local_puller.cpp 74d0e1ead7d630e65a7e75cb6123139b9197efef src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/39250/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39250: Puller refactor: moved untar to a common place
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39250/ --- (Updated Oct. 16, 2015, 12:20 a.m.) Review request for mesos and Timothy Chen. Changes --- review comments. Repository: mesos Description --- Puller refactor: moved untar to a common place Diffs (updated) - src/slave/containerizer/provisioner/docker/local_puller.cpp 74d0e1ead7d630e65a7e75cb6123139b9197efef src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/39250/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38747: Adding digest utilities
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38747/ --- (Updated Oct. 16, 2015, 1:45 a.m.) Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen. Changes --- Added std::async for async dispatch Repository: mesos Description --- Added SHA256, SHA512 implementation. Diffs (updated) - 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 99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 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 39353: Fixed and added tests for docker image name parsing.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39353/#review102816 --- src/slave/containerizer/provisioner/docker/message.hpp (line 46) <https://reviews.apache.org/r/39353/#comment160586> How about handling parse errors ? Maybe change this to a Try? src/slave/containerizer/provisioner/docker/message.hpp (line 51) <https://reviews.apache.org/r/39353/#comment160592> Would this also allow @@ or @@@ ? Wondering if we can use a regular expression parser for parsing ? - Jojy Varghese On Oct. 15, 2015, 7:17 p.m., Ben Mahler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39353/ > --- > > (Updated Oct. 15, 2015, 7:17 p.m.) > > > Review request for mesos, Jojy Varghese and Timothy Chen. > > > Repository: mesos > > > Description > --- > > The existing docker image parsing did not work in many cases: if a digest is > present, a registry port is included. And it could not resolve the ambiguity > that exists between the registry and the repository. > > This follows docker's approach to parsing, which is a bit hacky due to the > way the registry and repository are disambiguated, see the comments. > > > Diffs > - > > include/mesos/mesos.proto f2ea4fc62507ce30b7a7981fd6a8c4749eb97190 > src/slave/containerizer/provisioner/docker/message.hpp > 6368bf4caec6f8c3ac97282f41c55381f920bce9 > src/slave/containerizer/provisioner/docker/message.proto > bbac2e6c1f40a7ca3f9227baca56a44cd43f58c6 > src/slave/containerizer/provisioner/docker/store.cpp > 637c97c0973bda492826803a962c99635647692d > src/tests/containerizer/provisioner_docker_tests.cpp > 9c3c45a81be6398722a37911788e347a4e91cce8 > > Diff: https://reviews.apache.org/r/39353/diff/ > > > Testing > --- > > Added a test. > > > Thanks, > > Ben Mahler > >
Re: Review Request 39340: RegistryClient: Added streaming response read
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39340/ --- (Updated Oct. 15, 2015, 9:23 p.m.) Review request for mesos and Ben Mahler. Changes --- Fixed total read count Repository: mesos Description --- RegistryClient: Added streaming response read Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 471783d88b73b62afacac3d7952ebb5d5f442097 Diff: https://reviews.apache.org/r/39340/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38747: Adding digest utilities
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38747/ --- (Updated Oct. 15, 2015, 9:24 p.m.) Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen. Changes --- reverted back to async loop; fixed read size. Repository: mesos Description --- Added SHA256, SHA512 implementation. Diffs (updated) - 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 99c9754b6a5c7ceb12808a782da9cb9fb3fc731e 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 39353: Fixed and added tests for docker image name parsing.
> On Oct. 15, 2015, 8:28 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioner/docker/message.hpp, line 46 > > <https://reviews.apache.org/r/39353/diff/1/?file=1099046#file1099046line46> > > > > How about handling parse errors ? Maybe change this to a Try? > > Ben Mahler wrote: > Agreed, did you see the TODO..? > > ``` > // TODO(bmahler): Validate based on docker's validation logic > // and return a Try here. > ``` Ah my bad. > On Oct. 15, 2015, 8:28 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioner/docker/message.hpp, line 51 > > <https://reviews.apache.org/r/39353/diff/1/?file=1099046#file1099046line51> > > > > Would this also allow @@ or @@@ ? Wondering if we can use a regular > > expression parser for parsing ? > > Ben Mahler wrote: > Yes, this won't reject invalid input (given this doesn't return a Try). > > The first step (this change) is to fix our code to accept valid input > (which we weren't doing for all cases!). I've left the TODO for validation > and rejecting bad input. Agreed. What do you think about using a RE parser? - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39353/#review102816 --- On Oct. 15, 2015, 7:17 p.m., Ben Mahler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39353/ > --- > > (Updated Oct. 15, 2015, 7:17 p.m.) > > > Review request for mesos, Jojy Varghese and Timothy Chen. > > > Repository: mesos > > > Description > --- > > The existing docker image parsing did not work in many cases: if a digest is > present, a registry port is included. And it could not resolve the ambiguity > that exists between the registry and the repository. > > This follows docker's approach to parsing, which is a bit hacky due to the > way the registry and repository are disambiguated, see the comments. > > > Diffs > - > > include/mesos/mesos.proto f2ea4fc62507ce30b7a7981fd6a8c4749eb97190 > src/slave/containerizer/provisioner/docker/message.hpp > 6368bf4caec6f8c3ac97282f41c55381f920bce9 > src/slave/containerizer/provisioner/docker/message.proto > bbac2e6c1f40a7ca3f9227baca56a44cd43f58c6 > src/slave/containerizer/provisioner/docker/store.cpp > 637c97c0973bda492826803a962c99635647692d > src/tests/containerizer/provisioner_docker_tests.cpp > 9c3c45a81be6398722a37911788e347a4e91cce8 > > Diff: https://reviews.apache.org/r/39353/diff/ > > > Testing > --- > > Added a test. > > > Thanks, > > Ben Mahler > >
Review Request 39456: Documentation: added containerizer internals
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39456/ --- Review request for mesos and Timothy Chen. Repository: mesos Description --- Documentation: added containerizer internals Diffs - docs/containerizer-internals.md PRE-CREATION Diff: https://reviews.apache.org/r/39456/diff/ Testing --- Thanks, Jojy Varghese
Re: Review Request 38579: Refactored registry client
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38579/ --- (Updated Oct. 9, 2015, 12:58 a.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- created more sub-patches. Repository: mesos Description --- - Cleanup and broke big methods into smaller chunks. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38579/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38747: Adding digest utilities
> 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 read
Re: Review Request 38747: Adding digest utilities
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38747/ --- (Updated Oct. 12, 2015, 9:14 p.m.) Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen. Changes --- review comments adderssed. Repository: mesos Description --- Added SHA256, SHA512 implementation. Diffs (updated) - 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
> On Oct. 12, 2015, 7:18 p.m., Jiang Yan Xu wrote: > > 3rdparty/libprocess/include/process/digest.hpp, line 61 > > <https://reviews.apache.org/r/38747/diff/13/?file=1088004#file1088004line61> > > > > Consider using `boost::shared_array data` (see io.cpp). Since c++11 standard way is to use shared_ptr with deleter, shared_ptr is used. I have changed the deleter to use the std default deleter. I am not passing smart pointer to the called function as its not required. > On Oct. 12, 2015, 7:18 p.m., Jiang Yan Xu wrote: > > 3rdparty/libprocess/include/process/digest.hpp, lines 73-75 > > <https://reviews.apache.org/r/38747/diff/13/?file=1088004#file1088004line73> > > > > What does this mean? EOF? Add a comment? > > > > Also, we don't need to special case this right? It will terminate at > > the next iteration right? You are right. The early return is just an optimization. > On Oct. 12, 2015, 7:18 p.m., Jiang Yan Xu wrote: > > 3rdparty/libprocess/include/process/digest.hpp, line 299 > > <https://reviews.apache.org/r/38747/diff/13/?file=1088004#file1088004line299> > > > > Would it be safer to just have the caller select from the `Digest` enum? This API was chosen to enable use cases like Docker which gets the SHA method as a string. This reduces the logic at client/caller site. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38747/#review101451 --- On Oct. 12, 2015, 9: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, 9: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 38580: Added docker registry RemotePuller
> On Oct. 1, 2015, 6:53 p.m., Timothy Chen wrote: > > src/slave/containerizer/provisioner/docker/remote_puller.cpp, line 227 > > <https://reviews.apache.org/r/38580/diff/7/?file=1088724#file1088724line227> > > > > I thought you wanted to move this to somewhere shared? We can create a > > base puller class and move this there. > > Jojy Varghese wrote: > Thought about it a little more and realized that the functionality of > "untar a tarball into a dierctory" should belong in a common place like > libprocess. Its not a function of puller but maybe a Tar class. > > Timothy Chen wrote: > Sure, are you going to do that? I think it's easier to put it in a shared > place for now since it's going to take longer to merge to libprocess IMO. Can i add that as a separate patch? - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38580/#review101261 ------- On Oct. 12, 2015, 9:35 p.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38580/ > --- > > (Updated Oct. 12, 2015, 9:35 p.m.) > > > Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu. > > > Repository: mesos > > > Description > --- > > Integrated remote puller with store. Tests will follow. > > > Diffs > - > > src/CMakeLists.txt 828dbb690841c561816811dfbb044aa3afead89d > src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e > src/slave/containerizer/provisioner/docker/local_puller.hpp > 4574e8a04663482625d7b54f765741f221ec13e0 > src/slave/containerizer/provisioner/docker/local_puller.cpp > 74d0e1ead7d630e65a7e75cb6123139b9197efef > src/slave/containerizer/provisioner/docker/puller.hpp > 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 > src/slave/containerizer/provisioner/docker/puller.cpp > cb05324689ffa26ce830b513e2d71b55517da3cb > src/slave/containerizer/provisioner/docker/registry_client.hpp > fdb68b675582f603ffb3e96f31c1c9405e238567 > src/slave/containerizer/provisioner/docker/registry_client.cpp > 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 > src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION > src/slave/containerizer/provisioner/docker/store.cpp > 637c97c0973bda492826803a962c99635647692d > src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 > src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 > src/tests/containerizer/provisioner_docker_tests.cpp > d895eb9d0723e52cff8b21ef2deeaef1911d019c > > Diff: https://reviews.apache.org/r/38580/diff/ > > > Testing > --- > > make check. > > > Thanks, > > Jojy Varghese > >
Re: Review Request 39250: Puller refactor: moved untar to a common place
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39250/ --- (Updated Oct. 13, 2015, 12:37 a.m.) Review request for mesos and Timothy Chen. Changes --- moved more untar code to use common code. Repository: mesos Description --- Puller refactor: moved untar to a common place Diffs (updated) - src/slave/containerizer/provisioner/docker/local_puller.cpp 74d0e1ead7d630e65a7e75cb6123139b9197efef src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/39250/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38580/ --- (Updated Oct. 12, 2015, 9:35 p.m.) Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu. Changes --- Added to cmake. Repository: mesos Description --- Integrated remote puller with store. Tests will follow. Diffs (updated) - src/CMakeLists.txt 828dbb690841c561816811dfbb044aa3afead89d src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e src/slave/containerizer/provisioner/docker/local_puller.hpp 4574e8a04663482625d7b54f765741f221ec13e0 src/slave/containerizer/provisioner/docker/local_puller.cpp 74d0e1ead7d630e65a7e75cb6123139b9197efef src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION src/slave/containerizer/provisioner/docker/store.cpp 637c97c0973bda492826803a962c99635647692d src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38580/diff/ Testing --- make check. Thanks, Jojy Varghese
Review Request 39250: Puller refactor: moved untar to a common place
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39250/ --- Review request for mesos and Timothy Chen. Repository: mesos Description --- Puller refactor: moved untar to a common place Diffs - src/slave/containerizer/provisioner/docker/local_puller.cpp 74d0e1ead7d630e65a7e75cb6123139b9197efef src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/39250/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38747: Adding digest utilities
> On Oct. 13, 2015, 12:54 a.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, lines 85-120
> > <https://reviews.apache.org/r/38747/diff/14/?file=1092722#file1092722line85>
> >
> > These templates (here and below) have different levels of
> > specializations, traits, typedefs in various places, which is hard to
> > understand.
> >
> > Do you think the following is simpler?
> >
> > For each digest type, there is one **low-level** implementation, i.e.,
> > Init, Update and Final are called separatedly but they expose one common
> > interface (without digest-type specific) arguments.
> >
> > ```
> > class DigestImpl
> > {
> > public:
> > void init() == 0;
> > int update(const void*, size_t) == 0;
> > int final(uint8_t*) == 0;
> > static Try<DigestImpl*> create(...);
> > };
> > ```
> >
> > You have a low implementation for each type that inherits this
> > interface and encapsulates its specific context variables in its member
> > variables. The low-level implementation should be simply calling openssl
> > APIs so it should be short and has no more redundant code among different
> > implementations than specializations in different places and this
> > consolidates handling of specific digest-types in one single place.
> >
> > The high-level DigestUtil implementation or simply, the static generic
> > method implementation can just create a low-level impl class and you can
> > put common logic there.
> >
> > What do you think?
I should probably try to reason about the choice of templates (as opposed to
runtime polymorphism)
SSL digest API can be seen as "logic" and "structure". The logic is the same
for all SHAs. Only structure is different. The difference in structure can be
represented as template traits. This allows the difference between SHAs to be
viewed as *declarative* (as opposed to *procedural*).
This has following advantages:
1. Any new SHA digest can be added by just adding the type traits
*declaratively*. For example, to add SHA224, we just need to add the following
lines:
```
template <>
struct DigestTypeTraits
{
typedef SHA224_CTX ctx_type;
static const size_t digest_length = SSHA224_DIGEST_LENGTH;
};
template <>
DigestFunctionTraits::Init_fn
DigestFunctionTraits::Init = SHA224_Init;
template <>
DigestFunctionTraits::Update_fn
DigestFunctionTraits::Update = SHA224_Update;
template <>
DigestFunctionTraits::Final_fn
DigestFunctionTraits::Final = SHA224_Final;
```
If you notice all of the above new code is declarative and does not add any new
logic. This is possible because we have abstracted out the difference between
SHA implementations into templates.
2. At the call site, clients can call digests simply by:
```
DigestUtil::digest(string)
```
This avoids runtime polymorphism.
> On Oct. 13, 2015, 12:54 a.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/include/process/digest.hpp, lines 159-160
> > <https://reviews.apache.org/r/38747/diff/14/?file=1092722#file1092722line159>
> >
> > About templatization. I previously commented that the implementation
> > should be put in the CPP file.
> >
> > You use templates but your interafce is not templatized and used only
> > by this single component. Can't all the templates just be in the same
> > source file as the caller so that the header only has the API declarations?
> >
> > This of course becomes a moot point is we don't use template as I was
> > suggesting above.
One of the reasons the templates are in header file i
Re: Review Request 38580: Added docker registry RemotePuller
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38580/ --- (Updated Oct. 5, 2015, 9:03 p.m.) Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu. Changes --- Added simple puller test. Repository: mesos Description --- Integrated remote puller with store. Tests will follow. Diffs (updated) - src/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 src/slave/containerizer/provisioner/docker/local_puller.hpp 4574e8a04663482625d7b54f765741f221ec13e0 src/slave/containerizer/provisioner/docker/local_puller.cpp 4a0b7d11f013941084571f2d89d835a4668a3d8b src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 src/slave/containerizer/provisioner/docker/registry_client.cpp c2040b48ea43fdb29766994c244273d3fa9ee3cd src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION src/slave/containerizer/provisioner/docker/store.cpp cbb67686d45513f0395a0cf1bc5c43cb4935adae src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38580/diff/ Testing --- make check. Thanks, Jojy Varghese
Review Request 39016: RegistryClient refactor: refactored lambdas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39016/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- RegistryClient refactor: refactored lambdas as per review comments. Diffs - src/slave/containerizer/provisioner/docker/registry_client.cpp c2040b48ea43fdb29766994c244273d3fa9ee3cd Diff: https://reviews.apache.org/r/39016/diff/ Testing --- Make check. Thanks, Jojy Varghese
Review Request 39015: RegistryClient refactor: expanded abbreviated names.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39015/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- RegistryClient refactor: expanded abbreviated names. Diffs - src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 src/slave/containerizer/provisioner/docker/registry_client.cpp c2040b48ea43fdb29766994c244273d3fa9ee3cd Diff: https://reviews.apache.org/r/39015/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38941: Moved structs outside RegistryClient
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38941/ --- (Updated Oct. 5, 2015, 9:01 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- RegistryClient refactor: moved structs outside RegistryClient Repository: mesos Description --- Moved: - ManifestResponse - FilesystemLayerInfo Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 src/slave/containerizer/provisioner/docker/registry_client.cpp c2040b48ea43fdb29766994c244273d3fa9ee3cd src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38941/diff/ Testing --- make check. Thanks, Jojy Varghese
Review Request 39013: RegistryClient refactor: Fixed comments style.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39013/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- RegistryClient refactor: Fixed comments style. Diffs - src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 src/slave/containerizer/provisioner/docker/registry_client.cpp c2040b48ea43fdb29766994c244273d3fa9ee3cd Diff: https://reviews.apache.org/r/39013/diff/ Testing --- make check. Thanks, Jojy Varghese
Review Request 39014: RegistryClient refactor: renamed ManifestResponse
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39014/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- RegistryClient refactor: renamed ManifestResponse as per review comments. Diffs - src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 src/slave/containerizer/provisioner/docker/registry_client.cpp c2040b48ea43fdb29766994c244273d3fa9ee3cd src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39014/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39014: RegistryClient refactor: renamed ManifestResponse
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39014/ --- (Updated Oct. 7, 2015, 12:16 a.m.) Review request for mesos and Ben Mahler. Changes --- separating out other changes. Repository: mesos Description --- RegistryClient refactor: renamed ManifestResponse as per review comments. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39014/diff/ Testing --- make check. Thanks, Jojy Varghese
Review Request 39068: RegistryClient refactor: Renamed fsLayerInfoList
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39068/ --- Review request for mesos. Repository: mesos Description --- RegistryClient refactor: Renamed fsLayerInfoList Diffs - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39068/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38941: Moved structs outside RegistryClient
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38941/ --- (Updated Oct. 7, 2015, 12:19 a.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- rebased after reordering patches. Repository: mesos Description --- Moved: - ManifestResponse - FilesystemLayerInfo Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38941/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39068: RegistryClient refactor: Renamed fsLayerInfoList
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39068/ --- (Updated Oct. 7, 2015, 12:39 a.m.) Review request for mesos and Ben Mahler. Changes --- fixed braced initializer Repository: mesos Description (updated) --- RegistryClient refactor: renamed fsLayerInfoList Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39068/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39015: RegistryClient refactor: expanded abbreviated names.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39015/ --- (Updated Oct. 7, 2015, 12:41 a.m.) Review request for mesos and Ben Mahler. Changes --- rebased after updating patches. Repository: mesos Description --- RegistryClient refactor: expanded abbreviated names. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 Diff: https://reviews.apache.org/r/39015/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39017: RegistryClient refactor: encapsulated Manifest
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39017/ --- (Updated Oct. 7, 2015, 12:41 a.m.) Review request for mesos and Ben Mahler. Changes --- rebased after updating patches. Repository: mesos Description --- Wrapped Manifest with ManifestResponse. getManifest returns the encapsulated ManifestResponse. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39017/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39016: RegistryClient refactor: refactored lambdas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39016/ --- (Updated Oct. 7, 2015, 12:41 a.m.) Review request for mesos and Ben Mahler. Changes --- rebased after updating patches. Repository: mesos Description --- RegistryClient refactor: refactored lambdas as per review comments. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 Diff: https://reviews.apache.org/r/39016/diff/ Testing --- Make check. Thanks, Jojy Varghese
Re: Review Request 38941: Moved structs outside RegistryClient
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38941/ --- (Updated Oct. 7, 2015, 12:40 a.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- rebased after updating patches. Repository: mesos Description --- Moved: - ManifestResponse - FilesystemLayerInfo Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38941/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39053: RegistryClient refactor: priv method const'ness
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39053/ --- (Updated Oct. 7, 2015, 12:42 a.m.) Review request for mesos and Ben Mahler. Changes --- rebased after updating patches. Repository: mesos Description --- RegistryClient refactor: priv method const'ness Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 Diff: https://reviews.apache.org/r/39053/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39014: RegistryClient refactor: renamed ManifestResponse
> On Oct. 6, 2015, 7:57 p.m., Ben Mahler wrote: > > src/slave/containerizer/provisioner/docker/registry_client.hpp, line 67 > > <https://reviews.apache.org/r/39014/diff/1/?file=1091227#file1091227line67> > > > > Please let's avoid conflating independent changes in these patches, as > > it makes it much easier to go through review when we're doing one thing at > > a time. > > > > We should pull out naming cleanup into a seperate patch, it looks like > > there is a lot of naming cleanup we need to do on these files outside of > > just fsLayers. Also, how about s/fsLayers/layers/? I would like to keep fsLayers as it reflects the language of manifest. > On Oct. 6, 2015, 7:57 p.m., Ben Mahler wrote: > > src/tests/containerizer/provisioner_docker_tests.cpp, line 67 > > <https://reviews.apache.org/r/39014/diff/1/?file=1091229#file1091229line67> > > > > Can you do a sweep to remove all of the namespace aliases? If you want > > to pull them out of the RegistryClient let's use another patch. Its already removed in a later patch in the series (https://reviews.apache.org/r/38941) > On Oct. 6, 2015, 7:57 p.m., Ben Mahler wrote: > > src/tests/containerizer/provisioner_docker_tests.cpp, line 429 > > <https://reviews.apache.org/r/39014/diff/1/?file=1091229#file1091229line429> > > > > In general please try to use succict, non-redundant names: > > > > s/manifestResponseFuture/manifest > > > > It's clear here that this is a future from the type and now that we've > > removed response from the type we should remove it from the name as well. The idea was to make it obvious 50 lines down from initialization. I can change it and create another patch for replacing all xxxFuture with xxx for variable names. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39014/#review101681 --- On Oct. 5, 2015, 8:57 p.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39014/ > --- > > (Updated Oct. 5, 2015, 8:57 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos > > > Description > --- > > RegistryClient refactor: renamed ManifestResponse as per review comments. > > > Diffs > - > > src/slave/containerizer/provisioner/docker/registry_client.hpp > 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 > src/slave/containerizer/provisioner/docker/registry_client.cpp > c2040b48ea43fdb29766994c244273d3fa9ee3cd > src/tests/containerizer/provisioner_docker_tests.cpp > d895eb9d0723e52cff8b21ef2deeaef1911d019c > > Diff: https://reviews.apache.org/r/39014/diff/ > > > Testing > --- > > make check. > > > Thanks, > > Jojy Varghese > >
Review Request 39197: Provider tests: minor style fixes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39197/ --- Review request for mesos and Timothy Chen. Repository: mesos Description --- Provider tests: minor style fixes. Diffs - src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39197/diff/ Testing --- make check. Thanks, Jojy Varghese
Review Request 39196: Puller tests: removed extraneous sandbox directory
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39196/ --- Review request for mesos and Timothy Chen. Repository: mesos Description --- Puller tests: removed extraneous sandbox directory Diffs - src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39196/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39015: RegistryClient refactor: expanded abbreviated names.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39015/ --- (Updated Oct. 13, 2015, 7:15 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- RegistryClient refactor: expanded abbreviated names. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 Diff: https://reviews.apache.org/r/39015/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39017: RegistryClient refactor: encapsulated Manifest
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39017/ --- (Updated Oct. 13, 2015, 7:16 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- Wrapped Manifest with ManifestResponse. getManifest returns the encapsulated ManifestResponse. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39017/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39155: RegistryClient refactor: removed nested namespace references
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39155/ --- (Updated Oct. 13, 2015, 7:17 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- RegistryClient refactor: removed nested namespace references Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 Diff: https://reviews.apache.org/r/39155/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39112: RegistryClient refactor: fixed variable names
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39112/ --- (Updated Oct. 13, 2015, 7:17 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- RegistryClient refactor: fixed variable names. This patch will serve as catch-all for any variable name related changes in the refactor. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39112/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39053: RegistryClient refactor: priv method const'ness
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39053/ --- (Updated Oct. 13, 2015, 7:16 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- RegistryClient refactor: priv method const'ness Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 Diff: https://reviews.apache.org/r/39053/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39014: RegistryClient refactor: renamed ManifestResponse
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39014/ --- (Updated Oct. 13, 2015, 7:18 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- RegistryClient refactor: renamed ManifestResponse as per review comments. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39014/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38579: Refactored registry client
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38579/ --- (Updated Oct. 13, 2015, 7:18 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- rebased Repository: mesos Description --- - Cleanup and broke big methods into smaller chunks. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38579/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38747: Adding digest utilities
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38747/ --- (Updated Oct. 13, 2015, 7:21 p.m.) Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen. Repository: mesos Description --- Added SHA256, SHA512 implementation. Diffs (updated) - 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 39068: RegistryClient refactor: Renamed fsLayerInfoList
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39068/ --- (Updated Oct. 13, 2015, 7:15 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- RegistryClient refactor: renamed fsLayerInfoList Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39068/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38941: Moved structs outside RegistryClient
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38941/ --- (Updated Oct. 13, 2015, 7:15 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- rebased Repository: mesos Description --- Moved: - ManifestResponse - FilesystemLayerInfo Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38941/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39156: RegistryClient refactor: changed getManifest interface
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39156/ --- (Updated Oct. 13, 2015, 7:18 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased Repository: mesos Description --- RegistryClient refactor: changed getManifest interface Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/39156/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38580/ --- (Updated Oct. 13, 2015, 7:19 p.m.) Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu. Changes --- rebased Repository: mesos Description --- Integrated remote puller with store. Tests will follow. Diffs (updated) - src/CMakeLists.txt 536a99f3dc2f48e6002e70b605598f4d218a4324 src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e src/slave/containerizer/provisioner/docker/local_puller.hpp 4574e8a04663482625d7b54f765741f221ec13e0 src/slave/containerizer/provisioner/docker/local_puller.cpp 74d0e1ead7d630e65a7e75cb6123139b9197efef src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION src/slave/containerizer/provisioner/docker/store.cpp 637c97c0973bda492826803a962c99635647692d src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38580/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39250: Puller refactor: moved untar to a common place
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39250/ --- (Updated Oct. 13, 2015, 7:19 p.m.) Review request for mesos and Timothy Chen. Changes --- formatting Repository: mesos Description --- Puller refactor: moved untar to a common place Diffs (updated) - src/slave/containerizer/provisioner/docker/local_puller.cpp 74d0e1ead7d630e65a7e75cb6123139b9197efef src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/39250/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39250: Puller refactor: moved untar to a common place
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39250/ --- (Updated Oct. 14, 2015, 12:45 a.m.) Review request for mesos and Timothy Chen. Changes --- fixed order of arguments to untar. Repository: mesos Description --- Puller refactor: moved untar to a common place Diffs (updated) - src/slave/containerizer/provisioner/docker/local_puller.cpp 74d0e1ead7d630e65a7e75cb6123139b9197efef src/slave/containerizer/provisioner/docker/puller.hpp 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 src/slave/containerizer/provisioner/docker/puller.cpp cb05324689ffa26ce830b513e2d71b55517da3cb src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION Diff: https://reviews.apache.org/r/39250/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38579: Refactored registry client
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38579/ --- (Updated Oct. 9, 2015, 6:36 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- rebased. Repository: mesos Description --- - Cleanup and broke big methods into smaller chunks. Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38579/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 39184: RegistryClient refactor: reordered ctor parameters
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39184/ --- (Updated Oct. 9, 2015, 6:35 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased after reordering patches. Repository: mesos Description --- RegistryClient refactor: reordered ctor parameters Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 Diff: https://reviews.apache.org/r/39184/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38443: Added layerid information to ManifestResponse
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38443/ --- (Updated Oct. 9, 2015, 6:34 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Changes --- Split change into small patches Repository: mesos Description --- Added layerid information to ManifestResponse Diffs (updated) - src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567 src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c Diff: https://reviews.apache.org/r/38443/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
> On Oct. 1, 2015, 6:53 p.m., Timothy Chen wrote: > > src/slave/containerizer/provisioner/docker/remote_puller.cpp, line 227 > > <https://reviews.apache.org/r/38580/diff/7/?file=1088724#file1088724line227> > > > > I thought you wanted to move this to somewhere shared? We can create a > > base puller class and move this there. > > Jojy Varghese wrote: > Thought about it a little more and realized that the functionality of > "untar a tarball into a dierctory" should belong in a common place like > libprocess. Its not a function of puller but maybe a Tar class. > > Timothy Chen wrote: > Sure, are you going to do that? I think it's easier to put it in a shared > place for now since it's going to take longer to merge to libprocess IMO. > > Jojy Varghese wrote: > Can i add that as a separate patch? https://reviews.apache.org/r/39250 - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38580/#review101261 --- On Oct. 12, 2015, 9:35 p.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38580/ > --- > > (Updated Oct. 12, 2015, 9:35 p.m.) > > > Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu. > > > Repository: mesos > > > Description > --- > > Integrated remote puller with store. Tests will follow. > > > Diffs > - > > src/CMakeLists.txt 828dbb690841c561816811dfbb044aa3afead89d > src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e > src/slave/containerizer/provisioner/docker/local_puller.hpp > 4574e8a04663482625d7b54f765741f221ec13e0 > src/slave/containerizer/provisioner/docker/local_puller.cpp > 74d0e1ead7d630e65a7e75cb6123139b9197efef > src/slave/containerizer/provisioner/docker/puller.hpp > 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 > src/slave/containerizer/provisioner/docker/puller.cpp > cb05324689ffa26ce830b513e2d71b55517da3cb > src/slave/containerizer/provisioner/docker/registry_client.hpp > fdb68b675582f603ffb3e96f31c1c9405e238567 > src/slave/containerizer/provisioner/docker/registry_client.cpp > 4931ae8869a697b1e9d8d4cbc0a871e7cd506285 > src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION > src/slave/containerizer/provisioner/docker/store.cpp > 637c97c0973bda492826803a962c99635647692d > src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 > src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 > src/tests/containerizer/provisioner_docker_tests.cpp > d895eb9d0723e52cff8b21ef2deeaef1911d019c > > Diff: https://reviews.apache.org/r/38580/diff/ > > > Testing > --- > > make check. > > > Thanks, > > Jojy Varghese > >
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 8, 2015, 9:40 p.m.) Review request for mesos and Timothy Chen. Changes --- error message fix Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 8, 2015, 9:02 p.m.) Review request for mesos and Timothy Chen. Changes --- minor formatting changes Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 443-472 https://reviews.apache.org/r/36106/diff/1/?file=997646#file997646line443 Thanks! (1) Do you mind updating my TODO on cgroups::stat() to reflect that cpuacct::stat is implemented? (2) Can we just make this a simple struct with two non-const fields and remove the parse method and getters? Keep it simple and small, if we want immutability we can just have a 'const Stat' rather than forcing it on the caller :) (3) Any reason not to use 'Duration' for these fields? Jojy Varghese wrote: 1) Absolutely I can. 2) I wanted to reflect the semantics of the stats call. When you make a stats call, the data you get is immutable. By forcing external const, it would imply that the value is immutable. 3) Duration is a period ( i think) and the values here are absolute values derived from ticks. Joris Van Remoortere wrote: Regarding 3): Duration indeed represents an amount of time, rather than a specific point in time. I believe this is what we intend to represent in this code as well, right? The comments for your functions suggest an amount of time rather than a point in time. Regarding 1): I am working on another patch that addresses the containerizer (docker) using this API. I will also change the cpushare code along with that patch. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 --- On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1266 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1266 I think you reordered things here that used to be in _usage. We used to first check the containers map, then check is it destroying, that look at the pid and finally passing that on. Here the order is now pid check, and some reason skip the map check to get it but then checking it later on. I think we should go back to the exact same order as before unless you have reasons to do this change. The thought was as follows: - We called inspect to get the pid. So first we check for a pid. Then we set the pid for the container (if the container exists). Then we call collectUsage to collect stats. - The check for DESTROYING is already being centralized in the collectUsage lambda. On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1272 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1272 If you like to a leave a comment why we called this function we should probably comment at the place we call it not in the function (before docker-inspect). also s/didnt/didn't/g Wanted to emphasize the logic of why we need to update the pid there (and not say in collectUSage). On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1342 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1342 This seems too verbose being LOG(INFO). And what value do you think we have here logging this? I can change it to LOG(DEBUG). Wanted to log there to help us debug. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91342 --- On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1266 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1266 I think you reordered things here that used to be in _usage. We used to first check the containers map, then check is it destroying, that look at the pid and finally passing that on. Here the order is now pid check, and some reason skip the map check to get it but then checking it later on. I think we should go back to the exact same order as before unless you have reasons to do this change. Jojy Varghese wrote: The thought was as follows: - We called inspect to get the pid. So first we check for a pid. Then we set the pid for the container (if the container exists). Then we call collectUsage to collect stats. - The check for DESTROYING is already being centralized in the collectUsage lambda. wanted to point out that we are not skipping the check for contanier at all. We pick the pid from the Docker::Container object being passed in by the then continuation. We still check the existence of container in the collection (member variable collection_). - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91342 --- On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 10, 2015, 8:47 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Changes --- review comments @benm Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 10, 2015, 10:46 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Changes --- code review @benm Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 10:47 p.m.) Review request for mesos and Timothy Chen. Changes --- review comments @tim Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 11:03 p.m.) Review request for mesos and Timothy Chen. Changes --- review comments @tim Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description (updated) --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 9, 2015, 7:36 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 438-447 https://reviews.apache.org/r/36106/diff/4/?file=1000826#file1000826line438 What is the plan for introducing javadoc comments? cgroups.hpp seems like a reasonable candidate, but we should avoid inconsistent comment formatting within a file :( I'd propose commenting in the existing style, and following up by doing a javadoc formatting sweep across the file, if you're interested. Sound good? I was explicitly asked to do this way for now (see above reviews). I can remove it but will be conflicting with the other reviewer. On July 9, 2015, 7:36 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 450-455 https://reviews.apache.org/r/36106/diff/4/?file=1000826#file1000826line450 Why are we documenting the cpuacct.stat file format here? The caller should only cares about the user and system times, not the format of the underlying file :) I thought extra information about where that data come from will help. On July 9, 2015, 7:36 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 458-466 https://reviews.apache.org/r/36106/diff/4/?file=1000826#file1000826line458 Are these kinds of comments providing any value? Just for serving doxygen. Not sure what else I could have added. Sugestions are welcome. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review91168 --- On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Changes --- review comments @tim Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
On July 8, 2015, 9:57 p.m., Timothy Chen wrote: src/slave/containerizer/docker.hpp, line 230 https://reviews.apache.org/r/36326/diff/3/?file=1002973#file1002973line230 I dont' think we need to expose this. Since the method is a bit long, kept it but replaced other usage methods with lamdas. On July 8, 2015, 9:57 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1239 https://reviews.apache.org/r/36326/diff/3/?file=1002974#file1002974line1239 It's probably not going to be used outside of usage, so I think it's safe to inline this in _usage, and if you do want a seperate method this probably don't need to be exposed too. same comments as above. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91005 --- On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 40285: Changed untar process to pipe STDERR.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40285/ --- (Updated Nov. 17, 2015, 6:41 a.m.) Review request for mesos and Timothy Chen. Changes --- fixed isReady Repository: mesos Description --- By piping stderr to logs, it would be easier to debug problems with untar. Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/puller.cpp 13f5e2877f4d7951e79ba07073a42848217604b3 Diff: https://reviews.apache.org/r/40285/diff/ Testing --- make check; Thanks, Jojy Varghese
Re: Review Request 40285: Changed untar process to pipe STDERR.
> On Nov. 17, 2015, 4:49 a.m., Timothy Chen wrote: > > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 107 > > <https://reviews.apache.org/r/40285/diff/5/?file=1127825#file1127825line107> > > > > You should also include or at least log why we couldn't io::read right? I didnt add that in the error message because the untar error message will then have io::read context. Maybe i can log it? - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40285/#review106794 --- On Nov. 17, 2015, 6:41 a.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40285/ > --- > > (Updated Nov. 17, 2015, 6:41 a.m.) > > > Review request for mesos and Timothy Chen. > > > Repository: mesos > > > Description > --- > > By piping stderr to logs, it would be easier to debug problems with untar. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/puller.cpp > 13f5e2877f4d7951e79ba07073a42848217604b3 > > Diff: https://reviews.apache.org/r/40285/diff/ > > > Testing > --- > > make check; > > > Thanks, > > Jojy Varghese > >
Re: Review Request 40285: Changed untar process to pipe STDERR.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40285/ --- (Updated Nov. 17, 2015, 6:57 a.m.) Review request for mesos and Timothy Chen. Changes --- better logs. Repository: mesos Description --- By piping stderr to logs, it would be easier to debug problems with untar. Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/puller.cpp 13f5e2877f4d7951e79ba07073a42848217604b3 Diff: https://reviews.apache.org/r/40285/diff/ Testing --- make check; Thanks, Jojy Varghese
Re: Review Request 37200: Refactored DockerImage struct to store a list of layer ids instead of linked list of DockerLayers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37200/#review96561 --- src/slave/containerizer/provisioners/docker.hpp (line 61) https://reviews.apache.org/r/37200/#comment152075 Would a factory method be more apporoprate here(create)? What if the string passed in is arbitary? src/slave/containerizer/provisioners/docker.hpp https://reviews.apache.org/r/37200/#comment152079 Needs explicit on the constructor? src/slave/containerizer/provisioners/docker.hpp (line 62) https://reviews.apache.org/r/37200/#comment152078 Ctor too long to be eligible for inlined in header file. src/slave/containerizer/provisioners/docker.hpp (lines 66 - 68) https://reviews.apache.org/r/37200/#comment152073 Can avoid else block as Option state is default constructed to None. src/slave/containerizer/provisioners/docker.hpp (line 69) https://reviews.apache.org/r/37200/#comment152077 No need for this variable. Could be folded into the if condition. src/slave/containerizer/provisioners/docker.cpp (line 189) https://reviews.apache.org/r/37200/#comment152081 Are we always over writing existing provisioned containers? src/slave/containerizer/provisioners/docker.cpp (line 199) https://reviews.apache.org/r/37200/#comment152082 Use explicit capture. src/slave/containerizer/provisioners/docker.cpp (line 221) https://reviews.apache.org/r/37200/#comment152083 Use explicit capture. src/slave/containerizer/provisioners/docker.cpp (line 251) https://reviews.apache.org/r/37200/#comment152085 What happens on failure? - Jojy Varghese On Aug. 25, 2015, 8:59 p.m., Lily Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37200/ --- (Updated Aug. 25, 2015, 8:59 p.m.) Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, and Jiang Yan Xu. Bugs: MESOS-2849 https://issues.apache.org/jira/browse/MESOS-2849 Repository: mesos Description --- Refactored DockerImage struct to store a list of layer ids instead of linked list of DockerLayers. Diffs - src/slave/containerizer/provisioners/docker.hpp PRE-CREATION src/slave/containerizer/provisioners/docker.cpp PRE-CREATION src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 Diff: https://reviews.apache.org/r/37200/diff/ Testing --- make check Thanks, Lily Chen
Re: Review Request 37427: Docker registry: adding TokenManager.
On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote: src/slave/containerizer/provisioners/docker/token_manager.hpp, line 223 https://reviews.apache.org/r/37427/diff/12/?file=1052806#file1052806line223 Is there a reason you need to include this in the header? Can we just put it in cpp? Jojy Varghese wrote: The only reason being that this is a property of the TokenManager. Timothy Chen wrote: I see, does it need to be? Can't we define a static const in the cpp? We could but then it wont be a class property. Here the constant reflects the property of the class. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/#review96752 --- On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/ --- (Updated Aug. 28, 2015, 4:27 a.m.) Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- Changes: - Added Token implementation (RFC 7519). - Added TokenManager implementation. This component keeps a cache of tokens requested for any future requests. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37427/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 37427: Docker registry: adding TokenManager.
On Aug. 28, 2015, 7:56 a.m., Timothy Chen wrote: src/slave/containerizer/provisioners/docker/token_manager.cpp, line 223 https://reviews.apache.org/r/37427/diff/14/?file=1057180#file1057180line223 If you put the cache here in TokenManager instead of TokenManagerProcess then you'll going to be concurrent access of all the fields since it's not serialized by libprocess. I think tokenCache_ will have undefined behavior when you run at/contains while it's being inserted or deleted then. I suggest you move all this into the process instead. Here the getToken is called in the context of a dispatch from RegistryClient. Do we still need another dispatch? - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/#review96841 --- On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/ --- (Updated Aug. 28, 2015, 4:27 a.m.) Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- Changes: - Added Token implementation (RFC 7519). - Added TokenManager implementation. This component keeps a cache of tokens requested for any future requests. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37427/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 37427: Docker registry: adding TokenManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/ --- (Updated Aug. 30, 2015, 3:10 p.m.) Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen. Changes --- review comments. Repository: mesos Description --- Changes: - Added Token implementation (RFC 7519). - Added TokenManager implementation. This component keeps a cache of tokens requested for any future requests. Diffs (updated) - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37427/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 37773: Docker: Adding registry client.
> On Aug. 31, 2015, 8:42 p.m., Timothy Chen wrote: > > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 456 > > <https://reviews.apache.org/r/37773/diff/7/?file=1059875#file1059875line456> > > > > is maxSize being used at all? If not we should remove all references of > > it. I added it as part of the external facing interface so that it doesnt change when we add validation of max size. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/#review97138 --- On Aug. 30, 2015, 3:12 p.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37773/ > --- > > (Updated Aug. 30, 2015, 3:12 p.m.) > > > Review request for mesos, Lily Chen and Timothy Chen. > > > Repository: mesos > > > Description > --- > > Added implementation for docker registry's Get Manifest and Get Blob APIs. > > > Diffs > - > > src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 > src/slave/containerizer/provisioners/docker/registry_client.hpp > PRE-CREATION > src/slave/containerizer/provisioners/docker/registry_client.cpp > PRE-CREATION > src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/37773/diff/ > > > Testing > --- > > make check > > > Thanks, > > Jojy Varghese > >
Re: Review Request 37427: Docker registry: adding TokenManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/ --- (Updated Aug. 31, 2015, 10:39 p.m.) Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen. Changes --- review comments addressed. Repository: mesos Description --- Changes: - Added Token implementation (RFC 7519). - Added TokenManager implementation. This component keeps a cache of tokens requested for any future requests. Diffs (updated) - src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37427/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 37871: SSL tests refactoring
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37871/ --- (Updated Aug. 31, 2015, 2:32 p.m.) Review request for mesos, Joris Van Remoortere and Timothy Chen. Bugs: MESOS-3337 https://issues.apache.org/jira/browse/MESOS-3337 Repository: mesos Description --- Refactored libprocess SSL tests to be available for reuse by other projects. Diffs - 3rdparty/libprocess/Makefile.am 7ef515848508c2e84ab7607595f635f67e24b19b 3rdparty/libprocess/include/Makefile.am 3b6108dd37d23bd5104162e9e8f4a3aa0518cdcd 3rdparty/libprocess/include/process/ssl/gtest.hpp PRE-CREATION 3rdparty/libprocess/src/openssl_util.hpp 3rdparty/libprocess/src/openssl_util.cpp 42b2860beccff929563ed05905da226b781918b7 3rdparty/libprocess/src/tests/ssl_tests.cpp 7a316bc10575325ffe732fcc87d72d15a4fc5eaf Diff: https://reviews.apache.org/r/37871/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 37427: Docker registry: adding TokenManager.
> On Sept. 1, 2015, 9:13 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 61 > > <https://reviews.apache.org/r/37427/diff/17/?file=1060731#file1060731line61> > > > > Its effectively the same. const reference as method argument is just a > > good practice, thats all. > > Anand Mazumdar wrote: > Can you elaborate a bit more on what does the const reference buys us > here since you end up removing the const-ness later ? > > Anyways if you still want to keep it why not name it as a continuation > variable ? Something similar to this: > > string segment_(segment); // Remove const. We dont remove the const'ness of the original string. The original string object is unchanged. So now we have to decide if we want to copy the object when calling the function or you want to copy the object inside the function. The choice made here is to keep the style of "passing by const reference" and do teh copy inside the function. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/#review97336 --- On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37427/ > --- > > (Updated Aug. 31, 2015, 10:39 p.m.) > > > Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen. > > > Repository: mesos > > > Description > --- > > Changes: > - Added Token implementation (RFC 7519). > - Added TokenManager implementation. This component keeps a cache of tokens > requested for any future requests. > > > Diffs > - > > src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 > src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION > src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION > src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/37427/diff/ > > > Testing > --- > > make check. > > > Thanks, > > Jojy Varghese > >
Re: Review Request 37773: Docker: Adding registry client.
> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote: > > src/tests/provisioners/docker_provisioner_tests.cpp, line 232 > > <https://reviews.apache.org/r/37773/diff/9/?file=1060829#file1060829line232> > > > > Why is this needed? So that we cleanup temporaries created in the SSLTest setup. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/#review97331 --- On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37773/ > --- > > (Updated Sept. 1, 2015, 9:36 p.m.) > > > Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen. > > > Repository: mesos > > > Description > --- > > Added implementation for docker registry's Get Manifest and Get Blob APIs. > > > Diffs > - > > src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 > src/slave/containerizer/provisioners/docker/registry_client.hpp > PRE-CREATION > src/slave/containerizer/provisioners/docker/registry_client.cpp > PRE-CREATION > src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/37773/diff/ > > > Testing > --- > > make check > > > Thanks, > > Jojy Varghese > >
