runtime: changing runtime features to be ABSL_FLAGs based by alyssawilk · Pull Request #19880 · envoyproxy/envoy

alyssawilk · 2022-02-08T22:05:10Z

Note: this does NOT remove the runtime singleton, which will be done in a follow-up PR.

Risk Level: high
Testing: existing tests cover
Docs Changes: n/a
Release Notes:
Fixes #19847
Part of envoyproxy/envoy-mobile#2003

repokitteh-read-only · 2022-02-08T22:05:12Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #19880 was opened by alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <[email protected]>

adisuissa

Thanks for working on this.
Overall looks good.
Left a few comments. My concern is regarding some non-conventional usage of the previous Runtime Layer by extensions that I think could be broken by this.

adisuissa · 2022-02-09T19:34:22Z

source/common/runtime/runtime_features.cc

 #include "absl/strings/match.h"
+#include "absl/strings/str_replace.h"
+
+/* To add a runtime guard to Envoy, add 2 lines to this file.


Could find a way to fetch the number of abseil flags, but if there is one, I suggest to make this constraint in code.
Specifically, adding a RELEASE_ASSERT in RuntimeFeatures c'tor to validate that the number of abseil flags equals to the size of runtime_features + size of non-bool flags.

I don't think that's necessary? If you add a runtime flag and only add it to the top, then when you try to access it you'll fail the ENVOY_BUG in runtimeFeatureEnabled so all existing tests going near that code path will hard fail. Meanwhile if we add that check it'll 100% break import so I'd prefer to stick with the ENVOY_BUG :-)

adisuissa · 2022-02-09T19:45:54Z

source/common/runtime/runtime_features.cc

-  if (Runtime::LoaderSingleton::getExisting()) {
-    return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->getInteger(
-        std::string(feature), default_value);
+  if (!absl::StartsWith(feature, "envoy.")) {


Thinking out loud here. Technically, an extension can use this method, and fetch a value that is set by RTDS.
I'm pretty confident no one does it, but if they do, should we have an ENVOY_BUG or log a warning here?

good call, refactored to catch both cases and done. Alternately I could also rename to getDeprecatedInteger and break folks' compile if they use it - think that's worth doing?

source/common/runtime/runtime_features.cc

adisuissa · 2022-02-09T20:21:58Z

source/common/runtime/runtime_impl.cc

      quiche_flags_override[it.first.substr(quiche::EnvoyFeaturePrefix.length())] =
          it.second.bool_value_.value();
    }
+    if (it.second.bool_value_.has_value() && isRuntimeFeature(it.first)) {


Again for the exceptional case where someone sets a non-conventional runtime flag by RTDS, we should probably have an ENVOY_BUG here.

sorry you're saying if someone sets a boolean in their own code base envoy.reloadable_flag.not_in_runtime_features? Their code would crash on lookup any time they tried to reference the value so I'm not convinced anyone would compile that in but I can add a check regardless if you think it's worth it.

Maybe my understanding of the previous runtime and loader is incorrect.
I thought that envoy.reloadable_flag.not_in_runtime_features can be set by RTDS (or more specifically when a new snapshot is created), and it can be read by any extension, no?

Yes someone can set a random name, and it could be read from the snapshot, which is fine. It will only refresh a flag if the flag exists. I'm not sure what Alyssa's crashing comment is referring to?

I think Adi was asking if previously we supported a non-envoy-registered envoy.reloadable_feature being set via runtime config and fetched via runtimeFeatureEnabled(). It was not supported previously - you can set it in RTDS but runtimeFeatureEnabled checked that Envoy knew about it so you could only access it via explicit runtime checks. I think basically we have support parity here.

adisuissa · 2022-02-09T20:30:28Z

BTW: will it be possible to add the absl flags usage example into test/common/runtime/BUILD?

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk

FWIW //test/server/config_validation:xds_fuzz_test still fails - will debug tomorrow

alyssawilk · 2022-02-09T21:12:55Z

source/common/runtime/runtime_features.cc

 #include "absl/strings/match.h"
+#include "absl/strings/str_replace.h"
+
+/* To add a runtime guard to Envoy, add 2 lines to this file.


I don't think that's necessary? If you add a runtime flag and only add it to the top, then when you try to access it you'll fail the ENVOY_BUG in runtimeFeatureEnabled so all existing tests going near that code path will hard fail. Meanwhile if we add that check it'll 100% break import so I'd prefer to stick with the ENVOY_BUG :-)

alyssawilk · 2022-02-09T21:15:05Z

source/common/runtime/runtime_features.cc

-  if (Runtime::LoaderSingleton::getExisting()) {
-    return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->getInteger(
-        std::string(feature), default_value);
+  if (!absl::StartsWith(feature, "envoy.")) {


good call, refactored to catch both cases and done. Alternately I could also rename to getDeprecatedInteger and break folks' compile if they use it - think that's worth doing?

source/common/runtime/runtime_features.cc

alyssawilk · 2022-02-09T21:19:25Z

source/common/runtime/runtime_impl.cc

      quiche_flags_override[it.first.substr(quiche::EnvoyFeaturePrefix.length())] =
          it.second.bool_value_.value();
    }
+    if (it.second.bool_value_.has_value() && isRuntimeFeature(it.first)) {


sorry you're saying if someone sets a boolean in their own code base envoy.reloadable_flag.not_in_runtime_features? Their code would crash on lookup any time they tried to reference the value so I'm not convinced anyone would compile that in but I can add a check regardless if you think it's worth it.

alyssawilk · 2022-02-10T13:33:03Z

"BTW: will it be possible to add the absl flags usage example into test/common/runtime/BUILD?"
I don't think so. we're not parsing command line flags, just using them for storage.

Signed-off-by: Alyssa Wilk <[email protected]>

repokitteh-read-only · 2022-02-10T18:39:12Z

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 @wrowe

🐱

Caused by: #19880 was synchronize by alyssawilk.

see: more, trace.

moderation · 2022-02-10T22:51:04Z

One line patch change

/lgtm deps

Signed-off-by: Alyssa Wilk <[email protected]>

adisuissa

Thanks, LGTM modulo a nit.
/assign-from @envoyproxy/senior-maintainers

adisuissa · 2022-02-16T14:19:36Z

source/common/runtime/runtime_features.cc

+    std::string envoy_name = swapPrefix(std::string(reflection.Name()));
+    all_features_.emplace(envoy_name, feature);
+    absl::optional<bool> value = reflection.TryGet<bool>();
+    if (value.has_value() && value.value()) {


nit: consider adding:

if (!value.has_value()) { IS_ENVOY_BUG("Feature not found in ABSL flags..."); }

added an assert - everything in that struct has to be a boolean by definition so really the value check is silly.

adisuissa · 2022-02-16T14:28:26Z

test/test_listener.cc

+                   absl::StrCat("MainThreadLeak: [", test_info.test_suite_name(), ".",
+                                test_info.name(), "] test exited before main thread shut down"));
+  }
+  Runtime::RuntimeFeaturesDefaults::get().restoreDefaults();


Interesting...
My initial thoughts were that this wouldn't work, because some tests will have a Scope defined as a class member, and override a runtime value in the c'tor, but if CI is green pass I guess that's ok.

yeah all we need to do is set the flags back between unit tests

test/test_runner.cc

repokitteh-read-only · 2022-02-16T14:31:44Z

@envoyproxy/senior-maintainers assignee is @mattklein123

🐱

Caused by: a #19880 (review) was submitted by @adisuissa.

see: more, trace.

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk · 2022-02-17T13:40:48Z

CI sorted, ptal!

adisuissa

Overall LGTM, left a question about the integer defaults.

adisuissa · 2022-02-17T14:34:27Z

source/common/runtime/runtime_features.cc

+  }
+}
+
+void RuntimeFeatures::restoreDefaults() const {


All the calls to this method are done when ending tests/scope, however I don't see where the default values for the integer-based flags are set.
What am I missing?

I'm just using the same values from where they're defined, which I snagged from the defaults to the getInteger code in call sites.
It's really inelegant but per the TODO I think I can get rid of most if not all of these so hopefully it won't be inelegant long.

Nvm, my bad... I missed lines 66-72 in this file where these are initialized.
Thanks!

adisuissa

LGTM

mattklein123

At a high level LGTM! One question about upstreaming the absl path.

mattklein123 · 2022-02-17T17:36:23Z

bazel/abseil.patch



 // ABSL_OPTION_USE_INLINE_NAMESPACE
+diff --git a/absl/flags/commandlineflag.h b/absl/flags/commandlineflag.h


Is this being upstreamed?

I'm negotiating with the absl team about that or pragma changes yep

mattklein123 · 2022-02-17T17:48:52Z

source/common/runtime/runtime_impl.cc

-// TODO(12923): Document the Quiche reloadable flag setup.
-#ifdef ENVOY_ENABLE_QUIC
-void refreshQuicheReloadableFlags(const Snapshot::EntryMap& flag_map) {
+void refreshReloadableFlags(const Snapshot::EntryMap& flag_map) {


Just so I understand this is now being used for all refresh, right? So it's no longer H3 specific?

correct, that's why I took quiche out of the name :-)

mattklein123 · 2022-02-17T17:50:32Z

source/common/runtime/runtime_impl.cc

      quiche_flags_override[it.first.substr(quiche::EnvoyFeaturePrefix.length())] =
          it.second.bool_value_.value();
    }
+    if (it.second.bool_value_.has_value() && isRuntimeFeature(it.first)) {


Yes someone can set a random name, and it could be read from the snapshot, which is fine. It will only refresh a flag if the flag exists. I'm not sure what Alyssa's crashing comment is referring to?

alyssawilk mentioned this pull request Feb 8, 2022

remove global runtime: rebase runtime features on ABSL_flags #19847

Closed

alyssawilk added 3 commits February 9, 2022 09:23

runtime: switching runtime flags to be based off ABSL flags

7098f07

Signed-off-by: Alyssa Wilk <[email protected]>

Merge branch 'main' into runtime

df5a986

Signed-off-by: Alyssa Wilk <[email protected]>

cleanup

a844e4a

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk force-pushed the runtime branch from 16a707e to a844e4a Compare February 9, 2022 14:47

alyssawilk changed the title ~~Runtime~~ runtime: changing runtime features to be ABSL_FLAGs based Feb 9, 2022

alyssawilk marked this pull request as ready for review February 9, 2022 14:48

alyssawilk assigned adisuissa Feb 9, 2022

alyssawilk added 2 commits February 9, 2022 11:39

check_pre

2e1c8a1

Signed-off-by: Alyssa Wilk <[email protected]>

fix

7460daf

Signed-off-by: Alyssa Wilk <[email protected]>

adisuissa reviewed Feb 9, 2022

View reviewed changes

comments

cefd040

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk commented Feb 9, 2022

View reviewed changes

alyssawilk added 2 commits February 10, 2022 09:07

fix fuzz tests by restoring runtime state

1271875

Signed-off-by: Alyssa Wilk <[email protected]>

fixes

dce2cc4

Signed-off-by: Alyssa Wilk <[email protected]>

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

repokitteh-read-only bot assigned wrowe Feb 10, 2022

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Feb 10, 2022

alyssawilk added 4 commits February 15, 2022 10:48

Merge branch 'main' into runtime

a9a0cf0

Signed-off-by: Alyssa Wilk <[email protected]>

FORMAT

14b74b0

Signed-off-by: Alyssa Wilk <[email protected]>

alpha

c24b2a6

Signed-off-by: Alyssa Wilk <[email protected]>

compile time build

71bfa16

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk assigned mattklein123 Feb 15, 2022

adisuissa previously approved these changes Feb 16, 2022

View reviewed changes

alyssawilk added 3 commits February 16, 2022 09:35

Merge branch 'main' into runtime

c72e652

Signed-off-by: Alyssa Wilk <[email protected]>

comment

db4ada6

Signed-off-by: Alyssa Wilk <[email protected]>

merge fixups and comment

f9b93ad

Signed-off-by: Alyssa Wilk <[email protected]>

alyssawilk dismissed adisuissa’s stale review via f9b93ad February 16, 2022 14:57

alyssawilk added 2 commits February 16, 2022 13:19

Merge branch 'main' into runtime

3761e7e

Signed-off-by: Alyssa Wilk <[email protected]>

tidy

0741855

Signed-off-by: Alyssa Wilk <[email protected]>

adisuissa reviewed Feb 17, 2022

View reviewed changes

adisuissa approved these changes Feb 17, 2022

View reviewed changes

mattklein123 approved these changes Feb 17, 2022

View reviewed changes

alyssawilk merged commit 93cd7c7 into envoyproxy:main Feb 17, 2022

junr03 mentioned this pull request Mar 2, 2022

envoy: 3267361cedf936 envoyproxy/envoy-mobile#2076

Merged

alyssawilk mentioned this pull request Mar 3, 2022

runtime: reject runtime variables with the runtime guard naming if they're not known flags #20212

Open



		// ABSL_OPTION_USE_INLINE_NAMESPACE
		diff --git a/absl/flags/commandlineflag.h b/absl/flags/commandlineflag.h

Conversation

alyssawilk commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 8, 2022

Uh oh!

adisuissa 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!

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!

adisuissa commented Feb 9, 2022

Uh oh!

alyssawilk 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Feb 10, 2022

Uh oh!

repokitteh-read-only bot commented Feb 10, 2022

Uh oh!

moderation commented Feb 10, 2022

Uh oh!

adisuissa 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!

Uh oh!

repokitteh-read-only bot commented Feb 16, 2022

Uh oh!

alyssawilk commented Feb 17, 2022

Uh oh!

adisuissa 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!

adisuissa left a comment

Choose a reason for hiding this comment

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

alyssawilk commented Feb 8, 2022 •

edited

Loading