TestFdLeaksSystemd failure with criu-dev

[kolyshkin]

TestFdLeaksSystemd starts failing frequently when running with criu-dev.

=== RUN   TestFdLeaksSystemd
    exec_test.go:1750: extra fd 9 -> /12224/task/13831/fd
    exec_test.go:1753: found 1 extra fds after container.Run
--- FAIL: TestFdLeaksSystemd (0.10s)

[kolyshkin]

Hmm, is this a test issue and it should use runtime.LockOSThread, or something like that?

Or is there a code path which caches /proc/<pid>/task/<tid>/fd for reuse (and for some reason it's not open after the first container run)?

Or should we exclude fdDir.Fd() from the fds slice in fdList?

Or is this a genuine failure (somewhere we forget to call Close and the fd is only closed when Go GC calls Release on the underlying object? I ran TestFdLeaksSystemd with GOGC=off (which should disable GC entirely) and did not see any "extra" fds.

I also see that commit 8e8b136 changed the test logic a bit -- previously we only checked that we number of opened fds is the same (if len(fds1) == len(fds0) { return } , now it checks if numeric fds itself are the same (if reflect.DeepEqual(fds0, fds1) { return }).

Or, maybe we need to remove this test entirely as it does not catch any real issues and is prone to flakes.

Alas I have more questions than answers. @cyphar can you take a look?

[kolyshkin]

Hmm, I also thought that runContainerOK may not be entirely synchronous and so we are reading /proc/self/fd at different stages. But it is synchronous as it calls wait on the process being executed. I don't know how can we get different results (other than Go playing with threads, that is).

[cyphar]

I saw this as well, and my first impression was that this was likely caused by the change to the test code in ff6fe13 ("utils: use safe procfs for /proc/self/fd loop code") -- we are now opening a file descriptor during the logic to get a list of file descriptors. If the file descriptor happens to be allocated to a different number, you'll get an error.

I was going to change fdList to filter out the file descriptor but I think it's probably better to just revert the change? (Then again, readdir requires a file descriptor so I suspect this doesn't make a difference and the filtering approach is probably better.)