Arvados

The failed process used a single S3 URL to an entire bucket as the input. On reviewing the source I don't believe this is correct use of the workflow. A run that used two S3 file URLs as input succeeded (see process).

So, the issue might be triggered by an empty output directory, possibly Keep-backed? I still think we shouldn't crash in this case but it's a clue.

Brett Smith wrote in #note-5:

So, the issue might be triggered by an empty output directory, possibly Keep-backed?

Perhaps even more specifically: the output directory only contains other mounts. (In the case of the S3 downloader, that's download.sh and .aws/config from an intermediate collection.)

ee21f342b1ab0152fbe5030dc39ed1fc5cc2ec1b adds a test to reproduce the problem. TestOutputCollectionWithOnlySubmounts panics with the exact same stack trace as seen in the logs.

It does not crash if .arvados#collection has an empty manifest_text. So that's really key to the bug. It's less clear what a proper fix would be.

It's possible there are multiple bugs here.

Here's a run that crashed. This is the run that produced the logs above. It was launched through Workbench's "Run a workflow" interface.

Here's a run that succeeded. It was launched by me running arvados-cwl-runner in my development environment. It doesn't try to copy the file mounted at the top level of the output collection:

2024-10-21T18:40:28.065539182Z copying "*" 
2024-10-21T18:40:28.071408808Z copying ".aws/config" from 88d703c507f2a67d364f465541bc1904+112/.aws/config
2024-10-21T18:40:28.104926517Z not copying "download.sh" because contents cannot match output globs

And sure enough, the successful run has an output_glob that corresponds to the provided S3 URLs:

"output_glob": [
  "aws-s3-bulk-download.cwl",
  "aws-s3-bulk-download.cwl/**",
  "aws-s3-download.cwl",
  "aws-s3-download.cwl/**",
  "cwl.output.json" 
]

Whereas the failed run has an empty output_glob.

Need to investigate why that discrepancy exists. It may be that tordo runs a-c-r inside a Docker container with an older version that doesn't generate output_glob.

We may still want to fix the crash in crunch-run anyway, since it seems like the user could always write an output_glob that would include a mounted file in this same way.

Brett Smith wrote in #note-9:

Need to investigate why that discrepancy exists. It may be that tordo runs a-c-r inside a Docker container with an older version that doesn't generate output_glob.

That's not it, from the failed run logs:

2024-10-21T14:48:57.432384841Z INFO /usr/lib/python3-arvados-cwl-runner/bin/arvados-cwl-runner 2.8.0.dev20241008154607, arvados-python-client 2.8.0.dev20241008144825, cwltool 3.1.20240508115724

Whereas the failed run has an empty output_glob.

It's probably actually the difference in inputs. Since the failed run has a single S3 URL that tries to download an entire bucket, the output_glob reduces to the empty set.

Branch for discussion of requirements and impact check, it may be none of this gets merged as written: 22226-empty-output-panic @ 5bacea932060f175fd13a24ee909b88844dd3f17 - developer-run-tests: #4520 (I assume the Workbench tests failure is not related but maybe I should dig in)

Tom Clegg wrote in #note-13:

22226-empty-output-panic @ 7c16d2d0e821d73f791b247499cd79923aaab4bd -- developer-run-tests: #4522

Keeps the test case added by Brett above, and makes it pass by fixing a "stale parent pointer" bug in the Splice implementation. Details in commit message.

Question about keeping the test case: at standup we talked about how the mounted file should be copied to the output collection. Does that mean the asserts on the copier should be updated to check that that's happening? Or am I misunderstanding the mechanism here?

If I cleaned up the test to use fixtures from arvadostest, would you take that change?

I realize nobody else will ever care about this but I'd appreciate if you were listed as the author of the most recent commit instead of me, since you wrote literally the entire thing.

Thanks.

Brett Smith wrote in #note-15:

Question about keeping the test case: at standup we talked about how the mounted file should be copied to the output collection. Does that mean the asserts on the copier should be updated to check that that's happening? Or am I misunderstanding the mechanism here?

Yes, I've updated the test case to confirm the mounted file gets included in the output.

If I cleaned up the test to use fixtures from arvadostest, would you take that change?

I've updated it to use arvadostest.FooCollectionPDH.

I realize nobody else will ever care about this but I'd appreciate if you were listed as the author of the most recent commit instead of me, since you wrote literally the entire thing.

Oops, I squashed commits and forgot that it would keep the author from the first commit. Fixed.

22226-empty-output-panic @ 3ada7fc03565cffdfe660ad553e3aaca2b02d504 -- developer-run-tests: #4525

Tom Clegg wrote in #note-16:

22226-empty-output-panic @ 3ada7fc03565cffdfe660ad553e3aaca2b02d504 -- developer-run-tests: #4525

So, I believe the test as written tests the thing it's intending to test. The comments help a lot with that, so thank you for those. However, I think it's also a little confusing to follow, because it sets up a situation that can't happen in reality by writing an output file that then gets mounted over. Real compute jobs don't get a chance to execute any code in between all the different mount setups.

The only reason I wrote the foo file initially was to make the environment as close as possible to the "real world" conditions triggering the bug. If I'm following right, with your new asserts, we don't need to write it at all: we can just test that the existence of the mount record means foo gets staged.

If I have all that right, I'd suggest deleting all the foo-writing code as a readability improvement. I think it's easier to follow "foo gets mounted and then gets staged" as opposed to playing the shell game of figuring out which foo is the real foo.

Either way, assuming tests pass this is good to merge. Thanks.