Arvados

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?

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

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?

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

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.

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)

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.

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