runtime: codifying runtime guarded features #6134
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,13 @@ class Snapshot { | |
| // configuration of "false" in runtime config. | ||
| virtual bool deprecatedFeatureEnabled(const std::string& key) const PURE; | ||
|
|
||
| // Returns true if a runtime feature is enabled. | ||
| // | ||
| // Runtime features are used to easily allow switching between old and new code paths for high | ||
| // risk changes. The intent is for the old code path to be short lived - the old code path is | ||
| // deprecated as the feature is defaulted true, and removed with the following Envoy release. | ||
| virtual bool runtimeFeatureEnabled(const std::string& key) const PURE; | ||
|
There was a problem hiding this comment. use There was a problem hiding this comment. As written I think someone could If they do the inefficient If I switch to string_view I think the map lookup will convert the string piece to the string on every single call, and there's no way to avoid it, so I think that's strictly less performant? I could update the instructions to encourage static string on the canonical non-latch path if we think that makes more sense There was a problem hiding this comment. As long as it is an There was a problem hiding this comment. TIL! Ok, got that to work for the flat hash, but unfortunately to string_view all the way up the API we need to also change Runtime::EntryMap in include/envoy/runtime/runtime.h to have a comparator. I'd prefer to land this as-is and do a separate PR converting to string piece, especially as none of the call sites will need to change. What do you think of a TODO and a follow-up? There was a problem hiding this comment. Yeah fine to leave as TODO. |
||
|
|
||
| /** | ||
| * Test if a feature is enabled using the built in random generator. This is done by generating | ||
| * a random number in the range 0-99 and seeing if this number is < the value stored in the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| #include "common/runtime/runtime_features.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Runtime { | ||
|
|
||
| // Add additional features here to enable the new code paths by default. | ||
|
There was a problem hiding this comment. Sorry to harp on this, but I think what would help me/readers here is a short description of the behavior of what an entry in runtime_features and disallowed_features do. I know this is described elsewhere, but it's hard enough to keep track of (and important enough) that IMO we should replicate a short version here? |
||
| // | ||
| // These features should not be overridden with run-time guards without a bug | ||
|
There was a problem hiding this comment. I think you are talking to end users here, right? Can you clarify slightly and say something like, "If issues are found that that require a feature to be disabled, it should be reported upstream ASAP, ..." |
||
| // being filed on github as once high risk features are true by default, the | ||
| // old code path will be removed with the next release. | ||
| constexpr const char* runtime_features[] = { | ||
| // Enabled | ||
| "envoy.reloadable_features.test_feature_true", | ||
| // Disabled | ||
| // "envoy.reloadable_features.test_feature_false", | ||
|
There was a problem hiding this comment. Do we mean for this to be commented out here? If this is a documented way of defaulting to disabled, can you add more comments? There was a problem hiding this comment. I was thinking at some point it'd be nice to have all flags documented in that file, so if folks want to preemptively flip they can do so. I'll remove it and wait until we have utils and see if it still makes sense |
||
| }; | ||
|
|
||
| // TODO(alyssawilk) handle deprecation of reloadable_features. Ideally runtime | ||
| // override of a deprecated feature will log(warn) on runtime-load if not deprecated | ||
| // and hard-fail once it has been deprecated. | ||
| constexpr const char* disallowed_features[] = { | ||
| // Acts as both a test entry for deprecated.proto and a marker for the Envoy | ||
| // deprecation scripts. | ||
| "envoy.deprecated_features.deprecated.proto:is_deprecated_fatal", | ||
| }; | ||
|
|
||
| RuntimeFeatures::RuntimeFeatures() { | ||
| for (auto& feature : disallowed_features) { | ||
| disallowed_features_.insert(feature); | ||
| } | ||
| for (auto& feature : runtime_features) { | ||
| enabled_features_.insert(feature); | ||
| } | ||
| } | ||
|
|
||
| } // namespace Runtime | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| #include "common/runtime/runtime_impl.h" | ||
|
|
||
| #include "gmock/gmock.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Runtime { | ||
|
|
||
| // In the envoy_cc_test declaration, the flag is set | ||
| // "--runtime-feature-override-for-tests=envoy.reloadable_features.test_feature_false" | ||
| TEST(RuntimeFlagOverrideTest, OverridesWork) { | ||
| EXPECT_TRUE(Runtime::runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false")); | ||
| } | ||
|
|
||
| } // namespace Runtime | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| #pragma once | ||
|
|
||
| #include "common/runtime/runtime_features.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Runtime { | ||
|
|
||
| class RuntimeFeaturesPeer { | ||
| public: | ||
| static bool addFeature(const std::string& feature) { | ||
| return const_cast<Runtime::RuntimeFeatures*>(&Runtime::RuntimeFeaturesDefaults::get()) | ||
| ->enabled_features_.insert(feature) | ||
| .second; | ||
| } | ||
| static void removeFeature(const std::string& feature) { | ||
| const_cast<Runtime::RuntimeFeatures*>(&Runtime::RuntimeFeaturesDefaults::get()) | ||
| ->enabled_features_.erase(feature); | ||
| } | ||
| }; | ||
|
|
||
| } // namespace Runtime | ||
| } // namespace Envoy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a different namespace for this type of thing?
envoy.startup_features.or similar? It's a little confusing that the docs point out that everything named like this is reloadable at runtime and then we talk about how it's not always true. :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I need to remove the buffer mentions on this one. My thought for latching is if we latched at the hcm, or at a network::connection or some such, where the lifetime were transient. As called out on the buffer PR that's truly restart-only not reloadable.
I do want to support envoy.restart_features but I also need to talk to you about how to do that. In my priors, restart flags were done on the command line (only when the binary was brought up) and it was illegal to change then when pushing config (would cause that config to get rejected). Otherwise you do a config push with a new-but-buggy restart flag, the config "passes", you hot restart, and your fleet is down. Only with Envoy and hot restarts I'm less clear on how we push config which is restart only. My default plan would be to that test-only runtime tweaking flag to be a actual production runtime tweaking flag, so folks can change restart values at start-up and we can hard-fail attempts to reload them at runtime, but I recall you saying that most people don't do flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK yeah let's chat about that offline and maybe figure out in a follow up? I think for now maybe just clarify somehow that this should only be used for truly reloadable features and if there are features that should be startup configured it should be discussed? (Up to you how to phrase)