Cache/retrieve extracted CIPD packages in local isolate cache

Cache/retrieve extracted CIPD packages in local isolate cache


BUG=701930

[kjlubick, 2017-05-03 18:28:08]

Description was changed from

==========
WIP isolate cipd packages

BUG=701930
==========

to

==========
Cache/retrieve extracted CIPD packages in local isolate cache


BUG=701930
==========

[kjlubick, 2017-05-03 18:28:32]

kjlubick@google.com changed reviewers:
+ maruel@chromium.org

[kjlubick, 2017-05-03 18:28:33]

https://codereview.chromium.org/2847153002/diff/80001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/80001/client/cipd.py#newcode26
client/cipd.py:26: import isolate
This introduces a dependency cycle that I have no idea how to break.

[M-A Ruel, 2017-05-08 13:13:14]

(Pushing a draft I had forgot to publish, will review)

https://codereview.chromium.org/2847153002/diff/80001/client/utils/fs.py
File client/utils/fs.py (right):

https://codereview.chromium.org/2847153002/diff/80001/client/utils/fs.py#newc...
client/utils/fs.py:124: assert os.path.isabs(path), '%s must be absolute' % path
update lines 37-38 to be coherent
Use %r
what do you mean by "path or unicode" ?

[kjlubick, 2017-05-08 18:59:29]

PTAL

https://codereview.chromium.org/2847153002/diff/80001/client/utils/fs.py
File client/utils/fs.py (right):

https://codereview.chromium.org/2847153002/diff/80001/client/utils/fs.py#newc...
client/utils/fs.py:124: assert os.path.isabs(path), '%s must be absolute' % path
On 2017/05/08 at 13:13:14, M-A Ruel wrote:
> update lines 37-38 to be coherent
> Use %r
> what do you mean by "path or unicode" ?

Done.

I don't know what I meant.  

To quote Gary Larson "It was late and I was tired"

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode215
client/cipd.py:215: def _isolate_cipd(self, root, subdirs, isolate_cache,
cipd_cache):
This doesn't support includes like we mentioned.  I couldn't figure out how to
incorporate it or where the includes would come from, be accessed, etc.

Any advice?

