Arvados

AFAICT the current documented+enforced restriction ("must not be writable") comes from #9397 and its sole purpose was to get the then-required feature out faster by deferring the "copy" implementation.

Mount points underneath output_path which have "writable":true will be copied into the target output_path and may be updated by the container

This sounds like a sensible implementation.

Just a suggestion, we might want to rephrase that a bit for the docs so it describes the user-visible behavior rather than the implementation. The meaning of writable=true ("can modify files, but modifications are not saved") is already mentioned elsewhere but might bear repeating. I wonder if it would be better to mention the consequences of the "copied into the target" part rather than just stating how it's implemented -- e.g., if your output path is in a tmpfs, you need to make sure it's big enough to accommodate copies of the inner writable mounts.

Tom Clegg wrote:

Just a suggestion, we might want to rephrase that a bit for the docs so it describes the user-visible behavior rather than the implementation. The meaning of writable=true ("can modify files, but modifications are not saved") is already mentioned elsewhere but might bear repeating. I wonder if it would be better to mention the consequences of the "copied into the target" part rather than just stating how it's implemented -- e.g., if your output path is in a tmpfs, you need to make sure it's big enough to accommodate copies of the inner writable mounts.

Updated text:

"Mount points underneath output_path which have "writable":true are copied into output_path during container initialization and may be updated, renamed, or deleted by the running container. The original collection is not modified. On container completion, files remaining in the output are saved to the output collection. The mount at output_path must be big enough to accommodate copies of the inner writable mounts."

I've rebased on latest and am going to re-run tests, assuming they pass, any other comments prior to merge?

12764-writable-file @ 3e51fd869e600fd3d60e892346896fd9ce3d8c9c

I'm not keen on this pattern

err = something()
if err == nil {
    // happy path
    if foo {
        err = somethingelse()
    } else {
        err = somethingdifferent()
    }
}
if err != nil {
    return err
}

Probably better to stick with the Go style "don't pyramid / handle errors first", even if that means using the same error fmt string twice.

Might help debugging if the error message mentions which mount was being processed. I guess that would involve adding "bind" to the copyFile struct?

The error handling in the WalkFunc seems a bit suspect. Is it intentional that it silently ignores non-nil incoming walkerr, and non-regular (pipe/socket/device/symlink) files? I would think all of those should be fatal errors, since they are unexpected/"impossible".

Tom Clegg wrote:

Might help debugging if the error message mentions which mount was being processed. I guess that would involve adding "bind" to the copyFile struct?

The error handling in the WalkFunc seems a bit suspect. Is it intentional that it silently ignores non-nil incoming walkerr, and non-regular (pipe/socket/device/symlink) files? I would think all of those should be fatal errors, since they are unexpected/"impossible".

Cleaned up error handling a bit:

12764-writable-file @ c8891a5eeec540404c7aff02a200fd95f6dbe64b

LGTM

Tests at https://ci.curoverse.com/job/developer-run-tests/560/

... looks like there's a couple things left to fix.

reviewing 12764-secure-tmp-parent @ 2eb85f70168d331e49e9d38bfbd2292d586dfcc1

Looks like runner.ParentTemp makes all the runner.CleanupTempDir stuff superfluous, so we should remove it.

apart from that, LGTM

I wonder if it would be worthwhile to call the tempdir "zzzzz-dz642-asdfasdfasdfasd-123456" instead of "crunch-run123456"?