Feature #8015

[Crunch2] Keep-backed mount points: tmp/output dir, read-only collection, read-only by-id dir

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

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Crunch
Target version:
Start date:
02/16/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5
Release:
Release relationship:
Auto

Description

Implement mount points in crunch-run as described here: https://dev.arvados.org/projects/arvados/wiki/Containers_API#Mount-types

Scope

Support the following mount types:
  • kind=collection, writable=false, uuid=X
  • kind=collection, writable=false, portable_data_hash=X
  • kind=collection, writable=true, no uuid or portable_data_hash
  • kind=tmp, device_type={disk or network} (for now this can be a plain docker data volume, although at some point we might need something like a loopback device so we can reserve/limit capacity)

Note the "kind=collection, writable=true, and uuid-or-portable_data_hash provided" feature is not included here because it requires additional support from arv-mount that isn't implemented yet.

Approach

  1. Before starting the docker container, build an arv-mount command line and run arv-mount in a new temporary directory
  2. Add host bind mounts to the container, mapping the arv-mount directories to the desired targets inside the container.
  3. Run the docker container, wait for it to finish
  4. If the container used one of its mount points as an output directory, record its output (i.e., get the manifest_text from the JSON object in the magic .arvados#collection file, create a new collection, and record the resulting PDH)
  5. Run "fusermount -u" to unmount the arv-mount directory
  6. Delete temporary directories

Subtasks

Task #8356: Review 8015-crunch2-mountResolvedRadhika Chippada

Task #8362: Add testsResolvedPeter Amstutz

Task #8361: Mount point supportResolvedPeter Amstutz

Associated revisions

Revision 0f8dc3d8
Added by Peter Amstutz over 5 years ago

Merge branch '8015-crunch2-mount' closes #8015

Revision be191fe4 (diff)
Added by Peter Amstutz over 5 years ago

Fix crunch-run tests to pass reliably. refs #8015

History

#1 Updated by Peter Amstutz over 5 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg over 5 years ago

  • Description updated (diff)

#3 Updated by Tom Clegg over 5 years ago

  • Subject changed from [Crunch2] Set up mount points to [Crunch2] Keep-backed mount points: tmp/output dir, read-only collection, read-only by-id dir

#4 Updated by Tom Clegg over 5 years ago

  • Description updated (diff)
  • Category set to Crunch

#5 Updated by Brett Smith over 5 years ago

  • Story points set to 2.0

#6 Updated by Tom Clegg over 5 years ago

  • Target version set to 2016-02-17 Sprint

#7 Updated by Peter Amstutz over 5 years ago

  • Assigned To set to Peter Amstutz

#8 Updated by Peter Amstutz over 5 years ago

Note that kind=collection, writable=false, uuid=X will make the job impure, because the collection could change. I thought we were trying to avoid that sort of thing?

#9 Updated by Brett Smith over 5 years ago

  • Target version deleted (2016-02-17 Sprint)
  • Assigned To deleted (Peter Amstutz)

#10 Updated by Peter Amstutz over 5 years ago

This is ready for review on @ 8015-crunch2-mount

#11 Updated by Brett Smith over 5 years ago

  • Status changed from New to In Progress
  • Assigned To set to Peter Amstutz
  • Target version set to 2016-02-17 Sprint

#12 Updated by Tom Clegg over 5 years ago

Peter Amstutz wrote:

Note that kind=collection, writable=false, uuid=X will make the job impure, because the collection could change. I thought we were trying to avoid that sort of thing?

We want to
  • permit impure containers
  • make it harder to accidentally and unknowingly run impure containers

Use of this kind of mount is an example of how clients like Workbench (and job re-use features) can detect/predict/guess that a container is impure. But I don't think crunch-run needs to care one way or the other...

#13 Updated by Brett Smith over 5 years ago

  • Target version changed from 2016-02-17 Sprint to 2016-03-02 sprint
  • Story points changed from 2.0 to 0.5

#14 Updated by Peter Amstutz over 5 years ago

A couple notes:

This branch ended up bringing #8020 (handle outputs) along for the ride, so some of the bulk is from adding the ability to upload results (in addition to using writable keep). The uploading code was adapted from the existing crunchrunner code so it's not completely new.

I after the actual arv-mount handling, most of the rest of the changes are from general improvements/refactoring around logging. I went to the Docker API and figured how to correctly handle stdout/stderr multiplexed on the single stream.

#15 Updated by Radhika Chippada over 5 years ago

crunchrun.go

  • It does not appear that SetupSignals would return any non-nil error?
  • Should runner.CrunchLog.Printf be using closeerr instead of readerr in the following? (one more like this in the next if statement as well)
    if closeerr != nil {
    runner.CrunchLog.Printf("While closing stdout logs: %v", readerr)
    }
  • AttachStreams comment says “AttachLogs”
  • Line 644 (CrunchLog.Close()): Would this result in writing the log (with any errors in steps 8 and 9) to keep again ? Would this result in changes to collection such as pdh?
  • Line 650: Should this be “err = waiterr” instead of runerr?

upload.go

  • func ReadFrom reads from reader and writes to keep. Should it be better named to reflect this? ReadAndWrite or something like that
  • The comment “CollectionWriter makes implements creating new Keep collections” seems like too many verbs makes and implements
  • 6 tests are failing (it could be my env; if they are passing for you, you can ignore this comment)

#16 Updated by Peter Amstutz over 5 years ago

Radhika Chippada wrote:

crunchrun.go

  • It does not appear that SetupSignals would return any non-nil error?

Fixed to not return anything now (because signal.Notify() doesn't return an error either).

  • Should runner.CrunchLog.Printf be using closeerr instead of readerr in the following? (one more like this in the next if statement as well)
    if closeerr != nil {
    runner.CrunchLog.Printf("While closing stdout logs: %v", readerr)
    }

Fixed.

  • AttachStreams comment says “AttachLogs”

Fixed. Also fixed error handling around that code path.

  • Line 644 (CrunchLog.Close()): Would this result in writing the log (with any errors in steps 8 and 9) to keep again ? Would this result in changes to collection such as pdh?

The code is correct, after committing to keep in CommitLogs() it creates a new logger (so that any late shutdown errors can be logged to the API server) but without a CollectionFileWriter attached.

  • Line 650: Should this be “err = waiterr” instead of runerr?

Fixed.

upload.go

  • func ReadFrom reads from reader and writes to keep. Should it be better named to reflect this? ReadAndWrite or something like that

It's named that way to fulfill the "ReaderFrom" interface (https://golang.org/pkg/io/)

  • The comment “CollectionWriter makes implements creating new Keep collections” seems like too many verbs makes and implements

Fixed.

  • 6 tests are failing (it could be my env; if they are passing for you, you can ignore this comment)

With arvbox restart test --only services/crunch-run I get 1 error. Looking into it.

#17 Updated by Radhika Chippada over 5 years ago

I am still not using arvbox. All tests passed for me. LGTM

#18 Updated by Peter Amstutz over 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 67 to 100

Applied in changeset arvados|commit:0f8dc3d824f03b82b8db9f61bbcc0592b62b998f.

Also available in: Atom PDF