[M-A Ruel, 2017-05-08 20:43:16]

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode172
client/cipd.py:172: with open(cipd_isolated , 'r') as f:
'rb'
it makes it faster on windows.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode181
client/cipd.py:181: ifile = isolated_format.IsolatedFile(digest,
self._cache_hash)
This part should be a function not a method. I'm thinking in isolateserver.py,
as it's similar to
https://github.com/luci/luci-py/blob/master/client/isolateserver.py#L1769 or so.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode186
client/cipd.py:186: for f in files.keys():
for f, props in files.iteritems():

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode228
client/cipd.py:228: subdirs: dict of subdir -> name_verision where name_version
is
name_version

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode267
client/cipd.py:267: with open(os.path.join(subdir, infile.path) , 'r') as f:
'rb'

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode271
client/cipd.py:271: with open(isolated_file , 'r') as f:
'rb'

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode273
client/cipd.py:273: isolate_cache.write(digest, content)
you can calculate hash here, so the file is not read twice.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode313
client/cipd.py:313: file_path.ensure_tree(unicode(cache_dir))
I'd prefer to update call sites instead.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode325
client/cipd.py:325: versions = [p[1] for p in pkgs]
why not each package be individually packed?

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode326
client/cipd.py:326: cipd_isolated = '%s.%s.isolated.%s' % (subdir,
'_'.join(versions),
u

[kjlubick, 2017-05-09 17:38:25]

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode172
client/cipd.py:172: with open(cipd_isolated , 'r') as f:
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> 'rb'
> it makes it faster on windows.

Done.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode181
client/cipd.py:181: ifile = isolated_format.IsolatedFile(digest,
self._cache_hash)
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> This part should be a function not a method. I'm thinking in isolateserver.py,
as it's similar to
https://github.com/luci/luci-py/blob/master/client/isolateserver.py#L1769 or so.

Done.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode186
client/cipd.py:186: for f in files.keys():
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> for f, props in files.iteritems():
I can't - I get "Too many values to unpack"  Comment added

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode228
client/cipd.py:228: subdirs: dict of subdir -> name_verision where name_version
is
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> name_version

Done.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode267
client/cipd.py:267: with open(os.path.join(subdir, infile.path) , 'r') as f:
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> 'rb'

Done.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode271
client/cipd.py:271: with open(isolated_file , 'r') as f:
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> 'rb'

Done

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode273
client/cipd.py:273: isolate_cache.write(digest, content)
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> you can calculate hash here, so the file is not read twice.

Done.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode313
client/cipd.py:313: file_path.ensure_tree(unicode(cache_dir))
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> I'd prefer to update call sites instead.

Done.  Only run_isolated needed it.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode325
client/cipd.py:325: versions = [p[1] for p in pkgs]
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> why not each package be individually packed?

I had tried that originally, but that gets tricky when you have two packages
extracted into the same subdirectory. It ends up being cleaner to cache on a
directory by directory basis, at the expense of more cache churn if only one of
the packages updates.

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode326
client/cipd.py:326: cipd_isolated = '%s.%s.isolated.%s' % (subdir,
'_'.join(versions),
On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> u

Done.

[M-A Ruel, 2017-05-10 13:31:58]

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/180001/client/cipd.py#newcode325
client/cipd.py:325: versions = [p[1] for p in pkgs]
On 2017/05/09 17:38:25, kjlubick wrote:
> On 2017/05/08 at 20:43:16, M-A Ruel wrote:
> > why not each package be individually packed?
> 
> I had tried that originally, but that gets tricky when you have two packages
> extracted into the same subdirectory. It ends up being cleaner to cache on a
> directory by directory basis, at the expense of more cache churn if only one
of
> the packages updates.

Hummm I still think it's not a great idea. Since there can be collision, extract
in a temporary directory, archive this, then remap into the destination. It's 3x
more I/O initially but I think it's worth it because then you can detect if
there are files conflicts and it will improve cache hit rate when one package is
rolled frequently but not the others.

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode150
client/cipd.py:150: Retrieves the CIPD subdirectories from the isolate cache, if
they exist,
please start comment on the first line
generally prefer public methods first then private methods

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode156
client/cipd.py:156: target_dir: directory in which to put the files.
inconsistent dots, remove

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode173
client/cipd.py:173: digest = str(f.read())
it's already an str.

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode176
client/cipd.py:176: except Exception as e:
that's too broad, please scope down

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode198
client/cipd.py:198: Puts the content of the CIPD subdirectories into the isolate
cache,
single sentence
empty line
rest

[kjlubick, 2017-05-10 19:41:01]

cipd packages are handled separately now

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode150
client/cipd.py:150: Retrieves the CIPD subdirectories from the isolate cache, if
they exist,
On 2017/05/10 at 13:31:58, M-A Ruel wrote:
> please start comment on the first line
> generally prefer public methods first then private methods

Done.

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode156
client/cipd.py:156: target_dir: directory in which to put the files.
On 2017/05/10 at 13:31:58, M-A Ruel wrote:
> inconsistent dots, remove

Done.

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode173
client/cipd.py:173: digest = str(f.read())
On 2017/05/10 at 13:31:58, M-A Ruel wrote:
> it's already an str.

Done.

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode176
client/cipd.py:176: except Exception as e:
On 2017/05/10 at 13:31:58, M-A Ruel wrote:
> that's too broad, please scope down

Done.

https://codereview.chromium.org/2847153002/diff/220001/client/cipd.py#newcode198
client/cipd.py:198: Puts the content of the CIPD subdirectories into the isolate
cache,
On 2017/05/10 at 13:31:58, M-A Ruel wrote:
> single sentence
> empty line
> rest

Done.

[M-A Ruel, 2017-05-10 20:00:56]

maruel@chromium.org changed reviewers:
+ vadimsh@chromium.org

[M-A Ruel, 2017-05-10 20:00:57]

I also want Vadim to take a look in case I missed anything serious.

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode203
client/cipd.py:203: os.write(ensure_file_handle, '@Subdir %s\n' % pkg)
One problem with this code is that the cipd package isn't tagged, which will
cause it to be evicted sooner. Probably worth having a ensure for all packages.

Also wondering about unpinned packages, e.g. :latest tag, should we not isolate
them?

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode217
client/cipd.py:217: temp_root = tempfile.mkdtemp()
prefix=u'cpid'
this directory is never deleted?

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode260
client/cipd.py:260: 
remove empty line

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode270
client/cipd.py:270: if (self._ensure_from_isolate(os.path.join(site_root,
subdir),
wrapping () not necessary.

I'd prefer to reverse the condition.

if not self._ensure_from_isolate(...):
  raise Error(...)

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode313
client/cipd.py:313: content = isolate_cache.getfileobj(digest).read()
with isolate_cache.getfileobj(digest) as f:
  content = f.read()

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode314
client/cipd.py:314: except Exception as e:
too broad, you want isolateserver.CacheMiss

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode328
client/cipd.py:328: except OSError as e:
I'm wondering; it would apply to lines 310 and 311 but not sure about the rest.

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode379
client/cipd.py:379: isolated_file = unicode(os.path.join(cipd_cache,
inconsistent argument wrapping

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode397
client/cipd.py:397: digest = self._cache_hash(content).hexdigest()
this can be outside the closure

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode400
client/cipd.py:400: with open(os.path.join(cipd_cache, hashfile), 'w') as f:
'wb'

[kjlubick, 2017-05-10 20:21:00]

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode203
client/cipd.py:203: os.write(ensure_file_handle, '@Subdir %s\n' % pkg)
On 2017/05/10 at 20:00:57, M-A Ruel wrote:
> One problem with this code is that the cipd package isn't tagged, which will
cause it to be evicted sooner. Probably worth having a ensure for all packages.

I'm not quite sure what you mean by "cipd package isn't tagged" or "ensure for
all packages".

> 
> Also wondering about unpinned packages, e.g. :latest tag, should we not
isolate them?

Oh, that's a good point.  Probably not to avoid cache inconsistencies.

What's a good way to figure out if it's a unpinned vs pinned?  I could check to
see if version looks like a hash, but is there a way to do that consistent for
any of the hash algorithms?

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode217
client/cipd.py:217: temp_root = tempfile.mkdtemp()
On 2017/05/10 at 20:00:57, M-A Ruel wrote:
> prefix=u'cpid'
> this directory is never deleted?

Done.

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode260
client/cipd.py:260: 
On 2017/05/10 at 20:00:57, M-A Ruel wrote:
> remove empty line

Done.

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode270
client/cipd.py:270: if (self._ensure_from_isolate(os.path.join(site_root,
subdir),
On 2017/05/10 at 20:00:57, M-A Ruel wrote:
> wrapping () not necessary.
> 
> I'd prefer to reverse the condition.
> 
> if not self._ensure_from_isolate(...):
>   raise Error(...)

Done.

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode313
client/cipd.py:313: content = isolate_cache.getfileobj(digest).read()
On 2017/05/10 at 20:00:56, M-A Ruel wrote:
> with isolate_cache.getfileobj(digest) as f:
>   content = f.read()

Done.

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode314
client/cipd.py:314: except Exception as e:
On 2017/05/10 at 20:00:56, M-A Ruel wrote:
> too broad, you want isolateserver.CacheMiss

Done.

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode328
client/cipd.py:328: except OSError as e:
On 2017/05/10 at 20:00:57, M-A Ruel wrote:
> I'm wondering; it would apply to lines 310 and 311 but not sure about the
rest.

Good point. I think I had it for debugging asserts, but it's not really needed
anymore.  Removed

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode379
client/cipd.py:379: isolated_file = unicode(os.path.join(cipd_cache,
On 2017/05/10 at 20:00:57, M-A Ruel wrote:
> inconsistent argument wrapping

Done.

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode397
client/cipd.py:397: digest = self._cache_hash(content).hexdigest()
On 2017/05/10 at 20:00:57, M-A Ruel wrote:
> this can be outside the closure

Done.

https://codereview.chromium.org/2847153002/diff/240001/client/cipd.py#newcode400
client/cipd.py:400: with open(os.path.join(cipd_cache, hashfile), 'w') as f:
On 2017/05/10 at 20:00:56, M-A Ruel wrote:
> 'wb'

Done.

[Vadim Sh., 2017-05-11 03:09:09]

Sorry, I dislike this CL very much.

I believe it adds fragility and complexity in already quite complicated code by
tightly coupling different systems.

I believe if we to pursue this approach we need to add very clear API boundaries
between CIPD client, Isolate client and Isolate cache.

For example:

1. Introduce a notion of CacheHandler (in pseudocode):

interface CacheHandler {
  // Hardlinks cached item into 'destinationPath' if it is in the cache.
  GrabFromCache(digest, destinationPath) => (Success | CacheMiss)

  // Copies given file into the cache.
  PutIntoCache(originalPath) => digest
}

The API can be as simple as a command line invocations:

./cache_handler grab <digest> <dest>
./cache_handler put <path>

2. Make Swarming bot implement cache_handler API.

3. Make run_isolated.py and cipd client accept implementation of the API:
cipd ensure <...> -cache-handler "./cache_handler"

(Or something along this lines. APIs don't always mean RPC calls over TCP).

Without clear concise boundaries we will end up with unmanageable buggy code (we
already very close to this state, TBH).

https://codereview.chromium.org/2847153002/diff/260001/client/cipd.py
File client/cipd.py (right):

https://codereview.chromium.org/2847153002/diff/260001/client/cipd.py#newcode195
client/cipd.py:195: cipd_isolated = u'%s.%s.isolated.%s' % (pkg, version,
how does this look?

IIUC, it will look like
'infra/tools/luci/kitchen/linux-amd64.git_revision:f027117e5549a5defd39f3f779c3bb9d3856cd0b.isolated.sha1'
which is kind of bad and won't work on Windows at all (due to ':').

When this files are cleaned up?

Also, there seem to be no way to use movable references (like 'latest'). Bots
will stuck using whatever version they managed to isolate.

https://codereview.chromium.org/2847153002/diff/260001/client/cipd.py#newcode204
client/cipd.py:204: os.write(ensure_file_handle, '@Subdir %s\n' % pkg)
this doesn't look right, it should be 'subdir'. This also implies to_isolate is
using wrong key (it should use tuple of subdir and pkg).

https://codereview.chromium.org/2847153002/diff/260001/client/isolateserver.py
File client/isolateserver.py (right):

https://codereview.chromium.org/2847153002/diff/260001/client/isolateserver.p...
client/isolateserver.py:372: for f in files.keys():
nit: .keys() is not necessary. Enumerating a dict already returns its keys.

https://codereview.chromium.org/2847153002/diff/260001/client/run_isolated.py
File client/run_isolated.py (right):

https://codereview.chromium.org/2847153002/diff/260001/client/run_isolated.py...
client/run_isolated.py:128: package.add_python_file(os.path.join(BASE_DIR,
'run_isolated.py'))
note that this file may already be included in the zip as __main__.py

(depending on 'executable').

Also, I think run_isolated.zip is kind of dead code now. Nothing is using it in
production (Swarming bot has run_isolated.py copy itself). So I'm surprised you
are modifying it.

https://codereview.chromium.org/2847153002/diff/260001/client/run_isolated.py...
client/run_isolated.py:692: isolate_cache=None):
please document in Args, at least its type (in all other places where it is
added too)

[kjlubick, 2017-05-11 17:20:43]

On 2017/05/11 at 03:09:09, vadimsh wrote:
> Sorry, I dislike this CL very much.
> 
> I believe it adds fragility and complexity in already quite complicated code
by tightly coupling different systems.
> 
> I believe if we to pursue this approach we need to add very clear API
boundaries between CIPD client, Isolate client and Isolate cache.
> 
> For example:
> 
> 1. Introduce a notion of CacheHandler (in pseudocode):
> 
> interface CacheHandler {
>   // Hardlinks cached item into 'destinationPath' if it is in the cache.
>   GrabFromCache(digest, destinationPath) => (Success | CacheMiss)
> 
>   // Copies given file into the cache.
>   PutIntoCache(originalPath) => digest
> }
> 
> The API can be as simple as a command line invocations:
> 
> ./cache_handler grab <digest> <dest>
> ./cache_handler put <path>
> 
> 2. Make Swarming bot implement cache_handler API.
> 
> 3. Make run_isolated.py and cipd client accept implementation of the API:
> cipd ensure <...> -cache-handler "./cache_handler"
> 
> (Or something along this lines. APIs don't always mean RPC calls over TCP).
> 
> Without clear concise boundaries we will end up with unmanageable buggy code
(we already very close to this state, TBH).
> 
> https://codereview.chromium.org/2847153002/diff/260001/client/cipd.py
> File client/cipd.py (right):
> 
>
https://codereview.chromium.org/2847153002/diff/260001/client/cipd.py#newcode195
> client/cipd.py:195: cipd_isolated = u'%s.%s.isolated.%s' % (pkg, version,
> how does this look?
> 
> IIUC, it will look like
'infra/tools/luci/kitchen/linux-amd64.git_revision:f027117e5549a5defd39f3f779c3bb9d3856cd0b.isolated.sha1'
which is kind of bad and won't work on Windows at all (due to ':').
> 
> When this files are cleaned up?
> 
> Also, there seem to be no way to use movable references (like 'latest'). Bots
will stuck using whatever version they managed to isolate.
> 
>
https://codereview.chromium.org/2847153002/diff/260001/client/cipd.py#newcode204
> client/cipd.py:204: os.write(ensure_file_handle, '@Subdir %s\n' % pkg)
> this doesn't look right, it should be 'subdir'. This also implies to_isolate
is using wrong key (it should use tuple of subdir and pkg).
> 
> https://codereview.chromium.org/2847153002/diff/260001/client/isolateserver.py
> File client/isolateserver.py (right):
> 
>
https://codereview.chromium.org/2847153002/diff/260001/client/isolateserver.p...
> client/isolateserver.py:372: for f in files.keys():
> nit: .keys() is not necessary. Enumerating a dict already returns its keys.
> 
> https://codereview.chromium.org/2847153002/diff/260001/client/run_isolated.py
> File client/run_isolated.py (right):
> 
>
https://codereview.chromium.org/2847153002/diff/260001/client/run_isolated.py...
> client/run_isolated.py:128: package.add_python_file(os.path.join(BASE_DIR,
'run_isolated.py'))
> note that this file may already be included in the zip as __main__.py
> 
> (depending on 'executable').
> 
> Also, I think run_isolated.zip is kind of dead code now. Nothing is using it
in production (Swarming bot has run_isolated.py copy itself). So I'm surprised
you are modifying it.
> 
>
https://codereview.chromium.org/2847153002/diff/260001/client/run_isolated.py...
> client/run_isolated.py:692: isolate_cache=None):
> please document in Args, at least its type (in all other places where it is
added too)

Abandoning this change in favor of https://skia-review.googlesource.com/c/16490