mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilbert Song <songzihao1...@gmail.com>
Subject Re: Review Request 56721: Implemented pruneImages with a mark and sweep in docker store.
Date Tue, 03 Oct 2017 01:19:28 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message