Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2866283002:
named caches: move instead of symlinking (Closed)
Descriptionnamed caches: move instead of symlinking
Moving named caches is better than symlinking because
- C++ compiler used in swarming tasks reads symlinks =>
compiler output contains absolute paths to named caches =>
compiler cache key contains CWD =>
compiler cache is useless
- if a bot dies, there is a higher chance that a cache is corrupted =>
better to discard the cache by not moving it back
- simpler
Move named cache directories to the task rundir before running a task,
and move them back when task completes (and bot did not die).
Revisit NamedCacheManager design, simplify its API.
install() leaves the cache manager in consistent state: the entries are removed
from the cache. uninstall() puts them back.
R=maruel@chromium.org
BUG=715326
Review-Url: https://codereview.chromium.org/2866283002
Committed: https://github.com/luci/luci-py/commit/d1ab255eac4e9ef88d437d476a586d22cda15952
Patch Set 1 #Patch Set 2 : self review #
Total comments: 14
Patch Set 3 : address comments #Patch Set 4 : update comments and strings, unlint #Patch Set 5 : docstring #
Total comments: 6
Patch Set 6 : final comments #
Messages
Total messages: 24 (17 generated)
Description was changed from ========== named caches: move instead of symlinking Moving named caches is better than symlinking because - C++ compiler used in swarming tasks reads symlinks => compiler output contains absolute paths to named caches => compiler cache key contains CWD => compiler cache is useless - if a bot dies, there is a higher chance that a cache is corrupted => better to discard the cache by not moving it back - simpler Move named cache directories to the task rundir before running a task, and move them back when task completes (and bot did not die). Revisit NamedCacheManager design, simplify its API. R=maruel@chromium.org BUG=715326 ========== to ========== named caches: move instead of symlinking Moving named caches is better than symlinking because - C++ compiler used in swarming tasks reads symlinks => compiler output contains absolute paths to named caches => compiler cache key contains CWD => compiler cache is useless - if a bot dies, there is a higher chance that a cache is corrupted => better to discard the cache by not moving it back - simpler Move named cache directories to the task rundir before running a task, and move them back when task completes (and bot did not die). Revisit NamedCacheManager design, simplify its API. install() leaves the cache manager in consistent state: the entries are removed from the cache. uninstall() puts them back. R=maruel@chromium.org BUG=715326 ==========
Description was changed from ========== named caches: move instead of symlinking Moving named caches is better than symlinking because - C++ compiler used in swarming tasks reads symlinks => compiler output contains absolute paths to named caches => compiler cache key contains CWD => compiler cache is useless - if a bot dies, there is a higher chance that a cache is corrupted => better to discard the cache by not moving it back - simpler Move named cache directories to the task rundir before running a task, and move them back when task completes (and bot did not die). Revisit NamedCacheManager design, simplify its API. install() leaves the cache manager in consistent state: the entries are removed from the cache. uninstall() puts them back. R=maruel@chromium.org BUG=715326 ========== to ========== named caches: move instead of symlinking Moving named caches is better than symlinking because - C++ compiler used in swarming tasks reads symlinks => compiler output contains absolute paths to named caches => compiler cache key contains CWD => compiler cache is useless - if a bot dies, there is a higher chance that a cache is corrupted => better to discard the cache by not moving it back - simpler Move named cache directories to the task rundir before running a task, and move them back when task completes (and bot did not die). Revisit NamedCacheManager design, simplify its API. install() leaves the cache manager in consistent state: the entries are removed from the cache. uninstall() puts them back. Also fix a bug in run_isolated: named cache manager must NOT be open for the duration of the task. R=maruel@chromium.org BUG=715326 ==========
Description was changed from ========== named caches: move instead of symlinking Moving named caches is better than symlinking because - C++ compiler used in swarming tasks reads symlinks => compiler output contains absolute paths to named caches => compiler cache key contains CWD => compiler cache is useless - if a bot dies, there is a higher chance that a cache is corrupted => better to discard the cache by not moving it back - simpler Move named cache directories to the task rundir before running a task, and move them back when task completes (and bot did not die). Revisit NamedCacheManager design, simplify its API. install() leaves the cache manager in consistent state: the entries are removed from the cache. uninstall() puts them back. Also fix a bug in run_isolated: named cache manager must NOT be open for the duration of the task. R=maruel@chromium.org BUG=715326 ========== to ========== named caches: move instead of symlinking Moving named caches is better than symlinking because - C++ compiler used in swarming tasks reads symlinks => compiler output contains absolute paths to named caches => compiler cache key contains CWD => compiler cache is useless - if a bot dies, there is a higher chance that a cache is corrupted => better to discard the cache by not moving it back - simpler Move named cache directories to the task rundir before running a task, and move them back when task completes (and bot did not die). Revisit NamedCacheManager design, simplify its API. install() leaves the cache manager in consistent state: the entries are removed from the cache. uninstall() puts them back. R=maruel@chromium.org BUG=715326 ==========
The CQ bit was checked by nodir@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/36074190eada2510)
lgtm with comments I with CachedManager and LocalCache had similar APIs. We should refactor them to be similar eventually. https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py File client/named_cache.py (right): https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:96: NamedCache must be open.. one dot https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:124: def install(self, root, named_caches): I'd prefer to have this function accept one named_cache, the loop should be outside. https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:129: NamedCache must be open.. same https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:164: def uninstall(self, root, named_caches): same about one call per named cache. Makes the function more readable. https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:173: rel_install = unicode(rel_install) don't, make sure caller is fixed. https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:219: NamedCache must be open.. same https://codereview.chromium.org/2866283002/diff/60001/client/tests/named_cach... File client/tests/named_cache_test.py (right): https://codereview.chromium.org/2866283002/diff/60001/client/tests/named_cach... client/tests/named_cache_test.py:140: def write_file(path, text): keep utility functions at the top. https://codereview.chromium.org/2866283002/diff/60001/client/tests/named_cach... client/tests/named_cache_test.py:141: with open(path, 'w') as f: 'wb' and 'rb' ?
I like that NamedCacheManager API is simple and low level, but we can build another layer of abstraction that works for both named cache and isolated cache and have two implementations of the common interface https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py File client/named_cache.py (right): https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:96: NamedCache must be open.. On 2017/05/09 18:35:14, M-A Ruel wrote: > one dot Done. https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:124: def install(self, root, named_caches): On 2017/05/09 18:35:14, M-A Ruel wrote: > I'd prefer to have this function accept one named_cache, the loop should be > outside. do'h! Yeah, this is much simpler https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:129: NamedCache must be open.. On 2017/05/09 18:35:14, M-A Ruel wrote: > same Done. https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:164: def uninstall(self, root, named_caches): On 2017/05/09 18:35:14, M-A Ruel wrote: > same about one call per named cache. Makes the function more readable. Done. https://codereview.chromium.org/2866283002/diff/60001/client/named_cache.py#n... client/named_cache.py:173: rel_install = unicode(rel_install) On 2017/05/09 18:35:14, M-A Ruel wrote: > don't, make sure caller is fixed. Done. https://codereview.chromium.org/2866283002/diff/60001/client/tests/named_cach... File client/tests/named_cache_test.py (right): https://codereview.chromium.org/2866283002/diff/60001/client/tests/named_cach... client/tests/named_cache_test.py:141: with open(path, 'w') as f: On 2017/05/09 18:35:14, M-A Ruel wrote: > 'wb' and 'rb' ? dunno, I intentionally called the parameter text. Changed
The CQ bit was checked by nodir@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2866283002/diff/120001/client/named_cache.py File client/named_cache.py (right): https://codereview.chromium.org/2866283002/diff/120001/client/named_cache.py#... client/named_cache.py:153: # Just create an empty directory. Remove the Just. Then merge both sentences; # The named cache does not exist, create an empty directory. https://codereview.chromium.org/2866283002/diff/120001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2866283002/diff/120001/client/run_isolated.py... client/run_isolated.py:1018: with named_cache_manager.open(): you don't want it to be kept open the whole time? e.g. with named_cache_manager.open(): for path, name in caches: named_cache_manager.uninstall(path, name) try: yield finally: for path, name in caches: named_cache_manager.uninstall(path, name) https://codereview.chromium.org/2866283002/diff/120001/client/tests/run_isola... File client/tests/run_isolated_test.py (right): https://codereview.chromium.org/2866283002/diff/120001/client/tests/run_isola... client/tests/run_isolated_test.py:76: tdir = unicode(tempfile.mkdtemp(prefix='run_isolated_test')) if you make the prefix unicode, the returned string will be unicode.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/360c7a0c20873510)
The CQ bit was checked by nodir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2866283002/#ps140001 (title: "final comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2866283002/diff/120001/client/named_cache.py File client/named_cache.py (right): https://codereview.chromium.org/2866283002/diff/120001/client/named_cache.py#... client/named_cache.py:153: # Just create an empty directory. On 2017/05/10 18:48:01, M-A Ruel wrote: > Remove the Just. Then merge both sentences; > > # The named cache does not exist, create an empty directory. Done. https://codereview.chromium.org/2866283002/diff/120001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2866283002/diff/120001/client/run_isolated.py... client/run_isolated.py:1018: with named_cache_manager.open(): On 2017/05/10 18:48:01, M-A Ruel wrote: > you don't want it to be kept open the whole time? e.g. > > with named_cache_manager.open(): > for path, name in caches: > named_cache_manager.uninstall(path, name) > try: > yield > finally: > for path, name in caches: > named_cache_manager.uninstall(path, name) definitely not. closing a cache manager saves the state to disk. We should keep it open for the shortest time possible. Definitely, not for the duration of task run because bot may die https://codereview.chromium.org/2866283002/diff/120001/client/tests/run_isola... File client/tests/run_isolated_test.py (right): https://codereview.chromium.org/2866283002/diff/120001/client/tests/run_isola... client/tests/run_isolated_test.py:76: tdir = unicode(tempfile.mkdtemp(prefix='run_isolated_test')) On 2017/05/10 18:48:01, M-A Ruel wrote: > if you make the prefix unicode, the returned string will be unicode. nice :) Done
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494443730270820,
"parent_rev": "f1b353533186324cb28a51007ea2550be23f9e12", "commit_rev":
"d1ab255eac4e9ef88d437d476a586d22cda15952"}
Message was sent while issue was closed.
Description was changed from ========== named caches: move instead of symlinking Moving named caches is better than symlinking because - C++ compiler used in swarming tasks reads symlinks => compiler output contains absolute paths to named caches => compiler cache key contains CWD => compiler cache is useless - if a bot dies, there is a higher chance that a cache is corrupted => better to discard the cache by not moving it back - simpler Move named cache directories to the task rundir before running a task, and move them back when task completes (and bot did not die). Revisit NamedCacheManager design, simplify its API. install() leaves the cache manager in consistent state: the entries are removed from the cache. uninstall() puts them back. R=maruel@chromium.org BUG=715326 ========== to ========== named caches: move instead of symlinking Moving named caches is better than symlinking because - C++ compiler used in swarming tasks reads symlinks => compiler output contains absolute paths to named caches => compiler cache key contains CWD => compiler cache is useless - if a bot dies, there is a higher chance that a cache is corrupted => better to discard the cache by not moving it back - simpler Move named cache directories to the task rundir before running a task, and move them back when task completes (and bot did not die). Revisit NamedCacheManager design, simplify its API. install() leaves the cache manager in consistent state: the entries are removed from the cache. uninstall() puts them back. R=maruel@chromium.org BUG=715326 Review-Url: https://codereview.chromium.org/2866283002 Committed: https://github.com/luci/luci-py/commit/d1ab255eac4e9ef88d437d476a586d22cda15952 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://github.com/luci/luci-py/commit/d1ab255eac4e9ef88d437d476a586d22cda15952 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
