LinkCreatesNamedFile test failure

[jwendell]

This test is consistently failing in my environment:

envoy/test/extensions/common/async_files/async_file_handle_thread_pool_test.cc

Line 140 in 35bf11a

TEST_F(AsyncFileHandleTest, LinkCreatesNamedFile) {

Here's my attempt to explain why:

The function manager_->createAnonymousFile() relies on the underlying filesystem supporting the O_TMPFILE flag:

envoy/source/extensions/common/async_files/async_file_manager_thread_pool.cc

Line 135 in 35bf11a

open_result = posix().open(path_.c_str(), O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);

The fallback code (a few lines below) creates a file and deletes it, without closing it, returning its file descriptor. I'm reaching this fallback code, because my filesystem doesn't support the O_TMPFILE flag.

The problem happens when a hard link is created later with linkat():

envoy/source/extensions/common/async_files/async_file_context_thread_pool.cc

Line 49 in 35bf11a

auto result = posix().linkat(fileDescriptor(), procfile.c_str(), AT_FDCWD, filename_.c_str(),

It fails with ENOENT, as stated in the manual page:

ENOENT An attempt was made to link to a /proc/self/fd/NN file
              corresponding to a file that has been deleted.

As seen here: https://man7.org/linux/man-pages/man2/link.2.html

cc @ravenblackx

[ravenblackx]

What sort of environment is it that exhibits this failure? (Thinking maybe an NTFS filesystem on Linux?)

I would suggest this could be fixed by having the fallback behavior create a named temporary file (unfortunately), keeping track of that file in the AsyncFileContext object, moving it in lieu of linkat or deleting it during close. It should maybe be a distinct AsyncFileContext (though perhaps a subclass of AsyncFileContextThreadPool) since the more common case works without that information. That variant could also potentially make this able to function in Windows.

(Unfortunate because it could potentially make for inconsistent behavior with any code that looks at directory contents, and it will leave a mess behind if a crash occurs while the file exists, like the regular variant does not.)

[jwendell]

The env is a docker container running in a k8s cluster probably running in an old kernel.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[ravenblackx]

I have a plan to address this, involving migrating AsyncFileManager to using Envoy::Filesystem::File, after making File have an implementation for temp files and all the other required operations.
I'd assign this issue to me but I don't think I have permission to do that.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[ravenblackx]

Still working on it, bot!

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[ravenblackx]

Still in my queue, not stale!

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.