swarming: custom cipd package paths

swarming-cipd: custom installation paths

run_isolated.py: replace --cipd-package option with --cipd-package-list
which is a path to a JSON file that contains list of packages and their paths.
Create a site_root per unique package path relative to run dir. Remove
${CIPD_PATH} parameter because packages are installed always under run
dir at deterministic location.

isolatedserver.py: before writing a file on disk, check if it exists; if
does, raise AlreadyExists.  This is consistent with current behavior:
CMDdownload verifies that destination directory does not exist.

run_isolated.py: split --cipd-client-package to --cipd-client-package and --cipd-client-version.

Add "path" to CipdPackage ndb model and RPC entity. Validate it.
Propagate it to run_isolated.py.

Update user_task.html to group packages by installation path.

R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
Committed: https://github.com/luci/luci-py/commit/296ce9e86230c641edf33cb6792a8d4648dfeee1

[nodir, 2016-06-15 05:54:23]

Description was changed from

==========
swarming: custom cipd package paths

run_isolated.py: replace --cipd-package option with --cipd-package-list
which is a path to a JSON file that contains list of packages and their paths.
Create a site_root per unique package path relative to run dir. Remove
${CIPD_PATH} parameter because packages are installed always under run
dir at determinstic location.

isolatedserver.py: before writing a file on disk, check if it exists; if
does, raise AlreadyExists.  This is consistent with current behavior:
CMDdownload verifies that destination directory does not exist.

Add "path" to CipdPackage ndb model and RPC entity. Validate it.
Propagate it to run_isolated.py.

BUG=
==========

to

==========
swarming: custom cipd package paths

run_isolated.py: replace --cipd-package option with --cipd-package-list
which is a path to a JSON file that contains list of packages and their paths.
Create a site_root per unique package path relative to run dir. Remove
${CIPD_PATH} parameter because packages are installed always under run
dir at deterministic location.

isolatedserver.py: before writing a file on disk, check if it exists; if
does, raise AlreadyExists.  This is consistent with current behavior:
CMDdownload verifies that destination directory does not exist.

run_isolated.py: split --cipd-client-package to --cipd-client-package and
--cipd-client-version.

Add "path" to CipdPackage ndb model and RPC entity. Validate it.
Propagate it to run_isolated.py.

Update user_task.html to group packages by installation path.

R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
==========

[nodir, 2016-06-15 05:54:24]

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

[nodir, 2016-06-15 05:54:32]

PTAL

[nodir, 2016-06-15 05:54:41]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 05:54:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069903003/1

[commit-bot: I haz the power, 2016-06-15 05:57:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 05:58:00]

Dry run: This issue passed the CQ dry run.

[nodir, 2016-06-15 06:16:41]

Description was changed from

==========
swarming: custom cipd package paths

run_isolated.py: replace --cipd-package option with --cipd-package-list
which is a path to a JSON file that contains list of packages and their paths.
Create a site_root per unique package path relative to run dir. Remove
${CIPD_PATH} parameter because packages are installed always under run
dir at deterministic location.

isolatedserver.py: before writing a file on disk, check if it exists; if
does, raise AlreadyExists.  This is consistent with current behavior:
CMDdownload verifies that destination directory does not exist.

run_isolated.py: split --cipd-client-package to --cipd-client-package and
--cipd-client-version.

Add "path" to CipdPackage ndb model and RPC entity. Validate it.
Propagate it to run_isolated.py.

Update user_task.html to group packages by installation path.

R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
==========

to

==========
swarming-cipd: custom installation paths

run_isolated.py: replace --cipd-package option with --cipd-package-list
which is a path to a JSON file that contains list of packages and their paths.
Create a site_root per unique package path relative to run dir. Remove
${CIPD_PATH} parameter because packages are installed always under run
dir at deterministic location.

isolatedserver.py: before writing a file on disk, check if it exists; if
does, raise AlreadyExists.  This is consistent with current behavior:
CMDdownload verifies that destination directory does not exist.

run_isolated.py: split --cipd-client-package to --cipd-client-package and
--cipd-client-version.

Add "path" to CipdPackage ndb model and RPC entity. Validate it.
Propagate it to run_isolated.py.

Update user_task.html to group packages by installation path.

R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
==========

[nodir, 2016-06-15 06:16:53]

Patchset #2 (id:20001) has been deleted

[nodir, 2016-06-15 06:20:28]

Deployed:

linux:
https://2137-cf69edd-tainted-nodir-dot-chromium-swarm-dev.appspot.com/user/ta...
mac:
https://2137-cf69edd-tainted-nodir-dot-chromium-swarm-dev.appspot.com/user/ta...
win:
https://2137-cf69edd-tainted-nodir-dot-chromium-swarm-dev.appspot.com/user/ta...

