----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56721/#review186320 -----------------------------------------------------------
src/slave/containerizer/containerizer.hpp Lines 156 (patched) <https://reviews.apache.org/r/56721/#comment262815> Could we add some comments? src/slave/containerizer/containerizer.hpp Lines 156-157 (patched) <https://reviews.apache.org/r/56721/#comment263289> maybe return failure in this virtual function by default? so that you don't need to do that on docker containerizer. src/slave/containerizer/docker.hpp Lines 112-114 (patched) <https://reviews.apache.org/r/56721/#comment263290> not needed. src/slave/containerizer/docker.cpp Lines 873-880 (patched) <https://reviews.apache.org/r/56721/#comment263291> not needed. src/slave/containerizer/mesos/containerizer.cpp Lines 2628 (patched) <https://reviews.apache.org/r/56721/#comment263693> Need to comment here since it depends on the checkpointed containerconfig introduced recently. src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp Lines 145-147 (patched) <https://reviews.apache.org/r/56721/#comment263766> two more spaces? src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp Lines 209-211 (patched) <https://reviews.apache.org/r/56721/#comment263776> three scenarios: 1. checkpointed cache is cleaned up for some reasons 2. the `activeImages` contains `excludedImages`, which the operator specifys some images that are never pulled before on this agent. and I just think of the 3rd case: 3. if a couple big images are still being pulled (pulling started before `prune()`), it means thay are not in `storedImages` yet. 4. same as #3, we just unlock the store and resume the image pulling, then the requests in queue will be executed simultanuously, but another prune() call come in from the operator for some reason. for the case of #3 and #4, we may need to fix: https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp#L384~#L388 because container launch may fail if any of the layers are included in those ongoing pull but still get marked from pre-existed unused images. src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp Lines 212 (patched) <https://reviews.apache.org/r/56721/#comment263717> extra space? src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp Lines 217 (patched) <https://reviews.apache.org/r/56721/#comment263786> It means your cache is always empty. `retainedImages` instead? src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp Lines 219 (patched) <https://reviews.apache.org/r/56721/#comment263787> s/image.get().layer_ids()/image->layer_ids()/g src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp Lines 228 (patched) <https://reviews.apache.org/r/56721/#comment263788> rename it as `unusedLayers`? src/slave/containerizer/mesos/provisioner/docker/paths.hpp Lines 30-42 (original), 30-42 (patched) <https://reviews.apache.org/r/56721/#comment263708> Could you fix the comment as well? src/slave/containerizer/mesos/provisioner/docker/paths.cpp Lines 106 (patched) <https://reviews.apache.org/r/56721/#comment263714> Understand this dir is used for sweeping only since marking is already done. but as a helper, we should follow whatever named for this dir, e.g., `gc`. maybe call it `getGcPath()`? (we should use the same patern in this file e.g., xxxxPath(), the `getStagingDir()` is a tech debt). src/slave/containerizer/mesos/provisioner/docker/paths.cpp Lines 112 (patched) <https://reviews.apache.org/r/56721/#comment263715> getGcLayerPath()? src/slave/containerizer/mesos/provisioner/docker/paths.cpp Lines 114-115 (patched) <https://reviews.apache.org/r/56721/#comment263712> avoid this extra var? src/slave/containerizer/mesos/provisioner/docker/paths.cpp Lines 115 (patched) <https://reviews.apache.org/r/56721/#comment263718> are you prefer namoseconds than milliseconds for precise? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 133 (patched) <https://reviews.apache.org/r/56721/#comment263730> what do you think renaming this struct to `ImagePullingInfo`? since we are having a queue of `Queued` sounds a little weird. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 145 (patched) <https://reviews.apache.org/r/56721/#comment263807> any reason it is not a boolean? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 146 (patched) <https://reviews.apache.org/r/56721/#comment263731> s/queued/imagePullingQueue/g src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 280 (patched) <https://reviews.apache.org/r/56721/#comment263729> VLOG(1) << "Queuing to get the image '" << name << "' since images are being pruned"; src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 281 (patched) <https://reviews.apache.org/r/56721/#comment263733> newline above. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 518 (patched) <https://reviews.apache.org/r/56721/#comment263740> s/Another pruning is already happening!/Another image pruning is happening/g src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 536 (patched) <https://reviews.apache.org/r/56721/#comment263811> should be a boolean. and basically this is the lock. we should put it at the very beginning of this method, right below the check of any other pruning happening. also, let's comment on this lock. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 551 (patched) <https://reviews.apache.org/r/56721/#comment263819> The store is already unlocked src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 575 (patched) <https://reviews.apache.org/r/56721/#comment263806> ditto. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 582 (patched) <https://reviews.apache.org/r/56721/#comment263812> quote the path and no `!` at the end? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 586 (patched) <https://reviews.apache.org/r/56721/#comment263813> ditto. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 589 (patched) <https://reviews.apache.org/r/56721/#comment263814> VLOG(1)? since there might be hundreds of layers. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 589-590 (patched) <https://reviews.apache.org/r/56721/#comment263815> quote these 3 vars. basically we should quote all variables that are not generated by mesos. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 591 (patched) <https://reviews.apache.org/r/56721/#comment263816> newline above. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 598 (patched) <https://reviews.apache.org/r/56721/#comment263818> what does this comment mean? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 619 (patched) <https://reviews.apache.org/r/56721/#comment263821> quote the path. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 621 (patched) <https://reviews.apache.org/r/56721/#comment263822> ditto. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 623 (patched) <https://reviews.apache.org/r/56721/#comment263823> ditto. - Gilbert Song On Sept. 26, 2017, 11:15 a.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56721/ > ----------------------------------------------------------- > > (Updated Sept. 26, 2017, 11:15 a.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu. > > > Bugs: MESOS-4945 > https://issues.apache.org/jira/browse/MESOS-4945 > > > Repository: mesos > > > Description > ------- > > This includes the following changes: > - add a `pruneImages()` function on the chain of relevant classes; > - implement prune in docker store; > - fix mock interface to keep existing tests pass. > > > Diffs > ----- > > src/slave/containerizer/composing.hpp > 06d68eef5de7745e32f0e808f11016bcc285dd8f > src/slave/containerizer/composing.cpp > 587f009384f0c7ef87482686578dc822d3d5b208 > src/slave/containerizer/containerizer.hpp > 449bb5d0902936faae7bf9bae9c703b219aed842 > src/slave/containerizer/docker.hpp b602a5698cae12686f51c4b9370a06042cda6270 > src/slave/containerizer/docker.cpp 292eecbca246edf068ec8c262aff4f3ce9cd8c67 > src/slave/containerizer/mesos/containerizer.hpp > cc23b4d91be16fc95a131c09d07378b801e34d6f > src/slave/containerizer/mesos/containerizer.cpp > 4d5dc13f363f5d8886983d7dd06a5cecc177c345 > src/slave/containerizer/mesos/provisioner/docker/metadata_manager.hpp > 954da1681778878c8aff6150002e52ecb648d1bb > src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp > d86afd2a6ff0bf87e624db1c99255c85068bf6ab > src/slave/containerizer/mesos/provisioner/docker/paths.hpp > 232c027f8f96da0cb30b957bce4607d3695050d2 > src/slave/containerizer/mesos/provisioner/docker/paths.cpp > cd684b33eb308ce1eeb4539a5b2d51985d835db7 > src/slave/containerizer/mesos/provisioner/docker/store.hpp > 1cf68665d33bd40a7605d26c96fb7b618407fdd0 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > f357710cb19aec3654b0604f7909d068eaf20095 > src/slave/containerizer/mesos/provisioner/provisioner.hpp > 7cba54ce490d1e6e17081cd7e04fd6759ceddb8e > src/slave/containerizer/mesos/provisioner/provisioner.cpp > 450a3b32d69d2882973a6ed4e94e169a0256056b > src/slave/containerizer/mesos/provisioner/store.hpp > 01ab83dca79e51b8c96d18ee65705912b0ac8324 > src/slave/containerizer/mesos/provisioner/store.cpp > cc5cc81e05f29bb0e11ffa13cdb8d63d4397114f > src/tests/containerizer.hpp a778b8581904bacea9eec3ff50c3c009959b5dac > src/tests/containerizer.cpp cd140f4263621a0a33a34b7e062a9ca6cf426e7a > src/tests/containerizer/mock_containerizer.hpp > 0adcb01e6c12d6cc4abed1f14fa2df833ffc6569 > > > Diff: https://reviews.apache.org/r/56721/diff/8/ > > > Testing > ------- > > > Thanks, > > Zhitao Li > >
