Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review141519 --- Closing this review due to inactivity. Please see our [guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md) for reopening reviews. - Joris Van Remoortere On July 7, 2015, 7:43 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34140/ > --- > > (Updated July 7, 2015, 7:43 p.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Images are fetched into the store (after discovery). Stored images are > currently kept indefinitely. > > > Diffs > - > > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION > src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION > src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b > src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 > > Diff: https://reviews.apache.org/r/34140/diff/ > > > Testing > --- > > > Thanks, > > Ian Downes > >
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review113545 --- Can we close this now? I don't think this is relevant anymore. - Timothy Chen On July 7, 2015, 7:43 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34140/ > --- > > (Updated July 7, 2015, 7:43 p.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Images are fetched into the store (after discovery). Stored images are > currently kept indefinitely. > > > Diffs > - > > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION > src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION > src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b > src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 > > Diff: https://reviews.apache.org/r/34140/diff/ > > > Testing > --- > > > Thanks, > > Ian Downes > >
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review91989 --- Not necessary to be done in this review but we should allow `putting` images that are already in the staging folder, this will support mechanisms that download stuff out-of-band. Also we should rely on a `paths.hpp` when doing all the path manipulation. src/slave/containerizer/provisioners/appc/store.hpp (line 50) https://reviews.apache.org/r/34140/#comment145928 It's worth noting that the id is computed from the sha512 hash here as this is actually the only place where the mechanism by which that this id is derived matters. Everywhere else it should be understood as an opaque string. src/slave/containerizer/provisioners/appc/store.hpp (lines 80 - 83) https://reviews.apache.org/r/34140/#comment145766 Who uses this? src/slave/containerizer/provisioners/appc/store.cpp (line 148) https://reviews.apache.org/r/34140/#comment146052 Here if the `stage` directory is moved into the store then `rmdir()` returns Error. Of course we don't check the result but maybe checking its existence before rmdir and log other rmdir Errors is better? src/slave/containerizer/provisioners/appc/store.cpp (line 397) https://reviews.apache.org/r/34140/#comment145936 So at this point we have already downloaded the image, we might as well just go ahead and overwrite the existing folder. If not, we probably don't want to spend all the resources to download it in the first place (i.e. directly return in `put()`). But I think it would make sense to overwrite if we are going to open up some HTTP API to allow operators to resue some faulty images. src/slave/containerizer/provisioners/appc/store.cpp (lines 406 - 415) https://reviews.apache.org/r/34140/#comment145933 Manifest validation is only one type of validation. We should also do other validation checks. e.g. whether rootfs exists; whether there are additional files outside of rootfs, etc. - Jiang Yan Xu On July 7, 2015, 12:43 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: AppC image store
On May 27, 2015, 4:10 p.m., Paul Brett wrote: src/slave/containerizer/provisioners/appc/store.cpp, line 267 https://reviews.apache.org/r/34140/diff/1/?file=957277#file957277line267 Why not do the decompress, hash untar as a pipeline to reduce disk usage? Ian Downes wrote: Because we need to hash the (decrypted (uncompressed)) tarball, i.e., an intermediate object, and because we don't know how it is compressed. It may take more effort but I think it can be done so maybe this is what we should eventually do? 1) detecting the encryption and compression type up front. 2) `tee` the decompression output to do the hash and write it to a file. For bigger images I do think the saves both temporary disk space as well as reduce latency (less disk writes). Thoughts? - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review85456 --- On July 7, 2015, 12:43 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review90954 --- src/slave/flags.cpp (line 74) https://reviews.apache.org/r/34140/#comment144111 Since we're also adding docker, I think we should make this /tmp/mesos/store/[appc|docker] - Timothy Chen On July 7, 2015, 7:43 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated July 7, 2015, 7:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated June 22, 2015, 9:51 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Renamed flag and invoke fetcher properly. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs (updated) - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: AppC image store
On May 26, 2015, 11:28 a.m., Timothy Chen wrote: src/slave/containerizer/provisioners/appc/store.cpp, line 79 https://reviews.apache.org/r/34140/diff/1/?file=957277#file957277line79 Check exists first? os::mkdir() ignores EEXIST so won't return an error if it already exists. On May 26, 2015, 11:28 a.m., Timothy Chen wrote: src/slave/containerizer/provisioners/appc/store.hpp, line 55 https://reviews.apache.org/r/34140/diff/1/?file=957276#file957276line55 Is this suppose to be a generic Store? It returns a list of AppcImage but this is a generic namespace under slave. Should we have a base class like ContainerImage? No, it's specific to appc images at the moment. Could be refactored if/when necessary. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review83840 --- On May 26, 2015, 11:25 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated May 26, 2015, 11:25 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review85456 --- Disk usage by the store is currently unbounded. Do we need to add the ability to discover or limit the size of the image store? Will you limit the number of fetchers that can be run concurrently to limit the performance impact to existing containers? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137028 And log the error? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137029 Does the mesos-fetcher choose the name or is that down to the requester? Can we assume that the name is safe to use? Why not pass it from StoreProcess rather than try to work it out after the fact? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137030 We probably want to keep a track on how long it took so we can debug performance issues. src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137031 Do we cache the hashes? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment137032 Why not do the decompress, hash untar as a pipeline to reduce disk usage? - Paul Brett On May 26, 2015, 6:25 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated May 26, 2015, 6:25 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review83840 --- src/slave/containerizer/provisioners/appc/store.hpp https://reviews.apache.org/r/34140/#comment134927 Is this suppose to be a generic Store? It returns a list of AppcImage but this is a generic namespace under slave. Should we have a base class like ContainerImage? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment134949 Check exists first? src/slave/containerizer/provisioners/appc/store.cpp https://reviews.apache.org/r/34140/#comment134950 Why not use os::rmdir? - Timothy Chen On May 26, 2015, 6:25 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated May 26, 2015, 6:25 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated May 26, 2015, 11:25 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Summary (updated) - AppC image store Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: Appc image store
On May 18, 2015, 4:38 p.m., Chi Zhang wrote: src/slave/containerizer/provisioners/appc/store.hpp, line 116 https://reviews.apache.org/r/34140/diff/1/?file=957276#file957276line116 Looks like this is a global store for all images. Would it make sense to make sure at most one StoreProcess can be instantiated? There should only be one Store for a given store directory, not necessarily globally. I don't think it's necessary to complicate the code to enforce uniqueness. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review84233 --- On May 12, 2015, 5:48 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated May 12, 2015, 5:48 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34140: Appc image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review84233 --- src/slave/containerizer/provisioners/appc/store.hpp https://reviews.apache.org/r/34140/#comment135392 Looks like this is a global store for all images. Would it make sense to make sure at most one StoreProcess can be instantiated? - Chi Zhang On May 13, 2015, 12:48 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated May 13, 2015, 12:48 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