[M-A Ruel, 2016-06-15 17:28:27]

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:277: if self.path:
Use a validator instead.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:332: packages[p.path or '.'].append(p)
I don't like that path=None and path='.' have the same outcome. This means one
can specify the same task in two different ways and get different
properties_hash. I prefer to unconditionally store '.', never None. This
simplify the code, as in user_task.html.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:333: for pkgs in packages.values():
itervalues()

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:336: 
2 lines

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/task_runner.py:110: # Package JSON
object format matches.
Can't parse this sentence

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/task_runner.py:113: with
open(package_list, 'w') as f:
'wb'

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:93: # Path to dir, relative to the run dir,
where to install the package.
root dir?
because this can be confused with relative_cwd.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:94: # If empty, the package will be
installed in run dir.
in run dir?
will be installed a the root of the mapped directory.

Worth a note about conflicting file?

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:112: # client_package.path is ignored.
must be empty

https://codereview.chromium.org/2069903003/diff/40001/client/isolateserver.py
File client/isolateserver.py (right):

https://codereview.chromium.org/2069903003/diff/40001/client/isolateserver.py...
client/isolateserver.py:228: if os.path.exists(outfile):
I'm not super excited about adding another system call here, could we trap the
exception generated by os.symlink()?

import errno

try:
  os.symlink(...)
except OSError as e:
  if e.errno == errno.EEXIST:
    raise AlreadyExists(...)
  raise

tested to work on both Ubuntu and OSX

https://codereview.chromium.org/2069903003/diff/40001/client/isolateserver.py...
client/isolateserver.py:1977: if os.path.exists(dest):
this one is harder to skip since the actual execution is implementation
dependent.

[nodir, 2016-06-15 17:53:42]

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:277: if self.path:
On 2016/06/15 17:28:27, M-A Ruel wrote:
> Use a validator instead.

Done.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:332: packages[p.path or '.'].append(p)
On 2016/06/15 17:28:27, M-A Ruel wrote:
> I don't like that path=None and path='.' have the same outcome. This means one
> can specify the same task in two different ways and get different
> properties_hash. I prefer to unconditionally store '.', never None. This
> simplify the code, as in user_task.html.

Good point, done.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:333: for pkgs in packages.values():
On 2016/06/15 17:28:27, M-A Ruel wrote:
> itervalues()

Done.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_request.py:336: 
On 2016/06/15 17:28:27, M-A Ruel wrote:
> 2 lines

Done.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
File appengine/swarming/swarming_bot/bot_code/task_runner.py (right):

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/task_runner.py:110: # Package JSON
object format matches.
On 2016/06/15 17:28:27, M-A Ruel wrote:
> Can't parse this sentence

Done.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_bot/bot_code/task_runner.py:113: with
open(package_list, 'w') as f:
On 2016/06/15 17:28:27, M-A Ruel wrote:
> 'wb'

Done.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:93: # Path to dir, relative to the run dir,
where to install the package.
On 2016/06/15 17:28:27, M-A Ruel wrote:
> root dir?
> because this can be confused with relative_cwd.

Done.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:94: # If empty, the package will be
installed in run dir.
On 2016/06/15 17:28:27, M-A Ruel wrote:
> in run dir?
> will be installed a the root of the mapped directory.
> 
> Worth a note about conflicting file?

Done.

https://codereview.chromium.org/2069903003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:112: # client_package.path is ignored.
On 2016/06/15 17:28:27, M-A Ruel wrote:
> must be empty

Done.

https://codereview.chromium.org/2069903003/diff/40001/client/isolateserver.py
File client/isolateserver.py (right):

https://codereview.chromium.org/2069903003/diff/40001/client/isolateserver.py...
client/isolateserver.py:228: if os.path.exists(outfile):
On 2016/06/15 17:28:27, M-A Ruel wrote:
> I'm not super excited about adding another system call here, could we trap the
> exception generated by os.symlink()?
> 
> import errno
> 
> try:
>   os.symlink(...)
> except OSError as e:
>   if e.errno == errno.EEXIST:
>     raise AlreadyExists(...)
>   raise
> 
> tested to work on both Ubuntu and OSX

Done

https://codereview.chromium.org/2069903003/diff/40001/client/isolateserver.py...
client/isolateserver.py:1977: if os.path.exists(dest):
On 2016/06/15 17:28:27, M-A Ruel wrote:
> this one is harder to skip since the actual execution is implementation
> dependent.

Acknowledged.

[M-A Ruel, 2016-06-15 18:24:09]

+Tim FYI

lgtm

https://codereview.chromium.org/2069903003/diff/60001/client/isolateserver.py
File client/isolateserver.py (right):

https://codereview.chromium.org/2069903003/diff/60001/client/isolateserver.py...
client/isolateserver.py:8: __version__ = '0.4.8'
bump

[nodir, 2016-06-15 18:27:29]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-06-15 18:27:30]

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/2069903003/#ps80001
(title: "bumped versions")

