Project

General

Profile

Actions

Bug #10194

closed

[CWL] Avoid version skew between arvados-cwl-runner, arvados/jobs and cwl-runner

Added by Peter Amstutz over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
10/19/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

The arvados-cwl-runner client, the arvados/jobs Docker image and cwl-runner crunch script need to be the same or closely compatible versions or they will break. Currently this is handled as an ops issue, but has been a consistent source of problems. Investigate a version pinning scheme that propagates a specific code version across arvados-cwl-runner, arvados/jobs and cwl-runner.


Subtasks 2 (0 open2 closed)

Task #10334: Review 10194-jobs-image-versioningResolvedPeter Amstutz10/19/2016

Actions
Task #10294: Review 10194-cwl-version-skewResolvedLucas Di Pentima10/19/2016

Actions
Actions #1

Updated by Peter Amstutz over 6 years ago

  • Description updated (diff)
Actions #2

Updated by Peter Amstutz over 6 years ago

  • Status changed from New to In Progress
Actions #3

Updated by Peter Amstutz over 6 years ago

  • Target version set to 2016-10-26 sprint
Actions #4

Updated by Peter Amstutz over 6 years ago

  • Assigned To set to Peter Amstutz
Actions #5

Updated by Peter Amstutz over 6 years ago

Fixes in branch 10194-cwl-version-skew:

  • setup.py records git version to _version.py
  • --submit propagates git version as tag to arvados/jobs docker image (instead of using "latest")
  • tweak crunch v1 submit behavior: create job, then create pipeline instance containing that job, but mark pipeline instance as RunningOnServer instead of RunningOnClient so that a-r-p-i is responsible for marking the pipeline instance as completed.
  • add script to test that python-arvados-cwl-runner package installs successfully
  • packaging fixes as a result of fallout from adding test scripts
Actions #6

Updated by Lucas Di Pentima over 6 years ago

A few comments:

  • sdk/cwl/setup.py:
    • Line 20 & 23: Is the CWD guaranteed to be the same as the script’s dir at the moment of execution?
    • Line 25: I think it would be better to fail instead of ignore exceptions, we could be getting version skew again without noticing, caused by unforseen reasons. (If this is executed manually, disregard the comment as the warning message should be enough)
  • sdk/cwl/arvados_cwl/arvdocker.py:
    • Line 26: imageId var is not used anywhere, is the execution of cwltool.docker.get_image() needed?

Local sdk/cwl tests passed OK. So besides these coments, this LGTM.

Actions #8

Updated by Peter Amstutz over 6 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:670b4ba238d93910ff087794d359da0d5ac469fa.

Actions #9

Updated by Lucas Di Pentima over 6 years ago

(I was writing this review comment when the branch got merged, still leaving the comment just in case you find it useful)

On setup.py, if the idea is to check for subprocess.check_output() success, do you think moving the versionfile writing to an else clause would be clearer? Something like:

[...]
versionfile = os.path.join(SETUP_DIR, "arvados_cwl/_version.py")
try:
    gitinfo = subprocess.check_output(
        ['git', 'log', '--first-parent', '--max-count=1',
         '--format=format:%H', SETUP_DIR]).strip()
except Exception as e:
    # When installing from package, it won't be part of a git repository, and
    # check_output() will raise an exception.  But the package should include the
    # version file, so we can proceed.
    if not os.path.exists(versionfile):
        raise
else:
    with open(versionfile, "w") as f:
        f.write("__version__ = '%s'\n" % gitinfo)
[...]
Actions #10

Updated by Peter Amstutz over 6 years ago

Lucas Di Pentima wrote:

(I was writing this review comment when the branch got merged, still leaving the comment just in case you find it useful)

Whoops, sorry about that, I should have checked in with you. (I interpreted "LGTM with minor changes" as "ok to merge" but maybe we should clarify that at the next retrospective).

On setup.py, if the idea is to check for subprocess.check_output() success, do you think moving the versionfile writing to an else clause would be clearer? Something like:

I agree that would be a bit more elegant, maybe I'll sneak that into a future branch.

Actions #11

Updated by Peter Amstutz over 6 years ago

  • Status changed from Resolved to In Progress

Additional branch 10194-jobs-image-versioning which improves handling of package versions related to building the arvados/jobs Docker image, which is necessary for Docker image pinning to work as intended.

Actions #12

Updated by Ward Vandewege over 6 years ago

Peter Amstutz wrote:

Additional branch 10194-jobs-image-versioning which improves handling of package versions related to building the arvados/jobs Docker image, which is necessary for Docker image pinning to work as intended.

Reviewing 10194-jobs-image-versioning at 4264bbad60a87547cfca97e48848ff120e660372

build/run-build-docker-jobs-image.sh:

  • line 121: python_version_sdk prefix is being set incorrectly, it is 0.1 not 1.0, and the suffix is -2 not -3 (this really needs to be factored out into the library, but that's for later I think)
  • line 162: missing $ before 'gittag'
  • line 141: this code tests the exit value for docker tag, not docker build. It needs a patch along these lines:
--- a/build/run-build-docker-jobs-image.sh
+++ b/build/run-build-docker-jobs-image.sh
@@ -147,6 +145,23 @@ fi
 checkexit $ECODE "docker build" 
 title "docker build complete (`timer`)" 

+if [[ "$ECODE" != "0" ]]; then
+  exit_cleanly
+fi
+
+timer_reset
+
+docker tag arvados/jobs:$gittag arvados/jobs:latest
+
+ECODE=$?
+
+if [[ "$ECODE" != "0" ]]; then
+    EXITCODE=$(($EXITCODE + $ECODE))
+fi
+
+checkexit $ECODE "docker tag" 
+title "docker tag complete (`timer`)" 
+
 title "uploading images" 

 timer_reset

Otherwise, this looks good to me. Thanks!

Actions #13

Updated by Peter Amstutz over 6 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF