Add async_files library to support filesystem buffering and filesystem caching by ravenblackx · Pull Request #20332 · envoyproxy/envoy

ravenblackx · 2022-03-14T16:08:05Z

Signed-off-by: Raven Black ravenblack@dropbox.com

Commit Message: Add async_files library to support filesystem buffering and filesystem caching.
Additional Description: This is a support library for subsequent PRs for an http filter contrib to buffer to local filesystem (issue #19026) and a cache implementation contrib that caches to local filesystem. Being sent for review separately because each part is already large for review.
Risk Level: None on its own, because it's not used yet.
Testing: I believe there's good unit test coverage; relying on the PR CI to tell me how good, as I wasn't able to extract any information via do_ci.sh bazel.coverage and this documentation suggests using the PR.
Docs Changes: There's a readme for the library, no changes for external docs.
Release Notes: No changes to existing behavior.
Platform Specific Features: Current implementation may be Linux-based only; not sure if other filesystems support unnamed files. Shouldn't need guards since the library is going to be used for extensions only - can simply not load the extension where it's unsupported.

ravenblackx · 2022-03-14T19:33:41Z

Please don't review yet, still trying to get a good coverage report out of it. I'm going to be amend-pushing which would make a pre-emptive review frustrating.

mattklein123 · 2022-03-14T19:36:19Z

OK just ping me when ready, thanks.

/wait

ravenblackx · 2022-03-15T20:31:43Z

Turns out CI isn't useful for getting a usable coverage report for a thing in contrib, but I was able to get one by doing:

ENVOY_DOCKER_BUILD_DIR=/home/ravenblack/envoy_artefacts ./ci/run_envoy_docker.sh 'BAZEL_EXTRA_TEST_OPTIONS="--test_env=ENVOY_IP_TEST_VERSIONS=v4only" ./ci/do_ci.sh bazel.coverage //contrib/common/async_files/...'

genhtml $(find /home/ravenblack/envoy_artefacts/ -name "coverage.dat") --output-directory ~/coverage

(Briefly documented here in case it shows up in search results for how to get a useful coverage report, or so I can refer back to it myself later!)

Which turned up 93% coverage, and the only uncovered lines are ones that I don't think it's possible to provoke without running in different environments (the path taken when the filesystem doesn't support O_TMPFILE, and the paths taken when pread and pwrite somehow do only a partial read/write). So I think this is about as good as it's going to get.

mattklein123 · 2022-03-15T20:46:08Z

Turns out CI isn't useful for getting a usable coverage report for a thing in contrib

I think this should go into core as WiP xDS APIs and extensions. These are features that I think the project should widely support and I'm happy to sponsor and help with reviews. Can you move the code over and then I can review in the new location? Thank you.

repokitteh-read-only · 2022-03-16T14:57:56Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20332 was synchronize by ravenblackx.

see: more, trace.

ravenblackx · 2022-03-16T18:14:30Z

Sorry for the accidental CCs, some unnecessary files got modified by check_format.py and slipped into the commit without me noticing, one of the 5+ times it happened. (Partial fix for the cause of that in #20372)

…m caching Signed-off-by: Raven Black <ravenblack@dropbox.com>

mattklein123 · 2022-03-17T16:18:31Z

High level question before getting going with review. Given that we are about to introduce support for io_uring in #19339, should we actually implement this using io_uring and avoid the thread pool? Granted, that will not work on OSX or Windows, though right now this code doesn't work on Windows anyway. Thoughts? cc @rojkov

ravenblackx · 2022-03-17T16:56:03Z

/retest

Per discussion in slack, it's designed to be compile-time-switchable (maybe even run-time-switchable) to other implementations later - the API should be compatible without modification for different implementations. I went with the "lowest common denominator" implementation first, in part because at least some of our deployment environments don't support io_uring yet.

Though it is a little weird that this and the io_uring wrapper are essentially aiming to be the same abstraction (an asynchronous file operations wrapper that's implementation-agnostic), just with different initial implementations. Perhaps ideally they should merge into one later, but I'd rather have something that works and then later migrate to something unified, than try to migrate while the implementation is still a bit of a moving target. I think this might be a higher-level abstraction than the io_uring wrapper, in which case it might end up making sense to have both, and the io_uring one just be a dependency of one of the optional implementations of this one.

repokitteh-read-only · 2022-03-17T16:56:06Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20332 (comment) was created by @ravenblackx.

see: more, trace.

repokitteh-read-only · 2022-04-13T16:09:40Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #20332 was synchronize by ravenblackx.

see: more, trace.

…_files Signed-off-by: Raven Black <ravenblack@dropbox.com>

mattklein123

Sorry for the runaround, let's get the build stuff sorted and then we can finish this out.

/wait

mattklein123 · 2022-04-14T15:35:23Z

api/envoy/extensions/common/async_files/v3/async_file_manager.proto

+
+// [#protodoc-title: AsyncFileManager configuration]
+
+// [#extension: envoy.common.support.async_files]


Sorry for the runaround, but I think this is where everything went sideways for you. Essentially something in the docs is requiring that this have an extension category. The thing is, there shouldn't be any category for this message. This is just a helper message that will get included in other real config (filters, etc.). So I don't think there should be any extension here or category as it's not a real thing. Can you remove this tag and the one below? What error do you get if you do so? If something in the docs is causing you issues, I would just mark this [not-implemented-hide] (or whatever the tag is) until something actually includes this message that we need documented.

Done (and yes, once it's de-extensioned the whole cascade of requirements goes away).

mattklein123 · 2022-04-14T15:35:58Z

bazel/repositories.bzl

 PPC_SKIP_TARGETS = ["envoy.filters.http.lua"]

 WINDOWS_SKIP_TARGETS = [
+    "envoy.common.support.async_files",


Per above we shouldn't need this. What you have now is just helper libraries, not a real extension.

mattklein123 · 2022-04-14T15:36:39Z

source/extensions/common/async_files/BUILD

@@ -0,0 +1,84 @@
+load(
+    "//bazel:envoy_build_system.bzl",
+    "envoy_cc_extension",


This is probably where things went wrong. Just use envoy_cc_library, the actual extension will be the thing that actually ends up including this library. Bazel will take care of the rest.

Ah, my fault, I was momentarily thinking of extensions like shared objects / dll style plugins, where if you have a library that two extensions are going to want to share then you need that library to also be a plugin, but yeah, of course envoy is not doing it like that.

mattklein123 · 2022-04-14T15:37:16Z

tools/extensions/extensions_check.py

    "envoy.rbac.matchers", "envoy.access_loggers.extension_filters", "envoy.http.stateful_session",
    "envoy.matching.http.input", "envoy.matching.network.input",
-    "envoy.matching.network.custom_matchers")
+    "envoy.matching.network.custom_matchers", "envoy.common.support")


remove per other comments

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx · 2022-04-14T21:54:31Z

/retest

repokitteh-read-only · 2022-04-14T21:54:35Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20332 (comment) was created by @ravenblackx.

see: more, trace.

mattklein123

Thanks LGTM to ship and iterate. Cool stuff. Just a few small random comments. Thank you!

/wait

mattklein123 · 2022-04-18T15:35:27Z

source/extensions/extensions_build_config.bzl

    # Access loggers
    #
-
-    "envoy.access_loggers.file":                        "//source/extensions/access_loggers/file:config",


nit: revert changes to this file. I don't think they are needed anymore.

Done. Autoformat! [shakefist]

mattklein123 · 2022-04-18T15:39:24Z

source/extensions/common/async_files/async_file_manager_thread_pool.cc

+}
+
+std::function<void()> AsyncFileManagerThreadPool::enqueue(std::shared_ptr<AsyncFileAction> action) {
+  auto cancelFunc = [action]() { action->cancel(); };


nit: cancel_func, same elsewhere if I missed it (snake case for local vars)

Done. Linter! [shakefist]

mattklein123 · 2022-04-18T15:40:31Z

source/extensions/common/async_files/async_file_manager_thread_pool.cc

+std::function<void()> AsyncFileManagerThreadPool::enqueue(std::shared_ptr<AsyncFileAction> action) {
+  auto cancelFunc = [action]() { action->cancel(); };
+  absl::MutexLock lock(&queue_mutex_);
+  if (ThreadIsWorker) {


Can you add a comment here? This is an optimization if this is being called from within a callback running on a worker thread? Technically couldn't you avoid grabbing the lock in this case and just acquire the lock below?

Good catch on the lock saving. It's not just an optimization, it's an intentional behavior difference, the new action doesn't yield the thread, essentially skipping to the head of the queue - added a comment to that effect.

mattklein123 · 2022-04-18T15:42:20Z

source/extensions/common/async_files/async_file_manager_thread_pool.cc

+    case AsyncFileManager::Mode::WriteOnly:
+      return O_WRONLY;
+    case AsyncFileManager::Mode::ReadWrite:
+    default:


remove default so we have a compiler error if something else is added?

mattklein123 · 2022-04-18T15:45:29Z

source/extensions/common/async_files/async_file_manager_factory.cc

+    default:
+      throw EnvoyException("unrecognized AsyncFileManagerConfig::ManagerType");


This can't happen due to the PGV validation. I'm not sure what we do here now since we removed the GCOV NOT_REACHED macros but I would look at other similar cases and see what Alyssa changed them to.

Looks like throwing an exception is the new way (in at least the first two examples I found, e.g. this one.)

Added a comment about it being expected to be unreachable.

mattklein123 · 2022-04-18T15:46:26Z

source/extensions/common/async_files/async_file_manager_factory.cc

+  absl::flat_hash_map<std::string, std::shared_ptr<AsyncFileManager>>
+      managers_ ABSL_GUARDED_BY(mu_);
+  absl::flat_hash_map<std::string,
+                      envoy::extensions::common::async_files::v3::AsyncFileManagerConfig>
+      configs_ ABSL_GUARDED_BY(mu_);


nit: just have a single map with a struct containing both the manager and config?

That was very lazy of me, sorry. Done.

…and explain, snake locals Signed-off-by: Raven Black <ravenblack@dropbox.com>

mattklein123

Thanks!

Signed-off-by: Raven Black <ravenblack@dropbox.com>

…m caching (envoyproxy#20332) Signed-off-by: Raven Black <ravenblack@dropbox.com>

mattklein123 self-assigned this Mar 14, 2022

repokitteh-read-only bot added the waiting label Mar 14, 2022

ravenblackx force-pushed the async_files branch from bd3360c to 7f74f24 Compare March 14, 2022 20:07

repokitteh-read-only bot removed the waiting label Mar 14, 2022

ravenblackx force-pushed the async_files branch 2 times, most recently from 0753c8f to 139c9e7 Compare March 15, 2022 14:14

mattklein123 added the waiting:any label Mar 15, 2022

repokitteh-read-only bot removed the waiting:any label Mar 15, 2022

mattklein123 added the waiting label Mar 15, 2022

ravenblackx force-pushed the async_files branch from 139c9e7 to 4cac752 Compare March 16, 2022 14:45

repokitteh-read-only bot added v2-freeze api and removed waiting labels Mar 16, 2022

ravenblackx force-pushed the async_files branch 3 times, most recently from a6027fd to 5a76b3e Compare March 16, 2022 17:45

ravenblackx force-pushed the async_files branch from 5a76b3e to fa15080 Compare March 16, 2022 20:56

Add async_files library to support filesystem buffering and filesyste…

b021036

…m caching Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx force-pushed the async_files branch from fa15080 to b021036 Compare March 16, 2022 21:57

mattklein123 removed api v2-freeze labels Mar 16, 2022

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 13, 2022

Merge branch 'main' of https://github.com/envoyproxy/envoy into async…

700ebe9

…_files Signed-off-by: Raven Black <ravenblack@dropbox.com>

mattklein123 requested changes Apr 14, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 14, 2022

De-extension the async_files library

2fe97f3

Signed-off-by: Raven Black <ravenblack@dropbox.com>

repokitteh-read-only bot removed the waiting label Apr 14, 2022

mattklein123 requested changes Apr 18, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 18, 2022

Undo bzl autoformat, flatten list, make switches stricter, lock less …

d09c87f

…and explain, snake locals Signed-off-by: Raven Black <ravenblack@dropbox.com>

repokitteh-read-only bot removed the waiting label Apr 18, 2022

mattklein123 previously approved these changes Apr 18, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Apr 18, 2022

Add PANIC_DUE_TO_CORRUPT_ENUM, and lose the 100% coverage

94eec25

Signed-off-by: Raven Black <ravenblack@dropbox.com>

ravenblackx dismissed mattklein123’s stale review via 94eec25 April 18, 2022 18:01

repokitteh-read-only bot added the api label Apr 18, 2022

Use envoy_package not envoy_extension_package

6e6aa04

Signed-off-by: Raven Black <ravenblack@dropbox.com>

mattklein123 added the waiting label Apr 18, 2022

Back to extension_package!

fd45a56

Signed-off-by: Raven Black <ravenblack@dropbox.com>

repokitteh-read-only bot removed the waiting label Apr 19, 2022

Remove extension_package, autofix was not a good fix!

e61dd51

Signed-off-by: Raven Black <ravenblack@dropbox.com>

mattklein123 approved these changes Apr 19, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Apr 19, 2022

mattklein123 enabled auto-merge (squash) April 19, 2022 16:21

mattklein123 merged commit 18c779a into envoyproxy:main Apr 19, 2022

ravenblackx deleted the async_files branch April 19, 2022 21:05

Lynskylate mentioned this pull request May 10, 2022

Mac Compile Error: use of undeclared identifier 'o_tmpfile' #21217

Closed

ravenblackx added a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022

Add async_files library to support filesystem buffering and filesyste…

ce51113

…m caching (envoyproxy#20332) Signed-off-by: Raven Black <ravenblack@dropbox.com>


		// [#protodoc-title: AsyncFileManager configuration]

		// [#extension: envoy.common.support.async_files]

		default:
		throw EnvoyException("unrecognized AsyncFileManagerConfig::ManagerType");

Conversation

ravenblackx commented Mar 14, 2022

Uh oh!

ravenblackx commented Mar 14, 2022

Uh oh!

mattklein123 commented Mar 14, 2022

Uh oh!

ravenblackx commented Mar 15, 2022

Uh oh!

mattklein123 commented Mar 15, 2022

Uh oh!

repokitteh-read-only bot commented Mar 16, 2022

Uh oh!

ravenblackx commented Mar 16, 2022

Uh oh!

mattklein123 commented Mar 17, 2022

Uh oh!

ravenblackx commented Mar 17, 2022

Uh oh!

repokitteh-read-only bot commented Mar 17, 2022

Uh oh!

repokitteh-read-only bot commented Apr 13, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravenblackx commented Apr 14, 2022

Uh oh!

repokitteh-read-only bot commented Apr 14, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects