Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2037253002:
run_isolated.py: install CIPD packages (Closed)
Descriptionrun_isolated.py: install CIPD packages
add cipd options to run_isolated.py:
--cipd-package: package name and version, both required. Can be specified multiple times
--cipd-server: URL of CIPD server
--cipd-client-package: CIPD client package to install
--cipd-cache: cache dir for CIPD clients, client versions and packages
expand ${platform} and ${os_ver} parameters in package name with values
read from current machine.
expand ${CIPD_PACKAGE} and ${EXECUTABLE_SUFFIX} parameters on command line with
path to CIPD package installation dir and (.exe|) suffix.
cipd client installation:
- lookup client version (typically a git_revision tag) in version cache
(typically DiskCache)
- on cache miss, resolve version to instance id using CIPD backend API
- lookup client instance id in client cache (DiskCache)
- on cache miss, fetch it from cipd backend / GS, put to disk cache and
make executable
package installation:
- put list of packages to a file
- use cipd-ensure to install packages
- pass -verbose and log all cipd-ensure output: stdout->INFO,
stderr->DEBUGpass -verbose and log all cipd-ensure output:
stdout->INFO, stderr->DEBUG. Typically there is only stderr
- set -cache-dir flag to "<cipd-cache flag value>/packages"
R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
Committed: https://github.com/luci/luci-py/commit/45eab10a17a55f1978f3bfa02c35457dee1c64e4
Patch Set 1 #
Total comments: 49
Patch Set 2 : addressed comments #
Total comments: 27
Patch Set 3 : addressed comments #
Total comments: 6
Patch Set 4 : get_client is a context manager #
Total comments: 6
Patch Set 5 : simplified closing of list file #
Total comments: 9
Patch Set 6 : nits #Patch Set 7 : fix client fetching #
Messages
Total messages: 35 (10 generated)
PTAL
./run_isolated.py -vvv --cipd-package 'infra/tools/authutil/${platform}:latest'
'${CIPD_PATH}/authutil'
prints
90238 2016-06-04 07:29:40.849 D:
make_tree_read_only(/Users/nodir/gh/luci/luci-py/client/isolate_cache)
90238 2016-06-04 07:29:40.850 I: Profiling: Section Setup took 0.001 seconds
90238 2016-06-04 07:29:40.854 D:
make_tree_read_only(/Users/nodir/gh/luci/luci-py/client/cipd_cache/versions)
90238 2016-06-04 07:29:40.854 I: Profiling: Section Setup took 0.001 seconds
90238 2016-06-04 07:29:40.855 D:
make_tree_read_only(/Users/nodir/gh/luci/luci-py/client/cipd_cache/clients)
90238 2016-06-04 07:29:40.855 I: Profiling: Section Setup took 0.001 seconds
90238 2016-06-04 07:29:40.864 I:
rmtree(/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq)
90238 2016-06-04 07:29:40.864 D:
make_tree_deleteable(/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq)
90238 2016-06-04 07:29:40.865 D:
make_tree_writeable(/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq)
90238 2016-06-04 07:29:40.865 I: Installing packages
['infra/tools/authutil/${platform}:latest'] into
/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq
90238 2016-06-04 07:29:40.866 D: Running
[u'/Users/nodir/gh/luci/luci-py/client/cipd_cache/clients/c111be343c692e5285113a6b1c999887adbb268e',
'ensure', '-root', u'/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq',
'-list',
'/var/folders/00/1sgt0000h01000cxqpysvccm0075z8/T/cipd-ensure-list-GLE6bb',
'-verbose', '-cache-dir',
'/Users/nodir/gh/luci/luci-py/client/cipd_cache/cipd_internal']
90238 2016-06-04 07:29:40.879 D: cipd client: [P90242 00:29:40.879 remote.go:100
D] cipd: GET
https://chrome-infra-packages.appspot.com/_ah/api/repo/v1/instance/resolve?pa...
90238 2016-06-04 07:29:41.105 D: cipd client: [P90242 00:29:41.105 client.go:945
I] Packages to be installed:
90238 2016-06-04 07:29:41.105 D: cipd client: [P90242 00:29:41.105 client.go:947
I] infra/tools/authutil/mac-amd64:d0d876b3c7b9d3b75067f1591b3d743860c6000c
90238 2016-06-04 07:29:41.109 D: cipd client: [P90242 00:29:41.109 client.go:801
D] cipd: instance cache hit for
infra/tools/authutil/mac-amd64:d0d876b3c7b9d3b75067f1591b3d743860c6000c
90238 2016-06-04 07:29:41.119 D: cipd client: [P90242 00:29:41.118
deployer.go:131 I] Deploying
infra/tools/authutil/mac-amd64:d0d876b3c7b9d3b75067f1591b3d743860c6000c into
/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq
90238 2016-06-04 07:29:41.272 D: cipd client: [P90242 00:29:41.272
deployer.go:221 I] Successfully deployed
infra/tools/authutil/mac-amd64:d0d876b3c7b9d3b75067f1591b3d743860c6000c
90238 2016-06-04 07:29:41.276 D: cipd client: [P90242 00:29:41.276
client.go:1007 I] All changes applied.
90238 2016-06-04 07:29:41.279 I: Installing CIPD client and packages took 0
seconds
90238 2016-06-04 07:29:41.280 D:
make_tree_files_read_only(/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq)
90238 2016-06-04 07:29:41.281 I:
run_command([u'/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq/authutil'],
/Users/nodir/gh/luci/luci-py/client/isolated_runqdL7EA)
LUCI Authentication Utility
Usage: authutil [command] [arguments]
Commands:
help prints help about a command
info prints an email address associated with currently cached token
login performs interactive login flow
logout removes cached credentials
token prints an access token
Use "authutil help [command]" for more information about a command.
90238 2016-06-04 07:29:41.336 I: Waiting for proces exit
90238 2016-06-04 07:29:41.336 I: Profiling: Section RunTest took 0.055 seconds
90238 2016-06-04 07:29:41.336 I: Command finished with exit code 2 (0x2)
90238 2016-06-04 07:29:41.336 I:
rmtree(/Users/nodir/gh/luci/luci-py/client/isolated_runqdL7EA)
90238 2016-06-04 07:29:41.337 D:
make_tree_deleteable(/Users/nodir/gh/luci/luci-py/client/isolated_runqdL7EA)
90238 2016-06-04 07:29:41.337 I:
rmtree(/Users/nodir/gh/luci/luci-py/client/isolated_tmp7Dx3dc)
90238 2016-06-04 07:29:41.337 D:
make_tree_deleteable(/Users/nodir/gh/luci/luci-py/client/isolated_tmp7Dx3dc)
90238 2016-06-04 07:29:41.338 I: Result:
{"duration":0.0556337833404541,"exit_code":2,"had_hard_timeout":false,"internal_failure":null,"outputs_ref":null,"stats":{"cipd":{"duration":0.4257199764251709,"get_client_duration":0.009033918380737305}},"version":5}
90238 2016-06-04 07:29:41.338 I:
rmtree(/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq)
90238 2016-06-04 07:29:41.338 D:
make_tree_deleteable(/Users/nodir/gh/luci/luci-py/client/cipd_site_root4O1Wqq)
left some comments https://codereview.chromium.org/2037253002/diff/1/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode118 client/cipd.py:118: with tempfile.NamedTemporaryFile(prefix='cipd-ensure-list-') as list_file: keep it under cache_dir or site_root, not in global /tmp https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode123 client/cipd.py:123: list_file.flush() there's а chance this won't work on Windows due to file locking. Consider properly closing the file before passing to the cipd. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode143 client/cipd.py:143: 'Could not install packages; took more than %d seconds' % timeout) We should probably add this to the cipd client too :( It should be relatively simple, internal API supports context.Context already. https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:164: AssertionError if a parameter is requested in |command| but its value is not ValueError assertion errors must not happen in correctly constructed program (regardless of what's given as its input). https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:704: parser.set_defaults(cache='isolate_cache', cipd_cache='cipd_cache') I believe this will "leak" all existing isolate caches on all the bots when deployed. Either come up with a deployment strategy that involves migrating the directory cleanly, or just don't change it. https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:740: file_path.ensure_tree(root_dir, 0700) nit: do it after all options are validated https://codereview.chromium.org/2037253002/diff/1/client/utils/subprocess42.py File client/utils/subprocess42.py (right): https://codereview.chromium.org/2037253002/diff/1/client/utils/subprocess42.p... client/utils/subprocess42.py:653: If a chunk of data from one pipe does not end with the separator, and next I don't think that's a good idea (see below, also the code that uses yield_lines doesn't seem to rely on this). Also consider cleanly separating it into two processors: def split(....): splitters = defaultdict(splitter) for pipe, chunk in data: for line in splitters[pipe].feed(): yield pipe, line for pipe, splitter in sorted(splitters.iteritems()): for line in splitter.flush(): yield pipe, line https://codereview.chromium.org/2037253002/diff/1/client/utils/subprocess42.p... client/utils/subprocess42.py:672: if pending_chunks_pipe_name != pipe_name: why? interleaving of stdout and stderr in general is "random" (depends on CPU scheduling delays), it will produce "random" splits in the output https://codereview.chromium.org/2037253002/diff/1/client/utils/tools.py File client/utils/tools.py (right): https://codereview.chromium.org/2037253002/diff/1/client/utils/tools.py#newco... client/utils/tools.py:321: class Timeouter(object): not sure it's worth the dedicated class... I don't mind much though.
https://codereview.chromium.org/2037253002/diff/1/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode1 client/cipd.py:1: # Copyright 2012 The LUCI Authors. All rights reserved. 2012? https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode33 client/cipd.py:33: help=( () are not needed for str concatenation https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode123 client/cipd.py:123: list_file.flush() On 2016/06/06 21:32:13, Vadim Sh. wrote: > there's а chance this won't work on Windows due to file locking. Consider > properly closing the file before passing to the cipd. Vadim is right, this is broken on Windows. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode141 client/cipd.py:141: if pipe_name is None: It could also mean that one pipe closed, not necessarily that it timed out. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode179 client/cipd.py:179: }.get(platform.machine().lower()) https://github.com/luci/luci-py/blob/master/appengine/swarming/swarming_bot/a... this works on ARM too. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode212 client/cipd.py:212: result = template this line is not useful. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode215 client/cipd.py:215: return result this is not needed either https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode237 client/cipd.py:237: if res and res.get('status') == 'SUCCESS': I'd replace the whole function with: if not res: raise Error('%s: no response' % (fmt & args)) if res.get('status') != 'SUCCESS': raise Error('%s: %s' % ( fmt & args, res.get('error_message') or 'status is %s' % res.get('status'))) https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode285 client/cipd.py:285: res = net.url_open(fetch_url, stream=False) that assumes the client binary fits in memory, we're generally fine but it could be stretching it on a lower end rPi. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode334 client/cipd.py:334: version_digest = sha1.hexdigest() version_digest = hashlib.sha1(version).hexdigest() https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode352 client/cipd.py:352: client_cache.write(instance_id, res.iter_content(64 * 1024)) iter_content() doesn't matter here since stream=False is used. https://codereview.chromium.org/2037253002/diff/1/client/isolateserver.py File client/isolateserver.py (right): https://codereview.chromium.org/2037253002/diff/1/client/isolateserver.py#new... client/isolateserver.py:1531: """ if digest not in self._lru: raise CacheMiss(digest) since lru.LRUDict implements __contains__ and it is largely faster than doing a disk I/O. https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py File client/run_isolated.py (left): https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#oldc... client/run_isolated.py:507: 'version': 2, oops! https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:16: Any ${EXE_SUFFIX} on the command line will be replaced with ".exe" string on The isolate client uses EXECUTABLE_SUFFIX. It think it'd be less confusing to use the same wording. https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:375: # }, Add expectation about: 'cipd': { ... }, https://codereview.chromium.org/2037253002/diff/1/client/utils/subprocess42.py File client/utils/subprocess42.py (right): https://codereview.chromium.org/2037253002/diff/1/client/utils/subprocess42.p... client/utils/subprocess42.py:428: to = timeout() if timeout else None if you want to make this more readable, do: to = None if timeout: to = max(to - (time.time() - last_yield), 0) https://codereview.chromium.org/2037253002/diff/1/client/utils/tools.py File client/utils/tools.py (right): https://codereview.chromium.org/2037253002/diff/1/client/utils/tools.py#newco... client/utils/tools.py:321: class Timeouter(object): On 2016/06/06 21:32:14, Vadim Sh. wrote: > not sure it's worth the dedicated class... I don't mind much though. I agree.
I'm not excited about --cipd-client-package. The same way one cannot specify run_isolated version. I wish it was server controlled. Frankly, I'd rather inject the per-OS executable right into swarming_bot.zip. That's what I was thinking about with run_isolated in Go.
On 2016/06/06 23:53:57, M-A Ruel wrote: > I'm not excited about --cipd-client-package. The same way one cannot specify > run_isolated version. I wish it was server controlled. > > Frankly, I'd rather inject the per-OS executable right into swarming_bot.zip. > That's what I was thinking about with run_isolated in Go. we discussed this in IM and maruel is OK with this for now. To be fair, it is server-controlled via server settings. Task properties can specify alternative cipd client, but in reality likely only a handful of people familiar with CIPD will use this feature to test new versions of CIPD.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2037253002/diff/1/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode1 client/cipd.py:1: # Copyright 2012 The LUCI Authors. All rights reserved. On 2016/06/06 23:34:50, M-A Ruel wrote: > 2012? Done. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode33 client/cipd.py:33: help=( On 2016/06/06 23:34:50, M-A Ruel wrote: > () are not needed for str concatenation Done. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode118 client/cipd.py:118: with tempfile.NamedTemporaryFile(prefix='cipd-ensure-list-') as list_file: On 2016/06/06 21:32:13, Vadim Sh. wrote: > keep it under cache_dir or site_root, not in global /tmp Done. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode123 client/cipd.py:123: list_file.flush() On 2016/06/06 23:34:50, M-A Ruel wrote: > On 2016/06/06 21:32:13, Vadim Sh. wrote: > > there's а chance this won't work on Windows due to file locking. Consider > > properly closing the file before passing to the cipd. > > Vadim is right, this is broken on Windows. Done. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode141 client/cipd.py:141: if pipe_name is None: On 2016/06/06 23:34:51, M-A Ruel wrote: > It could also mean that one pipe closed, not necessarily that it timed out. Done https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode143 client/cipd.py:143: 'Could not install packages; took more than %d seconds' % timeout) On 2016/06/06 21:32:13, Vadim Sh. wrote: > We should probably add this to the cipd client too :( It should be relatively > simple, internal API supports context.Context already. I started doing that, but it turned out much more work than doing it here, primarily because cipd code does not check for ctx.Err(). Please consider adding timeout when you have time. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode179 client/cipd.py:179: }.get(platform.machine().lower()) On 2016/06/06 23:34:50, M-A Ruel wrote: > https://github.com/luci/luci-py/blob/master/appengine/swarming/swarming_bot/a... > > this works on ARM too. that function cannot be imported here because this code is in client/ and that one is in swarming_bot. I've adapted that one line https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode212 client/cipd.py:212: result = template On 2016/06/06 23:34:50, M-A Ruel wrote: > this line is not useful. Done. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode215 client/cipd.py:215: return result On 2016/06/06 23:34:50, M-A Ruel wrote: > this is not needed either Done https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode237 client/cipd.py:237: if res and res.get('status') == 'SUCCESS': On 2016/06/06 23:34:50, M-A Ruel wrote: > I'd replace the whole function with: > > if not res: > raise Error('%s: no response' % (fmt & args)) > if res.get('status') != 'SUCCESS': > raise Error('%s: %s' % ( > fmt & args, > res.get('error_message') or 'status is %s' % res.get('status'))) Done. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode285 client/cipd.py:285: res = net.url_open(fetch_url, stream=False) On 2016/06/06 23:34:50, M-A Ruel wrote: > that assumes the client binary fits in memory, we're generally fine but it could > be stretching it on a lower end rPi. I reworked fetch_client_binary to (url_open() | disk_cache.write) with retries and exponential back-off and added more exception handling https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode334 client/cipd.py:334: version_digest = sha1.hexdigest() On 2016/06/06 23:34:50, M-A Ruel wrote: > version_digest = hashlib.sha1(version).hexdigest() Done. https://codereview.chromium.org/2037253002/diff/1/client/cipd.py#newcode352 client/cipd.py:352: client_cache.write(instance_id, res.iter_content(64 * 1024)) On 2016/06/06 23:34:50, M-A Ruel wrote: > iter_content() doesn't matter here since stream=False is used. changed to streaming https://codereview.chromium.org/2037253002/diff/1/client/isolateserver.py File client/isolateserver.py (right): https://codereview.chromium.org/2037253002/diff/1/client/isolateserver.py#new... client/isolateserver.py:1531: """ On 2016/06/06 23:34:51, M-A Ruel wrote: > if digest not in self._lru: > raise CacheMiss(digest) > > > since lru.LRUDict implements __contains__ and it is largely faster than doing a > disk I/O. Done https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:16: Any ${EXE_SUFFIX} on the command line will be replaced with ".exe" string on On 2016/06/06 23:34:51, M-A Ruel wrote: > The isolate client uses EXECUTABLE_SUFFIX. It think it'd be less confusing to > use the same wording. Done https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:164: AssertionError if a parameter is requested in |command| but its value is not On 2016/06/06 21:32:13, Vadim Sh. wrote: > ValueError > > assertion errors must not happen in correctly constructed program (regardless of > what's given as its input). the code is correct. The cmdline is checked before calling this method. I've changed to ValueError https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:375: # }, On 2016/06/06 23:34:51, M-A Ruel wrote: > Add expectation about: > 'cipd': { > ... > }, Done. https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:704: parser.set_defaults(cache='isolate_cache', cipd_cache='cipd_cache') On 2016/06/06 21:32:14, Vadim Sh. wrote: > I believe this will "leak" all existing isolate caches on all the bots when > deployed. > > Either come up with a deployment strategy that involves migrating the directory > cleanly, or just don't change it. Done. https://codereview.chromium.org/2037253002/diff/1/client/run_isolated.py#newc... client/run_isolated.py:740: file_path.ensure_tree(root_dir, 0700) On 2016/06/06 21:32:14, Vadim Sh. wrote: > nit: do it after all options are validated Done. https://codereview.chromium.org/2037253002/diff/1/client/utils/subprocess42.py File client/utils/subprocess42.py (right): https://codereview.chromium.org/2037253002/diff/1/client/utils/subprocess42.p... client/utils/subprocess42.py:428: to = timeout() if timeout else None On 2016/06/06 23:34:51, M-A Ruel wrote: > if you want to make this more readable, do: > > to = None > if timeout: > to = max(to - (time.time() - last_yield), 0) to be clear: the purpose of the change was to allow timeout() function to return None, not to make it more readable. Still I've adapted "if" statement; it looks better now https://codereview.chromium.org/2037253002/diff/1/client/utils/subprocess42.p... client/utils/subprocess42.py:653: If a chunk of data from one pipe does not end with the separator, and next On 2016/06/06 21:32:14, Vadim Sh. wrote: > I don't think that's a good idea (see below, also the code that uses yield_lines > doesn't seem to rely on this). > > Also consider cleanly separating it into two processors: > > def split(....): > splitters = defaultdict(splitter) > for pipe, chunk in data: > for line in splitters[pipe].feed(): > yield pipe, line > for pipe, splitter in sorted(splitters.iteritems()): > for line in splitter.flush(): > yield pipe, line the only reason I did this is to buffer less, but I guess it is fine. done https://codereview.chromium.org/2037253002/diff/1/client/utils/subprocess42.p... client/utils/subprocess42.py:672: if pending_chunks_pipe_name != pipe_name: On 2016/06/06 21:32:14, Vadim Sh. wrote: > why? interleaving of stdout and stderr in general is "random" (depends on CPU > scheduling delays), it will produce "random" splits in the output removed, simplified https://codereview.chromium.org/2037253002/diff/1/client/utils/tools.py File client/utils/tools.py (right): https://codereview.chromium.org/2037253002/diff/1/client/utils/tools.py#newco... client/utils/tools.py:321: class Timeouter(object): On 2016/06/06 23:34:51, M-A Ruel wrote: > On 2016/06/06 21:32:14, Vadim Sh. wrote: > > not sure it's worth the dedicated class... I don't mind much though. > > I agree. changed to a function that returns a closure
https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode42 client/cipd.py:42: default='https://chrome-infra-packages.appspot.com') Please remove. We do not reference the chromium instances by default. The current exception is luci-config and I'd like it to see go away too (be configurable, without a default value). https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode63 client/cipd.py:63: 'Default: "%default".') there's no defaul? https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode200 client/cipd.py:200: platform_arch = 'amd64' if sys.maxsize > 2**32 else '386' that's still highly incorrect on arm. amd64 and 386 are odd choices of keywords (but I guess it's too late to change). https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode204 client/cipd.py:204: def get_os_ver(): I wonder what's the value for Windows and OSX, especially that the way the string is created for Windows is useless in practice. https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode381 client/cipd.py:381: instance_cache = isolateserver.DiskCache( The instance should outlive the mapped files https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode395 client/cipd.py:395: # A single bot can run multiple swarming clients, but ATM they do not share s/single bot/single host/ s/clients/bots/ s/share cache/share same root bot directory/ https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode400 client/cipd.py:400: file_path.hardlink(cached_binary_path, binary_path) I'd prefer to use instance_cache.hardlink() https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode401 client/cipd.py:401: fs.chmod(binary_path, 0711) # -rwx--x--x Why write bit set? https://codereview.chromium.org/2037253002/diff/60001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/60001/client/run_isolated.py#... client/run_isolated.py:360: # 'duration': 0., it should also log if the CIPD packages were hot or cold and the associated size. https://codereview.chromium.org/2037253002/diff/60001/client/run_isolated.py#... client/run_isolated.py:626: file_path.ensure_tree(site_root, 0777) other doesn't need access for sure https://codereview.chromium.org/2037253002/diff/60001/client/run_isolated.py#... client/run_isolated.py:762: print ex.message >> sys.stderr,
https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode200 client/cipd.py:200: platform_arch = 'amd64' if sys.maxsize > 2**32 else '386' On 2016/06/07 20:43:39, M-A Ruel wrote: > that's still highly incorrect on arm. amd64 and 386 are odd choices of keywords > (but I guess it's too late to change). That's what Golang is uses :) I adopted it for cipd too. https://golang.org/dl/ For arm they use armv6l (I guess it's the minimal instruction set they need).
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode42 client/cipd.py:42: default='https://chrome-infra-packages.appspot.com') On 2016/06/07 20:43:39, M-A Ruel wrote: > Please remove. We do not reference the chromium instances by default. The > current exception is luci-config and I'd like it to see go away too (be > configurable, without a default value). Done. https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode63 client/cipd.py:63: 'Default: "%default".') On 2016/06/07 20:43:39, M-A Ruel wrote: > there's no defaul? https://cs.chromium.org/chromium/infra/luci/client/isolateserver.py?q=isolate... does not have default either defaults for both options are set by the caller. I've set it to '' so having %default does not hurt (it will display "" if not set) https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode200 client/cipd.py:200: platform_arch = 'amd64' if sys.maxsize > 2**32 else '386' On 2016/06/07 20:43:39, M-A Ruel wrote: > that's still highly incorrect on arm. Done. Adapted from https://cs.chromium.org/chromium/infra/luci/appengine/swarming/swarming_bot/a... > amd64 and 386 are odd choices of keywords > (but I guess it's too late to change). it is not late because we actually never created cipd packages for arm https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode204 client/cipd.py:204: def get_os_ver(): On 2016/06/07 20:43:39, M-A Ruel wrote: > I wonder what's the value for Windows and OSX See examples below > , especially that the way the > string is created for Windows is useless in practice. Ask Vadim. I don't know what is the value of this string, but i added it for compatibility with https://chromium.googlesource.com/infra/infra/+/aaf9586/build/README.md https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode381 client/cipd.py:381: instance_cache = isolateserver.DiskCache( On 2016/06/07 20:43:39, M-A Ruel wrote: > The instance should outlive the mapped files I don't understand this comment. what is the difference between "instance" and "mapped files"? why wouldn't it outlive if we have exclusive access to the cache? https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode395 client/cipd.py:395: # A single bot can run multiple swarming clients, but ATM they do not share On 2016/06/07 20:43:39, M-A Ruel wrote: > s/single bot/single host/ > s/clients/bots/ > s/share cache/share same root bot directory/ Done. https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode400 client/cipd.py:400: file_path.hardlink(cached_binary_path, binary_path) On 2016/06/07 20:43:39, M-A Ruel wrote: > I'd prefer to use instance_cache.hardlink() yeah, that's better. I thought hardlink allows to _put_ a file into the cache using a hardlink, not vice versa https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode401 client/cipd.py:401: fs.chmod(binary_path, 0711) # -rwx--x--x On 2016/06/07 20:43:39, M-A Ruel wrote: > Why write bit set? I assumed you cannot delete a file without w bit. Turns out it is false. Removed write bit https://codereview.chromium.org/2037253002/diff/60001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/60001/client/run_isolated.py#... client/run_isolated.py:360: # 'duration': 0., On 2016/06/07 20:43:40, M-A Ruel wrote: > it should also log if the CIPD packages were hot or cold and the associated > size. cipd does not have that :( I can that in a separate CL though https://codereview.chromium.org/2037253002/diff/60001/client/run_isolated.py#... client/run_isolated.py:626: file_path.ensure_tree(site_root, 0777) On 2016/06/07 20:43:40, M-A Ruel wrote: > other doesn't need access for sure Done. https://codereview.chromium.org/2037253002/diff/60001/client/run_isolated.py#... client/run_isolated.py:762: print ex.message On 2016/06/07 20:43:40, M-A Ruel wrote: > >> sys.stderr, Done.
https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode381 client/cipd.py:381: instance_cache = isolateserver.DiskCache( On 2016/06/07 21:51:37, nodir wrote: > On 2016/06/07 20:43:39, M-A Ruel wrote: > > The instance should outlive the mapped files > > I don't understand this comment. what is the difference between "instance" and > "mapped files"? why wouldn't it outlive if we have exclusive access to the > cache? instance = object. This means instance_cache should be created by the caller. you should use with version_cache: so it activates its initialization code. Right now it's not doing much but it could later. https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode401 client/cipd.py:401: fs.chmod(binary_path, 0711) # -rwx--x--x On 2016/06/07 21:51:37, nodir wrote: > On 2016/06/07 20:43:39, M-A Ruel wrote: > > Why write bit set? > > I assumed you cannot delete a file without w bit. Turns out it is false. Removed > write bit In practice, that is not true on Windows; as removing the w bit *add* the read_only bit on Windows. And unlike on POSIX, where the directory ACL controls which file can be deleted, on Windows it's the file's ACL that decide if the file can be deleted. https://codereview.chromium.org/2037253002/diff/100001/client/isolateserver.py File client/isolateserver.py (right): https://codereview.chromium.org/2037253002/diff/100001/client/isolateserver.p... client/isolateserver.py:1537: def item_path(self, digest): It's not needed. https://codereview.chromium.org/2037253002/diff/100001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/100001/client/run_isolated.py... client/run_isolated.py:606: assert not client_cache or isinstance(client_cache, isolateserver.DiskCache) it shouldn't matter even if it is a isolateserver.LocalCache https://codereview.chromium.org/2037253002/diff/100001/client/run_isolated.py... client/run_isolated.py:618: client = cipd.get_client( so basically, the cache gets created here and with cache_obj: lines 618~642 so that the __exit__ is called after the yield resumed.
https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode381 client/cipd.py:381: instance_cache = isolateserver.DiskCache( On 2016/06/08 20:37:39, M-A Ruel wrote: > On 2016/06/07 21:51:37, nodir wrote: > > On 2016/06/07 20:43:39, M-A Ruel wrote: > > > The instance should outlive the mapped files > > > > I don't understand this comment. what is the difference between "instance" and > > "mapped files"? why wouldn't it outlive if we have exclusive access to the > > cache? > > instance = object. This means instance_cache should be created by the caller. Made get_client return a context manager that yields the client. > you should use > with version_cache: > so it activates its initialization code. Right now it's not doing much but it > could later. Added with-clause for version_cache https://codereview.chromium.org/2037253002/diff/60001/client/cipd.py#newcode401 client/cipd.py:401: fs.chmod(binary_path, 0711) # -rwx--x--x On 2016/06/08 20:37:39, M-A Ruel wrote: > On 2016/06/07 21:51:37, nodir wrote: > > On 2016/06/07 20:43:39, M-A Ruel wrote: > > > Why write bit set? > > > > I assumed you cannot delete a file without w bit. Turns out it is false. > Removed > > write bit > > In practice, that is not true on Windows; as removing the w bit *add* the > read_only bit on Windows. And unlike on POSIX, where the directory ACL controls > which file can be deleted, on Windows it's the file's ACL that decide if the > file can be deleted. I've sshed to a windows machine, created a file with 0111 permissions, verified that oct(os.stat('the file').st_mode & 0777) == '0111' and then ran os.remove on it. it was deleted successfully, so current code is probably fine https://codereview.chromium.org/2037253002/diff/100001/client/isolateserver.py File client/isolateserver.py (right): https://codereview.chromium.org/2037253002/diff/100001/client/isolateserver.p... client/isolateserver.py:1537: def item_path(self, digest): On 2016/06/08 20:37:39, M-A Ruel wrote: > It's not needed. Done. https://codereview.chromium.org/2037253002/diff/100001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/100001/client/run_isolated.py... client/run_isolated.py:606: assert not client_cache or isinstance(client_cache, isolateserver.DiskCache) On 2016/06/08 20:37:40, M-A Ruel wrote: > it shouldn't matter even if it is a isolateserver.LocalCache I forgot to remove this parameter, removed. It did matter because I used hardlink or item_path https://codereview.chromium.org/2037253002/diff/100001/client/run_isolated.py... client/run_isolated.py:618: client = cipd.get_client( On 2016/06/08 20:37:40, M-A Ruel wrote: > so basically, the cache gets created here and > > with cache_obj: > > lines 618~642 so that the __exit__ is called after the yield resumed. I did something like that without exposing diskcache to the caller of get_client
ping? the CL is being dragged for almost a week
Description was changed from
==========
run_isolated.py: install CIPD packages
add cipd options to run_isolated.py:
--cipd-package: package name and version, both required. Can be specified
multiple times
--cipd-server: URL of CIPD server
--cipd-client-package: CIPD client package to install
--cipd-cache: cache dir for CIPD clients, client versions and packages
expand ${platform} and ${os_ver} parameters in package name with values
read from current machine.
expand ${CIPD_PACKAGE} and ${EXE_SUFFIX} parameters on command line with
path to CIPD package installation dir and (.exe|) suffix.
cipd client installation:
- lookup client version (typically a git_revision tag) in version cache
(typically DiskCache)
- on cache miss, resolve version to instance id using CIPD backend API
- lookup client instance id in client cache (DiskCache)
- on cache miss, fetch it from cipd backend / GS, put to disk cache and
make executable
package installation:
- put list of packages to a file
- use cipd-ensure to install packages
- pass -verbose and log all cipd-ensure output: stdout->INFO,
stderr->DEBUGpass -verbose and log all cipd-ensure output:
stdout->INFO, stderr->DEBUG. Typically there is only stderr
- set -cache-dir flag to "<cipd-cache flag value>/packages"
R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
==========
to
==========
run_isolated.py: install CIPD packages
add cipd options to run_isolated.py:
--cipd-package: package name and version, both required. Can be specified
multiple times
--cipd-server: URL of CIPD server
--cipd-client-package: CIPD client package to install
--cipd-cache: cache dir for CIPD clients, client versions and packages
expand ${platform} and ${os_ver} parameters in package name with values
read from current machine.
expand ${CIPD_PACKAGE} and ${EXECUTABLE_SUFFIX} parameters on command line with
path to CIPD package installation dir and (.exe|) suffix.
cipd client installation:
- lookup client version (typically a git_revision tag) in version cache
(typically DiskCache)
- on cache miss, resolve version to instance id using CIPD backend API
- lookup client instance id in client cache (DiskCache)
- on cache miss, fetch it from cipd backend / GS, put to disk cache and
make executable
package installation:
- put list of packages to a file
- use cipd-ensure to install packages
- pass -verbose and log all cipd-ensure output: stdout->INFO,
stderr->DEBUGpass -verbose and log all cipd-ensure output:
stdout->INFO, stderr->DEBUG. Typically there is only stderr
- set -cache-dir flag to "<cipd-cache flag value>/packages"
R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
==========
looks like I had unsent changes :/ Fixed CL description for EXECUTABLE_SUFFIX. https://codereview.chromium.org/2037253002/diff/120001/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/120001/client/cipd.py#newcode126 client/cipd.py:126: list_file_closed = False not needed https://codereview.chromium.org/2037253002/diff/120001/client/cipd.py#newcode133 client/cipd.py:133: list_file_closed = True list_file_handle = None but in practice I'd prefer a second try/finally to close this handle. https://codereview.chromium.org/2037253002/diff/120001/client/cipd.py#newcode370 client/cipd.py:370: # Convert |version| to a string that may be used as a filename in disk cache wrap at 80 cols
https://codereview.chromium.org/2037253002/diff/120001/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/120001/client/cipd.py#newcode126 client/cipd.py:126: list_file_closed = False On 2016/06/09 16:21:04, M-A Ruel wrote: > not needed Done https://codereview.chromium.org/2037253002/diff/120001/client/cipd.py#newcode133 client/cipd.py:133: list_file_closed = True On 2016/06/09 16:21:03, M-A Ruel wrote: > list_file_handle = None > > but in practice I'd prefer a second try/finally to close this handle. Done. https://codereview.chromium.org/2037253002/diff/120001/client/cipd.py#newcode370 client/cipd.py:370: # Convert |version| to a string that may be used as a filename in disk cache On 2016/06/09 16:21:03, M-A Ruel wrote: > wrap at 80 cols Done.
lgtm with nits https://codereview.chromium.org/2037253002/diff/140001/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/140001/client/cipd.py#newcode391 client/cipd.py:391: # A single host can run multiple swarming bots, but ATM they do not share same 80 cols https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py... client/run_isolated.py:360: # 'duration': 0., 'get_client_duration' ? https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py... client/run_isolated.py:624: file_path.make_tree_writeable(site_root) This function call is not needed, since the tree was just created with 0770.
lgtm with nits
https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py... client/run_isolated.py:360: # 'duration': 0., On 2016/06/09 18:31:11, M-A Ruel wrote: > 'get_client_duration' ? There is field in OperationStats for get_client_duration. Shall we define new model?
https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py... client/run_isolated.py:360: # 'duration': 0., On 2016/06/09 18:49:05, nodir wrote: > On 2016/06/09 18:31:11, M-A Ruel wrote: > > 'get_client_duration' ? > > There is field in OperationStats for get_client_duration. Shall we define new > model? No, I just mean adding it here since this value will be added.
https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py... client/run_isolated.py:360: # 'duration': 0., On 2016/06/09 18:52:55, M-A Ruel wrote: > On 2016/06/09 18:49:05, nodir wrote: > > On 2016/06/09 18:31:11, M-A Ruel wrote: > > > 'get_client_duration' ? > > > > There is field in OperationStats for get_client_duration. Shall we define new > > model? > > No, I just mean adding it here since this value will be added. OK
On 2016/06/09 18:57:06, nodir wrote: > > No, I just mean adding it here since this value will be added. > > OK still lgtm
https://codereview.chromium.org/2037253002/diff/140001/client/cipd.py File client/cipd.py (right): https://codereview.chromium.org/2037253002/diff/140001/client/cipd.py#newcode391 client/cipd.py:391: # A single host can run multiple swarming bots, but ATM they do not share same On 2016/06/09 18:31:11, M-A Ruel wrote: > 80 cols Done. https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py File client/run_isolated.py (right): https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py... client/run_isolated.py:360: # 'duration': 0., On 2016/06/09 18:57:06, nodir wrote: > On 2016/06/09 18:52:55, M-A Ruel wrote: > > On 2016/06/09 18:49:05, nodir wrote: > > > On 2016/06/09 18:31:11, M-A Ruel wrote: > > > > 'get_client_duration' ? > > > > > > There is field in OperationStats for get_client_duration. Shall we define > new > > > model? > > > > No, I just mean adding it here since this value will be added. > > OK Done. https://codereview.chromium.org/2037253002/diff/140001/client/run_isolated.py... client/run_isolated.py:624: file_path.make_tree_writeable(site_root) On 2016/06/09 18:31:11, M-A Ruel wrote: > This function call is not needed, since the tree was just created with 0770. Done.
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/2037253002/#ps160001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037253002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Luci-py Presubmit on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Luci-py%20Presubmit/bui...)
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/2037253002/#ps180001 (title: "fix client fetching")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2037253002/180001
Message was sent while issue was closed.
Description was changed from
==========
run_isolated.py: install CIPD packages
add cipd options to run_isolated.py:
--cipd-package: package name and version, both required. Can be specified
multiple times
--cipd-server: URL of CIPD server
--cipd-client-package: CIPD client package to install
--cipd-cache: cache dir for CIPD clients, client versions and packages
expand ${platform} and ${os_ver} parameters in package name with values
read from current machine.
expand ${CIPD_PACKAGE} and ${EXECUTABLE_SUFFIX} parameters on command line with
path to CIPD package installation dir and (.exe|) suffix.
cipd client installation:
- lookup client version (typically a git_revision tag) in version cache
(typically DiskCache)
- on cache miss, resolve version to instance id using CIPD backend API
- lookup client instance id in client cache (DiskCache)
- on cache miss, fetch it from cipd backend / GS, put to disk cache and
make executable
package installation:
- put list of packages to a file
- use cipd-ensure to install packages
- pass -verbose and log all cipd-ensure output: stdout->INFO,
stderr->DEBUGpass -verbose and log all cipd-ensure output:
stdout->INFO, stderr->DEBUG. Typically there is only stderr
- set -cache-dir flag to "<cipd-cache flag value>/packages"
R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
==========
to
==========
run_isolated.py: install CIPD packages
add cipd options to run_isolated.py:
--cipd-package: package name and version, both required. Can be specified
multiple times
--cipd-server: URL of CIPD server
--cipd-client-package: CIPD client package to install
--cipd-cache: cache dir for CIPD clients, client versions and packages
expand ${platform} and ${os_ver} parameters in package name with values
read from current machine.
expand ${CIPD_PACKAGE} and ${EXECUTABLE_SUFFIX} parameters on command line with
path to CIPD package installation dir and (.exe|) suffix.
cipd client installation:
- lookup client version (typically a git_revision tag) in version cache
(typically DiskCache)
- on cache miss, resolve version to instance id using CIPD backend API
- lookup client instance id in client cache (DiskCache)
- on cache miss, fetch it from cipd backend / GS, put to disk cache and
make executable
package installation:
- put list of packages to a file
- use cipd-ensure to install packages
- pass -verbose and log all cipd-ensure output: stdout->INFO,
stderr->DEBUGpass -verbose and log all cipd-ensure output:
stdout->INFO, stderr->DEBUG. Typically there is only stderr
- set -cache-dir flag to "<cipd-cache flag value>/packages"
R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
Committed:
https://github.com/luci/luci-py/commit/45eab10a17a55f1978f3bfa02c35457dee1c64e4
==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://github.com/luci/luci-py/commit/45eab10a17a55f1978f3bfa02c35457dee1c64e4 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