[commit-bot: I haz the power, 2016-06-15 18:27:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069903003/80001

[commit-bot: I haz the power, 2016-06-15 18:32:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 18:32:53]

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

[nodir, 2016-06-15 20:17:41]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-06-15 20:17:42]

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/2069903003/#ps100001
(title: "fix _validate_cipd_path")

[commit-bot: I haz the power, 2016-06-15 20:17:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069903003/100001

[commit-bot: I haz the power, 2016-06-15 20:35:23]

Description was changed from

==========
swarming-cipd: custom installation paths

run_isolated.py: replace --cipd-package option with --cipd-package-list
which is a path to a JSON file that contains list of packages and their paths.
Create a site_root per unique package path relative to run dir. Remove
${CIPD_PATH} parameter because packages are installed always under run
dir at deterministic location.

isolatedserver.py: before writing a file on disk, check if it exists; if
does, raise AlreadyExists.  This is consistent with current behavior:
CMDdownload verifies that destination directory does not exist.

run_isolated.py: split --cipd-client-package to --cipd-client-package and
--cipd-client-version.

Add "path" to CipdPackage ndb model and RPC entity. Validate it.
Propagate it to run_isolated.py.

Update user_task.html to group packages by installation path.

R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022
==========

to

==========
swarming-cipd: custom installation paths

run_isolated.py: replace --cipd-package option with --cipd-package-list
which is a path to a JSON file that contains list of packages and their paths.
Create a site_root per unique package path relative to run dir. Remove
${CIPD_PATH} parameter because packages are installed always under run
dir at deterministic location.

isolatedserver.py: before writing a file on disk, check if it exists; if
does, raise AlreadyExists.  This is consistent with current behavior:
CMDdownload verifies that destination directory does not exist.

run_isolated.py: split --cipd-client-package to --cipd-client-package and
--cipd-client-version.

Add "path" to CipdPackage ndb model and RPC entity. Validate it.
Propagate it to run_isolated.py.

Update user_task.html to group packages by installation path.

R=maruel@chromium.org, vadimsh@chromium.org
BUG=601022

Committed:
https://github.com/luci/luci-py/commit/296ce9e86230c641edf33cb6792a8d4648dfeee1
==========

[commit-bot: I haz the power, 2016-06-15 20:35:24]

Committed patchset #5 (id:100001) as
https://github.com/luci/luci-py/commit/296ce9e86230c641edf33cb6792a8d4648dfeee1

[nodir, 2016-06-15 20:58:15]

https://codereview.chromium.org/2069903003/diff/60001/client/isolateserver.py
File client/isolateserver.py (right):

https://codereview.chromium.org/2069903003/diff/60001/client/isolateserver.py...
client/isolateserver.py:8: __version__ = '0.4.8'
On 2016/06/15 18:24:09, M-A Ruel wrote:
> bump

bumped all versions

[mithro, 2016-06-16 11:23:02]

On 2016/06/15 20:58:15, nodir wrote:
> https://codereview.chromium.org/2069903003/diff/60001/client/isolateserver.py
> File client/isolateserver.py (right):
> 
>
https://codereview.chromium.org/2069903003/diff/60001/client/isolateserver.py...
> client/isolateserver.py:8: __version__ = '0.4.8'
> On 2016/06/15 18:24:09, M-A Ruel wrote:
> > bump
> 
> bumped all versions

I'm getting the following two directories in my git repo now;
# On branch master
# Your branch is ahead of 'origin/master' by 1 commit.
#   (use "git push" to publish your local commits)
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       client/cache/
#       client/cipd_cache/
#

Did you miss adding something to clean up something for add something to
.gitignore?

[M-A Ruel, 2016-06-16 14:07:57]

On 2016/06/16 11:23:02, mithro wrote:
> I'm getting the following two directories in my git repo now;
> # On branch master
> # Your branch is ahead of 'origin/master' by 1 commit.
> #   (use "git push" to publish your local commits)
> #
> # Untracked files:
> #   (use "git add <file>..." to include in what will be committed)
> #
> #       client/cache/
> #       client/cipd_cache/
> #
> 
> Did you miss adding something to clean up something for add something to
> .gitignore?

This is a bug in the unit test, this should be fixed. The cache/ leak was there
before this CL.

[nodir, 2016-06-16 22:33:56]

On 2016/06/16 14:07:57, M-A Ruel wrote:
> On 2016/06/16 11:23:02, mithro wrote:
> > I'm getting the following two directories in my git repo now;
> > # On branch master
> > # Your branch is ahead of 'origin/master' by 1 commit.
> > #   (use "git push" to publish your local commits)
> > #
> > # Untracked files:
> > #   (use "git add <file>..." to include in what will be committed)
> > #
> > #       client/cache/
> > #       client/cipd_cache/
> > #
> > 
> > Did you miss adding something to clean up something for add something to
> > .gitignore?
> 
> This is a bug in the unit test, this should be fixed. The cache/ leak was
there
> before this CL.

Done https://codereview.chromium.org/2078713002/