Arvados

func (cp *copier) copyFromCollection(dest string, coll *arvados.Collection, srcRelPath string) error {
    tmpfs, err := coll.FileSystem(cp.client, cp.keepClient)
    if err != nil {
        return err
    }
    snap, err := arvados.Snapshot(tmpfs, srcRelPath)
    if err != nil {
        return err
    }
    // Create ancestors of dest, if necessary.
    for i, c := range dest {
        if i > 0 && c == '/' {
            err = cp.staged.Mkdir(dest[:i], 0777)
            if err != nil && !os.IsExist(err) {
                return err
            }
        }
    }
    return arvados.Splice(cp.staged, dest, snap)
}

How efficient is it to create a new FileSystem object and then make a Snapshot and then do a Splice for every file when you are copying 1000s of files from the same collection?

How efficient is it to create a new FileSystem object and then make a Snapshot and then do a Splice for every file when you are copying 1000s of files from the same collection?

That copyFromCollection method runs once per collection mount point, not once per file. Creating a FileSystem and doing Snapshot+Splice with 1000s of files should not be a problem.

I don't have a good theory about the OOM situation yet. I suppose it's even possible the copying stage is causing arv-mount to do something inefficient...?

Meanwhile here's a fix for the "not getting any logs from the container itself because copying failed" issue:

22827-save-log-before-copy @ 48cc8d55837bf8c3bbe3edf4f6a3584b804ef088 -- developer-run-tests: #4775
retry remainder run-tests-remainder: #5231

• ✅ save log collection after container finishes, before copying outputs
• ✨ refactor "save log collection" code a little, hoping to make it a little easier to follow (saveLogCollection updates the container record and stashes PDH in a "current log PDH" field, instead of returning a collection record and leaving it up to the caller to update the container record)

Tom Clegg wrote in #note-11:

How efficient is it to create a new FileSystem object and then make a Snapshot and then do a Splice for every file when you are copying 1000s of files from the same collection?

That copyFromCollection method runs once per collection mount point, not once per file. Creating a FileSystem and doing Snapshot+Splice with 1000s of files should not be a problem.

I don't have a good theory about the OOM situation yet. I suppose it's even possible the copying stage is causing arv-mount to do something inefficient...?

I still owe you a copy of the container record that was actually causing the problem, that may shed some light on the situation.

Meanwhile here's a fix for the "not getting any logs from the container itself because copying failed" issue:

22827-save-log-before-copy @ 48cc8d55837bf8c3bbe3edf4f6a3584b804ef088 -- developer-run-tests: #4775
retry remainder run-tests-remainder: #5231

• ✅ save log collection after container finishes, before copying outputs
• ✨ refactor "save log collection" code a little, hoping to make it a little easier to follow (saveLogCollection updates the container record and stashes PDH in a "current log PDH" field, instead of returning a collection record and leaving it up to the caller to update the container record)

This LGTM.

It occurred to me that something that has come up a few times is that container.json in the log collection is only written once before the container executes, so if anything useful was reported in runtime_status it isn't available. It would be nice to stick another call to LogContainerRecord() in there before committing the log collection.

Tom Clegg wrote in #note-14:

This is on hold until we see an example.

Here are the logs from the offending run:

https://workbench.jutro.arvadosapi.com/collections/jutro-4zz18-dtz86ioahajk75l

I was able to get logs from this run because I was watching it closely (and actually logged into the compute node) and cancelled it before it went fully unresponsive, so it was able to commit logs.

The symptom in the field was using up 100% of RAM and going completely into a unresponsive state and eventually just getting shut down for lack of responsiveness, and not having any logs to see what went wrong.

I haven't yet confirmed this with a test case, but I suspect this is caused by a memory leak in Snapshot/Splice. In Splice(), the "fs" fields of dirnodes and filenodes are updated to point to the new filesystem, but the "kc" field of storedSegments still points to the original source filesystem. Copier creates a new source filesystem for each of 3000 mount points (even if it's the same PDH mounted 3000 times, as it is here) and the lingering "kc" pointer prevents any of the 3000 source filesystems from being garbage-collected. The manifest itself is 356K. If the filesystem representation is 4x that, we would see about 3.2 GB memory usage.

1. When traversing mounts, cache the filesystem representation of each collection, not just the manifest text (this would solve the memory issue and be faster in the current case, but in other cases it will use more memory than the current approach)
2. When traversing mounts, retain the filesystem representation of the most recent source collection, so we can skip re-generating it when the next one happens to be identical (this would be faster in all cases, and would solve the memory issue in the current case)
3. Fix the lingering "kc" pointers (this would solve the memory issue in the general case, but wouldn't be faster)

I think we should do options 2 and 3.

Tom Clegg wrote in #note-17:

I haven't yet confirmed this with a test case, but I suspect this is caused by a memory leak in Snapshot/Splice. In Splice(), the "fs" fields of dirnodes and filenodes are updated to point to the new filesystem, but the "kc" field of storedSegments still points to the original source filesystem. Copier creates a new source filesystem for each of 3000 mount points (even if it's the same PDH mounted 3000 times, as it is here) and the lingering "kc" pointer prevents any of the 3000 source filesystems from being garbage-collected. The manifest itself is 356K. If the filesystem representation is 4x that, we would see about 3.2 GB memory usage.

Ways to fix:

1. When traversing mounts, cache the filesystem representation of each collection, not just the manifest text (this would solve the memory issue and be faster in the current case, but in other cases it will use more memory than the current approach)
2. When traversing mounts, retain the filesystem representation of the most recent source collection, so we can skip re-generating it when the next one happens to be identical (this would be faster in all cases, and would solve the memory issue in the current case)
3. Fix the lingering "kc" pointers (this would solve the memory issue in the general case, but wouldn't be faster)

I think we should do options 2 and 3.

This sounds like a good plan to me.

22827-many-mounts-inside-output-dir @ fd026d718bc67e1d9c8320fc646b9109da357370 -- developer-run-tests: #4804

• All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
  • ✅ Fix memory leak
  • ✅ Don't discard+recompute collection filesystem when consecutive mounts copy from the same collection
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • n/a
• Code is tested and passing, both automated and manual, what manual testing was done is described.
  • ✅ Test case checks memory before/after copying files from 100 mounts of the same 100-file collection; usage down from 254 MB to 5 MB
  • ✅ Manually checked (by editing test case params) 100 mounts of two alternating 100-file collection; ditto
  • ✅ Manually checked (by editing test case params) 500 mounts of the same 500-file collection; usage down from 1500 MB to 15 MB
• New or changed UX/UX and has gotten feedback from stakeholders.
  • n/a
• Documentation has been updated.
  • n/a
• Behaves appropriately at the intended scale (describe intended scale).
  • ✅ Much improved -- memory usage is now dominated by the output collection manifest, not the inputs
• Considered backwards and forwards compatibility issues between client and server.
  • ✅
• Follows our coding standards and GUI style guidelines.
  • ✅

So what I see from the code, the main fixes are:

• correctly updating the backlinks from segments to the filesystem object in Splice() and Snapshot()
• the simplest possible optimization of not reloading the collection when the previously loaded collection was the same one

LGTM