----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56721/#review190636 -----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp Lines 2790-2792 (patched) <https://reviews.apache.org/r/56721/#comment268146> We are not using this var, right? src/slave/containerizer/mesos/containerizer.cpp Lines 2793 (patched) <https://reviews.apache.org/r/56721/#comment268185> Should we just use `Hashset<Image>`? src/slave/containerizer/mesos/containerizer.cpp Lines 2797 (patched) <https://reviews.apache.org/r/56721/#comment268149> just a little rephasing: Checkpointing `ContainerConfig` is introduced recently. Lagacy containers do not have the information of which image is used. Image pruning is disabled. src/slave/containerizer/mesos/containerizer.cpp Lines 2803-2804 (patched) <https://reviews.apache.org/r/56721/#comment268183> You might need foreachpair for log out the containerID: ``` return Failure( "Container " + containerId + " does not have ContainerConfig checkpointed. Image pruning is disabled"); ``` src/slave/containerizer/mesos/provisioner/docker/metadata_manager.hpp Lines 103-107 (patched) <https://reviews.apache.org/r/56721/#comment268215> could we use `excludedImages` consistently in favor of adding that to operator API in near future. src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp Lines 83-85 (patched) <https://reviews.apache.org/r/56721/#comment268217> do we still need this variable? src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp Lines 213 (patched) <https://reviews.apache.org/r/56721/#comment268216> s/stored/cached/g? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 82 (patched) <https://reviews.apache.org/r/56721/#comment268220> why do we neet to add this? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 79-80 (original), 83-88 (patched) <https://reviews.apache.org/r/56721/#comment268223> could we not change the style for now in this file? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 262-263 (patched) <https://reviews.apache.org/r/56721/#comment268224> why adding this? src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 531 (patched) <https://reviews.apache.org/r/56721/#comment268228> maybe we should checkpoint the layer dir instead of the layerRootfs dir in the provisioner. src/slave/containerizer/mesos/provisioner/provisioner.hpp Lines 110 (patched) <https://reviews.apache.org/r/56721/#comment268186> then, pass a hashset of Images to provisioner::pruneImages()? src/slave/containerizer/mesos/provisioner/provisioner.hpp Lines 207-210 (patched) <https://reviews.apache.org/r/56721/#comment268203> Could we add more comments to describe the critical section of this readwritelock? E.g., store directory and the provisioner dir are the critical section. and basically, `provision()` and `destroy` are not expected to touch the same critical section simultaneously. This is guarantteed by the mesos containerizer, e.g., a destroy will always wait for a container provisioning finishes, then do the cleanup. src/slave/containerizer/mesos/provisioner/provisioner.hpp Lines 209 (patched) <https://reviews.apache.org/r/56721/#comment268202> s/prune/destroy/g? src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 486-487 (patched) <https://reviews.apache.org/r/56721/#comment268204> quote destroy and provisioner, as well as pruneImages. src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 569-570 (patched) <https://reviews.apache.org/r/56721/#comment268205> ditto. src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 714-715 (patched) <https://reviews.apache.org/r/56721/#comment268206> ditto. src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 727 (patched) <https://reviews.apache.org/r/56721/#comment268210> one more space after foreach, e.g., `foreachXXX (`. ditto to all others. src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 728-729 (patched) <https://reviews.apache.org/r/56721/#comment268208> use `info->layers.isNone()` instead? src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 728-730 (patched) <https://reviews.apache.org/r/56721/#comment268209> we need to add some comments here. generally, we might have 3 scenarios that we need to think about: 1. a lagacy container? no, impossible to hit this code since lagacy containers are already excluded by the containerizer. 2. the agent crashes after we call backend::provision() but before checkpointing the `layers`? yes, in this case it is fine to skip those layers sinece they are not used for any running containers yet. 3. someone deletes the checkpointed `layers` files? yes, possible but we dont expect this is allowed. it means we may delete layers that a running container is using. we need to at least `VLOG(1)` to that containerID (using `foreachpair` instead?). Thoughts? src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 732-734 (patched) <https://reviews.apache.org/r/56721/#comment268212> fit in one line? src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 750-753 (patched) <https://reviews.apache.org/r/56721/#comment268214> fit in one line after the change? src/slave/containerizer/mesos/provisioner/provisioner.cpp Lines 751 (patched) <https://reviews.apache.org/r/56721/#comment268213> s/[=]/[]/g? and do you need `const list<Nothing>&`? src/slave/containerizer/mesos/provisioner/store.hpp Lines 97-98 (patched) <https://reviews.apache.org/r/56721/#comment268211> Could we add comments for these two parameter? especially, people would ask while passing in the images, why do we need the active layers? it is because in the store (e.g., docker store) the cache is not the source of true. we need to not only keep the excluded images from the operator API, but also maintain the cache. - Gilbert Song On Oct. 30, 2017, 4:59 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56721/ > ----------------------------------------------------------- > > (Updated Oct. 30, 2017, 4:59 p.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 > 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b > src/slave/containerizer/mesos/containerizer.cpp > 100e3bbda543d87808da9ff6bea42da5099ea8c5 > 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/14/ > > > Testing > ------- > > > Thanks, > > Zhitao Li > >
